Page MenuHomePhabricator

FlaggedRevs-specific group rights from core-Permissions.php get overridden by flaggedrevs.php
Open, Needs TriagePublic

Description

T414277 requested removing the movestable right from the autoconfirmed and confirmed groups. However, despite the attached config change setting 'movestable' => false for those two groups, they still have the right after that change was deployed:

$ curl -sLA'https://phabricator.wikimedia.org/T414684' 'https://uk.wikipedia.org/w/api.php?action=query&meta=siteinfo&siprop=usergroups&format=j
son&formatversion=2' | jq '.query.usergroups | .[] | select(.rights | contains(["movestable"])) | { name, rights }'                                                                           
{                                                                                                                                                                                             
  "name": "autoconfirmed",                                                                                                                                                                    
  "rights": [                                                                                                                                                                                 
    "movestable",                                                                                                                                                                             
    "reupload",                                                                                                                                                                               
    "upload",                                                                                                                                                                                 
    "move",                                                                                                                                                                                   
    "collectionsaveasuserpage",                                                                                                                                                               
    "collectionsaveascommunitypage",
    "autoconfirmed",
    "editsemiprotected",
    "skipcaptcha",
    "abusefilter-log-detail",
    "abusefilter-view",
    "abusefilter-log",
    "transcode-reset",
    "transcode-status"
  ]
}
{                                                                                                                                                                                             
  "name": "bot",                                                                                                                                                                              
  "rights": [ /* snip */ ]
}
{                                                                                                                                                                                             
  "name": "sysop",                                                                                                                                                                            
  "rights": [ /* snip */ ]
}
{
  "name": "autoreview",
  "rights": [ /* snip */ ]
}
{
  "name": "editor",
  "rights": [ /* snip */ ]
}
{
  "name": "confirmed",
  "rights": [
    "movestable",
    "reupload",
    "upload",
    "move",
    "collectionsaveasuserpage",
    "collectionsaveascommunitypage",
    "autoconfirmed",
    "editsemiprotected",
    "skipcaptcha",
    "abusefilter-log-detail",
    "abusefilter-view",
    "abusefilter-log",
    "transcode-reset",
    "transcode-status"
  ]
}

@A_smart_kitten suspects (#wikimedia-operations 2026-01-15) that this is due to FlaggedRevs’ special config handling, and that the setting needs to be set in flaggedrevs.php instead. And indeed that file already contains other group permission changes, such as:

} elseif ( $wgDBname == 'elwikinews' ) {
	$wgFlaggedRevsNamespaces[] = NS_CATEGORY;
	$wgFlaggedRevsNamespaces[] = 100;
	$wgGroupPermissions['editor']['rollback'] = true;
	$wgGroupPermissions['editor']['autoreview'] = false;
	$wgGroupPermissions['sysop']['stablesettings'] = true;
	$wgGroupPermissions['sysop']['autoreview'] = false;

	unset( $wgGroupPermissions['reviewer'] );
}

Based on this, I expect we should:

  • move movestable right assignments from core-Permissions.php to flaggedrevs.php (not just for ukwiki, but also for ruwikinews, which has a similar override credited to T201265), and
  • add a PHPUnit test to check for any mistaken movestable right assignments in core-Permissions.php (with a message like “you need to add this to flaggedrevs.php instead”), to catch other instances of this mistake in CI in future

Event Timeline

I currently believe that this line in flaggedrevs.php is what's likely causing the attempted removals of movestable from the autoconfirmed groups on ukwiki & ruwikinews to not have an effect:

