diff --git a/changelog.d/12857.feature b/changelog.d/12857.feature new file mode 100644 index 000000000000..ef631cdc3ac0 --- /dev/null +++ b/changelog.d/12857.feature @@ -0,0 +1 @@ +Port spam-checker API callbacks `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` to more powerful and less ambiguous `Union[Allow, Codes]`. diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 71f6f9f0ab45..5fa9e9c1f84c 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -11,7 +11,7 @@ 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_ +_Signature extended to support Allow and Codes in Synapse v1.60.0_ _Boolean and string return value types deprecated in Synapse v1.60.0_ ```python @@ -38,14 +38,22 @@ will not call any of the subsequent implementations of this callback. ### `user_may_join_room` _First introduced in Synapse v1.37.0_ +_Signature extended to support Allow and Codes in Synapse v1.60.0_ +_Boolean return value type deprecated in Synapse v1.60.0_ ```python -async def user_may_join_room(user: str, room: str, is_invited: bool) -> bool +async def user_may_join_room(user: str, room: str, is_invited: bool) -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", bool] ``` -Called when a user is trying to join a room. The module must return a `bool` to indicate -whether the user can join the room. Return `False` to prevent the user from joining the -room; otherwise return `True` to permit the joining. +Called when a user is trying to join a room. 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. + - (deprecated) on `True`, behave as `ALLOW`. Deprecated as confusing, as some + callbacks in expect `True` to allow and others `True` to reject. + - (deprecated) on `False`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as + some callbacks in expect `True` to allow and others `True` to reject. The user is represented by their Matrix user ID (e.g. `@alice:example.com`) and the room is represented by its Matrix ID (e.g. @@ -55,32 +63,32 @@ currently has a pending invite in the room. This callback isn't called if the join is performed by a server administrator, or in the context of a room creation. -If multiple modules implement this callback, they will be considered in order. If a -callback returns `True`, Synapse falls through to the next one. The value of the first -callback that does not return `True` will be used. If this happens, Synapse will not call -any of the subsequent implementations of this callback. - ### `user_may_invite` _First introduced in Synapse v1.37.0_ +_Signature extended to support Allow and Codes in Synapse v1.60.0_ +_Boolean return value type deprecated in Synapse v1.60.0_ ```python -async def user_may_invite(inviter: str, invitee: str, room_id: str) -> bool +async def user_may_invite(inviter: str, invitee: str, room_id: str) -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", bool] ``` -Called when processing an invitation. The module must return a `bool` indicating whether -the inviter can invite the invitee to the given room. Both inviter and invitee are -represented by their Matrix user ID (e.g. `@alice:example.com`). Return `False` to prevent -the invitation; otherwise return `True` to permit it. +Called when processing an invitation. Called when a user is trying to join a room. 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. + - (deprecated) on `True`, behave as `ALLOW`. Deprecated as confusing, as some + callbacks in expect `True` to allow and others `True` to reject. + - (deprecated) on `False`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as + some callbacks in expect `True` to allow and others `True` to reject. -If multiple modules implement this callback, they will be considered in order. If a -callback returns `True`, Synapse falls through to the next one. The value of the first -callback that does not return `True` will be used. If this happens, Synapse will not call -any of the subsequent implementations of this callback. ### `user_may_send_3pid_invite` _First introduced in Synapse v1.45.0_ +_Signature extended to support Allow and Codes in Synapse v1.60.0_ +_Boolean return value type deprecated in Synapse v1.60.0_ ```python async def user_may_send_3pid_invite( @@ -88,13 +96,19 @@ async def user_may_send_3pid_invite( medium: str, address: str, room_id: str, -) -> bool +) -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", bool] ``` -Called when processing an invitation using a third-party identifier (also called a 3PID, -e.g. an email address or a phone number). The module must return a `bool` indicating -whether the inviter can invite the invitee to the given room. Return `False` to prevent -the invitation; otherwise return `True` to permit it. +Called when processing an invitation using a third-party identifier (also called a 3PID, +e.g. an email address or a phone number). 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. + - (deprecated) on `True`, behave as `ALLOW`. Deprecated as confusing, as some + callbacks in expect `True` to allow and others `True` to reject. + - (deprecated) on `False`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as + some callbacks in expect `True` to allow and others `True` to reject. The inviter is represented by their Matrix user ID (e.g. `@alice:example.com`), and the invitee is represented by its medium (e.g. "email") and its address @@ -124,55 +138,62 @@ any of the subsequent implementations of this callback. ### `user_may_create_room` _First introduced in Synapse v1.37.0_ +_Signature extended to support Allow and Codes in Synapse v1.60.0_ +_Boolean return value type deprecated in Synapse v1.60.0_ ```python -async def user_may_create_room(user: str) -> bool +async def user_may_create_room(user: str) -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", bool] ``` -Called when processing a room creation request. The module must return a `bool` indicating -whether the given user (represented by their Matrix user ID) is allowed to create a room. -Return `False` to prevent room creation; otherwise return `True` to permit it. - -If multiple modules implement this callback, they will be considered in order. If a -callback returns `True`, Synapse falls through to the next one. The value of the first -callback that does not return `True` will be used. If this happens, Synapse will not call -any of the subsequent implementations of this callback. +Called when processing a room creation request. 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. + - (deprecated) on `True`, behave as `ALLOW`. Deprecated as confusing, as some + callbacks in expect `True` to allow and others `True` to reject. + - (deprecated) on `False`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as + some callbacks in expect `True` to allow and others `True` to reject. ### `user_may_create_room_alias` _First introduced in Synapse v1.37.0_ +_Signature extended to support Allow and Codes in Synapse v1.60.0_ +_Boolean return value type deprecated in Synapse v1.60.0_ ```python async def user_may_create_room_alias(user: str, room_alias: "synapse.types.RoomAlias") -> bool ``` -Called when trying to associate an alias with an existing room. The module must return a -`bool` indicating whether the given user (represented by their Matrix user ID) is allowed -to set the given alias. Return `False` to prevent the alias creation; otherwise return -`True` to permit it. - -If multiple modules implement this callback, they will be considered in order. If a -callback returns `True`, Synapse falls through to the next one. The value of the first -callback that does not return `True` will be used. If this happens, Synapse will not call -any of the subsequent implementations of this callback. +Called when trying to associate an alias with an existing room. 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. + - (deprecated) on `True`, behave as `ALLOW`. Deprecated as confusing, as some + callbacks in expect `True` to allow and others `True` to reject. + - (deprecated) on `False`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as + some callbacks in expect `True` to allow and others `True` to reject. ### `user_may_publish_room` _First introduced in Synapse v1.37.0_ +_Signature extended to support Allow and Codes in Synapse v1.60.0_ +_Boolean return value type deprecated in Synapse v1.60.0_ ```python -async def user_may_publish_room(user: str, room_id: str) -> bool +async def user_may_publish_room(user: str, room_id: str) -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", bool] ``` -Called when trying to publish a room to the homeserver's public rooms directory. The -module must return a `bool` indicating whether the given user (represented by their -Matrix user ID) is allowed to publish the given room. Return `False` to prevent the -room from being published; otherwise return `True` to permit its publication. - -If multiple modules implement this callback, they will be considered in order. If a -callback returns `True`, Synapse falls through to the next one. The value of the first -callback that does not return `True` will be used. If this happens, Synapse will not call -any of the subsequent implementations of this callback. +Called when trying to publish a room to the homeserver's public rooms directory. 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. + - (deprecated) on `True`, behave as `ALLOW`. Deprecated as confusing, as some + callbacks in expect `True` to allow and others `True` to reject. + - (deprecated) on `False`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as + some callbacks in expect `True` to allow and others `True` to reject. ### `check_username_for_spam` @@ -239,22 +260,25 @@ this callback. ### `check_media_file_for_spam` _First introduced in Synapse v1.37.0_ +_Signature extended to support Allow and Codes in Synapse v1.60.0_ +_Boolean return value type deprecated in Synapse v1.60.0_ ```python async def check_media_file_for_spam( file_wrapper: "synapse.rest.media.v1.media_storage.ReadableFileWrapper", file_info: "synapse.rest.media.v1._base.FileInfo", -) -> bool +) -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", bool] ``` -Called when storing a local or remote file. The module must return a `bool` indicating -whether the given file should be excluded from the homeserver's media store. Return -`True` to prevent this file from being stored; otherwise return `False`. - -If multiple modules implement this callback, they will be considered in order. If a -callback returns `False`, Synapse falls through to the next one. The value of the first -callback that does not return `False` will be used. If this happens, Synapse will not call -any of the subsequent implementations of this callback. +Called when storing a local or remote file. 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. + - (deprecated) on `True`, behave as `ALLOW`. Deprecated as confusing, as some + callbacks in expect `True` to allow and others `True` to reject. + - (deprecated) on `False`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as + some callbacks in expect `True` to allow and others `True` to reject. ### `should_drop_federated_event` @@ -317,6 +341,8 @@ class ListSpamChecker: resource=IsUserEvilResource(config), ) - async def check_event_for_spam(self, event: "synapse.events.EventBase") -> Union[bool, str]: - return event.sender not in self.evil_users + async def check_event_for_spam(self, event: "synapse.events.EventBase") -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes"]: + if event.sender not in self.evil_users: + return synapse.module_api.ALLOW + return synapse.module_api.error.Codes.FORBIDDEN ``` diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 7984874e21df..f96540dd5c34 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -59,12 +59,72 @@ ["synapse.events.EventBase"], Awaitable[Union[bool, str]], ] -USER_MAY_JOIN_ROOM_CALLBACK = Callable[[str, str, bool], Awaitable[bool]] -USER_MAY_INVITE_CALLBACK = Callable[[str, str, str], Awaitable[bool]] -USER_MAY_SEND_3PID_INVITE_CALLBACK = Callable[[str, str, str, str], Awaitable[bool]] -USER_MAY_CREATE_ROOM_CALLBACK = Callable[[str], Awaitable[bool]] -USER_MAY_CREATE_ROOM_ALIAS_CALLBACK = Callable[[str, RoomAlias], Awaitable[bool]] -USER_MAY_PUBLISH_ROOM_CALLBACK = Callable[[str, str], Awaitable[bool]] +USER_MAY_JOIN_ROOM_CALLBACK = Callable[ + [str, str, bool], + Awaitable[ + Union[ + Allow, + Codes, + # Deprecated + bool, + ] + ], +] +USER_MAY_INVITE_CALLBACK = Callable[ + [str, str, str], + Awaitable[ + Union[ + Allow, + Codes, + # Deprecated + bool, + ] + ], +] +USER_MAY_SEND_3PID_INVITE_CALLBACK = Callable[ + [str, str, str, str], + Awaitable[ + Union[ + Allow, + Codes, + # Deprecated + bool, + ] + ], +] +USER_MAY_CREATE_ROOM_CALLBACK = Callable[ + [str], + Awaitable[ + Union[ + Allow, + Codes, + # Deprecated + bool, + ] + ], +] +USER_MAY_CREATE_ROOM_ALIAS_CALLBACK = Callable[ + [str, RoomAlias], + Awaitable[ + Union[ + Allow, + Codes, + # Deprecated + bool, + ] + ], +] +USER_MAY_PUBLISH_ROOM_CALLBACK = Callable[ + [str, str], + Awaitable[ + Union[ + Allow, + Codes, + # Deprecated + bool, + ] + ], +] CHECK_USERNAME_FOR_SPAM_CALLBACK = Callable[[UserProfile], Awaitable[bool]] LEGACY_CHECK_REGISTRATION_FOR_SPAM_CALLBACK = Callable[ [ @@ -85,7 +145,14 @@ ] CHECK_MEDIA_FILE_FOR_SPAM_CALLBACK = Callable[ [ReadableFileWrapper, FileInfo], - Awaitable[bool], + Awaitable[ + Union[ + Allow, + Codes, + # Deprecated + bool, + ] + ], ] @@ -339,7 +406,7 @@ async def should_drop_federated_event( async def user_may_join_room( self, user_id: str, room_id: str, is_invited: bool - ) -> bool: + ) -> Decision: """Checks if a given users is allowed to join a room. Not called when a user creates a room. @@ -349,7 +416,7 @@ async def user_may_join_room( is_invited: Whether the user is invited into the room Returns: - Whether the user may join the room + ALLOW if the operation is permitted, Codes otherwise. """ for callback in self._user_may_join_room_callbacks: with Measure( @@ -358,25 +425,28 @@ async def user_may_join_room( may_join_room = await delay_cancellation( callback(user_id, room_id, is_invited) ) - if may_join_room is False: - return False - - return True + if may_join_room is True or may_join_room is Allow.ALLOW: + continue + elif may_join_room is False: + return Codes.FORBIDDEN + else: + return may_join_room + + # No spam-checker has rejected the request, let it pass. + return Allow.ALLOW async def user_may_invite( self, inviter_userid: str, invitee_userid: str, room_id: str - ) -> bool: + ) -> Decision: """Checks if a given user may send an invite - If this method returns false, the invite will be rejected. - Args: inviter_userid: The user ID of the sender of the invitation invitee_userid: The user ID targeted in the invitation room_id: The room ID Returns: - True if the user may send an invite, otherwise False + ALLOW if the operation is permitted, Codes otherwise. """ for callback in self._user_may_invite_callbacks: with Measure( @@ -385,18 +455,21 @@ async def user_may_invite( may_invite = await delay_cancellation( callback(inviter_userid, invitee_userid, room_id) ) - if may_invite is False: - return False - - return True + if may_invite is True or may_invite is Allow.ALLOW: + continue + elif may_invite is False: + return Codes.FORBIDDEN + else: + return may_invite + + # No spam-checker has rejected the request, let it pass. + return Allow.ALLOW async def user_may_send_3pid_invite( self, inviter_userid: str, medium: str, address: str, room_id: str - ) -> bool: + ) -> Decision: """Checks if a given user may invite a given threepid into the room - If this method returns false, the threepid invite will be rejected. - Note that if the threepid is already associated with a Matrix user ID, Synapse will call user_may_invite with said user ID instead. @@ -407,7 +480,7 @@ async def user_may_send_3pid_invite( room_id: The room ID Returns: - True if the user may send the invite, otherwise False + ALLOW if the operation is permitted, Codes otherwise. """ for callback in self._user_may_send_3pid_invite_callbacks: with Measure( @@ -416,45 +489,44 @@ async def user_may_send_3pid_invite( may_send_3pid_invite = await delay_cancellation( callback(inviter_userid, medium, address, room_id) ) - if may_send_3pid_invite is False: - return False + if may_send_3pid_invite is True or may_send_3pid_invite is Allow.ALLOW: + continue + elif may_send_3pid_invite is False: + return Codes.FORBIDDEN + else: + return may_send_3pid_invite - return True + return Allow.ALLOW - async def user_may_create_room(self, userid: str) -> bool: + async def user_may_create_room(self, userid: str) -> Decision: """Checks if a given user may create a room - If this method returns false, the creation request will be rejected. - Args: userid: The ID of the user attempting to create a room - - Returns: - True if the user may create a room, otherwise False """ for callback in self._user_may_create_room_callbacks: with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): may_create_room = await delay_cancellation(callback(userid)) - if may_create_room is False: - return False + if may_create_room is True or may_create_room is Allow.ALLOW: + continue + elif may_create_room is False: + return Codes.FORBIDDEN + else: + return may_create_room - return True + return Allow.ALLOW async def user_may_create_room_alias( self, userid: str, room_alias: RoomAlias - ) -> bool: + ) -> Decision: """Checks if a given user may create a room alias - If this method returns false, the association request will be rejected. - Args: userid: The ID of the user attempting to create a room alias room_alias: The alias to be created - Returns: - True if the user may create a room alias, otherwise False """ for callback in self._user_may_create_room_alias_callbacks: with Measure( @@ -463,32 +535,35 @@ async def user_may_create_room_alias( may_create_room_alias = await delay_cancellation( callback(userid, room_alias) ) - if may_create_room_alias is False: - return False + if may_create_room_alias is True or may_create_room_alias is Allow.ALLOW: + continue + elif may_create_room_alias is False: + return Codes.FORBIDDEN + else: + return may_create_room_alias - return True + return Allow.ALLOW - async def user_may_publish_room(self, userid: str, room_id: str) -> bool: + async def user_may_publish_room(self, userid: str, room_id: str) -> Decision: """Checks if a given user may publish a room to the directory - If this method returns false, the publish request will be rejected. - Args: userid: The user ID attempting to publish the room room_id: The ID of the room that would be published - - Returns: - True if the user may publish the room, otherwise False """ for callback in self._user_may_publish_room_callbacks: with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): may_publish_room = await delay_cancellation(callback(userid, room_id)) - if may_publish_room is False: - return False + if may_publish_room is True or may_publish_room is Allow.ALLOW: + continue + elif may_publish_room is False: + return Codes.FORBIDDEN + else: + return may_publish_room - return True + return Allow.ALLOW async def check_username_for_spam(self, user_profile: UserProfile) -> bool: """Checks if a user ID or display name are considered "spammy" by this server. @@ -554,7 +629,7 @@ async def check_registration_for_spam( async def check_media_file_for_spam( self, file_wrapper: ReadableFileWrapper, file_info: FileInfo - ) -> bool: + ) -> Decision: """Checks if a piece of newly uploaded media should be blocked. This will be called for local uploads, downloads of remote media, each @@ -567,23 +642,19 @@ async def check_media_file_for_spam( async def check_media_file_for_spam( self, file: ReadableFileWrapper, file_info: FileInfo - ) -> bool: + ) -> Decision: buffer = BytesIO() await file.write_chunks_to(buffer.write) if buffer.getvalue() == b"Hello World": - return True + return ALLOW - return False + return Codes.FORBIDDEN Args: file: An object that allows reading the contents of the media. file_info: Metadata about the file. - - Returns: - True if the media should be blocked or False if it should be - allowed. """ for callback in self._check_media_file_for_spam_callbacks: @@ -591,7 +662,11 @@ async def check_media_file_for_spam( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): spam = await delay_cancellation(callback(file_wrapper, file_info)) - if spam: - return True + if spam is False or spam is Allow.ALLOW: + continue + elif spam is True: + return Codes.FORBIDDEN + else: + return spam - return False + return Allow.ALLOW diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 4aa33df884ac..4832f972a46c 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -140,10 +140,15 @@ async def create_association( 403, "You must be in the room to create an alias for it" ) - if not await self.spam_checker.user_may_create_room_alias( + spam_check = await self.spam_checker.user_may_create_room_alias( user_id, room_alias - ): - raise AuthError(403, "This user is not permitted to create this alias") + ) + if isinstance(spam_check, Codes): + raise AuthError( + 403, + "This user is not permitted to publish rooms to the room list", + spam_check, + ) if not self.config.roomdirectory.is_alias_creation_allowed( user_id, room_id, room_alias_str @@ -429,9 +434,12 @@ async def edit_published_room_list( """ user_id = requester.user.to_string() - if not await self.spam_checker.user_may_publish_room(user_id, room_id): + spam_check = await self.spam_checker.user_may_publish_room(user_id, room_id) + if isinstance(spam_check, Codes): raise AuthError( - 403, "This user is not permitted to publish rooms to the room list" + 403, + "This user is not permitted to publish rooms to the room list", + spam_check, ) if requester.is_guest: diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 0386d0a07bba..f86d78b07758 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -800,11 +800,14 @@ async def on_invite_request( if self.hs.config.server.block_non_admin_invites: raise SynapseError(403, "This server does not accept room invites") - if not await self.spam_checker.user_may_invite( + spam_check = await self.spam_checker.user_may_invite( event.sender, event.state_key, event.room_id - ): + ) + if isinstance(spam_check, Codes): raise SynapseError( - 403, "This user is not permitted to send invites to this server/user" + 403, + "This user is not permitted to send invites to this server/user", + spam_check, ) membership = event.content.get("membership") diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 92e1de050071..1bfdbd62469f 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -436,10 +436,9 @@ async def clone_existing_room( """ user_id = requester.user.to_string() - if not await self.spam_checker.user_may_create_room(user_id): - raise SynapseError( - 403, "You are not permitted to create rooms", Codes.FORBIDDEN - ) + spam_check = await self.spam_checker.user_may_create_room(user_id) + if isinstance(spam_check, Codes): + raise SynapseError(403, "You are not permitted to create rooms", spam_check) creation_content: JsonDict = { "room_version": new_room_version.identifier, @@ -723,12 +722,12 @@ async def create_room( invite_3pid_list = config.get("invite_3pid", []) invite_list = config.get("invite", []) - if not is_requester_admin and not ( - await self.spam_checker.user_may_create_room(user_id) - ): - raise SynapseError( - 403, "You are not permitted to create rooms", Codes.FORBIDDEN - ) + if not is_requester_admin: + spam_check = await self.spam_checker.user_may_create_room(user_id) + if isinstance(spam_check, Codes): + raise SynapseError( + 403, "You are not permitted to create rooms", spam_check + ) if ratelimit: await self.request_ratelimiter.ratelimit(requester) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index ea876c168de7..c25b0495d68e 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -682,7 +682,7 @@ async def update_membership_locked( if target_id == self._server_notices_mxid: raise SynapseError(HTTPStatus.FORBIDDEN, "Cannot invite this user") - block_invite = False + block_invite_code = None if ( self._server_notices_mxid is not None @@ -700,16 +700,19 @@ async def update_membership_locked( "Blocking invite: user is not admin and non-admin " "invites disabled" ) - block_invite = True + block_invite_code = Codes.FORBIDDEN - if not await self.spam_checker.user_may_invite( + spam_check = await self.spam_checker.user_may_invite( requester.user.to_string(), target_id, room_id - ): + ) + if isinstance(spam_check, Codes): logger.info("Blocking invite due to spam checker") - block_invite = True + block_invite_code = spam_check - if block_invite: - raise SynapseError(403, "Invites have been disabled on this server") + if block_invite_code is not None: + raise SynapseError( + 403, "Invites have been disabled on this server", block_invite_code + ) # An empty prev_events list is allowed as long as the auth_event_ids are present if prev_event_ids is not None: @@ -817,11 +820,12 @@ async def update_membership_locked( # We assume that if the spam checker allowed the user to create # a room then they're allowed to join it. and not new_room - and not await self.spam_checker.user_may_join_room( + ): + spam_check = await self.spam_checker.user_may_join_room( target.to_string(), room_id, is_invited=inviter is not None ) - ): - raise SynapseError(403, "Not allowed to join this room") + if isinstance(spam_check, Codes): + raise SynapseError(403, "Not allowed to join this room", spam_check) # Check if a remote join should be performed. remote_join, remote_room_hosts = await self._should_perform_remote_join( @@ -1377,13 +1381,14 @@ async def do_3pid_invite( ) else: # Check if the spamchecker(s) allow this invite to go through. - if not await self.spam_checker.user_may_send_3pid_invite( + spam_check = await self.spam_checker.user_may_send_3pid_invite( inviter_userid=requester.user.to_string(), medium=medium, address=address, room_id=room_id, - ): - raise SynapseError(403, "Cannot send threepid invite") + ) + if isinstance(spam_check, Codes): + raise SynapseError(403, "Cannot send threepid invite", spam_check) 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 604f18bf52d3..b0669ad0f0e4 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -36,7 +36,7 @@ from twisted.internet.interfaces import IConsumer from twisted.protocols.basic import FileSender -from synapse.api.errors import NotFoundError +from synapse.api.errors import Codes, NotFoundError from synapse.logging.context import defer_to_thread, make_deferred_yieldable from synapse.util import Clock from synapse.util.file_consumer import BackgroundFileConsumer @@ -145,15 +145,15 @@ async def finish() -> None: f.flush() f.close() - spam = await self.spam_checker.check_media_file_for_spam( + spam_check = await self.spam_checker.check_media_file_for_spam( ReadableFileWrapper(self.clock, fname), file_info ) - if spam: + if isinstance(spam_check, Codes): logger.info("Blocking media due to spam checker") # 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() + raise SpamMediaException("Not found", spam_check) for provider in self.storage_providers: await provider.store_file(path, file_info) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 4d658d29cab5..8cee66997b49 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -11,7 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from typing import Tuple +from typing import Tuple, Union from unittest.mock import Mock, patch from urllib.parse import quote @@ -19,10 +19,12 @@ import synapse.rest.admin from synapse.api.constants import UserTypes +from synapse.api.errors import Codes from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.appservice import ApplicationService from synapse.rest.client import login, register, room, user_directory from synapse.server import HomeServer +from synapse.spam_checker_api import Allow from synapse.storage.roommember import ProfileInfo from synapse.types import create_requester from synapse.util import Clock @@ -773,11 +775,24 @@ def test_spam_checker(self) -> None: s = self.get_success(self.handler.search_users(u1, "user2", 10)) self.assertEqual(len(s["results"]), 1) - async def allow_all(user_profile: ProfileInfo) -> bool: - # Allow all users. + async def allow_all_deprecated(user_profile: ProfileInfo) -> bool: + # Allow all users, deprecated API. return False - # Configure a spam checker that does not filter any users. + # Configure a spam checker that does not filter any users (deprecated API). + spam_checker = self.hs.get_spam_checker() + spam_checker._check_username_for_spam_callbacks = [allow_all_deprecated] + + async def allow_all(user_profile: ProfileInfo) -> Union[Allow, Codes]: + # Allow all users. + return Allow.ALLOW + + # The results do not change: + # We get one search result when searching for user2 by user1. + s = self.get_success(self.handler.search_users(u1, "user2", 10)) + self.assertEqual(len(s["results"]), 1) + + # Configure a spam checker that does not filter any users (deprecated API). spam_checker = self.hs.get_spam_checker() spam_checker._check_username_for_spam_callbacks = [allow_all] @@ -787,10 +802,21 @@ async def allow_all(user_profile: ProfileInfo) -> bool: self.assertEqual(len(s["results"]), 1) # Configure a spam checker that filters all users. - async def block_all(user_profile: ProfileInfo) -> bool: + async def block_all_deprecated(user_profile: ProfileInfo) -> bool: # All users are spammy. return True + spam_checker._check_username_for_spam_callbacks = [block_all_deprecated] + + # User1 now gets no search results for any of the other users. + s = self.get_success(self.handler.search_users(u1, "user2", 10)) + self.assertEqual(len(s["results"]), 0) + + # Configure a spam checker that filters all users. + async def block_all(user_profile: ProfileInfo) -> Union[Allow, Codes]: + # All users are spammy. + return Codes.CONSENT_NOT_GIVEN + spam_checker._check_username_for_spam_callbacks = [block_all] # User1 now gets no search results for any of the other users. diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index d0197aca94a0..40bae6e602f7 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 +from typing import Any, Dict, Iterable, List, Optional, Union from unittest.mock import Mock, call from urllib import parse as urlparse @@ -36,6 +36,7 @@ from synapse.rest import admin from synapse.rest.client import account, directory, login, profile, room, sync from synapse.server import HomeServer +from synapse.spam_checker_api import Allow, Decision from synapse.types import JsonDict, RoomAlias, UserID, create_requester from synapse.util import Clock from synapse.util.stringutils import random_string @@ -676,7 +677,7 @@ def test_post_room_invitees_ratelimit(self) -> None: channel = self.make_request("POST", "/createRoom", content) self.assertEqual(200, channel.code) - def test_spam_checker_may_join_room(self) -> None: + def test_spam_checker_may_join_room_deprecated(self) -> None: """Tests that the user_may_join_room spam checker callback is correctly bypassed when creating a new room. """ @@ -700,6 +701,30 @@ async def user_may_join_room( self.assertEqual(join_mock.call_count, 0) + def test_spam_checker_may_join_room(self) -> None: + """Tests that the user_may_join_room spam checker callback is correctly bypassed + when creating a new room. + """ + + async def user_may_join_room( + mxid: str, + room_id: str, + is_invite: bool, + ) -> Decision: + return Codes.CONSENT_NOT_GIVEN + + join_mock = Mock(side_effect=user_may_join_room) + self.hs.get_spam_checker()._user_may_join_room_callbacks.append(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.""" @@ -910,7 +935,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.room2 = self.helper.create_room_as(room_creator=self.user1, tok=self.tok1) self.room3 = self.helper.create_room_as(room_creator=self.user1, tok=self.tok1) - def test_spam_checker_may_join_room(self) -> None: + def test_spam_checker_may_join_room_deprecated(self) -> None: """Tests that the user_may_join_room spam checker callback is correctly called and blocks room joins when needed. """ @@ -967,6 +992,63 @@ async def user_may_join_room( return_value = False self.helper.join(self.room3, self.user2, expect_code=403, tok=self.tok2) + def test_spam_checker_may_join_room(self) -> None: + """Tests that the user_may_join_room spam checker callback is correctly called + and blocks room joins when needed. + """ + + # Register a dummy callback. Make it allow all room joins for now. + return_value: Union[Allow, Codes] = Allow.ALLOW + + async def user_may_join_room( + userid: str, + room_id: str, + is_invited: bool, + ) -> Union[Allow, Codes]: + return return_value + + callback_mock = Mock(side_effect=user_may_join_room, spec=lambda *x: None) + self.hs.get_spam_checker()._user_may_join_room_callbacks.append(callback_mock) + + # Join a first room, without being invited to it. + self.helper.join(self.room1, self.user2, tok=self.tok2) + + # Check that the callback was called with the right arguments. + expected_call_args = ( + ( + self.user2, + self.room1, + False, + ), + ) + self.assertEqual( + callback_mock.call_args, + expected_call_args, + callback_mock.call_args, + ) + + # Join a second room, this time with an invite for it. + self.helper.invite(self.room2, self.user1, self.user2, tok=self.tok1) + self.helper.join(self.room2, self.user2, tok=self.tok2) + + # Check that the callback was called with the right arguments. + expected_call_args = ( + ( + self.user2, + self.room2, + True, + ), + ) + self.assertEqual( + callback_mock.call_args, + expected_call_args, + callback_mock.call_args, + ) + + # Now make the callback deny all room joins, and check that a join actually fails. + return_value = Codes.CONSENT_NOT_GIVEN + self.helper.join(self.room3, self.user2, expect_code=403, tok=self.tok2) + class RoomJoinRatelimitTestCase(RoomBase): user_id = "@sid1:red" @@ -2844,7 +2926,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.room_id = self.helper.create_room_as(self.user_id, tok=self.tok) - def test_threepid_invite_spamcheck(self) -> None: + def test_threepid_invite_spamcheck_deprecated(self) -> None: # Mock a few functions to prevent the test from failing due to failing to talk to # a remote IS. We keep the mock for _mock_make_and_store_3pid_invite around so we # can check its call_count later on during the test. @@ -2900,3 +2982,60 @@ 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() + + def test_threepid_invite_spamcheck(self) -> None: + # Mock a few functions to prevent the test from failing due to failing to talk to + # a remote IS. We keep the mock for _mock_make_and_store_3pid_invite around so we + # can check its call_count later on during the test. + make_invite_mock = Mock(return_value=make_awaitable(0)) + self.hs.get_room_member_handler()._make_and_store_3pid_invite = make_invite_mock + self.hs.get_identity_handler().lookup_3pid = Mock( + return_value=make_awaitable(None), + ) + + # Add a mock to the spamchecker callbacks for user_may_send_3pid_invite. Make it + # allow everything for now. + # `spec` argument is needed for this function mock to have `__qualname__`, which + # is needed for `Measure` metrics buried in SpamChecker. + mock = Mock(return_value=make_awaitable(Allow.ALLOW), spec=lambda *x: None) + self.hs.get_spam_checker()._user_may_send_3pid_invite_callbacks.append(mock) + + # Send a 3PID invite into the room and check that it succeeded. + email_to_invite = "teresa@example.com" + 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, 200) + + # Check that the callback was called with the right params. + mock.assert_called_with(self.user_id, "email", email_to_invite, self.room_id) + + # Check that the call to send the invite was made. + 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. + 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()