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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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