Page MenuHomePhabricator

Need for re-caching even in cases where user groups are not accessed!?
Closed, ResolvedPublic

Description

Starting with MediaWiki 1.27.0 there seems to be a requirement for re-caching even in cases where user groups are not accessed. This caused a breakage for the Lockdown extension as reported with T137051. See the comments by Daniel Kinzler on patchset 1 of gerrit 302385 which is trying to mitigate the issue by "forceing the effective user groups to be re-cached also for the early-exit cases of the hook handler" for the Lockdown extension.

Event Timeline

Issue confirmed on latest master (1.28dev): With Lockdown enabled, users seem to be be in any group. No groups are shown on the Preferences page, and permissions assigned by group are not effective.

Suspicion: getEffectiveGroups() is called early, at a stage where no user groups are known, causing an empty set of groups to be cached in the user object.

Cause: User::loadGroups() is called while $this->mId is null. Consequently, no matches are found in the user_groups table, and $this->mGroups is set to an empty array.

Trigger: Lockdown implements the SearchableNamespaces hook, which is called (indirectly) from User::getDefaultObject. Lockdown's hook handler calls getEffectiveGroup on the global $wgUser, which is currently in the process of being initialized (User::load() is on the stack, $wgUser->mItemsLoaded is true, but $wgUser->mId is still null).

Remedy: User::getGroups()/loadGroups(), getAutomaticGroups(), and getEffectiveGroups() must not cache results if $this->mId is null. Since we know that in such a case the result returned by these methods is incorrect, a warning should be issued.

This remedy fixes the error hat hand, but is bound to have other issues. After all, some calls to getEffectiveGroups will get wrong results, and later calls to that method will give different results.

The underlying problem is of conceptual nature: Lockdown adjusts which namespaces are callable on a per-user basis. This makes sense when rendering the search page, but it does not when loading default options. Further investigation is needed to find an alternative approach to adjusting the namespaces presented on Special:Search.


Issues found in Lockdown while investigating:

The $arr parameter to lockdownSearchableNamespaces must be a reference parameter, otherwise this hook has no effect whatsoever. Since this is also the function that triggers the problematic behavior, one remedy would be to simply remove it, and live with inaccessible namespaces being listed as searchable. Due to this bug, this would not change the behavior.

Also, Lockdown should not call getEffectiveGroups() with the recache flag set. Recaching user groups is expensive, and some of the hooks can be called hundreds of times while a page is rendering. Recaching should only be necessary when manipulating a user's group membership, which Lockdown does not do.

Change 303358 had a related patch set uploaded (by Daniel Kinzler):
Avoid caching a bad set of user groups.

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

Change 303359 had a related patch set uploaded (by Daniel Kinzler):
Don't use SearchEngineConfig::searchableNamespaces in User::getDefaultOptions.

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

Proposed resolution:

For 1.28, fix caching of user groups and construction of default options in core, see https://gerrit.wikimedia.org/r/303358 and https://gerrit.wikimedia.org/r/303359

For 1.27, use the workaround provided Kghbln for Lockdown, see https://gerrit.wikimedia.org/r/#/c/303360/1.

For further development of Lockdown, fix the SearchableNamepaces hook and avoid re-caching user groups, see https://gerrit.wikimedia.org/r/#/c/303363/ and https://gerrit.wikimedia.org/r/#/c/303358/1

Change 303369 had a related patch set uploaded (by Daniel Kinzler):
Don't force loading of groups and options on User::loadFromUserObject

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

Change 303369 merged by jenkins-bot:
Don't force loading of groups and options on User::loadFromUserObject

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

@daniel Thank you for tackling this issue. This is indeed very much appreciated!!

Change 303359 merged by jenkins-bot:
Don't use SearchEngineConfig::searchableNamespaces in User::getDefaultOptions.

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

Change 325566 had a related patch set uploaded (by Aaron Schulz):
Don't force loading of groups and options on User::loadFromUserObject

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

Change 325566 had a related patch set uploaded (by Aaron Schulz):
Don't force loading of groups and options on User::loadFromUserObject

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

Because of T148582. Thanks a lot for the backport!!

Change 325566 merged by jenkins-bot:
Don't force loading of groups and options on User::loadFromUserObject

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

I'm having some weird behaviour and someone reffered me here as this issue might be the cause.
I'm using Auth_remoteuser ( https://www.mediawiki.org/wiki/Extension:Auth_remoteuser ) to authenticate my users.
When enabling extention Dynamic Page List (third-party) ( https://www.mediawiki.org/wiki/Extension:DynamicPageList_(third-party) ) I'm getting an error that my user should be inside the "users" group.
Obviously he is.
Is there any information I could provide to you, or any changes I could do to trigger the rights refresh on time (switching the load order of plugins has been tried)?

This task has been assigned to the same task owner for more than two years. Resetting task assignee due to inactivity, to decrease task cookie-licking and to get a slightly more realistic overview of plans. Please feel free to assign this task to yourself again if you still realistically work or plan to work on this task - it would be welcome!

For tips how to manage individual work in Phabricator (noisy notifications, lists of task, etc.), see https://phabricator.wikimedia.org/T228575#6237124 for available options.
(For the records, two emails were sent to assignee addresses before resetting assignees. See T228575 for more info and for potential feedback. Thanks!)

Change 303358 abandoned by Daniel Kinzler:

[mediawiki/core@master] Avoid caching a bad set of user groups.

Reason:

ancient history

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

daniel claimed this task.

I assume this is no longer an issue since the introduction of UserGroupManager. Please re-open as apporpriate.