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
diff --git a/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java b/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
index c193115..1974ba7 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.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 @@
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 @@
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 @@
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 735abbf..2f0fafa 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.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 @@
@Before
public void setUp() throws Exception {
- req = new FakeHttpServletRequest();
+ req = new FakeHttpServletRequest("gerrit.example.com", 80, "", "");
res = new FakeHttpServletResponse();
authSuccessful =
@@ -102,9 +104,8 @@
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 @@
.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 @@
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 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,16 +187,11 @@
}
@Test
- public void shouldNotReauthenticateIfAlreadySignedIn() throws Exception {
- initMockedWebSession();
- doReturn(true).when(webSession).isSignedIn();
- requestBasicAuth(req);
- res.setStatus(HttpServletResponse.SC_OK);
-
- ProjectBasicAuthFilter basicAuthFilter =
- new ProjectBasicAuthFilter(webSessionItem, accountCache, accountManager, authConfig);
-
- basicAuthFilter.doFilter(req, res, chain);
+ 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();
verify(accountManager, never()).authenticate(any());
verify(chain).doFilter(eq(req), any());
@@ -222,16 +199,28 @@
}
@Test
- public void shouldNotReauthenticateIfHasExistingCookie() throws Exception {
+ public void shouldReauthenticateForRegularRequestEvenIfAlreadySignedIn() throws Exception {
+ doReturn(GitBasicAuthPolicy.LDAP).when(authConfig).getGitBasicAuthPolicy();
+ doFilterForRequestWhenAlreadySignedIn();
+
+ verify(accountManager).authenticate(any());
+ verify(chain).doFilter(eq(req), any());
+ assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
+ }
+
+ @Test
+ 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 @@
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 2efa94b..0bb4de4 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 @@
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 @@
attributes = Maps.newConcurrentMap();
parameters = LinkedListMultimap.create();
headers = LinkedListMultimap.create();
+ method = "GET";
}
@Override
@@ -105,6 +107,11 @@
@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 @@
@Override
public String getMethod() {
- return "GET";
+ return method;
+ }
+
+ public void setMethod(String method) {
+ this.method = method;
}
@Override
