Page MenuHomePhabricator

Remove FlaggedRevs-related user groups from all FlaggedRevs wikis that shouldn't have them
Closed, ResolvedPublic

Description

Problem description

Several FlaggedRevs-enabled wikis decided to remove some of FlaggedRevs-related user groups. However, respective settings from flaggedrevs.php, namely the unset() statements, like this:

	unset( $wgGroupPermissions['editor'] );

This might be relevant to T226054.

Problem diagnosis

  • This must be okay at the end of flaggedrevs.php, since there are if statements (line 776 onwards) that checks if the groups should exist and if so, allow bureaucrats/sysops to assign them.
  • Tried to re-add unset() at the bottom of CommonSettings.php. Since CommonSettings.php is loaded directly by our LocalSettings.php (and said load is the only one statement there), it has to be okay after config loads. Something must've changed that value after.

Original description

Permissions of these groups (@Urbanecm: autoreview and review) was disabled in T202139 and T205997. But these still shown on https://ru.wikisource.org/wiki/Special:UserRights, https://ru.wikisource.org/wiki/Special:ListGroupRights, etc. So, them mistakenly still given to users.
Remove them, please.

Details

Related Gerrit Patches:

Related Objects

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 14 2019, 11:06 AM
Zppix claimed this task.Jun 16 2019, 10:31 PM
Restricted Application added a project: User-Zppix. · View Herald TranscriptJun 16 2019, 10:31 PM
Zppix removed Zppix as the assignee of this task.Jun 16 2019, 10:33 PM
Zppix removed a project: User-Zppix.
Zppix added a subscriber: Zppix.

(Unassigning as i assigned myself by mistake)

MarcoAurelio added a subscriber: Urbanecm.EditedJun 17 2019, 3:15 PM

Permissions were disabled previously but not removed from the wiki config. Given that those are FR permissions and I don't have much experience with them, I am not sure if if we remove:

	unset(
		$wgGroupPermissions['autoreview'], // T202139
		$wgGroupPermissions['reviewer'] // T205997
	);

we won't be breaking stuff...
Ping @Urbanecm.

@MarcoAurelio Thanks for pinging me. Lacking a permission should never make any other mess than "being unable to do something given permission control". However, unset statement is supposed to remove them from the configuration, since they're configured outside of flaggedrevs.php (in FlaggedRevs, probably). If we remove the unset statement, the group should be defined after flaggedrevs.php is loaded. Don't see any autoreviewer configuration for ruwikisource in IS.php nor CS.php.

Checked bswiki as well, which says unset( $wgGroupPermissions['reviewer'] ); in flaggedrevs.php. However, according to https://bs.wikipedia.org/wiki/Posebno:ListaKorisni%C4%8DkihPrava, Editors exists.

What is strange is this:

As quoted by @MarcoAurelio, ruwikisource's section in flaggedrevs.php says:

	unset(
		$wgGroupPermissions['autoreview'], // T202139
		$wgGroupPermissions['reviewer'] // T205997
	);

Unless I'm blind, there's no $wgAddGroups nor $wgRemoveGroups modification. Not sure if flaggedrevs itself handles it, but if it doesn't, then the if must be true for some reason. Not sure why, through. Will investigate more in deep later.

Unless I'm blind, there's no $wgAddGroups nor $wgRemoveGroups modification. Not sure if flaggedrevs itself handles it, but if it doesn't, then the if must be true for some reason. Not sure why, through. Will investigate more in deep later.

Okay, I oversaw an important thing yesterday. https://ru.wikisource.org/wiki/%D0%A1%D0%BB%D1%83%D0%B6%D0%B5%D0%B1%D0%BD%D0%B0%D1%8F:%D0%9F%D1%80%D0%B0%D0%B2%D0%B0_%D0%B3%D1%80%D1%83%D0%BF%D0%BF_%D1%83%D1%87%D0%B0%D1%81%D1%82%D0%BD%D0%B8%D0%BA%D0%BE%D0%B2 does not allow anyone to actually assign reviewer, but does allow sysops to assign autoreview. That's because of https://github.com/wikimedia/operations-mediawiki-config/blob/master/wmf-config/flaggedrevs.php#L79-L80, which allows sysops to assign autoreview unconditionally. I'm going to change that behaviour to behave in a similar way like reviewer (sysops are allowed to do this if, and only if, that group should exist on the wiki according to flaggedrevs.php (same as reviewer user group). That should remove the ability to promote users to autoreviewers from sysops. Since no one would be able to assign the group after that, it'll be useless and the effect would be pretty similar to actual removing.

