[#719] Fix NullPointerException decoding an ACI bind rule with a missing and/or operand

[vharseko]

Fixes #719.

Problem

Aci.decode throws a NullPointerException instead of an AciException when a bind rule contains an and/or boolean whose operand is missing. Reported via ACI:

(version 3.0; acl "ac"; allow (search)(()or) (userdn="ldap:///self"); )

java.lang.NullPointerException: Cannot invoke "...BindRule.setNegate(boolean)" because "bindrule_2" is null
    at ...BindRule.createBindRule(BindRule.java:314)
    at ...BindRule.decode(BindRule.java:206)
    ...

Root cause

An empty group such as () decodes to a null bind rule (the base case of BindRule.decode). That null was then dereferenced without a check — as the right operand of an and/or (the exact NPE from the issue), as the left operand (a complex BindRule with left == null, which NPEs later during evaluation), or returned bare up to PermBindRulePair.

Fix

BindRule.decode now rejects an empty group instead of letting a null propagate:

• A single bindrule_1 == null guard in decode (hoisted above the remaining-chars test) rejects both a bare empty group (()) and an empty left operand (()or userdn=...), reporting the full offending input.
• A bindrule_2 == null guard in createBindRule rejects an empty right operand (... or, ... and).

Both throw WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX (AciException). Same class of malformed-input defect as #716. Rebased on #715, whose blank-bind-rule fix (decode(" ") → null) this change relies on.

Tests

New BindRuleOperandTest:

• Malformed (must throw AciException): (()or), (... or), (... and), ()or ..., (), (()), ( ), (... or ()), not ().
• Positive controls (must decode non-null): ((userdn=...)), (userdn=... or userdn=...).
• Aci.decode: the full ACI from the issue, plus the missing-left case ()or userdn=....

Tests run: 13, Failures: 0, Errors: 0, Skipped: 0

Regression: AciTests 515, AciBodyTest 8, BindRuleParenTest 8 — all pass.

[maximthomas]

Review summary

Approve with changes. No blocking correctness defect. One incorrect code comment,
one guard that should be hoisted/merged, one undocumented public-API contract, and
several test gaps.

Suggested changes

1. Merge guards 1 and 2 into one, hoisted in decode

createBindRule is private with exactly two call sites. The one at
BindRule.java:239 passes the result of parseAndCreateBindrule, which never returns
null (it returns new BindRule(...) or throws). So the only null-left path is the
paren branch. Collapse both guards into a single check in decode, placed above
the currentPos < bindruleArray.length - 1 test:

// after the numOpen > numClose check
if (bindrule_1 == null) {
  // An empty group such as "()" decodes to a null bind rule.
  throw new AciException(WARN_ACI_SYNTAX_INVALID_BIND_RULE_SYNTAX.get(input));
}
if (currentPos < bindruleArray.length - 1) {
  String remainingBindruleStr = bindruleStr.substring(currentPos + 1);
  return createBindRule(bindrule_1, remainingBindruleStr);
}
return bindrule_1;

Then delete the bindrule == null guard added at the top of createBindRule.

Benefits: one guard instead of two; the message names the full offending input
(()or userdn="...") rather than remainingBindruleStr (or userdn="..."), which
currently blames the right operand for a missing left operand; and it removes a
defensive null check on a private method's parameter.

2. Fix the incorrect comment on the bindrule_2 == null guard

The added comment says the or in (()or) "has nothing to its right (issue #719)".
That is true of the pre-fix stack trace but false of the post-fix code: with the
left-operand guard in place, (()or) now throws from the left-operand check and
never reaches the bindrule_2 check. The input that actually reaches this branch is
userdn="ldap:///self" or. Reword to use that example.

3. Document (or eliminate) the null base case of the public decode

decode is public and still returns null for null and blank input, while
decode("()") now throws. The javadoc says only "@return A BindRule class
representing the bind rule." Either:

• add @return ... or {@code null} if the input is null or blank; callers must reject this — the null is load-bearing for the recursion, so this is the lighter option; or
• make those two sites throw and drop the guards entirely (costs message quality: the
  offending text is the empty string).

Also fix the stale comment at BindRule.java:155-158, which claims a blank bind rule
"must be rejected as a syntax error" on a line that does return null.

4. Test gaps in BindRuleOperandTest

Add to the malformed data provider:

• (()) — nested empty group
• ( ) — whitespace-only group (the case the [#712] Fix StringIndexOutOfBoundsException on blank bind rule in ACI #715 comment describes)
• (userdn="ldap:///self" or ()) — empty right group; exercises the decode-level
  guard from inside createBindRule, a combination no current test covers
• not ()

Add a positive control test so guard 1 cannot silently start over-rejecting:

• ((userdn="ldap:///self")) — valid nested group, must decode non-null
• (userdn="ldap:///self" or userdn="ldap:///anyone") — valid complex rule

Add an Aci.decode case for ()or userdn="ldap:///self". That input was previously
accepted and stored, producing a BindRule with left == null that then NPEs at
AciBody.java:256 (p.getBindRule().evaluate(evalCtx)) on every evaluation. That is
the most serious defect the PR fixes and it has no ACI-level regression test.

Verification

mvn -pl opendj-server-legacy test -Dtest=BindRuleOperandTest,BindRuleParenTest,AciBodyTest

Tests are picked up by maven-failsafe (**/*Test.java include, pom.xml:1262).
Also run AciTests to confirm no valid ACI regressed into rejection.

[vharseko]

Thanks for the thorough review, @maximthomas — addressed all four points in 6d478626b5.

First, I rebased onto master: this fix builds on the #715 blank-rule change. Without it decode(" ") throws StringIndexOutOfBoundsException, and your suggested ( ) test relies on that guard.

1. Merged/hoisted the left-side guards. Collapsed both into a single bindrule_1 == null check in decode, above the remaining-chars test. It now rejects both () and ()or userdn=..., and the message names the full input instead of the right-hand fragment. Removed the guard from createBindRule — its only null-capable caller is now guarded upstream (the :239 site passes a non-null parseAndCreateBindrule result).
2. bindrule_2 comment. Reworded — (()or) is caught by the left guard, so the example is now userdn="ldap:///self" or, which is what actually reaches that branch.
3. decode null contract. Documented the null base case in the javadoc (kept the null — it's load-bearing for the recursion) and fixed the stale "must be rejected as a syntax error" comment on the blank-rule return null.
4. Tests. Added malformed (()), ( ), (userdn="ldap:///self" or ()), not (); positive controls ((userdn="ldap:///self")) and (userdn="ldap:///self" or userdn="ldap:///anyone"); and an Aci.decode case for ()or userdn="ldap:///self" (the previously accepted-and-stored left == null defect).

Verification (-Pprecommit): BindRuleOperandTest 13/13; regression AciTests 515, AciBodyTest 8, BindRuleParenTest 8 — all pass, 0 failures.