Merge stable-3.6 into master
Change-Id: I2b322fc6d3ab08b4c937bed9ebbc0eb7d969c49f
diff --git a/src/main/java/com/googlesource/gerrit/plugins/serviceuser/ValidateServiceUserCommits.java b/src/main/java/com/googlesource/gerrit/plugins/serviceuser/ValidateServiceUserCommits.java
index 6d4783e..f4ef36a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/serviceuser/ValidateServiceUserCommits.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/serviceuser/ValidateServiceUserCommits.java
@@ -51,27 +51,31 @@
PersonIdent committer = receiveEvent.commit.getCommitterIdent();
ServiceUserInfo serviceUser = serviceUserResolver.getAsServiceUser(committer);
if (serviceUser != null) {
- if (serviceUser.owner != null
- && serviceUserResolver.listActiveOwners(serviceUser).isEmpty()) {
- throw new CommitValidationException(
- String.format(
- "Commit %s of service user %s (%s) is rejected because "
- + "all service user owner accounts are inactive.",
- receiveEvent.commit.getId().getName(),
- committer.getName(),
- committer.getEmailAddress()));
- }
Optional<AccountState> creator =
accountCache.get(Account.id(serviceUser.createdBy._accountId));
- if (!creator.isPresent() || !creator.get().account().isActive()) {
- throw new CommitValidationException(
- String.format(
- "Commit %s of service user %s (%s) is rejected because "
- + "the account of the service creator is inactive.",
- receiveEvent.commit.getId().getName(),
- committer.getName(),
- committer.getEmailAddress()));
+
+ if (creator.isPresent() && creator.get().account().isActive()) {
+ return Collections.emptyList();
}
+
+ if (!serviceUserResolver.listActiveOwners(serviceUser).isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ StringBuilder msg = new StringBuilder();
+ msg.append(
+ String.format(
+ "Commit %s of service user %s (%s) is rejected because "
+ + " the account which created the service user is inactive",
+ receiveEvent.commit.getId().name(),
+ committer.getName(),
+ committer.getEmailAddress()));
+
+ if (serviceUser.owner != null) {
+ msg.append(" and all accounts in the owner group are inactive");
+ }
+
+ throw new CommitValidationException(msg.toString());
}
} catch (RestApiException e) {
log.error(e.getMessage(), e);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/serviceuser/ValidateServiceUserCommitsTest.java b/src/test/java/com/googlesource/gerrit/plugins/serviceuser/ValidateServiceUserCommitsTest.java
new file mode 100644
index 0000000..ab6a8b4
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/serviceuser/ValidateServiceUserCommitsTest.java
@@ -0,0 +1,165 @@
+// Copyright (C) 2023 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.googlesource.gerrit.plugins.serviceuser;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.common.GroupInfo;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.events.CommitReceivedEvent;
+import com.google.gerrit.server.git.validators.CommitValidationException;
+import com.googlesource.gerrit.plugins.serviceuser.GetServiceUser.ServiceUserInfo;
+import java.util.Collections;
+import java.util.Optional;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class ValidateServiceUserCommitsTest {
+ private static final Account.Id SERVICE_USER_ACCOUNT_ID = Account.id(100);
+ private static final Account.Id CREATOR_ACCOUNT_ID = Account.id(200);
+ private static final Account.Id OWNER_ACCOUNT_ID = Account.id(300);
+
+ @Mock ServiceUserResolver resolver;
+ @Mock AccountCache accountCache;
+
+ TestRepository<?> testRepo;
+ CommitReceivedEvent event = new CommitReceivedEvent();
+ AccountInfo serviceUserAccountInfo = new AccountInfo(SERVICE_USER_ACCOUNT_ID.get());
+ PersonIdent serviceUserIdent = new PersonIdent("robot", "robot@example.com");
+
+ ValidateServiceUserCommits validator;
+
+ @Before
+ public void setUp() throws Exception {
+ testRepo = new TestRepository<>(new InMemoryRepository(new DfsRepositoryDescription("repo")));
+ event.commit = testRepo.commit().committer(serviceUserIdent).create();
+ validator = new ValidateServiceUserCommits(resolver, accountCache);
+ }
+
+ @Test
+ public void committerNotServiceUser_pass() throws Exception {
+ when(resolver.getAsServiceUser(serviceUserIdent)).thenReturn(null);
+ assertThat(validator.onCommitReceived(event)).isEmpty();
+ }
+
+ @Test
+ public void noCreatorAndNoActiveOwners_reject() throws Exception {
+ ServiceUserInfo serviceUser = setupWithNonExistingCreator();
+ ownerGroupNotSet(serviceUser);
+ assertThrows(CommitValidationException.class, () -> validator.onCommitReceived(event));
+
+ ownerGroupSetButContainsNoActiveMembers(serviceUser);
+ assertThrows(CommitValidationException.class, () -> validator.onCommitReceived(event));
+ }
+
+ @Test
+ public void inactiveCreatorAndNoActiveOwners_reject() throws Exception {
+ ServiceUserInfo serviceUser = setupWithInactiveCreator();
+ ownerGroupNotSet(serviceUser);
+ assertThrows(CommitValidationException.class, () -> validator.onCommitReceived(event));
+
+ ownerGroupSetButContainsNoActiveMembers(serviceUser);
+ assertThrows(CommitValidationException.class, () -> validator.onCommitReceived(event));
+ }
+
+ @Test
+ public void activeCreatorAnNoActiveOwners_pass() throws Exception {
+ ServiceUserInfo serviceUser = setupWithActiveCreator();
+ ownerGroupNotSet(serviceUser);
+ assertThat(validator.onCommitReceived(event)).isEmpty();
+
+ ownerGroupSetButContainsNoActiveMembers(serviceUser);
+ assertThat(validator.onCommitReceived(event)).isEmpty();
+ }
+
+ @Test
+ public void inactiveCreatorAndActiveOwners_pass() throws Exception {
+ ServiceUserInfo serviceUser = setupWithInactiveCreator();
+ ownerGroupSetAndContainsActiveMembers(serviceUser);
+ assertThat(validator.onCommitReceived(event)).isEmpty();
+ }
+
+ @Test
+ public void activeCreatorAndActiveOwners_pass() throws Exception {
+ ServiceUserInfo serviceUser = setupWithActiveCreator();
+ ownerGroupSetAndContainsActiveMembers(serviceUser);
+ assertThat(validator.onCommitReceived(event)).isEmpty();
+ }
+
+ private ServiceUserInfo setupWithNonExistingCreator() throws Exception {
+ ServiceUserInfo serviceUser = new ServiceUserInfo(serviceUserAccountInfo);
+ when(resolver.getAsServiceUser(serviceUserIdent)).thenReturn(serviceUser);
+
+ serviceUser.createdBy = new AccountInfo(CREATOR_ACCOUNT_ID.get());
+ // accountCache returns empty Optional: means creator account not found
+ when(accountCache.get(CREATOR_ACCOUNT_ID)).thenReturn(Optional.empty());
+
+ return serviceUser;
+ }
+
+ private ServiceUserInfo setupWithActiveCreator() throws Exception {
+ return setupWithCreator(true);
+ }
+
+ private ServiceUserInfo setupWithInactiveCreator() throws Exception {
+ return setupWithCreator(false);
+ }
+
+ private ServiceUserInfo setupWithCreator(boolean active) throws Exception {
+ ServiceUserInfo serviceUser = new ServiceUserInfo(serviceUserAccountInfo);
+ when(resolver.getAsServiceUser(serviceUserIdent)).thenReturn(serviceUser);
+
+ serviceUser.createdBy = new AccountInfo(CREATOR_ACCOUNT_ID.get());
+ AccountState creatorAccountState = mock(AccountState.class);
+ Account creatorAccount = mock(Account.class);
+ when(creatorAccountState.account()).thenReturn(creatorAccount);
+ when(creatorAccount.isActive()).thenReturn(active);
+ when(accountCache.get(CREATOR_ACCOUNT_ID)).thenReturn(Optional.of(creatorAccountState));
+
+ return serviceUser;
+ }
+
+ private void ownerGroupNotSet(ServiceUserInfo serviceUser) {
+ serviceUser.owner = null;
+ }
+
+ private void ownerGroupSetButContainsNoActiveMembers(ServiceUserInfo serviceUser)
+ throws Exception {
+ serviceUser.owner = new GroupInfo();
+ lenient().when(resolver.listActiveOwners(serviceUser)).thenReturn(Collections.emptyList());
+ }
+
+ private void ownerGroupSetAndContainsActiveMembers(ServiceUserInfo serviceUser) throws Exception {
+ serviceUser.owner = new GroupInfo();
+ lenient()
+ .when(resolver.listActiveOwners(serviceUser))
+ .thenReturn(Collections.singletonList(new AccountInfo(OWNER_ACCOUNT_ID.get())));
+ }
+}
