From db34eb7166bf68469f82c943f2fe4244faefcd4c Mon Sep 17 00:00:00 2001 From: Hanadi Tamimi Date: Sun, 10 Sep 2023 09:46:16 +0200 Subject: [PATCH 1/5] fix: invalidated refresh tokens for rehydrated devices --- synapse/handlers/device.py | 7 +++-- .../storage/databases/main/registration.py | 31 +++++++++++++++++++ tests/handlers/test_device.py | 10 ++++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py index 9d240ad4ee30..84582c2f89d8 100644 --- a/synapse/handlers/device.py +++ b/synapse/handlers/device.py @@ -758,12 +758,13 @@ async def rehydrate_device( # If the dehydrated device was successfully deleted (the device ID # matched the stored dehydrated device), then modify the access - # token to use the dehydrated device's ID and copy the old device - # display name to the dehydrated device, and destroy the old device - # ID + # token and refresh token to use the dehydrated device's ID and + # copy the old device display name to the dehydrated device, + # and destroy the old device ID old_device_id = await self.store.set_device_for_access_token( access_token, device_id ) + await self.store.move_device_refresh_token(old_device_id, device_id) old_device = await self.store.get_device(user_id, old_device_id) if old_device is None: raise errors.NotFoundError() diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 7e85b73e8e3a..05e058e9adbd 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -2312,6 +2312,37 @@ async def add_refresh_token_to_user( return next_id + def _move_device_refresh_token_txn( + self, txn: LoggingTransaction, old_device_id: str, device_id: str + ) -> None: + """Updates rows of old_device_id with current device_id""" + + self.db_pool.simple_update_txn( + txn, + "refresh_tokens", + {"device_id": old_device_id}, + {"device_id": device_id}, + ) + + async def move_device_refresh_token( + self, old_device_id: str, device_id: str + ) -> None: + """Moves refresh tokens from old device to current device + + Args: + old_device_id: The old device. + device_id: The new device ID. + Returns: + None + """ + + await self.db_pool.runInteraction( + "move_device_refresh_token", + self._move_device_refresh_token_txn, + old_device_id, + device_id, + ) + def _set_device_for_access_token_txn( self, txn: LoggingTransaction, token: str, device_id: str ) -> str: diff --git a/tests/handlers/test_device.py b/tests/handlers/test_device.py index 79d327499baa..d023632da700 100644 --- a/tests/handlers/test_device.py +++ b/tests/handlers/test_device.py @@ -461,6 +461,7 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: self.message_handler = hs.get_device_message_handler() self.registration = hs.get_registration_handler() self.auth = hs.get_auth() + self.auth_handler = hs.get_auth_handler() self.store = hs.get_datastores().main return hs @@ -492,6 +493,7 @@ def test_dehydrate_and_rehydrate_device(self) -> None: user_id=user_id, device_id=None, initial_display_name="new device", + should_issue_refresh_token=True, ) ) @@ -522,6 +524,14 @@ def test_dehydrate_and_rehydrate_device(self) -> None: self.assertEqual(user_info.device_id, retrieved_device_id) + # make sure the user device has the refresh token + if _refresh_token: + self.get_success( + self.auth_handler.refresh_token( + _refresh_token, 5 * 60 * 1000, 5 * 60 * 1000 + ) + ) + # make sure the device has the display name that was set from the login res = self.get_success(self.handler.get_device(user_id, retrieved_device_id)) From d1cf1fb18ddb7531b6763aaeb48b5f18c8477ff5 Mon Sep 17 00:00:00 2001 From: Hanadi Tamimi Date: Sun, 10 Sep 2023 09:51:06 +0200 Subject: [PATCH 2/5] docs: add changelog for bugfix --- changelog.d/16288.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16288.bugfix diff --git a/changelog.d/16288.bugfix b/changelog.d/16288.bugfix new file mode 100644 index 000000000000..58890aa60c95 --- /dev/null +++ b/changelog.d/16288.bugfix @@ -0,0 +1 @@ +Fix bug when rehydrated devices do not have the device refresh tokens. Contributed by Hanadi. From bd9b514b0372146b5f8e01d79ea7edb48bda9207 Mon Sep 17 00:00:00 2001 From: Hanadi Tamimi Date: Tue, 12 Sep 2023 16:59:01 +0200 Subject: [PATCH 3/5] fix: use simple update and scope statement with user id --- synapse/handlers/device.py | 2 +- .../storage/databases/main/registration.py | 27 ++++++------------- tests/handlers/test_device.py | 12 ++++----- 3 files changed, 14 insertions(+), 27 deletions(-) diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py index 84582c2f89d8..9cd457d0017b 100644 --- a/synapse/handlers/device.py +++ b/synapse/handlers/device.py @@ -764,7 +764,7 @@ async def rehydrate_device( old_device_id = await self.store.set_device_for_access_token( access_token, device_id ) - await self.store.move_device_refresh_token(old_device_id, device_id) + await self.store.set_device_for_refresh_token(user_id, old_device_id, device_id) old_device = await self.store.get_device(user_id, old_device_id) if old_device is None: raise errors.NotFoundError() diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 05e058e9adbd..949e98078b4d 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -2312,35 +2312,24 @@ async def add_refresh_token_to_user( return next_id - def _move_device_refresh_token_txn( - self, txn: LoggingTransaction, old_device_id: str, device_id: str - ) -> None: - """Updates rows of old_device_id with current device_id""" - - self.db_pool.simple_update_txn( - txn, - "refresh_tokens", - {"device_id": old_device_id}, - {"device_id": device_id}, - ) - - async def move_device_refresh_token( - self, old_device_id: str, device_id: str + async def set_device_for_refresh_token( + self, user_id: str, old_device_id: str, device_id: str ) -> None: """Moves refresh tokens from old device to current device Args: + user_id: The user of the devices. old_device_id: The old device. device_id: The new device ID. Returns: None """ - await self.db_pool.runInteraction( - "move_device_refresh_token", - self._move_device_refresh_token_txn, - old_device_id, - device_id, + await self.db_pool.simple_update( + "refresh_tokens", + {"user_id": user_id, "device_id": old_device_id}, + {"device_id": device_id}, + "set_device_for_refresh_token", ) def _set_device_for_access_token_txn( diff --git a/tests/handlers/test_device.py b/tests/handlers/test_device.py index d023632da700..d4ed068357ae 100644 --- a/tests/handlers/test_device.py +++ b/tests/handlers/test_device.py @@ -488,7 +488,7 @@ def test_dehydrate_and_rehydrate_device(self) -> None: self.assertEqual(device_data, {"device_data": {"foo": "bar"}}) # Create a new login for the user and dehydrated the device - device_id, access_token, _expiration_time, _refresh_token = self.get_success( + device_id, access_token, _expiration_time, refresh_token = self.get_success( self.registration.register_device( user_id=user_id, device_id=None, @@ -525,12 +525,10 @@ def test_dehydrate_and_rehydrate_device(self) -> None: self.assertEqual(user_info.device_id, retrieved_device_id) # make sure the user device has the refresh token - if _refresh_token: - self.get_success( - self.auth_handler.refresh_token( - _refresh_token, 5 * 60 * 1000, 5 * 60 * 1000 - ) - ) + assert refresh_token is not None + self.get_success( + self.auth_handler.refresh_token(refresh_token, 5 * 60 * 1000, 5 * 60 * 1000) + ) # make sure the device has the display name that was set from the login res = self.get_success(self.handler.get_device(user_id, retrieved_device_id)) From 0746bdc74627920f89c34d930821e4c1765f4aa5 Mon Sep 17 00:00:00 2001 From: Hanadi Date: Wed, 13 Sep 2023 06:56:52 +0200 Subject: [PATCH 4/5] docs: update changelog.d/16288.bugfix Co-authored-by: Patrick Cloke --- changelog.d/16288.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/16288.bugfix b/changelog.d/16288.bugfix index 58890aa60c95..f08d10d1f3c5 100644 --- a/changelog.d/16288.bugfix +++ b/changelog.d/16288.bugfix @@ -1 +1 @@ -Fix bug when rehydrated devices do not have the device refresh tokens. Contributed by Hanadi. +Fix bug introduced in Synapse 1.49.0 when using dehydrated devices ([MSC2697](https://github.com/matrix-org/matrix-spec-proposals/pull/2697)) and refresh tokens. Contributed by Hanadi. From 23dd07911ea7b2ae138488f871ba83d0bd339274 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 13 Sep 2023 07:22:20 -0400 Subject: [PATCH 5/5] Use keyword arguments. --- synapse/storage/databases/main/registration.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 949e98078b4d..e34156dc5584 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -2327,9 +2327,9 @@ async def set_device_for_refresh_token( await self.db_pool.simple_update( "refresh_tokens", - {"user_id": user_id, "device_id": old_device_id}, - {"device_id": device_id}, - "set_device_for_refresh_token", + keyvalues={"user_id": user_id, "device_id": old_device_id}, + updatevalues={"device_id": device_id}, + desc="set_device_for_refresh_token", ) def _set_device_for_access_token_txn(