From 4b54fadd1e213e0d7d6a3df086c2750c86a8edc8 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Fri, 28 Oct 2016 09:59:55 +0200 Subject: [PATCH] SONAR-5430 Improve error handling When a functional error is generated, the user is redirect to a page where he can see the error, and not errors in logs are generated --- .../java/it/user/SsoAuthenticationTest.java | 19 ++++++++++++++++--- .../authentication/SsoAuthenticator.java | 10 ++++++++++ .../sonar/server/user/UserSessionFilter.java | 7 +++++++ .../authentication/SsoAuthenticatorTest.java | 11 +++++++++++ .../server/user/UserSessionFilterTest.java | 11 +++++++++++ 5 files changed, 55 insertions(+), 3 deletions(-) diff --git a/it/it-tests/src/test/java/it/user/SsoAuthenticationTest.java b/it/it-tests/src/test/java/it/user/SsoAuthenticationTest.java index 78816596e2d..d13ff117f46 100644 --- a/it/it-tests/src/test/java/it/user/SsoAuthenticationTest.java +++ b/it/it-tests/src/test/java/it/user/SsoAuthenticationTest.java @@ -88,6 +88,16 @@ public class SsoAuthenticationTest { USER_RULE.verifyUserExists(USER_LOGIN, USER_LOGIN, null); } + @Test + public void update_user_when_headers_are_updated() { + call(USER_LOGIN, USER_NAME, USER_EMAIL, null); + USER_RULE.verifyUserExists(USER_LOGIN, USER_NAME, USER_EMAIL); + + // As we don't keep the JWT token is the test, the user is updated + call(USER_LOGIN, "new name", "new email", null); + USER_RULE.verifyUserExists(USER_LOGIN, "new name", "new email"); + } + @Test public void authenticate_with_groups() { call(USER_LOGIN, null, null, GROUP_1); @@ -116,12 +126,15 @@ public class SsoAuthenticationTest { } @Test - public void fail_when_login_is_invalid() throws Exception { + public void display_message_in_ui_but_not_in_log_when_unauthorized_exception() throws Exception { Response response = doCall("invalid login $", null, null, null); - assertThat(response.code()).isEqualTo(500); + assertThat(response.code()).isEqualTo(200); + assertThat(response.body().string()).contains("You're not authorized to access this page. Please contact the administrator"); + List logsLines = FileUtils.readLines(orchestrator.getServer().getLogs(), Charsets.UTF_8); - assertThat(logsLines).contains("org.sonar.server.exceptions.BadRequestException: user.bad_login"); + assertThat(logsLines).doesNotContain("org.sonar.server.exceptions.BadRequestException: user.bad_login"); + USER_RULE.verifyUserDoesNotExist(USER_LOGIN); } private static Response call(String login, @Nullable String name, @Nullable String email, @Nullable String groups) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/SsoAuthenticator.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/SsoAuthenticator.java index 239806115d9..17aba9a45b4 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/SsoAuthenticator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/SsoAuthenticator.java @@ -35,9 +35,11 @@ import javax.servlet.http.HttpServletResponse; import org.sonar.api.config.Settings; import org.sonar.api.server.authentication.Display; import org.sonar.api.server.authentication.IdentityProvider; +import org.sonar.api.server.authentication.UnauthorizedException; import org.sonar.api.server.authentication.UserIdentity; import org.sonar.api.utils.System2; import org.sonar.db.user.UserDto; +import org.sonar.server.exceptions.BadRequestException; import static org.apache.commons.lang.StringUtils.defaultIfBlank; import static org.apache.commons.lang.time.DateUtils.addMinutes; @@ -86,6 +88,14 @@ public class SsoAuthenticator { } public Optional authenticate(HttpServletRequest request, HttpServletResponse response) { + try { + return doAuthenticate(request, response); + } catch (BadRequestException e) { + throw new UnauthorizedException(e.getMessage(), e); + } + } + + private Optional doAuthenticate(HttpServletRequest request, HttpServletResponse response) { if (!settings.getBoolean(ENABLE_PARAM)) { return Optional.empty(); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/UserSessionFilter.java b/server/sonar-server/src/main/java/org/sonar/server/user/UserSessionFilter.java index 73ca6d740d4..271fa5e2f94 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/UserSessionFilter.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/UserSessionFilter.java @@ -30,11 +30,14 @@ import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.sonar.api.server.authentication.UnauthorizedException; import org.sonar.server.authentication.UserSessionInitializer; import org.sonar.server.organization.DefaultOrganizationCache; import org.sonar.server.platform.Platform; import org.sonar.server.setting.ThreadLocalSettings; +import static org.sonar.server.authentication.AuthenticationError.handleUnauthorizedError; + public class UserSessionFilter implements Filter { private final Platform platform; @@ -64,6 +67,10 @@ public class UserSessionFilter implements Filter { } finally { settings.unload(); } + } catch (UnauthorizedException e) { + // Functional exceptions generated by Identity provider API (for instance, when authenticating with SSO and a bad login is given) + // should redirect the user to a unauthorized page with the error message + handleUnauthorizedError(e, response); } finally { defaultOrganizationCache.unload(); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/SsoAuthenticatorTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/SsoAuthenticatorTest.java index 99c3bb511ea..310f9385d11 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/authentication/SsoAuthenticatorTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/SsoAuthenticatorTest.java @@ -34,6 +34,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.sonar.api.config.MapSettings; import org.sonar.api.config.Settings; +import org.sonar.api.server.authentication.UnauthorizedException; import org.sonar.api.utils.System2; import org.sonar.api.utils.internal.AlwaysIncreasingSystem2; import org.sonar.core.util.stream.Collectors; @@ -315,6 +316,16 @@ public class SsoAuthenticatorTest { verifyZeroInteractions(jwtHttpHandler); } + @Test + public void throw_UnauthorizedException_when_BadRequestException_is_generated() throws Exception { + enableSso(); + setNotUserInToken(); + + expectedException.expect(UnauthorizedException.class); + expectedException.expectMessage("user.bad_login"); + underTest.authenticate(createRequest("invalid login", DEFAULT_NAME, DEFAULT_EMAIL, GROUPS), response); + } + private void enableSso() { settings.setProperty("sonar.sso.enable", true); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/UserSessionFilterTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/UserSessionFilterTest.java index 3768bfc388b..d04643132d1 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/UserSessionFilterTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/UserSessionFilterTest.java @@ -29,6 +29,7 @@ import org.junit.Before; import org.junit.Test; import org.mockito.InOrder; import org.mockito.Mockito; +import org.sonar.api.server.authentication.UnauthorizedException; import org.sonar.server.authentication.UserSessionInitializer; import org.sonar.server.organization.DefaultOrganizationCache; import org.sonar.server.platform.Platform; @@ -203,6 +204,16 @@ public class UserSessionFilterTest { } } + @Test + public void send_redirect_when_catching_functional_unauthorized_errors() throws Exception { + mockUserSessionInitializer(true); + doThrow(new UnauthorizedException("bad login")).when(userSessionInitializer).initUserSession(request, response); + + underTest.doFilter(request, response, chain); + + verify(response).sendRedirect("/sessions/unauthorized?message=bad+login"); + } + @Test public void just_for_fun_and_coverage() throws ServletException { UserSessionFilter filter = new UserSessionFilter(); -- 2.39.5