Merge pull request #616 from jtnord/JENKINS-69488

[JENKINS-69488] do not SnapShot GitHubAppCredentials

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.
+ *
+ * <p>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<GitHubAppCredentials> {
+
+  @Override
+  public GitHubAppCredentials snapshot(GitHubAppCredentials credentials) {
+    return credentials;
+  }
+
+  @Override
+  public Class<GitHubAppCredentials> type() {
+    return GitHubAppCredentials.class;
+  }
+}

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.

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;

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;

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;

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<String> 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<String> 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) {

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;
 

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;