From 3c9142cce79000f185b8d78a7c00caf7d2cce2d1 Mon Sep 17 00:00:00 2001 From: Bryan Turner Date: Wed, 28 Feb 2018 20:49:08 -0800 Subject: [PATCH 1/2] Use a separate ExecutorService to fix #71. - Updated AsyncProcessor to look at the Bitbucket Server verson and choose between using the shared ExecutorService (5.9+) or creating its own ExecutorService (previous versions) - Bitbucket Server 5.9 includes a fix which prevents add-ons using the shared BucketedExecutor from blocking hook callbacks - Removed some dead code in AsyncProcessor and PullRequestListener - Simplified Maven structure and reconfigured how integration and unit tests are distinguished - Moved unit tests from src/test/java/ut to src/test/java - Added a simple UserResourceTest which requests configuration for a repository which hasn't had any applied - This verifies that the PR Harmony plugin has been installed and enabled successfully, and that its REST resource is available - Simplified some of the processing in UserUtils to use the API more efficiently and avoid various IDEA inspection warnings --- pom.xml | 160 +++++++----------- .../monitorjbl/plugins/AsyncProcessor.java | 54 +++--- .../plugins/PullRequestListener.java | 3 - .../com/monitorjbl/plugins/UserUtils.java | 68 ++++---- .../plugins/config/UserResource.java | 13 +- src/main/resources/atlassian-plugin.xml | 5 +- .../plugins/AsyncProcessorTest.java | 53 ++++++ .../plugins/CommitBlockerHookTest.java | 0 .../monitorjbl/plugins/MergeBlockerTest.java | 0 .../plugins/PullRequestApprovalTest.java | 0 .../plugins/PullRequestListenerTest.java | 0 .../monitorjbl/plugins/RegexUtilsTest.java | 0 .../com/monitorjbl/plugins/TestUtils.java | 0 .../com/monitorjbl/plugins/UserUtilsTest.java | 0 .../plugins/config/ConfigDaoTest.java | 0 .../plugins/config/ConfigServletTest.java | 0 .../it/com/monitorjbl/plugins/ui/.gitignore | 0 .../plugins/ui/UserResourceTest.java | 31 ++++ start-test-server.sh | 2 +- 19 files changed, 230 insertions(+), 159 deletions(-) create mode 100644 src/test/java/com/monitorjbl/plugins/AsyncProcessorTest.java rename src/test/java/{ut => }/com/monitorjbl/plugins/CommitBlockerHookTest.java (100%) rename src/test/java/{ut => }/com/monitorjbl/plugins/MergeBlockerTest.java (100%) rename src/test/java/{ut => }/com/monitorjbl/plugins/PullRequestApprovalTest.java (100%) rename src/test/java/{ut => }/com/monitorjbl/plugins/PullRequestListenerTest.java (100%) rename src/test/java/{ut => }/com/monitorjbl/plugins/RegexUtilsTest.java (100%) rename src/test/java/{ut => }/com/monitorjbl/plugins/TestUtils.java (100%) rename src/test/java/{ut => }/com/monitorjbl/plugins/UserUtilsTest.java (100%) rename src/test/java/{ut => }/com/monitorjbl/plugins/config/ConfigDaoTest.java (100%) rename src/test/java/{ut => }/com/monitorjbl/plugins/config/ConfigServletTest.java (100%) delete mode 100644 src/test/java/it/com/monitorjbl/plugins/ui/.gitignore create mode 100644 src/test/java/it/com/monitorjbl/plugins/ui/UserResourceTest.java diff --git a/pom.xml b/pom.xml index 65f5713..f443b21 100644 --- a/pom.xml +++ b/pom.xml @@ -1,11 +1,11 @@ - 4.0.0 + com.monitorjbl.plugins pr-harmony - 2.5.0 + 2.5.1-SNAPSHOT atlassian-plugin PR Harmony @@ -33,64 +33,79 @@ com.atlassian.bitbucket.server bitbucket-api provided - 4.11.1 com.atlassian.bitbucket.server - bitbucket-spi + bitbucket-build-api provided - com.atlassian.sal - sal-api + com.atlassian.bitbucket.server + bitbucket-spi provided com.atlassian.bitbucket.server - bitbucket-build-api + bitbucket-util + provided + + + + com.atlassian.plugins.rest + atlassian-rest-common + provided + + + com.atlassian.sal + sal-api provided com.atlassian.templaterenderer atlassian-template-renderer-api - 1.5.7 provided + javax.servlet javax.servlet-api provided - - commons-lang - commons-lang - 2.6 - provided - com.google.guava guava - 21.0 provided + + com.atlassian.bitbucket.server + bitbucket-it-common + test + + + com.atlassian.bitbucket.server + bitbucket-page-objects + test + + + ch.qos.logback + logback-classic + test + junit junit - 4.11 test org.hamcrest - hamcrest-all - 1.3 + hamcrest-library test org.mockito mockito-core - 1.9.5 org.hamcrest @@ -99,47 +114,9 @@ test - - - - com.atlassian.plugins - atlassian-plugins-osgi-testrunner - ${plugin.testrunner.version} - test - - - javax.ws.rs - jsr311-api - 1.1.1 - provided - - - com.google.code.gson - gson - 2.2.2-atlassian-1 - - - org.slf4j - slf4j-log4j12 - 1.7.12 - test - - - org.seleniumhq.selenium - selenium-firefox-driver - 2.53.1 - test - - - commons-logging - commons-logging - 1.2 - test - - ${project.basedir}/src/test/java/ut com.atlassian.maven.plugins @@ -151,64 +128,55 @@ bitbucket bitbucket - ${bitbucket.version} - ${bitbucket.data.version} + ${bitbucket.test.version} + ${bitbucket.test.version} + + + bitbucket-integration + + it/**/*.java + + + bitbucket + + + org.apache.maven.plugins maven-compiler-plugin - 3.3 + 3.7.0 1.8 1.8 - org.codehaus.mojo - build-helper-maven-plugin - 3.0.0 + org.apache.maven.plugins + maven-surefire-plugin + 2.20.1 + + + it/**/*.java + + - - - integration - - false - - - - - org.codehaus.mojo - build-helper-maven-plugin - - - generate-sources - - add-test-source - - - - ${project.basedir}/src/test/java/it - - - - - - - - - - - 5.0.1 - 5.0.1 - 6.3.0 + 4.8.6 + ${bitbucket.version} + + ${bitbucket.version} + 6.3.14 1.2.3 UTF-8 + + true diff --git a/src/main/java/com/monitorjbl/plugins/AsyncProcessor.java b/src/main/java/com/monitorjbl/plugins/AsyncProcessor.java index b88e1e1..7a09530 100644 --- a/src/main/java/com/monitorjbl/plugins/AsyncProcessor.java +++ b/src/main/java/com/monitorjbl/plugins/AsyncProcessor.java @@ -1,32 +1,48 @@ package com.monitorjbl.plugins; -import com.atlassian.bitbucket.concurrent.BucketProcessor; +import com.atlassian.bitbucket.util.Version; +import com.atlassian.bitbucket.util.concurrent.ExecutorUtils; +import com.atlassian.sal.api.ApplicationProperties; +import com.atlassian.sal.api.lifecycle.LifecycleAware; +import com.atlassian.util.concurrent.ThreadFactories; +import org.slf4j.LoggerFactory; -import javax.annotation.Nonnull; -import java.io.Serializable; -import java.util.List; import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; -public class AsyncProcessor { - private static final String PREFIX = "com.monitorjbl.plugins:pr-harmony:"; - private final ExecutorService concurrencyService; +public class AsyncProcessor implements LifecycleAware { - public AsyncProcessor(ExecutorService concurrencyService) { - this.concurrencyService = concurrencyService; - } + private static final Version VERSION_5_9 = new Version(5, 9); - @SuppressWarnings("unchecked") - public void dispatch(Runnable taskRequest) { - concurrencyService.submit(taskRequest); - } + private final ExecutorService executorService; + private final boolean ownsExecutor; - public static abstract class TaskProcessor implements BucketProcessor { - @Override - public void process(@Nonnull String s, @Nonnull List list) { - list.forEach(this::handleTask); + public AsyncProcessor(ApplicationProperties applicationProperties, ExecutorService executorService) { + ownsExecutor = new Version(applicationProperties.getVersion()).compareTo(VERSION_5_9) < 0; + if (ownsExecutor) { + //For Bitbucket Server versions before 5.9, use our own ExecutorService to avoid starving out hook callbacks + this.executorService = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors(), + ThreadFactories.namedThreadFactory("pr-harmony", ThreadFactories.Type.DAEMON)); + } else { + //Bitbucket Server 5.9 includes a fix for BSERV-10652, which moves hook callback to their own threadpool, so + //for 5.9+ use the shared executor + this.executorService = executorService; } + } - public abstract void handleTask(T task); + public void dispatch(Runnable taskRequest) { + executorService.execute(taskRequest); } + @Override + public void onStart() { + //No-op + } + + @Override + public void onStop() { + if (ownsExecutor) { + ExecutorUtils.shutdown(executorService, LoggerFactory.getLogger(getClass())); + } + } } diff --git a/src/main/java/com/monitorjbl/plugins/PullRequestListener.java b/src/main/java/com/monitorjbl/plugins/PullRequestListener.java index 33a0dc9..0443ff5 100644 --- a/src/main/java/com/monitorjbl/plugins/PullRequestListener.java +++ b/src/main/java/com/monitorjbl/plugins/PullRequestListener.java @@ -20,9 +20,6 @@ import java.util.concurrent.atomic.AtomicBoolean; public class PullRequestListener { - private static final String PR_APPROVE_BUCKET = "AUTOMERGE_PR_APPROVAL"; - private static final String BUILD_APPROVE_BUCKET = "AUTOMERGE_BUILD_APPROVAL"; - public static final int MAX_COMMITS = 1048576; public static final int SEARCH_PAGE_SIZE = 50; private final AsyncProcessor asyncProcessor; diff --git a/src/main/java/com/monitorjbl/plugins/UserUtils.java b/src/main/java/com/monitorjbl/plugins/UserUtils.java index 103ba90..3d19b8c 100644 --- a/src/main/java/com/monitorjbl/plugins/UserUtils.java +++ b/src/main/java/com/monitorjbl/plugins/UserUtils.java @@ -2,14 +2,18 @@ import com.atlassian.bitbucket.user.ApplicationUser; import com.atlassian.bitbucket.user.UserService; -import com.atlassian.bitbucket.util.PageRequestImpl; +import com.atlassian.bitbucket.util.PagedIterable; import com.monitorjbl.plugins.config.User; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; import java.util.List; -import java.util.Optional; -import java.util.stream.Collectors; +import java.util.Set; -import static com.google.common.collect.Lists.newArrayList; +import static com.atlassian.bitbucket.util.MoreStreams.streamIterable; +import static java.util.Optional.ofNullable; +import static java.util.stream.Collectors.toList; public class UserUtils { private static final int RESULTS_PER_REQUEST = 25; @@ -19,44 +23,44 @@ public UserUtils(UserService userService) { this.userService = userService; } - public List dereferenceUsers(Iterable users) { - List list = newArrayList(); - for(String u : users) { - Optional user = getUserByName(u); - if(user.isPresent()) { - list.add(user.get()); - } + public List dereferenceUsers(Collection usernames) { + Set distinctNames = new HashSet<>(usernames); + if (distinctNames.isEmpty()) { + return Collections.emptyList(); + } + + Set usersByName = userService.getUsersByName(distinctNames); + if (usersByName.isEmpty()) { + return Collections.emptyList(); } - return list; - } - public Optional getUserByName(String username) { - ApplicationUser user = userService.getUserByName(username); - return user == null ? Optional.empty() : Optional.of(new User(user.getName(), user.getDisplayName())); + return usersByName.stream() + .map(user -> new User(user.getName(), user.getDisplayName())) + .collect(toList()); } public String getUserDisplayNameByName(String username) { - ApplicationUser user = userService.getUserByName(username); - return user.getDisplayName() == null ? username : user.getDisplayName(); + return ofNullable(userService.getUserByName(username)) + .map(ApplicationUser::getDisplayName) + .orElse(username); } public ApplicationUser getApplicationUserByName(String username) { return userService.getUserByName(username); } - public List dereferenceGroups(List groups) { - List users = newArrayList(); - for(String group : groups) { - List results = newArrayList(userService.findUsersByGroup(group, new PageRequestImpl(0, RESULTS_PER_REQUEST)).getValues()); - for(int i = 1; results.size() > 0; i++) { - users.addAll(results.stream() - .map(ApplicationUser::getSlug) - .collect(Collectors.toList())); - results = newArrayList(userService.findUsersByGroup(group, new PageRequestImpl(i * RESULTS_PER_REQUEST, RESULTS_PER_REQUEST)).getValues()); - } - } - return users; + public List dereferenceGroups(Collection groups) { + return groups.stream() + //Replace each group with a stream of the users in that group + .flatMap(group -> + streamIterable(new PagedIterable<>( + pageRequest -> userService.findUsersByGroup(group, pageRequest), + RESULTS_PER_REQUEST))) + //Transform each user to its slug + .map(ApplicationUser::getSlug) + //Drop any duplicates for users who exist in multiple groups + .distinct() + //Return a list of the distinct slugs + .collect(toList()); } - - } diff --git a/src/main/java/com/monitorjbl/plugins/config/UserResource.java b/src/main/java/com/monitorjbl/plugins/config/UserResource.java index a38bb47..14cdaf7 100644 --- a/src/main/java/com/monitorjbl/plugins/config/UserResource.java +++ b/src/main/java/com/monitorjbl/plugins/config/UserResource.java @@ -12,7 +12,6 @@ import static com.google.common.collect.Iterables.concat; import static com.google.common.collect.Lists.newArrayList; -import static com.google.common.collect.Sets.newHashSet; @Path("/users/{projectKey}/{repoSlug}") public class UserResource { @@ -31,12 +30,12 @@ public Response get(@PathParam("projectKey") String projectKey, @PathParam("repo return Response.ok(ImmutableMap.of( "requiredReviews", config.getRequiredReviews() == null ? "" : config.getRequiredReviews(), "blockMergeIfPrNeedsWork", config.getBlockMergeIfPrNeedsWork() == null ? "" : config.getBlockMergeIfPrNeedsWork(), - "requiredReviewers", utils.dereferenceUsers(newArrayList(newHashSet(concat( - utils.dereferenceGroups(config.getRequiredReviewerGroups()), - config.getRequiredReviewers())))), - "defaultReviewers", utils.dereferenceUsers(newArrayList(newHashSet(concat( - utils.dereferenceGroups(config.getDefaultReviewerGroups()), - config.getDefaultReviewers())))) + "requiredReviewers", newArrayList(concat( + utils.dereferenceUsers(config.getRequiredReviewers()), + utils.dereferenceGroups(config.getRequiredReviewerGroups()))), + "defaultReviewers", newArrayList(concat( + utils.dereferenceUsers(config.getDefaultReviewers()), + utils.dereferenceGroups(config.getDefaultReviewerGroups()))) )).build(); } } diff --git a/src/main/resources/atlassian-plugin.xml b/src/main/resources/atlassian-plugin.xml index 53ace26..41e7b43 100644 --- a/src/main/resources/atlassian-plugin.xml +++ b/src/main/resources/atlassian-plugin.xml @@ -25,7 +25,10 @@ - + + + com.atlassian.sal.api.lifecycle.LifecycleAware + diff --git a/src/test/java/com/monitorjbl/plugins/AsyncProcessorTest.java b/src/test/java/com/monitorjbl/plugins/AsyncProcessorTest.java new file mode 100644 index 0000000..12745f9 --- /dev/null +++ b/src/test/java/com/monitorjbl/plugins/AsyncProcessorTest.java @@ -0,0 +1,53 @@ +package com.monitorjbl.plugins; + +import com.atlassian.sal.api.ApplicationProperties; +import com.google.common.util.concurrent.MoreExecutors; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Spy; +import org.mockito.runners.MockitoJUnitRunner; + +import java.util.concurrent.ExecutorService; + +import static org.mockito.Mockito.same; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.when; + +@RunWith(MockitoJUnitRunner.class) +public class AsyncProcessorTest { + + @Mock + private ApplicationProperties applicationProperties; + @Spy + private ExecutorService executorService = MoreExecutors.newDirectExecutorService(); + @Mock + private Runnable task; + + @Test + public void testDispatchFor5_8() { + AsyncProcessor processor = create("5.8.1"); + processor.dispatch(task); + processor.onStop(); + + verify(task).run(); + verifyZeroInteractions(executorService); + } + + @Test + public void testDispatchFor5_9() { + AsyncProcessor processor = create("5.9.0"); + processor.dispatch(task); + processor.onStop(); + + verify(executorService).execute(same(task)); + verify(task).run(); + } + + private AsyncProcessor create(String version) { + when(applicationProperties.getVersion()).thenReturn(version); + + return new AsyncProcessor(applicationProperties, executorService); + } +} diff --git a/src/test/java/ut/com/monitorjbl/plugins/CommitBlockerHookTest.java b/src/test/java/com/monitorjbl/plugins/CommitBlockerHookTest.java similarity index 100% rename from src/test/java/ut/com/monitorjbl/plugins/CommitBlockerHookTest.java rename to src/test/java/com/monitorjbl/plugins/CommitBlockerHookTest.java diff --git a/src/test/java/ut/com/monitorjbl/plugins/MergeBlockerTest.java b/src/test/java/com/monitorjbl/plugins/MergeBlockerTest.java similarity index 100% rename from src/test/java/ut/com/monitorjbl/plugins/MergeBlockerTest.java rename to src/test/java/com/monitorjbl/plugins/MergeBlockerTest.java diff --git a/src/test/java/ut/com/monitorjbl/plugins/PullRequestApprovalTest.java b/src/test/java/com/monitorjbl/plugins/PullRequestApprovalTest.java similarity index 100% rename from src/test/java/ut/com/monitorjbl/plugins/PullRequestApprovalTest.java rename to src/test/java/com/monitorjbl/plugins/PullRequestApprovalTest.java diff --git a/src/test/java/ut/com/monitorjbl/plugins/PullRequestListenerTest.java b/src/test/java/com/monitorjbl/plugins/PullRequestListenerTest.java similarity index 100% rename from src/test/java/ut/com/monitorjbl/plugins/PullRequestListenerTest.java rename to src/test/java/com/monitorjbl/plugins/PullRequestListenerTest.java diff --git a/src/test/java/ut/com/monitorjbl/plugins/RegexUtilsTest.java b/src/test/java/com/monitorjbl/plugins/RegexUtilsTest.java similarity index 100% rename from src/test/java/ut/com/monitorjbl/plugins/RegexUtilsTest.java rename to src/test/java/com/monitorjbl/plugins/RegexUtilsTest.java diff --git a/src/test/java/ut/com/monitorjbl/plugins/TestUtils.java b/src/test/java/com/monitorjbl/plugins/TestUtils.java similarity index 100% rename from src/test/java/ut/com/monitorjbl/plugins/TestUtils.java rename to src/test/java/com/monitorjbl/plugins/TestUtils.java diff --git a/src/test/java/ut/com/monitorjbl/plugins/UserUtilsTest.java b/src/test/java/com/monitorjbl/plugins/UserUtilsTest.java similarity index 100% rename from src/test/java/ut/com/monitorjbl/plugins/UserUtilsTest.java rename to src/test/java/com/monitorjbl/plugins/UserUtilsTest.java diff --git a/src/test/java/ut/com/monitorjbl/plugins/config/ConfigDaoTest.java b/src/test/java/com/monitorjbl/plugins/config/ConfigDaoTest.java similarity index 100% rename from src/test/java/ut/com/monitorjbl/plugins/config/ConfigDaoTest.java rename to src/test/java/com/monitorjbl/plugins/config/ConfigDaoTest.java diff --git a/src/test/java/ut/com/monitorjbl/plugins/config/ConfigServletTest.java b/src/test/java/com/monitorjbl/plugins/config/ConfigServletTest.java similarity index 100% rename from src/test/java/ut/com/monitorjbl/plugins/config/ConfigServletTest.java rename to src/test/java/com/monitorjbl/plugins/config/ConfigServletTest.java diff --git a/src/test/java/it/com/monitorjbl/plugins/ui/.gitignore b/src/test/java/it/com/monitorjbl/plugins/ui/.gitignore deleted file mode 100644 index e69de29..0000000 diff --git a/src/test/java/it/com/monitorjbl/plugins/ui/UserResourceTest.java b/src/test/java/it/com/monitorjbl/plugins/ui/UserResourceTest.java new file mode 100644 index 0000000..5d23404 --- /dev/null +++ b/src/test/java/it/com/monitorjbl/plugins/ui/UserResourceTest.java @@ -0,0 +1,31 @@ +package it.com.monitorjbl.plugins.ui; + +import com.atlassian.bitbucket.test.BaseFuncTest; +import com.jayway.restassured.RestAssured; +import org.junit.Test; + +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminPassword; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminUser; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1Repository1; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getRestURL; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.isEmptyString; + +//At the moment this test doesn't cover any significant. The biggest benefit it offers is that, by calling the +//UserResource, it verifies that the plugin has been installed and started successfully +public class UserResourceTest extends BaseFuncTest { + + @Test + public void testGetWithoutConfig() { + RestAssured.given() + .auth().preemptive().basic(getAdminUser(), getAdminPassword()) + .expect() + .body("requiredReviews", isEmptyString()) + .body("blockMergeIfPrNeedsWork", isEmptyString()) + .body("requiredReviewers", empty()) + .body("defaultReviewers", empty()) + .when() + .get(getRestURL("pr-harmony", "1.0") + "/users/" + getProject1() + "/" + getProject1Repository1()); + } +} diff --git a/start-test-server.sh b/start-test-server.sh index 9e58d00..b1b5204 100755 --- a/start-test-server.sh +++ b/start-test-server.sh @@ -1,3 +1,3 @@ #!/bin/bash -atlas-run -u 6.3.0 --jvmargs "-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005" +atlas-run -u 6.3.14 --jvmargs "-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005" From d72dc58161b78f53384cd8353be813939cbd49f8 Mon Sep 17 00:00:00 2001 From: Bryan Turner Date: Wed, 7 Mar 2018 16:52:19 -0800 Subject: [PATCH 2/2] Rework for pull request #72. - Added more integration tests - ConfigResource and UserResource are both fully tested - CommitBlockerHook is now tested using real pushes, in addition to its existing unit tests - Fixed deduplication for users in UserResource.get's response - Also fixed an unintentional change in the "shape" of the payload where group members would have been returned as simple strings instead of Users - Fixed an issue in CommitBlockerHook where checking for users excluded by groups was comparing usernames against slugs - UserUtils.dereferenceGroups returns _slugs_, not usernames - Also fixed the hook so it doesn't apply its rules to tags - Revised how the hook applies its checks to avoid dereferencing users and groups on every iteration of the loop, even if the ref being tested doesn't match any blocking pattern - Revised the rejection wording to include all blocked branches when a single push attempts to update more than one - Removed RegexUtils.formatBranchName and instead simplified callsites to use MinimalRef.getDisplayId, which already omits refs/heads/ - Updated ConfigServlet to use AuthenticationContext to retrieve the current user instead of the SAL UserManager - This simplifies applying permission checks later - Updated component-imports in atlassian-plugin.xml to ensure every component the add-on uses is explicitly imported --- .../monitorjbl/plugins/CommitBlockerHook.java | 80 ++++++--- .../com/monitorjbl/plugins/MergeBlocker.java | 28 ++-- .../plugins/PullRequestApproval.java | 7 +- .../plugins/PullRequestListener.java | 4 +- .../com/monitorjbl/plugins/RegexUtils.java | 6 - .../com/monitorjbl/plugins/UserUtils.java | 77 ++++++--- .../plugins/config/ConfigServlet.java | 56 +++---- .../plugins/config/UserResource.java | 11 +- src/main/resources/atlassian-plugin.xml | 20 +-- .../plugins/CommitBlockerHookTest.java | 152 ++++++++++-------- .../monitorjbl/plugins/MergeBlockerTest.java | 75 +++++---- .../plugins/PullRequestApprovalTest.java | 34 ++-- .../plugins/PullRequestListenerTest.java | 118 +++++--------- .../monitorjbl/plugins/RegexUtilsTest.java | 35 ++-- .../com/monitorjbl/plugins/UserUtilsTest.java | 26 +-- .../plugins/config/ConfigDaoTest.java | 60 +++---- .../plugins/config/ConfigServletTest.java | 62 ++++--- .../plugins/BaseIntegrationTest.java | 70 ++++++++ .../hosting/CommitBlockerHookTest.java | 84 ++++++++++ .../plugins/rest/ConfigResourceTest.java | 119 ++++++++++++++ .../plugins/rest/UserResourceTest.java | 101 ++++++++++++ .../plugins/ui/UserResourceTest.java | 31 ---- 22 files changed, 796 insertions(+), 460 deletions(-) create mode 100644 src/test/java/it/com/monitorjbl/plugins/BaseIntegrationTest.java create mode 100644 src/test/java/it/com/monitorjbl/plugins/hosting/CommitBlockerHookTest.java create mode 100644 src/test/java/it/com/monitorjbl/plugins/rest/ConfigResourceTest.java create mode 100644 src/test/java/it/com/monitorjbl/plugins/rest/UserResourceTest.java delete mode 100644 src/test/java/it/com/monitorjbl/plugins/ui/UserResourceTest.java diff --git a/src/main/java/com/monitorjbl/plugins/CommitBlockerHook.java b/src/main/java/com/monitorjbl/plugins/CommitBlockerHook.java index 133e97f..f92c6a5 100644 --- a/src/main/java/com/monitorjbl/plugins/CommitBlockerHook.java +++ b/src/main/java/com/monitorjbl/plugins/CommitBlockerHook.java @@ -1,54 +1,84 @@ package com.monitorjbl.plugins; -import com.atlassian.sal.api.user.UserManager; -import com.atlassian.sal.api.user.UserProfile; +import com.atlassian.bitbucket.auth.AuthenticationContext; import com.atlassian.bitbucket.hook.HookResponse; import com.atlassian.bitbucket.hook.PreReceiveHook; +import com.atlassian.bitbucket.repository.MinimalRef; import com.atlassian.bitbucket.repository.RefChange; import com.atlassian.bitbucket.repository.Repository; +import com.atlassian.bitbucket.repository.StandardRefType; +import com.atlassian.bitbucket.user.ApplicationUser; import com.monitorjbl.plugins.config.Config; import com.monitorjbl.plugins.config.ConfigDao; +import javax.annotation.Nonnull; import java.util.Collection; -import java.util.Set; +import java.util.List; -import static com.google.common.collect.Iterables.concat; -import static com.google.common.collect.Sets.newHashSet; +import static java.util.stream.Collectors.joining; +import static java.util.stream.Collectors.toList; public class CommitBlockerHook implements PreReceiveHook { + private final AuthenticationContext authenticationContext; private final ConfigDao configDao; - private final UserManager userManager; private final RegexUtils regexUtils; private final UserUtils userUtils; - public CommitBlockerHook(ConfigDao configDao, UserManager userManager, RegexUtils regexUtils, UserUtils userUtils) { + public CommitBlockerHook(AuthenticationContext authenticationContext, ConfigDao configDao, + RegexUtils regexUtils, UserUtils userUtils) { + this.authenticationContext = authenticationContext; this.configDao = configDao; - this.userManager = userManager; this.regexUtils = regexUtils; this.userUtils = userUtils; } @Override - public boolean onReceive(Repository repository, Collection collection, HookResponse hookResponse) { + public boolean onReceive(@Nonnull Repository repository, @Nonnull Collection collection, + @Nonnull HookResponse hookResponse) { Config config = configDao.getConfigForRepo(repository.getProject().getKey(), repository.getSlug()); + List blockedCommits = config.getBlockedCommits(); + if (blockedCommits.isEmpty()) { + //If no branches have been configured to block direct commits, allow the push + return true; + } - UserProfile user = userManager.getRemoteUser(); - for(RefChange ch : collection) { - String branch = regexUtils.formatBranchName(ch.getRef().getId()); - Set excluded = newHashSet(concat(config.getExcludedUsers(), userUtils.dereferenceGroups(config.getExcludedGroups()))); - if(regexUtils.match(config.getBlockedCommits(), branch) && !excluded.contains(user.getUsername())) { - hookResponse.err().write("\n" + - "******************************\n" + - "* !! Commit Rejected !! *\n" + - "******************************\n\n" + - "Direct commits are not allowed\n" + - "to branch [" + branch + "].\n\n" - ); - return false; - } + List restrictedBranches = collection.stream() + .map(RefChange::getRef) + .filter(ref -> StandardRefType.TAG != ref.getType()) //Only consider branches + .map(MinimalRef::getDisplayId) //Use branch name, not ID (no refs/heads/ prefix) + .filter(branchName -> regexUtils.match(blockedCommits, branchName)) + .collect(toList()); + if (restrictedBranches.isEmpty()) { + //If none of the branches being pushed to is configured to block direct commits, allow the push + return true; + } + + //Otherwise, if the user is pushing to at least one branch which does not allow direct commits, check + //whether the they've been excluded from the restriction + ApplicationUser user = authenticationContext.getCurrentUser(); + if (user != null && isExcluded(config, user)) { + //If they are, allow the push + return true; } - return true; - } + //If they're not, list the branch(es) the user isn't allowed to push to directly and block the push + hookResponse.err().write("\n" + + "******************************\n" + + "* !! Push Rejected !! *\n" + + "******************************\n\n"); + if (restrictedBranches.size() == 1) { + hookResponse.err().write("Direct pushes are not allowed for [" + restrictedBranches.get(0) + "]"); + } else { + hookResponse.err().write("The following branches do not allow direct pushes:\n\t" + + restrictedBranches.stream().collect(joining("\n\t"))); + } + hookResponse.err().write("\n\n"); + + return false; + } + private boolean isExcluded(Config config, ApplicationUser user) { + return config.getExcludedUsers().contains(user.getName()) || + userUtils.dereferenceGroups(config.getExcludedGroups()).contains(user.getSlug()); + } } diff --git a/src/main/java/com/monitorjbl/plugins/MergeBlocker.java b/src/main/java/com/monitorjbl/plugins/MergeBlocker.java index 332867b..3bed83c 100644 --- a/src/main/java/com/monitorjbl/plugins/MergeBlocker.java +++ b/src/main/java/com/monitorjbl/plugins/MergeBlocker.java @@ -12,31 +12,32 @@ import javax.annotation.Nonnull; import java.util.Set; +import java.util.function.Predicate; public class MergeBlocker implements MergeRequestCheck { private final ConfigDao configDao; - private final UserUtils userUtils; private final RegexUtils regexUtils; + private final UserUtils userUtils; - public MergeBlocker(ConfigDao configDao, UserUtils userUtils, RegexUtils regexUtils) { + public MergeBlocker(ConfigDao configDao, RegexUtils regexUtils, UserUtils userUtils) { this.configDao = configDao; - this.userUtils = userUtils; this.regexUtils = regexUtils; + this.userUtils = userUtils; } @Override public void check(@Nonnull MergeRequest mergeRequest) { PullRequest pr = mergeRequest.getPullRequest(); Repository repo = pr.getToRef().getRepository(); - final Config config = configDao.getConfigForRepo(repo.getProject().getKey(), repo.getSlug()); + Config config = configDao.getConfigForRepo(repo.getProject().getKey(), repo.getSlug()); - String branch = regexUtils.formatBranchName(pr.getToRef().getId()); + String branch = pr.getToRef().getDisplayId(); if (regexUtils.match(config.getBlockedPRs(), branch)) { mergeRequest.veto("Pull Request Blocked", "Pull requests have been disabled for branch [" + branch + "]"); } else { PullRequestApproval approval = new PullRequestApproval(config, userUtils); if (!approval.isPullRequestApproved(pr)) { - Set missing = approval.missingRevieiwersNames(pr); + Set missing = approval.missingReviewersNames(pr); mergeRequest.veto("Required reviewers must approve", (config.getRequiredReviews() - approval.seenReviewers(pr).size()) + " more approvals required from the following users: " + Joiner.on(", ").join(missing)); } else { @@ -50,16 +51,9 @@ public void check(@Nonnull MergeRequest mergeRequest) { } } - private boolean needsWork(final PullRequest pr) { - boolean needsWork = false; - - for (PullRequestParticipant reviewer : pr.getReviewers()) { - if (reviewer.getStatus() == PullRequestParticipantStatus.NEEDS_WORK) { - needsWork = true; - break; - } - } - - return needsWork; + private boolean needsWork(PullRequest pr) { + return pr.getReviewers().stream() + .map(PullRequestParticipant::getStatus) + .anyMatch(Predicate.isEqual(PullRequestParticipantStatus.NEEDS_WORK)); } } diff --git a/src/main/java/com/monitorjbl/plugins/PullRequestApproval.java b/src/main/java/com/monitorjbl/plugins/PullRequestApproval.java index 1fce967..cacee66 100644 --- a/src/main/java/com/monitorjbl/plugins/PullRequestApproval.java +++ b/src/main/java/com/monitorjbl/plugins/PullRequestApproval.java @@ -2,7 +2,6 @@ import com.atlassian.bitbucket.pull.PullRequest; import com.atlassian.bitbucket.pull.PullRequestParticipant; -import com.google.common.base.Function; import com.monitorjbl.plugins.config.Config; import java.util.Map; @@ -29,7 +28,7 @@ public boolean isPullRequestApproved(PullRequest pr) { return requiredReviews == null || seenReviewers(pr).size() >= requiredReviews; } - public Set missingRevieiwers(PullRequest pr) { + public Set missingReviewers(PullRequest pr) { Map map = transformReviewers(pr); Set missingReviewers = newHashSet(); @@ -41,7 +40,7 @@ public Set missingRevieiwers(PullRequest pr) { return missingReviewers; } - public Set missingRevieiwersNames(PullRequest pr) { + public Set missingReviewersNames(PullRequest pr) { Map map = transformReviewers(pr); Set missingReviewers = newHashSet(); @@ -55,7 +54,7 @@ public Set missingRevieiwersNames(PullRequest pr) { public Set seenReviewers(PullRequest pr) { Set required = newHashSet(concat(config.getRequiredReviewers(), utils.dereferenceGroups(config.getRequiredReviewerGroups()))); - return difference(required, missingRevieiwers(pr)); + return difference(required, missingReviewers(pr)); } Map transformReviewers(PullRequest pr) { diff --git a/src/main/java/com/monitorjbl/plugins/PullRequestListener.java b/src/main/java/com/monitorjbl/plugins/PullRequestListener.java index 0443ff5..5b89802 100644 --- a/src/main/java/com/monitorjbl/plugins/PullRequestListener.java +++ b/src/main/java/com/monitorjbl/plugins/PullRequestListener.java @@ -50,8 +50,8 @@ public void buildStatusListener(BuildStatusSetEvent event) { void automergePullRequest(PullRequest pr) { Repository repo = pr.getToRef().getRepository(); Config config = configDao.getConfigForRepo(repo.getProject().getKey(), repo.getSlug()); - String toBranch = regexUtils.formatBranchName(pr.getToRef().getId()); - String fromBranch = regexUtils.formatBranchName(pr.getFromRef().getId()); + String toBranch = pr.getToRef().getDisplayId(); + String fromBranch = pr.getFromRef().getDisplayId(); if((regexUtils.match(config.getAutomergePRs(), toBranch) || regexUtils.match(config.getAutomergePRsFrom(), fromBranch)) && !regexUtils.match(config.getBlockedPRs(), toBranch) && prService.canMerge(repo.getId(), pr.getId()).canMerge()) { diff --git a/src/main/java/com/monitorjbl/plugins/RegexUtils.java b/src/main/java/com/monitorjbl/plugins/RegexUtils.java index f1786eb..d16f3de 100644 --- a/src/main/java/com/monitorjbl/plugins/RegexUtils.java +++ b/src/main/java/com/monitorjbl/plugins/RegexUtils.java @@ -4,8 +4,6 @@ import java.util.regex.Pattern; public class RegexUtils { - public static final String REFS_PREFIX = "refs/heads/"; - public boolean match(List regexes, String value) { for (String regex : regexes) { regex = regex.replaceAll("\\.","\\\\.").replaceAll("\\*",".*"); @@ -16,8 +14,4 @@ public boolean match(List regexes, String value) { } return false; } - - public String formatBranchName(String refspec) { - return refspec.replace(REFS_PREFIX, ""); - } } diff --git a/src/main/java/com/monitorjbl/plugins/UserUtils.java b/src/main/java/com/monitorjbl/plugins/UserUtils.java index 3d19b8c..e69d501 100644 --- a/src/main/java/com/monitorjbl/plugins/UserUtils.java +++ b/src/main/java/com/monitorjbl/plugins/UserUtils.java @@ -3,6 +3,7 @@ import com.atlassian.bitbucket.user.ApplicationUser; import com.atlassian.bitbucket.user.UserService; import com.atlassian.bitbucket.util.PagedIterable; +import com.google.common.collect.Iterables; import com.monitorjbl.plugins.config.User; import java.util.Collection; @@ -10,33 +11,41 @@ import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.stream.Stream; import static com.atlassian.bitbucket.util.MoreStreams.streamIterable; import static java.util.Optional.ofNullable; import static java.util.stream.Collectors.toList; +import static java.util.stream.Collectors.toSet; public class UserUtils { - private static final int RESULTS_PER_REQUEST = 25; + private static final int RESULTS_PER_REQUEST = 50; + private final UserService userService; public UserUtils(UserService userService) { this.userService = userService; } - public List dereferenceUsers(Collection usernames) { - Set distinctNames = new HashSet<>(usernames); - if (distinctNames.isEmpty()) { - return Collections.emptyList(); - } - - Set usersByName = userService.getUsersByName(distinctNames); - if (usersByName.isEmpty()) { + /** + * Returns a list of the distinct users across the provided set of usernames and members from the specified groups. + * If a given user is present in multiple groups, or is included by both a group and name, they will only appear + * once in the returned list. The ordering of the returned list is not guaranteed. + * + * @param usernames a collection containing zero or more usernames + * @param groupNames a collection containing zero or more group names + * @return a list of the unique users between all of the provided usernames and groups + */ + public List dereferenceUsers(Collection usernames, Collection groupNames) { + if (usernames.isEmpty() && groupNames.isEmpty()) { return Collections.emptyList(); } - return usersByName.stream() - .map(user -> new User(user.getName(), user.getDisplayName())) - .collect(toList()); + Set slugs = new HashSet<>(); + return Stream.concat(streamUsers(usernames), streamGroupMembers(groupNames)) + .filter(user -> slugs.add(user.getSlug())) //Only add each user once + .map(user -> new User(user.getName(), user.getDisplayName())) + .collect(toList()); } public String getUserDisplayNameByName(String username) { @@ -44,23 +53,43 @@ public String getUserDisplayNameByName(String username) { .map(ApplicationUser::getDisplayName) .orElse(username); } - - public ApplicationUser getApplicationUserByName(String username) { - return userService.getUserByName(username); + + /** + * Returns a set of {@link ApplicationUser#getSlug slugs} for all of the users in the specified groups. + * + * @param groups a collection containing one or more group names + * @return a set of {@link ApplicationUser#getSlug user slugs} for group members + */ + public Set dereferenceGroups(Collection groups) { + return streamGroupMembers(groups) + //Transform each user to its slug + .map(ApplicationUser::getSlug) + //Return a set of the distinct slugs + .collect(toSet()); } - public List dereferenceGroups(Collection groups) { + private Stream streamGroupMembers(Collection groups) { + if (groups.isEmpty()) { + return Stream.empty(); + } + //Replace each group with a stream of the users in that group return groups.stream() - //Replace each group with a stream of the users in that group .flatMap(group -> streamIterable(new PagedIterable<>( pageRequest -> userService.findUsersByGroup(group, pageRequest), - RESULTS_PER_REQUEST))) - //Transform each user to its slug - .map(ApplicationUser::getSlug) - //Drop any duplicates for users who exist in multiple groups - .distinct() - //Return a list of the distinct slugs - .collect(toList()); + RESULTS_PER_REQUEST))); + } + + private Stream streamUsers(Collection usernames) { + if (usernames.isEmpty()) { + return Stream.empty(); + } + //Look up users in batches of up to 50. For most configurations this will retrieve all of the users in a + //single lookup, and for configurations with a large number of users it will avoid loading a large number + //of ApplicationUser instances into memory + return streamIterable(Iterables.partition(usernames, RESULTS_PER_REQUEST)) + .map(HashSet::new) + .map(userService::getUsersByName) + .flatMap(Collection::stream); } } diff --git a/src/main/java/com/monitorjbl/plugins/config/ConfigServlet.java b/src/main/java/com/monitorjbl/plugins/config/ConfigServlet.java index 6e67bc4..dc08906 100644 --- a/src/main/java/com/monitorjbl/plugins/config/ConfigServlet.java +++ b/src/main/java/com/monitorjbl/plugins/config/ConfigServlet.java @@ -1,5 +1,6 @@ package com.monitorjbl.plugins.config; +import com.atlassian.bitbucket.auth.AuthenticationContext; import com.atlassian.bitbucket.permission.Permission; import com.atlassian.bitbucket.permission.PermissionService; import com.atlassian.bitbucket.project.Project; @@ -8,15 +9,11 @@ import com.atlassian.bitbucket.repository.RepositoryService; import com.atlassian.bitbucket.user.ApplicationUser; import com.atlassian.sal.api.auth.LoginUriProvider; -import com.atlassian.sal.api.user.UserManager; -import com.atlassian.sal.api.user.UserProfile; import com.atlassian.templaterenderer.TemplateRenderer; import com.google.common.collect.ImmutableMap; -import com.monitorjbl.plugins.UserUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -25,21 +22,20 @@ public class ConfigServlet extends HttpServlet { public static final String SERVLET_PATH = "/plugins/servlet/pr-harmony/"; + private static final Logger logger = LoggerFactory.getLogger(ConfigServlet.class); - private final UserManager userManager; - private final UserUtils userUtils; + private final AuthenticationContext authenticationContext; private final RepositoryService repoService; private final ProjectService projectService; private final TemplateRenderer renderer; private final PermissionService permissionService; private final LoginUriProvider loginUriProvider; - public ConfigServlet(UserManager userManager, UserUtils userUtils, RepositoryService repoService, - ProjectService projectService, TemplateRenderer renderer, PermissionService permissionService, - LoginUriProvider loginUriProvider) { - this.userManager = userManager; - this.userUtils = userUtils; + public ConfigServlet(AuthenticationContext authenticationContext, RepositoryService repoService, + ProjectService projectService, TemplateRenderer renderer, + PermissionService permissionService, LoginUriProvider loginUriProvider) { + this.authenticationContext = authenticationContext; this.repoService = repoService; this.projectService = projectService; this.renderer = renderer; @@ -48,66 +44,64 @@ public ConfigServlet(UserManager userManager, UserUtils userUtils, RepositorySer } @Override - public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { - UserProfile user = userManager.getRemoteUser(); + public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException { + ApplicationUser user = authenticationContext.getCurrentUser(); if(user == null) { response.sendRedirect(loginUriProvider.getLoginUri(getUri(request)).toASCIIString()); } else { - handleRequest(request.getRequestURI(), user.getUsername(), response); + handleRequest(request.getRequestURI(), user, response); } } - void handleRequest(String requestPath, String username, HttpServletResponse response) throws IOException { + void handleRequest(String requestPath, ApplicationUser user, HttpServletResponse response) throws IOException { String[] coords = requestPath.replaceAll(".*" + SERVLET_PATH, "").split("/"); if(coords.length == 1) { - renderProjectSettings(coords[0], username, response); + renderProjectSettings(coords[0], user, response); } else if(coords.length == 2) { - renderRepoSettings(coords[0], coords[1], username, response); + renderRepoSettings(coords[0], coords[1], user, response); } else { logger.warn("Malformed request path, expecting {}{projectKey}/{repoSlug}", SERVLET_PATH); response.setStatus(HttpServletResponse.SC_BAD_REQUEST); } } - void renderRepoSettings(String projectKey, String repoSlug, String username, HttpServletResponse response) throws IOException { + void renderRepoSettings(String projectKey, String repoSlug, ApplicationUser user, HttpServletResponse response) throws IOException { Repository repo = repoService.getBySlug(projectKey, repoSlug); if(repo == null) { - logger.warn("Project/Repo [{}/{}] not found for user {}", projectKey, repoSlug, username); + logger.warn("Project/Repo [{}/{}] not found for user {}", projectKey, repoSlug, user.getName()); response.setStatus(HttpServletResponse.SC_NOT_FOUND); return; } - ApplicationUser appUser = userUtils.getApplicationUserByName(username); - if(permissionService.hasRepositoryPermission(appUser, repo, Permission.REPO_ADMIN)) { + if(permissionService.hasRepositoryPermission(user, repo, Permission.REPO_ADMIN)) { response.setStatus(HttpServletResponse.SC_OK); response.setContentType("text/html;charset=utf-8"); - renderer.render("repo-config.html", ImmutableMap.of( + renderer.render("repo-config.html", ImmutableMap.of( "projectKey", projectKey, "repositorySlug", repoSlug - ), response.getWriter()); + ), response.getWriter()); } else { - logger.debug("Permission denied for user [{}]", username); + logger.debug("Permission denied for user [{}]", user.getName()); response.setStatus(HttpServletResponse.SC_FORBIDDEN); } } - void renderProjectSettings(String projectKey, String username, HttpServletResponse response) throws IOException { + void renderProjectSettings(String projectKey, ApplicationUser user, HttpServletResponse response) throws IOException { Project project = projectService.getByKey(projectKey); if(project == null) { - logger.warn("Project [{}] not found for user {}", projectKey, username); + logger.warn("Project [{}] not found for user {}", projectKey, user.getName()); response.setStatus(HttpServletResponse.SC_NOT_FOUND); return; } - ApplicationUser appUser = userUtils.getApplicationUserByName(username); - if(permissionService.hasProjectPermission(appUser, project, Permission.PROJECT_ADMIN)) { + if(permissionService.hasProjectPermission(user, project, Permission.PROJECT_ADMIN)) { response.setStatus(HttpServletResponse.SC_OK); response.setContentType("text/html;charset=utf-8"); - renderer.render("project-config.html", ImmutableMap.of( + renderer.render("project-config.html", ImmutableMap.of( "projectKey", projectKey - ), response.getWriter()); + ), response.getWriter()); } else { - logger.debug("Permission denied for user [{}]", username); + logger.debug("Permission denied for user [{}]", user.getName()); response.setStatus(HttpServletResponse.SC_FORBIDDEN); } } diff --git a/src/main/java/com/monitorjbl/plugins/config/UserResource.java b/src/main/java/com/monitorjbl/plugins/config/UserResource.java index 14cdaf7..dffc77e 100644 --- a/src/main/java/com/monitorjbl/plugins/config/UserResource.java +++ b/src/main/java/com/monitorjbl/plugins/config/UserResource.java @@ -10,9 +10,6 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; -import static com.google.common.collect.Iterables.concat; -import static com.google.common.collect.Lists.newArrayList; - @Path("/users/{projectKey}/{repoSlug}") public class UserResource { private final ConfigDao configDao; @@ -30,12 +27,8 @@ public Response get(@PathParam("projectKey") String projectKey, @PathParam("repo return Response.ok(ImmutableMap.of( "requiredReviews", config.getRequiredReviews() == null ? "" : config.getRequiredReviews(), "blockMergeIfPrNeedsWork", config.getBlockMergeIfPrNeedsWork() == null ? "" : config.getBlockMergeIfPrNeedsWork(), - "requiredReviewers", newArrayList(concat( - utils.dereferenceUsers(config.getRequiredReviewers()), - utils.dereferenceGroups(config.getRequiredReviewerGroups()))), - "defaultReviewers", newArrayList(concat( - utils.dereferenceUsers(config.getDefaultReviewers()), - utils.dereferenceGroups(config.getDefaultReviewerGroups()))) + "requiredReviewers", utils.dereferenceUsers(config.getRequiredReviewers(), config.getRequiredReviewerGroups()), + "defaultReviewers", utils.dereferenceUsers(config.getDefaultReviewers(), config.getDefaultReviewerGroups()) )).build(); } } diff --git a/src/main/resources/atlassian-plugin.xml b/src/main/resources/atlassian-plugin.xml index 41e7b43..514c469 100644 --- a/src/main/resources/atlassian-plugin.xml +++ b/src/main/resources/atlassian-plugin.xml @@ -8,17 +8,19 @@ true - - - - - - - - + + + + + + + + + + diff --git a/src/test/java/com/monitorjbl/plugins/CommitBlockerHookTest.java b/src/test/java/com/monitorjbl/plugins/CommitBlockerHookTest.java index 0455f2d..e1bea2f 100644 --- a/src/test/java/com/monitorjbl/plugins/CommitBlockerHookTest.java +++ b/src/test/java/com/monitorjbl/plugins/CommitBlockerHookTest.java @@ -1,118 +1,144 @@ package com.monitorjbl.plugins; +import com.atlassian.bitbucket.auth.AuthenticationContext; import com.atlassian.bitbucket.hook.HookResponse; import com.atlassian.bitbucket.project.Project; import com.atlassian.bitbucket.repository.MinimalRef; import com.atlassian.bitbucket.repository.RefChange; import com.atlassian.bitbucket.repository.Repository; -import com.atlassian.sal.api.user.UserManager; -import com.atlassian.sal.api.user.UserProfile; -import com.google.common.collect.Lists; +import com.atlassian.bitbucket.repository.StandardRefType; +import com.atlassian.bitbucket.user.ApplicationUser; import com.monitorjbl.plugins.config.Config; import com.monitorjbl.plugins.config.ConfigDao; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; +import org.mockito.Spy; +import org.mockito.runners.MockitoJUnitRunner; import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.Collections; import static com.google.common.collect.Lists.newArrayList; +import static org.hamcrest.CoreMatchers.allOf; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertThat; -import static org.mockito.Matchers.anyList; -import static org.mockito.Matchers.anyString; -import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Matchers.anyListOf; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; -@SuppressWarnings("unchecked") +@RunWith(MockitoJUnitRunner.class) public class CommitBlockerHookTest { + private final Config config = Config.builder().blockedCommits(Collections.singletonList("master")).build(); + private final StringWriter stderr = new StringWriter(); + @Mock - private ConfigDao configDao; - @Mock - private UserManager userManager; + private AuthenticationContext authenticationContext; @Mock + private ConfigDao configDao; + @InjectMocks + private CommitBlockerHook hook; + @Spy + @SuppressWarnings("unused") //Used via @InjectMocks private RegexUtils regexUtils; @Mock private UserUtils userUtils; - @InjectMocks - private CommitBlockerHook sut; @Mock - Repository repository; - @Mock - Project project; + private RefChange change; @Mock - RefChange change; + private HookResponse hookResponse; @Mock - MinimalRef ref; + private Project project; @Mock - HookResponse hookResponse; + private MinimalRef ref; @Mock - PrintWriter stderr; + private Repository repository; @Mock - UserProfile user; + private ApplicationUser user; @Before - public void before() throws Exception { - MockitoAnnotations.initMocks(this); - when(hookResponse.err()).thenReturn(stderr); + public void before() { + when(authenticationContext.getCurrentUser()).thenReturn(user); + when(change.getRef()).thenReturn(ref); + when(configDao.getConfigForRepo(eq("PRJ"), eq("repo_1"))).thenReturn(config); + when(hookResponse.err()).thenReturn(new PrintWriter(stderr)); + when(project.getKey()).thenReturn("PRJ"); + when(ref.getDisplayId()).thenReturn("master"); + when(ref.getType()).thenReturn(StandardRefType.BRANCH); when(repository.getProject()).thenReturn(project); when(repository.getSlug()).thenReturn("repo_1"); - when(project.getKey()).thenReturn("PRJ"); - when(userManager.getRemoteUser()).thenReturn(user); - when(user.getUsername()).thenReturn("user1"); - when(userUtils.dereferenceGroups(anyList())).thenReturn(Lists.newArrayList()); - when(regexUtils.match(anyList(), anyString())).thenCallRealMethod(); - when(regexUtils.formatBranchName(anyString())).thenCallRealMethod(); - when(change.getRef()).thenReturn(ref); + when(userUtils.dereferenceGroups(anyListOf(String.class))).thenReturn(Collections.emptySet()); + when(user.getName()).thenReturn("name"); + when(user.getSlug()).thenReturn("slug"); + } + + @Test + public void testCommit_blocked() { + assertThat(hook.onReceive(repository, newArrayList(change), hookResponse), is(false)); + + assertThat(stderr.toString(), allOf( + containsString("Push Rejected"), + containsString("Direct pushes are not allowed for [master]"))); } @Test - public void testCommit_blocked() throws Exception { - when(ref.getId()).thenReturn("master"); - when(configDao.getConfigForRepo(project.getKey(), repository.getSlug())).thenReturn(Config.builder() - .blockedCommits(newArrayList("master")) - .build()); - assertThat(sut.onReceive(repository, newArrayList(change), hookResponse), is(false)); - verify(stderr, atLeastOnce()).write(anyString()); + public void testCommit_blockedWithExcludedUser() { + config.setExcludedUsers(Collections.singletonList("name")); + + assertThat(hook.onReceive(repository, newArrayList(change), hookResponse), is(true)); + + verifyZeroInteractions(hookResponse); } @Test - public void testCommit_blockedWithExcludedUser() throws Exception { - when(ref.getId()).thenReturn(RegexUtils.REFS_PREFIX + "master"); - when(configDao.getConfigForRepo(project.getKey(), repository.getSlug())).thenReturn(Config.builder() - .blockedCommits(newArrayList("master")) - .excludedUsers(newArrayList("user1")) - .build()); - assertThat(sut.onReceive(repository, newArrayList(change), hookResponse), is(true)); - verify(stderr, never()).write(anyString()); + public void testCommit_blockedWithExcludedGroup() { + config.setExcludedGroups(Collections.singletonList("group")); + + when(userUtils.dereferenceGroups(newArrayList("group"))).thenReturn(Collections.singleton("slug")); + + assertThat(hook.onReceive(repository, newArrayList(change), hookResponse), is(true)); + + verifyZeroInteractions(hookResponse); } @Test - public void testCommit_blockedWithExcludedGroup() throws Exception { - when(ref.getId()).thenReturn(RegexUtils.REFS_PREFIX + "master"); - when(configDao.getConfigForRepo(project.getKey(), repository.getSlug())).thenReturn(Config.builder() - .blockedCommits(newArrayList("master")) - .excludedGroups(newArrayList("group1")) - .build()); - when(userUtils.dereferenceGroups(newArrayList("group1"))).thenReturn(newArrayList("user1")); - - assertThat(sut.onReceive(repository, newArrayList(change), hookResponse), is(true)); - verify(stderr, never()).write(anyString()); + public void testCommit_notBlockedForTags() { + reset(ref); + when(ref.getType()).thenReturn(StandardRefType.TAG); + + assertThat(hook.onReceive(repository, newArrayList(change), hookResponse), is(true)); + + //Tags should not be checked, so as soon as the hook determines the push was to a tag it should return + verify(ref, never()).getDisplayId(); + verify(ref, never()).getId(); + verifyZeroInteractions(authenticationContext, hookResponse, regexUtils, userUtils); } @Test - public void testCommit_notBlocked() throws Exception { - when(ref.getId()).thenReturn(RegexUtils.REFS_PREFIX + "master"); - when(configDao.getConfigForRepo(project.getKey(), repository.getSlug())).thenReturn(Config.builder() - .blockedCommits(newArrayList("bugfix")) - .build()); - assertThat(sut.onReceive(repository, newArrayList(change), hookResponse), is(true)); - verify(stderr, never()).write(anyString()); + public void testCommit_notBlockedNoMatch() { + config.setBlockedCommits(Collections.singletonList("bugfix")); + + assertThat(hook.onReceive(repository, newArrayList(change), hookResponse), is(true)); + + verifyZeroInteractions(hookResponse, userUtils); } + @Test + public void testCommit_notBlockedNotConfigured() { + config.setBlockedCommits(Collections.emptyList()); + + assertThat(hook.onReceive(repository, newArrayList(change), hookResponse), is(true)); + + //When no restricted branches are configured, the hook should essentially be a no-op + verifyZeroInteractions(authenticationContext, change, hookResponse, ref, regexUtils, userUtils); + } } diff --git a/src/test/java/com/monitorjbl/plugins/MergeBlockerTest.java b/src/test/java/com/monitorjbl/plugins/MergeBlockerTest.java index 69325b3..0e128de 100644 --- a/src/test/java/com/monitorjbl/plugins/MergeBlockerTest.java +++ b/src/test/java/com/monitorjbl/plugins/MergeBlockerTest.java @@ -7,88 +7,87 @@ import com.atlassian.bitbucket.pull.PullRequestRef; import com.atlassian.bitbucket.repository.Repository; import com.atlassian.bitbucket.scm.pull.MergeRequest; -import com.google.common.collect.Lists; import com.monitorjbl.plugins.config.Config; import com.monitorjbl.plugins.config.ConfigDao; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; -import org.mockito.MockitoAnnotations; +import org.mockito.Spy; +import org.mockito.runners.MockitoJUnitRunner; +import java.util.Collections; import java.util.Set; import static com.google.common.collect.Lists.newArrayList; import static com.google.common.collect.Sets.newHashSet; import static com.monitorjbl.plugins.TestUtils.mockParticipant; -import static org.mockito.Matchers.anyList; +import static org.mockito.Matchers.anyListOf; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +@RunWith(MockitoJUnitRunner.class) public class MergeBlockerTest { @Mock private ConfigDao configDao; + @InjectMocks + private MergeBlocker mergeBlocker; + @Spy + @SuppressWarnings("unused") //Used by @InjectMocks + private RegexUtils regexUtils; @Mock private UserUtils userUtils; - @Mock - private RegexUtils regexUtils; - @InjectMocks - private MergeBlocker sut; @Mock - MergeRequest merge; + private MergeRequest merge; @Mock - PullRequest pr; + private PullRequest pr; @Mock - Repository repository; + private Project project; @Mock - Project project; + private PullRequestRef ref; @Mock - PullRequestRef ref; - + private Repository repository; @Before - @SuppressWarnings("unchecked") - public void setUp() throws Exception { - MockitoAnnotations.initMocks(this); + public void setUp() { when(merge.getPullRequest()).thenReturn(pr); when(pr.getToRef()).thenReturn(ref); when(ref.getRepository()).thenReturn(repository); - when(ref.getId()).thenReturn(RegexUtils.REFS_PREFIX + "master"); + when(ref.getDisplayId()).thenReturn("master"); when(repository.getProject()).thenReturn(project); when(repository.getSlug()).thenReturn("repo_1"); when(project.getKey()).thenReturn("PRJ"); - when(userUtils.dereferenceGroups(anyList())).thenReturn(Lists.newArrayList()); - when(regexUtils.match(anyList(), anyString())).thenCallRealMethod(); - when(regexUtils.formatBranchName(anyString())).thenCallRealMethod(); + when(userUtils.dereferenceGroups(anyListOf(String.class))).thenReturn(Collections.emptySet()); when(userUtils.getUserDisplayNameByName(Mockito.eq("user1"))).thenReturn("First user"); when(userUtils.getUserDisplayNameByName(Mockito.eq("user2"))).thenReturn("Second user"); } @Test - public void testBlocking_blocked() throws Exception { + public void testBlocking_blocked() { when(configDao.getConfigForRepo(project.getKey(), repository.getSlug())).thenReturn(Config.builder() .blockedPRs(newArrayList("master")) .build()); - sut.check(merge); + mergeBlocker.check(merge); verify(merge, times(1)).veto(anyString(), anyString()); } @Test - public void testBlocking_notBlocked() throws Exception { + public void testBlocking_notBlocked() { when(configDao.getConfigForRepo(project.getKey(), repository.getSlug())).thenReturn(Config.builder() .blockedPRs(newArrayList("bugfix")) .build()); - sut.check(merge); + mergeBlocker.check(merge); verify(merge, never()).veto(anyString(), anyString()); } @Test - public void testBlocking_missingRequiredReviewer() throws Exception { + public void testBlocking_missingRequiredReviewer() { Set p = newHashSet( mockParticipant("user1", false) ); @@ -100,12 +99,12 @@ public void testBlocking_missingRequiredReviewer() throws Exception { .requiredReviewers(newArrayList("user1")) .requiredReviews(1) .build()); - sut.check(merge); + mergeBlocker.check(merge); verify(merge, times(1)).veto(anyString(), anyString()); } @Test - public void testBlocking_reviewerIsAuthor_notEnoughApprovals() throws Exception { + public void testBlocking_reviewerIsAuthor_notEnoughApprovals() { Set p = newHashSet( mockParticipant("user2", false) ); @@ -117,12 +116,12 @@ public void testBlocking_reviewerIsAuthor_notEnoughApprovals() throws Exception .requiredReviewers(newArrayList("user1", "user2")) .requiredReviews(1) .build()); - sut.check(merge); + mergeBlocker.check(merge); verify(merge, times(1)).veto(anyString(), anyString()); } @Test - public void testBlocking_reviewerIsAuthor_matchingNumberOfApprovals() throws Exception { + public void testBlocking_reviewerIsAuthor_matchingNumberOfApprovals() { Set p = newHashSet( mockParticipant("user2", true) ); @@ -134,12 +133,12 @@ public void testBlocking_reviewerIsAuthor_matchingNumberOfApprovals() throws Exc .requiredReviewers(newArrayList("user1", "user2")) .requiredReviews(2) .build()); - sut.check(merge); + mergeBlocker.check(merge); verify(merge, never()).veto(anyString(), anyString()); } @Test - public void testBlocking_reviewerIsAuthor_approved() throws Exception { + public void testBlocking_reviewerIsAuthor_approved() { Set p = newHashSet( mockParticipant("user2", true) ); @@ -151,12 +150,12 @@ public void testBlocking_reviewerIsAuthor_approved() throws Exception { .requiredReviewers(newArrayList("user2")) .requiredReviews(1) .build()); - sut.check(merge); + mergeBlocker.check(merge); verify(merge, never()).veto(anyString(), anyString()); } @Test - public void testBlocking_prNeedsWorkEnabledAndOneNeedsWorkSet() throws Exception { + public void testBlocking_prNeedsWorkEnabledAndOneNeedsWorkSet() { final PullRequestParticipant reviewer = mockParticipant("user2", true); when(reviewer.getStatus()).thenReturn(PullRequestParticipantStatus.NEEDS_WORK); Set p = newHashSet( @@ -169,12 +168,12 @@ public void testBlocking_prNeedsWorkEnabledAndOneNeedsWorkSet() throws Exception when(configDao.getConfigForRepo(project.getKey(), repository.getSlug())).thenReturn(Config.builder() .blockMergeIfPrNeedsWork(true) .build()); - sut.check(merge); + mergeBlocker.check(merge); verify(merge).veto(anyString(), anyString()); } @Test - public void testBlocking_prNeedsWorkEnabledAndNoNeedsWorkSet() throws Exception { + public void testBlocking_prNeedsWorkEnabledAndNoNeedsWorkSet() { final PullRequestParticipant reviewer = mockParticipant("user2", true); when(reviewer.getStatus()).thenReturn(PullRequestParticipantStatus.APPROVED); Set p = newHashSet( @@ -187,12 +186,12 @@ public void testBlocking_prNeedsWorkEnabledAndNoNeedsWorkSet() throws Exception when(configDao.getConfigForRepo(project.getKey(), repository.getSlug())).thenReturn(Config.builder() .blockMergeIfPrNeedsWork(true) .build()); - sut.check(merge); + mergeBlocker.check(merge); verify(merge, never()).veto(anyString(), anyString()); } @Test - public void testBlocking_prNeedsWorkDisabledAndOneNeedsWorkSet() throws Exception { + public void testBlocking_prNeedsWorkDisabledAndOneNeedsWorkSet() { final PullRequestParticipant reviewer = mockParticipant("user2", true); when(reviewer.getStatus()).thenReturn(PullRequestParticipantStatus.NEEDS_WORK); Set p = newHashSet( @@ -205,7 +204,7 @@ public void testBlocking_prNeedsWorkDisabledAndOneNeedsWorkSet() throws Exceptio when(configDao.getConfigForRepo(project.getKey(), repository.getSlug())).thenReturn(Config.builder() .blockMergeIfPrNeedsWork(false) .build()); - sut.check(merge); + mergeBlocker.check(merge); verify(merge, never()).veto(anyString(), anyString()); } } diff --git a/src/test/java/com/monitorjbl/plugins/PullRequestApprovalTest.java b/src/test/java/com/monitorjbl/plugins/PullRequestApprovalTest.java index 771b19c..e6494f2 100644 --- a/src/test/java/com/monitorjbl/plugins/PullRequestApprovalTest.java +++ b/src/test/java/com/monitorjbl/plugins/PullRequestApprovalTest.java @@ -2,13 +2,15 @@ import com.atlassian.bitbucket.pull.PullRequest; import com.atlassian.bitbucket.pull.PullRequestParticipant; -import com.google.common.collect.Sets; +import com.google.common.collect.ImmutableSet; import com.monitorjbl.plugins.config.Config; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; +import org.mockito.runners.MockitoJUnitRunner; +import java.util.Collections; import java.util.Set; import static com.google.common.collect.Lists.newArrayList; @@ -20,15 +22,15 @@ import static org.junit.Assert.assertThat; import static org.mockito.Mockito.when; +@RunWith(MockitoJUnitRunner.class) public class PullRequestApprovalTest { @Mock - PullRequest pr; + private PullRequest pr; @Mock - UserUtils utils; + private UserUtils utils; @Before public void before() { - MockitoAnnotations.initMocks(this); PullRequestParticipant author = mockParticipant("author", false); when(pr.getAuthor()).thenReturn(author); } @@ -44,7 +46,7 @@ public void testDefaultConfiguration_withRevieiwers() { @Test public void testDefaultConfiguration_noRevieiwers() { - when(pr.getReviewers()).thenReturn(Sets.newHashSet()); + when(pr.getReviewers()).thenReturn(Collections.emptySet()); assertThat(new PullRequestApproval(Config.builder().build(), utils).isPullRequestApproved(pr), is(true)); } @@ -77,7 +79,7 @@ public void testSingleApprover_approved() { @Test public void testSingleApprover_submitterIsApprover() { PullRequestParticipant author = mockParticipant("user1", false); - when(pr.getReviewers()).thenReturn(Sets.newHashSet()); + when(pr.getReviewers()).thenReturn(Collections.emptySet()); when(pr.getAuthor()).thenReturn(author); assertThat(new PullRequestApproval(Config.builder() .requiredReviewers(newArrayList("user1")) @@ -171,7 +173,7 @@ public void testMissingReviewers_oneMissing() { .requiredReviewers(newArrayList("user1")) .requiredReviews(1) .build(), - utils).missingRevieiwers(pr).size(), is(1)); + utils).missingReviewers(pr).size(), is(1)); } @Test @@ -186,7 +188,7 @@ public void testMissingReviewers_twoOutOfThreeMissing() { .requiredReviewers(newArrayList("user1", "user2", "user3")) .requiredReviews(1) .build(), - utils).missingRevieiwers(pr).size(), is(2)); + utils).missingReviewers(pr).size(), is(2)); } @Test @@ -201,7 +203,7 @@ public void testMissingReviewers_zeroMissing() { .requiredReviewers(newArrayList("user1", "user2", "user3")) .requiredReviews(1) .build(), - utils).missingRevieiwers(pr).size(), is(0)); + utils).missingReviewers(pr).size(), is(0)); } @Test @@ -223,14 +225,14 @@ public void testSeenRevieiwers_twoOutOfThree() { } @Test - public void testGroupRequiredReviewers_mixed() throws Exception { + public void testGroupRequiredReviewers_mixed() { Set p = newHashSet( mockParticipant("user1", true), mockParticipant("user2", true), mockParticipant("user3", true) ); when(pr.getReviewers()).thenReturn(p); - when(utils.dereferenceGroups(newArrayList("group1"))).thenReturn(newArrayList("user2", "user3")); + when(utils.dereferenceGroups(newArrayList("group1"))).thenReturn(ImmutableSet.of("user2", "user3")); Set seen = new PullRequestApproval(Config.builder() .requiredReviewers(newArrayList("user1")) @@ -246,14 +248,14 @@ public void testGroupRequiredReviewers_mixed() throws Exception { } @Test - public void testGroupRequiredReviewers_groupOnly() throws Exception { + public void testGroupRequiredReviewers_groupOnly() { Set p = newHashSet( mockParticipant("user1", true), mockParticipant("user2", true), mockParticipant("user3", true) ); when(pr.getReviewers()).thenReturn(p); - when(utils.dereferenceGroups(newArrayList("group1"))).thenReturn(newArrayList("user2", "user3")); + when(utils.dereferenceGroups(newArrayList("group1"))).thenReturn(ImmutableSet.of("user2", "user3")); Set seen = new PullRequestApproval(Config.builder() .requiredReviewerGroups(newArrayList("group1")) @@ -267,14 +269,14 @@ public void testGroupRequiredReviewers_groupOnly() throws Exception { } @Test - public void testGroupRequiredReviewers_multipleGroups() throws Exception { + public void testGroupRequiredReviewers_multipleGroups() { Set p = newHashSet( mockParticipant("user1", true), mockParticipant("user2", true), mockParticipant("user3", true) ); when(pr.getReviewers()).thenReturn(p); - when(utils.dereferenceGroups(newArrayList("group1", "group2"))).thenReturn(newArrayList("user2", "user3")); + when(utils.dereferenceGroups(newArrayList("group1", "group2"))).thenReturn(ImmutableSet.of("user2", "user3")); Set seen = new PullRequestApproval(Config.builder() .requiredReviewerGroups(newArrayList("group1", "group2")) diff --git a/src/test/java/com/monitorjbl/plugins/PullRequestListenerTest.java b/src/test/java/com/monitorjbl/plugins/PullRequestListenerTest.java index beefbb5..136d716 100644 --- a/src/test/java/com/monitorjbl/plugins/PullRequestListenerTest.java +++ b/src/test/java/com/monitorjbl/plugins/PullRequestListenerTest.java @@ -1,7 +1,6 @@ package com.monitorjbl.plugins; import com.atlassian.bitbucket.event.pull.PullRequestOpenedEvent; -import com.atlassian.bitbucket.permission.Permission; import com.atlassian.bitbucket.project.Project; import com.atlassian.bitbucket.pull.PullRequest; import com.atlassian.bitbucket.pull.PullRequestMergeRequest; @@ -11,72 +10,68 @@ import com.atlassian.bitbucket.pull.PullRequestService; import com.atlassian.bitbucket.repository.Repository; import com.atlassian.bitbucket.user.ApplicationUser; -import com.atlassian.bitbucket.user.EscalatedSecurityContext; +import com.atlassian.bitbucket.user.DummySecurityService; import com.atlassian.bitbucket.user.SecurityService; -import com.atlassian.bitbucket.util.Operation; -import com.google.common.collect.Lists; import com.monitorjbl.plugins.config.Config; import com.monitorjbl.plugins.config.ConfigDao; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; +import org.mockito.Spy; +import org.mockito.runners.MockitoJUnitRunner; -import javax.annotation.Nonnull; -import java.util.Set; +import java.util.Collections; import static com.google.common.collect.Lists.newArrayList; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyList; -import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.anyListOf; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -@SuppressWarnings("unchecked") +@RunWith(MockitoJUnitRunner.class) public class PullRequestListenerTest { @Mock private ConfigDao configDao; + @InjectMocks + private PullRequestListener listener; @Mock private PullRequestService prService; - @Mock - private SecurityService securityService; + @Spy + @SuppressWarnings("unused") //Used by @InjectMocks + private RegexUtils regexUtils; + @Spy + @SuppressWarnings("unused") //Used by @InjectMocks + private SecurityService securityService = new DummySecurityService(); @Mock private UserUtils userUtils; - @Mock - private RegexUtils regexUtils; - @InjectMocks - private PullRequestListener sut; @Mock - PullRequestOpenedEvent openedEvent; + private PullRequestParticipant author; @Mock - PullRequest pr; + private ApplicationUser authorUser; @Mock - Repository toRepo; + private PullRequestRef fromRef; @Mock - Repository fromRepo; + private Repository fromRepo; @Mock - Project project; + private PullRequestMergeability mergeability; @Mock - PullRequestRef toRef; + private PullRequestOpenedEvent openedEvent; @Mock - PullRequestRef fromRef; + private PullRequest pr; @Mock - PullRequestMergeability mergeability; + private Project project; @Mock - PullRequestParticipant author; + private PullRequestRef toRef; @Mock - ApplicationUser authorUser; - @Mock - EscalatedSecurityContext securityContext; + private Repository toRepo; @Before - public void setUp() throws Exception { - MockitoAnnotations.initMocks(this); - when(securityService.withPermission(any(Permission.class), anyString())).thenReturn(new MockSecurityContext()); + public void setUp() { when(openedEvent.getPullRequest()).thenReturn(pr); when(authorUser.getSlug()).thenReturn("someguy"); when(author.getUser()).thenReturn(authorUser); @@ -86,9 +81,9 @@ public void setUp() throws Exception { when(pr.getVersion()).thenReturn(10384); when(pr.getAuthor()).thenReturn(author); when(toRef.getRepository()).thenReturn(toRepo); - when(toRef.getId()).thenReturn(RegexUtils.REFS_PREFIX + "master"); + when(toRef.getDisplayId()).thenReturn("master"); when(fromRef.getRepository()).thenReturn(fromRepo); - when(fromRef.getId()).thenReturn(RegexUtils.REFS_PREFIX + "otherMaster"); + when(fromRef.getDisplayId()).thenReturn("otherMaster"); when(toRepo.getProject()).thenReturn(project); when(toRepo.getId()).thenReturn(20); when(toRepo.getSlug()).thenReturn("repo_1"); @@ -96,30 +91,28 @@ public void setUp() throws Exception { when(fromRepo.getId()).thenReturn(30); when(fromRepo.getSlug()).thenReturn("repo_2"); when(project.getKey()).thenReturn("PRJ"); - when(userUtils.dereferenceGroups(anyList())).thenReturn(Lists.newArrayList()); - when(regexUtils.match(anyList(), anyString())).thenCallRealMethod(); - when(regexUtils.formatBranchName(anyString())).thenCallRealMethod(); + when(userUtils.dereferenceGroups(anyListOf(String.class))).thenReturn(Collections.emptySet()); } @Test - public void testAutomerge_defaultConfig() throws Exception { + public void testAutomerge_defaultConfig() { when(configDao.getConfigForRepo(project.getKey(), toRepo.getSlug())).thenReturn(Config.builder().build()); - sut.automergePullRequest(pr); + listener.automergePullRequest(pr); verify(prService, never()).merge(any(PullRequestMergeRequest.class)); } @Test - public void testAutomerge_blockedBranches() throws Exception { + public void testAutomerge_blockedBranches() { when(configDao.getConfigForRepo(project.getKey(), toRepo.getSlug())).thenReturn(Config.builder() .automergePRs(newArrayList("master")) .blockedPRs(newArrayList("master")) .build()); - sut.automergePullRequest(pr); + listener.automergePullRequest(pr); verify(prService, never()).merge(any(PullRequestMergeRequest.class)); } @Test - public void testAutomerge_canMerge() throws Exception { + public void testAutomerge_canMerge() { when(mergeability.canMerge()).thenReturn(true); when(prService.canMerge(toRepo.getId(), pr.getId())).thenReturn(mergeability); when(configDao.getConfigForRepo(project.getKey(), toRepo.getSlug())).thenReturn(Config.builder() @@ -127,17 +120,12 @@ public void testAutomerge_canMerge() throws Exception { .requiredReviewers(newArrayList("user1")) .requiredReviews(1) .build()); - when(securityService.impersonating(any(), any())).thenReturn(securityContext); - when(securityContext.call(any())).then(inv -> { - ((Operation) inv.getArguments()[0]).perform(); - return null; - }); - sut.automergePullRequest(pr); + listener.automergePullRequest(pr); verify(prService, times(1)).merge(any(PullRequestMergeRequest.class)); } @Test - public void testAutomerge_cannotMerge() throws Exception { + public void testAutomerge_cannotMerge() { when(mergeability.canMerge()).thenReturn(false); when(prService.canMerge(toRepo.getId(), pr.getId())).thenReturn(mergeability); when(configDao.getConfigForRepo(project.getKey(), toRepo.getSlug())).thenReturn(Config.builder() @@ -145,39 +133,7 @@ public void testAutomerge_cannotMerge() throws Exception { .requiredReviewers(newArrayList("user1")) .requiredReviews(1) .build()); - sut.automergePullRequest(pr); + listener.automergePullRequest(pr); verify(prService, never()).merge(any(PullRequestMergeRequest.class)); } - - static class MockSecurityContext implements EscalatedSecurityContext { - - @Override - public T call(Operation operation) throws E { - operation.perform(); - return null; - } - - @Override - public void applyToRequest() { - - } - - @Nonnull - @Override - public EscalatedSecurityContext withPermission(Permission permission) { - return null; - } - - @Nonnull - @Override - public EscalatedSecurityContext withPermission(@Nonnull Object o, @Nonnull Permission permission) { - return null; - } - - @Nonnull - @Override - public EscalatedSecurityContext withPermissions(@Nonnull Set set) { - return null; - } - } } diff --git a/src/test/java/com/monitorjbl/plugins/RegexUtilsTest.java b/src/test/java/com/monitorjbl/plugins/RegexUtilsTest.java index 9670149..67b4af2 100644 --- a/src/test/java/com/monitorjbl/plugins/RegexUtilsTest.java +++ b/src/test/java/com/monitorjbl/plugins/RegexUtilsTest.java @@ -1,42 +1,35 @@ package com.monitorjbl.plugins; +import com.google.common.collect.ImmutableList; import org.junit.Test; -import static com.google.common.collect.Lists.newArrayList; -import static org.hamcrest.CoreMatchers.equalTo; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; public class RegexUtilsTest { - RegexUtils sut = new RegexUtils(); + RegexUtils regexUtils = new RegexUtils(); @Test - public void testSinglePattern() throws Exception { - assertTrue(sut.match(newArrayList("bugfix/*"), "bugfix/iss53")); - assertFalse(sut.match(newArrayList("test/*"), "bugfix/iss53")); + public void testSinglePattern() { + assertTrue(regexUtils.match(ImmutableList.of("bugfix/*"), "bugfix/iss53")); + assertFalse(regexUtils.match(ImmutableList.of("test/*"), "bugfix/iss53")); } @Test - public void testSinglePattern_specialCharacters() throws Exception { - assertTrue(sut.match(newArrayList("bug.fix/*"), "bug.fix/iss53")); - assertFalse(sut.match(newArrayList("te.st/*"), "bug.fix/iss53")); + public void testSinglePattern_specialCharacters() { + assertTrue(regexUtils.match(ImmutableList.of("bug.fix/*"), "bug.fix/iss53")); + assertFalse(regexUtils.match(ImmutableList.of("te.st/*"), "bug.fix/iss53")); } @Test - public void testMultiPattern() throws Exception { - assertTrue(sut.match(newArrayList("master", "bugfix/*"), "bugfix/iss53")); - assertFalse(sut.match(newArrayList("master", "test/*"), "bugfix/iss53")); + public void testMultiPattern() { + assertTrue(regexUtils.match(ImmutableList.of("master", "bugfix/*"), "bugfix/iss53")); + assertFalse(regexUtils.match(ImmutableList.of("master", "test/*"), "bugfix/iss53")); } @Test - public void testExactMatch() throws Exception { - assertTrue(sut.match(newArrayList("bugfix/iss53"), "bugfix/iss53")); - assertFalse(sut.match(newArrayList("bugfix/iss53"), "bugfix/iss54")); - } - - @Test - public void testFormatBranchName() throws Exception { - assertThat(sut.formatBranchName("refs/heads/master"), equalTo("master")); + public void testExactMatch() { + assertTrue(regexUtils.match(ImmutableList.of("bugfix/iss53"), "bugfix/iss53")); + assertFalse(regexUtils.match(ImmutableList.of("bugfix/iss53"), "bugfix/iss54")); } } diff --git a/src/test/java/com/monitorjbl/plugins/UserUtilsTest.java b/src/test/java/com/monitorjbl/plugins/UserUtilsTest.java index a2547e9..aeb7b5f 100644 --- a/src/test/java/com/monitorjbl/plugins/UserUtilsTest.java +++ b/src/test/java/com/monitorjbl/plugins/UserUtilsTest.java @@ -4,16 +4,18 @@ import com.atlassian.bitbucket.user.UserService; import com.atlassian.bitbucket.util.Page; import com.atlassian.bitbucket.util.PageRequest; +import com.google.common.collect.ImmutableList; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; +import org.mockito.runners.MockitoJUnitRunner; import org.mockito.stubbing.Answer; import java.util.List; +import java.util.Set; -import static com.google.common.collect.Lists.newArrayList; import static com.monitorjbl.plugins.TestUtils.mockApplicationUser; import static java.util.Collections.emptyList; import static org.hamcrest.CoreMatchers.equalTo; @@ -25,25 +27,23 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +@RunWith(MockitoJUnitRunner.class) public class UserUtilsTest { - @Mock private UserService userService; @InjectMocks - UserUtils sut; + private UserUtils userUtils; @Before - @SuppressWarnings("unchecked") - public void setup() throws Exception { - MockitoAnnotations.initMocks(this); + public void setup() { ApplicationUser userA = mockApplicationUser("userA"); ApplicationUser userB = mockApplicationUser("userB"); ApplicationUser user1 = mockApplicationUser("user1"); ApplicationUser user2 = mockApplicationUser("user2"); ApplicationUser user3 = mockApplicationUser("user3"); ApplicationUser user4 = mockApplicationUser("user4"); - List userList1 = newArrayList(user1, user2); - List userList2 = newArrayList(user3, user4); + List userList1 = ImmutableList.of(user1, user2); + List userList2 = ImmutableList.of(user3, user4); Page p1 = mock(Page.class); Page p2 = mock(Page.class); @@ -73,16 +73,16 @@ public void setup() throws Exception { } @Test - public void testDereferenceGroups_single() throws Exception { - List result = sut.dereferenceGroups(newArrayList("group1")); + public void testDereferenceGroups_single() { + Set result = userUtils.dereferenceGroups(ImmutableList.of("group1")); assertThat(result, is(notNullValue())); assertThat(result.size(), equalTo(2)); assertThat(result, hasItems("user1", "user2")); } @Test - public void testDereferenceGroups_multiple() throws Exception { - List result = sut.dereferenceGroups(newArrayList("group1", "group2")); + public void testDereferenceGroups_multiple() { + Set result = userUtils.dereferenceGroups(ImmutableList.of("group1", "group2")); assertThat(result, is(notNullValue())); assertThat(result.size(), equalTo(4)); assertThat(result, hasItems("user1", "user2", "user3", "user4")); diff --git a/src/test/java/com/monitorjbl/plugins/config/ConfigDaoTest.java b/src/test/java/com/monitorjbl/plugins/config/ConfigDaoTest.java index da46607..19a80c8 100644 --- a/src/test/java/com/monitorjbl/plugins/config/ConfigDaoTest.java +++ b/src/test/java/com/monitorjbl/plugins/config/ConfigDaoTest.java @@ -1,15 +1,15 @@ package com.monitorjbl.plugins.config; -import com.atlassian.sal.api.pluginsettings.PluginSettingsFactory; import com.atlassian.bitbucket.user.UserService; -import com.google.common.base.Predicate; +import com.atlassian.sal.api.pluginsettings.PluginSettingsFactory; +import com.google.common.base.Predicates; import com.google.common.collect.Lists; import org.hamcrest.CoreMatchers; -import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; +import org.mockito.runners.MockitoJUnitRunner; import java.util.List; @@ -19,66 +19,56 @@ import static org.hamcrest.Matchers.contains; import static org.junit.Assert.assertThat; -@SuppressWarnings("unchecked") +@RunWith(MockitoJUnitRunner.class) public class ConfigDaoTest { + @InjectMocks + private ConfigDao dao; @Mock + @SuppressWarnings("unused") //Used by @InjectMocks private PluginSettingsFactory pluginSettingsFactory; @Mock + @SuppressWarnings("unused") //Used by @InjectMocks private UserService userService; - @InjectMocks - ConfigDao sut; - - Predicate noopPredicate = new Predicate() { - @Override - public boolean apply(Object input) { - return true; - } - }; - - @Before - public void setup() throws Exception { - MockitoAnnotations.initMocks(this); - } @Test - public void testJoin() throws Exception { - String val = sut.join(newArrayList("UseR1", "user2", "USER3"), noopPredicate); + public void testJoin() { + String val = dao.join(newArrayList("UseR1", "user2", "USER3"), Predicates.alwaysTrue()); assertThat(val, equalTo("UseR1, user2, USER3")); } @Test - public void testSplit() throws Exception { - List str = sut.split("usER1, user2, USER3"); - assertThat(str, CoreMatchers.>equalTo(newArrayList("usER1", "user2", "USER3"))); + public void testSplit() { + List str = dao.split("usER1, user2, USER3"); + assertThat(str, CoreMatchers.equalTo(newArrayList("usER1", "user2", "USER3"))); } @Test - public void testSplit_blank() throws Exception { - List str = sut.split(""); - assertThat(str, CoreMatchers.>equalTo(Lists.newArrayList())); + public void testSplit_blank() { + List str = dao.split(""); + assertThat(str, CoreMatchers.equalTo(Lists.newArrayList())); } @Test - public void testOverlayList_topHasValue() throws Exception { - List str = sut.overlay(newArrayList("bottom"), newArrayList("top")); + public void testOverlayList_topHasValue() { + List str = dao.overlay(newArrayList("bottom"), newArrayList("top")); assertThat(str, contains("top")); } @Test - public void testOverlayList_topHasNoValue() throws Exception { - List str = sut.overlay(newArrayList("bottom"), Lists.newArrayList()); + public void testOverlayList_topHasNoValue() { + List str = dao.overlay(newArrayList("bottom"), Lists.newArrayList()); assertThat(str, contains("bottom")); } @Test - public void testReverseOverlayList_valuesEqual() throws Exception { - List str = sut.reverseOverlay(newArrayList("bottom"), newArrayList("bottom")); + public void testReverseOverlayList_valuesEqual() { + List str = dao.reverseOverlay(newArrayList("bottom"), newArrayList("bottom")); assertThat(str, nullValue()); } @Test - public void testReverseOverlayList_valuesNotEqual() throws Exception { - List str = sut.reverseOverlay(newArrayList("bottom"), newArrayList("top")); + public void testReverseOverlayList_valuesNotEqual() { + List str = dao.reverseOverlay(newArrayList("bottom"), newArrayList("top")); assertThat(str, contains("top")); } } diff --git a/src/test/java/com/monitorjbl/plugins/config/ConfigServletTest.java b/src/test/java/com/monitorjbl/plugins/config/ConfigServletTest.java index 6d6d57e..53659db 100644 --- a/src/test/java/com/monitorjbl/plugins/config/ConfigServletTest.java +++ b/src/test/java/com/monitorjbl/plugins/config/ConfigServletTest.java @@ -5,88 +5,80 @@ import com.atlassian.bitbucket.repository.Repository; import com.atlassian.bitbucket.repository.RepositoryService; import com.atlassian.bitbucket.user.ApplicationUser; -import com.atlassian.sal.api.auth.LoginUriProvider; -import com.atlassian.sal.api.user.UserManager; import com.atlassian.templaterenderer.TemplateRenderer; -import com.monitorjbl.plugins.UserUtils; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; +import org.mockito.runners.MockitoJUnitRunner; import javax.servlet.http.HttpServletResponse; import java.io.Writer; -import java.util.Map; +import static org.mockito.Matchers.anyMapOf; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.any; -import static org.mockito.Mockito.atLeastOnce; -import static org.mockito.Mockito.times; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -@SuppressWarnings("unchecked") +@RunWith(MockitoJUnitRunner.class) public class ConfigServletTest { - @Mock - private UserManager userManager; - @Mock - private UserUtils userUtils; @Mock private RepositoryService repoService; @Mock - private TemplateRenderer renderer; - @Mock private PermissionService permissionService; - @Mock - private LoginUriProvider loginUriProvider; @InjectMocks - private ConfigServlet sut; + private ConfigServlet servlet; + @Mock + private TemplateRenderer templateRenderer; @Mock - HttpServletResponse response; + private Repository repo; @Mock - ApplicationUser user; + private HttpServletResponse response; @Mock - Repository repo; + private ApplicationUser user; @Before - public void setUp() throws Exception { - MockitoAnnotations.initMocks(this); - when(userUtils.getApplicationUserByName("user1")).thenReturn(user); + public void setUp() { when(repoService.getBySlug("PRJ", "repo1")).thenReturn(repo); when(permissionService.hasRepositoryPermission(user, repo, Permission.REPO_ADMIN)).thenReturn(true); + when(user.getName()).thenReturn("user1"); } @Test public void testFoundRepo() throws Exception { - sut.handleRequest("/plugins/servlet/pr-harmony/PRJ/repo1", "user1", response); - verify(response, times(1)).setStatus(HttpServletResponse.SC_OK); - verify(renderer, times(1)).render(anyString(), any(Map.class), any(Writer.class)); + servlet.handleRequest("/plugins/servlet/pr-harmony/PRJ/repo1", user, response); + verify(response).setStatus(HttpServletResponse.SC_OK); + verify(templateRenderer).render(anyString(), anyMapOf(String.class, Object.class), any(Writer.class)); } @Test public void testFoundRepo_differentContext() throws Exception { - sut.handleRequest("/bitbucket/plugins/servlet/pr-harmony/PRJ/repo1", "user1", response); - verify(response, times(1)).setStatus(HttpServletResponse.SC_OK); - verify(renderer, times(1)).render(anyString(), any(Map.class), any(Writer.class)); + servlet.handleRequest("/bitbucket/plugins/servlet/pr-harmony/PRJ/repo1", user, response); + verify(response).setStatus(HttpServletResponse.SC_OK); + verify(templateRenderer).render(anyString(), anyMapOf(String.class, Object.class), any(Writer.class)); } @Test public void testHandleMalformedRequest() throws Exception { - sut.handleRequest("/plugins/servlet/saywhat/pr-harmony/PRJ/repo1", "user1", response); - verify(response, atLeastOnce()).setStatus(HttpServletResponse.SC_BAD_REQUEST); + servlet.handleRequest("/plugins/servlet/saywhat/pr-harmony/PRJ/repo1", user, response); + verify(response).setStatus(HttpServletResponse.SC_BAD_REQUEST); } @Test public void testHandleMissingRepo() throws Exception { - sut.handleRequest("/plugins/servlet/pr-harmony/FAKE/repo1", "user1", response); - verify(response, atLeastOnce()).setStatus(HttpServletResponse.SC_NOT_FOUND); + servlet.handleRequest("/plugins/servlet/pr-harmony/FAKE/repo1", user, response); + verify(response).setStatus(HttpServletResponse.SC_NOT_FOUND); } @Test public void testPermissionDenied() throws Exception { - sut.handleRequest("/plugins/servlet/pr-harmony/PRJ/repo1", "nonuser1", response); - verify(response, atLeastOnce()).setStatus(HttpServletResponse.SC_FORBIDDEN); + reset(permissionService); + + servlet.handleRequest("/plugins/servlet/pr-harmony/PRJ/repo1", user, response); + verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN); } } diff --git a/src/test/java/it/com/monitorjbl/plugins/BaseIntegrationTest.java b/src/test/java/it/com/monitorjbl/plugins/BaseIntegrationTest.java new file mode 100644 index 0000000..8d78096 --- /dev/null +++ b/src/test/java/it/com/monitorjbl/plugins/BaseIntegrationTest.java @@ -0,0 +1,70 @@ +package it.com.monitorjbl.plugins; + +import com.atlassian.bitbucket.test.BaseFuncTest; +import com.atlassian.bitbucket.test.TestContext; +import com.jayway.restassured.RestAssured; +import com.jayway.restassured.http.ContentType; +import com.monitorjbl.plugins.config.Config; + +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminPassword; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminUser; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getRestURL; +import static java.util.Collections.singletonList; + +public class BaseIntegrationTest extends BaseFuncTest { + protected static final Config TEST_CONFIG = Config.builder() + .automergePRs(singletonList("master")) + .automergePRsFrom(singletonList("release/.*")) + .blockedCommits(singletonList("master")) + .blockMergeIfPrNeedsWork(true) + .blockedPRs(singletonList("release/.*")) + .defaultReviewerGroups(singletonList("default-group")) + .defaultReviewers(singletonList("bob")) + .excludedGroups(singletonList("excluded-group")) + .excludedUsers(singletonList("janed")) + .requiredReviews(1) + .requiredReviewerGroups(singletonList("required-group")) + .requiredReviewers(singletonList("bob")) + .build(); + protected static final String TEST_PROJECT = "HRMNY"; + protected static final String TEST_REPO = "pr-harmony"; + + protected static String getProjectUrl(String resource, String projectKey) { + return getRestURL("pr-harmony", "1.0") + "/" + resource + "/" + projectKey; + } + + protected static String getRepositoryUrl(String resource, String projectKey, String repositorySlug) { + return getProjectUrl(resource, projectKey) + "/" + repositorySlug; + } + + protected static void putProjectConfig(Config config, String projectKey) { + putConfig(config, getProjectUrl("config", projectKey)); + } + + protected static void putRepositoryConfig(Config config, String projectKey, String repositorySlug) { + putConfig(config, getRepositoryUrl("config", projectKey, repositorySlug)); + } + + protected static void setUpUsersAndGroups(TestContext testContext) { + testContext + .user("bob") + .user("janed") + .user("jdoe") + .group("all-group", "bob", "janed", "jdoe") + .group("default-group", "jdoe", "janed") + .group("excluded-group", "bob") + .group("required-group", "janed"); + } + + private static void putConfig(Config config, String url) { + RestAssured.given() + .auth().preemptive().basic(getAdminUser(), getAdminPassword()) + .body(config) + .contentType(ContentType.JSON) + .expect() + .log().ifValidationFails() + .statusCode(200) + .when() + .put(url); + } +} diff --git a/src/test/java/it/com/monitorjbl/plugins/hosting/CommitBlockerHookTest.java b/src/test/java/it/com/monitorjbl/plugins/hosting/CommitBlockerHookTest.java new file mode 100644 index 0000000..e800b6f --- /dev/null +++ b/src/test/java/it/com/monitorjbl/plugins/hosting/CommitBlockerHookTest.java @@ -0,0 +1,84 @@ +package it.com.monitorjbl.plugins.hosting; + +import com.atlassian.bitbucket.permission.Permission; +import com.atlassian.bitbucket.test.ProcessFailedException; +import com.atlassian.bitbucket.test.RepositoryTestHelper; +import com.monitorjbl.plugins.config.Config; +import it.com.monitorjbl.plugins.BaseIntegrationTest; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.util.Collections; +import java.util.Locale; + +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminPassword; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminUser; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getBaseURL; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1Repository1; +import static org.hamcrest.CoreMatchers.allOf; +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; + +public class CommitBlockerHookTest extends BaseIntegrationTest { + private static final Config FORK_CONFIG = Config.builder() + .blockedCommits(Collections.singletonList("master")) + .excludedGroups(Collections.singletonList("exclusions")) + .excludedUsers(Collections.singletonList("excluded-by-name")) + .build(); + private static final String FORK_PROJECT = ("~" + getAdminUser()).toUpperCase(Locale.ROOT); + private static final String FORK_REPO = getProject1Repository1(); + + @BeforeClass + public static void setupRepository() { + getClassTestContext() + //Create some users to facilitate the various tests + .user("blocked-user") + .user("excluded-by-group") + .user("excluded-by-name") + //Create some groups for the new users + .group("exclusions", "excluded-by-group") + .group("test-users", "blocked-user", "excluded-by-group", "excluded-by-name") + //Fork the default repository to our personal project so we don't make any changes + //to the default repository directly, which may throw off other tests + .fork(getProject1(), getProject1Repository1(), getAdminUser(), getAdminPassword()) + //Grant all of the test users permission to push to the test repository + .repositoryPermissionForGroup(FORK_PROJECT, FORK_REPO, "test-users", Permission.REPO_WRITE); + putRepositoryConfig(FORK_CONFIG, FORK_PROJECT, FORK_REPO); + } + + @Test + public void testPush_blocked() throws Exception { + try { + RepositoryTestHelper.pushCommit(getBaseURL(), "blocked-user", "blocked-user", FORK_PROJECT, FORK_REPO, "master", + "Johnny B. Blocked", "jbblocked@test.com", "harmony", "harmonious.txt", "Test", "Should be blocked"); + + fail("The blocked user should not be allowed to push"); + } catch (ProcessFailedException expected) { + assertThat(expected.getStdErr(), allOf( + containsString("Push Rejected"), + containsString("Direct pushes are not allowed for [master]") + )); + } + } + + @Test + public void testPush_excludedByGroup() throws Exception { + RepositoryTestHelper.pushCommit(getBaseURL(), "excluded-by-group", "excluded-by-group", FORK_PROJECT, FORK_REPO, + "master", "John Doe", "jdoe@test.com", "harmony", "harmonious.txt", "Excluded by group", "Should be accepted"); + } + + @Test + public void testPush_excludedByName() throws Exception { + RepositoryTestHelper.pushCommit(getBaseURL(), "excluded-by-name", "excluded-by-name", FORK_PROJECT, FORK_REPO, + "master", "Jane Doe", "janed@test.com", "harmony", "harmonious.txt", "Excluded by name", "Should be accepted"); + } + + @Test + public void testPush_unrestrictedBranch() throws Exception { + //This user has no exclusions, so they can only push if the branch isn't restricted + RepositoryTestHelper.pushCommit(getBaseURL(), "blocked-user", "blocked-user", FORK_PROJECT, FORK_REPO, + "basic_branching", "Jane Doe", "janed@test.com", "harmony", "harmonious.txt", "Test", "Unrestricted"); + } +} diff --git a/src/test/java/it/com/monitorjbl/plugins/rest/ConfigResourceTest.java b/src/test/java/it/com/monitorjbl/plugins/rest/ConfigResourceTest.java new file mode 100644 index 0000000..c6138c5 --- /dev/null +++ b/src/test/java/it/com/monitorjbl/plugins/rest/ConfigResourceTest.java @@ -0,0 +1,119 @@ +package it.com.monitorjbl.plugins.rest; + +import com.jayway.restassured.RestAssured; +import it.com.monitorjbl.plugins.BaseIntegrationTest; +import org.junit.BeforeClass; +import org.junit.Test; + +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminPassword; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminUser; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1Repository1; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getRestURL; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; + +public class ConfigResourceTest extends BaseIntegrationTest { + @BeforeClass + public static void setUpUsersAndGroups() { + //Setup groups/users to use in other tests. + setUpUsersAndGroups(getClassTestContext()); + } + + @Test + public void testGetProjectConfig_noConfig() { + RestAssured.given() + .auth().preemptive().basic(getAdminUser(), getAdminPassword()) + .expect() + .body("automergePRs", empty()) + .body("automergePRsFrom", empty()) + .body("blockedCommits", empty()) + .body("blockedPRs", empty()) + .body("defaultReviewerGroups", empty()) + .body("defaultReviewers", empty()) + .body("excludedGroups", empty()) + .body("excludedUsers", empty()) + .body("requiredReviewerGroups", empty()) + .body("requiredReviewers", empty()) + .log().ifValidationFails() + .when() + .get(getRestURL("pr-harmony", "1.0") + "/config/" + getProject1()); + } + + @Test + public void testGetRepoConfig_noConfig() { + RestAssured.given() + .auth().preemptive().basic(getAdminUser(), getAdminPassword()) + .expect() + .body("automergePRs", empty()) + .body("automergePRsFrom", empty()) + .body("blockedCommits", empty()) + .body("blockedPRs", empty()) + .body("defaultReviewerGroups", empty()) + .body("defaultReviewers", empty()) + .body("excludedGroups", empty()) + .body("excludedUsers", empty()) + .body("requiredReviewerGroups", empty()) + .body("requiredReviewers", empty()) + .log().ifValidationFails() + .when() + .get(getRestURL("pr-harmony", "1.0") + "/config/" + getProject1() + "/" + getProject1Repository1()); + } + + @Test + public void testProjectConfig_roundtrip() { + //Create a project specifically for this test, to ensure we don't apply configuration to any + //project that might be reused later and interfere with subsequent tests + testContext.project(TEST_PROJECT, "Harmony"); + putProjectConfig(TEST_CONFIG, TEST_PROJECT); + + RestAssured.given() + .auth().preemptive().basic(getAdminUser(), getAdminPassword()) + .expect() + .body("automergePRs", equalTo(TEST_CONFIG.getAutomergePRs())) + .body("automergePRsFrom", equalTo(TEST_CONFIG.getAutomergePRsFrom())) + .body("blockedCommits", equalTo(TEST_CONFIG.getBlockedCommits())) + .body("blockedPRs", equalTo(TEST_CONFIG.getBlockedPRs())) + .body("blockMergeIfPrNeedsWork", equalTo(TEST_CONFIG.getBlockMergeIfPrNeedsWork())) + .body("defaultReviewerGroups", equalTo(TEST_CONFIG.getDefaultReviewerGroups())) + .body("defaultReviewers", equalTo(TEST_CONFIG.getDefaultReviewers())) + .body("excludedGroups", equalTo(TEST_CONFIG.getExcludedGroups())) + .body("excludedUsers", equalTo(TEST_CONFIG.getExcludedUsers())) + .body("requiredReviewerGroups", equalTo(TEST_CONFIG.getRequiredReviewerGroups())) + .body("requiredReviewers", equalTo(TEST_CONFIG.getRequiredReviewers())) + .body("requiredReviews", equalTo(TEST_CONFIG.getRequiredReviews())) + .log().ifValidationFails() + .expect() + .when() + .get(getRestURL("pr-harmony", "1.0") + "/config/" + TEST_PROJECT); + } + + @Test + public void testRepoConfig_roundtrip() { + //Create a repository specifically for this test, to ensure we don't apply configuration to any + //repository that might be reused later and interfere with subsequent tests + testContext.project(TEST_PROJECT, "Harmony") + .repository(TEST_PROJECT, TEST_REPO); + putRepositoryConfig(TEST_CONFIG, TEST_PROJECT, TEST_REPO); + + RestAssured.given() + .auth().preemptive().basic(getAdminUser(), getAdminPassword()) + .expect() + .body("automergePRs", equalTo(TEST_CONFIG.getAutomergePRs())) + .body("automergePRsFrom", equalTo(TEST_CONFIG.getAutomergePRsFrom())) + .body("blockedCommits", equalTo(TEST_CONFIG.getBlockedCommits())) + .body("blockedPRs", equalTo(TEST_CONFIG.getBlockedPRs())) + .body("blockMergeIfPrNeedsWork", equalTo(TEST_CONFIG.getBlockMergeIfPrNeedsWork())) + .body("defaultReviewerGroups", equalTo(TEST_CONFIG.getDefaultReviewerGroups())) + .body("defaultReviewers", equalTo(TEST_CONFIG.getDefaultReviewers())) + .body("excludedGroups", equalTo(TEST_CONFIG.getExcludedGroups())) + .body("excludedUsers", equalTo(TEST_CONFIG.getExcludedUsers())) + .body("requiredReviewerGroups", equalTo(TEST_CONFIG.getRequiredReviewerGroups())) + .body("requiredReviewers", equalTo(TEST_CONFIG.getRequiredReviewers())) + .body("requiredReviews", equalTo(TEST_CONFIG.getRequiredReviews())) + .log().ifValidationFails() + .expect() + .when() + .get(getRestURL("pr-harmony", "1.0") + "/config/" + TEST_PROJECT + "/" + TEST_REPO); + } +} diff --git a/src/test/java/it/com/monitorjbl/plugins/rest/UserResourceTest.java b/src/test/java/it/com/monitorjbl/plugins/rest/UserResourceTest.java new file mode 100644 index 0000000..99b2136 --- /dev/null +++ b/src/test/java/it/com/monitorjbl/plugins/rest/UserResourceTest.java @@ -0,0 +1,101 @@ +package it.com.monitorjbl.plugins.rest; + +import com.google.common.collect.ImmutableList; +import com.jayway.jsonpath.JsonPath; +import com.jayway.jsonpath.ReadContext; +import com.jayway.restassured.RestAssured; +import com.monitorjbl.plugins.config.Config; +import it.com.monitorjbl.plugins.BaseIntegrationTest; +import org.junit.Test; + +import java.io.InputStream; +import java.util.List; + +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminPassword; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminUser; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1; +import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1Repository1; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.isEmptyString; +import static org.junit.Assert.assertThat; + +public class UserResourceTest extends BaseIntegrationTest { + @Test + public void testGet() { + testContext.project(TEST_PROJECT, "Harmony") + .repository(TEST_PROJECT, TEST_REPO); + setUpUsersAndGroups(testContext); + putRepositoryConfig(TEST_CONFIG, TEST_PROJECT, TEST_REPO); + + InputStream response = RestAssured.given() + .auth().preemptive().basic(getAdminUser(), getAdminPassword()) + .expect() + .body("blockMergeIfPrNeedsWork", equalTo(true)) + .body("defaultReviewers", hasSize(3)) + .body("requiredReviewers", hasSize(2)) + .body("requiredReviews", equalTo(1)) + .log().ifValidationFails() + .when() + .get(getRepositoryUrl("users", TEST_PROJECT, TEST_REPO)) + .asInputStream(); + + //containsInAnyOrder is used because the way user lists are created doesn't guarantee ordering + ReadContext ctx = JsonPath.parse(response); + assertThat(ctx.read("defaultReviewers[*].name"), containsInAnyOrder("bob", "janed", "jdoe")); + assertThat(ctx.read("requiredReviewers[*].name"), containsInAnyOrder("bob", "janed")); + } + + @Test + public void testGet_duplicatesRemoved() { + List allUsers = ImmutableList.of("bob", "janed", "jdoe"); + Config config = TEST_CONFIG.copyBuilder() + //Include all of the users and all the groups for default and required. This means: + //- "bob" gets referenced once (by name) + //- "janed" gets referenced 4 times (by name, and once for each group) + //- "jdoe" gets referenced 3 times (by name, by "all-group" and by "default-group") + .defaultReviewers(allUsers) + .defaultReviewerGroups(ImmutableList.of("all-group", "default-group", "required-group")) + .requiredReviewers(allUsers) + .requiredReviewerGroups(ImmutableList.of("all-group", "default-group", "required-group")) + .build(); + + testContext.project(TEST_PROJECT, "Harmony") + .repository(TEST_PROJECT, TEST_REPO); + setUpUsersAndGroups(testContext); + putRepositoryConfig(config, TEST_PROJECT, TEST_REPO); + + InputStream response = RestAssured.given() + .auth().preemptive().basic(getAdminUser(), getAdminPassword()) + .expect() + .body("blockMergeIfPrNeedsWork", equalTo(true)) + .body("defaultReviewers", hasSize(3)) + .body("requiredReviewers", hasSize(3)) + .body("requiredReviews", equalTo(1)) + .log().ifValidationFails() + .when() + .get(getRepositoryUrl("users", TEST_PROJECT, TEST_REPO)) + .asInputStream(); + + //containsInAnyOrder is used because the way user lists are created doesn't guarantee ordering + ReadContext ctx = JsonPath.parse(response); + assertThat(ctx.read("defaultReviewers[*].name"), containsInAnyOrder(allUsers.toArray())); + assertThat(ctx.read("requiredReviewers[*].name"), containsInAnyOrder(allUsers.toArray())); + } + + @Test + public void testGet_noConfig() { + RestAssured.given() + .auth().preemptive().basic(getAdminUser(), getAdminPassword()) + .expect() + .body("blockMergeIfPrNeedsWork", isEmptyString()) + .body("defaultReviewers", empty()) + .body("requiredReviewers", empty()) + .body("requiredReviews", isEmptyString()) + .log().ifValidationFails() + .when() + .get(getRepositoryUrl("users", getProject1(), getProject1Repository1())); + } +} diff --git a/src/test/java/it/com/monitorjbl/plugins/ui/UserResourceTest.java b/src/test/java/it/com/monitorjbl/plugins/ui/UserResourceTest.java deleted file mode 100644 index 5d23404..0000000 --- a/src/test/java/it/com/monitorjbl/plugins/ui/UserResourceTest.java +++ /dev/null @@ -1,31 +0,0 @@ -package it.com.monitorjbl.plugins.ui; - -import com.atlassian.bitbucket.test.BaseFuncTest; -import com.jayway.restassured.RestAssured; -import org.junit.Test; - -import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminPassword; -import static com.atlassian.bitbucket.test.DefaultFuncTestData.getAdminUser; -import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1; -import static com.atlassian.bitbucket.test.DefaultFuncTestData.getProject1Repository1; -import static com.atlassian.bitbucket.test.DefaultFuncTestData.getRestURL; -import static org.hamcrest.Matchers.empty; -import static org.hamcrest.Matchers.isEmptyString; - -//At the moment this test doesn't cover any significant. The biggest benefit it offers is that, by calling the -//UserResource, it verifies that the plugin has been installed and started successfully -public class UserResourceTest extends BaseFuncTest { - - @Test - public void testGetWithoutConfig() { - RestAssured.given() - .auth().preemptive().basic(getAdminUser(), getAdminPassword()) - .expect() - .body("requiredReviews", isEmptyString()) - .body("blockMergeIfPrNeedsWork", isEmptyString()) - .body("requiredReviewers", empty()) - .body("defaultReviewers", empty()) - .when() - .get(getRestURL("pr-harmony", "1.0") + "/users/" + getProject1() + "/" + getProject1Repository1()); - } -}