[#1155] Make the rest2ldap bind templates and HTTP Basic credentials work as documented

[vharseko]

Problem

rest2ldap could not build an LDAP name from HTTP Basic credentials in several configurations the reference documentation recommends. See #1155.

• sasl-plain with an authzIdTemplate starting with dn: failed every bind with INVALID_DN_SYNTAX, because the whole dn:... string went to DN.format().
• The simple bind with its default bindDnTemplate, {username}, never worked. DN.format() escaped the whole DN user name as one attribute value. The parse error was thrown from authenticate() instead of failing the returned promise.
• An HTTP Basic password that contains : was dropped: the credentials were split with split(":") and anything but two parts meant "no credentials". A bare Basic header threw StringIndexOutOfBoundsException.
• A % in the fixed part of a template was read by String.format() as a format specifier (o=100%,dc=example → MissingFormatArgumentException: '%,d'). DnTemplate also stripped a trailing ,.. after an escaped comma.

Change

• Bind DN templates (authz/Utils.formatBindDn, used by SimpleBindStrategy and the dn: branch of SaslPlainStrategy): a template which is just {username} takes the user name as the DN (DN.valueOf); any other template keeps DN.format(), which escapes the user name as one attribute value. A user name which does not give a DN fails the returned promise with INVALID_CREDENTIALS (HTTP 401) before a connection is taken.
• sasl-plain: only the part after dn: is formatted, and the authentication ID sent is dn:<DN>. Spaces after dn: are ignored, as for the OAuth2 template, so dn: {username} reads as dn:{username}.
• OAuth2 authzIdTemplate (AuthzIdTemplate): a dn: template which is just one placeholder, such as dn:{sub}, takes the field value as the DN as well. Without this, it failed in the same way as the default simple template.
• HTTP Basic (CredentialExtractors): the credentials are split at the first :, as RFC 7617 §2 requires, and the scheme must be followed by a space. The case-insensitive match no longer depends on the default locale. Credentials which do not decode as base64, such as Basic abc or unpadded base64, are "no credentials" instead of a NullPointerException.
• Empty password (HttpBasicAuthenticationFilter): refused with 401 before any bind. A simple bind with a DN and an empty password is an unauthenticated bind (RFC 4513 §5.1.2). A server which accepts it would let the request run as the named user through proxied authorization without checking any password. Before this change, user: from the Basic header was "no credentials" and fell through to the next authorization mechanism; an empty alternative password header already reached the bind strategy. The server's own HTTP Basic mechanism (HttpBasicAuthorizationMechanism) uses the same filter and extractor, so it gets both changes.
• % in templates: DnTemplate, AuthzIdTemplate and the {username} templates of the basic configuration (bindDnTemplate, authzIdTemplate, filterTemplate) escape % in their fixed parts. The configuration defaults are now {username} and u:{username} instead of the raw format strings %s and u:%s. A literal %s written in config.json, which was never documented, is now kept as text.
• DnTemplate: a trailing ,.. is stripped only when its comma is not escaped (an even number of backslashes before it).
• The reference (appendix-rest2ldap.adoc) says how a template which is just {username} or one field is read, and gives the sasl-plain default.

Departures from the issue text: an empty password is parsed but refused (see above), and a user name which does not give a DN is reported as INVALID_CREDENTIALS (401) rather than INVALID_DN_SYNTAX, which maps to 400.

Test

• SimpleBindStrategyTest (new, in-memory backend): the default template binds with a DN user name. A template escapes the user name, so bjensen,ou=People cannot add RDNs. A user name which is not a DN fails the promise with INVALID_CREDENTIALS and takes no connection.
• SaslPlainStrategyTest (new): the authentication ID sent for dn:%s, dn: %s, dn:uid=%s,... (escaped) and u:%s. A user name which is not a DN fails without taking a connection or sending a bind.
• BasicJsonConfigurationTestCase (new): the basic configuration defaults for simple and sasl-plain, dn:{username}, and % in all three templates, read from the request sent to the server.
• CredentialExtractorsTest: bjensen:se:cret, bjensen::, a DN user name and bjensen:. testBasicReturnNullOnInvalidCredentials used to fill headers and then pass new Headers(); it now checks the headers it builds, including a bare Basic, Basic abc and unpadded base64.
• HttpBasicAuthenticationFilterTest: an empty password gives 401 and the strategy is never called.
• AuthzIdTemplateTest, DnTemplateTest: % in the fixed part, dn:{dn} and dn: {dn} with a DN value, \,.. and \\,...

Results:

• Against master, 27 of the new checks fail, each for the reason the issue describes.
• With the fix, the whole opendj-rest2ldap suite passes (571 tests). Javadoc doclint passes, and a broken {@link} added as a negative control fails it.
• Nine mutants each turn at least one test red: DN.valueOf(String.format(...)) for every template, the empty password let through, any backslash escaping the comma, the old %s default, a split at the last colon, no null check after Base64.decode, no trim after dn:, and getConnectionAsync() called before the user name is checked, in SimpleBindStrategy and in SaslPlainStrategy.

Fixes #1155

[maximthomas]

praise: The bind templates and the Basic parser now fail where the issue says they should, and the new tests pin it.

• Utils.formatBindDn runs before getConnectionAsync() in both SimpleBindStrategy.authenticate and SaslPlainStrategy.authenticate, so a user name that does not give a DN fails the returned promise with INVALID_CREDENTIALS. At the base, the exception was thrown after the connection had been requested and before thenFinally(close) was attached, so that connection was never closed.
• CredentialExtractors.parseUsernamePassword splits at the first : (RFC 7617) and matches the scheme with regionMatches(true, …), so the match no longer depends on the default locale.
• testBasicReturnNullOnInvalidCredentials now checks the headers it builds, not a fresh new Headers().

---

issue (non-blocking): A Basic header whose credentials have a legal-alphabet length that is not a multiple of 4, such as Authorization: Basic abc, still throws NullPointerException from the parser.

opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/authz/CredentialExtractors.java:127

org.forgerock.util.encode.Base64.decode drops illegal characters, then returns null when the rest is not a multiple of 4 long (probe against util-3.2.0: "abc" → null, "!!!" → empty array). new String((byte[]) null) throws, and nothing catches it on the way up from Authorization.canApplyFilter or HttpBasicAuthenticationFilter.filter, so the request ends in an unchecked exception instead of a 401 or a fall-through to the next mechanism. This affects the rest2ldap gateway and the server's HttpBasicAuthorizationMechanism. The same expression was at the base, but this method is the one the PR hardens, and the new Basic !!! row only reaches the empty-array case.

final byte[] decoded = Base64.decode(base64UserCredentials);
if (decoded == null) {
    return null;
}
final String userCredentials = new String(decoded);

Pin: a { "Basic abc" } row in the testBasicReturnNullOnInvalidCredentials data provider is red without the null check.

---

suggestion (non-blocking): For sasl-plain, dn: {username} (with a space after dn:) is not read as the whole-DN form, but the OAuth2 dn: {dn} is.

opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/authz/SaslPlainStrategy.java:72, Utils.java:80

SaslPlainStrategy keeps " %s" after the dn: key, and formatBindDn treats a template as whole-DN only when it equals "%s" exactly. DN.format then escapes a DN user name such as uid=bjensen,ou=People,dc=example,dc=com as one RDN value, and every bind gets a 401. AuthzIdTemplate.removeTemplateKey (AuthzIdTemplate.java:110) trims after the key, and AuthzIdTemplateTest pins dn: {dn}, so the same spelling works for OAuth2 and fails for sasl-plain.

final String dnTemplate = authcIdTemplate.substring("dn:".length()).trim();

Pin: a SaslPlainStrategyTest case with dn: %s and a DN user name, expecting the authentication ID dn:uid=bjensen,ou=People,dc=example,dc=com.

---

suggestion (non-blocking): testUserNameWhichIsNotADnFailsThePromise does not pin that a user name which does not give a DN opens no connection.

opendj-rest2ldap/src/test/java/org/forgerock/opendj/rest2ldap/authz/SimpleBindStrategyTest.java:91-95

The case runs against an in-memory factory and asserts only the result code. A mutant that moves connectionFactory.getConnectionAsync() above the try { formatBindDn(...) } block still fails with INVALID_CREDENTIALS, opens a connection that is never closed (the leak at the base), and leaves all five cases green. Not run.

@Test
public void testUserNameWhichIsNotADnTakesNoConnection() throws Exception {
    final ConnectionFactory connections = mock(ConnectionFactory.class);
    assertFailsWith(newSimpleBindStrategy(connections, "%s", Schema.getDefaultSchema())
            .authenticate("bjensen", "secret", new RootContext()), ResultCode.INVALID_CREDENTIALS);
    verify(connections, never()).getConnectionAsync();
}

Pin: verify(connections, never()).getConnectionAsync() fails against the hoisted-connection mutant (imports org.mockito.Mockito.mock, never, verify).

[vharseko]

Thanks for the review. All three points are taken in 87dff9e. The branch is rebased on the current master (751a4d2); the round 1 commit did not change in the rebase.

Basic abc → NullPointerException. Confirmed against util-3.2.0: Base64.decode returns null for abc, a, YQ and also for well-formed but unpadded credentials such as Zm9vOmJhcg (foo:bar). parseUsernamePassword now returns null for them, so the request gets a 401 or falls through to the next mechanism, like any other unusable header. invalidBasicHeaders has two new rows, Basic abc and the unpadded foo:bar; both fail with the NPE without the check.

dn: {username} for sasl-plain. SaslPlainStrategy now trims after dn:, as AuthzIdTemplate.removeTemplateKey does. The authentication ID sent is still dn:<DN> with no space. New row in SaslPlainStrategyTest.authcIdTemplates: dn: %s with a DN user name. Without the trim it fails with INVALID_CREDENTIALS on " uid\=bjensen\,ou\=People\,…".

No connection for a user name which is not a DN. Added SimpleBindStrategyTest.testUserNameWhichIsNotADnTakesNoConnection as you suggested. SaslPlainStrategyTest.testUserNameWhichIsNotADnFailsThePromise had the same gap: it checked only that no bind was sent, and with getConnectionAsync() moved above the formatter the bind is never attached, so it stayed green. It now also checks verify(factory, never()).getConnectionAsync().

Checks:

• opendj-rest2ldap: 571 tests pass (567 + 4 new).
• Four mutants, each turns its test red: the null check removed (Basic abc, unpadded row), the trim removed (dn: %s row), getConnectionAsync() moved above formatBindDn in SimpleBindStrategy (NeverWantedButInvoked) and above the formatter in SaslPlainStrategy (same).
• Javadoc doclint passes for opendj-rest2ldap.

[maximthomas]

praise: The new commit closes all three round-1 items, and each one has a test that pins it.

• CredentialExtractors returns null when Base64.decode returns null, so Basic abc means "no credentials" rather than a NullPointerException. The invalidBasicHeaders provider pins it with Basic abc and an unpadded row.
• SimpleBindStrategyTest.testUserNameWhichIsNotADnTakesNoConnection and the SaslPlainStrategyTest non-DN case both verify(..., never()).getConnectionAsync(), so Utils.formatBindDn failing before a connection is taken is now pinned.
• SaslPlainStrategy trims the template after dn:, so dn: {username} reads like the OAuth2 dn: {dn}.