Merge pull request #14464 from nextcloud/fix/noid/ldap-nested-group-filter

resolve user and groups in nested groups first before filtering the results

.drone.yml

@@ -1049,7 +1049,7 @@ services:
       matrix:
         TESTS: acceptance
   openldap:
-    image: nextcloudci/openldap:openldap-6
+    image: nextcloudci/openldap:openldap-7
     environment:
       - SLAPD_DOMAIN=nextcloud.ci
       - SLAPD_ORGANIZATION=Nextcloud

apps/user_ldap/lib/Connection.php

@@ -62,6 +62,9 @@ use OCP\ILogger;
  * @property string ldapEmailAttribute
  * @property string ldapExtStorageHomeAttribute
  * @property string homeFolderNamingRule
+ * @property bool|string ldapNestedGroups
+ * @property string[] ldapBaseGroups
+ * @property string ldapGroupFilter
  */
 class Connection extends LDAPUtility {
 	private $ldapConnectionRes = null;

apps/user_ldap/lib/Group_LDAP.php

@@ -58,6 +58,11 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 */
 	protected $cachedGroupsByMember;
 
+	/**
+	 * @var string[] $cachedNestedGroups array of groups with gid (DN) as key
+	 */
+	protected $cachedNestedGroups;
+
 	/** @var GroupPluginManager */
 	protected $groupPluginManager;
 
@@ -71,6 +76,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 
 		$this->cachedGroupMembers = new CappedMemoryCache();
 		$this->cachedGroupsByMember = new CappedMemoryCache();
+		$this->cachedNestedGroups = new CappedMemoryCache();
 		$this->groupPluginManager = $groupPluginManager;
 	}
 
@@ -212,12 +218,12 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 */
 	private function _groupMembers($dnGroup, &$seen = null) {
 		if ($seen === null) {
-			$seen = array();
+			$seen = [];
 		}
-		$allMembers = array();
+		$allMembers = [];
 		if (array_key_exists($dnGroup, $seen)) {
 			// avoid loops
-			return array();
+			return [];
 		}
 		// used extensively in cron job, caching makes sense for nested groups
 		$cacheKey = '_groupMembers'.$dnGroup;
@@ -226,19 +232,12 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			return $groupMembers;
 		}
 		$seen[$dnGroup] = 1;
-		$members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr,
+		$members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr);
-												$this->access->connection->ldapGroupFilter);
 		if (is_array($members)) {
-			foreach ($members as $member) {
+			$fetcher = function($memberDN, &$seen) {
-				$allMembers[$member] = 1;
+				return $this->_groupMembers($memberDN, $seen);
-				$nestedGroups = $this->access->connection->ldapNestedGroups;
+			};
-				if (!empty($nestedGroups)) {
+			$allMembers = $this->walkNestedGroups($dnGroup, $fetcher, $members);
-					$subMembers = $this->_groupMembers($member, $seen);
-					if ($subMembers) {
-						$allMembers += $subMembers;
-					}
-				}
-			}
 		}
 
 		$allMembers += $this->getDynamicGroupMembers($dnGroup);
@@ -251,30 +250,69 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 * @param string $DN
 	 * @param array|null &$seen
 	 * @return array
+	 * @throws \OC\ServerNotAvailableException
 	 */