Of course, I don't plan to close this task after that, but to continue investigating, why the group exists while it shouldn't.

Change 517629 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[operations/mediawiki-config@master] Allow sysops to manage flaggedrevs group membership only if the group exists

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

Change 517629 merged by jenkins-bot:
[operations/mediawiki-config@master] Allow sysops to manage flaggedrevs group membership only if the group exists

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

Mentioned in SAL (#wikimedia-operations) [2019-06-18T12:03:09Z] <urbanecm@deploy1001> Synchronized wmf-config/flaggedrevs.php: [[:gerrit:517629|Allow sysops to manage flaggedrevs group membership only if the group exists]] (T225797) (duration: 00m 47s)

First part is done. No one should be able (apart from stewards) to grant both groups.

Ratte added a subscriber: Ratte.Jun 18 2019, 7:26 PM

BTW, both unable groups (Autoreview and Reviewers) are shown in https://ru.wikisource.org/wiki/Special:Statistics (red links)
In the same time, active groups Importers and Transwiki importers are not shown in https://ru.wikisource.org/wiki/Special:Statistics

Urbanecm renamed this task from Remove the "autoreview" and "reviewer" user groups from ru.wikisource to Remove FlaggedRevs-related user groups from all FlaggedRevs wikis that shouldn't have them.Jun 20 2019, 9:29 PM
Urbanecm updated the task description. (Show Details)
Urbanecm added subscribers: TerraCodes, Amorymeltzer, Reedy and 3 others.

Since there are more wikis that are affected, I've renamed the task to more generic name and merged a dupe here.

Urbanecm updated the task description. (Show Details)Jun 20 2019, 10:05 PM
Quiddity removed a subscriber: Quiddity.Jun 21 2019, 9:46 PM

I've managed to reproduce this issue locally, and according to git bisect, the faulty commit is 4eba7cf0f3e40d6695cfeb9f563efc153a5224e6, authored by @Reedy, reviewed by @Jdforrester-WMF. Definitely something in extension registration.

I think I know what's happening:

  1. flaggedrevs.php calls include "$IP/extensions/FlaggedRevs/FlaggedRevs.php";, which calls wfLoadExtension(). This method calls ExtensionRegistry::queue() and ends.
  2. since the include call finished, flaggedrevs.php continues its work - without $wgGroupPermissions being altered yet.
  3. flaggedrevs.php unsets $wgGroupPermissions['reviewer'] (and others), while it is not set yet, so it's a noop basically
  4. Setup.php calls ExtensionRegistry::loadFromQueue(), $wgGroupPermissions is altered

I've locally tried to call ExtensionRegistry::load() instead of ExtensionRegistry::queue(), groups disappeared, since load() causes the extension to be loaded immediately and not wait till queue is processed.

Reedy added a comment.EditedJun 22 2019, 7:17 PM

We know it was extension registration.... you really didn’t need to bisect to work that out ;)

The problem is the way the config isn’t overriden, it’s merged as it’s not loaded actually immediately, as you say. There’s numerous bugs about it and the problems it’s caused on other extensions (including suggestions of ways to unconditionally override config variables, which was declined)

Basically, it probably means the defaults in FR aren’t sane, and should portentously be fixed

Or we just do all of the config a callback of extension functions when we know we can override stuff as extreg has loaded the defaults :)

Basically it’s all tech debt from how we’ve allowed this sort of stuff in extensions over the years. And not having an extension in extreg is more tech debt too

As above, I’m tempted to just do it all in a callback to solve them all in one go, and clean it up properly later

Reedy closed this task as Resolved.Jun 24 2019, 5:12 PM
Reedy claimed this task.