From 11c83010765522302f0a2cc40e2ccb5f6b1460ea Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 6 Jul 2026 13:48:33 +0300 Subject: [PATCH 1/2] Fix ACI grouped bind rule wrongly rejected when a value contains parentheses BindRule.decode located the matching close parenthesis by counting '(' and ')' across the whole string, including characters inside a quoted expression. A grouped bind rule whose value contained a literal parenthesis - for example (userdn="ldap:///cn=a)b,dc=x" or userdn="ldap:///self") - was split at the wrong parenthesis and rejected with AciException. Track quote state so parentheses inside a quoted expression are treated as literal data. New BindRuleParenTest covers simple and grouped rules with '(' / ')' / '()' in the value. The full AciTests suite (515 tests) passes except the two cases already fixed separately by PR #675. --- .../authorization/dseecompat/BindRule.java | 26 ++++--- .../dseecompat/BindRuleParenTest.java | 76 +++++++++++++++++++ 2 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleParenTest.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java index 52f7b09ac3..8952ea6ae3 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java @@ -13,6 +13,7 @@ * * Copyright 2008 Sun Microsystems, Inc. * Portions Copyright 2013-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC */ package org.opends.server.authorization.dseecompat; @@ -133,11 +134,10 @@ private BindRule(BindRule left, BindRule right, EnumBooleanTypes booleanType) { } /* - * TODO Verify this method handles escaped parentheses by writing - * a unit test. - * - * It doesn't look like the decode() method handles the possibility of - * escaped parentheses in a bind rule. + * Parentheses embedded in a quoted bind rule expression (for example a DN + * value such as userdn="ldap:///cn=a(b),dc=example,dc=com") are treated as + * literal data and are not mistaken for grouping parentheses. See the + * quote-aware scan below and BindRuleParenTest for the covering cases. */ /** * Decode an ACI bind rule string representation. @@ -160,19 +160,27 @@ public static BindRule decode (String input) throws AciException { int currentPos; int numOpen = 0; int numClose = 0; + boolean inQuotes = false; - // Find the associated closed parenthesis + // Find the associated closed parenthesis. Parentheses that appear + // inside a quoted bind rule expression (e.g. a DN value containing + // '(' or ')') are literal data and must not be counted as grouping. for (currentPos = 0; currentPos < bindruleArray.length; currentPos++) { - if (bindruleArray[currentPos] == '(') + char currentChar = bindruleArray[currentPos]; + if (currentChar == '"') + { + inQuotes = !inQuotes; + } + else if (!inQuotes && currentChar == '(') { numOpen++; } - else if (bindruleArray[currentPos] == ')') + else if (!inQuotes && currentChar == ')') { numClose++; } - if (numClose == numOpen) + if (!inQuotes && numClose == numOpen) { // We found the associated closed parenthesis the parenthesis are removed String bindruleStr1 = bindruleStr.substring(1, currentPos); diff --git a/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleParenTest.java b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleParenTest.java new file mode 100644 index 0000000000..803dc02001 --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleParenTest.java @@ -0,0 +1,76 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions Copyright [year] [name of copyright owner]". + * + * Portions Copyright 2026 3A Systems, LLC + */ +package org.opends.server.authorization.dseecompat; + +import static org.assertj.core.api.Assertions.*; + +import org.opends.server.DirectoryServerTestCase; +import org.opends.server.TestCaseUtils; +import org.opends.server.types.DirectoryException; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +/** + * Verifies that {@link BindRule#decode(String)} treats parentheses embedded in a + * quoted bind rule expression (e.g. a DN value containing '(' or ')') as literal + * data instead of grouping parentheses. Before the quote-aware scan, a grouped + * bind rule whose value contained a parenthesis was wrongly rejected because the + * embedded parenthesis was counted when locating the matching close parenthesis. + */ +@SuppressWarnings("javadoc") +public class BindRuleParenTest extends DirectoryServerTestCase +{ + @BeforeClass + public void setUp() throws Exception + { + TestCaseUtils.startFakeServer(); + } + + @AfterClass + public void tearDown() throws DirectoryException + { + TestCaseUtils.shutdownFakeServer(); + } + + @DataProvider(name = "acisWithParenInValue") + public Object[][] acisWithParenInValue() + { + return new Object[][] { + // simple (ungrouped) bind rule with a ')' / '(' in the DN value + { "(version 3.0; acl \"t\"; allow (search) userdn=\"ldap:///cn=a)b,dc=x\";)" }, + { "(version 3.0; acl \"t\"; allow (search) userdn=\"ldap:///cn=a(b,dc=x\";)" }, + // grouped bind rule with a ')' embedded in a value + { "(version 3.0; acl \"t\"; allow (search) " + + "(userdn=\"ldap:///cn=a)b,dc=x\" or userdn=\"ldap:///self\");)" }, + // grouped bind rule with a '(' embedded in a value + { "(version 3.0; acl \"t\"; allow (search) " + + "(userdn=\"ldap:///cn=a(b,dc=x\" or userdn=\"ldap:///self\");)" }, + // grouped bind rule with matched parentheses embedded in a value + { "(version 3.0; acl \"t\"; allow (search) " + + "(userdn=\"ldap:///cn=a(b),dc=x\" or userdn=\"ldap:///self\");)" }, + }; + } + + @Test(dataProvider = "acisWithParenInValue") + public void decodesParenInsideQuotedValue(String aci) throws Exception + { + AciBody body = AciBody.decode(aci); + assertThat(body.getPermBindRulePairs()).hasSize(1); + assertThat(body.getPermBindRulePairs().get(0).getBindRule()).isNotNull(); + } +} From f5e2e6985b042b4b1dc20ce6cae65888e73e1522 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 7 Jul 2026 10:09:30 +0300 Subject: [PATCH 2/2] Add negative tests for unbalanced ACI bind rule parentheses Confirm BindRule.decode still rejects genuinely malformed grouped bind rules after the quote-aware scan: a missing close parenthesis, an extra unmatched open parenthesis, and a group whose only ')' is embedded in a quoted value (which must not be mistaken for the group's close). All three throw AciException, guarding against a fail-open regression. --- .../dseecompat/BindRuleParenTest.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleParenTest.java b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleParenTest.java index 803dc02001..7174f08b7c 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleParenTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleParenTest.java @@ -31,6 +31,8 @@ * data instead of grouping parentheses. Before the quote-aware scan, a grouped * bind rule whose value contained a parenthesis was wrongly rejected because the * embedded parenthesis was counted when locating the matching close parenthesis. + * Also verifies the scan stays fail-closed: genuinely unbalanced grouping + * parentheses (outside quotes) are still rejected with an {@link AciException}. */ @SuppressWarnings("javadoc") public class BindRuleParenTest extends DirectoryServerTestCase @@ -73,4 +75,25 @@ public void decodesParenInsideQuotedValue(String aci) throws Exception assertThat(body.getPermBindRulePairs()).hasSize(1); assertThat(body.getPermBindRulePairs().get(0).getBindRule()).isNotNull(); } + + @DataProvider(name = "malformedBindRules") + public Object[][] malformedBindRules() + { + return new Object[][] { + // grouped bind rule with no close parenthesis at all + { "(userdn=\"ldap:///self\"" }, + // grouped bind rule with one extra (unmatched) open parenthesis + { "((userdn=\"ldap:///self\")" }, + // grouped bind rule whose only ')' is embedded in a quoted value, so the + // group is genuinely unterminated: the quote-aware scan must not treat the + // embedded ')' as the closing parenthesis and silently accept the input. + { "(userdn=\"ldap:///cn=a)b,dc=x\" or userdn=\"ldap:///self\"" }, + }; + } + + @Test(dataProvider = "malformedBindRules") + public void rejectsUnbalancedParensOutsideQuotes(String bindRule) + { + assertThatThrownBy(() -> BindRule.decode(bindRule)).isInstanceOf(AciException.class); + } }