From ac47c36385d4bc9efa36607ffbb797b3c8308302 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Fri, 22 Nov 2019 21:40:13 -0800 Subject: Bazel: Consistently use bazelisk during publishing of artifacts Change-Id: I1e3d7ceaf7908f3b53a5b11a257aa752eaec066e (cherry picked from commit 9c7428264769e9c211c0cf23a2d6ec9406be791b) --- tools/maven/package.bzl | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/maven/package.bzl b/tools/maven/package.bzl index 5b497f83b6..ce60db9ebc 100644 --- a/tools/maven/package.bzl +++ b/tools/maven/package.bzl @@ -17,6 +17,14 @@ sh_bang_template = (" && ".join([ "echo \"# this script should run from the root of your workspace.\" >> $@", "echo \"set -e\" >> $@", "echo \"\" >> $@", + "echo 'function bazel_cmd() {' >> $@", + "echo ' if [[ `which bazelisk` ]]; then' >> $@", + "echo ' bazelisk \"$$@\"' >> $@", + "echo ' else' >> $@", + "echo ' bazel \"$$@\"' >> $@", + "echo ' fi' >> $@", + "echo '}' >> $@", + "echo \"\" >> $@", "echo 'if [[ \"$$VERBOSE\" ]]; then set -x ; fi' >> $@", "echo \"\" >> $@", "echo %s >> $@", @@ -32,7 +40,7 @@ def maven_package( src = {}, doc = {}, war = {}): - build_cmd = ["bazel", "build"] + build_cmd = ["bazel_cmd", "build"] mvn_cmd = ["python", "tools/maven/mvn.py", "-v", version] api_cmd = mvn_cmd[:] api_targets = [] -- cgit v1.2.3 From 06a3a5bfea30ef1907463c66ee1bf447eeb958e9 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 13 May 2020 01:46:24 +0100 Subject: 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 --- .../gerrit/acceptance/api/accounts/AccountIT.java | 85 ++++++++++++++++++++++ .../google/gerrit/httpd/CacheBasedWebSession.java | 30 ++++++-- .../gerrit/httpd/H2CacheBasedWebSession.java | 12 ++- 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 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) { @@ -206,6 +230,43 @@ public class AccountIT extends AbstractDaemonTest { assertThat(gApi.accounts().id("user").getActive()).isTrue(); } + @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); @@ -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 getCookiesNames() { + Set 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 anonymousProvider; private final IdentifiedUser.RequestFactory identified; private final EnumSet 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 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 cache, AuthConfig authConfig, Provider 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); } } -- cgit v1.2.3 From 938db887d803889e34a1adfd762c259eadcdd6a3 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Fri, 22 May 2020 16:25:36 +0200 Subject: Bazel: Add always pass test to avoid boilerplate in the CI We don't know whether or not all plugins have test rules, so running a generic command: $ bazelisk test plugins/{name}/... can lead to no tests found outcome, in which case Bazel will return exit code 4 that needs to be checked to differentiate from test failure (exit code 1). To avoid those checks, pass the virtual test that will always succeed: $ bazelisk test //tools/bzl:always_pass_test plugins/{name}/... See also this upstream issue for this suggested workaround: [1]. [1] https://github.com/bazelbuild/bazel/issues/11465 Change-Id: Ideab64674482400cc48fad55f7ed9f8085909b84 (cherry picked from commit d69773ceb5c84cfcf13749b2f555bf92d22e6f71) --- tools/bzl/BUILD | 5 +++++ tools/bzl/always_pass_test.sh | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100755 tools/bzl/always_pass_test.sh diff --git a/tools/bzl/BUILD b/tools/bzl/BUILD index a0f5bd1408..7febbacd8f 100644 --- a/tools/bzl/BUILD +++ b/tools/bzl/BUILD @@ -3,3 +3,8 @@ exports_files([ "test_empty.sh", "test_license.sh", ]) + +sh_test( + name = "always_pass_test", + srcs = ["always_pass_test.sh"], +) diff --git a/tools/bzl/always_pass_test.sh b/tools/bzl/always_pass_test.sh new file mode 100755 index 0000000000..15c58cad54 --- /dev/null +++ b/tools/bzl/always_pass_test.sh @@ -0,0 +1,21 @@ +#!/bin/sh +# +# Copyright (C) 2020 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# This is a dummy test to put on the command line to avoid no tests +# found outcome in `bazel test` command. See this upstream issue: +# https://github.com/bazelbuild/bazel/issues/11465 + +exit 0 -- cgit v1.2.3 From bd65cc1fb2390f07b71a730107bfed06ec1c21e9 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Wed, 6 May 2020 17:27:05 +0200 Subject: Add account listener integration tests using a real plugin While AccountIT already provides an integration test for the validation listeners, it does so without using a real plugin. Add a wiring integration test using a real plugin to make sure plugin wiring is working as expected. See review of https://gerrit-review.googlesource.com/c/gerrit/+/265638 Change-Id: I35988f3eb4cfe947fe65c418bb7cfc51af5d2124 --- .../api/accounts/AccountListenersIT.java | 165 +++++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java new file mode 100644 index 0000000000..d37f019b19 --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java @@ -0,0 +1,165 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.api.accounts; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.testing.GerritJUnit.assertThrows; + +import com.google.gerrit.acceptance.LightweightPluginDaemonTest; +import com.google.gerrit.acceptance.TestPlugin; +import com.google.gerrit.acceptance.testsuite.account.AccountOperations; +import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.validators.AccountActivationValidationListener; +import com.google.gerrit.server.validators.ValidationException; +import com.google.inject.AbstractModule; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import org.junit.Before; +import org.junit.Test; + +/** + * Tests the wiring of a real plugin's account listeners + * + *

