Why this isn't trivial technically

We're introducing a new concept here. MediaWiki can handle whether a performing user can change a target user's group membership, based on the performer's own permissions. But the concept of whether it's possible based on characteristics of the target user is new.

Related code

One user can change another user's groups via Special:UserRights (SpecialUserRights) or the API (ApiUserRights). The actual work is done in both cases by SpecialUserRights::doSaveUserGroups (as is common in MediaWiki, the API module calls the special page).

Whether a user can change another user's groups is defined in the configs AddGroups, RemoveGroups, and related configs. This can be handily can be looked up via UserGroupManager::getGroupsChangeableByGroup.

Note that there is a separate system for changing global groups, in the CentralAuth extension. This includes SpecialGlobalGroupMembership and ApiGlobalUserRights. I'm ignoring this here, since we don't have a use-case yet for technically restricting global groups.

Possible approaches

Caveat: These would only work for local groups.

Quick, bespoke:

• Run a new hook from SpecialUserRights::changeableGroups that allows you to modify the which groups the performer can change
• Handle that hook in CheckUser: remove temporary-account-viewer from $groups['add'] if the target user does not meet the criteria
• Update SpecialUserRights to ensure that it calls changeableGroups whenever it checks which groups can be changed
• Downside: If we do this, then which groups can be changed is no longer guaranteed by the config - any extension could mess with it

More reusable:

• Add a new config, similar to $wgAddGroups, $wgRemoveGroups, etc, which imposes conditions
• E.g. $wgAddGroups['temporary-account-viewer'] = ['canAdd' => ['sysop', 'steward'], 'needs' => ['&', ['editCount' => 300], ['accountAge' => 86400 * 30 * 6]] ];
• Handle these new configs in UserGroupManager (currently can't be getGroupsChangeableBy, since this is unaware of the target user)
• Update SpecialUserRights to call this new functionality in UserGroupManager
• Downside: We now have 6 different config for specifying who can change other users' groups
• Downside: We need to add condition checking and parsing, similar to autopromote. We'd need to define scope (do we only support account age and edit count at first? Should we first generalize autopromote's condition checking and reuse it?)

Cleaner ideal to aim for:

• Update existing configs $wgAddGroups, $wgRemoveGroups, etc to handle conditions as above.
• Downside: In addition to the above, we would need to update all calling code.

Conclusion

I'm tempted to go for the quick, bespoke option, since we only have one use-case. If we decide to implement this more widely, we can plan a larger piece of work and look further at the other options, and consider global groups too.

FWIW, it'd be great if whatever you come up with here is usable for T391699: Add functionality to disallow bureaucrats who do not have 2fa enabled to grant certain privileged rights/groups as well :-)

Thanks @taavi - I've spoken a little with @Tgr and @EMill-WMF about addressing these problems together. It would be a larger piece of work to implement the reusable options, but I think it's worth doing something along these lines.

I guess the eventual solution will have the added complexity that we'd need to look at:

• Groups of the performing user
• Characteristics of the target user
• Characteristics of the performing user (e.g. whether they have 2FA) - not covered in T393615#10801569

Thinking again about our autopromote conditions, maybe we could use similar condition checking for both the performing user and the target user.

(While we work this out, we might have to do something quick and temporary just to support temporary accounts so that people can start being added to the temporary-account-viewergroup sooner rather than later.)

In T393615#10801569, @Tchanders wrote:

I'm tempted to go for the quick, bespoke option, since we only have one use-case. If we decide to implement this more widely, we can plan a larger piece of work and look further at the other options, and consider global groups too.

+1, I think that would be fine for now. The larger, more generic solution should be done after we've had enough time (without a deadline looming) to gather requirements.

In general IMO it's a good idea to wait for multiple use cases before trying to generalize something, unless you are confident it will be needed eventually.

Wrt adding a hook vs supporting autopromote conditions (or something similar) in $wgAddGroups, I'm not sure which one of those is more reusable. The hook approach is more flexible, which can be a good thing or a bad thing. It also seems more messy - if you want a UI like the one shown in F59515477, autopromote conditions would be specific enough to be able to automatically generate that. With a hook, it would have to be able to provide a message explaining why that option was removed (so it would have to return something like a new group name => Message array, rather than just modifying $wgAddGroups in place). Or you need another hook for manipulating the form descriptor.

