Record which restriction instance(s) disabled auto-approval - #25344
Record which restriction instance(s) disabled auto-approval#25344jasonBirchall wants to merge 12 commits into
Conversation
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.
jasonBirchall
changed the title
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.
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.
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.
ce4cf43 to
da750bd
Compare
jasonBirchall
changed the title
| LongNameIndex( | ||
| fields=('restriction_content_type', 'restriction_object_id'), | ||
| name='users_userrestrictionhistory_restriction_content_type_object_id', | ||
| ), |
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.
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.
|
Another comment (and another great suggestion) I've addressed from @eviljeff. "can we add a restriction so we can't end up with the fk pointing to a random other model?" Edit: There doesn't seem to be a db-level way to pin a content_type, so I've borrowed another technique from other classes. A save() method that raises unless restriction_content_type resolves to a subclass of RestrictionAbstractBaseModel (as per Mat's suggestion). See commit f4e40b5 for more details. |
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().
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.
diox
left a comment
diox
left a comment
There was a problem hiding this comment.
General idea looks great. A couple comments on things to improve
| ) | ||
| assert version.addon.auto_approval_disabled | ||
|
|
||
| def test_restriction_shown_in_reviewer_tools_after_denial(self): |
There was a problem hiding this comment.
For this test I think we can simplify and put the add-on in the right state without using the API, and move the test to reviewers/tests/test_views.py.
That also highlights something that is missing from that PR: I think we should expose the exact restriction instance(s) responsible for the auto-approval being disabled in reviewer tools. We can do that as a follow-up, but we should have direct links leading to the admin for each, in the review page (and tests proving we have the right link(s) and support showing multiple restrictions if several matched).
There was a problem hiding this comment.
For this test I think we can simplify and put the add-on in the right state without using the API, and move the test to reviewers/tests/test_views.py.
Yep, completely agree and implemented in d3d4c79.
That also highlights something that is missing from that PR: I think we should expose the exact restriction instance(s) responsible for the auto-approval being disabled in reviewer tools. We can do that as a follow-up, but we should have direct links leading to the admin for each, in the review page (and tests proving we have the right link(s) and support showing multiple restrictions if several matched).
Agreed on this point as well. That was the original plan for completing the third acceptance criteria of mozilla/addons#16408: Giving a direct link to the admin page needed for operators. It'll come in a followup PR.
| # 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( |
There was a problem hiding this comment.
This should maybe be an error instead, it's a pretty bad case we don't want to ever see
There was a problem hiding this comment.
Yes, a good point. I have added this in d1701bf0, but I've also included another path, because the empty case turned out to cover two situations of different severity. So we have:
-
The same condition the fast path fail-closes on (e.g. a bad last_login_ip), there was never anything to search for, get_matching_restrictions() now returns None for this, and it's logged as a warning: expected behaviour, but worth visibility, since the user was blocked on data quality rather than an actual rule. (Only the IP class can hit this; the others allow when their input is missing, so their slow paths can't be reached this way.)
-
The search ran and genuinely found nothing, which, given the fast path just denied, should be impossible: either the two predicates have drifted apart, or the restriction was deleted between the two checks. This is the case we never want to see, and it's logged as an error.
Row creation is unchanged in both cases (single row, NULL instance fields), and both paths are pinned by tests.
How does that fit with you? I'm happy to ignore the warning path and just include the error if you're confident it's not a problem
There was a problem hiding this comment.
I think distinguishing between both cases isn't really useful: both are equally bad and should never happen, even though it's for different reasons.
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.
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.
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.
2 participants
Fixes mozilla/addons#16408 (Jira 2752)
Agreed with @diox that we can review this large PR.
Problem
When a submission fails a restriction check and is pushed into a manual review queue, we record which kind of restriction caught it, but not which one. A reviewer looking at an add-on's history sees "Listed auto-approval automatically disabled because of a restriction" and nothing more. Ops can't tell whether a given restriction entry (an email pattern, an IP network) is doing useful work or just generating queue noise, and reviewers can't quickly tell whether the match is one they should worry about.
The issue asks for three things:
"Add-on Approval" restrictions are the priority; "Rating Flag for Moderation" restrictions are the second case.
How it works today
Version.from_upload()callsRestrictionChecker(upload=upload).is_auto_approval_allowed(). InsideRestrictionChecker._is_action_allowed(), each restriction class'sallow_auto_approval()runs as a fast boolean check, optimised to evaluate all of that class's rows at once. On failure, the checker:self.failed_restrictions,RESTRICTEDactivity log entry on the user withdetails={'restriction': '<ClassName>'},UserRestrictionHistoryrow holding the user, IPs, and an integer identifying the class.The identity of the matched row is discarded at that point & the fast check only ever returns a boolean.
Back in
from_upload(), the checker instance is discarded too. TheDISABLE_AUTO_APPROVALactivity log written on the add-on contains only the channel and a fixed comment string; it doesn't record the class, let alone the instance, and nothing ties theUserRestrictionHistoryrow to the version that was affected.So today the data trail is: a user hit a restriction of class X at some point and, separately, this add-on's auto-approval was disabled for some restriction. Neither answers "which entry matched, for which submission."
How it works after this change
The fast path is unchanged. On failure only, the checker takes a second, slower pass to find out what matched and records it.
One commit per layer, followed by commits responding to review:
Schema.
UserRestrictionHistorygains three nullable fields: a generic foreign key (restriction_content_type+restriction_object_id, indexed together asurh_restriction_instance_idx) pointing at the matched restriction row in whichever table it lives, and aversionFK. Existing rows keep their meaning with these left NULL, as do rows written for stateless restrictions (e.g. the developer agreement check), where there is no instance to point at.Matching.
Each DB-backed restriction class gets a
get_matching_restrictions()classmethod that returns the instances responsible for the failure, using the same matching semantics as itsallow_*method.allow_*methods are not modified. The method is declared onRestrictionAbstractBaseModelraisingNotImplementedError(mirroringallow_auto_approval()onRestrictionAbstractBase), and a meta-test asserts every DB-backed class overrides it — so a future restriction class can't be added without one.Checker.
In the failure branch of
_is_action_allowed(), for authenticated users only (nothing can be recorded otherwise, matching today's behaviour), if the class is DB-backed the checker callsget_matching_restrictions()and creates oneUserRestrictionHistoryrow per matched instance, each pointing at that instance. An empty enumeration for a DB-backed class logs a warning: it means either a structural denial (e.g. an unparseable IP) or that the fast and slow predicates disagree. TheRESTRICTEDactivity log gains arestriction_idslist alongside the class name. The checker keeps the rows it created onself.history_entriesso callers can enrich them. Because rating moderation goes through the same method, "Rating Flag for Moderation" restrictions get instance recording as part of the same change.Version creation.
from_upload()keeps a reference to the checker. On failure it setsversionon the history rows the checker just created, and theDISABLE_AUTO_APPROVALactivity log gains two structured keys,restrictions(class names) andrestriction_history_ids, plus the class name(s) appended to the existing comment: "Listed auto-approval automatically disabled because of a restriction (EmailUserRestriction)". The text before the parenthesis is unchanged so existing string matching still works.What reviewers and ops see.
Immediately: the reviewer tools history entry names the restriction class.
In Redash:
users_userrestrictionhistorycan be joined toversionsand, viadjango_content_type, to the specific restriction table to answer "which restriction entries disabled auto-approval this month, and how often."A follow-up PR will render the matched instances as admin links in the reviewer tools entry, which completes the third acceptance criterion.
Design notes
UserRestrictionHistoryrow per matched instance, rather than one per class. I checked every consumer of the table before choosing this: no production code reads rows in a way that assumes one row per failed class. The only reader that counts rows is the admin user page (see behaviour changes below). Multi-row-per-user is already an accepted shape elsewhere in the tests.GenericForeignKeyfor the matched row, since the five DB-backed restriction classes live in five tables. Costs a django_content_type join in SQL; gives admin links for free. Per review, the pointer can't dangle onto arbitrary models:save()refuses any content type that doesn't resolve to a subclass ofRestrictionAbstractBaseModel. The check is dynamic, so a future restriction class is allowed automatically — keeping the forward-compatibility that motivated the generic FK. (There's no database-level way to pin a content type: their ids aren't stable across environments, and MySQL CHECK constraints can't run subqueries.)versionis set after the fact byfrom_upload()rather than passed into the checker, keepingRestrictionCheckerunaware of versions since it also serves submission and rating checks.DISABLE_AUTO_APPROVALhashide_developer = Trueand is filtered out of every developer-facing surface, so the class name can't reach the restricted developer.Behaviour changes to note
restriction_history_for_this_userlink text is a raw row count. It now counts matched restriction instances rather than failure events, so it goes up by one for each additional row a single class matched.DISABLE_AUTO_APPROVALcomment gains the class name(s) in parentheses. Text before the parentheses is unchanged; the three existing tests that assert the exact string are updated.NULLis a legitimate state: stateless restrictions (developer agreement, reputation services) and structural denials (e.g. unparseable IP) have no row to point at.Test plan
Version.from_upload()) with anADDON_APPROVALrestriction in place. One asserts the full recorded chain — history row pointing at the exact instance and the created version, the enrichedDISABLE_AUTO_APPROVALdetails and comment, auto-approval disabled. The other continues to the reviewer: a user withAddons:Reviewloads the review page and sees "…disabled because of a restriction (EmailUserRestriction)" in the important-changes history.get_matching_restrictions()tests: one match, none, wrong-type rows never returned, missing input → empty. Multiple simultaneous matches are covered for email and IP; the other three classes have unique(value, restriction_type)constraints, so a single input can match at most one row.RestrictionCheckertests: one row per matched instance; the recorded instance asserted on every existing single-restriction failure test; NULL instance fields plus a logged warning on structural denial; therestriction_idsdetails key;history_entriescontents; the rating moderation path; unauthenticated failures enumerate and record nothing (mocked — the fast paths themselves assume an authenticated user).from_upload(): the three existing restriction tests extended (listed email, listed IP, unlisted IP) for the new details keys, comment string, and the history row pointing at instance and version; a new test covers two classes failing on one upload.save()refuses a non-restriction instance; the baseget_matching_restrictions()raisesNotImplementedError; a meta-test pins that all five DB-backed classes implement it.sqlmigrate); no data rewrite. Fullusers,versions,activity,reviewersandsigningmodules pass.