From 9dc3293e0b3a5cbf6fcc4a0cef7386b531190882 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 7 Jun 2022 07:43:35 -0400 Subject: [PATCH] Consolidate the logic of delete_device/delete_devices. (#12970) By always using delete_devices and sometimes passing a list with a single device ID. Previously these methods had gotten out of sync with each other and it seems there's little benefit to the single-device variant. --- changelog.d/12970.misc | 1 + synapse/handlers/device.py | 33 ++--------------------- synapse/module_api/__init__.py | 2 +- synapse/rest/admin/devices.py | 2 +- synapse/rest/client/devices.py | 4 ++- synapse/rest/client/logout.py | 4 +-- synapse/storage/databases/main/devices.py | 10 ------- tests/handlers/test_device.py | 4 +-- 8 files changed, 12 insertions(+), 48 deletions(-) create mode 100644 changelog.d/12970.misc diff --git a/changelog.d/12970.misc b/changelog.d/12970.misc new file mode 100644 index 000000000..8f874aa07 --- /dev/null +++ b/changelog.d/12970.misc @@ -0,0 +1 @@ +Remove the `delete_device` method and always call `delete_devices`. diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py index a0cbeedc3..b79c55170 100644 --- a/synapse/handlers/device.py +++ b/synapse/handlers/device.py @@ -397,35 +397,6 @@ class DeviceHandler(DeviceWorkerHandler): for user_id, user_devices in devices.items(): await self.delete_devices(user_id, user_devices) - @trace - async def delete_device(self, user_id: str, device_id: str) -> None: - """Delete the given device - - Args: - user_id: The user to delete the device from. - device_id: The device to delete. - """ - - try: - await self.store.delete_device(user_id, device_id) - except errors.StoreError as e: - if e.code == 404: - # no match - set_tag("error", True) - log_kv( - {"reason": "User doesn't have device id.", "device_id": device_id} - ) - else: - raise - - await self._auth_handler.delete_access_tokens_for_user( - user_id, device_id=device_id - ) - - await self.store.delete_e2e_keys_by_device(user_id=user_id, device_id=device_id) - - await self.notify_device_update(user_id, [device_id]) - @trace async def delete_all_devices_for_user( self, user_id: str, except_device_id: Optional[str] = None @@ -591,7 +562,7 @@ class DeviceHandler(DeviceWorkerHandler): user_id, device_id, device_data ) if old_device_id is not None: - await self.delete_device(user_id, old_device_id) + await self.delete_devices(user_id, [old_device_id]) return device_id async def get_dehydrated_device( @@ -638,7 +609,7 @@ class DeviceHandler(DeviceWorkerHandler): await self.store.update_device(user_id, device_id, old_device["display_name"]) # can't call self.delete_device because that will clobber the # access token so call the storage layer directly - await self.store.delete_device(user_id, old_device_id) + await self.store.delete_devices(user_id, [old_device_id]) await self.store.delete_e2e_keys_by_device( user_id=user_id, device_id=old_device_id ) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index a8ad575fc..30b2aeffd 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -799,7 +799,7 @@ class ModuleApi: if device_id: # delete the device, which will also delete its access tokens yield defer.ensureDeferred( - self._hs.get_device_handler().delete_device(user_id, device_id) + self._hs.get_device_handler().delete_devices(user_id, [device_id]) ) else: # no associated device. Just delete the access token. diff --git a/synapse/rest/admin/devices.py b/synapse/rest/admin/devices.py index cef46ba0d..d93488010 100644 --- a/synapse/rest/admin/devices.py +++ b/synapse/rest/admin/devices.py @@ -80,7 +80,7 @@ class DeviceRestServlet(RestServlet): if u is None: raise NotFoundError("Unknown user") - await self.device_handler.delete_device(target_user.to_string(), device_id) + await self.device_handler.delete_devices(target_user.to_string(), [device_id]) return HTTPStatus.OK, {} async def on_PUT( diff --git a/synapse/rest/client/devices.py b/synapse/rest/client/devices.py index ad6fd6492..6fab10243 100644 --- a/synapse/rest/client/devices.py +++ b/synapse/rest/client/devices.py @@ -147,7 +147,9 @@ class DeviceRestServlet(RestServlet): can_skip_ui_auth=True, ) - await self.device_handler.delete_device(requester.user.to_string(), device_id) + await self.device_handler.delete_devices( + requester.user.to_string(), [device_id] + ) return 200, {} async def on_PUT( diff --git a/synapse/rest/client/logout.py b/synapse/rest/client/logout.py index 193a6951b..23dfa4518 100644 --- a/synapse/rest/client/logout.py +++ b/synapse/rest/client/logout.py @@ -45,8 +45,8 @@ class LogoutRestServlet(RestServlet): access_token = self.auth.get_access_token_from_request(request) await self._auth_handler.delete_access_token(access_token) else: - await self._device_handler.delete_device( - requester.user.to_string(), requester.device_id + await self._device_handler.delete_devices( + requester.user.to_string(), [requester.device_id] ) return 200, {} diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py index d900064c0..71e7863dd 100644 --- a/synapse/storage/databases/main/devices.py +++ b/synapse/storage/databases/main/devices.py @@ -1433,16 +1433,6 @@ class DeviceStore(DeviceWorkerStore, DeviceBackgroundUpdateStore): ) raise StoreError(500, "Problem storing device.") - async def delete_device(self, user_id: str, device_id: str) -> None: - """Delete a device and its device_inbox. - - Args: - user_id: The ID of the user which owns the device - device_id: The ID of the device to delete - """ - - await self.delete_devices(user_id, [device_id]) - async def delete_devices(self, user_id: str, device_ids: List[str]) -> None: """Deletes several devices. diff --git a/tests/handlers/test_device.py b/tests/handlers/test_device.py index 01ea7d2a4..b8b465d35 100644 --- a/tests/handlers/test_device.py +++ b/tests/handlers/test_device.py @@ -154,7 +154,7 @@ class DeviceTestCase(unittest.HomeserverTestCase): self._record_users() # delete the device - self.get_success(self.handler.delete_device(user1, "abc")) + self.get_success(self.handler.delete_devices(user1, ["abc"])) # check the device was deleted self.get_failure(self.handler.get_device(user1, "abc"), NotFoundError) @@ -179,7 +179,7 @@ class DeviceTestCase(unittest.HomeserverTestCase): ) # delete the device - self.get_success(self.handler.delete_device(user1, "abc")) + self.get_success(self.handler.delete_devices(user1, ["abc"])) # check that the device_inbox was deleted res = self.get_success(