From 000a104966e621932feb179ef66e146682855298 Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 14 Jun 2022 10:16:25 +0200 Subject: [PATCH 01/24] Uniformize spam-checker API, part 5: expand other spam-checker callbacks to return Tuple[Codes, dict] --- synapse/api/errors.py | 4 +- synapse/events/spamcheck.py | 79 +++++++++++++++++++++----- synapse/handlers/directory.py | 6 +- synapse/handlers/federation.py | 3 +- synapse/handlers/room.py | 2 +- synapse/handlers/room_member.py | 12 ++-- synapse/rest/media/v1/media_storage.py | 4 +- 7 files changed, 82 insertions(+), 28 deletions(-) diff --git a/synapse/api/errors.py b/synapse/api/errors.py index cc7b785472c6..34875669cffa 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -297,8 +297,8 @@ class AuthError(SynapseError): other poorly-defined times. """ - def __init__(self, code: int, msg: str, errcode: str = Codes.FORBIDDEN): - super().__init__(code, msg, errcode) + def __init__(self, code: int, msg: str, errcode: str = Codes.FORBIDDEN, additional_fields: Optional[dict] = None): + super().__init__(code, msg, errcode, additional_fields) class InvalidClientCredentialsError(SynapseError): diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 32712d2042ef..25d8288fc734 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -71,6 +71,11 @@ Union[ Literal["NOT_SPAM"], "synapse.api.errors.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["synapse.api.errors.Codes", Dict], # Deprecated bool, ] @@ -82,6 +87,11 @@ Union[ Literal["NOT_SPAM"], "synapse.api.errors.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["synapse.api.errors.Codes", Dict], # Deprecated bool, ] @@ -93,6 +103,11 @@ Union[ Literal["NOT_SPAM"], "synapse.api.errors.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["synapse.api.errors.Codes", Dict], # Deprecated bool, ] @@ -104,6 +119,11 @@ Union[ Literal["NOT_SPAM"], "synapse.api.errors.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["synapse.api.errors.Codes", Dict], # Deprecated bool, ] @@ -115,6 +135,11 @@ Union[ Literal["NOT_SPAM"], "synapse.api.errors.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["synapse.api.errors.Codes", Dict], # Deprecated bool, ] @@ -126,6 +151,11 @@ Union[ Literal["NOT_SPAM"], "synapse.api.errors.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["synapse.api.errors.Codes", Dict], # Deprecated bool, ] @@ -155,6 +185,11 @@ Union[ Literal["NOT_SPAM"], "synapse.api.errors.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["synapse.api.errors.Codes", Dict], # Deprecated bool, ] @@ -422,7 +457,7 @@ async def should_drop_federated_event( async def user_may_join_room( self, user_id: str, room_id: str, is_invited: bool - ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: + ) -> Union[Tuple["synapse.api.errors.Codes", Dict], Literal["NOT_SPAM"]]: """Checks if a given users is allowed to join a room. Not called when a user creates a room. @@ -432,7 +467,7 @@ async def user_may_join_room( is_invited: Whether the user is invited into the room Returns: - NOT_SPAM if the operation is permitted, Codes otherwise. + NOT_SPAM if the operation is permitted, [Codes, Dict] otherwise. """ for callback in self._user_may_join_room_callbacks: with Measure( @@ -443,8 +478,10 @@ async def user_may_join_room( if res is True or res is self.NOT_SPAM: continue elif res is False: - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): + return (res, {}) + elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): return res else: logger.warning( @@ -457,7 +494,7 @@ async def user_may_join_room( async def user_may_invite( self, inviter_userid: str, invitee_userid: str, room_id: str - ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: + ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: """Checks if a given user may send an invite Args: @@ -479,8 +516,10 @@ async def user_may_invite( if res is True or res is self.NOT_SPAM: continue elif res is False: - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): + return (res, {}) + elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): return res else: logger.warning( @@ -493,7 +532,7 @@ async def user_may_invite( async def user_may_send_3pid_invite( self, inviter_userid: str, medium: str, address: str, room_id: str - ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: + ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: """Checks if a given user may invite a given threepid into the room Note that if the threepid is already associated with a Matrix user ID, Synapse @@ -519,8 +558,10 @@ async def user_may_send_3pid_invite( if res is True or res is self.NOT_SPAM: continue elif res is False: - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): + return (res, {}) + elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): return res else: logger.warning( @@ -532,7 +573,7 @@ async def user_may_send_3pid_invite( async def user_may_create_room( self, userid: str - ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: + ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: """Checks if a given user may create a room Args: @@ -546,8 +587,10 @@ async def user_may_create_room( if res is True or res is self.NOT_SPAM: continue elif res is False: - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): + return (res, {}) + elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): return res else: logger.warning( @@ -559,7 +602,7 @@ async def user_may_create_room( async def user_may_create_room_alias( self, userid: str, room_alias: RoomAlias - ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: + ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: """Checks if a given user may create a room alias Args: @@ -575,8 +618,10 @@ async def user_may_create_room_alias( if res is True or res is self.NOT_SPAM: continue elif res is False: - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): + return (res, {}) + elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): return res else: logger.warning( @@ -588,7 +633,7 @@ async def user_may_create_room_alias( async def user_may_publish_room( self, userid: str, room_id: str - ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: + ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: """Checks if a given user may publish a room to the directory Args: @@ -603,8 +648,10 @@ async def user_may_publish_room( if res is True or res is self.NOT_SPAM: continue elif res is False: - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): + return (res, {}) + elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): return res else: logger.warning( @@ -678,7 +725,7 @@ async def check_registration_for_spam( async def check_media_file_for_spam( self, file_wrapper: ReadableFileWrapper, file_info: FileInfo - ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: + ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: """Checks if a piece of newly uploaded media should be blocked. This will be called for local uploads, downloads of remote media, each @@ -715,8 +762,10 @@ async def check_media_file_for_spam( if res is False or res is self.NOT_SPAM: continue elif res is True: - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): + return (res, {}) + elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): return res else: logger.warning( diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 8b0f16f9652f..6fd7abb18f2c 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -149,7 +149,8 @@ async def create_association( raise AuthError( 403, "This user is not permitted to create this alias", - spam_check, + errcode = spam_check[0], + additional_fields = spam_check[1] ) if not self.config.roomdirectory.is_alias_creation_allowed( @@ -441,7 +442,8 @@ async def edit_published_room_list( raise AuthError( 403, "This user is not permitted to publish rooms to the room list", - spam_check, + errcode = spam_check[0], + additional_fields = spam_check[1] ) if requester.is_guest: diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 34cc5ecd1144..929ba0865216 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -828,7 +828,8 @@ async def on_invite_request( raise SynapseError( 403, "This user is not permitted to send invites to this server/user", - spam_check, + errcode = spam_check[0], + additional_fields = spam_check[1] ) membership = event.content.get("membership") diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 42aae4a215a8..5b591573c1a5 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -439,7 +439,7 @@ async def clone_existing_room( spam_check = await self.spam_checker.user_may_create_room(user_id) if spam_check != NOT_SPAM: - raise SynapseError(403, "You are not permitted to create rooms", spam_check) + raise SynapseError(403, "You are not permitted to create rooms", errcode = spam_check[0], additional_fields = spam_check[1]) creation_content: JsonDict = { "room_version": new_room_version.identifier, diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index e89b7441adff..b8d414aff66a 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -702,18 +702,18 @@ async def update_membership_locked( "Blocking invite: user is not admin and non-admin " "invites disabled" ) - block_invite_code = Codes.FORBIDDEN + block_invite_result = (Codes.FORBIDDEN, {}) spam_check = await self.spam_checker.user_may_invite( requester.user.to_string(), target_id, room_id ) if spam_check != NOT_SPAM: logger.info("Blocking invite due to spam checker") - block_invite_code = spam_check + block_invite_result = spam_check - if block_invite_code is not None: + if block_invite_result is not None: raise SynapseError( - 403, "Invites have been disabled on this server", block_invite_code + 403, "Invites have been disabled on this server", errcode = block_invite_result[0], additional_fields = block_invite_result[1] ) # An empty prev_events list is allowed as long as the auth_event_ids are present @@ -827,7 +827,7 @@ async def update_membership_locked( target.to_string(), room_id, is_invited=inviter is not None ) if spam_check != NOT_SPAM: - raise SynapseError(403, "Not allowed to join this room", spam_check) + raise SynapseError(403, "Not allowed to join this room", errcode = spam_check[0], additional_fields = spam_check[1]) # Check if a remote join should be performed. remote_join, remote_room_hosts = await self._should_perform_remote_join( @@ -1381,7 +1381,7 @@ async def do_3pid_invite( room_id=room_id, ) if spam_check != NOT_SPAM: - raise SynapseError(403, "Cannot send threepid invite", spam_check) + raise SynapseError(403, "Cannot send threepid invite", errcode=spam_check[0], additional_fields=spam_check[1]) stream_id = await self._make_and_store_3pid_invite( requester, diff --git a/synapse/rest/media/v1/media_storage.py b/synapse/rest/media/v1/media_storage.py index 913741734239..a5c3de192f6f 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -154,7 +154,9 @@ async def finish() -> None: # Note that we'll delete the stored media, due to the # try/except below. The media also won't be stored in # the DB. - raise SpamMediaException(errcode=spam_check) + # We currently ignore any additional field returned by + # the spam-check API. + raise SpamMediaException(errcode=spam_check[0]) for provider in self.storage_providers: await provider.store_file(path, file_info) From e71aacd87021857147055aecbce65dbb61441063 Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 14 Jun 2022 10:30:56 +0200 Subject: [PATCH 02/24] WIP: lint --- synapse/api/errors.py | 8 +++++- synapse/events/spamcheck.py | 49 ++++++++++++++++++++++++++++----- synapse/handlers/directory.py | 8 +++--- synapse/handlers/federation.py | 4 +-- synapse/handlers/room.py | 7 ++++- synapse/handlers/room_member.py | 19 +++++++++++-- 6 files changed, 77 insertions(+), 18 deletions(-) diff --git a/synapse/api/errors.py b/synapse/api/errors.py index 34875669cffa..1c74e131f2b2 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -297,7 +297,13 @@ class AuthError(SynapseError): other poorly-defined times. """ - def __init__(self, code: int, msg: str, errcode: str = Codes.FORBIDDEN, additional_fields: Optional[dict] = None): + def __init__( + self, + code: int, + msg: str, + errcode: str = Codes.FORBIDDEN, + additional_fields: Optional[dict] = None, + ): super().__init__(code, msg, errcode, additional_fields) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 25d8288fc734..62046d4aa0b7 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -481,7 +481,12 @@ async def user_may_join_room( return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): return (res, {}) - elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): + elif ( + isinstance(res, Tuple) + and len(res) == 2 + and isinstance(res[0], synapse.api.errors.Codes) + and isinstance(res[1], dict) + ): return res else: logger.warning( @@ -519,7 +524,12 @@ async def user_may_invite( return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): return (res, {}) - elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): + elif ( + isinstance(res, Tuple) + and len(res) == 2 + and isinstance(res[0], synapse.api.errors.Codes) + and isinstance(res[1], dict) + ): return res else: logger.warning( @@ -561,7 +571,12 @@ async def user_may_send_3pid_invite( return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): return (res, {}) - elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): + elif ( + isinstance(res, Tuple) + and len(res) == 2 + and isinstance(res[0], synapse.api.errors.Codes) + and isinstance(res[1], dict) + ): return res else: logger.warning( @@ -590,7 +605,12 @@ async def user_may_create_room( return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): return (res, {}) - elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): + elif ( + isinstance(res, Tuple) + and len(res) == 2 + and isinstance(res[0], synapse.api.errors.Codes) + and isinstance(res[1], dict) + ): return res else: logger.warning( @@ -621,7 +641,12 @@ async def user_may_create_room_alias( return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): return (res, {}) - elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): + elif ( + isinstance(res, Tuple) + and len(res) == 2 + and isinstance(res[0], synapse.api.errors.Codes) + and isinstance(res[1], dict) + ): return res else: logger.warning( @@ -651,7 +676,12 @@ async def user_may_publish_room( return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): return (res, {}) - elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): + elif ( + isinstance(res, Tuple) + and len(res) == 2 + and isinstance(res[0], synapse.api.errors.Codes) + and isinstance(res[1], dict) + ): return res else: logger.warning( @@ -765,7 +795,12 @@ async def check_media_file_for_spam( return (synapse.api.errors.Codes.FORBIDDEN, {}) elif isinstance(res, synapse.api.errors.Codes): return (res, {}) - elif isinstance(res, Tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict): + elif ( + isinstance(res, Tuple) + and len(res) == 2 + and isinstance(res[0], synapse.api.errors.Codes) + and isinstance(res[1], dict) + ): return res else: logger.warning( diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 6fd7abb18f2c..09a7a4b2380a 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -149,8 +149,8 @@ async def create_association( raise AuthError( 403, "This user is not permitted to create this alias", - errcode = spam_check[0], - additional_fields = spam_check[1] + errcode=spam_check[0], + additional_fields=spam_check[1], ) if not self.config.roomdirectory.is_alias_creation_allowed( @@ -442,8 +442,8 @@ async def edit_published_room_list( raise AuthError( 403, "This user is not permitted to publish rooms to the room list", - errcode = spam_check[0], - additional_fields = spam_check[1] + errcode=spam_check[0], + additional_fields=spam_check[1], ) if requester.is_guest: diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 929ba0865216..5e658bc9128a 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -828,8 +828,8 @@ async def on_invite_request( raise SynapseError( 403, "This user is not permitted to send invites to this server/user", - errcode = spam_check[0], - additional_fields = spam_check[1] + errcode=spam_check[0], + additional_fields=spam_check[1], ) membership = event.content.get("membership") diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 5b591573c1a5..820dafc745bb 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -439,7 +439,12 @@ async def clone_existing_room( spam_check = await self.spam_checker.user_may_create_room(user_id) if spam_check != NOT_SPAM: - raise SynapseError(403, "You are not permitted to create rooms", errcode = spam_check[0], additional_fields = spam_check[1]) + raise SynapseError( + 403, + "You are not permitted to create rooms", + errcode=spam_check[0], + additional_fields=spam_check[1], + ) creation_content: JsonDict = { "room_version": new_room_version.identifier, diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index b8d414aff66a..9f11deb78a65 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -713,7 +713,10 @@ async def update_membership_locked( if block_invite_result is not None: raise SynapseError( - 403, "Invites have been disabled on this server", errcode = block_invite_result[0], additional_fields = block_invite_result[1] + 403, + "Invites have been disabled on this server", + errcode=block_invite_result[0], + additional_fields=block_invite_result[1], ) # An empty prev_events list is allowed as long as the auth_event_ids are present @@ -827,7 +830,12 @@ async def update_membership_locked( target.to_string(), room_id, is_invited=inviter is not None ) if spam_check != NOT_SPAM: - raise SynapseError(403, "Not allowed to join this room", errcode = spam_check[0], additional_fields = spam_check[1]) + raise SynapseError( + 403, + "Not allowed to join this room", + errcode=spam_check[0], + additional_fields=spam_check[1], + ) # Check if a remote join should be performed. remote_join, remote_room_hosts = await self._should_perform_remote_join( @@ -1381,7 +1389,12 @@ async def do_3pid_invite( room_id=room_id, ) if spam_check != NOT_SPAM: - raise SynapseError(403, "Cannot send threepid invite", errcode=spam_check[0], additional_fields=spam_check[1]) + raise SynapseError( + 403, + "Cannot send threepid invite", + errcode=spam_check[0], + additional_fields=spam_check[1], + ) stream_id = await self._make_and_store_3pid_invite( requester, From 2f3c8f7e8f6630f9cfa1f0110d764e49688b4f5e Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 14 Jun 2022 10:48:55 +0200 Subject: [PATCH 03/24] WIP: Lint --- synapse/events/spamcheck.py | 28 ++++++++++++++-------------- synapse/handlers/room.py | 5 ++++- synapse/handlers/room_member.py | 2 +- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 62046d4aa0b7..5df0669d8c18 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -482,7 +482,7 @@ async def user_may_join_room( elif isinstance(res, synapse.api.errors.Codes): return (res, {}) elif ( - isinstance(res, Tuple) + isinstance(res, tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict) @@ -492,7 +492,7 @@ async def user_may_join_room( logger.warning( "Module returned invalid value, rejecting join as spam" ) - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) # No spam-checker has rejected the request, let it pass. return self.NOT_SPAM @@ -525,7 +525,7 @@ async def user_may_invite( elif isinstance(res, synapse.api.errors.Codes): return (res, {}) elif ( - isinstance(res, Tuple) + isinstance(res, tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict) @@ -535,7 +535,7 @@ async def user_may_invite( logger.warning( "Module returned invalid value, rejecting invite as spam" ) - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) # No spam-checker has rejected the request, let it pass. return self.NOT_SPAM @@ -572,7 +572,7 @@ async def user_may_send_3pid_invite( elif isinstance(res, synapse.api.errors.Codes): return (res, {}) elif ( - isinstance(res, Tuple) + isinstance(res, tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict) @@ -582,7 +582,7 @@ async def user_may_send_3pid_invite( logger.warning( "Module returned invalid value, rejecting 3pid invite as spam" ) - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) return self.NOT_SPAM @@ -606,7 +606,7 @@ async def user_may_create_room( elif isinstance(res, synapse.api.errors.Codes): return (res, {}) elif ( - isinstance(res, Tuple) + isinstance(res, tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict) @@ -616,7 +616,7 @@ async def user_may_create_room( logger.warning( "Module returned invalid value, rejecting room creation as spam" ) - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) return self.NOT_SPAM @@ -642,7 +642,7 @@ async def user_may_create_room_alias( elif isinstance(res, synapse.api.errors.Codes): return (res, {}) elif ( - isinstance(res, Tuple) + isinstance(res, tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict) @@ -652,7 +652,7 @@ async def user_may_create_room_alias( logger.warning( "Module returned invalid value, rejecting room create as spam" ) - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) return self.NOT_SPAM @@ -677,7 +677,7 @@ async def user_may_publish_room( elif isinstance(res, synapse.api.errors.Codes): return (res, {}) elif ( - isinstance(res, Tuple) + isinstance(res, tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict) @@ -687,7 +687,7 @@ async def user_may_publish_room( logger.warning( "Module returned invalid value, rejecting room publication as spam" ) - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) return self.NOT_SPAM @@ -796,7 +796,7 @@ async def check_media_file_for_spam( elif isinstance(res, synapse.api.errors.Codes): return (res, {}) elif ( - isinstance(res, Tuple) + isinstance(res, tuple) and len(res) == 2 and isinstance(res[0], synapse.api.errors.Codes) and isinstance(res[1], dict) @@ -806,6 +806,6 @@ async def check_media_file_for_spam( logger.warning( "Module returned invalid value, rejecting media file as spam" ) - return synapse.api.errors.Codes.FORBIDDEN + return (synapse.api.errors.Codes.FORBIDDEN, {}) return self.NOT_SPAM diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 820dafc745bb..f4a998dc97a6 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -735,7 +735,10 @@ async def create_room( spam_check = await self.spam_checker.user_may_create_room(user_id) if spam_check != NOT_SPAM: raise SynapseError( - 403, "You are not permitted to create rooms", spam_check + 403, + "You are not permitted to create rooms", + errcode=spam_check[0], + additional_fields=spam_check[1], ) if ratelimit: diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 9f11deb78a65..c44adf6e3e83 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -684,7 +684,7 @@ async def update_membership_locked( if target_id == self._server_notices_mxid: raise SynapseError(HTTPStatus.FORBIDDEN, "Cannot invite this user") - block_invite_code = None + block_invite_result = None if ( self._server_notices_mxid is not None From e714f8dd1d3ddeabb918084e22433dfcdd372719 Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 14 Jun 2022 10:53:50 +0200 Subject: [PATCH 04/24] Signed-off-by: David Teller WIP: Added changelog --- changelog.d/13044.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13044.misc diff --git a/changelog.d/13044.misc b/changelog.d/13044.misc new file mode 100644 index 000000000000..07652a87c025 --- /dev/null +++ b/changelog.d/13044.misc @@ -0,0 +1 @@ +Experimental: expand Spam-Checker API callbacks of `user_may_join_room`, `user_may_invite`, `user_may_send_3pid_invite`, `user_may_create_room`, `user_may_create_room_alias`, `user_may_publish_room`, `check_media_file_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. \ No newline at end of file From f97cd9d7e1664e734629fc20d01fde14f9f712c9 Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 14 Jun 2022 13:16:08 +0200 Subject: [PATCH 05/24] WIP: New tests --- tests/rest/client/test_rooms.py | 41 ++++++++++++++- tests/rest/media/v1/test_media_storage.py | 63 +++++++++++++++++++++-- 2 files changed, 98 insertions(+), 6 deletions(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 35c59ee9e034..19c9c8c48dae 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -813,14 +813,14 @@ def test_spam_checker_may_join_room(self) -> None: In this test, we use the more recent API in which callbacks return a `Union[Codes, Literal["NOT_SPAM"]]`. """ - async def user_may_join_room( + async def user_may_join_room_codes( mxid: str, room_id: str, is_invite: bool, ) -> Codes: return Codes.CONSENT_NOT_GIVEN - join_mock = Mock(side_effect=user_may_join_room) + join_mock = Mock(side_effect=user_may_join_room_codes) self.hs.get_spam_checker()._user_may_join_room_callbacks.append(join_mock) channel = self.make_request( @@ -832,6 +832,25 @@ async def user_may_join_room( self.assertEqual(join_mock.call_count, 0) + async def user_may_join_room_tuple( + mxid: str, + room_id: str, + is_invite: bool, + ) -> Codes: + return [Codes.CONSENT_NOT_GIVEN, {}] + + join_mock = Mock(side_effect=user_may_join_room_codes) + self.hs.get_spam_checker()._user_may_join_room_callbacks = [join_mock] + + channel = self.make_request( + "POST", + "/createRoom", + {}, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + self.assertEqual(join_mock.call_count, 0) + class RoomTopicTestCase(RoomBase): """Tests /rooms/$room_id/topic REST events.""" @@ -3166,3 +3185,21 @@ def test_threepid_invite_spamcheck(self) -> None: # Also check that it stopped before calling _make_and_store_3pid_invite. make_invite_mock.assert_called_once() + + # Run variant with `Tuple[Codes, dict]`. + mock.return_value = make_awaitable((Codes.CONSENT_NOT_GIVEN, {})) + channel = self.make_request( + method="POST", + path="/rooms/" + self.room_id + "/invite", + content={ + "id_server": "example.com", + "id_access_token": "sometoken", + "medium": "email", + "address": email_to_invite, + }, + access_token=self.tok, + ) + self.assertEqual(channel.code, 403) + + # Also check that it stopped before calling _make_and_store_3pid_invite. + make_invite_mock.assert_called_once() diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 7204b2dfe075..5f33c27df15b 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -16,7 +16,7 @@ import tempfile from binascii import unhexlify from io import BytesIO -from typing import Any, BinaryIO, Dict, List, Optional, Union +from typing import Any, BinaryIO, Dict, List, Literal, Optional, Union from unittest.mock import Mock from urllib import parse @@ -27,6 +27,7 @@ from twisted.internet import defer from twisted.internet.defer import Deferred from twisted.test.proto_helpers import MemoryReactor +from synapse.api.errors import Codes from synapse.events import EventBase from synapse.events.spamcheck import load_legacy_spam_checkers @@ -550,9 +551,11 @@ def test_x_robots_tag_header(self) -> None: ) -class TestSpamChecker: +class TestSpamCheckerDeprecated: """A spam checker module that rejects all media that includes the bytes `evil`. + + Uses the deprecated Spam-Checker API. """ def __init__(self, config: Dict[str, Any], api: ModuleApi) -> None: @@ -593,7 +596,7 @@ async def check_media_file_for_spam( return b"evil" in buf.getvalue() -class SpamCheckerTestCase(unittest.HomeserverTestCase): +class SpamCheckerTestCaseDeprecated(unittest.HomeserverTestCase): servlets = [ login.register_servlets, admin.register_servlets, @@ -617,7 +620,7 @@ def default_config(self) -> Dict[str, Any]: { "spam_checker": [ { - "module": TestSpamChecker.__module__ + ".TestSpamChecker", + "module": TestSpamCheckerDeprecated.__module__ + ".TestSpamChecker", "config": {}, } ] @@ -642,3 +645,55 @@ def test_upload_ban(self) -> None: self.helper.upload_media( self.upload_resource, data, tok=self.tok, expect_code=400 ) + +class SpamCheckerTestCase(unittest.HomeserverTestCase): + servlets = [ + login.register_servlets, + admin.register_servlets, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.user = self.register_user("user", "pass") + self.tok = self.login("user", "pass") + + # Allow for uploading and downloading to/from the media repo + self.media_repo = hs.get_media_repository_resource() + self.download_resource = self.media_repo.children[b"download"] + self.upload_resource = self.media_repo.children[b"upload"] + + hs.get_module_api().register_spam_checker_callbacks( + check_media_file_for_spam=self.check_media_file_for_spam + ) + + async def check_media_file_for_spam( + self, file_wrapper: ReadableFileWrapper, file_info: FileInfo + ) -> Union[Codes, Literal["NOT_SPAM"]]: + buf = BytesIO() + await file_wrapper.write_chunks_to(buf.write) + + if b"evil" in buf.getvalue(): + return Codes.FORBIDDEN + elif b"experimental" in buf.getvalue(): + return (Codes.FORBIDDEN, {}) + else: + return "NOT_SPAM" + + + def test_upload_innocent(self) -> None: + """Attempt to upload some innocent data that should be allowed.""" + self.helper.upload_media( + self.upload_resource, SMALL_PNG, tok=self.tok, expect_code=200 + ) + + def test_upload_ban(self) -> None: + """Attempt to upload some data that includes bytes "evil", which should + get rejected by the spam checker. + """ + + self.helper.upload_media( + self.upload_resource, b"Some evil data", tok=self.tok, expect_code=400 + ) + + self.helper.upload_media( + self.upload_resource, b"Let's try the experimental API", tok=self.tok, expect_code=400 + ) From 102daec88d85a0fe6e97c4f3e496d8d6e2887aa2 Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 14 Jun 2022 13:21:20 +0200 Subject: [PATCH 06/24] WIP: Linting --- tests/rest/client/test_rooms.py | 6 +++--- tests/rest/media/v1/test_media_storage.py | 12 ++++++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 19c9c8c48dae..21e0c3b26a01 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -18,7 +18,7 @@ """Tests REST events for /rooms paths.""" import json -from typing import Any, Dict, Iterable, List, Optional, Union +from typing import Any, Dict, Iterable, List, Optional, Tuple, Union from unittest.mock import Mock, call from urllib import parse as urlparse @@ -836,8 +836,8 @@ async def user_may_join_room_tuple( mxid: str, room_id: str, is_invite: bool, - ) -> Codes: - return [Codes.CONSENT_NOT_GIVEN, {}] + ) -> Tuple[Codes, dict]: + return (Codes.CONSENT_NOT_GIVEN, {}) join_mock = Mock(side_effect=user_may_join_room_codes) self.hs.get_spam_checker()._user_may_join_room_callbacks = [join_mock] diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 5f33c27df15b..9e5160c77134 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -27,8 +27,8 @@ from twisted.internet import defer from twisted.internet.defer import Deferred from twisted.test.proto_helpers import MemoryReactor -from synapse.api.errors import Codes +from synapse.api.errors import Codes from synapse.events import EventBase from synapse.events.spamcheck import load_legacy_spam_checkers from synapse.logging.context import make_deferred_yieldable @@ -620,7 +620,8 @@ def default_config(self) -> Dict[str, Any]: { "spam_checker": [ { - "module": TestSpamCheckerDeprecated.__module__ + ".TestSpamChecker", + "module": TestSpamCheckerDeprecated.__module__ + + ".TestSpamChecker", "config": {}, } ] @@ -646,6 +647,7 @@ def test_upload_ban(self) -> None: self.upload_resource, data, tok=self.tok, expect_code=400 ) + class SpamCheckerTestCase(unittest.HomeserverTestCase): servlets = [ login.register_servlets, @@ -678,7 +680,6 @@ async def check_media_file_for_spam( else: return "NOT_SPAM" - def test_upload_innocent(self) -> None: """Attempt to upload some innocent data that should be allowed.""" self.helper.upload_media( @@ -695,5 +696,8 @@ def test_upload_ban(self) -> None: ) self.helper.upload_media( - self.upload_resource, b"Let's try the experimental API", tok=self.tok, expect_code=400 + self.upload_resource, + b"Let's try the experimental API", + tok=self.tok, + expect_code=400, ) From 2710757d1aa5140192ba75ccce2ca229c1a801a6 Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 14 Jun 2022 13:54:47 +0200 Subject: [PATCH 07/24] WIP: Fixing test --- tests/rest/media/v1/test_media_storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 9e5160c77134..27c11e8c692f 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -621,7 +621,7 @@ def default_config(self) -> Dict[str, Any]: "spam_checker": [ { "module": TestSpamCheckerDeprecated.__module__ - + ".TestSpamChecker", + + ".TestSpamCheckerDeprecated", "config": {}, } ] From b31f2545b7a2ad9debf994ecfc77f9a602047e0e Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 14 Jun 2022 15:32:36 +0200 Subject: [PATCH 08/24] WIP: Oops, let's not forget 3.7 with our tests --- tests/rest/media/v1/test_media_storage.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 27c11e8c692f..b815111d8e52 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -16,7 +16,9 @@ import tempfile from binascii import unhexlify from io import BytesIO -from typing import Any, BinaryIO, Dict, List, Literal, Optional, Union +from typing import Any, BinaryIO, Dict, List, Optional, Union +# `Literal` appears with Python 3.8. +from typing_extensions import Literal from unittest.mock import Mock from urllib import parse From 5c3487d8198ba3c736dc6b7959d6b06cf7c3d46e Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 14 Jun 2022 15:42:39 +0200 Subject: [PATCH 09/24] WIP: Linting --- tests/rest/media/v1/test_media_storage.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index b815111d8e52..810fa0d564f1 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -17,8 +17,6 @@ from binascii import unhexlify from io import BytesIO from typing import Any, BinaryIO, Dict, List, Optional, Union -# `Literal` appears with Python 3.8. -from typing_extensions import Literal from unittest.mock import Mock from urllib import parse @@ -26,6 +24,9 @@ from parameterized import parameterized, parameterized_class from PIL import Image as Image +# `Literal` appears with Python 3.8. +from typing_extensions import Literal + from twisted.internet import defer from twisted.internet.defer import Deferred from twisted.test.proto_helpers import MemoryReactor From c3ba90005e6654d1199fa2178ce6191446e1bb09 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 16 Jun 2022 10:18:21 +0200 Subject: [PATCH 10/24] WIP: Missing import --- synapse/module_api/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 6191c2dc9633..6d8bf5408364 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.api import errors from synapse.api.errors import SynapseError from synapse.events import EventBase from synapse.events.presence_router import ( From 9792cd50a55ec61edbfb99c84b3bc0d29c4433aa Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 16 Jun 2022 10:24:25 +0200 Subject: [PATCH 11/24] WIP: Linting --- tests/rest/media/v1/test_media_storage.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 810fa0d564f1..3c10aa327da6 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -23,7 +23,6 @@ import attr from parameterized import parameterized, parameterized_class from PIL import Image as Image - # `Literal` appears with Python 3.8. from typing_extensions import Literal From 27788f73be52fe8354178dcedf5f0aff021e1ae3 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 16 Jun 2022 10:38:06 +0200 Subject: [PATCH 12/24] WIP: Wait a second, what happened to the ability for check_event_for_spam to return a Tuple[Codes, dict]? --- synapse/events/spamcheck.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 5df0669d8c18..265e4d76cc52 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -412,6 +412,13 @@ async def check_event_for_spam( # This spam-checker rejects the event with deprecated # return value `True` return (synapse.api.errors.Codes.FORBIDDEN, {}) + elif ( + isinstance(res, tuple) + and len(res) == 2 + and isinstance(res[0], synapse.api.errors.Codes) + and isinstance(res[1], dict) + ): + return res 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 From f06e8e219a37e68f14b83743ec67fe01a2c80ca9 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 16 Jun 2022 11:05:51 +0200 Subject: [PATCH 13/24] WIP: Linting --- tests/rest/media/v1/test_media_storage.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 3c10aa327da6..fd9cb606ee0c 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -23,7 +23,6 @@ import attr from parameterized import parameterized, parameterized_class from PIL import Image as Image -# `Literal` appears with Python 3.8. from typing_extensions import Literal from twisted.internet import defer From 05773190583ecaebe365568a3cd8e82dfaea5a72 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 16 Jun 2022 16:10:16 +0200 Subject: [PATCH 14/24] WIP: Applied feedback --- changelog.d/13044.misc | 2 +- synapse/events/spamcheck.py | 23 +++++++++++------------ tests/rest/client/test_rooms.py | 8 +++++--- tests/rest/media/v1/test_media_storage.py | 12 ++++++++---- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/changelog.d/13044.misc b/changelog.d/13044.misc index 07652a87c025..f9a0669dd35e 100644 --- a/changelog.d/13044.misc +++ b/changelog.d/13044.misc @@ -1 +1 @@ -Experimental: expand Spam-Checker API callbacks of `user_may_join_room`, `user_may_invite`, `user_may_send_3pid_invite`, `user_may_create_room`, `user_may_create_room_alias`, `user_may_publish_room`, `check_media_file_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. \ No newline at end of file +Support temporary experimental return values for spam checker module callbacks. \ No newline at end of file diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 265e4d76cc52..9d1f5ae1de54 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -21,7 +21,6 @@ Awaitable, Callable, Collection, - Dict, List, Optional, Tuple, @@ -35,7 +34,7 @@ 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.types import RoomAlias, UserProfile +from synapse.types import JsonDict, RoomAlias, UserProfile from synapse.util.async_helpers import delay_cancellation, maybe_awaitable from synapse.util.metrics import Measure @@ -55,7 +54,7 @@ # disappear without warning depending on the results of ongoing # experiments. # Use this to return additional information as part of an error. - Tuple["synapse.api.errors.Codes", Dict], + Tuple["synapse.api.errors.Codes", JsonDict], # Deprecated bool, ] @@ -75,7 +74,7 @@ # disappear without warning depending on the results of ongoing # experiments. # Use this to return additional information as part of an error. - Tuple["synapse.api.errors.Codes", Dict], + Tuple["synapse.api.errors.Codes", JsonDict], # Deprecated bool, ] @@ -91,7 +90,7 @@ # disappear without warning depending on the results of ongoing # experiments. # Use this to return additional information as part of an error. - Tuple["synapse.api.errors.Codes", Dict], + Tuple["synapse.api.errors.Codes", JsonDict], # Deprecated bool, ] @@ -107,7 +106,7 @@ # disappear without warning depending on the results of ongoing # experiments. # Use this to return additional information as part of an error. - Tuple["synapse.api.errors.Codes", Dict], + Tuple["synapse.api.errors.Codes", JsonDict], # Deprecated bool, ] @@ -123,7 +122,7 @@ # disappear without warning depending on the results of ongoing # experiments. # Use this to return additional information as part of an error. - Tuple["synapse.api.errors.Codes", Dict], + Tuple["synapse.api.errors.Codes", JsonDict], # Deprecated bool, ] @@ -139,7 +138,7 @@ # disappear without warning depending on the results of ongoing # experiments. # Use this to return additional information as part of an error. - Tuple["synapse.api.errors.Codes", Dict], + Tuple["synapse.api.errors.Codes", JsonDict], # Deprecated bool, ] @@ -155,7 +154,7 @@ # disappear without warning depending on the results of ongoing # experiments. # Use this to return additional information as part of an error. - Tuple["synapse.api.errors.Codes", Dict], + Tuple["synapse.api.errors.Codes", JsonDict], # Deprecated bool, ] @@ -189,7 +188,7 @@ # disappear without warning depending on the results of ongoing # experiments. # Use this to return additional information as part of an error. - Tuple["synapse.api.errors.Codes", Dict], + Tuple["synapse.api.errors.Codes", JsonDict], # Deprecated bool, ] @@ -380,7 +379,7 @@ def register_callbacks( async def check_event_for_spam( self, event: "synapse.events.EventBase" - ) -> Union[Tuple["synapse.api.errors.Codes", Dict], str]: + ) -> Union[Tuple["synapse.api.errors.Codes", JsonDict], 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 @@ -464,7 +463,7 @@ async def should_drop_federated_event( async def user_may_join_room( self, user_id: str, room_id: str, is_invited: bool - ) -> Union[Tuple["synapse.api.errors.Codes", Dict], Literal["NOT_SPAM"]]: + ) -> Union[Tuple["synapse.api.errors.Codes", JsonDict], Literal["NOT_SPAM"]]: """Checks if a given users is allowed to join a room. Not called when a user creates a room. diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 21e0c3b26a01..61f5c7fc615c 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -839,8 +839,7 @@ async def user_may_join_room_tuple( ) -> Tuple[Codes, dict]: return (Codes.CONSENT_NOT_GIVEN, {}) - join_mock = Mock(side_effect=user_may_join_room_codes) - self.hs.get_spam_checker()._user_may_join_room_callbacks = [join_mock] + join_mock.side_effect = user_may_join_room_tuple channel = self.make_request( "POST", @@ -3182,12 +3181,13 @@ def test_threepid_invite_spamcheck(self) -> None: access_token=self.tok, ) self.assertEqual(channel.code, 403) + self.assertEqual(channel.json_body.errcode, Codes.CONSENT_NOT_GIVEN) # Also check that it stopped before calling _make_and_store_3pid_invite. make_invite_mock.assert_called_once() # Run variant with `Tuple[Codes, dict]`. - mock.return_value = make_awaitable((Codes.CONSENT_NOT_GIVEN, {})) + mock.return_value = make_awaitable((Codes.EXPIRED_ACCOUNT, {"field": "value"})) channel = self.make_request( method="POST", path="/rooms/" + self.room_id + "/invite", @@ -3200,6 +3200,8 @@ def test_threepid_invite_spamcheck(self) -> None: access_token=self.tok, ) self.assertEqual(channel.code, 403) + self.assertEqual(channel.json_body.errcode, Codes.EXPIRED_ACCOUNT) + self.assertEqual(channel.json_body.field, "value") # Also check that it stopped before calling _make_and_store_3pid_invite. make_invite_mock.assert_called_once() diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index fd9cb606ee0c..aff00b370f0b 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -649,6 +649,10 @@ def test_upload_ban(self) -> None: ) +EVIL_DATA = b"Some evil data" +EVIL_DATA_EXPERIMENT = b"Some evil data to trigger the experimental tuple API" + + class SpamCheckerTestCase(unittest.HomeserverTestCase): servlets = [ login.register_servlets, @@ -674,9 +678,9 @@ async def check_media_file_for_spam( buf = BytesIO() await file_wrapper.write_chunks_to(buf.write) - if b"evil" in buf.getvalue(): + if buf.getvalue() == EVIL_DATA: return Codes.FORBIDDEN - elif b"experimental" in buf.getvalue(): + elif buf.getvalue() == EVIL_DATA_EXPERIMENT: return (Codes.FORBIDDEN, {}) else: return "NOT_SPAM" @@ -693,12 +697,12 @@ def test_upload_ban(self) -> None: """ self.helper.upload_media( - self.upload_resource, b"Some evil data", tok=self.tok, expect_code=400 + self.upload_resource, EVIL_DATA, tok=self.tok, expect_code=400 ) self.helper.upload_media( self.upload_resource, - b"Let's try the experimental API", + EVIL_DATA_EXPERIMENT, tok=self.tok, expect_code=400, ) From f6baf7e1e8391a752263fa8462917d9615d0385b Mon Sep 17 00:00:00 2001 From: David Teller Date: Fri, 17 Jun 2022 10:32:18 +0200 Subject: [PATCH 15/24] WIP: Applying feedback --- synapse/events/spamcheck.py | 93 +++++++++++++++++---------------- tests/rest/client/test_rooms.py | 19 +++++-- 2 files changed, 61 insertions(+), 51 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 9d1f5ae1de54..4ef56b55f22b 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -31,6 +31,7 @@ from typing_extensions import Literal import synapse +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 @@ -49,12 +50,12 @@ Awaitable[ Union[ str, - "synapse.api.errors.Codes", + 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["synapse.api.errors.Codes", JsonDict], + Tuple[Codes, JsonDict], # Deprecated bool, ] @@ -69,12 +70,12 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - "synapse.api.errors.Codes", + 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["synapse.api.errors.Codes", JsonDict], + Tuple[Codes, JsonDict], # Deprecated bool, ] @@ -85,12 +86,12 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - "synapse.api.errors.Codes", + 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["synapse.api.errors.Codes", JsonDict], + Tuple[Codes, JsonDict], # Deprecated bool, ] @@ -101,12 +102,12 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - "synapse.api.errors.Codes", + 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["synapse.api.errors.Codes", JsonDict], + Tuple[Codes, JsonDict], # Deprecated bool, ] @@ -117,12 +118,12 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - "synapse.api.errors.Codes", + 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["synapse.api.errors.Codes", JsonDict], + Tuple[Codes, JsonDict], # Deprecated bool, ] @@ -133,12 +134,12 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - "synapse.api.errors.Codes", + 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["synapse.api.errors.Codes", JsonDict], + Tuple[Codes, JsonDict], # Deprecated bool, ] @@ -149,12 +150,12 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - "synapse.api.errors.Codes", + 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["synapse.api.errors.Codes", JsonDict], + Tuple[Codes, JsonDict], # Deprecated bool, ] @@ -183,12 +184,12 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - "synapse.api.errors.Codes", + 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["synapse.api.errors.Codes", JsonDict], + Tuple[Codes, JsonDict], # Deprecated bool, ] @@ -379,7 +380,7 @@ def register_callbacks( async def check_event_for_spam( self, event: "synapse.events.EventBase" - ) -> Union[Tuple["synapse.api.errors.Codes", JsonDict], str]: + ) -> Union[Tuple[Codes, JsonDict], 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 @@ -410,7 +411,7 @@ async def check_event_for_spam( elif res is True: # This spam-checker rejects the event with deprecated # return value `True` - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} elif ( isinstance(res, tuple) and len(res) == 2 @@ -463,7 +464,7 @@ async def should_drop_federated_event( async def user_may_join_room( self, user_id: str, room_id: str, is_invited: bool - ) -> Union[Tuple["synapse.api.errors.Codes", JsonDict], Literal["NOT_SPAM"]]: + ) -> Union[Tuple[Codes, JsonDict], Literal["NOT_SPAM"]]: """Checks if a given users is allowed to join a room. Not called when a user creates a room. @@ -484,9 +485,9 @@ async def user_may_join_room( if res is True or res is self.NOT_SPAM: continue elif res is False: - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} elif isinstance(res, synapse.api.errors.Codes): - return (res, {}) + return res, {} elif ( isinstance(res, tuple) and len(res) == 2 @@ -498,14 +499,14 @@ async def user_may_join_room( logger.warning( "Module returned invalid value, rejecting join as spam" ) - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} # No spam-checker has rejected the request, let it pass. return self.NOT_SPAM async def user_may_invite( self, inviter_userid: str, invitee_userid: str, room_id: str - ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: + ) -> Union[Tuple[Codes, dict], Literal["NOT_SPAM"]]: """Checks if a given user may send an invite Args: @@ -527,9 +528,9 @@ async def user_may_invite( if res is True or res is self.NOT_SPAM: continue elif res is False: - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} elif isinstance(res, synapse.api.errors.Codes): - return (res, {}) + return res, {} elif ( isinstance(res, tuple) and len(res) == 2 @@ -541,14 +542,14 @@ async def user_may_invite( logger.warning( "Module returned invalid value, rejecting invite as spam" ) - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} # No spam-checker has rejected the request, let it pass. return self.NOT_SPAM async def user_may_send_3pid_invite( self, inviter_userid: str, medium: str, address: str, room_id: str - ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: + ) -> Union[Tuple[Codes, dict], Literal["NOT_SPAM"]]: """Checks if a given user may invite a given threepid into the room Note that if the threepid is already associated with a Matrix user ID, Synapse @@ -574,9 +575,9 @@ async def user_may_send_3pid_invite( if res is True or res is self.NOT_SPAM: continue elif res is False: - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} elif isinstance(res, synapse.api.errors.Codes): - return (res, {}) + return res, {} elif ( isinstance(res, tuple) and len(res) == 2 @@ -588,13 +589,13 @@ async def user_may_send_3pid_invite( logger.warning( "Module returned invalid value, rejecting 3pid invite as spam" ) - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} return self.NOT_SPAM async def user_may_create_room( self, userid: str - ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: + ) -> Union[Tuple[Codes, dict], Literal["NOT_SPAM"]]: """Checks if a given user may create a room Args: @@ -608,9 +609,9 @@ async def user_may_create_room( if res is True or res is self.NOT_SPAM: continue elif res is False: - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} elif isinstance(res, synapse.api.errors.Codes): - return (res, {}) + return res, {} elif ( isinstance(res, tuple) and len(res) == 2 @@ -622,13 +623,13 @@ async def user_may_create_room( logger.warning( "Module returned invalid value, rejecting room creation as spam" ) - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} return self.NOT_SPAM async def user_may_create_room_alias( self, userid: str, room_alias: RoomAlias - ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: + ) -> Union[Tuple[Codes, dict], Literal["NOT_SPAM"]]: """Checks if a given user may create a room alias Args: @@ -644,9 +645,9 @@ async def user_may_create_room_alias( if res is True or res is self.NOT_SPAM: continue elif res is False: - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} elif isinstance(res, synapse.api.errors.Codes): - return (res, {}) + return res, {} elif ( isinstance(res, tuple) and len(res) == 2 @@ -658,13 +659,13 @@ async def user_may_create_room_alias( logger.warning( "Module returned invalid value, rejecting room create as spam" ) - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} return self.NOT_SPAM async def user_may_publish_room( self, userid: str, room_id: str - ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: + ) -> Union[Tuple[Codes, dict], Literal["NOT_SPAM"]]: """Checks if a given user may publish a room to the directory Args: @@ -679,9 +680,9 @@ async def user_may_publish_room( if res is True or res is self.NOT_SPAM: continue elif res is False: - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} elif isinstance(res, synapse.api.errors.Codes): - return (res, {}) + return res, {} elif ( isinstance(res, tuple) and len(res) == 2 @@ -693,7 +694,7 @@ async def user_may_publish_room( logger.warning( "Module returned invalid value, rejecting room publication as spam" ) - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} return self.NOT_SPAM @@ -761,7 +762,7 @@ async def check_registration_for_spam( async def check_media_file_for_spam( self, file_wrapper: ReadableFileWrapper, file_info: FileInfo - ) -> Union[Tuple["synapse.api.errors.Codes", dict], Literal["NOT_SPAM"]]: + ) -> Union[Tuple[Codes, dict], Literal["NOT_SPAM"]]: """Checks if a piece of newly uploaded media should be blocked. This will be called for local uploads, downloads of remote media, each @@ -798,9 +799,9 @@ async def check_media_file_for_spam( if res is False or res is self.NOT_SPAM: continue elif res is True: - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} elif isinstance(res, synapse.api.errors.Codes): - return (res, {}) + return res, {} elif ( isinstance(res, tuple) and len(res) == 2 @@ -812,6 +813,6 @@ async def check_media_file_for_spam( logger.warning( "Module returned invalid value, rejecting media file as spam" ) - return (synapse.api.errors.Codes.FORBIDDEN, {}) + return synapse.api.errors.Codes.FORBIDDEN, {} return self.NOT_SPAM diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 61f5c7fc615c..7ae8671f5134 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -832,12 +832,15 @@ async def user_may_join_room_codes( self.assertEqual(join_mock.call_count, 0) + # Now change the return value of the callback to deny any joinn and test that + # we can't join. We pick an arbitrary error code to be able to check + # that the same code has been returned async def user_may_join_room_tuple( mxid: str, room_id: str, is_invite: bool, ) -> Tuple[Codes, dict]: - return (Codes.CONSENT_NOT_GIVEN, {}) + return (Codes.INCOMPATIBLE_ROOM_VERSION, {}) join_mock.side_effect = user_may_join_room_tuple @@ -847,6 +850,11 @@ async def user_may_join_room_tuple( {}, ) self.assertEqual(channel.code, 200, channel.json_body) + self.assertEqual( + channel.json_body["errcode"], + Codes.INCOMPATIBLE_ROOM_VERSION, + channel.json_body, + ) self.assertEqual(join_mock.call_count, 0) @@ -3167,7 +3175,8 @@ def test_threepid_invite_spamcheck(self) -> None: make_invite_mock.assert_called_once() # Now change the return value of the callback to deny any invite and test that - # we can't send the invite. + # we can't send the invite. We pick an arbitrary error code to be able to check + # that the same code has been returned mock.return_value = make_awaitable(Codes.CONSENT_NOT_GIVEN) channel = self.make_request( method="POST", @@ -3181,7 +3190,7 @@ def test_threepid_invite_spamcheck(self) -> None: access_token=self.tok, ) self.assertEqual(channel.code, 403) - self.assertEqual(channel.json_body.errcode, Codes.CONSENT_NOT_GIVEN) + self.assertEqual(channel.json_body["errcode"], Codes.CONSENT_NOT_GIVEN) # Also check that it stopped before calling _make_and_store_3pid_invite. make_invite_mock.assert_called_once() @@ -3200,8 +3209,8 @@ def test_threepid_invite_spamcheck(self) -> None: access_token=self.tok, ) self.assertEqual(channel.code, 403) - self.assertEqual(channel.json_body.errcode, Codes.EXPIRED_ACCOUNT) - self.assertEqual(channel.json_body.field, "value") + self.assertEqual(channel.json_body["errcode"], Codes.EXPIRED_ACCOUNT) + self.assertEqual(channel.json_body["field"], "value") # Also check that it stopped before calling _make_and_store_3pid_invite. make_invite_mock.assert_called_once() From 2578f7435743b309d1290d9a9614669c9db914ca Mon Sep 17 00:00:00 2001 From: David Teller Date: Fri, 17 Jun 2022 10:49:25 +0200 Subject: [PATCH 16/24] WIP: Moar tests --- tests/rest/client/test_rooms.py | 38 +++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 7ae8671f5134..39bd377bad84 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -1338,6 +1338,44 @@ def test_rooms_messages_sent(self) -> None: channel = self.make_request("PUT", path, content) self.assertEqual(200, channel.code, msg=channel.result["body"]) + def test_spam_checker_check_event_for_spam(self) -> None: + mock_return_value: Union[str, Codes, Tuple[Codes, JsonDict], bool] = "NOT_SPAM" + mock_event: Optional[synapse.events.EventBase] = None + async def check_event_for_spam( + event: synapse.events.EventBase + ) -> Union[str, Codes, Tuple[Codes, JsonDict], bool]: + mock_event = event + return mock_return_value + # `spec` argument is needed for this function mock to have `__qualname__`, which + # is needed for `Measure` metrics buried in SpamChecker. + callback_mock = Mock(side_effect=check_event_for_spam, spec=lambda *x: None) + self.hs.get_spam_checker()._check_event_for_spam_callbacks.append(callback_mock) + + for i, (value, expected_code, expected_dict) in enumerate([ + # Allow + ("NOT_SPAM", 200, {}), + (False, 200, {}), + # Block + ("ANY OTHER STRING", 400, {"errcode": "M_FORBIDDEN"}), + (True, 400, {"errcode": "M_FORBIDDEN"}), + (Codes.LIMIT_EXCEEDED, 400, {"errcode": "M_LIMIT_EXCEEDED"}), + ((Codes.SERVER_NOT_TRUSTED, {"additional_field": "12345"}), 400, {"errcode": "M_SERVER_NOT_TRUSTED", "additional_field": "12345"}) + ]): + # Inject `value` as mock_return_value + mock_return_value = value + path = "/rooms/%s/send/m.room.message/check_event_for_spam_%s" % (urlparse.quote(self.room_id), i) + body = b"test-%s" % i + content = b'{"body":%s,"msgtype":"m.text"}' % body + channel = self.make_request("PUT", path, content) + + # Check that the callback has witnessed the correct event. + self.assertEqual(mock_event.get_dict()["body"], body, mock_event.get_dict()) + + # Check that we have the correct result. + self.assertEqual(expected_code, channel.code, msg=channel.result["body"]) + for expected_field, expected_value in expected_dict.items(): + self.assertEqual(channel.json_body.get(expected_field, None), expected_value, "Field %s absent or invalid " % expected_field) + class RoomPowerLevelOverridesTestCase(RoomBase): """Tests that the power levels can be overridden with server config.""" From f99a7e6f49cd70ae1839b5ac62b571f6d2043b47 Mon Sep 17 00:00:00 2001 From: David Teller Date: Fri, 17 Jun 2022 11:01:53 +0200 Subject: [PATCH 17/24] WIP: Moar tests --- tests/rest/client/test_rooms.py | 37 ++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 39bd377bad84..25d11f648aae 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -1341,17 +1341,22 @@ def test_rooms_messages_sent(self) -> None: def test_spam_checker_check_event_for_spam(self) -> None: mock_return_value: Union[str, Codes, Tuple[Codes, JsonDict], bool] = "NOT_SPAM" mock_event: Optional[synapse.events.EventBase] = None + async def check_event_for_spam( - event: synapse.events.EventBase + event: synapse.events.EventBase, ) -> Union[str, Codes, Tuple[Codes, JsonDict], bool]: mock_event = event + _ = mock_event # We get a spurious warning that mock_event is unused return mock_return_value + # `spec` argument is needed for this function mock to have `__qualname__`, which # is needed for `Measure` metrics buried in SpamChecker. callback_mock = Mock(side_effect=check_event_for_spam, spec=lambda *x: None) self.hs.get_spam_checker()._check_event_for_spam_callbacks.append(callback_mock) - for i, (value, expected_code, expected_dict) in enumerate([ + SAMPLES: List[ + Tuple[Union[str, Codes, Tuple[Codes, JsonDict], bool], int, dict] + ] = [ # Allow ("NOT_SPAM", 200, {}), (False, 200, {}), @@ -1359,22 +1364,40 @@ async def check_event_for_spam( ("ANY OTHER STRING", 400, {"errcode": "M_FORBIDDEN"}), (True, 400, {"errcode": "M_FORBIDDEN"}), (Codes.LIMIT_EXCEEDED, 400, {"errcode": "M_LIMIT_EXCEEDED"}), - ((Codes.SERVER_NOT_TRUSTED, {"additional_field": "12345"}), 400, {"errcode": "M_SERVER_NOT_TRUSTED", "additional_field": "12345"}) - ]): + ( + (Codes.SERVER_NOT_TRUSTED, {"additional_field": "12345"}), + 400, + {"errcode": "M_SERVER_NOT_TRUSTED", "additional_field": "12345"}, + ), + ] + for i, (value, expected_code, expected_dict) in enumerate(SAMPLES): # Inject `value` as mock_return_value mock_return_value = value - path = "/rooms/%s/send/m.room.message/check_event_for_spam_%s" % (urlparse.quote(self.room_id), i) + path = "/rooms/%s/send/m.room.message/check_event_for_spam_%s" % ( + urlparse.quote(self.room_id), + i, + ) body = b"test-%s" % i content = b'{"body":%s,"msgtype":"m.text"}' % body channel = self.make_request("PUT", path, content) # Check that the callback has witnessed the correct event. - self.assertEqual(mock_event.get_dict()["body"], body, mock_event.get_dict()) + self.assertIsNotNone(mock_event) + if ( + mock_event is not None + ): # Checked just above, but mypy doesn't know about that. + self.assertEqual( + mock_event.get_dict()["body"], body, mock_event.get_dict() + ) # Check that we have the correct result. self.assertEqual(expected_code, channel.code, msg=channel.result["body"]) for expected_field, expected_value in expected_dict.items(): - self.assertEqual(channel.json_body.get(expected_field, None), expected_value, "Field %s absent or invalid " % expected_field) + self.assertEqual( + channel.json_body.get(expected_field, None), + expected_value, + "Field %s absent or invalid " % expected_field, + ) class RoomPowerLevelOverridesTestCase(RoomBase): From 384e5a7672951ce9b9f7ac6854873eb46f68cc2e Mon Sep 17 00:00:00 2001 From: David Teller Date: Fri, 17 Jun 2022 11:07:26 +0200 Subject: [PATCH 18/24] WIP: Linting --- tests/rest/client/test_rooms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 25d11f648aae..47fd713172fc 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -1346,7 +1346,7 @@ async def check_event_for_spam( event: synapse.events.EventBase, ) -> Union[str, Codes, Tuple[Codes, JsonDict], bool]: mock_event = event - _ = mock_event # We get a spurious warning that mock_event is unused + _ = mock_event # We get a spurious warning that mock_event is unused return mock_return_value # `spec` argument is needed for this function mock to have `__qualname__`, which From fb10a60dc2e32ce760c7bcb0f7ba2ef7dd046ccb Mon Sep 17 00:00:00 2001 From: David Teller Date: Fri, 17 Jun 2022 14:43:33 +0200 Subject: [PATCH 19/24] WIP: Now with tests that actually pass --- synapse/events/spamcheck.py | 2 ++ tests/rest/client/test_rooms.py | 48 ++++++++++++++++++--------------- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 4ef56b55f22b..4a3bfb38f1dc 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -419,6 +419,8 @@ async def check_event_for_spam( and isinstance(res[1], dict) ): return res + elif isinstance(res, synapse.api.errors.Codes): + return res, {} 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 diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 47fd713172fc..ff05001f090b 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -1339,20 +1339,24 @@ def test_rooms_messages_sent(self) -> None: self.assertEqual(200, channel.code, msg=channel.result["body"]) def test_spam_checker_check_event_for_spam(self) -> None: - mock_return_value: Union[str, Codes, Tuple[Codes, JsonDict], bool] = "NOT_SPAM" - mock_event: Optional[synapse.events.EventBase] = None + class SpamCheck: + mock_return_value: Union[ + str, Codes, Tuple[Codes, JsonDict], bool + ] = "NOT_SPAM" + mock_content: Optional[JsonDict] = None - async def check_event_for_spam( - event: synapse.events.EventBase, - ) -> Union[str, Codes, Tuple[Codes, JsonDict], bool]: - mock_event = event - _ = mock_event # We get a spurious warning that mock_event is unused - return mock_return_value + async def check_event_for_spam( + self, + event: synapse.events.EventBase, + ) -> Union[str, Codes, Tuple[Codes, JsonDict], bool]: + self.mock_content = event.content + return self.mock_return_value - # `spec` argument is needed for this function mock to have `__qualname__`, which - # is needed for `Measure` metrics buried in SpamChecker. - callback_mock = Mock(side_effect=check_event_for_spam, spec=lambda *x: None) - self.hs.get_spam_checker()._check_event_for_spam_callbacks.append(callback_mock) + spam_checker = SpamCheck() + + self.hs.get_spam_checker()._check_event_for_spam_callbacks.append( + spam_checker.check_event_for_spam + ) SAMPLES: List[ Tuple[Union[str, Codes, Tuple[Codes, JsonDict], bool], int, dict] @@ -1361,33 +1365,33 @@ async def check_event_for_spam( ("NOT_SPAM", 200, {}), (False, 200, {}), # Block - ("ANY OTHER STRING", 400, {"errcode": "M_FORBIDDEN"}), - (True, 400, {"errcode": "M_FORBIDDEN"}), - (Codes.LIMIT_EXCEEDED, 400, {"errcode": "M_LIMIT_EXCEEDED"}), + ("ANY OTHER STRING", 403, {"errcode": "M_FORBIDDEN"}), + (True, 403, {"errcode": "M_FORBIDDEN"}), + (Codes.LIMIT_EXCEEDED, 403, {"errcode": "M_LIMIT_EXCEEDED"}), ( (Codes.SERVER_NOT_TRUSTED, {"additional_field": "12345"}), - 400, + 403, {"errcode": "M_SERVER_NOT_TRUSTED", "additional_field": "12345"}, ), ] for i, (value, expected_code, expected_dict) in enumerate(SAMPLES): # Inject `value` as mock_return_value - mock_return_value = value + spam_checker.mock_return_value = value path = "/rooms/%s/send/m.room.message/check_event_for_spam_%s" % ( urlparse.quote(self.room_id), i, ) - body = b"test-%s" % i - content = b'{"body":%s,"msgtype":"m.text"}' % body + body = "test-%s" % i + content = '{"body":"%s","msgtype":"m.text"}' % body channel = self.make_request("PUT", path, content) # Check that the callback has witnessed the correct event. - self.assertIsNotNone(mock_event) + self.assertIsNotNone(spam_checker.mock_content) if ( - mock_event is not None + spam_checker.mock_content is not None ): # Checked just above, but mypy doesn't know about that. self.assertEqual( - mock_event.get_dict()["body"], body, mock_event.get_dict() + spam_checker.mock_content["body"], body, spam_checker.mock_content ) # Check that we have the correct result. From b44033e0200acc61301dee39fe455e70e1f6c954 Mon Sep 17 00:00:00 2001 From: David Teller Date: Fri, 17 Jun 2022 16:33:08 +0200 Subject: [PATCH 20/24] WIP: More tests --- tests/rest/client/test_rooms.py | 34 ++++++++++++++++++++++++--------- tests/rest/client/utils.py | 21 ++++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index ff05001f090b..9c67b76be5bf 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -850,12 +850,6 @@ async def user_may_join_room_tuple( {}, ) self.assertEqual(channel.code, 200, channel.json_body) - self.assertEqual( - channel.json_body["errcode"], - Codes.INCOMPATIBLE_ROOM_VERSION, - channel.json_body, - ) - self.assertEqual(join_mock.call_count, 0) @@ -1137,13 +1131,15 @@ def test_spam_checker_may_join_room(self) -> None: """ # Register a dummy callback. Make it allow all room joins for now. - return_value: Union[Literal["NOT_SPAM"], Codes] = synapse.module_api.NOT_SPAM + return_value: Union[ + Literal["NOT_SPAM"], Tuple[Codes, dict], Codes + ] = synapse.module_api.NOT_SPAM async def user_may_join_room( userid: str, room_id: str, is_invited: bool, - ) -> Union[Literal["NOT_SPAM"], Codes]: + ) -> Union[Literal["NOT_SPAM"], Tuple[Codes, dict], Codes]: return return_value # `spec` argument is needed for this function mock to have `__qualname__`, which @@ -1187,8 +1183,28 @@ async def user_may_join_room( ) # Now make the callback deny all room joins, and check that a join actually fails. + # We pick an arbitrary Codes rather than the default `Codes.FORBIDDEN`. return_value = Codes.CONSENT_NOT_GIVEN - self.helper.join(self.room3, self.user2, expect_code=403, tok=self.tok2) + self.helper.invite(self.room3, self.user1, self.user2, tok=self.tok1) + self.helper.join( + self.room3, + self.user2, + expect_code=403, + expect_errcode=return_value, + tok=self.tok2, + ) + + # Now make the callback deny all room joins, and check that a join actually fails. + # As above, with the experimental extension that lets us return dictionaries. + return_value = Codes.BAD_ALIAS, {"another_field": "12345"} + self.helper.join( + self.room3, + self.user2, + expect_code=403, + expect_errcode=return_value[0], + tok=self.tok2, + expect_additional_fields=return_value[1], + ) class RoomJoinRatelimitTestCase(RoomBase): diff --git a/tests/rest/client/utils.py b/tests/rest/client/utils.py index a0788b1bb0ad..93f749744d0a 100644 --- a/tests/rest/client/utils.py +++ b/tests/rest/client/utils.py @@ -41,6 +41,7 @@ from twisted.web.server import Site from synapse.api.constants import Membership +from synapse.api.errors import Codes from synapse.server import HomeServer from synapse.types import JsonDict @@ -171,6 +172,8 @@ def join( expect_code: int = HTTPStatus.OK, tok: Optional[str] = None, appservice_user_id: Optional[str] = None, + expect_errcode: Optional[Codes] = None, + expect_additional_fields: Optional[dict] = None, ) -> None: self.change_membership( room=room, @@ -180,6 +183,8 @@ def join( appservice_user_id=appservice_user_id, membership=Membership.JOIN, expect_code=expect_code, + expect_errcode=expect_errcode, + expect_additional_fields=expect_additional_fields, ) def knock( @@ -263,6 +268,7 @@ def change_membership( appservice_user_id: Optional[str] = None, expect_code: int = HTTPStatus.OK, expect_errcode: Optional[str] = None, + expect_additional_fields: Optional[dict] = None, ) -> None: """ Send a membership state event into a room. @@ -323,6 +329,21 @@ def change_membership( channel.result["body"], ) + if expect_additional_fields is not None: + for expect_key, expect_value in expect_additional_fields.items(): + assert expect_key in channel.json_body, "Expected field %s, got %s" % ( + expect_key, + channel.json_body, + ) + assert ( + channel.json_body[expect_key] == expect_value + ), "Expected: %s at %s, got: %s, resp: %s" % ( + expect_value, + expect_key, + channel.json_body[expect_key], + channel.json_body, + ) + self.auth_user_id = temp_id def send( From 3687890ba3fcfcf5e7a1c21d48743d43040ef629 Mon Sep 17 00:00:00 2001 From: David Teller Date: Fri, 1 Jul 2022 10:26:14 +0200 Subject: [PATCH 21/24] WIP: Applied feedback --- tests/rest/client/test_rooms.py | 4 ++-- tests/rest/media/v1/test_media_storage.py | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 9c67b76be5bf..25615c32c020 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -840,7 +840,7 @@ async def user_may_join_room_tuple( room_id: str, is_invite: bool, ) -> Tuple[Codes, dict]: - return (Codes.INCOMPATIBLE_ROOM_VERSION, {}) + return Codes.INCOMPATIBLE_ROOM_VERSION, {} join_mock.side_effect = user_may_join_room_tuple @@ -1196,7 +1196,7 @@ async def user_may_join_room( # Now make the callback deny all room joins, and check that a join actually fails. # As above, with the experimental extension that lets us return dictionaries. - return_value = Codes.BAD_ALIAS, {"another_field": "12345"} + return_value = (Codes.BAD_ALIAS, {"another_field": "12345"}) self.helper.join( self.room3, self.user2, diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index aff00b370f0b..49744d5d1408 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -552,11 +552,11 @@ def test_x_robots_tag_header(self) -> None: ) -class TestSpamCheckerDeprecated: +class TestSpamCheckerLegacy: """A spam checker module that rejects all media that includes the bytes `evil`. - Uses the deprecated Spam-Checker API. + Uses the legacy Spam-Checker API. """ def __init__(self, config: Dict[str, Any], api: ModuleApi) -> None: @@ -597,7 +597,7 @@ async def check_media_file_for_spam( return b"evil" in buf.getvalue() -class SpamCheckerTestCaseDeprecated(unittest.HomeserverTestCase): +class SpamCheckerTestCaseLegacy(unittest.HomeserverTestCase): servlets = [ login.register_servlets, admin.register_servlets, @@ -621,8 +621,8 @@ def default_config(self) -> Dict[str, Any]: { "spam_checker": [ { - "module": TestSpamCheckerDeprecated.__module__ - + ".TestSpamCheckerDeprecated", + "module": TestSpamCheckerLegacy.__module__ + + ".TestSpamCheckerLegacy", "config": {}, } ] From 0302d362f6f3bd657d33bad19dcbf06ca88b0ffe Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 4 Jul 2022 08:28:46 +0200 Subject: [PATCH 22/24] WIP --- tests/rest/client/test_rooms.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 25615c32c020..0fca2d57027c 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -832,9 +832,8 @@ async def user_may_join_room_codes( self.assertEqual(join_mock.call_count, 0) - # Now change the return value of the callback to deny any joinn and test that - # we can't join. We pick an arbitrary error code to be able to check - # that the same code has been returned + # Now change the return value of the callback to deny any join. Since we're + # creating the room, despite the return value, we should be able to join. async def user_may_join_room_tuple( mxid: str, room_id: str, From 921b2a9698f8ec45f784d01dfd2939cec8c20315 Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 5 Jul 2022 15:22:11 +0200 Subject: [PATCH 23/24] WIP: Using parameterized for test --- tests/rest/client/test_rooms.py | 118 ++++++++++++++++++++------------ 1 file changed, 73 insertions(+), 45 deletions(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 0fca2d57027c..05a8ac6d0f94 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -22,6 +22,8 @@ from unittest.mock import Mock, call from urllib import parse as urlparse +from parameterized import param, parameterized + # `Literal` appears with Python 3.8. from typing_extensions import Literal @@ -1353,10 +1355,53 @@ def test_rooms_messages_sent(self) -> None: channel = self.make_request("PUT", path, content) self.assertEqual(200, channel.code, msg=channel.result["body"]) - def test_spam_checker_check_event_for_spam(self) -> None: + @parameterized.expand( + [ + # Allow + param( + name="NOT_SPAM", value="NOT_SPAM", expected_code=200, expected_fields={} + ), + param(name="False", value=False, expected_code=200, expected_fields={}), + # Block + param( + name="scalene string", + value="ANY OTHER STRING", + expected_code=403, + expected_fields={"errcode": "M_FORBIDDEN"}, + ), + param( + name="True", + value=True, + expected_code=403, + expected_fields={"errcode": "M_FORBIDDEN"}, + ), + param( + name="Code", + value=Codes.LIMIT_EXCEEDED, + expected_code=403, + expected_fields={"errcode": "M_LIMIT_EXCEEDED"}, + ), + param( + name="Tuple", + value=(Codes.SERVER_NOT_TRUSTED, {"additional_field": "12345"}), + expected_code=403, + expected_fields={ + "errcode": "M_SERVER_NOT_TRUSTED", + "additional_field": "12345", + }, + ), + ] + ) + def test_spam_checker_check_event_for_spam( + self, + name: str, + value: Union[str, bool, Codes, Tuple[Codes, JsonDict]], + expected_code: int, + expected_fields: dict, + ) -> None: class SpamCheck: mock_return_value: Union[ - str, Codes, Tuple[Codes, JsonDict], bool + str, bool, Codes, Tuple[Codes, JsonDict], bool ] = "NOT_SPAM" mock_content: Optional[JsonDict] = None @@ -1373,50 +1418,33 @@ async def check_event_for_spam( spam_checker.check_event_for_spam ) - SAMPLES: List[ - Tuple[Union[str, Codes, Tuple[Codes, JsonDict], bool], int, dict] - ] = [ - # Allow - ("NOT_SPAM", 200, {}), - (False, 200, {}), - # Block - ("ANY OTHER STRING", 403, {"errcode": "M_FORBIDDEN"}), - (True, 403, {"errcode": "M_FORBIDDEN"}), - (Codes.LIMIT_EXCEEDED, 403, {"errcode": "M_LIMIT_EXCEEDED"}), - ( - (Codes.SERVER_NOT_TRUSTED, {"additional_field": "12345"}), - 403, - {"errcode": "M_SERVER_NOT_TRUSTED", "additional_field": "12345"}, - ), - ] - for i, (value, expected_code, expected_dict) in enumerate(SAMPLES): - # Inject `value` as mock_return_value - spam_checker.mock_return_value = value - path = "/rooms/%s/send/m.room.message/check_event_for_spam_%s" % ( - urlparse.quote(self.room_id), - i, + # Inject `value` as mock_return_value + spam_checker.mock_return_value = value + path = "/rooms/%s/send/m.room.message/check_event_for_spam_%s" % ( + urlparse.quote(self.room_id), + urlparse.quote(name), + ) + body = "test-%s" % name + content = '{"body":"%s","msgtype":"m.text"}' % body + channel = self.make_request("PUT", path, content) + + # Check that the callback has witnessed the correct event. + self.assertIsNotNone(spam_checker.mock_content) + if ( + spam_checker.mock_content is not None + ): # Checked just above, but mypy doesn't know about that. + self.assertEqual( + spam_checker.mock_content["body"], body, spam_checker.mock_content + ) + + # Check that we have the correct result. + self.assertEqual(expected_code, channel.code, msg=channel.result["body"]) + for expected_key, expected_value in expected_fields.items(): + self.assertEqual( + channel.json_body.get(expected_key, None), + expected_value, + "Field %s absent or invalid " % expected_key, ) - body = "test-%s" % i - content = '{"body":"%s","msgtype":"m.text"}' % body - channel = self.make_request("PUT", path, content) - - # Check that the callback has witnessed the correct event. - self.assertIsNotNone(spam_checker.mock_content) - if ( - spam_checker.mock_content is not None - ): # Checked just above, but mypy doesn't know about that. - self.assertEqual( - spam_checker.mock_content["body"], body, spam_checker.mock_content - ) - - # Check that we have the correct result. - self.assertEqual(expected_code, channel.code, msg=channel.result["body"]) - for expected_field, expected_value in expected_dict.items(): - self.assertEqual( - channel.json_body.get(expected_field, None), - expected_value, - "Field %s absent or invalid " % expected_field, - ) class RoomPowerLevelOverridesTestCase(RoomBase): From 13010551f67340419f29b4a1074e8b0f59dc7af9 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 7 Jul 2022 07:43:46 +0200 Subject: [PATCH 24/24] WIP: Lint --- tests/rest/client/test_rooms.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 05a8ac6d0f94..587e61ee8ec6 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -23,8 +23,6 @@ from urllib import parse as urlparse from parameterized import param, parameterized - -# `Literal` appears with Python 3.8. from typing_extensions import Literal from twisted.test.proto_helpers import MemoryReactor