SONAR-8416 restore error message displayed to user

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<AuthenticationException> 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();