Merge pull request #16736 from owncloud/utilize-member-of

Utilize memberOf to boost loading time on users page (depending on LDAP server config)

5 files changed, 110 insertions, 35 deletions

diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php
index 4c5c01743aa..0395a4a80e3 100644
--- a/apps/user_ldap/group_ldap.php
+++ b/apps/user_ldap/group_ldap.php
@@ -251,7 +251,14 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface {
 	 * @return string|bool
 	 */
 	public function getUserPrimaryGroupIDs($dn) {
-		return $this->getEntryGroupID($dn, 'primaryGroupID');
+		$primaryGroupID = false;
+		if($this->access->connection->hasPrimaryGroups) {
+			$primaryGroupID = $this->getEntryGroupID($dn, 'primaryGroupID');
+			if($primaryGroupID === false) {
+				$this->access->connection->hasPrimaryGroups = false;
+			}
+		}
+		return $primaryGroupID;
 	}
 
 	/**
@@ -362,6 +369,27 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface {
 			return array();
 		}
 
+		$groups = [];
+		$primaryGroup = $this->getUserPrimaryGroup($userDN);
+
+		// if possible, read out membership via memberOf. It's far faster than
+		// performing a search, which still is a fallback later.
+		if(intval($this->access->connection->hasMemberOfFilterSupport) === 1
+			&& intval($this->access->connection->useMemberOfToDetectMembership) === 1
+		) {
+			$groupDNs = $this->access->readAttribute($userDN, 'memberOf');
+			if (is_array($groupDNs)) {
+				foreach ($groupDNs as $dn) {
+					$groups[] = $this->access->dn2groupname($dn);;
+				}
+			}
+			if($primaryGroup !== false) {
+				$groups[] = $primaryGroup;
+			}
+			$this->access->connection->writeToCache($cacheKey, $groups);
+			return $groups;
+		}
+
 		//uniqueMember takes DN, memberuid the uid, so we need to distinguish
 		if((strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'uniquemember')
 			|| (strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'member')
@@ -387,7 +415,6 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface {
 			$this->cachedGroupsByMember[$uid] = $groups;
 		}
 
-		$primaryGroup = $this->getUserPrimaryGroup($userDN);
 		if($primaryGroup !== false) {
 			$groups[] = $primaryGroup;
 		}
diff --git a/apps/user_ldap/lib/configuration.php b/apps/user_ldap/lib/configuration.php
index 373c5b48417..0af819ff66f 100644
--- a/apps/user_ldap/lib/configuration.php
+++ b/apps/user_ldap/lib/configuration.php
@@ -76,6 +76,7 @@ class Configuration {
 		'homeFolderNamingRule' => null,
 		'hasPagedResultSupport' => false,
 		'hasMemberOfFilterSupport' => false,
+		'useMemberOfToDetectMembership' => true,
 		'ldapExpertUsernameAttr' => null,
 		'ldapExpertUUIDUserAttr' => null,
 		'ldapExpertUUIDGroupAttr' => null,
@@ -395,6 +396,7 @@ class Configuration {
 			'ldap_expert_uuid_user_attr'        => '',
 			'ldap_expert_uuid_group_attr'       => '',
 			'has_memberof_filter_support'       => 0,
+			'use_memberof_to_detect_membership' => 1,
 			'last_jpegPhoto_lookup'             => 0,
 			'ldap_nested_groups'                => 0,
 			'ldap_paging_size'                  => 500,
@@ -449,6 +451,7 @@ class Configuration {
 			'ldap_expert_uuid_user_attr'        => 'ldapExpertUUIDUserAttr',
 			'ldap_expert_uuid_group_attr'       => 'ldapExpertUUIDGroupAttr',
 			'has_memberof_filter_support'       => 'hasMemberOfFilterSupport',
+			'use_memberof_to_detect_membership' => 'useMemberOfToDetectMembership',
 			'last_jpegPhoto_lookup'             => 'lastJpegPhotoLookup',
 			'ldap_nested_groups'                => 'ldapNestedGroups',
 			'ldap_paging_size'                  => 'ldapPagingSize',
diff --git a/apps/user_ldap/lib/connection.php b/apps/user_ldap/lib/connection.php
index d6f4bdcde04..0328259d78f 100644
--- a/apps/user_ldap/lib/connection.php
+++ b/apps/user_ldap/lib/connection.php
@@ -53,6 +53,11 @@ class Connection extends LDAPUtility {
 	private $dontDestruct = false;
 	private $hasPagedResultSupport = true;
 
+	/**
+	 * @var bool runtime flag that indicates whether supported primary groups are available
+	 */
+	public $hasPrimaryGroups = true;
+
 	//cache handler
 	protected $cache;
 
diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php
index 49cbe6d72bd..6ca84c8718f 100644
--- a/apps/user_ldap/lib/wizard.php
+++ b/apps/user_ldap/lib/wizard.php
@@ -391,10 +391,10 @@ class Wizard extends LDAPUtility {
 			throw new \Exception('Could not connect to LDAP');
 		}
 
-		$groups = $this->fetchGroups($dbKey, $confKey);
+		$this->fetchGroups($dbKey, $confKey);
 
 		if($testMemberOf) {
-			$this->configuration->hasMemberOfFilterSupport = $this->testMemberOf($groups);
+			$this->configuration->hasMemberOfFilterSupport = $this->testMemberOf();
 			$this->result->markChange();
 			if(!$this->configuration->hasMemberOfFilterSupport) {
 				throw new \Exception('memberOf is not supported by the server');
@@ -405,10 +405,12 @@ class Wizard extends LDAPUtility {
 	}
 
 	/**
-	 * fetches all groups from LDAP
+	 * fetches all groups from LDAP and adds them to the result object
+	 *
 	 * @param string $dbKey
 	 * @param string $confKey
 	 * @return array $groupEntries
+	 * @throws \Exception
 	 */
 	public function fetchGroups($dbKey, $confKey) {
 		$obclasses = array('posixGroup', 'group', 'zimbraDistributionList', 'groupOfNames');
@@ -487,7 +489,7 @@ class Wizard extends LDAPUtility {
 			throw new \Exception('Could not connect to LDAP');
 		}
 
-		$obclasses = array('group', 'posixGroup', '*');
+		$obclasses = array('groupOfNames', 'group', 'posixGroup', '*');
 		$this->determineFeature($obclasses,
 								'objectclass',
 								'ldap_groupfilter_objectclass',
@@ -833,43 +835,22 @@ class Wizard extends LDAPUtility {
 
 	/**
 	 * Checks whether the server supports memberOf in LDAP Filter.
-	 * Requires that groups are determined, thus internally called from within
-	 * determineGroups()
-	 * @param array $groups
+	 * Note: at least in OpenLDAP, availability of memberOf is dependent on
+	 * a configured objectClass. I.e. not necessarily for all available groups
+	 * memberOf does work.
+	 *
 	 * @return bool true if it does, false otherwise
 	 * @throws \Exception
 	 */
-	private function testMemberOf($groups) {
+	private function testMemberOf() {
 		$cr = $this->getConnection();
 		if(!$cr) {
 			throw new \Exception('Could not connect to LDAP');
 		}
-		if(!is_array($this->configuration->ldapBase)
-		   || !isset($this->configuration->ldapBase[0])) {
-			return false;
-		}
-		$base = $this->configuration->ldapBase[0];
-		$filterPrefix = '(&(objectclass=*)(memberOf=';
-		$filterSuffix = '))';
-
-		foreach($groups as $groupProperties) {
-			if(!isset($groupProperties['cn'])) {
-				//assuming only groups have their cn cached :)
-				continue;
-			}
-			$filter = strtolower($filterPrefix . $groupProperties['dn'] . $filterSuffix);
-			$rr = $this->ldap->search($cr, $base, $filter, array('dn'));
-			if(!$this->ldap->isResource($rr)) {
-				continue;
-			}
-			$entries = $this->ldap->countEntries($cr, $rr);
-			//we do not know which groups are empty, so test any and return
-			//success on the first match that returns at least one user
-			if(($entries !== false) && ($entries > 0)) {
-				return true;
-			}
+		$result = $this->access->countUsers('memberOf=*', array('memberOf'), 1);
+		if(is_int($result) &&  $result > 0) {
+			return true;
 		}
-
 		return false;
 	}
 
diff --git a/apps/user_ldap/tests/group_ldap.php b/apps/user_ldap/tests/group_ldap.php
index d91f1503abd..aeb306174f0 100644
--- a/apps/user_ldap/tests/group_ldap.php
+++ b/apps/user_ldap/tests/group_ldap.php
@@ -383,4 +383,63 @@ class Test_Group_Ldap extends \Test\TestCase {
 		$this->assertSame(4, $users);
 	}
 
+	public function testGetUserGroupsMemberOf() {
+		$access = $this->getAccessMock();
+		$this->enableGroups($access);
+
+		$dn = 'cn=userX,dc=foobar';
+
+		$access->connection->hasPrimaryGroups = false;
+
+		$access->expects($this->once())
+			->method('username2dn')
+			->will($this->returnValue($dn));
+
+		$access->expects($this->once())
+			->method('readAttribute')
+			->with($dn, 'memberOf')
+			->will($this->returnValue(['cn=groupA,dc=foobar', 'cn=groupB,dc=foobar']));
+
+		$access->expects($this->exactly(2))
+			->method('dn2groupname')
+			->will($this->returnArgument(0));
+
+		$groupBackend = new GroupLDAP($access);
+		$groups = $groupBackend->getUserGroups('userX');
+
+		$this->assertSame(2, count($groups));
+	}
+
+	public function testGetUserGroupsMemberOfDisabled() {
+		$access = $this->getAccessMock();
+
+		$access->connection->expects($this->any())
+			->method('__get')
+			->will($this->returnCallback(function($name) {
+				if($name === 'useMemberOfToDetectMembership') {
+					return 0;
+				}
+				return 1;
+			}));
+
+		$dn = 'cn=userX,dc=foobar';
+
+		$access->connection->hasPrimaryGroups = false;
+
+		$access->expects($this->once())
+			->method('username2dn')
+			->will($this->returnValue($dn));
+
+		$access->expects($this->never())
+			->method('readAttribute')
+			->with($dn, 'memberOf');
+
+		$access->expects($this->once())
+			->method('ownCloudGroupNames')
+			->will($this->returnValue([]));
+
+		$groupBackend = new GroupLDAP($access);
+		$groupBackend->getUserGroups('userX');
+	}
+
 }