Allow "user.edit" to enable or disable users
ClosedPublic
Actions

Authored by epriestley on Aug 13 2018, 10:40 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13164: Plans: 2018 Week 31 - 33 Bonus Content

Commits

rPf9673a72a87d: Allow "user.edit" to enable or disable users

Summary

Depends on D19577. Ref T13164. See PHI642. This adds modern transaction-oriented enable/disable support.

Currently, this also doesn't let you disable normal users even when you're an administrator. I'll refine the policy model later in this change series, since that's also the goal here (let users set "Can Disable Users" to some more broad set of users than "Administrators").

This also leaves us with two different edit pathways: the old UserEditor one and the new UserTransactionEditor one. The next couple diffs will redefine the other pathways in terms of this pathway.

Test Plan

Enabled/disabled a bot.
Tried to disable another non-bot user. This isn't allowed yet, since even as an administrator you don't have CAN_EDIT on them and currently need it: right now, there's no way for a particular set of transactions to say they can move forward with reduced permissions.
Tried to enable/disable myself. This isn't allowed since you can't enable/disable yourself.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Aug 13 2018, 10:40 PM

Harbormaster completed remote builds in B20609: Diff 46809.Aug 13 2018, 10:42 PM

epriestley requested review of this revision.Aug 13 2018, 10:42 PM

amckinley accepted this revision.Aug 13 2018, 10:52 PM

amckinley added inline comments.

src/applications/people/xaction/PhabricatorUserDisableTransaction.php
64	How hard should we try not to let user shoot themselves in the foot? I'm thinking about users disabling all the administrators on an instance (or all the instance managers). I guess as long as you have the enable/disable ability and you can't disable yourself, every situation should at least be recoverable.

This revision is now accepted and ready to land.Aug 13 2018, 10:52 PM

Yeah, I think you can't completely wedge an install because even if "Can Disable User" is "All Users", one user will win the battle royale and be unable to disable themselves. They can then re-enable an administrator and all the damage can be undone from there.

I'll probably add a bin/user disable/enable or similar too just as an easy backup for support and such. I'd like to sort of move away from the very-old, non-scriptable, weird-feeling bin/accountadmin.

You can also use bin/conduit --as the-battle-royale-winner ... to forcefully unwedge an install if you have CLI access but don't want to touch the database, I suppose.

epriestley added a child revision: D19581: Remove deprecated Maniphest "Can Edit <Specific Property>" capabilities.Aug 14 2018, 5:52 PM

Closed by commit rPf9673a72a87d: Allow "user.edit" to enable or disable users (authored by epriestley). · Explain WhyAug 16 2018, 5:49 PM

This revision was automatically updated to reflect the committed changes.