From 7743dffc2820ff86b8d436c6ee40c31db377fbc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean=20Charles=20Del=C3=A9pine?= Date: Mon, 7 Sep 2026 12:20:45 +0200 Subject: [PATCH] feat(group): wire up ldap driver in modern GroupService GroupServiceFactory only supported 'sql'; LdapGroupService and LdapGroupServiceFactory already existed but were never wired in. - Add 'ldap' case to GroupServiceFactory::create() - Fix LdapGroupServiceFactory reading 'groups.params' instead of the correct 'group.params' config key - LdapGroupService now delegates filter building to Horde_Ldap_Filter::build(), matching legacy Horde_Group_Ldap, instead of a narrower objectClass-only reimplementation - Fix LdapGroupService::get() passing a malformed attributes parameter to Horde_Ldap::getEntry() (double-wrapped in an 'attributes' key instead of the flat list getEntry() expects), which silently dropped the member attribute from every fetched group entry even when it was present in LDAP - Guard the 'member' and 'mail' reads in get() with exists(), since Horde_Ldap_Entry::getValue() throws when an attribute is genuinely absent rather than returning null/empty - Read the member attribute with explicit mode 'all': without it, getValue() defaults to 'single' and silently collapses a multi-valued attribute to just its first value as a string, dropping every other member with no error at all - Add GroupServiceFactoryTest and LdapGroupServiceFactoryTest covering the new 'ldap' wiring and the per-service LDAP connection resolution --- src/Factory/GroupServiceFactory.php | 5 +- src/Factory/LdapGroupServiceFactory.php | 14 +-- src/Service/LdapGroupService.php | 35 +++--- test/Unit/Factory/GroupServiceFactoryTest.php | 89 +++++++++++++++ .../Factory/LdapGroupServiceFactoryTest.php | 101 ++++++++++++++++++ test/Unit/Service/LdapGroupServiceTest.php | 33 ++++++ 6 files changed, 251 insertions(+), 26 deletions(-) create mode 100644 test/Unit/Factory/GroupServiceFactoryTest.php create mode 100644 test/Unit/Factory/LdapGroupServiceFactoryTest.php diff --git a/src/Factory/GroupServiceFactory.php b/src/Factory/GroupServiceFactory.php index f4a54a93..239081fa 100644 --- 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 index 6a2643fe..87f74900 100644 --- 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 index 682297a7..1c031402 100644 --- a/src/Service/LdapGroupService.php +++ b/src/Service/LdapGroupService.php @@ -43,7 +43,10 @@ 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( @@ -51,7 +54,7 @@ public function __construct( 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 new file mode 100644 index 00000000..8543e411 --- /dev/null +++ b/test/Unit/Factory/GroupServiceFactoryTest.php @@ -0,0 +1,89 @@ + $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 new file mode 100644 index 00000000..668bd0ed --- /dev/null +++ b/test/Unit/Factory/LdapGroupServiceFactoryTest.php @@ -0,0 +1,101 @@ + $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 index dc5a2c9f..96a4cbca 100644 --- 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'];