ProjectBasicAuthFilter: check account by username scheme for HTTP*upstream/stable-3.9

The ProjectBasicAuthFilter authenticates the incoming requests using
regular username/password for batch REST-API or Git/HTTP protocol calls.

When the authentication protocol is set to HTTP or HTTP_LDAP, it is
correct to expect that the username specified in the HTTP request is the
one present in the external-ids with the 'username' scheme.

If the basic authentication protocol is set to LDAP, looking up the user
with the 'username' scheme is incorrect, because the scheme used by LDAP
authentication is the 'gerrit' scheme.

Move the account lookup by username inside the condition of
GitBasicAuthPolicy set to HTTP or HTTP_LDAP, for preventing the LDAP
authentication requests to be checked against the 'username' scheme for
account existence.

Bug: Issue 439435039
Release-Notes: Check for account existence with username scheme for HTTP and HTTP_LDAP only
Change-Id: I1bbe545ca8777d7b0085fa6ced76d7872dcebc26

2 files changed, 61 insertions, 12 deletions

diff --git a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
index b0c7615ada..7cb23571d1 100644
--- a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
+++ b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
@@ -148,19 +148,20 @@ class ProjectBasicAuthFilter implements Filter {
       username = username.toLowerCase(Locale.US);
     }
 
-    Optional<AccountState> accountState =
-        accountCache.getByUsername(username).filter(a -> a.account().isActive());
-    if (!accountState.isPresent()) {
-      logger.atWarning().log(
-          "Authentication failed for %s: account inactive or not provisioned in Gerrit", username);
-      rsp.sendError(SC_UNAUTHORIZED);
-      return false;
-    }
-
-    AccountState who = accountState.get();
     GitBasicAuthPolicy gitBasicAuthPolicy = authConfig.getGitBasicAuthPolicy();
     if (gitBasicAuthPolicy == GitBasicAuthPolicy.HTTP
         || gitBasicAuthPolicy == GitBasicAuthPolicy.HTTP_LDAP) {
+      Optional<AccountState> accountState =
+          accountCache.getByUsername(username).filter(a -> a.account().isActive());
+      if (!accountState.isPresent()) {
+        logger.atWarning().log(
+            "Authentication failed for %s: account inactive or not provisioned in Gerrit",
+            username);
+        rsp.sendError(SC_UNAUTHORIZED);
+        return false;
+      }
+
+      AccountState who = accountState.get();
       if (passwordVerifier.checkPassword(who.externalIds(), username, password)) {
         logger.atFine().log(
             "HTTP:%s %s username/password authentication succeeded",
@@ -183,8 +184,16 @@ class ProjectBasicAuthFilter implements Filter {
           "HTTP:%s %s Realm authentication succeeded", req.getMethod(), req.getRequestURI());
       return true;
     } catch (NoSuchUserException e) {
-      if (passwordVerifier.checkPassword(who.externalIds(), username, password)) {
-        return succeedAuthentication(who, null);
+      // We are here in a condition where the basic authentication is set to LDAP and
+      // the user does not exist on the LDAP server. In theory this should trigger an
+      // authentication error; however, Edwin has introduced a fallback with I030c88108
+      // where the account is checked *also* for its HTTP password as a fallback, because
+      // of the need to still be able to authenticate service users.
+      Optional<AccountState> who =
+          accountCache.getByUsername(username).filter(a -> a.account().isActive());
+      if (who.isPresent()
+          && passwordVerifier.checkPassword(who.get().externalIds(), username, password)) {
+        return succeedAuthentication(who.get(), null);
       }
       logger.atWarning().withCause(e).log("%s", authenticationFailedMsg(username, req));
       rsp.sendError(SC_UNAUTHORIZED);
diff --git a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
index 0389c4f8a8..d67b6bd146 100644
--- a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
+++ b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
@@ -162,6 +162,46 @@ public class ProjectBasicAuthFilterTest {
   }
 
   @Test
+  public void shouldAuthenticateWithRealmEvenIfGerritUserDoesNotExist() throws Exception {
+    initWebSessionWithoutCookie();
+    requestBasicAuth(req);
+    doReturn(authSuccessful).when(accountManager).authenticate(any());
+    doReturn(GitBasicAuthPolicy.LDAP).when(authConfig).getGitBasicAuthPolicy();
+
+    ProjectBasicAuthFilter basicAuthFilter =
+        new ProjectBasicAuthFilter(
+            webSessionItem,
+            accountCache,
+            accountManager,
+            authConfig,
+            authRequestFactory,
+            pwdVerifier);
+
+    basicAuthFilter.doFilter(req, res, chain);
+    assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
+  }
+
+  @Test
+  public void shouldFailAuthenticationIfGerritUserDoesNotExist() throws Exception {
+    doReturn(Optional.empty()).when(accountCache).getByUsername(AUTH_USER);
+    initWebSessionWithoutCookie();
+    requestBasicAuth(req);
+    doReturn(GitBasicAuthPolicy.HTTP).when(authConfig).getGitBasicAuthPolicy();
+
+    ProjectBasicAuthFilter basicAuthFilter =
+        new ProjectBasicAuthFilter(
+            webSessionItem,
+            accountCache,
+            accountManager,
+            authConfig,
+            authRequestFactory,
+            pwdVerifier);
+
+    basicAuthFilter.doFilter(req, res, chain);
+    assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
+  }
+
+  @Test
   public void shouldAuthenticateSucessfullyAgainstRealmAndReturnCookie() throws Exception {
     initAccount();
     initWebSessionWithoutCookie();