From 24904d014c5654416327d8bd4d8d29034cea07a4 Mon Sep 17 00:00:00 2001 From: Binal Patel Date: Tue, 11 Aug 2026 19:56:33 -0600 Subject: [PATCH 1/4] Audit reauthentication as its own event and sharpen its diagnostics --- .../api/security/AuthenticationManager.java | 80 +++++++++++++++++-- .../org/labkey/api/security/UserManager.java | 1 + .../labkey/core/login/LoginController.java | 2 +- 3 files changed, 76 insertions(+), 7 deletions(-) diff --git a/api/src/org/labkey/api/security/AuthenticationManager.java b/api/src/org/labkey/api/security/AuthenticationManager.java index ba60877c838..4f6c7b9a873 100644 --- a/api/src/org/labkey/api/security/AuthenticationManager.java +++ b/api/src/org/labkey/api/security/AuthenticationManager.java @@ -632,6 +632,10 @@ private ModelAndView getReauthView(AuthenticationResponse response, BindExceptio AuthenticationManager.setReauthUser(reauthUser, getUser(), getViewContext().getRequestOrThrow(), errorMessage, url); + // A token on the URL means setReauthUser() accepted the reauthentication. + if (null != reauthUser && null != url.getParameter(REAUTH_TOKEN_NAME)) + AuthenticationManager.auditReauthSuccess(reauthUser, response); + throw new RedirectException(url); } @@ -1091,6 +1095,16 @@ public static void addAuditEvent(@NotNull User user, HttpServletRequest request, public static @NotNull PrimaryAuthenticationResult authenticate(HttpServletRequest request, String id, String password, URLHelper returnUrl, boolean logFailures) throws InvalidEmailException + { + return authenticate(request, id, password, returnUrl, logFailures, false); + } + + + /** + * @param reauth True when reauthenticating an already signed-in user rather than logging one in. See + * {@link #finalizePrimaryAuthentication(HttpServletRequest, AuthenticationResponse, boolean)}. + */ + public static @NotNull PrimaryAuthenticationResult authenticate(HttpServletRequest request, String id, String password, URLHelper returnUrl, boolean logFailures, boolean reauth) throws InvalidEmailException { PrimaryAuthenticationResult result = null; try @@ -1098,7 +1112,7 @@ public static void addAuditEvent(@NotNull User user, HttpServletRequest request, result = _beforeAuthenticate(request, id, password); if (null != result) return result; - result = _authenticate(request, id, password, returnUrl, logFailures); + result = _authenticate(request, id, password, returnUrl, logFailures, reauth); return result; } finally @@ -1108,7 +1122,7 @@ public static void addAuditEvent(@NotNull User user, HttpServletRequest request, } - private static @NotNull PrimaryAuthenticationResult _authenticate(HttpServletRequest request, final String id, String password, URLHelper returnUrl, boolean logFailures) throws InvalidEmailException + private static @NotNull PrimaryAuthenticationResult _authenticate(HttpServletRequest request, final String id, String password, URLHelper returnUrl, boolean logFailures, boolean reauth) throws InvalidEmailException { if (areNotBlank(id, password)) { @@ -1132,7 +1146,7 @@ public static void addAuditEvent(@NotNull User user, HttpServletRequest request, if (authResponse.isAuthenticated()) { - return finalizePrimaryAuthentication(request, authResponse); + return finalizePrimaryAuthentication(request, authResponse, reauth); } else { @@ -1229,6 +1243,18 @@ else if (null != emailAddress) @NotNull public static PrimaryAuthenticationResult finalizePrimaryAuthentication(HttpServletRequest request, AuthenticationResponse response) + { + return finalizePrimaryAuthentication(request, response, false); + } + + /** + * @param reauth True when reauthenticating an already signed-in user rather than logging one in. Suppresses the + * "logged in" audit event: no session is created and the user was already signed in, so recording a + * login misstates what happened. Callers passing true are responsible for recording the + * reauthentication via {@link #auditReauthSuccess(User, AuthenticationResponse)}. + */ + @NotNull + public static PrimaryAuthenticationResult finalizePrimaryAuthentication(HttpServletRequest request, AuthenticationResponse response, boolean reauth) { User user = response.getUser(); final String emailAddress; @@ -1288,7 +1314,8 @@ public static PrimaryAuthenticationResult finalizePrimaryAuthentication(HttpServ return new PrimaryAuthenticationResult(AuthenticationStatus.InactiveUser); } - addAuditEvent(user, request, emailAddress + " " + UserManager.UserAuditEvent.LOGGED_IN + " successfully via " + response.getSuccessDetails() + "."); + if (!reauth) + addAuditEvent(user, request, emailAddress + " " + UserManager.UserAuditEvent.LOGGED_IN + " successfully via " + response.getSuccessDetails() + "."); return new PrimaryAuthenticationResult(user, response); } @@ -1714,6 +1741,11 @@ public URLHelper getRedirectURL() session.removeAttribute(getReauthFlowSessionKey()); URLHelper url = getAfterReauthURL(c, getLoginReturnProperties(request), primaryAuthUser); setReauthUser(primaryAuthUser, reauthFlow.local() ? SecurityManager.getSessionUser(request) : null, request, null, url); + + // A token on the URL means setReauthUser() accepted the reauthentication. + if (null != url.getParameter(REAUTH_TOKEN_NAME)) + auditReauthSuccess(primaryAuthUser, primaryAuthResult.getResponse()); + return new AuthenticationResult(primaryAuthUser, url); } @@ -1884,11 +1916,33 @@ public boolean isExpired() * @param errorMessage Pre-existing error message to add to the URL * @param redirectUrl URL to which the token (on success) or error message (on failure) gets added */ - public static void setReauthUser(User reauthUser, @Nullable User sessionUser, HttpServletRequest request, @Nullable String errorMessage, URLHelper redirectUrl) + public static void setReauthUser(@Nullable User reauthUser, @Nullable User sessionUser, HttpServletRequest request, @Nullable String errorMessage, URLHelper redirectUrl) { if (errorMessage == null && sessionUser != null && !sessionUser.equals(reauthUser)) { - errorMessage = "Reauthentication failed: wrong user reauthenticated"; + // One condition in code, but three different problems in practice -- sign in as the right user, fix the + // IdP's claim mapping, or fix the session cookie -- so each gets a message that says which one it is. + if (sessionUser.isGuest()) + { + // The SSO validate actions are @RequiresNoPermission, so getUser() returns guest whenever the request + // carries no signed-in session -- typically because the session cookie didn't accompany the IdP's + // cross-site POST to the validate action, or because the session timed out mid-flow. + errorMessage = "Reauthentication failed: this browser is no longer signed in; please sign in again"; + _log.warn("Reauthentication failed for \"{}\": the request carried no signed-in session. Check that the session cookie accompanies the identity provider's response to the validate action -- a JSESSIONID with no explicit SameSite value is withheld from that cross-site POST once it is more than a couple of minutes old.", null != reauthUser ? reauthUser.getEmail() : "an unrecognized identity"); + } + else if (null == reauthUser) + // Narrow, but sign-in and reauthentication resolve users differently: finalizePrimaryAuthentication() + // can auto-create an account, and reauthentication never does. Reaching here means the asserted + // identity has no account by the time reauth runs -- deleted or renamed mid-session, or the IdP + // asserting a different identifier than it did at sign-in. + errorMessage = "Reauthentication failed: the reauthenticated identity does not match a LabKey user account"; + else + { + errorMessage = "Reauthentication failed: wrong user reauthenticated"; + // The only place that knows both identities. The audit log records neither, since no reauthentication + // completed, so without this the pairing can't be reconstructed afterward. + _log.warn("Reauthentication failed for \"{}\": \"{}\" reauthenticated instead.", sessionUser.getEmail(), reauthUser.getEmail()); + } } if (errorMessage != null) @@ -1905,6 +1959,20 @@ public static void setReauthUser(User reauthUser, @Nullable User sessionUser, Ht } } + /** + * Records that a reauthentication happened; 21 CFR Part 11 wants proof of it. SSO reauth never reaches + * finalizePrimaryAuthentication(), so it left no server-side record at all, and local and signing reauth recorded + * themselves as logins. Mirrors the "logged in" event's phrasing so the two read alike in the audit log. + */ + public static void auditReauthSuccess(@NotNull User reauthUser, @NotNull AuthenticationResponse response) + { + // Deliberately not addAuditEvent(), which throttles repeats of an identical message from the same user and + // address. Signing several records in a row produces exactly that, and dropping the second signature's + // reauthentication is the opposite of what an audit trail is for. + UserManager.addAuditEvent(reauthUser, ContainerManager.getRoot(), reauthUser, + reauthUser.getEmail() + " " + UserManager.UserAuditEvent.REAUTHENTICATED + " successfully via " + response.getSuccessDetails() + "."); + } + // Separate method to allow unit testing private static void addToken(HttpServletRequest request, User reauthUser, String reauthToken, Instant expiration) { diff --git a/api/src/org/labkey/api/security/UserManager.java b/api/src/org/labkey/api/security/UserManager.java index 4ef4a087d6c..acfe7ce9004 100644 --- a/api/src/org/labkey/api/security/UserManager.java +++ b/api/src/org/labkey/api/security/UserManager.java @@ -1110,6 +1110,7 @@ public static class UserAuditEvent extends AuditTypeEvent public static final String LOGGED_IN = "logged in"; public static final String LOGGED_OUT = "logged out"; public static final String API_KEY = "an API key"; + public static final String REAUTHENTICATED = "reauthenticated"; int _user; diff --git a/core/src/org/labkey/core/login/LoginController.java b/core/src/org/labkey/core/login/LoginController.java index 9e46a3afd46..52fc4cca58a 100644 --- a/core/src/org/labkey/core/login/LoginController.java +++ b/core/src/org/labkey/core/login/LoginController.java @@ -354,7 +354,7 @@ private static boolean authenticate(LoginForm form, BindException errors, HttpSe { // Attempt authentication with all active form providers String formEmail = form.getEmail(); - PrimaryAuthenticationResult result = AuthenticationManager.authenticate(request, formEmail, form.getPassword(), form.getReturnUrlHelper(), true); + PrimaryAuthenticationResult result = AuthenticationManager.authenticate(request, formEmail, form.getPassword(), form.getReturnUrlHelper(), true, form.isForceReauth()); AuthenticationStatus status = result.getStatus(); if (Success == status) From 7e9803b60df82090a81131f2b6f10349acd94539 Mon Sep 17 00:00:00 2001 From: Binal Patel Date: Tue, 11 Aug 2026 23:25:04 -0600 Subject: [PATCH 2/4] Update comments --- .../labkey/api/security/AuthenticationManager.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/api/src/org/labkey/api/security/AuthenticationManager.java b/api/src/org/labkey/api/security/AuthenticationManager.java index 4f6c7b9a873..7a771b24e06 100644 --- a/api/src/org/labkey/api/security/AuthenticationManager.java +++ b/api/src/org/labkey/api/security/AuthenticationManager.java @@ -1960,15 +1960,15 @@ else if (null == reauthUser) } /** - * Records that a reauthentication happened; 21 CFR Part 11 wants proof of it. SSO reauth never reaches - * finalizePrimaryAuthentication(), so it left no server-side record at all, and local and signing reauth recorded - * themselves as logins. Mirrors the "logged in" event's phrasing so the two read alike in the audit log. + * Records that a reauthentication happened. SSO reauth never reaches finalizePrimaryAuthentication(), so it left + * no server-side record at all, and local and signing reauth recorded themselves as logins. Mirrors the "logged + * in" event's phrasing so the two read alike in the audit log. */ public static void auditReauthSuccess(@NotNull User reauthUser, @NotNull AuthenticationResponse response) { - // Deliberately not addAuditEvent(), which throttles repeats of an identical message from the same user and - // address. Signing several records in a row produces exactly that, and dropping the second signature's - // reauthentication is the opposite of what an audit trail is for. + // Calls UserManager.addAuditEvent() directly rather than this class's addAuditEvent(), which drops a message + // identical to the previous one from the same user and address. Signing several records in a row produces + // exactly those identical messages, and dropping them would leave real reauthentications unrecorded. UserManager.addAuditEvent(reauthUser, ContainerManager.getRoot(), reauthUser, reauthUser.getEmail() + " " + UserManager.UserAuditEvent.REAUTHENTICATED + " successfully via " + response.getSuccessDetails() + "."); } From 85f63afc7ca6d79f26f2ffc90db695a111c636a5 Mon Sep 17 00:00:00 2001 From: Binal Patel Date: Tue, 18 Aug 2026 19:53:32 -0600 Subject: [PATCH 3/4] Update reauthentication failure messages and diagnostics --- .../api/security/AuthenticationManager.java | 39 ++++++++++++------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/api/src/org/labkey/api/security/AuthenticationManager.java b/api/src/org/labkey/api/security/AuthenticationManager.java index 7a771b24e06..763aefd2ce2 100644 --- a/api/src/org/labkey/api/security/AuthenticationManager.java +++ b/api/src/org/labkey/api/security/AuthenticationManager.java @@ -630,7 +630,7 @@ private ModelAndView getReauthView(AuthenticationResponse response, BindExceptio @Nullable User reauthUser = response.isAuthenticated() ? UserManager.getUser(response.getValidEmail()) : null; - AuthenticationManager.setReauthUser(reauthUser, getUser(), getViewContext().getRequestOrThrow(), errorMessage, url); + AuthenticationManager.setReauthUser(reauthUser, response.isAuthenticated() ? response.getValidEmail().getEmailAddress() : null, getUser(), getViewContext().getRequestOrThrow(), errorMessage, url); // A token on the URL means setReauthUser() accepted the reauthentication. if (null != reauthUser && null != url.getParameter(REAUTH_TOKEN_NAME)) @@ -1740,7 +1740,7 @@ public URLHelper getRedirectURL() { session.removeAttribute(getReauthFlowSessionKey()); URLHelper url = getAfterReauthURL(c, getLoginReturnProperties(request), primaryAuthUser); - setReauthUser(primaryAuthUser, reauthFlow.local() ? SecurityManager.getSessionUser(request) : null, request, null, url); + setReauthUser(primaryAuthUser, null != primaryAuthUser ? primaryAuthUser.getEmail() : null, reauthFlow.local() ? SecurityManager.getSessionUser(request) : null, request, null, url); // A token on the URL means setReauthUser() accepted the reauthentication. if (null != url.getParameter(REAUTH_TOKEN_NAME)) @@ -1910,13 +1910,16 @@ public boolean isExpired() public static final String REAUTH_TOKEN_MAP_NAME = "reauthTokenSet"; // Session attribute name for token map /** - * @param reauthUser Re-auth user to stash in session with the re-auth token - * @param sessionUser If not null, validate that this user and reauthUser are the same - * @param request Request from which to retrieve the session - * @param errorMessage Pre-existing error message to add to the URL - * @param redirectUrl URL to which the token (on success) or error message (on failure) gets added + * @param reauthUser Re-auth user to stash in session with the re-auth token + * @param assertedEmail Identity asserted by the authentication provider, which is not always resolvable to a user. + * Only used for diagnostics: when reauthUser is null this is the sole record of what was + * asserted, since the caller has already discarded the response by the time reauth fails. + * @param sessionUser If not null, validate that this user and reauthUser are the same + * @param request Request from which to retrieve the session + * @param errorMessage Pre-existing error message to add to the URL + * @param redirectUrl URL to which the token (on success) or error message (on failure) gets added */ - public static void setReauthUser(@Nullable User reauthUser, @Nullable User sessionUser, HttpServletRequest request, @Nullable String errorMessage, URLHelper redirectUrl) + public static void setReauthUser(@Nullable User reauthUser, @Nullable String assertedEmail, @Nullable User sessionUser, HttpServletRequest request, @Nullable String errorMessage, URLHelper redirectUrl) { if (errorMessage == null && sessionUser != null && !sessionUser.equals(reauthUser)) { @@ -1927,15 +1930,25 @@ public static void setReauthUser(@Nullable User reauthUser, @Nullable User sessi // The SSO validate actions are @RequiresNoPermission, so getUser() returns guest whenever the request // carries no signed-in session -- typically because the session cookie didn't accompany the IdP's // cross-site POST to the validate action, or because the session timed out mid-flow. - errorMessage = "Reauthentication failed: this browser is no longer signed in; please sign in again"; - _log.warn("Reauthentication failed for \"{}\": the request carried no signed-in session. Check that the session cookie accompanies the identity provider's response to the validate action -- a JSESSIONID with no explicit SameSite value is withheld from that cross-site POST once it is more than a couple of minutes old.", null != reauthUser ? reauthUser.getEmail() : "an unrecognized identity"); + // Deliberately does not claim the browser is signed out: the signed-in session usually still exists and + // works for same-site requests -- it just didn't accompany this one. Offers both remedies because the + // two causes (withheld cookie, expired session) are indistinguishable from here. + errorMessage = "Reauthentication failed: this request did not include your signed-in session. Try signing in again; if the problem persists, contact your administrator."; + // Names the remedy, not just the symptom: the only person who can act on this reads the server log, + // and the property is the same in dev and production even though the file's location is not. + _log.warn("Reauthentication failed for \"{}\": the identity provider's cross-site POST to the validate action carried no JSESSIONID, so the request had no signed-in session. Chromium-based browsers withhold a session cookie that has no explicit SameSite value from that POST once the cookie is more than a couple of minutes old. To fix, set server.servlet.session.cookie.same-site=none and server.servlet.session.cookie.secure=true in application.properties -- these require HTTPS -- and restart the server.", null != reauthUser ? reauthUser.getEmail() : "an unrecognized identity"); } else if (null == reauthUser) + { // Narrow, but sign-in and reauthentication resolve users differently: finalizePrimaryAuthentication() // can auto-create an account, and reauthentication never does. Reaching here means the asserted // identity has no account by the time reauth runs -- deleted or renamed mid-session, or the IdP // asserting a different identifier than it did at sign-in. errorMessage = "Reauthentication failed: the reauthenticated identity does not match a LabKey user account"; + // The asserted identity is the whole diagnosis here and it appears nowhere else: no user resolved, so + // the audit log records nothing and the user-facing message can't name an account that doesn't exist. + _log.warn("Reauthentication failed for \"{}\": the identity provider asserted \"{}\", which matches no LabKey user account.", sessionUser.getEmail(), null != assertedEmail ? assertedEmail : "an unrecognized identity"); + } else { errorMessage = "Reauthentication failed: wrong user reauthenticated"; @@ -2064,7 +2077,7 @@ public void testReauthTokens() throws InterruptedException ActionURL url = new ActionURL("core", "begin.view", ContainerManager.getRoot()); ActionURL clone = url.clone(); - setReauthUser(admin, admin, request, null, clone); + setReauthUser(admin, admin.getEmail(), admin, request, null, clone); assertEquals(initialCount + 1, map.size()); String token = clone.getParameter(REAUTH_TOKEN_NAME); ReauthContext ctx = map.get(token); @@ -2080,14 +2093,14 @@ public void testReauthTokens() throws InterruptedException // Wrong user on set case clone = url.clone(); - setReauthUser(admin, new User(), request, null, clone); + setReauthUser(admin, admin.getEmail(), new User(), request, null, clone); assertNull(clone.getParameter(REAUTH_TOKEN_NAME)); assertEquals("Reauthentication failed: wrong user reauthenticated", clone.getParameter(ERROR_MESSAGE)); assertEquals(initialCount, map.size()); // Wrong user on get case clone = url.clone(); - setReauthUser(admin, admin, request, null, clone); + setReauthUser(admin, admin.getEmail(), admin, request, null, clone); assertEquals(initialCount + 1, map.size()); token = clone.getParameter(REAUTH_TOKEN_NAME); ctx = map.get(token); From fe99e818e48d2eec2784d46820c96772eeb77513 Mon Sep 17 00:00:00 2001 From: Binal Patel Date: Tue, 18 Aug 2026 22:56:18 -0600 Subject: [PATCH 4/4] Add coverage for reauthentication failure scenarios --- .../api/security/AuthenticationManager.java | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/api/src/org/labkey/api/security/AuthenticationManager.java b/api/src/org/labkey/api/security/AuthenticationManager.java index 763aefd2ce2..f1e2ac45e36 100644 --- a/api/src/org/labkey/api/security/AuthenticationManager.java +++ b/api/src/org/labkey/api/security/AuthenticationManager.java @@ -2098,6 +2098,47 @@ public void testReauthTokens() throws InterruptedException assertEquals("Reauthentication failed: wrong user reauthenticated", clone.getParameter(ERROR_MESSAGE)); assertEquals(initialCount, map.size()); + // The remaining cases all fail the same way -- no token, no map entry -- but each reports a different + // problem, so assert the exact text. These messages are what an administrator greps for. + String noSessionMessage = "Reauthentication failed: this request did not include your signed-in session. Try signing in again; if the problem persists, contact your administrator."; + String noAccountMessage = "Reauthentication failed: the reauthenticated identity does not match a LabKey user account"; + + // Guest session: the request carried no signed-in session, so getUser() returned guest + clone = url.clone(); + setReauthUser(admin, admin.getEmail(), User.guest, request, null, clone); + assertNull(clone.getParameter(REAUTH_TOKEN_NAME)); + assertEquals(noSessionMessage, clone.getParameter(ERROR_MESSAGE)); + assertEquals(initialCount, map.size()); + + // Same, with nothing to name in the warning -- covers the "unrecognized identity" fallback, which is + // impractical to reach against a live identity provider + clone = url.clone(); + setReauthUser(null, null, User.guest, request, null, clone); + assertNull(clone.getParameter(REAUTH_TOKEN_NAME)); + assertEquals(noSessionMessage, clone.getParameter(ERROR_MESSAGE)); + assertEquals(initialCount, map.size()); + + // Asserted identity resolves to no LabKey account + clone = url.clone(); + setReauthUser(null, "nobody@labkey.test", admin, request, null, clone); + assertNull(clone.getParameter(REAUTH_TOKEN_NAME)); + assertEquals(noAccountMessage, clone.getParameter(ERROR_MESSAGE)); + assertEquals(initialCount, map.size()); + + // Same, with no asserted identity to report + clone = url.clone(); + setReauthUser(null, null, admin, request, null, clone); + assertNull(clone.getParameter(REAUTH_TOKEN_NAME)); + assertEquals(noAccountMessage, clone.getParameter(ERROR_MESSAGE)); + assertEquals(initialCount, map.size()); + + // A pre-existing error message short-circuits the branch entirely, so the caller's text survives + clone = url.clone(); + setReauthUser(admin, admin.getEmail(), new User(), request, "Reauthentication failed", clone); + assertNull(clone.getParameter(REAUTH_TOKEN_NAME)); + assertEquals("Reauthentication failed", clone.getParameter(ERROR_MESSAGE)); + assertEquals(initialCount, map.size()); + // Wrong user on get case clone = url.clone(); setReauthUser(admin, admin.getEmail(), admin, request, null, clone);