From 85d5ac5c91933e0697d506e521c38d77cb800571 Mon Sep 17 00:00:00 2001
From: Arthur Schiwon <blizzz@arthur-schiwon.de>
Date: Mon, 19 Oct 2020 13:17:06 +0200
Subject: [PATCH] when nesting is not enabled, the group filter can be applied
 right away

- helps performance, but skipping unnecessary entries
- reduces reoccuring info-level log output against groups that do not
  qualify ("no or empty name")

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
---
 apps/user_ldap/lib/Group_LDAP.php       |  8 ++++-
 apps/user_ldap/tests/Group_LDAPTest.php | 42 +++++++++++++++++++++----
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index 54a8eaf08a..233077c99f 100644
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -343,7 +343,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 		}
 
 		$seen = [];
-		while ($record = array_pop($list)) {
+		while ($record = array_shift($list)) {
 			$recordDN = $recordMode ? $record['dn'][0] : $record;
 			if ($recordDN === $dn || array_key_exists($recordDN, $seen)) {
 				// Prevent loops
@@ -841,6 +841,12 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 		$allGroups = [];
 		$seen[$dn] = true;
 		$filter = $this->access->connection->ldapGroupMemberAssocAttr . '=' . $dn;
+
+		$nesting = (int)$this->access->connection->ldapNestedGroups;
+		if ($nesting === 0) {
+			$filter = $this->access->combineFilterWithAnd([$filter, $this->access->connection->ldapGroupFilter]);
+		}
+
 		$groups = $this->access->fetchListOfGroups($filter,
 			[strtolower($this->access->connection->ldapGroupMemberAssocAttr), $this->access->connection->ldapGroupDisplayName, 'dn']);
 		if (is_array($groups)) {
diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php
index 60ec90c1ef..a770149c24 100644
--- a/apps/user_ldap/tests/Group_LDAPTest.php
+++ b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -727,23 +727,36 @@ class Group_LDAPTest extends TestCase {
 		$groupBackend->getUserGroups('userX');
 	}
 
-	public function testGetGroupsByMember() {
+	public function nestedGroupsProvider(): array {
+		return [
+			[ true ],
+			[ false ],
+		];
+	}
+
+	/**
+	 * @dataProvider nestedGroupsProvider
+	 */
+	public function testGetGroupsByMember(bool $nestedGroups) {
 		$access = $this->getAccessMock();
 		$pluginManager = $this->getPluginManagerMock();
 
+		$groupFilter = '(&(objectclass=nextcloudGroup)(nextcloudEnabled=TRUE))';
 		$access->connection = $this->createMock(Connection::class);
 		$access->connection->expects($this->any())
 			->method('__get')
-			->willReturnCallback(function ($name) {
+			->willReturnCallback(function ($name) use ($nestedGroups, $groupFilter) {
 				switch ($name) {
 					case 'useMemberOfToDetectMembership':
 						return 0;
 					case 'ldapDynamicGroupMemberURL':
 						return '';
 					case 'ldapNestedGroups':
-						return false;
+						return $nestedGroups;
 					case 'ldapGroupMemberAssocAttr':
 						return 'member';
+					case 'ldapGroupFilter':
+						return $groupFilter;
 				}
 				return 1;
 			});
@@ -756,10 +769,15 @@ class Group_LDAPTest extends TestCase {
 		$access->expects($this->exactly(2))
 			->method('username2dn')
 			->willReturn($dn);
-
 		$access->expects($this->any())
 			->method('readAttribute')
 			->willReturn([]);
+		$access->expects($this->any())
+			->method('combineFilterWithAnd')
+			->willReturnCallback(function (array $filterParts) {
+				// ⚠ returns a pseudo-filter only, not real LDAP Filter syntax
+				return implode('&', $filterParts);
+			});
 
 		$group1 = [
 			'cn' => 'group1',
@@ -774,9 +792,21 @@ class Group_LDAPTest extends TestCase {
 			->method('nextcloudGroupNames')
 			->with([$group1, $group2])
 			->willReturn(['group1', 'group2']);
-		$access->expects($this->once())
+		$access->expects($nestedGroups ? $this->atLeastOnce() : $this->once())
 			->method('fetchListOfGroups')
-			->willReturn([$group1, $group2]);
+			->willReturnCallback(function ($filter, $attr, $limit, $offset) use ($nestedGroups, $groupFilter, $group1, $group2) {
+				static $firstRun = true;
+				if (!$nestedGroups) {
+					// When nested groups are enabled, groups cannot be filtered early as it would
+					// exclude intermediate groups. But we can, and should, when working with flat groups.
+					$this->assertTrue(strpos($filter, $groupFilter) !== false);
+				}
+				if ($firstRun) {
+					$firstRun = false;
+					return [$group1, $group2];
+				}
+				return [];
+			});
 		$access->expects($this->any())
 			->method('dn2groupname')
 			->willReturnCallback(function (string $dn) {