Page MenuHomePhabricator

Update accountadmin to use new admin empowerment code
ClosedPublic

Authored by amckinley on Dec 19 2018, 7:32 PM.
Tags
None
Referenced Files
F12838122: D19915.id47536.diff
Thu, Mar 28, 5:52 PM
Unknown Object (File)
Sat, Mar 23, 5:44 PM
Unknown Object (File)
Wed, Mar 20, 3:20 PM
Unknown Object (File)
Feb 10 2024, 1:41 AM
Unknown Object (File)
Jan 12 2024, 1:17 PM
Unknown Object (File)
Jan 9 2024, 9:20 AM
Unknown Object (File)
Dec 26 2023, 7:22 PM
Unknown Object (File)
Dec 21 2023, 9:50 AM
Subscribers

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 Passed
Unit
Tests Passed
Build Status
Buildable 21389
Build 29125: Run Core Tests
Build 29124: arc lint + arc unit

Event Timeline

src/applications/people/xaction/PhabricatorUserEmpowerTransaction.php
49

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

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. 🐴

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

epriestley added inline comments.
src/applications/auth/controller/PhabricatorAuthRegisterController.php
426–427 ↗(On Diff #47536)

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.