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>

diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index ebbab7b21099cdc23cc4250acb6ce26ba10ffb52..5444f0815e3230cb663019e1c2ba855dda0eb51e 100644 (file)
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -344,7 +344,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
                }
 
                $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
@@ -822,6 +822,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
                        $filter .= '@*';
                }
 
+               $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 4ddf9f5247c6051db5dbaf033b586a4b3270a3bc..74dd2b467cf9b0215d5fc9cd6516bf139d49c55d 100644 (file)
--- a/apps/user_ldap/tests/Group_LDAPTest.php
+++ b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -719,23 +719,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;
                        });
@@ -748,10 +761,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',
@@ -766,9 +784,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) {