From 736795b48443727ef3bfe9ed2ce8d7db49226a47 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 5 May 2021 22:39:03 +0100 Subject: 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 +- .../gerrit/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()); -- cgit v1.2.3