horde · ralflang · Sep 14, 2026 · Sep 7, 2026
diff --git a/src/Factory/GroupServiceFactory.php b/src/Factory/GroupServiceFactory.php
@@ -63,10 +63,11 @@ public function create(Injector $injector): GroupService
 
         return match ($driver) {
             'sql' => $this->createSqlBackend($injector, $params),
+            'ldap' => $injector->getInstance(LdapGroupServiceFactory::class)->create($injector),
             default => throw new RuntimeException(
                 "Unsupported group driver: {$driver}. "
-                . "Modern GroupService currently supports 'sql' only. "
-                . "LDAP, File, and other legacy drivers are not yet ported."
+                . "Modern GroupService currently supports 'sql' and 'ldap'. "
+                . "File and other legacy drivers are not yet ported."
             ),
         };
     }

diff --git a/src/Factory/LdapGroupServiceFactory.php b/src/Factory/LdapGroupServiceFactory.php
@@ -17,7 +17,6 @@
 namespace Horde\Core\Factory;
 
 use Horde\Core\Service\LdapGroupService;
-use Horde\Core\Service\HordeLdapService;
 use Horde\Core\Config\ConfigLoader;
 use Horde\Injector\Injector;
 use RuntimeException;
@@ -44,11 +43,14 @@ public function create(Injector $injector): LdapGroupService
         $loader = $injector->getInstance(ConfigLoader::class);
         $config = $loader->load('horde');
 
-        // Get LDAP service (may use service-specific connection)
-        $ldapService = $injector->getInstance(HordeLdapService::class);
+        // Resolve the LDAP connection for the 'groups' service specifically:
+        // falls back to the default 'ldap' config if 'ldap.service.groups'
+        // isn't set (see HordeLdapServiceFactory::resolveConfig()).
+        $ldapFactory = $injector->getInstance(HordeLdapServiceFactory::class);
+        $ldapService = $ldapFactory->create($injector, 'horde:groups');
 
-        // Get group configuration
-        $params = $config->get('groups.params', []);
+        // Get group configuration (legacy conf.php key is singular: 'group', not 'groups')
+        $params = $config->get('group.params', []);
 
         if (empty($params['basedn'])) {
             throw new RuntimeException('LDAP groups require basedn configuration');
@@ -59,7 +61,7 @@ public function create(Injector $injector): LdapGroupService
             basedn: $params['basedn'],
             gidAttr: $params['gid'] ?? 'cn',
             memberAttr: $params['memberuid'] ?? 'memberUid',
-            objectClass: $params['objectclass'] ?? ['posixGroup'],
+            search: $params['search'] ?? ['objectclass' => ['posixGroup']],
             newGroupObjectClass: $params['newgroup_objectclass'] ?? ['posixGroup']
         );
     }

diff --git a/src/Service/LdapGroupService.php b/src/Service/LdapGroupService.php
@@ -43,15 +43,18 @@ class LdapGroupService implements GroupService
      * @param string $basedn Base DN for group searches
      * @param string $gidAttr Attribute for group ID (default: 'cn')
      * @param string $memberAttr Attribute for member list (default: 'memberUid')
-     * @param array $objectClass Object classes for searching (default: ['posixGroup'])
+     * @param array $search Search filter config, same shape as legacy
+     *                       Horde_Group_Ldap's 'search' param: either
+     *                       ['objectclass' => 'name'|['name',...]] or
+     *                       ['filter' => '(raw ldap filter)']
      * @param array $newGroupObjectClass Object classes for new groups (default: ['posixGroup'])
      */
     public function __construct(
         private HordeLdapService $ldapService,
         private string $basedn,
         private string $gidAttr = 'cn',
         private string $memberAttr = 'memberUid',
-        private array $objectClass = ['posixGroup'],
+        private array $search = ['objectclass' => ['posixGroup']],
         private array $newGroupObjectClass = ['posixGroup']
     ) {}
 
@@ -125,12 +128,17 @@ public function get(string $id): GroupInfo
             $ldap = $this->ldapService->getAdapter();
             $dn = $this->buildDN($id);
 
