Fix LDAP recursive nested group support

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

2 files changed, 21 insertions, 19 deletions

diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index 70cc7a0107a..d5d715d1b51 100644
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -62,7 +62,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 	protected CappedMemoryCache $cachedGroupsByMember;
 	/** @var CappedMemoryCache<string[]> $cachedNestedGroups array of groups with gid (DN) as key */
 	protected CappedMemoryCache $cachedNestedGroups;
-	protected GroupInterface $groupPluginManager;
+	protected GroupPluginManager $groupPluginManager;
 	protected LoggerInterface $logger;
 
 	/**
@@ -243,8 +243,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 	 * @psalm-param array<string, bool> $seen List of DN that have already been processed.
 	 * @throws ServerNotAvailableException
 	 */
-	private function _groupMembers(string $dnGroup, array &$seen = []): array {
+	private function _groupMembers(string $dnGroup, array $seen = [], bool &$recursive = false): array {
 		if (isset($seen[$dnGroup])) {
+			$recursive = true;
 			return [];
 		}
 		$seen[$dnGroup] = true;
@@ -293,7 +294,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 		if (is_array($members)) {
 			if ((int)$this->access->connection->ldapNestedGroups === 1) {
 				while ($recordDn = array_shift($members)) {
-					$nestedMembers = $this->_groupMembers($recordDn, $seen);
+					$nestedMembers = $this->_groupMembers($recordDn, $seen, $recursive);
 					if (!empty($nestedMembers)) {
 						// Group, queue its members for processing
 						$members = array_merge($members, $nestedMembers);
@@ -317,7 +318,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 			unset($allMembers[$index]);
 		}
 
-		$this->access->connection->writeToCache($cacheKey, $allMembers);
+		if (!$recursive) {
+			$this->access->connection->writeToCache($cacheKey, $allMembers);
+		}
 
 		if (isset($attemptedLdapMatchingRuleInChain)
 			&& $this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_UNKNOWN
diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php
index d9bfe81e63c..edcb4be8819 100644
--- a/apps/user_ldap/tests/Group_LDAPTest.php
+++ b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -1322,35 +1322,32 @@ class Group_LDAPTest extends TestCase {
 
 		return [
 			[ #0 – test DNs
-				'cn=Birds,' . $base,
-				$birdsDn,
+				['cn=Birds,' . $base => $birdsDn],
 				['cn=Birds,' . $base => $birdsDn]
 			],
 			[ #1 – test uids
-				'cn=Birds,' . $base,
-				$birdsUid,
+				['cn=Birds,' . $base => $birdsUid],
 				['cn=Birds,' . $base => $birdsUid]
 			],
 			[ #2 – test simple nested group
-				'cn=Animals,' . $base,
-				array_merge($birdsDn, $animalsDn),
+				['cn=Animals,' . $base => array_merge($birdsDn, $animalsDn)],
 				[
 					'cn=Animals,' . $base => array_merge(['cn=Birds,' . $base], $animalsDn),
 					'cn=Birds,' . $base => $birdsDn,
 				]
 			],
 			[ #3 – test recursive nested group
-				'cn=Animals,' . $base,
-				// Because of complicated nesting the group gets returned as a member
-				array_merge(['cn=Birds,' . $base], $birdsDn, $animalsDn),
+				[
+					'cn=Animals,' . $base => array_merge($birdsDn, $animalsDn),
+					'cn=Birds,' . $base => array_merge($birdsDn, $animalsDn),
+				],
 				[
 					'cn=Animals,' . $base => array_merge(['cn=Birds,' . $base,'cn=Birds,' . $base,'cn=Animals,' . $base], $animalsDn),
 					'cn=Birds,' . $base => array_merge(['cn=Animals,' . $base,'cn=Birds,' . $base], $birdsDn),
 				]
 			],
 			[ #4 – Complicated nested group
-				'cn=Things,' . $base,
-				array_merge($birdsDn, $animalsDn, $thingsDn, $plantsDn),
+				['cn=Things,' . $base => array_merge($birdsDn, $animalsDn, $thingsDn, $plantsDn)],
 				[
 					'cn=Animals,' . $base => array_merge(['cn=Birds,' . $base], $animalsDn),
 					'cn=Birds,' . $base => $birdsDn,
@@ -1365,11 +1362,11 @@ class Group_LDAPTest extends TestCase {
 	 * @param string[] $expectedMembers
 	 * @dataProvider groupMemberProvider
 	 */
-	public function testGroupMembers(string $groupDN, array $expectedMembers, $groupsInfo = null) {
+	public function testGroupMembers(array $expectedResult, array $groupsInfo = null) {
 		$access = $this->getAccessMock();
 		$access->expects($this->any())
 			->method('readAttribute')
-			->willReturnCallback(function ($group) use ($groupDN, $expectedMembers, $groupsInfo) {
+			->willReturnCallback(function ($group) use ($groupsInfo) {
 				if (isset($groupsInfo[$group])) {
 					return $groupsInfo[$group];
 				}
@@ -1392,9 +1389,11 @@ class Group_LDAPTest extends TestCase {
 		$pluginManager = $this->createMock(GroupPluginManager::class);
 
 		$ldap = new GroupLDAP($access, $pluginManager);
-		$resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]);
+		foreach ($expectedResult as $groupDN => $expectedMembers) {
+			$resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]);
 
-		$this->assertEqualsCanonicalizing($expectedMembers, $resultingMembers);
+			$this->assertEqualsCanonicalizing($expectedMembers, $resultingMembers);
+		}
 	}
 
 	public function displayNameProvider() {