fix(credentials): threadlocal credentials are cleared before request …

…processing Signed-off-by: Andrew Azores <aazores@redhat.com>
cryostatio · Mar 14, 2023 · 79738cc · 79738cc
1 parent d675fef
commit 79738cc
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 3 deletions.
diff --git a/src/main/java/io/cryostat/net/web/http/AbstractAuthenticatedRequestHandler.java b/src/main/java/io/cryostat/net/web/http/AbstractAuthenticatedRequestHandler.java
@@ -67,6 +67,7 @@
 import io.vertx.core.http.HttpServerRequest;
 import io.vertx.ext.web.RoutingContext;
 import io.vertx.ext.web.handler.HttpException;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.exception.ExceptionUtils;
 
 public abstract class AbstractAuthenticatedRequestHandler implements RequestHandler {
@@ -120,6 +121,10 @@ protected Future<Boolean> validateRequestAuthorization(HttpServerRequest req) th
 
     protected ConnectionDescriptor getConnectionDescriptorFromContext(RoutingContext ctx) {
         String targetId = ctx.pathParam("targetId");
+        if (StringUtils.isBlank(targetId)) {
+            throw new HttpException(404);
+        }
+        credentialsManager.setSessionCredentials(targetId, null);
         try {
             Credentials credentials;
             if (ctx.request().headers().contains(JMX_AUTHORIZATION_HEADER)) {
@@ -157,8 +162,14 @@ protected ConnectionDescriptor getConnectionDescriptorFromContext(RoutingContext
                 }
                 credentials = new Credentials(parts[0], parts[1]);
                 credentialsManager.setSessionCredentials(targetId, credentials);
-                ctx.addEndHandler(
-                        unused -> credentialsManager.setSessionCredentials(targetId, null));
+                // use the headers end handler to clear the ThreadLocal session credentials since
+                // this handler will run synchronously in the context thread, ensuring the
+                // ThreadLocal is cleared on time and not left dangling until an async cleanup
+                ctx.response()
+                        .headersEndHandler(
+                                unused -> credentialsManager.setSessionCredentials(targetId, null));
+                // ctx.addEndHandler(
+                //         unused -> credentialsManager.setSessionCredentials(targetId, null));
             } else {
                 credentials = credentialsManager.getCredentialsByTargetId(targetId);
             }

diff --git a/src/main/java/io/cryostat/net/web/http/api/v2/AbstractV2RequestHandler.java b/src/main/java/io/cryostat/net/web/http/api/v2/AbstractV2RequestHandler.java
@@ -96,7 +96,7 @@ public final void handle(RoutingContext ctx) {
         RequestParameters requestParams = RequestParameters.from(ctx);
         String targetId = requestParams.getPathParams().get("targetId");
         if (targetId != null) {
-            ctx.addEndHandler(unused -> credentialsManager.setSessionCredentials(targetId, null));
+            credentialsManager.setSessionCredentials(targetId, null);
         }
         try {
             if (requiresAuthentication()) {
@@ -110,6 +110,9 @@ public final void handle(RoutingContext ctx) {
                 }
             }
             writeResponse(ctx, handle(requestParams));
+            if (targetId != null) {
+                credentialsManager.setSessionCredentials(targetId, null);
+            }
         } catch (ApiException | HttpException e) {
             throw e;
         } catch (Exception e) {

diff --git a/src/main/java/io/cryostat/net/web/http/api/v2/graph/AbstractPermissionedDataFetcher.java b/src/main/java/io/cryostat/net/web/http/api/v2/graph/AbstractPermissionedDataFetcher.java
@@ -98,6 +98,7 @@ public final T get(DataFetchingEnvironment environment) throws Exception {
     // targetId to credentials
     protected Optional<Credentials> getSessionCredentials(
             DataFetchingEnvironment environment, String targetId) {
+        credentialsManager.setSessionCredentials(targetId, null);
         RoutingContext ctx = GraphQLHandler.getRoutingContext(environment.getGraphQlContext());
         if (!ctx.request()
                 .headers()

diff --git a/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingDeleteHandlerTest.java b/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingDeleteHandlerTest.java
@@ -130,6 +130,7 @@ void shouldHandleDeletedRecording() throws Exception {
     @Test
     void shouldHandleRecordingNotFound() throws Exception {
         Mockito.when(ctx.pathParam("recordingName")).thenReturn("someRecording");
+        Mockito.when(ctx.pathParam("targetId")).thenReturn("someTarget");
         Mockito.when(ctx.request()).thenReturn(req);
         Mockito.when(ctx.request().headers()).thenReturn(MultiMap.caseInsensitiveMultiMap());