Page MenuHomePhabricator

When performing complex edits, pause sub-editors before they publish to propagate "Must Encrypt" and other state
ClosedPublic

Authored by epriestley on Wed, Mar 13, 4:07 PM.

Details

Summary

See PHI1134. Previously, see T13082 and D19969 for some sort-of-related stuff.

Currently, edits work roughly like this:

  • Suppose we're editing object X, and we're also going to edit some other object, Y, because X mentioned Y or the edit is making X a child or parent of Y, or unblocking Y.
  • Do the actual edit to X, including inverse edits ("alice mentioned Y on X.", "alice added a child revision: X", etc) which apply to Y.
  • Run Herald rules on X.
  • Publish the edit to X.

The "inverse edits" currently do this whole process inline, in a sub-editor. So the flow expands like this:

  • Begin editing X.
  • Update properties on X.
    • Begin inverse-edge editing Y.
    • Update properties on Y.
    • Run (actually, skip) Herald rules on Y.
    • Publish edits to Y.
  • Run Herald rules on X.
  • Publish edits to X.

Notably, the "Y" stuff publishes before the "X" Herald rules run. This creates potential problems:

  • Herald rules may change the name or visibility policy of "X", but we'll publish mail about it via the edits to Y before those edits apply. This is a problem only in theory, we don't ship any upstream rules like this today.
  • Herald rules may "Require Secure Mail", but we won't know that at the time we're building mail about the indirect change to "Y". This is a problem in practice.

Instead, switch to this new flow, where we stop the sub-editors before they publish, then publish everything at the very end once all the edits are complete:

  • Begin editing X.
  • Update properties on X.
    • Begin inverse-edge editing Y.
    • Update properties on Y.
    • Skip Herald on Y.
  • Run Herald rules on X.
  • Publish X.
    • Publish all child-editors of X.
      • Publish Y.
Test Plan
  • Created "Must Encrypt" Herald rules for Tasks and Revisions.
  • Edited object "A", an object which the rules applied to directly, and set object "B" (a different object which the rules did not hit) as its parent/child and/or unblocked it.
  • In bin/mail list-outbound, saw:
    • Mail about object "A" all flagged as "Must Encrypt".
    • Normal mail from object B not flagged "Must Encrypt".
    • Mail from object B about changing relationships to object A flagged as "Must Encrypt".

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Wed, Mar 13, 4:07 PM
epriestley requested review of this revision.Wed, Mar 13, 4:09 PM
epriestley added a comment.EditedWed, Mar 13, 4:11 PM

Herald rules may "Require Secure Mail", but we won't know that at the time we're building mail about the indirect change to "Y". This is a problem in practice.

Specifically, the problem this creates is that we may send mail from object T999 which says: alice added a subtask: T123 Description of an important security issue.

Although mail about object T123 is correctly flagged as "Must Encrypt", the indirect mail from T999 is not, and can leak information about T123.

This also probably fixes a couple of minor bugs. For example, I suspect this is currently a bug, although I haven't verified it:

If you use a silent bulk edit to close tasks, parents of those tasks still send non-silent notification email that a subtask was closed.

Moving everything to use a single method for generating a new sub/related editor should help solve this stuff in the future.

amckinley accepted this revision.Mon, Mar 18, 6:48 PM
amckinley added inline comments.
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
1339

All hail Herlad!

This revision is now accepted and ready to land.Mon, Mar 18, 6:48 PM

🎺 HERE COMES HERLAD 🎺

epriestley updated this revision to Diff 48434.Mon, Mar 18, 7:03 PM
  • Farewell, Herlad.
This revision was automatically updated to reflect the committed changes.