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