-	private function _getGroupDNsFromMemberOf($DN, &$seen = null) {
+	private function _getGroupDNsFromMemberOf($DN) {
-		if ($seen === null) {
-			$seen = array();
-		}
-		if (array_key_exists($DN, $seen)) {
-			// avoid loops
-			return array();
-		}
-		$seen[$DN] = 1;
 		$groups = $this->access->readAttribute($DN, 'memberOf');
 		if (!is_array($groups)) {
-			return array();
+			return [];
 		}
-		$groups = $this->access->groupsMatchFilter($groups);
+
-		$allGroups =  $groups;
+		$fetcher = function($groupDN) {
-		$nestedGroups = $this->access->connection->ldapNestedGroups;
+			if (isset($this->cachedNestedGroups[$groupDN])) {
-		if ((int)$nestedGroups === 1) {
+				$nestedGroups = $this->cachedNestedGroups[$groupDN];
-			foreach ($groups as $group) {
+			} else {
-				$subGroups = $this->_getGroupDNsFromMemberOf($group, $seen);
+				$nestedGroups = $this->access->readAttribute($groupDN, 'memberOf');
-				$allGroups = array_merge($allGroups, $subGroups);
+				if (!is_array($nestedGroups)) {
+					$nestedGroups = [];
+				}
+				$this->cachedNestedGroups[$groupDN] = $nestedGroups;
 			}
+			return $nestedGroups;
+		};
+
+		$groups = $this->walkNestedGroups($DN, $fetcher, $groups);
+		return $this->access->groupsMatchFilter($groups);
+	}
+
+	/**
+	 * @param string $dn
+	 * @param \Closure $fetcher args: string $dn, array $seen, returns: string[] of dns
+	 * @param array $list
+	 * @return array
+	 */
+	private function walkNestedGroups(string $dn, \Closure $fetcher, array $list): array {
+		$nesting = (int) $this->access->connection->ldapNestedGroups;
+		// depending on the input, we either have a list of DNs or a list of LDAP records
+		// also, the output expects either DNs or records. Testing the first element should suffice.
+		$recordMode = is_array($list) && isset($list[0]) && is_array($list[0]) && isset($list[0]['dn'][0]);
+
+		if ($nesting !== 1) {
+			if($recordMode) {
+				// the keys are numeric, but should hold the DN
+				return array_reduce($list, function ($transformed, $record) use ($dn) {
+					if($record['dn'][0] != $dn) {
+						$transformed[$record['dn'][0]] = $record;
+					}
+					return $transformed;
+				}, []);
+			}
+			return $list;
 		}
-		return $allGroups;
+
+		$seen = [];
+		while ($record = array_pop($list)) {
+			$recordDN = $recordMode ? $record['dn'][0] : $record;
+			if ($recordDN === $dn || array_key_exists($recordDN, $seen)) {
+				// Prevent loops
+				continue;
+			}
+			$fetched = $fetcher($record, $seen);
+			$list = array_merge($list, $fetched);
+			$seen[$recordDN] = $record;
+		}
+
+		return $recordMode ? $seen : array_keys($seen);
 	}
 
 	/**
@@ -737,34 +775,28 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 */
 	private function getGroupsByMember($dn, &$seen = null) {
 		if ($seen === null) {
-			$seen = array();
+			$seen = [];
 		}
-		$allGroups = array();
 		if (array_key_exists($dn, $seen)) {
 			// avoid loops
-			return array();
+			return [];
 		}
+		$allGroups = [];
 		$seen[$dn] = true;
-		$filter = $this->access->combineFilterWithAnd(array(
+		$filter = $this->access->connection->ldapGroupMemberAssocAttr.'='.$dn;
-			$this->access->connection->ldapGroupFilter,
-			$this->access->connection->ldapGroupMemberAssocAttr.'='.$dn
-		));
 		$groups = $this->access->fetchListOfGroups($filter,
-			array($this->access->connection->ldapGroupDisplayName, 'dn'));
+			[$this->access->connection->ldapGroupDisplayName, 'dn']);
 		if (is_array($groups)) {
-			foreach ($groups as $groupobj) {
+			$fetcher = function ($dn, &$seen) {
-				$groupDN = $groupobj['dn'][0];
+				if(is_array($dn) && isset($dn['dn'][0])) {
-				$allGroups[$groupDN] = $groupobj;
+					$dn = $dn['dn'][0];
-				$nestedGroups = $this->access->connection->ldapNestedGroups;
-				if (!empty($nestedGroups)) {
-					$supergroups = $this->getGroupsByMember($groupDN, $seen);
-					if (is_array($supergroups) && (count($supergroups)>0)) {
-						$allGroups = array_merge($allGroups, $supergroups);
-					}
 				}
-			}
+				return $this->getGroupsByMember($dn, $seen);
+			};
+			$allGroups = $this->walkNestedGroups($dn, $fetcher, $groups);
 		}
-		return $allGroups;
+		$visibleGroups = $this->access->groupsMatchFilter(array_keys($allGroups));
+		return array_intersect_key($allGroups, array_flip($visibleGroups));
 	}
 
 	/**
@@ -811,7 +843,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 
 		$primaryUsers = $this->getUsersInPrimaryGroup($groupDN, $search, $limit, $offset);
 		$posixGroupUsers = $this->getUsersInGidNumber($groupDN, $search, $limit, $offset);
-		$members = array_keys($this->_groupMembers($groupDN));
+		$members = $this->_groupMembers($groupDN);
 		if(!$members && empty($posixGroupUsers) && empty($primaryUsers)) {
 			//in case users could not be retrieved, return empty result set
 			$this->access->connection->writeToCache($cacheKey, []);
@@ -886,7 +918,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			return false;
 		}
 
-		$members = array_keys($this->_groupMembers($groupDN));
+		$members = $this->_groupMembers($groupDN);
 		$primaryUserCount = $this->countUsersInPrimaryGroup($groupDN, '');
 		if(!$members && $primaryUserCount === 0) {
 			//in case users could not be retrieved, return empty result set

apps/user_ldap/tests/Group_LDAPTest.php

@@ -98,16 +98,27 @@ class Group_LDAPTest extends TestCase {
 	public function testCountEmptySearchString() {
 		$access = $this->getAccessMock();
 		$pluginManager = $this->getPluginManagerMock();
+		$groupDN = 'cn=group,dc=foo,dc=bar';
 
 		$this->enableGroups($access);
 
 		$access->expects($this->any())
 			->method('groupname2dn')
-			->will($this->returnValue('cn=group,dc=foo,dc=bar'));
+			->will($this->returnValue($groupDN));
 
 		$access->expects($this->any())
 			->method('readAttribute')
-			->will($this->returnValue(array('u11', 'u22', 'u33', 'u34')));
+			->willReturnCallback(function($dn) use ($groupDN) {
+				if($dn === $groupDN) {
+					return [
+						'uid=u11,ou=users,dc=foo,dc=bar',
+						'uid=u22,ou=users,dc=foo,dc=bar',
+						'uid=u33,ou=users,dc=foo,dc=bar',
+						'uid=u34,ou=users,dc=foo,dc=bar'
+					];
+				}
+				return [];
+			});
 
 		// for primary groups
 		$access->expects($this->once())
@@ -132,7 +143,7 @@ class Group_LDAPTest extends TestCase {
 
 		$access->expects($this->any())
 			->method('fetchListOfUsers')
-			->will($this->returnValue(array()));
+			->will($this->returnValue([]));
 
 		$access->expects($this->any())
 			->method('readAttribute')
@@ -145,7 +156,7 @@ class Group_LDAPTest extends TestCase {
 				if(strpos($name, 'u') === 0) {
 					return strpos($name, '3');
 				}
-				return array('u11', 'u22', 'u33', 'u34');
+				return ['u11', 'u22', 'u33', 'u34'];
 			}));
 
 		$access->expects($this->any())
@@ -625,7 +636,7 @@ class Group_LDAPTest extends TestCase {
 			->method('dn2groupname')
 			->will($this->returnArgument(0));
 
-		$access->expects($this->exactly(3))
+		$access->expects($this->exactly(1))
 			->method('groupsMatchFilter')
 			->will($this->returnArgument(0));
 
@@ -659,14 +670,15 @@ class Group_LDAPTest extends TestCase {
 		$access->expects($this->once())
 			->method('username2dn')
 			->will($this->returnValue($dn));
-
 		$access->expects($this->never())
 			->method('readAttribute')
 			->with($dn, 'memberOf');
-
 		$access->expects($this->once())
 			->method('nextcloudGroupNames')
 			->will($this->returnValue([]));
+		$access->expects($this->any())
+			->method('groupsMatchFilter')
+			->willReturnArgument(0);
 
 		$groupBackend = new GroupLDAP($access, $pluginManager);
 		$groupBackend->getUserGroups('userX');
@@ -680,12 +692,15 @@ class Group_LDAPTest extends TestCase {
 		$access->connection->expects($this->any())
 			->method('__get')
 			->will($this->returnCallback(function($name) {
-				if($name === 'useMemberOfToDetectMembership') {
+				switch($name) {
-					return 0;
+					case 'useMemberOfToDetectMembership':
-				} else if($name === 'ldapDynamicGroupMemberURL') {
+						return 0;
-					return '';
+					case 'ldapDynamicGroupMemberURL':
-				} else if($name === 'ldapNestedGroups') {
+						return '';
-					return false;
+					case 'ldapNestedGroups':
+						return false;
+					case 'ldapGroupMemberAssocAttr':
+						return 'member';
 				}
 				return 1;
 			}));
@@ -716,10 +731,12 @@ class Group_LDAPTest extends TestCase {
 			->method('nextcloudGroupNames')
 			->with([$group1, $group2])
 			->will($this->returnValue(['group1', 'group2']));
-
 		$access->expects($this->once())
 			->method('fetchListOfGroups')
 			->will($this->returnValue([$group1, $group2]));
+		$access->expects($this->any())
+			->method('groupsMatchFilter')
+			->willReturnArgument(0);
 
 		$groupBackend = new GroupLDAP($access, $pluginManager);
 		$groups = $groupBackend->getUserGroups('userX');
@@ -999,14 +1016,6 @@ class Group_LDAPTest extends TestCase {
 				$groups1,
 				['cn=Birds,' . $base => $groups1]
 			],
-			[ #2 – test uids with nested groups
-				'cn=Birds,' . $base,
-				$expGroups2,
-				[
-					'cn=Birds,' . $base => $groups1,
-					'8427' => $groups2Nested, // simplified - nested groups would work with DNs
-				],
-			],
 		];
 	}
 
@@ -1045,9 +1054,7 @@ class Group_LDAPTest extends TestCase {
 		$ldap = new GroupLDAP($access, $pluginManager);
 		$resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]);
 
-		$expected = array_keys(array_flip($expectedMembers));
+		$this->assertEquals($expectedMembers, $resultingMembers, '', 0.0, 10, true);
-
-		$this->assertEquals($expected, array_keys($resultingMembers), '', 0.0, 10, true);
 	}
 
 }

build/integration/features/bootstrap/LDAPContext.php

@@ -27,6 +27,7 @@ use PHPUnit\Framework\Assert;
 
 class LDAPContext implements Context {
 	use BasicStructure;
+	use CommandLine;
 
 	protected $configID;
 
@@ -37,6 +38,8 @@ class LDAPContext implements Context {
 		if($this->configID === null) {
 			return;
 		}
+		$this->disableLDAPConfiguration(); # via occ in case of big config issues
+		$this->asAn('admin');
 		$this->sendingTo('DELETE', $this->apiUrl . '/' . $this->configID);
 	}
 
@@ -196,4 +199,9 @@ class LDAPContext implements Context {
 		$backend = (string)simplexml_load_string($this->response->getBody())->data[0]->backend;
 		Assert::assertEquals('LDAP', $backend);
 	}
+
+	public function disableLDAPConfiguration() {
+		$configKey = $this->configID . 'ldap_configuration_active';
+		$this->invokingTheCommand('config:app:set user_ldap ' . $configKey . ' --value="0"');
+	}
 }

build/integration/ldap_features/ldap-openldap.feature

@@ -102,3 +102,67 @@ Feature: LDAP
       | ldapHost       | foo.bar  |
       | ldapPort       | 2456     |
     Then Expect ServerException on failed web login as "alice"
+
+  Scenario: Test LDAP group membership with intermediate groups not matching filter
+    Given modify LDAP configuration
+      | ldapBaseGroups                | ou=OtherGroups,dc=nextcloud,dc=ci |
+      | ldapGroupFilter               | (&(cn=Gardeners)(objectclass=groupOfNames)) |
+      | ldapNestedGroups              | 1 |
+      | useMemberOfToDetectMembership | 1 |
+      | ldapUserFilter                | (&(objectclass=inetorgperson)(!(uid=alice))) |
+      | ldapExpertUsernameAttr        | uid |
+      | ldapGroupMemberAssocAttr      | member |
+    And As an "admin"
+    # for population
+    And sending "GET" to "/cloud/groups"
+    And sending "GET" to "/cloud/groups/Gardeners/users"
+    Then the OCS status code should be "200"
+    And the "users" result should match
+      | alice  | 0 |
+      | clara  | 1 |
+      | elisa  | 1 |
+      | gustaf | 1 |
+      | jesper | 1 |
+
+  Scenario: Test LDAP group membership with intermediate groups not matching filter and without memberof
+    Given modify LDAP configuration
+      | ldapBaseGroups                | ou=OtherGroups,dc=nextcloud,dc=ci |
+      | ldapGroupFilter               | (&(cn=Gardeners)(objectclass=groupOfNames)) |
+      | ldapNestedGroups              | 1 |
+      | useMemberOfToDetectMembership | 0 |
+      | ldapUserFilter                | (&(objectclass=inetorgperson)(!(uid=alice))) |
+      | ldapExpertUsernameAttr        | uid |
+      | ldapGroupMemberAssocAttr      | member |
+    And As an "admin"
+    # for population
+    And sending "GET" to "/cloud/groups"
+    And sending "GET" to "/cloud/groups/Gardeners/users"
+    Then the OCS status code should be "200"
+    And the "users" result should match
+      | alice  | 0 |
+      | clara  | 1 |
+      | elisa  | 1 |
+      | gustaf | 1 |
+      | jesper | 1 |
+
+  Scenario: Test LDAP group membership with intermediate groups not matching filter, numeric group ids
+    Given modify LDAP configuration
+      | ldapBaseGroups                | ou=NumericGroups,dc=nextcloud,dc=ci |
+      | ldapGroupFilter               | (&(cn=2000)(objectclass=groupOfNames)) |
+      | ldapNestedGroups              | 1 |
+      | useMemberOfToDetectMembership | 1 |
+      | ldapUserFilter                | (&(objectclass=inetorgperson)(!(uid=alice))) |
+      | ldapExpertUsernameAttr        | uid |
+      | ldapGroupMemberAssocAttr      | member |
+    And As an "admin"
+    # for population
+    And sending "GET" to "/cloud/groups"
+    And sending "GET" to "/cloud/groups/2000/users"
+    Then the OCS status code should be "200"
+    And the "users" result should match
+      | alice  | 0 |
+      | clara  | 1 |
+      | elisa  | 1 |
+      | gustaf | 1 |
+      | jesper | 1 |
+

build/integration/ldap_features/openldap-numerical-id.feature

@@ -29,3 +29,39 @@ Scenario: Test by logging in
   And Logging in using web as "92379"
   And Sending a "GET" to "/remote.php/webdav/welcome.txt" with requesttoken
   Then the HTTP status code should be "200"
+
+Scenario: Test LDAP group retrieval with numeric group ids and nesting
+  # Nesting does not play a role here really
+  Given modify LDAP configuration
+    | ldapBaseGroups                | ou=NumericGroups,dc=nextcloud,dc=ci |
+    | ldapGroupFilter               | (objectclass=groupOfNames) |
+    | ldapNestedGroups              | 1 |
+    | useMemberOfToDetectMembership | 1 |
+  And As an "admin"
+  And sending "GET" to "/cloud/groups"
+  Then the OCS status code should be "200"
+  And the "groups" result should match
+    | 2000 | 1 |
+    | 3000 | 1 |
+    | 3001 | 1 |
+    | 3002 | 1 |
+
+Scenario: Test LDAP group membership with intermediate groups not matching filter, numeric group ids
+  Given modify LDAP configuration
+    | ldapBaseGroups                | ou=NumericGroups,dc=nextcloud,dc=ci |
+    | ldapGroupFilter               | (&(cn=2000)(objectclass=groupOfNames)) |
+    | ldapNestedGroups              | 1 |
+    | useMemberOfToDetectMembership | 1 |
+    | ldapUserFilter                | (&(objectclass=inetorgperson)(!(uid=alice))) |
+    | ldapGroupMemberAssocAttr      | member |
+  And As an "admin"
+  # for population
+  And sending "GET" to "/cloud/groups"
+  And sending "GET" to "/cloud/groups/2000/users"
+  Then the OCS status code should be "200"
+  And the "users" result should match
+    | 92379 | 0 |
+    | 54172 | 1 |
+    | 50194 | 1 |
+    | 59376 | 1 |
+    | 59463 | 1 |