diff --git a/src/olympia/addons/tests/test_views.py b/src/olympia/addons/tests/test_views.py index e2d65712a2d4..fbdb56deaa02 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,35 @@ 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 + 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) 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..69716790a3c8 --- /dev/null +++ b/src/olympia/users/migrations/0027_userrestrictionhistory_restriction_instance_and_version.py @@ -0,0 +1,35 @@ +# Generated by Django 5.2.17 on 2026-08-25 14:53 + +import django.db.models.deletion +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=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 e6a5ba017840..b3e908dafa0a 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 @@ -920,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) @@ -1020,6 +1034,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) @@ -1058,6 +1116,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 @@ -1178,6 +1255,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( @@ -1229,6 +1340,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) @@ -1269,6 +1400,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 @@ -1430,6 +1580,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,8 +1613,27 @@ class Meta: fields=('last_login_ip',), name='users_userrestrictionhistory_last_login_ip_d58d95ff', ), + models.Index( + fields=('restriction_content_type', 'restriction_object_id'), + name='urh_restriction_instance_idx', + ), ] + 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 3a6f55e2ac7e..cf2cc658049e 100644 --- a/src/olympia/users/tests/test_models.py +++ b/src/olympia/users/tests/test_models.py @@ -49,10 +49,12 @@ FingerprintRestriction, IPNetworkUserRestriction, IPReputationRestriction, + RestrictionAbstractBaseModel, SuppressedEmail, SuppressedEmailVerification, UserEmailField, UserProfile, + UserRestrictionHistory, generate_auth_id, get_anonymized_username, ) @@ -1501,6 +1503,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 +1627,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 +1831,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 +1969,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 +2098,108 @@ 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 + ) + == [] + ) + + +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 + + +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', diff --git a/src/olympia/users/tests/test_user_utils.py b/src/olympia/users/tests/test_user_utils.py index 698c8bddd75d..42c376ec10e0 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,214 @@ 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 an error 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.error') as error_mock: + assert not checker.is_auto_approval_allowed() + error_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_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 + # 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..0a148e17f575 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,56 @@ 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: + # 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, + 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. + 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 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.