Page MenuHomePhabricator

Fix an issue where handles could load with the incorrect viewer when building mail about changes to related objects
ClosedPublic

Authored by epriestley on May 19 2019, 10:59 PM.

Details

Summary

See https://phabricator.wikimedia.org/T179591. Some time ago, all handle rendering preloaded handles: things emitted a list of PHIDs they'd need handles for, then later used only those PHIDs.

Later, we introduced HandlePool and lazy/on-demand handle loading. Modern transactions mostly use this to render object PHIDs.

When we build mail, many newer transactions use an on-demand load to fetch handles to render transactions. This on-demand load may use the original viewer (the acting user) instead of the correct viewer (the mail recipient): we fetch and reset handles using the correct viewer, but do not overwrite the active viewer for on-demand loading. This could cause mail to leak the titles of related objects to users who don't have permission to see them.

Instead, just reload the transactions with the correct viewer when building mail instead of playing a bunch of setViewer() and clone games. Until we're 100% on modular transactions, several pieces of the stack cache viewer or state information.

Test Plan
  • Created task A (public) with subtask B (private).
  • Closed subtask B as a user with access to it.
  • Viewed mail sent to subscribers of task A who can not see subtask B.
    • Before change: mail discloses title of subtask B.
    • After change: mail properly labels subtask B as "Restricted Task".

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.May 19 2019, 10:59 PM
epriestley edited the test plan for this revision. (Show Details)May 19 2019, 11:00 PM
epriestley requested review of this revision.May 19 2019, 11:01 PM
amckinley accepted this revision.May 21 2019, 5:43 PM
This revision is now accepted and ready to land.May 21 2019, 5:43 PM

Also, I think this only affects "closed subtask X" stories: all the other "related object" stories use edges, which are still on the older handle-based stuff.