diff --git a/api/src/org/labkey/api/security/WikiTermsOfUseProvider.java b/api/src/org/labkey/api/security/WikiTermsOfUseProvider.java index 7def1be53e8..1eca0597337 100644 --- a/api/src/org/labkey/api/security/WikiTermsOfUseProvider.java +++ b/api/src/org/labkey/api/security/WikiTermsOfUseProvider.java @@ -95,7 +95,7 @@ public static boolean isTermsOfUseRequired(ViewContext ctx, @Nullable Project pr // termsContainer is guaranteed to have a terms-of-use wiki public static boolean isTermsOfUseApproved(ViewContext ctx, @NotNull Container termsContainer) { - HttpSession session = ctx.getRequest().getSession(false); + HttpSession session = ctx.getRequestOrThrow().getSession(false); if (null == session) return false; boolean approved; @@ -114,6 +114,9 @@ public static boolean isTermsOfUseApproved(ViewContext ctx, @NotNull Container t int frequencySeconds = AppProps.getInstance().getTermsOfUseFrequencySeconds(); if (frequencySeconds > 0) { + // Look up the LAST_TERMS_ACCEPTANCE dates of the impersonator + if (user.isImpersonated()) + user = user.getImpersonatingUser(); String isoDateString = PropertyManager.getProperties(user, termsContainer, LAST_TERMS_ACCEPTANCE).get(DATE); if (isoDateString != null) { @@ -127,7 +130,7 @@ public static boolean isTermsOfUseApproved(ViewContext ctx, @NotNull Container t { try (var ignored = SpringActionController.ignoreSqlUpdates()) { - setTermsOfUseApprovedInSession(ctx, termsContainer); + setTermsOfUseApprovedInSession(ctx, termsContainer, ctx.getUser()); } } } @@ -203,29 +206,46 @@ public static TermsOfUse getTermsOfUse(@Nullable Project project) public static void setTermsOfUseApproved(ViewContext ctx, @NotNull Container termsContainer) { - setTermsOfUseApprovedInSession(ctx, termsContainer); - User user = ctx.getUser(); - if (!user.isGuest() && AppProps.getInstance().getTermsOfUseFrequencySeconds() > 0) + setTermsOfUseApproved(ctx, termsContainer, ctx.getUser()); + } + + // Callers mid-authentication (e.g., LoginApiAction) can't rely on ctx.getUser(), since the ViewContext's user + // isn't updated until after this request completes, and the fully-authenticated User may not exist yet if + // secondary authentication is pending. Resolve and pass the accepting user explicitly instead. + public static void setTermsOfUseApproved(ViewContext ctx, @NotNull Container termsContainer, @Nullable User user) + { + setTermsOfUseApprovedInSession(ctx, termsContainer, user); + if (null != user && !user.isGuest()) { - if (user.isImpersonated()) - user = user.getImpersonatingUser(); - WritablePropertyMap map = PropertyManager.getWritableProperties(user, termsContainer, LAST_TERMS_ACCEPTANCE, true); - map.put(DATE, Instant.now().toString()); - map.save(); - LOG.debug("Saving terms acceptance timestamp for {} in {}", user, termsContainer); + // If impersonating, terms-of-use audit event gets logged as accept by impersonated user while being + // impersonated by impersonator + UserManager.addAuditEvent(user, termsContainer, user, "Agreed to terms of use"); + if (AppProps.getInstance().getTermsOfUseFrequencySeconds() > 0) + { + // Unlike the audit entry above (which will clearly show if the user was being impersonated), we want + // to attribute LAST_TERMS_ACCEPTANCE dates to the impersonator. + if (user.isImpersonated()) + user = user.getImpersonatingUser(); + WritablePropertyMap map = PropertyManager.getWritableProperties(user, termsContainer, LAST_TERMS_ACCEPTANCE, true); + map.put(DATE, Instant.now().toString()); + map.save(); + LOG.debug("Saving terms acceptance timestamp for {} in {}", user, termsContainer); + } } } - private static void setTermsOfUseApprovedInSession(ViewContext ctx, @NotNull Container termsContainer) + // Same ctx.getUser() caveat as setTermsOfUseApproved(): callers mid-authentication must pass the accepting user + // explicitly, since ctx.getUser() won't reflect this request's authentication and may be Guest or unavailable. + private static void setTermsOfUseApprovedInSession(ViewContext ctx, @NotNull Container termsContainer, @Nullable User user) { - HttpSession session = ctx.getRequest().getSession(true); + HttpSession session = ctx.getRequestOrThrow().getSession(true); synchronized (SessionHelper.getSessionLock(session)) { Set termsApproved = getApprovedTerms(session); termsApproved.add(termsContainer); } - User user = ctx.getUser(); - LOG.debug("Stashing terms acceptance in session for {} in {}", user + (user.isImpersonated() ? " (impersonated by " + user.getImpersonatingUser() + ")" : ""), termsContainer); + String userDescription = null != user ? user + (user.isImpersonated() ? " (impersonated by " + user.getImpersonatingUser() + ")" : "") : ""; + LOG.debug("Stashing terms acceptance in session for {} in {}", userDescription, termsContainer); } public enum TermsOfUseType implements SafeToRenderEnum diff --git a/api/src/org/labkey/api/util/Rate.java b/api/src/org/labkey/api/util/Rate.java index 8e9f2493194..fcaa087bf13 100644 --- a/api/src/org/labkey/api/util/Rate.java +++ b/api/src/org/labkey/api/util/Rate.java @@ -19,11 +19,6 @@ import java.util.concurrent.TimeUnit; -/** -* User: adam -* Date: 6/11/13 -* Time: 10:05 AM -*/ public class Rate { private final double _rate; @@ -45,9 +40,9 @@ public Rate(long count, long duration, TimeUnit unit) _rate = (double)count / (double)unit.toMillis(duration); if (duration == 1) - _toString = "" + count + "/" + StringUtils.stripEnd(unit.toString(), "S"); + _toString = count + "/" + StringUtils.stripEnd(unit.toString(), "S"); else - _toString = "" + count + "/(" + duration + " " + unit + ")"; + _toString = count + "/(" + duration + " " + unit + ")"; } // Count per millisecond diff --git a/core/src/org/labkey/core/login/LoginController.java b/core/src/org/labkey/core/login/LoginController.java index c62d03d6f2d..7b72bc76c35 100644 --- a/core/src/org/labkey/core/login/LoginController.java +++ b/core/src/org/labkey/core/login/LoginController.java @@ -712,10 +712,21 @@ public Object execute(LoginForm form, BindException errors) if (!form.isForceReauth() && form.isApprovedTermsOfUse()) { + // Pass in the user to setTermsOfUseApproved(), since the ViewContext doesn't have the authenticated + // user yet. Also, if MFA is configured, authResult user will be null; pull user from primary + // result in that case. + User acceptingUser = user; + if (null == acceptingUser) + { + PrimaryAuthenticationResult primaryResult = AuthenticationManager.getPrimaryAuthenticationResult(request.getSession(true)); + if (primaryResult != null) + acceptingUser = primaryResult.getUser(); + } + if (form.getTermsOfUseType() == TermsOfUseType.PROJECT_LEVEL) - WikiTermsOfUseProvider.setTermsOfUseApproved(getViewContext(), WikiTermsOfUseProvider.getTermsContainer(termsProject)); + WikiTermsOfUseProvider.setTermsOfUseApproved(getViewContext(), WikiTermsOfUseProvider.getTermsContainer(termsProject), acceptingUser); else if (form.getTermsOfUseType() == TermsOfUseType.SITE_WIDE) - WikiTermsOfUseProvider.setTermsOfUseApproved(getViewContext(), ContainerManager.getRoot()); + WikiTermsOfUseProvider.setTermsOfUseApproved(getViewContext(), ContainerManager.getRoot(), acceptingUser); response.put("approvedTermsOfUse", true); } @@ -732,8 +743,8 @@ else if (form.getTermsOfUseType() == TermsOfUseType.SITE_WIDE) } else { - // AuthenticationResult returned by AuthenticationManager.handleAuthentication indicated that a secondary authentication is needed - // in the ajax response inform js handler to load page from secondary authenticator url + // AuthenticationResult indicated that a secondary authentication is needed. Inform js handler to + // load page from secondary authenticator url response.put(ActionURL.Param.returnUrl.name(), redirectString); } }