From 59220a19e0a1893600856b3346709f7582dfcf8b Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 16:59:05 +0100 Subject: [PATCH 01/15] Fix import in module_api module --- synapse/module_api/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 95f3b2792793..35be8254d0e5 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -35,7 +35,6 @@ 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 ( @@ -101,6 +100,7 @@ ) from synapse.metrics.background_process_metrics import run_as_background_process from synapse.rest.client.login import LoginResponse +from synapse.spam_checker_api import Allow from synapse.storage import DataStore from synapse.storage.background_updates import ( DEFAULT_BATCH_SIZE_CALLBACK, @@ -141,7 +141,7 @@ PRESENCE_ALL_USERS = PresenceRouter.ALL_USERS -ALLOW = spam_checker_api.Allow.ALLOW +ALLOW = Allow.ALLOW # Singleton value used to mark a message as permitted. __all__ = [ From 2a6bce875374b8f91ecea4b56052d38baa47fe91 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 17:01:01 +0100 Subject: [PATCH 02/15] Changelog --- changelog.d/12918.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12918.bugfix diff --git a/changelog.d/12918.bugfix b/changelog.d/12918.bugfix new file mode 100644 index 000000000000..4e7fc4648225 --- /dev/null +++ b/changelog.d/12918.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse 1.60.0rc1 that would prevent importing the `Allow` type from `synapse.module_api`. From 319cd896451f41c78016f3e621685c7be515c337 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 17:37:30 +0100 Subject: [PATCH 03/15] Fix export --- synapse/module_api/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 35be8254d0e5..fbd6be33c782 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -151,7 +151,7 @@ "respond_with_html", "run_in_background", "cached", - "Allow", + "ALLOW", "UserID", "DatabasePool", "LoggingTransaction", From e70ead2877643897980025103628b33d5d0313cc Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 17:41:45 +0100 Subject: [PATCH 04/15] Also fix upgrade notes --- docs/upgrade.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/upgrade.md b/docs/upgrade.md index e7eadadb64bf..d6370878bbe9 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -177,11 +177,11 @@ 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. +## New signature for the spam checker callback `check_event_for_spam` 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"]`. +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. From 9df86d0562e2d5a8fd4d22a85331ba1b1dfd992c Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 17:46:55 +0100 Subject: [PATCH 05/15] Turns out the doc was also wrong on some things, ugh --- docs/modules/spam_checker_callbacks.md | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 71f6f9f0ab45..06e2e36f2ad1 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -11,24 +11,22 @@ 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 and string return value types deprecated in Synapse v1.60.0_ + +_Changed in Synapse v1.60.0: `synapse.module_api.ALLOW` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean or a string is now deprecated._ ```python -async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", str, 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.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.error.Codes.FORBIDDEN` is a good error code. + - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case + of doubt, `synapse.module_api.errors.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.error.Codes.FORBIDDEN`. Deprecated as confusing, as - some callbacks in expect `True` to allow and others `True` to reject. + - (deprecated) `False`, which is the same as returning `synapse.module_api.ALLOW`. + - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. If multiple modules implement this callback, they will be considered in order. If a callback returns `synapse.module_api.ALLOW`, Synapse falls through to the next one. The value of the From 791c94d23c4a284c815ef7533250b867253f5383 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 18:26:45 +0100 Subject: [PATCH 06/15] Change Allow.ALLOW into NOT_SPAM --- docs/modules/spam_checker_callbacks.md | 17 ++++++++-------- docs/upgrade.md | 6 +++--- synapse/events/spamcheck.py | 28 +++++++++++++------------- synapse/federation/federation_base.py | 3 +-- synapse/handlers/message.py | 18 +++++++++++++---- synapse/module_api/__init__.py | 8 +++----- synapse/spam_checker_api/__init__.py | 25 ----------------------- 7 files changed, 44 insertions(+), 61 deletions(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 06e2e36f2ad1..7b958ff59a9c 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -12,26 +12,27 @@ The available spam checker callbacks are: _First introduced in Synapse v1.37.0_ -_Changed in Synapse v1.60.0: `synapse.module_api.ALLOW` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean or a string is now deprecated._ +_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean or a string is now deprecated._ ```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.NOT_SPAM", "synapse.module_api.errors.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.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still + decide to reject it. - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.module_api.errors.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) `False`, which is the same as returning `synapse.module_api.ALLOW`. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. If multiple modules implement this callback, they will be considered in order. If a -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. +callback returns `synapse.module_api.NOT_SPAM`, Synapse falls through to the next one. +The value of the first callback that does not return `synapse.module_api.NOT_SPAM` 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 d6370878bbe9..e3c64da17fd8 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -181,7 +181,7 @@ has queries that can be used to check a database for this problem in advance. 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"]`. +Whereas `check_event_for_spam` callbacks used to return `Union[str, bool]`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. @@ -204,8 +204,8 @@ async def check_event_for_spam(event): # 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 + # Event is not spam, mark it as such. + return synapse.module_api.NOT_SPAM ``` # Upgrading to v1.59.0 diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 7984874e21df..6d60eeb75515 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, Decision, RegistrationBehaviour +from synapse.spam_checker_api import RegistrationBehaviour from synapse.types import RoomAlias, UserProfile from synapse.util.async_helpers import delay_cancellation, maybe_awaitable from synapse.util.metrics import Measure @@ -46,7 +46,7 @@ ["synapse.events.EventBase"], Awaitable[ Union[ - Allow, + str, Codes, # Deprecated bool, @@ -178,6 +178,8 @@ def run(*args: Any, **kwargs: Any) -> Awaitable: class SpamChecker: + NOT_SPAM = "NOT_SPAM" + def __init__(self, hs: "synapse.server.HomeServer") -> None: self.hs = hs self.clock = hs.get_clock() @@ -270,7 +272,7 @@ def register_callbacks( async def check_event_for_spam( self, event: "synapse.events.EventBase" - ) -> Union[Decision, str]: + ) -> Union[Codes, 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 @@ -281,22 +283,20 @@ async def check_event_for_spam( event: the event to be checked Returns: - - 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 + - `NOT_SPAM` if the event is considered good (non-spammy) and should be let + through. Other spamcheck filters may still reject it. + - A `Code` if 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. + - A string that isn't `NOT_SPAM` if the event is considered spammy and the + string should be used as the client-facing 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[Decision, str, bool] = await delay_cancellation( - callback(event) - ) - if res is False or res is Allow.ALLOW: + res: Union[Codes, str, bool] = await delay_cancellation(callback(event)) + if res is False or res == self.NOT_SPAM: # This spam-checker accepts the event. # Other spam-checkers may reject it, though. continue @@ -310,7 +310,7 @@ async def check_event_for_spam( return res # No spam-checker has rejected the event, let it pass. - return Allow.ALLOW + return self.NOT_SPAM async def should_drop_federated_event( self, event: "synapse.events.EventBase" diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 1e866b19d87b..7bc54b99881e 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -15,7 +15,6 @@ 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 @@ -101,7 +100,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.ALLOW: + if spam_check != self.spam_checker.NOT_SPAM: 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 cb1bc4c06f1c..c2c38de8a91a 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -23,7 +23,6 @@ from twisted.internet.interfaces import IDelayedCall -import synapse from synapse import event_auth from synapse.api.constants import ( EventContentFields, @@ -886,10 +885,21 @@ async def create_and_send_nonmember_event( event.sender, ) - spam_check = await self.spam_checker.check_event_for_spam(event) - if spam_check is not synapse.spam_checker_api.Allow.ALLOW: + spam_check_result = await self.spam_checker.check_event_for_spam(event) + if isinstance(spam_check_result, Codes): raise SynapseError( - 403, "This message had been rejected as probable spam", spam_check + 403, + "This message had been rejected as probable spam", + spam_check_result, + ) + elif ( + isinstance(spam_check_result, str) + and spam_check_result != self.spam_checker.NOT_SPAM + ): + raise SynapseError( + 403, + spam_check_result, + Codes.FORBIDDEN, ) ev = await self.handle_new_client_event( diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index fbd6be33c782..c44e9da121ba 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -54,6 +54,7 @@ USER_MAY_JOIN_ROOM_CALLBACK, USER_MAY_PUBLISH_ROOM_CALLBACK, USER_MAY_SEND_3PID_INVITE_CALLBACK, + SpamChecker, ) from synapse.events.third_party_rules import ( CHECK_CAN_DEACTIVATE_USER_CALLBACK, @@ -100,7 +101,6 @@ ) from synapse.metrics.background_process_metrics import run_as_background_process from synapse.rest.client.login import LoginResponse -from synapse.spam_checker_api import Allow from synapse.storage import DataStore from synapse.storage.background_updates import ( DEFAULT_BATCH_SIZE_CALLBACK, @@ -140,9 +140,7 @@ """ PRESENCE_ALL_USERS = PresenceRouter.ALL_USERS - -ALLOW = Allow.ALLOW -# Singleton value used to mark a message as permitted. +NOT_SPAM = SpamChecker.NOT_SPAM __all__ = [ "errors", @@ -151,7 +149,7 @@ "respond_with_html", "run_in_background", "cached", - "ALLOW", + "NOT_SPAM", "UserID", "DatabasePool", "LoggingTransaction", diff --git a/synapse/spam_checker_api/__init__.py b/synapse/spam_checker_api/__init__.py index 95132c80b70e..75578270ac10 100644 --- a/synapse/spam_checker_api/__init__.py +++ b/synapse/spam_checker_api/__init__.py @@ -12,9 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. from enum import Enum -from typing import Union - -from synapse.api.errors import Codes class RegistrationBehaviour(Enum): @@ -25,25 +22,3 @@ class RegistrationBehaviour(Enum): ALLOW = "allow" SHADOW_BAN = "shadow_ban" 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. - """ - - ALLOW = "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 14b91ed4579b025906cb1df61142941adba6f114 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 18:29:09 +0100 Subject: [PATCH 07/15] Amend changelog --- changelog.d/12918.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12918.bugfix b/changelog.d/12918.bugfix index 4e7fc4648225..38bdd80700e3 100644 --- a/changelog.d/12918.bugfix +++ b/changelog.d/12918.bugfix @@ -1 +1 @@ -Fix a bug introduced in Synapse 1.60.0rc1 that would prevent importing the `Allow` type from `synapse.module_api`. +Fix a bug introduced in Synapse 1.60.0rc1 that would break some imports from `synapse.module_api`. From 3a71525a65bdbcdc6b2a408f0d5202ebc8e622c3 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 18:53:10 +0100 Subject: [PATCH 08/15] Hopefully make the condition block clearer --- synapse/handlers/message.py | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index c2c38de8a91a..8f0ec7e505e3 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -886,16 +886,28 @@ async def create_and_send_nonmember_event( ) spam_check_result = await self.spam_checker.check_event_for_spam(event) - if isinstance(spam_check_result, Codes): - raise SynapseError( - 403, - "This message had been rejected as probable spam", - spam_check_result, - ) - elif ( - isinstance(spam_check_result, str) - and spam_check_result != self.spam_checker.NOT_SPAM + # For backwards compatibility, we need to raise if the result is either + # NOT_SPAM or the boolean True (the latter is now deprecated). + if ( + spam_check_result != self.spam_checker.NOT_SPAM + or spam_check_result is True ): + # We need to check whether the return value is an error code + # specifically for backwards compatibility, since values in Codes + # are also strings and this callback allows modules to return + # arbitrary strings to be used in SynapseErrors (which is now + # deprecated). + if isinstance(spam_check_result, Codes): + raise SynapseError( + 403, + "This message had been rejected as probable spam", + spam_check_result, + ) + + if not isinstance(spam_check_result, str): + # Make sure we give the SynapseError a string as the error message. + spam_check_result = "This message had been rejected as probable spam" + raise SynapseError( 403, spam_check_result, From db4c2e86e5b16d040902564a8d8b3191980d7494 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 19:53:52 +0200 Subject: [PATCH 09/15] Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- docs/modules/spam_checker_callbacks.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 7b958ff59a9c..ad35e667ed4b 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -18,12 +18,12 @@ _Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_a async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", str, bool] ``` -Called when receiving an event from a client or via federation. The callback must return either: +Called when receiving an event from a client or via federation. The callback must return one of: - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still decide to reject it. - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. - - (deprecated) a `str` to reject the operation and specify an error message. Note that clients + - (deprecated) a non-`Codes` `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) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. From 3007fa5e5a312ea9b58253675510f09f37426103 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 18:55:22 +0100 Subject: [PATCH 10/15] Incorporate review --- synapse/events/spamcheck.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 6d60eeb75515..aa3b53d8153d 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -47,11 +47,8 @@ Awaitable[ Union[ str, - Codes, # Deprecated bool, - # Deprecated - str, ] ], ] @@ -272,7 +269,7 @@ def register_callbacks( async def check_event_for_spam( self, event: "synapse.events.EventBase" - ) -> Union[Codes, str]: + ) -> Union[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 @@ -295,7 +292,7 @@ async def check_event_for_spam( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - res: Union[Codes, str, bool] = await delay_cancellation(callback(event)) + res: Union[str, bool] = await delay_cancellation(callback(event)) if res is False or res == self.NOT_SPAM: # This spam-checker accepts the event. # Other spam-checkers may reject it, though. From 4a1f98af2f81acfea6095c4ab4d66bf4889f8a69 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 18:56:13 +0100 Subject: [PATCH 11/15] Fix phrasing --- synapse/handlers/message.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 8f0ec7e505e3..093291a5810e 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -900,13 +900,13 @@ async def create_and_send_nonmember_event( if isinstance(spam_check_result, Codes): raise SynapseError( 403, - "This message had been rejected as probable spam", + "This message has been rejected as probable spam", spam_check_result, ) if not isinstance(spam_check_result, str): # Make sure we give the SynapseError a string as the error message. - spam_check_result = "This message had been rejected as probable spam" + spam_check_result = "This message has been rejected as probable spam" raise SynapseError( 403, From 5f0de4f9f639c7dbceb066c34df01f35f5023667 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 30 May 2022 19:07:07 +0100 Subject: [PATCH 12/15] Fix lint and types, and move check to callback layer --- synapse/events/spamcheck.py | 12 +++++++++++- synapse/handlers/message.py | 11 +---------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index aa3b53d8153d..28a7d9c65e6b 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -292,7 +292,7 @@ async def check_event_for_spam( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - res: Union[str, bool] = await delay_cancellation(callback(event)) + res = await delay_cancellation(callback(event)) if res is False or res == self.NOT_SPAM: # This spam-checker accepts the event. # Other spam-checkers may reject it, though. @@ -304,6 +304,16 @@ async def check_event_for_spam( else: # This spam-checker rejects the event either with a `str` # or with a `Codes`. In either case, we stop here. + if not isinstance(res, str): + # Make sure we give the SynapseError a string as the error + # message. + # mypy complains that we can't reach this code because of the + # return type in CHECK_EVENT_FOR_SPAM_CALLBACK, but we don't know + # for sure that the module actually returns it. + res = ( # type: ignore[unreachable] + "This message has been rejected as probable spam" + ) + return res # No spam-checker has rejected the event, let it pass. diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 093291a5810e..952f2f607b39 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -886,12 +886,7 @@ async def create_and_send_nonmember_event( ) spam_check_result = await self.spam_checker.check_event_for_spam(event) - # For backwards compatibility, we need to raise if the result is either - # NOT_SPAM or the boolean True (the latter is now deprecated). - if ( - spam_check_result != self.spam_checker.NOT_SPAM - or spam_check_result is True - ): + if spam_check_result != self.spam_checker.NOT_SPAM: # We need to check whether the return value is an error code # specifically for backwards compatibility, since values in Codes # are also strings and this callback allows modules to return @@ -904,10 +899,6 @@ async def create_and_send_nonmember_event( spam_check_result, ) - if not isinstance(spam_check_result, str): - # Make sure we give the SynapseError a string as the error message. - spam_check_result = "This message has been rejected as probable spam" - raise SynapseError( 403, spam_check_result, From 11a1b9af676a67595ddfea9e127482d7423ed48b Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 31 May 2022 11:10:29 +0200 Subject: [PATCH 13/15] Update synapse/events/spamcheck.py Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/events/spamcheck.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 28a7d9c65e6b..5eafdb2a89c5 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -269,7 +269,7 @@ def register_callbacks( async def check_event_for_spam( self, event: "synapse.events.EventBase" - ) -> Union[str]: + ) -> 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 From f4c574bce13a5c9b678db7b89bc88224c3a9bc34 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 31 May 2022 10:24:29 +0100 Subject: [PATCH 14/15] Incorporate review --- synapse/events/spamcheck.py | 24 ++++++++++++------------ synapse/handlers/message.py | 8 +++----- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 28a7d9c65e6b..3323482c141a 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -301,20 +301,20 @@ async def check_event_for_spam( # This spam-checker rejects the event with deprecated # return value `True` return Codes.FORBIDDEN + elif not isinstance(res, str): + # mypy complains that we can't reach this code because of the + # return type in CHECK_EVENT_FOR_SPAM_CALLBACK, but we don't know + # for sure that the module actually returns it. + logger.warning( # type: ignore[unreachable] + "Module returned invalid value, rejecting message as spam" + ) + res = "This message has been rejected as probable spam" else: - # This spam-checker rejects the event either with a `str` - # or with a `Codes`. In either case, we stop here. - if not isinstance(res, str): - # Make sure we give the SynapseError a string as the error - # message. - # mypy complains that we can't reach this code because of the - # return type in CHECK_EVENT_FOR_SPAM_CALLBACK, but we don't know - # for sure that the module actually returns it. - res = ( # type: ignore[unreachable] - "This message has been rejected as probable spam" - ) + # The module rejected the event either with a `Codes` + # or some other `str`. In either case, we stop here. + pass - return res + return res # No spam-checker has rejected the event, let it pass. return self.NOT_SPAM diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 952f2f607b39..22cdad3f3353 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -887,11 +887,6 @@ async def create_and_send_nonmember_event( spam_check_result = await self.spam_checker.check_event_for_spam(event) if spam_check_result != self.spam_checker.NOT_SPAM: - # We need to check whether the return value is an error code - # specifically for backwards compatibility, since values in Codes - # are also strings and this callback allows modules to return - # arbitrary strings to be used in SynapseErrors (which is now - # deprecated). if isinstance(spam_check_result, Codes): raise SynapseError( 403, @@ -899,6 +894,9 @@ async def create_and_send_nonmember_event( spam_check_result, ) + # Backwards compatibility: if the return value is not an error code, it + # means the module returned an error message to be included in the + # SynapseError (which is now deprecated). raise SynapseError( 403, spam_check_result, From 9f5d43e5f77f2f90c90d36c501aacf331b7b0933 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 31 May 2022 10:28:39 +0100 Subject: [PATCH 15/15] Lint --- synapse/events/spamcheck.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index b592dae34a73..1048b4c825f6 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -267,9 +267,7 @@ def register_callbacks( if check_media_file_for_spam is not None: self._check_media_file_for_spam_callbacks.append(check_media_file_for_spam) - async def check_event_for_spam( - self, event: "synapse.events.EventBase" - ) -> str: + async def check_event_for_spam(self, event: "synapse.events.EventBase") -> 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