[LDAP] improve processing nested groups

- split walkNestedGroups into two distinct methods, for $recordMode determiniation was always wrong in subsequent runs. This also simplifies the code. - add runtime caching to avoid duplicated LDAP requests. Later on, members are cached on Redis. Signed-off-by: Arthur Schiwon <[email protected]>
nextcloud · Jan 11, 2022 · 04bafe7 · 04bafe7
1 parent b23934a
commit 04bafe7
Showing 1 changed file with 45 additions and 34 deletions.
diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
@@ -64,10 +64,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 	protected $groupPluginManager;
 	/** @var LoggerInterface */
 	protected $logger;
-
-	/**
-	 * @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name
-	 */
+	/** @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name */
 	protected $ldapGroupMemberAssocAttr;
 
 	public function __construct(Access $access, GroupPluginManager $groupPluginManager) {
@@ -304,7 +301,7 @@ private function _groupMembers(string $dnGroup, ?array &$seen = null): array {
 			$fetcher = function ($memberDN) use (&$seen) {
 				return $this->_groupMembers($memberDN, $seen);
 			};
-			$allMembers = $this->walkNestedGroups($dnGroup, $fetcher, $members, $seen);
+			$allMembers = $this->walkNestedGroupsReturnDNs($dnGroup, $fetcher, $members, $seen);
 		}
 
 		$allMembers += $this->getDynamicGroupMembers($dnGroup);
@@ -351,29 +348,11 @@ private function _getGroupDNsFromMemberOf(string $dn): array {
 			return $nestedGroups;
 		};
 
-		$groups = $this->walkNestedGroups($dn, $fetcher, $groups);
+		$groups = $this->walkNestedGroupsReturnDNs($dn, $fetcher, $groups);
 		return $this->filterValidGroups($groups);
 	}
 
-	private function walkNestedGroups(string $dn, Closure $fetcher, array $list, array &$seen = []): 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;
-		}
-
+	private function processListFromWalkingNestedGroups(array &$list, array &$seen, string $dn, Closure $fetcher): void {
 		while ($record = array_shift($list)) {
 			$recordDN = $record['dn'][0] ?? $record;
 			if ($recordDN === $dn || array_key_exists($recordDN, $seen)) {
@@ -386,9 +365,35 @@ private function walkNestedGroups(string $dn, Closure $fetcher, array $list, arr
 				$seen[$recordDN] = $record;
 			}
 		}
+	}
+
+	private function walkNestedGroupsReturnDNs(string $dn, Closure $fetcher, array $list, array &$seen = []): array {
+		$nesting = (int)$this->access->connection->ldapNestedGroups;
+
+		if ($nesting !== 1) {
+			return $list;
+		}
+
+		$this->processListFromWalkingNestedGroups($list, $seen, $dn, $fetcher);
+		return array_keys($seen);
+	}
+
+	private function walkNestedGroupsReturnRecords(string $dn, Closure $fetcher, array $list, array &$seen = []): array {
+		$nesting = (int)$this->access->connection->ldapNestedGroups;
+
+		if ($nesting !== 1) {
+			// the keys are numeric, but we need them to hold the DN
+			return array_reduce($list, function ($transformed, $record) use ($dn) {
+				if ($record['dn'][0] != $dn) {
+					$transformed[$record['dn'][0]] = $record;
+				}
+				return $transformed;
+			}, []);
+		}
 
-		// on record mode, filter out intermediate state
-		return $recordMode ? array_filter($seen, 'is_array') : array_keys($seen);
+		$this->processListFromWalkingNestedGroups($list, $seen, $dn, $fetcher);
+		// filter out intermediate state
+		return array_filter($seen, 'is_array');
 	}
 
 	/**
@@ -847,6 +852,9 @@ private function getGroupsByMember(string $dn, array &$seen = null): array {
 			// avoid loops
 			return [];
 		}
+		if ($this->cachedGroupsByMember[$dn]) {
+			return $this->cachedGroupsByMember[$dn];
+		}
 		$allGroups = [];
 		$seen[$dn] = true;
 		$filter = $this->access->connection->ldapGroupMemberAssocAttr . '=' . $dn;
@@ -871,14 +879,12 @@ private function getGroupsByMember(string $dn, array &$seen = null): array {
 				return $this->getGroupsByMember($dn, $seen);
 			};
 
-			if (empty($dn)) {
-				$dn = "";
-			}
-
-			$allGroups = $this->walkNestedGroups($dn, $fetcher, $groups, $seen);
+			$allGroups = $this->walkNestedGroupsReturnRecords($dn, $fetcher, $groups, $seen);
 		}
 		$visibleGroups = $this->filterValidGroups($allGroups);
-		return array_intersect_key($allGroups, $visibleGroups);
+		$effectiveGroups = array_intersect_key($allGroups, $visibleGroups);
+		$this->cachedGroupsByMember[$dn] = $effectiveGroups;
+		return $effectiveGroups;
 	}
 
 	/**
@@ -1184,7 +1190,12 @@ protected function filterValidGroups(array $listOfGroups): array {
 		$validGroupDNs = [];
 		foreach ($listOfGroups as $key => $item) {
 			$dn = is_string($item) ? $item : $item['dn'][0];
-			$gid = $this->access->dn2groupname($dn);
+			if (is_array($item) && !isset($item[$this->access->connection->ldapGroupDisplayName][0])) {
+				// group details were fetched, but a name attribute was not set: do not bother to re-fetch
+				continue;
+			}
+			$name = $item[$this->access->connection->ldapGroupDisplayName][0] ?? null;
+			$gid = $this->access->dn2groupname($dn, $name);
 			if (!$gid) {
 				continue;
 			}