Deny access over HTTP for disabled accounts
When an account is disabled by the Gerrit admin through the set-account
command, it should not be enabled to perform any further authentication
or other actions.
Allowing a disabled account to continue operating in Gerrit until his
session expiration would be a security risk.
Bug: Issue 12717
Change-Id: I4cbad70bb3c83a1916f0d6939c5ccbbe7c734619
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 83bb7be..42a82ac 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -39,10 +39,12 @@
import com.google.common.io.BaseEncoding;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.AccountCreator;
+import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.UseSsh;
import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.extensions.api.accounts.AccountApi;
import com.google.gerrit.extensions.api.accounts.EmailInput;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -61,6 +63,7 @@
import com.google.gerrit.gpg.PublicKeyStore;
import com.google.gerrit.gpg.server.GpgKeys;
import com.google.gerrit.gpg.testutil.TestKey;
+import com.google.gerrit.httpd.CacheBasedWebSession;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.account.AccountByEmailCache;
@@ -77,6 +80,7 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.ByteArrayOutputStream;
+import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
@@ -86,6 +90,14 @@
import java.util.Locale;
import java.util.Map;
import java.util.Set;
+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;
@@ -120,6 +132,8 @@
private ExternalIdsUpdate externalIdsUpdate;
private List<ExternalId> savedExternalIds;
+ private BasicCookieStore httpCookieStore;
+ private CloseableHttpClient httpclient;
@Before
public void saveExternalIds() throws Exception {
@@ -130,6 +144,16 @@
savedExternalIds.addAll(getExternalIds(user));
}
+ @Before
+ public void createHttpClient() {
+ httpCookieStore = new BasicCookieStore();
+ httpclient =
+ HttpClientBuilder.create()
+ .disableRedirectHandling()
+ .setDefaultCookieStore(httpCookieStore)
+ .build();
+ }
+
@After
public void restoreExternalIds() throws Exception {
if (savedExternalIds != null) {
@@ -207,6 +231,43 @@
}
@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 deactivateSelf() throws Exception {
exception.expect(ResourceConflictException.class);
exception.expectMessage("cannot deactivate own account");
@@ -996,4 +1057,28 @@
assertThat(accounts).hasSize(1);
assertThat(Iterables.getOnlyElement(accounts)).isEqualTo(expectedAccount.getId());
}
+
+ 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);
+ }
}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java
index 9676cd3..5b5a3b0 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java
@@ -16,6 +16,7 @@
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;
@@ -26,6 +27,8 @@
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.AccountState;
import com.google.gerrit.server.account.AuthResult;
import com.google.gerrit.server.account.ExternalId;
import com.google.gerrit.server.config.AuthConfig;
@@ -39,7 +42,7 @@
@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;
@@ -49,6 +52,7 @@
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;
@@ -61,13 +65,15 @@
final WebSessionManager manager,
final AuthConfig authConfig,
final Provider<AnonymousUser> anonymousProvider,
- final IdentifiedUser.RequestFactory identified) {
+ final 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();
@@ -80,11 +86,15 @@
val = manager.createVal(key, val);
}
- String token = request.getHeader(HostPageData.XSRF_HEADER_NAME);
- if (val != null && token != null && token.equals(val.getAuth())) {
- okPaths.add(AccessPath.REST_API);
+ if (val != null && !checkAccountStatus(val.getAccountId())) {
+ val = null;
}
}
+
+ String token = request.getHeader(HostPageData.XSRF_HEADER_NAME);
+ if (val != null && token != null && token.equals(val.getAuth())) {
+ okPaths.add(AccessPath.REST_API);
+ }
}
}
@@ -157,6 +167,11 @@
manager.destroy(key);
}
+ if (!checkAccountStatus(id)) {
+ val = null;
+ return;
+ }
+
key = manager.createKey(id);
val = manager.createVal(key, id, rememberMe, identity, null, null);
saveCookie();
@@ -187,6 +202,11 @@
return val != null ? val.getSessionId() : null;
}
+ private boolean checkAccountStatus(Account.Id id) {
+ AccountState accountState = byIdCache.get(id);
+ return accountState != null && accountState.getAccount().isActive();
+ }
+
private void saveCookie() {
if (response == null) {
return;
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/H2CacheBasedWebSession.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/H2CacheBasedWebSession.java
index c466290..c77f76f 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/H2CacheBasedWebSession.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/H2CacheBasedWebSession.java
@@ -22,6 +22,7 @@
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;
@@ -62,8 +63,15 @@
@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);
}
}
