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

Jump to bottom

[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 2 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
15 changes: 13 additions & 2 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 @@ -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());

Expand Down Expand Up @@ -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();

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("SECURITY-3252")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong reference?

Suggested change
@Issue("SECURITY-3252")
@Issue("JENKINS-72611")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, updated in eb2d6ca

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));
}
}
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
2 changes: 1 addition & 1 deletion 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 @@ -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();

Expand Down
9 changes: 6 additions & 3 deletions src/test/java/com/cloudbees/plugins/credentials/impl/DummyCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -35,15 +34,19 @@
/**
* A test credentials impl.
*/
public class DummyCredentials extends BaseCredentials implements UsernamePasswordCredentials {
public class DummyCredentials extends BaseStandardCredentials implements UsernamePasswordCredentials {

private final String username;

private final Secret password;

@DataBoundConstructor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the old constructor remain the DataBoundConstructor or should that move to the new one? Do we have UI tests that even use this? Should a null ID be a problem?

Copy link
Member

@jtnord jtnord Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have UI tests that even use this

there's a jelly file at least so at some point in time Stephen went to the effort of ensuring the UI at least worked...

<st:include page="id-and-description" class="${descriptor.clazz}"/>

Should a null ID be a problem?

Shouldn;t be as BaseStandardCredentials will generate a uniqueID if null (so long as not one is expecting anything stable here!).

credentials-plugin/src/main/java/com/cloudbees/plugins/credentials/impl/BaseStandardCredentials.java

Lines 78 to 82 in 7eb51b3

public BaseStandardCredentials(@CheckForNull String id, @CheckForNull String description) {
super();
this.id = IdCredentials.Helpers.fixEmptyId(id);
this.description = Util.fixNull(description);
}

credentials-plugin/src/main/java/com/cloudbees/plugins/credentials/common/IdCredentials.java

Lines 94 to 96 in 7eb51b3

public static String fixEmptyId(@CheckForNull String id) {
return StringUtils.isEmpty(id) ? UUID.randomUUID().toString() : id;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just found out that https://github.com/jenkinsci/credentials-plugin/blob/master/src/test/java/com/cloudbees/plugins/credentials/impl/DummyIdCredentials.java exists. I'll revert the change here and only adapt affected tests to use DummyIdCredentials.java

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);
}
Expand Down
Loading