From 857c966db460df76c426946b54074d004b04ef4c Mon Sep 17 00:00:00 2001
From: Matthew Swift <matthew.swift@forgerock.com>
Date: Tue, 09 Apr 2013 10:03:04 +0000
Subject: [PATCH] Fix for OPENDJ-856: Make Rest2LDAP close the cached authenticated connection.
---
opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Context.java | 49 ++++++++++--------------
opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Utils.java | 26 ++++++++++--
opendj3/opendj-rest2ldap-servlet/src/main/java/org/forgerock/opendj/rest2ldap/servlet/Rest2LDAPAuthnFilter.java | 10 ++++-
3 files changed, 50 insertions(+), 35 deletions(-)
diff --git a/opendj3/opendj-rest2ldap-servlet/src/main/java/org/forgerock/opendj/rest2ldap/servlet/Rest2LDAPAuthnFilter.java b/opendj3/opendj-rest2ldap-servlet/src/main/java/org/forgerock/opendj/rest2ldap/servlet/Rest2LDAPAuthnFilter.java
index f7d936d..ae8b537 100644
--- a/opendj3/opendj-rest2ldap-servlet/src/main/java/org/forgerock/opendj/rest2ldap/servlet/Rest2LDAPAuthnFilter.java
+++ b/opendj3/opendj-rest2ldap-servlet/src/main/java/org/forgerock/opendj/rest2ldap/servlet/Rest2LDAPAuthnFilter.java
@@ -56,6 +56,7 @@
import org.forgerock.opendj.ldap.ByteString;
import org.forgerock.opendj.ldap.Connection;
import org.forgerock.opendj.ldap.ConnectionFactory;
+import org.forgerock.opendj.ldap.Connections;
import org.forgerock.opendj.ldap.DN;
import org.forgerock.opendj.ldap.EntryNotFoundException;
import org.forgerock.opendj.ldap.ErrorResultException;
@@ -430,9 +431,14 @@
@Override
public void handleResult(final BindResult result) {
- // Cache the pre-authenticated connection.
+ /*
+ * Cache the pre-authenticated connection and prevent
+ * downstream components from closing it since this
+ * filter will close it.
+ */
if (reuseAuthenticatedConnection) {
- request.setAttribute(ATTRIBUTE_AUTHN_CONNECTION, connection);
+ request.setAttribute(ATTRIBUTE_AUTHN_CONNECTION, Connections
+ .uncloseable(connection));
}
// Pass through the authentication ID and authorization principals.
diff --git a/opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Context.java b/opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Context.java
index ba92a5b..eec4cc6 100644
--- a/opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Context.java
+++ b/opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Context.java
@@ -17,13 +17,13 @@
import static org.forgerock.opendj.ldap.ErrorResultException.newErrorResult;
import static org.forgerock.opendj.rest2ldap.Rest2LDAP.asResourceException;
+import static org.forgerock.opendj.rest2ldap.Utils.i18n;
import java.io.Closeable;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.atomic.AtomicReference;
import org.forgerock.json.resource.InternalServerErrorException;
import org.forgerock.json.resource.ResourceException;
@@ -121,7 +121,10 @@
}
FutureResult<Result> getFutureResult() {
- // Perform uninterrupted wait since this method is unlikely to block for a long time.
+ /*
+ * Perform uninterrupted wait since this method is unlikely to block
+ * for a long time.
+ */
boolean wasInterrupted = false;
while (true) {
try {
@@ -197,23 +200,25 @@
};
private final Config config;
- private final AtomicReference<Connection> connection = new AtomicReference<Connection>();
private final ServerContext context;
- private final Connection preAuthenticatedConnection;
+ private Connection connection;
private Control proxiedAuthzControl = null;
Context(final Config config, final ServerContext context) {
this.config = config;
this.context = context;
- // Re-use the pre-authenticated connection if available and the authorization policy allows.
+ /*
+ * Re-use the pre-authenticated connection if available and the
+ * authorization policy allows.
+ */
if (config.getAuthorizationPolicy() != AuthorizationPolicy.NONE
&& context.containsContext(AuthenticatedConnectionContext.class)) {
final Connection connection =
context.asContext(AuthenticatedConnectionContext.class).getConnection();
- this.preAuthenticatedConnection = connection != null ? wrap(connection) : null;
+ this.connection = wrap(connection);
} else {
- this.preAuthenticatedConnection = null;
+ this.connection = null; // We'll allocate the connection.
}
}
@@ -222,15 +227,7 @@
*/
@Override
public void close() {
- /*
- * Only release the connection that we acquired. Don't release the
- * cached connection since that is the responsibility of the component
- * which acquired it.
- */
- final Connection c = connection.getAndSet(null);
- if (c != null) {
- c.close();
- }
+ connection.close();
}
Config getConfig() {
@@ -238,7 +235,7 @@
}
Connection getConnection() {
- return preAuthenticatedConnection != null ? preAuthenticatedConnection : connection.get();
+ return connection;
}
ServerContext getServerContext() {
@@ -268,8 +265,7 @@
* cached connection since cached connections are supposed to have been
* pre-authenticated and therefore do not require proxied authorization.
*/
- if (preAuthenticatedConnection == null
- && config.getAuthorizationPolicy() == AuthorizationPolicy.PROXY) {
+ if (connection == null && config.getAuthorizationPolicy() == AuthorizationPolicy.PROXY) {
if (context.containsContext(SecurityContext.class)) {
try {
final SecurityContext securityContext =
@@ -283,9 +279,9 @@
return;
}
} else {
- // FIXME: i18n.
handler.handleError(new InternalServerErrorException(
- "The request could not be authorized because it did not contain a security context"));
+ i18n("The request could not be authorized because it did "
+ + "not contain a security context")));
return;
}
}
@@ -295,7 +291,7 @@
* requests. A null factory indicates that Rest2LDAP has been configured
* to re-use the LDAP connection which was used for authentication.
*/
- if (preAuthenticatedConnection != null) {
+ if (connection != null) {
// Invoke the handler immediately since a connection is available.
runnable.run();
} else if (config.connectionFactory() != null) {
@@ -307,17 +303,14 @@
@Override
public final void handleResult(final Connection result) {
- if (!connection.compareAndSet(null, wrap(result))) {
- // This should never happen.
- throw new IllegalStateException("LDAP connection obtained multiple times");
- }
+ connection = wrap(result);
runnable.run();
}
});
} else {
- // FIXME: i18n
handler.handleError(new InternalServerErrorException(
- "The request could not be processed because there was no LDAP connection available for use"));
+ i18n("The request could not be processed because there was no LDAP "
+ + "connection available for use")));
}
}
diff --git a/opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Utils.java b/opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Utils.java
index c2d87d2..51a9adb 100644
--- a/opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Utils.java
+++ b/opendj3/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/Utils.java
@@ -57,10 +57,10 @@
* The type of result.
*/
private static final class AccumulatingResultHandler<V> implements ResultHandler<V> {
+ private ResourceException exception; // Guarded by latch.
private final ResultHandler<List<V>> handler;
private final AtomicInteger latch;
private final List<V> results;
- private ResourceException exception; // Guarded by latch.
private AccumulatingResultHandler(final int size, final ResultHandler<List<V>> handler) {
if (size <= 0) {
@@ -88,9 +88,12 @@
}
private void latch() {
- // Invoke the handler once all results have been received. Avoid failing-fast
- // when an error occurs because some in-flight tasks may depend on resources
- // (e.g. connections) which are automatically closed on completion.
+ /*
+ * Invoke the handler once all results have been received. Avoid
+ * failing-fast when an error occurs because some in-flight tasks
+ * may depend on resources (e.g. connections) which are
+ * automatically closed on completion.
+ */
if (latch.decrementAndGet() == 0) {
if (exception != null) {
handler.handleError(exception);
@@ -217,6 +220,19 @@
return a.getAttributeDescription().withoutOption("binary").toString();
}
+ /**
+ * Stub formatter for i18n strings.
+ *
+ * @param format
+ * The format string.
+ * @param args
+ * The string arguments.
+ * @return The formatted string.
+ */
+ static String i18n(final String format, final Object... args) {
+ return String.format(format, args);
+ }
+
static boolean isJSONPrimitive(final Object value) {
return value instanceof String || value instanceof Boolean || value instanceof Number;
}
@@ -322,7 +338,7 @@
public void handleResult(final M result) {
try {
handler.handleResult(f.apply(result, null));
- } catch (Throwable t) {
+ } catch (final Throwable t) {
handler.handleError(asResourceException(t));
}
}
--
Gitblit v1.10.0