From 333df8fcdeb2bd7c351a53ffacd2fb8a763b1e7b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 1 Feb 2021 15:21:27 +0000 Subject: [PATCH 1/5] Ensure pushers are deleted for deactivated accounts --- synapse/handlers/deactivate_account.py | 5 +++ synapse/storage/databases/main/pusher.py | 41 +++++++++++++++++++ ...elete_pushers_for_deactivated_accounts.sql | 21 ++++++++++ 3 files changed, 67 insertions(+) create mode 100644 synapse/storage/databases/main/schema/delta/59/08delete_pushers_for_deactivated_accounts.sql diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 94f3f3163f11..3886d3124d69 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -120,6 +120,11 @@ async def deactivate_account( await self.store.user_set_password_hash(user_id, None) + # Most of the pushers will have been deleted when we logged out the + # associated devices above, but we still need to delete pushers not + # associated with devices, e.g. email pushers. + await self.store.delete_all_pushers_for_user(user_id) + # Add the user to a table of users pending deactivation (ie. # removal from all the rooms they're a member of) await self.store.add_user_pending_deactivation(user_id) diff --git a/synapse/storage/databases/main/pusher.py b/synapse/storage/databases/main/pusher.py index 7cb69dd6bd71..ba31f1bc999f 100644 --- a/synapse/storage/databases/main/pusher.py +++ b/synapse/storage/databases/main/pusher.py @@ -373,3 +373,44 @@ def delete_pusher_txn(txn, stream_id): await self.db_pool.runInteraction( "delete_pusher", delete_pusher_txn, stream_id ) + + async def delete_all_pushers_for_user(self, user_id: str) -> None: + """Delete all pushers associated with an account.""" + + # We want to generate a row in `deleted_pushers` for each pusher we're + # deleting, so we fetch the list now so we can generate the appropriate + # number of stream IDs. + # + # Note: technically there could be a race here between adding/deleting + # pushers, but a) the worst case if we don't stop a pusher until the + # next restart and b) this is only called when we're deactivating an + # account. + pushers = list(await self.get_pushers_by_user_id(user_id)) + + def delete_pushers_txn(txn, stream_ids): + self._invalidate_cache_and_stream( # type: ignore + txn, self.get_if_user_has_pusher, (user_id,) + ) + + self.db_pool.simple_delete_txn( + txn, + table="pushers", + keyvalues={"user_name": user_id}, + ) + + for stream_id, pusher in zip(stream_ids, pushers): + self.db_pool.simple_insert_txn( + txn, + table="deleted_pushers", + values={ + "stream_id": stream_id, + "app_id": pusher.app_id, + "pushkey": pusher.pushkey, + "user_id": user_id, + }, + ) + + async with self._pushers_id_gen.get_next_mult(len(pushers)) as stream_ids: + await self.db_pool.runInteraction( + "delete_all_pushers_for_user", delete_pushers_txn, stream_ids + ) diff --git a/synapse/storage/databases/main/schema/delta/59/08delete_pushers_for_deactivated_accounts.sql b/synapse/storage/databases/main/schema/delta/59/08delete_pushers_for_deactivated_accounts.sql new file mode 100644 index 000000000000..20ba4abca30f --- /dev/null +++ b/synapse/storage/databases/main/schema/delta/59/08delete_pushers_for_deactivated_accounts.sql @@ -0,0 +1,21 @@ +/* Copyright 2021 The Matrix.org Foundation C.I.C + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + +-- We may not have deleted all pushers for deactivated accounts. Do so now. +-- +-- Note: We don't bother updating the `deleted_pushers` table as it's just use +-- to stop pushers on workers, and that will happen when they get next restarted. +DELETE FROM pushers WHERE user_name IN (SELECT name FROM users WHERE deactivated = 1); From 2e3c1d53ff2afdf5cda6eed997141c89e8c8697d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 1 Feb 2021 15:25:20 +0000 Subject: [PATCH 2/5] Newsfile --- changelog.d/9285.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9285.bugfix diff --git a/changelog.d/9285.bugfix b/changelog.d/9285.bugfix new file mode 100644 index 000000000000..ca3dde15c7db --- /dev/null +++ b/changelog.d/9285.bugfix @@ -0,0 +1 @@ +Fix bug where we deleted correctly delete all pushers for a user when they deactivated their account. From 8bc461004b27866412eebb64f9ad82a2a4e713e1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 24 Feb 2021 17:13:06 +0000 Subject: [PATCH 3/5] Better changelog maybe? --- changelog.d/9285.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/9285.bugfix b/changelog.d/9285.bugfix index ca3dde15c7db..8708e3fe250d 100644 --- a/changelog.d/9285.bugfix +++ b/changelog.d/9285.bugfix @@ -1 +1 @@ -Fix bug where we deleted correctly delete all pushers for a user when they deactivated their account. +Fix bug where a user's pushers were not all deleted when they deactivated their account. From 6824cb7cb3335d8fa01611e4be6b54fef940c34a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 24 Feb 2021 17:26:17 +0000 Subject: [PATCH 4/5] Forgot to save the changelog... --- changelog.d/9285.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/9285.bugfix b/changelog.d/9285.bugfix index 8708e3fe250d..81188c5473eb 100644 --- a/changelog.d/9285.bugfix +++ b/changelog.d/9285.bugfix @@ -1 +1 @@ -Fix bug where a user's pushers were not all deleted when they deactivated their account. +Fix a bug where users' pushers were not all deleted when they deactivated their account. From 06e86a6961674e25deda8d8bf5b259c41ea6ae84 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 25 Feb 2021 13:28:21 +0000 Subject: [PATCH 5/5] Use simple_insert_many --- synapse/storage/databases/main/pusher.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/synapse/storage/databases/main/pusher.py b/synapse/storage/databases/main/pusher.py index ba31f1bc999f..74219cb05ead 100644 --- a/synapse/storage/databases/main/pusher.py +++ b/synapse/storage/databases/main/pusher.py @@ -398,17 +398,19 @@ def delete_pushers_txn(txn, stream_ids): keyvalues={"user_name": user_id}, ) - for stream_id, pusher in zip(stream_ids, pushers): - self.db_pool.simple_insert_txn( - txn, - table="deleted_pushers", - values={ + self.db_pool.simple_insert_many_txn( + txn, + table="deleted_pushers", + values=[ + { "stream_id": stream_id, "app_id": pusher.app_id, "pushkey": pusher.pushkey, "user_id": user_id, - }, - ) + } + for stream_id, pusher in zip(stream_ids, pushers) + ], + ) async with self._pushers_id_gen.get_next_mult(len(pushers)) as stream_ids: await self.db_pool.runInteraction(