Page MenuHomePhabricator

Phriction document header doesn't render policy strength variations
Closed, ResolvedPublic

Description

See PHI590. This is because PhabricatorPolicyQuery::getDefaultPolicyForObject() returns null for Phriction documents, so PHUIHeaderView can't decide if the current object policy is different from the default.

Digging deeper into the code, it looks like this happens because PhabricatorApplication->getDefaultObjectTypePolicyMap() only fetches an application's custom capabilities, of which Phriction has none.

I'm gonna guess the fix here is either:

  • add a "Default View" capability to the Phriction application (which wouldn't change this behavior for all the other applications that don't define custom capabilities)
  • decide that this is a bug, and make getDefaultObjectTypePolicyMap() also include the default CAN_VIEW and CAN_EDIT capabilities. I have no idea what the ramifications of that would be.

Event Timeline

amckinley renamed this task from Phriction document header doesn't render policy variations to Phriction document header doesn't render policy strength variations.Apr 18 2018, 8:15 PM
amckinley triaged this task as Normal priority.
amckinley created this task.

Since Phriction policies are hierarchical, I think a dedicated setting probably doesn't make sense -- it would only effectively apply to creating the root document for the first time, at least in most cases.

It would also potentially be misleading to render this using normal rules, since you can have a hierarchy like this:

/ Visible to: All Users
/animals/ Visible to: Only Alice
/animals/cow/ Visible to: Public

The "Cow" page has a weaker direct policy than the "default" policy ("All Users", on /), but the aggregate policy is much stronger (effectively, only Alice can see "Cow", since you must be able to see all of a page's parents to see that page).

If we use the default UI rules, we'll render the policy on "Cow" as green (because "Public" is a weaker policy than "All Users"), even though it has a more restrictive effectively policy than the "default" policy and the same effective restriction as the parent.

We have this in some other cases already -- notably, you can have a Differential Revision where policy color hints don't necessarily make much sense because the effective policy is controlled entirely by policy rules on the repository that the revision is attached to. However, this case is simpler and more intuitive, I think.

One possible approach is to use rules like this:

  • Never render a policy as green, because it's impossible for /x/y/ to have a weaker effective policy than /x/.
  • If the /x/y/ policy is stronger than the /x/ policy, render red.
  • If the /x/y/ policy is not stronger than the /x/ policy, render grey.

One problem with this is that /employees_only/ will render red (stronger policy than the root), but /employes_only/secret_handshake/ will render grey (same policy as the parent). Maybe this is misleading. If we think it's misleading, we could do this instead:

  • Never render green.
  • If /x/y/ is stronger than the root, render red.
  • If /x/y/ is not stronger than the root, render grey.

That basically means that all pages with any kind of policy constraint anywhere in the tree will become red. This makes red a little less meaningful, but maybe also a little less confusing? It's probably reasonable if you have a wiki with a lot of "All Users" documents and then some locked-down sections, although I'm not sure how common this is.

I'm not sure how much work is involved in implementing either of these approaches.

I'm inclined to say the latter ruleset ("never green; red if stronger than root policy") is probably the best place to start with? I don't see any obvious problems with it offhand, and I think it's a non-confusing net improvement over "always grey", even if it doesn't maximize how much information is conveyed in all cases.

I wholeheartedly embrace the latter ruleset. I also suspect that "a lot of 'All Users' documents and then some locked-down sections" is a common use-case, and the latter ruleset fits that use-case perfectly. I'll take a look at that when I get the chance.

Regardless of doing something special with the access highlighting for Phriction, I still think there's a bug with the way the default policy-finder currently works, since every other product that doesn't use capabilities but does use a PHUIHeaderView should have the same behavior. I'm not sure how long that list is, but it at least includes Dashboard (and I think Phlux as well, but I can't test because of T13129).

amckinley lowered the priority of this task from Normal to Low.Apr 18 2018, 9:41 PM
amckinley added projects: Phriction, PHUI, Policy.

I'm going to tentatively put this (the Phriction rule, specifically) into T13130 for this week although I'm not sure how much work getting getDefaultPolicyForObject() to return the root document policy is and maybe that's Unbelievably Difficult. Let me take a quick look at that and you can either run with it or I can pick it up if it's some kind of weird arcana.

The other thing is probably at least approximately a bug. As you surmise, I think the approaches are probably:

  1. Give every object which can render a policy header a real default policy.
  2. Let the method return something like null (what are the implications?) to mean "there is no default policy", and then handle things from there (e.g., never color the header).

I'm not sure how long the list in (1) is either, or what the implications of returning null in (2) are. But this generally seems very low-urgency to address since the list is probably pretty obscure even if it's not super short. So maybe the path forward is something like this:

  • Really gradually build up that list, e.g. by noting things here when we notice and remember, or maybe by actually going through the code.
  • Once we think we probably have the list mostly right, see if we need (2) for some reason (e.g., something on the list legitimately can't have a meaningful default policy; or the list is kind of long and we just don't want to add all those config options since almost no one would realistically ever use most of them).
  • Do (1), or (2), or both, with that information.

After actually looking at the code, it looks like null can already be returned and works, it's just kind of wrong for some objects (like Dashboards) because they really do have a default policy -- it's just hard-coded.

For Phriction specifically, I think the best approach might be:

  • Have PhrictionDocument implement PolicyCodexInterface. This is an interface which allows an object to return a "Policy Codex", which is an object that can customize policy behavior in greater detail.
  • Add a method like getDefaultPolicyForObject($viewer, $object, $capability) to PolicyCodex.
  • Have PhabricatorPolicyQuery::getDefaultPolicyForObject(...) call through to this method if the $object implements PolicyCodexInterface.
  • Return the result if it isn't null.
  • Build a PhrictionDocumentPolicyCodex which queries the policy of the root document, if it exists.

I think we have to stick some kind of special case into PhabricatorPolicyQuery::getDefaultPolicyForObject(...) and that seems like the least-bad way to special case it, offhand. But I'm also open to anything else if you came up with another idea.

I suppose the approach proposed above gets a questionable result here:

/                          - All Users
/engineering/              - Members of "Engineering"
/engineering/javascript/   - All Users

That will render grey on /engineering/javascript/ since it's the same policy as the root. Ideally, we'd probably like it to render red, I think.

Maybe PolicyCodex should have a method like compareToDefaultPolicy(...) instead, which returns stronger/weaker/null.

Then the logic in both HeaderView and ExplainController can look like this:

if ($object instanceof PolicyCodexInterface) {
  $strength = $object->newPolicyCodex()->compareToDefaultPolicy(...);
} else {
  $default_policy = PhabricatorPolicyQuery::getDefaultPolicyForObject(...);
  if ($policy->isStrongerThan(...)) {
    $strength = 'stronger';
  } else if (...) {
    ...
  }
  ...
}

if (stronger) {
  ...
} else if (weaker) {
  ...
} else {
  ...
}

Then PhrictionPolicyCodex can just walk down the tree looking for any document with a stronger policy than the root, and we'll be able to mark all documents underneath it as red.