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

3 files changed, 120 insertions, 7 deletions

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 83bb7be88e..42a82ac224 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.collect.Iterables;
 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.Fingerprint;
 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.gerrit.testutil.FakeEmailSender.Message;
 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.List;
 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 @@ public class AccountIT extends AbstractDaemonTest {
 
   private ExternalIdsUpdate externalIdsUpdate;
   private List<ExternalId> savedExternalIds;
+  private BasicCookieStore httpCookieStore;
+  private CloseableHttpClient httpclient;
 
   @Before
   public void saveExternalIds() throws Exception {
@@ -130,6 +144,16 @@ public class AccountIT extends AbstractDaemonTest {
     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 @@ 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 deactivateSelf() throws Exception {
     exception.expect(ResourceConflictException.class);
     exception.expectMessage("cannot deactivate own account");
@@ -996,4 +1057,28 @@ public class AccountIT extends AbstractDaemonTest {
     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 9676cd37fc..5b5a3b043b 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 @@ 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;
@@ -26,6 +27,8 @@ 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.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 @@ 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;
@@ -49,6 +52,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;
@@ -61,13 +65,15 @@ public abstract class CacheBasedWebSession implements WebSession {
       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 @@ public abstract class CacheBasedWebSession implements WebSession {
           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 @@ 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();
@@ -187,6 +202,11 @@ public abstract class CacheBasedWebSession implements WebSession {
     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 c466290b28..c77f76f7d8 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.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;
@@ -62,8 +63,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);
   }
 }