This is mostly pre-existing and philosophical (and pretty easy to fix later) so I don't think you should do anything about it, but I think we're using null to mean two very subtly different things:

• "This object has no meaningful default policy."
• "This object has the same policy as the default policy."

That is, there are sort of five states:

• (Red) Stronger policy than default.
• (Green) Weaker policy than default.
• (Grey) Policy has been adjusted from the default, but it isn't known to be stronger or weaker.
• (Grey) Same policy as the default.
• (Grey) There is no default policy.

This doesn't seem important to fix to me, and I'm not sure "there is no default" is really a legitimate state for any object. The objects I'm aware of which return null today, like Dashboards, do actually have a default policy, they just don't return it correctly.

This patch generally looks like it has the right shape and structure to me. I think I see two actual issues:

• getStrengthInformation() in PhabricatorPolicyExplainController should also use this new Codex logic. This powers the dialog you get when you click the policy tag, which explains the policy in more detail. You should be able to hit the logic by clicking the policy tag (or "open in new tab" on the link to get an easier-to-test dialog on a standalone page). Whatever it says should agree with / explain why the tag is whatever color it is. (Maybe the Codex needs to do a bit more work to support this?)
• I think we can improve the actual policy comparison logic to get slightly better results (see inline).

...but the current logic gives us a grey tag, I think. A red tag seems better, since you're in a "private area" of the wiki.

Yeah, the existing logic gives a grey tag. I think it's kind of counter-intuitive to have a weaker policy still show as red, but it's true that you're still in a "restricted" section of the wiki. Maybe it would be a good idea to add something to the explain text like "This document has a weaker policy than the strongest policy of its ancestors ('foo-policy'), so this weaker policy doesn't do anything."

On a related note, I don't really like the fact that it's even possible to set weaker policies on a child, because the obvious expected behavior ("give wider access to this particular document") doesn't happen with no obvious next steps. Users are probably much more used to "Google docs"-style permissions, where there's no hierarchy and you can always share stuff directly with particular users. (Actually, now that I think about it, I'm not sure what happens if you try and share a Google doc in a restricted folder with someone who can't see the folder... but I bet it Just Works). I get that unix file permissions work the same way, and that not allowing this state of affairs makes it harder to change policies across multiple docs, but maybe we could do one of:

1. Warn users when attempting to apply "futile" policies to a document. This seems like the least-intrusive solution, bu I don't know if we have any infrastructure for showing "warnings but not errors" on edits.
2. Put a header at the top of either the document itself, or the edit dialogue, warning that the current policy is futile. This would cause disruption and possibly fear for installs that have their policies configured incorrectly, but maybe it's better to tell them that their policies aren't working the way they expect.

I don't think either of these checks would impact performance, since we already have to fetch the policies for the document's parents. We could also apply a migration in combination with one or both of these solutions that tightens up futile policies, but that might be begging for trouble.

if ($root_policy === null) {
  $root_policy = $ancestor_policy;
 }

Is it guaranteed that the root document is always first in this list, or should I check the slug too?

Maybe it would be a good idea to add something to the explain text

You should be able to override methods in the Codex to add text like this (I think) or change how the text on the tag itself renders. I think adding extra explanatory text to the dialog is very reasonable.

It would be nice if the tag could always clearly describe the entire policy in a few words, but I'm not sure how to do this for some objects. One example is revisions, which you must be able to view the repository for -- should they say [ rXYZ · Phacility Core ∩ (All Users ∪ Author ∪ Reviewers) ]? Should Files say [ All Users ∪ Author (epriestley) ∪ T123 (attached) ∪ T456 (attached)]? Should almost every object just say [Complicated policy, click to explain]?

These all seem kind of worse than showing the local policy and trying to teach users how to learn about and understand the entire policy, but showing only the local policy still leads to confusion at least sometimes.

Is it guaranteed that the root document is always first in this list, or should I check the slug too?

I believe ancestors are guaranteed to be in depth order. If they aren't, I think that's a bug and we should fix it. Projects does something similar and definitely guarantees order.

It's also possible for a root document to not actually exist (maybe requires bin/remove destroy shenanigans in modern code?) and the "use the first one" logic will get the right result in that case, so I think assuming/guaranteeing order is probably a little cleaner than testing the slug.

Warn users when attempting to apply "futile" policies to a document. This seems like the least-intrusive solution, bu I don't know if we have any infrastructure for showing "warnings but not errors" on edits.

