From affe74b483e95862fac672c4b3092e72a9828c9a Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 17 Aug 2021 18:33:19 +0200 Subject: [PATCH 1/5] Remove not needed database updates in modify user admin API --- docs/admin_api/user_admin_api.md | 8 ++- synapse/rest/admin/users.py | 37 ++++++++----- .../storage/databases/main/registration.py | 25 ++++++--- tests/rest/admin/test_user.py | 54 +++++++++++++++++-- 4 files changed, 99 insertions(+), 25 deletions(-) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 6a9335d6ecfc..60dc9139154c 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -21,11 +21,15 @@ It returns a JSON body like the following: "threepids": [ { "medium": "email", - "address": "" + "address": "", + "added_at": 1586458409743, + "validated_at": 1586458409743 }, { "medium": "email", - "address": "" + "address": "", + "added_at": 1586458409743, + "validated_at": 1586458409743 } ], "avatar_url": "", diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 93193b0864f8..573dbec6af17 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -229,7 +229,7 @@ async def on_PUT( if not isinstance(deactivate, bool): raise SynapseError(400, "'deactivated' parameter is not of type boolean") - # convert into List[Tuple[str, str]] + # convert List[Dict[str, str]] into List[Tuple[str, str]] if external_ids is not None: new_external_ids = [] for external_id in external_ids: @@ -237,6 +237,12 @@ async def on_PUT( (external_id["auth_provider"], external_id["external_id"]) ) + # convert List[Dict[str, str]] into List[Tuple[str, str]] + if threepids is not None: + new_threepids = [] + for threepid in threepids: + new_threepids.append((threepid["medium"], threepid["address"])) + if user: # modify user if "displayname" in body: await self.profile_handler.set_displayname( @@ -244,22 +250,29 @@ async def on_PUT( ) if threepids is not None: - # remove old threepids from user - old_threepids = await self.store.user_get_threepids(user_id) - for threepid in old_threepids: + # get changed threepids (added and removed) + # convert List[Dict[str, Any]] into List[Tuple[str, str]] + cur_threepids = [] + for threepid in await self.store.user_get_threepids(user_id): + cur_threepids.append((threepid["medium"], threepid["address"])) + add_threepids = set(new_threepids) - set(cur_threepids) + del_threepids = set(cur_threepids) - set(new_threepids) + + # remove old threepids + for medium, address in del_threepids: try: await self.auth_handler.delete_threepid( - user_id, threepid["medium"], threepid["address"], None + user_id, medium, address, None ) except Exception: logger.exception("Failed to remove threepids") raise SynapseError(500, "Failed to remove threepids") - # add new threepids to user + # add new threepids current_time = self.hs.get_clock().time_msec() - for threepid in threepids: + for medium, address in add_threepids: await self.auth_handler.add_threepid( - user_id, threepid["medium"], threepid["address"], current_time + user_id, medium, address, current_time ) if external_ids is not None: @@ -349,9 +362,9 @@ async def on_PUT( if threepids is not None: current_time = self.hs.get_clock().time_msec() - for threepid in threepids: + for medium, address in new_threepids: await self.auth_handler.add_threepid( - user_id, threepid["medium"], threepid["address"], current_time + user_id, medium, address, current_time ) if ( self.hs.config.email_enable_notifs @@ -363,8 +376,8 @@ async def on_PUT( kind="email", app_id="m.email", app_display_name="Email Notifications", - device_display_name=threepid["address"], - pushkey=threepid["address"], + device_display_name=address, + pushkey=address, lang=None, # We don't know a user's language here data={}, ) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index c67bea81c6b9..54c1bf86f929 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -754,16 +754,18 @@ async def get_user_id_by_threepid(self, medium: str, address: str) -> Optional[s ) return user_id - def get_user_id_by_threepid_txn(self, txn, medium, address): + def get_user_id_by_threepid_txn( + self, txn, medium: str, address: str + ) -> Optional[str]: """Returns user id from threepid Args: txn (cursor): - medium (str): threepid medium e.g. email - address (str): threepid address e.g. me@example.com + medium: threepid medium e.g. email + address: threepid address e.g. me@example.com Returns: - str|None: user id or None if no user id/threepid mapping exists + user id or None if no user id/threepid mapping exists """ ret = self.db_pool.simple_select_one_txn( txn, @@ -776,14 +778,21 @@ def get_user_id_by_threepid_txn(self, txn, medium, address): return ret["user_id"] return None - async def user_add_threepid(self, user_id, medium, address, validated_at, added_at): + async def user_add_threepid( + self, + user_id: str, + medium: str, + address: str, + validated_at: int, + added_at: int, + ) -> None: await self.db_pool.simple_upsert( "user_threepids", {"medium": medium, "address": address}, {"user_id": user_id, "validated_at": validated_at, "added_at": added_at}, ) - async def user_get_threepids(self, user_id): + async def user_get_threepids(self, user_id) -> List[Dict[str, Any]]: return await self.db_pool.simple_select_list( "user_threepids", {"user_id": user_id}, @@ -791,7 +800,9 @@ async def user_get_threepids(self, user_id): "user_get_threepids", ) - async def user_delete_threepid(self, user_id, medium, address) -> None: + async def user_delete_threepid( + self, user_id: str, medium: str, address: str + ) -> None: await self.db_pool.simple_delete( "user_threepids", keyvalues={"user_id": user_id, "medium": medium, "address": address}, diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index ef7727523870..5d1e4f11e9d1 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1431,12 +1431,14 @@ def test_create_user(self): self.assertEqual("Bob's name", channel.json_body["displayname"]) self.assertEqual("email", channel.json_body["threepids"][0]["medium"]) self.assertEqual("bob@bob.bob", channel.json_body["threepids"][0]["address"]) + self.assertEqual(1, len(channel.json_body["threepids"])) self.assertEqual( "external_id1", channel.json_body["external_ids"][0]["external_id"] ) self.assertEqual( "auth_provider1", channel.json_body["external_ids"][0]["auth_provider"] ) + self.assertEqual(1, len(channel.json_body["external_ids"])) self.assertFalse(channel.json_body["admin"]) self.assertEqual("mxc://fibble/wibble", channel.json_body["avatar_url"]) self._check_fields(channel.json_body) @@ -1676,18 +1678,47 @@ def test_set_threepid(self): Test setting threepid for an other user. """ - # Delete old and add new threepid to user + # Add two threepids to user channel = self.make_request( "PUT", self.url_other_user, access_token=self.admin_user_tok, - content={"threepids": [{"medium": "email", "address": "bob3@bob.bob"}]}, + content={ + "threepids": [ + {"medium": "email", "address": "bob1@bob.bob"}, + {"medium": "email", "address": "bob2@bob.bob"}, + ], + }, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("@user:test", channel.json_body["name"]) + self.assertEqual(2, len(channel.json_body["threepids"])) + self.assertEqual("email", channel.json_body["threepids"][0]["medium"]) + self.assertEqual("bob1@bob.bob", channel.json_body["threepids"][0]["address"]) + self.assertEqual("email", channel.json_body["threepids"][1]["medium"]) + self.assertEqual("bob2@bob.bob", channel.json_body["threepids"][1]["address"]) + + # Set a new and remove a threepid + channel = self.make_request( + "PUT", + self.url_other_user, + access_token=self.admin_user_tok, + content={ + "threepids": [ + {"medium": "email", "address": "bob2@bob.bob"}, + {"medium": "email", "address": "bob3@bob.bob"}, + ], + }, ) self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) + self.assertEqual(2, len(channel.json_body["threepids"])) self.assertEqual("email", channel.json_body["threepids"][0]["medium"]) - self.assertEqual("bob3@bob.bob", channel.json_body["threepids"][0]["address"]) + self.assertEqual("bob2@bob.bob", channel.json_body["threepids"][0]["address"]) + self.assertEqual("email", channel.json_body["threepids"][1]["medium"]) + self.assertEqual("bob3@bob.bob", channel.json_body["threepids"][1]["address"]) # Get user channel = self.make_request( @@ -1698,8 +1729,22 @@ def test_set_threepid(self): self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) + self.assertEqual(2, len(channel.json_body["threepids"])) self.assertEqual("email", channel.json_body["threepids"][0]["medium"]) - self.assertEqual("bob3@bob.bob", channel.json_body["threepids"][0]["address"]) + self.assertEqual("bob2@bob.bob", channel.json_body["threepids"][0]["address"]) + self.assertEqual("email", channel.json_body["threepids"][1]["medium"]) + self.assertEqual("bob3@bob.bob", channel.json_body["threepids"][1]["address"]) + + # Remove threepids + channel = self.make_request( + "PUT", + self.url_other_user, + access_token=self.admin_user_tok, + content={"threepids": []}, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("@user:test", channel.json_body["name"]) + self.assertEqual(0, len(channel.json_body["threepids"])) def test_set_external_id(self): """ @@ -1778,6 +1823,7 @@ def test_set_external_id(self): self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) + self.assertEqual(2, len(channel.json_body["external_ids"])) self.assertEqual( channel.json_body["external_ids"], [ From 0afc4ec715f70faff02e1be9c95d55099da3c3a9 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 17 Aug 2021 18:40:14 +0200 Subject: [PATCH 2/5] newsfile --- changelog.d/10627.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10627.misc diff --git a/changelog.d/10627.misc b/changelog.d/10627.misc new file mode 100644 index 000000000000..e6d314976efb --- /dev/null +++ b/changelog.d/10627.misc @@ -0,0 +1 @@ +Remove not needed database updates in modify user admin API. \ No newline at end of file From 48e3bbc51a1706bd0ce37dcf40acc58c6dba64c7 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 17 Aug 2021 20:23:15 +0200 Subject: [PATCH 3/5] update test --- tests/rest/admin/test_user.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 5d1e4f11e9d1..ee204c404b12 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1694,10 +1694,15 @@ def test_set_threepid(self): self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) self.assertEqual(2, len(channel.json_body["threepids"])) - self.assertEqual("email", channel.json_body["threepids"][0]["medium"]) - self.assertEqual("bob1@bob.bob", channel.json_body["threepids"][0]["address"]) - self.assertEqual("email", channel.json_body["threepids"][1]["medium"]) - self.assertEqual("bob2@bob.bob", channel.json_body["threepids"][1]["address"]) + # result does not always have the same sort order, therefore it becomes sorted + sorted_result = sorted( + channel.json_body["threepids"], key=lambda k: k["address"] + ) + self.assertEqual("email", sorted_result[0]["medium"]) + self.assertEqual("bob1@bob.bob", sorted_result[0]["address"]) + self.assertEqual("email", sorted_result[1]["medium"]) + self.assertEqual("bob2@bob.bob", sorted_result[1]["address"]) + self._check_fields(channel.json_body) # Set a new and remove a threepid channel = self.make_request( @@ -1719,6 +1724,7 @@ def test_set_threepid(self): self.assertEqual("bob2@bob.bob", channel.json_body["threepids"][0]["address"]) self.assertEqual("email", channel.json_body["threepids"][1]["medium"]) self.assertEqual("bob3@bob.bob", channel.json_body["threepids"][1]["address"]) + self._check_fields(channel.json_body) # Get user channel = self.make_request( @@ -1734,6 +1740,7 @@ def test_set_threepid(self): self.assertEqual("bob2@bob.bob", channel.json_body["threepids"][0]["address"]) self.assertEqual("email", channel.json_body["threepids"][1]["medium"]) self.assertEqual("bob3@bob.bob", channel.json_body["threepids"][1]["address"]) + self._check_fields(channel.json_body) # Remove threepids channel = self.make_request( @@ -1745,6 +1752,7 @@ def test_set_threepid(self): self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) self.assertEqual(0, len(channel.json_body["threepids"])) + self._check_fields(channel.json_body) def test_set_external_id(self): """ From 1725e94b23bc870e166f602748176e2e198054f8 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Wed, 18 Aug 2021 15:24:45 +0200 Subject: [PATCH 4/5] change from List to Set --- synapse/rest/admin/users.py | 40 ++++++++++--------- .../storage/databases/main/registration.py | 2 +- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 573dbec6af17..0029d45a9cdb 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -229,19 +229,20 @@ async def on_PUT( if not isinstance(deactivate, bool): raise SynapseError(400, "'deactivated' parameter is not of type boolean") - # convert List[Dict[str, str]] into List[Tuple[str, str]] + # convert List[Dict[str, str]] into Set[Tuple[str, str]] if external_ids is not None: new_external_ids = [] - for external_id in external_ids: - new_external_ids.append( - (external_id["auth_provider"], external_id["external_id"]) - ) + new_external_ids = { + (external_id["auth_provider"], external_id["external_id"]) + for external_id in external_ids + } - # convert List[Dict[str, str]] into List[Tuple[str, str]] + # convert List[Dict[str, str]] into Set[Tuple[str, str]] if threepids is not None: - new_threepids = [] - for threepid in threepids: - new_threepids.append((threepid["medium"], threepid["address"])) + new_threepids = { + (threepid["medium"], threepid["address"]) + for threepid in threepids + } if user: # modify user if "displayname" in body: @@ -251,12 +252,13 @@ async def on_PUT( if threepids is not None: # get changed threepids (added and removed) - # convert List[Dict[str, Any]] into List[Tuple[str, str]] - cur_threepids = [] - for threepid in await self.store.user_get_threepids(user_id): - cur_threepids.append((threepid["medium"], threepid["address"])) - add_threepids = set(new_threepids) - set(cur_threepids) - del_threepids = set(cur_threepids) - set(new_threepids) + # convert List[Dict[str, Any]] into Set[Tuple[str, str]] + cur_threepids = { + (threepid["medium"], threepid["address"]) + for threepid in await self.store.user_get_threepids(user_id) + } + add_threepids = new_threepids - cur_threepids + del_threepids = cur_threepids - new_threepids # remove old threepids for medium, address in del_threepids: @@ -277,9 +279,11 @@ async def on_PUT( if external_ids is not None: # get changed external_ids (added and removed) - cur_external_ids = await self.store.get_external_ids_by_user(user_id) - add_external_ids = set(new_external_ids) - set(cur_external_ids) - del_external_ids = set(cur_external_ids) - set(new_external_ids) + cur_external_ids = set( + await self.store.get_external_ids_by_user(user_id) + ) + add_external_ids = new_external_ids - cur_external_ids + del_external_ids = cur_external_ids - new_external_ids # remove old external_ids for auth_provider, external_id in del_external_ids: diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 54c1bf86f929..469dd53e0ce0 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -765,7 +765,7 @@ def get_user_id_by_threepid_txn( address: threepid address e.g. me@example.com Returns: - user id or None if no user id/threepid mapping exists + user id, or None if no user id/threepid mapping exists """ ret = self.db_pool.simple_select_one_txn( txn, From b0fef362e8750fcddb0a48e1f2329705f5782866 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Wed, 18 Aug 2021 15:28:21 +0200 Subject: [PATCH 5/5] lint + mypy --- synapse/rest/admin/users.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 0029d45a9cdb..b54376f98f5c 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -231,7 +231,6 @@ async def on_PUT( # convert List[Dict[str, str]] into Set[Tuple[str, str]] if external_ids is not None: - new_external_ids = [] new_external_ids = { (external_id["auth_provider"], external_id["external_id"]) for external_id in external_ids @@ -240,8 +239,7 @@ async def on_PUT( # convert List[Dict[str, str]] into Set[Tuple[str, str]] if threepids is not None: new_threepids = { - (threepid["medium"], threepid["address"]) - for threepid in threepids + (threepid["medium"], threepid["address"]) for threepid in threepids } if user: # modify user