Allow GerritAccount Cookie authentication for Git/HTTP

Since the introduction of basic auth with Change-Id: Ibe589ab2b0,
the mechanism of keeping a session (it was a digest before)
across calls has not been preserved and the basic-auth implementation
resulted in multiple authentications with the configured realm.
Triggering a full authentication handshake could be an
issue when using potentially expensive authentication backends
like LDAP.

Allow to create a Gerrit session from the GerritAccount
cookie set on the Git client, so that only the first HTTP call
will actually authenticate and create a session whilst all
the others would just reuse the existing cookie.

The Git client needs to have HTTP cookies enabled by setting
the http.cookieFile in Git config pointing to a local
file. For keeping HTTP cookies across Git/HTTP commands, the
extra http.saveCookie Git config variable needs to be set to
true.

Previously all Git/HTTP requests were ignored for parsing
the GerritAccount cookie whilst now they are excluded only when
the account token is passed as URL parameter. This problem was
there since the very beginning of the introduction of Git/HTTP
basic auth.

NOTE: Gerrit does not generate HTTP cookies when using
password-based authentication against the external-ids rather
than using the realm: that is expected because it would not
be correct to allocate a cookie when a real authentication
against the realm has not been performed, therefore the
cookie-based authentication for Git/HTTP would not be
available.

Bug: Issue 14508
Change-Id: I2a56197ee0dad479f0973192157e5970d9deac25

7 files changed, 159 insertions, 54 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 53993b2673..bcb13fe5a7 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -582,7 +582,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 cb58ad9e12..d55cb48c00 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -28,6 +28,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 5c4830c617..3a84a296f8 100644
--- a/java/com/google/gerrit/httpd/CacheBasedWebSession.java
+++ b/java/com/google/gerrit/httpd/CacheBasedWebSession.java
@@ -75,30 +75,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 55ffef797b..c1931157fc 100644
--- a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
+++ b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
@@ -21,6 +21,7 @@ import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
 import com.google.common.flogger.FluentLogger;
+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/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