Skip to content

Commit

Permalink
SONAR-8416 restore error message displayed to user
Browse files Browse the repository at this point in the history
  • Loading branch information
@sns-seb
sns-seb committed Dec 1, 2016
1 parent 6d14dc3 commit b750c6e
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 33 deletions.
8 changes: 5 additions & 3 deletions ...sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,18 @@ private UserDto registerNewUser(DbSession dbSession, UserIdentity user, Identity
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();
Expand Down
27 changes: 13 additions & 14 deletions ...er/sonar-server/src/main/java/org/sonar/server/authentication/UserSessionInitializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
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;

Expand All @@ -40,6 +39,8 @@
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;
Expand Down Expand Up @@ -105,24 +106,19 @@ public boolean initUserSession(HttpServletRequest request, HttpServletResponse r
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) {
Expand All @@ -133,7 +129,10 @@ private void setUserSession(HttpServletRequest request, HttpServletResponse resp
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, "-");
Expand Down
6 changes: 3 additions & 3 deletions ...sonar-server/src/main/java/org/sonar/server/authentication/event/AuthenticationEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,15 +134,15 @@ public static Source external(IdentityProvider identityProvider) {
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;
}

Expand Down
12 changes: 7 additions & 5 deletions ...r-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,8 +361,8 @@ public void fail_to_authenticate_new_user_when_allow_users_to_signup_is_false()
.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);
}

Expand All @@ -374,9 +374,11 @@ public void fail_to_authenticate_new_user_when_email_already_exists() throws Exc
.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);
}

Expand Down
33 changes: 25 additions & 8 deletions ...onar-server/src/test/java/org/sonar/server/authentication/UserSessionInitializerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
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;
Expand All @@ -34,14 +35,15 @@
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;

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;
Expand All @@ -50,6 +52,8 @@
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 {

Expand All @@ -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 {
Expand Down Expand Up @@ -154,16 +160,18 @@ public void validate_session_from_sso() throws Exception {
@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());
Expand All @@ -172,27 +180,36 @@ public void return_code_401_when_not_authenticated_and_with_force_authentication

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);
}

@Test
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();

Expand Down

0 comments on commit b750c6e

Please sign in to comment.