Fix PUT/POST/DELETE REST-API with cookie authentication

Change-Id: I2a56197ee0 has broken existing Python (or other)
scripting when performing automation with Gerrit REST-API.
That is due to the generation of the GerritAccount cookie in
the HTTP response, which Python automatically manages
to reuse in subsequent calls.

Gerrit REST-API have a stricter requirement for incoming calls
that are not GET or HEAD requests: they need the X-Gerrit-Auth
HTTP header matching the associated attribute in the user's session.
When the X-Gerrit-Auth header isn't there OR does not correspond
to the user's session, the REST-API execution fails with
403 FORBIDDEN even though the user has an active session associated
with the cookie.

Python has no way to manage that logic out of the box and therefore
it is the responsibility of the Gerrit backend to request explicit
authentication when the incoming call isn't from a Git/HTTP client.

For the Git/HTTP requests instead, the requirement for X-Gerrit-Auth
isn't there and therefore, the current cookie-based authentication can
continue to be used as usual and won't cause any trouble.

Bug: Issue 14553
Change-Id: I62a7a59b07333eeb1a36d4a6b8b67edd5da76440

3 files changed, 92 insertions, 38 deletions

diff --git a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
index c1931157fc..1974ba78b1 100644
--- a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
+++ b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
@@ -52,6 +52,7 @@ import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpServletResponseWrapper;
 import org.apache.commons.codec.binary.Base64;
