LDAP: when checking group for matching filter, also take base DN into consideration. Fixes #17516

4 files changed, 96 insertions, 2 deletions

diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php
index a7a90c75832..24695f64fa8 100644
--- a/apps/user_ldap/group_ldap.php
+++ b/apps/user_ldap/group_ldap.php
@@ -382,7 +382,12 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface {
 			if (is_array($groupDNs)) {
 				$groupDNs = $this->access->groupsMatchFilter($groupDNs);
 				foreach ($groupDNs as $dn) {
-					$groups[] = $this->access->dn2groupname($dn);
+					$groupName = $this->access->dn2groupname($dn);
+					if(is_string($groupName)) {
+						// be sure to never return false if the dn could not be
+						// resolved to a name, for whatever reason.
+						$groups[] = $groupName;
+					}
 				}
 			}
 			if($primaryGroup !== false) {
diff --git a/apps/user_ldap/lib/access.php b/apps/user_ldap/lib/access.php
index b201220d725..44237b52393 100644
--- a/apps/user_ldap/lib/access.php
+++ b/apps/user_ldap/lib/access.php
@@ -365,10 +365,21 @@ class Access extends LDAPUtility implements user\IUserTools {
 				continue;
 			}
 
+			// Check the base DN first. If this is not met already, we don't
+			// need to ask the server at all.
+			if(!$this->isDNPartOfBase($dn, $this->connection->ldapBaseGroups)) {
+				$this->connection->writeToCache($cacheKey, false);
+				continue;
+			}
+
 			$result = $this->readAttribute($dn, 'cn', $this->connection->ldapGroupFilter);
 			if(is_array($result)) {
+				$this->connection->writeToCache($cacheKey, true);
 				$validGroupDNs[] = $dn;
+			} else {
+				$this->connection->writeToCache($cacheKey, false);
 			}
+
 		}
 		return $validGroupDNs;
 	}
diff --git a/apps/user_ldap/tests/integration/lib/IntegrationTestAccessGroupsMatchFilter.php b/apps/user_ldap/tests/integration/lib/IntegrationTestAccessGroupsMatchFilter.php
index 6560153bb63..92035d94b4b 100644
--- a/apps/user_ldap/tests/integration/lib/IntegrationTestAccessGroupsMatchFilter.php
+++ b/apps/user_ldap/tests/integration/lib/IntegrationTestAccessGroupsMatchFilter.php
@@ -43,6 +43,7 @@ class IntegrationTestAccessGroupsMatchFilter {
 	public function init() {
 		require('setup-scripts/createExplicitUsers.php');
 		require('setup-scripts/createExplicitGroups.php');
+		require('setup-scripts/createExplicitGroupsDifferentOU.php');
 
 		$this->initLDAPWrapper();
 		$this->initConnection();
@@ -55,7 +56,7 @@ class IntegrationTestAccessGroupsMatchFilter {
 	 * If a test failed, the script is exited with return code 1.
 	 */
 	public function run() {
-		$cases = ['case1', 'case2'];
+		$cases = ['case1', 'case2', 'case3'];
 
 		foreach ($cases as $case) {
 			print("running $case " . PHP_EOL);
@@ -107,6 +108,30 @@ class IntegrationTestAccessGroupsMatchFilter {
 	}
 
 	/**
+	 * Tests whether a filter for limited groups is effective when more existing
+	 * groups were passed for validation.
+	 *
+	 * @return bool
+	 */
+	private function case3() {
+		$this->connection->setConfiguration(['ldapGroupFilter' => '(objectclass=groupOfNames)']);
+
+		$dns = [
+			'cn=RedGroup,ou=Groups,' . $this->base,
+			'cn=PurpleGroup,ou=Groups,' . $this->base,
+			'cn=SquaredCircleGroup,ou=SpecialGroups,' . $this->base
+		];
+		$result = $this->access->groupsMatchFilter($dns);
+
+		$status =
+			count($result) === 2
+			&& in_array('cn=RedGroup,ou=Groups,' . $this->base, $result)
+			&& in_array('cn=PurpleGroup,ou=Groups,' . $this->base, $result);
+
+		return $status;
+	}
+
+	/**
 	 * initializes the Access test instance
 	 */
 	private function initAccess() {
@@ -129,6 +154,7 @@ class IntegrationTestAccessGroupsMatchFilter {
 			'ldapHost' => $this->server['host'],
 			'ldapPort' => $this->server['port'],
 			'ldapBase' => $this->base,
+			'ldapBaseGroups' => 'ou=Groups,' . $this->base,
 			'ldapAgentName' => $this->server['dn'],
 			'ldapAgentPassword' => $this->server['pwd'],
 			'ldapUserFilter' => 'objectclass=inetOrgPerson',
diff --git a/apps/user_ldap/tests/integration/setup-scripts/createExplicitGroupsDifferentOU.php b/apps/user_ldap/tests/integration/setup-scripts/createExplicitGroupsDifferentOU.php
new file mode 100644
index 00000000000..361881969cc
--- /dev/null
+++ b/apps/user_ldap/tests/integration/setup-scripts/createExplicitGroupsDifferentOU.php
@@ -0,0 +1,52 @@
+<?php
+
+if(php_sapi_name() !== 'cli') {
+	print('Only via CLI, please.');
+	exit(1);
+}
+
+include __DIR__ . '/config.php';
+
+$cr = ldap_connect($host, $port);
+ldap_set_option($cr, LDAP_OPT_PROTOCOL_VERSION, 3);
+$ok = ldap_bind($cr, $adn, $apwd);
+
+if (!$ok) {
+	die(ldap_error($cr));
+}
+
+$ouName = 'SpecialGroups';
+$ouDN = 'ou=' . $ouName . ',' . $bdn;
+
+//creates an OU
+if (true) {
+	$entry = [];
+	$entry['objectclass'][] = 'top';
+	$entry['objectclass'][] = 'organizationalunit';
+	$entry['ou'] = $ouName;
+	$b = ldap_add($cr, $ouDN, $entry);
+	if (!$b) {
+		die(ldap_error($cr));
+	}
+}
+
+$groups = ['SquareGroup', 'CircleGroup', 'TriangleGroup', 'SquaredCircleGroup'];
+// groupOfNames requires groups to have at least one member
+// the member used is created by createExplicitUsers.php script
+$omniMember = 'uid=alice,ou=Users,' . $bdn;
+
+foreach ($groups as $cn) {
+	$newDN = 'cn=' . $cn . ',' . $ouDN;
+
+	$entry = [];
+	$entry['cn'] = $cn;
+	$entry['objectclass'][] = 'groupOfNames';
+	$entry['member'][] = $omniMember;
+
+	$ok = ldap_add($cr, $newDN, $entry);
+	if ($ok) {
+		echo('created group ' . ': ' . $entry['cn'] . PHP_EOL);
+	} else {
+		die(ldap_error($cr));
+	}
+}