[#809] Fix DSML gateway NPE on abandonRequest and on missing Content-Type

[vharseko]

Fixes #809

1. abandonRequest caused an NPE and leaked the LDAP connection

performLDAPRequest() deliberately returns null for an abandon request (no response element is defined for it in DSMLv2), but doPost() guarded the null only when adding to the response list and then dereferenced it unconditionally, so a batch containing <abandonRequest abandonID="1"/> ended in a NullPointerException escaping to the container as a 500.

Since the connection block was not wrapped in try/finally, connection.close(nextMessageID) was skipped as well: org.opends.server.tools.LDAPConnection closes its socket only in close(), so one connection to the directory server was leaked per request — trivially repeatable until the connection handler is exhausted.

The loop now skips the null result, and the connection is closed in a finally and cleared, so a second batchRequest in the same SOAP body no longer reuses a closed connection.

2. NPE when the request has no Content-Type header

messageFactory was assigned only inside the header loop, when a Content-Type header matching SOAP 1.1 or SOAP 1.2 was present. A POST without that header is legal HTTP and left the field at null, which was then dereferenced when parsing the request (NPE → 500) and again on the response path, where it was swallowed by catch (Exception e) { e.printStackTrace(); } — a client whose credentials were rejected got an empty HTTP 200 instead of the error.

A missing or unsupported Content-Type is now answered with a malformedRequest batch response (SOAP 1.1 is used for the reply), which also replaces the ServletException previously thrown for a non-SOAP content type. Failures to send the response are logged instead of being printed to stderr.

Both defects predate the Open Identity Platform fork (imported in 1577c0a).

3. Prerequisite: LDAPConnection never connected its socket

Found while writing the regression test: since #279, LDAPConnection.createSocket() binds the newly created client socket to the target server address instead of connecting to it, so every plain or StartTLS connection made through org.opends.server.tools.LDAPConnection fails with Address already in use (server on the same host) or Cannot assign requested address (remote server). Pure SSL uses createSSLSocket() and is unaffected, and ldapsearch/ldapmodify go through the SDK, which is why this went unnoticed; the DSML gateway, stop-ds, manage-account, the tools built on LDAPConnectionArgumentParser and CryptoManagerImpl do use it. Kept as a separate commit in case it should be tracked as its own issue.

Tests

New DSMLServletTestCase drives doPost() against a fake LDAP endpoint (a real socket that answers the bind and records the message types it receives), with the servlet API stubbed through java.lang.reflect.Proxy — no new test dependency:

• an abandon request is forwarded and the connection is closed (the endpoint sees bind → abandon → unbind);
• a request without Content-Type is answered with malformedRequest, and no connection is opened;
• an unsupported Content-Type likewise;
• a credentials error is still reported when Content-Type is missing.

mvn -pl opendj-dsml-servlet test → Tests run: 64, Failures: 0, Errors: 0. Reverting only DSMLServlet.java makes all four new tests fail with the original NPEs.

[maximthomas]

Both defects are real and the fixes are correct. I reproduced everything locally: with DSMLServlet.java reverted to master a single <abandonRequest/> throws NullPointerException out of doPost and the fake LDAP endpoint sees bind, abandon with no unbind — the leak, exactly as described. With the patch it sees bind, abandon, unbind.

Worth adding to the description: the socket regression only affects callers that reach createSocket(), i.e. plain LDAP or StartTLS. StopDS and ManageAccount install an SSLConnectionFactory unconditionally and CryptoManagerImpl sets setUseSSL(true), so those take createSSLSocket() and are unaffected. Actually affected: the DSML gateway (default web.xml has ldap.usessl=false) and LDAPConnectionArgumentParser tools (import-ldif, export-ldif, backup, restore, rebuild-index, manage-tasks) without --useSSL. Also note BindException is not a ConnectException, so the broken code escaped the per-address catch into the outer catch (Exception ex) — the fix restores failover across multiple A records.

Four things to address before merge.

The new log call is silently dropped (medium)

opendj-dsml-servlet/src/main/java/org/opends/dsml/protocol/DSMLServlet.java

} catch (Exception e) {
  // the client gets an empty response: at least make the cause visible
  Logger.getLogger(PKG_NAME).log(Level.SEVERE, "Unable to send the DSML response", e);
}

LDAPConnection.connectToHost() calls JDKLogging.disableLogging() on every non-verbose connection, which does LogManager.getLogManager().reset(); Logger.getLogger("").setLevel(Level.OFF). After the first LDAP connection attempt in the JVM this record has no handlers and an OFF root level and is never emitted — I confirmed isLoggable(SEVERE)=false, rootLevel=OFF, rootHandlers=0. printStackTrace() at least reached catalina.out, so this is a net loss.

SLF4J is not an alternative here: the war ships slf4j-api plus the i18n bridge but no SLF4JServiceProvider, so it would be a NOP. The working option is the container log — but init(ServletConfig) never calls super.init(config), so getServletContext() NPEs today:

public void init(ServletConfig config) throws ServletException {
  super.init(config);   // currently missing
  ...
}
...
} catch (Exception e) {
  getServletContext().log("Unable to send the DSML response", e);
}

