Page MenuHomePhabricator

Sysops are no longer able to add education extension groups
Closed, ResolvedPublic

Description

9aba38ae815 conversion of EducationProgram to extension registration failed to keep the ability of sysops to assign any of the extension user rights.

This is affecting the current wmf deployment, as those groups can no longer be given locally.

Event Timeline

Is this actually a MW core bug?

I note UploadWizard has

	"AddGroups": {
		"sysop": [
			"upwizcampeditors"
		]
	},

But in InitialiseSettings I see...

	'+commonswiki' => [
		'sysop' => [ 'rollbacker', 'confirmed', 'patroller', 'autopatrolled', 'filemover', 'Image-reviewer', 'upwizcampeditors' ],
	],

which makes it hard to see which is actually setting the groups...

And it's fine on my dev wiki...

Add groups: Upload Wizard campaign editors, Course online volunteers, Course campus volunteers, Course instructors and Course coordinators
Remove groups: Upload Wizard campaign editors, Course online volunteers, Course campus volunteers, Course instructors and Course coordinators

So WMF config is breaking it?

Are you perhaps a «powerful bureaucrat» on your dev wiki?

Groups like epcoordinator or epinstructor appear nowhere on mediawiki-config.

February extension code contained:

$wgAddGroups['sysop'] = array_merge( $wgAddGroups['sysop'], [ 'eponline', 'epcampus', 'epinstructor', 'epcoordinator' ] );

How was that translated into json?

Are you perhaps a «powerful bureaucrat» on your dev wiki?

Doesn't matter, the following are under sysop in Special:UserGroupRights, the text is copied verbatim from that table:

Add groups: Upload Wizard campaign editors, Course online volunteers, Course campus volunteers, Course instructors and Course coordinators
Remove groups: Upload Wizard campaign editors, Course online volunteers, Course campus volunteers, Course instructors and Course coordinators

Groups like epcoordinator or epinstructor appear nowhere on mediawiki-config.

And they shouldn't need to

February extension code contained:

$wgAddGroups['sysop'] = array_merge( $wgAddGroups['sysop'], [ 'eponline', 'epcampus', 'epinstructor', 'epcoordinator' ] );

How was that translated into json?

https://github.com/wikimedia/mediawiki-extensions-EducationProgram/blob/master/extension.json#L48

Which is the same as other extensions do it, why they do for other extensions, is presumably due to overrides, misunderstanding of how things work...

https://github.com/wikimedia/mediawiki-extensions-UploadWizard/blob/master/extension.json#L48-L52

https://github.com/wikimedia/mediawiki-extensions-WikimediaIncubator/blob/master/extension.json#L89-L93 -- https://incubator.wikimedia.org/wiki/Special:ListGroupRights it seems 'test-sysop' is added by WMF config similar to how the UW ones are

I should note that I'm not saying it's right, and I've not dug into when those groups were added, but I'm guessing for whatever reason, they were in the config before extension registration happened to those extensions, and hence, the breakage wasn't noticed

AddGroups is only in a handful of extensions, and 3/5 are deployed on WMF wikis. but when 2/3 have their config in InitialiseSettings too doing exactly the same thing.... It's kinda understandable that no other extension noticed this problem

Dev wiki:

$ MW_INSTALL_PATH=/var/www/wiki/mediawiki/core php maintenance/eval.php
var> var_dump( $wgAddGroups );
/var/www/wiki/mediawiki/core/maintenance/eval.php(77) : eval()'d code:1:
array(2) {
  'sysop' =>
  array(5) {
    [0] =>
    string(16) "upwizcampeditors"
    [1] =>
    string(8) "eponline"
    [2] =>
    string(8) "epcampus"
    [3] =>
    string(12) "epinstructor"
    [4] =>
    string(13) "epcoordinator"
  }
  'epcoordinator' =>
  array(3) {
    [0] =>
    string(8) "eponline"
    [1] =>
    string(8) "epcampus"
    [2] =>
    string(12) "epinstructor"
  }
}

But then enwiki has...

	'+enwiki' => [
		'bureaucrat' => [ 'accountcreator', 'flow-bot' ],
		'sysop' => [ 'abusefilter', 'accountcreator', 'autoreviewer', 'confirmed', 'filemover', 'reviewer', 'rollbacker', 'templateeditor', 'massmessage-sender', 'extendedconfirmed', 'extendedmover', 'patroller' ], // T126607, T133981, T149019
	],

and then in PHP this is as below after various other extensions loading stuff in different ways

reedy@tin:~$ mwscript eval.php enwiki
var_dump> var_dump( $wgAddGroups );
array(3) {
  ["epcoordinator"]=>
  array(3) {
    [0]=>
    string(8) "eponline"
    [1]=>
    string(8) "epcampus"
    [2]=>
    string(12) "epinstructor"
  }
  ["sysop"]=>
  array(14) {
    [0]=>
    string(11) "abusefilter"
    [1]=>
    string(14) "accountcreator"
    [2]=>
    string(12) "autoreviewer"
    [3]=>
    string(9) "confirmed"
    [4]=>
    string(9) "filemover"
    [5]=>
    string(8) "reviewer"
    [6]=>
    string(10) "rollbacker"
    [7]=>
    string(14) "templateeditor"
    [8]=>
    string(18) "massmessage-sender"
    [9]=>
    string(17) "extendedconfirmed"
    [10]=>
    string(13) "extendedmover"
    [11]=>
    string(9) "patroller"
    [12]=>
    string(14) "ipblock-exempt"
    [15]=>
    string(8) "reviewer"
  }
  ["bureaucrat"]=>
  array(6) {
    [0]=>
    string(14) "accountcreator"
    [1]=>
    string(8) "flow-bot"
    [2]=>
    string(5) "sysop"
    [3]=>
    string(10) "bureaucrat"
    [4]=>
    string(3) "bot"
    [5]=>
    string(8) "reviewer"
  }
}

So... The + merging done by SiteConfiguration isn't working with extension registration?

A quick glance at https://github.com/wikimedia/mediawiki/blob/master/tests/phpunit/includes/SiteConfigurationTest.php suggests it doesn't even have test cases for what we're doing (sub arrays)... Never mind any further additions by extension registration etc. We should probably fix that, and it might identify some broken-ness

$wgAddGroups['sysop'] = array_merge( $wgAddGroups['sysop'], [ 'eponline', 'epcampus', 'epinstructor', 'epcoordinator' ] );

Depending on how long this may take to fix (and how much of an issue it's causing for various wikis), it may just make sense to add the line above to CommonSettings.php under the guarded $wmgUseEducationProgram as a workaround

@Reedy is it okay if I assign myself considering im working on a patch already?

Change 349427 had a related patch set uploaded (by Zppix):
[operations/mediawiki-config@master] Fixes EducationProgram user rights so that they can be assigned/removed by sysops & Bureaucrats

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

@Zppix This is just a workaround I think. Adding to CS.php shouldn't be required from my POV.

@Urbanecm going through each wiki that runs EP and adding the required code would be tedious and not to mention having it just in one line condenses the space that the fix uses and keep the already huge InitSettings.PHP from getting bigger.

Okay, in another words. Adding EP groups definition anywhere in mediawiki-config (no matter if IS.php or CS.php) shouldn't be required as it is defined in the extension's JSON file. What if we create another supercoordinator group? Would we need to add it to both places? I'm pretty sure we forget this once.

I think it should go in CS (no point duplicating) under the wfLoadExtension with big references to this and the core bug that it should be removed when the core bug T123085 is fixed

It's superficially trivial, but not so trivial when you dig into it. Otherwise it would have been fixed already

Change 349427 merged by jenkins-bot:
[operations/mediawiki-config@master] Fix EducationProgram user rights so that they can be assigned/removed by sysops

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

Confirmed working. Thanks for the report!