Page MenuHomePhabricator

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

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

Kghbln created this task.Aug 6 2016, 10:34 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 6 2016, 10:34 AM

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

daniel added a comment.Aug 6 2016, 1:56 PM

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

daniel triaged this task as Normal priority.Aug 6 2016, 1:56 PM

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

Kghbln added a comment.Aug 8 2016, 1:43 PM

@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

Kghbln added a comment.Dec 6 2016, 4:54 PM

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)?