From 415a6cc69d1012b6f3a89f2393bf314fe7530c6a Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 6 Jan 2026 11:25:12 -0500 Subject: [PATCH 01/10] Sketch --- .../credentials/CredentialsMatchers.java | 1 + .../credentials/CredentialsProvider.java | 4 ++ .../plugins/credentials/ByIdTest.java | 58 +++++++++++++++++++ 3 files changed, 63 insertions(+) create mode 100644 src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java index 6974c9e8..3e001cd0 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java @@ -129,6 +129,7 @@ public static CredentialsMatcher instanceOf(@NonNull Class clazz) { * @param id the {@link com.cloudbees.plugins.credentials.common.IdCredentials#getId()} to match. * @return a matcher that matches {@link com.cloudbees.plugins.credentials.common.IdCredentials} with the * supplied {@link com.cloudbees.plugins.credentials.common.IdCredentials#getId()} + * @see TODO link to new CredentialsProvider methods */ @NonNull public static CredentialsMatcher withId(@NonNull String id) { diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java index 221172e9..44149db5 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java @@ -557,12 +557,14 @@ public static List lookupCredentialsInItem(@NonNull C Set ids = new HashSet<>(); for (CredentialsProvider provider : all()) { if (provider.isEnabled(item) && provider.isApplicable(type)) { + LOGGER.fine(() -> "checking " + provider + " for " + type); try { for (C c: provider.getCredentialsInItem(type, item, authentication, domainRequirements)) { if (!(c instanceof IdCredentials) || ids.add(((IdCredentials) c).getId())) { // if IdCredentials, only add if we haven't added already // if not IdCredentials, always add result.add(c); + LOGGER.fine(() -> "got " + c + " from " + provider); } } } catch (NoClassDefFoundError e) { @@ -1023,6 +1025,8 @@ private static C contextualize(@NonNull Class type, @ return null; } + // TODO static + abstract variants taking credentialsId but Item or ItemGroup rather than Run + /** * Returns the list of all {@link CredentialsProvider}. * diff --git a/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java b/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java new file mode 100644 index 00000000..62544efe --- /dev/null +++ b/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java @@ -0,0 +1,58 @@ +/* + * The MIT License + * + * Copyright 2026 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package com.cloudbees.plugins.credentials; + +import com.cloudbees.plugins.credentials.common.IdCredentials; +import java.util.ArrayList; +import java.util.Collections; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.instanceOf; +import org.junit.jupiter.api.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.TestExtension; +import org.jvnet.hudson.test.junit.jupiter.WithJenkins; + +/** + * Exercises {@link CredentialsProvider} searches for {@link IdCredentials}. + */ +@WithJenkins +final class ByIdTest { + + @Test void xxx(JenkinsRule r) { + CredentialsProvider.all().forEach(System.out::println); + assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 1), instanceOf(LazyProvider1.class)); + } + + @TestExtension public static final class LazyProvider1 extends CredentialsProvider { + + // @TestExtension lacks ordinal, and CredentialsProvider.getDisplayName uses Class.simpleName, + // so to sort CredentialsProvider.all we must use a special nested class name or override this: + @Override public String getDisplayName() { + return "ZZZ Lazy Provider #1"; + } + + } + +} From e315e32d962c986ed61cfa157133ec8a6572d872 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 6 Jan 2026 14:10:59 -0500 Subject: [PATCH 02/10] Proposal by Claude --- .../credentials/CredentialsMatchers.java | 3 +- .../credentials/CredentialsProvider.java | 188 +++++++++++-- .../plugins/credentials/ByIdTest.java | 266 +++++++++++++++++- 3 files changed, 428 insertions(+), 29 deletions(-) diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java index 3e001cd0..e4cdff2c 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java @@ -129,7 +129,8 @@ public static CredentialsMatcher instanceOf(@NonNull Class clazz) { * @param id the {@link com.cloudbees.plugins.credentials.common.IdCredentials#getId()} to match. * @return a matcher that matches {@link com.cloudbees.plugins.credentials.common.IdCredentials} with the * supplied {@link com.cloudbees.plugins.credentials.common.IdCredentials#getId()} - * @see TODO link to new CredentialsProvider methods + * @see CredentialsProvider#findCredentialById(String, Class, Item, org.springframework.security.core.Authentication, java.util.List) + * @see CredentialsProvider#findCredentialById(String, Class, hudson.model.ItemGroup, org.springframework.security.core.Authentication, java.util.List) */ @NonNull public static CredentialsMatcher withId(@NonNull String id) { diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java index 44149db5..4af4f915 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java @@ -846,6 +846,93 @@ public static C snapshot(Class clazz, C credential) { } } + /** + * Returns the credential with the specified ID which is available to the specified {@link Authentication} + * for use by the {@link Item}s in the specified {@link ItemGroup}. + * + * @param id the ID of the credential to find. + * @param type the type of credential to find. + * @param itemGroup the item group (if {@code null} assume {@link Jenkins#get()}). + * @param authentication the authentication (if {@code null} assume {@link ACL#SYSTEM2}). + * @param domainRequirements the credential domains to match (if {@code null} assume empty list). + * @param the credentials type. + * @return the credential or {@code null} if no credential with the specified ID is found. + * @since TODO + */ + @CheckForNull + public static C findCredentialById(@NonNull String id, + @NonNull Class type, + @Nullable ItemGroup itemGroup, + @Nullable Authentication authentication, + @Nullable List domainRequirements) { + Objects.requireNonNull(id); + Objects.requireNonNull(type); + itemGroup = itemGroup == null ? Jenkins.get() : itemGroup; + authentication = authentication == null ? ACL.SYSTEM2 : authentication; + domainRequirements = domainRequirements == null ? Collections.emptyList() : domainRequirements; + + for (CredentialsProvider provider : all()) { + if (provider.isEnabled(itemGroup) && provider.isApplicable(type)) { + try { + C credential = provider.getCredentialById(id, type, itemGroup, authentication, domainRequirements); + if (credential != null) { + return credential; + } + } catch (NoClassDefFoundError e) { + LOGGER.log(Level.FINE, "Could not retrieve provider credentials from " + provider + + " likely due to missing optional dependency", e); + } + } + } + return null; + } + + /** + * Returns the credential with the specified ID which is available to the specified {@link Authentication} + * for use by the specified {@link Item}. + * + * @param id the ID of the credential to find. + * @param type the type of credential to find. + * @param item the item (if {@code null} assume {@link Jenkins#get()}). + * @param authentication the authentication (if {@code null} assume {@link ACL#SYSTEM2}). + * @param domainRequirements the credential domains to match (if {@code null} assume empty list). + * @param the credentials type. + * @return the credential or {@code null} if no credential with the specified ID is found. + * @since TODO + */ + @CheckForNull + public static C findCredentialById(@NonNull String id, + @NonNull Class type, + @Nullable Item item, + @Nullable Authentication authentication, + @Nullable List domainRequirements) { + Objects.requireNonNull(id); + Objects.requireNonNull(type); + if (item == null) { + return findCredentialById(id, type, Jenkins.get(), authentication, domainRequirements); + } + if (item instanceof ItemGroup) { + return findCredentialById(id, type, (ItemGroup) item, authentication, domainRequirements); + } + authentication = authentication == null ? ACL.SYSTEM2 : authentication; + domainRequirements = domainRequirements == null ? Collections.emptyList() : domainRequirements; + + for (CredentialsProvider provider : all()) { + if (provider.isEnabled(item) && provider.isApplicable(type)) { + try { + C credential = provider.getCredentialById(id, type, item, authentication, domainRequirements); + if (credential != null) { + return credential; + } + } catch (NoClassDefFoundError e) { + LOGGER.log(Level.FINE, "Could not retrieve provider credentials from " + provider + + " likely due to missing optional dependency", e); + } + } + } + return null; + } + /** * A common requirement for plugins is to resolve a specific credential by id in the context of a specific run. * Given that the credential itself could be resulting from a build parameter expression and the complexities of @@ -927,55 +1014,46 @@ public static C findCredentialById(@NonNull String id, // as you would have no way to configure it Authentication runAuth = CredentialsProvider.getDefaultAuthenticationOf2(run.getParent()); // we want the credentials available to the user the build is running as - List candidates = new ArrayList<>( - CredentialsProvider.lookupCredentialsInItem(type, run.getParent(), runAuth, domainRequirements) - ); - // if that user can use the item's credentials, add those in too - if (runAuth != ACL.SYSTEM2 && run.hasPermission2(runAuth, CredentialsProvider.USE_ITEM)) { - candidates.addAll( - CredentialsProvider.lookupCredentialsInItem(type, run.getParent(), ACL.SYSTEM2, domainRequirements) - ); + C credential = findCredentialById(id, type, run.getParent(), runAuth, domainRequirements); + // if that user can use the item's credentials, try those too + if (credential == null && runAuth != ACL.SYSTEM2 && run.hasPermission2(runAuth, CredentialsProvider.USE_ITEM)) { + credential = findCredentialById(id, type, run.getParent(), ACL.SYSTEM2, domainRequirements); } // TODO should this be calling track? - return contextualize(type, CredentialsMatchers.firstOrNull(candidates, CredentialsMatchers.withId(id)), run); + return contextualize(type, credential, run); } // this is a parameter and not the default value, we need to determine who triggered the build final Map.Entry> triggeredBy = triggeredBy(run); final Authentication a = triggeredBy == null ? Jenkins.ANONYMOUS2 : triggeredBy.getKey().impersonate2(); - List candidates = new ArrayList<>(); + C result = null; if (triggeredBy != null && run == triggeredBy.getValue() && run.hasPermission2(a, CredentialsProvider.USE_OWN)) { // the user triggered this job directly and they are allowed to supply their own credentials, so - // add those into the list. We do not want to follow the chain for the user's authentication + // search those first. We do not want to follow the chain for the user's authentication // though, as there is no way to limit how far the passed-through parameters can be used - candidates.addAll(CredentialsProvider.lookupCredentialsInItem(type, run.getParent(), a, domainRequirements)); + result = findCredentialById(id, type, run.getParent(), a, domainRequirements); } - if (inputUserId != null) { + if (result == null && inputUserId != null) { final User inputUser = User.getById(inputUserId, false); if (inputUser != null) { final Authentication inputAuth = inputUser.impersonate2(); if (run.hasPermission2(inputAuth, CredentialsProvider.USE_OWN)) { - candidates.addAll(CredentialsProvider.lookupCredentialsInItem(type, run.getParent(), inputAuth, domainRequirements)); + result = findCredentialById(id, type, run.getParent(), inputAuth, domainRequirements); } } } - if (run.hasPermission2(a, CredentialsProvider.USE_ITEM)) { - // the triggering user is allowed to use the item's credentials, so add those into the list + if (result == null && run.hasPermission2(a, CredentialsProvider.USE_ITEM)) { + // the triggering user is allowed to use the item's credentials, so search those // we use the default authentication of the job as those are the only ones that can be configured // if a different strategy is in play it doesn't make sense to consider the run-time authentication // as you would have no way to configure it Authentication runAuth = CredentialsProvider.getDefaultAuthenticationOf2(run.getParent()); // we want the credentials available to the user the build is running as - candidates.addAll( - CredentialsProvider.lookupCredentialsInItem(type, run.getParent(), runAuth, domainRequirements) - ); - // if that user can use the item's credentials, add those in too - if (runAuth != ACL.SYSTEM2 && run.hasPermission2(runAuth, CredentialsProvider.USE_ITEM)) { - candidates.addAll( - CredentialsProvider.lookupCredentialsInItem(type, run.getParent(), ACL.SYSTEM2, domainRequirements) - ); + result = findCredentialById(id, type, run.getParent(), runAuth, domainRequirements); + // if that user can use the item's credentials, try those too + if (result == null && runAuth != ACL.SYSTEM2 && run.hasPermission2(runAuth, CredentialsProvider.USE_ITEM)) { + result = findCredentialById(id, type, run.getParent(), ACL.SYSTEM2, domainRequirements); } } - C result = CredentialsMatchers.firstOrNull(candidates, CredentialsMatchers.withId(id)); // if the run has not completed yet then we can safely assume that the credential is being used for this run // so we will track it's usage. We use isLogUpdated() as it could be used during post production if (run.isLogUpdated()) { @@ -1212,6 +1290,66 @@ public ListBoxModel getCredentialIds(@NonNull Class return getCredentialIdsInItemGroup(type, itemGroup, authentication == null ? null : authentication.toSpring(), domainRequirements, matcher); } + /** + * Returns the credential with the specified ID provided by this provider which is available to the + * specified {@link Authentication} for items in the specified {@link ItemGroup} and is appropriate for the + * specified {@link DomainRequirement}s. + * NOTE: implementations are recommended to override this method if the actual secret information + * is being stored external from Jenkins and looking up by ID can avoid loading credentials that do not match. + * The default implementation uses {@link #getCredentialsInItemGroup(Class, ItemGroup, Authentication, List)} + * and filters the results. + * + * @param the credentials type. + * @param id the ID of the credential to find. + * @param type the type of credentials to return. + * @param itemGroup the item group. + * @param authentication the authentication. + * @param domainRequirements the credential domain to match. + * @return the credential or {@code null} if no credential with the specified ID is found. + * @since TODO + */ + @CheckForNull + public C getCredentialById(@NonNull String id, + @NonNull Class type, + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { + return CredentialsMatchers.firstOrNull( + getCredentialsInItemGroup(type, itemGroup, authentication, domainRequirements), + CredentialsMatchers.withId(id) + ); + } + + /** + * Returns the credential with the specified ID provided by this provider which is available to the + * specified {@link Authentication} for the specified {@link Item} and is appropriate for the + * specified {@link DomainRequirement}s. + * NOTE: implementations are recommended to override this method if the actual secret information + * is being stored external from Jenkins and looking up by ID can avoid loading credentials that do not match. + * The default implementation uses {@link #getCredentialsInItem(Class, Item, Authentication, List)} + * and filters the results. + * + * @param the credentials type. + * @param id the ID of the credential to find. + * @param type the type of credentials to return. + * @param item the item. + * @param authentication the authentication. + * @param domainRequirements the credential domain to match. + * @return the credential or {@code null} if no credential with the specified ID is found. + * @since TODO + */ + @CheckForNull + public C getCredentialById(@NonNull String id, + @NonNull Class type, + @NonNull Item item, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { + return CredentialsMatchers.firstOrNull( + getCredentialsInItem(type, item, authentication, domainRequirements), + CredentialsMatchers.withId(id) + ); + } + /** * Returns a {@link ListBoxModel} of the credentials provided by this provider which are available to the * specified {@link Authentication} for items in the specified {@link ItemGroup} and are appropriate for the diff --git a/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java b/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java index 62544efe..d0a7e54d 100644 --- a/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java +++ b/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java @@ -25,10 +25,23 @@ package com.cloudbees.plugins.credentials; import com.cloudbees.plugins.credentials.common.IdCredentials; -import java.util.ArrayList; +import com.cloudbees.plugins.credentials.domains.DomainRequirement; +import edu.umd.cs.findbugs.annotations.CheckForNull; +import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.model.Item; +import hudson.model.ItemGroup; +import hudson.security.ACL; import java.util.Collections; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import jenkins.model.Jenkins; +import org.springframework.security.core.Authentication; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.jupiter.api.Assertions.assertEquals; import org.junit.jupiter.api.Test; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.TestExtension; @@ -40,9 +53,128 @@ @WithJenkins final class ByIdTest { - @Test void xxx(JenkinsRule r) { + @Test void providerOrder(JenkinsRule r) { CredentialsProvider.all().forEach(System.out::println); - assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 1), instanceOf(LazyProvider1.class)); + // Verify that our test providers are at the end of the list (they start with "ZZZ") + assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 1), instanceOf(LazyProvider3.class)); + assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 2), instanceOf(LazyProvider2.class)); + assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 3), instanceOf(LazyProvider1.class)); + } + + @Test void lazyEvaluationWithEarlyMatch(JenkinsRule r) throws Exception { + // Add a credential to the system store (early provider) + SystemCredentialsProvider.getInstance().getCredentials().add( + new DummyIdCredentials(CredentialsScope.GLOBAL, "early-cred", "Early Credential") + ); + SystemCredentialsProvider.getInstance().save(); + + // Add credentials to lazy providers + LazyProvider2.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy2-cred", "Lazy2 Credential")); + LazyProvider3.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy3-cred", "Lazy3 Credential")); + + // Reset counters + LazyProvider2.resetCounters(); + LazyProvider3.resetCounters(); + + // Search for early credential + IdCredentials result = CredentialsProvider.findCredentialById( + "early-cred", + IdCredentials.class, + (ItemGroup) null, + ACL.SYSTEM2, + Collections.emptyList() + ); + + assertThat(result, notNullValue()); + assertThat(result.getId(), is("early-cred")); + + // Verify lazy providers were not consulted + assertEquals(0, LazyProvider2.getByIdCalls.get(), "LazyProvider2.getCredentialById should not be called"); + assertEquals(0, LazyProvider2.listCalls.get(), "LazyProvider2.getCredentialsInItemGroup should not be called"); + assertEquals(0, LazyProvider3.listCalls.get(), "LazyProvider3.getCredentialsInItemGroup should not be called"); + } + + @Test void lazyEvaluationWithOptimizedProvider(JenkinsRule r) { + // Add credentials to lazy providers + LazyProvider2.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy2-cred", "Lazy2 Credential")); + LazyProvider3.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy3-cred", "Lazy3 Credential")); + + // Reset counters + LazyProvider2.resetCounters(); + LazyProvider3.resetCounters(); + + // Search for lazy2 credential + IdCredentials result = CredentialsProvider.findCredentialById( + "lazy2-cred", + IdCredentials.class, + (ItemGroup) null, + ACL.SYSTEM2, + Collections.emptyList() + ); + + assertThat(result, notNullValue()); + assertThat(result.getId(), is("lazy2-cred")); + + // Verify LazyProvider2's optimized method was called + assertEquals(1, LazyProvider2.getByIdCalls.get(), "LazyProvider2.getCredentialById should be called once"); + // Verify LazyProvider2's list method was NOT called (optimized path) + assertEquals(0, LazyProvider2.listCalls.get(), "LazyProvider2.getCredentialsInItemGroup should not be called"); + // Verify LazyProvider3 was not consulted at all + assertEquals(0, LazyProvider3.listCalls.get(), "LazyProvider3.getCredentialsInItemGroup should not be called"); + } + + @Test void lazyEvaluationWithUnoptimizedProvider(JenkinsRule r) { + // Add credentials to lazy providers + LazyProvider2.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy2-cred", "Lazy2 Credential")); + LazyProvider3.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy3-cred", "Lazy3 Credential")); + + // Reset counters + LazyProvider2.resetCounters(); + LazyProvider3.resetCounters(); + + // Search for lazy3 credential + IdCredentials result = CredentialsProvider.findCredentialById( + "lazy3-cred", + IdCredentials.class, + (ItemGroup) null, + ACL.SYSTEM2, + Collections.emptyList() + ); + + assertThat(result, notNullValue()); + assertThat(result.getId(), is("lazy3-cred")); + + // Verify LazyProvider2 was consulted but didn't find it + assertEquals(1, LazyProvider2.getByIdCalls.get(), "LazyProvider2.getCredentialById should be called once"); + assertEquals(0, LazyProvider2.listCalls.get(), "LazyProvider2.getCredentialsInItemGroup should not be called (uses optimized path)"); + // Verify LazyProvider3's list method WAS called (unoptimized, uses default implementation) + assertEquals(1, LazyProvider3.listCalls.get(), "LazyProvider3.getCredentialsInItemGroup should be called once"); + } + + @Test void lazyEvaluationWithNonexistentCredential(JenkinsRule r) { + // Add credentials to lazy providers + LazyProvider2.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy2-cred", "Lazy2 Credential")); + LazyProvider3.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy3-cred", "Lazy3 Credential")); + + // Reset counters + LazyProvider2.resetCounters(); + LazyProvider3.resetCounters(); + + // Search for nonexistent credential + IdCredentials result = CredentialsProvider.findCredentialById( + "nonexistent", + IdCredentials.class, + (ItemGroup) null, + ACL.SYSTEM2, + Collections.emptyList() + ); + + assertThat(result, nullValue()); + + // Verify both lazy providers were consulted + assertEquals(1, LazyProvider2.getByIdCalls.get(), "LazyProvider2.getCredentialById should be called once"); + assertEquals(0, LazyProvider2.listCalls.get(), "LazyProvider2.getCredentialsInItemGroup should not be called (uses optimized path)"); + assertEquals(1, LazyProvider3.listCalls.get(), "LazyProvider3.getCredentialsInItemGroup should be called once"); } @TestExtension public static final class LazyProvider1 extends CredentialsProvider { @@ -53,6 +185,134 @@ final class ByIdTest { return "ZZZ Lazy Provider #1"; } + @Override + @NonNull + public List getCredentialsInItemGroup(@NonNull Class type, + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { + return Collections.emptyList(); + } + + } + + /** + * A lazy provider that implements the optimized getCredentialById methods. + */ + @TestExtension public static final class LazyProvider2 extends CredentialsProvider { + + private static final List credentials = new java.util.concurrent.CopyOnWriteArrayList<>(); + static final AtomicInteger getByIdCalls = new AtomicInteger(0); + static final AtomicInteger listCalls = new AtomicInteger(0); + + static void addCredential(IdCredentials credential) { + credentials.add(credential); + } + + static void resetCounters() { + getByIdCalls.set(0); + listCalls.set(0); + } + + @Override public String getDisplayName() { + return "ZZZ Lazy Provider #2 (Optimized)"; + } + + @Override + @CheckForNull + public C getCredentialById(@NonNull String id, + @NonNull Class type, + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { + getByIdCalls.incrementAndGet(); + for (IdCredentials cred : credentials) { + if (cred.getId().equals(id) && type.isInstance(cred)) { + return type.cast(cred); + } + } + return null; + } + + @Override + @CheckForNull + public C getCredentialById(@NonNull String id, + @NonNull Class type, + @NonNull Item item, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { + getByIdCalls.incrementAndGet(); + for (IdCredentials cred : credentials) { + if (cred.getId().equals(id) && type.isInstance(cred)) { + return type.cast(cred); + } + } + return null; + } + + @Override + @NonNull + public List getCredentialsInItemGroup(@NonNull Class type, + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { + listCalls.incrementAndGet(); + List result = new java.util.ArrayList<>(); + for (IdCredentials cred : credentials) { + if (type.isInstance(cred)) { + result.add(type.cast(cred)); + } + } + return result; + } + } + + /** + * A lazy provider that does NOT implement optimized getCredentialById methods, + * relying on default implementations. + */ + @TestExtension public static final class LazyProvider3 extends CredentialsProvider { + + private static final List credentials = new java.util.concurrent.CopyOnWriteArrayList<>(); + static final AtomicInteger listCalls = new AtomicInteger(0); + + static void addCredential(IdCredentials credential) { + credentials.add(credential); + } + + static void resetCounters() { + listCalls.set(0); + } + + @Override public String getDisplayName() { + return "ZZZ Lazy Provider #3 (Unoptimized)"; + } + + @Override + @NonNull + public List getCredentialsInItemGroup(@NonNull Class type, + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { + listCalls.incrementAndGet(); + List result = new java.util.ArrayList<>(); + for (IdCredentials cred : credentials) { + if (type.isInstance(cred)) { + result.add(type.cast(cred)); + } + } + return result; + } + } + + /** + * Simple test credential with ID. + */ + private static class DummyIdCredentials extends com.cloudbees.plugins.credentials.impl.BaseStandardCredentials { + + DummyIdCredentials(CredentialsScope scope, String id, String description) { + super(scope, id, description); + } } } From be9a9ae71415a4bcfb794f506ef102e566d36e8a Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 6 Jan 2026 14:24:41 -0500 Subject: [PATCH 03/10] API refinements: avoid method overloading and rawtypes --- .../credentials/CredentialsMatchers.java | 4 +- .../credentials/CredentialsProvider.java | 64 +++++++++---------- .../plugins/credentials/ByIdTest.java | 50 ++++++++------- 3 files changed, 58 insertions(+), 60 deletions(-) diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java index e4cdff2c..275c0c40 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java @@ -129,8 +129,8 @@ public static CredentialsMatcher instanceOf(@NonNull Class clazz) { * @param id the {@link com.cloudbees.plugins.credentials.common.IdCredentials#getId()} to match. * @return a matcher that matches {@link com.cloudbees.plugins.credentials.common.IdCredentials} with the * supplied {@link com.cloudbees.plugins.credentials.common.IdCredentials#getId()} - * @see CredentialsProvider#findCredentialById(String, Class, Item, org.springframework.security.core.Authentication, java.util.List) - * @see CredentialsProvider#findCredentialById(String, Class, hudson.model.ItemGroup, org.springframework.security.core.Authentication, java.util.List) + * @see CredentialsProvider#findCredentialByIdInItem + * @see CredentialsProvider#findCredentialByIdInItemGroup */ @NonNull public static CredentialsMatcher withId(@NonNull String id) { diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java index 4af4f915..0ea26986 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java @@ -857,14 +857,13 @@ public static C snapshot(Class clazz, C credential) { * @param domainRequirements the credential domains to match (if {@code null} assume empty list). * @param the credentials type. * @return the credential or {@code null} if no credential with the specified ID is found. - * @since TODO */ @CheckForNull - public static C findCredentialById(@NonNull String id, - @NonNull Class type, - @Nullable ItemGroup itemGroup, - @Nullable Authentication authentication, - @Nullable List domainRequirements) { + public static C findCredentialByIdInItemGroup(@NonNull String id, + @NonNull Class type, + @Nullable ItemGroup itemGroup, + @Nullable Authentication authentication, + @Nullable List domainRequirements) { Objects.requireNonNull(id); Objects.requireNonNull(type); itemGroup = itemGroup == null ? Jenkins.get() : itemGroup; @@ -874,7 +873,7 @@ public static C findCredentialById(@NonNull String id, for (CredentialsProvider provider : all()) { if (provider.isEnabled(itemGroup) && provider.isApplicable(type)) { try { - C credential = provider.getCredentialById(id, type, itemGroup, authentication, domainRequirements); + C credential = provider.getCredentialByIdInItemGroup(id, type, itemGroup, authentication, domainRequirements); if (credential != null) { return credential; } @@ -898,21 +897,20 @@ public static C findCredentialById(@NonNull String id, * @param domainRequirements the credential domains to match (if {@code null} assume empty list). * @param the credentials type. * @return the credential or {@code null} if no credential with the specified ID is found. - * @since TODO */ @CheckForNull - public static C findCredentialById(@NonNull String id, - @NonNull Class type, - @Nullable Item item, - @Nullable Authentication authentication, - @Nullable List domainRequirements) { + public static C findCredentialByIdInItem(@NonNull String id, + @NonNull Class type, + @Nullable Item item, + @Nullable Authentication authentication, + @Nullable List domainRequirements) { Objects.requireNonNull(id); Objects.requireNonNull(type); if (item == null) { - return findCredentialById(id, type, Jenkins.get(), authentication, domainRequirements); + return findCredentialByIdInItemGroup(id, type, Jenkins.get(), authentication, domainRequirements); } if (item instanceof ItemGroup) { - return findCredentialById(id, type, (ItemGroup) item, authentication, domainRequirements); + return findCredentialByIdInItemGroup(id, type, (ItemGroup) item, authentication, domainRequirements); } authentication = authentication == null ? ACL.SYSTEM2 : authentication; domainRequirements = domainRequirements == null ? Collections.emptyList() : domainRequirements; @@ -920,7 +918,7 @@ public static C findCredentialById(@NonNull String id, for (CredentialsProvider provider : all()) { if (provider.isEnabled(item) && provider.isApplicable(type)) { try { - C credential = provider.getCredentialById(id, type, item, authentication, domainRequirements); + C credential = provider.getCredentialByIdInItem(id, type, item, authentication, domainRequirements); if (credential != null) { return credential; } @@ -1014,10 +1012,10 @@ public static C findCredentialById(@NonNull String id, // as you would have no way to configure it Authentication runAuth = CredentialsProvider.getDefaultAuthenticationOf2(run.getParent()); // we want the credentials available to the user the build is running as - C credential = findCredentialById(id, type, run.getParent(), runAuth, domainRequirements); + C credential = findCredentialByIdInItem(id, type, run.getParent(), runAuth, domainRequirements); // if that user can use the item's credentials, try those too if (credential == null && runAuth != ACL.SYSTEM2 && run.hasPermission2(runAuth, CredentialsProvider.USE_ITEM)) { - credential = findCredentialById(id, type, run.getParent(), ACL.SYSTEM2, domainRequirements); + credential = findCredentialByIdInItem(id, type, run.getParent(), ACL.SYSTEM2, domainRequirements); } // TODO should this be calling track? return contextualize(type, credential, run); @@ -1030,14 +1028,14 @@ public static C findCredentialById(@NonNull String id, // the user triggered this job directly and they are allowed to supply their own credentials, so // search those first. We do not want to follow the chain for the user's authentication // though, as there is no way to limit how far the passed-through parameters can be used - result = findCredentialById(id, type, run.getParent(), a, domainRequirements); + result = findCredentialByIdInItem(id, type, run.getParent(), a, domainRequirements); } if (result == null && inputUserId != null) { final User inputUser = User.getById(inputUserId, false); if (inputUser != null) { final Authentication inputAuth = inputUser.impersonate2(); if (run.hasPermission2(inputAuth, CredentialsProvider.USE_OWN)) { - result = findCredentialById(id, type, run.getParent(), inputAuth, domainRequirements); + result = findCredentialByIdInItem(id, type, run.getParent(), inputAuth, domainRequirements); } } } @@ -1048,10 +1046,10 @@ public static C findCredentialById(@NonNull String id, // as you would have no way to configure it Authentication runAuth = CredentialsProvider.getDefaultAuthenticationOf2(run.getParent()); // we want the credentials available to the user the build is running as - result = findCredentialById(id, type, run.getParent(), runAuth, domainRequirements); + result = findCredentialByIdInItem(id, type, run.getParent(), runAuth, domainRequirements); // if that user can use the item's credentials, try those too if (result == null && runAuth != ACL.SYSTEM2 && run.hasPermission2(runAuth, CredentialsProvider.USE_ITEM)) { - result = findCredentialById(id, type, run.getParent(), ACL.SYSTEM2, domainRequirements); + result = findCredentialByIdInItem(id, type, run.getParent(), ACL.SYSTEM2, domainRequirements); } } // if the run has not completed yet then we can safely assume that the credential is being used for this run @@ -1306,14 +1304,13 @@ public ListBoxModel getCredentialIds(@NonNull Class * @param authentication the authentication. * @param domainRequirements the credential domain to match. * @return the credential or {@code null} if no credential with the specified ID is found. - * @since TODO */ @CheckForNull - public C getCredentialById(@NonNull String id, - @NonNull Class type, - @NonNull ItemGroup itemGroup, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { + public C getCredentialByIdInItemGroup(@NonNull String id, + @NonNull Class type, + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { return CredentialsMatchers.firstOrNull( getCredentialsInItemGroup(type, itemGroup, authentication, domainRequirements), CredentialsMatchers.withId(id) @@ -1336,14 +1333,13 @@ public C getCredentialById(@NonNull String id, * @param authentication the authentication. * @param domainRequirements the credential domain to match. * @return the credential or {@code null} if no credential with the specified ID is found. - * @since TODO */ @CheckForNull - public C getCredentialById(@NonNull String id, - @NonNull Class type, - @NonNull Item item, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { + public C getCredentialByIdInItem(@NonNull String id, + @NonNull Class type, + @NonNull Item item, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { return CredentialsMatchers.firstOrNull( getCredentialsInItem(type, item, authentication, domainRequirements), CredentialsMatchers.withId(id) diff --git a/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java b/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java index d0a7e54d..d2c0cf9a 100644 --- a/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java +++ b/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java @@ -34,7 +34,6 @@ import java.util.Collections; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; -import jenkins.model.Jenkins; import org.springframework.security.core.Authentication; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.instanceOf; @@ -77,7 +76,7 @@ final class ByIdTest { LazyProvider3.resetCounters(); // Search for early credential - IdCredentials result = CredentialsProvider.findCredentialById( + IdCredentials result = CredentialsProvider.findCredentialByIdInItemGroup( "early-cred", IdCredentials.class, (ItemGroup) null, @@ -104,7 +103,7 @@ final class ByIdTest { LazyProvider3.resetCounters(); // Search for lazy2 credential - IdCredentials result = CredentialsProvider.findCredentialById( + IdCredentials result = CredentialsProvider.findCredentialByIdInItemGroup( "lazy2-cred", IdCredentials.class, (ItemGroup) null, @@ -133,7 +132,7 @@ final class ByIdTest { LazyProvider3.resetCounters(); // Search for lazy3 credential - IdCredentials result = CredentialsProvider.findCredentialById( + IdCredentials result = CredentialsProvider.findCredentialByIdInItemGroup( "lazy3-cred", IdCredentials.class, (ItemGroup) null, @@ -161,7 +160,7 @@ final class ByIdTest { LazyProvider3.resetCounters(); // Search for nonexistent credential - IdCredentials result = CredentialsProvider.findCredentialById( + IdCredentials result = CredentialsProvider.findCredentialByIdInItemGroup( "nonexistent", IdCredentials.class, (ItemGroup) null, @@ -185,12 +184,13 @@ final class ByIdTest { return "ZZZ Lazy Provider #1"; } + @SuppressWarnings("rawtypes") // historical mistake @Override @NonNull public List getCredentialsInItemGroup(@NonNull Class type, - @NonNull ItemGroup itemGroup, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { return Collections.emptyList(); } @@ -220,11 +220,11 @@ static void resetCounters() { @Override @CheckForNull - public C getCredentialById(@NonNull String id, - @NonNull Class type, - @NonNull ItemGroup itemGroup, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { + public C getCredentialByIdInItemGroup(@NonNull String id, + @NonNull Class type, + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { getByIdCalls.incrementAndGet(); for (IdCredentials cred : credentials) { if (cred.getId().equals(id) && type.isInstance(cred)) { @@ -236,11 +236,11 @@ public C getCredentialById(@NonNull String id, @Override @CheckForNull - public C getCredentialById(@NonNull String id, - @NonNull Class type, - @NonNull Item item, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { + public C getCredentialByIdInItem(@NonNull String id, + @NonNull Class type, + @NonNull Item item, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { getByIdCalls.incrementAndGet(); for (IdCredentials cred : credentials) { if (cred.getId().equals(id) && type.isInstance(cred)) { @@ -250,12 +250,13 @@ public C getCredentialById(@NonNull String id, return null; } + @SuppressWarnings("rawtypes") // historical mistake @Override @NonNull public List getCredentialsInItemGroup(@NonNull Class type, - @NonNull ItemGroup itemGroup, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { listCalls.incrementAndGet(); List result = new java.util.ArrayList<>(); for (IdCredentials cred : credentials) { @@ -288,12 +289,13 @@ static void resetCounters() { return "ZZZ Lazy Provider #3 (Unoptimized)"; } + @SuppressWarnings("rawtypes") // historical mistake @Override @NonNull public List getCredentialsInItemGroup(@NonNull Class type, - @NonNull ItemGroup itemGroup, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { + @NonNull ItemGroup itemGroup, + @NonNull Authentication authentication, + @NonNull List domainRequirements) { listCalls.incrementAndGet(); List result = new java.util.ArrayList<>(); for (IdCredentials cred : credentials) { From cbfb5770c9a95bd38c707231f907665cd07fe564 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 6 Jan 2026 14:28:04 -0500 Subject: [PATCH 04/10] Catching `NoClassDefFoundError` is dubious; omitting in new code --- .../credentials/CredentialsProvider.java | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java index 0ea26986..3dd3d8d3 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java @@ -872,14 +872,9 @@ public static C findCredentialByIdInItemGroup(@NonNull for (CredentialsProvider provider : all()) { if (provider.isEnabled(itemGroup) && provider.isApplicable(type)) { - try { - C credential = provider.getCredentialByIdInItemGroup(id, type, itemGroup, authentication, domainRequirements); - if (credential != null) { - return credential; - } - } catch (NoClassDefFoundError e) { - LOGGER.log(Level.FINE, "Could not retrieve provider credentials from " + provider - + " likely due to missing optional dependency", e); + C credential = provider.getCredentialByIdInItemGroup(id, type, itemGroup, authentication, domainRequirements); + if (credential != null) { + return credential; } } } @@ -917,14 +912,9 @@ public static C findCredentialByIdInItem(@NonNull Stri for (CredentialsProvider provider : all()) { if (provider.isEnabled(item) && provider.isApplicable(type)) { - try { - C credential = provider.getCredentialByIdInItem(id, type, item, authentication, domainRequirements); - if (credential != null) { - return credential; - } - } catch (NoClassDefFoundError e) { - LOGGER.log(Level.FINE, "Could not retrieve provider credentials from " + provider - + " likely due to missing optional dependency", e); + C credential = provider.getCredentialByIdInItem(id, type, item, authentication, domainRequirements); + if (credential != null) { + return credential; } } } From e45c44e490ee751024049d71f84d71592e9b3c73 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 6 Jan 2026 14:33:52 -0500 Subject: [PATCH 05/10] Simplifications to impls --- .../credentials/CredentialsProvider.java | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java index 3dd3d8d3..baef01a2 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java @@ -866,10 +866,15 @@ public static C findCredentialByIdInItemGroup(@NonNull @Nullable List domainRequirements) { Objects.requireNonNull(id); Objects.requireNonNull(type); - itemGroup = itemGroup == null ? Jenkins.get() : itemGroup; - authentication = authentication == null ? ACL.SYSTEM2 : authentication; - domainRequirements = domainRequirements == null ? Collections.emptyList() : domainRequirements; - + if (itemGroup == null) { + itemGroup = Jenkins.get(); + } + if (authentication == null) { + authentication = ACL.SYSTEM2; + } + if (domainRequirements == null) { + domainRequirements = List.of(); + } for (CredentialsProvider provider : all()) { if (provider.isEnabled(itemGroup) && provider.isApplicable(type)) { C credential = provider.getCredentialByIdInItemGroup(id, type, itemGroup, authentication, domainRequirements); @@ -904,12 +909,15 @@ public static C findCredentialByIdInItem(@NonNull Stri if (item == null) { return findCredentialByIdInItemGroup(id, type, Jenkins.get(), authentication, domainRequirements); } - if (item instanceof ItemGroup) { - return findCredentialByIdInItemGroup(id, type, (ItemGroup) item, authentication, domainRequirements); + if (item instanceof ItemGroup group) { + return findCredentialByIdInItemGroup(id, type, group, authentication, domainRequirements); + } + if (authentication == null) { + authentication = ACL.SYSTEM2; + } + if (domainRequirements == null) { + domainRequirements = List.of(); } - authentication = authentication == null ? ACL.SYSTEM2 : authentication; - domainRequirements = domainRequirements == null ? Collections.emptyList() : domainRequirements; - for (CredentialsProvider provider : all()) { if (provider.isEnabled(item) && provider.isApplicable(type)) { C credential = provider.getCredentialByIdInItem(id, type, item, authentication, domainRequirements); From 93d7763f7d0d34952ca11c2907a4e5272459ab10 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 6 Jan 2026 14:34:31 -0500 Subject: [PATCH 06/10] More logging for historical impls --- .../com/cloudbees/plugins/credentials/CredentialsProvider.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java index baef01a2..f725fb8c 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java @@ -383,12 +383,14 @@ public static List lookupCredentialsInItemGroup(@NonN Set ids = new HashSet<>(); for (CredentialsProvider provider : all()) { if (provider.isEnabled(itemGroup) && provider.isApplicable(type)) { + LOGGER.fine(() -> "checking " + provider + " for " + type); try { for (C c : provider.getCredentialsInItemGroup(type, itemGroup, authentication, domainRequirements)) { if (!(c instanceof IdCredentials) || ids.add(((IdCredentials) c).getId())) { // if IdCredentials, only add if we haven't added already // if not IdCredentials, always add result.add(c); + LOGGER.fine(() -> "got " + c + " from " + provider); } } } catch (NoClassDefFoundError e) { From b13b4cfb391da38c4f13edb1b69915ffe7cefee7 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 6 Jan 2026 16:43:28 -0500 Subject: [PATCH 07/10] Better logging in new lookup methods --- .../plugins/credentials/CredentialsProvider.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java index f725fb8c..58f70930 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java @@ -877,14 +877,19 @@ public static C findCredentialByIdInItemGroup(@NonNull if (domainRequirements == null) { domainRequirements = List.of(); } + var g = itemGroup; + LOGGER.fine(() -> "looking for " + id + " of " + type + " in " + g); for (CredentialsProvider provider : all()) { if (provider.isEnabled(itemGroup) && provider.isApplicable(type)) { + LOGGER.fine(() -> "checking " + provider + " for " + id); C credential = provider.getCredentialByIdInItemGroup(id, type, itemGroup, authentication, domainRequirements); if (credential != null) { + LOGGER.fine(() -> "found " + credential + " in " + provider); return credential; } } } + LOGGER.fine(() -> "did not find " + id); return null; } @@ -920,14 +925,18 @@ public static C findCredentialByIdInItem(@NonNull Stri if (domainRequirements == null) { domainRequirements = List.of(); } + LOGGER.fine(() -> "looking for " + id + " of " + type + " in " + item); for (CredentialsProvider provider : all()) { if (provider.isEnabled(item) && provider.isApplicable(type)) { + LOGGER.fine(() -> "checking " + provider + " for " + id); C credential = provider.getCredentialByIdInItem(id, type, item, authentication, domainRequirements); if (credential != null) { + LOGGER.fine(() -> "found " + credential + " in " + provider); return credential; } } } + LOGGER.fine(() -> "did not find " + id); return null; } From 72c31bb6aca725575ffd5ba26dde113919cf8580 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 9 Jan 2026 13:09:30 -0500 Subject: [PATCH 08/10] More compact `ByIdTest` --- .../plugins/credentials/ByIdTest.java | 288 +++++------------- 1 file changed, 72 insertions(+), 216 deletions(-) diff --git a/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java b/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java index d2c0cf9a..dde6683a 100644 --- a/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java +++ b/src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java @@ -26,14 +26,13 @@ import com.cloudbees.plugins.credentials.common.IdCredentials; import com.cloudbees.plugins.credentials.domains.DomainRequirement; +import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl; import edu.umd.cs.findbugs.annotations.CheckForNull; -import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.ExtensionList; import hudson.model.Item; import hudson.model.ItemGroup; -import hudson.security.ACL; -import java.util.Collections; +import java.util.ArrayList; import java.util.List; -import java.util.concurrent.atomic.AtomicInteger; import org.springframework.security.core.Authentication; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.instanceOf; @@ -49,272 +48,129 @@ /** * Exercises {@link CredentialsProvider} searches for {@link IdCredentials}. */ +@SuppressWarnings("rawtypes") // historical mistake with ItemGroup @WithJenkins final class ByIdTest { - @Test void providerOrder(JenkinsRule r) { - CredentialsProvider.all().forEach(System.out::println); - // Verify that our test providers are at the end of the list (they start with "ZZZ") - assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 1), instanceOf(LazyProvider3.class)); - assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 2), instanceOf(LazyProvider2.class)); + private LazyProvider2 lp2; + private LazyProvider3 lp3; + + private void setUp() throws Exception { + // Verify that our test providers are at the end of the list (they start with "ZZZ"); will be after folder, system, mock, user assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 3), instanceOf(LazyProvider1.class)); - } + assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 2), instanceOf(LazyProvider2.class)); + assertThat(CredentialsProvider.all().get(CredentialsProvider.all().size() - 1), instanceOf(LazyProvider3.class)); - @Test void lazyEvaluationWithEarlyMatch(JenkinsRule r) throws Exception { - // Add a credential to the system store (early provider) - SystemCredentialsProvider.getInstance().getCredentials().add( - new DummyIdCredentials(CredentialsScope.GLOBAL, "early-cred", "Early Credential") - ); - SystemCredentialsProvider.getInstance().save(); + lp2 = ExtensionList.lookupSingleton(LazyProvider2.class); + lp3 = ExtensionList.lookupSingleton(LazyProvider3.class); // Add credentials to lazy providers - LazyProvider2.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy2-cred", "Lazy2 Credential")); - LazyProvider3.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy3-cred", "Lazy3 Credential")); + lp2.credentials.add(new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, "lazy2-cred", null, "user", "pass")); + lp3.credentials.add(new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, "lazy3-cred", null, "user", "pass")); // Reset counters - LazyProvider2.resetCounters(); - LazyProvider3.resetCounters(); + lp2.getByIdCalls = 0; + lp2.listCalls = 0; + lp3.listCalls = 0; + } + + @Test void lazyEvaluationWithEarlyMatch(JenkinsRule r) throws Exception { + setUp(); + + // Add a credential to the system store (early provider) + SystemCredentialsProvider.getInstance().getCredentials().add(new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, "early-cred", null, "user", "pass")); + SystemCredentialsProvider.getInstance().save(); // Search for early credential - IdCredentials result = CredentialsProvider.findCredentialByIdInItemGroup( - "early-cred", - IdCredentials.class, - (ItemGroup) null, - ACL.SYSTEM2, - Collections.emptyList() - ); + var result = CredentialsProvider.findCredentialByIdInItemGroup("early-cred", IdCredentials.class, null, null, null); assertThat(result, notNullValue()); assertThat(result.getId(), is("early-cred")); // Verify lazy providers were not consulted - assertEquals(0, LazyProvider2.getByIdCalls.get(), "LazyProvider2.getCredentialById should not be called"); - assertEquals(0, LazyProvider2.listCalls.get(), "LazyProvider2.getCredentialsInItemGroup should not be called"); - assertEquals(0, LazyProvider3.listCalls.get(), "LazyProvider3.getCredentialsInItemGroup should not be called"); + assertEquals(0, lp2.getByIdCalls, "LazyProvider2.getCredentialById should not be called"); + assertEquals(0, lp2.listCalls, "LazyProvider2.getCredentialsInItemGroup should not be called"); + assertEquals(0, lp3.listCalls, "LazyProvider3.getCredentialsInItemGroup should not be called"); } - @Test void lazyEvaluationWithOptimizedProvider(JenkinsRule r) { - // Add credentials to lazy providers - LazyProvider2.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy2-cred", "Lazy2 Credential")); - LazyProvider3.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy3-cred", "Lazy3 Credential")); - - // Reset counters - LazyProvider2.resetCounters(); - LazyProvider3.resetCounters(); - - // Search for lazy2 credential - IdCredentials result = CredentialsProvider.findCredentialByIdInItemGroup( - "lazy2-cred", - IdCredentials.class, - (ItemGroup) null, - ACL.SYSTEM2, - Collections.emptyList() - ); - + @Test void lazyEvaluationWithOptimizedProvider(JenkinsRule r) throws Exception { + setUp(); + var result = CredentialsProvider.findCredentialByIdInItemGroup("lazy2-cred", IdCredentials.class, null, null, null); assertThat(result, notNullValue()); assertThat(result.getId(), is("lazy2-cred")); - - // Verify LazyProvider2's optimized method was called - assertEquals(1, LazyProvider2.getByIdCalls.get(), "LazyProvider2.getCredentialById should be called once"); - // Verify LazyProvider2's list method was NOT called (optimized path) - assertEquals(0, LazyProvider2.listCalls.get(), "LazyProvider2.getCredentialsInItemGroup should not be called"); - // Verify LazyProvider3 was not consulted at all - assertEquals(0, LazyProvider3.listCalls.get(), "LazyProvider3.getCredentialsInItemGroup should not be called"); + assertEquals(1, lp2.getByIdCalls, "LazyProvider2.getCredentialById should be called once"); + assertEquals(0, lp2.listCalls, "LazyProvider2.getCredentialsInItemGroup should not be called"); + assertEquals(0, lp3.listCalls, "LazyProvider3.getCredentialsInItemGroup should not be called"); } - @Test void lazyEvaluationWithUnoptimizedProvider(JenkinsRule r) { - // Add credentials to lazy providers - LazyProvider2.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy2-cred", "Lazy2 Credential")); - LazyProvider3.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy3-cred", "Lazy3 Credential")); - - // Reset counters - LazyProvider2.resetCounters(); - LazyProvider3.resetCounters(); - - // Search for lazy3 credential - IdCredentials result = CredentialsProvider.findCredentialByIdInItemGroup( - "lazy3-cred", - IdCredentials.class, - (ItemGroup) null, - ACL.SYSTEM2, - Collections.emptyList() - ); - + @Test void lazyEvaluationWithUnoptimizedProvider(JenkinsRule r) throws Exception { + setUp(); + var result = CredentialsProvider.findCredentialByIdInItemGroup("lazy3-cred", IdCredentials.class, null, null, null); assertThat(result, notNullValue()); assertThat(result.getId(), is("lazy3-cred")); - - // Verify LazyProvider2 was consulted but didn't find it - assertEquals(1, LazyProvider2.getByIdCalls.get(), "LazyProvider2.getCredentialById should be called once"); - assertEquals(0, LazyProvider2.listCalls.get(), "LazyProvider2.getCredentialsInItemGroup should not be called (uses optimized path)"); - // Verify LazyProvider3's list method WAS called (unoptimized, uses default implementation) - assertEquals(1, LazyProvider3.listCalls.get(), "LazyProvider3.getCredentialsInItemGroup should be called once"); + assertEquals(1, lp2.getByIdCalls, "LazyProvider2.getCredentialById should be called once"); + assertEquals(0, lp2.listCalls, "LazyProvider2.getCredentialsInItemGroup should not be called (uses optimized path)"); + assertEquals(1, lp3.listCalls, "LazyProvider3.getCredentialsInItemGroup should be called once"); } - @Test void lazyEvaluationWithNonexistentCredential(JenkinsRule r) { - // Add credentials to lazy providers - LazyProvider2.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy2-cred", "Lazy2 Credential")); - LazyProvider3.addCredential(new DummyIdCredentials(CredentialsScope.GLOBAL, "lazy3-cred", "Lazy3 Credential")); - - // Reset counters - LazyProvider2.resetCounters(); - LazyProvider3.resetCounters(); - - // Search for nonexistent credential - IdCredentials result = CredentialsProvider.findCredentialByIdInItemGroup( - "nonexistent", - IdCredentials.class, - (ItemGroup) null, - ACL.SYSTEM2, - Collections.emptyList() - ); - + @Test void lazyEvaluationWithNonexistentCredential(JenkinsRule r) throws Exception { + setUp(); + var result = CredentialsProvider.findCredentialByIdInItemGroup("nonexistent", IdCredentials.class, null, null, null); assertThat(result, nullValue()); - - // Verify both lazy providers were consulted - assertEquals(1, LazyProvider2.getByIdCalls.get(), "LazyProvider2.getCredentialById should be called once"); - assertEquals(0, LazyProvider2.listCalls.get(), "LazyProvider2.getCredentialsInItemGroup should not be called (uses optimized path)"); - assertEquals(1, LazyProvider3.listCalls.get(), "LazyProvider3.getCredentialsInItemGroup should be called once"); + assertEquals(1, lp2.getByIdCalls, "LazyProvider2.getCredentialById should be called once"); + assertEquals(0, lp2.listCalls, "LazyProvider2.getCredentialsInItemGroup should not be called (uses optimized path)"); + assertEquals(1, lp3.listCalls, "LazyProvider3.getCredentialsInItemGroup should be called once"); } @TestExtension public static final class LazyProvider1 extends CredentialsProvider { - // @TestExtension lacks ordinal, and CredentialsProvider.getDisplayName uses Class.simpleName, // so to sort CredentialsProvider.all we must use a special nested class name or override this: @Override public String getDisplayName() { return "ZZZ Lazy Provider #1"; } - - @SuppressWarnings("rawtypes") // historical mistake - @Override - @NonNull - public List getCredentialsInItemGroup(@NonNull Class type, - @NonNull ItemGroup itemGroup, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { - return Collections.emptyList(); + @Override public List getCredentialsInItemGroup(Class type, ItemGroup itemGroup, Authentication authentication, List domainRequirements) { + return List.of(); } - } - /** - * A lazy provider that implements the optimized getCredentialById methods. - */ @TestExtension public static final class LazyProvider2 extends CredentialsProvider { - - private static final List credentials = new java.util.concurrent.CopyOnWriteArrayList<>(); - static final AtomicInteger getByIdCalls = new AtomicInteger(0); - static final AtomicInteger listCalls = new AtomicInteger(0); - - static void addCredential(IdCredentials credential) { - credentials.add(credential); - } - - static void resetCounters() { - getByIdCalls.set(0); - listCalls.set(0); - } - + final List credentials = new ArrayList<>(); + int getByIdCalls = 0; + int listCalls = 0; @Override public String getDisplayName() { return "ZZZ Lazy Provider #2 (Optimized)"; } - - @Override - @CheckForNull - public C getCredentialByIdInItemGroup(@NonNull String id, - @NonNull Class type, - @NonNull ItemGroup itemGroup, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { - getByIdCalls.incrementAndGet(); - for (IdCredentials cred : credentials) { - if (cred.getId().equals(id) && type.isInstance(cred)) { - return type.cast(cred); - } - } - return null; + @Override public C getCredentialByIdInItemGroup(String id, Class type, ItemGroup itemGroup, Authentication authentication, List domainRequirements) { + getByIdCalls++; + return find(id, type); } - - @Override - @CheckForNull - public C getCredentialByIdInItem(@NonNull String id, - @NonNull Class type, - @NonNull Item item, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { - getByIdCalls.incrementAndGet(); - for (IdCredentials cred : credentials) { - if (cred.getId().equals(id) && type.isInstance(cred)) { - return type.cast(cred); - } - } - return null; + @Override public C getCredentialByIdInItem(String id, Class type, Item item, Authentication authentication, List domainRequirements) { + getByIdCalls++; + return find(id, type); } - - @SuppressWarnings("rawtypes") // historical mistake - @Override - @NonNull - public List getCredentialsInItemGroup(@NonNull Class type, - @NonNull ItemGroup itemGroup, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { - listCalls.incrementAndGet(); - List result = new java.util.ArrayList<>(); - for (IdCredentials cred : credentials) { - if (type.isInstance(cred)) { - result.add(type.cast(cred)); - } - } - return result; + private @CheckForNull C find(String id, Class type) { + return type.cast(credentials.stream().filter(cred -> cred.getId().equals(id) && type.isInstance(cred)).findAny().orElse(null)); + } + @Override public List getCredentialsInItemGroup(Class type, ItemGroup itemGroup, Authentication authentication, List domainRequirements) { + listCalls++; + return filter(type, credentials); } } - /** - * A lazy provider that does NOT implement optimized getCredentialById methods, - * relying on default implementations. - */ @TestExtension public static final class LazyProvider3 extends CredentialsProvider { - - private static final List credentials = new java.util.concurrent.CopyOnWriteArrayList<>(); - static final AtomicInteger listCalls = new AtomicInteger(0); - - static void addCredential(IdCredentials credential) { - credentials.add(credential); - } - - static void resetCounters() { - listCalls.set(0); - } - + final List credentials = new ArrayList<>(); + int listCalls = 0; @Override public String getDisplayName() { return "ZZZ Lazy Provider #3 (Unoptimized)"; } - - @SuppressWarnings("rawtypes") // historical mistake - @Override - @NonNull - public List getCredentialsInItemGroup(@NonNull Class type, - @NonNull ItemGroup itemGroup, - @NonNull Authentication authentication, - @NonNull List domainRequirements) { - listCalls.incrementAndGet(); - List result = new java.util.ArrayList<>(); - for (IdCredentials cred : credentials) { - if (type.isInstance(cred)) { - result.add(type.cast(cred)); - } - } - return result; + @Override public List getCredentialsInItemGroup(Class type, ItemGroup itemGroup, Authentication authentication, List domainRequirements) { + listCalls++; + return filter(type, credentials); } } - /** - * Simple test credential with ID. - */ - private static class DummyIdCredentials extends com.cloudbees.plugins.credentials.impl.BaseStandardCredentials { - - DummyIdCredentials(CredentialsScope scope, String id, String description) { - super(scope, id, description); - } + private static List filter(Class type, List credentials) { + return credentials.stream().filter(type::isInstance).map(type::cast).toList(); } } From 54bfc32074685aa2da60b715e02e3c070d11c8b7 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 9 Jan 2026 16:00:19 -0500 Subject: [PATCH 09/10] Forgotten comment --- .../com/cloudbees/plugins/credentials/CredentialsProvider.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java index e5947d66..63e98903 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java @@ -1110,8 +1110,6 @@ private static C contextualize(@NonNull Class type, @ return null; } - // TODO static + abstract variants taking credentialsId but Item or ItemGroup rather than Run - /** * Returns the list of all {@link CredentialsProvider}. * From 7471248360dd6bf02e5f69903c34d0064205e334 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 12 Jan 2026 11:07:40 -0500 Subject: [PATCH 10/10] Updated docs to encourage direct lookup by id --- docs/consumer.adoc | 51 ++++++++-------------------------------- docs/implementation.adoc | 2 ++ 2 files changed, 12 insertions(+), 41 deletions(-) diff --git a/docs/consumer.adoc b/docs/consumer.adoc index 970ce4fc..544929ae 100644 --- a/docs/consumer.adoc +++ b/docs/consumer.adoc @@ -359,24 +359,21 @@ If we have a job, "foobar", and we configure a credentials parameter on that job If you are working outside the context of a `Run` then you will not have to deal with the complexities of credentials expressions. -In most cases the retrieval will just be a call to one of the `CredentialsProvider.lookupCredentialsInItem(...)`/`CredentialsProvider.lookupCredentialsInItemGroup(...)` wrapped within `CredentialsMatchers.firstOrNull(..., CredentialsMatchers.withId(...))`, for example: +In most cases the retrieval will just be a call to `CredentialsProvider.findCredentialByIdInItemGroup` or `CredentialsProvider.findCredentialByIdInItem`: [source,java] ---- -StandardCredentials c = CredentialsMatchers.firstOrNull( - CredentialsProvider.lookupCredentialsInItem( +StandardCredentials c = CredentialsProvider.findCredentialByIdInItem( + credentialsId, StandardCredentials.class, // <1> job, // <1> job instanceof Queue.Task // <1> ? Tasks.getAuthenticationOf((Queue.Task)job)) : ACL.SYSTEM2, URIRequirementBuilder.fromUri(...) // <1> - ), - CredentialsMatchers.withId(credentialsId) // <2> -); + ); ---- <1> These should be the same as your call to `CredentialsProvider.listCredentialsInItem(...)`/`CredentialsProvider.listCredentialsInItemGroup(...)`/`StandardListBoxModel.includeMatchingAs(...)` in order to ensure that we get the same credential instance back. -<2> If you had additional `CredentialsMatcher` expressions in your call to `CredentialsProvider.listCredentialsInItem(...)`/`CredentialsProvider.listCredentialsInItemGroup(...)`/`StandardListBoxModel.includeMatchingAs(...)` then you should merge them here with a `CredentialsMatchers.allOf(...)` Once you have retrieved a non-null credentials instance, all non-secret properties can be assumed as eager-fetch immutable. @@ -392,20 +389,18 @@ The recommended way to use a credential is through the https://plugins.jenkins.i [source,java] ---- -StandardCredentials c = CredentialsMatchers.firstOrNull( // <1> - CredentialsProvider.listCredentialsInItem( +StandardCredentials c = CredentialsProvider.findCredentialByIdInItem( // <1> + credentialsId, StandardCredentials.class, job, job instanceof Queue.Task ? Tasks.getAuthenticationOf2((Queue.Task)job)) : ACL.SYSTEM2, URIRequirementBuilder.fromUri(issueTrackerUrl) - ), - CredentialsMatchers.allOf( - CredentialsMatchers.withId(credentialsId), - AuthenticationTokens.matcher(IssueTrackerAuthentication.class) // <2> - ) - ); + ); +if (c != null && !AuthenticationTokens.matcher(IssueTrackerAuthentication.class).matches(c)) { + c = null; +} IssueTrackerAuthentication auth = AuthenticationTokens.convert( IssueTrackerAuthentication.class, // <2> c // <3> @@ -429,32 +424,6 @@ StandardCredentials c = ...; CredentialsProvider.track(job, c); ---- -In most cases we can avoid holding object references longer than necessary by combining all these methods together: - -[source,java] ----- -IssueTrackerAuthentication auth = AuthenticationTokens.convert( - IssueTrackerAuthentication.class, - CredentialsProvider.track( - job, - CredentialsMatchers.firstOrNull( - CredentialsProvider.listCredentialsInItem( - StandardCredentials.class, - job, - job instanceof Queue.Task - ? Tasks.getAuthenticationOf2((Queue.Task)job)) - : ACL.SYSTEM2, - URIRequirementBuilder.fromUri(issueTrackerUrl) - ), - CredentialsMatchers.allOf( - CredentialsMatchers.withId(credentialsId), - AuthenticationTokens.matcher(IssueTrackerAuthentication.class) - ) - ) - ) -); ----- - === Binding user supplied credentials parameters to builds A running build can be supplied with credentials parameter values. diff --git a/docs/implementation.adoc b/docs/implementation.adoc index 93fcc497..b33d2a9a 100644 --- a/docs/implementation.adoc +++ b/docs/implementation.adoc @@ -696,6 +696,8 @@ Listing credentials operations are normally restricted to the population of cred Such requests are AJAX requests, so we have the option to block without affecting the rest of the Jenkins UI. Blocking for more than between 5 and 10 seconds, however, will cause user frustration, thus for this type of request we try to serve the response live and fall-back to the cache if the live response takes too long. + +Runtime lookups of credentials is normally limited to loading a specific credential by id, so consider overriding the methods which take an id argument. ==== These different caching concerns are addresses at different points in the credentials API: