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

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 17 2017, 11:20 PM

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?

Reedy added a comment.Apr 18 2017, 1:29 AM

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

Reedy added a comment.EditedApr 18 2017, 1:37 AM
$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

Zppix added a comment.Apr 21 2017, 8:01 PM

@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 claimed this task.Apr 22 2017, 9:31 PM

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

Urbanecm triaged this task as High priority.Apr 22 2017, 9:38 PM
Zppix added a comment.Apr 22 2017, 9:39 PM

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

Reedy added a comment.Apr 23 2017, 7:58 AM

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

Zppix closed this task as Resolved.Apr 24 2017, 1:35 PM

Confirmed working. Thanks for the report!