Skip to content

Commit 41fbc8b

Browse files
dwnusbaumdwnusbaum
authored
Prevent infinite recursion when migrating secrets to credentials while loading GitLab server configuration (#450)
1 parent 6f19df3 commit 41fbc8b

File tree

4 files changed

+123
-8
lines changed

4 files changed

+123
-8
lines changed

‎src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java

+8-8
Original file line numberDiff line numberDiff line change
@@ -407,17 +407,16 @@ public String getSecretTokenAsPlainText() {
407407
return secretToken;
408408
}
409409

410-
private Object readResolve() {
411-
if (StringUtils.isBlank(webhookSecretCredentialsId) && secretToken != null) {
412-
migrateWebhookSecretCredentials();
413-
}
414-
return this;
415-
}
416-
417410
/**
418411
* Migrate webhook secret token to Jenkins credentials
412+
*
413+
* @return {@code true} if migration occurred, {@code false} otherwise
414+
* @see GitLabServers#migrateWebhookSecretsToCredentials
419415
*/
420-
private void migrateWebhookSecretCredentials() {
416+
boolean migrateWebhookSecretCredentials() {
417+
if (!StringUtils.isBlank(webhookSecretCredentialsId) || secretToken == null) {
418+
return false;
419+
}
421420
final List<StringCredentials> credentials = CredentialsProvider.lookupCredentials(
422421
StringCredentials.class, Jenkins.get(), ACL.SYSTEM, Collections.emptyList());
423422
for (final StringCredentials cred : credentials) {
@@ -438,6 +437,7 @@ private void migrateWebhookSecretCredentials() {
438437
webhookSecretCredentialsId = newCredentials.getId();
439438
}
440439
secretToken = null;
440+
return true;
441441
}
442442

443443
/**

‎src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServers.java

+14
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
import edu.umd.cs.findbugs.annotations.NonNull;
77
import hudson.Extension;
88
import hudson.ExtensionList;
9+
import hudson.init.InitMilestone;
10+
import hudson.init.Initializer;
911
import hudson.model.Descriptor;
1012
import hudson.model.PersistentDescriptor;
1113
import hudson.security.Permission;
@@ -196,4 +198,16 @@ public GitLabServer findServer(@CheckForNull String serverName) {
196198
.findAny()
197199
.orElse(null);
198200
}
201+
202+
@Initializer(after = InitMilestone.EXTENSIONS_AUGMENTED)
203+
public static void migrateWebhookSecretsToCredentials() {
204+
boolean modified = false;
205+
var servers = GitLabServers.get();
206+
for (GitLabServer server : servers.getServers()) {
207+
modified |= server.migrateWebhookSecretCredentials();
208+
}
209+
if (modified) {
210+
servers.save();
211+
}
212+
}
199213
}

‎src/test/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest.java

+78
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
package io.jenkins.plugins.gitlabserverconfig.servers;
2+
3+
import static org.hamcrest.MatcherAssert.assertThat;
4+
import static org.hamcrest.Matchers.containsString;
5+
import static org.hamcrest.Matchers.equalTo;
6+
import static org.hamcrest.Matchers.hasItem;
7+
import static org.hamcrest.Matchers.hasSize;
8+
import static org.hamcrest.Matchers.not;
9+
10+
import com.cloudbees.plugins.credentials.Credentials;
11+
import com.cloudbees.plugins.credentials.CredentialsMatchers;
12+
import com.cloudbees.plugins.credentials.CredentialsProvider;
13+
import com.cloudbees.plugins.credentials.domains.DomainRequirement;
14+
import edu.umd.cs.findbugs.annotations.NonNull;
15+
import edu.umd.cs.findbugs.annotations.Nullable;
16+
import hudson.diagnosis.OldDataMonitor;
17+
import hudson.model.ItemGroup;
18+
import hudson.security.ACL;
19+
import java.util.List;
20+
import java.util.logging.Level;
21+
import jenkins.model.Jenkins;
22+
import org.acegisecurity.Authentication;
23+
import org.jenkinsci.plugins.plaincredentials.StringCredentials;
24+
import org.junit.Rule;
25+
import org.junit.Test;
26+
import org.jvnet.hudson.test.JenkinsRule;
27+
import org.jvnet.hudson.test.LoggerRule;
28+
import org.jvnet.hudson.test.TestExtension;
29+
import org.jvnet.hudson.test.recipes.LocalData;
30+
31+
public class GitLabServersTest {
32+
@Rule
33+
public LoggerRule logger =
34+
new LoggerRule().record(OldDataMonitor.class, Level.FINE).capture(50);
35+
36+
@Rule
37+
public JenkinsRule j = new JenkinsRule();
38+
39+
@LocalData
40+
@Test
41+
public void migrationToCredentials() throws Throwable {
42+
// LocalData creating using the following:
43+
/*
44+
GitLabServer server = new GitLabServer("http://localhost", "my-server", null);
45+
server.setSecretToken(Secret.fromString("s3cr3t!"));
46+
GitLabServers.get().addServer(server);
47+
*/
48+
var server = GitLabServers.get().getServers().stream()
49+
.filter(s -> s.getName().equals("my-server"))
50+
.findFirst()
51+
.orElseThrow();
52+
var credentialsId = server.getWebhookSecretCredentialsId();
53+
var credentials = CredentialsMatchers.filter(
54+
CredentialsProvider.lookupCredentialsInItemGroup(StringCredentials.class, Jenkins.get(), ACL.SYSTEM2),
55+
CredentialsMatchers.withId(credentialsId));
56+
assertThat(credentials, hasSize(1));
57+
assertThat(credentials.get(0).getSecret().getPlainText(), equalTo("s3cr3t!"));
58+
assertThat(
59+
logger.getMessages(), not(hasItem(containsString("Trouble loading " + GitLabServers.class.getName()))));
60+
}
61+
62+
@TestExtension("migrationToCredentials")
63+
public static class CredentialsProviderThatRequiresDescriptorLookup extends CredentialsProvider {
64+
@Override
65+
public <C extends Credentials> List<C> getCredentials(
66+
@NonNull Class<C> type,
67+
@Nullable ItemGroup itemGroup,
68+
@Nullable Authentication authentication,
69+
@NonNull List<DomainRequirement> domainRequirements) {
70+
// Prior to fix, this caused the GitLabServer migration code to recurse infinitely, causing problems when
71+
// starting Jenkins.
72+
// In practice this was caused by a lookup of another descriptor type, but I am using GitLabServers for
73+
// clarity.
74+
Jenkins.get().getDescriptor(GitLabServers.class);
75+
return List.of();
76+
}
77+
}
78+
}

‎src/test/resources/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest/migrationToCredentials/io.jenkins.plugins.gitlabserverconfig.servers.GitLabServers.xml

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?xml version='1.1' encoding='UTF-8'?>
2+
<io.jenkins.plugins.gitlabserverconfig.servers.GitLabServers>
3+
<servers>
4+
<io.jenkins.plugins.gitlabserverconfig.servers.GitLabServer>
5+
<name>default</name>
6+
<serverUrl>https://gitlab.com</serverUrl>
7+
<manageWebHooks>false</manageWebHooks>
8+
<manageSystemHooks>false</manageSystemHooks>
9+
<credentialsId></credentialsId>
10+
<webhookSecretCredentialsId></webhookSecretCredentialsId>
11+
<immediateHookTrigger>false</immediateHookTrigger>
12+
</io.jenkins.plugins.gitlabserverconfig.servers.GitLabServer>
13+
<io.jenkins.plugins.gitlabserverconfig.servers.GitLabServer>
14+
<name>my-server</name>
15+
<serverUrl>http://localhost</serverUrl>
16+
<manageWebHooks>false</manageWebHooks>
17+
<manageSystemHooks>false</manageSystemHooks>
18+
<secretToken>s3cr3t!</secretToken> <!-- Manually modified to be plain text rather than the encrypted value so we do not have to store secret.key as a test resource, which makes security scanners unhappy. -->
19+
<webhookSecretCredentialsId></webhookSecretCredentialsId>
20+
<immediateHookTrigger>false</immediateHookTrigger>
21+
</io.jenkins.plugins.gitlabserverconfig.servers.GitLabServer>
22+
</servers>
23+
</io.jenkins.plugins.gitlabserverconfig.servers.GitLabServers>

0 commit comments

Comments
 (0)