Page MenuHomePhabricator

Add support for "Extended Policies"

Authored by epriestley on Jun 1 2015, 9:44 PM.



Ref T7703. See that task and inline for a bunch of discussion.

Briefly, when we run implicit policy rules ("to see a revision, you must also be able to see its repository") at query time, they don't apply to other viewers we might check later.

We do this very rarely, but when we do we're often doing it for a bunch of different viewers (for example, in Herald) so I don't want to just reload the object a million times.

Test Plan
  • Added and executed unit tests.
  • Wrote a "flag everything" Herald rule, as in the original report in T7703, and no longer got "Unknown Object" flags on revisions.
  • Rigged up a lot of cases in the web UI and couldn't find any inconsistencies, although this case is normally very hard to hit.

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley updated this revision to Diff 31636.Jun 1 2015, 9:44 PM
epriestley retitled this revision from to Add support for "Extended Policies".
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
epriestley updated this revision to Diff 31638.Jun 1 2015, 9:49 PM
  • Whitespace / word wrap / minor cleanup.

Stuff not covered here:

  • Implementing these policies in more applications.
    • I think only Phriction Documents can hit the Herald flavor of this today.
    • Fixing T6367 will make this matter more for other applications (anything which sends mail).
    • T6183 will also make T6367 worse.
  • "or/any" policies, which I think are: Phriction edit (edit any parent), File view (view any attached object). I have a plan for these but left it out for now. We do not need to be able to express extended edit policies for a long time (maybe never).
  • UX for error messages is a bit rough, but users can't actually see them right now.
  • Maybe caching the "pass a PHID" stuff is worthwhile, but I figured I'd wait on that, since invalidating the cache isn't trivial.
btrahan accepted this revision.Jun 1 2015, 10:37 PM
btrahan edited edge metadata.
This revision is now accepted and ready to land.Jun 1 2015, 10:37 PM
This revision was automatically updated to reflect the committed changes.