Page MenuHomePhabricator

Policy checks may execute incompletely for non-viewers
Closed, ResolvedPublic


I have some herald rules which flags items.

On secure.phab, I would up with the case show in the screenshot. Presumably this is a deleted or hidden item?

Event Timeline

sshannin created this task.Mar 30 2015, 9:52 PM
sshannin raised the priority of this task from to Needs Triage.
sshannin updated the task description. (Show Details)
sshannin added a project: Flags.
sshannin added a subscriber: sshannin.

Also, note that floating text in the search box, hehe

chad added a subscriber: chad.Mar 30 2015, 9:53 PM

what browser is that?

sshannin added a comment.EditedMar 30 2015, 9:53 PM

26.0 build for linux mint (aka mint build 1.0)

chad added a comment.EditedMar 30 2015, 9:55 PM

I presume this to be a deleted item.

chad added a comment.Mar 30 2015, 10:26 PM

Actually, I'm probably wrong. It looks like (based on Feed in the screenshot) there is plenty of activity you don't have access to see. I'm not really sure we should let you flag an object you can't see. What specifically is the Herald rule?

The expectation is that the rule will fail to run when the user can't see the object. The code is consistent with this expectation, looking into it..

We are doing a policy check here, but it's an unusual check and it's not fully effective.

Essentially everywhere else in the code, we do policy checks only for the user who loaded objects. So you load a page, we load some objects "as you", and then we might do some additional check (to see if you have edit permission, for example, to determine whether we should disable an action like "Edit Thing").

In Herald, we load the object as one user (the actor) and then execute checks for different users (the rule owners). These checks are effective for the primary policy rules, but avoid some implicit checks -- notably, in Differential, the implicit check that requires you be able to see the repository.

This might also affect diffs (which have the same repository-implicit rule) and Phriction documents (which have an implicit dependency on their parents).

I don't see a terribly clear path forward:

  • We can repeat the entire load as the rule owner, which will produce the correct behavior but introduce a large performance penalty.
  • We can modify PhabricatorPolicyInterface and add a method like getPolicyRequirements($capability), which would return a list of <object, required capabilities> pairs that need to be checked for the check to succeed. This seems structurally correct but is a lot of work.
  • This is important-ish to fix but not hugely critical so maybe I can think about it for a bit and come up with something better.
epriestley triaged this task as Normal priority.Mar 30 2015, 11:12 PM
epriestley added a project: Policy.

To your question @chad (although it seems like the clarity is not needed anymore), it just flags any newly created diff.

epriestley renamed this task from Flagged Unknown Object to Policy checks may execute incompletely for non-viewers.Apr 11 2015, 8:18 PM

Project watchers are affected in the same way; see T6183.

This seems structurally correct but is a lot of work.

I haven't come up with a better approach for this yet. Assuming I don't, I think this is roughly 4-6 hours of work.

eadler added a subscriber: eadler.Jul 10 2015, 5:09 AM
urzds added a subscriber: urzds.Jul 12 2017, 11:13 AM

I'm unable to close this, but I believe this to be resolved (or at least my specific repro of flags via herald no longer applies I believe).

epriestley closed this task as Resolved.May 3 2019, 4:53 AM

Although I'm not entirely confident that 100% of objects which should implement ExtendedPolicyInterface actually do today, I think we've gotten pretty much all of them. This approach also seems stable.

Later, this more often took the form of:

When I set parent object X to an object policy like "Subscribers" or "Task Author", child object Y fatals or becomes invisible [because the policy can not be evaluated against Y].

This form of the issue was more visible than "Herald might not fully execute all related policy checks in all cases" and I believe it rooted the rest of these out.