Page MenuHomePhabricator

Improve Phortune policy behavior
ClosedPublic

Authored by epriestley on Mar 3 2015, 4:14 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 21, 4:32 AM
Unknown Object (File)
Feb 20 2024, 5:12 PM
Unknown Object (File)
Feb 19 2024, 4:52 AM
Unknown Object (File)
Feb 10 2024, 8:54 AM
Unknown Object (File)
Feb 8 2024, 2:51 AM
Unknown Object (File)
Jan 31 2024, 1:55 AM
Unknown Object (File)
Jan 26 2024, 5:45 PM
Unknown Object (File)
Jan 22 2024, 3:58 PM
Subscribers

Details

Reviewers
btrahan
Commits
Restricted Diffusion Commit
rPab4743b216f2: Improve Phortune policy behavior
Summary

Currently, PhortuneAccounts have a very open default policy to allow merchants to see and interact with them.

This has the undesirable side effect of leaking their names in too many places, because all users are allowed to load the handles for the accounts. Although this information is not super sensitive, we shouldn't expose it.

I went through about 5 really messy diffs trying to fix this. It's very complicated because there are a lot of objects and many of them are related to PhortuneAccounts, but PhortuneAccounts are not bound to a specific merchant. This lead to a lot of threading viewers and merchants all over the place through the call stack and some really sketchy diffs with OmnipotentUsers that weren't going anywhere good.

This is the cleanest approach I came up with, by far:

  • Introduce the concept of an "Authority", which gives a user more powers as a viewer. For now, since we only have one use case, this is pretty open-ended.
  • When a viewer is acting as a merchant, grant them authority through the merchant.
  • Have Accounts check if the viewer is acting with merchant authority. This lets us easily implement the rule "merchants can see this stuff" without being too broad.

Then update the Subscription view to respect Merchant Authority.

I partially updated the Cart views to respect it. I'll finish this up in a separate diff, but this seemed like a good checkpoint that introduced the concept without too much extra baggage.

This feels pretty good/clean to me, overall, even ignoring the series of horrible messes I made on my way here.

Test Plan
  • Verified I can see everything I need to as a merchant (modulo un-updated Cart UIs).
  • Verified I can see nothing when acting as a normal user.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Improve Phortune policy behavior.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.

Broadly, we sort of want to use the merchant itself as the $viewer in these cases, as it's really the merchant that's being policy checked.

Obviously, we can't do that without changing 100 million lines of code, since we have PhabricatorUser $viewer in a gazillion places.

And we don't really want to do that anyway, since we'd still want to know who the real user was, in case they made an update or something.

So I think this ends up being pretty clean and reasonable and sensible: we just make the $viewer more powerful in a specific way for the duration of the request, but not by throwing in the towel and just replacing them with an omnipotent user.

The only part that feels a little flimsy to me is that it seems like grantAuthority($thing) should be typehinted, but I'm not sure what the type of $thing should be. I'm assuming it will become more clear once we have a second use case, and we can nail it down and formalize it later.

We can probably use this approach to clean up at least some overbroad use of OmnipotentUser -- I'd guess authority grants can cover at least a few of the use cases.

btrahan edited edge metadata.
This revision is now accepted and ready to land.Mar 3 2015, 3:23 PM
This revision was automatically updated to reflect the committed changes.