Page MenuHomePhabricator

Policy behavior for Herald rules is murky and inconsistent
Closed, ResolvedPublic

Assigned To
None
Authored By
nipunn
Sep 13 2015, 7:54 PM
Tags
Referenced Files
F804356: Screen Shot 2015-09-13 at 1.55.56 PM.png
Sep 13 2015, 8:57 PM
Tokens
"Orange Medal" token, awarded by dgoldstein.

Description

Use case: I see that a herald rule added a project to a revision, so I click to the herald transcript. I search for the project and find that a particular herald rule (say H123) passed adding the project. Then, I have to do several clicks/typing/copypaste to get to H123 if I want to see why it was added.

The herald transcript page contains a list of rules, but no links to those rules. Seems like it should, as it would be easy to click over to the rule.

Seems like a subset of T7617

Event Timeline

nipunn updated the task description. (Show Details)
nipunn added a project: Restricted Project.
nipunn added subscribers: jhurwitz, angie, nipunn.

Then, I have to do several clicks/typing/copypaste to get to H123 if I want to see why it was added.

Isn't this shown in the transcript itself? For example, if I want to understand why Blessed Reviewers was added as a reviewer for D14104, I go look at the transcript:

https://secure.phabricator.com/herald/transcript/118353/

...which shows why:

Screen Shot 2015-09-13 at 1.55.56 PM.png (174×553 px, 33 KB)

That is, it was added because the author wasn't a member of the project and the revision was new.

What additional information is present on the rule detail page which isn't present here?

Ah you are right. You can see the set of conditions that were checked for the rule.
In my particular case, I wanted to find the rule that was adding me as a subscriber and disable it.
I ended up typing "/herald/rule/(rulenumber)" into my URL bar to get to the page of the rule.

However, the more broad use case I see is discoverability.

Eg. I see that a "security block" was added to their code review. They can click the herald transcript, find the herald rule that set up the block, see what particular part of the or-based-rule succeeded, and then click on the rule to see all the possible reasons why such a herald check might succeed.

These aren't linked partially because in the general case, the policy model for Herald rules is a little murky:

  • You can always see the effects of Herald rules, and we have no choice but to allow this most cases. We could hide "send email", "flag", etc., effects, though.
  • We currently let you gain significant insight into the content of Herald rules by reviewing transcripts.
  • You can not actually look at other users' private rules, per se.

This model is inconsistent and attempts to limit how public Herald is are probably futile, and maybe we should just change the policy so that all rules are always public.

But this is likely moot if the real issue is that the security team on your particular install has a large number of complicated, code-matching regular expression-based Herald rules as documented in T7617. We consider this a very poor fit for Herald and don't intend to support it in a meaningful way. Is this primarily what you're hitting?

I'm don't see why the number-of-rules is relevant

The current flow is that I see the herald transcript, find a rule that I want to modify / learn more about, dump it into my URL bar, (and maybe get a 403 like message). Having a link just reduces that flow by one step. I don't believe that the existence of the link changes the effect of the policy model (regardless of how murky it is).

You can build additional view into the herald transcript page, but in the end, the herald rule page has more functionality (editable, shows the full ruleset). When I see a rule, I want to be able to click on it and learn more. I can't tell from the page what the full ruleset is (or if there even is more to learn). And definitely can't edit rules from that page.

In the end, this is just a couple click/URL optimization, so if you believe that having it will change behavior (and encourage bad things), then I'm happy to work around it. Or if it's a problem that won't be a problem after some policy changes.

I'm don't see why the number-of-rules is relevant

I want to solve problems, not implement features. It's very important to me to understand the problem you are facing before making changes to resolve it. We can often find better or more general solutions to problems than users can suggest because we have more context. See:

https://secure.phabricator.com/book/phabcontrib/article/feature_requests/#describe-problems

I consider linking, not linking, and selectively linking to all be bad:

  • Not linking is bad because it forces you into the workaround you describe, although my belief is that only users on your install find this workflow compelling, and substantially or exclusively because your install has chosen to make a unique, off-label use of Herald which is very far outside of the expected set of use cases (discussed in T7617).
  • Linking is bad because some of these links will hit policy violations. This is confusing and inconsistent with the rest of the product: if you can't see objects in every other application, we don't show you the titles either or link the objects, and show them as "Restricted Task", etc. This directly teaches users that applications are inconsistent in their handling of policies, which isn't desirable. If we restrict the Herald policy model in the future, we'd also have to undo this change, and be teaching users about an interaction which we'd later invalidate.
  • Linking only stuff that you can see is bad because it's also confusing and there's no direct way to figure out why some stuff isn't linked. This is probably the most-bad from the upstream perspective.

I'd prefer to find a not-bad solution. One not-bad solution is making Herald rules completely public, then linking them. This has no UI tradeoffs but requires a change to the Herald policy model.

Another not-bad solution might be to remove the desire to examine the rules in detail so that it's not important that rule policies have internal inconsistencies. Resolving T7617 by moving these regexp rules to a more appropriate mechanism might substantially do this. This is potentially more straightforward than resolving the Herald policy model.

eadler added a project: Restricted Project.Jan 11 2016, 9:38 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jan 11 2016, 9:42 PM
epriestley renamed this task from Herald Transcript does not contain links back to original rules to Policy behavior for Herald rules is murky and inconsistent.Mar 8 2016, 11:56 PM

To provide a little context, I suspect we'll just make personal rules publicly visible. Long ago, transaction records were not as explicit or complete as they are today and you could sort of hide your "keep an eye on the suspicious interns" rules a little better, so this policy behavior wasn't totally silly, but it's almost impossible to actually hide any information now.

(And if someone filed a request for a "Send me an email, secretly" action I'd almost certainly suggest they fork/extend, so I don't really see a way back toward greater rule secrecy.)

Primarily, I'm just hesitant to make changes where you upgrade and previously-private stuff becomes public, since I think that's a terrifying behavior in the general case and maybe even worse than destroying data.

While the argument for changing this particular behavior is pretty strong and rules aren't meaningfully private right now, I think this case isn't a completely black-and-white bug/mistake where no reasonable user could possibly expect "personal" rules to be private in some sense.

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:15 PM

T11428 puts the corporate communication wheels in motion to make this change.

I expect to make the actual change after the release cut this week (circa Aug 6) and that it will promote to stable next week (circa Aug 12) unless we receive feedback that there are policy considerations we've overlooked. I'll discuss this change in this week's and next week's changelogs.