Page MenuHomePhabricator

Move more code out of PhabricatorUserEditor and into transactions
Needs ReviewPublic

Authored by amckinley on Mon, Jan 21, 11:30 PM.

Details

Reviewers
epriestley
Summary

Finally removes the last of the "role" editing from PhabricatorUserEditor. There are a few rough edges here. It would be nice to move the entirety of user creation out of PhabricatorPeopleNewController, but we aren't there yet. Questions to answer:

  • Should it be possible to turn existing users into bots/mailing lists or vice versa? bin/accountadmin lets you fiddle the isSystemAgent flag, but maybe that was a bad idea from the beginning.
  • Are being a bot and being a mailing list mutually exclusive?
  • Is it correct to require being an admin to take these actions?
  • Should we just not have these transactions at all? These classes seem useful in the future where conduit is capable of creating users, and it's the last of the old "role" editing stuff, but there's also the principle of sleeping dogs.
Test Plan

Made a bunch of users, fiddled them using the UI and bin/accountadmin. I want to do more testing once we've agreed on the answers to the rough edges above.

Grepped for removed methods.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 21634
Build 29498: Run Core Tests
Build 29497: arc lint + arc unit

Event Timeline

amckinley created this revision.Mon, Jan 21, 11:30 PM
amckinley requested review of this revision.Mon, Jan 21, 11:32 PM

The actual code here looks correct to me, I think it's just a question of what we want to do product-wise.


Should it be possible to turn existing users into bots/mailing lists or vice versa? bin/accountadmin lets you fiddle the isSystemAgent flag, but maybe that was a bad idea from the beginning.

I don't think we need to allow this, although it probably (?) doesn't hurt too much today. I think workarounds like "destroy the bot/list" or "rename the bot/list to oops-mistake-bot and disable it" normally cover everything pretty well. The latter has the advantage that you can do it from the web UI.

If we do continue to allow this, I think it should remain CLI-only. Otherwise, you can potentially do sneaky stuff like turn a deploy bot into a real user, establish a web session, and get around API call restrictions from some future version of the software where we let you generate a token with only a subset of permissions. I'd say that thinking through all the attack paths here to make sure nothing bad can really happen probably isn't worth the value of ever having web-UI role switching.

Since "rename" works from the web UI and "re-role" probably can't, maybe that's an argument against continuing to support "re-role".

The fact that we support bot/unbot but not mailing-list/un-mailing-list is maybe an argument for dropping this entirely, since the lack of requests for the latter suggest the former probably isn't seeing much use, either.

Are being a bot and being a mailing list mutually exclusive?

They probably should be. I think they are in practice today, except that you can make a mailing list and then bin/accountadmin it into a bot + mailing list?

Is it correct to require being an admin to take these actions?
Should we just not have these transactions at all? These classes seem useful in the future where conduit is capable of creating users, and it's the last of the old "role" editing stuff, but there's also the principle of sleeping dogs.

Possible approach:

  • Combine them into a single "role" transaction, with values "bot" or "mailing list", to clarify the one-or-the-other thing.
  • Don't let it apply if the user isn't being created ("You can only make a bot or mailing list account when creating a user, you can not transmogrify an existing user into a bot or list.").
  • Remove the ability to flip the switch from bin/accountadmin when not creating a user. Or entirely; I don't think bin/accountadmin has any particular value in bot creation over the web UI.

Then "admin" is the right policy, and this transaction is useful if we let user.edit create users in the future (see also T4184).