From d89ef2883448e29355f6566524847872fdaf3561 Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Mon, 24 Aug 2026 17:51:25 +0100 Subject: [PATCH] Extend UserRestrictionHistory to record matched restriction instances When a RestrictionChecker fails we currently record only which restriction class it failed on, not which specific restriction row matched. The matching row is known at the point of failure, but then discarded. so the reviewers and Redash can see "an email restriction fired" and never "becaude of the pattern *@example.com" for example. This add the schema to hold that, but it doesn't actually wire it up just yet: - restriction_type, upload and version on UserRestrictionHistory, so a record says which action was being checked and what it applied to. Existing records predate them, the request-based paths have no upload, and the version is only known after the check runs. - UserRestrictionHistoryMatch, holding one row per matched restriction. A child model rather than fields, because a single failed check can match several restrictions. The link is a generic foreign key since matches span five restriction models, alongside a snapshot of str() at match time. A failure with no matches is legitimate and the schema allows it: some restrictions classes are not backed by the database at all. The ones that are can deny structurally without any row being involved. --- .../0027_restriction_history_matches.py | 50 ++++++++++++++++++ src/olympia/users/models.py | 51 +++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 src/olympia/users/migrations/0027_restriction_history_matches.py diff --git a/src/olympia/users/migrations/0027_restriction_history_matches.py b/src/olympia/users/migrations/0027_restriction_history_matches.py new file mode 100644 index 000000000000..3503461a82e0 --- /dev/null +++ b/src/olympia/users/migrations/0027_restriction_history_matches.py @@ -0,0 +1,50 @@ +# Generated by Django 5.2.17 on 2026-08-24 16:23 + +import django.db.models.deletion +import django.utils.timezone +import olympia.amo.models +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('contenttypes', '0002_remove_content_type_name'), + ('files', '0038_remove_enable_mv3_submissions_switch'), + ('users', '0026_alter_userrestrictionhistory_restriction_and_more'), + ('versions', '0053_auto_20260720_1345'), + ] + + operations = [ + migrations.AddField( + model_name='userrestrictionhistory', + name='restriction_type', + field=models.PositiveSmallIntegerField(choices=[(1, 'Add-on Submission'), (2, 'Add-on Approval'), (3, 'Rating'), (4, 'Rating Flag for Moderation')], null=True), + ), + migrations.AddField( + model_name='userrestrictionhistory', + name='upload', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='restriction_history', to='files.fileupload'), + ), + migrations.AddField( + model_name='userrestrictionhistory', + name='version', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='restriction_history', to='versions.version'), + ), + migrations.CreateModel( + name='UserRestrictionHistoryMatch', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', models.DateTimeField(blank=True, default=django.utils.timezone.now, editable=False)), + ('modified', models.DateTimeField(auto_now=True)), + ('object_id', models.PositiveIntegerField()), + ('snapshot', models.CharField(max_length=255)), + ('content_type', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='contenttypes.contenttype')), + ('history', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='matches', to='users.userrestrictionhistory')), + ], + options={ + 'verbose_name_plural': 'User Restriction History Matches', + }, + bases=(olympia.amo.models.SaveUpdateMixin, models.Model), + ), + ] diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index e6a5ba017840..9fc3c06a9299 100644 --- a/src/olympia/users/models.py +++ b/src/olympia/users/models.py @@ -13,6 +13,8 @@ from django.conf import settings from django.contrib.auth.models import AbstractBaseUser, BaseUserManager from django.contrib.auth.signals import user_logged_in +from django.contrib.contenttypes.fields import GenericForeignKey +from django.contrib.contenttypes.models import ContentType from django.core import validators from django.core.exceptions import ValidationError from django.core.files.uploadedfile import SimpleUploadedFile @@ -1430,6 +1432,27 @@ class UserRestrictionHistory(ModelBase): ) ip_address = models.CharField(default='', max_length=45) last_login_ip = models.CharField(default='', max_length=45) + # The action that was being checked when the restriction fired. Null on + # records created before we started recording it. + restriction_type = models.PositiveSmallIntegerField( + null=True, choices=RESTRICTION_TYPES.choices + ) + # Set by RestrictionChecker when it was given an upload (auto-approval), + # null on the request-based paths where there is no upload. + upload = models.ForeignKey( + 'files.FileUpload', + related_name='restriction_history', + on_delete=models.SET_NULL, + null=True, + ) + # Backfilled by Version.from_upload(): the checker runs before the version + # exists, so it can't set this itself. + version = models.ForeignKey( + 'versions.Version', + related_name='restriction_history', + on_delete=models.SET_NULL, + null=True, + ) class Meta: verbose_name_plural = 'User Restriction History' @@ -1445,6 +1468,34 @@ class Meta: ] +class UserRestrictionHistoryMatch(ModelBase): + """A specific restriction instance that matched during a failed check. + + A single failed check can match more than one restriction (unlike the + boolean path, which stops at the first match), so these hang off + UserRestrictionHistory rather than being fields on it. A failure with no + matches at all is legitimate: several restriction classes aren't backed by + the database, and the database-backed ones can deny structurally, without + any row being involved. + """ + + history = models.ForeignKey( + UserRestrictionHistory, related_name='matches', on_delete=models.CASCADE + ) + # The matched restriction can belong to any of the database-backed + # restriction models, hence a generic foreign key rather than a plain one. + content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) + object_id = models.PositiveIntegerField() + restriction = GenericForeignKey('content_type', 'object_id') + # str() of the restriction as it was when it matched. Restrictions get + # edited and bulk-deleted, so the foreign key above will eventually dangle + # or point at something that has since changed; this keeps history honest. + snapshot = models.CharField(max_length=255) + + class Meta: + verbose_name_plural = 'User Restriction History Matches' + + class UserHistory(ModelBase): id = PositiveAutoField(primary_key=True) email = models.EmailField(max_length=75)