diff --git a/src/main/java/com/cloudbees/plugins/credentials/SystemCredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/SystemCredentialsProvider.java index 15f24d18c..75a6b4867 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/SystemCredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/SystemCredentialsProvider.java @@ -323,6 +323,14 @@ private synchronized boolean removeCredentials(@NonNull Domain domain, @NonNull private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull Credentials current, @NonNull Credentials replacement) throws IOException { checkPermission(CredentialsProvider.UPDATE); + + // See IdCredentials.Helper#equals. + // Note: We probably should not assume that all credentials are IdCredentials. We also do this a few lines below in list.indexOf(current) call. + // If this one breaks, that one breaks as well. + if (!current.equals(replacement)) { + throw new IllegalArgumentException("Credentials' IDs do not match, will not update."); + } + Map> domainCredentialsMap = getDomainCredentialsMap(); if (domainCredentialsMap.containsKey(domain)) { List list = domainCredentialsMap.get(domain); diff --git a/src/main/java/com/cloudbees/plugins/credentials/UserCredentialsProvider.java b/src/main/java/com/cloudbees/plugins/credentials/UserCredentialsProvider.java index 9f0ddadca..b181e2395 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/UserCredentialsProvider.java +++ b/src/main/java/com/cloudbees/plugins/credentials/UserCredentialsProvider.java @@ -392,6 +392,14 @@ private synchronized boolean removeCredentials(@NonNull Domain domain, @NonNull private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull Credentials current, @NonNull Credentials replacement) throws IOException { checkPermission(CredentialsProvider.UPDATE); + + // See IdCredentials.Helper#equals. + // Note: We probably should not assume that all credentials are IdCredentials. We also do this a few lines below in list.indexOf(current) call. + // If this one breaks, that one breaks as well. + if (!current.equals(replacement)) { + throw new IllegalArgumentException("Credentials' IDs do not match, will not update."); + } + Map> domainCredentialsMap = getDomainCredentialsMap(); if (domainCredentialsMap.containsKey(domain)) { List list = domainCredentialsMap.get(domain); diff --git a/src/test/java/com/cloudbees/plugins/credentials/CredentialsProviderTest.java b/src/test/java/com/cloudbees/plugins/credentials/CredentialsProviderTest.java index 9dffe0806..e0d4bedbd 100644 --- a/src/test/java/com/cloudbees/plugins/credentials/CredentialsProviderTest.java +++ b/src/test/java/com/cloudbees/plugins/credentials/CredentialsProviderTest.java @@ -41,6 +41,7 @@ import hudson.security.ACL; import hudson.util.ListBoxModel; import jenkins.model.Jenkins; +import org.junit.Assert; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Issue; @@ -255,7 +256,7 @@ public void testManageUserCredentials() throws IOException { final User alice = User.getById("alice", true); DummyCredentials aliceCred1 = new DummyCredentials(CredentialsScope.USER, "aliceCred1", "pwd"); DummyCredentials aliceCred2 = new DummyCredentials(CredentialsScope.USER, "aliceCred2", "pwd"); - DummyCredentials aliceCred3 = new DummyCredentials(CredentialsScope.USER, "aliceCred3", "pwd"); + DummyCredentials aliceCred3 = new DummyCredentials(CredentialsScope.USER, aliceCred1.getId(), aliceCred1.getDescription(), "aliceCred3", "pwd"); r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); @@ -289,7 +290,7 @@ public void testUpdateAndDeleteCredentials() throws IOException { DummyCredentials systemCred = new DummyCredentials(CredentialsScope.SYSTEM, "systemCred", "pwd"); DummyCredentials systemCred2 = new DummyCredentials(CredentialsScope.SYSTEM, "systemCred2", "pwd"); DummyCredentials globalCred = new DummyCredentials(CredentialsScope.GLOBAL, "globalCred", "pwd"); - DummyCredentials modCredential = new DummyCredentials(CredentialsScope.GLOBAL, "modCredential", "pwd"); + DummyCredentials modCredential = new DummyCredentials(CredentialsScope.GLOBAL, globalCred.getId(), globalCred.getDescription(), "modCredential", "pwd"); CredentialsStore store = CredentialsProvider.lookupStores(Jenkins.get()).iterator().next(); @@ -457,4 +458,14 @@ public void credentialsSortedByNameInUI() { assertThat(options.get(0).value, is("2")); assertThat(options.get(1).value, is("1")); } + + @Test + @Issue("SECURITY-3252") + public void cannotUpdateCredentialsId() { + DummyCredentials cred1 = new DummyCredentials(CredentialsScope.GLOBAL, "cred1", "pwd"); + DummyCredentials cred2 = new DummyCredentials(CredentialsScope.GLOBAL, "cred2", "pwd"); + CredentialsStore store = CredentialsProvider.lookupStores(Jenkins.get()).iterator().next(); + + Assert.assertThrows(IllegalArgumentException.class, () -> store.updateCredentials(Domain.global(), cred1, cred2)); + } } diff --git a/src/test/java/com/cloudbees/plugins/credentials/MockFolderCredentialsProvider.java b/src/test/java/com/cloudbees/plugins/credentials/MockFolderCredentialsProvider.java index 07b13ea22..bd9ffa35d 100644 --- a/src/test/java/com/cloudbees/plugins/credentials/MockFolderCredentialsProvider.java +++ b/src/test/java/com/cloudbees/plugins/credentials/MockFolderCredentialsProvider.java @@ -330,6 +330,14 @@ private synchronized boolean removeCredentials(@NonNull Domain domain, @NonNull private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull Credentials current, @NonNull Credentials replacement) throws IOException { checkPermission(CredentialsProvider.UPDATE); + + // See IdCredentials.Helper#equals. + // Note: We probably should not assume that all credentials are IdCredentials. We also do this a few lines below in list.indexOf(current) call. + // If this one breaks, that one breaks as well. + if (!current.equals(replacement)) { + throw new IllegalArgumentException("Credentials' IDs do not match, will not update."); + } + Map> domainCredentialsMap = getDomainCredentialsMap(); if (domainCredentialsMap.containsKey(domain)) { List list = domainCredentialsMap.get(domain); diff --git a/src/test/java/com/cloudbees/plugins/credentials/domains/DomainTest.java b/src/test/java/com/cloudbees/plugins/credentials/domains/DomainTest.java index d9d571646..875cd75cd 100644 --- a/src/test/java/com/cloudbees/plugins/credentials/domains/DomainTest.java +++ b/src/test/java/com/cloudbees/plugins/credentials/domains/DomainTest.java @@ -84,7 +84,7 @@ public void testCredentialsInCustomDomains() throws IOException { Domain domainBar = new Domain("domainBar", "Path domain", Arrays.asList(new DomainSpecification[] { new HostnameSpecification("bar.com", "") })); DummyCredentials systemCred = new DummyCredentials(CredentialsScope.SYSTEM, "systemCred", "pwd"); DummyCredentials systemCred1 = new DummyCredentials(CredentialsScope.SYSTEM, "systemCred1", "pwd"); - DummyCredentials systemCredMod = new DummyCredentials(CredentialsScope.SYSTEM, "systemCredMod", "pwd"); + DummyCredentials systemCredMod = new DummyCredentials(CredentialsScope.SYSTEM, systemCred.getId(), systemCred.getDescription(), "systemCredMod", "pwd"); CredentialsStore store = CredentialsProvider.lookupStores(Jenkins.get()).iterator().next(); diff --git a/src/test/java/com/cloudbees/plugins/credentials/impl/DummyCredentials.java b/src/test/java/com/cloudbees/plugins/credentials/impl/DummyCredentials.java index fefc48c54..5105da7fb 100644 --- a/src/test/java/com/cloudbees/plugins/credentials/impl/DummyCredentials.java +++ b/src/test/java/com/cloudbees/plugins/credentials/impl/DummyCredentials.java @@ -23,7 +23,6 @@ */ package com.cloudbees.plugins.credentials.impl; -import com.cloudbees.plugins.credentials.BaseCredentials; import com.cloudbees.plugins.credentials.CredentialsDescriptor; import com.cloudbees.plugins.credentials.CredentialsScope; import com.cloudbees.plugins.credentials.common.UsernamePasswordCredentials; @@ -35,7 +34,7 @@ /** * A test credentials impl. */ -public class DummyCredentials extends BaseCredentials implements UsernamePasswordCredentials { +public class DummyCredentials extends BaseStandardCredentials implements UsernamePasswordCredentials { private final String username; @@ -43,7 +42,11 @@ public class DummyCredentials extends BaseCredentials implements UsernamePasswor @DataBoundConstructor public DummyCredentials(CredentialsScope scope, String username, String password) { - super(scope); + this(scope, null, null, username, password); + } + + public DummyCredentials(CredentialsScope scope, String id, String description, String username, String password) { + super(scope, id, description); this.username = username; this.password = Secret.fromString(password); }