From 0f0174532ba5982e290e1e47e2dd9c7c1181c637 Mon Sep 17 00:00:00 2001 From: "H.Shay" Date: Wed, 29 Sep 2021 11:52:32 -0700 Subject: [PATCH 1/9] add test --- tests/test_mau.py | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/tests/test_mau.py b/tests/test_mau.py index 66111eb3674b..117b418a9c38 100644 --- a/tests/test_mau.py +++ b/tests/test_mau.py @@ -13,11 +13,11 @@ # limitations under the License. """Tests REST events for /rooms paths.""" - +import synapse.rest.admin from synapse.api.constants import APP_SERVICE_REGISTRATION_TYPE, LoginType from synapse.api.errors import Codes, HttpResponseException, SynapseError from synapse.appservice import ApplicationService -from synapse.rest.client import register, sync +from synapse.rest.client import login, profile, register, sync from tests import unittest from tests.unittest import override_config @@ -26,7 +26,13 @@ class TestMauLimit(unittest.HomeserverTestCase): - servlets = [register.register_servlets, sync.register_servlets] + servlets = [ + register.register_servlets, + sync.register_servlets, + synapse.rest.admin.register_servlets_for_client_rest_resource, + profile.register_servlets, + login.register_servlets, + ] def default_config(self): config = default_config("test") @@ -229,6 +235,32 @@ def test_tracked_but_not_limited(self): self.reactor.advance(100) self.assertEqual(2, self.successResultOf(count)) + def test_deactivated_users_dont_count_towards_mau(self): + user1 = self.register_user("madonna", "password") + self.register_user("prince", "password2") + self.register_user("frodo", "onering", True) + + token1 = self.login("madonna", "password") + token2 = self.login("prince", "password2") + admin_token = self.login("frodo", "onering") + + self.do_sync_for_user(token1) + self.do_sync_for_user(token2) + + # Check that mau count is what we expect + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, 2) + + # Deactivate user1 + url = "/_synapse/admin/v1/deactivate/%s" % user1 + channel = self.make_request("POST", url, access_token=admin_token) + print(channel.json_body) + self.assertIn("success", channel.json_body["id_server_unbind_result"]) + + # Check that deactivated user is no longer counted + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, 1) + def create_user(self, localpart, token=None, appservice=False): request_data = { "username": localpart, From 10d59118eab6149ba0afa9a606d26673a35831b6 Mon Sep 17 00:00:00 2001 From: "H.Shay" Date: Wed, 29 Sep 2021 11:53:36 -0700 Subject: [PATCH 2/9] add function to remove user from monthly active table in deactivate code --- synapse/handlers/deactivate_account.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 9ae5b7750eaf..1b8f0c5e1746 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -133,6 +133,12 @@ async def deactivate_account( # delete from user directory await self.user_directory_handler.handle_local_user_deactivated(user_id) + # If the user is present in the monthly active users table, + # remove them + monthly_user = await self.store.user_last_seen_monthly_active(user_id) + if monthly_user: + await self.store.remove_deactivated_user(user_id) + # Mark the user as erased, if they asked for that if erase_data: user = UserID.from_string(user_id) From d111718431a4b64209573141e4b43991df129606 Mon Sep 17 00:00:00 2001 From: "H.Shay" Date: Wed, 29 Sep 2021 11:54:10 -0700 Subject: [PATCH 3/9] add function to remove user from monthly active table --- .../storage/databases/main/monthly_active_users.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/synapse/storage/databases/main/monthly_active_users.py b/synapse/storage/databases/main/monthly_active_users.py index b76ee51a9b46..0356d0e0cdac 100644 --- a/synapse/storage/databases/main/monthly_active_users.py +++ b/synapse/storage/databases/main/monthly_active_users.py @@ -354,3 +354,17 @@ async def populate_monthly_active_users(self, user_id): await self.upsert_monthly_active_user(user_id) elif now - last_seen_timestamp > LAST_SEEN_GRANULARITY: await self.upsert_monthly_active_user(user_id) + + async def remove_deactivated_user(self, user_id: str) -> None: + """ + Removes a deactivated user from the monthly active user + table and resets the "get_monthly_active_count" cache. + + Args: + user_id(str): the user_id to remove + """ + + await self.db_pool.simple_delete_one( + table="monthly_active_users", keyvalues={"user_id": user_id} + ) + await self.invalidate_cache_and_stream("get_monthly_active_count", ()) From dc4f15b5b653b1322de20097daa994c9231fb4ff Mon Sep 17 00:00:00 2001 From: "H.Shay" Date: Wed, 29 Sep 2021 11:58:46 -0700 Subject: [PATCH 4/9] add changelog entry --- changelog.d/10946.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10946.bugfix diff --git a/changelog.d/10946.bugfix b/changelog.d/10946.bugfix new file mode 100644 index 000000000000..40c70d3ece9f --- /dev/null +++ b/changelog.d/10946.bugfix @@ -0,0 +1 @@ +Fixes a long-standing bug wherin deactivated users still count towards the mau limit. \ No newline at end of file From 76613ed9eb610675bdae1a27f5e31679b199d120 Mon Sep 17 00:00:00 2001 From: "H.Shay" Date: Wed, 29 Sep 2021 12:10:33 -0700 Subject: [PATCH 5/9] update changelog number --- changelog.d/{10946.bugfix => 10947.bugfix} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{10946.bugfix => 10947.bugfix} (100%) diff --git a/changelog.d/10946.bugfix b/changelog.d/10947.bugfix similarity index 100% rename from changelog.d/10946.bugfix rename to changelog.d/10947.bugfix From 51b70676d8121cbf4d16d2e0ba3464c214e19f52 Mon Sep 17 00:00:00 2001 From: "H.Shay" Date: Fri, 1 Oct 2021 12:51:00 -0700 Subject: [PATCH 6/9] requested changes --- synapse/handlers/deactivate_account.py | 6 ++---- .../databases/main/monthly_active_users.py | 18 ++++++++++++++---- tests/test_mau.py | 1 - 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 1b8f0c5e1746..12bdca744510 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -133,11 +133,9 @@ async def deactivate_account( # delete from user directory await self.user_directory_handler.handle_local_user_deactivated(user_id) - # If the user is present in the monthly active users table, + # If the user is present in the monthly active users table # remove them - monthly_user = await self.store.user_last_seen_monthly_active(user_id) - if monthly_user: - await self.store.remove_deactivated_user(user_id) + await self.store.remove_deactivated_user_from_mau_table(user_id) # Mark the user as erased, if they asked for that if erase_data: diff --git a/synapse/storage/databases/main/monthly_active_users.py b/synapse/storage/databases/main/monthly_active_users.py index 0356d0e0cdac..20aa44717e88 100644 --- a/synapse/storage/databases/main/monthly_active_users.py +++ b/synapse/storage/databases/main/monthly_active_users.py @@ -355,7 +355,7 @@ async def populate_monthly_active_users(self, user_id): elif now - last_seen_timestamp > LAST_SEEN_GRANULARITY: await self.upsert_monthly_active_user(user_id) - async def remove_deactivated_user(self, user_id: str) -> None: + async def remove_deactivated_user_from_mau_table(self, user_id: str) -> None: """ Removes a deactivated user from the monthly active user table and resets the "get_monthly_active_count" cache. @@ -364,7 +364,17 @@ async def remove_deactivated_user(self, user_id: str) -> None: user_id(str): the user_id to remove """ - await self.db_pool.simple_delete_one( - table="monthly_active_users", keyvalues={"user_id": user_id} + rows_deleted = await self.db_pool.simple_delete( + table="monthly_active_users", + keyvalues={"user_id": user_id}, + desc="simple_delete", ) - await self.invalidate_cache_and_stream("get_monthly_active_count", ()) + + if rows_deleted is not 0: + await self.invalidate_cache_and_stream( + "user_last_seen_monthly_active", (user_id) + ) + await self.invalidate_cache_and_stream("get_monthly_active_count", ()) + await self.invalidate_cache_and_stream( + "get_monthly_active_count_by_service", () + ) diff --git a/tests/test_mau.py b/tests/test_mau.py index 117b418a9c38..bd65d3be5b45 100644 --- a/tests/test_mau.py +++ b/tests/test_mau.py @@ -254,7 +254,6 @@ def test_deactivated_users_dont_count_towards_mau(self): # Deactivate user1 url = "/_synapse/admin/v1/deactivate/%s" % user1 channel = self.make_request("POST", url, access_token=admin_token) - print(channel.json_body) self.assertIn("success", channel.json_body["id_server_unbind_result"]) # Check that deactivated user is no longer counted From 621ae33ac26b665d3c87d917cf7e41cef5b00f39 Mon Sep 17 00:00:00 2001 From: "H.Shay" Date: Fri, 1 Oct 2021 12:53:56 -0700 Subject: [PATCH 7/9] update docstring on new function --- synapse/storage/databases/main/monthly_active_users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/monthly_active_users.py b/synapse/storage/databases/main/monthly_active_users.py index 20aa44717e88..0d0662bda304 100644 --- a/synapse/storage/databases/main/monthly_active_users.py +++ b/synapse/storage/databases/main/monthly_active_users.py @@ -358,7 +358,7 @@ async def populate_monthly_active_users(self, user_id): async def remove_deactivated_user_from_mau_table(self, user_id: str) -> None: """ Removes a deactivated user from the monthly active user - table and resets the "get_monthly_active_count" cache. + table and resets affected caches. Args: user_id(str): the user_id to remove From b5974a2289dca70f5177ec5ffaa792c51a1daeaa Mon Sep 17 00:00:00 2001 From: "H.Shay" Date: Fri, 1 Oct 2021 13:12:39 -0700 Subject: [PATCH 8/9] fix lint error --- synapse/storage/databases/main/monthly_active_users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/monthly_active_users.py b/synapse/storage/databases/main/monthly_active_users.py index 0d0662bda304..7bd9c1d574b0 100644 --- a/synapse/storage/databases/main/monthly_active_users.py +++ b/synapse/storage/databases/main/monthly_active_users.py @@ -370,7 +370,7 @@ async def remove_deactivated_user_from_mau_table(self, user_id: str) -> None: desc="simple_delete", ) - if rows_deleted is not 0: + if rows_deleted != 0: await self.invalidate_cache_and_stream( "user_last_seen_monthly_active", (user_id) ) From faa460ae33aebe5838166a252c6e97dcb4a8532a Mon Sep 17 00:00:00 2001 From: Hillery Shay Date: Mon, 4 Oct 2021 08:05:32 -0700 Subject: [PATCH 9/9] Update synapse/storage/databases/main/monthly_active_users.py Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/storage/databases/main/monthly_active_users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/monthly_active_users.py b/synapse/storage/databases/main/monthly_active_users.py index 7bd9c1d574b0..8444aaa7e26b 100644 --- a/synapse/storage/databases/main/monthly_active_users.py +++ b/synapse/storage/databases/main/monthly_active_users.py @@ -372,7 +372,7 @@ async def remove_deactivated_user_from_mau_table(self, user_id: str) -> None: if rows_deleted != 0: await self.invalidate_cache_and_stream( - "user_last_seen_monthly_active", (user_id) + "user_last_seen_monthly_active", (user_id,) ) await self.invalidate_cache_and_stream("get_monthly_active_count", ()) await self.invalidate_cache_and_stream(