From 1312c7ccd79cdbee4928cab206eefb14c266fbd3 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Wed, 8 Nov 2023 20:02:10 +0100 Subject: [PATCH] zilencer: Add mechanism to update RemoteRealm when Realm is changed. This requires a migration to allow RemoteRealmAuditLog.remote_id to be NULL, and to add a RemoteRealmAuditLog.remote_realm. --- zerver/models.py | 5 ++ zerver/tests/test_push_notifications.py | 68 +++++++++++++++++-- ...moterealmauditlog_remote_realm_and_more.py | 25 +++++++ zilencer/models.py | 6 +- zilencer/views.py | 53 +++++++++++++-- 5 files changed, 144 insertions(+), 13 deletions(-) create mode 100644 zilencer/migrations/0034_remoterealmauditlog_remote_realm_and_more.py diff --git a/zerver/models.py b/zerver/models.py index 1edab6be75..7299307bbe 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -4795,6 +4795,11 @@ class AbstractRealmAuditLog(models.Model): REMOTE_SERVER_PLAN_TYPE_CHANGED = 10204 REMOTE_SERVER_DEACTIVATED = 10201 + # This value is for RemoteRealmAuditLog entries tracking changes to the + # RemoteRealm model resulting from modified realm information sent to us + # via send_analytics_to_push_bouncer. + REMOTE_REALM_VALUE_UPDATED = 20001 + event_type = models.PositiveSmallIntegerField() # event_types synced from on-prem installations to Zulip Cloud when diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index 0be27d9588..3af9789142 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -27,6 +27,7 @@ from analytics.lib.counts import CountStat, LoggingCountStat from analytics.models import InstallationCount, RealmCount from zerver.actions.message_delete import do_delete_messages from zerver.actions.message_flags import do_mark_stream_messages_as_read, do_update_message_flags +from zerver.actions.realm_settings import do_deactivate_realm from zerver.actions.user_groups import check_add_user_group from zerver.actions.user_settings import do_change_user_setting, do_regenerate_api_key from zerver.actions.user_topics import do_set_user_topic_visibility_policy @@ -1044,9 +1045,64 @@ class AnalyticsBouncerTest(BouncerTestCase): ], ) + # Modify a realm and verify the remote realm data that should get updated, get updated. + zephyr_realm = get_realm("zephyr") + zephyr_original_host = zephyr_realm.host + zephyr_realm.string_id = "zephyr2" + + # date_created can't be updated. + original_date_created = zephyr_realm.date_created + zephyr_realm.date_created = now() + zephyr_realm.save() + # Deactivation is synced. + do_deactivate_realm(zephyr_realm, acting_user=None) + + send_analytics_to_push_bouncer() + check_counts(3, 3, 1, 1, 4) + + zephyr_remote_realm = RemoteRealm.objects.get(uuid=zephyr_realm.uuid) + self.assertEqual(zephyr_remote_realm.host, zephyr_realm.host) + self.assertEqual(zephyr_remote_realm.realm_date_created, original_date_created) + self.assertEqual(zephyr_remote_realm.realm_deactivated, True) + + # Verify the RemoteRealmAuditLog entries created. + remote_audit_logs = ( + RemoteRealmAuditLog.objects.filter( + event_type=RemoteRealmAuditLog.REMOTE_REALM_VALUE_UPDATED + ) + .order_by("id") + .values("event_type", "remote_id", "realm_id", "extra_data") + ) + + self.assertEqual( + list(remote_audit_logs), + [ + dict( + event_type=RemoteRealmAuditLog.REMOTE_REALM_VALUE_UPDATED, + remote_id=None, + realm_id=zephyr_realm.id, + extra_data={ + "attr_name": "host", + "old_value": zephyr_original_host, + "new_value": zephyr_realm.host, + }, + ), + dict( + event_type=RemoteRealmAuditLog.REMOTE_REALM_VALUE_UPDATED, + remote_id=None, + realm_id=zephyr_realm.id, + extra_data={ + "attr_name": "realm_deactivated", + "old_value": False, + "new_value": True, + }, + ), + ], + ) + # Test having no new rows send_analytics_to_push_bouncer() - check_counts(3, 2, 1, 1, 1) + check_counts(4, 3, 1, 1, 4) # Test only having new RealmCount rows RealmCount.objects.create( @@ -1062,14 +1118,14 @@ class AnalyticsBouncerTest(BouncerTestCase): value=9, ) send_analytics_to_push_bouncer() - check_counts(4, 3, 3, 1, 1) + check_counts(5, 4, 3, 1, 4) # Test only having new InstallationCount rows InstallationCount.objects.create( property=realm_stat.property, end_time=end_time + datetime.timedelta(days=1), value=6 ) send_analytics_to_push_bouncer() - check_counts(5, 4, 3, 2, 1) + check_counts(6, 5, 3, 2, 4) # Test only having new RealmAuditLog rows # Non-synced event @@ -1081,7 +1137,7 @@ class AnalyticsBouncerTest(BouncerTestCase): extra_data={"data": "foo"}, ) send_analytics_to_push_bouncer() - check_counts(6, 4, 3, 2, 1) + check_counts(7, 5, 3, 2, 4) # Synced event RealmAuditLog.objects.create( realm=user.realm, @@ -1093,7 +1149,7 @@ class AnalyticsBouncerTest(BouncerTestCase): }, ) send_analytics_to_push_bouncer() - check_counts(7, 5, 3, 2, 2) + check_counts(8, 6, 3, 2, 5) # Now create an InstallationCount with a property that's not supposed # to be tracked by the remote server - since the bouncer itself tracks @@ -1112,7 +1168,7 @@ class AnalyticsBouncerTest(BouncerTestCase): ) # The analytics endpoint call counts increase by 1, but the actual RemoteCounts remain unchanged, # since syncing the data failed. - check_counts(8, 6, 3, 2, 2) + check_counts(9, 7, 3, 2, 5) forbidden_installation_count.delete() (realm_count_data, installation_count_data, realmauditlog_data) = build_analytics_data( diff --git a/zilencer/migrations/0034_remoterealmauditlog_remote_realm_and_more.py b/zilencer/migrations/0034_remoterealmauditlog_remote_realm_and_more.py new file mode 100644 index 0000000000..913ff0b52c --- /dev/null +++ b/zilencer/migrations/0034_remoterealmauditlog_remote_realm_and_more.py @@ -0,0 +1,25 @@ +# Generated by Django 4.2.6 on 2023-11-08 23:17 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zilencer", "0033_remoterealm"), + ] + + operations = [ + migrations.AddField( + model_name="remoterealmauditlog", + name="remote_realm", + field=models.ForeignKey( + null=True, on_delete=django.db.models.deletion.CASCADE, to="zilencer.remoterealm" + ), + ), + migrations.AlterField( + model_name="remoterealmauditlog", + name="remote_id", + field=models.IntegerField(null=True), + ), + ] diff --git a/zilencer/models.py b/zilencer/models.py index 2142adb6d4..46440c1962 100644 --- a/zilencer/models.py +++ b/zilencer/models.py @@ -142,9 +142,13 @@ class RemoteRealmAuditLog(AbstractRealmAuditLog): """ server = models.ForeignKey(RemoteZulipServer, on_delete=models.CASCADE) + + # For pre-8.0 servers, we might only have the realm ID. realm_id = models.IntegerField() + # With newer servers, we can link to the RemoteRealm object. + remote_realm = models.ForeignKey(RemoteRealm, on_delete=models.CASCADE, null=True) # The remote_id field lets us deduplicate data from the remote server - remote_id = models.IntegerField() + remote_id = models.IntegerField(null=True) @override def __str__(self) -> str: diff --git a/zilencer/views.py b/zilencer/views.py index bcf32bcfb1..30e821aa03 100644 --- a/zilencer/views.py +++ b/zilencer/views.py @@ -507,12 +507,10 @@ def update_remote_realm_data_for_server( server: RemoteZulipServer, server_realms_info: List[Dict[str, Any]] ) -> None: uuids = [realm["uuid"] for realm in server_realms_info] - already_registered_uuids = set( - str(uuid) - for uuid in RemoteRealm.objects.filter(uuid__in=uuids, server=server).values_list( - "uuid", flat=True - ) - ) + already_registered_remote_realms = RemoteRealm.objects.filter(uuid__in=uuids, server=server) + already_registered_uuids = { + str(remote_realm.uuid) for remote_realm in already_registered_remote_realms + } new_remote_realms = [ RemoteRealm( @@ -532,6 +530,49 @@ def update_remote_realm_data_for_server( except IntegrityError: raise JsonableError(_("Duplicate registration detected.")) + uuid_to_realm_dict = {str(realm["uuid"]): realm for realm in server_realms_info} + remote_realms_to_update = [] + remote_realm_audit_logs = [] + now = timezone_now() + + # 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: + modified = False + realm = uuid_to_realm_dict[str(remote_realm.uuid)] + for remote_realm_attr, realm_dict_key in [ + ("host", "host"), + ("realm_deactivated", "deactivated"), + ]: + old_value = getattr(remote_realm, remote_realm_attr) + new_value = realm[realm_dict_key] + if old_value == new_value: + continue + + setattr(remote_realm, remote_realm_attr, new_value) + remote_realm_audit_logs.append( + RemoteRealmAuditLog( + server=server, + remote_id=None, + remote_realm=remote_realm, + realm_id=realm["id"], + event_type=RemoteRealmAuditLog.REMOTE_REALM_VALUE_UPDATED, + event_time=now, + extra_data={ + "attr_name": remote_realm_attr, + "old_value": old_value, + "new_value": new_value, + }, + ) + ) + modified = True + + if modified: + remote_realms_to_update.append(remote_realm) + + RemoteRealm.objects.bulk_update(remote_realms_to_update, ["host", "realm_deactivated"]) + RemoteRealmAuditLog.objects.bulk_create(remote_realm_audit_logs) + @has_request_variables def remote_server_post_analytics(