From ebfad7894a43bc485b53caaec66b0cb8a0971b72 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Fri, 30 Nov 2018 13:02:39 +0100 Subject: [PATCH] SONARCLOUD-213 refactor Authenticators to return UserSession - Authenticators renamed RequestAuthenticator - the logic between UserDto and UserSession is moved outside UserSessionInitializer --- .../authentication/AuthenticationModule.java | 2 +- .../HttpHeadersAuthenticator.java | 2 +- ...icators.java => RequestAuthenticator.java} | 8 +-- ...mpl.java => RequestAuthenticatorImpl.java} | 23 ++++++-- .../UserSessionInitializer.java | 40 ++++--------- ...java => RequestAuthenticatorImplTest.java} | 36 +++++++---- .../UserSessionInitializerTest.java | 59 ++++++++----------- 7 files changed, 86 insertions(+), 84 deletions(-) rename server/sonar-server/src/main/java/org/sonar/server/authentication/{Authenticators.java => RequestAuthenticator.java} (78%) rename server/sonar-server/src/main/java/org/sonar/server/authentication/{AuthenticatorsImpl.java => RequestAuthenticatorImpl.java} (63%) rename server/sonar-server/src/test/java/org/sonar/server/authentication/{AuthenticatorsImplTest.java => RequestAuthenticatorImplTest.java} (71%) diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/AuthenticationModule.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/AuthenticationModule.java index dfc64b840e0..91a527ce855 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/AuthenticationModule.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/AuthenticationModule.java @@ -52,6 +52,6 @@ public class AuthenticationModule extends Module { BasicAuthenticator.class, ValidateAction.class, HttpHeadersAuthenticator.class, - AuthenticatorsImpl.class); + RequestAuthenticatorImpl.class); } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/HttpHeadersAuthenticator.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/HttpHeadersAuthenticator.java index 5282b62bd16..d2eb32b0009 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/HttpHeadersAuthenticator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/HttpHeadersAuthenticator.java @@ -83,7 +83,7 @@ public class HttpHeadersAuthenticator implements Startable { private Map settingsByKey = new HashMap<>(); public HttpHeadersAuthenticator(System2 system2, Configuration config, UserIdentityAuthenticator userIdentityAuthenticator, - JwtHttpHandler jwtHttpHandler, AuthenticationEvent authenticationEvent) { + JwtHttpHandler jwtHttpHandler, AuthenticationEvent authenticationEvent) { this.system2 = system2; this.config = config; this.userIdentityAuthenticator = userIdentityAuthenticator; diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/Authenticators.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/RequestAuthenticator.java similarity index 78% rename from server/sonar-server/src/main/java/org/sonar/server/authentication/Authenticators.java rename to server/sonar-server/src/main/java/org/sonar/server/authentication/RequestAuthenticator.java index 83db5262087..ab565c615c8 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/Authenticators.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/RequestAuthenticator.java @@ -19,13 +19,13 @@ */ package org.sonar.server.authentication; -import java.util.Optional; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import org.sonar.db.user.UserDto; +import org.sonar.server.authentication.event.AuthenticationException; +import org.sonar.server.user.UserSession; -public interface Authenticators { +public interface RequestAuthenticator { - Optional authenticate(HttpServletRequest request, HttpServletResponse response); + UserSession authenticate(HttpServletRequest request, HttpServletResponse response) throws AuthenticationException; } diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/AuthenticatorsImpl.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/RequestAuthenticatorImpl.java similarity index 63% rename from server/sonar-server/src/main/java/org/sonar/server/authentication/AuthenticatorsImpl.java rename to server/sonar-server/src/main/java/org/sonar/server/authentication/RequestAuthenticatorImpl.java index a1bd581afa5..76d3f68513a 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/AuthenticatorsImpl.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/RequestAuthenticatorImpl.java @@ -23,22 +23,37 @@ import java.util.Optional; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.sonar.db.user.UserDto; +import org.sonar.server.authentication.event.AuthenticationException; +import org.sonar.server.user.UserSession; +import org.sonar.server.user.UserSessionFactory; -public class AuthenticatorsImpl implements Authenticators { +public class RequestAuthenticatorImpl implements RequestAuthenticator { private final JwtHttpHandler jwtHttpHandler; private final BasicAuthenticator basicAuthenticator; private final HttpHeadersAuthenticator httpHeadersAuthenticator; + private final UserSessionFactory userSessionFactory; - public AuthenticatorsImpl(JwtHttpHandler jwtHttpHandler, BasicAuthenticator basicAuthenticator, HttpHeadersAuthenticator httpHeadersAuthenticator) { + public RequestAuthenticatorImpl(JwtHttpHandler jwtHttpHandler, BasicAuthenticator basicAuthenticator, HttpHeadersAuthenticator httpHeadersAuthenticator, + UserSessionFactory userSessionFactory) throws AuthenticationException { this.jwtHttpHandler = jwtHttpHandler; this.basicAuthenticator = basicAuthenticator; this.httpHeadersAuthenticator = httpHeadersAuthenticator; + this.userSessionFactory = userSessionFactory; } - // Try first to authenticate from SSO, then JWT token, then try from basic http header @Override - public Optional authenticate(HttpServletRequest request, HttpServletResponse response) { + public UserSession authenticate(HttpServletRequest request, HttpServletResponse response) { + Optional userOpt = loadUser(request, response); + if (userOpt.isPresent()) { + return userSessionFactory.create(userOpt.get()); + } + return userSessionFactory.createAnonymous(); + } + + private Optional loadUser(HttpServletRequest request, HttpServletResponse response) { + // Try first to authenticate from SSO, then JWT token, then try from basic http header + // SSO authentication should come first in order to update JWT if user from header is not the same is user from JWT Optional user = httpHeadersAuthenticator.authenticate(request, response); if (user.isPresent()) { 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 780c5618365..f72fa92d5cd 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 @@ -20,21 +20,17 @@ package org.sonar.server.authentication; import com.google.common.collect.ImmutableSet; -import java.util.Optional; import java.util.Set; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.sonar.api.config.Configuration; import org.sonar.api.server.ServerSide; import org.sonar.api.web.ServletFilter.UrlPattern; -import org.sonar.db.user.UserDto; import org.sonar.server.authentication.event.AuthenticationEvent; -import org.sonar.server.authentication.event.AuthenticationEvent.Method; import org.sonar.server.authentication.event.AuthenticationEvent.Source; import org.sonar.server.authentication.event.AuthenticationException; import org.sonar.server.user.ThreadLocalUserSession; import org.sonar.server.user.UserSession; -import org.sonar.server.user.UserSessionFactory; import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED; import static org.apache.commons.lang.StringUtils.defaultString; @@ -66,8 +62,7 @@ public class UserSessionInitializer { LOGIN_URL, LOGOUT_URL, VALIDATE_URL); private static final Set URL_USING_PASSCODE = ImmutableSet.of( - "/api/ce/info", "/api/ce/pause", "/api/ce/resume", "/api/system/health", "/api/system/analytics" - ); + "/api/ce/info", "/api/ce/pause", "/api/ce/resume", "/api/system/health", "/api/system/analytics"); private static final UrlPattern URL_PATTERN = UrlPattern.builder() .includes("/*") @@ -82,16 +77,14 @@ public class UserSessionInitializer { private final Configuration config; private final ThreadLocalUserSession threadLocalSession; private final AuthenticationEvent authenticationEvent; - private final UserSessionFactory userSessionFactory; - private final Authenticators authenticators; + private final RequestAuthenticator requestAuthenticator; public UserSessionInitializer(Configuration config, ThreadLocalUserSession threadLocalSession, AuthenticationEvent authenticationEvent, - UserSessionFactory userSessionFactory, Authenticators authenticators) { + RequestAuthenticator requestAuthenticator) { this.config = config; this.threadLocalSession = threadLocalSession; this.authenticationEvent = authenticationEvent; - this.userSessionFactory = userSessionFactory; - this.authenticators = authenticators; + this.requestAuthenticator = requestAuthenticator; } public boolean initUserSession(HttpServletRequest request, HttpServletResponse response) { @@ -123,28 +116,17 @@ public class UserSessionInitializer { return provider != AuthenticationEvent.Provider.LOCAL && provider != AuthenticationEvent.Provider.JWT; } - private void loadUserSession(HttpServletRequest request, HttpServletResponse response, boolean usingPasscode) { - UserSession session; - Optional user = authenticators.authenticate(request, response); - if (user.isPresent()) { - session = userSessionFactory.create(user.get()); - } else { - if (!usingPasscode) { - failIfAuthenticationIsRequired(); - } - session = userSessionFactory.createAnonymous(); - } - threadLocalSession.set(session); - request.setAttribute(ACCESS_LOG_LOGIN, defaultString(session.getLogin(), "-")); - } - - private void failIfAuthenticationIsRequired() { - if (config.getBoolean(CORE_FORCE_AUTHENTICATION_PROPERTY).orElse(false)) { + private void loadUserSession(HttpServletRequest request, HttpServletResponse response, boolean urlSupportsSystemPasscode) { + UserSession session = requestAuthenticator.authenticate(request, response); + if (!session.isLoggedIn() && !urlSupportsSystemPasscode && config.getBoolean(CORE_FORCE_AUTHENTICATION_PROPERTY).orElse(false)) { + // authentication is required throw AuthenticationException.newBuilder() - .setSource(Source.local(Method.BASIC)) + .setSource(Source.local(AuthenticationEvent.Method.BASIC)) .setMessage("User must be authenticated") .build(); } + threadLocalSession.set(session); + request.setAttribute(ACCESS_LOG_LOGIN, defaultString(session.getLogin(), "-")); } public void removeUserSession() { diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/AuthenticatorsImplTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/RequestAuthenticatorImplTest.java similarity index 71% rename from server/sonar-server/src/test/java/org/sonar/server/authentication/AuthenticatorsImplTest.java rename to server/sonar-server/src/test/java/org/sonar/server/authentication/RequestAuthenticatorImplTest.java index 492fb8bfa0b..c395407b5c7 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/authentication/AuthenticatorsImplTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/RequestAuthenticatorImplTest.java @@ -22,8 +22,13 @@ package org.sonar.server.authentication; import java.util.Optional; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.junit.Before; import org.junit.Test; import org.sonar.db.user.UserDto; +import org.sonar.server.tester.AnonymousMockUserSession; +import org.sonar.server.tester.MockUserSession; +import org.sonar.server.user.UserSession; +import org.sonar.server.user.UserSessionFactory; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyInt; @@ -33,32 +38,40 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.sonar.db.user.UserTesting.newUserDto; -public class AuthenticatorsImplTest { +public class RequestAuthenticatorImplTest { + + private static final UserDto A_USER = newUserDto(); - private UserDto user = newUserDto(); private HttpServletRequest request = mock(HttpServletRequest.class); private HttpServletResponse response = mock(HttpServletResponse.class); private JwtHttpHandler jwtHttpHandler = mock(JwtHttpHandler.class); private BasicAuthenticator basicAuthenticator = mock(BasicAuthenticator.class); private HttpHeadersAuthenticator httpHeadersAuthenticator = mock(HttpHeadersAuthenticator.class); - private Authenticators underTest = new AuthenticatorsImpl(jwtHttpHandler, basicAuthenticator, httpHeadersAuthenticator); + private UserSessionFactory sessionFactory = mock(UserSessionFactory.class); + private RequestAuthenticator underTest = new RequestAuthenticatorImpl(jwtHttpHandler, basicAuthenticator, httpHeadersAuthenticator, sessionFactory); + + @Before + public void setUp() throws Exception { + when(sessionFactory.create(A_USER)).thenReturn(new MockUserSession(A_USER)); + when(sessionFactory.createAnonymous()).thenReturn(new AnonymousMockUserSession()); + } @Test public void authenticate_from_jwt_token() { when(httpHeadersAuthenticator.authenticate(request, response)).thenReturn(Optional.empty()); - when(jwtHttpHandler.validateToken(request, response)).thenReturn(Optional.of(user)); + when(jwtHttpHandler.validateToken(request, response)).thenReturn(Optional.of(A_USER)); - assertThat(underTest.authenticate(request, response)).hasValue(user); + assertThat(underTest.authenticate(request, response).getUuid()).isEqualTo(A_USER.getUuid()); verify(response, never()).setStatus(anyInt()); } @Test public void authenticate_from_basic_header() { - when(basicAuthenticator.authenticate(request)).thenReturn(Optional.of(user)); + when(basicAuthenticator.authenticate(request)).thenReturn(Optional.of(A_USER)); when(httpHeadersAuthenticator.authenticate(request, response)).thenReturn(Optional.empty()); when(jwtHttpHandler.validateToken(request, response)).thenReturn(Optional.empty()); - assertThat(underTest.authenticate(request, response)).hasValue(user); + assertThat(underTest.authenticate(request, response).getUuid()).isEqualTo(A_USER.getUuid()); verify(jwtHttpHandler).validateToken(request, response); verify(basicAuthenticator).authenticate(request); @@ -67,10 +80,10 @@ public class AuthenticatorsImplTest { @Test public void authenticate_from_sso() { - when(httpHeadersAuthenticator.authenticate(request, response)).thenReturn(Optional.of(user)); + when(httpHeadersAuthenticator.authenticate(request, response)).thenReturn(Optional.of(A_USER)); when(jwtHttpHandler.validateToken(request, response)).thenReturn(Optional.empty()); - assertThat(underTest.authenticate(request, response)).hasValue(user); + assertThat(underTest.authenticate(request, response).getUuid()).isEqualTo(A_USER.getUuid()); verify(httpHeadersAuthenticator).authenticate(request, response); verify(jwtHttpHandler, never()).validateToken(request, response); @@ -83,7 +96,10 @@ public class AuthenticatorsImplTest { when(httpHeadersAuthenticator.authenticate(request, response)).thenReturn(Optional.empty()); when(basicAuthenticator.authenticate(request)).thenReturn(Optional.empty()); - assertThat(underTest.authenticate(request, response)).isEmpty(); + UserSession session = underTest.authenticate(request, response); + assertThat(session.isLoggedIn()).isFalse(); + assertThat(session.getUuid()).isNull(); verify(response, never()).setStatus(anyInt()); } + } 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 de82d3b190e..6ad1dc21893 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 @@ -19,7 +19,6 @@ */ package org.sonar.server.authentication; -import java.util.Optional; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.junit.Before; @@ -29,20 +28,17 @@ import org.mockito.ArgumentCaptor; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.server.authentication.BaseIdentityProvider; import org.sonar.api.utils.System2; -import org.sonar.db.DbClient; -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.authentication.event.AuthenticationEvent.Method; import org.sonar.server.authentication.event.AuthenticationEvent.Source; import org.sonar.server.authentication.event.AuthenticationException; -import org.sonar.server.user.TestUserSessionFactory; +import org.sonar.server.tester.AnonymousMockUserSession; +import org.sonar.server.tester.MockUserSession; import org.sonar.server.user.ThreadLocalUserSession; import org.sonar.server.user.UserSession; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; @@ -50,32 +46,22 @@ import static org.mockito.Mockito.reset; 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; public class UserSessionInitializerTest { @Rule public DbTester dbTester = DbTester.create(System2.INSTANCE); - private DbClient dbClient = dbTester.getDbClient(); - - private DbSession dbSession = dbTester.getSession(); - - private ThreadLocalUserSession userSession = mock(ThreadLocalUserSession.class); - + private ThreadLocalUserSession threadLocalSession = mock(ThreadLocalUserSession.class); private HttpServletRequest request = mock(HttpServletRequest.class); private HttpServletResponse response = mock(HttpServletResponse.class); - private Authenticators authenticators = mock(Authenticators.class); + private RequestAuthenticator authenticator = mock(RequestAuthenticator.class); private AuthenticationEvent authenticationEvent = mock(AuthenticationEvent.class); - private TestUserSessionFactory userSessionFactory = TestUserSessionFactory.standalone(); private MapSettings settings = new MapSettings(); - private UserDto user = newUserDto(); - private UserSessionInitializer underTest = new UserSessionInitializer(settings.asConfig(), userSession, authenticationEvent, userSessionFactory, authenticators); + private UserSessionInitializer underTest = new UserSessionInitializer(settings.asConfig(), threadLocalSession, authenticationEvent, authenticator); @Before public void setUp() throws Exception { - dbClient.userDao().insert(dbSession, user); - dbSession.commit(); when(request.getContextPath()).thenReturn(""); when(request.getRequestURI()).thenReturn("/measures"); } @@ -120,15 +106,15 @@ public class UserSessionInitializerTest { @Test public void return_code_401_when_not_authenticated_and_with_force_authentication() { ArgumentCaptor exceptionArgumentCaptor = ArgumentCaptor.forClass(AuthenticationException.class); - when(userSession.isLoggedIn()).thenReturn(false); - when(authenticators.authenticate(request, response)).thenReturn(Optional.empty()); + when(threadLocalSession.isLoggedIn()).thenReturn(false); + when(authenticator.authenticate(request, response)).thenReturn(new AnonymousMockUserSession()); settings.setProperty("sonar.forceAuthentication", true); assertThat(underTest.initUserSession(request, response)).isTrue(); verifyZeroInteractions(response); verify(authenticationEvent).loginFailure(eq(request), exceptionArgumentCaptor.capture()); - verifyZeroInteractions(userSession); + verifyZeroInteractions(threadLocalSession); AuthenticationException authenticationException = exceptionArgumentCaptor.getValue(); assertThat(authenticationException.getSource()).isEqualTo(Source.local(Method.BASIC)); assertThat(authenticationException.getLogin()).isNull(); @@ -140,31 +126,31 @@ public class UserSessionInitializerTest { public void return_401_and_stop_on_ws() { when(request.getRequestURI()).thenReturn("/api/issues"); AuthenticationException authenticationException = AuthenticationException.newBuilder().setSource(Source.jwt()).setMessage("Token id hasn't been found").build(); - doThrow(authenticationException).when(authenticators).authenticate(request, response); + doThrow(authenticationException).when(authenticator).authenticate(request, response); assertThat(underTest.initUserSession(request, response)).isFalse(); verify(response).setStatus(401); verify(authenticationEvent).loginFailure(request, authenticationException); - verifyZeroInteractions(userSession); + verifyZeroInteractions(threadLocalSession); } @Test public void return_401_and_stop_on_batch_ws() { when(request.getRequestURI()).thenReturn("/batch/global"); doThrow(AuthenticationException.newBuilder().setSource(Source.jwt()).setMessage("Token id hasn't been found").build()) - .when(authenticators).authenticate(request, response); + .when(authenticator).authenticate(request, response); assertThat(underTest.initUserSession(request, response)).isFalse(); verify(response).setStatus(401); - verifyZeroInteractions(userSession); + verifyZeroInteractions(threadLocalSession); } @Test public void return_to_session_unauthorized_when_error_on_from_external_provider() throws Exception { doThrow(AuthenticationException.newBuilder().setSource(Source.external(newBasicIdentityProvider("failing"))).setPublicMessage("Token id hasn't been found").build()) - .when(authenticators).authenticate(request, response); + .when(authenticator).authenticate(request, response); assertThat(underTest.initUserSession(request, response)).isFalse(); @@ -175,7 +161,7 @@ public class UserSessionInitializerTest { public void return_to_session_unauthorized_when_error_on_from_external_provider_with_context_path() throws Exception { when(request.getContextPath()).thenReturn("/sonarqube"); doThrow(AuthenticationException.newBuilder().setSource(Source.external(newBasicIdentityProvider("failing"))).setPublicMessage("Token id hasn't been found").build()) - .when(authenticators).authenticate(request, response); + .when(authenticator).authenticate(request, response); assertThat(underTest.initUserSession(request, response)).isFalse(); @@ -187,27 +173,30 @@ public class UserSessionInitializerTest { assertThat(underTest.initUserSession(request, response)).isTrue(); - verifyZeroInteractions(userSession, authenticators); - reset(userSession, authenticators); + verifyZeroInteractions(threadLocalSession, authenticator); + reset(threadLocalSession, authenticator); } private void assertPathIsIgnoredWithAnonymousAccess(String path) { when(request.getRequestURI()).thenReturn(path); + UserSession session = new AnonymousMockUserSession(); + when(authenticator.authenticate(request, response)).thenReturn(session); assertThat(underTest.initUserSession(request, response)).isTrue(); - verify(userSession).set(any(UserSession.class)); - reset(userSession, authenticators); + verify(threadLocalSession).set(session); + reset(threadLocalSession, authenticator); } private void assertPathIsNotIgnored(String path) { when(request.getRequestURI()).thenReturn(path); - when(authenticators.authenticate(request, response)).thenReturn(Optional.of(user)); + UserSession session = new MockUserSession("foo"); + when(authenticator.authenticate(request, response)).thenReturn(session); assertThat(underTest.initUserSession(request, response)).isTrue(); - verify(userSession).set(any(UserSession.class)); - reset(userSession, authenticators); + verify(threadLocalSession).set(session); + reset(threadLocalSession, authenticator); } private static BaseIdentityProvider newBasicIdentityProvider(String name) { -- 2.39.5