diff --git a/api/src/org/labkey/api/action/ApiResponseWriter.java b/api/src/org/labkey/api/action/ApiResponseWriter.java index 9a3e7207eb9..0ca58d6a2ba 100644 --- a/api/src/org/labkey/api/action/ApiResponseWriter.java +++ b/api/src/org/labkey/api/action/ApiResponseWriter.java @@ -52,11 +52,10 @@ public abstract class ApiResponseWriter implements AutoCloseable private static final String RESPONSE_FORMAT_ATTRIBUTE = ApiResponseWriter.class.getName() + "$responseFormat"; /* - * (MAB) This code defaults to using setting the response to SC_BAD_REQUEST - * when any error is encountered. I think this is wrong. Expected - * errors should be encoded in a normal JSON response and SC_OK. + * (MAB) This code defaults to setting the response to SC_BAD_REQUEST when any error is encountered. I think this + * is wrong. Expected errors should be encoded in a normal JSON response and SC_OK. * - * Also, this using SC_BAD_REQUEST means failed APIs get taggd by BlockListFilter + * Also, this using SC_BAD_REQUEST means failed APIs get tagged by BlockListFilter * * Allow new code to specify that SC_OK should be used for errors */ @@ -341,7 +340,7 @@ protected void write(ApiResponse response) throws IOException /** * Even though the default exception handling is implemented here, we require that response.render() - * invokes this method in its own catch() block before endResponse(). This seems cleaner than coordinating + * invokes this method in its own catch() block before endResponse(). This seems cleaner than coordinating * to have render() not output matching { open and close } braces. * * @see ApiResponse#render(ApiResponseWriter) diff --git a/api/src/org/labkey/api/action/SpringActionController.java b/api/src/org/labkey/api/action/SpringActionController.java index 07fed20be14..2db3bf8b148 100644 --- a/api/src/org/labkey/api/action/SpringActionController.java +++ b/api/src/org/labkey/api/action/SpringActionController.java @@ -16,11 +16,16 @@ package org.labkey.api.action; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.json.JSONObject; +import org.labkey.api.action.ApiResponseWriter.Format; import org.labkey.api.admin.AdminUrls; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.data.Container; @@ -70,9 +75,6 @@ import org.springframework.web.servlet.mvc.Controller; import org.springframework.web.servlet.view.RedirectView; -import jakarta.servlet.ServletException; -import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpServletResponse; import java.io.File; import java.io.IOException; import java.io.PrintWriter; @@ -378,6 +380,19 @@ private ModelAndView handleBadRequestException(HttpServletRequest request, HttpS return null; } + public static void setResponseFormat(@NotNull HttpServletRequest request, @Nullable Format defaultFormat) + { + Format responseFormat = Format.getFormatByName(request.getParameter(RESPONSE_FORMAT_PARAMETER_NAME), null); + if (responseFormat == null) + { + responseFormat = defaultFormat; + } + if (responseFormat != null) + { + ApiResponseWriter.setResponseFormat(request, responseFormat); + } + } + @Override public ModelAndView handleRequest(HttpServletRequest request, HttpServletResponse response) { @@ -407,15 +422,7 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons if (!(controller instanceof PermissionCheckable checkable)) throw new IllegalStateException("All actions must implement PermissionCheckable. " + controller.getClass().getName() + " should extend PermissionCheckableAction or one of its subclasses."); - ApiResponseWriter.Format responseFormat = ApiResponseWriter.Format.getFormatByName(request.getParameter(RESPONSE_FORMAT_PARAMETER_NAME), null); - if (responseFormat == null) - { - responseFormat = checkable.getDefaultResponseFormat(); - } - if (responseFormat != null) - { - ApiResponseWriter.setResponseFormat(request, responseFormat); - } + setResponseFormat(request, checkable.getDefaultResponseFormat()); ActionURL redirectURL = getUpgradeMaintenanceRedirect(request, controller); diff --git a/api/src/org/labkey/api/security/AuthFilter.java b/api/src/org/labkey/api/security/AuthFilter.java index 2d4025c2cf2..cf3098b90ec 100644 --- a/api/src/org/labkey/api/security/AuthFilter.java +++ b/api/src/org/labkey/api/security/AuthFilter.java @@ -16,6 +16,15 @@ package org.labkey.api.security; +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.FilterConfig; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import jakarta.servlet.http.HttpSession; import org.apache.commons.collections4.IteratorUtils; import org.apache.commons.lang3.StringUtils; import org.labkey.api.module.ModuleLoader; @@ -31,18 +40,10 @@ import org.labkey.api.util.HttpUtil; import org.labkey.api.util.HttpsUtil; import org.labkey.api.util.Pair; +import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewServlet; import org.labkey.api.view.template.PageConfig; -import jakarta.servlet.Filter; -import jakarta.servlet.FilterChain; -import jakarta.servlet.FilterConfig; -import jakarta.servlet.ServletException; -import jakarta.servlet.ServletRequest; -import jakarta.servlet.ServletResponse; -import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpServletResponse; -import jakarta.servlet.http.HttpSession; import java.io.IOException; import java.io.UnsupportedEncodingException; import java.net.URL; @@ -187,6 +188,11 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha resp.sendError(HttpServletResponse.SC_BAD_REQUEST, uee.getMessage()); return; } + catch (UnauthorizedException ue) + { + ExceptionUtil.handleException(req, resp, ue, ue.getMessage(), false); + return; + } if (null == user) { diff --git a/api/src/org/labkey/api/security/AuthenticationManager.java b/api/src/org/labkey/api/security/AuthenticationManager.java index 36f0ca1e35e..646ecc2c9f8 100644 --- a/api/src/org/labkey/api/security/AuthenticationManager.java +++ b/api/src/org/labkey/api/security/AuthenticationManager.java @@ -25,6 +25,7 @@ import org.jetbrains.annotations.Nullable; import org.junit.Assert; import org.junit.Test; +import org.labkey.api.action.ApiResponseWriter.Format; import org.labkey.api.action.LabKeyError; import org.labkey.api.action.LabKeyErrorWithHtml; import org.labkey.api.action.LabKeyErrorWithLink; @@ -60,6 +61,7 @@ import org.labkey.api.security.AuthenticationProvider.AuthenticationResponse; import org.labkey.api.security.AuthenticationProvider.DisableLoginProvider; import org.labkey.api.security.AuthenticationProvider.ExpireAccountProvider; +import org.labkey.api.security.AuthenticationProvider.FailureReason; import org.labkey.api.security.AuthenticationProvider.LoginFormAuthenticationProvider; import org.labkey.api.security.AuthenticationProvider.PrimaryAuthenticationProvider; import org.labkey.api.security.AuthenticationProvider.ResetPasswordProvider; @@ -95,6 +97,7 @@ import org.labkey.api.view.NavTree; import org.labkey.api.view.NotFoundException; import org.labkey.api.view.RedirectException; +import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.template.PageConfig; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; @@ -659,7 +662,7 @@ public enum AuthenticationStatus Success { @Override - public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL) + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) { throw new IllegalStateException("Shouldn't be adding an error message in success case"); } @@ -667,12 +670,13 @@ public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResul BadCredentials { @Override - public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL) + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) { - errors.addError(new LabKeyError("The email address and password you entered did not match any accounts on file.\nNote: Passwords are case sensitive; make sure your Caps Lock is off.")); + String errorMessage = "The email address and password you entered did not match any accounts on file." + (DisplayLocation.WebUI == location ? "\nNote: Passwords are case sensitive; make sure your Caps Lock is off." : ""); + errors.addError(new LabKeyError(errorMessage)); // Provide additional guidance on failed login, pointing user toward the SSO configuration(s) claiming their email domain - if (null != fullEmailAddress && null != returnURL) + if (null != fullEmailAddress && null != returnURL && DisplayLocation.WebUI == location) { String domain = fullEmailAddress.split("@")[1]; // Callers must ensure that fullEmailAddress includes @ Collection ssoConfigs = AuthenticationConfigurationCache.getActiveConfigurationsForDomain(domain).stream() @@ -702,7 +706,7 @@ public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResul InactiveUser { @Override - public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL) + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) { errors.addError(new ContactAnAdministratorError("Your account has been deactivated.", "to request reactivation of this account.")); } @@ -710,7 +714,7 @@ public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResul LoginDisabled { @Override - public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL) + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) { String errorMessage = result.getMessage() == null ? "Due to the number of recent failed login attempts, authentication has been temporarily paused.\nTry again in one minute." : result.getMessage(); errors.reject(ERROR_MSG, errorMessage); @@ -719,7 +723,7 @@ public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResul LoginPaused { @Override - public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL) + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) { errors.reject(ERROR_MSG, "Due to the number of recent failed login attempts, authentication has been temporarily paused.\nTry again in one minute."); } @@ -727,7 +731,7 @@ public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResul UserCreationError { @Override - public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL) + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) { errors.addError(new ContactAnAdministratorError("The server could not create your account.", "for assistance.")); } @@ -735,7 +739,7 @@ public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResul UserCreationNotAllowed { @Override - public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL) + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) { errors.addError(new ContactAnAdministratorError("This server is not configured to create new accounts automatically.", "to request a new account.")); } @@ -743,31 +747,54 @@ public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResul PasswordExpired { @Override - public boolean requiresRedirect() + public boolean handleRedirect() { return true; } + + @Override + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) + { + errors.reject(ERROR_MSG, "Your password has expired; please choose a new password."); + } }, Complexity { @Override - public boolean requiresRedirect() + public boolean handleRedirect() { return true; } + + @Override + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) + { + errors.reject(ERROR_MSG, "Your password does not meet the complexity requirements; please choose a new password."); + } }; - // Add an appropriate error message to display to the user - public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL) + // Add an appropriate error message to display to the user in the web UI + public final void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL) { + addUserErrorMessage(errors, result, fullEmailAddress, returnURL, DisplayLocation.WebUI); } - public boolean requiresRedirect() + // Add an appropriate error message to show the user in the specified DisplayLocation + public void addUserErrorMessage(BindException errors, PrimaryAuthenticationResult result, @Nullable String fullEmailAddress, @Nullable URLHelper returnURL, DisplayLocation location) + { + } + + public boolean handleRedirect() { return false; } } + public enum DisplayLocation + { + WebUI, API + } + private static final class ContactAnAdministratorError extends LabKeyErrorWithLink { public ContactAnAdministratorError(String message, String adviceTextSuffix) @@ -821,7 +848,7 @@ public PrimaryAuthenticationResult(@NotNull AuthenticationStatus status, String } // Failure case with redirect (e.g., reset password page) - private PrimaryAuthenticationResult(@NotNull URLHelper redirectURL, AuthenticationStatus status) + private PrimaryAuthenticationResult(@Nullable URLHelper redirectURL, AuthenticationStatus status) { _user = null; _response = null; @@ -853,10 +880,17 @@ public URLHelper getRedirectURL() return _redirectURL; } - public String getMessage() + public @Nullable String getMessage() { return _message; } + + public @Nullable String getStatusErrorMessage(DisplayLocation location) + { + BindException errors = new BindException(new Object(), "dummy"); + getStatus().addUserErrorMessage(errors, this, null, null, location); + return errors.hasErrors() ? errors.getAllErrors().get(0).getDefaultMessage() : null; + } } @@ -926,7 +960,7 @@ public static void addAuditEvent(@NotNull User user, HttpServletRequest request, } else { - AuthenticationProvider.FailureReason reason = authResponse.getFailureReason(); + FailureReason reason = authResponse.getFailureReason(); switch (reason.getReportType()) { @@ -998,22 +1032,20 @@ else if (null != emailAddress) // For now, redirectURL is only checked in the failure case, see #19778 for some history on redirect handling ActionURL redirectURL = firstFailure.getRedirectURL(); - if (null != redirectURL) + // if labkey db authentication determines password has expired or does not meet requirements then return + // result with appropriate status. Note that redirectUrl might be null (e.g., API case). + FailureReason firstFailureReason = firstFailure.getFailureReason(); + if (firstFailureReason == expired) { - // if labkey db authenticate determines password has expired or that password does not meet complexity requirements then return url to redirect user - firstFailure.getFailureReason(); - if (firstFailure.getFailureReason() == expired) - { - return new PrimaryAuthenticationResult(redirectURL, AuthenticationStatus.PasswordExpired); - } - else if (firstFailure.getFailureReason() == complexity) - { - return new PrimaryAuthenticationResult(redirectURL, AuthenticationStatus.Complexity); - } - else - { - throw new RedirectException(redirectURL); - } + return new PrimaryAuthenticationResult(redirectURL, AuthenticationStatus.PasswordExpired); + } + else if (firstFailureReason == complexity) + { + return new PrimaryAuthenticationResult(redirectURL, AuthenticationStatus.Complexity); + } + else if (null != redirectURL) + { + throw new RedirectException(redirectURL); } } } @@ -1191,10 +1223,11 @@ private static void addDefaultUserLoginDelay(HttpServletRequest request, String // Attempts to authenticate using only LoginFormAuthenticationProviders (e.g., DbLogin, LDAP). This is for the case // where you have an id & password in hand and want to ignore SSO and other delegated authentication mechanisms that - // rely on cookies, browser redirects, etc. Current usages include basic auth, API keys, and test cases. Note that - // this will always fail if any secondary authentication is enabled (e.g., Duo). + // rely on cookies, browser redirects, etc. Current usages include basic auth and a test case. Note that this will + // fail if any secondary authentication is enabled (e.g., TOTP, Duo) unless an API key is passed. - // Returns null if credentials are incorrect, user doesn't exist, user is inactive, or secondary auth is enabled. + // Throws UnauthorizedException if credentials are incorrect, password is expired, password is not complex enough, + // user doesn't exist, user is inactive, or secondary auth is enabled and an API key hasn't been used. public static @Nullable User authenticate(HttpServletRequest request, String id, String password) throws InvalidEmailException { PrimaryAuthenticationResult primaryResult = authenticate(request, id, password, null, true); @@ -1207,9 +1240,16 @@ private static void addDefaultUserLoginDelay(HttpServletRequest request, String return handleAuthentication(request, ContainerManager.getRoot()).getUser(); } - return null; - } + // Basic auth has failed so send error response in a format that APIs can consume (JSON unless request specifies + // otherwise). Note: If needed to handle other pre-action cases, setResponseFormat() could be called earlier, + // e.g., in AuthFilter.doFilter(), but for now, the basic auth failure case is all we care about. Regardless, + // SpringActionController.handleRequest() will need to call this again since actions can specify the default + // format. + SpringActionController.setResponseFormat(request, Format.JSON); + String message = primaryResult.getMessage(); + throw new UnauthorizedException(message != null ? message : primaryResult.getStatusErrorMessage(DisplayLocation.API)); + } public static URLHelper logout(@NotNull User user, @NotNull HttpServletRequest request, URLHelper returnURL) { diff --git a/api/src/org/labkey/api/util/ExceptionUtil.java b/api/src/org/labkey/api/util/ExceptionUtil.java index 8510b08c826..281727fdefd 100644 --- a/api/src/org/labkey/api/util/ExceptionUtil.java +++ b/api/src/org/labkey/api/util/ExceptionUtil.java @@ -816,7 +816,7 @@ else if (ex instanceof UnauthorizedException uae) // useful for when the page wants to handle 401s itself String headerHint = request.getHeader("X-ONUNAUTHORIZED"); - boolean isGuest = user != null && user.isGuest(); + boolean isGuest = user == null || user.isGuest(); UnauthorizedException.Type type = uae.getType(); boolean overrideBasicAuth = "UNAUTHORIZED".equals(headerHint); boolean isCSRFViolation = uae instanceof CSRFException; @@ -825,7 +825,7 @@ else if (ex instanceof UnauthorizedException uae) if (isGET) { // If user has not logged in or agreed to terms, not really unauthorized yet... - if (!isCSRFViolation && isGuest && type == UnauthorizedException.Type.redirectToLogin && !overrideBasicAuth) + if (!isCSRFViolation && isGuest && type == UnauthorizedException.Type.redirectToLogin && !overrideBasicAuth && HttpView.hasCurrentView()) { // Issue 43307: If this is a locked project then just show the login page in the root. Register, // forgot my password, profile update, password reset, etc. aren't going to work right in a locked diff --git a/core/src/org/labkey/core/login/LoginController.java b/core/src/org/labkey/core/login/LoginController.java index 02b7d5f29c8..7a9cea5875c 100644 --- a/core/src/org/labkey/core/login/LoginController.java +++ b/core/src/org/labkey/core/login/LoginController.java @@ -335,7 +335,7 @@ private static boolean authenticate(LoginForm form, BindException errors, HttpSe // Explicit test for valid email ValidEmail email = new ValidEmail(form.getEmail()); - if (status.requiresRedirect()) + if (status.handleRedirect()) { AuthenticationManager.setLoginReturnProperties(request, new LoginReturnProperties(result.getRedirectURL(), form.getUrlhash(), form.getSkipProfile())); }