From e63498de1ae5b5231dc2ad925926a2182f21ca37 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 15 Jul 2026 17:52:39 +0300 Subject: [PATCH 1/2] [#739] Fix alias dereferencing dropping entries and accumulating DNs The set guarding against alias loops in SearchOperationBasis.returnEntry recorded the DN of every entry the search returned, and was never cleared. The DN of an alias target was recorded before recursing onto it, so the recursion found the DN already there and dropped the entry: an alias in the search results returned neither itself nor its target, unless the target was in the scope of the search on its own. A persistent search keeps its operation for its whole lifetime, so the set also outlived the search phase: the second and later notifications for an entry were silently dropped, and the set was written from the worker thread of every operation that notified it. Track the aliases of the current chain in a set local to that chain, and keep the DNs already returned only for the duration of the search phase. Skip an alias whose target does not exist instead of dereferencing null, and report a DirectoryException instead of wrapping it in a RuntimeException. --- .../opends/server/core/SearchOperation.java | 9 ++ .../server/core/SearchOperationBasis.java | 103 +++++++++++++--- .../server/core/SearchOperationWrapper.java | 7 ++ .../LocalBackendSearchOperation.java | 3 + .../opendj/AliasTestCase.java | 113 ++++++++++++++++++ 5 files changed, 220 insertions(+), 15 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperation.java b/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperation.java index 3ddefde30a..3b06726d72 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperation.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperation.java @@ -13,6 +13,7 @@ * * Copyright 2006-2009 Sun Microsystems, Inc. * Portions Copyright 2011-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.core; @@ -262,6 +263,14 @@ public interface SearchOperation extends Operation boolean returnEntry(Entry entry, List controls, boolean evaluateAci); + /** + * Indicates that the search phase is over and that any further entry comes from a persistent + * search. State kept to dereference aliases during the search phase is released, and no further + * entry is matched against it: a persistent search must report every change it is notified of, + * whether or not the entry was returned by the search phase. + */ + void endSearchPhase(); + /** * Used as a callback for backends to indicate that the provided search * reference was encountered during processing and that additional processing diff --git a/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationBasis.java b/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationBasis.java index 00a8074d5b..133c8e705d 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationBasis.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationBasis.java @@ -13,11 +13,12 @@ * * Copyright 2006-2010 Sun Microsystems, Inc. * Portions Copyright 2011-2016 ForgeRock AS. - * Portions Copyright 2024 3A Systems, LLC. + * Portions Copyright 2024-2026 3A Systems, LLC. */ package org.opends.server.core; import java.util.*; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import org.forgerock.i18n.LocalizedIllegalArgumentException; @@ -449,11 +450,46 @@ public final boolean returnEntry(Entry entry, List controls) { return returnEntry(entry, controls, true); } - Set dereferenced=new HashSet<>(); + + /** + * The DNs of the entries already returned by the search phase. An alias may be dereferenced onto + * an entry which is in the scope of the search as well, and that entry must only be returned once. + * It is emptied once the search phase is over, because a persistent search must report every + * change it is notified of, whether or not the entry was returned before. + */ + private final Set returnedDNs = ConcurrentHashMap.newKeySet(); + + /** Whether the search phase is over and only persistent search notifications remain. */ + private volatile boolean searchPhaseOver; + + @Override + public final void endSearchPhase() + { + searchPhaseOver = true; + returnedDNs.clear(); + } @Override public final boolean returnEntry(Entry entry, List controls, boolean evaluateAci) + { + return returnEntry(entry, controls, evaluateAci, null); + } + + /** + * Returns the provided entry to the client, dereferencing it first if it is an alias and the + * deref policy asks for it. + * + * @param entry The entry to return. + * @param controls The controls to attach to the entry. + * @param evaluateAci Whether the access control handler must be consulted. + * @param aliasChain The DNs of the aliases already dereferenced on the way to this entry, or + * {@code null} if no alias was dereferenced yet. It only spans the current + * chain, so it cannot grow beyond the length of that chain. + * @return {@code true} if the search should continue, {@code false} if it should stop. + */ + private boolean returnEntry(Entry entry, List controls, + boolean evaluateAci, Set aliasChain) { boolean typesOnly = getTypesOnly(); @@ -566,22 +602,12 @@ else if (state.isDisabled()) //DereferenceAliasesPolicy if ( DereferenceAliasesPolicy.ALWAYS.equals(getDerefPolicy()) || DereferenceAliasesPolicy.IN_SEARCHING.equals(getDerefPolicy()) ) { if (entry.isAlias() && !baseDN.equals(entry.getName())) { - try { - final DN dn = entry.getAliasedDN(); - final Entry dereference = DirectoryServer.getEntry(dn); - if (dereferenced.contains(dn)) { - return true; - } - dereferenced.add(dn); - return returnEntry(dereference, controls, true); - } catch (DirectoryException e) { - throw new RuntimeException(e); - } + return returnAliasedEntry(entry, controls, aliasChain); } - if (dereferenced.contains(entry.getName())) { + if (!searchPhaseOver && !returnedDNs.add(entry.getName())) { + // This entry was already returned by the search, through an alias or on its own. return true; } - dereferenced.add(entry.getName()); } // Make a copy of the entry and pare it down to only include the set @@ -709,6 +735,53 @@ else if (state.isDisabled()) return pluginResult.continueProcessing(); } + /** + * Returns the entry the provided alias points to, in place of the alias itself. + * + * @param alias The alias entry to dereference. + * @param controls The controls to attach to the entry. + * @param aliasChain The DNs of the aliases already dereferenced on the way to this alias, or + * {@code null} if this alias is the first one of the chain. + * @return {@code true} if the search should continue, {@code false} if it should stop. + */ + private boolean returnAliasedEntry(Entry alias, List controls, Set aliasChain) + { + final DN aliasedDN; + final Entry aliasedEntry; + try + { + aliasedDN = alias.getAliasedDN(); + aliasedEntry = DirectoryServer.getEntry(aliasedDN); + } + catch (DirectoryException e) + { + // A single alias which cannot be read must not fail the whole search. + logger.traceException(e); + return true; + } + + if (aliasedEntry == null) + { + // The alias points to an entry which does not exist: there is nothing to return for it. + return true; + } + if (!searchPhaseOver && returnedDNs.contains(aliasedDN)) + { + // The aliased entry was already returned by the search. + return true; + } + if (aliasChain == null) + { + aliasChain = new HashSet<>(); + } + if (!aliasChain.add(aliasedDN)) + { + // The aliases point at each other: stop before looping forever. + return true; + } + return returnEntry(aliasedEntry, controls, true, aliasChain); + } + private AccessControlHandler getACIHandler() { return AccessControlConfigManager.getInstance().getAccessControlHandler(); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationWrapper.java b/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationWrapper.java index 42afbd3710..5bc8617054 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationWrapper.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationWrapper.java @@ -13,6 +13,7 @@ * * Copyright 2008-2009 Sun Microsystems, Inc. * Portions Copyright 2011-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.core; @@ -57,6 +58,12 @@ public boolean returnEntry(Entry entry, List controls, return getOperation().returnEntry(entry, controls, evaluateAci); } + @Override + public void endSearchPhase() + { + getOperation().endSearchPhase(); + } + @Override public boolean returnReference(DN dn, SearchResultReference reference) { diff --git a/opendj-server-legacy/src/main/java/org/opends/server/workflowelement/localbackend/LocalBackendSearchOperation.java b/opendj-server-legacy/src/main/java/org/opends/server/workflowelement/localbackend/LocalBackendSearchOperation.java index 8d9db9bdec..7b47d981b5 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/workflowelement/localbackend/LocalBackendSearchOperation.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/workflowelement/localbackend/LocalBackendSearchOperation.java @@ -258,6 +258,9 @@ private void processSearch(AtomicBoolean executePostOpPlugins) throws CanceledOp // Process the search in the backend and all its subordinates. backend.search(this); } + + // Any entry returned from now on comes from the persistent search, if there is one. + endSearchPhase(); } catch (DirectoryException de) { diff --git a/opendj-server-legacy/src/test/java/org/openidentityplatform/opendj/AliasTestCase.java b/opendj-server-legacy/src/test/java/org/openidentityplatform/opendj/AliasTestCase.java index 75c2deffa8..885ed249f3 100644 --- a/opendj-server-legacy/src/test/java/org/openidentityplatform/opendj/AliasTestCase.java +++ b/opendj-server-legacy/src/test/java/org/openidentityplatform/opendj/AliasTestCase.java @@ -16,9 +16,12 @@ package org.openidentityplatform.opendj; import org.forgerock.opendj.ldap.*; +import org.forgerock.opendj.ldap.controls.PersistentSearchChangeType; +import org.forgerock.opendj.ldap.controls.PersistentSearchRequestControl; import org.forgerock.opendj.ldap.requests.Requests; import org.forgerock.opendj.ldap.requests.SearchRequest; import org.forgerock.opendj.ldap.responses.SearchResultEntry; +import org.forgerock.opendj.ldap.responses.SearchResultReference; import org.forgerock.opendj.ldif.ConnectionEntryReader; import org.opends.server.DirectoryServerTestCase; import org.opends.server.TestCaseUtils; @@ -30,6 +33,9 @@ import org.testng.annotations.Test; import java.util.HashMap; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.TimeUnit; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.fail; @@ -464,6 +470,113 @@ public void test_alias_target_without_backend() throws SearchResultReferenceIOEx } } + // An alias whose target lies outside the search scope: the target is only + // reachable by dereferencing, so it must be returned by the search. + @Test + public void test_deref_target_outside_scope() throws Exception { + TestCaseUtils.addEntries( + "dn: ou=pointers,o=test", + "objectClass: top", + "objectClass: organizationalUnit", + "ou: pointers", + "", + "dn: cn=ptr,ou=pointers,o=test", + "objectClass: alias", + "objectClass: top", + "objectClass: extensibleObject", + "cn: ptr", + "aliasedObjectName: cn=John Doe,o=MyCompany,o=test", + "" + ); + HashMap res = + search("ou=pointers,o=test", SearchScope.WHOLE_SUBTREE, DereferenceAliasesPolicy.ALWAYS); + + assertThat(res.containsKey("cn=John Doe,o=MyCompany,o=test")).isTrue(); + } + + // An alias pointing at an entry which does not exist is skipped, and the entries next to it are + // still returned: aliases are not checked when they are written, so a dangling one is expected. + @Test + public void test_dangling_alias() throws Exception { + TestCaseUtils.addEntries( + "dn: ou=dangling,o=test", + "objectClass: top", + "objectClass: organizationalUnit", + "ou: dangling", + "", + "dn: cn=broken,ou=dangling,o=test", + "objectClass: alias", + "objectClass: top", + "objectClass: extensibleObject", + "cn: broken", + "aliasedObjectName: cn=does-not-exist,ou=dangling,o=test", + "", + "dn: cn=intact,ou=dangling,o=test", + "cn: intact", + "sn: intact", + "objectClass: person", + "" + ); + HashMap res = + search("ou=dangling,o=test", SearchScope.WHOLE_SUBTREE, DereferenceAliasesPolicy.ALWAYS); + + assertThat(res.containsKey("cn=does-not-exist,ou=dangling,o=test")).isFalse(); + assertThat(res.containsKey("ou=dangling,o=test")).isTrue(); + assertThat(res.containsKey("cn=intact,ou=dangling,o=test")).isTrue(); + } + + // A persistent search reports every change it is notified of. Dereferencing aliases must not + // make it drop a change because the entry was notified before. + @Test + public void test_persistent_search_reports_repeated_changes() throws Exception { + TestCaseUtils.addEntries( + "dn: ou=psearch,o=test", + "objectClass: top", + "objectClass: organizationalUnit", + "ou: psearch", + "", + "dn: cn=changing,ou=psearch,o=test", + "cn: changing", + "sn: changing", + "objectClass: person", + "" + ); + + final BlockingQueue notified = new LinkedBlockingQueue<>(); + final SearchRequest request = + Requests.newSearchRequest("ou=psearch,o=test", SearchScope.WHOLE_SUBTREE, "(objectclass=*)") + .setDereferenceAliasesPolicy(DereferenceAliasesPolicy.ALWAYS) + .addControl(PersistentSearchRequestControl.newControl( + true, true, false, PersistentSearchChangeType.MODIFY)); + + final LDAPConnectionFactory factory = + new LDAPConnectionFactory("localhost", TestCaseUtils.getServerLdapPort()); + try (Connection psearch = factory.getConnection()) { + psearch.bind("cn=Directory Manager", "password".toCharArray()); + psearch.searchAsync(request, new SearchResultHandler() { + @Override + public boolean handleEntry(SearchResultEntry entry) { + notified.add(entry.getName().toString()); + return true; + } + + @Override + public boolean handleReference(SearchResultReference reference) { + return true; + } + }); + + // The same entry is modified repeatedly: each change must reach the persistent search. + for (int i = 1; i <= 3; i++) { + connection.modify(Requests.newModifyRequest("cn=changing,ou=psearch,o=test") + .addModification(ModificationType.REPLACE, "description", "change " + i)); + assertThat(notified.poll(30, TimeUnit.SECONDS)) + .as("notification for change " + i) + .isEqualTo("cn=changing,ou=psearch,o=test"); + } + } + } + @Test(expectedExceptions = LdapException.class) public void test_stackoverflow() throws Exception { From 56d708cfa5ee8df4200855e2f41ada376ad18e4d Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 16 Jul 2026 11:26:46 +0300 Subject: [PATCH 2/2] [#739] Address review: cover malformed/dangling alias deref branches Add the two tests requested in review on #742 and fix a gap one of them exposed: - returnAliasedEntry only caught DirectoryException, but Entry.getAliasedDN() parses aliasedObjectName with the SDK DN, which throws the unchecked LocalizedIllegalArgumentException for a malformed value. A malformed alias therefore still failed the whole search with result code 80. Catch it too so the alias is logged and skipped. - test_alias_with_malformed_target_is_skipped stores a malformed aliasedObjectName (syntax enforcement relaxed to WARN for the add) and asserts the search still succeeds, exercising that catch branch. - test_deref_alias_before_target_returns_target_once pins the alias-before-target ordering, guarding the dedup path against the original order-sensitive regression. Also make test_persistent_search_reports_repeated_changes deterministic: it issued the first modify before searchAsync had registered the persistent search, so the first notification could be missed. Wait until the backend reports the search as registered before modifying. --- .../server/core/SearchOperationBasis.java | 4 +- .../opendj/AliasTestCase.java | 113 ++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationBasis.java b/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationBasis.java index 133c8e705d..9b3bb83f23 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationBasis.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationBasis.java @@ -750,10 +750,12 @@ private boolean returnAliasedEntry(Entry alias, List controls, Set final Entry aliasedEntry; try { + // getAliasedDN() parses the aliasedObjectName value, which throws the unchecked + // LocalizedIllegalArgumentException when that value is not a valid DN. aliasedDN = alias.getAliasedDN(); aliasedEntry = DirectoryServer.getEntry(aliasedDN); } - catch (DirectoryException e) + catch (DirectoryException | LocalizedIllegalArgumentException e) { // A single alias which cannot be read must not fail the whole search. logger.traceException(e); diff --git a/opendj-server-legacy/src/test/java/org/openidentityplatform/opendj/AliasTestCase.java b/opendj-server-legacy/src/test/java/org/openidentityplatform/opendj/AliasTestCase.java index 885ed249f3..b3360fe99b 100644 --- a/opendj-server-legacy/src/test/java/org/openidentityplatform/opendj/AliasTestCase.java +++ b/opendj-server-legacy/src/test/java/org/openidentityplatform/opendj/AliasTestCase.java @@ -15,6 +15,7 @@ */ package org.openidentityplatform.opendj; +import org.forgerock.opendj.config.client.ManagementContext; import org.forgerock.opendj.ldap.*; import org.forgerock.opendj.ldap.controls.PersistentSearchChangeType; import org.forgerock.opendj.ldap.controls.PersistentSearchRequestControl; @@ -23,10 +24,15 @@ import org.forgerock.opendj.ldap.responses.SearchResultEntry; import org.forgerock.opendj.ldap.responses.SearchResultReference; import org.forgerock.opendj.ldif.ConnectionEntryReader; +import org.forgerock.opendj.server.config.client.GlobalCfgClient; +import org.forgerock.opendj.server.config.meta.GlobalCfgDefn; import org.opends.server.DirectoryServerTestCase; import org.opends.server.TestCaseUtils; +import org.opends.server.api.LocalBackend; import org.opends.server.backends.MemoryBackend; +import org.opends.server.core.DirectoryServer; +import org.opends.server.types.AcceptRejectWarn; import org.opends.server.types.Entry; import org.testng.annotations.AfterClass; import org.testng.annotations.BeforeClass; @@ -566,6 +572,16 @@ public boolean handleReference(SearchResultReference reference) { } }); + // searchAsync returns before the server has registered the persistent search, so wait + // until the backend reports it; otherwise the first modification below can be notified + // before the search is listening and be missed. + final LocalBackend backend = TestCaseUtils.getServerContext() + .getBackendConfigManager().getLocalBackendById(TestCaseUtils.TEST_BACKEND_ID); + for (int i = 0; backend.getPersistentSearches().isEmpty() && i < 500; i++) { + Thread.sleep(10); + } + assertThat(backend.getPersistentSearches()).isNotEmpty(); + // The same entry is modified repeatedly: each change must reach the persistent search. for (int i = 1; i <= 3; i++) { connection.modify(Requests.newModifyRequest("cn=changing,ou=psearch,o=test") @@ -577,6 +593,103 @@ public boolean handleReference(SearchResultReference reference) { } } + // An alias is dereferenced before its target is reached on its own: the target must still be + // returned, and exactly once. The original regression was order-sensitive, dropping the target + // when the alias reached it first, so this pins the alias-before-target order specifically. The + // MemoryBackend serving o=test walks entries in insertion order, so listing the alias before the + // target below guarantees the alias is visited first. + @Test + public void test_deref_alias_before_target_returns_target_once() throws Exception { + TestCaseUtils.addEntries( + "dn: ou=order,o=test", + "objectClass: top", + "objectClass: organizationalUnit", + "ou: order", + "", + "dn: cn=aaa-alias,ou=order,o=test", + "objectClass: alias", + "objectClass: top", + "objectClass: extensibleObject", + "cn: aaa-alias", + "aliasedObjectName: cn=zzz-target,ou=order,o=test", + "", + "dn: cn=zzz-target,ou=order,o=test", + "cn: zzz-target", + "sn: target", + "objectClass: person", + "" + ); + // search() asserts every returned DN is unique, so a duplicated target would fail this call. + HashMap res = + search("ou=order,o=test", SearchScope.WHOLE_SUBTREE, DereferenceAliasesPolicy.ALWAYS); + + // The target is returned, reached through the alias visited before it ... + assertThat(res.containsKey("cn=zzz-target,ou=order,o=test")).isTrue(); + // ... and the alias entry itself is replaced by its target, never returned as-is. + assertThat(res.containsKey("cn=aaa-alias,ou=order,o=test")).isFalse(); + } + + // An alias whose aliasedObjectName is not a valid DN cannot be dereferenced. Reading it throws a + // DirectoryException, which must be logged and the alias skipped, not turned into a failed search. + @Test + public void test_alias_with_malformed_target_is_skipped() throws Exception { + // A malformed DN can only be stored while syntax enforcement is relaxed; the default REJECT + // policy refuses the add outright. + final AcceptRejectWarn previous = DirectoryServer.getCoreConfigManager().getSyntaxEnforcementPolicy(); + setSyntaxEnforcementPolicy(GlobalCfgDefn.InvalidAttributeSyntaxBehavior.WARN); + try { + TestCaseUtils.addEntries( + "dn: ou=malformed,o=test", + "objectClass: top", + "objectClass: organizationalUnit", + "ou: malformed", + "", + "dn: cn=badptr,ou=malformed,o=test", + "objectClass: alias", + "objectClass: top", + "objectClass: extensibleObject", + "cn: badptr", + "aliasedObjectName: not a valid dn", + "", + "dn: cn=neighbour,ou=malformed,o=test", + "cn: neighbour", + "sn: neighbour", + "objectClass: person", + "" + ); + } finally { + setSyntaxEnforcementPolicy(asSyntaxBehavior(previous)); + } + + // The whole search still succeeds: the broken alias is skipped, its neighbours returned. + HashMap res = + search("ou=malformed,o=test", SearchScope.WHOLE_SUBTREE, DereferenceAliasesPolicy.ALWAYS); + + assertThat(res.containsKey("cn=badptr,ou=malformed,o=test")).isFalse(); + assertThat(res.containsKey("ou=malformed,o=test")).isTrue(); + assertThat(res.containsKey("cn=neighbour,ou=malformed,o=test")).isTrue(); + } + + private static void setSyntaxEnforcementPolicy(GlobalCfgDefn.InvalidAttributeSyntaxBehavior value) throws Exception { + try (ManagementContext conf = TestCaseUtils.getServer().getConfiguration()) { + GlobalCfgClient globalCfg = conf.getRootConfiguration().getGlobalConfiguration(); + globalCfg.setInvalidAttributeSyntaxBehavior(value); + globalCfg.commit(); + } + } + + private static GlobalCfgDefn.InvalidAttributeSyntaxBehavior asSyntaxBehavior(AcceptRejectWarn policy) { + switch (policy) { + case ACCEPT: + return GlobalCfgDefn.InvalidAttributeSyntaxBehavior.ACCEPT; + case WARN: + return GlobalCfgDefn.InvalidAttributeSyntaxBehavior.WARN; + case REJECT: + default: + return GlobalCfgDefn.InvalidAttributeSyntaxBehavior.REJECT; + } + } + @Test(expectedExceptions = LdapException.class) public void test_stackoverflow() throws Exception {