Topic on Extension talk:LDAP Authentication

Hey everyone, most likely I'm doing something wrong here but I wanted to start a discussion anyways.

I'm trying to synchronize groups from AD as described here:

http://www.mediawiki.org/wiki/Extension:LDAP_Authentication/Options#Synchronizing_LDAP_groups_with_MediaWiki_security_groups

However in the debug log, I see that a user will be removed from groups if they are no longer a the synchronization get's to the hasLDAPGroup but returns false for every group.

The function is as follows:

/**
* Returns true if this group is in the list of the currently authenticated
* user's groups, else false.
*
* @param string $group
* @return bool
* @access private
*/
function hasLDAPGroup( $group ) {
	$this->printDebug( "Entering hasLDAPGroup", NONSENSITIVE );
	return in_array( strtolower( $group ), $this->userLDAPGroups["short"] );
}

However after searching the LDAPAuthentication.php file for $this->userLDAPGroups["short"], it seems that it never gets set. It should (from my understanding) be set in the getGroups function.

(I'm using memberOf)

towards the end of the function:

$groups = array( "dn" => array(), "short" => array() );
foreach ( $memberOfMembers as $mem ) {
	array_push( $groups["dn"], strtolower( $mem ) );
}
$this->userLDAPGroups = $groups;

It seems like there should be some logic to extract the "short" name. Perhaps searching the dn for the cn entry and pushing that into "short".

Thanks

There is a possibility I never tested memberOf with group synchronization. It does indeed look like I'm not setting a short name there. Try this:

$groups = array( "dn" => array(), "short" => array() );
foreach ( $memberOfMembers as $mem ) {
	array_push( $groups["dn"], strtolower( $mem ) );
	if ( isset( $wgLDAPGroupNameAttribute[$_SESSION['wsDomain']] ) ) {
		$attr = $wgLDAPGroupNameAttribute[$_SESSION['wsDomain']];
	} else {
		$this->printDebug("wgLDAPGroupNameAttribute is not set when trying to do memberOf search; this can be ignored if you aren't doing group synchronization.", NONSENSITIVE);
		$attr = "";
	}
	preg_match('/^' . $attr . '=(\w),.*/i', $mem, $matches);
	if ( isset( $matches[0] ) ) {
		array_push( $groups["short"], strtolower( $matches[0] ) );
	}
}
$this->userLDAPGroups = $groups;

Of course, you'll also need to set $wgLDAPGroupNameAttribute in your settings. If you are using AD, this will be "cn".

I haven't actually tested this code, as I have no quick way to test this right now. Let me know if it works, and I'll commit it.

Thanks Ryan,

For my purposes I hacked it quickly, just wanted to let you know so you could come up with a more appropriate method (for example allowing the user to set the name attribute). :P

The code you posted works fine for groups without spaces given the (\w). I'm not very good with regular expressions so I replaced it the regex (in my case the group happens to be two words).

$groups = array( "dn" => array(), "short" => array() );
foreach ( $memberOfMembers as $mem ) {
	array_push( $groups["dn"], strtolower( $mem ) );
	if ( isset( $wgLDAPGroupNameAttribute[$_SESSION['wsDomain']] ) ) {
		$attr = $wgLDAPGroupNameAttribute[$_SESSION['wsDomain']];
	} else {
		$this->printDebug("wgLDAPGroupNameAttribute is not set when trying to do memberOf search; this can be ignored if you aren't doing group synchronization.", NONSENSITIVE);
		$attr = "";
	}
	// simple hackery
	$memAttrs = explode(',', strtolower($mem));
	if (isset( $memAttrs[0] ))
		$memAttrs = explode('=', $memAttrs[0]);
	if ( isset( $memAttrs[0] ) && $memAttrs[0] == strtolower($attr) ) {
		array_push( $groups["short"], strtolower( $memAttrs[1] ) );
	}
}
$this->userLDAPGroups = $groups;

You'll also need a global statement for $wgLDAPGroupNameAttribute

Ah, you have a better solution. In this solution, the wiki admin doesn't need to define an attribute at all, and I won't need to mess around with regular expressions (which I hate to have in my code). I changed your solution slightly. Can you try the following code block for me?

                                        $groups = array( "dn"=> array(), "short"=>array() );

                                        foreach( $memberOfMembers as $mem ) {
                                                array_push( $groups["dn"], strtolower( $mem ) );

                                                // Get short name of group
                                                $memAttrs = explode( ',', strtolower( $mem ) );
                                                if ( isset( $memAttrs[0] ) ) {
                                                        $memAttrs = explode( '=', $memAttrs[0] );
                                                        if ( isset( $memAttrs[0] ) ) {
                                                                array_push( $groups["short"], strtolower( $memAttrs[1] ) );
                                                        }
                                                }
                                        }

                                        $this->userLDAPGroups = $groups;

Ah yes, if the user is utilizing memberOf then they are using AD which uses CN as the first attribute so this method should work fine.

Tested and works. :)

Thanks Ryan

Even better, the first attribute can be anything; with this, it is simply ignored. The first attribute is always the naming attribute, and should always be the one used in the shortname, so this solution is generic. Best of all, it doesn't require any extra configuration to work properly.

Is this patch from Teh drizzle, or Teddy? I'd like to know who to credit in the changelog.

Thanks for testing, BTW!

You're right!

And 'Teddy Reed' is fine if you'd like. Thanks :P