Page MenuHomePhabricator

User group assignment no longer properly detected
Closed, ResolvedPublic

Description

Now that issue T127456 was fixed, i. e. we do no longer get fatals after invoking the extension an new issue emerged, became visible:

On "Special:Preference" in the "User profile" section (mw-prefsection-personal) no group assignment is shown. This corresponds with the observation that only actions which are connected to mere existence of an account work. Actions connected to an additionally assigned user group like sysop actions etc. can no longer be performed. I must note that on "Special:ListUsers" I can still see that there is still a user group assignment existing for the user account, however not functional.

Setup:

  • MediaWiki 1.27.0-rc.0 (57f722a) 23:13, 31 May 2016
  • PHP 5.6.20-0+deb8u1 (apache2handler)
  • MariaDB 10.0.25-MariaDB-1~jessie
  • Lockdown – (0d59ae7) 06:53, 6 May 2016

Event Timeline

Kghbln created this task.Jun 4 2016, 9:48 PM
Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptJun 4 2016, 9:48 PM

Once this is fixed task T137025 may be done for best user experience.

@Bhofmann @HermannSchwaerzler Adding you here since you were directly involved with T127456.

Dieudo added a subscriber: Dieudo.Jun 4 2016, 9:57 PM
Kghbln updated the task description. (Show Details)Jun 4 2016, 9:57 PM
Kghbln updated the task description. (Show Details)
StasR added a subscriber: StasR.Jun 6 2016, 1:21 PM
Wess added a subscriber: Wess.Jun 12 2016, 10:21 PM
daniel claimed this task.Jun 13 2016, 2:23 PM
daniel triaged this task as Normal priority.
daniel added a subscriber: daniel.
Kghbln added a comment.EditedJun 16 2016, 2:08 PM

@daniel Thanks a lot for claiming. Keeping fingers crossed here. A fix should also be backported to REL1_27 to allow for best experience for people depending on the extension distributor. This is actually a separate issue T137025 I once created in premature enthusiasm.

Aloist added a subscriber: Aloist.Jul 7 2016, 3:00 PM

I think I have a fix.

Using the version from git master, file Lockdown.php dated 31-may-2016, I replaced in function lockdownSearchableNamespaces
the line

