SONARCLOUD-213 refactor Authenticators to return UserSession

- Authenticators renamed RequestAuthenticator

- the logic between UserDto and UserSession is moved outside
  UserSessionInitializer

7 files changed, 86 insertions, 84 deletions

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<String, String> 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
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<UserDto> 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
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<UserDto> authenticate(HttpServletRequest request, HttpServletResponse response) {
+  public UserSession authenticate(HttpServletRequest request, HttpServletResponse response) {
+    Optional<UserDto> userOpt = loadUser(request, response);
+    if (userOpt.isPresent()) {
+      return userSessionFactory.create(userOpt.get());
+    }
+    return userSessionFactory.createAnonymous();
+  }
+
+  private Optional<UserDto> 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<UserDto> 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<String> 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<UserDto> 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
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<AuthenticationException> 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) {