It's also not obvious to me at a glance whether one would be much more work than the other. With the configuration approach, UserGroupManager::checkCondition() would have to be moved to a new class, maybe "autopromote" renamed to "promote" in some places (e.g. PCOND_* rather than APCOND_*), and then the conditions checked when rendering the UI, I think that's pretty much it.

I guess it's also somewhat of a product question. Where do you want that "access criteria" link to go? To a dedicated help page? Or an auto-generated list of requirements? The latter is only doable with a promote condition array.

In T393615#10801595, @taavi wrote:

FWIW, it'd be great if whatever you come up with here is usable for T391699: Add functionality to disallow bureaucrats who do not have 2fa enabled to grant certain privileged rights/groups as well :-)

The problem with that is that it's almost entirely useless as described. An attacker can take over a bureaucrat account, set up 2FA, and then grant interface-admin - it is barely a slowdown. It doesn't even qualify as security by obscurity since the UI will tell they need to set up 2FA first. (Or at least I assume it would. It does for the similar $wgOATHRequiredForGroups feature.)

We will probably need something very unique and un-generalizable there. I proposed T393085: Require email verification (sometimes) to enable 2FA, but maybe we want some sort of review queue instead, or a delay in some bureaucrat actions taking effect.
For some potential solutions, this task would still be useful. E.g. if we go with T393085, we could still use this feature, we'd just slightly change what it means to check whether the user has 2FA enabled. For other solutions, it would be irrelevant. So we should probably figure out the long term plan for T391699 first before considering it a use case.

BTW, what happens if the user group change happens via the API? Should that also support a somewhat specific error message like the UI, or just a slightly misleading "you cannot assign this group to others"? A specific message would again be much easier with a condition array. But then that runs the risk that you build this elaborate thing for limiting user group assignments, and the next time an actual use case shows up, it doesn't actually fit. Hooks are more flexible in that regard.

Change #1143668 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/core@master] SpecialUserRights: Check changeable groups via a single method

https://gerrit.wikimedia.org/r/1143668

Change #1143669 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/core@master] WIP Introduce hook for enforcing requirements on group members

https://gerrit.wikimedia.org/r/1143669

Wrt adding a hook vs supporting autopromote conditions (or something similar) in $wgAddGroups, I'm not sure which one of those is more reusable.

I think the single config would be easier to use, since adding or changing group requirements could be done with a config patch, and per wiki / per group settings would be easier to find.

The hook approach is more flexible, which can be a good thing or a bad thing. It also seems more messy - if you want a UI like the one shown in F59515477, autopromote conditions would be specific enough to be able to automatically generate that. With a hook, it would have to be able to provide a message explaining why that option was removed (so it would have to return something like a new group name => Message array, rather than just modifying $wgAddGroups in place). Or you need another hook for manipulating the form descriptor.

I think we'd want a hook with quite a narrow application to defend against the possibility that it takes on a life of its own and becomes difficult to walk back. I like the idea of returning a group name => Message array, then narrowly interpreting it as a disallowed group and a reason - this would limit the hook's power, rather than giving extensions free reign to mess with the config. (https://gerrit.wikimedia.org/r/1143669 implements this.)

Though I could also imagine changing the product specs when we build this as a more general solution, and displaying what the actual restrictions are rather than linking out to various different policies. The link in the current designs is to our access policy, so the text will need to be provided via WikimediaMessages.

It's also not obvious to me at a glance whether one would be much more work than the other. With the configuration approach, UserGroupManager::checkCondition() would have to be moved to a new class, maybe "autopromote" renamed to "promote" in some places (e.g. PCOND_* rather than APCOND_*), and then the conditions checked when rendering the UI, I think that's pretty much it.

The extra work I'm thinking of is mostly figuring out the requirements and scope (do we configure adding and removing groups separately, do we configure requirements for the performer too, does anything change if the performer is changing their own groups, do the requirements interact with group expiries at all, what should we do for global groups, etc). Also responding to odd cases the community knows about but we didn't anticipate, improving existing architecture/test coverage where necessary, etc. I don't feel confident that taking on this work wouldn't push back temporary accounts deployment... But I think it would be a really nice thing to do once we don't have that time pressure.

BTW, what happens if the user group change happens via the API? Should that also support a somewhat specific error message like the UI, or just a slightly misleading "you cannot assign this group to others"? A specific message would again be much easier with a condition array. But then that runs the risk that you build this elaborate thing for limiting user group assignments, and the next time an actual use case shows up, it doesn't actually fit. Hooks are more flexible in that regard.

It seems like it would be acceptable not to explain why the group assignment didn't work via the API, for now at least. It's not currently explained when it fails because the performer didn't have permission, as documented here. Although it would be a bit harder to find the reason (e.g. ApiQueryUserInfo wouldn't tell you since it only looks at the permissions), any performing user shouldn't be trying to assign a group like this without understanding the policy first.

