From f0d71a822d69721294739063627fbfcd523ec254 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 20 Apr 2018 13:03:34 +0200 Subject: [PATCH] Add validation listener for account activation Change-Id: Ib238701b2a9563157f37834fa0944b8e40db3701 Signed-off-by: Edwin Kempin --- Documentation/config-validation.txt | 8 ++ .../server/account/SetInactiveFlag.java | 39 +++++++- .../server/config/GerritGlobalModule.java | 2 + .../server/restapi/account/PutActive.java | 4 +- .../AccountActivationValidationListener.java | 41 ++++++++ .../acceptance/api/accounts/AccountIT.java | 94 +++++++++++++++++++ 6 files changed, 183 insertions(+), 5 deletions(-) create mode 100644 java/com/google/gerrit/server/validators/AccountActivationValidationListener.java diff --git a/Documentation/config-validation.txt b/Documentation/config-validation.txt index c54717bcc1..24932a8d00 100644 --- a/Documentation/config-validation.txt +++ b/Documentation/config-validation.txt @@ -114,6 +114,14 @@ This interface provides a low-level e-mail filtering API for plugins. Plugins implementing the `OutgoingEmailValidationListener` interface can perform filtering of outgoing e-mails just before they are sent. +[[account-activation-validation]] +== Account activation validation + + +Plugins implementing the `AccountActivationValidationListener` interface can +perform validation when an account is activated or deactivated via the Gerrit +REST API or the Java extension API. + GERRIT ------ diff --git a/java/com/google/gerrit/server/account/SetInactiveFlag.java b/java/com/google/gerrit/server/account/SetInactiveFlag.java index 89fc0f3c8a..af68d05269 100644 --- a/java/com/google/gerrit/server/account/SetInactiveFlag.java +++ b/java/com/google/gerrit/server/account/SetInactiveFlag.java @@ -14,33 +14,43 @@ package com.google.gerrit.server.account; +import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.ServerInitiated; +import com.google.gerrit.server.validators.AccountActivationValidationListener; +import com.google.gerrit.server.validators.ValidationException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; +import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class SetInactiveFlag { - + private final DynamicSet + accountActivationValidationListeners; private final Provider accountsUpdateProvider; @Inject - SetInactiveFlag(@ServerInitiated Provider accountsUpdateProvider) { + SetInactiveFlag( + DynamicSet accountActivationValidationListeners, + @ServerInitiated Provider accountsUpdateProvider) { + this.accountActivationValidationListeners = accountActivationValidationListeners; this.accountsUpdateProvider = accountsUpdateProvider; } public Response deactivate(Account.Id accountId) throws RestApiException, IOException, ConfigInvalidException, OrmException { AtomicBoolean alreadyInactive = new AtomicBoolean(false); + AtomicReference> exception = new AtomicReference<>(Optional.empty()); accountsUpdateProvider .get() .update( @@ -50,10 +60,21 @@ public class SetInactiveFlag { if (!a.getAccount().isActive()) { alreadyInactive.set(true); } else { + for (AccountActivationValidationListener l : accountActivationValidationListeners) { + try { + l.validateDeactivation(a); + } catch (ValidationException e) { + exception.set(Optional.of(new ResourceConflictException(e.getMessage(), e))); + return; + } + } u.setActive(false); } }) .orElseThrow(() -> new ResourceNotFoundException("account not found")); + if (exception.get().isPresent()) { + throw exception.get().get(); + } if (alreadyInactive.get()) { throw new ResourceConflictException("account not active"); } @@ -61,8 +82,9 @@ public class SetInactiveFlag { } public Response activate(Account.Id accountId) - throws ResourceNotFoundException, IOException, ConfigInvalidException, OrmException { + throws RestApiException, IOException, ConfigInvalidException, OrmException { AtomicBoolean alreadyActive = new AtomicBoolean(false); + AtomicReference> exception = new AtomicReference<>(Optional.empty()); accountsUpdateProvider .get() .update( @@ -72,10 +94,21 @@ public class SetInactiveFlag { if (a.getAccount().isActive()) { alreadyActive.set(true); } else { + for (AccountActivationValidationListener l : accountActivationValidationListeners) { + try { + l.validateActivation(a); + } catch (ValidationException e) { + exception.set(Optional.of(new ResourceConflictException(e.getMessage(), e))); + return; + } + } u.setActive(true); } }) .orElseThrow(() -> new ResourceNotFoundException("account not found")); + if (exception.get().isPresent()) { + throw exception.get().get(); + } return alreadyActive.get() ? Response.ok("") : Response.created(""); } } diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index 5da5d1a0ae..09687f59c6 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -184,6 +184,7 @@ import com.google.gerrit.server.tools.ToolsCatalog; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.util.IdGenerator; import com.google.gerrit.server.util.ThreadLocalRequestContext; +import com.google.gerrit.server.validators.AccountActivationValidationListener; import com.google.gerrit.server.validators.AssigneeValidationListener; import com.google.gerrit.server.validators.GroupCreationValidationListener; import com.google.gerrit.server.validators.HashtagValidationListener; @@ -365,6 +366,7 @@ public class GerritGlobalModule extends FactoryModule { DynamicSet.setOf(binder(), GroupCreationValidationListener.class); DynamicSet.setOf(binder(), HashtagValidationListener.class); DynamicSet.setOf(binder(), OutgoingEmailValidationListener.class); + DynamicSet.setOf(binder(), AccountActivationValidationListener.class); DynamicItem.itemOf(binder(), AvatarProvider.class); DynamicSet.setOf(binder(), LifecycleListener.class); DynamicSet.setOf(binder(), TopMenu.class); diff --git a/java/com/google/gerrit/server/restapi/account/PutActive.java b/java/com/google/gerrit/server/restapi/account/PutActive.java index 147b20f767..82557814cb 100644 --- a/java/com/google/gerrit/server/restapi/account/PutActive.java +++ b/java/com/google/gerrit/server/restapi/account/PutActive.java @@ -17,8 +17,8 @@ package com.google.gerrit.server.restapi.account; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.extensions.annotations.RequiresCapability; import com.google.gerrit.extensions.common.Input; -import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.server.account.AccountResource; import com.google.gerrit.server.account.SetInactiveFlag; @@ -41,7 +41,7 @@ public class PutActive implements RestModifyView { @Override public Response apply(AccountResource rsrc, Input input) - throws ResourceNotFoundException, OrmException, IOException, ConfigInvalidException { + throws RestApiException, OrmException, IOException, ConfigInvalidException { return setInactiveFlag.activate(rsrc.getUser().getAccountId()); } } diff --git a/java/com/google/gerrit/server/validators/AccountActivationValidationListener.java b/java/com/google/gerrit/server/validators/AccountActivationValidationListener.java new file mode 100644 index 0000000000..bc52308609 --- /dev/null +++ b/java/com/google/gerrit/server/validators/AccountActivationValidationListener.java @@ -0,0 +1,41 @@ +// Copyright (C) 2018 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.server.validators; + +import com.google.gerrit.extensions.annotations.ExtensionPoint; +import com.google.gerrit.server.account.AccountState; + +/** + * Validator that is invoked when an account activated or deactivated via the Gerrit REST API or the + * Java extension API. + */ +@ExtensionPoint +public interface AccountActivationValidationListener { + /** + * Called when an account should be activated to allow validation of the account activation. + * + * @param account the account that should be activated + * @throws ValidationException if validation fails + */ + void validateActivation(AccountState account) throws ValidationException; + + /** + * Called when an account should be deactivated to allow validation of the account deactivation. + * + * @param account the account that should be deactivated + * @throws ValidationException if validation fails + */ + void validateDeactivation(AccountState account) throws ValidationException; +} diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index e88e662468..73fbb08117 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -117,6 +117,8 @@ import com.google.gerrit.server.project.RefPattern; import com.google.gerrit.server.query.account.InternalAccountQuery; import com.google.gerrit.server.update.RetryHelper; import com.google.gerrit.server.util.MagicBranch; +import com.google.gerrit.server.validators.AccountActivationValidationListener; +import com.google.gerrit.server.validators.ValidationException; import com.google.gerrit.testing.ConfigSuite; import com.google.gerrit.testing.FakeEmailSender.Message; import com.google.gerrit.testing.TestTimeUtil; @@ -216,6 +218,9 @@ public class AccountIT extends AbstractDaemonTest { @Inject private AccountOperations accountOperations; + @Inject + private DynamicSet accountActivationValidationListeners; + private AccountIndexedCounter accountIndexedCounter; private RegistrationHandle accountIndexEventCounterHandle; private RefUpdateCounter refUpdateCounter; @@ -518,6 +523,95 @@ public class AccountIT extends AbstractDaemonTest { accountIndexedCounter.assertReindexOf(user); } + @Test + public void validateAccountActivation() throws Exception { + com.google.gerrit.acceptance.testsuite.account.TestAccount activatableAccount = + accountOperations.newAccount().inactive().preferredEmail("foo@activatable.com").create(); + com.google.gerrit.acceptance.testsuite.account.TestAccount deactivatableAccount = + accountOperations.newAccount().preferredEmail("foo@deactivatable.com").create(); + RegistrationHandle registrationHandle = + accountActivationValidationListeners.add( + new AccountActivationValidationListener() { + @Override + public void validateActivation(AccountState account) throws ValidationException { + String preferredEmail = account.getAccount().getPreferredEmail(); + if (preferredEmail == null || !preferredEmail.endsWith("@activatable.com")) { + throw new ValidationException("not allowed to active account"); + } + } + + @Override + public void validateDeactivation(AccountState account) throws ValidationException { + String preferredEmail = account.getAccount().getPreferredEmail(); + if (preferredEmail == null || !preferredEmail.endsWith("@deactivatable.com")) { + throw new ValidationException("not allowed to deactive account"); + } + } + }); + try { + /* Test account that can be activated, but not deactivated */ + // Deactivate account that is already inactive + try { + gApi.accounts().id(activatableAccount.accountId().get()).setActive(false); + fail("Expected exception"); + } catch (ResourceConflictException e) { + assertThat(e.getMessage()).isEqualTo("account not active"); + } + assertThat(accountOperations.account(activatableAccount.accountId()).get().active()) + .isFalse(); + + // Activate account that can be activated + gApi.accounts().id(activatableAccount.accountId().get()).setActive(true); + assertThat(accountOperations.account(activatableAccount.accountId()).get().active()).isTrue(); + + // Activate account that is already active + gApi.accounts().id(activatableAccount.accountId().get()).setActive(true); + assertThat(accountOperations.account(activatableAccount.accountId()).get().active()).isTrue(); + + // Try deactivating account that cannot be deactivated + try { + gApi.accounts().id(activatableAccount.accountId().get()).setActive(false); + fail("Expected exception"); + } catch (ResourceConflictException e) { + assertThat(e.getMessage()).isEqualTo("not allowed to deactive account"); + } + assertThat(accountOperations.account(activatableAccount.accountId()).get().active()).isTrue(); + + /* Test account that can be deactivated, but not activated */ + // Activate account that is already inactive + gApi.accounts().id(deactivatableAccount.accountId().get()).setActive(true); + assertThat(accountOperations.account(deactivatableAccount.accountId()).get().active()) + .isTrue(); + + // Deactivate account that can be deactivated + gApi.accounts().id(deactivatableAccount.accountId().get()).setActive(false); + assertThat(accountOperations.account(deactivatableAccount.accountId()).get().active()) + .isFalse(); + + // Deactivate account that is already inactive + try { + gApi.accounts().id(deactivatableAccount.accountId().get()).setActive(false); + fail("Expected exception"); + } catch (ResourceConflictException e) { + assertThat(e.getMessage()).isEqualTo("account not active"); + } + assertThat(accountOperations.account(deactivatableAccount.accountId()).get().active()) + .isFalse(); + + // Try activating account that cannot be activated + try { + gApi.accounts().id(deactivatableAccount.accountId().get()).setActive(true); + fail("Expected exception"); + } catch (ResourceConflictException e) { + assertThat(e.getMessage()).isEqualTo("not allowed to active account"); + } + assertThat(accountOperations.account(deactivatableAccount.accountId()).get().active()) + .isFalse(); + } finally { + registrationHandle.remove(); + } + } + @Test public void deactivateSelf() throws Exception { exception.expect(ResourceConflictException.class);