This test really puts focus on the wiring of the account listeners. Tests for the inner + * workings of account activation/deactivation can be found in {@link AccountIT}. + */ +@TestPlugin( + name = "account-listener-it-plugin", + sysModule = "com.google.gerrit.acceptance.api.accounts.AccountListenersIT$Module") +public class AccountListenersIT extends LightweightPluginDaemonTest { + @Inject private AccountOperations accountOperations; + + public static class Module extends AbstractModule { + @Override + protected void configure() { + DynamicSet.bind(binder(), AccountActivationValidationListener.class).to(Validator.class); + } + } + + Validator validator; + + @Before + public void setUp() { + validator = plugin.getSysInjector().getInstance(Validator.class); + } + + @Test + public void testActivation() throws RestApiException { + int id = accountOperations.newAccount().inactive().create().get(); + + gApi.accounts().id(id).setActive(true); + + validator.assertActivationValidation(id); + validator.assertNoMoreEvents(); + assertThat(gApi.accounts().id(id).getActive()).isTrue(); + } + + @Test + public void testActivationProhibited() throws RestApiException { + int id = accountOperations.newAccount().inactive().create().get(); + + validator.failActivationValidations(); + + assertThrows( + ResourceConflictException.class, + () -> { + gApi.accounts().id(id).setActive(true); + }); + + validator.assertActivationValidation(id); + validator.assertNoMoreEvents(); + assertThat(gApi.accounts().id(id).getActive()).isFalse(); + } + + @Test + public void testDeactivation() throws RestApiException { + int id = accountOperations.newAccount().active().create().get(); + + gApi.accounts().id(id).setActive(false); + + validator.assertDeactivationValidation(id); + validator.assertNoMoreEvents(); + assertThat(gApi.accounts().id(id).getActive()).isFalse(); + } + + @Test + public void testDeactivationProhibited() throws RestApiException { + int id = accountOperations.newAccount().active().create().get(); + + validator.failDeactivationValidations(); + + assertThrows( + ResourceConflictException.class, + () -> { + gApi.accounts().id(id).setActive(false); + }); + + validator.assertDeactivationValidation(id); + validator.assertNoMoreEvents(); + assertThat(gApi.accounts().id(id).getActive()).isTrue(); + } + + @Singleton + public static class Validator implements AccountActivationValidationListener { + private Integer lastIdActivationValidation; + private Integer lastIdDeactivationValidation; + private boolean failActivationValidations; + private boolean failDeactivationValidations; + + @Override + public void validateActivation(AccountState account) throws ValidationException { + assertThat(lastIdActivationValidation).isNull(); + lastIdActivationValidation = account.account().id().get(); + if (failActivationValidations) { + throw new ValidationException("testing validation failure"); + } + } + + @Override + public void validateDeactivation(AccountState account) throws ValidationException { + assertThat(lastIdDeactivationValidation).isNull(); + lastIdDeactivationValidation = account.account().id().get(); + if (failDeactivationValidations) { + throw new ValidationException("testing validation failure"); + } + } + + public void failActivationValidations() { + failActivationValidations = true; + } + + public void failDeactivationValidations() { + failDeactivationValidations = true; + } + + private void assertNoMoreEvents() { + assertThat(lastIdActivationValidation).isNull(); + assertThat(lastIdDeactivationValidation).isNull(); + } + + private void assertActivationValidation(int id) { + assertThat(lastIdActivationValidation).isEqualTo(id); + lastIdActivationValidation = null; + } + + private void assertDeactivationValidation(int id) { + assertThat(lastIdDeactivationValidation).isEqualTo(id); + lastIdDeactivationValidation = null; + } + } +} -- cgit v1.2.3 From c6a55c43c4f5a2335d31d5bc1c5568255eb84869 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sat, 6 Jun 2020 10:21:22 +0900 Subject: CacheBasedWebSession: Remove unnecessary 'final' in constructor args Change-Id: I77f11f9cc80a68f3ec63520fa6d12f0a70afc2f9 --- .../src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6239e3b5e7..d1950e0b48 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 @@ -68,7 +68,7 @@ public abstract class CacheBasedWebSession implements WebSession { AuthConfig authConfig, Provider anonymousProvider, IdentifiedUser.RequestFactory identified, - final AccountCache byIdCache) { + AccountCache byIdCache) { this.request = request; this.response = response; this.manager = manager; -- cgit v1.2.3 From 43d517e0f079b885a54f8f746e5e1e6323f25dcc Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Thu, 7 May 2020 14:19:45 +0200 Subject: Extract method to iterate SSH sessions Thereby, we make code re-usable for upcoming commits. Add coverage to the SSH command close-connection in the SshCommandsIT to make sure that the refactoring does not introduce any regression. Change-Id: Id017687b53750360a52dd020335a7f433d123e6e --- java/com/google/gerrit/sshd/SshUtil.java | 30 +++++++++++++ .../gerrit/sshd/commands/CloseConnection.java | 51 ++++++++-------------- .../gerrit/acceptance/ssh/SshCommandsIT.java | 48 ++++++++++++++++++++ 3 files changed, 97 insertions(+), 32 deletions(-) diff --git a/java/com/google/gerrit/sshd/SshUtil.java b/java/com/google/gerrit/sshd/SshUtil.java index 39366f0c68..6176628c4d 100644 --- a/java/com/google/gerrit/sshd/SshUtil.java +++ b/java/com/google/gerrit/sshd/SshUtil.java @@ -18,6 +18,7 @@ import com.google.gerrit.entities.Account; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountSshKey; +import com.google.gerrit.sshd.BaseCommand.Failure; import com.google.gerrit.sshd.SshScope.Context; import java.io.BufferedReader; import java.io.IOException; @@ -30,7 +31,10 @@ import java.security.interfaces.RSAPublicKey; import java.security.spec.InvalidKeySpecException; import org.apache.commons.codec.binary.Base64; import org.apache.sshd.common.SshException; +import org.apache.sshd.common.io.IoAcceptor; +import org.apache.sshd.common.io.IoSession; import org.apache.sshd.common.keyprovider.KeyPairProvider; +import org.apache.sshd.common.session.helpers.AbstractSession; import org.apache.sshd.common.util.buffer.ByteArrayBuffer; import org.apache.sshd.server.session.ServerSession; import org.eclipse.jgit.lib.Constants; @@ -154,4 +158,30 @@ public class SshUtil { final Account.Id account) { return userFactory.create(sd.getRemoteAddress(), account); } + + public static void forEachSshSession(SshDaemon sshDaemon, SessionConsumer consumer) + throws Failure { + IoAcceptor ioAcceptor = sshDaemon.getIoAcceptor(); + if (ioAcceptor == null) { + throw new Failure(1, "fatal: sshd no longer running"); + } + ioAcceptor + .getManagedSessions() + .forEach( + (id, ioSession) -> { + AbstractSession abstractSession = AbstractSession.getSession(ioSession, true); + if (abstractSession != null) { + SshSession sshSession = abstractSession.getAttribute(SshSession.KEY); + if (sshSession != null) { + consumer.accept(id, sshSession, abstractSession, ioSession); + } + } + }); + } + + @FunctionalInterface + public interface SessionConsumer { + public void accept( + Long id, SshSession sshSession, AbstractSession abstractSession, IoSession ioSession); + } } diff --git a/java/com/google/gerrit/sshd/commands/CloseConnection.java b/java/com/google/gerrit/sshd/commands/CloseConnection.java index 60a878a62c..093f647997 100644 --- a/java/com/google/gerrit/sshd/commands/CloseConnection.java +++ b/java/com/google/gerrit/sshd/commands/CloseConnection.java @@ -23,15 +23,12 @@ import com.google.gerrit.sshd.AdminHighPriorityCommand; import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.SshCommand; import com.google.gerrit.sshd.SshDaemon; -import com.google.gerrit.sshd.SshSession; +import com.google.gerrit.sshd.SshUtil; import com.google.inject.Inject; import java.io.IOException; import java.util.ArrayList; import java.util.List; import org.apache.sshd.common.future.CloseFuture; -import org.apache.sshd.common.io.IoAcceptor; -import org.apache.sshd.common.io.IoSession; -import org.apache.sshd.common.session.helpers.AbstractSession; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; @@ -60,36 +57,26 @@ final class CloseConnection extends SshCommand { @Override protected void run() throws Failure { - IoAcceptor acceptor = sshDaemon.getIoAcceptor(); - if (acceptor == null) { - throw new Failure(1, "fatal: sshd no longer running"); - } - for (String sessionId : sessionIds) { - boolean connectionFound = false; - int id = (int) Long.parseLong(sessionId, 16); - for (IoSession io : acceptor.getManagedSessions().values()) { - AbstractSession serverSession = AbstractSession.getSession(io, true); - SshSession sshSession = - serverSession != null ? serverSession.getAttribute(SshSession.KEY) : null; - if (sshSession != null && sshSession.getSessionId() == id) { - connectionFound = true; - stdout.println("closing connection " + sessionId + "..."); - CloseFuture future = io.close(true); - if (wait) { - try { - future.await(); - stdout.println("closed connection " + sessionId); - } catch (IOException e) { - logger.atWarning().log( - "Wait for connection to close interrupted: %s", e.getMessage()); + SshUtil.forEachSshSession( + sshDaemon, + (k, sshSession, abstractSession, ioSession) -> { + String sessionId = String.format("%08x", sshSession.getSessionId()); + if (sessionIds.remove(sessionId)) { + stdout.println("closing connection " + sessionId + "..."); + CloseFuture future = ioSession.close(true); + if (wait) { + try { + future.await(); + stdout.println("closed connection " + sessionId); + } catch (IOException e) { + logger.atWarning().log( + "Wait for connection to close interrupted: %s", e.getMessage()); + } } } - break; - } - } - if (!connectionFound) { - stderr.print("close connection " + sessionId + ": no such connection\n"); - } + }); + for (String sessionId : sessionIds) { + stderr.print("close connection " + sessionId + ": no such connection\n"); } } } diff --git a/javatests/com/google/gerrit/acceptance/ssh/SshCommandsIT.java b/javatests/com/google/gerrit/acceptance/ssh/SshCommandsIT.java index 4e9c4a49e9..012e9980d0 100644 --- a/javatests/com/google/gerrit/acceptance/ssh/SshCommandsIT.java +++ b/javatests/com/google/gerrit/acceptance/ssh/SshCommandsIT.java @@ -18,6 +18,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Streams; @@ -29,6 +30,9 @@ import com.google.gerrit.acceptance.UseSsh; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Spliterator; +import java.util.stream.Collectors; +import java.util.stream.StreamSupport; import org.junit.Test; @NoHttpd @@ -162,6 +166,38 @@ public class SshCommandsIT extends AbstractDaemonTest { assertThat(commands).containsExactlyElementsIn(SLAVE_COMMANDS.get("gerrit")).inOrder(); } + @Test + @Sandboxed + public void showConnections() throws Exception { + Spliterator connectionsOutput = + getOutputLines(adminSshSession.exec("gerrit show-connections")); + + assertThat(findConnectionsInOutput(connectionsOutput, "user")).hasSize(1); + } + + @Test + @Sandboxed + public void cloeConnections() throws Exception { + List connectionsOutput = + findConnectionsInOutput( + getOutputLines(adminSshSession.exec("gerrit show-connections")), "user"); + String connectionId = + Splitter.on(" ") + .trimResults() + .omitEmptyStrings() + .split(connectionsOutput.get(0)) + .iterator() + .next(); + + String closeConnectionOutput = adminSshSession.exec("gerrit close-connection " + connectionId); + assertThat(closeConnectionOutput).contains(connectionId); + + assertThat( + findConnectionsInOutput( + getOutputLines(adminSshSession.exec("gerrit show-connections")), "user")) + .isEmpty(); + } + private List parseCommandsFromGerritHelpText(String helpText) { List commands = new ArrayList<>(); @@ -194,4 +230,16 @@ public class SshCommandsIT extends AbstractDaemonTest { return commands; } + + private List findConnectionsInOutput(Spliterator connectionsOutput, String user) { + List connections = + StreamSupport.stream(connectionsOutput, false) + .filter(s -> s.contains("localhost") && s.contains(user)) + .collect(Collectors.toList()); + return connections; + } + + private Spliterator getOutputLines(String output) throws Exception { + return Splitter.on("\n").trimResults().omitEmptyStrings().split(output).spliterator(); + } } -- cgit v1.2.3 From db930bd5c7770dccd94411bee04a3b84e29e8f3e Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Fri, 1 May 2020 11:05:38 +0200 Subject: Allow to listen for account deactivations Deactivating an account currently does not purge active SSH connections (E.g.: long-running stream-events) and does not purge the web session cache. So deactivating a user for real requires to also manually purge these connections and caches, which is unnecessarily tedious. Implementing such code directly in SetInactiveFlag is not straight forward as com.google.server.account does not have access to the sshd package, and naively adding it to the build introduces a circular dependency. So we add an extension point for account deactivation. Thereby, core components and plugins can self-cater cache purging and other house-holding tasks upon user deactivation without messing with the build. Change-Id: I1dfa1779acfe56e5c7f1666b1e83e83f954083e1 --- Documentation/dev-plugins.txt | 4 ++ .../gerrit/acceptance/ExtensionRegistry.java | 8 ++++ .../events/AccountActivationListener.java | 42 +++++++++++++++++ .../gerrit/server/account/SetInactiveFlag.java | 24 +++++++++- .../gerrit/server/config/GerritGlobalModule.java | 2 + .../AccountActivationValidationListener.java | 6 +++ .../gerrit/acceptance/api/accounts/AccountIT.java | 23 ++++++++- .../api/accounts/AccountListenersIT.java | 55 ++++++++++++++++++++-- 8 files changed, 156 insertions(+), 8 deletions(-) create mode 100644 java/com/google/gerrit/extensions/events/AccountActivationListener.java diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index 799c13c663..aad81bdfd9 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -390,6 +390,10 @@ Allows to listen to events visible to the specified user. These are the same link:cmd-stream-events.html#events[events] that are also streamed by the link:cmd-stream-events.html[gerrit stream-events] command. +* `com.google.gerrit.extensions.events.AccountActivationListener`: ++ +User account got activated or deactivated + * `com.google.gerrit.extensions.events.LifecycleListener`: + Plugin start and stop diff --git a/java/com/google/gerrit/acceptance/ExtensionRegistry.java b/java/com/google/gerrit/acceptance/ExtensionRegistry.java index f9116a19a1..eaf03b332c 100644 --- a/java/com/google/gerrit/acceptance/ExtensionRegistry.java +++ b/java/com/google/gerrit/acceptance/ExtensionRegistry.java @@ -16,6 +16,7 @@ package com.google.gerrit.acceptance; import com.google.gerrit.extensions.api.changes.ActionVisitor; import com.google.gerrit.extensions.config.DownloadScheme; +import com.google.gerrit.extensions.events.AccountActivationListener; import com.google.gerrit.extensions.events.AccountIndexedListener; import com.google.gerrit.extensions.events.ChangeIndexedListener; import com.google.gerrit.extensions.events.CommentAddedListener; @@ -68,6 +69,7 @@ public class ExtensionRegistry { private final DynamicSet groupBackends; private final DynamicSet accountActivationValidationListeners; + private final DynamicSet accountActivationListeners; private final DynamicSet onSubmitValidationListeners; @Inject @@ -93,6 +95,7 @@ public class ExtensionRegistry { DynamicSet revisionCreatedListeners, DynamicSet groupBackends, DynamicSet accountActivationValidationListeners, + DynamicSet accountActivationListeners, DynamicSet onSubmitValidationListeners) { this.accountIndexedListeners = accountIndexedListeners; this.changeIndexedListeners = changeIndexedListeners; @@ -115,6 +118,7 @@ public class ExtensionRegistry { this.revisionCreatedListeners = revisionCreatedListeners; this.groupBackends = groupBackends; this.accountActivationValidationListeners = accountActivationValidationListeners; + this.accountActivationListeners = accountActivationListeners; this.onSubmitValidationListeners = onSubmitValidationListeners; } @@ -215,6 +219,10 @@ public class ExtensionRegistry { return add(accountActivationValidationListeners, accountActivationValidationListener); } + public Registration add(AccountActivationListener accountDeactivatedListener) { + return add(accountActivationListeners, accountDeactivatedListener); + } + public Registration add(OnSubmitValidationListener onSubmitValidationListener) { return add(onSubmitValidationListeners, onSubmitValidationListener); } diff --git a/java/com/google/gerrit/extensions/events/AccountActivationListener.java b/java/com/google/gerrit/extensions/events/AccountActivationListener.java new file mode 100644 index 0000000000..b45533b8bf --- /dev/null +++ b/java/com/google/gerrit/extensions/events/AccountActivationListener.java @@ -0,0 +1,42 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.extensions.events; + +import com.google.gerrit.extensions.annotations.ExtensionPoint; + +/** + * Notified whenever an account got activated or deactivated. + * + *

This listener is called only after an account got (de)activated and hence cannot cancel the + * (de)activation. See {@link + * com.google.gerrit.server.validators.AccountActivationValidationListener} for a listener that can + * cancel a (de)activation. + */ +@ExtensionPoint +public interface AccountActivationListener { + /** + * Invoked after an account got activated + * + * @param id of the account + */ + default void onAccountActivated(int id) {} + + /** + * Invoked after an account got deactivated + * + * @param id of the account + */ + default void onAccountDeactivated(int id) {} +} diff --git a/java/com/google/gerrit/server/account/SetInactiveFlag.java b/java/com/google/gerrit/server/account/SetInactiveFlag.java index fb3d4ea00d..40cc185ff4 100644 --- a/java/com/google/gerrit/server/account/SetInactiveFlag.java +++ b/java/com/google/gerrit/server/account/SetInactiveFlag.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.account; import com.google.gerrit.entities.Account; +import com.google.gerrit.extensions.events.AccountActivationListener; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; @@ -37,13 +38,16 @@ public class SetInactiveFlag { private final PluginSetContext accountActivationValidationListeners; private final Provider accountsUpdateProvider; + private final PluginSetContext accountActivationListeners; @Inject SetInactiveFlag( PluginSetContext accountActivationValidationListeners, - @ServerInitiated Provider accountsUpdateProvider) { + @ServerInitiated Provider accountsUpdateProvider, + PluginSetContext accountActivationListeners) { this.accountActivationValidationListeners = accountActivationValidationListeners; this.accountsUpdateProvider = accountsUpdateProvider; + this.accountActivationListeners = accountActivationListeners; } public Response deactivate(Account.Id accountId) @@ -76,6 +80,12 @@ public class SetInactiveFlag { if (alreadyInactive.get()) { throw new ResourceConflictException("account not active"); } + + // At this point the account got set inactive and no errors occurred + + int id = accountId.get(); + accountActivationListeners.runEach(l -> l.onAccountDeactivated(id)); + return Response.none(); } @@ -106,6 +116,16 @@ public class SetInactiveFlag { if (exception.get().isPresent()) { throw exception.get().get(); } - return alreadyActive.get() ? Response.ok("") : Response.created(""); + + Response res; + if (alreadyActive.get()) { + res = Response.ok(""); + } else { + res = Response.created(""); + + int id = accountId.get(); + accountActivationListeners.runEach(l -> l.onAccountActivated(id)); + } + return res; } } diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index e17fb02a05..25f2b20269 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -30,6 +30,7 @@ import com.google.gerrit.extensions.config.DownloadScheme; import com.google.gerrit.extensions.config.ExternalIncludedIn; import com.google.gerrit.extensions.config.FactoryModule; import com.google.gerrit.extensions.config.PluginProjectPermissionDefinition; +import com.google.gerrit.extensions.events.AccountActivationListener; import com.google.gerrit.extensions.events.AccountIndexedListener; import com.google.gerrit.extensions.events.AgreementSignupListener; import com.google.gerrit.extensions.events.AssigneeChangedListener; @@ -332,6 +333,7 @@ public class GerritGlobalModule extends FactoryModule { DynamicSet.setOf(binder(), PostReceiveHook.class); DynamicSet.setOf(binder(), PreUploadHook.class); DynamicSet.setOf(binder(), PostUploadHook.class); + DynamicSet.setOf(binder(), AccountActivationListener.class); DynamicSet.setOf(binder(), AccountIndexedListener.class); DynamicSet.setOf(binder(), ChangeIndexedListener.class); DynamicSet.setOf(binder(), GroupIndexedListener.class); diff --git a/java/com/google/gerrit/server/validators/AccountActivationValidationListener.java b/java/com/google/gerrit/server/validators/AccountActivationValidationListener.java index bc52308609..9fdc9e6d71 100644 --- a/java/com/google/gerrit/server/validators/AccountActivationValidationListener.java +++ b/java/com/google/gerrit/server/validators/AccountActivationValidationListener.java @@ -26,6 +26,9 @@ public interface AccountActivationValidationListener { /** * Called when an account should be activated to allow validation of the account activation. * + *

See {@link com.google.gerrit.extensions.events.AccountActivationListener} for a listener + * that's run after the account got activated. + * * @param account the account that should be activated * @throws ValidationException if validation fails */ @@ -34,6 +37,9 @@ public interface AccountActivationValidationListener { /** * Called when an account should be deactivated to allow validation of the account deactivation. * + *

See {@link com.google.gerrit.extensions.events.AccountActivationListener} for a listener + * that's run after the account got deactivated. + * * @param account the account that should be deactivated * @throws ValidationException if validation fails */ diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index ac007827c5..5ba6d395b5 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -44,6 +44,10 @@ import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.verifyZeroInteractions; import com.github.rholder.retry.StopStrategies; import com.google.common.cache.LoadingCache; @@ -104,6 +108,7 @@ import com.google.gerrit.extensions.common.EmailInfo; import com.google.gerrit.extensions.common.GpgKeyInfo; import com.google.gerrit.extensions.common.GroupInfo; import com.google.gerrit.extensions.common.SshKeyInfo; +import com.google.gerrit.extensions.events.AccountActivationListener; import com.google.gerrit.extensions.events.GitReferenceUpdatedListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.AuthException; @@ -589,7 +594,7 @@ public class AccountIT extends AbstractDaemonTest { Account.Id deactivatableAccountId = accountOperations.newAccount().preferredEmail("foo@deactivatable.com").create(); - AccountActivationValidationListener listener = + AccountActivationValidationListener validationListener = new AccountActivationValidationListener() { @Override public void validateActivation(AccountState account) throws ValidationException { @@ -607,7 +612,11 @@ public class AccountIT extends AbstractDaemonTest { } } }; - try (Registration registration = extensionRegistry.newRegistration().add(listener)) { + + AccountActivationListener listener = mock(AccountActivationListener.class); + + try (Registration registration = + extensionRegistry.newRegistration().add(validationListener).add(listener)) { /* Test account that can be activated, but not deactivated */ // Deactivate account that is already inactive ResourceConflictException thrown = @@ -616,14 +625,18 @@ public class AccountIT extends AbstractDaemonTest { () -> gApi.accounts().id(activatableAccountId.get()).setActive(false)); assertThat(thrown).hasMessageThat().isEqualTo("account not active"); assertThat(accountOperations.account(activatableAccountId).get().active()).isFalse(); + verifyZeroInteractions(listener); // Activate account that can be activated gApi.accounts().id(activatableAccountId.get()).setActive(true); assertThat(accountOperations.account(activatableAccountId).get().active()).isTrue(); + verify(listener).onAccountActivated(activatableAccountId.get()); + verifyNoMoreInteractions(listener); // Activate account that is already active gApi.accounts().id(activatableAccountId.get()).setActive(true); assertThat(accountOperations.account(activatableAccountId).get().active()).isTrue(); + verifyZeroInteractions(listener); // Try deactivating account that cannot be deactivated thrown = @@ -632,15 +645,19 @@ public class AccountIT extends AbstractDaemonTest { () -> gApi.accounts().id(activatableAccountId.get()).setActive(false)); assertThat(thrown).hasMessageThat().isEqualTo("not allowed to deactive account"); assertThat(accountOperations.account(activatableAccountId).get().active()).isTrue(); + verifyZeroInteractions(listener); /* Test account that can be deactivated, but not activated */ // Activate account that is already inactive gApi.accounts().id(deactivatableAccountId.get()).setActive(true); assertThat(accountOperations.account(deactivatableAccountId).get().active()).isTrue(); + verifyZeroInteractions(listener); // Deactivate account that can be deactivated gApi.accounts().id(deactivatableAccountId.get()).setActive(false); assertThat(accountOperations.account(deactivatableAccountId).get().active()).isFalse(); + verify(listener).onAccountDeactivated(deactivatableAccountId.get()); + verifyNoMoreInteractions(listener); // Deactivate account that is already inactive thrown = @@ -649,6 +666,7 @@ public class AccountIT extends AbstractDaemonTest { () -> gApi.accounts().id(deactivatableAccountId.get()).setActive(false)); assertThat(thrown).hasMessageThat().isEqualTo("account not active"); assertThat(accountOperations.account(deactivatableAccountId).get().active()).isFalse(); + verifyZeroInteractions(listener); // Try activating account that cannot be activated thrown = @@ -657,6 +675,7 @@ public class AccountIT extends AbstractDaemonTest { () -> gApi.accounts().id(deactivatableAccountId.get()).setActive(true)); assertThat(thrown).hasMessageThat().isEqualTo("not allowed to active account"); assertThat(accountOperations.account(deactivatableAccountId).get().active()).isFalse(); + verifyZeroInteractions(listener); } } diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java index d37f019b19..80eff9606a 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountListenersIT.java @@ -20,6 +20,7 @@ import static com.google.gerrit.testing.GerritJUnit.assertThrows; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.testsuite.account.AccountOperations; +import com.google.gerrit.extensions.events.AccountActivationListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; @@ -48,14 +49,18 @@ public class AccountListenersIT extends LightweightPluginDaemonTest { @Override protected void configure() { DynamicSet.bind(binder(), AccountActivationValidationListener.class).to(Validator.class); + DynamicSet.bind(binder(), AccountActivationListener.class).to(Listener.class); } } Validator validator; + Listener listener; @Before public void setUp() { validator = plugin.getSysInjector().getInstance(Validator.class); + + listener = plugin.getSysInjector().getInstance(Listener.class); } @Test @@ -65,7 +70,8 @@ public class AccountListenersIT extends LightweightPluginDaemonTest { gApi.accounts().id(id).setActive(true); validator.assertActivationValidation(id); - validator.assertNoMoreEvents(); + listener.assertActivated(id); + assertNoMoreEvents(); assertThat(gApi.accounts().id(id).getActive()).isTrue(); } @@ -82,7 +88,8 @@ public class AccountListenersIT extends LightweightPluginDaemonTest { }); validator.assertActivationValidation(id); - validator.assertNoMoreEvents(); + // No call to activation listener as validation failed + assertNoMoreEvents(); assertThat(gApi.accounts().id(id).getActive()).isFalse(); } @@ -93,7 +100,8 @@ public class AccountListenersIT extends LightweightPluginDaemonTest { gApi.accounts().id(id).setActive(false); validator.assertDeactivationValidation(id); - validator.assertNoMoreEvents(); + listener.assertDeactivated(id); + assertNoMoreEvents(); assertThat(gApi.accounts().id(id).getActive()).isFalse(); } @@ -110,10 +118,16 @@ public class AccountListenersIT extends LightweightPluginDaemonTest { }); validator.assertDeactivationValidation(id); - validator.assertNoMoreEvents(); + // No call to activation listener as validation failed + assertNoMoreEvents(); assertThat(gApi.accounts().id(id).getActive()).isTrue(); } + private void assertNoMoreEvents() { + validator.assertNoMoreEvents(); + listener.assertNoMoreEvents(); + } + @Singleton public static class Validator implements AccountActivationValidationListener { private Integer lastIdActivationValidation; @@ -162,4 +176,37 @@ public class AccountListenersIT extends LightweightPluginDaemonTest { lastIdDeactivationValidation = null; } } + + @Singleton + public static class Listener implements AccountActivationListener { + private Integer lastIdActivated; + private Integer lastIdDeactivated; + + @Override + public void onAccountActivated(int id) { + assertThat(lastIdActivated).isNull(); + lastIdActivated = id; + } + + @Override + public void onAccountDeactivated(int id) { + assertThat(lastIdDeactivated).isNull(); + lastIdDeactivated = id; + } + + private void assertNoMoreEvents() { + assertThat(lastIdActivated).isNull(); + assertThat(lastIdDeactivated).isNull(); + } + + private void assertDeactivated(int id) { + assertThat(lastIdDeactivated).isEqualTo(id); + lastIdDeactivated = null; + } + + private void assertActivated(int id) { + assertThat(lastIdActivated).isEqualTo(id); + lastIdActivated = null; + } + } } -- cgit v1.2.3 From 9b2d1da73fd1a1dbe40d8bba9428194201a2ecf9 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Fri, 1 May 2020 11:12:44 +0200 Subject: Close open SSH connections upon account deactivation Deactivating an account prohibits logging in on new SSH connections. But it did not close already open connections (which may be kept open long into the future as for example stream-events) and did not prohibit opening new sessions on existing authenticated connections. By closing all open sessions for a user upon deactivation, we force the user through authentication again. Thereby, we lock out inactive users, as this authentication will fail as the account is now inactive. Change-Id: Icbe6f04a96297ee7d7e3748562a3f95206fb9ed2 --- .../gerrit/sshd/InactiveAccountDisconnector.java | 60 ++++++++++++++++++++++ java/com/google/gerrit/sshd/SshModule.java | 4 ++ 2 files changed, 64 insertions(+) create mode 100644 java/com/google/gerrit/sshd/InactiveAccountDisconnector.java diff --git a/java/com/google/gerrit/sshd/InactiveAccountDisconnector.java b/java/com/google/gerrit/sshd/InactiveAccountDisconnector.java new file mode 100644 index 0000000000..1086626ce6 --- /dev/null +++ b/java/com/google/gerrit/sshd/InactiveAccountDisconnector.java @@ -0,0 +1,60 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.sshd; + +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.extensions.events.AccountActivationListener; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.sshd.BaseCommand.Failure; +import com.google.inject.Inject; +import java.io.IOException; + +/** Closes open SSH connections upon account deactivation. */ +public class InactiveAccountDisconnector implements AccountActivationListener { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + private final SshDaemon sshDaemon; + + @Inject + InactiveAccountDisconnector(SshDaemon sshDaemon) { + this.sshDaemon = sshDaemon; + } + + @Override + public void onAccountDeactivated(int id) { + try { + SshUtil.forEachSshSession( + sshDaemon, + (sshId, sshSession, abstractSession, ioSession) -> { + CurrentUser sessionUser = sshSession.getUser(); + if (sessionUser.isIdentifiedUser() && sessionUser.getAccountId().get() == id) { + logger.atInfo().log( + "Disconnecting SSH session %s because user %s(%d) got deactivated", + abstractSession, sessionUser.getLoggableName(), id); + try { + abstractSession.disconnect(-1, "user deactivated"); + } catch (IOException e) { + logger.atWarning().withCause(e).log( + "Failure while deactivating session %s", abstractSession); + } + } + }); + } catch (Failure e) { + // Ssh Daemon no longer running. Since we're only disconnecting connections anyways, this is + // most likely ok, so we log only at info level. + logger.atInfo().withCause(e).log("Failure while disconnecting deactivated account %d", id); + } + } +} diff --git a/java/com/google/gerrit/sshd/SshModule.java b/java/com/google/gerrit/sshd/SshModule.java index e4aa14cf30..9301f8a39a 100644 --- a/java/com/google/gerrit/sshd/SshModule.java +++ b/java/com/google/gerrit/sshd/SshModule.java @@ -19,6 +19,7 @@ import static com.google.inject.Scopes.SINGLETON; import com.google.common.base.CharMatcher; import com.google.common.base.Splitter; +import com.google.gerrit.extensions.events.AccountActivationListener; import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.lifecycle.LifecycleModule; @@ -103,6 +104,9 @@ public class SshModule extends LifecycleModule { DynamicItem.itemOf(binder(), SshCreateCommandInterceptor.class); DynamicSet.setOf(binder(), SshExecuteCommandInterceptor.class); + DynamicSet.bind(binder(), AccountActivationListener.class) + .to(InactiveAccountDisconnector.class); + listener().toInstance(registerInParentInjectors()); listener().to(SshLog.class); listener().to(SshDaemon.class); -- cgit v1.2.3 From 090cbb55925a5b73fbfe0e3f093de26eddfdf3c0 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Sat, 6 Jun 2020 02:29:54 +0000 Subject: Set version to 2.15.19 Change-Id: I9c1b41bcb134b799fb8508514b66a7a4a83a65ca --- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index 265b68fb82..1a2b628255 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.15.19-SNAPSHOT + 2.15.19 jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index 408298875c..6180915ecb 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.15.19-SNAPSHOT + 2.15.19 jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index f3dd8260e8..6c0cf800f0 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.15.19-SNAPSHOT + 2.15.19 jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index 77a31e5fff..3569574798 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.15.19-SNAPSHOT + 2.15.19 jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index c2acf61b1d..ff66efa841 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.15.19-SNAPSHOT + 2.15.19 war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index 746722b8a6..e84f4e417f 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.15.19-SNAPSHOT" +GERRIT_VERSION = "2.15.19" -- cgit v1.2.3 From eaf17f60ce9b04943e0de2e3e510e13c529a0dc0 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Sat, 6 Jun 2020 02:51:43 +0000 Subject: Set version to 2.15.20-SNAPSHOT Change-Id: Ia6aa8dd0fbc17f09329f7efe40bfa41a6e3f793b --- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index 1a2b628255..82199d88e4 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.15.19 + 2.15.20-SNAPSHOT jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index 6180915ecb..fefe81d734 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.15.19 + 2.15.20-SNAPSHOT jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index 6c0cf800f0..9c24d1163f 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.15.19 + 2.15.20-SNAPSHOT jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index 3569574798..ccca22a5de 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.15.19 + 2.15.20-SNAPSHOT jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index ff66efa841..568dddd158 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.15.19 + 2.15.20-SNAPSHOT war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index e84f4e417f..4b922458ff 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.15.19" +GERRIT_VERSION = "2.15.20-SNAPSHOT" -- cgit v1.2.3 From 2ba3b5a55d37315189104d4237c007bae01f908d Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sat, 6 Jun 2020 15:04:24 +0900 Subject: Fix header of "Pitfalls" subsection of Private Changes documentation Change-Id: I3dacba7ffd910122cf1dbd123276c6ef29fe5f14 --- Documentation/intro-user.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt index e502cdd1a3..6b5b543dca 100644 --- a/Documentation/intro-user.txt +++ b/Documentation/intro-user.txt @@ -605,8 +605,7 @@ In that case, care should be taken to prevent the CI system from exposing secret details. [[private-changes-pitfalls]] -Pitfalls -=== +=== Pitfalls If private changes are used, be aware of the following pitfalls: -- cgit v1.2.3 From d962d1240a88daf0cd50b40029e8b575b14e8d1f Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sat, 6 Jun 2020 15:19:02 +0900 Subject: AccountIT#accountIdApi: Get account API with id rather than name On later branches, getting the account API with anything other than the id fails. Change-Id: If02dcf0f9268994b74d50256992ede8460d6c5e9 --- javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 046365fb44..23fc1c0e75 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -3418,7 +3418,7 @@ public class AccountIT extends AbstractDaemonTest { } private AccountApi accountIdApi() throws RestApiException { - return gApi.accounts().id("user"); + return gApi.accounts().id(user.id.get()); } private Set getCookiesNames() { -- cgit v1.2.3 From 43be7b9ae2204a8ff5b761dbcb7b61b360ad148c Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sat, 6 Jun 2020 15:04:24 +0900 Subject: Fix header of "Pitfalls" subsection of Private Changes documentation Change-Id: I3dacba7ffd910122cf1dbd123276c6ef29fe5f14 (cherry picked from commit 2ba3b5a55d37315189104d4237c007bae01f908d) --- Documentation/intro-user.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt index 03f388006a..6e586086ba 100644 --- a/Documentation/intro-user.txt +++ b/Documentation/intro-user.txt @@ -605,8 +605,7 @@ In that case, care should be taken to prevent the CI system from exposing secret details. [[private-changes-pitfalls]] -Pitfalls -=== +=== Pitfalls If private changes are used, be aware of the following pitfalls: -- cgit v1.2.3