In many cases, we can't tell. An example is "Members of Engineering" vs "Members of Employees Named Scott". Those policies aren't naturally stronger or weaker than one another, and even if they are today (all employees named Scott are also engineers) they may not be tomorrow.

We could put a warning in the header when we do know, e.g. "This document is set to 'All Users' but the parent is set to 'Members of Engineering' so that stronger policy is in effect.". However, I'm not sure users are actually confused by this today -- at least, not very often -- even though it seems like it should be kind of confusing? I'm hesitant to add a warning element if it means that ~every page will have a warning which doesn't help anyone, users will just ignore, and which users can't easily fix (e.g., if an existing section of the wiki gets locked down).

(We have some support for popping a warning dialog on edits, but never do it from "Edit Form" edits today -- only from comment edits. Stuff like "your status change won't have any effect, post your comment anyway?".)

We could add some kind of support to the actual edit form, e.g., change this:

Visible to: [ Whoever ]

...into this:

To view a wiki page, you must also be able to view all of its ancestors. Ancestors
of this page have these policies:

  / - All Users
  /x/ - Whatever

If you choose a weaker policy than the union of all these policies, it will have no
effect.

Visible to: [ Whoever ]

...but this feels like a lot of sermonizing to solve a problem that users don't really seem to have today.

We could also do something like this:

• The policy for / must always be the most open policy.
• Only pages at depth 1 may have other policies.
• (Pages at depth 2+ always have their depth-1 ancestor's policies.)

...but, at least today, this feels like a reduction in power when users don't seem to be having much trouble with it.

Any mistakes here also fail safely (i.e., an author thinks they're sharing a document when they really aren't) instead of failing dangerously (i.e., an author thinks they've locked down a document when it's really open), so I'd expect we'd hear about it if a lot of users were running into issues here since the failure of expectation should be obvious ("I sent alice a link but she can't see it, how does this work??").

However, I'm not sure users are actually confused by this today -- at least, not very often -- even though it seems like it should be kind of confusing?

I should have given more context: I expect that when this change goes out, users will notice the suddenly new colors, and will likely be confused as to why the policy hints on random pages changed from grey to red.

In terms of longer-term vision, I'm not totally sold on forcing a bunch of stuff to implement Codex in the future, but I sort of think of describeAutomaticCapability() as "version 1" of the Codex, and the Codex as "version 2". describeAutomaticCapability() proved nowhere near powerful enough to cover all the weird policy rules that some objects actually have which is why the Codex approach in "version 2" is so much heavier.

I'd think I'd like to get rid of describeAutomaticCapability() eventually and make everything that wants to explain things implement a Codex instead

Ok, then I'll migrate PhrictionDocument->describeAutomaticCapability() over to the new Codex and make the dialogue text more information about why the colors are exciting and new.

Can you give me some ideas for more stuff to test before landing this? Phriction and otherwise.

I can't really think of any super tricky cases, but stuff I'd maybe try:

• Task with public policy should be green (assuming "All Users" default).
• Task with "just me" policy should be red.
• Normal task should be grey.
• Clicking those tags should work and do sensible things.
• Root document in Phriction doesn't crash.
• Root document in Phriction always grey.
• Root document in Phriction shows sensible message when policy tag is clicked.
• Sub-document red/grey.
• Sub-document not green even when set to "Public".
• Root at "All Users" + /engineering/ at "Members of X" + /engineering/javascript/ at "All Users" is red (maybe with sensible policy text).
• Maybe destroy the Phriction root document with bin/remove destroy and see if that breaks anything? But I think you can't get there normally (we require /x/ exist before we'll let you create /x/y/) so whatever you hit might be pre-existing.

This change might be too big; I'm happy to refactor if you think so.

Example updated edit dialogue:

Example updated policy explainer:

Looks reasonable to me.

Maybe destroy the Phriction root document with bin/remove destroy and see if that breaks anything? But I think you can't get there normally (we require /x/ exist before we'll let you create /x/y/) so whatever you hit might be pre-existing.

This results in the "create new root doc" menu and the hierarchy links at the bottom generating 500's if you attempt to view any of them. If you re-create the root document, everything goes back to working. I think this is pretty much ok? If not, I can add a lot of if (!$root) {} checks all over the place (mostly in PhrictionDocumentPolicyCodex.

The other tests all look good. I'm going to land this soon unless you can think of anything else to test.

That sounds reasonable to me, I'm pretty sure it's basically impossible to get into that state naturally anyway.

Go for it. 🚀