diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsSnapshotTaker.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsSnapshotTaker.java new file mode 100644 index 000000000..8c1fdb388 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsSnapshotTaker.java @@ -0,0 +1,30 @@ +package org.jenkinsci.plugins.github_branch_source; + +import com.cloudbees.plugins.credentials.CredentialsSnapshotTaker; +import com.cloudbees.plugins.credentials.common.UsernamePasswordCredentials; +import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsSnapshotTaker; +import hudson.Extension; + +/** + * A {@link CredentialsSnapshotTaker} for {@link GitHubAppCredentials} that is a no-op. + * + *

As {@code GitHubAppCredentials} tokens are time limited they need to be refreshed + * periodically. This is currently addressed by its use of the {@code writeReplace()} and {@code + * readResolve}, but as these credentials are {@link UsernamePasswordCredentials} this behaviour + * conflicts with the {@link UsernamePasswordCredentialsSnapshotTaker}. This SnapshotTaker restores + * the status quo allowing the Credentials to be replaced using the existing mechanism. + */ +@Extension +public class GitHubAppCredentialsSnapshotTaker + extends CredentialsSnapshotTaker { + + @Override + public GitHubAppCredentials snapshot(GitHubAppCredentials credentials) { + return credentials; + } + + @Override + public Class type() { + return GitHubAppCredentials.class; + } +} diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/AbstractGitHubWireMockTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/AbstractGitHubWireMockTest.java index 26fef7f41..09ef65e1d 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/AbstractGitHubWireMockTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/AbstractGitHubWireMockTest.java @@ -12,14 +12,13 @@ import com.github.tomakehurst.wiremock.http.Response; import com.github.tomakehurst.wiremock.junit.WireMockRule; import java.io.File; -import org.junit.Assert; import org.junit.Before; import org.junit.ClassRule; import org.junit.Rule; import org.jvnet.hudson.test.JenkinsRule; /** @author Liam Newman */ -public abstract class AbstractGitHubWireMockTest extends Assert { +public abstract class AbstractGitHubWireMockTest { // By default the wiremock tests will run without proxy // The tests will use only the stubbed data and will fail if requests are made for missing data. diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java index 8db653974..a2dec2daf 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java @@ -4,6 +4,10 @@ import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.resetAllScenarios; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; import com.github.tomakehurst.wiremock.WireMockServer; import com.github.tomakehurst.wiremock.client.ScenarioMappingBuilder; diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystemTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystemTest.java index d7bfcd5b2..abfe75032 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystemTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystemTest.java @@ -28,12 +28,14 @@ import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.core.IsNull.nullValue; +import static org.junit.Assert.fail; import static org.junit.Assume.assumeThat; import hudson.AbortException; diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java index 4d7aefa4b..e8c2acc5d 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java @@ -25,7 +25,9 @@ package org.jenkinsci.plugins.github_branch_source; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; +import static org.junit.Assert.assertEquals; import com.cloudbees.plugins.credentials.Credentials; import com.cloudbees.plugins.credentials.CredentialsScope; diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java index d15500dba..ae2e73153 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java @@ -1,8 +1,10 @@ package org.jenkinsci.plugins.github_branch_source; import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; import static org.jenkinsci.plugins.github_branch_source.Connector.createGitHubBuilder; +import static org.junit.Assert.assertThrows; import com.cloudbees.plugins.credentials.CredentialsProvider; import com.cloudbees.plugins.credentials.CredentialsScope; @@ -12,6 +14,7 @@ import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder; import hudson.logging.LogRecorder; import hudson.logging.LogRecorderManager; +import hudson.model.Label; import hudson.model.ParametersDefinitionProperty; import hudson.model.Result; import hudson.model.Slave; @@ -22,7 +25,7 @@ import java.time.format.DateTimeFormatter; import java.time.temporal.ChronoUnit; import java.util.ArrayList; -import java.util.Collections; +import java.util.Comparator; import java.util.Date; import java.util.List; import java.util.logging.Formatter; @@ -31,17 +34,16 @@ import java.util.logging.SimpleFormatter; import java.util.stream.Collectors; import jenkins.plugins.git.GitSampleRepoRule; -import org.apache.commons.lang3.JavaVersion; -import org.apache.commons.lang3.SystemUtils; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; -import org.junit.Assume; import org.junit.Before; import org.junit.BeforeClass; +import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; -import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.BuildWatcher; +import org.jvnet.hudson.test.LoggerRule; import org.kohsuke.github.GitHub; import org.kohsuke.github.authorization.AuthorizationProvider; @@ -91,6 +93,14 @@ public class GithubAppCredentialsTest extends AbstractGitHubWireMockTest { @Rule public GitSampleRepoRule sampleRepo = new GitSampleRepoRule(); + @Rule public BuildWatcher buildWatcher = new BuildWatcher(); + + // here to aid debugging - we can not use LoggerRule for the test assertion as it only captures + // logs from the controller + @ClassRule + public static LoggerRule loggerRule = + new LoggerRule().record(GitHubAppCredentials.class, Level.FINE); + @BeforeClass public static void setUpJenkins() throws Exception { // Add credential (Must have valid private key for Jwt to work, but App doesn't have to actually @@ -115,8 +125,7 @@ public static void setUpJenkins() throws Exception { store.addCredentials(Domain.global(), appCredentialsNoOwner); // Add agent - agent = r.createOnlineSlave(); - agent.setLabelString("my-agent"); + agent = r.createOnlineSlave(Label.get("my-agent")); // Would use LoggerRule, but need to get agent logs as well LogRecorderManager mgr = r.jenkins.getLog(); @@ -126,6 +135,8 @@ public static void setUpJenkins() throws Exception { logRecorder.getLoggers().add(t); logRecorder.save(); t.enable(); + // but even though we can not capture the logs we want to echo them + r.showAgentLogs(agent, loggerRule); } @Before @@ -311,7 +322,8 @@ public void setUpWireMock() throws Exception { @Test public void testProviderRefresh() throws Exception { - long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; + final long notStaleSeconds = + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; try { appCredentials.setApiUri(githubApi.baseUrl()); @@ -390,12 +402,8 @@ public void testProviderRefresh() throws Exception { @Test public void testAgentRefresh() throws Exception { - // test is really flaky with Java 8 so let's make this Java 11 minimum as we will be Java 11 - // soon - Assume.assumeTrue( - "Test will run only on Java11 minimum", - SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_11)); - long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; + final long notStaleSeconds = + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; try { appCredentials.setApiUri(githubApi.baseUrl()); @@ -445,15 +453,14 @@ public void testAgentRefresh() throws Exception { // Create a pipeline job that points the above repo WorkflowJob job = r.createProject(WorkflowJob.class, "test-creds"); - job.setDefinition(new CpsFlowDefinition(jenkinsfile, false)); + job.setDefinition(new CpsFlowDefinition(jenkinsfile, true)); job.addProperty( new ParametersDefinitionProperty( new StringParameterDefinition("REPO", sampleRepo.toString()))); + WorkflowRun run = job.scheduleBuild2(0).waitForStart(); r.waitUntilNoActivity(); - System.out.println(JenkinsRule.getLog(run)); - List credentialsLog = getOutputLines(); // Verify correct messages from GitHubAppCredential logger indicating token was retrieved on @@ -462,39 +469,44 @@ public void testAgentRefresh() throws Exception { "Creds should cache on master, pass to agent, and refresh agent from master once", credentialsLog, contains( - // (agent log added out of order, see below) - "Generating App Installation Token for app ID 54321", // 1 - "Generating App Installation Token for app ID 54321", // 2 - "Failed to generate new GitHub App Installation Token for app ID 54321: cached token is stale but has not expired", // 3 // node ('my-agent') { - // checkout scm - // checkout scm + // echo 'First Checkout on agent should use cached token passed via remoting' + // git url: REPO, credentialsId: 'myAppCredentialsId' + "Generating App Installation Token for app ID 54321", + // echo 'Multiple checkouts in quick succession should use cached token' + // git .... + // (No token generation) + // sleep + // echo 'Checkout after token is stale refreshes via remoting - fallback due to + // unexpired token' + // git .... "Generating App Installation Token for app ID 54321", // (error forced by wiremock) "Failed to generate new GitHub App Installation Token for app ID 54321: cached token is stale but has not expired", // (error forced by wiremock - failed refresh on the agent) - // "Generating App Installation Token for app ID 54321 on agent", // 1 - "Generating App Installation Token for app ID 54321" // , - // // stop - // // (agent log added out of order) "Keeping cached GitHub App - // Installation Token for - // // app ID 54321 on agent: token is stale but has not expired", // 2 - // // checkout scm - refresh on controller - // "Generating App Installation Token for app ID 54321", - // // sleep - // // checkout scm - // "Generating App Installation Token for app ID 54321", - // // (error forced by wiremock) - // "Failed to update stale GitHub App installation token for app ID - // 54321 - // before sending to agent", - // // "Generating App Installation Token for app ID 54321 on agent", // - // 3 - // "Generating App Installation Token for app ID 54321 for agent", - // // checkout scm - refresh on controller - // "Generating App Installation Token for app ID 54321" - // // checkout scm - // // (No token generation) + "Generating App Installation Token for app ID 54321 on agent", + "Generating App Installation Token for app ID 54321 for agent", + "Failed to generate new GitHub App Installation Token for app ID 54321 on agent: cached token is stale but has not expired", + // echo 'Checkout after error will refresh again on controller - new token expired + // but not stale' + // git .... + "Generating App Installation Token for app ID 54321", + // sleep + // echo 'Checkout after token is stale refreshes via remoting - error on controller + // is not catastrophic' + // git .... + "Generating App Installation Token for app ID 54321", + // (error forced by wiremock) + "Failed to update stale GitHub App installation token for app ID 54321 before sending to agent", + "Generating App Installation Token for app ID 54321 on agent", + "Generating App Installation Token for app ID 54321 for agent", + // echo 'Checkout after error will refresh again on controller - new token expired + // but not stale' + // git .... + "Generating App Installation Token for app ID 54321" + // echo 'Multiple checkouts in quick succession should use cached token' + // git .... + // (No token generation) )); // Check success after output. Output will be more informative if something goes wrong. @@ -541,17 +553,13 @@ public void testPassword() throws Exception { // Test credentials when owner is not set appCredentialsNoOwner.setApiUri(githubApi.baseUrl()); - try { - appCredentialsNoOwner.getPassword(); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException e) { - // ok - assertEquals( - e.getMessage(), - "Found multiple installations for GitHub app ID 54321 but none match credential owner \"\". " - + "Set the right owner in the credential advanced options"); - } - + IllegalArgumentException expected = + assertThrows(IllegalArgumentException.class, () -> appCredentialsNoOwner.getPassword()); + assertThat( + expected.getMessage(), + is( + "Found multiple installations for GitHub app ID 54321 but none match credential owner \"\". " + + "Set the right owner in the credential advanced options")); } finally { GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds; logRecorder.doClear(); @@ -565,8 +573,13 @@ private List getOutputLines() { if (agentLogs != null) { result.addAll(agentLogs); } - Collections.reverse(result); - return result.stream().map(formatter::formatMessage).collect(Collectors.toList()); + + // sort the logs into chronological order + // then just format the message. + return result.stream() + .sorted(Comparator.comparingLong(LogRecord::getMillis)) + .map(formatter::formatMessage) + .collect(Collectors.toList()); } static String printDate(Date dt) { diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/IgnoreDraftPullRequestFilterTraitTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/IgnoreDraftPullRequestFilterTraitTest.java index 7d980e1d0..39c132029 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/IgnoreDraftPullRequestFilterTraitTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/IgnoreDraftPullRequestFilterTraitTest.java @@ -1,5 +1,6 @@ package org.jenkinsci.plugins.github_branch_source; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevisionTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevisionTest.java index 774b23148..3ba6df1c3 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevisionTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevisionTest.java @@ -25,10 +25,14 @@ package org.jenkinsci.plugins.github_branch_source; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import hudson.AbortException; import jenkins.scm.api.SCMHead;