Page MenuHomePhabricator

Changing user groups from $wgExtensionFunctions no longer works reliably
Open, Needs TriagePublic

Description

Recent changes in MediaWiki caused that changing user groups from $wgExtensionFunctions no longer works reliably. That caused a couple of bugs in Wikimedia production, and I do think that a lot of third party users are also going to be affected. I'll add the Wikimedia bugs as subtasks soon, trying to fix them in our own environment, but I also think we should make it work as before for MW-1.36-notes, or clearly communicate the breakage (and perhaps throw an exception to make it clear).

Related Objects

Event Timeline

@Pchelolo I created this task to track fixing of the root cause of the recent issues (wgExtensionFunctions can't no longer reliably change permissions). Some of the subtasks don't have a simple config-only solution, considering it's impossible to ie. delete extension-provided groups in config outside of an extension function. In T275333#6847294, you said T273511: Convert CentralAuthUtilityService to Authority instead of PermissionManager and T273510: Convert AuthManager to Authority. Any thoughts on whether anything can be done in Wikimedia config are appreciated.

holger.knust added a subscriber: holger.knust.

@Pchelolo Is there anything for us to do? Put it on Watching for now

$wgExtensionFunctions callbacks are run late in the bootstrap sequence, when setup is complete. At that point, changing globals may no longer have any affect, since service instances may already have been created.

Running $wgExtensionFunctions earlier would break the expectation that setup is complete when these functions run.

So, if we run the callbacks late (as we do now), we break the expectation that they can manipulate config. If we run them earlier, we break the expectation that bootstrap is complete.

The only solution I currently see for extensions that want to change globals during setup in a way that gets picked up by the service container will have to use the MediaWikiServices hook instead.

In the future, when moving away from global variables for configuration, we may introduce a hook especially for manipulating config settings.

Thanks for summarizing the issue @daniel. Breaking the assumption extension functions can modify config sadly means that there is no longer any way for local site administrators to alter extension provided config. If I unset a group in LocalSettings.php, it doesn't work, because the extensions are loaded after LS.php is. If I do it in extension function, it doesn't work, because some services are already initiated. Where else can I do this kind of a task?

We need something that runs after extensions are loaded, but before any config is actually used by services.

Thanks for summarizing the issue @daniel. Breaking the assumption extension functions can modify config sadly means that there is no longer any way for local site administrators to alter extension provided config.

For config settings that extensions expose via the "config" key, that works properly, right?

In general, extensions should not override config coming from LocalSettings.php. And I think in general, they don't. Things get tricky if the change you want to make is to unset it. Then the extension would put it back. But I'd expect that to be rare. What are the concrete use cases?

If I unset a group in LocalSettings.php, it doesn't work, because the extensions are loaded after LS.php is. If I do it in extension function, it doesn't work, because some services are already initiated. Where else can I do this kind of a task?
We need something that runs after extensions are loaded, but before any config is actually used by services.

The MediaWikiServices hook. It can be used from LocalSettings, though that's kind of hacky.

Thanks for summarizing the issue @daniel. Breaking the assumption extension functions can modify config sadly means that there is no longer any way for local site administrators to alter extension provided config.

For config settings that extensions expose via the "config" key, that works properly, right?

Negative. See T275336 as an example of our own config being unable to delete a group introduced via the config key by an extension.

In general, extensions should not override config coming from LocalSettings.php. And I think in general, they don't. Things get tricky if the change you want to make is to unset it. Then the extension would put it back. But I'd expect that to be rare. What are the concrete use cases?

Maybe there's a slight misunderstanding. I'm not talking about extensions modifying LS.php config. I'm talking about LS.php modifying config introduced by extensions, which is pretty common, I would say. For one example, see T275336. Other usecases can be seen in flaggedrevs.php in our own config.

I'm pretty sure there are also third party administrators who do not want certain groups to be introduced to their systems, perhaps because they assign the rights needed via some other means, or to other groups.

If I unset a group in LocalSettings.php, it doesn't work, because the extensions are loaded after LS.php is. If I do it in extension function, it doesn't work, because some services are already initiated. Where else can I do this kind of a task?
We need something that runs after extensions are loaded, but before any config is actually used by services.

The MediaWikiServices hook. It can be used from LocalSettings, though that's kind of hacky.

I agree it's hacky, on the other hand, the extension functions are also a pretty hacky concept. Should we note the hook as the proper way to do the things I mentioned? Or should we instead figure out a better solution?

Change 673159 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[operations/mediawiki-config@master] Define confirmed group in MediaWikiServices hook

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

Change 673159 merged by jenkins-bot:
[operations/mediawiki-config@master] Define confirmed group in MediaWikiServices hook

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

Mentioned in SAL (#wikimedia-operations) [2021-03-17T23:40:18Z] <urbanecm@deploy1002> Synchronized wmf-config/CommonSettings.php: 5c14e7d2045f0905f7e85b249e821bbe8d69c600: Define confirmed group in MediaWikiServices hook (T275334, T277704, T275310, T275333) (duration: 01m 08s)

Legoktm added a subscriber: Legoktm.

Relying on hooks and extension functions to implement *configuration* is always going to go badly. I suggest we add:

  • $wgGroupInheritsPermissions = ['confirmed' => 'autoconfirmed'] - instructs that "confirmed" inherits all the rights that "autoconfirmed" has, in addition to any rights set explicitly (do we need multi-inheritance here?)
  • $wgDisabledGroups = ['whatever' => true] - entirely disables a user group from MediaWiki, even if an extension is the one creating it.

Both directives would be evaluated at runtime in PermissionsManager (or elsewhere) so no hook hackery is needed.

Relying on hooks and extension functions to implement *configuration* is always going to go badly. I suggest we add:

  • $wgGroupInheritsPermissions = ['confirmed' => 'autoconfirmed'] - instructs that "confirmed" inherits all the rights that "autoconfirmed" has, in addition to any rights set explicitly (do we need multi-inheritance here?)
  • $wgDisabledGroups = ['whatever' => true] - entirely disables a user group from MediaWiki, even if an extension is the one creating it.

Both directives would be evaluated at runtime in PermissionsManager (or elsewhere) so no hook hackery is needed.

That could work for the common use case of manipulating groups. But it seems like we will need a more generic approach for programmatically manipulating settings.

Hey there, should this be moved to 1.37? The cut for 1.36 has happened, and 1.36.0-rc.0 will be cut in a fortnight or so, after which feature changes shouldn't be landed and back-ported.

Hey there, should this be moved to 1.37? The cut for 1.36 has happened, and 1.36.0-rc.0 will be cut in a fortnight or so, after which feature changes shouldn't be landed and back-ported.

Hello James, let me re-state what will happen if the current code is released: it will break any third-party installation that relies on extension functions to change permissions (at least, possibly other config too). I personally feel that's a pretty common usage that we shouldn't break, but it's not really my call.

T273317: some users with access are unable to configure pending changes is fixed (for the extension; not for Wikimedia yet, but that does not block the release), T275335: suppressor group exists sometimes at Wikimedia wikis and T275336: push-subscription-manager group is sometimes available at all wikis are caused by Wikimedia configuration, not extension code. So I think there is no release blocker here, other than communicating that ExtensionFunctions should not be used for config changes. (That probably never worked fully reliably; FlaggedRevs has had issues for two years. Nevertheless, might be worth a release note.)

The task description also says "perhaps throw an exception to make it clear", but I don't see how that would be feasible.

Change 682560 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@REL1_36] Warn about config changes done in ExtensionFunctions

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

Change 682560 merged by jenkins-bot:

[mediawiki/core@REL1_36] Warn about config changes done in ExtensionFunctions

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

Change 682707 had a related patch set uploaded (by Jforrester; author: Gergő Tisza):

[mediawiki/core@master] Warn about config changes done in ExtensionFunctions

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

Change 682707 merged by jenkins-bot:

[mediawiki/core@master] Warn about config changes done in ExtensionFunctions

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

Change 710134 had a related patch set uploaded (by Legoktm; author: Legoktm):

[mediawiki/core@master] Support having groups inherit permissions from another group

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

Change 710136 had a related patch set uploaded (by Legoktm; author: Legoktm):

[mediawiki/core@master] [WIP] Allow disabling user groups

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

I took a stab at implementing what I proposed in T275334#6923618, group inheritance seems to be straightforward, disabling groups a little less so.