From 19d83e9ae190229582b3b666543bab99fa0db00b Mon Sep 17 00:00:00 2001 From: David Teller Date: Fri, 20 May 2022 08:49:40 +0200 Subject: [PATCH 1/9] Introduce an enum `Code` to replace the namespace class `Codes`. This is a first step towards a refactoring of the Spam-checker API towards more uniform and more powerful API/type signatures. --- changelog.d/12703.misc | 2 +- synapse/api/errors.py | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/changelog.d/12703.misc b/changelog.d/12703.misc index 9aaa1bbaa3d0..a4ca6e265b85 100644 --- a/changelog.d/12703.misc +++ b/changelog.d/12703.misc @@ -1 +1 @@ -Convert namespace class `Codes` into a string enum. \ No newline at end of file +Convert namespace class `Codes` into a string enum. diff --git a/synapse/api/errors.py b/synapse/api/errors.py index 9614be6b4e46..6650e826d5af 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -270,9 +270,7 @@ class UnrecognizedRequestError(SynapseError): """An error indicating we don't understand the request you're trying to make""" def __init__( - self, - msg: str = "Unrecognized request", - errcode: str = Codes.UNRECOGNIZED, + self, msg: str = "Unrecognized request", errcode: str = Codes.UNRECOGNIZED ): super().__init__(400, msg, errcode) From 468dd05687df568daaf69de22bf108fc929eb34b Mon Sep 17 00:00:00 2001 From: David Teller Date: Fri, 20 May 2022 09:04:35 +0200 Subject: [PATCH 2/9] Uniformize spam-checker API, part 2: check_event_for_spam Signed-off-by: David Teller --- changelog.d/12808.feature | 1 + docs/modules/spam_checker_callbacks.md | 27 +++++++++------ synapse/events/spamcheck.py | 46 ++++++++++++++++++++------ synapse/federation/federation_base.py | 5 +-- synapse/handlers/message.py | 11 +++--- synapse/spam_checker_api/__init__.py | 31 ++++++++++++++++- 6 files changed, 93 insertions(+), 28 deletions(-) create mode 100644 changelog.d/12808.feature diff --git a/changelog.d/12808.feature b/changelog.d/12808.feature new file mode 100644 index 000000000000..561c8b9d34a4 --- /dev/null +++ b/changelog.d/12808.feature @@ -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). \ No newline at end of file diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 472d95718087..e6ec07e17412 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -11,22 +11,29 @@ The available spam checker callbacks are: ### `check_event_for_spam` _First introduced in Synapse v1.37.0_ +_Signature extended to support Allow and Code in Synapse v1.60.0_ +_Boolean return value deprecated in Synapse v1.60.0_ ```python -async def check_event_for_spam(event: "synapse.events.EventBase") -> Union[bool, str] +async def check_event_for_spam(event: "synapse.events.EventBase") -> Union[Allow, Code, DEPRECATED_STR, DEPRECATED_BOOL] ``` -Called when receiving an event from a client or via federation. The callback must return -either: -- an error message string, to indicate the event must be rejected because of spam and - give a rejection reason to forward to clients; -- the boolean `True`, to indicate that the event is spammy, but not provide further details; or -- the booelan `False`, to indicate that the event is not considered spammy. +Called when receiving an event from a client or via federation. The callback must return either: + - `synapse.spam_checker_api.ALLOW`, to allow the operation. Other callbacks + may still decide to reject it. + - `synapse.api.errors.Code` to reject the operation with an error code. In case + of doubt, `Code.FORBIDDEN` is a good error code. + - (deprecated) a `str` to reject the operation and specify an error message. Note that clients + typically will not localize the error message to the user's preferred locale. + - (deprecated) on `False`, behave as `ALLOW`. Deprecated as confusing, as some + callbacks in expect `True` to allow and others `True` to reject. + - (deprecated) on `True`, behave as `Code.FORBIDDEN`. Deprecated as confusing, as + some callbacks in expect `True` to allow and others `True` to reject. If multiple modules implement this callback, they will be considered in order. If a -callback returns `False`, Synapse falls through to the next one. The value of the first -callback that does not return `False` will be used. If this happens, Synapse will not call -any of the subsequent implementations of this callback. +callback returns `ALLOW`, Synapse falls through to the next one. The value of the +first callback that does not return `ALLOW` will be used. If this happens, Synapse +will not call any of the subsequent implementations of this callback. ### `user_may_join_room` diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index f30207376ae2..b3f451c6150c 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -27,9 +27,10 @@ Union, ) +from synapse.api.errors import Codes from synapse.rest.media.v1._base import FileInfo from synapse.rest.media.v1.media_storage import ReadableFileWrapper -from synapse.spam_checker_api import RegistrationBehaviour +from synapse.spam_checker_api import ALLOW, Allow, Decision, RegistrationBehaviour from synapse.types import RoomAlias, UserProfile from synapse.util.async_helpers import delay_cancellation, maybe_awaitable from synapse.util.metrics import Measure @@ -40,9 +41,16 @@ logger = logging.getLogger(__name__) + +# A boolean returned value, kept for backwards compatibility but deprecated. +DEPRECATED_BOOL = bool + +# A string returned value, kept for backwards compatibility but deprecated. +DEPRECATED_STR = str + CHECK_EVENT_FOR_SPAM_CALLBACK = Callable[ ["synapse.events.EventBase"], - Awaitable[Union[bool, str]], + Awaitable[Union[Allow, Codes, DEPRECATED_BOOL, DEPRECATED_STR]], ] USER_MAY_JOIN_ROOM_CALLBACK = Callable[[str, str, bool], Awaitable[bool]] USER_MAY_INVITE_CALLBACK = Callable[[str, str, str], Awaitable[bool]] @@ -244,7 +252,7 @@ def register_callbacks( async def check_event_for_spam( self, event: "synapse.events.EventBase" - ) -> Union[bool, str]: + ) -> Union[Decision, str]: """Checks if a given event is considered "spammy" by this server. If the server considers an event spammy, then it will be rejected if @@ -255,18 +263,36 @@ async def check_event_for_spam( event: the event to be checked Returns: - True or a string if the event is spammy. If a string is returned it - will be used as the error message returned to the user. + - on `ALLOW`, the event is considered good (non-spammy) and should + be let through. Other spamcheck filters may still reject it. + - on `Code`, the event is considered spammy and is rejected with a specific + error message/code. + - on `str`, the event is considered spammy and the string is used as error + message. This usage is generally discouraged as it doesn't support + internationalization. """ for callback in self._check_event_for_spam_callbacks: with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - res: Union[bool, str] = await delay_cancellation(callback(event)) - if res: - return res - - return False + res: Union[ + Decision, DEPRECATED_STR, DEPRECATED_BOOL + ] = await delay_cancellation(callback(event)) + if res is False or res is ALLOW: + # This spam-checker accepts the event. + # Other spam-checkers may reject it, though. + continue + elif res is True: + # This spam-checker rejects the event with deprecated + # 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. + return res + + # No spam-checker has rejected the event, let it pass. + return ALLOW async def user_may_join_room( self, user_id: str, room_id: str, is_invited: bool diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 41ac49fdc8bf..45c277711787 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -15,6 +15,7 @@ import logging from typing import TYPE_CHECKING +import synapse from synapse.api.constants import MAX_DEPTH, EventContentFields, EventTypes, Membership from synapse.api.errors import Codes, SynapseError from synapse.api.room_versions import EventFormatVersions, RoomVersion @@ -98,9 +99,9 @@ async def _check_sigs_and_hash( ) return redacted_event - result = await self.spam_checker.check_event_for_spam(pdu) + spam_check = await self.spam_checker.check_event_for_spam(pdu) - if result: + if spam_check is not synapse.spam_checker_api.ALLOW: logger.warning("Event contains spam, soft-failing %s", pdu.event_id) # we redact (to save disk space) as well as soft-failing (to stop # using the event in prev_events). diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index e566ff1f8ed8..f4d8def966f6 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -23,6 +23,7 @@ from twisted.internet.interfaces import IDelayedCall +import synapse from synapse import event_auth from synapse.api.constants import ( EventContentFields, @@ -885,11 +886,11 @@ async def create_and_send_nonmember_event( event.sender, ) - spam_error = await self.spam_checker.check_event_for_spam(event) - if spam_error: - if not isinstance(spam_error, str): - spam_error = "Spam is not permitted here" - raise SynapseError(403, spam_error, Codes.FORBIDDEN) + spam_check = await self.spam_checker.check_event_for_spam(event) + if spam_check is not synapse.spam_checker_api.ALLOW: + raise SynapseError( + 403, "This message had been rejected as probable spam", spam_check + ) ev = await self.handle_new_client_event( requester=requester, diff --git a/synapse/spam_checker_api/__init__.py b/synapse/spam_checker_api/__init__.py index 73018f2d002e..d0e0bd8362cb 100644 --- a/synapse/spam_checker_api/__init__.py +++ b/synapse/spam_checker_api/__init__.py @@ -12,13 +12,42 @@ # See the License for the specific language governing permissions and # limitations under the License. from enum import Enum +from typing import NewType, Union + +from synapse.api.errors import Codes class RegistrationBehaviour(Enum): """ - Enum to define whether a registration request should allowed, denied, or shadow-banned. + Enum to define whether a registration request should be allowed, denied, or shadow-banned. """ ALLOW = "allow" SHADOW_BAN = "shadow_ban" DENY = "deny" + + +# Define a strongly-typed singleton value `ALLOW`. + +# Private NewType, to make sure that nobody outside this module +# defines an instance of `Allow`. +_Allow = NewType("_Allow", str) + +# Public NewType, to let the rest of the code mention type `Allow`. +Allow = NewType("Allow", _Allow) + +ALLOW = Allow(_Allow("Allow")) +""" +Return this constant to allow a message to pass. + +This is the ONLY legal value of type `Allow`. +""" + +Decision = Union[Allow, Codes] +""" +Union to define whether a request should be allowed or rejected. + +To accept a request, return `ALLOW`. + +To reject a request without any specific information, use `Codes.FORBIDDEN`. +""" From e56d5d07bbaa498a0a9b5be7e681f2898b5dc0af Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 23 May 2022 10:38:14 +0200 Subject: [PATCH 3/9] WIP: Applied feedback --- changelog.d/12703.misc | 2 +- docs/modules/spam_checker_callbacks.md | 16 ++++++++-------- synapse/module_api/__init__.py | 5 +++++ synapse/spam_checker_api/__init__.py | 22 ++++++---------------- 4 files changed, 20 insertions(+), 25 deletions(-) diff --git a/changelog.d/12703.misc b/changelog.d/12703.misc index a4ca6e265b85..9aaa1bbaa3d0 100644 --- a/changelog.d/12703.misc +++ b/changelog.d/12703.misc @@ -1 +1 @@ -Convert namespace class `Codes` into a string enum. +Convert namespace class `Codes` into a string enum. \ No newline at end of file diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index e6ec07e17412..fa2534e4bcbb 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -12,27 +12,27 @@ The available spam checker callbacks are: _First introduced in Synapse v1.37.0_ _Signature extended to support Allow and Code in Synapse v1.60.0_ -_Boolean return value deprecated in Synapse v1.60.0_ +_Boolean and string return value types deprecated in Synapse v1.60.0_ ```python -async def check_event_for_spam(event: "synapse.events.EventBase") -> Union[Allow, Code, DEPRECATED_STR, DEPRECATED_BOOL] +async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.Allow", "synapse.module_api.errors.Codes", str, bool] ``` Called when receiving an event from a client or via federation. The callback must return either: - - `synapse.spam_checker_api.ALLOW`, to allow the operation. Other callbacks + - `synapse.module_api.Allow.ALLOW`, to allow the operation. Other callbacks may still decide to reject it. - - `synapse.api.errors.Code` to reject the operation with an error code. In case - of doubt, `Code.FORBIDDEN` is a good error code. + - `synapse.api.errors.Codes` to reject the operation with an error code. In case + of doubt, `synapse.api.errors.Code.FORBIDDEN` is a good error code. - (deprecated) a `str` to reject the operation and specify an error message. Note that clients typically will not localize the error message to the user's preferred locale. - (deprecated) on `False`, behave as `ALLOW`. Deprecated as confusing, as some callbacks in expect `True` to allow and others `True` to reject. - - (deprecated) on `True`, behave as `Code.FORBIDDEN`. Deprecated as confusing, as + - (deprecated) on `True`, behave as `synapse.api.errors.Code.FORBIDDEN`. Deprecated as confusing, as some callbacks in expect `True` to allow and others `True` to reject. If multiple modules implement this callback, they will be considered in order. If a -callback returns `ALLOW`, Synapse falls through to the next one. The value of the -first callback that does not return `ALLOW` will be used. If this happens, Synapse +callback returns `synapse.module_api.Allow.ALLOW`, Synapse falls through to the next one. The value of the +first callback that does not return `synapse.module_api.Allow.ALLOW` will be used. If this happens, Synapse will not call any of the subsequent implementations of this callback. ### `user_may_join_room` diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 73f92d2df8d6..067febd671f4 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -35,6 +35,7 @@ from twisted.internet import defer from twisted.web.resource import Resource +from synapse import spam_checker_api from synapse.api.errors import SynapseError from synapse.events import EventBase from synapse.events.presence_router import ( @@ -139,6 +140,9 @@ PRESENCE_ALL_USERS = PresenceRouter.ALL_USERS +Allow = spam_checker_api.Allow +# Singleton value used to mark a message as permitted. + __all__ = [ "errors", "make_deferred_yieldable", @@ -146,6 +150,7 @@ "respond_with_html", "run_in_background", "cached", + "Allow", "UserID", "DatabasePool", "LoggingTransaction", diff --git a/synapse/spam_checker_api/__init__.py b/synapse/spam_checker_api/__init__.py index d0e0bd8362cb..b33d0cc2ef6e 100644 --- a/synapse/spam_checker_api/__init__.py +++ b/synapse/spam_checker_api/__init__.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. from enum import Enum -from typing import NewType, Union +from typing import Union from synapse.api.errors import Codes @@ -26,22 +26,12 @@ class RegistrationBehaviour(Enum): SHADOW_BAN = "shadow_ban" DENY = "deny" +class Allow(Enum): + """ + Singleton to allow events to pass through in SpamChecker APIs. + """ + ALLOW = "allow" -# Define a strongly-typed singleton value `ALLOW`. - -# Private NewType, to make sure that nobody outside this module -# defines an instance of `Allow`. -_Allow = NewType("_Allow", str) - -# Public NewType, to let the rest of the code mention type `Allow`. -Allow = NewType("Allow", _Allow) - -ALLOW = Allow(_Allow("Allow")) -""" -Return this constant to allow a message to pass. - -This is the ONLY legal value of type `Allow`. -""" Decision = Union[Allow, Codes] """ From 6dca5a6a9cb5524982817d5287a1869b2e7005de Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 23 May 2022 10:47:18 +0200 Subject: [PATCH 4/9] WIP: Linter --- synapse/events/spamcheck.py | 6 +++--- synapse/federation/federation_base.py | 2 +- synapse/handlers/message.py | 2 +- synapse/spam_checker_api/__init__.py | 2 ++ 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index b3f451c6150c..acf85278b061 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -30,7 +30,7 @@ from synapse.api.errors import Codes from synapse.rest.media.v1._base import FileInfo from synapse.rest.media.v1.media_storage import ReadableFileWrapper -from synapse.spam_checker_api import ALLOW, Allow, Decision, RegistrationBehaviour +from synapse.spam_checker_api import Allow, Decision, RegistrationBehaviour from synapse.types import RoomAlias, UserProfile from synapse.util.async_helpers import delay_cancellation, maybe_awaitable from synapse.util.metrics import Measure @@ -278,7 +278,7 @@ async def check_event_for_spam( res: Union[ Decision, DEPRECATED_STR, DEPRECATED_BOOL ] = await delay_cancellation(callback(event)) - if res is False or res is ALLOW: + if res is False or res is Allow.ALLOW: # This spam-checker accepts the event. # Other spam-checkers may reject it, though. continue @@ -292,7 +292,7 @@ async def check_event_for_spam( return res # No spam-checker has rejected the event, let it pass. - return ALLOW + return Allow.ALLOW async def user_may_join_room( self, user_id: str, room_id: str, is_invited: bool diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 45c277711787..1e866b19d87b 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -101,7 +101,7 @@ async def _check_sigs_and_hash( spam_check = await self.spam_checker.check_event_for_spam(pdu) - if spam_check is not synapse.spam_checker_api.ALLOW: + if spam_check is not synapse.spam_checker_api.Allow.ALLOW: logger.warning("Event contains spam, soft-failing %s", pdu.event_id) # we redact (to save disk space) as well as soft-failing (to stop # using the event in prev_events). diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index f4d8def966f6..cb1bc4c06f1c 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -887,7 +887,7 @@ 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: + if spam_check is not synapse.spam_checker_api.Allow.ALLOW: raise SynapseError( 403, "This message had been rejected as probable spam", spam_check ) diff --git a/synapse/spam_checker_api/__init__.py b/synapse/spam_checker_api/__init__.py index b33d0cc2ef6e..15ed00f1c469 100644 --- a/synapse/spam_checker_api/__init__.py +++ b/synapse/spam_checker_api/__init__.py @@ -26,10 +26,12 @@ class RegistrationBehaviour(Enum): SHADOW_BAN = "shadow_ban" DENY = "deny" + class Allow(Enum): """ Singleton to allow events to pass through in SpamChecker APIs. """ + ALLOW = "allow" From 88717707eb8768bdb4450aa81b207cf0bed339f4 Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 23 May 2022 11:28:16 +0200 Subject: [PATCH 5/9] WIP: Upgrade notes --- docs/upgrade.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/docs/upgrade.md b/docs/upgrade.md index 92ca31b2f8de..a155da1dee0d 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -177,7 +177,35 @@ has queries that can be used to check a database for this problem in advance. +## SpamChecker API's `check_event_for_spam` has a new signature. +The previous signature has been deprecated. + +Whereas `check_event_for_spam` callbacks used to return `Union[str, bool]`, they should now return `Union["synapse.module_api.Allow", "synapse.module_api.errors.Codes"]`. + +This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. + +If you previously had + +```python +async def check_event_for_spam(event): + if ...: + # Event is spam + return True + # Event is not spam + return False +``` + +you should now rather write + +```python +async def check_event_for_spam(event): + if ...: + # Event is spam, mark it as forbidden (or some more precise error code). + return synapse.module_api.errors.Codes.FORBIDDEN + # Event is not spam, mark it as `ALLOW`. + return synapse.module_api.Allow.ALLOW +``` # Upgrading to v1.59.0 From 485278e31b0efc8fb14ee9809ffc5eb25a332b2f Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 23 May 2022 13:04:13 +0200 Subject: [PATCH 6/9] WIP: Applied feedback --- docs/modules/spam_checker_callbacks.md | 6 +++--- docs/upgrade.md | 2 +- synapse/module_api/__init__.py | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index fa2534e4bcbb..61b12e4a8b87 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -19,7 +19,7 @@ async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union[" ``` Called when receiving an event from a client or via federation. The callback must return either: - - `synapse.module_api.Allow.ALLOW`, to allow the operation. Other callbacks + - `synapse.module_api.ALLOW`, to allow the operation. Other callbacks may still decide to reject it. - `synapse.api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.api.errors.Code.FORBIDDEN` is a good error code. @@ -31,8 +31,8 @@ Called when receiving an event from a client or via federation. The callback mus some callbacks in expect `True` to allow and others `True` to reject. If multiple modules implement this callback, they will be considered in order. If a -callback returns `synapse.module_api.Allow.ALLOW`, Synapse falls through to the next one. The value of the -first callback that does not return `synapse.module_api.Allow.ALLOW` will be used. If this happens, Synapse +callback returns `synapse.module_api.ALLOW`, Synapse falls through to the next one. The value of the +first callback that does not return `synapse.module_api.ALLOW` will be used. If this happens, Synapse will not call any of the subsequent implementations of this callback. ### `user_may_join_room` diff --git a/docs/upgrade.md b/docs/upgrade.md index a155da1dee0d..ac09fcc5667c 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -204,7 +204,7 @@ async def check_event_for_spam(event): # Event is spam, mark it as forbidden (or some more precise error code). return synapse.module_api.errors.Codes.FORBIDDEN # Event is not spam, mark it as `ALLOW`. - return synapse.module_api.Allow.ALLOW + return synapse.module_api.ALLOW ``` # Upgrading to v1.59.0 diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 067febd671f4..92b100c0d368 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -140,7 +140,7 @@ PRESENCE_ALL_USERS = PresenceRouter.ALL_USERS -Allow = spam_checker_api.Allow +ALLOW = spam_checker_api.Allow.ALLOW # Singleton value used to mark a message as permitted. __all__ = [ From f39d5681267cced167145b8b8a750d10c4f0f5b1 Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 23 May 2022 13:52:04 +0200 Subject: [PATCH 7/9] WIP: Applied feedback --- docs/modules/spam_checker_callbacks.md | 8 ++++---- docs/upgrade.md | 7 ++++--- synapse/module_api/__init__.py | 3 ++- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 61b12e4a8b87..afd9b1c82c1e 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -15,19 +15,19 @@ _Signature extended to support Allow and Code in Synapse v1.60.0_ _Boolean and string return value types deprecated in Synapse v1.60.0_ ```python -async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.Allow", "synapse.module_api.errors.Codes", str, bool] +async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.Allow", "synapse.module_api.Codes", str, bool] ``` Called when receiving an event from a client or via federation. The callback must return either: - `synapse.module_api.ALLOW`, to allow the operation. Other callbacks may still decide to reject it. - - `synapse.api.errors.Codes` to reject the operation with an error code. In case - of doubt, `synapse.api.errors.Code.FORBIDDEN` is a good error code. + - `synapse.api.Codes` to reject the operation with an error code. In case + of doubt, `synapse.api.Codes.FORBIDDEN` is a good error code. - (deprecated) a `str` to reject the operation and specify an error message. Note that clients typically will not localize the error message to the user's preferred locale. - (deprecated) on `False`, behave as `ALLOW`. Deprecated as confusing, as some callbacks in expect `True` to allow and others `True` to reject. - - (deprecated) on `True`, behave as `synapse.api.errors.Code.FORBIDDEN`. Deprecated as confusing, as + - (deprecated) on `True`, behave as `synapse.api.Codes.FORBIDDEN`. Deprecated as confusing, as some callbacks in expect `True` to allow and others `True` to reject. If multiple modules implement this callback, they will be considered in order. If a diff --git a/docs/upgrade.md b/docs/upgrade.md index ac09fcc5667c..e7eadadb64bf 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -185,7 +185,7 @@ Whereas `check_event_for_spam` callbacks used to return `Union[str, bool]`, they This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. -If you previously had +If your module implements `check_event_for_spam` as follows: ```python async def check_event_for_spam(event): @@ -196,12 +196,13 @@ async def check_event_for_spam(event): return False ``` -you should now rather write +you should rewrite it as follows: ```python async def check_event_for_spam(event): if ...: - # Event is spam, mark it as forbidden (or some more precise error code). + # Event is spam, mark it as forbidden (you may use some more precise error + # code if it is useful). return synapse.module_api.errors.Codes.FORBIDDEN # Event is not spam, mark it as `ALLOW`. return synapse.module_api.ALLOW diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 92b100c0d368..f82d7ec27561 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -36,7 +36,7 @@ from twisted.web.resource import Resource from synapse import spam_checker_api -from synapse.api.errors import SynapseError +from synapse.api.errors import Codes, SynapseError from synapse.events import EventBase from synapse.events.presence_router import ( GET_INTERESTED_USERS_CALLBACK, @@ -151,6 +151,7 @@ "run_in_background", "cached", "Allow", + "Codes", "UserID", "DatabasePool", "LoggingTransaction", From 92b87d5026eb7171853381d3c52b849c1a59b05c Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 23 May 2022 17:53:56 +0200 Subject: [PATCH 8/9] WIP: Applied feedback --- docs/modules/spam_checker_callbacks.md | 6 +++--- synapse/events/spamcheck.py | 6 +++--- synapse/module_api/__init__.py | 3 +-- synapse/module_api/errors.py | 2 ++ synapse/spam_checker_api/__init__.py | 4 ++++ 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index afd9b1c82c1e..87a17fdd6cd9 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -15,19 +15,19 @@ _Signature extended to support Allow and Code in Synapse v1.60.0_ _Boolean and string return value types deprecated in Synapse v1.60.0_ ```python -async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.Allow", "synapse.module_api.Codes", str, bool] +async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", str, bool] ``` Called when receiving an event from a client or via federation. The callback must return either: - `synapse.module_api.ALLOW`, to allow the operation. Other callbacks may still decide to reject it. - `synapse.api.Codes` to reject the operation with an error code. In case - of doubt, `synapse.api.Codes.FORBIDDEN` is a good error code. + of doubt, `synapse.api.error.Codes.FORBIDDEN` is a good error code. - (deprecated) a `str` to reject the operation and specify an error message. Note that clients typically will not localize the error message to the user's preferred locale. - (deprecated) on `False`, behave as `ALLOW`. Deprecated as confusing, as some callbacks in expect `True` to allow and others `True` to reject. - - (deprecated) on `True`, behave as `synapse.api.Codes.FORBIDDEN`. Deprecated as confusing, as + - (deprecated) on `True`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as some callbacks in expect `True` to allow and others `True` to reject. If multiple modules implement this callback, they will be considered in order. If a diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index acf85278b061..aa1447b70efc 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -275,9 +275,9 @@ async def check_event_for_spam( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - res: Union[ - Decision, DEPRECATED_STR, DEPRECATED_BOOL - ] = await delay_cancellation(callback(event)) + res: Union[Decision, 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. diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index f82d7ec27561..92b100c0d368 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -36,7 +36,7 @@ from twisted.web.resource import Resource from synapse import spam_checker_api -from synapse.api.errors import Codes, SynapseError +from synapse.api.errors import SynapseError from synapse.events import EventBase from synapse.events.presence_router import ( GET_INTERESTED_USERS_CALLBACK, @@ -151,7 +151,6 @@ "run_in_background", "cached", "Allow", - "Codes", "UserID", "DatabasePool", "LoggingTransaction", diff --git a/synapse/module_api/errors.py b/synapse/module_api/errors.py index e58e0e60feab..bedd045d6fe1 100644 --- a/synapse/module_api/errors.py +++ b/synapse/module_api/errors.py @@ -15,6 +15,7 @@ """Exception types which are exposed as part of the stable module API""" from synapse.api.errors import ( + Codes, InvalidClientCredentialsError, RedirectException, SynapseError, @@ -24,6 +25,7 @@ from synapse.storage.push_rule import RuleNotFoundException __all__ = [ + "Codes", "InvalidClientCredentialsError", "RedirectException", "SynapseError", diff --git a/synapse/spam_checker_api/__init__.py b/synapse/spam_checker_api/__init__.py index 15ed00f1c469..95132c80b70e 100644 --- a/synapse/spam_checker_api/__init__.py +++ b/synapse/spam_checker_api/__init__.py @@ -27,6 +27,10 @@ class RegistrationBehaviour(Enum): DENY = "deny" +# We define the following singleton enum rather than a string to be able to +# write `Union[Allow, ..., str]` in some of the callbacks for the spam-checker +# API, where the `str` is required to maintain backwards compatibility with +# previous versions of the API. class Allow(Enum): """ Singleton to allow events to pass through in SpamChecker APIs. From f449081a3b945b15532d75491e36435ba81c6800 Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 23 May 2022 18:59:02 +0200 Subject: [PATCH 9/9] WIP: Applied feedback --- synapse/events/spamcheck.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index aa1447b70efc..3a318bce2558 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -42,15 +42,18 @@ logger = logging.getLogger(__name__) -# A boolean returned value, kept for backwards compatibility but deprecated. -DEPRECATED_BOOL = bool - -# A string returned value, kept for backwards compatibility but deprecated. -DEPRECATED_STR = str - CHECK_EVENT_FOR_SPAM_CALLBACK = Callable[ ["synapse.events.EventBase"], - Awaitable[Union[Allow, Codes, DEPRECATED_BOOL, DEPRECATED_STR]], + Awaitable[ + Union[ + Allow, + Codes, + # Deprecated + bool, + # Deprecated + str, + ] + ], ] USER_MAY_JOIN_ROOM_CALLBACK = Callable[[str, str, bool], Awaitable[bool]] USER_MAY_INVITE_CALLBACK = Callable[[str, str, str], Awaitable[bool]]