Skip to content

[JENKINS-69488] do not SnapShot GitHubAppCredentials #616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 6, 2022
Merged

[JENKINS-69488] do not SnapShot GitHubAppCredentials #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6b4afa5
[JENKINS-69488] do not SnapShot GitHubAppCredentials
jtnord Sep 1, 2022
7ffae8c
do not extend assert
jtnord Sep 2, 2022
e655ec4
Restore working unit test
jtnord Sep 2, 2022
751a870
sort all logs by chronological order
jtnord Sep 2, 2022
f961f1d
remove stray logging left over from debug
jtnord Sep 2, 2022
a45d053
Update src/test/java/org/jenkinsci/plugins/github_branch_source/Githu…
jtnord Sep 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions ...in/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsSnapshotTaker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
}
3 changes: 1 addition & 2 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/AbstractGitHubWireMockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystemTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
129 changes: 71 additions & 58 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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
Expand All @@ -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();
Expand All @@ -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);
Comment on lines +138 to +139
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could probably rework showAgentLogs to mirror messages to the controller with a prefix or whatever.

Copy link
Member Author

@jtnord jtnord Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand.
Do you mean set a formatter here that prepends "$agentName"?

In which case something for JTH outside of the scope here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something for JTH

Right.

outside of the scope here

Well, except insofar as it would be handy to pick up here.

Copy link
Member Author

@jtnord jtnord Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logs already contain the agent name., slave0 in the below example

61.145 [test-creds #1] using credential myAppCredentialsId
61.145 [test-creds #1] Fetching changes from the remote Git repository
61.206 [slave0] [id=211]	FINE	o.j.p.g.GitHubAppCredentials$DelegatingGitHubAppCredentials#getPassword: Generating App Installation Token for app ID 54321 on agent
61.207 [id=102]	FINE	o.j.p.g.GitHubAppCredentials$DelegatingGitHubAppCredentials$GetToken#call: Generating App Installation Token for app ID 54321 for agent
61.403 [test-creds #1] Checking out Revision a9cd113a37fe363bc18dc40ac3bb1024bbc62842 (refs/remotes/origin/master)
61.558 [test-creds #1] Commit message: "init"
61.578 [id=181]	FINE	o.j.p.g.GitHubAppCredentials#getToken: Generating App Installation Token for app ID 54321

So I'm no longer clear what is being asked here @jglick

showAgentLogs -> publish

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it is the comment that is missleading. it is in reference to the comment on the LoggerRule

github-branch-source-plugin/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java

Lines 98 to 102 in a45d053

// 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);

}

@Before
Expand Down Expand Up @@ -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;
Comment on lines -314 to +326
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No-op?

Copy link
Member Author

@jtnord jtnord Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used in the finally block which is untouched so not in the diff.

github-branch-source-plugin/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java

Lines 397 to 400 in f961f1d

} finally {
GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds;
logRecorder.doClear();
}

Made it final to avoid it being changed accidentally..

Probably should use a FlagRule but probably best left for a larger cleanup

try {
appCredentials.setApiUri(githubApi.baseUrl());

Expand Down Expand Up @@ -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;
Comment on lines -398 to +406
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

try {
appCredentials.setApiUri(githubApi.baseUrl());

Expand Down Expand Up @@ -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));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced by the BuildWatcher.


List<String> credentialsLog = getOutputLines();

// Verify correct messages from GitHubAppCredential logger indicating token was retrieved on
Expand All @@ -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 ....
Comment on lines -465 to +482
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a diff link to the pre-#609 state for ease of review?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jtnord/github-branch-source-plugin@a14edd2...JENKINS-69488#diff-03b8ddb1abb4051c64c1e2898e14bf79ff304a1fce3442fa3e789f0af96cdeab

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I guess the non-obvious parts of the test diff are due to the addition of chronological sorting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. those where already commented inline (but the message may have changed slightly

"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.
Expand Down Expand Up @@ -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();
Expand All @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions ...ava/org/jenkinsci/plugins/github_branch_source/IgnoreDraftPullRequestFilterTraitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;

Expand Down
4 changes: 4 additions & 0 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevisionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down