diff --git a/api/src/org/labkey/api/security/AuthenticationManager.java b/api/src/org/labkey/api/security/AuthenticationManager.java index ba60877c838..f1e2ac45e36 100644 --- a/api/src/org/labkey/api/security/AuthenticationManager.java +++ b/api/src/org/labkey/api/security/AuthenticationManager.java @@ -630,7 +630,11 @@ 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)) + 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); } @@ -1713,7 +1740,12 @@ 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)) + auditReauthSuccess(primaryAuthUser, primaryAuthResult.getResponse()); + return new AuthenticationResult(primaryAuthUser, url); } @@ -1878,17 +1910,52 @@ 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(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)) { - 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. + // 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"; + // 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 +1972,20 @@ public static void setReauthUser(User reauthUser, @Nullable User sessionUser, Ht } } + /** + * 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) + { + // 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() + "."); + } + // Separate method to allow unit testing private static void addToken(HttpServletRequest request, User reauthUser, String reauthToken, Instant expiration) { @@ -1996,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); @@ -2012,14 +2093,55 @@ 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()); + // 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, 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); 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)