From fb5137f8b5fbc3d83d0c7c4526fb6ed0b01d8803 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Fri, 15 Dec 2023 16:01:04 +0100 Subject: [PATCH] zilencer: Handle deleted realms nicely at server/analytics. --- zerver/models.py | 1 + zerver/tests/test_push_notifications.py | 33 +++++++++++-- .../0056_remoterealm_realm_locally_deleted.py | 17 +++++++ zilencer/models.py | 3 ++ zilencer/views.py | 48 ++++++++++++++++++- 5 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 zilencer/migrations/0056_remoterealm_realm_locally_deleted.py diff --git a/zerver/models.py b/zerver/models.py index 8368a91e50..cb337ea3d9 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -4861,6 +4861,7 @@ class AbstractRealmAuditLog(models.Model): # via send_server_data_to_push_bouncer. REMOTE_REALM_VALUE_UPDATED = 20001 REMOTE_PLAN_TRANSFERRED_SERVER_TO_REALM = 20002 + REMOTE_REALM_LOCALLY_DELETED = 20003 event_type = models.PositiveSmallIntegerField() diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index 3345f2f1bf..5f3ba70adf 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -1,5 +1,6 @@ import asyncio import base64 +import logging import uuid from contextlib import contextmanager from datetime import datetime, timedelta, timezone @@ -1998,19 +1999,25 @@ class AnalyticsBouncerTest(BouncerTestCase): @override_settings(PUSH_NOTIFICATION_BOUNCER_URL="https://push.zulip.org.example.com") @responses.activate - def test_non_existent_realm_uuid(self) -> None: + def test_deleted_realm(self) -> None: self.add_mock_response() + logger = logging.getLogger("zulip.analytics") + realm_info = get_realms_info_for_push_bouncer() # Hard-delete a realm to test the non existent realm uuid case. - realm = Realm.objects.order_by("id").first() + realm = get_realm("zephyr") assert realm is not None deleted_realm_uuid = realm.uuid realm.delete() + # This mock causes us to still send data to the bouncer as if the realm existed, + # causing the bouncer to include its corresponding info in the response. Through + # that, we're testing our graceful handling of seeing a non-existent realm uuid + # in that response. with mock.patch( "zerver.lib.remote_server.get_realms_info_for_push_bouncer", return_value=realm_info - ) as m, self.assertLogs("zulip.analytics", level="WARNING") as analytics_logger: + ) as m, self.assertLogs(logger, level="WARNING") as analytics_logger: send_server_data_to_push_bouncer(consider_usage_statistics=False) m.assert_called() realms = Realm.objects.all() @@ -2026,6 +2033,26 @@ class AnalyticsBouncerTest(BouncerTestCase): ], ) + # Now we want to test the other side of this - bouncer's handling + # of a deleted realm. + with self.assertLogs(logger, level="WARNING") as analytics_logger: + # This time the logger shouldn't get triggered - because the bouncer doesn't + # include .realm_locally_deleted realms in its response. + # Note: This is hacky, because until Python 3.10 we don't have access to + # assertNoLogs - and regular assertLogs demands that the logger gets triggered. + # So we do a dummy warning ourselves here, to satisfy it. + # TODO: Replace this with assertNoLogs once we fully upgrade to Python 3.10. + logger.warning("Dummy warning") + send_server_data_to_push_bouncer(consider_usage_statistics=False) + remote_realm_for_deleted_realm = RemoteRealm.objects.get(uuid=deleted_realm_uuid) + self.assertEqual(remote_realm_for_deleted_realm.registration_deactivated, True) + self.assertEqual(remote_realm_for_deleted_realm.realm_locally_deleted, True) + self.assertEqual(analytics_logger.output, ["WARNING:zulip.analytics:Dummy warning"]) + + audit_log = RemoteRealmAuditLog.objects.latest("id") + self.assertEqual(audit_log.event_type, RemoteRealmAuditLog.REMOTE_REALM_LOCALLY_DELETED) + self.assertEqual(audit_log.remote_realm, remote_realm_for_deleted_realm) + class PushNotificationTest(BouncerTestCase): @override diff --git a/zilencer/migrations/0056_remoterealm_realm_locally_deleted.py b/zilencer/migrations/0056_remoterealm_realm_locally_deleted.py new file mode 100644 index 0000000000..de98173657 --- /dev/null +++ b/zilencer/migrations/0056_remoterealm_realm_locally_deleted.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.8 on 2023-12-15 13:52 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zilencer", "0055_remoteserverbillinguser_tos_version"), + ] + + operations = [ + migrations.AddField( + model_name="remoterealm", + name="realm_locally_deleted", + field=models.BooleanField(default=False), + ), + ] diff --git a/zilencer/models.py b/zilencer/models.py index 560976c9ba..b746ed55a4 100644 --- a/zilencer/models.py +++ b/zilencer/models.py @@ -147,6 +147,9 @@ class RemoteRealm(models.Model): registration_deactivated = models.BooleanField(default=False) # Whether the realm has been deactivated on the remote server. realm_deactivated = models.BooleanField(default=False) + # Whether we believe the remote server deleted this realm + # from the database. + realm_locally_deleted = models.BooleanField(default=False) # When the realm was created on the remote server. realm_date_created = models.DateTimeField() diff --git a/zilencer/views.py b/zilencer/views.py index 82e9ab8f96..fca570df8e 100644 --- a/zilencer/views.py +++ b/zilencer/views.py @@ -654,7 +654,21 @@ def update_remote_realm_data_for_server( server: RemoteZulipServer, server_realms_info: List[RealmDataForAnalytics] ) -> None: uuids = [realm.uuid for realm in server_realms_info] - already_registered_remote_realms = RemoteRealm.objects.filter(uuid__in=uuids, server=server) + all_registered_remote_realms_for_server = list(RemoteRealm.objects.filter(server=server)) + already_registered_remote_realms = [ + remote_realm + for remote_realm in all_registered_remote_realms_for_server + if remote_realm.uuid in uuids + ] + # RemoteRealm registrations that we have for this server, but aren't + # present in the data sent to us. We assume this to mean the server + # must have deleted those realms from the database. + remote_realms_missing_from_server_data = [ + remote_realm + for remote_realm in all_registered_remote_realms_for_server + if remote_realm.uuid not in uuids + ] + already_registered_uuids = { remote_realm.uuid for remote_realm in already_registered_remote_realms } @@ -689,6 +703,10 @@ def update_remote_realm_data_for_server( # Update RemoteRealm entries, for which the corresponding realm's info has changed # (for the attributes that make sense to sync like this). for remote_realm in already_registered_remote_realms: + # TODO: We'll also want to check if .realm_locally_deleted is True, and if so, + # toggle it off (and potentially restore registration_deactivated=True too), + # since the server is now sending us data for this realm again. + modified = False realm = uuid_to_realm_dict[str(remote_realm.uuid)] for remote_realm_attr, realm_dict_key in [ @@ -739,6 +757,32 @@ def update_remote_realm_data_for_server( ) RemoteRealmAuditLog.objects.bulk_create(remote_realm_audit_logs) + remote_realms_to_update = [] + remote_realm_audit_logs = [] + for remote_realm in remote_realms_missing_from_server_data: + if not remote_realm.realm_locally_deleted: + # Otherwise we already knew about this, so nothing to do. + remote_realm.realm_locally_deleted = True + remote_realm.registration_deactivated = True + + remote_realm_audit_logs.append( + RemoteRealmAuditLog( + server=server, + remote_id=None, + remote_realm=remote_realm, + realm_id=None, + event_type=RemoteRealmAuditLog.REMOTE_REALM_LOCALLY_DELETED, + event_time=now, + ) + ) + remote_realms_to_update.append(remote_realm) + + RemoteRealm.objects.bulk_update( + remote_realms_to_update, + ["realm_locally_deleted", "registration_deactivated"], + ) + RemoteRealmAuditLog.objects.bulk_create(remote_realm_audit_logs) + def get_human_user_realm_uuids(realms: List[RealmDataForAnalytics]) -> List[UUID]: # nocoverage billable_realm_uuids = [] @@ -998,7 +1042,7 @@ def remote_server_post_analytics( remote_server_billing_session.sync_license_ledger_if_needed() remote_realm_dict: Dict[str, RemoteRealmDictValue] = {} - remote_realms = RemoteRealm.objects.filter(server=server) + remote_realms = RemoteRealm.objects.filter(server=server, realm_locally_deleted=False) for remote_realm in remote_realms: uuid = str(remote_realm.uuid) billing_session = RemoteRealmBillingSession(remote_realm)