+import org.eclipse.jgit.http.server.GitSmartHttpTools;
 
 /**
  * Authenticates the current user by HTTP basic authentication.
@@ -100,11 +101,21 @@ class ProjectBasicAuthFilter implements Filter {
     HttpServletRequest req = (HttpServletRequest) request;
     Response rsp = new Response((HttpServletResponse) response);
 
-    if (session.get().isSignedIn() || verify(req, rsp)) {
+    if (isSignedInGitRequest(req) || verify(req, rsp)) {
       chain.doFilter(req, rsp);
     }
   }
 
+  private boolean isSignedInGitRequest(HttpServletRequest req) {
+    boolean isGitRequest = req.getRequestURI() != null && GitSmartHttpTools.isGitClient(req);
+    boolean isAlreadySignedIn = session.get().isSignedIn();
+    boolean res = isAlreadySignedIn && isGitRequest;
+    logger.atFine().log(
+        "HTTP:%s %s signedIn=%s (isAlreadySignedIn=%s, isGitRequest=%s)",
+        req.getMethod(), req.getRequestURI(), res, isAlreadySignedIn, isGitRequest);
+    return res;
+  }
+
   private boolean verify(HttpServletRequest req, Response rsp) throws IOException {
     final String hdr = req.getHeader(AUTHORIZATION);
     if (hdr == null || !hdr.startsWith(LIT_BASIC)) {
@@ -145,6 +156,9 @@ class ProjectBasicAuthFilter implements Filter {
     if (gitBasicAuthPolicy == GitBasicAuthPolicy.HTTP
         || gitBasicAuthPolicy == GitBasicAuthPolicy.HTTP_LDAP) {
       if (PasswordVerifier.checkPassword(who.externalIds(), username, password)) {
+        logger.atFine().log(
+            "HTTP:%s %s username/password authentication succeeded",
+            req.getMethod(), req.getRequestURI());
         return succeedAuthentication(who, null);
       }
     }
@@ -159,6 +173,8 @@ class ProjectBasicAuthFilter implements Filter {
     try {
       AuthResult whoAuthResult = accountManager.authenticate(whoAuth);
       setUserIdentified(whoAuthResult.getAccountId(), whoAuthResult);
+      logger.atFine().log(
+          "HTTP:%s %s Realm authentication succeeded", req.getMethod(), req.getRequestURI());
       return true;
     } catch (NoSuchUserException e) {
       if (PasswordVerifier.checkPassword(who.externalIds(), username, password)) {
diff --git a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
index 735abbf66d..2f0fafa00b 100644
--- a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
+++ b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
@@ -37,10 +37,12 @@ import com.google.gerrit.server.account.externalids.ExternalId;
 import com.google.gerrit.server.config.AuthConfig;
 import com.google.gerrit.util.http.testutil.FakeHttpServletRequest;
 import com.google.gerrit.util.http.testutil.FakeHttpServletResponse;
+import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.Base64;
 import java.util.Optional;
 import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletResponse;
 import org.junit.Before;
 import org.junit.Test;
@@ -94,7 +96,7 @@ public class ProjectBasicAuthFilterTest {
 
   @Before
   public void setUp() throws Exception {
-    req = new FakeHttpServletRequest();
+    req = new FakeHttpServletRequest("gerrit.example.com", 80, "", "");
     res = new FakeHttpServletResponse();
 
     authSuccessful =
@@ -102,9 +104,8 @@ public class ProjectBasicAuthFilterTest {
     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(true).when(account).isActive();
+    doReturn(authSuccessful).when(accountManager).authenticate(any());
 
     doReturn(new WebSessionManager.Key(AUTH_COOKIE_VALUE)).when(webSessionManager).createKey(any());
     WebSessionManager.Val webSessionValue =
@@ -114,23 +115,6 @@ public class ProjectBasicAuthFilterTest {
         .createVal(any(), any(), eq(false), any(), any(), any());
   }
 
-  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();
@@ -168,7 +152,6 @@ public class ProjectBasicAuthFilterTest {
     res.setStatus(HttpServletResponse.SC_OK);
 
     doReturn(true).when(account).isActive();
-    doReturn(authSuccessful).when(accountManager).authenticate(any());
     doReturn(GitBasicAuthPolicy.LDAP).when(authConfig).getGitBasicAuthPolicy();
 
     ProjectBasicAuthFilter basicAuthFilter =
@@ -187,10 +170,9 @@ public class ProjectBasicAuthFilterTest {
   public void shouldValidateUserPasswordAndNotReturnCookie() throws Exception {
     initWebSessionWithoutCookie();
     requestBasicAuth(req);
-    res.setStatus(HttpServletResponse.SC_OK);
-
-    doReturn(true).when(account).isActive();
+    initMockedUsernamePasswordExternalId();
     doReturn(GitBasicAuthPolicy.HTTP).when(authConfig).getGitBasicAuthPolicy();
+    res.setStatus(HttpServletResponse.SC_OK);
 
     ProjectBasicAuthFilter basicAuthFilter =
         new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig);
@@ -205,33 +187,40 @@ public class ProjectBasicAuthFilterTest {
   }
 
   @Test
-  public void shouldNotReauthenticateIfAlreadySignedIn() throws Exception {
-    initMockedWebSession();
-    doReturn(true).when(webSession).isSignedIn();
-    requestBasicAuth(req);
-    res.setStatus(HttpServletResponse.SC_OK);
+  public void shouldNotReauthenticateForGitPostRequest() throws Exception {
+    req.setPathInfo("/a/project.git/git-upload-pack");
+    req.setMethod("POST");
+    req.addHeader("Content-Type", "application/x-git-upload-pack-request");
+    doFilterForRequestWhenAlreadySignedIn();
 
-    ProjectBasicAuthFilter basicAuthFilter =
-        new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig);
+    verify(accountManager, never()).authenticate(any());
+    verify(chain).doFilter(eq(req), any());
+    assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
+  }
 
-    basicAuthFilter.doFilter(req, res, chain);
+  @Test
+  public void shouldReauthenticateForRegularRequestEvenIfAlreadySignedIn() throws Exception {
+    doReturn(GitBasicAuthPolicy.LDAP).when(authConfig).getGitBasicAuthPolicy();
+    doFilterForRequestWhenAlreadySignedIn();
 
-    verify(accountManager, never()).authenticate(any());
+    verify(accountManager).authenticate(any());
     verify(chain).doFilter(eq(req), any());
     assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
   }
 
   @Test
-  public void shouldNotReauthenticateIfHasExistingCookie() throws Exception {
+  public void shouldReauthenticateEvenIfHasExistingCookie() throws Exception {
     initWebSessionWithCookie("GerritAccount=" + AUTH_COOKIE_VALUE);
     res.setStatus(HttpServletResponse.SC_OK);
+    requestBasicAuth(req);
+    doReturn(GitBasicAuthPolicy.LDAP).when(authConfig).getGitBasicAuthPolicy();
 
     ProjectBasicAuthFilter basicAuthFilter =
         new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig);
 
     basicAuthFilter.doFilter(req, res, chain);
 
-    verify(accountManager, never()).authenticate(any());
+    verify(accountManager).authenticate(any());
     verify(chain).doFilter(eq(req), any());
     assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
   }
@@ -256,6 +245,44 @@ public class ProjectBasicAuthFilterTest {
     assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
   }
 
+  private void doFilterForRequestWhenAlreadySignedIn()
+      throws IOException, ServletException, AccountException {
+    initMockedWebSession();
+    doReturn(true).when(account).isActive();
+    doReturn(true).when(webSession).isSignedIn();
+    doReturn(authSuccessful).when(accountManager).authenticate(any());
+    requestBasicAuth(req);
+    res.setStatus(HttpServletResponse.SC_OK);
+
+    ProjectBasicAuthFilter basicAuthFilter =
+        new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig);
+
+    basicAuthFilter.doFilter(req, res, chain);
+  }
+
+  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();
+  }
+
+  private void initMockedUsernamePasswordExternalId() {
+    doReturn(ImmutableSet.builder().add(AUTH_USER_PASSWORD_EXTERNAL_ID).build())
+        .when(accountState)
+        .externalIds();
+  }
+
   private void requestBasicAuth(FakeHttpServletRequest fakeReq) {
     fakeReq.addHeader(
         "Authorization",
diff --git a/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java b/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java
index 2efa94b7c3..0bb4de493b 100644
--- a/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java
+++ b/javatests/com/google/gerrit/util/http/testutil/FakeHttpServletRequest.java
@@ -67,6 +67,7 @@ public class FakeHttpServletRequest implements HttpServletRequest {
   private String contextPath;
   private String servletPath;
   private String path;
+  private String method;
 
   public FakeHttpServletRequest() {
     this("gerrit.example.com", 80, "", SERVLET_PATH);
@@ -81,6 +82,7 @@ public class FakeHttpServletRequest implements HttpServletRequest {
     attributes = Maps.newConcurrentMap();
     parameters = LinkedListMultimap.create();
     headers = LinkedListMultimap.create();
+    method = "GET";
   }
 
   @Override
@@ -105,6 +107,11 @@ public class FakeHttpServletRequest implements HttpServletRequest {
 
   @Override
   public String getContentType() {
+    List<String> contentType = headers.get("Content-Type");
+    if (contentType != null && !contentType.isEmpty()) {
+      return contentType.get(0);
+    }
+
     return null;
   }
 
@@ -297,7 +304,11 @@ public class FakeHttpServletRequest implements HttpServletRequest {
 
   @Override
   public String getMethod() {
-    return "GET";
+    return method;
+  }
+
+  public void setMethod(String method) {
+    this.method = method;
   }
 
   @Override