Skip to content

Commit

Permalink
Merge branch 'stable-3.1' into stable-3.2
Browse files Browse the repository at this point in the history
* stable-3.1:
  Fix header of "Pitfalls" subsection of Private Changes documentation
  AccountIT#accountIdApi: Get account API with id rather than name
  Fix header of "Pitfalls" subsection of Private Changes documentation
  Set version to 2.15.20-SNAPSHOT
  Set version to 2.15.19
  Close open SSH connections upon account deactivation
  Allow to listen for account deactivations
  Extract method to iterate SSH sessions
  CacheBasedWebSession: Remove unnecessary 'final' in constructor args
  Add account listener integration tests using a real plugin
  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: I2d23a6fdcf6b44a9f55f3c01f5090aa8f0a343d4
  • Loading branch information
lucamilanesio committed Jun 7, 2020
2 parents 82db49e + 3e31609 commit 28869fc
Show file tree
Hide file tree
Showing 16 changed files with 594 additions and 42 deletions.
4 changes: 4 additions & 0 deletions Documentation/dev-plugins.txt
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,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
Expand Down
3 changes: 1 addition & 2 deletions Documentation/intro-user.txt
Original file line number Diff line number Diff line change
Expand Up @@ -615,8 +615,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:

Expand Down
8 changes: 8 additions & 0 deletions java/com/google/gerrit/acceptance/ExtensionRegistry.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.gerrit.extensions.config.CapabilityDefinition;
import com.google.gerrit.extensions.config.DownloadScheme;
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.ChangeIndexedListener;
import com.google.gerrit.extensions.events.CommentAddedListener;
Expand Down Expand Up @@ -73,6 +74,7 @@ public class ExtensionRegistry {
private final DynamicSet<GroupBackend> groupBackends;
private final DynamicSet<AccountActivationValidationListener>
accountActivationValidationListeners;
private final DynamicSet<AccountActivationListener> accountActivationListeners;
private final DynamicSet<OnSubmitValidationListener> onSubmitValidationListeners;
private final DynamicSet<WorkInProgressStateChangedListener> workInProgressStateChangedListeners;
private final DynamicMap<CapabilityDefinition> capabilityDefinitions;
Expand Down Expand Up @@ -101,6 +103,7 @@ public class ExtensionRegistry {
DynamicSet<RevisionCreatedListener> revisionCreatedListeners,
DynamicSet<GroupBackend> groupBackends,
DynamicSet<AccountActivationValidationListener> accountActivationValidationListeners,
DynamicSet<AccountActivationListener> accountActivationListeners,
DynamicSet<OnSubmitValidationListener> onSubmitValidationListeners,
DynamicSet<WorkInProgressStateChangedListener> workInProgressStateChangedListeners,
DynamicMap<CapabilityDefinition> capabilityDefinitions,
Expand All @@ -126,6 +129,7 @@ public class ExtensionRegistry {
this.revisionCreatedListeners = revisionCreatedListeners;
this.groupBackends = groupBackends;
this.accountActivationValidationListeners = accountActivationValidationListeners;
this.accountActivationListeners = accountActivationListeners;
this.onSubmitValidationListeners = onSubmitValidationListeners;
this.workInProgressStateChangedListeners = workInProgressStateChangedListeners;
this.capabilityDefinitions = capabilityDefinitions;
Expand Down Expand Up @@ -229,6 +233,10 @@ public Registration add(
return add(accountActivationValidationListeners, accountActivationValidationListener);
}

public Registration add(AccountActivationListener accountDeactivatedListener) {
return add(accountActivationListeners, accountDeactivatedListener);
}

public Registration add(OnSubmitValidationListener onSubmitValidationListener) {
return add(onSubmitValidationListeners, onSubmitValidationListener);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
*
* <p>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) {}
}
22 changes: 20 additions & 2 deletions java/com/google/gerrit/httpd/CacheBasedWebSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.entities.Account;
Expand All @@ -27,6 +28,7 @@
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;
Expand All @@ -40,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;
Expand All @@ -50,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;
Expand All @@ -62,13 +65,15 @@ protected CacheBasedWebSession(
WebSessionManager manager,
AuthConfig authConfig,
Provider<AnonymousUser> anonymousProvider,
IdentifiedUser.RequestFactory identified) {
IdentifiedUser.RequestFactory identified,
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);
Expand All @@ -85,6 +90,10 @@ protected CacheBasedWebSession(
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);
Expand Down Expand Up @@ -177,6 +186,11 @@ public void login(AuthResult res, boolean rememberMe) {
manager.destroy(key);
}

if (!checkAccountStatus(id)) {
val = null;
return;
}

key = manager.createKey(id);
val = manager.createVal(key, id, rememberMe, identity, null, null);
saveCookie();
Expand Down Expand Up @@ -207,6 +221,10 @@ public String getSessionId() {
return val != null ? val.getSessionId() : null;
}

private boolean checkAccountStatus(Account.Id id) {
return byIdCache.get(id).filter(as -> as.account().isActive()).isPresent();
}

private void saveCookie() {
if (response == null) {
return;
Expand Down
12 changes: 10 additions & 2 deletions java/com/google/gerrit/httpd/H2CacheBasedWebSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,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;
Expand Down Expand Up @@ -59,8 +60,15 @@ protected void configure() {
@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);
}
}
24 changes: 22 additions & 2 deletions java/com/google/gerrit/server/account/SetInactiveFlag.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,13 +39,16 @@ public class SetInactiveFlag {
private final PluginSetContext<AccountActivationValidationListener>
accountActivationValidationListeners;
private final Provider<AccountsUpdate> accountsUpdateProvider;
private final PluginSetContext<AccountActivationListener> accountActivationListeners;

@Inject
SetInactiveFlag(
PluginSetContext<AccountActivationValidationListener> accountActivationValidationListeners,
@ServerInitiated Provider<AccountsUpdate> accountsUpdateProvider) {
@ServerInitiated Provider<AccountsUpdate> accountsUpdateProvider,
PluginSetContext<AccountActivationListener> accountActivationListeners) {
this.accountActivationValidationListeners = accountActivationValidationListeners;
this.accountsUpdateProvider = accountsUpdateProvider;
this.accountActivationListeners = accountActivationListeners;
}

public Response<?> deactivate(Account.Id accountId)
Expand Down Expand Up @@ -77,6 +81,12 @@ public Response<?> deactivate(Account.Id accountId)
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();
}

Expand Down Expand Up @@ -107,6 +117,16 @@ public Response<String> activate(Account.Id accountId)
if (exception.get().isPresent()) {
throw exception.get().get();
}
return alreadyActive.get() ? Response.ok() : Response.created();

Response<String> res;
if (alreadyActive.get()) {
res = Response.ok();
} else {
res = Response.created();

int id = accountId.get();
accountActivationListeners.runEach(l -> l.onAccountActivated(id));
}
return res;
}
}
2 changes: 2 additions & 0 deletions java/com/google/gerrit/server/config/GerritGlobalModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
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;
Expand Down Expand Up @@ -343,6 +344,7 @@ protected void configure() {
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ public interface AccountActivationValidationListener {
/**
* Called when an account should be activated to allow validation of the account activation.
*
* <p>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
*/
Expand All @@ -34,6 +37,9 @@ public interface AccountActivationValidationListener {
/**
* Called when an account should be deactivated to allow validation of the account deactivation.
*
* <p>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
*/
Expand Down
60 changes: 60 additions & 0 deletions java/com/google/gerrit/sshd/InactiveAccountDisconnector.java
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
4 changes: 4 additions & 0 deletions java/com/google/gerrit/sshd/SshModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

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;
Expand Down Expand Up @@ -103,6 +104,9 @@ protected void configure() {
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);
Expand Down

0 comments on commit 28869fc

Please sign in to comment.