Merge branch 'stable-3.3' into stable-3.4

* stable-3.3:
  Allow GerritAccount Cookie authentication for Git/HTTP

Change-Id: I124dbbb2a325d4ba23e9bf8794f16d1f23f0e8fe

8 files changed, 160 insertions, 54 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 1052c8e534..5f2ea09e0a 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -604,7 +604,14 @@ it does not match, it is then validated against the `LDAP` password.
 By default this is set to `LDAP` when link:#auth.type[`auth.type`] is `LDAP`
 and `OAUTH` when link:#auth.type[`auth.type`] is `OAUTH`.
 Otherwise, the default value is `HTTP`.
-
++
+When gitBasicAuthPolicy is set to `LDAP` or `HTTP_LDAP` and the user
+is authenticating with the LDAP username/password, the Git client config
+needs to have `http.cookieFile` set to a local file, otherwise every
+single call would trigger a full LDAP authentication and groups resolution
+which could introduce a noticeable latency on the overall execution
+and produce unwanted load to the LDAP server.
++
 [[auth.gitOAuthProvider]]auth.gitOAuthProvider::
 +
 Selects the OAuth 2 provider to authenticate git over HTTP traffic with.
diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt
index cdaf155e05..0670968dd5 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -29,6 +29,13 @@ credentials are validated using:
 * Both, the HTTP and the LDAP passwords (in this order) if `gitBasicAuthPolicy`
   is `HTTP_LDAP`.
 
+When gitBasicAuthPolicy is set to `LDAP` or `HTTP_LDAP` and the user
+is authenticating with the LDAP username/password, the Git client config
+needs to have `http.cookieFile` set to a local file, otherwise every
+single call would trigger a full LDAP authentication and groups resolution
+which could introduce a noticeable latency on the overall execution
+and produce unwanted load to the LDAP server.
+
 When gitBasicAuthPolicy is not `LDAP`, the user's HTTP credentials can
 be regenerated by going to `Settings`, and then accessing the `HTTP
 Password` tab. Revocation can effectively be done by regenerating the
