Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-72611] Forbid updating credentials ID for IdCredentials #506

Merged
merged 4 commits into from
Feb 12, 2024
Merged

[JENKINS-72611] Forbid updating credentials ID for IdCredentials #506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
662b16b
[JENKINS-72611] Forbid updating credentials ID for `IdCredentials`
yaroslavafenkin Jan 25, 2024
0620d25
Address review feedback
yaroslavafenkin Jan 31, 2024
4772231
Revert `DummyCredentials`, use `DummyIdCredentials` instead
yaroslavafenkin Feb 1, 2024
eb2d6ca
Fix the issue reference in a test
yaroslavafenkin Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/main/java/com/cloudbees/plugins/credentials/SystemCredentialsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package com.cloudbees.plugins.credentials;

import com.cloudbees.plugins.credentials.common.IdCredentials;
import com.cloudbees.plugins.credentials.domains.Domain;
import com.cloudbees.plugins.credentials.domains.DomainCredentials;
import com.cloudbees.plugins.credentials.domains.DomainRequirement;
Expand Down Expand Up @@ -325,6 +326,11 @@ private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull
checkPermission(CredentialsProvider.UPDATE);
Map<Domain, List<Credentials>> domainCredentialsMap = getDomainCredentialsMap();
if (domainCredentialsMap.containsKey(domain)) {
if (current instanceof IdCredentials || replacement instanceof IdCredentials) {
if (!current.equals(replacement)) {
throw new IllegalArgumentException("Credentials' IDs do not match, will not update.");
}
}
List<Credentials> list = domainCredentialsMap.get(domain);
int index = list.indexOf(current);
if (index == -1) {
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/cloudbees/plugins/credentials/UserCredentialsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package com.cloudbees.plugins.credentials;

import com.cloudbees.plugins.credentials.common.IdCredentials;
import com.cloudbees.plugins.credentials.domains.Domain;
import com.cloudbees.plugins.credentials.domains.DomainCredentials;
import com.cloudbees.plugins.credentials.domains.DomainRequirement;
Expand Down Expand Up @@ -394,6 +395,11 @@ private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull
checkPermission(CredentialsProvider.UPDATE);
Map<Domain, List<Credentials>> domainCredentialsMap = getDomainCredentialsMap();
if (domainCredentialsMap.containsKey(domain)) {
if (current instanceof IdCredentials || replacement instanceof IdCredentials) {
if (!current.equals(replacement)) {
throw new IllegalArgumentException("Credentials' IDs do not match, will not update.");
}
}
List<Credentials> list = domainCredentialsMap.get(domain);
int index = list.indexOf(current);
if (index == -1) {
Expand Down
57 changes: 34 additions & 23 deletions src/test/java/com/cloudbees/plugins/credentials/CredentialsProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -253,9 +254,9 @@ public void testNoCredentialsUntilWeAddSomeViaStore2() throws Exception {
@Test
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");
DummyIdCredentials aliceCred1 = new DummyIdCredentials(null, CredentialsScope.USER, "aliceCred1", "pwd", "Cred 1");
DummyIdCredentials aliceCred2 = new DummyIdCredentials(null, CredentialsScope.USER, "aliceCred2", "pwd", "Cred 2");
DummyIdCredentials aliceCred3 = new DummyIdCredentials(aliceCred1.getId(), CredentialsScope.USER, "aliceCred3", "pwd", aliceCred1.getDescription());

r.jenkins.setSecurityRealm(r.createDummySecurityRealm());

Expand All @@ -264,32 +265,32 @@ public void testManageUserCredentials() throws IOException {
userStore.addCredentials(Domain.global(), aliceCred1);
userStore.addCredentials(Domain.global(), aliceCred2);

assertEquals(2, CredentialsProvider.lookupCredentialsInItem(DummyCredentials.class, (Item) null, alice.impersonate2(), Collections.emptyList()).size());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).isEmpty());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, Jenkins.ANONYMOUS2, Collections.emptyList()).isEmpty());
assertEquals(2, CredentialsProvider.lookupCredentialsInItem(DummyIdCredentials.class, (Item) null, alice.impersonate2(), Collections.emptyList()).size());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).isEmpty());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, Jenkins.ANONYMOUS2, Collections.emptyList()).isEmpty());

// Remove credentials
userStore.removeCredentials(Domain.global(), aliceCred2);

assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyCredentials.class, (Item) null, alice.impersonate2(), Collections.emptyList()).size());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).isEmpty());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, Jenkins.ANONYMOUS2, Collections.emptyList()).isEmpty());
assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyIdCredentials.class, (Item) null, alice.impersonate2(), Collections.emptyList()).size());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).isEmpty());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, Jenkins.ANONYMOUS2, Collections.emptyList()).isEmpty());

// Update credentials
userStore.updateCredentials(Domain.global(), aliceCred1, aliceCred3);

assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyCredentials.class, (Item) null, alice.impersonate2(), Collections.emptyList()).size());
assertEquals(aliceCred3.getUsername(), CredentialsProvider.lookupCredentialsInItem(DummyCredentials.class, (Item) null, alice.impersonate2(), Collections.emptyList()).get(0).getUsername());
assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyIdCredentials.class, (Item) null, alice.impersonate2(), Collections.emptyList()).size());
assertEquals(aliceCred3.getUsername(), CredentialsProvider.lookupCredentialsInItem(DummyIdCredentials.class, (Item) null, alice.impersonate2(), Collections.emptyList()).get(0).getUsername());
}
}

@Test
public void testUpdateAndDeleteCredentials() throws IOException {
FreeStyleProject project = r.createFreeStyleProject();
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");
DummyIdCredentials systemCred = new DummyIdCredentials(null, CredentialsScope.SYSTEM, "systemCred", "pwd", "System 1");
DummyIdCredentials systemCred2 = new DummyIdCredentials(null, CredentialsScope.SYSTEM, "systemCred2", "pwd", "System 2");
DummyIdCredentials globalCred = new DummyIdCredentials(null, CredentialsScope.GLOBAL, "globalCred", "pwd", "Global 1");
DummyIdCredentials modCredential = new DummyIdCredentials(globalCred.getId(), CredentialsScope.GLOBAL, "modCredential", "pwd", globalCred.getDescription());

CredentialsStore store = CredentialsProvider.lookupStores(Jenkins.get()).iterator().next();

Expand All @@ -298,22 +299,22 @@ public void testUpdateAndDeleteCredentials() throws IOException {
store.addCredentials(Domain.global(), systemCred2);
store.addCredentials(Domain.global(), globalCred);

assertEquals(3, CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(globalCred.getUsername(), CredentialsProvider.lookupCredentialsInItem(DummyCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).get(0).getUsername());
assertEquals(3, CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyIdCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(globalCred.getUsername(), CredentialsProvider.lookupCredentialsInItem(DummyIdCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).get(0).getUsername());

// Update credentials
store.updateCredentials(Domain.global(), globalCred, modCredential);

assertEquals(3, CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(modCredential.getUsername(), CredentialsProvider.lookupCredentialsInItem(DummyCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).get(0).getUsername());
assertEquals(3, CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyIdCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(modCredential.getUsername(), CredentialsProvider.lookupCredentialsInItem(DummyIdCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).get(0).getUsername());

// Remove credentials
store.removeCredentials(Domain.global(), systemCred2);

assertEquals(2, CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(2, CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, Collections.emptyList()).size());
assertEquals(1, CredentialsProvider.lookupCredentialsInItem(DummyIdCredentials.class, project, ACL.SYSTEM2, Collections.emptyList()).size());
}

@Test
Expand Down Expand Up @@ -457,4 +458,14 @@ public void credentialsSortedByNameInUI() {
assertThat(options.get(0).value, is("2"));
assertThat(options.get(1).value, is("1"));
}

@Test
@Issue("JENKINS-72611")
public void credentialsIdCannotBeUpdated() {
DummyIdCredentials cred1 = new DummyIdCredentials(null, CredentialsScope.GLOBAL, "cred1", "pwd", "Cred 1");
DummyIdCredentials cred2 = new DummyIdCredentials(null, CredentialsScope.GLOBAL, "cred2", "pwd", "Cred 2");
CredentialsStore store = CredentialsProvider.lookupStores(Jenkins.get()).iterator().next();

Assert.assertThrows(IllegalArgumentException.class, () -> store.updateCredentials(Domain.global(), cred1, cred2));
}
}
6 changes: 6 additions & 0 deletions src/test/java/com/cloudbees/plugins/credentials/MockFolderCredentialsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

package com.cloudbees.plugins.credentials;

import com.cloudbees.plugins.credentials.common.IdCredentials;
import com.cloudbees.plugins.credentials.domains.Domain;
import com.cloudbees.plugins.credentials.domains.DomainCredentials;
import com.cloudbees.plugins.credentials.domains.DomainRequirement;
Expand Down Expand Up @@ -332,6 +333,11 @@ private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull
checkPermission(CredentialsProvider.UPDATE);
Map<Domain, List<Credentials>> domainCredentialsMap = getDomainCredentialsMap();
if (domainCredentialsMap.containsKey(domain)) {
if (current instanceof IdCredentials || replacement instanceof IdCredentials) {
if (!current.equals(replacement)) {
throw new IllegalArgumentException("Credentials' IDs do not match, will not update.");
}
}
List<Credentials> list = domainCredentialsMap.get(domain);
int index = list.indexOf(current);
if (index == -1) {
Expand Down
34 changes: 17 additions & 17 deletions src/test/java/com/cloudbees/plugins/credentials/domains/DomainTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@

package com.cloudbees.plugins.credentials.domains;

import com.cloudbees.plugins.credentials.impl.DummyIdCredentials;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;

import com.cloudbees.plugins.credentials.CredentialsProvider;
import com.cloudbees.plugins.credentials.CredentialsScope;
import com.cloudbees.plugins.credentials.CredentialsStore;
import com.cloudbees.plugins.credentials.impl.DummyCredentials;

import hudson.security.ACL;
import jenkins.model.Jenkins;
Expand Down Expand Up @@ -82,9 +82,9 @@ public void pathRequirements() {
public void testCredentialsInCustomDomains() throws IOException {
Domain domainFoo = new Domain("domainFoo", "Hostname domain", Arrays.asList(new DomainSpecification[] { new HostnameSpecification("foo.com", "") }));
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");
DummyIdCredentials systemCred = new DummyIdCredentials(null, CredentialsScope.SYSTEM, "systemCred", "pwd", "System 1");
DummyIdCredentials systemCred1 = new DummyIdCredentials(null, CredentialsScope.SYSTEM, "systemCred1", "pwd", "System 2");
DummyIdCredentials systemCredMod = new DummyIdCredentials(systemCred.getId(), CredentialsScope.SYSTEM, "systemCredMod", "pwd", systemCred.getDescription());

CredentialsStore store = CredentialsProvider.lookupStores(Jenkins.get()).iterator().next();

Expand All @@ -96,32 +96,32 @@ public void testCredentialsInCustomDomains() throws IOException {
List<DomainRequirement> reqFoo = Arrays.asList(new DomainRequirement[] { new HostnameRequirement("foo.com") });
List<DomainRequirement> reqBar = Arrays.asList(new DomainRequirement[] { new HostnameRequirement("bar.com") });

assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).isEmpty());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).isEmpty());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).isEmpty());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).isEmpty());

// Add credentials to domains
store.addCredentials(domainFoo, systemCred);
store.addCredentials(domainBar, systemCred1);

// Search credentials with specific domain restrictions
assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).size());
assertEquals(systemCred.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).get(0).getUsername());
assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).size());
assertEquals(systemCred1.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).get(0).getUsername());
assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).size());
assertEquals(systemCred.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).get(0).getUsername());
assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).size());
assertEquals(systemCred1.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).get(0).getUsername());

// Update credential from domain
store.updateCredentials(domainFoo, systemCred, systemCredMod);

assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).size());
assertEquals(systemCredMod.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).get(0).getUsername());
assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).size());
assertEquals(systemCred1.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).get(0).getUsername());
assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).size());
assertEquals(systemCredMod.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).get(0).getUsername());
assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).size());
assertEquals(systemCred1.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).get(0).getUsername());

// Remove credential from domain
store.removeCredentials(domainFoo, systemCredMod);

assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).isEmpty());
assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).size());
assertEquals(systemCred1.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).get(0).getUsername());
assertTrue(CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqFoo).isEmpty());
assertEquals(1, CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).size());
assertEquals(systemCred1.getUsername(), CredentialsProvider.lookupCredentialsInItemGroup(DummyIdCredentials.class, r.jenkins, ACL.SYSTEM2, reqBar).get(0).getUsername());
}
}