Page MenuHomePhabricator

Improve "Custom Policy" behavior in policy dialogs
Closed, ResolvedPublic

Description

See PHI1419. In Applications(Pick Something), if an application policy is set to "Custom Policy", the policy is not linked. This should be linked to a policy dialog.

See downstream https://phabricator.wikimedia.org/T231734. Currently, if you can not view an object, we may disclose an abnormal amount of information about certain policies, including "Members of Projects: ..." policies. Instead, this form of the dialog (where you do not have permission to view the object) should be less revealing so that it does not leak information like Members of projects: Security, File Uploads, SVG File Handling.

See downstream https://phabricator.wikimedia.org/T211498. Probably mostly see also T6802. This is the opposite case: when you can view an object, the policy dialog is not very helpful about explaining custom policies. At a minimum, they should be linked; better would for them to be inlined.

See T8808, which is probably adjacent and trivial.

Event Timeline

epriestley triaged this task as Low priority.Mon, Sep 9, 5:06 PM
epriestley created this task.
epriestley moved this task from Backlog to Next on the Policy board.Mon, Sep 9, 5:06 PM

In Applications(Pick Something), if an application policy is set to "Custom Policy", the policy is not linked.

This is currently used by Application detail views, Application edit views (if application policies are locked), the Diffusion "Policies" tab, Passphrase credential views, Phame blog manage views, project member pages in the "Joinable By" column, and Spaces detail views.

The Phame blog callsite, Passphrase callsite, and Spaces callsite can probably be removed, since these pages have "Edit" actions and full transaction records nowadays, and thus reasonable/standard ways to get this information. The other interfaces use this rendering to do something more unique or useful.

PhabricatorPolicy is currently an odd object, with properties getHref() and getWorkflow(). This should be reworked some day (perhaps partly here) into some more cohesive API, perhaps newLink().

For custom policies, currently we can only render them standalone in the context of a transaction. We can also render object capabilities in a dialog, but, today, you click "Custom Policy" (if it's linked up in the easy/obvious way) and get this:

This dialog basically just says "Yep, you sure just clicked 'custom policy'."

It's generally desirable that we render policies in the context of an object (and some object policies may depend on it), and it's probably correct that we render context (and messages from the Codex, if any) when you click this link. For example, if a project is "Joinable By" a custom policy, it's probably better for the explanatory dialog to include the codex information ("Users who can edit a project can always join it."), not just the raw rules.

So these dialogs need to inline all the rules (like the transaction variation does) before we really have somewhere to link to:

At the same time, these dialogs should be locked down when rendering policy details to users who do not have permission to view the object. In particular, this variation of the dialog:

...should change so that it does not disclose the project name. A sort-of reasonable change might be something in the vein of "Members of one or more particular projects can view it, but you don't have permission to know which projects these are.".

We can be somewhat flexible in making insignificant disclosures here (e.g., render separate strings for "one" or "more than one" project to make the grammar flow better, even though this technically discloses information) to make policies easier to understand.

I think this dialog also got murdered by some CSS change a while ago and a bunch of the spacing went a little crazy.

Ideally, T13381 would also be fixed here, although I think a satisfying fix there isn't really in scope.

One other thing is that PhabricatorApplicationPolicyChangeTransaction->renderApplicationPolicy() has unconventional behaviors which are not very helpful and not consistent with normal CAN_EDIT / CAN_VIEW transactions. This is somewhat perplexing because ModularTransactions has renderPolicy() already, which has better behavior. I think it didn't exist yet when Applications modularized in D17757, and when it was introduced in D19829 I just overlooked the opportunity to update it.

This should be reworked some day (perhaps partly here) into some more cohesive API, perhaps newLink().

This actually needs to render for several different targets. In bin/policy show, it should always render plain text. It currently does not:

epriestley@orbital ~/dev/phabricator $ ./bin/policy show T666
...

  edit
    <a href="/tag/suspicious_interns/" class="policy-link">Suspicious Interns</a> (Project)
    Members of the project "Suspicious Interns" can take this action.

  - Tasks with edits locked may only be edited by their owner.

In HTML contexts, we would sometimes like to render "Policy Name (Descriptor)", e.g. "Suspicious Interns (Project)". The intent here is to render "Members of Suspicious Interns" in a more compact way that's more concise for contexts like "alice changed the edit policy for this task from X to Y."

In other cases, we would like to render "Policy Name", as in "Custom Policy".

I'm currently planning to replace PhabricatorPolicy->renderDescription() with some presentation object and try to push all the rendering and URI construction logic into it.

bin/policy show should also probably get a mention in User Guide: Unlocking Objects.