From b750c6ec81a322fd1c61fff73cfa9f42cbfae84f Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Wed, 30 Nov 2016 12:33:10 +0100 Subject: [PATCH] SONAR-8416 restore error message displayed to user --- .../UserIdentityAuthenticator.java | 8 +++-- .../UserSessionInitializer.java | 27 ++++++++------- .../event/AuthenticationEvent.java | 6 ++-- .../UserIdentityAuthenticatorTest.java | 12 ++++--- .../UserSessionInitializerTest.java | 33 ++++++++++++++----- 5 files changed, 53 insertions(+), 33 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java index e95302d91e3..662338394ce 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java @@ -88,16 +88,18 @@ public class UserIdentityAuthenticator { throw AuthenticationException.newBuilder() .setSource(source) .setLogin(user.getLogin()) - .setMessage(format("'%s' users are not allowed to sign up", provider.getKey())) + .setMessage("user signup disabled for provider '" + provider.getKey() + "'") + .setPublicMessage(format("'%s' users are not allowed to sign up", provider.getKey())) .build(); } String email = user.getEmail(); if (email != null && dbClient.userDao().doesEmailExist(dbSession, email)) { throw AuthenticationException.newBuilder() - .setLogin(user.getLogin()) .setSource(source) - .setMessage(format( + .setLogin(user.getLogin()) + .setMessage(format("email '%s' is already used", email)) + .setPublicMessage(format( "You can't sign up because email '%s' is already used by an existing user. This means that you probably already registered with another account.", email)) .build(); diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserSessionInitializer.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserSessionInitializer.java index 8b1456aaacb..f63d4efd7d8 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserSessionInitializer.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserSessionInitializer.java @@ -31,7 +31,6 @@ import org.sonar.db.DbClient; import org.sonar.db.user.UserDto; import org.sonar.server.authentication.event.AuthenticationEvent; import org.sonar.server.authentication.event.AuthenticationException; -import org.sonar.server.exceptions.UnauthorizedException; import org.sonar.server.user.ServerUserSession; import org.sonar.server.user.ThreadLocalUserSession; @@ -40,6 +39,8 @@ import static org.sonar.api.CoreProperties.CORE_FORCE_AUTHENTICATION_PROPERTY; import static org.sonar.api.web.ServletFilter.UrlPattern; import static org.sonar.api.web.ServletFilter.UrlPattern.Builder.staticResourcePatterns; import static org.sonar.server.authentication.AuthenticationError.handleAuthenticationError; +import static org.sonar.server.authentication.event.AuthenticationEvent.Method; +import static org.sonar.server.authentication.event.AuthenticationEvent.Source; import static org.sonar.server.authentication.ws.LoginAction.AUTH_LOGIN_URL; import static org.sonar.server.authentication.ws.ValidateAction.AUTH_VALIDATE_URL; import static org.sonar.server.user.ServerUserSession.createForAnonymous; @@ -105,24 +106,19 @@ public class UserSessionInitializer { response.setStatus(HTTP_UNAUTHORIZED); return false; } - handleAuthenticationError(e, response); - return false; - } catch (UnauthorizedException e) { - response.setStatus(HTTP_UNAUTHORIZED); - if (isWsUrl(path)) { + if (isNotLocalOrJwt(e.getSource())) { + // redirect to Unauthorized error page + handleAuthenticationError(e, response); return false; } - // WS should stop here. Rails page should continue in order to deal with redirection + // Rails will redirect to login page return true; } } - private static boolean shouldContinueFilterOnError(String path) { - if (isWsUrl(path)) { - return false; - } - // WS should stop here. Rails page should continue in order to deal with redirection - return true; + private static boolean isNotLocalOrJwt(Source source) { + AuthenticationEvent.Provider provider = source.getProvider(); + return provider != AuthenticationEvent.Provider.LOCAL && provider != AuthenticationEvent.Provider.JWT; } private void setUserSession(HttpServletRequest request, HttpServletResponse response) { @@ -133,7 +129,10 @@ public class UserSessionInitializer { request.setAttribute(ACCESS_LOG_LOGIN, session.getLogin()); } else { if (settings.getBoolean(CORE_FORCE_AUTHENTICATION_PROPERTY)) { - throw new UnauthorizedException("User must be authenticated"); + throw AuthenticationException.newBuilder() + .setSource(Source.local(Method.BASIC)) + .setMessage("User must be authenticated") + .build(); } threadLocalSession.set(createForAnonymous(dbClient)); request.setAttribute(ACCESS_LOG_LOGIN, "-"); diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/event/AuthenticationEvent.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/event/AuthenticationEvent.java index d0eeac4f932..d01411c26ad 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/event/AuthenticationEvent.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/event/AuthenticationEvent.java @@ -134,15 +134,15 @@ public interface AuthenticationEvent { requireNonNull(identityProvider, "identityProvider can't be null").getName()); } - Method getMethod() { + public Method getMethod() { return method; } - Provider getProvider() { + public Provider getProvider() { return provider; } - String getProviderName() { + public String getProviderName() { return providerName; } diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java index 6c649243caa..767e79a4318 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java @@ -361,8 +361,8 @@ public class UserIdentityAuthenticatorTest { .setAllowsUsersToSignUp(false); Source source = Source.realm(Method.FORM, identityProvider.getName()); - thrown.expect(authenticationException().from(source).withLogin(USER_IDENTITY.getLogin()).andNoPublicMessage()); - thrown.expectMessage("'github' users are not allowed to sign up"); + thrown.expect(authenticationException().from(source).withLogin(USER_IDENTITY.getLogin()).andPublicMessage("'github' users are not allowed to sign up")); + thrown.expectMessage("user signup disabled for provider 'github'"); underTest.authenticate(USER_IDENTITY, identityProvider, source); } @@ -374,9 +374,11 @@ public class UserIdentityAuthenticatorTest { .setEmail("john@email.com")); Source source = Source.realm(Method.FORM, IDENTITY_PROVIDER.getName()); - thrown.expect(authenticationException().from(source).withLogin(USER_IDENTITY.getLogin()).andNoPublicMessage()); - thrown.expectMessage("You can't sign up because email 'john@email.com' is already used by an existing user. " + - "This means that you probably already registered with another account."); + thrown.expect(authenticationException().from(source) + .withLogin(USER_IDENTITY.getLogin()) + .andPublicMessage("You can't sign up because email 'john@email.com' is already used by an existing user. " + + "This means that you probably already registered with another account.")); + thrown.expectMessage("email 'john@email.com' is already used"); underTest.authenticate(USER_IDENTITY, IDENTITY_PROVIDER, source); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/UserSessionInitializerTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/UserSessionInitializerTest.java index a3f6a994e58..b57103037e9 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/authentication/UserSessionInitializerTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/UserSessionInitializerTest.java @@ -26,6 +26,7 @@ import javax.servlet.http.HttpServletResponse; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.mockito.ArgumentCaptor; import org.sonar.api.config.MapSettings; import org.sonar.api.config.Settings; import org.sonar.api.utils.System2; @@ -34,7 +35,7 @@ import org.sonar.db.DbSession; import org.sonar.db.DbTester; import org.sonar.db.user.UserDto; import org.sonar.server.authentication.event.AuthenticationEvent; -import org.sonar.server.exceptions.UnauthorizedException; +import org.sonar.server.authentication.event.AuthenticationException; import org.sonar.server.user.ServerUserSession; import org.sonar.server.user.ThreadLocalUserSession; import org.sonar.server.user.UserSession; @@ -42,6 +43,7 @@ import org.sonar.server.user.UserSession; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -50,6 +52,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; import static org.sonar.db.user.UserTesting.newUserDto; +import static org.sonar.server.authentication.event.AuthenticationEvent.*; +import static org.sonar.server.authentication.event.AuthenticationEvent.Source; public class UserSessionInitializerTest { @@ -68,12 +72,14 @@ public class UserSessionInitializerTest { private JwtHttpHandler jwtHttpHandler = mock(JwtHttpHandler.class); private BasicAuthenticator basicAuthenticator = mock(BasicAuthenticator.class); private SsoAuthenticator ssoAuthenticator = mock(SsoAuthenticator.class); + private AuthenticationEvent authenticationEvent = mock(AuthenticationEvent.class); private Settings settings = new MapSettings(); private UserDto user = newUserDto(); - private UserSessionInitializer underTest = new UserSessionInitializer(dbClient, settings, jwtHttpHandler, basicAuthenticator, ssoAuthenticator, userSession, mock(AuthenticationEvent.class)); + private UserSessionInitializer underTest = new UserSessionInitializer(dbClient, settings, jwtHttpHandler, basicAuthenticator, + ssoAuthenticator, userSession, authenticationEvent); @Before public void setUp() throws Exception { @@ -154,16 +160,18 @@ public class UserSessionInitializerTest { @Test public void return_code_401_when_invalid_token_exception() throws Exception { when(ssoAuthenticator.authenticate(request, response)).thenReturn(Optional.empty()); - doThrow(new UnauthorizedException("invalid token")).when(jwtHttpHandler).validateToken(request, response); + AuthenticationException authenticationException = AuthenticationException.newBuilder().setSource(Source.jwt()).setMessage("Token id hasn't been found").build(); + doThrow(authenticationException).when(jwtHttpHandler).validateToken(request, response); assertThat(underTest.initUserSession(request, response)).isTrue(); - verify(response).setStatus(401); - verifyZeroInteractions(userSession); + verify(authenticationEvent).failure(request, authenticationException); + verifyZeroInteractions(response, userSession); } @Test public void return_code_401_when_not_authenticated_and_with_force_authentication() throws Exception { + ArgumentCaptor exceptionArgumentCaptor = ArgumentCaptor.forClass(AuthenticationException.class); when(userSession.isLoggedIn()).thenReturn(false); when(basicAuthenticator.authenticate(request)).thenReturn(Optional.empty()); when(ssoAuthenticator.authenticate(request, response)).thenReturn(Optional.empty()); @@ -172,19 +180,27 @@ public class UserSessionInitializerTest { assertThat(underTest.initUserSession(request, response)).isTrue(); - verify(response).setStatus(401); + verifyZeroInteractions(response); + verify(authenticationEvent).failure(eq(request), exceptionArgumentCaptor.capture()); verifyZeroInteractions(userSession); + AuthenticationException authenticationException = exceptionArgumentCaptor.getValue(); + assertThat(authenticationException.getSource()).isEqualTo(Source.local(Method.BASIC)); + assertThat(authenticationException.getLogin()).isNull(); + assertThat(authenticationException.getMessage()).isEqualTo("User must be authenticated"); + assertThat(authenticationException.getPublicMessage()).isNull(); } @Test public void return_401_and_stop_on_ws() throws Exception { when(request.getRequestURI()).thenReturn("/api/issues"); when(ssoAuthenticator.authenticate(request, response)).thenReturn(Optional.empty()); - doThrow(new UnauthorizedException("invalid token")).when(jwtHttpHandler).validateToken(request, response); + AuthenticationException authenticationException = AuthenticationException.newBuilder().setSource(Source.jwt()).setMessage("Token id hasn't been found").build(); + doThrow(authenticationException).when(jwtHttpHandler).validateToken(request, response); assertThat(underTest.initUserSession(request, response)).isFalse(); verify(response).setStatus(401); + verify(authenticationEvent).failure(request, authenticationException); verifyZeroInteractions(userSession); } @@ -192,7 +208,8 @@ public class UserSessionInitializerTest { public void return_401_and_stop_on_batch_ws() throws Exception { when(request.getRequestURI()).thenReturn("/batch/global"); when(ssoAuthenticator.authenticate(request, response)).thenReturn(Optional.empty()); - doThrow(new UnauthorizedException("invalid token")).when(jwtHttpHandler).validateToken(request, response); + doThrow(AuthenticationException.newBuilder().setSource(Source.jwt()).setMessage("Token id hasn't been found").build()) + .when(jwtHttpHandler).validateToken(request, response); assertThat(underTest.initUserSession(request, response)).isFalse(); -- 2.39.5