One other requirement we should consider for both the immediate needs of this task and the longer term solution: make it easy/possible for user user groups to be able to be exempted from the restrictions when adding a user to a group, e.g. T389808: Allow stewards to assign IP Reveal to cross-wiki patrollers who do not meet the thresholds defined for technical enforcement

Change #1143668 merged by jenkins-bot:

[mediawiki/core@master] SpecialUserRights: Check changeable groups via a single method

https://gerrit.wikimedia.org/r/1143668

Change #1143869 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/CheckUser@master] WIP Enforce requirements for temporary-account-viewer group

https://gerrit.wikimedia.org/r/1143869

Change #1143878 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/core@master] SpecialUserRightsTest: Add a basic test to cover showing the form

https://gerrit.wikimedia.org/r/1143878

Change #1143878 merged by jenkins-bot:

[mediawiki/core@master] SpecialUserRightsTest: Add a basic test to cover showing the form

https://gerrit.wikimedia.org/r/1143878

In T393615#10804789, @Tgr wrote:

In T393615#10801595, @taavi wrote:

FWIW, it'd be great if whatever you come up with here is usable for T391699: Add functionality to disallow bureaucrats who do not have 2fa enabled to grant certain privileged rights/groups as well :-)

The problem with that is that it's almost entirely useless as described. An attacker can take over a bureaucrat account, set up 2FA, and then grant interface-admin - it is barely a slowdown.

It's far from perfect security, but this was also viewed as a way to nudge a significant number of bureaucrats into setting up 2fa for their accounts. At least that was one of my hopes.

In T393615#10814201, @sbassett wrote:

In T393615#10804789, @Tgr wrote:

In T393615#10801595, @taavi wrote:

FWIW, it'd be great if whatever you come up with here is usable for T391699: Add functionality to disallow bureaucrats who do not have 2fa enabled to grant certain privileged rights/groups as well :-)

The problem with that is that it's almost entirely useless as described. An attacker can take over a bureaucrat account, set up 2FA, and then grant interface-admin - it is barely a slowdown.

It's far from perfect security, but this was also viewed as a way to nudge a significant number of bureaucrats into setting up 2fa for their accounts. At least that was one of my hopes.

Yes, this is how I think of it - it's acting right now as a short-term lever to get 2FA deployed across the user groups we're applying it to. And that is a security benefit: once 2FA is enabled, their accounts are not vulnerable to takeover through credential stuffing. What it doesn't do is protect the accounts which haven't moved yet. So, it's not a security control, but is effective as part of a security campaign for now. A clear next step in that area is to design a security control, potentially replacing the feature we're currently using. That would also seem like the right time to explore a generalized enforcement framework for privileges and group membership.

@KColeman-WMF @Niharika I'm raising a design/product question that was asked in code review. In the situation where a steward is accessing Special:UserRights, and does not need to check the requirements, do we need to let them know that there are restrictions, which they can choose to ignore?

Change #1143669 merged by jenkins-bot:

[mediawiki/core@master] Introduce hook for enforcing requirements on group members

https://gerrit.wikimedia.org/r/1143669

Change #1143869 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Enforce requirements for temporary-account-viewer group

https://gerrit.wikimedia.org/r/1143869

Change #1147725 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/core@master] SpecialUserRights: Use HTML::rawElement for unaddable reason

https://gerrit.wikimedia.org/r/1147725

Change #1147726 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/WikimediaMessages@master] Add message to explain why user cannot be added to IP reveal group

https://gerrit.wikimedia.org/r/1147726

Change #1147725 merged by jenkins-bot:

[mediawiki/core@master] SpecialUserRights: Use HTML::rawElement for unaddable reason

https://gerrit.wikimedia.org/r/1147725

Change #1147726 merged by jenkins-bot:

[mediawiki/extensions/WikimediaMessages@master] Add message to explain why user cannot be added to IP reveal group

https://gerrit.wikimedia.org/r/1147726

@Niharika Should we add the configuration to technically enforce requiring 300 edits and 6 months, with an exception in case the user granting the group is a steward?

In T393615#10839325, @Tchanders wrote:

@Niharika Should we add the configuration to technically enforce requiring 300 edits and 6 months, with an exception in case the user granting the group is a steward?

Yes, please. Let's go ahead with that.

Change #1148827 had a related patch set uploaded (by Tchanders; author: Tchanders):

[operations/mediawiki-config@master] Temp accounts: Set group requirements for IP reveal group

https://gerrit.wikimedia.org/r/1148827

Thanks @Niharika - that's done in the latest patch.

Moving this to QA, since it's ready to test locally.

Testing steps

Specs are described in T325451: [Epic] Users with right privileges are able to view IP addresses

Setup:

• Can be tested on a local installation
• Needs CheckUser: wfLoadExtension('CheckUser');
• To see the correct message from T325451, also needs WikimediaMessages: wfLoadExtension('WikimediaMessages');
• Configure the IP reveal group restrictions (these are just an example, to make the testing steps consistent, and can be adjusted):

$wgCheckUserGroupRequirements = [
	'temporary-account-viewer' => [
		'edits' => 300,
		'age' => 86400 * 30 * 6,
		'exemptGroups' => [ 'bureaucrat' ],
		'reason' => 'checkuser-group-requirements-temporary-account-viewer',
	],
];

• Adjust edits and age to work with your local installation's users if necessary, so some meet the thresholds and others don't
• Ensure that both sysop and bureaucrat groups have the userrights permission

Testing:

• As a sysop, go to Special:UserRights and enter a user who meets the criteria. It should be possible to add and remove the temporary account IP viewer group.
• Now enter the name of a user who does not meet the criteria. The temporary account IP viewer group should appear in the list of groups you can change, with the reason as specified.
• As a bureaucrat, visit Special:UserRights and enter the user who does not meet the criteria. It should be possible to add and remove temporary account IP viewer.

Change #1148827 merged by jenkins-bot:

[operations/mediawiki-config@master] Temp accounts: Set group requirements for IP reveal group

https://gerrit.wikimedia.org/r/1148827

Mentioned in SAL (#wikimedia-operations) [2025-05-22T07:22:43Z] <stran@deploy1003> Started scap sync-world: Backport for [[gerrit:1148827|Temp accounts: Set group requirements for IP reveal group (T393615)]]

Mentioned in SAL (#wikimedia-operations) [2025-05-22T07:25:02Z] <stran@deploy1003> tchanders, stran: Backport for [[gerrit:1148827|Temp accounts: Set group requirements for IP reveal group (T393615)]] synced to the testservers (see https://wikitech.wikimedia.org/wiki/Mwdebug). Changes can now be verified there.

Mentioned in SAL (#wikimedia-operations) [2025-05-22T07:32:45Z] <stran@deploy1003> Finished scap sync-world: Backport for [[gerrit:1148827|Temp accounts: Set group requirements for IP reveal group (T393615)]] (duration: 10m 01s)

Change #1149699 had a related patch set uploaded (by Tchanders; author: Tchanders):

[operations/mediawiki-config@master] Temp accounts: Allow sysop/steward to grant and revoke IP reveal

https://gerrit.wikimedia.org/r/1149699

In T393615#10852997, @gerritbot wrote:

Change #1149699 had a related patch set uploaded (by Tchanders; author: Tchanders):

[operations/mediawiki-config@master] Temp accounts: Allow sysop/steward to grant and revoke IP reveal

https://gerrit.wikimedia.org/r/1149699

Wrong task tagged - updated the commit to tag T390942: Allow IP viewer temporary account group to be manually granted on all projects instead.

Change #1151265 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/WikimediaMessages@master] Fix link in checkuser-group-requirements-temporary-account-viewer

https://gerrit.wikimedia.org/r/1151265

Change #1151265 merged by jenkins-bot:

[mediawiki/extensions/WikimediaMessages@master] Fix link in checkuser-group-requirements-temporary-account-viewer

https://gerrit.wikimedia.org/r/1151265

Using localhost for testing: the new code changes have been verified and are functioning as expected (Per the Acceptance Criteria). Thank you @Tchanders.