Merge branch 'stable-3.3' into stable-3.4
* stable-3.3:
Allow GerritAccount Cookie authentication for Git/HTTP
Change-Id: I124dbbb2a325d4ba23e9bf8794f16d1f23f0e8fe
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 1052c8e..5f2ea09 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -604,7 +604,14 @@
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 cdaf155..0670968 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -29,6 +29,13 @@
* 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 a3a67e5..7212e3e 100644
--- a/java/com/google/gerrit/httpd/CacheBasedWebSession.java
+++ b/java/com/google/gerrit/httpd/CacheBasedWebSession.java
@@ -76,29 +76,27 @@
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);
- }
+ 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 && !checkAccountStatus(val.getAccountId())) {
- val = null;
- okPaths.clear();
+ if (token != null) {
+ authFromQueryParameter(token);
}
- if (val != null && val.needsCookieRefresh()) {
- // Session is more than half old; update cache entry with new expiration date.
- val = manager.createVal(key, val);
- }
+ }
+ 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);
}
}
diff --git a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
index ca780d0..7fcd4f8 100644
--- a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
+++ b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
@@ -22,6 +22,7 @@
import com.google.common.base.Strings;
import com.google.common.flogger.FluentLogger;
import com.google.common.io.BaseEncoding;
+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 @@
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 @@
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 @@
}
}
- 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 @@
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/BUILD b/javatests/com/google/gerrit/httpd/BUILD
index 75f005e..121cbc4 100644
--- a/javatests/com/google/gerrit/httpd/BUILD
+++ b/javatests/com/google/gerrit/httpd/BUILD
@@ -18,6 +18,7 @@
"//lib:junit",
"//lib:servlet-api-without-neverlink",
"//lib:soy",
+ "//lib/bouncycastle:bcprov",
"//lib/guice",
"//lib/guice:guice-servlet",
"//lib/mockito",
diff --git a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
index f7792ed..735abbf 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.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 @@
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 @@
@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();
- doReturn(Optional.of(accountState)).when(accountCache).getByUsername(AUTH_USER);
- doReturn(account).when(accountState).account();
-
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());
+ }
+
+ 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 @@
@Test
public void shouldRequestAuthenticationForBasicAuthRequest() throws Exception {
+ initMockedWebSession();
req.addHeader("Authorization", "Basic " + AUTH_USER_B64);
res.setStatus(HttpServletResponse.SC_OK);
@@ -116,16 +162,11 @@
}
@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 @@
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 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 {
- req.addHeader(
- "Authorization",
- "Basic "
- + B64_ENC.encodeToString(
- (AUTH_USER + ":" + AUTH_PASSWORD).getBytes(StandardCharsets.UTF_8)));
+ initMockedWebSession();
+ doReturn(true).when(webSession).isSignedIn();
+ requestBasicAuth(req);
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 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 @@
@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 @@
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 a4175e3..2efa94b 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 @@
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 @@
@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 9a98ecd..f39b875 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 @@
@Override
public void addCookie(Cookie cookie) {
- throw new UnsupportedOperationException();
+ addHeader("Set-Cookie", cookie.getName() + "=" + cookie.getValue());
}
@Override
