From d05140aabf12742083e4e44226ffa1495b5e7b51 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Fri, 18 Feb 2022 11:32:16 +0000 Subject: [PATCH 01/14] Add check_can_deactivate_user --- synapse/events/third_party_rules.py | 26 ++++++++++++++++++++++++++ synapse/handlers/deactivate_account.py | 7 +++++++ synapse/module_api/__init__.py | 2 ++ 3 files changed, 35 insertions(+) diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 1bb8ca7145fd..7b0258f85862 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -38,6 +38,7 @@ ] ON_NEW_EVENT_CALLBACK = Callable[[EventBase, StateMap[EventBase]], Awaitable] +CHECK_CAN_DEACTIVATE_USER_CALLBACK = Callable[[Requester, string], Awaitable[bool]] def load_legacy_third_party_event_rules(hs: "HomeServer") -> None: """Wrapper that loads a third party event rules module configured using the old @@ -154,6 +155,7 @@ def __init__(self, hs: "HomeServer"): CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = [] self._on_new_event_callbacks: List[ON_NEW_EVENT_CALLBACK] = [] + self._check_can_deactivate_user: List[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = [] def register_third_party_rules_callbacks( self, @@ -166,6 +168,7 @@ def register_third_party_rules_callbacks( CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = None, on_new_event: Optional[ON_NEW_EVENT_CALLBACK] = None, + check_can_deactivate_user: Optional[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = None, ) -> None: """Register callbacks from modules for each hook.""" if check_event_allowed is not None: @@ -187,6 +190,9 @@ def register_third_party_rules_callbacks( if on_new_event is not None: self._on_new_event_callbacks.append(on_new_event) + if _check_can_deactivate_user is not None: + self._check_can_deactivate_user.append(check_can_deactivate_user) + async def check_event_allowed( self, event: EventBase, context: EventContext ) -> Tuple[bool, Optional[dict]]: @@ -353,6 +359,26 @@ async def on_new_event(self, event_id: str) -> None: "Failed to run module API callback %s: %s", callback, e ) + async def check_can_deactivate_user(self, requester: Requester, user_id: str) -> None: + """Intercept requests to deactivate a user to maybe deny it by returning False. + + Args: + requester + user_id: The ID of the room. + + Raises: + ModuleFailureError if a callback raised any exception. + """ + for callback in self._check_can_deactivate_user: + try: + if await callback(requester, user_id) is False: + return False + except Exception as e: + logger.exception( + "Failed to run module API callback %s: %s", callback, e + ) + return True + async def _get_state_map_for_room(self, room_id: str) -> StateMap[EventBase]: """Given a room ID, return the state events of that room. diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 7a13d76a687f..9e3759c166c0 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -41,6 +41,7 @@ def __init__(self, hs: "HomeServer"): # Flag that indicates whether the process to part users from rooms is running self._user_parter_running = False + self._third_party_rules = hs.get_third_party_event_rules() # Start the user parter loop so it can resume parting users from rooms where # it left off (if it has work left to do). @@ -73,6 +74,12 @@ async def deactivate_account( Returns: True if identity server supports removing threepids, otherwise False. """ + + # Check if this user can be deactivated + if not await self._third_party_rules.check_can_deactivate_user(requester, user_id): + raise SynapseError(403, "Deactivation of this user is forbidden", Codes.FORBIDDEN) + + # FIXME: Theoretically there is a race here wherein user resets # password using threepid. diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 8a17b912d36d..358a391cb896 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -278,6 +278,7 @@ def register_third_party_rules_callbacks( CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = None, on_new_event: Optional[ON_NEW_EVENT_CALLBACK] = None, + check_can_deactivate_user: Optional[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = None, ) -> None: """Registers callbacks for third party event rules capabilities. @@ -289,6 +290,7 @@ def register_third_party_rules_callbacks( check_threepid_can_be_invited=check_threepid_can_be_invited, check_visibility_can_be_modified=check_visibility_can_be_modified, on_new_event=on_new_event, + check_can_deactivate_user=check_can_deactivate_user, ) def register_presence_router_callbacks( From 357542c0030fb040d9d119517a3fe6f84be1fef0 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Fri, 18 Feb 2022 11:32:30 +0000 Subject: [PATCH 02/14] Add check_can_shutdown_rooms --- synapse/events/third_party_rules.py | 34 +++++++++++++++++++++++--- synapse/handlers/deactivate_account.py | 11 ++++++--- synapse/handlers/room.py | 8 ++++++ synapse/module_api/__init__.py | 4 +++ 4 files changed, 50 insertions(+), 7 deletions(-) diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 7b0258f85862..1a8712c433a1 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -37,8 +37,9 @@ [str, StateMap[EventBase], str], Awaitable[bool] ] ON_NEW_EVENT_CALLBACK = Callable[[EventBase, StateMap[EventBase]], Awaitable] +CHECK_CAN_SHUTDOWN_ROOM_CALLBACK = Callable[[str, str], Awaitable[bool]] +CHECK_CAN_DEACTIVATE_USER_CALLBACK = Callable[[Requester, str], Awaitable[bool]] -CHECK_CAN_DEACTIVATE_USER_CALLBACK = Callable[[Requester, string], Awaitable[bool]] def load_legacy_third_party_event_rules(hs: "HomeServer") -> None: """Wrapper that loads a third party event rules module configured using the old @@ -155,6 +156,7 @@ def __init__(self, hs: "HomeServer"): CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = [] self._on_new_event_callbacks: List[ON_NEW_EVENT_CALLBACK] = [] + self._check_can_delete_room: List[CHECK_CAN_SHUTDOWN_ROOM_CALLBACK] = [] self._check_can_deactivate_user: List[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = [] def register_third_party_rules_callbacks( @@ -168,6 +170,7 @@ def register_third_party_rules_callbacks( CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = None, on_new_event: Optional[ON_NEW_EVENT_CALLBACK] = None, + check_can_shutdown_room: Optional[CHECK_CAN_SHUTDOWN_ROOM_CALLBACK] = None, check_can_deactivate_user: Optional[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = None, ) -> None: """Register callbacks from modules for each hook.""" @@ -190,7 +193,10 @@ def register_third_party_rules_callbacks( if on_new_event is not None: self._on_new_event_callbacks.append(on_new_event) - if _check_can_deactivate_user is not None: + if check_can_shutdown_room is not None: + self._check_can_delete_room.append(check_can_shutdown_room) + + if check_can_deactivate_user is not None: self._check_can_deactivate_user.append(check_can_deactivate_user) async def check_event_allowed( @@ -359,7 +365,29 @@ async def on_new_event(self, event_id: str) -> None: "Failed to run module API callback %s: %s", callback, e ) - async def check_can_deactivate_user(self, requester: Requester, user_id: str) -> None: + async def check_can_shutdown_room(self, requester: Requester, room_id: str) -> None: + """Intercept requests to delete room to maybe deny it by returning False. + + Args: + requester + room_id: The ID of the room. + + Raises: + ModuleFailureError if a callback raised any exception. + """ + for callback in self._check_can_delete_room: + try: + if await callback(requester, room_id) is False: + return False + except Exception as e: + logger.exception( + "Failed to run module API callback %s: %s", callback, e + ) + return True + + async def check_can_deactivate_user( + self, requester: Requester, user_id: str + ) -> None: """Intercept requests to deactivate a user to maybe deny it by returning False. Args: diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 9e3759c166c0..8344068dc707 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -17,7 +17,7 @@ from synapse.api.errors import SynapseError from synapse.metrics.background_process_metrics import run_as_background_process -from synapse.types import Requester, UserID, create_requester +from synapse.types import Codes, Requester, UserID, create_requester if TYPE_CHECKING: from synapse.server import HomeServer @@ -76,9 +76,12 @@ async def deactivate_account( """ # Check if this user can be deactivated - if not await self._third_party_rules.check_can_deactivate_user(requester, user_id): - raise SynapseError(403, "Deactivation of this user is forbidden", Codes.FORBIDDEN) - + if not await self._third_party_rules.check_can_deactivate_user( + requester, user_id + ): + raise SynapseError( + 403, "Deactivation of this user is forbidden", Codes.FORBIDDEN + ) # FIXME: Theoretically there is a race here wherein user resets # password using threepid. diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index a990727fc580..ba2e9fe3880f 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -1475,6 +1475,7 @@ def __init__(self, hs: "HomeServer"): self.room_member_handler = hs.get_room_member_handler() self._room_creation_handler = hs.get_room_creation_handler() self._replication = hs.get_replication_data_handler() + self._third_party_rules = hs.get_third_party_event_rules() self.event_creation_handler = hs.get_event_creation_handler() self.store = hs.get_datastore() @@ -1548,6 +1549,13 @@ async def shutdown_room( if not RoomID.is_valid(room_id): raise SynapseError(400, "%s is not a legal room ID" % (room_id,)) + if not await self._third_party_rules.check_can_shutdown_room( + requester_user_id, room_id + ): + raise SynapseError( + 403, "Shutdown of this room is forbidden", Codes.FORBIDDEN + ) + # Action the block first (even if the room doesn't exist yet) if block: # This will work even if the room is already blocked, but that is diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 358a391cb896..b61f3d90f25e 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -54,6 +54,8 @@ USER_MAY_SEND_3PID_INVITE_CALLBACK, ) from synapse.events.third_party_rules import ( + CHECK_CAN_DEACTIVATE_USER_CALLBACK, + CHECK_CAN_SHUTDOWN_ROOM_CALLBACK, CHECK_EVENT_ALLOWED_CALLBACK, CHECK_THREEPID_CAN_BE_INVITED_CALLBACK, CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK, @@ -278,6 +280,7 @@ def register_third_party_rules_callbacks( CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = None, on_new_event: Optional[ON_NEW_EVENT_CALLBACK] = None, + check_can_shutdown_room: Optional[CHECK_CAN_SHUTDOWN_ROOM_CALLBACK] = None, check_can_deactivate_user: Optional[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = None, ) -> None: """Registers callbacks for third party event rules capabilities. @@ -290,6 +293,7 @@ def register_third_party_rules_callbacks( check_threepid_can_be_invited=check_threepid_can_be_invited, check_visibility_can_be_modified=check_visibility_can_be_modified, on_new_event=on_new_event, + check_can_shutdown_room=check_can_shutdown_room, check_can_deactivate_user=check_can_deactivate_user, ) From 5884cb522858803ddd8f49bed6105fa016a26ec8 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Fri, 18 Feb 2022 12:42:22 +0000 Subject: [PATCH 03/14] Documentation --- changelog.d/12028.feature | 1 + docs/modules/third_party_rules_callbacks.md | 44 +++++++++++++++++++++ synapse/events/third_party_rules.py | 20 +++++----- 3 files changed, 56 insertions(+), 9 deletions(-) create mode 100644 changelog.d/12028.feature diff --git a/changelog.d/12028.feature b/changelog.d/12028.feature new file mode 100644 index 000000000000..31aefffa77c6 --- /dev/null +++ b/changelog.d/12028.feature @@ -0,0 +1 @@ +Add third party event rules functions `check_can_shutdown_room` and `check_can_deactivate_user`. diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index a3a17096a8f5..aa12ffe044b1 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -148,6 +148,50 @@ deny an incoming event, see [`check_event_for_spam`](spam_checker_callbacks.md#c If multiple modules implement this callback, Synapse runs them all in order. +### `check_can_shutdown_room` + +_First introduced in Synapse v1.5X.0_ + +```python +async def check_can_shutdown_room( + user_id: str + room_id: str, +) -> bool: +``` + +Called when a user requests the shutdown of a room. The module must return a +boolean indicating whether the shutdown can go through. If the callback returns `False`, +the shutdown will not proceed and the caller will see a `M_FOBIDDEN` error. + +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. + +### `check_can_deactivate_user` + +_First introduced in Synapse v1.5X.0_ + +```python +async def check_can_deactivate_user( + requester: "synapse.types.Requester", + user_id: str, +) -> bool: +``` + +Called when a user requests the deactivation of a user. User deactivation may be +performed by an admin or the user themselves, so developers are encouraged to check the +requester when implementing this callback. The module must return a +boolean indicating whether the deactivation can go through. If the callback returns `False`, +the deactivation will not proceed and the caller will see a `M_FOBIDDEN` error. + +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. + + + ## Example The example below is a module that implements the third-party rules callback diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 1a8712c433a1..8e7dd16b40c0 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -156,7 +156,7 @@ def __init__(self, hs: "HomeServer"): CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = [] self._on_new_event_callbacks: List[ON_NEW_EVENT_CALLBACK] = [] - self._check_can_delete_room: List[CHECK_CAN_SHUTDOWN_ROOM_CALLBACK] = [] + self._check_can_shutdown_room: List[CHECK_CAN_SHUTDOWN_ROOM_CALLBACK] = [] self._check_can_deactivate_user: List[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = [] def register_third_party_rules_callbacks( @@ -194,7 +194,7 @@ def register_third_party_rules_callbacks( self._on_new_event_callbacks.append(on_new_event) if check_can_shutdown_room is not None: - self._check_can_delete_room.append(check_can_shutdown_room) + self._check_can_shutdown_room.append(check_can_shutdown_room) if check_can_deactivate_user is not None: self._check_can_deactivate_user.append(check_can_deactivate_user) @@ -365,19 +365,20 @@ async def on_new_event(self, event_id: str) -> None: "Failed to run module API callback %s: %s", callback, e ) - async def check_can_shutdown_room(self, requester: Requester, room_id: str) -> None: - """Intercept requests to delete room to maybe deny it by returning False. + async def check_can_shutdown_room(self, user_id: str, room_id: str) -> bool: + """Intercept requests to shutdown a room. If `False` is returned, the + room should not be shut down. Args: - requester + requester: The ID of the user requesting the shutdown. room_id: The ID of the room. Raises: ModuleFailureError if a callback raised any exception. """ - for callback in self._check_can_delete_room: + for callback in self._check_can_shutdown_room: try: - if await callback(requester, room_id) is False: + if await callback(user_id, room_id) is False: return False except Exception as e: logger.exception( @@ -387,8 +388,9 @@ async def check_can_shutdown_room(self, requester: Requester, room_id: str) -> N async def check_can_deactivate_user( self, requester: Requester, user_id: str - ) -> None: - """Intercept requests to deactivate a user to maybe deny it by returning False. + ) -> bool: + """Intercept requests to deactivate a user. If `False` is returned, the + user should not be deactivated. Args: requester From 19364df4f5b0492777306338863fae37f782dc5c Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Fri, 18 Feb 2022 12:45:57 +0000 Subject: [PATCH 04/14] callbacks, not functions --- changelog.d/12028.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12028.feature b/changelog.d/12028.feature index 31aefffa77c6..9294c1ab2aac 100644 --- a/changelog.d/12028.feature +++ b/changelog.d/12028.feature @@ -1 +1 @@ -Add third party event rules functions `check_can_shutdown_room` and `check_can_deactivate_user`. +Add third party event rules callbacks `check_can_shutdown_room` and `check_can_deactivate_user`. From 6d892071222c9f1f5db05b0788f28d5ca89b1572 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 9 Mar 2022 11:57:26 +0000 Subject: [PATCH 05/14] Various suggested tweaks --- changelog.d/12028.feature | 2 +- docs/modules/third_party_rules_callbacks.md | 4 ++-- synapse/events/third_party_rules.py | 20 +++++++------------- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/changelog.d/12028.feature b/changelog.d/12028.feature index 9294c1ab2aac..5549c8f6fcf6 100644 --- a/changelog.d/12028.feature +++ b/changelog.d/12028.feature @@ -1 +1 @@ -Add third party event rules callbacks `check_can_shutdown_room` and `check_can_deactivate_user`. +Add third-party rules rules callbacks `check_can_shutdown_room` and `check_can_deactivate_user`. diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index aa12ffe044b1..d4db1253b8c2 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -159,7 +159,7 @@ async def check_can_shutdown_room( ) -> bool: ``` -Called when a user requests the shutdown of a room. The module must return a +Called when an admin user requests the shutdown of a room. The module must return a boolean indicating whether the shutdown can go through. If the callback returns `False`, the shutdown will not proceed and the caller will see a `M_FOBIDDEN` error. @@ -179,7 +179,7 @@ async def check_can_deactivate_user( ) -> bool: ``` -Called when a user requests the deactivation of a user. User deactivation may be +Called when the deactivation of a user is requested. User deactivation can be performed by an admin or the user themselves, so developers are encouraged to check the requester when implementing this callback. The module must return a boolean indicating whether the deactivation can go through. If the callback returns `False`, diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 8e7dd16b40c0..9d6a845acdbb 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -156,8 +156,8 @@ def __init__(self, hs: "HomeServer"): CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = [] self._on_new_event_callbacks: List[ON_NEW_EVENT_CALLBACK] = [] - self._check_can_shutdown_room: List[CHECK_CAN_SHUTDOWN_ROOM_CALLBACK] = [] - self._check_can_deactivate_user: List[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = [] + self._check_can_shutdown_room_callbacks: List[CHECK_CAN_SHUTDOWN_ROOM_CALLBACK] = [] + self._check_can_deactivate_user_callbacks: List[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = [] def register_third_party_rules_callbacks( self, @@ -194,10 +194,10 @@ def register_third_party_rules_callbacks( self._on_new_event_callbacks.append(on_new_event) if check_can_shutdown_room is not None: - self._check_can_shutdown_room.append(check_can_shutdown_room) + self._check_can_shutdown_room_callbacks.append(check_can_shutdown_room) if check_can_deactivate_user is not None: - self._check_can_deactivate_user.append(check_can_deactivate_user) + self._check_can_deactivate_user_callbacks.append(check_can_deactivate_user) async def check_event_allowed( self, event: EventBase, context: EventContext @@ -367,16 +367,13 @@ async def on_new_event(self, event_id: str) -> None: async def check_can_shutdown_room(self, user_id: str, room_id: str) -> bool: """Intercept requests to shutdown a room. If `False` is returned, the - room should not be shut down. + room must not be shut down. Args: requester: The ID of the user requesting the shutdown. room_id: The ID of the room. - - Raises: - ModuleFailureError if a callback raised any exception. """ - for callback in self._check_can_shutdown_room: + for callback in self._check_can_shutdown_room_callbacks: try: if await callback(user_id, room_id) is False: return False @@ -395,11 +392,8 @@ async def check_can_deactivate_user( Args: requester user_id: The ID of the room. - - Raises: - ModuleFailureError if a callback raised any exception. """ - for callback in self._check_can_deactivate_user: + for callback in self._check_can_deactivate_user_callbacks: try: if await callback(requester, user_id) is False: return False From e66329f4776ecc07159c5490cf4ee6c334ff8e05 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 9 Mar 2022 12:41:09 +0000 Subject: [PATCH 06/14] Add tests for test_check_can_shutdown_room and test_check_can_deactivate_user --- synapse/events/third_party_rules.py | 8 +- synapse/module_api/__init__.py | 6 - synapse/rest/admin/rooms.py | 9 ++ tests/rest/client/test_third_party_rules.py | 121 ++++++++++++++++++++ 4 files changed, 136 insertions(+), 8 deletions(-) diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 2393eb782235..e69d6fb15837 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -159,8 +159,12 @@ def __init__(self, hs: "HomeServer"): CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = [] self._on_new_event_callbacks: List[ON_NEW_EVENT_CALLBACK] = [] - self._check_can_shutdown_room_callbacks: List[CHECK_CAN_SHUTDOWN_ROOM_CALLBACK] = [] - self._check_can_deactivate_user_callbacks: List[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = [] + self._check_can_shutdown_room_callbacks: List[ + CHECK_CAN_SHUTDOWN_ROOM_CALLBACK + ] = [] + self._check_can_deactivate_user_callbacks: List[ + CHECK_CAN_DEACTIVATE_USER_CALLBACK + ] = [] self._on_profile_update_callbacks: List[ON_PROFILE_UPDATE_CALLBACK] = [] self._on_user_deactivation_status_changed_callbacks: List[ ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 4f06b9391ed2..d735c1d4616e 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -285,15 +285,12 @@ def register_third_party_rules_callbacks( CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = None, on_new_event: Optional[ON_NEW_EVENT_CALLBACK] = None, -<<<<<<< HEAD check_can_shutdown_room: Optional[CHECK_CAN_SHUTDOWN_ROOM_CALLBACK] = None, check_can_deactivate_user: Optional[CHECK_CAN_DEACTIVATE_USER_CALLBACK] = None, -======= on_profile_update: Optional[ON_PROFILE_UPDATE_CALLBACK] = None, on_user_deactivation_status_changed: Optional[ ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK ] = None, ->>>>>>> origin/develop ) -> None: """Registers callbacks for third party event rules capabilities. @@ -305,13 +302,10 @@ def register_third_party_rules_callbacks( check_threepid_can_be_invited=check_threepid_can_be_invited, check_visibility_can_be_modified=check_visibility_can_be_modified, on_new_event=on_new_event, -<<<<<<< HEAD check_can_shutdown_room=check_can_shutdown_room, check_can_deactivate_user=check_can_deactivate_user, -======= on_profile_update=on_profile_update, on_user_deactivation_status_changed=on_user_deactivation_status_changed, ->>>>>>> origin/develop ) def register_presence_router_callbacks( diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index f4736a3dad83..356d6f74d7ef 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -67,6 +67,7 @@ def __init__(self, hs: "HomeServer"): self._auth = hs.get_auth() self._store = hs.get_datastores().main self._pagination_handler = hs.get_pagination_handler() + self._third_party_rules = hs.get_third_party_event_rules() async def on_DELETE( self, request: SynapseRequest, room_id: str @@ -106,6 +107,14 @@ async def on_DELETE( HTTPStatus.BAD_REQUEST, "%s is not a legal room ID" % (room_id,) ) + # Check this here, as otherwise we'll only fail after the background job has been started. + if not await self._third_party_rules.check_can_shutdown_room( + requester.user.to_string(), room_id + ): + raise SynapseError( + 403, "Shutdown of this room is forbidden", Codes.FORBIDDEN + ) + delete_id = self._pagination_handler.start_shutdown_and_purge_room( room_id=room_id, new_room_user_id=content.get("new_room_user_id"), diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 58f1ea11b7da..1016581a6b08 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -775,3 +775,124 @@ def test_on_user_deactivation_status_changed_admin(self) -> None: self.assertEqual(args[0], user_id) self.assertFalse(args[1]) self.assertTrue(args[2]) + + def test_check_can_deactivate_user(self) -> None: + """Tests that the on_user_deactivation_status_changed module callback is called + correctly when processing a user's deactivation. + """ + # Register a mocked callback. + deactivation_mock = Mock(return_value=make_awaitable(False)) + third_party_rules = self.hs.get_third_party_event_rules() + third_party_rules._check_can_deactivate_user_callbacks.append( + deactivation_mock, + ) + + # Register a user that we'll deactivate. + user_id = self.register_user("altan", "password") + tok = self.login("altan", "password") + + # Deactivate that user. + channel = self.make_request( + "POST", + "/_matrix/client/v3/account/deactivate", + { + "auth": { + "type": LoginType.PASSWORD, + "password": "password", + "identifier": { + "type": "m.id.user", + "user": user_id, + }, + }, + "erase": True, + }, + access_token=tok, + ) + + # Check that the deactivation was blocked + self.assertEqual(channel.code, 403, channel.json_body) + + # Check that the mock was called once. + deactivation_mock.assert_called_once() + args = deactivation_mock.call_args[0] + + # Check that the mock was called with the right requester + self.assertEqual(args[0].user.to_string(), user_id) + + # Check that the mock was called with the right user ID + self.assertEqual(args[1], user_id) + + def test_check_can_deactivate_user_admin(self) -> None: + """Tests that the on_user_deactivation_status_changed module callback is called + correctly when processing a user's deactivation triggered by a server admin. + """ + # Register a mocked callback. + deactivation_mock = Mock(return_value=make_awaitable(False)) + third_party_rules = self.hs.get_third_party_event_rules() + third_party_rules._check_can_deactivate_user_callbacks.append( + deactivation_mock, + ) + + # Register an admin user. + admin_user_id = self.register_user("admin", "password", admin=True) + admin_tok = self.login("admin", "password") + + # Register a user that we'll deactivate. + user_id = self.register_user("altan", "password") + + # Deactivate the user. + channel = self.make_request( + "PUT", + "/_synapse/admin/v2/users/%s" % user_id, + {"deactivated": True}, + access_token=admin_tok, + ) + + # Check that the deactivation was blocked + self.assertEqual(channel.code, 403, channel.json_body) + + # Check that the mock was called once. + deactivation_mock.assert_called_once() + args = deactivation_mock.call_args[0] + + # Check that the mock was called with the right requester + self.assertEqual(args[0].user.to_string(), admin_user_id) + + # Check that the mock was called with the right user ID + self.assertEqual(args[1], user_id) + + def test_check_can_shutdown_room(self) -> None: + """Tests that the check_can_shutdown_room module callback is called + correctly when processing an admin's shutdown room request. + """ + # Register a mocked callback. + shutdown_mock = Mock(return_value=make_awaitable(False)) + third_party_rules = self.hs.get_third_party_event_rules() + third_party_rules._check_can_shutdown_room_callbacks.append( + shutdown_mock, + ) + + # Register an admin user. + admin_user_id = self.register_user("admin", "password", admin=True) + admin_tok = self.login("admin", "password") + + # Shutdown the room. + channel = self.make_request( + "DELETE", + "/_synapse/admin/v2/rooms/%s" % self.room_id, + {}, + access_token=admin_tok, + ) + + # Check that the shutdown was blocked + self.assertEqual(channel.code, 403, channel.json_body) + + # Check that the mock was called once. + shutdown_mock.assert_called_once() + args = shutdown_mock.call_args[0] + + # Check that the mock was called with the right user ID + self.assertEqual(args[0], admin_user_id) + + # Check that the mock was called with the right room ID + self.assertEqual(args[1], self.room_id) From 6c439bbf4dbdd5227ea1b08e017634f5f25c1a8a Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 9 Mar 2022 16:18:23 +0000 Subject: [PATCH 07/14] Update check_can_deactivate_user to not take a Requester --- docs/modules/third_party_rules_callbacks.md | 6 +++--- synapse/events/third_party_rules.py | 8 +++++--- synapse/handlers/deactivate_account.py | 2 +- tests/rest/client/test_third_party_rules.py | 16 ++++++++-------- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 947a92bd9358..9c1f88057707 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -161,7 +161,7 @@ async def check_can_shutdown_room( Called when an admin user requests the shutdown of a room. The module must return a boolean indicating whether the shutdown can go through. If the callback returns `False`, -the shutdown will not proceed and the caller will see a `M_FOBIDDEN` error. +the shutdown will not proceed and the caller will see a `M_FORBIDDEN` error. 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 @@ -174,8 +174,8 @@ _First introduced in Synapse v1.5X.0_ ```python async def check_can_deactivate_user( - requester: "synapse.types.Requester", user_id: str, + admin_user_id: str|None, ) -> bool: ``` @@ -183,7 +183,7 @@ Called when the deactivation of a user is requested. User deactivation can be performed by an admin or the user themselves, so developers are encouraged to check the requester when implementing this callback. The module must return a boolean indicating whether the deactivation can go through. If the callback returns `False`, -the deactivation will not proceed and the caller will see a `M_FOBIDDEN` error. +the deactivation will not proceed and the caller will see a `M_FORBIDDEN` error. 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 diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index e69d6fb15837..014ea1c38359 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -39,7 +39,7 @@ ] ON_NEW_EVENT_CALLBACK = Callable[[EventBase, StateMap[EventBase]], Awaitable] CHECK_CAN_SHUTDOWN_ROOM_CALLBACK = Callable[[str, str], Awaitable[bool]] -CHECK_CAN_DEACTIVATE_USER_CALLBACK = Callable[[Requester, str], Awaitable[bool]] +CHECK_CAN_DEACTIVATE_USER_CALLBACK = Callable[[str, str | None], Awaitable[bool]] ON_PROFILE_UPDATE_CALLBACK = Callable[[str, ProfileInfo, bool, bool], Awaitable] ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK = Callable[[str, bool, bool], Awaitable] @@ -403,7 +403,9 @@ async def check_can_shutdown_room(self, user_id: str, room_id: str) -> bool: return True async def check_can_deactivate_user( - self, requester: Requester, user_id: str + self, + user_id: str, + admin_user_id: str | None, ) -> bool: """Intercept requests to deactivate a user. If `False` is returned, the user should not be deactivated. @@ -414,7 +416,7 @@ async def check_can_deactivate_user( """ for callback in self._check_can_deactivate_user_callbacks: try: - if await callback(requester, user_id) is False: + if await callback(user_id, admin_user_id) is False: return False except Exception as e: logger.exception( diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index f997a340a10b..dfc5827f4f93 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -78,7 +78,7 @@ async def deactivate_account( # Check if this user can be deactivated if not await self._third_party_rules.check_can_deactivate_user( - requester, user_id + user_id, requester.user.to_string() if by_admin else None ): raise SynapseError( 403, "Deactivation of this user is forbidden", Codes.FORBIDDEN diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 1016581a6b08..fb1a755b6ea0 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -816,11 +816,11 @@ def test_check_can_deactivate_user(self) -> None: deactivation_mock.assert_called_once() args = deactivation_mock.call_args[0] - # Check that the mock was called with the right requester - self.assertEqual(args[0].user.to_string(), user_id) - # Check that the mock was called with the right user ID - self.assertEqual(args[1], user_id) + self.assertEqual(args[0], user_id) + + # Check that the admin user ID was not provided + self.assertEqual(args[1], None) def test_check_can_deactivate_user_admin(self) -> None: """Tests that the on_user_deactivation_status_changed module callback is called @@ -855,11 +855,11 @@ def test_check_can_deactivate_user_admin(self) -> None: deactivation_mock.assert_called_once() args = deactivation_mock.call_args[0] - # Check that the mock was called with the right requester - self.assertEqual(args[0].user.to_string(), admin_user_id) - # Check that the mock was called with the right user ID - self.assertEqual(args[1], user_id) + self.assertEqual(args[0], user_id) + + # Check that the mock was called with the right admin user id + self.assertEqual(args[1], admin_user_id) def test_check_can_shutdown_room(self) -> None: """Tests that the check_can_shutdown_room module callback is called From c2ce996b616acfe81a035f183c4a1e5862b7604d Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 9 Mar 2022 16:23:48 +0000 Subject: [PATCH 08/14] Fix check_can_shutdown_room docs --- docs/modules/third_party_rules_callbacks.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 9c1f88057707..b9527b1468e9 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -154,8 +154,7 @@ _First introduced in Synapse v1.5X.0_ ```python async def check_can_shutdown_room( - user_id: str - room_id: str, + user_id: str, room_id: str, ) -> bool: ``` From c64b95583f7885e0dd6ac3364f762da0a4e10020 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 9 Mar 2022 16:28:40 +0000 Subject: [PATCH 09/14] Renegade and use `by_admin` instead of `admin_user_id` --- docs/modules/third_party_rules_callbacks.md | 5 ++++- synapse/events/third_party_rules.py | 6 +++--- synapse/handlers/deactivate_account.py | 2 +- tests/rest/client/test_third_party_rules.py | 8 ++++---- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index b9527b1468e9..87632a1f6e3f 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -174,7 +174,7 @@ _First introduced in Synapse v1.5X.0_ ```python async def check_can_deactivate_user( user_id: str, - admin_user_id: str|None, + by_admin: bool, ) -> bool: ``` @@ -184,6 +184,9 @@ requester when implementing this callback. The module must return a boolean indicating whether the deactivation can go through. If the callback returns `False`, the deactivation will not proceed and the caller will see a `M_FORBIDDEN` error. +`by_admin` will be `True` if the request is made by an admin, or `False` if the request +was made by the user themselves. + 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 diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 014ea1c38359..bfca454f510d 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -39,7 +39,7 @@ ] ON_NEW_EVENT_CALLBACK = Callable[[EventBase, StateMap[EventBase]], Awaitable] CHECK_CAN_SHUTDOWN_ROOM_CALLBACK = Callable[[str, str], Awaitable[bool]] -CHECK_CAN_DEACTIVATE_USER_CALLBACK = Callable[[str, str | None], Awaitable[bool]] +CHECK_CAN_DEACTIVATE_USER_CALLBACK = Callable[[str, bool], Awaitable[bool]] ON_PROFILE_UPDATE_CALLBACK = Callable[[str, ProfileInfo, bool, bool], Awaitable] ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK = Callable[[str, bool, bool], Awaitable] @@ -405,7 +405,7 @@ async def check_can_shutdown_room(self, user_id: str, room_id: str) -> bool: async def check_can_deactivate_user( self, user_id: str, - admin_user_id: str | None, + by_admin: bool, ) -> bool: """Intercept requests to deactivate a user. If `False` is returned, the user should not be deactivated. @@ -416,7 +416,7 @@ async def check_can_deactivate_user( """ for callback in self._check_can_deactivate_user_callbacks: try: - if await callback(user_id, admin_user_id) is False: + if await callback(user_id, by_admin) is False: return False except Exception as e: logger.exception( diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index dfc5827f4f93..816e1a6d79c8 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -78,7 +78,7 @@ async def deactivate_account( # Check if this user can be deactivated if not await self._third_party_rules.check_can_deactivate_user( - user_id, requester.user.to_string() if by_admin else None + user_id, by_admin ): raise SynapseError( 403, "Deactivation of this user is forbidden", Codes.FORBIDDEN diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index fb1a755b6ea0..300501555fae 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -819,8 +819,8 @@ def test_check_can_deactivate_user(self) -> None: # Check that the mock was called with the right user ID self.assertEqual(args[0], user_id) - # Check that the admin user ID was not provided - self.assertEqual(args[1], None) + # Check that the request was not made by an admin + self.assertEqual(args[1], False) def test_check_can_deactivate_user_admin(self) -> None: """Tests that the on_user_deactivation_status_changed module callback is called @@ -858,8 +858,8 @@ def test_check_can_deactivate_user_admin(self) -> None: # Check that the mock was called with the right user ID self.assertEqual(args[0], user_id) - # Check that the mock was called with the right admin user id - self.assertEqual(args[1], admin_user_id) + # Check that the mock was made by an admin + self.assertEqual(args[1], True) def test_check_can_shutdown_room(self) -> None: """Tests that the check_can_shutdown_room module callback is called From a3c266758b4b464b10bf1e0200605b377a27f919 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 9 Mar 2022 16:46:44 +0000 Subject: [PATCH 10/14] fix lint --- tests/rest/client/test_third_party_rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 300501555fae..e7de67e3a3b0 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -834,7 +834,7 @@ def test_check_can_deactivate_user_admin(self) -> None: ) # Register an admin user. - admin_user_id = self.register_user("admin", "password", admin=True) + self.register_user("admin", "password", admin=True) admin_tok = self.login("admin", "password") # Register a user that we'll deactivate. From e72024b488112945a287ae194ba399b42467d38e Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 9 Mar 2022 17:50:33 +0000 Subject: [PATCH 11/14] Update docs/modules/third_party_rules_callbacks.md Co-authored-by: Brendan Abolivier --- docs/modules/third_party_rules_callbacks.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 87632a1f6e3f..e90903c672c5 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -184,8 +184,7 @@ requester when implementing this callback. The module must return a boolean indicating whether the deactivation can go through. If the callback returns `False`, the deactivation will not proceed and the caller will see a `M_FORBIDDEN` error. -`by_admin` will be `True` if the request is made by an admin, or `False` if the request -was made by the user themselves. +The module is passed two parameters, `user_id` which is the ID of the user being deactivated, and `by_admin` which is `True` if the request is made by a serve admin, and `False` otherwise. 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 From 98a05d0f24ec6621b9ef72fa59ec9522b90534d8 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 9 Mar 2022 17:50:38 +0000 Subject: [PATCH 12/14] Update docs/modules/third_party_rules_callbacks.md Co-authored-by: Brendan Abolivier --- docs/modules/third_party_rules_callbacks.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index e90903c672c5..766f4bfe2113 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -173,8 +173,7 @@ _First introduced in Synapse v1.5X.0_ ```python async def check_can_deactivate_user( - user_id: str, - by_admin: bool, + user_id: str, by_admin: bool, ) -> bool: ``` From f924cf1b71211d6709abced7140da5e42fe4f59d Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 9 Mar 2022 17:50:43 +0000 Subject: [PATCH 13/14] Update docs/modules/third_party_rules_callbacks.md Co-authored-by: Brendan Abolivier --- docs/modules/third_party_rules_callbacks.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 766f4bfe2113..9afc37a574f1 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -150,7 +150,7 @@ If multiple modules implement this callback, Synapse runs them all in order. ### `check_can_shutdown_room` -_First introduced in Synapse v1.5X.0_ +_First introduced in Synapse v1.55.0_ ```python async def check_can_shutdown_room( From 68c068ad3a54cdbae1bdc03b97a649dd5cd8474c Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 9 Mar 2022 17:50:48 +0000 Subject: [PATCH 14/14] Update docs/modules/third_party_rules_callbacks.md Co-authored-by: Brendan Abolivier --- docs/modules/third_party_rules_callbacks.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 9afc37a574f1..1d3c39967faa 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -169,7 +169,7 @@ any of the subsequent implementations of this callback. ### `check_can_deactivate_user` -_First introduced in Synapse v1.5X.0_ +_First introduced in Synapse v1.55.0_ ```python async def check_can_deactivate_user(