Merge pull request #5133 from gchq/gh-5132-empty-session-list

at055612 · web-flow · commit 62e5f4b1d24f · 2025-09-19T20:26:51.000+01:00
PR for #5132 - /stroom/sessionList is empty on 7.10.2
diff --git a/stroom-dropwizard-common/src/main/java/stroom/dropwizard/common/AuthenticationBypassCheckerImpl.java b/stroom-dropwizard-common/src/main/java/stroom/dropwizard/common/AuthenticationBypassCheckerImpl.java
@@ -33,7 +33,9 @@ void registerUnauthenticatedApiPath(final String path) {
     }
 
     @Override
-    public boolean isUnauthenticated(final String servletName, final String servletPath, final String fullPath) {
+    public boolean isUnauthenticated(final String servletName,
+                                     final String servletPath,
+                                     final String fullPath) {
         if (servletPath == null) {
             return false;
         } else {
@@ -49,8 +51,8 @@ public boolean isUnauthenticated(final String servletName, final String servletP
                 canBypassAuth = unauthenticatedServletNames.contains(servletName);
             }
 
-            LOGGER.debug("servletName: {}, servletPath: {}, canBypassAuth: {}",
-                    servletName, servletPath, canBypassAuth);
+            LOGGER.debug("isUnauthenticated() - servletName: {}, servletPath: {}, fullPath: {}, canBypassAuth: {}",
+                    servletName, servletPath, fullPath, canBypassAuth);
             return canBypassAuth;
         }
     }
diff --git a/stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/HasJwtClaims.java b/stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/HasJwtClaims.java
@@ -1,13 +1,15 @@
 package stroom.security.common.impl;
 
+import stroom.util.authentication.HasExpiry;
 import stroom.util.exception.ThrowingFunction;
 import stroom.util.shared.NullSafe;
 
 import org.jose4j.jwt.JwtClaims;
 
+import java.time.Instant;
 import java.util.Optional;
 
-public interface HasJwtClaims {
+public interface HasJwtClaims extends HasExpiry {
 
     JwtClaims getJwtClaims();
 
@@ -24,4 +26,12 @@ default <T> Optional<T> getClaimValue(final String claim, final Class<T> clazz)
                 ThrowingFunction.unchecked(jwtClaims ->
                         jwtClaims.getClaimValue(claim, clazz)));
     }
+
+    default Instant getExpireTime() {
+        return NullSafe.getOrElse(
+                getJwtClaims(),
+                ThrowingFunction.unchecked(JwtClaims::getExpirationTime),
+                numericDate -> Instant.ofEpochMilli(numericDate.getValueInMillis()),
+                Instant.MAX);
+    }
 }
diff --git a/stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/JwtContextFactory.java b/stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/JwtContextFactory.java
@@ -29,4 +29,5 @@ public interface JwtContextFactory {
      * like audience and subject only if doVerification is true.
      */
     Optional<JwtContext> getJwtContext(final String jwt, final boolean doVerification);
+
 }
