feat(group): wire up ldap driver in modern GroupService - #225
Merged
ralflang merged 1 commit intoSep 14, 2026
Merged
Conversation
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
ralflang
approved these changes
Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Found while continuing my OIDC/OAuth2 authentication work on Horde6:
constructing
Horde\Horde\Settings\OAuthAccountControllerfailed withHorde\Injector\NotFoundException:LdapGroupServiceandLdapGroupServiceFactoryalready exist and arelargely complete — they were simply never called by
GroupServiceFactory::create(), which only handled'sql'. This isnot a regression fix: the legacy driver (
Horde_Group_Ldap) hasalways worked fine; only the modern port was never wired in.
What this fixes
GroupServiceFactory: adds the'ldap'case.LdapGroupServiceFactory: was readinggroups.params(typo)instead of
group.params; was resolving the default LDAPconnection instead of the
'horde:groups'service, so it ignoreda dedicated
$conf['ldap']['service']['groups']if one was set.LdapGroupService::buildFilter(): was hand-building anobjectClassfilter (OR-only, no support forfilter_type = 'filter') instead of delegating toHorde_Ldap_Filter::build()the way the legacy driver does — sameresult for a single objectclass, but diverges as soon as there are
several objectclasses or a raw filter configured.
LdapGroupService::get()had two bugs that silently producedwrong data rather than erroring, both found by exercising it
against a real directory:
attributesparameter passed toHorde_Ldap::getEntry()was double-wrapped (
['attributes' => [...]]instead of theflat list
getEntry()expects), so the member attribute wasnever actually requested from the server — it silently came
back empty even when the entry had members.
getValue()'s default'single'mode, which collapses a multi-valued attribute tojust its first value as a string; reading a real 3-member group
silently returned only 1 (then 0, once the first bug above was
also fixed and
is_array()rejected the resulting string).memberandmailreads are now guarded withexists(),since
Horde_Ldap_Entry::getValue()throws when an attribute isgenuinely absent rather than returning null/empty — matching
legacy
Horde_Group_Ldap's own guard for the same reason.Deliberately out of scope
Two things were found while testing against a real directory and are
being tracked separately in #224 rather than folded into this PR:
attrisdn(LDAP members stored as full DNs rather than uids) — theconfig schema already accounts for it, but
LdapGroupServicedoesn't resolve DNs on write or normalize them on read. A prior
attempt at this is sitting in a dropped commit if useful as a
starting point, but it's not upstream-ready and not yet used by
any real deployment, so it seemed better to check with the
maintainers on the LDAP side before proposing something concrete.
GroupService's LDAP-backed group IDs are a barecnvalue(
LdapGroupService::buildDN()reconstructs the DN from it), whilelegacy
Horde_Group_Ldapuses the full DN as the group IDthroughout its API. The two are not interchangeable, which could
matter for any permissions/ACL data created via the legacy admin
UI.
See #224 for reproduction steps on both.
Tests
There was no existing test for
GroupServiceFactoryorLdapGroupServiceFactorybefore this PR. Both new test files followthe pattern already established in
HordeLdapServiceFactoryTest(injector stubbed via
willReturnMap, a realHorde\Core\Config\Stateinstead of mocking it, no deep-mockedConfigLoader) rather than inventing a new approach for this PR.LdapGroupServiceTest(pre-existing) gained coverage for the twoget()bugs above, including a regression test reproducing thereal directory entry that surfaced them.