fix nested group retrieval also for 2 other cases

and also consolidate logic in one method

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

6 files changed, 205 insertions, 85 deletions

diff --git a/.drone.yml b/.drone.yml
index e3cfc2d7be5..f821333ee91 100644
--- a/.drone.yml
+++ b/.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
diff --git a/apps/user_ldap/lib/Connection.php b/apps/user_ldap/lib/Connection.php
index ba393dffc12..4335f8e4397 100644
--- a/apps/user_ldap/lib/Connection.php
+++ b/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;
diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index 2e3bc0b4a5c..1658807c0dd 100644
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -218,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;
@@ -232,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,
-												$this->access->connection->ldapGroupFilter);
+		$members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr);
 		if (is_array($members)) {
-			foreach ($members as $member) {
-				$allMembers[$member] = 1;
-				$nestedGroups = $this->access->connection->ldapNestedGroups;
-				if (!empty($nestedGroups)) {
-					$subMembers = $this->_groupMembers($member, $seen);
-					if ($subMembers) {
-						$allMembers += $subMembers;
-					}
-				}
-			}
+			$fetcher = function($memberDN, &$seen) {
+				return $this->_groupMembers($memberDN, $seen);
+			};
+			$allMembers = $this->walkNestedGroups($dnGroup, $fetcher, $members);
 		}
 
 		$allMembers += $this->getDynamicGroupMembers($dnGroup);
@@ -257,40 +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) {
 		$groups = $this->access->readAttribute($DN, 'memberOf');
 		if (!is_array($groups)) {
-			return array();
+			return [];
 		}
-		$nestedGroups = (int) $this->access->connection->ldapNestedGroups;
-		if ($nestedGroups === 1) {
-			$seen = array();
-			while ($group = array_pop($groups)) {
-				if ($group === $DN || array_key_exists($group, $seen)) {
-					// Prevent loops
-					continue;
+
+		$fetcher = function($groupDN) {
+			if (isset($this->cachedNestedGroups[$groupDN])) {
+				$nestedGroups = $this->cachedNestedGroups[$groupDN];
+			} else {
+				$nestedGroups = $this->access->readAttribute($groupDN, 'memberOf');
+				if (!is_array($nestedGroups)) {
+					$nestedGroups = [];
 				}
-				$seen[$group] = 1;
+				$this->cachedNestedGroups[$groupDN] = $nestedGroups;
+			}
+			return $nestedGroups;
+		};
 
-				// Resolve nested groups
-				if (isset($cachedNestedGroups[$group])) {
-					$nestedGroups = $cachedNestedGroups[$group];
-				} else {
-					$nestedGroups = $this->access->readAttribute($group, 'memberOf');
-					if (!is_array($nestedGroups)) {
-						$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;
 					}
-					$cachedNestedGroups[$group] = $nestedGroups;
-				}
-				foreach ($nestedGroups as $nestedGroup) {
-					array_push($groups, $nestedGroup);
-				}
+					return $transformed;
+				}, []);
 			}
-			// Get unique group DN's from those we have visited in the loop
-			$groups = array_keys($seen);
+			return $list;
 		}
-		return $this->access->groupsMatchFilter($groups);
+
+		$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);
 	}
 
 	/**
@@ -753,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(
-			$this->access->connection->ldapGroupFilter,
-			$this->access->connection->ldapGroupMemberAssocAttr.'='.$dn
-		));
+		$filter = $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) {
-				$groupDN = $groupobj['dn'][0];
-				$allGroups[$groupDN] = $groupobj;
-				$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);
-					}
+			$fetcher = function ($dn, &$seen) {
+				if(is_array($dn) && isset($dn['dn'][0])) {
+					$dn = $dn['dn'][0];
 				}
-			}
+				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));
 	}
 
 	/**
@@ -827,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, []);
@@ -902,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
diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php
index 0c5a06144a0..870dddf1bd8 100644
--- a/apps/user_ldap/tests/Group_LDAPTest.php
+++ b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -39,6 +39,7 @@ use OCA\User_LDAP\Connection;
 use OCA\User_LDAP\Group_LDAP as GroupLDAP;
 use OCA\User_LDAP\ILDAPWrapper;
 use OCA\User_LDAP\User\Manager;
+use function SebastianBergmann\GlobalState\functions;
 use Test\TestCase;
 
 /**
@@ -98,16 +99,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 +144,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 +157,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())
@@ -659,14 +671,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 +693,15 @@ class Group_LDAPTest extends TestCase {
 		$access->connection->expects($this->any())
 			->method('__get')
 			->will($this->returnCallback(function($name) {
-				if($name === 'useMemberOfToDetectMembership') {
-					return 0;
-				} else if($name === 'ldapDynamicGroupMemberURL') {
-					return '';
-				} else if($name === 'ldapNestedGroups') {
-					return false;
+				switch($name) {
+					case 'useMemberOfToDetectMembership':
+						return 0;
+					case 'ldapDynamicGroupMemberURL':
+						return '';
+					case 'ldapNestedGroups':
+						return false;
+					case 'ldapGroupMemberAssocAttr':
+						return 'member';
 				}
 				return 1;
 			}));
@@ -716,10 +732,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 +1017,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 +1055,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($expected, array_keys($resultingMembers), '', 0.0, 10, true);
+		$this->assertEquals($expectedMembers, $resultingMembers, '', 0.0, 10, true);
 	}
 
 }
diff --git a/build/integration/ldap_features/ldap-openldap.feature b/build/integration/ldap_features/ldap-openldap.feature
index 4b0b02c5b4f..2e1f637a50a 100644
--- a/build/integration/ldap_features/ldap-openldap.feature
+++ b/build/integration/ldap_features/ldap-openldap.feature
@@ -102,3 +102,61 @@ 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))) |
+    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))) |
+    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))) |
+    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 |
+
diff --git a/build/integration/ldap_features/openldap-numerical-id.feature b/build/integration/ldap_features/openldap-numerical-id.feature
index 2d87ba33e6e..4959c7328e6 100644
--- a/build/integration/ldap_features/openldap-numerical-id.feature
+++ b/build/integration/ldap_features/openldap-numerical-id.feature
@@ -29,3 +29,38 @@ 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))) |
+  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 |