diff --git a/java/com/google/gerrit/httpd/CacheBasedWebSession.java b/java/com/google/gerrit/httpd/CacheBasedWebSession.java
index a3a67e5d78..7212e3e6e6 100644
--- a/java/com/google/gerrit/httpd/CacheBasedWebSession.java
+++ b/java/com/google/gerrit/httpd/CacheBasedWebSession.java
@@ -76,30 +76,28 @@ public abstract class CacheBasedWebSession implements WebSession {
     this.identified = identified;
     this.byIdCache = byIdCache;
 
-    if (request.getRequestURI() == null || !GitSmartHttpTools.isGitClient(request)) {
-      String cookie = readCookie(request);
-      if (cookie != null) {
-        authFromCookie(cookie);
-      } else {
-        String token;
-        try {
-          token = ParameterParser.getQueryParams(request).accessToken();
-        } catch (BadRequestException e) {
-          token = null;
-        }
-        if (token != null) {
-          authFromQueryParameter(token);
-        }
-      }
-      if (val != null && !checkAccountStatus(val.getAccountId())) {
-        val = null;
-        okPaths.clear();
+    String cookie = readCookie(request);
+    if (cookie != null) {
+      authFromCookie(cookie);
+    } else if (request.getRequestURI() == null || !GitSmartHttpTools.isGitClient(request)) {
+      String token;
+      try {
+        token = ParameterParser.getQueryParams(request).accessToken();
+      } catch (BadRequestException e) {
+        token = null;
       }
-      if (val != null && val.needsCookieRefresh()) {
-        // Session is more than half old; update cache entry with new expiration date.
-        val = manager.createVal(key, val);
+      if (token != null) {
+        authFromQueryParameter(token);
       }
     }
+    if (val != null && !checkAccountStatus(val.getAccountId())) {
+      val = null;
+      okPaths.clear();
+    }
+    if (val != null && val.needsCookieRefresh()) {
+      // Session is more than half old; update cache entry with new expiration date.
+      val = manager.createVal(key, val);
+    }
   }
 
   private void authFromCookie(String cookie) {
diff --git a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
index ca780d0a9d..7fcd4f8de9 100644
--- a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
+++ b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
@@ -22,6 +22,7 @@ import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.io.BaseEncoding;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.extensions.client.GitBasicAuthPolicy;
 import com.google.gerrit.extensions.registration.DynamicItem;
@@ -144,7 +145,7 @@ class ProjectBasicAuthFilter implements Filter {
     if (gitBasicAuthPolicy == GitBasicAuthPolicy.HTTP
         || gitBasicAuthPolicy == GitBasicAuthPolicy.HTTP_LDAP) {
       if (PasswordVerifier.checkPassword(who.externalIds(), username, password)) {
-        return succeedAuthentication(who);
+        return succeedAuthentication(who, null);
       }
     }
 
@@ -157,11 +158,11 @@ class ProjectBasicAuthFilter implements Filter {
 
     try {
       AuthResult whoAuthResult = accountManager.authenticate(whoAuth);
-      setUserIdentified(whoAuthResult.getAccountId());
+      setUserIdentified(whoAuthResult.getAccountId(), whoAuthResult);
       return true;
     } catch (NoSuchUserException e) {
       if (PasswordVerifier.checkPassword(who.externalIds(), username, password)) {
-        return succeedAuthentication(who);
+        return succeedAuthentication(who, null);
       }
       logger.atWarning().withCause(e).log(authenticationFailedMsg(username, req));
       rsp.sendError(SC_UNAUTHORIZED);
@@ -183,8 +184,8 @@ class ProjectBasicAuthFilter implements Filter {
     }
   }
 
-  private boolean succeedAuthentication(AccountState who) {
-    setUserIdentified(who.account().id());
+  private boolean succeedAuthentication(AccountState who, @Nullable AuthResult whoAuthResult) {
+    setUserIdentified(who.account().id(), whoAuthResult);
     return true;
   }
 
@@ -201,11 +202,15 @@ class ProjectBasicAuthFilter implements Filter {
     return String.format("Authentication from %s failed for %s", req.getRemoteAddr(), username);
   }
 
-  private void setUserIdentified(Account.Id id) {
+  private void setUserIdentified(Account.Id id, @Nullable AuthResult whoAuthResult) {
     WebSession ws = session.get();
     ws.setUserAccountId(id);
     ws.setAccessPathOk(AccessPath.GIT, true);
     ws.setAccessPathOk(AccessPath.REST_API, true);
+
+    if (whoAuthResult != null) {
+      ws.login(whoAuthResult, false);
+    }
   }
 
   private String encoding(HttpServletRequest req) {
diff --git a/javatests/com/google/gerrit/httpd/BUILD b/javatests/com/google/gerrit/httpd/BUILD
index 75f005e62a..121cbc482c 100644
--- a/javatests/com/google/gerrit/httpd/BUILD
+++ b/javatests/com/google/gerrit/httpd/BUILD
@@ -18,6 +18,7 @@ junit_tests(
         "//lib:junit",
         "//lib:servlet-api-without-neverlink",
         "//lib:soy",
+        "//lib/bouncycastle:bcprov",
         "//lib/guice",
         "//lib/guice:guice-servlet",
         "//lib/mockito",
diff --git a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
index f7792ed2d3..735abbf66d 100644
--- a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
+++ b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
@@ -19,12 +19,15 @@ import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.extensions.client.GitBasicAuthPolicy;
 import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountException;
 import com.google.gerrit.server.account.AccountManager;
@@ -55,11 +58,17 @@ public class ProjectBasicAuthFilterTest {
   private static final String AUTH_USER_B64 =
       B64_ENC.encodeToString(AUTH_USER.getBytes(StandardCharsets.UTF_8));
   private static final String AUTH_PASSWORD = "jd123";
+  private static final String GERRIT_COOKIE_KEY = "GerritAccount";
+  private static final String AUTH_COOKIE_VALUE = "gerritcookie";
+  private static final ExternalId AUTH_USER_PASSWORD_EXTERNAL_ID =
+      ExternalId.createWithPassword(
+          ExternalId.Key.create(ExternalId.SCHEME_USERNAME, AUTH_USER),
+          AUTH_ACCOUNT_ID,
+          null,
+          AUTH_PASSWORD);
 
   @Mock private DynamicItem<WebSession> webSessionItem;
 
-  @Mock private WebSession webSession;
-
   @Mock private AccountCache accountCache;
 
   @Mock private AccountState accountState;
@@ -74,21 +83,57 @@ public class ProjectBasicAuthFilterTest {
 
   @Captor private ArgumentCaptor<HttpServletResponse> filterResponseCaptor;
 
+  @Mock private IdentifiedUser.RequestFactory userRequestFactory;
+
+  @Mock private WebSessionManager webSessionManager;
+
+  private WebSession webSession;
   private FakeHttpServletRequest req;
   private HttpServletResponse res;
+  private AuthResult authSuccessful;
 
   @Before
   public void setUp() throws Exception {
-    doReturn(webSession).when(webSessionItem).get();
+    req = new FakeHttpServletRequest();
+    res = new FakeHttpServletResponse();
+
+    authSuccessful =
+        new AuthResult(AUTH_ACCOUNT_ID, ExternalId.Key.create("username", AUTH_USER), false);
     doReturn(Optional.of(accountState)).when(accountCache).getByUsername(AUTH_USER);
+    doReturn(Optional.of(accountState)).when(accountCache).get(AUTH_ACCOUNT_ID);
     doReturn(account).when(accountState).account();
+    doReturn(ImmutableSet.builder().add(AUTH_USER_PASSWORD_EXTERNAL_ID).build())
+        .when(accountState)
+        .externalIds();
+
+    doReturn(new WebSessionManager.Key(AUTH_COOKIE_VALUE)).when(webSessionManager).createKey(any());
+    WebSessionManager.Val webSessionValue =
+        new WebSessionManager.Val(AUTH_ACCOUNT_ID, 0L, false, null, 0L, "", "");
+    doReturn(webSessionValue)
+        .when(webSessionManager)
+        .createVal(any(), any(), eq(false), any(), any(), any());
+  }
 
-    req = new FakeHttpServletRequest();
-    res = new FakeHttpServletResponse();
+  private void initWebSessionWithCookie(String cookie) {
+    req.addHeader("Cookie", cookie);
+    initWebSessionWithoutCookie();
+  }
+
+  private void initWebSessionWithoutCookie() {
+    webSession =
+        new CacheBasedWebSession(
+            req, res, webSessionManager, authConfig, null, userRequestFactory, accountCache) {};
+    doReturn(webSession).when(webSessionItem).get();
+  }
+
+  private void initMockedWebSession() {
+    webSession = mock(WebSession.class);
+    doReturn(webSession).when(webSessionItem).get();
   }
 
   @Test
   public void shouldAllowAnonymousRequest() throws Exception {
+    initMockedWebSession();
     res.setStatus(HttpServletResponse.SC_OK);
 
     ProjectBasicAuthFilter basicAuthFilter =
@@ -102,6 +147,7 @@ public class ProjectBasicAuthFilterTest {
 
   @Test
   public void shouldRequestAuthenticationForBasicAuthRequest() throws Exception {
+    initMockedWebSession();
     req.addHeader("Authorization", "Basic " + AUTH_USER_B64);
     res.setStatus(HttpServletResponse.SC_OK);
 
@@ -116,16 +162,11 @@ public class ProjectBasicAuthFilterTest {
   }
 
   @Test
-  public void shouldAuthenticateSucessfullyAgainstRealm() throws Exception {
-    req.addHeader(
-        "Authorization",
-        "Basic "
-            + B64_ENC.encodeToString(
-                (AUTH_USER + ":" + AUTH_PASSWORD).getBytes(StandardCharsets.UTF_8)));
+  public void shouldAuthenticateSucessfullyAgainstRealmAndReturnCookie() throws Exception {
+    initWebSessionWithoutCookie();
+    requestBasicAuth(req);
     res.setStatus(HttpServletResponse.SC_OK);
 
-    AuthResult authSuccessful =
-        new AuthResult(AUTH_ACCOUNT_ID, ExternalId.Key.create("username", AUTH_USER), false);
     doReturn(true).when(account).isActive();
     doReturn(authSuccessful).when(accountManager).authenticate(any());
     doReturn(GitBasicAuthPolicy.LDAP).when(authConfig).getGitBasicAuthPolicy();
@@ -139,18 +180,51 @@ public class ProjectBasicAuthFilterTest {
 
     verify(chain).doFilter(eq(req), any());
     assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
+    assertThat(res.getHeader("Set-Cookie")).contains(GERRIT_COOKIE_KEY);
   }
 
   @Test
-  public void shouldNotReauthenticateIfAlreadySignedIn() throws Exception {
-    req.addHeader(
-        "Authorization",
-        "Basic "
-            + B64_ENC.encodeToString(
-                (AUTH_USER + ":" + AUTH_PASSWORD).getBytes(StandardCharsets.UTF_8)));
+  public void shouldValidateUserPasswordAndNotReturnCookie() throws Exception {
+    initWebSessionWithoutCookie();
+    requestBasicAuth(req);
     res.setStatus(HttpServletResponse.SC_OK);
 
+    doReturn(true).when(account).isActive();
+    doReturn(GitBasicAuthPolicy.HTTP).when(authConfig).getGitBasicAuthPolicy();
+
+    ProjectBasicAuthFilter basicAuthFilter =
+        new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig);
+
+    basicAuthFilter.doFilter(req, res, chain);
+
+    verify(accountManager, never()).authenticate(any());
+
+    verify(chain).doFilter(eq(req), any());
+    assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
+    assertThat(res.getHeader("Set-Cookie")).isNull();
+  }
+
+  @Test
+  public void shouldNotReauthenticateIfAlreadySignedIn() throws Exception {
+    initMockedWebSession();
     doReturn(true).when(webSession).isSignedIn();
+    requestBasicAuth(req);
+    res.setStatus(HttpServletResponse.SC_OK);
+
+    ProjectBasicAuthFilter basicAuthFilter =
+        new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig);
+
+    basicAuthFilter.doFilter(req, res, chain);
+
+    verify(accountManager, never()).authenticate(any());
+    verify(chain).doFilter(eq(req), any());
+    assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
+  }
+
+  @Test
+  public void shouldNotReauthenticateIfHasExistingCookie() throws Exception {
+    initWebSessionWithCookie("GerritAccount=" + AUTH_COOKIE_VALUE);
+    res.setStatus(HttpServletResponse.SC_OK);
 
     ProjectBasicAuthFilter basicAuthFilter =
         new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig);
@@ -164,11 +238,8 @@ public class ProjectBasicAuthFilterTest {
 
   @Test
   public void shouldFailedAuthenticationAgainstRealm() throws Exception {
-    req.addHeader(
-        "Authorization",
-        "Basic "
-            + B64_ENC.encodeToString(
-                (AUTH_USER + ":" + AUTH_PASSWORD).getBytes(StandardCharsets.UTF_8)));
+    initMockedWebSession();
+    requestBasicAuth(req);
 
     doReturn(true).when(account).isActive();
     doThrow(new AccountException("Authentication error")).when(accountManager).authenticate(any());
@@ -184,4 +255,12 @@ public class ProjectBasicAuthFilterTest {
     verify(chain, never()).doFilter(any(), any());
     assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
   }
+
+  private void requestBasicAuth(FakeHttpServletRequest fakeReq) {
+    fakeReq.addHeader(
+        "Authorization",
+        "Basic "
+            + B64_ENC.encodeToString(
+                (AUTH_USER + ":" + AUTH_PASSWORD).getBytes(StandardCharsets.UTF_8)));
+  }
 }
diff --git a/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java b/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java
index a4175e3eb7..2efa94b7c3 100644
--- a/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java
+++ b/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java
@@ -17,6 +17,7 @@ package com.google.gerrit.util.http.testutil;
 import static com.google.common.base.Preconditions.checkArgument;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toList;
 
 import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
@@ -257,7 +258,15 @@ public class FakeHttpServletRequest implements HttpServletRequest {
 
   @Override
   public Cookie[] getCookies() {
-    return new Cookie[0];
+    return Splitter.on(";").splitToList(Strings.nullToEmpty(getHeader("Cookie"))).stream()
+        .filter(s -> !s.isEmpty())
+        .map(
+            (String cookieValue) -> {
+              String[] kv = cookieValue.split("=");
+              return new Cookie(kv[0], kv[1]);
+            })
+        .collect(toList())
+        .toArray(new Cookie[0]);
   }
 
   @Override
diff --git a/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletResponse.java b/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletResponse.java
index 9a98ecdcaa..f39b875500 100644
--- a/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletResponse.java
+++ b/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletResponse.java
@@ -161,7 +161,7 @@ public class FakeHttpServletResponse implements HttpServletResponse {
 
   @Override
   public void addCookie(Cookie cookie) {
-    throw new UnsupportedOperationException();
+    addHeader("Set-Cookie", cookie.getName() + "=" + cookie.getValue());
   }
 
   @Override