From 0253899e5323a91f1c76410a225d82451b82da9e Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Tue, 25 Aug 2026 17:47:16 +0100 Subject: [PATCH 01/13] Add matched-instance and version fields to UserRestrictionHistory When a RestrictionChecked check fails we record which restriction class denied the action, but not which specific restriction row matched. The matching row is known at the point of failure, then discarded, so reviewers and Redash can see "an email restriction fired" but never the reason. This adds the schema to hold that, without wiring anything up yet. - it adds two generic foreign keys to the matched restriction row. Generic FKs because restrictions span five different tables. Indexed together so "every failure caused by restrictions X" is a cheap query. - it adds version, tying the auto-approval failure to the version affected. The checker runs before the version exists, so this will be backfilled by Version.from_upload() rather than set by the checker. All new columns are nullable and no existing rows are touched. This is because there are many records that predate this change. --- ...istory_restriction_instance_and_version.py | 36 +++++++++++++++++++ src/olympia/users/models.py | 27 ++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 src/olympia/users/migrations/0027_userrestrictionhistory_restriction_instance_and_version.py diff --git a/src/olympia/users/migrations/0027_userrestrictionhistory_restriction_instance_and_version.py b/src/olympia/users/migrations/0027_userrestrictionhistory_restriction_instance_and_version.py new file mode 100644 index 000000000000..0398bd3b5949 --- /dev/null +++ b/src/olympia/users/migrations/0027_userrestrictionhistory_restriction_instance_and_version.py @@ -0,0 +1,36 @@ +# Generated by Django 5.2.17 on 2026-08-25 14:53 + +import django.db.models.deletion +import olympia.amo.models +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('contenttypes', '0002_remove_content_type_name'), + ('users', '0026_alter_userrestrictionhistory_restriction_and_more'), + ('versions', '0053_auto_20260720_1345'), + ] + + operations = [ + migrations.AddField( + model_name='userrestrictionhistory', + name='restriction_content_type', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to='contenttypes.contenttype'), + ), + migrations.AddField( + model_name='userrestrictionhistory', + name='restriction_object_id', + field=models.PositiveIntegerField(null=True), + ), + 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.AddIndex( + model_name='userrestrictionhistory', + index=olympia.amo.models.LongNameIndex(fields=['restriction_content_type', 'restriction_object_id'], name='users_userrestrictionhistory_restriction_content_type_object_id'), + ), + ] diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index e6a5ba017840..f8e50778f825 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 specific restriction row that matched, e.g. an EmailUserRestriction + # or IPNetworkUserRestriction instance. A generic foreign key because the + # restriction classes live in different tables. NULL on rows recorded + # before these fields existed, and always NULL for restrictions that + # aren't backed by the database (developer agreement, reputation). + restriction_content_type = models.ForeignKey( + ContentType, null=True, on_delete=models.SET_NULL + ) + restriction_object_id = models.PositiveIntegerField(null=True) + restriction_instance = GenericForeignKey( + 'restriction_content_type', 'restriction_object_id' + ) + # The version whose auto-approval was being checked. NULL on rows recorded + # before this field existed, and always NULL for checks other than + # auto-approval, which aren't tied to a version. + version = models.ForeignKey( + 'versions.Version', + related_name='restriction_history', + null=True, + on_delete=models.SET_NULL, + ) class Meta: verbose_name_plural = 'User Restriction History' @@ -1442,6 +1465,10 @@ class Meta: fields=('last_login_ip',), name='users_userrestrictionhistory_last_login_ip_d58d95ff', ), + LongNameIndex( + fields=('restriction_content_type', 'restriction_object_id'), + name='users_userrestrictionhistory_restriction_content_type_object_id', + ), ] From c0a35d2c313c6742b94e8ad7ae2630c6c6401534 Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Wed, 26 Aug 2026 10:59:24 +0100 Subject: [PATCH 02/13] Add get_matching_restrictions() to database-backed restrictions When a restriction check fails we want to record which specific restriction row or rows matched. The allow_*() fast paths still deliberately answer the bool yes/no as cheaply as possible, using .exists() or stopping at the first match. With this change, each of the five database-backed restriction classes get a slow-path classmethod as well. They mirror the fast paths extractoin and semantics, but they collect all matches instead of stopping at the first. At this commit, nothing actually calls these methods yet. Wiring it into the RestrictionChecker will be the next step. --- src/olympia/users/models.py | 136 +++++++++++ src/olympia/users/tests/test_models.py | 318 +++++++++++++++++++++++++ 2 files changed, 454 insertions(+) diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index f8e50778f825..92f43a4d0bfe 100644 --- a/src/olympia/users/models.py +++ b/src/olympia/users/models.py @@ -1022,6 +1022,50 @@ def allow_ips(self, remote_addr, user_last_login_ip, *, restriction_type): return True + @classmethod + def get_matching_restrictions(cls, argument, *, restriction_type): + """ + Return a list of the restrictions matching the given request or + upload (which one depends on the restriction_type being checked). + + Slow path, meant to be called after the corresponding allow_*() check + has already failed in order to record which restriction(s) matched: + unlike the fast path it does not stop at the first match. Returns an + empty list when the input needed for matching is missing or invalid. + """ + # Mirrors the extraction in allow_auto_approval()/allow_request(). + if restriction_type == RESTRICTION_TYPES.ADDON_APPROVAL: + upload = argument + if not upload.user or not upload.ip_address: + return [] + try: + remote_addr = ipaddress.ip_address(upload.ip_address) + user_last_login_ip = ipaddress.ip_address(upload.user.last_login_ip) + except ValueError: + return [] + else: + request = argument + try: + remote_addr = ipaddress.ip_address(request.META.get('REMOTE_ADDR')) + # Unlike allow_request(), also guard on is_authenticated: + # AnonymousUser is truthy but has no last_login_ip. + user_last_login_ip = ( + ipaddress.ip_address(request.user.last_login_ip) + if request.user and request.user.is_authenticated + else None + ) + except ValueError: + return [] + return [ + restriction + for restriction in cls.objects.filter(restriction_type=restriction_type) + if remote_addr in restriction.network + or ( + user_last_login_ip is not None + and user_last_login_ip in restriction.network + ) + ] + class AsnUserRestriction(RestrictionAbstractBaseModel): asn = models.PositiveIntegerField(db_index=True) @@ -1060,6 +1104,25 @@ def allow_auto_approval(cls, upload): return True return cls.allow_asn(asn, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL) + @classmethod + def get_matching_restrictions(cls, argument, *, restriction_type): + """ + Return a list of the restrictions matching the given request or + upload (which one depends on the restriction_type being checked). + + Slow path, meant to be called after the corresponding allow_*() check + has already failed in order to record which restriction(s) matched. + Returns an empty list when the input needed for matching is missing. + """ + # Mirrors the extraction in allow_auto_approval()/allow_request(). + if restriction_type == RESTRICTION_TYPES.ADDON_APPROVAL: + asn = (argument.request_metadata or {}).get('Asn') + else: + asn = argument.headers.get('Asn') + if not asn: + return [] + return list(cls.objects.filter(asn=asn, restriction_type=restriction_type)) + class NormalizeEmailMixin: @classmethod @@ -1180,6 +1243,40 @@ def allow_email(cls, email, *, restriction_type): return True + @classmethod + def get_matching_restrictions(cls, argument, *, restriction_type): + """ + Return a list of the restrictions matching the given request or + upload (which one depends on the restriction_type being checked). + + Slow path, meant to be called after the corresponding allow_*() check + has already failed in order to record which restriction(s) matched. + Returns an empty list when the input needed for matching is missing. + """ + # request.user is an AnonymousUser when not authenticated, while + # upload.user is a UserProfile; either guard means there is no email + # to match against, mirroring allow_request()/allow_auto_approval(). + user = argument.user + if not user or not user.is_authenticated: + return [] + email = cls.normalize_email(user.email) + base_qs = cls.objects.filter(restriction_type=restriction_type) + # Unlike allow_email(), which returns as soon as it finds a single + # match, collect the exact pattern match and every wildcard pattern + # matching: they all contributed to the failure. + matches = list(base_qs.filter(email_pattern=email)) + complex_restrictions = base_qs.filter( + Q(email_pattern__contains='?') + | Q(email_pattern__contains='*') + | Q(email_pattern__contains='[') + ).exclude(email_pattern=email) + matches.extend( + restriction + for restriction in complex_restrictions + if fnmatchcase(email, restriction.email_pattern) + ) + return matches + class DisposableEmailDomainRestriction(RestrictionAbstractBaseModel): domain = models.CharField( @@ -1231,6 +1328,26 @@ def allow_email(cls, email, *, restriction_type): domain=email_domain, restriction_type=restriction_type ).exists() + @classmethod + def get_matching_restrictions(cls, argument, *, restriction_type): + """ + Return a list of the restrictions matching the given request or + upload (which one depends on the restriction_type being checked). + + Slow path, meant to be called after the corresponding allow_*() check + has already failed in order to record which restriction(s) matched. + Returns an empty list when the input needed for matching is missing. + """ + user = argument.user + if not user or not user.is_authenticated: + return [] + # Same domain extraction as allow_email() - the raw email, not the + # normalized one. + email_domain = user.email.rsplit('@', maxsplit=1)[-1] + return list( + cls.objects.filter(domain=email_domain, restriction_type=restriction_type) + ) + class FingerprintRestriction(RestrictionAbstractBaseModel): ja4 = models.CharField(max_length=36, db_index=True) @@ -1271,6 +1388,25 @@ def allow_auto_approval(cls, upload): return True return cls.allow_ja4(ja4, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL) + @classmethod + def get_matching_restrictions(cls, argument, *, restriction_type): + """ + Return a list of the restrictions matching the given request or + upload (which one depends on the restriction_type being checked). + + Slow path, meant to be called after the corresponding allow_*() check + has already failed in order to record which restriction(s) matched. + Returns an empty list when the input needed for matching is missing. + """ + # Mirrors the extraction in allow_auto_approval()/allow_request(). + if restriction_type == RESTRICTION_TYPES.ADDON_APPROVAL: + ja4 = (argument.request_metadata or {}).get('Client-JA4') + else: + ja4 = argument.headers.get('Client-JA4') + if not ja4: + return [] + return list(cls.objects.filter(ja4=ja4, restriction_type=restriction_type)) + class ReputationRestrictionMixin: reputation_threshold = 50 diff --git a/src/olympia/users/tests/test_models.py b/src/olympia/users/tests/test_models.py index 3a6f55e2ac7e..8b87b2e322e6 100644 --- a/src/olympia/users/tests/test_models.py +++ b/src/olympia/users/tests/test_models.py @@ -1501,6 +1501,79 @@ def test_network_from_ip_blank(self): with self.assertRaises(ValueError): IPNetworkUserRestriction.network_from_ip('') + def test_get_matching_restrictions_single_match(self): + request = RequestFactory(REMOTE_ADDR='192.168.0.1').get('/') + request.user = user_factory(last_login_ip='10.0.0.1') + restriction = IPNetworkUserRestriction.objects.create(network='192.168.0.0/28') + IPNetworkUserRestriction.objects.create(network='172.16.0.0/24') + assert IPNetworkUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) == [restriction] + + def test_get_matching_restrictions_multiple_matches(self): + request = RequestFactory(REMOTE_ADDR='192.168.0.1').get('/') + request.user = user_factory(last_login_ip='10.0.0.1') + exact = IPNetworkUserRestriction.objects.create(network='192.168.0.1/32') + subnet = IPNetworkUserRestriction.objects.create(network='192.168.0.0/24') + last_login = IPNetworkUserRestriction.objects.create(network='10.0.0.0/28') + IPNetworkUserRestriction.objects.create(network='172.16.0.0/24') + assert set( + IPNetworkUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + ) == {exact, subnet, last_login} + + def test_get_matching_restrictions_auto_approval_last_login_ip(self): + upload = FileUpload.objects.create( + ip_address='192.168.1.2', + user=user_factory(last_login_ip='192.168.0.1'), + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + ) + restriction = IPNetworkUserRestriction.objects.create( + network='192.168.0.0/28', + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + assert IPNetworkUserRestriction.get_matching_restrictions( + upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) == [restriction] + + def test_get_matching_restrictions_no_matches(self): + request = RequestFactory(REMOTE_ADDR='192.168.0.1').get('/') + request.user = user_factory(last_login_ip='10.0.0.1') + IPNetworkUserRestriction.objects.create(network='172.16.0.0/24') + # Matching network, but for another restriction_type. + IPNetworkUserRestriction.objects.create( + network='192.168.0.0/28', + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + assert ( + IPNetworkUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + == [] + ) + + def test_get_matching_restrictions_unparseable_ip(self): + # last_login_ip is empty, so the same structural denial as in + # allow_auto_approval() applies: no specific restriction matched. + upload = FileUpload.objects.create( + ip_address='192.168.0.1', + user=user_factory(last_login_ip=''), + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + ) + IPNetworkUserRestriction.objects.create( + network='192.168.0.0/28', + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + assert ( + IPNetworkUserRestriction.get_matching_restrictions( + upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + == [] + ) + class TestDisposableEmailDomainRestriction(TestCase): def test_email_allowed(self): @@ -1552,6 +1625,57 @@ def test_allowed_approval(self): ) assert DisposableEmailDomainRestriction.allow_auto_approval(upload) + def test_get_matching_restrictions_match(self): + request = RequestFactory().get('/') + request.user = user_factory(email='foo@bar.com') + restriction = DisposableEmailDomainRestriction.objects.create(domain='bar.com') + DisposableEmailDomainRestriction.objects.create(domain='other.com') + # (domain, restriction_type) is unique, so a given email can match at + # most one row. + assert DisposableEmailDomainRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) == [restriction] + + def test_get_matching_restrictions_auto_approval(self): + restriction = DisposableEmailDomainRestriction.objects.create( + domain='bar.com', restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + upload = FileUpload.objects.create( + ip_address='192.168.0.1', + user=user_factory(email='foo@bar.com'), + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + ) + assert DisposableEmailDomainRestriction.get_matching_restrictions( + upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) == [restriction] + + def test_get_matching_restrictions_no_matches(self): + request = RequestFactory().get('/') + request.user = user_factory(email='foo@bar.com') + DisposableEmailDomainRestriction.objects.create(domain='other.com') + # Matching domain, but for another restriction_type. + DisposableEmailDomainRestriction.objects.create( + domain='bar.com', restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + assert ( + DisposableEmailDomainRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + == [] + ) + + def test_get_matching_restrictions_not_authenticated(self): + DisposableEmailDomainRestriction.objects.create(domain='bar.com') + request = RequestFactory().get('/') + request.user = AnonymousUser() + assert ( + DisposableEmailDomainRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + == [] + ) + class TestEmailUserRestriction(TestCase): def test_str(self): @@ -1705,6 +1829,74 @@ def test_allowed_approval(self): ) assert EmailUserRestriction.allow_auto_approval(upload) + def test_get_matching_restrictions_single_match(self): + request = RequestFactory().get('/') + request.user = user_factory(email='foo@bar.com') + restriction = EmailUserRestriction.objects.create(email_pattern='foo@bar.com') + EmailUserRestriction.objects.create(email_pattern='someone@else.com') + assert EmailUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) == [restriction] + + def test_get_matching_restrictions_multiple_matches(self): + request = RequestFactory().get('/') + request.user = user_factory(email='f.oo+tag@faz.mail.com') + exact = EmailUserRestriction.objects.create(email_pattern='foo@faz.mail.com') + wildcard = EmailUserRestriction.objects.create(email_pattern='*.mail.com') + other_wildcard = EmailUserRestriction.objects.create( + email_pattern='foo@*.mail.com' + ) + EmailUserRestriction.objects.create(email_pattern='*@gmail.com') + # The email is normalized before matching, and unlike allow_email(), + # which stops at the exact match, every matching pattern is returned. + assert set( + EmailUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + ) == {exact, wildcard, other_wildcard} + + def test_get_matching_restrictions_auto_approval(self): + restriction = EmailUserRestriction.objects.create( + email_pattern='*.mail.com', + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + upload = FileUpload.objects.create( + ip_address='192.168.0.1', + user=user_factory(email='foo@faz.mail.com'), + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + ) + assert EmailUserRestriction.get_matching_restrictions( + upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) == [restriction] + + def test_get_matching_restrictions_no_matches(self): + request = RequestFactory().get('/') + request.user = user_factory(email='foo@bar.com') + EmailUserRestriction.objects.create(email_pattern='someone@else.com') + # Matching pattern, but for another restriction_type. + EmailUserRestriction.objects.create( + email_pattern='foo@bar.com', + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + assert ( + EmailUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + == [] + ) + + def test_get_matching_restrictions_not_authenticated(self): + EmailUserRestriction.objects.create(email_pattern='foo@bar.com') + request = RequestFactory().get('/') + request.user = AnonymousUser() + assert ( + EmailUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + == [] + ) + class TestFingerprintRestriction(TestCase): def test_str(self): @@ -1775,6 +1967,69 @@ def test_ja4_different_restriction(self): request = RequestFactory().get('/', headers={'Client-JA4': 'another_ja4'}) assert FingerprintRestriction.allow_submission(request) + def test_get_matching_restrictions_match(self): + restricted_ja4 = 'some_fake_ja4' + restriction = FingerprintRestriction.objects.create(ja4=restricted_ja4) + FingerprintRestriction.objects.create(ja4='another_ja4') + request = RequestFactory().get('/', headers={'Client-JA4': restricted_ja4}) + # (ja4, restriction_type) is unique, so a given fingerprint can match + # at most one row. + assert FingerprintRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) == [restriction] + + def test_get_matching_restrictions_auto_approval(self): + restricted_ja4 = 'some_fake_ja4' + restriction = FingerprintRestriction.objects.create( + ja4=restricted_ja4, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + upload = FileUpload.objects.create( + ip_address='192.168.0.1', + user=user_factory(), + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + request_metadata={'Client-JA4': restricted_ja4}, + ) + assert FingerprintRestriction.get_matching_restrictions( + upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) == [restriction] + + def test_get_matching_restrictions_no_matches(self): + FingerprintRestriction.objects.create(ja4='some_fake_ja4') + # Matching ja4, but for another restriction_type. + FingerprintRestriction.objects.create( + ja4='another_ja4', restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + request = RequestFactory().get('/', headers={'Client-JA4': 'another_ja4'}) + assert ( + FingerprintRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + == [] + ) + + def test_get_matching_restrictions_no_ja4(self): + FingerprintRestriction.objects.create(ja4='some_fake_ja4') + request = RequestFactory().get('/') + assert ( + FingerprintRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + == [] + ) + upload = FileUpload.objects.create( + ip_address='192.168.0.1', + user=user_factory(), + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + ) + assert ( + FingerprintRestriction.get_matching_restrictions( + upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + == [] + ) + class TestAsnRestriction(TestCase): def test_str(self): @@ -1841,6 +2096,69 @@ def test_asn_different_restriction(self): request = RequestFactory().get('/', headers={'Asn': '64499'}) assert AsnUserRestriction.allow_submission(request) + def test_get_matching_restrictions_match(self): + restricted_asn = 64498 + restriction = AsnUserRestriction.objects.create(asn=restricted_asn) + AsnUserRestriction.objects.create(asn=64499) + request = RequestFactory().get('/', headers={'Asn': restricted_asn}) + # (asn, restriction_type) is unique, so a given asn can match at most + # one row. + assert AsnUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) == [restriction] + + def test_get_matching_restrictions_auto_approval(self): + restricted_asn = 64498 + restriction = AsnUserRestriction.objects.create( + asn=restricted_asn, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + upload = FileUpload.objects.create( + ip_address='192.168.0.1', + user=user_factory(), + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + request_metadata={'Asn': restricted_asn}, + ) + assert AsnUserRestriction.get_matching_restrictions( + upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) == [restriction] + + def test_get_matching_restrictions_no_matches(self): + AsnUserRestriction.objects.create(asn=64498) + # Matching asn, but for another restriction_type. + AsnUserRestriction.objects.create( + asn=64499, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + request = RequestFactory().get('/', headers={'Asn': '64499'}) + assert ( + AsnUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + == [] + ) + + def test_get_matching_restrictions_no_asn(self): + AsnUserRestriction.objects.create(asn=64498) + request = RequestFactory().get('/') + assert ( + AsnUserRestriction.get_matching_restrictions( + request, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + == [] + ) + upload = FileUpload.objects.create( + ip_address='192.168.0.1', + user=user_factory(), + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + ) + assert ( + AsnUserRestriction.get_matching_restrictions( + upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + == [] + ) + @override_settings( REPUTATION_SERVICE_URL='https://reputation.example.com', From 5ab252e84c2d777e6cec7c80fac63fe9aa5e8ece Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Wed, 26 Aug 2026 12:54:25 +0100 Subject: [PATCH 03/13] Record matched restriction instances in UserRestrictionHistory When a check fails, RestrictionChecker now asks the failed class which specific restriction rows matched (get_matching_restrictions(), which was added in a previous commit). It writes one UserRestrictionHistory row per matched instance, each pointing at it through the generic foreign key. If there's a failure with nothing enumeratble, it still writes exactly one row, with the instance fields NULL, which is today's behaviour. The structural case also logs a warning. The RESTRICTED activity log entry gains the matched pks in its details, alongside the class name it already carried. --- src/olympia/users/tests/test_user_utils.py | 206 ++++++++++++++++++++- src/olympia/users/utils.py | 68 ++++++- 2 files changed, 258 insertions(+), 16 deletions(-) diff --git a/src/olympia/users/tests/test_user_utils.py b/src/olympia/users/tests/test_user_utils.py index 698c8bddd75d..3acb19d8a068 100644 --- a/src/olympia/users/tests/test_user_utils.py +++ b/src/olympia/users/tests/test_user_utils.py @@ -5,6 +5,7 @@ from urllib.parse import parse_qs, urlparse from django.conf import settings +from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ImproperlyConfigured from django.test.client import RequestFactory @@ -127,7 +128,7 @@ def test_user_is_allowed_to_bypass_restrictions(self, incr_mock): assert incr_mock.call_count == 0 def test_is_submission_allowed_ip_restricted(self, incr_mock): - IPNetworkUserRestriction.objects.create(network='10.0.0.0/24') + restriction = IPNetworkUserRestriction.objects.create(network='10.0.0.0/24') checker = RestrictionChecker(request=self.request) assert not checker.is_submission_allowed() assert checker.get_error_message() == ( @@ -147,6 +148,8 @@ def test_is_submission_allowed_ip_restricted(self, incr_mock): assert history.user == self.request.user assert history.last_login_ip == self.request.user.last_login_ip assert history.ip_address == '10.0.0.1' + assert history.restriction_instance == restriction + assert history.version is None assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1 activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get() assert activity.user == self.request.user @@ -156,7 +159,9 @@ def test_is_submission_allowed_ip_restricted(self, incr_mock): assert activity.requestfingerprintlog.signals == ['TAG', 'ANOTHERTAG'] def test_is_submission_allowed_email_restricted(self, incr_mock): - EmailUserRestriction.objects.create(email_pattern=self.request.user.email) + restriction = EmailUserRestriction.objects.create( + email_pattern=self.request.user.email + ) checker = RestrictionChecker(request=self.request) assert not checker.is_submission_allowed() assert checker.get_error_message() == ( @@ -175,6 +180,8 @@ def test_is_submission_allowed_email_restricted(self, incr_mock): assert history.user == self.request.user assert history.last_login_ip == self.request.user.last_login_ip assert history.ip_address == '10.0.0.1' + assert history.restriction_instance == restriction + assert history.version is None assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1 activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get() assert activity.user == self.request.user @@ -184,7 +191,7 @@ def test_is_submission_allowed_email_restricted(self, incr_mock): assert activity.requestfingerprintlog.signals == ['TAG', 'ANOTHERTAG'] def test_is_submission_allowed_ja4_restricted(self, incr_mock): - FingerprintRestriction.objects.create(ja4=self.ja4) + restriction = FingerprintRestriction.objects.create(ja4=self.ja4) checker = RestrictionChecker(request=self.request) assert not checker.is_submission_allowed() assert checker.get_error_message() == ( @@ -203,6 +210,8 @@ def test_is_submission_allowed_ja4_restricted(self, incr_mock): assert history.user == self.request.user assert history.last_login_ip == self.request.user.last_login_ip assert history.ip_address == '10.0.0.1' + assert history.restriction_instance == restriction + assert history.version is None assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1 activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get() assert activity.user == self.request.user @@ -212,7 +221,7 @@ def test_is_submission_allowed_ja4_restricted(self, incr_mock): assert activity.requestfingerprintlog.signals == ['TAG', 'ANOTHERTAG'] def test_is_submission_allowed_asn_restricted(self, incr_mock): - AsnUserRestriction.objects.create(asn=self.asn) + restriction = AsnUserRestriction.objects.create(asn=self.asn) checker = RestrictionChecker(request=self.request) assert not checker.is_submission_allowed() assert checker.get_error_message() == ( @@ -232,6 +241,8 @@ def test_is_submission_allowed_asn_restricted(self, incr_mock): assert history.user == self.request.user assert history.last_login_ip == self.request.user.last_login_ip assert history.ip_address == '10.0.0.1' + assert history.restriction_instance == restriction + assert history.version is None assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1 activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get() assert activity.user == self.request.user @@ -248,7 +259,9 @@ def test_is_submission_allowed_bypassing_read_dev_agreement_restricted( # this time, we're restricted by email while bypassing the read dev # agreement check. This ensures even when bypassing that check, we # still record everything properly when restricting. - EmailUserRestriction.objects.create(email_pattern=self.request.user.email) + restriction = EmailUserRestriction.objects.create( + email_pattern=self.request.user.email + ) checker = RestrictionChecker(request=self.request) assert not checker.is_submission_allowed(check_dev_agreement=False) assert checker.get_error_message() == ( @@ -267,6 +280,8 @@ def test_is_submission_allowed_bypassing_read_dev_agreement_restricted( assert history.user == self.request.user assert history.last_login_ip == self.request.user.last_login_ip assert history.ip_address == '10.0.0.1' + assert history.restriction_instance == restriction + assert history.version is None assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1 activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get() assert activity.user == self.request.user @@ -297,7 +312,7 @@ def test_is_auto_approval_allowed_email_restricted_only_for_submission( ) def test_is_auto_approval_allowed_email_restricted(self, incr_mock): - EmailUserRestriction.objects.create( + restriction = EmailUserRestriction.objects.create( email_pattern=self.request.user.email, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, ) @@ -328,6 +343,8 @@ def test_is_auto_approval_allowed_email_restricted(self, incr_mock): assert history.user == self.request.user assert history.last_login_ip == self.request.user.last_login_ip assert history.ip_address == '10.0.0.2' + assert history.restriction_instance == restriction + assert history.version is None assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1 activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get() assert activity.user == self.request.user @@ -339,7 +356,7 @@ def test_is_auto_approval_allowed_email_restricted(self, incr_mock): assert activity.requestfingerprintlog.signals == ['TAG2', 'ANOTHERTAG2'] def test_is_auto_approval_allowed_ja4_restricted(self, incr_mock): - FingerprintRestriction.objects.create( + restriction = FingerprintRestriction.objects.create( ja4=self.ja4, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, ) @@ -370,6 +387,8 @@ def test_is_auto_approval_allowed_ja4_restricted(self, incr_mock): assert history.user == self.request.user assert history.last_login_ip == self.request.user.last_login_ip assert history.ip_address == '10.0.0.2' + assert history.restriction_instance == restriction + assert history.version is None assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1 activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get() assert activity.user == self.request.user @@ -438,6 +457,179 @@ def test_is_auto_approval_allowed_with_mocks(self, incr_mock): for restriction_mock in allow_auto_approval_mocks: assert restriction_mock.call_count == 1 + def test_history_records_matched_instance_on_submission(self, incr_mock): + restriction = EmailUserRestriction.objects.create( + email_pattern=self.request.user.email + ) + checker = RestrictionChecker(request=self.request) + assert not checker.is_submission_allowed() + history = UserRestrictionHistory.objects.get() + assert history.get_restriction_display() == 'EmailUserRestriction' + assert history.restriction_instance == restriction + assert history.restriction_content_type == ContentType.objects.get_for_model( + EmailUserRestriction + ) + assert history.restriction_object_id == restriction.pk + assert history.version is None + assert checker.history_entries == [history] + + def test_history_one_row_per_matched_instance(self, incr_mock): + exact = IPNetworkUserRestriction.objects.create(network='10.0.0.1/32') + subnet = IPNetworkUserRestriction.objects.create(network='10.0.0.0/24') + checker = RestrictionChecker(request=self.request) + assert not checker.is_submission_allowed() + entries = UserRestrictionHistory.objects.filter(user=self.request.user) + assert entries.count() == 2 + assert {entry.restriction_instance for entry in entries} == {exact, subnet} + for entry in entries: + assert entry.get_restriction_display() == 'IPNetworkUserRestriction' + assert entry.ip_address == '10.0.0.1' + assert entry.last_login_ip == self.request.user.last_login_ip + assert entry.version is None + assert set(checker.history_entries) == set(entries) + # Still a single failed class: one activity log entry and one failure + # statsd increment for the class, plus the overall failure increment. + assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1 + assert incr_mock.call_count == 2 + + def test_history_instance_fields_null_when_nothing_matched(self, incr_mock): + self.request.user.update(read_dev_agreement=None) + checker = RestrictionChecker(request=self.request) + assert not checker.is_submission_allowed() + history = UserRestrictionHistory.objects.get() + assert history.get_restriction_display() == 'DeveloperAgreementRestriction' + assert history.restriction_instance is None + assert history.restriction_content_type is None + assert history.restriction_object_id is None + assert history.version is None + assert checker.history_entries == [history] + + def test_history_records_matched_instance_on_auto_approval(self, incr_mock): + restriction = EmailUserRestriction.objects.create( + email_pattern=self.request.user.email, + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + upload = FileUpload.objects.create( + user=self.request.user, + ip_address='10.0.0.2', + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + ) + checker = RestrictionChecker(upload=upload) + assert not checker.is_auto_approval_allowed() + history = UserRestrictionHistory.objects.get() + assert history.get_restriction_display() == 'EmailUserRestriction' + assert history.restriction_instance == restriction + assert history.version is None + assert checker.history_entries == [history] + + def test_history_records_matched_instance_on_rating_moderation(self, incr_mock): + restriction = EmailUserRestriction.objects.create( + email_pattern=self.request.user.email, + restriction_type=RESTRICTION_TYPES.RATING_MODERATE, + ) + checker = RestrictionChecker(request=self.request) + assert checker.should_moderate_rating() + history = UserRestrictionHistory.objects.get() + assert history.get_restriction_display() == 'EmailUserRestriction' + assert history.restriction_instance == restriction + assert history.version is None + assert checker.history_entries == [history] + + def test_history_entries_empty_on_success(self, incr_mock): + checker = RestrictionChecker(request=self.request) + assert checker.is_submission_allowed() + assert checker.history_entries == [] + + def test_history_two_matching_email_restrictions_on_auto_approval(self, incr_mock): + exact = EmailUserRestriction.objects.create( + email_pattern=self.request.user.email, + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + wildcard = EmailUserRestriction.objects.create( + email_pattern='*@%s' % self.request.user.email.rsplit('@', maxsplit=1)[-1], + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + upload = FileUpload.objects.create( + user=self.request.user, + ip_address='10.0.0.2', + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + ) + checker = RestrictionChecker(upload=upload) + assert not checker.is_auto_approval_allowed() + entries = UserRestrictionHistory.objects.filter(user=self.request.user) + assert entries.count() == 2 + assert {entry.restriction_instance for entry in entries} == {exact, wildcard} + for entry in entries: + assert entry.get_restriction_display() == 'EmailUserRestriction' + assert entry.version is None + assert set(checker.history_entries) == set(entries) + # A single failed class: one activity log entry, carrying both + # matched instance pks in its details. + assert ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).count() == 1 + activity = ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).get() + assert activity.details['restriction'] == 'EmailUserRestriction' + assert set(activity.details['restriction_ids']) == {exact.pk, wildcard.pk} + + def test_history_instance_fields_null_on_structural_ip_denial(self, incr_mock): + # An unparseable last_login_ip makes IPNetworkUserRestriction deny + # auto-approval structurally: the check fails, but no specific + # restriction instance matched, and a warning is logged. + self.request.user.update(last_login_ip='not.an.ip.address') + IPNetworkUserRestriction.objects.create( + network='10.0.0.0/24', + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + upload = FileUpload.objects.create( + user=self.request.user, + ip_address='10.0.0.2', + source=amo.UPLOAD_SOURCE_DEVHUB, + channel=amo.CHANNEL_LISTED, + ) + checker = RestrictionChecker(upload=upload) + with mock.patch('olympia.users.utils.log.warning') as warning_mock: + assert not checker.is_auto_approval_allowed() + warning_mock.assert_called_once_with( + 'No matching restrictions found for failed %s check on %s', + 'auto_approval', + 'IPNetworkUserRestriction', + ) + history = UserRestrictionHistory.objects.get() + assert history.get_restriction_display() == 'IPNetworkUserRestriction' + assert history.restriction_instance is None + assert history.restriction_content_type is None + assert history.restriction_object_id is None + assert history.version is None + assert checker.history_entries == [history] + + def test_no_enumeration_or_recording_for_unauthenticated_failure(self, incr_mock): + # The full stack never runs the checker unauthenticated (several + # allow_*() fast paths assume an authenticated user), so patch the + # fast paths like test_is_submission_allowed_with_mocks() does and + # prove the guard directly: an unauthenticated failure enumerates + # nothing and records nothing. + self.request.user = None + checker = RestrictionChecker(request=self.request) + with ExitStack() as stack: + for _, cls in UserRestrictionHistory.RESTRICTION_CLASSES_CHOICES: + stack.enter_context( + mock.patch.object( + cls, + 'allow_submission', + return_value=cls is not IPNetworkUserRestriction, + ) + ) + enumeration_mock = stack.enter_context( + mock.patch.object(IPNetworkUserRestriction, 'get_matching_restrictions') + ) + assert not checker.is_submission_allowed() + enumeration_mock.assert_not_called() + assert checker.failed_restrictions == [IPNetworkUserRestriction] + assert checker.history_entries == [] + assert not UserRestrictionHistory.objects.exists() + assert not ActivityLog.objects.filter(action=amo.LOG.RESTRICTED.id).exists() + class TestCheckSuppressedEmailConfirmation(TestCase): def setUp(self): diff --git a/src/olympia/users/utils.py b/src/olympia/users/utils.py index 4c48ebdb8d01..b159de962345 100644 --- a/src/olympia/users/utils.py +++ b/src/olympia/users/utils.py @@ -155,10 +155,26 @@ def __init__(self, *, request=None, upload=None): else: raise ImproperlyConfigured('RestrictionChecker needs a request or upload') self.failed_restrictions = [] + # UserRestrictionHistory instances created by the checks, exposed so + # that callers can annotate them further (Version.from_upload() sets + # version on them after the version has been created). + self.history_entries = [] def _is_action_allowed(self, action_type, *, restriction_choices=None): - from olympia.users.models import UserRestrictionHistory - + from olympia.users.models import RESTRICTION_TYPES, UserRestrictionHistory + + # Maps the action to the restriction_type its allow_*() method checks + # against - allow_submission(), allow_auto_approval(), allow_rating() + # and allow_rating_without_moderation() respectively. Keep in sync + # with those methods. 'rating_without_moderation' deliberately maps + # to RATING_MODERATE: the action is phrased as an allow-check, but + # the restrictions it consults are the flag-for-moderation ones. + restriction_type = { + 'submission': RESTRICTION_TYPES.ADDON_SUBMISSION, + 'auto_approval': RESTRICTION_TYPES.ADDON_APPROVAL, + 'rating': RESTRICTION_TYPES.RATING, + 'rating_without_moderation': RESTRICTION_TYPES.RATING_MODERATE, + }[action_type] if restriction_choices is None: # We use UserRestrictionHistory.RESTRICTION_CLASSES_CHOICES because it # currently matches the order we want to check things. If that ever @@ -181,20 +197,54 @@ def _is_action_allowed(self, action_type, *, restriction_choices=None): f'RestrictionChecker.is_{action_type}_allowed.{name}.failure' ) if self.user and self.user.is_authenticated: + # Slow path: enumerate which restriction(s) matched, so + # that each match gets its own history row pointing at + # the instance. Only for authenticated users - nothing + # is recorded otherwise. Stateless classes have no + # get_matching_restrictions(). + is_db_backed = hasattr(cls, 'get_matching_restrictions') + matched = ( + cls.get_matching_restrictions( + argument, restriction_type=restriction_type + ) + if is_db_backed + else [] + ) + if is_db_backed and not matched: + # Either the fast and slow predicates disagree, or + # the denial was structural (e.g. an unparseable IP) + # and there is no instance to record. + log.warning( + 'No matching restrictions found for failed %s check on %s', + action_type, + name, + ) with core.override_remote_addr_or_metadata( ip_address=self.ip_address, metadata=self.request_metadata ): activity.log_create( amo.LOG.RESTRICTED, user=self.user, - details={'restriction': str(cls.__name__)}, + details={ + 'restriction': str(cls.__name__), + 'restriction_ids': [ + restriction.pk for restriction in matched + ], + }, + ) + # A failure with nothing enumerable still gets a single + # row, with the instance fields left NULL - today's + # behaviour. + for matched_instance in matched or [None]: + self.history_entries.append( + UserRestrictionHistory.objects.create( + user=self.user, + ip_address=self.ip_address, + last_login_ip=self.user.last_login_ip or '', + restriction=restriction_number, + restriction_instance=matched_instance, + ) ) - UserRestrictionHistory.objects.create( - user=self.user, - ip_address=self.ip_address, - last_login_ip=self.user.last_login_ip or '', - restriction=restriction_number, - ) suffix = 'success' if not self.failed_restrictions else 'failure' statsd.incr(f'RestrictionChecker.is_{action_type}_allowed.%s' % suffix) return not self.failed_restrictions From da750bd7dfba2f57a168debdd6e8a565efe4887d Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Wed, 26 Aug 2026 16:50:05 +0100 Subject: [PATCH 04/13] Link restriction history to the version denied auto-approval The RestrictionChecker runs inside Version.from_upload() before the version exists, so the history rows it writes can't say which version they were about. After this commit, from_upload() now updates the rows the checker exposes on history_entries with the version it just created, scoped to exactly those pks. The DISABLE_AUTO_APPROVAL activity log entry also becomes traceable. Its details gain 'restrictions' (the failed class name' AND 'restriction_history_ids', AND the human-readable comment shown in the reviewer tools. The important changes history section now names the classes: - listed auto-approval automatically disabled because of a restriction (EmailUserRestriction). The text before the parenthesis is unchanged, and the version is deliberately not added to the ActivityLog arguments, that would change where the entry renders. --- src/olympia/versions/models.py | 16 +++- src/olympia/versions/tests/test_models.py | 91 +++++++++++++++++++---- 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/src/olympia/versions/models.py b/src/olympia/versions/models.py index 21a864b9cfb3..7f9ea2cfee69 100644 --- a/src/olympia/versions/models.py +++ b/src/olympia/versions/models.py @@ -54,7 +54,7 @@ TranslatedField, save_signal, ) -from olympia.users.models import UserProfile +from olympia.users.models import UserProfile, UserRestrictionHistory from olympia.users.utils import RestrictionChecker, get_task_user from olympia.versions.compare import version_int from olympia.zadmin.models import get_config @@ -571,13 +571,22 @@ def from_upload( reviewer_flags_defaults['auto_approval_disabled'] = True # Check if the approval should be restricted - if not RestrictionChecker(upload=upload).is_auto_approval_allowed(): + checker = RestrictionChecker(upload=upload) + if not checker.is_auto_approval_allowed(): flag = ( 'auto_approval_disabled' if channel == amo.CHANNEL_LISTED else 'auto_approval_disabled_unlisted' ) reviewer_flags_defaults[flag] = True + failed_names = [cls.__name__ for cls in checker.failed_restrictions] + history_ids = [entry.pk for entry in checker.history_entries] + # The checker ran before the version existed, so it could not + # record it on the history rows itself; backfill it now. + if history_ids: + UserRestrictionHistory.objects.filter(pk__in=history_ids).update( + version=version + ) activity.log_create( amo.LOG.DISABLE_AUTO_APPROVAL, addon, @@ -586,7 +595,10 @@ def from_upload( 'comments': ( f'{version.get_channel_display()} auto-approval automatically ' 'disabled because of a restriction' + f' ({", ".join(failed_names)})' ), + 'restrictions': failed_names, + 'restriction_history_ids': history_ids, }, user=get_task_user(), ) diff --git a/src/olympia/versions/tests/test_models.py b/src/olympia/versions/tests/test_models.py index 80dd7f3fcb3f..81ff85dd894b 100644 --- a/src/olympia/versions/tests/test_models.py +++ b/src/olympia/versions/tests/test_models.py @@ -39,6 +39,7 @@ EmailUserRestriction, IPNetworkUserRestriction, UserProfile, + UserRestrictionHistory, ) from olympia.users.utils import get_task_user from olympia.zadmin.models import set_config @@ -2535,12 +2536,12 @@ def test_auto_approval_not_disabled_if_not_restricted(self): ) def test_auto_approval_disabled_if_restricted_by_email(self): - EmailUserRestriction.objects.create( + restriction = EmailUserRestriction.objects.create( email_pattern=self.upload.user.email, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, ) assert not AddonReviewerFlags.objects.filter(addon=self.addon).exists() - Version.from_upload( + version = Version.from_upload( self.upload, self.addon, amo.CHANNEL_LISTED, @@ -2560,19 +2561,24 @@ def test_auto_approval_disabled_if_restricted_by_email(self): .get() ) assert activity_log.details['channel'] == amo.CHANNEL_LISTED - assert ( - activity_log.details['comments'] - == 'Listed auto-approval automatically disabled because of a restriction' + assert activity_log.details['comments'] == ( + 'Listed auto-approval automatically disabled because of a ' + 'restriction (EmailUserRestriction)' ) + assert activity_log.details['restrictions'] == ['EmailUserRestriction'] assert activity_log.user == get_task_user() + history = UserRestrictionHistory.objects.get() + assert activity_log.details['restriction_history_ids'] == [history.pk] + assert history.restriction_instance == restriction + assert history.version == version def test_auto_approval_disabled_if_restricted_by_ip(self): self.upload.user.update(last_login_ip='10.0.0.42') - IPNetworkUserRestriction.objects.create( + restriction = IPNetworkUserRestriction.objects.create( network='10.0.0.0/24', restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL ) assert not AddonReviewerFlags.objects.filter(addon=self.addon).exists() - Version.from_upload( + version = Version.from_upload( self.upload, self.addon, amo.CHANNEL_LISTED, @@ -2592,19 +2598,24 @@ def test_auto_approval_disabled_if_restricted_by_ip(self): .get() ) assert activity_log.details['channel'] == amo.CHANNEL_LISTED - assert ( - activity_log.details['comments'] - == 'Listed auto-approval automatically disabled because of a restriction' + assert activity_log.details['comments'] == ( + 'Listed auto-approval automatically disabled because of a ' + 'restriction (IPNetworkUserRestriction)' ) + assert activity_log.details['restrictions'] == ['IPNetworkUserRestriction'] assert activity_log.user == get_task_user() + history = UserRestrictionHistory.objects.get() + assert activity_log.details['restriction_history_ids'] == [history.pk] + assert history.restriction_instance == restriction + assert history.version == version def test_auto_approval_disabled_for_unlisted_if_restricted_by_ip(self): self.upload.user.update(last_login_ip='10.0.0.42') - IPNetworkUserRestriction.objects.create( + restriction = IPNetworkUserRestriction.objects.create( network='10.0.0.0/24', restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL ) assert not AddonReviewerFlags.objects.filter(addon=self.addon).exists() - Version.from_upload( + version = Version.from_upload( self.upload, self.addon, amo.CHANNEL_UNLISTED, @@ -2624,11 +2635,61 @@ def test_auto_approval_disabled_for_unlisted_if_restricted_by_ip(self): .get() ) assert activity_log.details['channel'] == amo.CHANNEL_UNLISTED - assert ( - activity_log.details['comments'] - == 'Unlisted auto-approval automatically disabled because of a restriction' + assert activity_log.details['comments'] == ( + 'Unlisted auto-approval automatically disabled because of a ' + 'restriction (IPNetworkUserRestriction)' ) + assert activity_log.details['restrictions'] == ['IPNetworkUserRestriction'] assert activity_log.user == get_task_user() + history = UserRestrictionHistory.objects.get() + assert activity_log.details['restriction_history_ids'] == [history.pk] + assert history.restriction_instance == restriction + assert history.version == version + + def test_auto_approval_disabled_if_restricted_by_email_and_ip(self): + # The IP restriction matches through last_login_ip, deliberately, so + # that it records an instance rather than denying structurally. + self.upload.user.update(last_login_ip='10.0.0.42') + email_restriction = EmailUserRestriction.objects.create( + email_pattern=self.upload.user.email, + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + ip_restriction = IPNetworkUserRestriction.objects.create( + network='10.0.0.0/24', restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL + ) + version = Version.from_upload( + self.upload, + self.addon, + amo.CHANNEL_LISTED, + selected_apps=[self.selected_app], + parsed_data=self.dummy_parsed_data, + ) + assert self.addon.auto_approval_disabled + activity_log = ( + ActivityLog.objects.for_addons(self.addon) + .filter(action=amo.LOG.DISABLE_AUTO_APPROVAL.id) + .get() + ) + # Class names appear in checker order. + assert activity_log.details['comments'] == ( + 'Listed auto-approval automatically disabled because of a ' + 'restriction (EmailUserRestriction, IPNetworkUserRestriction)' + ) + assert activity_log.details['restrictions'] == [ + 'EmailUserRestriction', + 'IPNetworkUserRestriction', + ] + entries = UserRestrictionHistory.objects.filter(user=self.upload.user) + assert entries.count() == 2 + assert sorted(activity_log.details['restriction_history_ids']) == sorted( + entry.pk for entry in entries + ) + assert {entry.restriction_instance for entry in entries} == { + email_restriction, + ip_restriction, + } + for entry in entries: + assert entry.version == version def test_dont_record_install_origins_when_waffle_switch_is_off(self): # Switch should be off by default. From 3cdcf4ebfbd90cc7a5b78cd7886caf05c79b1b76 Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Thu, 3 Sep 2026 10:32:20 +0100 Subject: [PATCH 05/13] Use a plain Index with a short name for the instance lookup From some review feedback in #25344: LongNameIndex exists to grandfather some legacy index names that predate a cap I didn't know about. New indexes shoudld fit the cap with a plain models.index. As the migration is unmerged, this is an edit in place rather than adding a rename migration. --- ...userrestrictionhistory_restriction_instance_and_version.py | 3 +-- src/olympia/users/models.py | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/olympia/users/migrations/0027_userrestrictionhistory_restriction_instance_and_version.py b/src/olympia/users/migrations/0027_userrestrictionhistory_restriction_instance_and_version.py index 0398bd3b5949..69716790a3c8 100644 --- a/src/olympia/users/migrations/0027_userrestrictionhistory_restriction_instance_and_version.py +++ b/src/olympia/users/migrations/0027_userrestrictionhistory_restriction_instance_and_version.py @@ -1,7 +1,6 @@ # Generated by Django 5.2.17 on 2026-08-25 14:53 import django.db.models.deletion -import olympia.amo.models from django.db import migrations, models @@ -31,6 +30,6 @@ class Migration(migrations.Migration): ), migrations.AddIndex( model_name='userrestrictionhistory', - index=olympia.amo.models.LongNameIndex(fields=['restriction_content_type', 'restriction_object_id'], name='users_userrestrictionhistory_restriction_content_type_object_id'), + index=models.Index(fields=['restriction_content_type', 'restriction_object_id'], name='urh_restriction_instance_idx'), ), ] diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index 92f43a4d0bfe..10bbf9429aad 100644 --- a/src/olympia/users/models.py +++ b/src/olympia/users/models.py @@ -1601,9 +1601,9 @@ class Meta: fields=('last_login_ip',), name='users_userrestrictionhistory_last_login_ip_d58d95ff', ), - LongNameIndex( + models.Index( fields=('restriction_content_type', 'restriction_object_id'), - name='users_userrestrictionhistory_restriction_content_type_object_id', + name='urh_restriction_instance_idx', ), ] From f4e40b5e6277d79b4b0f8a4e876e1e5aa006110a Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Thu, 3 Sep 2026 10:49:34 +0100 Subject: [PATCH 06/13] Refuse restriction_instance values that aren't restrictions From review feedback: Is there any way we can add a restriction so we can't end up with teh fk pointing to a random other model. After looking into this, a generic foreign key with no constraint could rightly point at any model. There is no database-level way to pin content type, so we've enforced it in save(): restriction_content_type must resolve to a subclass of RestrictionAbstractBaseModel. This check is dynamic rather than a hardcoded list of five current restriction models, so a future restriction class is allowed automatically. --- src/olympia/users/models.py | 15 +++++++++++++++ src/olympia/users/tests/test_models.py | 23 +++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index 10bbf9429aad..db0b77906eb1 100644 --- a/src/olympia/users/models.py +++ b/src/olympia/users/models.py @@ -1607,6 +1607,21 @@ class Meta: ), ] + def save(self, *args, **kwargs): + # There is no database-level way to constrain a generic foreign key + # to specific models (content type ids aren't stable across + # environments), so enforce it here: the matched instance must be one + # of the database-backed restriction models. Deliberately dynamic - + # any future subclass of RestrictionAbstractBaseModel is allowed + # without changes here. + if self.restriction_content_type is not None: + model = self.restriction_content_type.model_class() + if model is None or not issubclass(model, RestrictionAbstractBaseModel): + raise ValueError( + 'restriction_instance must point at a database-backed restriction' + ) + super().save(*args, **kwargs) + class UserHistory(ModelBase): id = PositiveAutoField(primary_key=True) diff --git a/src/olympia/users/tests/test_models.py b/src/olympia/users/tests/test_models.py index 8b87b2e322e6..2d75194fdb3e 100644 --- a/src/olympia/users/tests/test_models.py +++ b/src/olympia/users/tests/test_models.py @@ -53,6 +53,7 @@ SuppressedEmailVerification, UserEmailField, UserProfile, + UserRestrictionHistory, generate_auth_id, get_anonymized_username, ) @@ -2160,6 +2161,28 @@ def test_get_matching_restrictions_no_asn(self): ) +class TestUserRestrictionHistory(TestCase): + def test_restriction_instance_must_be_a_restriction(self): + user = user_factory() + # Anything that isn't a database-backed restriction is refused. + with self.assertRaises(ValueError): + UserRestrictionHistory.objects.create( + user=user, restriction_instance=user_factory() + ) + assert not UserRestrictionHistory.objects.exists() + + def test_restriction_instance_restriction_or_nothing_is_fine(self): + user = user_factory() + restriction = EmailUserRestriction.objects.create(email_pattern='foo@bar.com') + UserRestrictionHistory.objects.create( + user=user, restriction_instance=restriction + ) + # No instance at all is a legitimate state (stateless restrictions, + # structural denials). + UserRestrictionHistory.objects.create(user=user) + assert UserRestrictionHistory.objects.count() == 2 + + @override_settings( REPUTATION_SERVICE_URL='https://reputation.example.com', REPUTATION_SERVICE_TOKEN='fancy_token', From b5efb722f814c424d4dea02dbea7f856ac905cee Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Thu, 3 Sep 2026 11:07:27 +0100 Subject: [PATCH 07/13] Declare get_matching_restrictions() on the database-backed base class Following review feedback: this commit declares the slow-path enumeration on RestrictionAbstratBaseModel, and raises NotImplemtedError by default. This stops fuuture database-backed restriction classes to be added without this being added. It mirrors how the RestrictionAbstractBase declares allow_auto_approval() and allow_request(). --- src/olympia/users/models.py | 12 ++++++++++++ src/olympia/users/tests/test_models.py | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index db0b77906eb1..b3e908dafa0a 100644 --- a/src/olympia/users/models.py +++ b/src/olympia/users/models.py @@ -922,6 +922,18 @@ class RestrictionAbstractBaseModel(ModelBase, RestrictionAbstractBase): class Meta: abstract = True + @classmethod + def get_matching_restrictions(cls, argument, *, restriction_type): + """ + Return a list of the restrictions matching the given request or + upload (which one depends on the restriction_type being checked). + + Slow path, meant to be called after the corresponding allow_*() check + has already failed, in order to record which restriction(s) matched. + """ + # Should be implemented by child classes. + raise NotImplementedError + class IPNetworkUserRestriction(RestrictionAbstractBaseModel): id = PositiveAutoField(primary_key=True) diff --git a/src/olympia/users/tests/test_models.py b/src/olympia/users/tests/test_models.py index 2d75194fdb3e..cf2cc658049e 100644 --- a/src/olympia/users/tests/test_models.py +++ b/src/olympia/users/tests/test_models.py @@ -49,6 +49,7 @@ FingerprintRestriction, IPNetworkUserRestriction, IPReputationRestriction, + RestrictionAbstractBaseModel, SuppressedEmail, SuppressedEmailVerification, UserEmailField, @@ -2183,6 +2184,23 @@ def test_restriction_instance_restriction_or_nothing_is_fine(self): assert UserRestrictionHistory.objects.count() == 2 +class TestRestrictionAbstractBaseModel(TestCase): + def test_get_matching_restrictions_raises_if_not_implemented(self): + with self.assertRaises(NotImplementedError): + RestrictionAbstractBaseModel.get_matching_restrictions( + None, restriction_type=RESTRICTION_TYPES.ADDON_SUBMISSION + ) + + def test_get_matching_restrictions_implemented_by_all_children(self): + # Every database-backed restriction class must override the slow-path + # enumeration, not fall back to the base implementation. + for _, cls in UserRestrictionHistory.RESTRICTION_CLASSES_CHOICES: + if issubclass(cls, RestrictionAbstractBaseModel): + assert 'get_matching_restrictions' in cls.__dict__, ( + f'{cls.__name__} must implement get_matching_restrictions()' + ) + + @override_settings( REPUTATION_SERVICE_URL='https://reputation.example.com', REPUTATION_SERVICE_TOKEN='fancy_token', From 9b1418e17da1b766d047be8b8bf0f653659dc8df Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Thu, 3 Sep 2026 12:21:24 +0100 Subject: [PATCH 08/13] Add end-to-end tests for restriction instance recording After a spot of review feedback we've decided to add some functional tests proving we record the right info when hitting a restriction. When hitting, we show it in the reviewer tool. Both tests drive the real signing API. The first proves the recorded chain: the UserRestrictionHistory row points at the exact restriction instance and the created version. The DISABLE_AUTO_APPROVAL activity log carries the restrictions details and comment. The second continues the journey to the reviewer: after the same submission, a user with Addons:Review opening the review page sees "... automation disabled because of a restriction (EmailUserRestriction)" in the addons important-changes history. --- src/olympia/signing/tests/test_views.py | 55 +++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/olympia/signing/tests/test_views.py b/src/olympia/signing/tests/test_views.py index 416de083e742..740999b532b4 100644 --- a/src/olympia/signing/tests/test_views.py +++ b/src/olympia/signing/tests/test_views.py @@ -7,6 +7,7 @@ from django.forms import ValidationError from django.test.testcases import TransactionTestCase from django.test.utils import override_settings +from django.urls import reverse from django.utils import translation import responses @@ -25,11 +26,13 @@ developer_factory, get_random_ip, reverse_ns, + user_factory, ) from olympia.api.tests.utils import APIKeyAuthTestMixin from olympia.files.models import File, FileUpload from olympia.files.utils import get_sha256 from olympia.users.models import ( + RESTRICTION_TYPES, EmailUserRestriction, IPNetworkUserRestriction, UserProfile, @@ -263,6 +266,58 @@ def test_version_added(self): assert provenance.source == amo.UPLOAD_SOURCE_SIGNING_API assert provenance.client_info == 'web-ext/12.34' + def test_restriction_instance_recorded_on_auto_approval_denial(self): + # End to end: a restriction denying auto-approval during a real API + # submission is recorded with the specific matching instance, linked + # to the version that was created. + user_factory(pk=settings.TASK_USER_ID) # DISABLE_AUTO_APPROVAL author. + restriction = EmailUserRestriction.objects.create( + email_pattern=self.user.email, + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + response = self.request('PUT', self.url(self.guid, '3.0')) + assert response.status_code == 202 + + version = Version.objects.get(addon__guid=self.guid, version='3.0') + history = UserRestrictionHistory.objects.get(user=self.user) + assert history.get_restriction_display() == 'EmailUserRestriction' + assert history.restriction_instance == restriction + assert history.version == version + activity_log = ActivityLog.objects.filter( + action=amo.LOG.DISABLE_AUTO_APPROVAL.id + ).get() + assert activity_log.details['restrictions'] == ['EmailUserRestriction'] + assert activity_log.details['restriction_history_ids'] == [history.pk] + assert activity_log.details['comments'] == ( + 'Listed auto-approval automatically disabled because of a ' + 'restriction (EmailUserRestriction)' + ) + assert version.addon.auto_approval_disabled + + def test_restriction_shown_in_reviewer_tools_after_denial(self): + # End to end: after a restriction denies auto-approval during a real + # API submission, a reviewer opening the review page sees which + # restriction fired. + user_factory(pk=settings.TASK_USER_ID) # DISABLE_AUTO_APPROVAL author. + EmailUserRestriction.objects.create( + email_pattern=self.user.email, + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + response = self.request('PUT', self.url(self.guid, '3.0')) + assert response.status_code == 202 + + reviewer = user_factory() + self.grant_permission(reviewer, amo.permissions.ADDONS_REVIEW) + self.client.force_login(reviewer) + response = self.client.get( + reverse('reviewers.review', args=[Addon.objects.get(guid=self.guid).pk]) + ) + self.assertContains( + response, + 'Listed auto-approval automatically disabled because of a ' + 'restriction (EmailUserRestriction)', + ) + def test_version_already_uploaded(self): response = self.request('PUT', self.url(self.guid, '3.0')) assert response.status_code == 202 From 7b4055090c9d841c2e6d5a3a546d50233486eefe Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Fri, 4 Sep 2026 14:06:53 +0100 Subject: [PATCH 09/13] Move restriction e2e tests to the addon API submission flow Following some feedback the end to end test were driving legacy signing APIs, which we eventually want to move away from. They now live in TestVersionViewCreate and submit through the v5 addon api. The exact same coverage as before, just moved locations. --- src/olympia/addons/tests/test_views.py | 58 ++++++++++++++++++++++++- src/olympia/signing/tests/test_views.py | 55 ----------------------- 2 files changed, 57 insertions(+), 56 deletions(-) diff --git a/src/olympia/addons/tests/test_views.py b/src/olympia/addons/tests/test_views.py index e2d65712a2d4..48b6d903deb2 100644 --- a/src/olympia/addons/tests/test_views.py +++ b/src/olympia/addons/tests/test_views.py @@ -61,7 +61,12 @@ from olympia.search.utils import get_es from olympia.tags.models import Tag from olympia.translations.models import Translation -from olympia.users.models import EmailUserRestriction, UserProfile +from olympia.users.models import ( + RESTRICTION_TYPES, + EmailUserRestriction, + UserProfile, + UserRestrictionHistory, +) from olympia.versions.models import ( ApplicationsVersions, AppVersion, @@ -4088,6 +4093,57 @@ def _submit_source(self, filepath, error=False): version = None return response, version + def test_restriction_instance_recorded_on_auto_approval_denial(self): + # End to end: a restriction denying auto-approval during a real API + # submission is recorded with the specific matching instance, linked + # to the version that was created. + user_factory(pk=settings.TASK_USER_ID) # DISABLE_AUTO_APPROVAL author. + restriction = EmailUserRestriction.objects.create( + email_pattern=self.user.email, + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + response = self.client.post(self.url, data=self.minimal_data) + assert response.status_code == 201, response.content + + self.addon.reload() + version = self.addon.find_latest_version(channel=None) + history = UserRestrictionHistory.objects.get(user=self.user) + assert history.get_restriction_display() == 'EmailUserRestriction' + assert history.restriction_instance == restriction + assert history.version == version + activity_log = ActivityLog.objects.filter( + action=amo.LOG.DISABLE_AUTO_APPROVAL.id + ).get() + assert activity_log.details['restrictions'] == ['EmailUserRestriction'] + assert activity_log.details['restriction_history_ids'] == [history.pk] + assert activity_log.details['comments'] == ( + 'Unlisted auto-approval automatically disabled because of a ' + 'restriction (EmailUserRestriction)' + ) + assert self.addon.auto_approval_disabled_unlisted + + def test_restriction_shown_in_reviewer_tools_after_denial(self): + # End to end: after a restriction denies auto-approval during a real + # API submission, a reviewer opening the review page sees which + # restriction fired. + user_factory(pk=settings.TASK_USER_ID) # DISABLE_AUTO_APPROVAL author. + EmailUserRestriction.objects.create( + email_pattern=self.user.email, + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + response = self.client.post(self.url, data=self.minimal_data) + assert response.status_code == 201, response.content + + reviewer = user_factory() + self.grant_permission(reviewer, amo.permissions.ADDONS_REVIEW) + self.client.force_login(reviewer) + response = self.client.get(reverse('reviewers.review', args=[self.addon.pk])) + self.assertContains( + response, + 'Unlisted auto-approval automatically disabled because of a ' + 'restriction (EmailUserRestriction)', + ) + class TestVersionViewSetCreateJWTAuth(TestVersionViewSetCreate): client_class = APITestClientJWT diff --git a/src/olympia/signing/tests/test_views.py b/src/olympia/signing/tests/test_views.py index 740999b532b4..416de083e742 100644 --- a/src/olympia/signing/tests/test_views.py +++ b/src/olympia/signing/tests/test_views.py @@ -7,7 +7,6 @@ from django.forms import ValidationError from django.test.testcases import TransactionTestCase from django.test.utils import override_settings -from django.urls import reverse from django.utils import translation import responses @@ -26,13 +25,11 @@ developer_factory, get_random_ip, reverse_ns, - user_factory, ) from olympia.api.tests.utils import APIKeyAuthTestMixin from olympia.files.models import File, FileUpload from olympia.files.utils import get_sha256 from olympia.users.models import ( - RESTRICTION_TYPES, EmailUserRestriction, IPNetworkUserRestriction, UserProfile, @@ -266,58 +263,6 @@ def test_version_added(self): assert provenance.source == amo.UPLOAD_SOURCE_SIGNING_API assert provenance.client_info == 'web-ext/12.34' - def test_restriction_instance_recorded_on_auto_approval_denial(self): - # End to end: a restriction denying auto-approval during a real API - # submission is recorded with the specific matching instance, linked - # to the version that was created. - user_factory(pk=settings.TASK_USER_ID) # DISABLE_AUTO_APPROVAL author. - restriction = EmailUserRestriction.objects.create( - email_pattern=self.user.email, - restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, - ) - response = self.request('PUT', self.url(self.guid, '3.0')) - assert response.status_code == 202 - - version = Version.objects.get(addon__guid=self.guid, version='3.0') - history = UserRestrictionHistory.objects.get(user=self.user) - assert history.get_restriction_display() == 'EmailUserRestriction' - assert history.restriction_instance == restriction - assert history.version == version - activity_log = ActivityLog.objects.filter( - action=amo.LOG.DISABLE_AUTO_APPROVAL.id - ).get() - assert activity_log.details['restrictions'] == ['EmailUserRestriction'] - assert activity_log.details['restriction_history_ids'] == [history.pk] - assert activity_log.details['comments'] == ( - 'Listed auto-approval automatically disabled because of a ' - 'restriction (EmailUserRestriction)' - ) - assert version.addon.auto_approval_disabled - - def test_restriction_shown_in_reviewer_tools_after_denial(self): - # End to end: after a restriction denies auto-approval during a real - # API submission, a reviewer opening the review page sees which - # restriction fired. - user_factory(pk=settings.TASK_USER_ID) # DISABLE_AUTO_APPROVAL author. - EmailUserRestriction.objects.create( - email_pattern=self.user.email, - restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, - ) - response = self.request('PUT', self.url(self.guid, '3.0')) - assert response.status_code == 202 - - reviewer = user_factory() - self.grant_permission(reviewer, amo.permissions.ADDONS_REVIEW) - self.client.force_login(reviewer) - response = self.client.get( - reverse('reviewers.review', args=[Addon.objects.get(guid=self.guid).pk]) - ) - self.assertContains( - response, - 'Listed auto-approval automatically disabled because of a ' - 'restriction (EmailUserRestriction)', - ) - def test_version_already_uploaded(self): response = self.request('PUT', self.url(self.guid, '3.0')) assert response.status_code == 202 From d3d4c79a88244f8794536626a76b22b2654e5c6b Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Fri, 4 Sep 2026 14:18:32 +0100 Subject: [PATCH 10/13] Test reviewer tools restriction display from recorded state From feedback: the reviewr facing half of the e2e pair doesn't need to drive the submission API. The recording part is already proven by the remaining API tests and unit tests. It was suggested I move this to the reviewers/tests/test_views.py next to the other important-changes tests. This makes sense in hindsight. The matched instance on a UserRestrictionHistory row linked to the verions, and the DISABLE_AUTO_APPROVAL entry naming the class in its comment and carrying the structural details. The review page then shows which restriction fired. --- src/olympia/addons/tests/test_views.py | 22 ----------- src/olympia/reviewers/tests/test_views.py | 45 ++++++++++++++++++++++- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/src/olympia/addons/tests/test_views.py b/src/olympia/addons/tests/test_views.py index 48b6d903deb2..fbdb56deaa02 100644 --- a/src/olympia/addons/tests/test_views.py +++ b/src/olympia/addons/tests/test_views.py @@ -4122,28 +4122,6 @@ def test_restriction_instance_recorded_on_auto_approval_denial(self): ) assert self.addon.auto_approval_disabled_unlisted - def test_restriction_shown_in_reviewer_tools_after_denial(self): - # End to end: after a restriction denies auto-approval during a real - # API submission, a reviewer opening the review page sees which - # restriction fired. - user_factory(pk=settings.TASK_USER_ID) # DISABLE_AUTO_APPROVAL author. - EmailUserRestriction.objects.create( - email_pattern=self.user.email, - restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, - ) - response = self.client.post(self.url, data=self.minimal_data) - assert response.status_code == 201, response.content - - reviewer = user_factory() - self.grant_permission(reviewer, amo.permissions.ADDONS_REVIEW) - self.client.force_login(reviewer) - response = self.client.get(reverse('reviewers.review', args=[self.addon.pk])) - self.assertContains( - response, - 'Unlisted auto-approval automatically disabled because of a ' - 'restriction (EmailUserRestriction)', - ) - class TestVersionViewSetCreateJWTAuth(TestVersionViewSetCreate): client_class = APITestClientJWT diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index f8a2d26d3109..c3285307939f 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -74,7 +74,13 @@ from olympia.reviewers.views import queue from olympia.scanners.models import ScannerResult, ScannerRule from olympia.stats.utils import VERSION_ADU_LIMIT -from olympia.users.models import UserProfile +from olympia.users.models import ( + RESTRICTION_TYPES, + EmailUserRestriction, + UserProfile, + UserRestrictionHistory, +) +from olympia.users.utils import get_task_user from olympia.versions.models import ( ApplicationsVersions, AppVersion, @@ -5358,6 +5364,43 @@ def test_important_changes_log(self): change_text = doc('#important-changes-history .activity tr:nth-child(8)').text() assert change_text.startswith('Auto-Approval disabled (Unlisted)') + def test_important_changes_shows_restriction_that_disabled_auto_approval(self): + # State created exactly as Version.from_upload() records it when a + # restriction denies auto-approval: the matched instance on a + # UserRestrictionHistory row linked to the version, and the + # DISABLE_AUTO_APPROVAL entry naming the class. + restriction = EmailUserRestriction.objects.create( + email_pattern=self.addon_author.email, + restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL, + ) + history = UserRestrictionHistory.objects.create( + user=self.addon_author, + restriction=2, # EmailUserRestriction + restriction_instance=restriction, + version=self.version, + ) + core.set_user(get_task_user()) + ActivityLog.objects.create( + amo.LOG.DISABLE_AUTO_APPROVAL, + self.addon, + details={ + 'channel': amo.CHANNEL_LISTED, + 'comments': ( + 'Listed auto-approval automatically disabled because of a ' + 'restriction (EmailUserRestriction)' + ), + 'restrictions': ['EmailUserRestriction'], + 'restriction_history_ids': [history.pk], + }, + ) + response = self.client.get(self.url) + assert response.status_code == 200 + self.assertContains( + response, + 'Listed auto-approval automatically disabled because of a ' + 'restriction (EmailUserRestriction)', + ) + def test_important_changes_log_with_versions_attached(self): version1 = self.addon.versions.get() version2 = version_factory(addon=self.addon) From d1701bf0db2ba106cd2c1ae5f189aeccc2497d56 Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Fri, 4 Sep 2026 16:41:46 +0100 Subject: [PATCH 11/13] Split the no-instance log by cause: error vs warning Following some feedback on throwing a warning here. A database-backed restriction failing its check while the slow path finds nothing that matched is a case that we never want to see quitely. It turned out to cover two situations of very different severity, so they are distinguished rather than raising the whole thing as an error. The get_matching_restrictions() function now returns None when the input needed for matching was missing or invalid. So an empty list now purely means the search ran and found nothing, which, given the fast path just denied, should be impossible. THAT is an error. Only IPNetworkUserRestriction ever returns None, because it is the only class whose fast path denies on bad input. The others allow when input is missing, so their slow paths can't be reached structually. --- src/olympia/users/models.py | 15 ++++++--- src/olympia/users/tests/test_models.py | 2 +- src/olympia/users/tests/test_user_utils.py | 38 +++++++++++++++++++++- src/olympia/users/utils.py | 23 ++++++++++--- 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index b3e908dafa0a..6629ec992e34 100644 --- a/src/olympia/users/models.py +++ b/src/olympia/users/models.py @@ -930,6 +930,10 @@ def get_matching_restrictions(cls, argument, *, restriction_type): Slow path, meant to be called after the corresponding allow_*() check has already failed, in order to record which restriction(s) matched. + + Returns None when the input needed for matching was missing or + invalid - a structural denial, there was nothing to search for. An + empty list means the search ran and nothing matched. """ # Should be implemented by child classes. raise NotImplementedError @@ -1042,19 +1046,20 @@ def get_matching_restrictions(cls, argument, *, restriction_type): Slow path, meant to be called after the corresponding allow_*() check has already failed in order to record which restriction(s) matched: - unlike the fast path it does not stop at the first match. Returns an - empty list when the input needed for matching is missing or invalid. + unlike the fast path it does not stop at the first match. Returns + None when the input needed for matching is missing or invalid, the + same condition allow_*() denies on structurally. """ # Mirrors the extraction in allow_auto_approval()/allow_request(). if restriction_type == RESTRICTION_TYPES.ADDON_APPROVAL: upload = argument if not upload.user or not upload.ip_address: - return [] + return None try: remote_addr = ipaddress.ip_address(upload.ip_address) user_last_login_ip = ipaddress.ip_address(upload.user.last_login_ip) except ValueError: - return [] + return None else: request = argument try: @@ -1067,7 +1072,7 @@ def get_matching_restrictions(cls, argument, *, restriction_type): else None ) except ValueError: - return [] + return None return [ restriction for restriction in cls.objects.filter(restriction_type=restriction_type) diff --git a/src/olympia/users/tests/test_models.py b/src/olympia/users/tests/test_models.py index cf2cc658049e..8071962ad539 100644 --- a/src/olympia/users/tests/test_models.py +++ b/src/olympia/users/tests/test_models.py @@ -1573,7 +1573,7 @@ def test_get_matching_restrictions_unparseable_ip(self): IPNetworkUserRestriction.get_matching_restrictions( upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL ) - == [] + is None ) diff --git a/src/olympia/users/tests/test_user_utils.py b/src/olympia/users/tests/test_user_utils.py index 3acb19d8a068..988e15e84bee 100644 --- a/src/olympia/users/tests/test_user_utils.py +++ b/src/olympia/users/tests/test_user_utils.py @@ -591,7 +591,8 @@ def test_history_instance_fields_null_on_structural_ip_denial(self, incr_mock): with mock.patch('olympia.users.utils.log.warning') as warning_mock: assert not checker.is_auto_approval_allowed() warning_mock.assert_called_once_with( - 'No matching restrictions found for failed %s check on %s', + 'Failed %s check on %s denied structurally: ' + 'input missing or invalid, no instance to record', 'auto_approval', 'IPNetworkUserRestriction', ) @@ -603,6 +604,41 @@ def test_history_instance_fields_null_on_structural_ip_denial(self, incr_mock): assert history.version is None assert checker.history_entries == [history] + def test_error_logged_when_enumeration_finds_nothing(self, incr_mock): + # A database-backed class denying while enumeration can't reproduce + # any match should be impossible - the predicates have drifted + # apart, or the restriction was deleted in between - and is logged + # as an error. + checker = RestrictionChecker(request=self.request) + with ExitStack() as stack: + for _, cls in UserRestrictionHistory.RESTRICTION_CLASSES_CHOICES: + stack.enter_context( + mock.patch.object( + cls, + 'allow_submission', + return_value=cls is not IPNetworkUserRestriction, + ) + ) + stack.enter_context( + mock.patch.object( + IPNetworkUserRestriction, + 'get_matching_restrictions', + return_value=[], + ) + ) + error_mock = stack.enter_context( + mock.patch('olympia.users.utils.log.error') + ) + assert not checker.is_submission_allowed() + error_mock.assert_called_once_with( + 'No matching restrictions found for failed %s check on %s', + 'submission', + 'IPNetworkUserRestriction', + ) + history = UserRestrictionHistory.objects.get() + assert history.restriction_instance is None + assert checker.history_entries == [history] + def test_no_enumeration_or_recording_for_unauthenticated_failure(self, incr_mock): # The full stack never runs the checker unauthenticated (several # allow_*() fast paths assume an authenticated user), so patch the diff --git a/src/olympia/users/utils.py b/src/olympia/users/utils.py index b159de962345..edb78e89788f 100644 --- a/src/olympia/users/utils.py +++ b/src/olympia/users/utils.py @@ -210,11 +210,24 @@ def _is_action_allowed(self, action_type, *, restriction_choices=None): if is_db_backed else [] ) - if is_db_backed and not matched: - # Either the fast and slow predicates disagree, or - # the denial was structural (e.g. an unparseable IP) - # and there is no instance to record. + if is_db_backed and matched is None: + # Structural denial: the input needed for matching + # was missing or invalid (e.g. an unparseable IP), + # so the user was blocked on data quality rather + # than a specific rule. Expected, but worth + # visibility. log.warning( + 'Failed %s check on %s denied structurally: ' + 'input missing or invalid, no instance to record', + action_type, + name, + ) + elif is_db_backed and not matched: + # Should be impossible: the fast path found a match + # that enumeration can't reproduce. Either the two + # predicates have drifted apart, or the restriction + # was deleted in between. + log.error( 'No matching restrictions found for failed %s check on %s', action_type, name, @@ -228,7 +241,7 @@ def _is_action_allowed(self, action_type, *, restriction_choices=None): details={ 'restriction': str(cls.__name__), 'restriction_ids': [ - restriction.pk for restriction in matched + restriction.pk for restriction in matched or [] ], }, ) From bde29dd7f147fe86216dfa34db082178627a3606 Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Fri, 4 Sep 2026 16:55:16 +0100 Subject: [PATCH 12/13] Drop change-relative wording from comment --- src/olympia/users/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/olympia/users/utils.py b/src/olympia/users/utils.py index edb78e89788f..3a58e504aa49 100644 --- a/src/olympia/users/utils.py +++ b/src/olympia/users/utils.py @@ -246,8 +246,7 @@ def _is_action_allowed(self, action_type, *, restriction_choices=None): }, ) # A failure with nothing enumerable still gets a single - # row, with the instance fields left NULL - today's - # behaviour. + # row, with the instance fields left NULL. for matched_instance in matched or [None]: self.history_entries.append( UserRestrictionHistory.objects.create( From 20565973135563e8cc69f5996078b877d92b1163 Mon Sep 17 00:00:00 2001 From: Jason Birchall Date: Tue, 8 Sep 2026 10:26:45 +0100 Subject: [PATCH 13/13] Log every empty enumeration as an error Previously, we had two structural denials for different reasons. After a chat with a colleague, both are equally as bad and should in fact result in an error. get_matching_restrictions() now returns a list again in all cases, and any empty enumeration for a database-backed class logs a single error. --- src/olympia/users/models.py | 15 +++++-------- src/olympia/users/tests/test_models.py | 2 +- src/olympia/users/tests/test_user_utils.py | 9 ++++---- src/olympia/users/utils.py | 26 +++++++--------------- 4 files changed, 18 insertions(+), 34 deletions(-) diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index 6629ec992e34..b3e908dafa0a 100644 --- a/src/olympia/users/models.py +++ b/src/olympia/users/models.py @@ -930,10 +930,6 @@ def get_matching_restrictions(cls, argument, *, restriction_type): Slow path, meant to be called after the corresponding allow_*() check has already failed, in order to record which restriction(s) matched. - - Returns None when the input needed for matching was missing or - invalid - a structural denial, there was nothing to search for. An - empty list means the search ran and nothing matched. """ # Should be implemented by child classes. raise NotImplementedError @@ -1046,20 +1042,19 @@ def get_matching_restrictions(cls, argument, *, restriction_type): Slow path, meant to be called after the corresponding allow_*() check has already failed in order to record which restriction(s) matched: - unlike the fast path it does not stop at the first match. Returns - None when the input needed for matching is missing or invalid, the - same condition allow_*() denies on structurally. + unlike the fast path it does not stop at the first match. Returns an + empty list when the input needed for matching is missing or invalid. """ # Mirrors the extraction in allow_auto_approval()/allow_request(). if restriction_type == RESTRICTION_TYPES.ADDON_APPROVAL: upload = argument if not upload.user or not upload.ip_address: - return None + return [] try: remote_addr = ipaddress.ip_address(upload.ip_address) user_last_login_ip = ipaddress.ip_address(upload.user.last_login_ip) except ValueError: - return None + return [] else: request = argument try: @@ -1072,7 +1067,7 @@ def get_matching_restrictions(cls, argument, *, restriction_type): else None ) except ValueError: - return None + return [] return [ restriction for restriction in cls.objects.filter(restriction_type=restriction_type) diff --git a/src/olympia/users/tests/test_models.py b/src/olympia/users/tests/test_models.py index 8071962ad539..cf2cc658049e 100644 --- a/src/olympia/users/tests/test_models.py +++ b/src/olympia/users/tests/test_models.py @@ -1573,7 +1573,7 @@ def test_get_matching_restrictions_unparseable_ip(self): IPNetworkUserRestriction.get_matching_restrictions( upload, restriction_type=RESTRICTION_TYPES.ADDON_APPROVAL ) - is None + == [] ) diff --git a/src/olympia/users/tests/test_user_utils.py b/src/olympia/users/tests/test_user_utils.py index 988e15e84bee..42c376ec10e0 100644 --- a/src/olympia/users/tests/test_user_utils.py +++ b/src/olympia/users/tests/test_user_utils.py @@ -575,7 +575,7 @@ def test_history_two_matching_email_restrictions_on_auto_approval(self, incr_moc def test_history_instance_fields_null_on_structural_ip_denial(self, incr_mock): # An unparseable last_login_ip makes IPNetworkUserRestriction deny # auto-approval structurally: the check fails, but no specific - # restriction instance matched, and a warning is logged. + # restriction instance matched, and an error is logged. self.request.user.update(last_login_ip='not.an.ip.address') IPNetworkUserRestriction.objects.create( network='10.0.0.0/24', @@ -588,11 +588,10 @@ def test_history_instance_fields_null_on_structural_ip_denial(self, incr_mock): channel=amo.CHANNEL_LISTED, ) checker = RestrictionChecker(upload=upload) - with mock.patch('olympia.users.utils.log.warning') as warning_mock: + with mock.patch('olympia.users.utils.log.error') as error_mock: assert not checker.is_auto_approval_allowed() - warning_mock.assert_called_once_with( - 'Failed %s check on %s denied structurally: ' - 'input missing or invalid, no instance to record', + error_mock.assert_called_once_with( + 'No matching restrictions found for failed %s check on %s', 'auto_approval', 'IPNetworkUserRestriction', ) diff --git a/src/olympia/users/utils.py b/src/olympia/users/utils.py index 3a58e504aa49..0a148e17f575 100644 --- a/src/olympia/users/utils.py +++ b/src/olympia/users/utils.py @@ -210,23 +210,13 @@ def _is_action_allowed(self, action_type, *, restriction_choices=None): if is_db_backed else [] ) - if is_db_backed and matched is None: - # Structural denial: the input needed for matching - # was missing or invalid (e.g. an unparseable IP), - # so the user was blocked on data quality rather - # than a specific rule. Expected, but worth - # visibility. - log.warning( - 'Failed %s check on %s denied structurally: ' - 'input missing or invalid, no instance to record', - action_type, - name, - ) - elif is_db_backed and not matched: - # Should be impossible: the fast path found a match - # that enumeration can't reproduce. Either the two - # predicates have drifted apart, or the restriction - # was deleted in between. + if is_db_backed and not matched: + # Should never happen: either the fast and slow + # predicates have drifted apart, the restriction was + # deleted between the two checks, or the denial was + # structural (input missing or invalid, e.g. an + # unparseable IP, which the fast path fail-closes + # on). All are worth investigating. log.error( 'No matching restrictions found for failed %s check on %s', action_type, @@ -241,7 +231,7 @@ def _is_action_allowed(self, action_type, *, restriction_choices=None): details={ 'restriction': str(cls.__name__), 'restriction_ids': [ - restriction.pk for restriction in matched or [] + restriction.pk for restriction in matched ], }, )