From 0ff4703357fb8b2a6d887e8c199a04d016606081 Mon Sep 17 00:00:00 2001 From: arechavarria Date: Thu, 24 Oct 2024 10:05:34 -0600 Subject: [PATCH] Avoid caching the credential, instead get the credential value whenever it is required through the request filter (#1712) * Avoid caching the credential, instead get the credential value whenever it is required through the request filter (ApiHeaderTokenFilter) * Fix tests * spotless * Update src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCredentialResolver.java Co-authored-by: Kris Stern * Update src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCredentialResolver.java Co-authored-by: Kris Stern * include imports * Update src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCredentialResolver.java Co-authored-by: Kris Stern * Update src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCredentialResolver.java Co-authored-by: Kris Stern --------- Co-authored-by: Aaron Echavarria Co-authored-by: Kris Stern --- .../connection/GitLabConnection.java | 34 ++--------- .../connection/GitlabCredentialResolver.java | 38 ++++++++++++ .../gitlab/api/GitLabClientBuilder.java | 7 ++- .../impl/AutodetectGitLabClientBuilder.java | 9 ++- .../api/impl/AutodetectingGitLabClient.java | 11 ++-- .../api/impl/ResteasyGitLabClientBuilder.java | 60 ++++++++++++++++--- .../impl/AutodetectingGitLabClientTest.java | 11 ++-- .../gitlab/api/impl/TestUtility.java | 10 +++- .../testing/gitlab/rule/GitLabRule.java | 7 ++- 9 files changed, 131 insertions(+), 56 deletions(-) create mode 100644 src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCredentialResolver.java diff --git a/src/main/java/com/dabsquared/gitlabjenkins/connection/GitLabConnection.java b/src/main/java/com/dabsquared/gitlabjenkins/connection/GitLabConnection.java index 99eca4569..3b5546465 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/connection/GitLabConnection.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/connection/GitLabConnection.java @@ -1,10 +1,8 @@ package com.dabsquared.gitlabjenkins.connection; -import static com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentials; import static com.dabsquared.gitlabjenkins.gitlab.api.GitLabClientBuilder.getAllGitLabClientBuilders; import static com.dabsquared.gitlabjenkins.gitlab.api.GitLabClientBuilder.getGitLabClientBuilderById; -import com.cloudbees.plugins.credentials.CredentialsMatchers; import com.cloudbees.plugins.credentials.CredentialsProvider; import com.cloudbees.plugins.credentials.CredentialsScope; import com.cloudbees.plugins.credentials.CredentialsStore; @@ -22,7 +20,6 @@ import hudson.model.AbstractDescribableImpl; import hudson.model.Descriptor; import hudson.model.Item; -import hudson.model.ItemGroup; import hudson.security.ACL; import hudson.util.FormValidation; import hudson.util.ListBoxModel; @@ -36,7 +33,6 @@ import javax.ws.rs.WebApplicationException; import jenkins.model.Jenkins; import org.eclipse.jgit.util.StringUtils; -import org.jenkinsci.plugins.plaincredentials.StringCredentials; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.DoNotUse; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -145,44 +141,26 @@ public int getReadTimeout() { public GitLabClient getClient(Item item, String jobCredentialId) { final String clientId; final String token; + GitlabCredentialResolver credentialResolver = new GitlabCredentialResolver(); if ((jobCredentialId == null) || jobCredentialId.equals(apiTokenId)) { clientId = "global"; - token = getApiToken(apiTokenId, null); + credentialResolver.setCredentialsId(apiTokenId); } else { // Add prefix to credential ID to avoid collision with "global" clientId = "alternative-" + jobCredentialId; - token = getApiToken(jobCredentialId, item); + credentialResolver.setCredentialsId(jobCredentialId); + credentialResolver.setItem(item); } if (!clientCache.containsKey(clientId)) { clientCache.put( clientId, - clientBuilder.buildClient(url, token, ignoreCertificateErrors, connectionTimeout, readTimeout)); + clientBuilder.buildClient( + url, credentialResolver, ignoreCertificateErrors, connectionTimeout, readTimeout)); } return clientCache.get(clientId); } - @Restricted(NoExternalUse.class) - private String getApiToken(String apiTokenId, Item item) { - ItemGroup context = item != null ? item.getParent() : Jenkins.get(); - StandardCredentials credentials = CredentialsMatchers.firstOrNull( - lookupCredentials( - StandardCredentials.class, - context, - ACL.SYSTEM, - URIRequirementBuilder.fromUri(url).build()), - CredentialsMatchers.withId(apiTokenId)); - if (credentials != null) { - if (credentials instanceof GitLabApiToken) { - return ((GitLabApiToken) credentials).getApiToken().getPlainText(); - } - if (credentials instanceof StringCredentials) { - return ((StringCredentials) credentials).getSecret().getPlainText(); - } - } - throw new IllegalStateException("No credentials found for credentialsId: " + apiTokenId); - } - protected GitLabConnection readResolve() { if (connectionTimeout == null || readTimeout == null) { return new GitLabConnection( diff --git a/src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCredentialResolver.java b/src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCredentialResolver.java new file mode 100644 index 000000000..3b94450c5 --- /dev/null +++ b/src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCredentialResolver.java @@ -0,0 +1,38 @@ +package com.dabsquared.gitlabjenkins.connection; + +import hudson.model.Item; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +public class GitlabCredentialResolver { + + private Item item; + private String credentialsId; + + public GitlabCredentialResolver() {} + + public GitlabCredentialResolver(Item item, String credentialsId) { + this.item = item; + this.credentialsId = credentialsId; + } + + @Restricted(NoExternalUse.class) + public Item getItem() { + return item; + } + + @Restricted(NoExternalUse.class) + public void setItem(Item item) { + this.item = item; + } + + @Restricted(NoExternalUse.class) + public String getCredentialsId() { + return credentialsId; + } + + @Restricted(NoExternalUse.class) + public void setCredentialsId(String credentialsId) { + this.credentialsId = credentialsId; + } +} diff --git a/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/GitLabClientBuilder.java b/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/GitLabClientBuilder.java index 258aed0c3..ef32509d3 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/GitLabClientBuilder.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/GitLabClientBuilder.java @@ -2,6 +2,7 @@ import static java.util.Collections.sort; +import com.dabsquared.gitlabjenkins.connection.GitlabCredentialResolver; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionPoint; import java.io.Serializable; @@ -47,7 +48,11 @@ public final String id() { @NonNull public abstract GitLabClient buildClient( - String url, String token, boolean ignoreCertificateErrors, int connectionTimeout, int readTimeout); + String url, + GitlabCredentialResolver credentialResolver, + boolean ignoreCertificateErrors, + int connectionTimeout, + int readTimeout); @Override public final int compareTo(@NonNull GitLabClientBuilder other) { diff --git a/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectGitLabClientBuilder.java b/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectGitLabClientBuilder.java index acf09dd1d..a9e6fc07a 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectGitLabClientBuilder.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectGitLabClientBuilder.java @@ -1,5 +1,6 @@ package com.dabsquared.gitlabjenkins.gitlab.api.impl; +import com.dabsquared.gitlabjenkins.connection.GitlabCredentialResolver; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClient; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClientBuilder; import edu.umd.cs.findbugs.annotations.NonNull; @@ -19,10 +20,14 @@ public AutodetectGitLabClientBuilder() { @Override @NonNull public GitLabClient buildClient( - String url, String token, boolean ignoreCertificateErrors, int connectionTimeout, int readTimeout) { + String url, + GitlabCredentialResolver resolver, + boolean ignoreCertificateErrors, + int connectionTimeout, + int readTimeout) { Collection candidates = new ArrayList<>(getAllGitLabClientBuilders()); candidates.remove(this); return new AutodetectingGitLabClient( - candidates, url, token, ignoreCertificateErrors, connectionTimeout, readTimeout); + candidates, url, resolver, ignoreCertificateErrors, connectionTimeout, readTimeout); } } diff --git a/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectingGitLabClient.java b/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectingGitLabClient.java index 6def6d269..67fcf8199 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectingGitLabClient.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectingGitLabClient.java @@ -1,5 +1,6 @@ package com.dabsquared.gitlabjenkins.gitlab.api.impl; +import com.dabsquared.gitlabjenkins.connection.GitlabCredentialResolver; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClient; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClientBuilder; import com.dabsquared.gitlabjenkins.gitlab.api.model.*; @@ -11,7 +12,7 @@ final class AutodetectingGitLabClient implements GitLabClient { private final Iterable builders; private final String url; - private final String token; + private final GitlabCredentialResolver credentialResolver; private final boolean ignoreCertificateErrors; private final int connectionTimeout; private final int readTimeout; @@ -20,13 +21,13 @@ final class AutodetectingGitLabClient implements GitLabClient { AutodetectingGitLabClient( Iterable builders, String url, - String token, + GitlabCredentialResolver credentialResolver, boolean ignoreCertificateErrors, int connectionTimeout, int readTimeout) { this.builders = builders; this.url = url; - this.token = token; + this.credentialResolver = credentialResolver; this.ignoreCertificateErrors = ignoreCertificateErrors; this.connectionTimeout = connectionTimeout; this.readTimeout = readTimeout; @@ -380,8 +381,8 @@ private GitLabClient autodetectOrDie() { private GitLabClient autodetect() { for (GitLabClientBuilder candidate : builders) { - GitLabClient client = - candidate.buildClient(url, token, ignoreCertificateErrors, connectionTimeout, readTimeout); + GitLabClient client = candidate.buildClient( + url, credentialResolver, ignoreCertificateErrors, connectionTimeout, readTimeout); try { client.getCurrentUser(); return client; diff --git a/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/ResteasyGitLabClientBuilder.java b/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/ResteasyGitLabClientBuilder.java index 23f30821e..e57592be2 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/ResteasyGitLabClientBuilder.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/ResteasyGitLabClientBuilder.java @@ -1,7 +1,13 @@ package com.dabsquared.gitlabjenkins.gitlab.api.impl; +import static com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentials; import static java.net.Proxy.Type.HTTP; +import com.cloudbees.plugins.credentials.CredentialsMatchers; +import com.cloudbees.plugins.credentials.common.StandardCredentials; +import com.cloudbees.plugins.credentials.domains.URIRequirementBuilder; +import com.dabsquared.gitlabjenkins.connection.GitLabApiToken; +import com.dabsquared.gitlabjenkins.connection.GitlabCredentialResolver; import com.dabsquared.gitlabjenkins.gitlab.JacksonConfig; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClient; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClientBuilder; @@ -13,6 +19,9 @@ import hudson.ProxyConfiguration; import hudson.init.InitMilestone; import hudson.init.Initializer; +import hudson.model.Item; +import hudson.model.ItemGroup; +import hudson.security.ACL; import java.io.IOException; import java.io.InputStream; import java.net.InetSocketAddress; @@ -60,6 +69,7 @@ import org.jboss.resteasy.client.jaxrs.engines.factory.ApacheHttpClient4EngineFactory; import org.jboss.resteasy.plugins.providers.JaxrsFormProvider; import org.jboss.resteasy.spi.ResteasyProviderFactory; +import org.jenkinsci.plugins.plaincredentials.StringCredentials; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -89,10 +99,14 @@ public static void setRuntimeDelegate() { @NonNull @Override public final GitLabClient buildClient( - String url, String apiToken, boolean ignoreCertificateErrors, int connectionTimeout, int readTimeout) { + String url, + GitlabCredentialResolver credentialResolver, + boolean ignoreCertificateErrors, + int connectionTimeout, + int readTimeout) { return buildClient( url, - apiToken, + credentialResolver, Jenkins.getActiveInstance().proxy, ignoreCertificateErrors, connectionTimeout, @@ -101,7 +115,7 @@ public final GitLabClient buildClient( private GitLabClient buildClient( String url, - String apiToken, + GitlabCredentialResolver credentialResolver, ProxyConfiguration httpProxyConfig, boolean ignoreCertificateErrors, int connectionTimeout, @@ -142,7 +156,7 @@ private GitLabClient buildClient( .socketTimeout(readTimeout, TimeUnit.SECONDS) .register(new JacksonFeature()) .register(new JacksonConfig()) - .register(new ApiHeaderTokenFilter(apiToken)) + .register(new ApiHeaderTokenFilter(credentialResolver, url)) .register(new LoggingFilter()) .register(new RemoveAcceptEncodingFilter()) .register(new JaxrsFormProvider()) @@ -165,14 +179,44 @@ private String getHost(String url) { @Priority(Priorities.HEADER_DECORATOR) private static class ApiHeaderTokenFilter implements ClientRequestFilter { - private final String gitlabApiToken; + private final GitlabCredentialResolver credentialResolver; + private final String url; + + ApiHeaderTokenFilter(GitlabCredentialResolver credentialResolver, String url) { + this.credentialResolver = credentialResolver; + this.url = url; + } - ApiHeaderTokenFilter(String gitlabApiToken) { - this.gitlabApiToken = gitlabApiToken; + @Restricted(NoExternalUse.class) + private String getApiToken(GitlabCredentialResolver credentialResolver) { + Item item = credentialResolver.getItem(); + ItemGroup context = item != null ? item.getParent() : Jenkins.get(); + StandardCredentials credentials = CredentialsMatchers.firstOrNull( + lookupCredentials( + StandardCredentials.class, + context, + ACL.SYSTEM, + URIRequirementBuilder.fromUri(url).build()), + CredentialsMatchers.withId(credentialResolver.getCredentialsId())); + + if (item != null) { + com.cloudbees.plugins.credentials.CredentialsProvider.track(item, credentials); + } + + if (credentials != null) { + if (credentials instanceof GitLabApiToken) { + return ((GitLabApiToken) credentials).getApiToken().getPlainText(); + } + if (credentials instanceof StringCredentials) { + return ((StringCredentials) credentials).getSecret().getPlainText(); + } + } + throw new IllegalStateException( + "No credentials found for credentialsId: " + credentialResolver.getCredentialsId()); } public void filter(ClientRequestContext requestContext) { - requestContext.getHeaders().putSingle(PRIVATE_TOKEN, gitlabApiToken); + requestContext.getHeaders().putSingle(PRIVATE_TOKEN, getApiToken(credentialResolver)); } } diff --git a/src/test/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectingGitLabClientTest.java b/src/test/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectingGitLabClientTest.java index 38349424a..cf537969f 100644 --- a/src/test/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectingGitLabClientTest.java +++ b/src/test/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/AutodetectingGitLabClientTest.java @@ -1,15 +1,11 @@ package com.dabsquared.gitlabjenkins.gitlab.api.impl; -import static com.dabsquared.gitlabjenkins.gitlab.api.impl.TestUtility.API_TOKEN; -import static com.dabsquared.gitlabjenkins.gitlab.api.impl.TestUtility.addGitLabApiToken; -import static com.dabsquared.gitlabjenkins.gitlab.api.impl.TestUtility.assertApiImpl; -import static com.dabsquared.gitlabjenkins.gitlab.api.impl.TestUtility.responseNotFound; -import static com.dabsquared.gitlabjenkins.gitlab.api.impl.TestUtility.responseOk; -import static com.dabsquared.gitlabjenkins.gitlab.api.impl.TestUtility.versionRequest; +import static com.dabsquared.gitlabjenkins.gitlab.api.impl.TestUtility.*; import static org.junit.Assert.fail; import static org.mockserver.matchers.Times.exactly; import static org.mockserver.matchers.Times.once; +import com.dabsquared.gitlabjenkins.connection.GitlabCredentialResolver; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClientBuilder; import java.io.IOException; import java.util.Arrays; @@ -44,7 +40,8 @@ public void setup() throws IOException { List builders = Arrays.asList(new V3GitLabClientBuilder(), new V4GitLabClientBuilder()); - api = new AutodetectingGitLabClient(builders, gitLabUrl, API_TOKEN, true, 10, 10); + api = new AutodetectingGitLabClient( + builders, gitLabUrl, new GitlabCredentialResolver(null, API_TOKEN_ID), true, 10, 10); v3Request = versionRequest(V3GitLabApiProxy.ID); v4Request = versionRequest(V4GitLabApiProxy.ID); diff --git a/src/test/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/TestUtility.java b/src/test/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/TestUtility.java index 3ff67ac0f..9d3db5d5e 100644 --- a/src/test/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/TestUtility.java +++ b/src/test/java/com/dabsquared/gitlabjenkins/gitlab/api/impl/TestUtility.java @@ -10,6 +10,7 @@ import com.cloudbees.plugins.credentials.CredentialsStore; import com.cloudbees.plugins.credentials.SystemCredentialsProvider; import com.cloudbees.plugins.credentials.domains.Domain; +import com.dabsquared.gitlabjenkins.connection.GitlabCredentialResolver; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClient; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClientBuilder; import hudson.util.Secret; @@ -25,7 +26,7 @@ class TestUtility { static final String API_TOKEN = "secret"; - private static final String API_TOKEN_ID = "apiTokenId"; + static final String API_TOKEN_ID = "apiTokenId"; private static final boolean IGNORE_CERTIFICATE_ERRORS = true; private static final int CONNECTION_TIMEOUT = 10; private static final int READ_TIMEOUT = 10; @@ -65,7 +66,12 @@ private static HttpResponse responseWithStatus(Response.Status status) { } static GitLabClient buildClientWithDefaults(GitLabClientBuilder clientBuilder, String url) { - return clientBuilder.buildClient(url, API_TOKEN, IGNORE_CERTIFICATE_ERRORS, CONNECTION_TIMEOUT, READ_TIMEOUT); + return clientBuilder.buildClient( + url, + new GitlabCredentialResolver(null, API_TOKEN_ID), + IGNORE_CERTIFICATE_ERRORS, + CONNECTION_TIMEOUT, + READ_TIMEOUT); } static void assertApiImpl(GitLabClient client, Class apiImplClass) throws Exception { diff --git a/src/test/java/com/dabsquared/gitlabjenkins/testing/gitlab/rule/GitLabRule.java b/src/test/java/com/dabsquared/gitlabjenkins/testing/gitlab/rule/GitLabRule.java index a0621cc2a..573bfbcf1 100644 --- a/src/test/java/com/dabsquared/gitlabjenkins/testing/gitlab/rule/GitLabRule.java +++ b/src/test/java/com/dabsquared/gitlabjenkins/testing/gitlab/rule/GitLabRule.java @@ -8,6 +8,7 @@ import com.dabsquared.gitlabjenkins.connection.GitLabConnection; import com.dabsquared.gitlabjenkins.connection.GitLabConnectionConfig; import com.dabsquared.gitlabjenkins.connection.GitLabConnectionProperty; +import com.dabsquared.gitlabjenkins.connection.GitlabCredentialResolver; import com.dabsquared.gitlabjenkins.gitlab.api.GitLabClient; import com.dabsquared.gitlabjenkins.gitlab.api.impl.V3GitLabClientBuilder; import com.dabsquared.gitlabjenkins.gitlab.api.model.MergeRequest; @@ -90,7 +91,7 @@ public GitLabConnectionProperty createGitLabConnectionProperty() throws IOExcept CredentialsScope.SYSTEM, API_TOKEN_ID, "GitLab API Token", - Secret.fromString(getApiToken()))); + Secret.fromString(getApiToken().getCredentialsId()))); } } @@ -128,7 +129,7 @@ private void cleanup() { } } - private String getApiToken() { + private GitlabCredentialResolver getApiToken() { try { Class.forName("org.postgresql.Driver"); try (Connection connection = DriverManager.getConnection( @@ -137,7 +138,7 @@ private String getApiToken() { .createStatement() .executeQuery("SELECT authentication_token FROM users WHERE username = 'root'"); resultSet.next(); - return resultSet.getString("authentication_token"); + return new GitlabCredentialResolver(null, resultSet.getString("authentication_token")); } } catch (ClassNotFoundException | SQLException e) { throw new RuntimeException(e);