$wgHooks['MediaWikiServices'][] = static function () {
	global $wgAddGroups, $wgDBname, $wgDefaultUserOptions,
		$wgFlaggedRevsNamespaces, $wgFlaggedRevsRestrictionLevels,
		$wgFlaggedRevsTags, $wgFlaggedRevsTagsRestrictions,
		$wgGroupPermissions, $wgRemoveGroups;

	///////////////////////////////////////
	// Common configuration
	// DO NOT CHANGE without hard-coding these values into the relevant wikis first.
	///////////////////////////////////////
[...]
	$wgGroupPermissions['autoconfirmed']['movestable'] = true; // T16166

I believe that this global override would then currently also propagate to the confirmed group by virtue of $wgGroupInheritsPermissions.

  • add a PHPUnit test to check for any mistaken movestable right assignments in core-Permissions.php (with a message like “you need to add this to flaggedrevs.php instead”), to catch other instances of this mistake in CI in future

Would it make sense to say that assignments of all permissions from the FlaggedRevs extension should be set in flaggedrevs.php, so that they're all in one place (and in case e.g. another user-group is given a FlaggedRevs permission default from that file in the future)?

Would it make sense to say that assignments of all permissions from the FlaggedRevs extension should be set in flaggedrevs.php, so that they're all in one place (and in case e.g. another user-group is given a FlaggedRevs permission default from that file in the future)?

Yeap. Moving as many flaggedrevs user groups / user rights / config settings to flaggedrevs.php as possible sounds like a good idea to me. Just be careful to keep them in the same order when moving them. Some might need to go at the top of the file, some at the bottom, to match their current load orders.

Change #1227385 had a related patch set uploaded (by A smart kitten; author: A smart kitten):

[operations/mediawiki-config@master] ukwiki: Move assignments of FlaggedRevs permissions to flaggedrevs.php

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

  • move movestable right assignments from core-Permissions.php to flaggedrevs.php [...]

I've uploaded a patch that I believe should do this for ukwiki, to hopefully resolve the issue with that wiki's config for right now.
I don't necessarily have any objection to anyone else doing the same thing for ruwikinews; but I'm just holding off myself at the minute, given the length of time since https://gerrit.wikimedia.org/r/455232 was merged for T201265 in 2018 (ie., in case it'd be a surprise to [some editors on] the wiki for this config to be suddenly changed at this point).


	$wgGroupPermissions['autoconfirmed']['movestable'] = true; // T16166

For posterity, this global override in flaggedrevs.php seems to date back to the initial commit of the mediawiki-config repository AFAICS.

I don't necessarily have any objection to anyone else doing the same thing for ruwikinews; but I'm just holding off myself at the minute, given the length of time since https://gerrit.wikimedia.org/r/455232 was merged for T201265 in 2018 (ie., in case it'd be a surprise to [some editors on] the wiki for this config to be suddenly changed at this point).

I don't think "has been this way for 8 years" is a great reason not to refactor something. Refactors are no-ops and we need to be able to improve code at any time. But up to you :)

Refactors are no-ops [...]

For ruwikinews, it wouldn't be a no-op, though; as their config override for 'autoconfirmed' => [ 'movestable' => false ] that should be having an effect currently isn't.

Change #1227385 merged by jenkins-bot:

[operations/mediawiki-config@master] ukwiki: Move assignments of FlaggedRevs permissions to flaggedrevs.php

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

Mentioned in SAL (#wikimedia-operations) [2026-01-15T21:33:53Z] <jhuneidi@deploy2002> Started scap sync-world: Backport for [[gerrit:1227385|ukwiki: Move assignments of FlaggedRevs permissions to flaggedrevs.php (T414277 T414684)]], [[gerrit:1227394|ukwiki: Add "changetags" to sysop user group. (T414277)]]

Mentioned in SAL (#wikimedia-operations) [2026-01-15T21:35:49Z] <jhuneidi@deploy2002> asmartkitten, seawolf35gerrit, jhuneidi: Backport for [[gerrit:1227385|ukwiki: Move assignments of FlaggedRevs permissions to flaggedrevs.php (T414277 T414684)]], [[gerrit:1227394|ukwiki: Add "changetags" to sysop user group. (T414277)]] synced to the testservers (see https://wikitech.wikimedia.org/wiki/Mwdebug). Changes can now be verified there.

Mentioned in SAL (#wikimedia-operations) [2026-01-15T21:41:39Z] <jhuneidi@deploy2002> Finished scap sync-world: Backport for [[gerrit:1227385|ukwiki: Move assignments of FlaggedRevs permissions to flaggedrevs.php (T414277 T414684)]], [[gerrit:1227394|ukwiki: Add "changetags" to sysop user group. (T414277)]] (duration: 07m 46s)

Would it make sense to say that assignments of all permissions from the FlaggedRevs extension should be set in flaggedrevs.php, so that they're all in one place (and in case e.g. another user-group is given a FlaggedRevs permission default from that file in the future)?

Yeap. Moving as many flaggedrevs user groups / user rights / config settings to flaggedrevs.php as possible sounds like a good idea to me. Just be careful to keep them in the same order when moving them. Some might need to go at the top of the file, some at the bottom, to match their current load orders.

Moving FlaggedRevs permissions to one place is definitely a good idea, but I’d rather move them to where other permissions are, i.e. core-Permissions.php, if possible, so that one can get a quick overview of what permission customization (and what groups) exist on a given wiki.

For ruwikinews, it wouldn't be a no-op, though; as their config override for 'autoconfirmed' => [ 'movestable' => false ] that should be having an effect currently isn't.

Absolutely agree. Removing a permission eight years after it has been requested could come as a quite inconvenient and disruptive surprise. Unfortunately the user who filed T201265 has been banned since then, so a new contact needs to be found.