-            $entry = $ldap->getEntry($dn, [
-                'attributes' => [$this->gidAttr, $this->memberAttr, 'mail', 'description'],
-            ]);
+            $entry = $ldap->getEntry($dn, [$this->gidAttr, $this->memberAttr, 'mail', 'description']);
 
-            $members = $entry->getValue($this->memberAttr);
-            $mail = $entry->getValue('mail', 'single');
+            // Horde_Ldap_Entry::getValue() throws when an attribute is
+            // genuinely absent from the entry (not just empty) - mail and
+            // group membership are both optional on a given LDAP entry.
+            // Legacy Horde_Group_Ldap guards every read with exists() for
+            // the same reason.
+            $members = $entry->exists($this->memberAttr)
+                ? $entry->getValue($this->memberAttr, 'all')
+                : [];
+            $mail = $entry->exists('mail') ? $entry->getValue('mail', 'single') : null;
 
             $extra = [];
             if ($mail) {
@@ -377,16 +385,7 @@ public function isReadOnly(): bool
      */
     private function buildFilter(): Horde_Ldap_Filter
     {
-        if (count($this->objectClass) === 1) {
-            return Horde_Ldap_Filter::create('objectClass', 'equals', $this->objectClass[0]);
-        }
-
-        $filters = [];
-        foreach ($this->objectClass as $oc) {
-            $filters[] = Horde_Ldap_Filter::create('objectClass', 'equals', $oc);
-        }
-
-        return Horde_Ldap_Filter::combine('or', $filters);
+        return Horde_Ldap_Filter::build($this->search);
     }
 
     /**
@@ -409,7 +408,7 @@ private function getNextGidNumber(): int
     {
         try {
             $ldap = $this->ldapService->getAdapter();
-            $filter = Horde_Ldap_Filter::create('objectClass', 'equals', 'posixGroup');
+            $filter = $this->buildFilter();
 
             $search = $ldap->search($this->basedn, $filter, [
                 'attributes' => ['gidNumber'],

diff --git a/test/Unit/Factory/GroupServiceFactoryTest.php b/test/Unit/Factory/GroupServiceFactoryTest.php
@@ -0,0 +1,89 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * Copyright 2026 The Horde Project (http://www.horde.org/)
+ *
+ * See the enclosed file LICENSE for license information (LGPL). If you
+ * did not receive this file, see http://www.horde.org/licenses/lgpl21.
+ *
+ * @category Horde
+ * @package  Core
+ * @license  http://www.horde.org/licenses/lgpl21 LGPL 2.1
+ */
+
+namespace Horde\Core\Test\Unit\Factory;
+
+use Horde\Core\Factory\GroupServiceFactory;
+use Horde\Core\Factory\LdapGroupServiceFactory;
+use Horde\Core\Config\ConfigLoader;
+use Horde\Core\Config\State;
+use Horde\Core\Service\LdapGroupService;
+use Horde\Injector\Injector;
+use PHPUnit\Framework\TestCase;
+use PHPUnit\Framework\Attributes\CoversClass;
+use RuntimeException;
+
+/**
+ * Tests for GroupServiceFactory
+ *
+ * @category Horde
+ * @package  Core
+ * @license  http://www.horde.org/licenses/lgpl21 LGPL 2.1
+ */
+#[CoversClass(GroupServiceFactory::class)]
+class GroupServiceFactoryTest extends TestCase
+{
+    /**
+     * @param array<string, mixed> $conf
+     */
+    private function makeInjector(array $conf, ?LdapGroupServiceFactory $ldapGroupServiceFactory = null): Injector
+    {
+        $configLoader = $this->createStub(ConfigLoader::class);
+        $configLoader->method('load')->willReturn(new State($conf));
+
+        $map = [[ConfigLoader::class, $configLoader]];
+        if ($ldapGroupServiceFactory !== null) {
+            $map[] = [LdapGroupServiceFactory::class, $ldapGroupServiceFactory];
+        }
+
+        $injector = $this->createStub(Injector::class);
+        $injector->method('getInstance')->willReturnMap($map);
+
+        return $injector;
+    }
+
+    public function testCreateLdapBackendDelegatesToLdapGroupServiceFactory(): void
+    {
+        $expectedService = $this->createStub(LdapGroupService::class);
+        $ldapGroupServiceFactory = $this->createMock(LdapGroupServiceFactory::class);
+
+        $injector = $this->makeInjector(
+            ['group' => ['driver' => 'ldap', 'params' => ['basedn' => 'ou=group,dc=example,dc=com']]],
+            $ldapGroupServiceFactory
+        );
+
+        $ldapGroupServiceFactory->expects($this->once())
+            ->method('create')
+            ->with($injector)
+            ->willReturn($expectedService);
+
+        $factory = new GroupServiceFactory();
+        $result = $factory->create($injector);
+
+        $this->assertSame($expectedService, $result);
+    }
+
+    public function testUnsupportedDriverThrows(): void
+    {
+        $injector = $this->makeInjector(['group' => ['driver' => 'file']]);
+
+        $factory = new GroupServiceFactory();
+
+        $this->expectException(RuntimeException::class);
+        $this->expectExceptionMessage('Unsupported group driver: file');
+
+        $factory->create($injector);
+    }
+}
diff --git a/test/Unit/Factory/LdapGroupServiceFactoryTest.php b/test/Unit/Factory/LdapGroupServiceFactoryTest.php
@@ -0,0 +1,101 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * Copyright 2026 The Horde Project (http://www.horde.org/)
+ *
+ * See the enclosed file LICENSE for license information (LGPL). If you
+ * did not receive this file, see http://www.horde.org/licenses/lgpl21.
+ *
+ * @category Horde
+ * @package  Core
+ * @license  http://www.horde.org/licenses/lgpl21 LGPL 2.1
+ */
+
+namespace Horde\Core\Test\Unit\Factory;
+
+use Horde\Core\Factory\LdapGroupServiceFactory;
+use Horde\Core\Factory\HordeLdapServiceFactory;
+use Horde\Core\Config\ConfigLoader;
+use Horde\Core\Config\State;
+use Horde\Core\Service\StandardHordeLdapService;
+use Horde\Core\Service\LdapGroupService;
+use Horde\Injector\Injector;
+use PHPUnit\Framework\TestCase;
+use PHPUnit\Framework\Attributes\CoversClass;
+use RuntimeException;
+
+/**
+ * Tests for LdapGroupServiceFactory
+ *
+ * @requires extension ldap
+ * @category Horde
+ * @package  Core
+ * @license  http://www.horde.org/licenses/lgpl21 LGPL 2.1
+ */
+#[CoversClass(LdapGroupServiceFactory::class)]
+class LdapGroupServiceFactoryTest extends TestCase
+{
+    /**
+     * @param array<string, mixed> $conf
+     */
+    private function makeInjector(array $conf, ?HordeLdapServiceFactory $ldapServiceFactory = null): Injector
+    {
+        $configLoader = $this->createStub(ConfigLoader::class);
+        $configLoader->method('load')->willReturn(new State($conf));
+
+        $map = [[ConfigLoader::class, $configLoader]];
+        if ($ldapServiceFactory !== null) {
+            $map[] = [HordeLdapServiceFactory::class, $ldapServiceFactory];
+        }
+
+        $injector = $this->createStub(Injector::class);
+        $injector->method('getInstance')->willReturnMap($map);
+
+        return $injector;
+    }
+
+    public function testUsesGroupsSpecificLdapConnection(): void
+    {
+        $ldapService = $this->createStub(StandardHordeLdapService::class);
+        $ldapServiceFactory = $this->createMock(HordeLdapServiceFactory::class);
+
+        $injector = $this->makeInjector([
+            'group' => ['params' => [
+                'basedn' => 'ou=group,dc=example,dc=com',
+                'gid' => 'cn',
+                'memberuid' => 'memberUid',
+                'search' => ['objectclass' => ['posixGroup']],
+                'newgroup_objectclass' => ['posixGroup', 'hordeGroup'],
+            ]],
+        ], $ldapServiceFactory);
+
+        $ldapServiceFactory->expects($this->once())
+            ->method('create')
+            ->with($injector, 'horde:groups')
+            ->willReturn($ldapService);
+
+        $factory = new LdapGroupServiceFactory();
+        $result = $factory->create($injector);
+
+        $this->assertInstanceOf(LdapGroupService::class, $result);
+    }
+
+    public function testMissingBasednThrows(): void
+    {
+        $ldapServiceFactory = $this->createStub(HordeLdapServiceFactory::class);
+        $ldapServiceFactory->method('create')->willReturn($this->createStub(StandardHordeLdapService::class));
+
+        $injector = $this->makeInjector([
+            'group' => ['params' => ['gid' => 'cn']], // no basedn
+        ], $ldapServiceFactory);
+
+        $factory = new LdapGroupServiceFactory();
+
+        $this->expectException(RuntimeException::class);
+        $this->expectExceptionMessage('basedn');
+
+        $factory->create($injector);
+    }
+}
diff --git a/test/Unit/Service/LdapGroupServiceTest.php b/test/Unit/Service/LdapGroupServiceTest.php
@@ -107,6 +107,7 @@ public function testGetGroup(): void
         $entry = $this->getMockBuilder(Horde_Ldap_Entry::class)
             ->disableOriginalConstructor()
             ->getMock();
+        $entry->method('exists')->willReturn(true);
         $entry->expects($this->exactly(2))->method('getValue')->willReturnCallback(function ($attr, $mode = null) {
             if ($attr === 'memberUid') {
                 return ['alice', 'bob'];
@@ -132,6 +133,36 @@ public function testGetGroup(): void
         $this->assertEquals('dev@example.com', $group->extra['email'] ?? '');
     }
 
+    public function testGetGroupWithNoMailAttributeReturnsGroupWithoutEmail(): void
+    {
+        // Reproduces a real case: a group entry with actual members but no
+        // 'mail' attribute at all (not empty - genuinely absent).
+        $ldapAdapter = $this->createStub(Horde_Ldap::class);
+        $entry = $this->createStub(Horde_Ldap_Entry::class);
+        $entry->method('exists')->willReturnCallback(fn($attr) => $attr !== 'mail');
+        $entry->method('getValue')->willReturnCallback(function ($attr, $mode = null) {
+            if ($attr === 'memberUid') {
+                // Guards against the real bug this test reproduces: without
+                // explicit 'all', getValue() defaults to 'single' and
+                // collapses a multi-valued attribute to just its first
+                // value as a string.
+                $this->assertEquals('all', $mode, "memberAttr must be read with mode='all'");
+                return ['delepine', 'sdu-zac', 'ld-zac'];
+            }
+            return null;
+        });
+
+        $ldapAdapter->method('getEntry')->willReturn($entry);
+
+        $this->ldapService->method('getAdapter')->willReturn($ldapAdapter);
+        $service = new LdapGroupService($this->ldapService, 'ou=siham,ou=groups,dc=u-picardie,dc=fr');
+
+        $group = $service->get('DISI SSR CSYS');
+
+        $this->assertEquals(['delepine', 'sdu-zac', 'ld-zac'], $group->members);
+        $this->assertArrayNotHasKey('email', $group->extra);
+    }
+
     public function testCreateGroup(): void
     {
         $ldapAdapter = $this->createMock(Horde_Ldap::class);
@@ -196,6 +227,7 @@ public function testExistsTrue(): void
         $entry = $this->getMockBuilder(Horde_Ldap_Entry::class)
             ->disableOriginalConstructor()
             ->getMock();
+        $entry->method('exists')->willReturn(true);
         $entry->expects($this->exactly(2))->method('getValue')->willReturnCallback(function ($attr, $mode = null) {
             if ($attr === 'memberUid') {
                 return [];
@@ -285,6 +317,7 @@ public function testGetMembers(): void
         $entry = $this->getMockBuilder(Horde_Ldap_Entry::class)
             ->disableOriginalConstructor()
             ->getMock();
+        $entry->method('exists')->willReturn(true);
         $entry->expects($this->exactly(2))->method('getValue')->willReturnCallback(function ($attr, $mode = null) {
             if ($attr === 'memberUid') {
                 return ['alice', 'bob'];