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?
Description
Description
Revisions and Commits
Revisions and Commits
| rP Phabricator | |||
| D13070 Add repositories to Diviner | |||
| D13104 | rPd6247ffca59c Add support for "Extended Policies" | ||
Event Timeline
Comment Actions
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?
Comment Actions
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..
Comment Actions
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.
Comment Actions
To your question @chad (although it seems like the clarity is not needed anymore), it just flags any newly created diff.
Comment Actions
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.
Comment Actions
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).
Comment Actions
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.