Page MenuHomePhabricator

Update accountadmin to use new admin empowerment code
ClosedPublic

Authored by amckinley on Dec 19 2018, 7:32 PM.

Details

Summary

Fixes https://discourse.phabricator-community.org/t/admin-account-creation-fails-call-to-undefined-method-phabricatorusereditor-makeadminuser/2227. This callsite got skipped when updating the EmpowerController to use the new transactional admin approval code.

Test Plan

Invoked accountadmin to promote a user, no longer got an exception.

Diff Detail

Repository
rP Phabricator
Branch
account-admin (branched from master)
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 21390
Build 29127: Run Core Tests
Build 29126: arc lint + arc unit

Event Timeline

amckinley created this revision.Dec 19 2018, 7:32 PM
amckinley added inline comments.Dec 19 2018, 7:32 PM
src/applications/people/xaction/PhabricatorUserEmpowerTransaction.php
49

I'm not wild about this change; is there a better way?

amckinley requested review of this revision.Dec 19 2018, 7:34 PM

I'm not wild about this change; is there a better way?

You can do the actual check with ->isOmnipotent() instead of === $omnipotent_user, which is a little better (there is no guarantee that two omnipotent users are actually === identical -- it's possible something clone'd an ominpotent user).

There isn't really a cleaner way to do this today, since no object actually exposes any "can do user management" capability with an "administrators" policy. If something did, we could check against that capability or do a PhabricatorPolicyFilter::requireCapability(), but you can only require capabilities -- there's no API for "does user X satisfy policy Y in the abstract" since this question isn't meaningful in the general case -- you can't ask if user X "is a subscriber" or "is an author" without having some object to be asking that question about.

We could say that managing users now requires edit access to the People application instead of edit access to the user; this is a testable capability. The default value is "Administrators" so this wouldn't be a policy change, and it would let users restrict or expand who is allowed to do management stuff. I'm hesitant to just do this offhand, though, and we'd probably need to update a bunch of strings at a minimum ("You must be an administrator to do X...").

For the moment, I'd say just do isOmnipotent() and we can sort this out when there's a stronger case for refining user management?

Change looks good otherwise. 🐴

amckinley updated this revision to Diff 47536.Dec 19 2018, 7:56 PM

Fix other callsite. Tested by blowing away DB and going through initial setup.

epriestley accepted this revision.Dec 19 2018, 7:58 PM
epriestley added inline comments.
src/applications/auth/controller/PhabricatorAuthRegisterController.php
426–427

This should probably be PhabricatorContentSource::newFromRequest($request) instead of a console source, although this is a minor bookkeeping issue.

This revision is now accepted and ready to land.Dec 19 2018, 7:58 PM
This revision was automatically updated to reflect the committed changes.
amckinley marked an inline comment as done.Dec 19 2018, 8:06 PM