diff --git a/stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/UpdatableToken.java b/stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/UpdatableToken.java
@@ -92,8 +92,9 @@ public String getJwt() {
     /**
      * @return The time the token will expire (with a small buffer before the actual expiry included)
      */
+    @Override
     public Instant getExpireTime() {
-        return Instant.ofEpochMilli(mutableState.expireTimeWithBufferEpochMs);
+        return Instant.ofEpochMilli(mutableState.expireTimeEpochMs);
     }
 
     @Override
@@ -199,7 +200,8 @@ public String toString() {
                ", preferredUsername=" + NullSafe.get(mutableState.jwtClaims, claims ->
                 JwtUtil.getClaimValue(claims, OpenId.CLAIM__PREFERRED_USERNAME).orElse(null)) +
                ", expireTimeWithBuffer=" + Instant.ofEpochMilli(mutableState.expireTimeWithBufferEpochMs) +
-               ", timeTilExpire=" + Duration.between(Instant.now(), getExpireTime()) +
+               ", timeTilExpire=" + Duration.between(Instant.now(), Instant.ofEpochMilli(
+                mutableState.expireTimeWithBufferEpochMs())) +
                ", session=" + SessionUtil.getSessionId(session) +
                '}';
     }
diff --git a/stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/UserIdentitySessionUtil.java b/stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/UserIdentitySessionUtil.java
@@ -9,6 +9,7 @@
 
 import jakarta.servlet.http.HttpSession;
 
+import java.util.Objects;
 import java.util.Optional;
 
 public final class UserIdentitySessionUtil {
@@ -23,15 +24,16 @@ private UserIdentitySessionUtil() {
     /**
      * Set the {@link UserIdentity} on the session
      */
-    public static void set(final HttpSession session, final UserIdentity userIdentity) {
+    public static void setUserInSession(final HttpSession session, final UserIdentity userIdentity) {
+        Objects.requireNonNull(session);
         LOGGER.debug(() -> LogUtil.message("Setting userIdentity of type {} in session {}, userIdentity: {}",
                 LogUtil.getSimpleClassName(userIdentity),
                 NullSafe.get(session, HttpSession::getId),
                 userIdentity));
         session.setAttribute(SESSION_USER_IDENTITY, userIdentity);
     }
 
-    public static Optional<UserIdentity> get(final HttpSession session) {
+    public static Optional<UserIdentity> getUserFromSession(final HttpSession session) {
         final Optional<UserIdentity> optUserIdentity = Optional.ofNullable(session)
                 .map(session2 ->
                         (UserIdentity) session2.getAttribute(SESSION_USER_IDENTITY));
diff --git a/stroom-security/stroom-security-common-impl/src/test/java/stroom/security/common/impl/TestRefreshManager.java b/stroom-security/stroom-security-common-impl/src/test/java/stroom/security/common/impl/TestRefreshManager.java
@@ -148,6 +148,11 @@ public boolean refresh(final Consumer<Refreshable> onRefreshAction) {
             return true;
         }
 
+        @Override
+        public Instant getExpireTime() {
+            return expiry;
+        }
+
         @Override
         public long getExpireTimeEpochMs() {
             return expiry.toEpochMilli();
diff --git a/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/SecurityFilter.java b/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/SecurityFilter.java
@@ -23,6 +23,7 @@
 import stroom.security.common.impl.UserIdentitySessionUtil;
 import stroom.security.impl.OpenIdManager.RedirectUrl;
 import stroom.security.openid.api.OpenId;
+import stroom.util.authentication.HasExpiry;
 import stroom.util.logging.LambdaLogger;
 import stroom.util.logging.LambdaLoggerFactory;
 import stroom.util.logging.LogUtil;
@@ -136,25 +137,42 @@ private void filter(final HttpServletRequest request,
             chain.doFilter(request, response);
         } else if (isStaticResource(fullPath, servletPath, servletName)) {
             chain.doFilter(request, response);
+        } else if (shouldBypassAuthentication(request, fullPath, servletPath, servletName)) {
+            LOGGER.debug("Running as proc user for unauthenticated resource, servletName: {}, " +
+                         "fullPath: {}, servletPath: {}", servletName, fullPath, servletPath);
+            // Some paths don't need authentication. If that is the case then proceed as proc user.
+            securityContext.asProcessingUser(() ->
+                    process(request, response, chain));
         } else {
-            // Api requests that are not from the front-end should have a token.
-            // Also request from an AWS ALB will have an ALB signed token containing the claims
-            // Need to do this first, so we get a fresh token from AWS ALB rather than using a stale
-            // one from session.
-            Optional<UserIdentity> optUserIdentity = openIdManager.loginWithRequestToken(request);
-
-            // Log current user.
-            if (LOGGER.isDebugEnabled()) {
-                logUserIdentityToDebug(
-                        optUserIdentity, fullPath, "after trying to login with request token");
-            }
+            // First see if a previous call has placed a userIdentity in session
+            Optional<UserIdentity> optUserIdentity = UserIdentitySessionUtil.getUserFromSession(
+                    SessionUtil.getExistingSession(request));
+            logUserIdentityToDebug(optUserIdentity, fullPath, servletPath, "from session");
+
+            // Check if the underlying claims/token have expired. The expiry time of some impls
+            // may get refreshed over time, so we may never hit it. When code flow is handled by
+            // AWS ALB we will expire, so will just get the latest token from headers which the
+            // ALB will be refreshing.
+            optUserIdentity = optUserIdentity.map(userIdentity -> {
+                if (userIdentity instanceof final HasExpiry hasExpiry) {
+                    if (hasExpiry.hasExpired()) {
+                        LOGGER.info("UserIdentity {} has expired, expiry: {}",
+                                userIdentityToString(userIdentity), hasExpiry.getExpireTime());
+                        // Clear the identity, so we have to re-acquire it from headers or code flow
+                        return null;
+                    } else {
+                        LOGGER.debug(() -> LogUtil.message("UserIdentity {} expires in {}",
+                                userIdentityToString(userIdentity), hasExpiry.getTimeTilExpired()));
+                    }
+                }
+                return userIdentity;
+            });
 
-            // If no user from header token, see if we have one in session already.
+            // API requests that are not from the front-end should have a token.
+            // Also requests from an AWS ALB will have an ALB signed token containing the claims
             if (optUserIdentity.isEmpty()) {
-                optUserIdentity = UserIdentitySessionUtil.get(SessionUtil.getExistingSession(request));
-                if (LOGGER.isDebugEnabled()) {
-                    logUserIdentityToDebug(optUserIdentity, fullPath, "from session");
-                }
+                optUserIdentity = openIdManager.loginWithRequestToken(request);
+                logUserIdentityToDebug(optUserIdentity, fullPath, servletPath, "from request token");
             }
 
             if (optUserIdentity.isPresent()) {
@@ -163,19 +181,20 @@ private void filter(final HttpServletRequest request,
                 // Now we have the session make note of the user-agent for logging and sessionListServlet duties
                 UserAgentSessionUtil.setUserAgentInSession(request);
 
+                // If OIDC code flow has been handled by the AWS ALB then the session won't have been
+                // created by our code flow code. Thus, ensure we have a session with the user in it
+                if (isStroomUIServlet(servletName)) {
+                    SessionUtil.getOrCreateSession(request, aSession -> {
+                        LOGGER.info("Creating session {} for user {}, fullPath: {}, servlet: {}",
+                                aSession.getId(), userIdentity, fullPath, servletName);
+                        UserIdentitySessionUtil.setUserInSession(aSession, userIdentity);
+                    });
+                }
+
                 // Now handle the request as this user
                 securityContext.asUser(userIdentity, () ->
                         process(request, response, chain));
-
-            } else if (shouldBypassAuthentication(request, fullPath, servletPath, servletName)) {
-                LOGGER.debug("Running as proc user for unauthenticated servletName: {}, " +
-                             "fullPath: {}, servletPath: {}", servletName, fullPath, servletPath);
-                // Some paths don't need authentication. If that is the case then proceed as proc user.
-                securityContext.asProcessingUser(() ->
-                        process(request, response, chain));
-
-//            } else if (isApiRequest(servletPath)) {
-            } else if (Objects.equals(ResourcePaths.STROOM_SERVLET_NAME, servletName)) {
+            } else if (isStroomUIServlet(servletName)) {
                 doOpenIdFlow(request, response, fullPath);
             } else {
                 // If we couldn't log in with a token or couldn't get a token then error as this is an API call
@@ -187,6 +206,11 @@ private void filter(final HttpServletRequest request,
         }
     }
 
+    private boolean isStroomUIServlet(final String servletName) {
+        return Objects.equals(ResourcePaths.STROOM_SERVLET_NAME, servletName)
+               || Objects.equals(ResourcePaths.SESSION_LIST_SERVLET_NAME, servletName);
+    }
+
     private void doOpenIdFlow(final HttpServletRequest request,
                               final HttpServletResponse response,
                               final String fullPath) throws IOException {
@@ -245,20 +269,27 @@ private void doOpenIdFlow(final HttpServletRequest request,
     @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
     private void logUserIdentityToDebug(final Optional<UserIdentity> optUserIdentity,
                                         final String fullPath,
+                                        final String servletName,
                                         final String msg) {
-        LOGGER.debug("User identity ({}): {} path: {}",
+        LOGGER.debug(() -> LogUtil.message("User identity ({}): {}, fullPath: {}, servletName: {}",
                 msg,
-                optUserIdentity.map(
-                                identity -> {
-                                    final String id = identity.getDisplayName() != null
-                                            ? identity.getSubjectId() + " (" + identity.getDisplayName() + ")"
-                                            : identity.getSubjectId();
-                                    return LogUtil.message("'{}' {}",
-                                            id,
-                                            identity.getClass().getSimpleName());
-                                })
+                optUserIdentity.map(this::userIdentityToString)
                         .orElse("<empty>"),
-                fullPath);
+                fullPath,
+                servletName));
+    }
+
+    private String userIdentityToString(final UserIdentity userIdentity) {
+        if (userIdentity == null) {
+            return "";
+        } else {
+            final String id = userIdentity.getDisplayName() != null
+                    ? userIdentity.getSubjectId() + " (" + userIdentity.getDisplayName() + ")"
+                    : userIdentity.getSubjectId();
+            return LogUtil.message("'{}' {}",
+                    id,
+                    userIdentity.getClass().getSimpleName());
+        }
     }
 
     private String getPostAuthRedirectUri(final HttpServletRequest request) {
@@ -298,17 +329,6 @@ private boolean shouldBypassAuthentication(final HttpServletRequest servletReque
         } else {
             shouldBypass = authenticationBypassChecker.isUnauthenticated(servletName, servletPath, fullPath);
         }
-
-        if (LOGGER.isDebugEnabled()) {
-            if (shouldBypass) {
-                LOGGER.debug("Bypassing authentication for servletName: {}, fullPath: {}, servletPath: {}",
-                        NullSafe.get(
-                                servletRequest.getHttpServletMapping(),
-                                HttpServletMapping::getServletName),
-                        fullPath,
-                        servletPath);
-            }
-        }
         return shouldBypass;
     }
 
diff --git a/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/SessionListListener.java b/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/SessionListListener.java
@@ -100,7 +100,7 @@ public void sessionDestroyed(final HttpSessionEvent event) {
         try {
             // In case the session has been destroyed due to it expiring rather than an explicit
             // logout, we need to stop the refresh manager from trying to refresh tokens.
-            UserIdentitySessionUtil.get(httpSession)
+            UserIdentitySessionUtil.getUserFromSession(httpSession)
                     .ifPresent(stroomUserIdentityFactory::logoutUser);
 
             final UserRef userRef = getUserRefFromSession(httpSession);
@@ -205,7 +205,7 @@ private SessionListResponse listSessionsOnThisNode() {
     }
 
     private static UserRef getUserRefFromSession(final HttpSession httpSession) {
-        return UserIdentitySessionUtil.get(httpSession)
+        return UserIdentitySessionUtil.getUserFromSession(httpSession)
                 .filter(uid -> uid instanceof HasUserRef)
                 .map(uid -> ((HasUserRef) uid).getUserRef())
                 .orElse(null);
diff --git a/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/SessionListServlet.java b/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/SessionListServlet.java
@@ -167,4 +167,9 @@ private void writeCell(final Writer writer, final String value) throws IOExcepti
     public Set<String> getPathSpecs() {
         return PATH_SPECS;
     }
+
+    @Override
+    public String getName() {
+        return ResourcePaths.SESSION_LIST_SERVLET_NAME;
+    }
 }
diff --git a/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/SessionResourceImpl.java b/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/SessionResourceImpl.java
@@ -54,7 +54,7 @@ public UrlResponse logout(final String redirectUri) {
         // Get the session.
         final HttpSession session = SessionUtil.getExistingSession(request);
         if (session != null) {
-            final UserIdentity userIdentity = UserIdentitySessionUtil.get(session)
+            final UserIdentity userIdentity = UserIdentitySessionUtil.getUserFromSession(session)
                     .orElse(null);
             LOGGER.info(() -> LogUtil.message(
                     "logout() - Logout called for {}, userIdentity: {} {} ({}), session: {}, redirectUri: {}",
@@ -70,7 +70,7 @@ public UrlResponse logout(final String redirectUri) {
                 // Create an event for logout
                 authenticationEventLogProvider.get().logoff(userIdentity.getSubjectId());
                 // Remove the user identity from the current session.
-                UserIdentitySessionUtil.set(session, null);
+                UserIdentitySessionUtil.setUserInSession(session, null);
             }
             session.invalidate();
         } else {
diff --git a/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/StroomUserIdentityFactory.java b/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/StroomUserIdentityFactory.java
@@ -357,7 +357,7 @@ private UserIdentity createAuthFlowUserIdentity(final JwtClaims jwtClaims,
 
             // We have authenticated so set the userIdentity in the session for future requests
             // to retrieve it from
-            UserIdentitySessionUtil.set(session, userIdentity);
+            UserIdentitySessionUtil.setUserInSession(session, userIdentity);
 
             // Register the token with the refresh manager so it gets refreshed periodically.
             updatableToken.setUserIdentity(userIdentity);
diff --git a/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/UserIdentityImpl.java b/stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/UserIdentityImpl.java
@@ -6,6 +6,7 @@
 import stroom.security.common.impl.UpdatableToken;
 import stroom.security.common.impl.UserIdentitySessionUtil;
 import stroom.security.shared.HasUserRef;
+import stroom.util.authentication.HasExpiry;
 import stroom.util.authentication.HasRefreshable;
 import stroom.util.logging.LambdaLogger;
 import stroom.util.logging.LambdaLoggerFactory;
@@ -15,11 +16,12 @@
 
 import jakarta.servlet.http.HttpSession;
 
+import java.time.Instant;
 import java.util.Objects;
 import java.util.Optional;
 
 public class UserIdentityImpl
-        implements UserIdentity, HasSession, HasUserRef, HasJwt, HasRefreshable {
+        implements UserIdentity, HasSession, HasUserRef, HasJwt, HasRefreshable, HasExpiry {
 
     private static final LambdaLogger LOGGER = LambdaLoggerFactory.getLogger(UserIdentityImpl.class);
 
@@ -87,7 +89,7 @@ public void invalidateSession() {
      */
     public void removeUserFromSession() {
         if (httpSession != null) {
-            UserIdentitySessionUtil.set(httpSession, null);
+            UserIdentitySessionUtil.setUserInSession(httpSession, null);
         }
     }
 
@@ -114,7 +116,7 @@ public boolean isInSession() {
             final Optional<UserIdentity> optUserIdentity;
 
             try {
-                optUserIdentity = UserIdentitySessionUtil.get(httpSession);
+                optUserIdentity = UserIdentitySessionUtil.getUserFromSession(httpSession);
             } catch (final Exception e) {
                 LOGGER.debug(() -> LogUtil.message(
                         "Error getting identity from session, likely due to it being removed at logout: {}",
@@ -143,6 +145,11 @@ public UpdatableToken getRefreshable() {
         return updatableToken;
     }
 
+    @Override
+    public Instant getExpireTime() {
+        return updatableToken.getExpireTime();
+    }
+
     @Override
     public boolean equals(final Object o) {
         if (this == o) {
diff --git a/stroom-util-shared/src/main/java/stroom/util/shared/ResourcePaths.java b/stroom-util-shared/src/main/java/stroom/util/shared/ResourcePaths.java
@@ -61,6 +61,7 @@ public interface ResourcePaths {
     String UI_SERVLET_NAME = "UI";
     String STROOM_SERVLET_NAME = "StroomServlet";
     String SIGN_IN_SERVLET_NAME = "SignInServlet";
+    String SESSION_LIST_SERVLET_NAME = "SessionListServlet";
 
 
     static String addLegacyUnauthenticatedServletPrefix(final String... parts) {
diff --git a/stroom-util/src/main/java/stroom/util/authentication/HasExpiry.java b/stroom-util/src/main/java/stroom/util/authentication/HasExpiry.java
diff --git a/stroom-util/src/main/java/stroom/util/authentication/PerishableItem.java b/stroom-util/src/main/java/stroom/util/authentication/PerishableItem.java
diff --git a/stroom-util/src/main/java/stroom/util/authentication/Refreshable.java b/stroom-util/src/main/java/stroom/util/authentication/Refreshable.java
diff --git a/stroom-util/src/main/java/stroom/util/authentication/RefreshableItem.java b/stroom-util/src/main/java/stroom/util/authentication/RefreshableItem.java
diff --git a/stroom-util/src/main/java/stroom/util/servlet/SessionUtil.java b/stroom-util/src/main/java/stroom/util/servlet/SessionUtil.java
diff --git a/unreleased_changes/20250919_133650_510__5132.md b/unreleased_changes/20250919_133650_510__5132.md

Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,9 @@ void registerUnauthenticatedApiPath(final String path) {`
`33`	`33`	`}`
`34`	`34`
`35`	`35`	`@Override`
`36`		`- public boolean isUnauthenticated(final String servletName, final String servletPath, final String fullPath) {`
	`36`	`+ public boolean isUnauthenticated(final String servletName,`
	`37`	`+ final String servletPath,`
	`38`	`+ final String fullPath) {`
`37`	`39`	`if (servletPath == null) {`
`38`	`40`	`return false;`
`39`	`41`	`} else {`
`@@ -49,8 +51,8 @@ public boolean isUnauthenticated(final String servletName, final String servletP`
`49`	`51`	`canBypassAuth = unauthenticatedServletNames.contains(servletName);`
`50`	`52`	`}`
`51`	`53`
`52`		`- LOGGER.debug("servletName: {}, servletPath: {}, canBypassAuth: {}",`
`53`		`- servletName, servletPath, canBypassAuth);`
	`54`	`+ LOGGER.debug("isUnauthenticated() - servletName: {}, servletPath: {}, fullPath: {}, canBypassAuth: {}",`
	`55`	`+ servletName, servletPath, fullPath, canBypassAuth);`
`54`	`56`	`return canBypassAuth;`
`55`	`57`	`}`
`56`	`58`	`}`
Original file line number	Diff line number	Diff line change
`@@ -29,4 +29,5 @@ public interface JwtContextFactory {`
`29`	`29`	`* like audience and subject only if doVerification is true.`
`30`	`30`	`*/`
`31`	`31`	`Optional<JwtContext> getJwtContext(final String jwt, final boolean doVerification);`
	`32`	`+`
`32`	`33`	`}`
Original file line number	Diff line number	Diff line change
`@@ -167,4 +167,9 @@ private void writeCell(final Writer writer, final String value) throws IOExcepti`
`167`	`167`	`public Set<String> getPathSpecs() {`
`168`	`168`	`return PATH_SPECS;`
`169`	`169`	`}`
	`170`	`+`
	`171`	`+ @Override`
	`172`	`+ public String getName() {`
	`173`	`+ return ResourcePaths.SESSION_LIST_SERVLET_NAME;`
	`174`	`+ }`
`170`	`175`	`}`