Page MenuHomePhabricator

Require several advanced postgraduate degrees to understand object policies
ClosedPublic

Authored by epriestley on Nov 9 2016, 8:17 PM.

Details

Summary

Fixes T11836. See some prior discussion in T8376#120613.

The policy hint in headers in the UI is not exhaustive, and can not reasonably be exhaustive. For example, on a revision, it may say "All Users", but really mean "All users who can see the space this object is in and the repository it belongs to, plus the revision author and reviewers".

These rules are explained if you click (and, often, in the documentation), but "All Users" is still at least somewhat misleading.

I don't think there's any perfect solution here that balances the needs of both new and experienced users perfectly, but this change tries to do a bit better about avoiding cases where we say something very open (like "All Users") when the real policy is not very open.

Specifically, I've made these changes to the header:

  • Spaces are now listed in the tag, so it will say (S3 > All Users) instead of (All Users). They're already listed in the header, this just makes it more explicit that Spaces are a policy container and part of the view policy.
  • Extended policy objects are now listed in the tag, so it will say (S3 > rARC > All Users) for a revision in the Arcanist repository which is also in Space 3.
  • Objects can now provide a "Policy Codex", which is an object that represents a rulebook of more sophisticated policy descriptions. This codex can replace the tag with something else.
    • Imported calendar events now say "Uses Import Policy" instead of, e.g., "All Users".

I've made these changes to the policy dialog:

  • Split it into more visually separate sections.
  • Added an explicit section for extended policies ("You must also have access to these other objects: ...").
  • Broken the object policy rules into a "Special Rules" section (for rules like "you can only see a revision if you can see the repository it is part of") and an "Object Policy" section (for the actual object policy).
  • Tried to make it a little more readable?
  • The new policy dialogs are great to curl up with in front of a fire with a nice cup of cocoa.

I've made these changes to infrastructure:

  • Implementing PhabricatorPolicyInterface no longer requires you to implement describeAutomaticCapability().
  • Instead, implement PhabricatorPolicyCodexInterface and return a PhabricatorPolicyCodex object.
  • This "codex" is a policy rulebook which can set all the policy icons, labels, colors, rules, etc., to properly explain complex policies.
  • Broadly, the old method was usually either not useful (most objects have no special rules) or not powerful enough (objects with special rules often need to do more in order to explain them).
Test Plan

Screen Shot 2016-11-09 at 10.29.44 AM.png (99×165 px, 8 KB)

Screen Shot 2016-11-09 at 12.02.59 PM.png (214×395 px, 29 KB)

Screen Shot 2016-11-09 at 12.03.02 PM.png (837×932 px, 108 KB)

Screen Shot 2016-11-09 at 12.02.41 PM.png (668×927 px, 84 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Require several advanced postgraduate degrees to understand object policies.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
avivey added inline comments.
src/applications/differential/storage/DifferentialRevision.php
373–380

we can remove this now because some other magic is handling this case?

src/applications/policy/controller/PhabricatorPolicyExplainController.php
49–51

Do we have any mechanism to mark a deprecated method? Do we do this trick anywhere else?

src/applications/differential/storage/DifferentialRevision.php
373–380

This case is:

  • you create a revision;
  • someone else edits it, changing the "Repository" to something you can't see;
  • you can possibly no longer see the revision.

Until now, supporting this case (letting the author see it) cost nothing, but now the cost of supporting this case is that the UI doesn't show the author the new (rXYZ > abc) hint. I think the hint is about a million times more important ("a little important" vs "really not very important at all"), so I'm just dropping support for the someone-else-edits case.

Other arguments against supporting this:

  • I'm not sure if you can see it even with this code.
  • I think this scenario is rare to the point of being unheard of.
  • The other user could commandeer the revision and kick you off anyway if they're doing it on purpose.
  • T4776 should improve this case in the relatively near future if they're doing it by accident.
src/applications/policy/controller/PhabricatorPolicyExplainController.php
49–51

I think we don't, currently. I'm going to follow up and clean up as much of this as I can, which may be 100% of it. If there's some reason I can't clean up 100% I'll figure out if we should do some kind of formal thing.

the 'in the tag' UI feels like it could run away fast. i'd like to think about this a little.

Yeah, some thoughts on that:

  • We can use the Codex to overwrite the behavior if/when it gets too big.
    • e.g., Phriction can say (Parents > All Users) or something? I guess? Not sure that's the best phrasing. But it won't say (a > b > c > d > All Users).
  • Relatively few applications are actually impacted, since extended policies aren't terribly common to start with and a lot of extended policies are just implicit and obvious and don't show up in the UI (for example, commits don't have a policy tag at all, since it has been "obvious enough" that a commit's visibility is the same as the repository's visibility).
  • Or we could drop the magic and always add more X > Y explicitly, but I think (S1 > rXYZ > All Users) is about the best we can do in Differential?

Offhand I'd just like to do something like All Users (Limited) and make them click on it for the restrictions

epriestley edited edge metadata.
  • Remove space name and extended policy object references from header tags.
chad edited edge metadata.

yay thanks

This revision is now accepted and ready to land.Nov 9 2016, 10:01 PM

(I'm going to clean this code up a little bit before I land it.)

src/applications/differential/storage/DifferentialRevision.php
373–380

ahh, ok, sure.

epriestley edited edge metadata.
  • Make code structure in PhabricatorPolicyExplainController a little more reasonable.
This revision was automatically updated to reflect the committed changes.