From 41fbc8b2b7ae76e99f2ffd4dd7cdfdc2e126506e Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Tue, 15 Oct 2024 19:30:38 -0400 Subject: [PATCH] Prevent infinite recursion when migrating secrets to credentials while loading GitLab server configuration (#450) --- .../servers/GitLabServer.java | 16 ++-- .../servers/GitLabServers.java | 14 ++++ .../servers/GitLabServersTest.java | 78 +++++++++++++++++++ ...tlabserverconfig.servers.GitLabServers.xml | 23 ++++++ 4 files changed, 123 insertions(+), 8 deletions(-) create mode 100644 src/test/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest.java create mode 100644 src/test/resources/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest/migrationToCredentials/io.jenkins.plugins.gitlabserverconfig.servers.GitLabServers.xml diff --git a/src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java b/src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java index 8329e304..1b21e8bd 100644 --- a/src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java +++ b/src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServer.java @@ -407,17 +407,16 @@ public String getSecretTokenAsPlainText() { return secretToken; } - private Object readResolve() { - if (StringUtils.isBlank(webhookSecretCredentialsId) && secretToken != null) { - migrateWebhookSecretCredentials(); - } - return this; - } - /** * Migrate webhook secret token to Jenkins credentials + * + * @return {@code true} if migration occurred, {@code false} otherwise + * @see GitLabServers#migrateWebhookSecretsToCredentials */ - private void migrateWebhookSecretCredentials() { + boolean migrateWebhookSecretCredentials() { + if (!StringUtils.isBlank(webhookSecretCredentialsId) || secretToken == null) { + return false; + } final List credentials = CredentialsProvider.lookupCredentials( StringCredentials.class, Jenkins.get(), ACL.SYSTEM, Collections.emptyList()); for (final StringCredentials cred : credentials) { @@ -438,6 +437,7 @@ private void migrateWebhookSecretCredentials() { webhookSecretCredentialsId = newCredentials.getId(); } secretToken = null; + return true; } /** diff --git a/src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServers.java b/src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServers.java index 4ded5313..34a6949f 100644 --- a/src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServers.java +++ b/src/main/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServers.java @@ -6,6 +6,8 @@ import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; import hudson.ExtensionList; +import hudson.init.InitMilestone; +import hudson.init.Initializer; import hudson.model.Descriptor; import hudson.model.PersistentDescriptor; import hudson.security.Permission; @@ -196,4 +198,16 @@ public GitLabServer findServer(@CheckForNull String serverName) { .findAny() .orElse(null); } + + @Initializer(after = InitMilestone.EXTENSIONS_AUGMENTED) + public static void migrateWebhookSecretsToCredentials() { + boolean modified = false; + var servers = GitLabServers.get(); + for (GitLabServer server : servers.getServers()) { + modified |= server.migrateWebhookSecretCredentials(); + } + if (modified) { + servers.save(); + } + } } diff --git a/src/test/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest.java b/src/test/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest.java new file mode 100644 index 00000000..61a7e171 --- /dev/null +++ b/src/test/java/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest.java @@ -0,0 +1,78 @@ +package io.jenkins.plugins.gitlabserverconfig.servers; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.not; + +import com.cloudbees.plugins.credentials.Credentials; +import com.cloudbees.plugins.credentials.CredentialsMatchers; +import com.cloudbees.plugins.credentials.CredentialsProvider; +import com.cloudbees.plugins.credentials.domains.DomainRequirement; +import edu.umd.cs.findbugs.annotations.NonNull; +import edu.umd.cs.findbugs.annotations.Nullable; +import hudson.diagnosis.OldDataMonitor; +import hudson.model.ItemGroup; +import hudson.security.ACL; +import java.util.List; +import java.util.logging.Level; +import jenkins.model.Jenkins; +import org.acegisecurity.Authentication; +import org.jenkinsci.plugins.plaincredentials.StringCredentials; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.LoggerRule; +import org.jvnet.hudson.test.TestExtension; +import org.jvnet.hudson.test.recipes.LocalData; + +public class GitLabServersTest { + @Rule + public LoggerRule logger = + new LoggerRule().record(OldDataMonitor.class, Level.FINE).capture(50); + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @LocalData + @Test + public void migrationToCredentials() throws Throwable { + // LocalData creating using the following: + /* + GitLabServer server = new GitLabServer("http://localhost", "my-server", null); + server.setSecretToken(Secret.fromString("s3cr3t!")); + GitLabServers.get().addServer(server); + */ + var server = GitLabServers.get().getServers().stream() + .filter(s -> s.getName().equals("my-server")) + .findFirst() + .orElseThrow(); + var credentialsId = server.getWebhookSecretCredentialsId(); + var credentials = CredentialsMatchers.filter( + CredentialsProvider.lookupCredentialsInItemGroup(StringCredentials.class, Jenkins.get(), ACL.SYSTEM2), + CredentialsMatchers.withId(credentialsId)); + assertThat(credentials, hasSize(1)); + assertThat(credentials.get(0).getSecret().getPlainText(), equalTo("s3cr3t!")); + assertThat( + logger.getMessages(), not(hasItem(containsString("Trouble loading " + GitLabServers.class.getName())))); + } + + @TestExtension("migrationToCredentials") + public static class CredentialsProviderThatRequiresDescriptorLookup extends CredentialsProvider { + @Override + public List getCredentials( + @NonNull Class type, + @Nullable ItemGroup itemGroup, + @Nullable Authentication authentication, + @NonNull List domainRequirements) { + // Prior to fix, this caused the GitLabServer migration code to recurse infinitely, causing problems when + // starting Jenkins. + // In practice this was caused by a lookup of another descriptor type, but I am using GitLabServers for + // clarity. + Jenkins.get().getDescriptor(GitLabServers.class); + return List.of(); + } + } +} diff --git a/src/test/resources/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest/migrationToCredentials/io.jenkins.plugins.gitlabserverconfig.servers.GitLabServers.xml b/src/test/resources/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest/migrationToCredentials/io.jenkins.plugins.gitlabserverconfig.servers.GitLabServers.xml new file mode 100644 index 00000000..a94d7b3f --- /dev/null +++ b/src/test/resources/io/jenkins/plugins/gitlabserverconfig/servers/GitLabServersTest/migrationToCredentials/io.jenkins.plugins.gitlabserverconfig.servers.GitLabServers.xml @@ -0,0 +1,23 @@ + + + + + default + https://gitlab.com + false + false + + + false + + + my-server + http://localhost + false + false + s3cr3t! + + false + + + \ No newline at end of file