//if ( $user->getId() === null && $user->getName() === null ) {
with
if ( $user->getId() === null || $user->getName() === null || $user->getName() == '' ) {

I do not claim to understand it, but it resolves the problem that the extension locks out users from editing, who normally have the permission to edit.

Thanks for taking a look at the code!

You are very welcome to use developer access to submit the proposed code changes as a Git branch directly into Gerrit which makes it easier to review them quickly and provide feedback.
If you don't want to set up Git/Gerrit, you can also use the Gerrit Patch Uploader. Thanks again!

I think this is up to Daniel, who is the creator of the extension, and has already claimed the task.

I needed a patch urgently to get Lockdown to work, because I may have upgraded to MW 1.27 a bit to quickly.

As I said, I do not understand why my hack works, and I would not want to submit such code formally. It is intended for those poor souls like me who need a patch to protect parts of their wikis, without blocking everything.

Hello and thank you for the discussion.

I've only added the breaking of the function "lockdownSearchableNamespaces" some lines in the source before if $wgUser is not set.

I would prefer that Daniel will comment this problem first.

For me it would be nice to hear which PHP versions is used (Aklapper and Aloist).

I use php 5.6.23

Javawookie added a subscriber: Javawookie.EditedJul 27 2016, 1:24 PM

found a solution

I'm using MediaWiki 1.27 with Lockdown Extension.

reason of error:
getting the effective groups of user in function
lockdownUserPermissionsErrors() is just in a wrong position.

just put the line $ugroups = $user->getEffectiveGroups( true );
right at the beginning of function lockdownUserPermissionsErrors
after global Definition.

The function should look like this:

function lockdownUserPermissionsErrors( $title, $user, $action, &$result ) {
	global $wgNamespacePermissionLockdown, $wgSpecialPageLockdown, $wgWhitelistRead, $wgLang;

	// Javawookie: 2016/07/27
	// I placed it here 
	$ugroups = $user->getEffectiveGroups( true );
	
	$result = null;

	// don't impose extra restrictions on UI pages
	if ( $title->isCssJsSubpage() ) {
		return true;
	}

	if ( $action == 'read' && $wgWhitelistRead ) {
		// don't impose read restrictions on whitelisted pages
		if ( in_array( $title->getPrefixedText(), $wgWhitelistRead ) ) {
			return true;
		}
	}
	$groups = null;
	$ns = $title->getNamespace();
	if ( NS_SPECIAL == $ns ) {
		foreach ( $wgSpecialPageLockdown as $page => $g ) {
			if ( !$title->isSpecial( $page ) ) continue;
			$groups = $g;
			break;
		}
	}
	else {
		$groups = @$wgNamespacePermissionLockdown[$ns][$action];
		if ( $groups === null ) {
			$groups = @$wgNamespacePermissionLockdown['*'][$action];
		}
		if ( $groups === null ) {
			$groups = @$wgNamespacePermissionLockdown[$ns]['*'];
		}
	}

	if ( $groups === null ) {
		#no restrictions
		return true;
	}

	if ( !$groups ) {
		#no groups allowed

		$result = array(
			'badaccess-group0'
		);

		return false;
	}

	// Javawookie: 2016/07/27
	// I put the followin line at the beginning of the function and comment it here
	// $ugroups = $user->getEffectiveGroups( true );

	$match = array_intersect( $ugroups, $groups );

	if ( $match ) {
		# group is allowed - keep processing
		$result = null;
		return true;
	} else {
		# group is denied - abort
		$groupLinks = array_map( array( 'User', 'makeGroupLinkWiki' ), $groups );

		$result = array(
			'badaccess-groups',
			$wgLang->commaList( $groupLinks ),
			count( $groups )
		);

		return false;
	}
}

Cheers
Javawookie

@Javawookie: Could you propose your change in Gerrit please? See T137051#2447401 - thanks in advance!

I tested both @Aloist's workaround (T137051#2444268) and @Javawookie's (T137051#2498715) on a fresh installation of MediaWiki 1.27.0 (tarball released 2016-06-28) with Lockdown (9a85128d). Both seemed to restore the normal functionality of Lockdown, as well as restore the user group list provided under Special:Preference.

However, with @Aloist's workaround (T137051#2444268), I am experiencing a new issue with Special:UserRights. On my new wiki, there are no other accounts besides the one I created during installation. If I use Special:UserRights to remove myself from the administrator (sysop) group (which I did to test that Lockdown would restrict my access to a namespace), I receive this message: "You removed your own rights. As such, you are no longer able to access this page." I would expect to see this if I'd removed myself from the bureaucrats groups, but not any other group. If I then reload the page (after removing "&success=1" from the URL; sometimes multiple hard refreshes are required), I can adjust my group membership once again. The issue reoccurs if I add myself back to the administrators group.

Similarly, if I edit a page that is edit-restricted to non-administrators while I am in the administrator group, I can do so, but when I save and land back on the modified page, I see the "View source" link, rather than the "Edit" link. If I perform a hard refresh, I can see the "Edit" link again.

These issues do not exist with @Javawookie's solution (T137051#2498715), so it appears to be superior.

Change 302385 had a related patch set uploaded (by Kghbln):
Fix retrieval of the effective user groups

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

Kghbln added a comment.Aug 2 2016, 7:52 AM

To get things rolling I just uploaded a patch suggested by @Javawookie based on the field test of @Jpgill86. I myself will do a field test too. Touching wood that this fixes the issue for good. :)

Kghbln added a comment.EditedAug 2 2016, 8:20 AM

Affirmative, I have just tested (action, namespace and special page lockdown) the patch with MW 1.27.0 (758cd9d) and it appears to be working without side effects. Also the behaviour I described in the issue report is no longer detectable. Once the change is reviewed and merged I will do a backport to REL1_27.

Change 303360 had a related patch set uploaded (by Daniel Kinzler):
Fix retrieval of the effective user groups

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

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

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

daniel added a comment.Aug 6 2016, 2:58 PM

This is apparently caused by core aggressively caching user groups acquired with an invalid user ID (namely null). See my analysis in [[T142295#2530412]].

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:47 PM

@All Summing up the issue: The root cause of this issue here was an issue with MW core as described in T142295

So REL1_27 will get the fix as proposed by @Javawookie while master (MW 1.28 will get another fix to cater for the amendments to MediaWiki itself.

Thanks go to @daniel for looking into this closely and his effort to make things fluffy again.

Osnard added a subscriber: Osnard.Aug 16 2016, 7:40 AM

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

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

Change 303360 merged by jenkins-bot:
Fix retrieval of the effective user groups

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

Kghbln closed this task as Resolved.Aug 17 2016, 1:29 PM

The respective patch set were just merged. REL1_27 got the patch from @Javawookie and master (REL1_28 and beyond) got another patch which actually also caters for changes in core.

Thanks @daniel for dealing with this!

Jpgill86 removed a subscriber: Jpgill86.Aug 18 2016, 3:02 PM

Change 302385 abandoned by Hashar:
Fix retrieval of the effective user groups

Reason:
https://gerrit.wikimedia.org/r/#/c/303360/ got merged so apparently this patch is no more needed based on Kghbln last comment.

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

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