diff options
author | Luca Milanesio <luca.milanesio@gmail.com> | 2020-06-06 00:29:21 +0100 |
---|---|---|
committer | Luca Milanesio <luca.milanesio@gmail.com> | 2020-06-06 01:01:17 +0100 |
commit | 94842f7440be5a35c77557b852093c6e7eafb302 (patch) | |
tree | e0eb0fbf44d7d641ba3f49817d5dbbff396b823e | |
parent | e22bfc3afa60ca691d6a91ec34010f01c802bfea (diff) | |
parent | 2a30db520976c94762a33c9cae2534295065a822 (diff) |
Merge branch 'stable-2.15' into stable-2.16
* stable-2.15:
Bazel: Add always pass test to avoid boilerplate in the CI
Deny access over HTTP for disabled accounts
Bazel: Consistently use bazelisk during publishing of artifacts
Change-Id: I29dd411d196f9e38688cdfbf94ae3f5ea7ed0ba8
3 files changed, 113 insertions, 4 deletions
diff --git a/java/com/google/gerrit/httpd/CacheBasedWebSession.java b/java/com/google/gerrit/httpd/CacheBasedWebSession.java index 6a19be7455..a89db76a19 100644 --- a/java/com/google/gerrit/httpd/CacheBasedWebSession.java +++ b/java/com/google/gerrit/httpd/CacheBasedWebSession.java @@ -16,6 +16,7 @@ package com.google.gerrit.httpd; import static java.util.concurrent.TimeUnit.HOURS; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.HostPageData; @@ -28,6 +29,7 @@ import com.google.gerrit.server.AccessPath; import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AuthResult; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.config.AuthConfig; @@ -41,7 +43,7 @@ import org.eclipse.jgit.http.server.GitSmartHttpTools; @RequestScoped public abstract class CacheBasedWebSession implements WebSession { - private static final String ACCOUNT_COOKIE = "GerritAccount"; + @VisibleForTesting public static final String ACCOUNT_COOKIE = "GerritAccount"; protected static final long MAX_AGE_MINUTES = HOURS.toMinutes(12); private final HttpServletRequest request; @@ -51,6 +53,7 @@ public abstract class CacheBasedWebSession implements WebSession { private final Provider<AnonymousUser> anonymousProvider; private final IdentifiedUser.RequestFactory identified; private final EnumSet<AccessPath> okPaths = EnumSet.of(AccessPath.UNKNOWN); + private final AccountCache byIdCache; private Cookie outCookie; private Key key; @@ -63,13 +66,15 @@ public abstract class CacheBasedWebSession implements WebSession { WebSessionManager manager, AuthConfig authConfig, Provider<AnonymousUser> anonymousProvider, - IdentifiedUser.RequestFactory identified) { + IdentifiedUser.RequestFactory identified, + final AccountCache byIdCache) { this.request = request; this.response = response; this.manager = manager; this.authConfig = authConfig; this.anonymousProvider = anonymousProvider; this.identified = identified; + this.byIdCache = byIdCache; if (request.getRequestURI() == null || !GitSmartHttpTools.isGitClient(request)) { String cookie = readCookie(request); @@ -86,6 +91,10 @@ public abstract class CacheBasedWebSession implements WebSession { authFromQueryParameter(token); } } + 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); @@ -178,6 +187,11 @@ public abstract class CacheBasedWebSession implements WebSession { manager.destroy(key); } + if (!checkAccountStatus(id)) { + val = null; + return; + } + key = manager.createKey(id); val = manager.createVal(key, id, rememberMe, identity, null, null); saveCookie(); @@ -208,6 +222,10 @@ public abstract class CacheBasedWebSession implements WebSession { return val != null ? val.getSessionId() : null; } + private boolean checkAccountStatus(Account.Id id) { + return byIdCache.get(id).filter(as -> as.getAccount().isActive()).isPresent(); + } + private void saveCookie() { if (response == null) { return; diff --git a/java/com/google/gerrit/httpd/H2CacheBasedWebSession.java b/java/com/google/gerrit/httpd/H2CacheBasedWebSession.java index caced27a79..830d8d609b 100644 --- a/java/com/google/gerrit/httpd/H2CacheBasedWebSession.java +++ b/java/com/google/gerrit/httpd/H2CacheBasedWebSession.java @@ -20,6 +20,7 @@ import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.httpd.WebSessionManager.Val; import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.IdentifiedUser.RequestFactory; +import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.cache.CacheModule; import com.google.gerrit.server.config.AuthConfig; import com.google.inject.Inject; @@ -59,8 +60,15 @@ public class H2CacheBasedWebSession extends CacheBasedWebSession { @Named(WebSessionManager.CACHE_NAME) Cache<String, Val> cache, AuthConfig authConfig, Provider<AnonymousUser> anonymousProvider, - RequestFactory identified) { + RequestFactory identified, + AccountCache byIdCache) { super( - request, response, managerFactory.create(cache), authConfig, anonymousProvider, identified); + request, + response, + managerFactory.create(cache), + authConfig, + anonymousProvider, + identified, + byIdCache); } } diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 7975e08da6..046365fb44 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -64,6 +64,7 @@ import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule.Action; +import com.google.gerrit.extensions.api.accounts.AccountApi; import com.google.gerrit.extensions.api.accounts.AccountInput; import com.google.gerrit.extensions.api.accounts.DeleteDraftCommentsInput; import com.google.gerrit.extensions.api.accounts.DeletedDraftCommentInfo; @@ -97,6 +98,7 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.gpg.Fingerprint; import com.google.gerrit.gpg.PublicKeyStore; import com.google.gerrit.gpg.testing.TestKey; +import com.google.gerrit.httpd.CacheBasedWebSession; import com.google.gerrit.mail.Address; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; @@ -153,6 +155,14 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; +import javax.servlet.http.HttpServletResponse; +import org.apache.http.HttpResponse; +import org.apache.http.client.ClientProtocolException; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.impl.client.BasicCookieStore; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClientBuilder; import org.bouncycastle.bcpg.ArmoredOutputStream; import org.bouncycastle.openpgp.PGPPublicKey; import org.bouncycastle.openpgp.PGPPublicKeyRing; @@ -235,6 +245,8 @@ public class AccountIT extends AbstractDaemonTest { private RegistrationHandle accountIndexEventCounterHandle; private RefUpdateCounter refUpdateCounter; private RegistrationHandle refUpdateCounterHandle; + private BasicCookieStore httpCookieStore; + private CloseableHttpClient httpclient; @Before public void addAccountIndexEventCounter() { @@ -288,6 +300,16 @@ public class AccountIT extends AbstractDaemonTest { } } + @Before + public void createHttpClient() { + httpCookieStore = new BasicCookieStore(); + httpclient = + HttpClientBuilder.create() + .disableRedirectHandling() + .setDefaultCookieStore(httpCookieStore) + .build(); + } + protected void assertLabelPermission( Project.NameKey project, GroupReference groupReference, @@ -549,6 +571,43 @@ public class AccountIT extends AbstractDaemonTest { } @Test + @GerritConfig(name = "auth.type", value = "DEVELOPMENT_BECOME_ANY_ACCOUNT") + public void activeUserGetSessionCookieOnLogin() throws Exception { + Integer accountId = accountIdApi().get()._accountId; + assertThat(accountIdApi().getActive()).isTrue(); + + webLogin(accountId); + assertThat(getCookiesNames()).contains(CacheBasedWebSession.ACCOUNT_COOKIE); + } + + @Test + @GerritConfig(name = "auth.type", value = "DEVELOPMENT_BECOME_ANY_ACCOUNT") + public void inactiveUserDoesNotGetCookieOnLogin() throws Exception { + Integer accountId = accountIdApi().get()._accountId; + accountIdApi().setActive(false); + assertThat(accountIdApi().getActive()).isFalse(); + + webLogin(accountId); + assertThat(getCookiesNames()).isEmpty(); + } + + @Test + @GerritConfig(name = "auth.type", value = "DEVELOPMENT_BECOME_ANY_ACCOUNT") + public void userDeactivatedAfterLoginDoesNotGetCookie() throws Exception { + Integer accountId = accountIdApi().get()._accountId; + assertThat(accountIdApi().getActive()).isTrue(); + + webLogin(accountId); + assertThat(getCookiesNames()).contains(CacheBasedWebSession.ACCOUNT_COOKIE); + httpGetAndAssertStatus("accounts/self/detail", HttpServletResponse.SC_OK); + + accountIdApi().setActive(false); + assertThat(accountIdApi().getActive()).isFalse(); + + httpGetAndAssertStatus("accounts/self/detail", HttpServletResponse.SC_FORBIDDEN); + } + + @Test public void validateAccountActivation() throws Exception { Account.Id activatableAccountId = accountOperations.newAccount().inactive().preferredEmail("foo@activatable.com").create(); @@ -3358,6 +3417,30 @@ public class AccountIT extends AbstractDaemonTest { return ac; } + private AccountApi accountIdApi() throws RestApiException { + return gApi.accounts().id("user"); + } + + private Set<String> getCookiesNames() { + Set<String> cookieNames = + httpCookieStore.getCookies().stream() + .map(cookie -> cookie.getName()) + .collect(Collectors.toSet()); + return cookieNames; + } + + private void webLogin(Integer accountId) throws IOException, ClientProtocolException { + httpGetAndAssertStatus( + "login?account_id=" + accountId, HttpServletResponse.SC_MOVED_TEMPORARILY); + } + + private void httpGetAndAssertStatus(String urlPath, int expectedHttpStatus) + throws ClientProtocolException, IOException { + HttpGet httpGet = new HttpGet(canonicalWebUrl.get() + urlPath); + HttpResponse loginResponse = httpclient.execute(httpGet); + assertThat(loginResponse.getStatusLine().getStatusCode()).isEqualTo(expectedHttpStatus); + } + /** Checks if an account is indexed the correct number of times. */ private static class AccountIndexedCounter implements AccountIndexedListener { private final AtomicLongMap<Integer> countsByAccount = AtomicLongMap.create(); |