diff --git a/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java b/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java index ba6f778b0..5a29c5d82 100644 --- a/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java +++ b/src/main/java/org/jenkinsci/plugins/github/config/GitHubServerConfig.java @@ -227,7 +227,7 @@ public void setClientCacheSize(int clientCacheSize) { /** * @return cached GH client or null */ - private GitHub getCachedClient() { + protected synchronized GitHub getCachedClient() { return cachedClient; } @@ -236,7 +236,7 @@ private GitHub getCachedClient() { * * @param cachedClient updated client. Maybe null to invalidate cache */ - private synchronized void setCachedClient(GitHub cachedClient) { + protected synchronized void setCachedClient(GitHub cachedClient) { this.cachedClient = cachedClient; } diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java index 376ebd770..5db84fa3c 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java @@ -11,6 +11,7 @@ import org.jenkinsci.plugins.github.admin.GitHubHookRegisterProblemMonitor; import org.jenkinsci.plugins.github.config.HookSecretConfig; import org.jenkinsci.plugins.github.extension.GHEventsSubscriber; +import org.jenkinsci.plugins.github.util.FluentIterableWrapper; import org.jenkinsci.plugins.github.util.misc.NullSafeFunction; import org.jenkinsci.plugins.github.util.misc.NullSafePredicate; import org.kohsuke.github.GHEvent; @@ -31,7 +32,6 @@ import java.util.Set; import static com.cloudbees.jenkins.GitHubRepositoryNameContributor.parseAssociatedNames; -import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Predicates.notNull; import static com.google.common.base.Predicates.or; import static java.lang.String.format; @@ -143,10 +143,10 @@ public void run() { */ public void unregisterFor(GitHubRepositoryName name, List aliveRepos) { try { - GHRepository repo = checkNotNull( - from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(), - "There are no credentials with admin access to manage hooks on %s", name - ); + GHRepository repo = repoWithWebhookAccess(name); + if (repo == null) { + return; + } LOGGER.debug("Check {} for redundant hooks...", repo); @@ -165,6 +165,22 @@ public void unregisterFor(GitHubRepositoryName name, List } } + private GHRepository repoWithWebhookAccess(GitHubRepositoryName name) { + FluentIterableWrapper reposAllowedtoManageWebhooks = from(name.resolve(allowedToManageHooks())); + if (!reposAllowedtoManageWebhooks.first().isPresent()) { + LOGGER.debug("There are no github repos configured to allow webhook management for: {}", name); + return null; + } + com.google.common.base.Optional repoWithAdminAccess = reposAllowedtoManageWebhooks + .firstMatch(withAdminAccess()); + if (!repoWithAdminAccess.isPresent()) { + LOGGER.debug("None of the github repos configured have admin access for: {}", name); + return null; + } + GHRepository repo = repoWithAdminAccess.get(); + return repo; + } + /** * Main logic of {@link #registerFor(Item)}. * Updates hooks with replacing old ones with merged new ones @@ -178,10 +194,10 @@ protected Function createHookSubscribedTo(final Li @Override protected GHHook applyNullSafe(@Nonnull GitHubRepositoryName name) { try { - GHRepository repo = checkNotNull( - from(name.resolve(allowedToManageHooks())).firstMatch(withAdminAccess()).orNull(), - "There are no credentials with admin access to manage hooks on %s", name - ); + GHRepository repo = repoWithWebhookAccess(name); + if (repo == null) { + return null; + } Validate.notEmpty(events, "Events list for hook can't be empty"); diff --git a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java index 4bbabbf86..8a4f3e875 100644 --- a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitorTest.java @@ -6,6 +6,8 @@ import hudson.model.Item; import hudson.plugins.git.GitSCM; import org.jenkinsci.plugins.github.extension.GHSubscriberEvent; +import org.jenkinsci.plugins.github.GitHubPlugin; +import org.jenkinsci.plugins.github.config.GitHubServerConfig; import org.jenkinsci.plugins.github.extension.GHEventsSubscriber; import org.jenkinsci.plugins.github.webhook.WebhookManager; import org.jenkinsci.plugins.github.webhook.WebhookManagerTest; @@ -13,13 +15,19 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.runner.RunWith; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.recipes.LocalData; import org.kohsuke.github.GHEvent; +import org.kohsuke.github.GHRepository; +import org.kohsuke.github.GitHub; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; import javax.inject.Inject; import java.io.IOException; +import java.util.Arrays; import java.util.Collections; import static com.cloudbees.jenkins.GitHubRepositoryName.create; @@ -32,14 +40,17 @@ import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.when; /** * @author lanwen (Merkushev Kirill) */ @Issue("JENKINS-24690") +@RunWith(MockitoJUnitRunner.class) public class GitHubHookRegisterProblemMonitorTest { private static final GitHubRepositoryName REPO = new GitHubRepositoryName("host", "user", "repo"); - private static final GitSCM REPO_GIT_SCM = new GitSCM("git://host/user/repo.git"); + private static final String REPO_GIT_URI = "host/user/repo.git"; + private static final GitSCM REPO_GIT_SCM = new GitSCM("git://"+REPO_GIT_URI); private static final GitHubRepositoryName REPO_FROM_PING_PAYLOAD = create("https://github.com/lanwen/test"); @@ -55,9 +66,26 @@ public class GitHubHookRegisterProblemMonitorTest { @Rule public JenkinsRule jRule = new JenkinsRule(); + @Mock + private GitHub github; + @Mock + private GHRepository ghRepository; + + class GitHubServerConfigForTest extends GitHubServerConfig { + public GitHubServerConfigForTest(String credentialsId) { + super(credentialsId); + this.setCachedClient(github); + } + } + @Before public void setUp() throws Exception { jRule.getInstance().getInjector().injectMembers(this); + GitHubServerConfig config = new GitHubServerConfigForTest(""); + config.setApiUrl("http://" + REPO_GIT_URI); + GitHubPlugin.configuration().setConfigs(Arrays.asList(config)); + when(github.getRepository("user/repo")).thenReturn(ghRepository); + when(ghRepository.hasAdminAccess()).thenReturn(true); } @Test @@ -149,6 +177,8 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException { job.addTrigger(new GitHubPushTrigger()); job.setScm(REPO_GIT_SCM); + when(github.getRepository("user/repo")) + .thenThrow(new RuntimeException("shouldReportAboutHookProblemOnRegister")); WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) .registerFor((Item) job).run(); @@ -156,13 +186,35 @@ public void shouldReportAboutHookProblemOnRegister() throws IOException { } @Test - public void shouldReportAboutHookProblemOnUnregister() { + public void shouldNotReportAboutHookProblemOnRegister() throws IOException { + FreeStyleProject job = jRule.createFreeStyleProject(); + job.addTrigger(new GitHubPushTrigger()); + job.setScm(REPO_GIT_SCM); + + WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) + .registerFor((Item) job).run(); + + assertThat("should reg problem", monitor.isProblemWith(REPO), is(false)); + } + + @Test + public void shouldReportAboutHookProblemOnUnregister() throws IOException { + when(github.getRepository("user/repo")) + .thenThrow(new RuntimeException("shouldReportAboutHookProblemOnUnregister")); WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) .unregisterFor(REPO, Collections.emptyList()); assertThat("should reg problem", monitor.isProblemWith(REPO), is(true)); } + @Test + public void shouldNotReportAboutHookAuthProblemOnUnregister() { + WebhookManager.forHookUrl(WebhookManagerTest.HOOK_ENDPOINT) + .unregisterFor(REPO, Collections.emptyList()); + + assertThat("should not reg problem", monitor.isProblemWith(REPO), is(false)); + } + @Test public void shouldResolveOnPingHook() { monitor.registerProblem(REPO_FROM_PING_PAYLOAD, new IOException()); diff --git a/src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java b/src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java index d829d1fff..f6217fe1a 100644 --- a/src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/webhook/WebhookManagerTest.java @@ -86,7 +86,7 @@ public class WebhookManagerTest { @Test public void shouldDoNothingOnNoAdminRights() throws Exception { manager.unregisterFor(nonactive, newArrayList(active)); - verify(manager, times(1)).withAdminAccess(); + verify(manager, never()).withAdminAccess(); verify(manager, never()).fetchHooks(); }