Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Uniformize spam-checker API, part 3: Expand check_event_for_spam with the ability to return additional fields #12846

Merged
merged 4 commits into from
May 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/12808.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Update to `check_event_for_spam`. Deprecate the current callback signature, replace it with a new signature that is both less ambiguous (replacing booleans with explicit allow/block) and more powerful (ability to return explicit error codes).
1 change: 1 addition & 0 deletions changelog.d/12846.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Experimental: expand `check_event_for_spam` with ability to return additional fields. This enables spam-checker implementations to experiment with mechanisms to give users more information about why they are blocked and whether any action is needed from them to be unblocked.
23 changes: 13 additions & 10 deletions synapse/api/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,13 @@ class SynapseError(CodeMessageException):
errcode: Matrix error code e.g 'M_FORBIDDEN'
"""

def __init__(self, code: int, msg: str, errcode: str = Codes.UNKNOWN):
def __init__(
self,
code: int,
msg: str,
errcode: str = Codes.UNKNOWN,
additional_fields: Optional[Dict] = None,
):
"""Constructs a synapse error.

Args:
Expand All @@ -156,9 +162,13 @@ def __init__(self, code: int, msg: str, errcode: str = Codes.UNKNOWN):
"""
super().__init__(code, msg)
self.errcode = errcode
if additional_fields is None:
self._additional_fields: Dict = {}
else:
self._additional_fields = dict(additional_fields)

def error_dict(self) -> "JsonDict":
return cs_error(self.msg, self.errcode)
return cs_error(self.msg, self.errcode, **self._additional_fields)


class InvalidAPICallError(SynapseError):
Expand All @@ -183,14 +193,7 @@ def __init__(
errcode: str = Codes.UNKNOWN,
additional_fields: Optional[Dict] = None,
):
super().__init__(code, msg, errcode)
if additional_fields is None:
self._additional_fields: Dict = {}
else:
self._additional_fields = dict(additional_fields)

def error_dict(self) -> "JsonDict":
return cs_error(self.msg, self.errcode, **self._additional_fields)
super().__init__(code, msg, errcode, additional_fields)


class ConsentNotGivenError(SynapseError):
Expand Down
20 changes: 13 additions & 7 deletions synapse/events/spamcheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
Awaitable,
Callable,
Collection,
Dict,
List,
Optional,
Tuple,
Expand All @@ -41,13 +42,17 @@

logger = logging.getLogger(__name__)


CHECK_EVENT_FOR_SPAM_CALLBACK = Callable[
["synapse.events.EventBase"],
Awaitable[
Union[
Allow,
Codes,
# Highly experimental, not officially part of the spamchecker API, may
# disappear without warning depending on the results of ongoing
# experiments.
# Use this to return additional information as part of an error.
Tuple[Codes, Dict],
# Deprecated
bool,
# Deprecated
Expand Down Expand Up @@ -270,7 +275,7 @@ def register_callbacks(

async def check_event_for_spam(
self, event: "synapse.events.EventBase"
) -> Union[Decision, str]:
) -> Union[Decision, Tuple[Codes, Dict], str]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a super confusing api. why are there four different potential return types? (Decision is itself a Union of two types)

It also doesn't seem to correspond to the docstring.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(it's this function's job to flatten the myriad different potential return types from the module callback into a sane api for the rest of synapse. It seems to be spectacularly failing at that job currently)

"""Checks if a given event is considered "spammy" by this server.

If the server considers an event spammy, then it will be rejected if
Expand All @@ -293,9 +298,9 @@ async def check_event_for_spam(
with Measure(
self.clock, "{}.{}".format(callback.__module__, callback.__qualname__)
):
res: Union[Decision, str, bool] = await delay_cancellation(
callback(event)
)
res: Union[
Decision, Tuple[Codes, Dict], str, bool
] = await delay_cancellation(callback(event))
if res is False or res is Allow.ALLOW:
# This spam-checker accepts the event.
# Other spam-checkers may reject it, though.
Expand All @@ -305,8 +310,9 @@ async def check_event_for_spam(
# return value `True`
return Codes.FORBIDDEN
else:
# This spam-checker rejects the event either with a `str`
# or with a `Codes`. In either case, we stop here.
# This spam-checker rejects the event either with a `str`,
# with a `Codes` or with a `Tuple[Codes, Dict]`. In either
# case, we stop here.
return res

# No spam-checker has rejected the event, let it pass.
Expand Down
15 changes: 15 additions & 0 deletions synapse/handlers/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -895,6 +895,21 @@ async def create_and_send_nonmember_event(

spam_check = await self.spam_checker.check_event_for_spam(event)
if spam_check is not synapse.spam_checker_api.Allow.ALLOW:
if isinstance(spam_check, tuple):
try:
[code, dict] = spam_check
raise SynapseError(
403,
"This message had been rejected as probable spam",
code,
dict,
)
except ValueError:
logger.error(
"Spam-check module returned invalid error value. Expecting [code, dict], got %s",
spam_check,
)
spam_check = Codes.FORBIDDEN
raise SynapseError(
403, "This message had been rejected as probable spam", spam_check
)
Expand Down