From dfbc9d03bc2c98729af947a294858f72a51046d3 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 7 May 2021 00:46:29 +0100 Subject: [PATCH 1/3] Add unit-tests for ProjectBasicAuthFilter The Git/HTTP authentication filter did not have any test coverage: add some initial tests for verifying the main use-cases. Change-Id: Ib9abf133d2128b6a29751ecbeda26b0b43115bb3 --- javatests/com/google/gerrit/httpd/BUILD | 1 + .../httpd/ProjectBasicAuthFilterTest.java | 167 ++++++++++++++++++ 2 files changed, 168 insertions(+) create mode 100644 javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java diff --git a/javatests/com/google/gerrit/httpd/BUILD b/javatests/com/google/gerrit/httpd/BUILD index d7518907d3..75f005e62a 100644 --- a/javatests/com/google/gerrit/httpd/BUILD +++ b/javatests/com/google/gerrit/httpd/BUILD @@ -4,6 +4,7 @@ junit_tests( name = "httpd_tests", srcs = glob(["**/*.java"]), deps = [ + "//java/com/google/gerrit/entities", "//java/com/google/gerrit/extensions:api", "//java/com/google/gerrit/httpd", "//java/com/google/gerrit/server", diff --git a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java new file mode 100644 index 0000000000..1f06472f59 --- /dev/null +++ b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java @@ -0,0 +1,167 @@ +// Copyright (C) 2021 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.httpd; + +import static com.google.common.truth.Truth.assertThat; +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.never; +import static org.mockito.Mockito.verify; + +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.account.AccountCache; +import com.google.gerrit.server.account.AccountException; +import com.google.gerrit.server.account.AccountManager; +import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.account.AuthResult; +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.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.Optional; +import javax.servlet.FilterChain; +import javax.servlet.http.HttpServletResponse; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class ProjectBasicAuthFilterTest { + private static final Base64.Encoder B64_ENC = Base64.getEncoder(); + private static final Account.Id AUTH_ACCOUNT_ID = Account.id(1000); + private static final String AUTH_USER = "johndoe"; + private static final String AUTH_USER_B64 = + B64_ENC.encodeToString(AUTH_USER.getBytes(StandardCharsets.UTF_8)); + private static final String AUTH_PASSWORD = "jd123"; + + @Mock private DynamicItem webSessionItem; + + @Mock private WebSession webSession; + + @Mock private AccountCache accountCache; + + @Mock private AccountState accountState; + + @Mock private Account account; + + @Mock private AccountManager accountManager; + + @Mock private AuthConfig authConfig; + + @Mock private FilterChain chain; + + @Captor private ArgumentCaptor filterResponseCaptor; + + private FakeHttpServletRequest req; + private HttpServletResponse res; + + @Before + public void setUp() throws Exception { + doReturn(webSession).when(webSessionItem).get(); + doReturn(Optional.of(accountState)).when(accountCache).getByUsername(AUTH_USER); + doReturn(account).when(accountState).account(); + + req = new FakeHttpServletRequest(); + res = new FakeHttpServletResponse(); + } + + @Test + public void shouldAllowAnonymousRequest() throws Exception { + res.setStatus(HttpServletResponse.SC_OK); + + ProjectBasicAuthFilter basicAuthFilter = + new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig); + + basicAuthFilter.doFilter(req, res, chain); + + verify(chain).doFilter(eq(req), filterResponseCaptor.capture()); + assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK); + } + + @Test + public void shouldRequestAuthenticationForBasicAuthRequest() throws Exception { + req.addHeader("Authorization", "Basic " + AUTH_USER_B64); + res.setStatus(HttpServletResponse.SC_OK); + + ProjectBasicAuthFilter basicAuthFilter = + new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig); + + basicAuthFilter.doFilter(req, res, chain); + + verify(chain, never()).doFilter(any(), any()); + assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); + assertThat(res.getHeader("WWW-Authenticate")).contains("Basic realm="); + } + + @Test + public void shouldAuthenticateSucessfullyAgainstRealm() throws Exception { + req.addHeader( + "Authorization", + "Basic " + + B64_ENC.encodeToString( + (AUTH_USER + ":" + AUTH_PASSWORD).getBytes(StandardCharsets.UTF_8))); + 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(); + + ProjectBasicAuthFilter basicAuthFilter = + new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig); + + basicAuthFilter.doFilter(req, res, chain); + + verify(accountManager).authenticate(any()); + + verify(chain).doFilter(eq(req), any()); + assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK); + } + + @Test + public void shouldFailedAuthenticationAgainstRealm() throws Exception { + req.addHeader( + "Authorization", + "Basic " + + B64_ENC.encodeToString( + (AUTH_USER + ":" + AUTH_PASSWORD).getBytes(StandardCharsets.UTF_8))); + + doReturn(true).when(account).isActive(); + doThrow(new AccountException("Authentication error")).when(accountManager).authenticate(any()); + doReturn(GitBasicAuthPolicy.LDAP).when(authConfig).getGitBasicAuthPolicy(); + + ProjectBasicAuthFilter basicAuthFilter = + new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig); + + basicAuthFilter.doFilter(req, res, chain); + basicAuthFilter.destroy(); + + verify(accountManager).authenticate(any()); + + verify(chain, never()).doFilter(any(), any()); + assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); + } +} From 736795b48443727ef3bfe9ed2ce8d7db49226a47 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 5 May 2021 22:39:03 +0100 Subject: [PATCH 2/3] Avoid multiple auth requests for Git/HTTP access When authenticating incoming Git calls over HTTP the BasicAuth filter was called 3 times per call triggering multiple authentications against the backend. Example: Git protocol v1 triggering 2x HTTP calls, one for refs-advertisement and another for upload-pack was generating 6x authentication requests. When the backend is Gerrit's HTTP password authentication the operation is quite fast making the impact of the extra authentications negligible. However, when authenticating against a slower backend (e.g. corporate LDAP with groups resolution) the extra authentication calls were introducing unneeded latency and generating extra workload to the LDAP server. NOTE: It is still not possible to have one single authenticated session for multiple HTTP calls, because of the lack of support for GerritAccount cookie from the ProjectBasicAuthFilter. The next follow-up change is focused in solving that problem specifically, bringing the number of authentication requests to one. Bug: Issue 14497 Change-Id: Ibe41df0357b6be10bcdf0bd1f5a1b6160c34d4a4 --- .../gerrit/httpd/ProjectBasicAuthFilter.java | 2 +- .../httpd/ProjectBasicAuthFilterTest.java | 22 ++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java index e75d8feada..55ffef797b 100644 --- a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java +++ b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java @@ -99,7 +99,7 @@ class ProjectBasicAuthFilter implements Filter { HttpServletRequest req = (HttpServletRequest) request; Response rsp = new Response((HttpServletResponse) response); - if (verify(req, rsp)) { + if (session.get().isSignedIn() || verify(req, rsp)) { chain.doFilter(req, rsp); } } diff --git a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java index 1f06472f59..f7792ed2d3 100644 --- a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java +++ b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java @@ -141,6 +141,27 @@ public class ProjectBasicAuthFilterTest { assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK); } + @Test + public void shouldNotReauthenticateIfAlreadySignedIn() throws Exception { + req.addHeader( + "Authorization", + "Basic " + + B64_ENC.encodeToString( + (AUTH_USER + ":" + AUTH_PASSWORD).getBytes(StandardCharsets.UTF_8))); + res.setStatus(HttpServletResponse.SC_OK); + + doReturn(true).when(webSession).isSignedIn(); + + 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 shouldFailedAuthenticationAgainstRealm() throws Exception { req.addHeader( @@ -157,7 +178,6 @@ public class ProjectBasicAuthFilterTest { new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig); basicAuthFilter.doFilter(req, res, chain); - basicAuthFilter.destroy(); verify(accountManager).authenticate(any()); From ff89f79e83691d6d093a57187ecda094507600be Mon Sep 17 00:00:00 2001 From: Antoine Musso Date: Tue, 4 May 2021 11:14:05 +0200 Subject: [PATCH 3/3] download_file: download to GERRIT_CACHE_HOME when set When building a plugin as user `nobody`, download_file is unable to write the artifacts to the cache since the user does not have a home directory. For a CI build I also need to be set the cache directory to a predetermined value. Introduce the optional `GERRIT_CACHE_HOME` environment variable to support relocating downloaded artifacts. When the environment variable is not set, behavior is unchanged. Update documentation to mention support for `GERRIT_CACHE_HOME`. While at it, explain how to override the bazel repository and disk cache, would have same a bit of time the first time I had to tweak them. Change-Id: Ie4fac83928527e0e71b159b9500983234c2261ac --- Documentation/dev-bazel.txt | 4 ++++ tools/download_file.py | 7 +++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/Documentation/dev-bazel.txt b/Documentation/dev-bazel.txt index 782aba0678..2a8b7c17f0 100644 --- a/Documentation/dev-bazel.txt +++ b/Documentation/dev-bazel.txt @@ -495,6 +495,10 @@ To accelerate builds, several caches are activated per default: * ~/.gerritcodereview/bazel-cache/repository * ~/.gerritcodereview/bazel-cache/cas +The `downloaded-artifacts` cache can be relocated by setting the +`GERRIT_CACHE_HOME` environment variable. The other two can be adjusted with +`bazel build` options `--repository_cache` and `--disk_cache` respectively. + Currently none of these caches have a maximum size limit. See link:https://github.com/bazelbuild/bazel/issues/5139[this bazel issue] for details. Users should watch the cache sizes and clean them manually if diff --git a/tools/download_file.py b/tools/download_file.py index f86fd3e034..936bcef32c 100755 --- a/tools/download_file.py +++ b/tools/download_file.py @@ -17,7 +17,7 @@ from __future__ import print_function import argparse from hashlib import sha1 -from os import link, makedirs, path, remove +from os import environ, link, makedirs, path, remove import shutil from subprocess import check_call, CalledProcessError from sys import stderr @@ -25,7 +25,10 @@ from util import hash_file, resolve_url from zipfile import ZipFile, BadZipfile, LargeZipFile GERRIT_HOME = path.expanduser('~/.gerritcodereview') -CACHE_DIR = path.join(GERRIT_HOME, 'bazel-cache', 'downloaded-artifacts') +CACHE_DIR = environ.get( + 'GERRIT_CACHE_HOME', + path.join(GERRIT_HOME, 'bazel-cache', 'downloaded-artifacts')) + LOCAL_PROPERTIES = 'local.properties'