authzid accumulates across batch requests (medium)

connection = null in the finally is right, and it also fixes a second latent bug: previously a 2nd <batchRequest> in the same SOAP body was silently skipped (connection != null left connected == false). I verified this — two empty batch requests open one connection on master and two on this branch.

That makes the following reachable, because connOptions is built once per doPost and addSASLProperty appends to a per-key list:

connOptions.addSASLProperty("authzid=" + batchRequest.authRequest.getPrincipal());

With ldap.authzidtypeisid=true and two batch requests each carrying an <authRequest>, the second bind fails client-side. Reproduced:

<ns0:errorResponse type="couldNotConnect">
  <ns0:message>org.opends.server.tools.LDAPConnectionException:
    The "authzid" SASL property only accepts a single value</ns0:message>
</ns0:errorResponse>

Build LDAPConnectionOptions per batch request, or clear authzid before setting it.

The two commits are not independently mergeable (medium)

testAbandonRequestIsProcessedAndConnectionIsClosed cannot pass without a8616b08a8 — against a ~/.m2 copy of opendj-server-legacy built from master it fails with couldNotConnect: Address already in use (Bind failed). So the socket fix cannot be split off into its own issue, and mvn -pl opendj-dsml-servlet test only reports 64 passing tests when opendj-server-legacy has already been rebuilt from this branch. CI's full-reactor mvn verify is fine; the command in the description is misleading.

The ordering also matters the other way round: with the default ldap.usessl=false the gateway has been unable to connect at all since #279 (every release 4.5.5 through 5.1.2), which is what masked the abandon NPE. Fixing the socket without fixing the NPE would re-enable the gateway straight into the crash — so both belong in this PR.

malformedRequest response drops the requestID (low)

createXMLParsingErrorResponse() deliberately re-parses the buffered stream with SAX to recover the request ID and calls batchResponse.setRequestID(...). The new block builds a bare ErrorResponse, so clients cannot correlate the reply:

ErrorResponse errorResponse = objFactory.createErrorResponse();
errorResponse.setType(MALFORMED_REQUEST);
errorResponse.setMessage("Content-Type does not match SOAP 1.1 or SOAP 1.2");
batchResponses.add(objFactory.createBatchResponseErrorResponse(errorResponse));

Nothing has been read from is at that point and the mark is still valid, so createXMLParsingErrorResponse(is, objFactory, batchResponse, "Content-Type does not match SOAP 1.1 or SOAP 1.2") would keep the two malformed paths consistent.

Nits

• Dead null check: with the finally nulling it, connection is always null at if ( connection == null ), and the method-scope declaration is no longer needed — a loop-local LDAPConnection connection = new LDAPConnection(...) inside the try reads better. Given #790/#793 this will probably be flagged by CodeQL.
• Mockito is already available: org.mockito:mockito-all:1.10.19:test and assertj-core come from the root pom's top-level <dependencies>, so they are already on opendj-dsml-servlet's test classpath — the hand-rolled Proxy stubs are a fair choice, but "no new test dependency" isn't a constraint. Worth a word in the description to pre-empt the question.
• Silent failures in the fake server: FakeLdapServer.serve() ends with e.printStackTrace(), so a server-side error prints and the test still passes. Record the exception and assert it is null in awaitDisconnect().
• defaultValue(Method) primitive gap: returns null for char/byte/short/float/double returns, which would NPE out of the proxy. No call site hits it today; returnType.isPrimitive() catch-all removes the trap.
• Reply SOAP version: a malformed Authorization header breaks out of the header loop, so Content-Type may never be examined and a SOAP 1.2 request gets a SOAP 1.1 reply. continue instead of break avoids it.
• HTTP status: an unsupported Content-Type now yields 200 + malformedRequest instead of the previous ServletException → 500. Better, and consistent with how XML parse errors are already reported, but 415 alongside the body would be more correct HTTP — and deployments alerting on 5xx will see this class of request vanish.
• setReuseAddress(true): harmless, and it now applies to the implicit local bind that connect() performs — the flag survives the connect. Pre-#279 (new Socket(addr, port)) did not set it; either way not a defect. connect() still has no timeout, so connectToHost's timeout parameter never reaches the socket connect — pre-existing, but a candidate follow-up.
• Shared BatchResponse: all batch requests in one SOAP body share a single <batchResponse> whose requestID is overwritten each iteration, so two batches produce one <batchResponse requestID="2"> holding both batches' elements. Pre-existing, but only observable now that the second batch actually runs.
• Coverage gaps: no test for the multi-batchRequest reconnect, for the SOAP 1.2 path, or for an abandon mid-batch letting later operations run; and createSocket() has no test in the module that owns it — a few lines binding a ServerSocket and asserting connectToHost succeeds would pin the #279 regression in opendj-server-legacy.
• Why this went unnoticed: nothing in the repo referenced DSMLServlet from a test before this PR, and although the module wires Cargo/Tomcat into pre-integration-test it has no IT classes — the container starts, deploys and stops without a single request. DSMLServletTestCase is this servlet's first coverage; nice to have it.