From ada306b0eedae99e83ea9b993ca30730bc49705b Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 17 Jun 2024 10:08:40 +0200 Subject: [PATCH 1/4] Unused command removed --- .../commands/fix_roles_as_string.py | 36 ------------------- 1 file changed, 36 deletions(-) delete mode 100644 bid_main/management/commands/fix_roles_as_string.py diff --git a/bid_main/management/commands/fix_roles_as_string.py b/bid_main/management/commands/fix_roles_as_string.py deleted file mode 100644 index a015152..0000000 --- a/bid_main/management/commands/fix_roles_as_string.py +++ /dev/null @@ -1,36 +0,0 @@ -"""Updates users' public_roles_as_string property based on their roles.""" - -from django.core.management.base import BaseCommand -from django.contrib.auth import get_user_model - -from bid_main import models - - -class Command(BaseCommand): - help = "Updates users' public_roles_as_string property based on their roles" - - def handle(self, *args, **options): - from bid_api import signals - from django.db.models.signals import post_save - - post_save.disconnect(signals.modified_user_to_webhooks) - - self.stdout.write("Updating all users.") - - model: models.User = get_user_model() - all_users = model.objects.all() - user_count = len(all_users) - for idx, user in enumerate(all_users): - do_print = ( - (user_count > 250 and idx % 250 == 0) - or (250 >= user_count > 100 and idx % 10 == 0) - or (user_count <= 100) - ) - if do_print: - self.stdout.write(f" - {idx+1}/{user_count}") - - roles = user.public_roles() - user.public_roles_as_string = " ".join(sorted(roles)) - user.save(update_fields={"public_roles_as_string"}) - - self.stdout.write(self.style.SUCCESS(f"Done, updated {user_count} users.")) -- 2.30.2 From 6a95b03d702df06a68d339d24497624151ae7bec Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 17 Jun 2024 10:13:26 +0200 Subject: [PATCH 2/4] Send user-modified only to webhooks of relevant OAuth apps --- bid_api/admin.py | 1 + bid_api/migrations/0010_alter_webhook_app.py | 21 ++ bid_api/models.py | 2 - bid_api/signals.py | 18 +- bid_api/tests/test_webhooks.py | 218 +++++++++++++++++- .../blender_extensions_devserver.json | 1 + .../fixtures/blender_studio_devserver.json | 1 + .../tests/test_process_deletion_requests.py | 4 +- 8 files changed, 246 insertions(+), 20 deletions(-) create mode 100644 bid_api/migrations/0010_alter_webhook_app.py diff --git a/bid_api/admin.py b/bid_api/admin.py index 5a8ce63..6128a18 100644 --- a/bid_api/admin.py +++ b/bid_api/admin.py @@ -49,6 +49,7 @@ class WebhookAdmin(ModelAdmin): list_display = ( "name", "hook_type", + "app", "url", "enabled", ) diff --git a/bid_api/migrations/0010_alter_webhook_app.py b/bid_api/migrations/0010_alter_webhook_app.py new file mode 100644 index 0000000..1aba4fb --- /dev/null +++ b/bid_api/migrations/0010_alter_webhook_app.py @@ -0,0 +1,21 @@ +# Generated by Django 3.2.9 on 2024-06-17 06:57 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.OAUTH2_PROVIDER_APPLICATION_MODEL), + ('bid_api', '0009_remove_webhook_last_flush_attempt'), + ] + + operations = [ + migrations.AlterField( + model_name='webhook', + name='app', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='webhooks', to='bid_main.oauth2application'), + ), + ] diff --git a/bid_api/models.py b/bid_api/models.py index 44f734f..5f1bc45 100644 --- a/bid_api/models.py +++ b/bid_api/models.py @@ -36,8 +36,6 @@ class Webhook(models.Model): ) app = models.ForeignKey( 'bid_main.OAuth2Application', - null=True, - blank=True, related_name="webhooks", on_delete=models.CASCADE, ) diff --git a/bid_api/signals.py b/bid_api/signals.py index eb59c37..2c157f0 100644 --- a/bid_api/signals.py +++ b/bid_api/signals.py @@ -100,9 +100,10 @@ def inspect_modified_user(sender, user: UserModel, **kwargs): @receiver(post_save) @filter_user_save_hook -def modified_user_to_webhooks(sender, user: UserModel, **kwargs): - """Forwards modified user information to webhooks. +def handle_modifications(sender, user: UserModel, **kwargs): + """Act on various account modifications. + Clean up orphaned avatars, notify about email change and call webhooks. The payload is POSTed, and a HMAC-SHA256 checksum is sent using the X-Webhook-HMAC HTTP header. @@ -116,9 +117,6 @@ def modified_user_to_webhooks(sender, user: UserModel, **kwargs): # log.debug('Skipping save of %s', user.email) return - hooks = models.Webhook.objects.filter(enabled=True) - log.debug("Sending modification of %s to %d webhooks", user.email, len(hooks)) - # Get the old email address so that the webhook receiver can match by # either database ID or email address. webhook_pre_save = getattr(user, "webhook_pre_save", {}) @@ -145,6 +143,16 @@ def modified_user_to_webhooks(sender, user: UserModel, **kwargs): log.debug("User changed email from %s to %s", old_email, user.email) user_email_changed.send(sender, user=user, old_email=old_email) + hooks = models.Webhook.objects.filter(enabled=True).filter( + app__in=user.bid_main_oauth2accesstoken.distinct().values_list('application', flat=True) + ) + + if not hooks.count(): + log.info('Not sending modification of pk=%d to any webhooks', user.pk) + return + + log.info("Sending modification of pk=%d to %d webhooks", user.pk, len(hooks)) + # Do our own JSON encoding so that we can compute the HMAC using the hook's secret. payload = { "id": user.id, diff --git a/bid_api/tests/test_webhooks.py b/bid_api/tests/test_webhooks.py index c38f0ae..f1fc73e 100644 --- a/bid_api/tests/test_webhooks.py +++ b/bid_api/tests/test_webhooks.py @@ -11,7 +11,7 @@ from freezegun import freeze_time import responses from .abstract import UserModel -from bid_main.models import Role +from bid_main.models import Role, OAuth2Application from bid_api import models import bid_api.tasks as tasks @@ -48,19 +48,24 @@ def _call_task_directly(*args, **kwargs): new=_call_task_directly, ) class WebhookTest(TestCase): - HOOK_URL = "http://www.unit.test/api/webhook" + HOOK_URLS = [ + "http://www.unit.test/api/webhook/", + "http://www.yet-another-unit.test/api/webhook/", + ] def setUp(self): - self.hook = models.Webhook( + self.oauth_app = OAuth2Application.objects.create() + self.hook = models.Webhook.objects.create( + app=self.oauth_app, name="Unit test webhook", enabled=True, hook_type="USER_MODIFIED", - url=self.HOOK_URL, + url=self.HOOK_URLS[0], secret="ieBithiGhahh4hee5mac8zu0xiu8kae0", ) - self.hook.save() responses.start() - responses.add(responses.POST, self.HOOK_URL, json={"status": "success"}, status=200) + responses.add(responses.POST, self.HOOK_URLS[0], json={"status": "success"}, status=200) + responses.add(responses.POST, self.HOOK_URLS[1], json={"status": "success"}, status=200) super().setUp() def tearDown(self): @@ -69,6 +74,10 @@ class WebhookTest(TestCase): def test_modify_user_email_only(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.email = "new@user.com" user.save(update_fields={"email"}) self.assertEqual(user.public_roles_as_string, "") @@ -77,7 +86,7 @@ class WebhookTest(TestCase): self.assertEqual(1, len(responses.calls)) call = responses.calls[0] - self.assertEqual(self.HOOK_URL, call.request.url) + self.assertEqual(self.HOOK_URLS[0], call.request.url) payload = json.loads(call.request.body) self.assertEqual( @@ -106,8 +115,81 @@ class WebhookTest(TestCase): self.assertEqual(hexdigest, call.request.headers["X-Webhook-HMAC"]) self.assertEqual("application/json", call.request.headers["Content-Type"]) + def test_modify_user_email_and_full_name_not_sent_because_no_oauth_apps_linked_to_user(self): + user = UserModel.objects.create_user("test@user.com", "123456") + + user.email = "new@user.com" + user.full_name = "ဖန်စီဘောင်းဘီ" + user.save() + + self.assertTrue(user.webhook_user_modified) + + self.assertEqual(0, len(responses.calls)) + + def test_modify_user_email_and_full_name_only_sent_to_hooks_linked_to_oauth_app(self): + another_oauth_app1 = OAuth2Application.objects.create() + models.Webhook.objects.create( + app=another_oauth_app1, + name="Another webhook linked to a different OAuth app", + enabled=True, + hook_type="USER_MODIFIED", + url="http://www.another-unit.test/api/another-webhook/", + secret="asdfasdfasdfasfdasdfasdfas", + ) + another_oauth_app2 = OAuth2Application.objects.create() + models.Webhook.objects.create( + app=another_oauth_app2, + name="Yet another webhook linked to a different OAuth app", + enabled=True, + hook_type="USER_MODIFIED", + url=self.HOOK_URLS[1], + secret="asdfasdfasdfasfdasdfasdfas", + ) + models.Webhook.objects.create( + app=self.oauth_app, + name="Disabled webhook of the same OAuth app", + enabled=False, + hook_type="USER_MODIFIED", + url='http://example.com/disabled-webhook/', + secret="asdfasdfasasd12asdfasdfas", + ) + user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01', token='foobar1' + ) + user.bid_main_oauth2accesstoken.create( + application=another_oauth_app2, expires='2024-01-01 01:01:01', token='fooobar2' + ) + + user.email = "new@user.com" + user.full_name = "ဖန်စီဘောင်းဘီ" + user.save() + + self.assertTrue(user.webhook_user_modified) + + self.assertEqual(2, len(responses.calls)) + expected_payload = { + "id": user.id, + "old_email": "test@user.com", + "full_name": "ဖန်စီဘောင်းဘီ", + "email": "new@user.com", + "roles": [], + "avatar_changed": False, + "avatar_url": None, + "date_deletion_requested": None, + "confirmed_email_at": None, + "nickname": "", + "old_nickname": "", + } + self.assertEqual(expected_payload, json.loads(responses.calls[0].request.body)) + self.assertEqual(expected_payload, json.loads(responses.calls[1].request.body)) + def test_modify_user_email_and_full_name(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.email = "new@user.com" user.full_name = "ဖန်စီဘောင်းဘီ" user.save() @@ -135,6 +217,10 @@ class WebhookTest(TestCase): def test_modify_user_nickname(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.nickname = "propellorolleporp" user.save() @@ -184,6 +270,10 @@ class WebhookTest(TestCase): my_dir = pathlib.Path(__file__).absolute().parent with self.settings(MEDIA_ROOT=my_dir / "media"): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.avatar = "images/t-rex.png" user.save() @@ -210,6 +300,10 @@ class WebhookTest(TestCase): def test_modify_user_add_role(self): user = UserModel.objects.create_user("test@user.com", "123456", full_name="ဖန်စီဘောင်းဘီ") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + role1 = Role(name="cloud_subscriber", is_public=True, is_badge=True, is_active=True) role1.save() role2 = Role(name="admin", is_public=False, is_badge=False, is_active=True) @@ -271,6 +365,10 @@ class WebhookTest(TestCase): @freeze_time('2021-01-01T01:02:03+02:00') def test_modify_date_deletion_requested(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.date_deletion_requested = timezone.now() user.save(update_fields={"date_deletion_requested"}) @@ -278,7 +376,7 @@ class WebhookTest(TestCase): self.assertEqual(1, len(responses.calls)) call = responses.calls[0] - self.assertEqual(self.HOOK_URL, call.request.url) + self.assertEqual(self.HOOK_URLS[0], call.request.url) payload = json.loads(call.request.body) self.assertEqual( @@ -301,6 +399,10 @@ class WebhookTest(TestCase): @freeze_time('2021-01-01T04:05:06+02:00') def test_modify_confirmed_email_at(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.confirmed_email_at = timezone.now() user.save(update_fields={"confirmed_email_at"}) @@ -308,7 +410,7 @@ class WebhookTest(TestCase): self.assertEqual(1, len(responses.calls)) call = responses.calls[0] - self.assertEqual(self.HOOK_URL, call.request.url) + self.assertEqual(self.HOOK_URLS[0], call.request.url) payload = json.loads(call.request.body) self.assertEqual( @@ -375,19 +477,24 @@ class WebhookQueuedAsTaskTest(TestCase): def setUp(self): self.maxDiff = None - self.hook = models.Webhook( + self.oauth_app = OAuth2Application.objects.create() + self.hook = models.Webhook.objects.create( + app=self.oauth_app, name="Unit test webhook", enabled=True, hook_type="USER_MODIFIED", url=self.HOOK_URL, secret="ieBithiGhahh4hee5mac8zu0xiu8kae0", ) - self.hook.save() super().setUp() self.assertEqual(Task.objects.count(), 0) def test_modify_user_email_only(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.email = "new@user.com" user.save(update_fields={"email"}) self.assertEqual(user.public_roles_as_string, "") @@ -417,8 +524,77 @@ class WebhookQueuedAsTaskTest(TestCase): ], ) + def test_modify_user_email_and_full_name_not_sent_because_no_oauth_apps_linked_to_user(self): + user = UserModel.objects.create_user("test@user.com", "123456") + + user.email = "new@user.com" + user.full_name = "ဖန်စီဘောင်းဘီ" + user.save() + + self.assertTrue(user.webhook_user_modified) + + self.assertEqual(Task.objects.count(), 0) + + def test_modify_user_email_and_full_name_only_sent_to_hook_linked_to_oauth_app(self): + another_oauth_app = OAuth2Application.objects.create() + models.Webhook.objects.create( + app=another_oauth_app, + name="Unit test webhook linked to a different OAuth app", + enabled=True, + hook_type="USER_MODIFIED", + url="http://www.some-other-unit.test/api/another-webhook/", + secret="asdfasdfasdfasfdasdfasdfas", + ) + models.Webhook.objects.create( + app=self.oauth_app, + name="Disabled webhook of the same OAuth app", + enabled=False, + hook_type="USER_MODIFIED", + url=self.HOOK_URL, + secret="asdfasdfasasd12asdfasdfas", + ) + user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + + user.email = "new@user.com" + user.full_name = "ဖန်စီဘောင်းဘီ" + user.save() + + self.assertTrue(user.webhook_user_modified) + + self.assertEqual(Task.objects.count(), 1) + task = Task.objects.first() + self.assertEqual( + json.loads(task.task_params), + [ + [ + self.hook.pk, + { + "id": user.id, + "old_email": "test@user.com", + "full_name": "ဖန်စီဘောင်းဘီ", + "email": "new@user.com", + "roles": [], + "avatar_changed": False, + "avatar_url": None, + "date_deletion_requested": None, + "confirmed_email_at": None, + "nickname": "", + "old_nickname": "", + }, + ], + {}, + ], + ) + def test_modify_user_email_and_full_name(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.email = "new@user.com" user.full_name = "ဖန်စီဘောင်းဘီ" user.save() @@ -450,6 +626,10 @@ class WebhookQueuedAsTaskTest(TestCase): def test_modify_user_nickname(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.nickname = "propellorolleporp" user.save() @@ -509,6 +689,10 @@ class WebhookQueuedAsTaskTest(TestCase): my_dir = pathlib.Path(__file__).absolute().parent with self.settings(MEDIA_ROOT=my_dir / "media"): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.avatar = "images/t-rex.png" user.save() @@ -538,6 +722,10 @@ class WebhookQueuedAsTaskTest(TestCase): def test_modify_user_add_role(self): user = UserModel.objects.create_user("test@user.com", "123456", full_name="ဖန်စီဘောင်းဘီ") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + role1 = Role(name="cloud_subscriber", is_public=True, is_badge=True, is_active=True) role1.save() role2 = Role(name="admin", is_public=False, is_badge=False, is_active=True) @@ -611,6 +799,10 @@ class WebhookQueuedAsTaskTest(TestCase): @freeze_time('2021-01-01T01:02:03+02:00') def test_modify_date_deletion_requested(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.date_deletion_requested = timezone.now() user.save(update_fields={"date_deletion_requested"}) @@ -642,6 +834,10 @@ class WebhookQueuedAsTaskTest(TestCase): @freeze_time('2021-01-01T04:05:06+02:00') def test_modify_confirmed_email_at(self): user = UserModel.objects.create_user("test@user.com", "123456") + user.bid_main_oauth2accesstoken.create( + application=self.oauth_app, expires='2024-01-01 01:01:01' + ) + user.confirmed_email_at = timezone.now() user.save(update_fields={"confirmed_email_at"}) diff --git a/bid_main/fixtures/blender_extensions_devserver.json b/bid_main/fixtures/blender_extensions_devserver.json index 6b0eb34..8b06775 100644 --- a/bid_main/fixtures/blender_extensions_devserver.json +++ b/bid_main/fixtures/blender_extensions_devserver.json @@ -28,6 +28,7 @@ "model": "bid_api.webhook", "pk": 8111, "fields": { + "app_id": 8111, "description": "Webhook of Blender Extensions Development server; installed from a Django .json fixture", "enabled": true, "hook_type": "USER_MODIFIED", diff --git a/bid_main/fixtures/blender_studio_devserver.json b/bid_main/fixtures/blender_studio_devserver.json index dc133d5..2d35c11 100644 --- a/bid_main/fixtures/blender_studio_devserver.json +++ b/bid_main/fixtures/blender_studio_devserver.json @@ -28,6 +28,7 @@ "model": "bid_api.webhook", "pk": 1, "fields": { + "app_id": 1, "description": "Webhook of Blender Studio Development server; installed from a Django .json fixture", "enabled": true, "hook_type": "USER_MODIFIED", diff --git a/bid_main/management/commands/tests/test_process_deletion_requests.py b/bid_main/management/commands/tests/test_process_deletion_requests.py index 206820b..d751a79 100644 --- a/bid_main/management/commands/tests/test_process_deletion_requests.py +++ b/bid_main/management/commands/tests/test_process_deletion_requests.py @@ -22,7 +22,7 @@ class ProcessDeletionRequestsCommandTest(TestCase): mock_anonymize.assert_not_called() @patch('bid_main.models.User.delete') - @patch('bid_api.signals.modified_user_to_webhooks') + @patch('bid_api.signals.handle_modifications') def test_anonymises_users(self, mock_webhook, mock_delete): now = timezone.now() # create some users @@ -65,7 +65,7 @@ class ProcessDeletionRequestsCommandTest(TestCase): ) @patch('bid_main.models.User.delete') - @patch('bid_api.signals.modified_user_to_webhooks') + @patch('bid_api.signals.handle_modifications') def test_missing_avatar(self, mock_webhook, mock_delete): now = timezone.now() # a user who requested deletion long enough ago, with a missing avatar -- 2.30.2 From 4fb5796d6baa3f6e7cf82f8bbfb5cbb80cf35286 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 17 Jun 2024 10:20:19 +0200 Subject: [PATCH 3/4] Count the hooks once in the signal --- bid_api/signals.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bid_api/signals.py b/bid_api/signals.py index 2c157f0..2692e7e 100644 --- a/bid_api/signals.py +++ b/bid_api/signals.py @@ -147,11 +147,12 @@ def handle_modifications(sender, user: UserModel, **kwargs): app__in=user.bid_main_oauth2accesstoken.distinct().values_list('application', flat=True) ) - if not hooks.count(): + hooks_count = hooks.count() + if hooks_count == 0: log.info('Not sending modification of pk=%d to any webhooks', user.pk) return - log.info("Sending modification of pk=%d to %d webhooks", user.pk, len(hooks)) + log.info("Sending modification of pk=%d to %d webhooks", user.pk, hooks_count) # Do our own JSON encoding so that we can compute the HMAC using the hook's secret. payload = { -- 2.30.2 From 6aee840c38ca502f6fd729b6ac4d7a0a972ce305 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 17 Jun 2024 10:29:31 +0200 Subject: [PATCH 4/4] Extra query in signal cleaned up --- bid_api/signals.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/bid_api/signals.py b/bid_api/signals.py index 2692e7e..daae515 100644 --- a/bid_api/signals.py +++ b/bid_api/signals.py @@ -143,11 +143,13 @@ def handle_modifications(sender, user: UserModel, **kwargs): log.debug("User changed email from %s to %s", old_email, user.email) user_email_changed.send(sender, user=user, old_email=old_email) - hooks = models.Webhook.objects.filter(enabled=True).filter( - app__in=user.bid_main_oauth2accesstoken.distinct().values_list('application', flat=True) + hooks = list( + models.Webhook.objects.filter(enabled=True).filter( + app__in=user.bid_main_oauth2accesstoken.distinct().values_list('application', flat=True) + ) ) - hooks_count = hooks.count() + hooks_count = len(hooks) if hooks_count == 0: log.info('Not sending modification of pk=%d to any webhooks', user.pk) return -- 2.30.2