From b0838a343ac2a2177dcd8bebe8e779a7e56a3354 Mon Sep 17 00:00:00 2001 From: David Teller Date: Tue, 24 May 2022 15:50:38 +0200 Subject: [PATCH 01/14] Port the rest of the spam-checker API to accept `Union[Allow, Codes]` where relevant. --- changelog.d/12857.feature | 1 + synapse/events/spamcheck.py | 203 +++++++++++++++++-------- synapse/handlers/directory.py | 18 ++- synapse/handlers/federation.py | 9 +- synapse/handlers/room.py | 19 ++- synapse/handlers/room_member.py | 31 ++-- synapse/rest/media/v1/media_storage.py | 8 +- tests/handlers/test_user_directory.py | 36 ++++- tests/rest/client/test_rooms.py | 147 +++++++++++++++++- 9 files changed, 364 insertions(+), 108 deletions(-) create mode 100644 changelog.d/12857.feature 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/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index d2e06c754ef1..0ba7ba31f999 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -62,12 +62,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[ [ @@ -88,7 +148,14 @@ ] CHECK_MEDIA_FILE_FOR_SPAM_CALLBACK = Callable[ [ReadableFileWrapper, FileInfo], - Awaitable[bool], + Awaitable[ + Union[ + Allow, + Codes, + # Deprecated + bool, + ] + ], ] @@ -352,7 +419,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. @@ -362,7 +429,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( @@ -371,25 +438,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 + 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 - return True + # 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( @@ -398,18 +468,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 + if may_invite is True or may_invite is Allow.ALLOW: + continue + elif may_invite is False: + return Codes.FORBIDDEN + else: + return may_invite - return True + # 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. @@ -420,7 +493,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( @@ -429,45 +502,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( @@ -476,32 +548,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. @@ -567,7 +642,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 @@ -580,23 +655,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: @@ -604,7 +675,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 - - return False + if spam is False or spam is Allow.ALLOW: + continue + elif spam is True: + return Codes.FORBIDDEN + else: + return spam + + return Allow.ALLOW diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 44e84698c48e..a0146ad83c36 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -141,10 +141,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 @@ -430,9 +435,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 b212ee217295..7e4e2eee53de 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -821,11 +821,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 e2b0e519d45c..76580f6a5811 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -437,10 +437,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, @@ -727,12 +726,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 70c674ff8e25..32605bb5bb89 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -683,7 +683,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 @@ -701,16 +701,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: @@ -818,11 +821,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( @@ -1369,13 +1373,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 9e39cd97e5bf..725760317741 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 @@ -772,11 +774,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] @@ -786,10 +801,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 f523d89b8fcb..f8aa8c30dd4f 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 @@ -37,6 +37,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 @@ -677,7 +678,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. """ @@ -701,6 +702,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.""" @@ -911,7 +936,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. """ @@ -968,6 +993,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" @@ -2845,7 +2927,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. @@ -2901,3 +2983,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() From 45be51acc2b66ca983dc80c29fb765dbc53ada05 Mon Sep 17 00:00:00 2001 From: David Teller Date: Wed, 25 May 2022 12:46:06 +0200 Subject: [PATCH 02/14] WIP: Oops, that test shouldn't have been changed --- tests/handlers/test_user_directory.py | 36 ++++----------------------- 1 file changed, 5 insertions(+), 31 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 725760317741..9e39cd97e5bf 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, Union +from typing import Tuple from unittest.mock import Mock, patch from urllib.parse import quote @@ -19,12 +19,10 @@ 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 @@ -774,24 +772,11 @@ 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_deprecated(user_profile: ProfileInfo) -> bool: - # Allow all users, deprecated API. - return False - - # 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]: + async def allow_all(user_profile: ProfileInfo) -> bool: # 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) + return False - # Configure a spam checker that does not filter any users (deprecated API). + # Configure a spam checker that does not filter any users. spam_checker = self.hs.get_spam_checker() spam_checker._check_username_for_spam_callbacks = [allow_all] @@ -801,21 +786,10 @@ async def allow_all(user_profile: ProfileInfo) -> Union[Allow, Codes]: self.assertEqual(len(s["results"]), 1) # Configure a spam checker that filters all users. - async def block_all_deprecated(user_profile: ProfileInfo) -> bool: + async def block_all(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. From 8c40e2a94877e7563679c5a11b165d10bcbcac44 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 2 Jun 2022 10:24:21 +0200 Subject: [PATCH 03/14] WIP --- synapse/events/spamcheck.py | 78 ++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 0ba7ba31f999..4bb4ff10aaf7 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -23,12 +23,14 @@ Collection, Dict, List, + Literal, Optional, Tuple, Union, ) from synapse.api.errors import Codes +from synapse.module_api import NOT_SPAM from synapse.rest.media.v1._base import FileInfo from synapse.rest.media.v1.media_storage import ReadableFileWrapper from synapse.spam_checker_api import RegistrationBehaviour @@ -66,7 +68,7 @@ [str, str, bool], Awaitable[ Union[ - Allow, + Literal["NOT_SPAM"], Codes, # Deprecated bool, @@ -77,7 +79,7 @@ [str, str, str], Awaitable[ Union[ - Allow, + Literal["NOT_SPAM"], Codes, # Deprecated bool, @@ -88,7 +90,7 @@ [str, str, str, str], Awaitable[ Union[ - Allow, + Literal["NOT_SPAM"], Codes, # Deprecated bool, @@ -99,7 +101,7 @@ [str], Awaitable[ Union[ - Allow, + Literal["NOT_SPAM"], Codes, # Deprecated bool, @@ -110,7 +112,7 @@ [str, RoomAlias], Awaitable[ Union[ - Allow, + Literal["NOT_SPAM"], Codes, # Deprecated bool, @@ -121,7 +123,7 @@ [str, str], Awaitable[ Union[ - Allow, + Literal["NOT_SPAM"], Codes, # Deprecated bool, @@ -150,7 +152,7 @@ [ReadableFileWrapper, FileInfo], Awaitable[ Union[ - Allow, + Literal["NOT_SPAM"], Codes, # Deprecated bool, @@ -248,7 +250,7 @@ def run(*args: Any, **kwargs: Any) -> Awaitable: class SpamChecker: - NOT_SPAM = "NOT_SPAM" + NOT_SPAM: Literal["NOT_SPAM"] = "NOT_SPAM" def __init__(self, hs: "synapse.server.HomeServer") -> None: self.hs = hs @@ -390,7 +392,7 @@ async def check_event_for_spam( return res # No spam-checker has rejected the event, let it pass. - return self.NOT_SPAM + return "NOT_SPAM" async def should_drop_federated_event( self, event: "synapse.events.EventBase" @@ -419,7 +421,7 @@ async def should_drop_federated_event( async def user_may_join_room( self, user_id: str, room_id: str, is_invited: bool - ) -> Decision: + ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: """Checks if a given users is allowed to join a room. Not called when a user creates a room. @@ -429,7 +431,7 @@ async def user_may_join_room( is_invited: Whether the user is invited into the room Returns: - ALLOW if the operation is permitted, Codes otherwise. + NOT_SPAM if the operation is permitted, Codes otherwise. """ for callback in self._user_may_join_room_callbacks: with Measure( @@ -438,19 +440,19 @@ async def user_may_join_room( may_join_room = await delay_cancellation( callback(user_id, room_id, is_invited) ) - if may_join_room is True or may_join_room is Allow.ALLOW: + if may_join_room is True or may_join_room is NOT_SPAM: continue elif may_join_room is False: - return Codes.FORBIDDEN + return (Codes.FORBIDDEN, {}) else: return may_join_room # No spam-checker has rejected the request, let it pass. - return Allow.ALLOW + return "NOT_SPAM" async def user_may_invite( self, inviter_userid: str, invitee_userid: str, room_id: str - ) -> Decision: + ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: """Checks if a given user may send an invite Args: @@ -459,7 +461,7 @@ async def user_may_invite( room_id: The room ID Returns: - ALLOW if the operation is permitted, Codes otherwise. + NOT_SPAM if the operation is permitted, Codes otherwise. """ for callback in self._user_may_invite_callbacks: with Measure( @@ -468,19 +470,19 @@ async def user_may_invite( may_invite = await delay_cancellation( callback(inviter_userid, invitee_userid, room_id) ) - if may_invite is True or may_invite is Allow.ALLOW: + if may_invite is True or may_invite is NOT_SPAM: continue elif may_invite is False: - return Codes.FORBIDDEN + return [Codes.FORBIDDEN, {}] else: return may_invite # No spam-checker has rejected the request, let it pass. - return Allow.ALLOW + return "NOT_SPAM" async def user_may_send_3pid_invite( self, inviter_userid: str, medium: str, address: str, room_id: str - ) -> Decision: + ) -> 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 @@ -493,7 +495,7 @@ async def user_may_send_3pid_invite( room_id: The room ID Returns: - ALLOW if the operation is permitted, Codes otherwise. + NOT_SPAM if the operation is permitted, Codes otherwise. """ for callback in self._user_may_send_3pid_invite_callbacks: with Measure( @@ -502,16 +504,18 @@ 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 True or may_send_3pid_invite is Allow.ALLOW: + if may_send_3pid_invite is True or may_send_3pid_invite is NOT_SPAM: continue elif may_send_3pid_invite is False: return Codes.FORBIDDEN else: return may_send_3pid_invite - return Allow.ALLOW + return "NOT_SPAM" - async def user_may_create_room(self, userid: str) -> Decision: + async def user_may_create_room( + self, userid: str + ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: """Checks if a given user may create a room Args: @@ -522,18 +526,18 @@ async def user_may_create_room(self, userid: str) -> Decision: self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): may_create_room = await delay_cancellation(callback(userid)) - if may_create_room is True or may_create_room is Allow.ALLOW: + if may_create_room is True or may_create_room is NOT_SPAM: continue elif may_create_room is False: return Codes.FORBIDDEN else: return may_create_room - return Allow.ALLOW + return "NOT_SPAM" async def user_may_create_room_alias( self, userid: str, room_alias: RoomAlias - ) -> Decision: + ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: """Checks if a given user may create a room alias Args: @@ -548,16 +552,18 @@ async def user_may_create_room_alias( may_create_room_alias = await delay_cancellation( callback(userid, room_alias) ) - if may_create_room_alias is True or may_create_room_alias is Allow.ALLOW: + if may_create_room_alias is True or may_create_room_alias is NOT_SPAM: continue elif may_create_room_alias is False: return Codes.FORBIDDEN else: return may_create_room_alias - return Allow.ALLOW + return "NOT_SPAM" - async def user_may_publish_room(self, userid: str, room_id: str) -> Decision: + async def user_may_publish_room( + self, userid: str, room_id: str + ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: """Checks if a given user may publish a room to the directory Args: @@ -569,14 +575,14 @@ async def user_may_publish_room(self, userid: str, room_id: str) -> Decision: self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): may_publish_room = await delay_cancellation(callback(userid, room_id)) - if may_publish_room is True or may_publish_room is Allow.ALLOW: + if may_publish_room is True or may_publish_room is NOT_SPAM: continue elif may_publish_room is False: return Codes.FORBIDDEN else: return may_publish_room - return Allow.ALLOW + return "NOT_SPAM" 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. @@ -642,7 +648,7 @@ async def check_registration_for_spam( async def check_media_file_for_spam( self, file_wrapper: ReadableFileWrapper, file_info: FileInfo - ) -> Decision: + ) -> 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 @@ -660,7 +666,7 @@ async def check_media_file_for_spam( await file.write_chunks_to(buffer.write) if buffer.getvalue() == b"Hello World": - return ALLOW + return "NOT_SPAM" return Codes.FORBIDDEN @@ -675,11 +681,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 is False or spam is Allow.ALLOW: + if spam is False or spam is NOT_SPAM: continue elif spam is True: return Codes.FORBIDDEN else: return spam - return Allow.ALLOW + return "NOT_SPAM" From d8a7a20ea285b1dc6b4bb0301220b21890ecaa13 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 2 Jun 2022 11:09:55 +0200 Subject: [PATCH 04/14] WIP: Uniformizing with #12918 --- changelog.d/12857.feature | 2 +- docs/modules/spam_checker_callbacks.md | 196 +++++++++++++++++-------- synapse/events/spamcheck.py | 38 ++--- synapse/handlers/message.py | 12 +- tests/rest/client/test_rooms.py | 11 +- 5 files changed, 168 insertions(+), 91 deletions(-) diff --git a/changelog.d/12857.feature b/changelog.d/12857.feature index ef631cdc3ac0..6f5acbcbda36 100644 --- a/changelog.d/12857.feature +++ b/changelog.d/12857.feature @@ -1 +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]`. +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[Literal["NOT_SPAM"], Codes]`. This is a followup to #12703, #12808, #12846 and builds towards giving the spam-checker API the ability to inform users of *why* their event or operation has been rejected. diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index ad35e667ed4b..9a9ca809fadb 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -38,62 +38,83 @@ this callback. _First introduced in Synapse v1.37.0_ +_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ + ```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.NOT_SPAM", "synapse.module_api.errors.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. - -The user is represented by their Matrix user ID (e.g. +Called when a user is trying to join a room. 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. `!room:example.com`). The module is also given a boolean to indicate whether the user 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. +The callback must return one of: + - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still + decide to reject it. + - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case + of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. + typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. + - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. 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. +callback returns `synapse.module_api.NOT_SPAM`, Synapse falls through to the next one. +The value of the first callback that does not return `synapse.module_api.NOT_SPAM` will +be used. If this happens, Synapse will not call any of the subsequent implementations of +this callback. + + +This callback isn't called if the join is performed by a server administrator, or in the +context of a room creation. ### `user_may_invite` _First introduced in Synapse v1.37.0_ +_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ + ```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) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.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. Both inviter and invitee are +represented by their Matrix user ID (e.g. `@alice:example.com`). + + +The callback must return one of: + - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still + decide to reject it. + - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case + of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. + typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. + - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. 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. +callback returns `synapse.module_api.NOT_SPAM`, Synapse falls through to the next one. +The value of the first callback that does not return `synapse.module_api.NOT_SPAM` will +be used. If this happens, Synapse will not call any of the subsequent implementations of +this callback. + ### `user_may_send_3pid_invite` _First introduced in Synapse v1.45.0_ +_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ + ```python async def user_may_send_3pid_invite( inviter: str, medium: str, address: str, room_id: str, -) -> bool +) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.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. +e.g. an email address or a phone number). 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 @@ -115,63 +136,108 @@ await user_may_send_3pid_invite( **Note**: If the third-party identifier is already associated with a matrix user ID, [`user_may_invite`](#user_may_invite) will be used instead. +The callback must return one of: + - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still + decide to reject it. + - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case + of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. + typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. + - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. + 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. +callback returns `synapse.module_api.NOT_SPAM`, Synapse falls through to the next one. +The value of the first callback that does not return `synapse.module_api.NOT_SPAM` will +be used. If this happens, Synapse will not call any of the subsequent implementations of +this callback. + ### `user_may_create_room` _First introduced in Synapse v1.37.0_ +_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ + ```python -async def user_may_create_room(user: str) -> bool +async def user_may_create_room(user_id: str) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.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. +Called when processing a room creation request. + +The callback must return one of: + - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still + decide to reject it. + - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case + of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. + typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. + - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. 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. +callback returns `synapse.module_api.NOT_SPAM`, Synapse falls through to the next one. +The value of the first callback that does not return `synapse.module_api.NOT_SPAM` will +be used. If this happens, Synapse will not call any of the subsequent implementations of +this callback. + + ### `user_may_create_room_alias` _First introduced in Synapse v1.37.0_ +_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ + ```python -async def user_may_create_room_alias(user: str, room_alias: "synapse.types.RoomAlias") -> bool +async def user_may_create_room_alias(user_id: str, room_alias: "synapse.types.RoomAlias") -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", 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. +Called when trying to associate an alias with an existing room. + +The callback must return one of: + - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still + decide to reject it. + - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case + of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. + typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. + - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. 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. +callback returns `synapse.module_api.NOT_SPAM`, Synapse falls through to the next one. +The value of the first callback that does not return `synapse.module_api.NOT_SPAM` will +be used. If this happens, Synapse will not call any of the subsequent implementations of +this callback. + + ### `user_may_publish_room` _First introduced in Synapse v1.37.0_ +_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ + ```python -async def user_may_publish_room(user: str, room_id: str) -> bool +async def user_may_publish_room(user_id: str, room_id: str) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.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. +Called when trying to publish a room to the homeserver's public rooms directory. + +The callback must return one of: + - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still + decide to reject it. + - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case + of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. + typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. + - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. 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. +callback returns `synapse.module_api.NOT_SPAM`, Synapse falls through to the next one. +The value of the first callback that does not return `synapse.module_api.NOT_SPAM` will +be used. If this happens, Synapse will not call any of the subsequent implementations of +this callback. + + ### `check_username_for_spam` @@ -239,21 +305,32 @@ this callback. _First introduced in Synapse v1.37.0_ +_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ + ```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 +) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.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`. +Called when storing a local or remote file. + +The callback must return one of: + - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still + decide to reject it. + - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case + of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. + typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. + - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. If multiple modules implement this callback, they will be considered in order. If a -callback returns `False`, Synapse falls through to the next one. The value of the first -callback that does not return `False` will be used. If this happens, Synapse will not call -any of the subsequent implementations of this callback. +callback returns `synapse.module_api.NOT_SPAM`, Synapse falls through to the next one. +The value of the first callback that does not return `synapse.module_api.NOT_SPAM` will +be used. If this happens, Synapse will not call any of the subsequent implementations of +this callback. + ### `should_drop_federated_event` @@ -316,6 +393,9 @@ 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[Literal["NOT_SPAM"], Codes]: + if event.sender in self.evil_users: + return Codes.FORBIDDEN + else: + return "NOT_SPAM" ``` diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 4bb4ff10aaf7..0521d9b3f521 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -375,7 +375,7 @@ async def check_event_for_spam( elif res is True: # This spam-checker rejects the event with deprecated # return value `True` - return Codes.FORBIDDEN + return (Codes.FORBIDDEN, {}) elif not isinstance(res, str): # mypy complains that we can't reach this code because of the # return type in CHECK_EVENT_FOR_SPAM_CALLBACK, but we don't know @@ -392,7 +392,7 @@ async def check_event_for_spam( return res # No spam-checker has rejected the event, let it pass. - return "NOT_SPAM" + return NOT_SPAM async def should_drop_federated_event( self, event: "synapse.events.EventBase" @@ -421,7 +421,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[Codes, Dict], Literal["NOT_SPAM"]]: + ) -> Union[Codes, Literal["NOT_SPAM"]]: """Checks if a given users is allowed to join a room. Not called when a user creates a room. @@ -443,16 +443,16 @@ async def user_may_join_room( if may_join_room is True or may_join_room is NOT_SPAM: continue elif may_join_room is False: - return (Codes.FORBIDDEN, {}) + return Codes.FORBIDDEN else: return may_join_room # No spam-checker has rejected the request, let it pass. - return "NOT_SPAM" + return NOT_SPAM async def user_may_invite( self, inviter_userid: str, invitee_userid: str, room_id: str - ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: + ) -> Union[Codes, Literal["NOT_SPAM"]]: """Checks if a given user may send an invite Args: @@ -473,16 +473,16 @@ async def user_may_invite( if may_invite is True or may_invite is NOT_SPAM: continue elif may_invite is False: - return [Codes.FORBIDDEN, {}] + return Codes.FORBIDDEN else: return may_invite # No spam-checker has rejected the request, let it pass. - return "NOT_SPAM" + return NOT_SPAM async def user_may_send_3pid_invite( self, inviter_userid: str, medium: str, address: str, room_id: str - ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: + ) -> Union[Codes, 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 @@ -511,11 +511,11 @@ async def user_may_send_3pid_invite( else: return may_send_3pid_invite - return "NOT_SPAM" + return NOT_SPAM async def user_may_create_room( self, userid: str - ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: + ) -> Union[Codes, Literal["NOT_SPAM"]]: """Checks if a given user may create a room Args: @@ -533,11 +533,11 @@ async def user_may_create_room( else: return may_create_room - return "NOT_SPAM" + return NOT_SPAM async def user_may_create_room_alias( self, userid: str, room_alias: RoomAlias - ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: + ) -> Union[Codes, Literal["NOT_SPAM"]]: """Checks if a given user may create a room alias Args: @@ -559,11 +559,11 @@ async def user_may_create_room_alias( else: return may_create_room_alias - return "NOT_SPAM" + return NOT_SPAM async def user_may_publish_room( self, userid: str, room_id: str - ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: + ) -> Union[Codes, Literal["NOT_SPAM"]]: """Checks if a given user may publish a room to the directory Args: @@ -582,7 +582,7 @@ async def user_may_publish_room( else: return may_publish_room - return "NOT_SPAM" + return NOT_SPAM 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. @@ -648,7 +648,7 @@ async def check_registration_for_spam( async def check_media_file_for_spam( self, file_wrapper: ReadableFileWrapper, file_info: FileInfo - ) -> Union[Tuple[Codes, Dict], Literal["NOT_SPAM"]]: + ) -> Union[Codes, 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 @@ -666,7 +666,7 @@ async def check_media_file_for_spam( await file.write_chunks_to(buffer.write) if buffer.getvalue() == b"Hello World": - return "NOT_SPAM" + return NOT_SPAM return Codes.FORBIDDEN @@ -688,4 +688,4 @@ async def check_media_file_for_spam( else: return spam - return "NOT_SPAM" + return NOT_SPAM diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 081625f0bd30..ed73b1324d4d 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -952,14 +952,12 @@ async def create_and_send_nonmember_event( "Spam-check module returned invalid error value. Expecting [code, dict], got %s", spam_check_result, ) - spam_check_result = Codes.FORBIDDEN - if isinstance(spam_check_result, Codes): - raise SynapseError( - 403, - "This message has been rejected as probable spam", - spam_check_result, - ) + raise SynapseError( + 403, + "This message has been rejected as probable spam", + Codes.FORBIDDEN, + ) # Backwards compatibility: if the return value is not an error code, it # means the module returned an error message to be included in the diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index f8aa8c30dd4f..f69eda180a1c 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, Literal, Optional, Union from unittest.mock import Mock, call from urllib import parse as urlparse @@ -37,7 +37,6 @@ 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 @@ -711,7 +710,7 @@ async def user_may_join_room( mxid: str, room_id: str, is_invite: bool, - ) -> Decision: + ) -> Codes: return Codes.CONSENT_NOT_GIVEN join_mock = Mock(side_effect=user_may_join_room) @@ -999,13 +998,13 @@ def test_spam_checker_may_join_room(self) -> None: """ # Register a dummy callback. Make it allow all room joins for now. - return_value: Union[Allow, Codes] = Allow.ALLOW + return_value: Union[Literal["NOT_SPAM"], Codes] = "NOT_SPAM" async def user_may_join_room( userid: str, room_id: str, is_invited: bool, - ) -> Union[Allow, Codes]: + ) -> Union[Literal["NOT_SPAM"], Codes]: return return_value callback_mock = Mock(side_effect=user_may_join_room, spec=lambda *x: None) @@ -2998,7 +2997,7 @@ def test_threepid_invite_spamcheck(self) -> None: # 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) + mock = Mock(return_value=make_awaitable("NOT_SPAM"), 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. From 3224a95d51dfc29a4c6fe7fee145e5cd1600c893 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 2 Jun 2022 14:02:19 +0200 Subject: [PATCH 05/14] WIP: Updating version number --- docs/modules/spam_checker_callbacks.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 9a9ca809fadb..6ca3e80de74d 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -38,7 +38,7 @@ this callback. _First introduced in Synapse v1.37.0_ -_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ +_Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python async def user_may_join_room(user: str, room: str, is_invited: bool) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] @@ -72,7 +72,7 @@ context of a room creation. _First introduced in Synapse v1.37.0_ -_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ +_Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python async def user_may_invite(inviter: str, invitee: str, room_id: str) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] @@ -102,7 +102,7 @@ this callback. _First introduced in Synapse v1.45.0_ -_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ +_Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python async def user_may_send_3pid_invite( @@ -156,7 +156,7 @@ this callback. _First introduced in Synapse v1.37.0_ -_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ +_Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python async def user_may_create_room(user_id: str) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] @@ -185,7 +185,7 @@ this callback. _First introduced in Synapse v1.37.0_ -_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ +_Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python async def user_may_create_room_alias(user_id: str, room_alias: "synapse.types.RoomAlias") -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] @@ -214,7 +214,7 @@ this callback. _First introduced in Synapse v1.37.0_ -_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ +_Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python async def user_may_publish_room(user_id: str, room_id: str) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] @@ -305,7 +305,7 @@ this callback. _First introduced in Synapse v1.37.0_ -_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ +_Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python async def check_media_file_for_spam( From 88ce4005f3de8e7a8e5c029b5255012bd39ad4d7 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 2 Jun 2022 16:20:29 +0200 Subject: [PATCH 06/14] WIP: Circula dependencies --- synapse/events/spamcheck.py | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 0521d9b3f521..7f552168a044 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -30,7 +30,6 @@ ) from synapse.api.errors import Codes -from synapse.module_api import NOT_SPAM from synapse.rest.media.v1._base import FileInfo from synapse.rest.media.v1.media_storage import ReadableFileWrapper from synapse.spam_checker_api import RegistrationBehaviour @@ -392,7 +391,7 @@ async def check_event_for_spam( return res # No spam-checker has rejected the event, let it pass. - return NOT_SPAM + return self.NOT_SPAM async def should_drop_federated_event( self, event: "synapse.events.EventBase" @@ -440,7 +439,7 @@ async def user_may_join_room( may_join_room = await delay_cancellation( callback(user_id, room_id, is_invited) ) - if may_join_room is True or may_join_room is NOT_SPAM: + if may_join_room is True or may_join_room is self.NOT_SPAM: continue elif may_join_room is False: return Codes.FORBIDDEN @@ -448,7 +447,7 @@ async def user_may_join_room( return may_join_room # No spam-checker has rejected the request, let it pass. - return NOT_SPAM + return self.NOT_SPAM async def user_may_invite( self, inviter_userid: str, invitee_userid: str, room_id: str @@ -470,7 +469,7 @@ async def user_may_invite( may_invite = await delay_cancellation( callback(inviter_userid, invitee_userid, room_id) ) - if may_invite is True or may_invite is NOT_SPAM: + if may_invite is True or may_invite is self.NOT_SPAM: continue elif may_invite is False: return Codes.FORBIDDEN @@ -478,7 +477,7 @@ async def user_may_invite( return may_invite # No spam-checker has rejected the request, let it pass. - return NOT_SPAM + return self.NOT_SPAM async def user_may_send_3pid_invite( self, inviter_userid: str, medium: str, address: str, room_id: str @@ -504,14 +503,14 @@ 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 True or may_send_3pid_invite is NOT_SPAM: + if may_send_3pid_invite is True or may_send_3pid_invite is self.NOT_SPAM: continue elif may_send_3pid_invite is False: return Codes.FORBIDDEN else: return may_send_3pid_invite - return NOT_SPAM + return self.NOT_SPAM async def user_may_create_room( self, userid: str @@ -526,14 +525,14 @@ async def user_may_create_room( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): may_create_room = await delay_cancellation(callback(userid)) - if may_create_room is True or may_create_room is NOT_SPAM: + if may_create_room is True or may_create_room is self.NOT_SPAM: continue elif may_create_room is False: return Codes.FORBIDDEN else: return may_create_room - return NOT_SPAM + return self.NOT_SPAM async def user_may_create_room_alias( self, userid: str, room_alias: RoomAlias @@ -552,14 +551,14 @@ async def user_may_create_room_alias( may_create_room_alias = await delay_cancellation( callback(userid, room_alias) ) - if may_create_room_alias is True or may_create_room_alias is NOT_SPAM: + if may_create_room_alias is True or may_create_room_alias is self.NOT_SPAM: continue elif may_create_room_alias is False: return Codes.FORBIDDEN else: return may_create_room_alias - return NOT_SPAM + return self.NOT_SPAM async def user_may_publish_room( self, userid: str, room_id: str @@ -575,14 +574,14 @@ async def user_may_publish_room( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): may_publish_room = await delay_cancellation(callback(userid, room_id)) - if may_publish_room is True or may_publish_room is NOT_SPAM: + if may_publish_room is True or may_publish_room is self.NOT_SPAM: continue elif may_publish_room is False: return Codes.FORBIDDEN else: return may_publish_room - return NOT_SPAM + return self.NOT_SPAM 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. @@ -666,7 +665,7 @@ async def check_media_file_for_spam( await file.write_chunks_to(buffer.write) if buffer.getvalue() == b"Hello World": - return NOT_SPAM + return self.NOT_SPAM return Codes.FORBIDDEN @@ -681,11 +680,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 is False or spam is NOT_SPAM: + if spam is False or spam is self.NOT_SPAM: continue elif spam is True: return Codes.FORBIDDEN else: return spam - return NOT_SPAM + return self.NOT_SPAM From a376e609620e678982f8bb56ad5fb2e30d4c6ccd Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 2 Jun 2022 16:54:59 +0200 Subject: [PATCH 07/14] WIP: Making Literal 3.7-proof --- synapse/events/spamcheck.py | 4 +++- tests/rest/client/test_rooms.py | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 7f552168a044..b84bbdd49ef3 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -23,12 +23,14 @@ Collection, Dict, List, - Literal, Optional, Tuple, Union, ) +# `Literal` appears with Python 3.8. +from typing_extensions import Literal + from synapse.api.errors import Codes from synapse.rest.media.v1._base import FileInfo from synapse.rest.media.v1.media_storage import ReadableFileWrapper diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index f69eda180a1c..12e397de0978 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -18,10 +18,13 @@ """Tests REST events for /rooms paths.""" import json -from typing import Any, Dict, Iterable, List, Literal, Optional, Union +from typing import Any, Dict, Iterable, List, Optional, Union from unittest.mock import Mock, call from urllib import parse as urlparse +# `Literal` appears with Python 3.8. +from typing_extensions import Literal + from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin From 8166bd58ea472f31d7ce7f4bfee3d228d0e02b70 Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 6 Jun 2022 15:55:06 +0200 Subject: [PATCH 08/14] 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[Literal["NOT_SPAM"], Codes]`. This is a followup to #12703, #12808, #12846 and builds towards giving the spam-checker API the ability to inform users of *why* their event or operation has been rejected. WIP: Applying feedback. --- changelog.d/12857.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12857.feature b/changelog.d/12857.feature index 6f5acbcbda36..578095e48f59 100644 --- a/changelog.d/12857.feature +++ b/changelog.d/12857.feature @@ -1 +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[Literal["NOT_SPAM"], Codes]`. This is a followup to #12703, #12808, #12846 and builds towards giving the spam-checker API the ability to inform users of *why* their event or operation has been rejected. +Port spam-checker API callbacks to a new, richer API. This is part of an ongoing change to let spam-checker modules inform users of *why* their event or operation has been rejected. From 931676867a7a4a2e2900e193f85e4ff94d33597f Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 6 Jun 2022 16:58:53 +0200 Subject: [PATCH 09/14] WIP: Feedback --- docs/modules/spam_checker_callbacks.md | 26 ++- docs/upgrade.md | 225 +++++++++++++++++++++++++ synapse/events/spamcheck.py | 145 ++++++++++------ synapse/handlers/directory.py | 4 +- synapse/module_api/__init__.py | 2 + synapse/rest/media/v1/media_storage.py | 2 +- tests/rest/client/test_rooms.py | 28 ++- 7 files changed, 356 insertions(+), 76 deletions(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 6ca3e80de74d..91d48d668189 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -49,12 +49,14 @@ Called when a user is trying to join a room. The user is represented by their Ma `!room:example.com`). The module is also given a boolean to indicate whether the user 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. + The callback must return one of: - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still decide to reject it. - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. - typically will not localize the error message to the user's preferred locale. - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. @@ -64,10 +66,6 @@ The value of the first callback that does not return `synapse.module_api.NOT_SPA be used. If this happens, Synapse will not call any of the subsequent implementations of this callback. - -This callback isn't called if the join is performed by a server administrator, or in the -context of a room creation. - ### `user_may_invite` _First introduced in Synapse v1.37.0_ @@ -75,7 +73,7 @@ _First introduced in Synapse v1.37.0_ _Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python -async def user_may_invite(inviter: str, invitee: str, room_id: str) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] +async def user_may_invite(inviter: str, invitee: str, room_id: str) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` Called when processing an invitation. Both inviter and invitee are @@ -87,7 +85,7 @@ The callback must return one of: decide to reject it. - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. - typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. @@ -141,7 +139,7 @@ The callback must return one of: decide to reject it. - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. - typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. @@ -169,7 +167,7 @@ The callback must return one of: decide to reject it. - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. - typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. @@ -188,7 +186,7 @@ _First introduced in Synapse v1.37.0_ _Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python -async def user_may_create_room_alias(user_id: str, room_alias: "synapse.types.RoomAlias") -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] +async def user_may_create_room_alias(user_id: str, room_alias: "synapse.module_api.RoomAlias") -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` Called when trying to associate an alias with an existing room. @@ -198,7 +196,7 @@ The callback must return one of: decide to reject it. - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. - typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. @@ -227,7 +225,7 @@ The callback must return one of: decide to reject it. - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. - typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. @@ -321,7 +319,7 @@ The callback must return one of: decide to reject it. - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code. - typically will not localize the error message to the user's preferred locale. + - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`. - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`. @@ -397,5 +395,5 @@ class ListSpamChecker: if event.sender in self.evil_users: return Codes.FORBIDDEN else: - return "NOT_SPAM" + return synapse.module_api.NOT_SPAM ``` diff --git a/docs/upgrade.md b/docs/upgrade.md index e3c64da17fd8..8ce9ec83fe3a 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -89,6 +89,231 @@ process, for example: dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb ``` +# Upgrading to v1.61.0 + +## New signature for the spam checker callback `user_may_join_room` + +The previous signature has been deprecated. + +Whereas `user_may_join_room` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. + +This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. + +If your module implements `user_may_join_room` as follows: + +```python +async def user_may_join_room(self, user_id: str, room_id: str, is_invited: bool) + if ...: + # Request is spam + return False + # Request is not spam + return True +``` + +you should rewrite it as follows: + +```python +async def user_may_join_room(self, user_id: str, room_id: str, is_invited: bool) + if ...: + # Request is spam, mark it as forbidden (you may use some more precise error + # code if it is useful). + return synapse.module_api.errors.Codes.FORBIDDEN + # Request is not spam, mark it as such. + return synapse.module_api.NOT_SPAM +``` + + +## New signature for the spam checker callback `user_may_invite` + +The previous signature has been deprecated. + +Whereas `user_may_invite` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. + +This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. + +If your module implements `user_may_invite` as follows: + +```python +async def user_may_invite(self, self, inviter_userid: str, invitee_userid: str, room_id: str): + if ...: + # Request is spam + return False + # Request is not spam + return True +``` + +you should rewrite it as follows: + +```python +async def user_may_invite(self, self, inviter_userid: str, invitee_userid: str, room_id: str): + if ...: + # Request is spam, mark it as forbidden (you may use some more precise error + # code if it is useful). + return synapse.module_api.errors.Codes.FORBIDDEN + # Request is not spam, mark it as such. + return synapse.module_api.NOT_SPAM +``` + + +## New signature for the spam checker callback `user_may_send_3pid_invite` + +The previous signature has been deprecated. + +Whereas `user_may_send_3pid_invite` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. + +This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. + +If your module implements `user_may_send_3pid_invite` as follows: + +```python +async def user_may_send_3pid_invite(self, inviter_userid: str, medium: str, address: str, room_id: str): + if ...: + # Request is spam + return False + # Request is not spam + return True +``` + +you should rewrite it as follows: + +```python +async def user_may_send_3pid_invite(self, inviter_userid: str, medium: str, address: str, room_id: str): + if ...: + # Request is spam, mark it as forbidden (you may use some more precise error + # code if it is useful). + return synapse.module_api.errors.Codes.FORBIDDEN + # Request is not spam, mark it as such. + return synapse.module_api.NOT_SPAM +``` + +## New signature for the spam checker callback `user_may_create_room` + +The previous signature has been deprecated. + +Whereas `user_may_create_room` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. + +This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. + +If your module implements `user_may_create_room` as follows: + +```python +async def user_may_create_room(self, userid: str): + if ...: + # Request is spam + return False + # Request is not spam + return True +``` + +you should rewrite it as follows: + +```python +async def user_may_create_room(self, userid: str): + if ...: + # Request is spam, mark it as forbidden (you may use some more precise error + # code if it is useful). + return synapse.module_api.errors.Codes.FORBIDDEN + # Request is not spam, mark it as such. + return synapse.module_api.NOT_SPAM +``` + + +## New signature for the spam checker callback `user_may_create_room_alias` + +The previous signature has been deprecated. + +Whereas `user_may_create_room_alias` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. + +This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. + +If your module implements `user_may_create_room_alias` as follows: + +```python +async def user_may_create_room_alias(self, userid: str, room_alias: RoomAlias): + if ...: + # Request is spam + return False + # Request is not spam + return True +``` + +you should rewrite it as follows: + +```python +async def user_may_create_room_alias(self, userid: str, room_alias: RoomAlias): + if ...: + # Request is spam, mark it as forbidden (you may use some more precise error + # code if it is useful). + return synapse.module_api.errors.Codes.FORBIDDEN + # Request is not spam, mark it as such. + return synapse.module_api.NOT_SPAM +``` + + +## New signature for the spam checker callback `user_may_publish_room` + +The previous signature has been deprecated. + +Whereas `user_may_publish_room` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. + +This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. + +If your module implements `user_may_publish_room` as follows: + +```python +async def user_may_publish_room(self, userid: str, room_id: str): + if ...: + # Request is spam + return False + # Request is not spam + return True +``` + +you should rewrite it as follows: + +```python +async def user_may_publish_room(self, userid: str, room_id: str): + if ...: + # Request is spam, mark it as forbidden (you may use some more precise error + # code if it is useful). + return synapse.module_api.errors.Codes.FORBIDDEN + # Request is not spam, mark it as such. + return synapse.module_api.NOT_SPAM +``` + + +## New signature for the spam checker callback `check_media_file_for_spam` + +The previous signature has been deprecated. + +Whereas `check_media_file_for_spam` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. + +This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. + +If your module implements `check_media_file_for_spam` as follows: + +```python +async def check_media_file_for_spam(self, file_wrapper: ReadableFileWrapper, file_info: FileInfo): + if ...: + # Request is spam + return False + # Request is not spam + return True +``` + +you should rewrite it as follows: + +```python +async def check_media_file_for_spam(self, file_wrapper: ReadableFileWrapper, file_info: FileInfo): + if ...: + # Request is spam, mark it as forbidden (you may use some more precise error + # code if it is useful). + return synapse.module_api.errors.Codes.FORBIDDEN + # Request is not spam, mark it as such. + return synapse.module_api.NOT_SPAM +``` + + # Upgrading to v1.60.0 ## Adding a new unique index to `state_group_edges` could fail if your database is corrupted diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index b84bbdd49ef3..dea0abf72464 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -438,15 +438,19 @@ async def user_may_join_room( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - may_join_room = await delay_cancellation( - callback(user_id, room_id, is_invited) - ) - if may_join_room is True or may_join_room is self.NOT_SPAM: - continue - elif may_join_room is False: - return Codes.FORBIDDEN - else: - return may_join_room + res = await delay_cancellation(callback(user_id, room_id, is_invited)) + # Normalize return values to `Codes` or `"NOT_SPAM"`. + if res is True or res is self.NOT_SPAM: + continue + elif res is False: + return Codes.FORBIDDEN + elif isinstance(res, Codes): + return res + else: + logger.warning( + "Module returned invalid value, rejecting join as spam" + ) + return Codes.FORBIDDEN # No spam-checker has rejected the request, let it pass. return self.NOT_SPAM @@ -468,15 +472,21 @@ async def user_may_invite( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - may_invite = await delay_cancellation( + res = await delay_cancellation( callback(inviter_userid, invitee_userid, room_id) ) - if may_invite is True or may_invite is self.NOT_SPAM: - continue - elif may_invite is False: - return Codes.FORBIDDEN - else: - return may_invite + # Normalize return values to `Codes` or `"NOT_SPAM"`. + if res is True or res is self.NOT_SPAM: + continue + elif res is False: + return Codes.FORBIDDEN + elif isinstance(res, Codes): + return res + else: + logger.warning( + "Module returned invalid value, rejecting invite as spam" + ) + return Codes.FORBIDDEN # No spam-checker has rejected the request, let it pass. return self.NOT_SPAM @@ -502,15 +512,21 @@ async def user_may_send_3pid_invite( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - may_send_3pid_invite = await delay_cancellation( + res = await delay_cancellation( callback(inviter_userid, medium, address, room_id) ) - if may_send_3pid_invite is True or may_send_3pid_invite is self.NOT_SPAM: - continue - elif may_send_3pid_invite is False: - return Codes.FORBIDDEN - else: - return may_send_3pid_invite + # Normalize return values to `Codes` or `"NOT_SPAM"`. + if res is True or res is self.NOT_SPAM: + continue + elif res is False: + return Codes.FORBIDDEN + elif isinstance(res, Codes): + return res + else: + logger.warning( + "Module returned invalid value, rejecting 3pid invite as spam" + ) + return Codes.FORBIDDEN return self.NOT_SPAM @@ -526,13 +542,18 @@ async def user_may_create_room( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - may_create_room = await delay_cancellation(callback(userid)) - if may_create_room is True or may_create_room is self.NOT_SPAM: - continue - elif may_create_room is False: - return Codes.FORBIDDEN - else: - return may_create_room + res = await delay_cancellation(callback(userid)) + if res is True or res is self.NOT_SPAM: + continue + elif res is False: + return Codes.FORBIDDEN + elif isinstance(res, Codes): + return res + else: + logger.warning( + "Module returned invalid value, rejecting room creation as spam" + ) + return Codes.FORBIDDEN return self.NOT_SPAM @@ -550,15 +571,18 @@ async def user_may_create_room_alias( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - may_create_room_alias = await delay_cancellation( - callback(userid, room_alias) - ) - if may_create_room_alias is True or may_create_room_alias is self.NOT_SPAM: - continue - elif may_create_room_alias is False: - return Codes.FORBIDDEN - else: - return may_create_room_alias + res = await delay_cancellation(callback(userid, room_alias)) + if res is True or res is self.NOT_SPAM: + continue + elif res is False: + return Codes.FORBIDDEN + elif isinstance(res, Codes): + return res + else: + logger.warning( + "Module returned invalid value, rejecting room create as spam" + ) + return Codes.FORBIDDEN return self.NOT_SPAM @@ -575,13 +599,18 @@ async def user_may_publish_room( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - may_publish_room = await delay_cancellation(callback(userid, room_id)) - if may_publish_room is True or may_publish_room is self.NOT_SPAM: - continue - elif may_publish_room is False: - return Codes.FORBIDDEN - else: - return may_publish_room + res = await delay_cancellation(callback(userid, room_id)) + if res is True or res is self.NOT_SPAM: + continue + elif res is False: + return Codes.FORBIDDEN + elif isinstance(res, Codes): + return res + else: + logger.warning( + "Module returned invalid value, rejecting room publication as spam" + ) + return Codes.FORBIDDEN return self.NOT_SPAM @@ -662,12 +691,12 @@ async def check_media_file_for_spam( async def check_media_file_for_spam( self, file: ReadableFileWrapper, file_info: FileInfo - ) -> Decision: + ) -> Union[Codes, Literal["NOT_SPAM"]]: buffer = BytesIO() await file.write_chunks_to(buffer.write) if buffer.getvalue() == b"Hello World": - return self.NOT_SPAM + return synapse.module_api.NOT_SPAM return Codes.FORBIDDEN @@ -681,12 +710,18 @@ async def check_media_file_for_spam( with Measure( self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) ): - spam = await delay_cancellation(callback(file_wrapper, file_info)) - if spam is False or spam is self.NOT_SPAM: - continue - elif spam is True: - return Codes.FORBIDDEN - else: - return spam + res = await delay_cancellation(callback(file_wrapper, file_info)) + # Normalize return values to `Codes` or `"NOT_SPAM"`. + if res is False or res is self.NOT_SPAM: + continue + elif res is True: + return Codes.FORBIDDEN + elif isinstance(res, Codes): + return res + else: + logger.warning( + "Module returned invalid value, rejecting media file as spam" + ) + return Codes.FORBIDDEN return self.NOT_SPAM diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index a0146ad83c36..13230f4c81b3 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -144,10 +144,10 @@ async def create_association( spam_check = await self.spam_checker.user_may_create_room_alias( user_id, room_alias ) - if isinstance(spam_check, Codes): + if spam_check != self.spam_checker.NOT_SPAM: raise AuthError( 403, - "This user is not permitted to publish rooms to the room list", + "This user is not permitted to create this alias", spam_check, ) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index a8ad575fcd96..e76f14437c11 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -115,6 +115,7 @@ JsonDict, JsonMapping, Requester, + RoomAlias, StateMap, UserID, UserInfo, @@ -163,6 +164,7 @@ "EventBase", "StateMap", "ProfileInfo", + "RoomAlias", "UserProfile", ] diff --git a/synapse/rest/media/v1/media_storage.py b/synapse/rest/media/v1/media_storage.py index b0669ad0f0e4..200ef254a183 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -153,7 +153,7 @@ 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("Not found", spam_check) + raise SpamMediaException(errcode=spam_check) for provider in self.storage_providers: await provider.store_file(path, file_info) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 12e397de0978..8781bed9da6d 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -683,6 +683,8 @@ def test_post_room_invitees_ratelimit(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. + + In this test, we use the deprecated API in which callbacks return a bool. """ async def user_may_join_room( @@ -707,6 +709,8 @@ async def user_may_join_room( 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. + + 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( @@ -953,6 +957,8 @@ async def user_may_join_room( ) -> bool: return 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=user_may_join_room, spec=lambda *x: None) self.hs.get_spam_checker()._user_may_join_room_callbacks.append(callback_mock) @@ -1001,7 +1007,7 @@ 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] = "NOT_SPAM" + return_value: Union[Literal["NOT_SPAM"], Codes] = synapse.module_api.NOT_SPAM async def user_may_join_room( userid: str, @@ -1010,6 +1016,8 @@ async def user_may_join_room( ) -> Union[Literal["NOT_SPAM"], Codes]: return 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=user_may_join_room, spec=lambda *x: None) self.hs.get_spam_checker()._user_may_join_room_callbacks.append(callback_mock) @@ -2930,8 +2938,13 @@ 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_deprecated(self) -> None: + """ + Test allowing/blocking threepid invites with a spam-check module. + + In this test, we use the deprecated API in which callbacks return a bool. + """ # 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 + # a remote IS. We keep the mock for 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 @@ -2987,8 +3000,12 @@ def test_threepid_invite_spamcheck_deprecated(self) -> None: make_invite_mock.assert_called_once() def test_threepid_invite_spamcheck(self) -> None: + """ + Test allowing/blocking threepid invites with a spam-check module. + + In this test, we use the more recent API in which callbacks return a `Union[Codes, Literal["NOT_SPAM"]]`.""" # 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 + # a remote IS. We keep the mock for 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 @@ -3000,7 +3017,10 @@ def test_threepid_invite_spamcheck(self) -> None: # 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("NOT_SPAM"), spec=lambda *x: None) + mock = Mock( + return_value=make_awaitable(synapse.module_api.NOT_SPAM), + 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. From 2af54872f58b599fc67442bf0e03c3f8a42f82ca Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 9 Jun 2022 07:49:59 +0200 Subject: [PATCH 10/14] WIP: Feedback --- docs/upgrade.md | 210 ++------------------------------ tests/rest/client/test_rooms.py | 4 + 2 files changed, 17 insertions(+), 197 deletions(-) diff --git a/docs/upgrade.md b/docs/upgrade.md index 8ce9ec83fe3a..3ade86b1ac84 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -91,50 +91,26 @@ process, for example: # Upgrading to v1.61.0 -## New signature for the spam checker callback `user_may_join_room` +## New signatures for spam checker callbacks -The previous signature has been deprecated. +As a followup to changes in v1.60.0, the following spam-checker callbacks have changed signature: -Whereas `user_may_join_room` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. +- `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` -This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. +For each of these methods, the previous callback signature has been deprecated. -If your module implements `user_may_join_room` as follows: +Whereas callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. -```python -async def user_may_join_room(self, user_id: str, room_id: str, is_invited: bool) - if ...: - # Request is spam - return False - # Request is not spam - return True -``` - -you should rewrite it as follows: +For instance, if your module implements `user_may_join_room` as follows: ```python async def user_may_join_room(self, user_id: str, room_id: str, is_invited: bool) - if ...: - # Request is spam, mark it as forbidden (you may use some more precise error - # code if it is useful). - return synapse.module_api.errors.Codes.FORBIDDEN - # Request is not spam, mark it as such. - return synapse.module_api.NOT_SPAM -``` - - -## New signature for the spam checker callback `user_may_invite` - -The previous signature has been deprecated. - -Whereas `user_may_invite` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. - -This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. - -If your module implements `user_may_invite` as follows: - -```python -async def user_may_invite(self, self, inviter_userid: str, invitee_userid: str, room_id: str): if ...: # Request is spam return False @@ -145,166 +121,7 @@ async def user_may_invite(self, self, inviter_userid: str, invitee_userid: str, you should rewrite it as follows: ```python -async def user_may_invite(self, self, inviter_userid: str, invitee_userid: str, room_id: str): - if ...: - # Request is spam, mark it as forbidden (you may use some more precise error - # code if it is useful). - return synapse.module_api.errors.Codes.FORBIDDEN - # Request is not spam, mark it as such. - return synapse.module_api.NOT_SPAM -``` - - -## New signature for the spam checker callback `user_may_send_3pid_invite` - -The previous signature has been deprecated. - -Whereas `user_may_send_3pid_invite` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. - -This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. - -If your module implements `user_may_send_3pid_invite` as follows: - -```python -async def user_may_send_3pid_invite(self, inviter_userid: str, medium: str, address: str, room_id: str): - if ...: - # Request is spam - return False - # Request is not spam - return True -``` - -you should rewrite it as follows: - -```python -async def user_may_send_3pid_invite(self, inviter_userid: str, medium: str, address: str, room_id: str): - if ...: - # Request is spam, mark it as forbidden (you may use some more precise error - # code if it is useful). - return synapse.module_api.errors.Codes.FORBIDDEN - # Request is not spam, mark it as such. - return synapse.module_api.NOT_SPAM -``` - -## New signature for the spam checker callback `user_may_create_room` - -The previous signature has been deprecated. - -Whereas `user_may_create_room` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. - -This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. - -If your module implements `user_may_create_room` as follows: - -```python -async def user_may_create_room(self, userid: str): - if ...: - # Request is spam - return False - # Request is not spam - return True -``` - -you should rewrite it as follows: - -```python -async def user_may_create_room(self, userid: str): - if ...: - # Request is spam, mark it as forbidden (you may use some more precise error - # code if it is useful). - return synapse.module_api.errors.Codes.FORBIDDEN - # Request is not spam, mark it as such. - return synapse.module_api.NOT_SPAM -``` - - -## New signature for the spam checker callback `user_may_create_room_alias` - -The previous signature has been deprecated. - -Whereas `user_may_create_room_alias` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. - -This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. - -If your module implements `user_may_create_room_alias` as follows: - -```python -async def user_may_create_room_alias(self, userid: str, room_alias: RoomAlias): - if ...: - # Request is spam - return False - # Request is not spam - return True -``` - -you should rewrite it as follows: - -```python -async def user_may_create_room_alias(self, userid: str, room_alias: RoomAlias): - if ...: - # Request is spam, mark it as forbidden (you may use some more precise error - # code if it is useful). - return synapse.module_api.errors.Codes.FORBIDDEN - # Request is not spam, mark it as such. - return synapse.module_api.NOT_SPAM -``` - - -## New signature for the spam checker callback `user_may_publish_room` - -The previous signature has been deprecated. - -Whereas `user_may_publish_room` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. - -This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. - -If your module implements `user_may_publish_room` as follows: - -```python -async def user_may_publish_room(self, userid: str, room_id: str): - if ...: - # Request is spam - return False - # Request is not spam - return True -``` - -you should rewrite it as follows: - -```python -async def user_may_publish_room(self, userid: str, room_id: str): - if ...: - # Request is spam, mark it as forbidden (you may use some more precise error - # code if it is useful). - return synapse.module_api.errors.Codes.FORBIDDEN - # Request is not spam, mark it as such. - return synapse.module_api.NOT_SPAM -``` - - -## New signature for the spam checker callback `check_media_file_for_spam` - -The previous signature has been deprecated. - -Whereas `check_media_file_for_spam` callbacks used to return `bool`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`. - -This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful. - -If your module implements `check_media_file_for_spam` as follows: - -```python -async def check_media_file_for_spam(self, file_wrapper: ReadableFileWrapper, file_info: FileInfo): - if ...: - # Request is spam - return False - # Request is not spam - return True -``` - -you should rewrite it as follows: - -```python -async def check_media_file_for_spam(self, file_wrapper: ReadableFileWrapper, file_info: FileInfo): +async def user_may_join_room(self, user_id: str, room_id: str, is_invited: bool) if ...: # Request is spam, mark it as forbidden (you may use some more precise error # code if it is useful). @@ -313,7 +130,6 @@ async def check_media_file_for_spam(self, file_wrapper: ReadableFileWrapper, fil return synapse.module_api.NOT_SPAM ``` - # Upgrading to v1.60.0 ## Adding a new unique index to `state_group_edges` could fail if your database is corrupted diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 8781bed9da6d..f22e6dc26d8b 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -945,6 +945,8 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> 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. + + This test uses the deprecated API, in which callbacks return booleans. """ # Register a dummy callback. Make it allow all room joins for now. @@ -1004,6 +1006,8 @@ async def user_may_join_room( 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. + + This test uses the latest API to this day, in which callbacks return `NOT_SPAM` or `Codes`. """ # Register a dummy callback. Make it allow all room joins for now. From 5bde1fae2464a631cecb254880621d3b722285d2 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 9 Jun 2022 07:59:36 +0200 Subject: [PATCH 11/14] WIP: Applied feedback --- changelog.d/12857.feature | 2 +- docs/modules/spam_checker_callbacks.md | 8 ++++---- synapse/handlers/directory.py | 3 ++- synapse/handlers/federation.py | 3 ++- synapse/handlers/room.py | 5 +++-- synapse/handlers/room_member.py | 7 ++++--- synapse/rest/media/v1/media_storage.py | 5 +++-- 7 files changed, 19 insertions(+), 14 deletions(-) diff --git a/changelog.d/12857.feature b/changelog.d/12857.feature index 578095e48f59..ddd1dbe685ca 100644 --- a/changelog.d/12857.feature +++ b/changelog.d/12857.feature @@ -1 +1 @@ -Port spam-checker API callbacks to a new, richer API. This is part of an ongoing change to let spam-checker modules inform users of *why* their event or operation has been rejected. +Port spam-checker API callbacks to a new, richer API. This is part of an ongoing change to let spam-checker modules inform users of the reason their event or operation is rejected. diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 91d48d668189..02e47d6148b6 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -108,7 +108,7 @@ async def user_may_send_3pid_invite( medium: str, address: str, room_id: str, -) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] +) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` Called when processing an invitation using a third-party identifier (also called a 3PID, @@ -157,7 +157,7 @@ _First introduced in Synapse v1.37.0_ _Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python -async def user_may_create_room(user_id: str) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] +async def user_may_create_room(user_id: str) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` Called when processing a room creation request. @@ -186,7 +186,7 @@ _First introduced in Synapse v1.37.0_ _Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python -async def user_may_create_room_alias(user_id: str, room_alias: "synapse.module_api.RoomAlias") -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] +async def user_may_create_room_alias(user_id: str, room_alias: "synapse.module_api.RoomAlias") -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` Called when trying to associate an alias with an existing room. @@ -215,7 +215,7 @@ _First introduced in Synapse v1.37.0_ _Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean is now deprecated._ ```python -async def user_may_publish_room(user_id: str, room_id: str) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] +async def user_may_publish_room(user_id: str, room_id: str) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` Called when trying to publish a room to the homeserver's public rooms directory. diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 13230f4c81b3..fed8a46586d5 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -28,6 +28,7 @@ SynapseError, ) from synapse.appservice import ApplicationService +from synapse.module_api import NOT_SPAM from synapse.storage.databases.main.directory import RoomAliasMapping from synapse.types import JsonDict, Requester, RoomAlias, UserID, get_domain_from_id @@ -436,7 +437,7 @@ async def edit_published_room_list( user_id = requester.user.to_string() spam_check = await self.spam_checker.user_may_publish_room(user_id, room_id) - if isinstance(spam_check, Codes): + if spam_check != NOT_SPAM: raise AuthError( 403, "This user is not permitted to publish rooms to the room list", diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 7e4e2eee53de..24b3679b0279 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -59,6 +59,7 @@ from synapse.http.servlet import assert_params_in_dict from synapse.logging.context import nested_logging_context from synapse.metrics.background_process_metrics import run_as_background_process +from synapse.module_api import NOT_SPAM from synapse.replication.http.federation import ( ReplicationCleanRoomRestServlet, ReplicationStoreRoomOnOutlierMembershipRestServlet, @@ -824,7 +825,7 @@ async def on_invite_request( spam_check = await self.spam_checker.user_may_invite( event.sender, event.state_key, event.room_id ) - if isinstance(spam_check, Codes): + if spam_check != NOT_SPAM: raise SynapseError( 403, "This user is not permitted to send invites to this server/user", diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 76580f6a5811..7e597c6e2f38 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -62,6 +62,7 @@ from synapse.federation.federation_client import InvalidResponseError from synapse.handlers.federation import get_domains_from_state from synapse.handlers.relations import BundledAggregations +from synapse.module_api import NOT_SPAM from synapse.rest.admin._base import assert_user_is_admin from synapse.storage.state import StateFilter from synapse.streams import EventSource @@ -438,7 +439,7 @@ async def clone_existing_room( user_id = requester.user.to_string() spam_check = await self.spam_checker.user_may_create_room(user_id) - if isinstance(spam_check, Codes): + if spam_check != NOT_SPAM: raise SynapseError(403, "You are not permitted to create rooms", spam_check) creation_content: JsonDict = { @@ -728,7 +729,7 @@ async def create_room( if not is_requester_admin: spam_check = await self.spam_checker.user_may_create_room(user_id) - if isinstance(spam_check, Codes): + if spam_check != NOT_SPAM: raise SynapseError( 403, "You are not permitted to create rooms", spam_check ) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 32605bb5bb89..02045bff476e 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -38,6 +38,7 @@ from synapse.events import EventBase from synapse.events.snapshot import EventContext from synapse.handlers.profile import MAX_AVATAR_URL_LEN, MAX_DISPLAYNAME_LEN +from synapse.module_api import NOT_SPAM from synapse.storage.state import StateFilter from synapse.types import ( JsonDict, @@ -706,7 +707,7 @@ async def update_membership_locked( spam_check = await self.spam_checker.user_may_invite( requester.user.to_string(), target_id, room_id ) - if isinstance(spam_check, Codes): + if spam_check != NOT_SPAM: logger.info("Blocking invite due to spam checker") block_invite_code = spam_check @@ -825,7 +826,7 @@ async def update_membership_locked( spam_check = await self.spam_checker.user_may_join_room( target.to_string(), room_id, is_invited=inviter is not None ) - if isinstance(spam_check, Codes): + if spam_check != NOT_SPAM: raise SynapseError(403, "Not allowed to join this room", spam_check) # Check if a remote join should be performed. @@ -1379,7 +1380,7 @@ async def do_3pid_invite( address=address, room_id=room_id, ) - if isinstance(spam_check, Codes): + if spam_check != NOT_SPAM: raise SynapseError(403, "Cannot send threepid invite", spam_check) stream_id = await self._make_and_store_3pid_invite( diff --git a/synapse/rest/media/v1/media_storage.py b/synapse/rest/media/v1/media_storage.py index 200ef254a183..492ba860305c 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -36,8 +36,9 @@ from twisted.internet.interfaces import IConsumer from twisted.protocols.basic import FileSender -from synapse.api.errors import Codes, NotFoundError +from synapse.api.errors import NotFoundError from synapse.logging.context import defer_to_thread, make_deferred_yieldable +from synapse.module_api import NOT_SPAM from synapse.util import Clock from synapse.util.file_consumer import BackgroundFileConsumer @@ -148,7 +149,7 @@ async def finish() -> None: spam_check = await self.spam_checker.check_media_file_for_spam( ReadableFileWrapper(self.clock, fname), file_info ) - if isinstance(spam_check, Codes): + if spam_check != NOT_SPAM: 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 From d6d3d42a7fde49975ba8ead95353d6577df38da5 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 9 Jun 2022 10:43:53 +0200 Subject: [PATCH 12/14] WIP: Resolving circular dependencies --- synapse/events/spamcheck.py | 79 +++++++++++++------------- synapse/rest/media/v1/media_storage.py | 4 +- 2 files changed, 41 insertions(+), 42 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index dea0abf72464..877e7feae389 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -31,7 +31,6 @@ # `Literal` appears with Python 3.8. from typing_extensions import Literal -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 @@ -50,12 +49,12 @@ Awaitable[ Union[ str, - Codes, + "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[Codes, Dict], + Tuple["synapse.api.errors.Codes", Dict], # Deprecated bool, ] @@ -70,7 +69,7 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - Codes, + "synapse.api.errors.Codes", # Deprecated bool, ] @@ -81,7 +80,7 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - Codes, + "synapse.api.errors.Codes", # Deprecated bool, ] @@ -92,7 +91,7 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - Codes, + "synapse.api.errors.Codes", # Deprecated bool, ] @@ -103,7 +102,7 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - Codes, + "synapse.api.errors.Codes", # Deprecated bool, ] @@ -114,7 +113,7 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - Codes, + "synapse.api.errors.Codes", # Deprecated bool, ] @@ -125,7 +124,7 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - Codes, + "synapse.api.errors.Codes", # Deprecated bool, ] @@ -154,7 +153,7 @@ Awaitable[ Union[ Literal["NOT_SPAM"], - Codes, + "synapse.api.errors.Codes", # Deprecated bool, ] @@ -345,7 +344,7 @@ def register_callbacks( async def check_event_for_spam( self, event: "synapse.events.EventBase" - ) -> Union[Tuple[Codes, Dict], str]: + ) -> Union[Tuple["synapse.api.errors.Codes", Dict], 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 @@ -376,7 +375,7 @@ async def check_event_for_spam( elif res is True: # This spam-checker rejects the event with deprecated # return value `True` - return (Codes.FORBIDDEN, {}) + return (synapse.api.errors.Codes.FORBIDDEN, {}) elif not isinstance(res, str): # mypy complains that we can't reach this code because of the # return type in CHECK_EVENT_FOR_SPAM_CALLBACK, but we don't know @@ -422,7 +421,7 @@ async def should_drop_federated_event( async def user_may_join_room( self, user_id: str, room_id: str, is_invited: bool - ) -> Union[Codes, Literal["NOT_SPAM"]]: + ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: """Checks if a given users is allowed to join a room. Not called when a user creates a room. @@ -443,21 +442,21 @@ async def user_may_join_room( if res is True or res is self.NOT_SPAM: continue elif res is False: - return Codes.FORBIDDEN - elif isinstance(res, Codes): + return synapse.api.errors.Codes.FORBIDDEN + elif isinstance(res, synapse.api.errors.Codes): return res else: logger.warning( "Module returned invalid value, rejecting join as spam" ) - return 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[Codes, Literal["NOT_SPAM"]]: + ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: """Checks if a given user may send an invite Args: @@ -479,21 +478,21 @@ async def user_may_invite( if res is True or res is self.NOT_SPAM: continue elif res is False: - return Codes.FORBIDDEN - elif isinstance(res, Codes): + return synapse.api.errors.Codes.FORBIDDEN + elif isinstance(res, synapse.api.errors.Codes): return res else: logger.warning( "Module returned invalid value, rejecting invite as spam" ) - return 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[Codes, Literal["NOT_SPAM"]]: + ) -> Union["synapse.api.errors.Codes", 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,20 +518,20 @@ async def user_may_send_3pid_invite( if res is True or res is self.NOT_SPAM: continue elif res is False: - return Codes.FORBIDDEN - elif isinstance(res, Codes): + return synapse.api.errors.Codes.FORBIDDEN + elif isinstance(res, synapse.api.errors.Codes): return res else: logger.warning( "Module returned invalid value, rejecting 3pid invite as spam" ) - return Codes.FORBIDDEN + return synapse.api.errors.Codes.FORBIDDEN return self.NOT_SPAM async def user_may_create_room( self, userid: str - ) -> Union[Codes, Literal["NOT_SPAM"]]: + ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: """Checks if a given user may create a room Args: @@ -546,20 +545,20 @@ async def user_may_create_room( if res is True or res is self.NOT_SPAM: continue elif res is False: - return Codes.FORBIDDEN - elif isinstance(res, Codes): + return synapse.api.errors.Codes.FORBIDDEN + elif isinstance(res, synapse.api.errors.Codes): return res else: logger.warning( "Module returned invalid value, rejecting room creation as spam" ) - return 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[Codes, Literal["NOT_SPAM"]]: + ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: """Checks if a given user may create a room alias Args: @@ -575,20 +574,20 @@ async def user_may_create_room_alias( if res is True or res is self.NOT_SPAM: continue elif res is False: - return Codes.FORBIDDEN - elif isinstance(res, Codes): + return synapse.api.errors.Codes.FORBIDDEN + elif isinstance(res, synapse.api.errors.Codes): return res else: logger.warning( "Module returned invalid value, rejecting room create as spam" ) - return 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[Codes, Literal["NOT_SPAM"]]: + ) -> Union["synapse.api.errors.Codes", Literal["NOT_SPAM"]]: """Checks if a given user may publish a room to the directory Args: @@ -603,14 +602,14 @@ async def user_may_publish_room( if res is True or res is self.NOT_SPAM: continue elif res is False: - return Codes.FORBIDDEN - elif isinstance(res, Codes): + return synapse.api.errors.Codes.FORBIDDEN + elif isinstance(res, synapse.api.errors.Codes): return res else: logger.warning( "Module returned invalid value, rejecting room publication as spam" ) - return Codes.FORBIDDEN + return synapse.api.errors.Codes.FORBIDDEN return self.NOT_SPAM @@ -678,7 +677,7 @@ async def check_registration_for_spam( async def check_media_file_for_spam( self, file_wrapper: ReadableFileWrapper, file_info: FileInfo - ) -> Union[Codes, Literal["NOT_SPAM"]]: + ) -> Union["synapse.api.errors.Codes", 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,13 +714,13 @@ async def check_media_file_for_spam( if res is False or res is self.NOT_SPAM: continue elif res is True: - return Codes.FORBIDDEN - elif isinstance(res, Codes): + return synapse.api.errors.Codes.FORBIDDEN + elif isinstance(res, synapse.api.errors.Codes): return res else: logger.warning( "Module returned invalid value, rejecting media file as spam" ) - return Codes.FORBIDDEN + return synapse.api.errors.Codes.FORBIDDEN return self.NOT_SPAM diff --git a/synapse/rest/media/v1/media_storage.py b/synapse/rest/media/v1/media_storage.py index 492ba860305c..913741734239 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -36,9 +36,9 @@ from twisted.internet.interfaces import IConsumer from twisted.protocols.basic import FileSender +import synapse from synapse.api.errors import NotFoundError from synapse.logging.context import defer_to_thread, make_deferred_yieldable -from synapse.module_api import NOT_SPAM from synapse.util import Clock from synapse.util.file_consumer import BackgroundFileConsumer @@ -149,7 +149,7 @@ async def finish() -> None: spam_check = await self.spam_checker.check_media_file_for_spam( ReadableFileWrapper(self.clock, fname), file_info ) - if spam_check != NOT_SPAM: + if spam_check != synapse.module_api.NOT_SPAM: 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 From e421c0bef889c1375c5536af0eb2bbe62373d1ca Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 9 Jun 2022 11:08:40 +0200 Subject: [PATCH 13/14] WIP: Oops, missing import --- synapse/events/spamcheck.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 877e7feae389..32712d2042ef 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -31,6 +31,7 @@ # `Literal` appears with Python 3.8. from typing_extensions import Literal +import synapse 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 40aa9feea796575cce47379373c9fc37b4a2a478 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 13 Jun 2022 12:35:20 +0200 Subject: [PATCH 14/14] Update docs/modules/spam_checker_callbacks.md --- docs/modules/spam_checker_callbacks.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 02e47d6148b6..8ca7d5bdbfdc 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -309,7 +309,7 @@ _Changed in Synapse v1.61.0: `synapse.module_api.NOT_SPAM` and `synapse.module_a async def check_media_file_for_spam( file_wrapper: "synapse.rest.media.v1.media_storage.ReadableFileWrapper", file_info: "synapse.rest.media.v1._base.FileInfo", -) -> ["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] +) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` Called when storing a local or remote file.