[JENKINS-72611] Forbid editing credentials ID

[yaroslavafenkin]

https://issues.jenkins.io/browse/JENKINS-72611

Testing done

Tested interactively, made sure that two credentials cannot have the same ID.

Submitter checklist

Edit tasklist title

Beta Give feedback Tasklist Submitter checklist, more options

• Rename
• Copy markdown
• Delete tasklist

Delete tasklist

Delete tasklist block?

Are you sure? All relationships in this tasklist will be removed.

Cancel Delete

1. Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!

   Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
2. Ensure that the pull request title represents the desired changelog entry

   Ensure that the pull request title represents the desired changelog entry
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
3. Please describe what you did

   Please describe what you did
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
4. Link to relevant issues in GitHub or Jira

   Link to relevant issues in GitHub or Jira
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
5. Link to relevant pull requests, esp. upstream and downstream changes

   Link to relevant pull requests, esp. upstream and downstream changes
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

   Ensure you have provided tests - that demonstrates feature works or fixes the issue
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove

Add item to Submitter checklist

[jglick]

FTR

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

Lines 50 to 85 in cf0a900

	/**
	* The contract implementations of {@link #equals(Object)} and {@link #hashCode()} that implementations of
	* this class must use as the basis for equality tests and as the hash code.
	*
	* @since 1.6
	*/
	final class Helpers {
	/**
	* Standard {@link #equals(Object)} implementation.
	*
	* @param self the {@code this} reference.
	* @param o the other object.
	* @return true if equal.
	*/
	public static boolean equals(IdCredentials self, Object o) {
	if (self == o) {
	return true;
	}
	if (!(o instanceof IdCredentials)) {
	return false;
	}
	IdCredentials that = (IdCredentials) o;
	return self.getId().equals(that.getId());
	}
	/**
	* The standard {@link #hashCode()} implementation.
	*
	* @param self the {@code this} reference.
	* @return the hash code.
	*/
	public static int hashCode(IdCredentials self) {
	return self.getId().hashCode();
	}

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

Lines 116 to 130 in cf0a900

	/**
	* {@inheritDoc}
	*/
	@Override
	public final int hashCode() {
	return IdCredentials.Helpers.hashCode(this);
	}
	/**
	* {@inheritDoc}
	*/
	@Override
	public final boolean equals(Object o) {
	return IdCredentials.Helpers.equals(this, o);
	}

[alecharp]

Hello, this created a compatibility issue with plain-credentials plugin.
Since this was added, the BaseTest of plain-credentials are failing.

[jtnord]

how so - the list is of type Credentials not IdCredentials so that would not break with a big bad angry jenkins.
At worst it would not find current, as the list would use equals and it is up to current credential may have been correctly obtained by it's name or otherwise in the code path.

[jtnord]

indeed this causes regressions

->

failed to update a description of a credential. LoadFocus.com API key

[jglick]

@jtnord What concrete Credentials implementations are not instanceof IdCredentials? I was under the impression that was essentially impossible and it was only a technicality that getId had not been pushed up into Credentials itself. (How would you even select credentials without an id?)

the BaseTest of plain-credentials are failing

https://github.com/jenkinsci/plain-credentials-plugin/blob/ce8710821f17a043d91b448cf19b8568935d0167/src/test/java/org/jenkinsci/plugins/plaincredentials/BaseTest.java#L107? That just sounds like a meaningless test.

[jtnord]

@jtnord What concrete Credentials implementations are not instanceof IdCredentials?

for example -> https://github.com/jenkinsci/loadfocus-jmeter-load-testing-integration-plugin/blob/d2d2af5b0af276932dfb6c8191d3bb13791d6bc7/src/main/java/com/loadfocus/jenkins/impl/LoadCredentialImpl.java there may be companies with other implementations.

I was under the impression that was essentially impossible

Nope this works :)

and it was only a technicality that getId had not been pushed up into Credentials itself. (How would you even select credentials without an id?)

credentials-plugin/src/main/java/com/cloudbees/plugins/credentials/CredentialsStoreAction.java

Lines 744 to 748 in 7eb51b3

	while (result.containsKey("index-" + index)) {
	index++;
	}
	id = "index-" + index;
	index++;

you get a dummy ID based on the index in the list. For update and delete.
For usage you may be thinking too much about credentials-binding.

A plugin can perfectly ask for any credential of Type X for domain F and get the list CredentialsProvider.lookupCredentials and then do whatever it wants. (expect there is only one and use the first, or use a custom different method of identifying the suitable one, or just be truly random)

IDs are the future (or the now) but they are not /currently/ mandatory.

the BaseTest of plain-credentials are failing

https://github.com/jenkinsci/plain-credentials-plugin/blob/ce8710821f17a043d91b448cf19b8568935d0167/src/test/java/org/jenkinsci/plugins/plaincredentials/BaseTest.java#L107? That just sounds like a meaningless test.

[jglick]

Hmm https://github.com/jenkinsci/loadfocus-jmeter-load-testing-integration-plugin/blob/d2d2af5b0af276932dfb6c8191d3bb13791d6bc7/src/main/java/com/loadfocus/jenkins/LoadCredential.java#L13 looks like it was meant to extend IdCredentials. Is that the source of the screenshot above?

expect there is only one and use the first

Are there some examples of this? I can only think of cases where config includes a credentialsId.

🤷 anyway I guess it should be simple enough to fix things up to just ignore non-IdCredentials. Somewhere we should put a compile-time and/or runtime warning about these, because the intent was certainly for all implementations to be IdCredentials.

[jtnord]

I guess it should be simple enough to fix things up to just ignore non-IdCredentials

yes - we only need to do the comparison if one or other of the credentials is an IDCredential.

Somewhere we should put a compile-time and/or runtime warning about these, because the intent was certainly for all implementations to be IdCredentials.

if we want to kill non ID based credentials and cleanup the code then I am ok with doing that in a PR that is clearly saying "remove support for obsolete implementations of Credentials" so long as it is just doing that, and we also fixup the knwon fallout (which is not huge but there is some).
I'm not ok with killing it by accident and without release notes or an upgrade path. FTR-> https://issues.jenkins.io/browse/JENKINS-72618 and https://issues.jenkins.io/browse/JENKINS-72617 for LoadCredential