From 1da98c80a2498658cdb6e73b50215f9b4de446ab Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 13 Dec 2021 14:37:52 +0000 Subject: [PATCH 1/4] Make get_device return Optional rather than raising --- synapse/storage/databases/main/devices.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py index 838a2a6a3dd0..120d771d9ca1 100644 --- a/synapse/storage/databases/main/devices.py +++ b/synapse/storage/databases/main/devices.py @@ -101,7 +101,9 @@ def count_devices_by_users_txn(txn, user_ids): "count_devices_by_users", count_devices_by_users_txn, user_ids ) - async def get_device(self, user_id: str, device_id: str) -> Dict[str, Any]: + async def get_device( + self, user_id: str, device_id: str + ) -> Optional[Dict[str, Any]]: """Retrieve a device. Only returns devices that are not marked as hidden. @@ -109,15 +111,14 @@ async def get_device(self, user_id: str, device_id: str) -> Dict[str, Any]: user_id: The ID of the user which owns the device device_id: The ID of the device to retrieve Returns: - A dict containing the device information - Raises: - StoreError: if the device is not found + A dict containing the device information, or None if the device does not exist. """ return await self.db_pool.simple_select_one( table="devices", keyvalues={"user_id": user_id, "device_id": device_id, "hidden": False}, retcols=("user_id", "device_id", "display_name"), desc="get_device", + allow_none=True, ) async def get_devices_by_user(self, user_id: str) -> Dict[str, Dict[str, str]]: From f9ad151f04959354527a9abe12059b33fb1f02a5 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 13 Dec 2021 14:45:16 +0000 Subject: [PATCH 2/4] Preserve the original behaviour --- synapse/handlers/auth.py | 4 +--- synapse/handlers/device.py | 10 ++++++---- synapse/rest/admin/devices.py | 2 ++ synapse/rest/client/devices.py | 6 ++++-- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 61607cf2bad7..84724b207c9d 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -997,9 +997,7 @@ async def create_access_token_for_user_id( # really don't want is active access_tokens without a record of the # device, so we double-check it here. if device_id is not None: - try: - await self.store.get_device(user_id, device_id) - except StoreError: + if await self.store.get_device(user_id, device_id) is None: await self.store.delete_access_token(access_token) raise StoreError(400, "Login raced against device deletion") diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py index 82ee11e921e6..766542523218 100644 --- a/synapse/handlers/device.py +++ b/synapse/handlers/device.py @@ -106,10 +106,10 @@ async def get_device(self, user_id: str, device_id: str) -> JsonDict: Raises: errors.NotFoundError: if the device was not found """ - try: - device = await self.store.get_device(user_id, device_id) - except errors.StoreError: - raise errors.NotFoundError + device = await self.store.get_device(user_id, device_id) + if device is None: + raise errors.NotFoundError() + ips = await self.store.get_last_client_ip_by_device(user_id, device_id) _update_device_from_client_ips(device, ips) @@ -602,6 +602,8 @@ async def rehydrate_device( access_token, device_id ) old_device = await self.store.get_device(user_id, old_device_id) + if old_device is None: + raise errors.NotFoundError() 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 diff --git a/synapse/rest/admin/devices.py b/synapse/rest/admin/devices.py index 062a33d28d15..d9905ff560cb 100644 --- a/synapse/rest/admin/devices.py +++ b/synapse/rest/admin/devices.py @@ -63,6 +63,8 @@ async def on_GET( device = await self.device_handler.get_device( target_user.to_string(), device_id ) + if device is None: + raise NotFoundError("No device found") return HTTPStatus.OK, device async def on_DELETE( diff --git a/synapse/rest/client/devices.py b/synapse/rest/client/devices.py index 8566dc5cb594..ad6fd6492baa 100644 --- a/synapse/rest/client/devices.py +++ b/synapse/rest/client/devices.py @@ -17,6 +17,7 @@ from typing import TYPE_CHECKING, Tuple from synapse.api import errors +from synapse.api.errors import NotFoundError from synapse.http.server import HttpServer from synapse.http.servlet import ( RestServlet, @@ -24,10 +25,9 @@ parse_json_object_from_request, ) from synapse.http.site import SynapseRequest +from synapse.rest.client._base import client_patterns, interactive_auth_handler from synapse.types import JsonDict -from ._base import client_patterns, interactive_auth_handler - if TYPE_CHECKING: from synapse.server import HomeServer @@ -116,6 +116,8 @@ async def on_GET( device = await self.device_handler.get_device( requester.user.to_string(), device_id ) + if device is None: + raise NotFoundError("No device found") return 200, device @interactive_auth_handler From 9a1df2e95be56bc5bf778fe29db95931f7900d84 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 13 Dec 2021 14:45:52 +0000 Subject: [PATCH 3/4] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/11565.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11565.misc diff --git a/changelog.d/11565.misc b/changelog.d/11565.misc new file mode 100644 index 000000000000..ff21dd6ee4f3 --- /dev/null +++ b/changelog.d/11565.misc @@ -0,0 +1 @@ +Make `get_device` return None if the device doesn't exist rather than raising an exception. From f9b14510dfa2112a81ae2ee40f58ec983f9880ed Mon Sep 17 00:00:00 2001 From: reivilibre Date: Mon, 13 Dec 2021 15:11:51 +0000 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- changelog.d/11565.misc | 2 +- synapse/storage/databases/main/devices.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/changelog.d/11565.misc b/changelog.d/11565.misc index ff21dd6ee4f3..ddcafd32cbac 100644 --- a/changelog.d/11565.misc +++ b/changelog.d/11565.misc @@ -1 +1 @@ -Make `get_device` return None if the device doesn't exist rather than raising an exception. +Make `get_device` return `None` if the device doesn't exist rather than raising an exception. diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py index 120d771d9ca1..eff825dd2254 100644 --- a/synapse/storage/databases/main/devices.py +++ b/synapse/storage/databases/main/devices.py @@ -111,7 +111,8 @@ async def get_device( user_id: The ID of the user which owns the device device_id: The ID of the device to retrieve Returns: - A dict containing the device information, or None if the device does not exist. + A dict containing the device information, or `None` if the device does not + exist. """ return await self.db_pool.simple_select_one( table="devices",