Address review comments on #719 bind-rule operand fix

- Hoist the null left-operand guard into BindRule.decode, above the
  remaining-chars test, so one check rejects both a bare empty group ("()")
  and a missing left operand ("()or userdn=..."), and the message names the
  full offending input. Remove the now-redundant null guard in
  createBindRule (its only null-capable caller is guarded upstream).
- Correct the bindrule_2 comment: "(()or)" is caught by the left guard, so
  use the input that actually reaches it ('userdn="ldap:///self" or').
- Document decode's null base case in the javadoc and fix the stale
  blank-bind-rule comment.
- Add tests: malformed "(())", "(   )", '... or ()', "not ()"; positive
  controls for valid nested/complex rules; and an ACI-level missing-left
  regression case.

Author: vharseko

opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/BindRule.java

@@ -142,7 +142,9 @@ private BindRule(BindRule left, BindRule right, EnumBooleanTypes booleanType) {
     /**
      * Decode an ACI bind rule string representation.
      * @param input The string representation of the bind rule.
-     * @return A BindRule class representing the bind rule.
+     * @return A BindRule class representing the bind rule, or {@code null} if
+     * {@code input} is null or blank. This null is the recursion base case for
+     * an empty group; callers must reject it rather than dereference it.
      * @throws AciException If the string is an invalid bind rule.
      */
     public static BindRule decode (String input) throws AciException {
@@ -153,8 +155,10 @@ public static BindRule decode (String input) throws AciException {
         String bindruleStr = input.trim();
         if (bindruleStr.isEmpty())
         {
-          // A blank bind rule (e.g. "(          )") must be rejected as a
-          // syntax error rather than throwing StringIndexOutOfBoundsException.
+          // A blank bind rule (e.g. "(          )") decodes to null rather than
+          // throwing StringIndexOutOfBoundsException on charAt(0). This null is
+          // the recursion base case for an empty group; the caller rejects it as
+          // a syntax error (see the bindrule_1 == null check below).
           return null;
         }
         char firstChar = bindruleStr.charAt(0);
@@ -202,6 +206,17 @@ else if (!inQuotes && currentChar == ')')
           if (numOpen > numClose) {
               throw new AciException(WARN_ACI_SYNTAX_BIND_RULE_MISSING_CLOSE_PAREN.get(input));
           }
+          /*
+           * An empty group such as "()" or "(   )" decodes to a null bind rule.
+           * Such a group is not a valid bind rule on its own, and using it as the
+           * left operand of an "and"/"or" (e.g. "()or userdn=...") would build a
+           * complex rule with a null left side that is later dereferenced during
+           * evaluation. Reject both cases here, before the remaining-chars test,
+           * so the message names the full offending input rather than a fragment.
+           */
+          if (bindrule_1 == null) {
+            throw new AciException(WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(input));
+          }
           /*
            * If there are remaining chars => there MUST be an operand (AND / OR)
            * otherwise there is a syntax error
@@ -212,14 +227,6 @@ else if (!inQuotes && currentChar == ')')
                 bindruleStr.substring(currentPos + 1);
             return createBindRule(bindrule_1, remainingBindruleStr);
           }
-          /*
-           * An empty group such as "()" decodes to a null bind rule. A group
-           * with no bind rule inside is not valid, so reject it instead of
-           * returning null and letting the caller dereference it later.
-           */
-          if (bindrule_1 == null) {
-            throw new AciException(WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(input));
-          }
           return bindrule_1;
         }
         else
@@ -303,15 +310,6 @@ private static BindRule parseAndCreateBindrule(Matcher bindruleMatcher) throws A
      */
     private static BindRule createBindRule(BindRule bindrule,
             String remainingBindruleStr) throws AciException {
-        /*
-         * The left operand of an "and"/"or" bind rule must exist. It is null when
-         * the left side reduced to an empty group such as "()" (for example the
-         * bind rule "()or userdn=..."). A complex bind rule with a null left side
-         * would later be dereferenced during evaluation, so reject it here.
-         */
-        if (bindrule == null) {
-            throw new AciException(WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(remainingBindruleStr));
-        }
         Pattern remainingBindrulePattern = Pattern.compile(remainingBindruleRegex);
         Matcher remainingBindruleMatcher = remainingBindrulePattern.matcher(remainingBindruleStr);
         if (remainingBindruleMatcher.find()) {
@@ -338,7 +336,8 @@ private static BindRule createBindRule(BindRule bindrule,
             /*
              * The right operand of an "and"/"or" bind rule must exist. It is null
              * when the boolean operator is not followed by a bind rule, e.g. the
-             * "or" in "(()or)" has nothing to its right (issue #719).
+             * "or" in "userdn=\"ldap:///self\" or" has nothing to its right
+             * (issue #719).
              */
             if (bindrule_2 == null) {
                 throw new AciException(

opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/BindRuleOperandTest.java

@@ -65,6 +65,14 @@ public Object[][] bindRulesWithMissingOperand()
       { "()or userdn=\"ldap:///self\"" },
       // an empty group is not a bind rule on its own
       { "()" },
+      // a nested empty group is still empty
+      { "(())" },
+      // a whitespace-only group is empty once trimmed
+      { "(   )" },
+      // empty right *group*: reaches the decode-level guard from createBindRule
+      { "(userdn=\"ldap:///self\" or ())" },
+      // "not" applied to an empty group
+      { "not ()" },
     };
   }
 
@@ -74,11 +82,40 @@ public void rejectsMissingOperand(String bindRule)
     assertThatThrownBy(() -> BindRule.decode(bindRule)).isInstanceOf(AciException.class);
   }
 
-  /** Reproduces the full ACI from issue #719 through {@link Aci#decode}. */
-  @Test
-  public void decodeOfAciWithMissingOperandThrowsAciException()
+  @DataProvider(name = "validBindRules")
+  public Object[][] validBindRules()
+  {
+    return new Object[][] {
+      // a valid rule inside a nested group must still decode: guards must not
+      // start over-rejecting non-empty groups
+      { "((userdn=\"ldap:///self\"))" },
+      // a valid complex rule with both operands present
+      { "(userdn=\"ldap:///self\" or userdn=\"ldap:///anyone\")" },
+    };
+  }
+
+  @Test(dataProvider = "validBindRules")
+  public void acceptsValidBindRule(String bindRule) throws Exception
+  {
+    assertThat(BindRule.decode(bindRule)).isNotNull();
+  }
+
+  @DataProvider(name = "acisWithMissingOperand")
+  public Object[][] acisWithMissingOperand()
+  {
+    return new Object[][] {
+      // the full ACI from issue #719
+      { "(version 3.0; acl \"ac\"; allow (search)(()or) (userdn=\"ldap:///self\"); )" },
+      // missing *left* operand: previously accepted and stored with left == null,
+      // then NPEd during evaluation. Must now be rejected at decode time.
+      { "(version 3.0; acl \"ac\"; allow (search) ()or userdn=\"ldap:///self\"; )" },
+    };
+  }
+
+  /** Reproduces missing-operand ACIs from issue #719 through {@link Aci#decode}. */
+  @Test(dataProvider = "acisWithMissingOperand")
+  public void decodeOfAciWithMissingOperandThrowsAciException(String aci)
   {
-    String aci = "(version 3.0; acl \"ac\"; allow (search)(()or) (userdn=\"ldap:///self\"); )";
     assertThatThrownBy(() -> Aci.decode(ByteString.valueOfUtf8(aci), DN.rootDN()))
         .isInstanceOf(AciException.class);
   }