From 76fe7ec0029a1c6e8693efc2cfced408d9c53b86 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 22 Feb 2022 18:26:37 +0000 Subject: [PATCH 01/11] Add callbacks called on deactivation and profile update --- synapse/events/third_party_rules.py | 50 ++++++++++++++++++++++++-- synapse/handlers/deactivate_account.py | 4 +++ synapse/handlers/profile.py | 10 ++++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 1bb8ca7145fd..252f83b7090e 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -17,6 +17,7 @@ from synapse.api.errors import ModuleFailedException, SynapseError from synapse.events import EventBase from synapse.events.snapshot import EventContext +from synapse.storage.roommember import ProfileInfo from synapse.types import Requester, StateMap from synapse.util.async_helpers import maybe_awaitable @@ -37,6 +38,8 @@ [str, StateMap[EventBase], str], Awaitable[bool] ] ON_NEW_EVENT_CALLBACK = Callable[[EventBase, StateMap[EventBase]], Awaitable] +ON_PROFILE_UPDATE_CALLBACK = Callable[[str, ProfileInfo, bool], Awaitable] +ON_DEACTIVATION_CALLBACK = Callable[[str, bool], Awaitable] def load_legacy_third_party_event_rules(hs: "HomeServer") -> None: @@ -154,6 +157,8 @@ def __init__(self, hs: "HomeServer"): CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = [] self._on_new_event_callbacks: List[ON_NEW_EVENT_CALLBACK] = [] + self._on_profile_update_callbacks: List[ON_PROFILE_UPDATE_CALLBACK] = [] + self._on_deactivation_callbacks: List[ON_DEACTIVATION_CALLBACK] = [] def register_third_party_rules_callbacks( self, @@ -166,6 +171,8 @@ def register_third_party_rules_callbacks( CHECK_VISIBILITY_CAN_BE_MODIFIED_CALLBACK ] = None, on_new_event: Optional[ON_NEW_EVENT_CALLBACK] = None, + on_profile_update: Optional[ON_PROFILE_UPDATE_CALLBACK] = None, + on_deactivation: Optional[ON_DEACTIVATION_CALLBACK] = None, ) -> None: """Register callbacks from modules for each hook.""" if check_event_allowed is not None: @@ -187,6 +194,12 @@ def register_third_party_rules_callbacks( if on_new_event is not None: self._on_new_event_callbacks.append(on_new_event) + if on_profile_update is not None: + self._on_profile_update_callbacks.append(on_profile_update) + + if on_deactivation is not None: + self._on_deactivation_callbacks.append(on_deactivation) + async def check_event_allowed( self, event: EventBase, context: EventContext ) -> Tuple[bool, Optional[dict]]: @@ -334,9 +347,6 @@ async def on_new_event(self, event_id: str) -> None: Args: event_id: The ID of the event. - - Raises: - ModuleFailureError if a callback raised any exception. """ # Bail out early without hitting the store if we don't have any callbacks if len(self._on_new_event_callbacks) == 0: @@ -370,3 +380,37 @@ async def _get_state_map_for_room(self, room_id: str) -> StateMap[EventBase]: state_events[key] = room_state_events[event_id] return state_events + + async def on_profile_update( + self, user_id: str, new_profile: ProfileInfo, by_admin: bool + ) -> None: + """Called after the global profile of a user has been updated. Does not include + per-room profile changes. + + Args: + user_id: The user whose profile was changed. + new_profile: The updated profile for the user. + by_admin: Whether the profile update was performed by a server admin. + """ + for callback in self._on_profile_update_callbacks: + try: + await callback(user_id, new_profile, by_admin) + except Exception as e: + logger.exception( + "Failed to run module API callback %s: %s", callback, e + ) + + async def on_deactivation(self, user_id: str, by_admin: bool) -> None: + """Called after a user has been deactivated. + + Args: + user_id: The deactivated user. + by_admin: Whether the deactivation is performed by a server admin. + """ + for callback in self._on_deactivation_callbacks: + try: + await callback(user_id, by_admin) + except Exception as e: + logger.exception( + "Failed to run module API callback %s: %s", callback, e + ) diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 7a13d76a687f..ba648f1e49d9 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -38,6 +38,7 @@ def __init__(self, hs: "HomeServer"): self._profile_handler = hs.get_profile_handler() self.user_directory_handler = hs.get_user_directory_handler() self._server_name = hs.hostname + self._third_party_rules = hs.get_third_party_event_rules() # Flag that indicates whether the process to part users from rooms is running self._user_parter_running = False @@ -160,6 +161,9 @@ async def deactivate_account( # Remove account data (including ignored users and push rules). await self.store.purge_account_data_for_user(user_id) + # Let modules know the user has been deactivated. + await self._third_party_rules.on_deactivation(user_id, by_admin) + return identity_server_supports_unbinding async def _reject_pending_invites_for_user(self, user_id: str) -> None: diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 36e3ad2ba971..768a025b98fd 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -71,6 +71,8 @@ def __init__(self, hs: "HomeServer"): self.server_name = hs.config.server.server_name + self._third_party_rules = hs.get_third_party_event_rules() + if hs.config.worker.run_background_tasks: self.clock.looping_call( self._update_remote_profile_cache, self.PROFILE_UPDATE_MS @@ -227,6 +229,10 @@ async def set_displayname( target_user.to_string(), profile ) + await self._third_party_rules.on_profile_update( + target_user.to_string(), profile, by_admin + ) + await self._update_join_states(requester, target_user) async def get_avatar_url(self, target_user: UserID) -> Optional[str]: @@ -315,6 +321,10 @@ async def set_avatar_url( target_user.to_string(), profile ) + await self._third_party_rules.on_profile_update( + target_user.to_string(), profile, by_admin + ) + await self._update_join_states(requester, target_user) @cached() From bcbc8c2a536cac263d5964078680f27d87ba4c71 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 22 Feb 2022 18:26:56 +0000 Subject: [PATCH 02/11] Tests --- tests/rest/client/test_third_party_rules.py | 177 +++++++++++++++++++- 1 file changed, 175 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index ac6b86ff6b02..9b2fdd23dea6 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -15,12 +15,12 @@ from typing import TYPE_CHECKING, Dict, Optional, Tuple from unittest.mock import Mock -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, LoginType from synapse.api.errors import SynapseError from synapse.events import EventBase from synapse.events.third_party_rules import load_legacy_third_party_event_rules from synapse.rest import admin -from synapse.rest.client import login, room +from synapse.rest.client import login, room, profile, account from synapse.types import JsonDict, Requester, StateMap from synapse.util.frozenutils import unfreeze @@ -80,6 +80,8 @@ class ThirdPartyRulesTestCase(unittest.FederatingHomeserverTestCase): admin.register_servlets, login.register_servlets, room.register_servlets, + profile.register_servlets, + account.register_servlets, ] def make_homeserver(self, reactor, clock): @@ -530,3 +532,174 @@ def _update_power_levels(self, event_default: int = 0): }, tok=self.tok, ) + + def test_on_profile_update(self): + """Tests that the on_profile_update module callback is correctly called on + profile update. + """ + displayname = "Foo" + avatar_url = "mxc://matrix.org/oWQDvfewxmlRaRCkVbfetyEo" + + # Register a mock callback. + m = Mock(return_value=make_awaitable(None)) + self.hs.get_third_party_event_rules()._on_profile_update_callbacks.append(m) + + # Change the display name. + channel = self.make_request( + "PUT", + "/_matrix/client/v3/profile/%s/displayname" % self.user_id, + {"displayname": displayname}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Check that the callback has been called once for our user. + m.assert_called_once() + args = m.call_args[0] + self.assertEqual(args[0], self.user_id) + + # Test that by_admin is False. + self.assertFalse(args[2]) + + # Check that we've got the right profile data. + profile_info = args[1] + self.assertEqual(profile_info.display_name, displayname) + self.assertIsNone(profile_info.avatar_url) + + # Change the avatar. + channel = self.make_request( + "PUT", + "/_matrix/client/v3/profile/%s/avatar_url" % self.user_id, + {"avatar_url": avatar_url}, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Check that the callback has been called once for our user. + m.assert_called_once() + args = m.call_args[0] + self.assertEqual(args[0], self.user_id) + + # Test that by_admin is False. + self.assertFalse(args[2]) + + # Check that we've got the right profile data. + profile_info = args[1] + self.assertEqual(profile_info.display_name, displayname) + self.assertEqual( + profile_info.avatar_url, avatar_url + ) + + def test_on_profile_update_admin(self): + """Tests that the on_profile_update module callback is correctly called on + profile update triggered by a server admin. + """ + displayname = "Foo" + avatar_url = "mxc://matrix.org/oWQDvfewxmlRaRCkVbfetyEo" + + # Register a mock callback. + m = Mock(return_value=make_awaitable(None)) + self.hs.get_third_party_event_rules()._on_profile_update_callbacks.append(m) + + # Register an admin user. + self.register_user("admin", "password", admin=True) + admin_tok = self.login("admin", "password") + + # Change a user's profile. + channel = self.make_request( + "PUT", + "/_synapse/admin/v2/users/%s" % self.user_id, + {"displayname": displayname, "avatar_url": avatar_url}, + access_token=admin_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Check that the callback has been called twice (since we update the display name + # and avatar separately). + self.assertEqual(m.call_count, 2) + + # Get the arguments for the last call and check it's about the right user. + args = m.call_args[0] + self.assertEqual(args[0], self.user_id) + + # Check that by_admin is True. + self.assertTrue(args[2]) + + # Check that we've got the right profile data. + profile_info = args[1] + self.assertEqual(profile_info.display_name, displayname) + self.assertEqual( + profile_info.avatar_url, avatar_url + ) + + def test_on_deactivation(self): + """Tests that the on_deactivation module callback is called correctly when + processing a user's deactivation. + """ + # Register a mocked callback. + m = Mock(return_value=make_awaitable(None)) + self.hs.get_third_party_event_rules()._on_deactivation_callbacks.append(m) + + # 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, + }, + } + }, + access_token=tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Check that the mock was called once. + m.assert_called_once() + args = m.call_args[0] + + # Check that the mock was called with the right user ID, and with a False + # by_admin flag. + self.assertEqual(args[0], user_id) + self.assertFalse(args[1]) + + def test_on_deactivation_admin(self): + """Tests that the on_deactivation module callback is called correctly when + processing a user's deactivation triggered by a server admin. + """ + # Register a mock callback. + m = Mock(return_value=make_awaitable(None)) + self.hs.get_third_party_event_rules()._on_deactivation_callbacks.append(m) + + # Register an admin user. + 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") + + # Change a user's profile. + channel = self.make_request( + "PUT", + "/_synapse/admin/v2/users/%s" % user_id, + {"deactivated": True}, + access_token=admin_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Check that the mock was called once. + m.assert_called_once() + args = m.call_args[0] + + # Check that the mock was called with the right user ID, and with a True + # by_admin flag. + self.assertEqual(args[0], user_id) + self.assertTrue(args[1]) From ea612f2f8d8ee83e798a506a7c420f554b34223a Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 22 Feb 2022 18:27:10 +0000 Subject: [PATCH 03/11] Docs --- docs/modules/third_party_rules_callbacks.md | 38 +++++++++++++++++++++ synapse/module_api/__init__.py | 1 + 2 files changed, 39 insertions(+) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index a3a17096a8f5..263b8ed2516a 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -148,6 +148,44 @@ 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. +### `on_profile_update` + +_First introduced in Synapse v1.5X.0_ + +```python +async def on_profile_update( + user_id: str, + new_profile: "synapse.module_api.ProfileInfo", + by_admin: bool, +) -> None: +``` + +Called after updating a local user's profile. The update can be triggered either by the +user themselves or a server admin. The update can also be triggered by a user being +deactivated (in which case their display name is set to an empty string (`""`) and the +avatar URL is set to `None`). The module is passed the Matrix ID of the user whose profile +has been updated, their new profile, as well as a boolean that is `True` if the update +was triggered by a server admin (and `False` otherwise). Note that this boolean is also +`True` if the profile change happens as a result of the user logging through Single +Sing-On, and if a server admin updates their own profile. + +If multiple modules implement this callback, Synapse runs them all in order. + +### `on_deactivation` + +_First introduced in Synapse v1.5X.0_ + +```python +async def on_deactivation(user_id: str, by_admin: bool) -> None: +``` + +Called after deactivating a local user. The deactivation can be triggered either by the +user themselves or a server admin. The module is passed the Matrix ID of the user that's +been deactivated, as well as a boolean that is `True` if the deactivation was triggered +by a server admin (and `False` otherwise). + +If multiple modules implement this callback, Synapse runs them all in order. + ## Example The example below is a module that implements the third-party rules callback diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 07020bfb8d5a..49776e0ab613 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -145,6 +145,7 @@ "JsonDict", "EventBase", "StateMap", + "ProfileInfo", ] logger = logging.getLogger(__name__) From 17c372f9c6fadbdfd71b96712e688ac2131fc0d1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 22 Feb 2022 18:38:14 +0000 Subject: [PATCH 04/11] Lint --- tests/rest/client/test_third_party_rules.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 9b2fdd23dea6..fdedd9ddb0b6 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -15,12 +15,12 @@ from typing import TYPE_CHECKING, Dict, Optional, Tuple from unittest.mock import Mock -from synapse.api.constants import EventTypes, Membership, LoginType +from synapse.api.constants import EventTypes, LoginType, Membership from synapse.api.errors import SynapseError from synapse.events import EventBase from synapse.events.third_party_rules import load_legacy_third_party_event_rules from synapse.rest import admin -from synapse.rest.client import login, room, profile, account +from synapse.rest.client import account, login, profile, room from synapse.types import JsonDict, Requester, StateMap from synapse.util.frozenutils import unfreeze @@ -586,9 +586,7 @@ def test_on_profile_update(self): # Check that we've got the right profile data. profile_info = args[1] self.assertEqual(profile_info.display_name, displayname) - self.assertEqual( - profile_info.avatar_url, avatar_url - ) + self.assertEqual(profile_info.avatar_url, avatar_url) def test_on_profile_update_admin(self): """Tests that the on_profile_update module callback is correctly called on @@ -628,9 +626,7 @@ def test_on_profile_update_admin(self): # Check that we've got the right profile data. profile_info = args[1] self.assertEqual(profile_info.display_name, displayname) - self.assertEqual( - profile_info.avatar_url, avatar_url - ) + self.assertEqual(profile_info.avatar_url, avatar_url) def test_on_deactivation(self): """Tests that the on_deactivation module callback is called correctly when From 67beccff341b5092e8742fe77c40d5018a7b4f1c Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 22 Feb 2022 18:48:33 +0000 Subject: [PATCH 05/11] Changelog --- changelog.d/12062.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12062.feature diff --git a/changelog.d/12062.feature b/changelog.d/12062.feature new file mode 100644 index 000000000000..fee5ca3d8d5d --- /dev/null +++ b/changelog.d/12062.feature @@ -0,0 +1 @@ +Add module callbacks to react to user deactivations and profile udpates. From 7344e9ff09135c5aa140604becdac1d018a361b1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 28 Feb 2022 10:52:58 +0000 Subject: [PATCH 06/11] Fix faulty test --- 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 fdedd9ddb0b6..551184bdc5b4 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -576,7 +576,7 @@ def test_on_profile_update(self): self.assertEqual(channel.code, 200, channel.json_body) # Check that the callback has been called once for our user. - m.assert_called_once() + self.assertEqual(m.call_count, 2) args = m.call_args[0] self.assertEqual(args[0], self.user_id) From e1a9d12cbfe3b251429593edc197e5dc974d1711 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 28 Feb 2022 17:49:07 +0100 Subject: [PATCH 07/11] Apply suggestions from code review Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- changelog.d/12062.feature | 2 +- docs/modules/third_party_rules_callbacks.md | 4 ++-- synapse/events/third_party_rules.py | 2 +- tests/rest/client/test_third_party_rules.py | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/changelog.d/12062.feature b/changelog.d/12062.feature index fee5ca3d8d5d..7d5473945173 100644 --- a/changelog.d/12062.feature +++ b/changelog.d/12062.feature @@ -1 +1 @@ -Add module callbacks to react to user deactivations and profile udpates. +Add module callbacks to react to user deactivations and profile updates. diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 263b8ed2516a..4e9093e6b79f 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -166,8 +166,8 @@ deactivated (in which case their display name is set to an empty string (`""`) a avatar URL is set to `None`). The module is passed the Matrix ID of the user whose profile has been updated, their new profile, as well as a boolean that is `True` if the update was triggered by a server admin (and `False` otherwise). Note that this boolean is also -`True` if the profile change happens as a result of the user logging through Single -Sing-On, and if a server admin updates their own profile. +`True` if the profile change happens as a result of the user logging in through Single +Sign-On, or if a server admin updates their own profile. If multiple modules implement this callback, Synapse runs them all in order. diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 252f83b7090e..2206111cdb7f 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -405,7 +405,7 @@ async def on_deactivation(self, user_id: str, by_admin: bool) -> None: Args: user_id: The deactivated user. - by_admin: Whether the deactivation is performed by a server admin. + by_admin: Whether the deactivation was performed by a server admin. """ for callback in self._on_deactivation_callbacks: try: diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 551184bdc5b4..1a35ac04759b 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -535,7 +535,7 @@ def _update_power_levels(self, event_default: int = 0): def test_on_profile_update(self): """Tests that the on_profile_update module callback is correctly called on - profile update. + profile updates. """ displayname = "Foo" avatar_url = "mxc://matrix.org/oWQDvfewxmlRaRCkVbfetyEo" @@ -590,7 +590,7 @@ def test_on_profile_update(self): def test_on_profile_update_admin(self): """Tests that the on_profile_update module callback is correctly called on - profile update triggered by a server admin. + profile updates triggered by a server admin. """ displayname = "Foo" avatar_url = "mxc://matrix.org/oWQDvfewxmlRaRCkVbfetyEo" From 698afcf29a50f644423d587bbb04923249ecd9db Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 28 Feb 2022 17:51:48 +0000 Subject: [PATCH 08/11] Incorporate review --- docs/modules/third_party_rules_callbacks.md | 16 +++++++++-- synapse/events/third_party_rules.py | 7 +++-- synapse/handlers/deactivate_account.py | 8 ++++-- synapse/handlers/profile.py | 8 ++++-- tests/rest/client/test_third_party_rules.py | 32 +++++++++++++++++---- 5 files changed, 56 insertions(+), 15 deletions(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 4e9093e6b79f..7b85823a4350 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -157,6 +157,7 @@ async def on_profile_update( user_id: str, new_profile: "synapse.module_api.ProfileInfo", by_admin: bool, + deactivation: bool, ) -> None: ``` @@ -165,9 +166,18 @@ user themselves or a server admin. The update can also be triggered by a user be deactivated (in which case their display name is set to an empty string (`""`) and the avatar URL is set to `None`). The module is passed the Matrix ID of the user whose profile has been updated, their new profile, as well as a boolean that is `True` if the update -was triggered by a server admin (and `False` otherwise). Note that this boolean is also -`True` if the profile change happens as a result of the user logging in through Single -Sign-On, or if a server admin updates their own profile. +was triggered by a server admin (and `False` otherwise), and a boolean that is `True` if +the update is a result of the user being deactivated. Note that the `by_admin` boolean is +also `True` if the profile change happens as a result of the user logging in through +Single Sign-On, or if a server admin updates their own profile. + +Per-room profile changes do not trigger this callback to be called. Synapse administrators +wishing this callback to be called on every profile change are encouraged to disable +per-room profile globally using the `allow_per_room_profiles` configuration setting in +Synapse's configuration file. +This callback is not called when registering a user, even when setting it through the +[`get_displayname_for_registration`](https://matrix-org.github.io/synapse/latest/modules/password_auth_provider_callbacks.html#get_displayname_for_registration) +module callback. If multiple modules implement this callback, Synapse runs them all in order. diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 2206111cdb7f..364878857016 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -38,7 +38,7 @@ [str, StateMap[EventBase], str], Awaitable[bool] ] ON_NEW_EVENT_CALLBACK = Callable[[EventBase, StateMap[EventBase]], Awaitable] -ON_PROFILE_UPDATE_CALLBACK = Callable[[str, ProfileInfo, bool], Awaitable] +ON_PROFILE_UPDATE_CALLBACK = Callable[[str, ProfileInfo, bool, bool], Awaitable] ON_DEACTIVATION_CALLBACK = Callable[[str, bool], Awaitable] @@ -382,7 +382,7 @@ async def _get_state_map_for_room(self, room_id: str) -> StateMap[EventBase]: return state_events async def on_profile_update( - self, user_id: str, new_profile: ProfileInfo, by_admin: bool + self, user_id: str, new_profile: ProfileInfo, by_admin: bool, deactivation: bool ) -> None: """Called after the global profile of a user has been updated. Does not include per-room profile changes. @@ -391,10 +391,11 @@ async def on_profile_update( user_id: The user whose profile was changed. new_profile: The updated profile for the user. by_admin: Whether the profile update was performed by a server admin. + deactivation: Whether this change was made while deactivating the user. """ for callback in self._on_profile_update_callbacks: try: - await callback(user_id, new_profile, by_admin) + await callback(user_id, new_profile, by_admin, deactivation) except Exception as e: logger.exception( "Failed to run module API callback %s: %s", callback, e diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index ba648f1e49d9..1320f621c4d8 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -136,9 +136,13 @@ async def deactivate_account( if erase_data: user = UserID.from_string(user_id) # Remove avatar URL from this user - await self._profile_handler.set_avatar_url(user, requester, "", by_admin) + await self._profile_handler.set_avatar_url( + user, requester, "", by_admin, deactivation=True + ) # Remove displayname from this user - await self._profile_handler.set_displayname(user, requester, "", by_admin) + await self._profile_handler.set_displayname( + user, requester, "", by_admin, deactivation=True + ) logger.info("Marking %s as erased", user_id) await self.store.mark_user_erased(user_id) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 768a025b98fd..f8364b7e92d0 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -173,6 +173,7 @@ async def set_displayname( requester: Requester, new_displayname: str, by_admin: bool = False, + deactivation: bool = False, ) -> None: """Set the displayname of a user @@ -181,6 +182,7 @@ async def set_displayname( requester: The user attempting to make this change. new_displayname: The displayname to give this user. by_admin: Whether this change was made by an administrator. + deactivation: Whether this change was made while deactivating the user. """ if not self.hs.is_mine(target_user): raise SynapseError(400, "User is not hosted on this homeserver") @@ -230,7 +232,7 @@ async def set_displayname( ) await self._third_party_rules.on_profile_update( - target_user.to_string(), profile, by_admin + target_user.to_string(), profile, by_admin, deactivation ) await self._update_join_states(requester, target_user) @@ -267,6 +269,7 @@ async def set_avatar_url( requester: Requester, new_avatar_url: str, by_admin: bool = False, + deactivation: bool = False, ) -> None: """Set a new avatar URL for a user. @@ -275,6 +278,7 @@ async def set_avatar_url( requester: The user attempting to make this change. new_avatar_url: The avatar URL to give this user. by_admin: Whether this change was made by an administrator. + deactivation: Whether this change was made while deactivating the user. """ if not self.hs.is_mine(target_user): raise SynapseError(400, "User is not hosted on this homeserver") @@ -322,7 +326,7 @@ async def set_avatar_url( ) await self._third_party_rules.on_profile_update( - target_user.to_string(), profile, by_admin + target_user.to_string(), profile, by_admin, deactivation ) await self._update_join_states(requester, target_user) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 1a35ac04759b..e6d1e3ef6c2c 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -560,6 +560,8 @@ def test_on_profile_update(self): # Test that by_admin is False. self.assertFalse(args[2]) + # Test that deactivation is False. + self.assertFalse(args[3]) # Check that we've got the right profile data. profile_info = args[1] @@ -582,6 +584,8 @@ def test_on_profile_update(self): # Test that by_admin is False. self.assertFalse(args[2]) + # Test that deactivation is False. + self.assertFalse(args[3]) # Check that we've got the right profile data. profile_info = args[1] @@ -622,6 +626,8 @@ def test_on_profile_update_admin(self): # Check that by_admin is True. self.assertTrue(args[2]) + # Test that deactivation is False. + self.assertFalse(args[3]) # Check that we've got the right profile data. profile_info = args[1] @@ -633,8 +639,17 @@ def test_on_deactivation(self): processing a user's deactivation. """ # Register a mocked callback. - m = Mock(return_value=make_awaitable(None)) - self.hs.get_third_party_event_rules()._on_deactivation_callbacks.append(m) + deactivation_mock = Mock(return_value=make_awaitable(None)) + self.hs.get_third_party_event_rules()._on_deactivation_callbacks.append( + deactivation_mock, + ) + # Also register a mocked callback for profile updates, to check that the + # deactivation code calls it in a way that let modules know the user is being + # deactivated. + profile_mock = Mock(return_value=make_awaitable(None)) + self.hs.get_third_party_event_rules()._on_profile_update_callbacks.append( + profile_mock, + ) # Register a user that we'll deactivate. user_id = self.register_user("altan", "password") @@ -652,21 +667,28 @@ def test_on_deactivation(self): "type": "m.id.user", "user": user_id, }, - } + }, + "erase": True, }, access_token=tok, ) self.assertEqual(channel.code, 200, channel.json_body) # Check that the mock was called once. - m.assert_called_once() - args = m.call_args[0] + deactivation_mock.assert_called_once() + args = deactivation_mock.call_args[0] # Check that the mock was called with the right user ID, and with a False # by_admin flag. self.assertEqual(args[0], user_id) self.assertFalse(args[1]) + # Check that the profile update callback was called twice (once for the display + # name and once for the avatar URL), and that the "deactivation" boolean is true. + self.assertEqual(profile_mock.call_count, 2) + args = profile_mock.call_args[0] + self.assertTrue(args[3]) + def test_on_deactivation_admin(self): """Tests that the on_deactivation module callback is called correctly when processing a user's deactivation triggered by a server admin. From b7220460ec2d56c3f9d9c8b03d0acfbd6bd3e1a2 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 1 Mar 2022 12:56:19 +0000 Subject: [PATCH 09/11] Incorporate review --- docs/modules/third_party_rules_callbacks.md | 32 ++++++++----- synapse/events/third_party_rules.py | 21 +++++---- synapse/handlers/deactivate_account.py | 10 +++- tests/rest/client/test_third_party_rules.py | 52 +++++++++++++++------ 4 files changed, 80 insertions(+), 35 deletions(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 7b85823a4350..cf94e0a1f9fd 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -165,15 +165,17 @@ Called after updating a local user's profile. The update can be triggered either user themselves or a server admin. The update can also be triggered by a user being deactivated (in which case their display name is set to an empty string (`""`) and the avatar URL is set to `None`). The module is passed the Matrix ID of the user whose profile -has been updated, their new profile, as well as a boolean that is `True` if the update -was triggered by a server admin (and `False` otherwise), and a boolean that is `True` if -the update is a result of the user being deactivated. Note that the `by_admin` boolean is -also `True` if the profile change happens as a result of the user logging in through -Single Sign-On, or if a server admin updates their own profile. +has been updated, their new profile, as well as a `by_admin` boolean that is `True` if the +update was triggered by a server admin (and `False` otherwise), and a `deactivated` +boolean that is `True` if the update is a result of the user being deactivated. + +Note that the `by_admin` boolean is also `True` if the profile change happens as a result +of the user logging in through Single Sign-On, or if a server admin updates their own +profile. Per-room profile changes do not trigger this callback to be called. Synapse administrators wishing this callback to be called on every profile change are encouraged to disable -per-room profile globally using the `allow_per_room_profiles` configuration setting in +per-room profiles globally using the `allow_per_room_profiles` configuration setting in Synapse's configuration file. This callback is not called when registering a user, even when setting it through the [`get_displayname_for_registration`](https://matrix-org.github.io/synapse/latest/modules/password_auth_provider_callbacks.html#get_displayname_for_registration) @@ -181,18 +183,24 @@ module callback. If multiple modules implement this callback, Synapse runs them all in order. -### `on_deactivation` +### `on_user_deactivation_status_changed` _First introduced in Synapse v1.5X.0_ ```python -async def on_deactivation(user_id: str, by_admin: bool) -> None: +async def on_user_deactivation_status_changed( + user_id: str, deactivated: bool, by_admin: bool +) -> None: ``` -Called after deactivating a local user. The deactivation can be triggered either by the -user themselves or a server admin. The module is passed the Matrix ID of the user that's -been deactivated, as well as a boolean that is `True` if the deactivation was triggered -by a server admin (and `False` otherwise). +Called after deactivating a local user, or reactivating them through the admin API. The +deactivation can be triggered either by the user themselves or a server admin. The module +is passed the Matrix ID of the user whose status is changed, as well as a `deactivated` +boolean that is `True` if the user is being deactivated and `False` if they're being +reactivated, and a `by_admin` boolean that is `True` if the deactivation was triggered by +a server admin (and `False` otherwise). This latter `by_admin` boolean is always `True` +if the user is being reactivated, as this operation can only be performed through the +admin API. If multiple modules implement this callback, Synapse runs them all in order. diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index fa3f830fdd21..dd3104faf3ac 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] ON_PROFILE_UPDATE_CALLBACK = Callable[[str, ProfileInfo, bool, bool], Awaitable] -ON_DEACTIVATION_CALLBACK = Callable[[str, bool], Awaitable] +ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK = Callable[[str, bool, bool], Awaitable] def load_legacy_third_party_event_rules(hs: "HomeServer") -> None: @@ -158,7 +158,9 @@ def __init__(self, hs: "HomeServer"): ] = [] self._on_new_event_callbacks: List[ON_NEW_EVENT_CALLBACK] = [] self._on_profile_update_callbacks: List[ON_PROFILE_UPDATE_CALLBACK] = [] - self._on_deactivation_callbacks: List[ON_DEACTIVATION_CALLBACK] = [] + self._on_user_deactivation_status_changed_callbacks: List[ + ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK + ] = [] def register_third_party_rules_callbacks( self, @@ -172,7 +174,7 @@ def register_third_party_rules_callbacks( ] = None, on_new_event: Optional[ON_NEW_EVENT_CALLBACK] = None, on_profile_update: Optional[ON_PROFILE_UPDATE_CALLBACK] = None, - on_deactivation: Optional[ON_DEACTIVATION_CALLBACK] = None, + on_deactivation: Optional[ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK] = None, ) -> None: """Register callbacks from modules for each hook.""" if check_event_allowed is not None: @@ -198,7 +200,7 @@ def register_third_party_rules_callbacks( self._on_profile_update_callbacks.append(on_profile_update) if on_deactivation is not None: - self._on_deactivation_callbacks.append(on_deactivation) + self._on_user_deactivation_status_changed_callbacks.append(on_deactivation) async def check_event_allowed( self, event: EventBase, context: EventContext @@ -401,16 +403,19 @@ async def on_profile_update( "Failed to run module API callback %s: %s", callback, e ) - async def on_deactivation(self, user_id: str, by_admin: bool) -> None: - """Called after a user has been deactivated. + async def on_user_deactivation_status_changed( + self, user_id: str, deactivated: bool, by_admin: bool + ) -> None: + """Called after a user has been deactivated or reactivated. Args: user_id: The deactivated user. + deactivated: Whether the user is now deactivated. by_admin: Whether the deactivation was performed by a server admin. """ - for callback in self._on_deactivation_callbacks: + for callback in self._on_user_deactivation_status_changed_callbacks: try: - await callback(user_id, by_admin) + await callback(user_id, deactivated, by_admin) except Exception as e: logger.exception( "Failed to run module API callback %s: %s", callback, e diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 3d6f5e359f49..76ae768e6ef5 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -166,7 +166,11 @@ async def deactivate_account( await self.store.purge_account_data_for_user(user_id) # Let modules know the user has been deactivated. - await self._third_party_rules.on_deactivation(user_id, by_admin) + await self._third_party_rules.on_user_deactivation_status_changed( + user_id, + True, + by_admin, + ) return identity_server_supports_unbinding @@ -272,6 +276,10 @@ async def activate_account(self, user_id: str) -> None: # Mark the user as active. await self.store.set_user_deactivated_status(user_id, False) + await self._third_party_rules.on_user_deactivation_status_changed( + user_id, False, True + ) + # Add the user to the directory, if necessary. Note that # this must be done after the user is re-activated, because # deactivated users are excluded from the user directory. diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 8beb53f7c3da..bfc04785b7b2 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -634,13 +634,14 @@ def test_on_profile_update_admin(self): self.assertEqual(profile_info.display_name, displayname) self.assertEqual(profile_info.avatar_url, avatar_url) - def test_on_deactivation(self): - """Tests that the on_deactivation module callback is called correctly when - processing a user's deactivation. + def test_on_user_deactivation_status_changed(self): + """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(None)) - self.hs.get_third_party_event_rules()._on_deactivation_callbacks.append( + third_party_rules = self.hs.get_third_party_event_rules() + third_party_rules._on_user_deactivation_status_changed_callbacks.append( deactivation_mock, ) # Also register a mocked callback for profile updates, to check that the @@ -678,10 +679,11 @@ def test_on_deactivation(self): deactivation_mock.assert_called_once() args = deactivation_mock.call_args[0] - # Check that the mock was called with the right user ID, and with a False - # by_admin flag. + # Check that the mock was called with the right user ID, and with a True + # deactivated flag and a False by_admin flag. self.assertEqual(args[0], user_id) - self.assertFalse(args[1]) + self.assertTrue(args[1]) + self.assertFalse(args[2]) # Check that the profile update callback was called twice (once for the display # name and once for the avatar URL), and that the "deactivation" boolean is true. @@ -689,13 +691,15 @@ def test_on_deactivation(self): args = profile_mock.call_args[0] self.assertTrue(args[3]) - def test_on_deactivation_admin(self): - """Tests that the on_deactivation module callback is called correctly when - processing a user's deactivation triggered by a server admin. + def test_on_user_deactivation_status_changed_admin(self): + """Tests that the on_user_deactivation_status_changed module callback is called + correctly when processing a user's deactivation triggered by a server admin as + well as a reactivation. """ # Register a mock callback. m = Mock(return_value=make_awaitable(None)) - self.hs.get_third_party_event_rules()._on_deactivation_callbacks.append(m) + third_party_rules = self.hs.get_third_party_event_rules() + third_party_rules._on_user_deactivation_status_changed_callbacks.append(m) # Register an admin user. self.register_user("admin", "password", admin=True) @@ -704,7 +708,7 @@ def test_on_deactivation_admin(self): # Register a user that we'll deactivate. user_id = self.register_user("altan", "password") - # Change a user's profile. + # Deactivate the user. channel = self.make_request( "PUT", "/_synapse/admin/v2/users/%s" % user_id, @@ -717,7 +721,27 @@ def test_on_deactivation_admin(self): m.assert_called_once() args = m.call_args[0] - # Check that the mock was called with the right user ID, and with a True - # by_admin flag. + # Check that the mock was called with the right user ID, and with True deactivated + # and by_admin flags. self.assertEqual(args[0], user_id) self.assertTrue(args[1]) + self.assertTrue(args[2]) + + # Reactivate the user. + channel = self.make_request( + "PUT", + "/_synapse/admin/v2/users/%s" % user_id, + {"deactivated": False, "password": "hackme"}, + access_token=admin_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Check that the mock was called once. + self.assertEqual(m.call_count, 2) + args = m.call_args[0] + + # Check that the mock was called with the right user ID, and with a False + # deactivated flag and a True by_admin flag. + self.assertEqual(args[0], user_id) + self.assertFalse(args[1]) + self.assertTrue(args[2]) From e3ae931195c32cab32ba96cf9c0180449a1fed32 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 1 Mar 2022 12:58:20 +0000 Subject: [PATCH 10/11] Update changelog --- changelog.d/12062.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12062.feature b/changelog.d/12062.feature index 7d5473945173..46a606709da8 100644 --- a/changelog.d/12062.feature +++ b/changelog.d/12062.feature @@ -1 +1 @@ -Add module callbacks to react to user deactivations and profile updates. +Add module callbacks to react to user deactivation status changes (i.e. deactivations and reactivations) and profile updates. From 6b20ce74a9de7a2c14319bf4736b69d7e474cd57 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 1 Mar 2022 14:27:49 +0000 Subject: [PATCH 11/11] Apply suggestions from code review --- docs/modules/third_party_rules_callbacks.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index cf94e0a1f9fd..09ac838107b8 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. ### `on_profile_update` -_First introduced in Synapse v1.5X.0_ +_First introduced in Synapse v1.54.0_ ```python async def on_profile_update( @@ -185,7 +185,7 @@ If multiple modules implement this callback, Synapse runs them all in order. ### `on_user_deactivation_status_changed` -_First introduced in Synapse v1.5X.0_ +_First introduced in Synapse v1.54.0_ ```python async def on_user_deactivation_status_changed(