Add audit reauthentication as its own event and improve diagnostics

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,14 +1095,24 @@ 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
         {
             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);

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;
 

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)