Page MenuHomePhabricator

Introduce CAN_EDIT for ExternalAccount, and make CAN_VIEW more liberal
ClosedPublic

Authored by epriestley on Jun 28 2014, 6:05 PM.
Tags
None
Referenced Files
F13051654: D9767.id23705.diff
Fri, Apr 19, 3:40 AM
F13051653: D9767.id23449.diff
Fri, Apr 19, 3:40 AM
F13051652: D9767.id.diff
Fri, Apr 19, 3:40 AM
Unknown Object (File)
Thu, Apr 11, 11:31 AM
Unknown Object (File)
Thu, Apr 11, 8:48 AM
Unknown Object (File)
Thu, Apr 11, 4:53 AM
Unknown Object (File)
Tue, Apr 9, 9:37 AM
Unknown Object (File)
Tue, Apr 9, 9:37 AM
Subscribers

Details

Reviewers
btrahan
chad
Maniphest Tasks
Restricted Maniphest Task
T1205: Allow grey users in some form or other
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rPe46826ad364d: Introduce CAN_EDIT for ExternalAccount, and make CAN_VIEW more liberal
Summary

Fixes T3732. Ref T1205. Ref T3116.

External accounts (like emails used as identities, Facebook accounts, LDAP accounts, etc.) are stored in "ExternalAccount" objects.

Currently, we have a very restrictive CAN_VIEW policy for ExternalAccounts, to add an extra layer of protection to make sure users can't use them in unintended ways. For example, it would be bad if a user could link their Phabricator account to a Facebook account without proper authentication. All of the controllers which do sensitive things have checks anyway, but a restrictive CAN_VIEW provided an extra layer of protection. Se T3116 for some discussion.

However, this means that when grey/external users take actions (via email, or via applications like Legalpad) other users can't load the account handles and can't see anything about the actor (they just see "Restricted External Account" or similar).

Balancing these concerns is mostly about not making a huge mess while doing it. This seems like a reasonable approach:

  • Add CAN_EDIT on these objects.
  • Make that very restricted, but open up CAN_VIEW.
  • Require CAN_EDIT any time we're going to do something authentication/identity related.

This is slightly easier to get wrong (forget CAN_EDIT) than other approaches, but pretty simple, and we always have extra checks in place anyway -- this is just a safety net.

I'm not quite sure how we should identify external accounts, so for now we're just rendering "Email User" or similar -- clearly not a bug, but not identifying. We can figure out what to render in the long term elsewhere.

Test Plan
  • Viewed external accounts.
  • Linked an external account.
  • Refreshed an external account.
  • Edited profile picture.
  • Viewed sessions panel.
  • Published a bunch of stuff to Asana/JIRA.
  • Legalpad signature page now shows external accounts.

Screen_Shot_2014-06-28_at_11.00.36_AM.png (1×1 px, 159 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Introduce CAN_EDIT for ExternalAccount, and make CAN_VIEW more liberal.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: chad, btrahan.
epriestley added tasks: Restricted Maniphest Task, T1205: Allow grey users in some form or other, Restricted Maniphest Task.

T5504 discusses balancing concerns when rendering these things in the UI.

This one can safely wait for @btrahan to come back -- all it does is turn "Restricted External User" into "Email User" in a few places in the UI, and it doesn't block anything.

btrahan edited edge metadata.
This revision is now accepted and ready to land.Jul 10 2014, 5:03 PM
epriestley updated this revision to Diff 23705.

Closed by commit rPe46826ad364d (authored by @epriestley).