Adding credentials should not require a check on CredentialsStore#isDomainsModifiable

[Dohbedoh]

Fix a regression caused by #481. Since this PR, store implementation that define methods to manage credentials but not to manage domains cannot use the Add button anymore... This is the case for CloudBees implementation for Cyberark / Hashicorp Vault for example.

It fails with a 400 and the "Domain is read-only" error on screen. Before this PR, the method was adding credentials properly and send return 400. Since this PR, it is returning early with the error.

Trying to make sense of this condition, I think it is is wrong. Per the javadoc for CredentialsStore#isDomainsModifiable(), it return if the store supports making changes to the list of domains or whether it only supports a fixed set of domains (which may only be one domain). This is about altering Domains. This method does not update or create a domain in any way, it simply add credentials to a selected domain. If the domain selected does not exist it fails with Store does not have selected domain. Also if the domain is not modifiable, you would not be able to select it in the dropdown:\

So per my understanding, this condition should be removed.

Testing done

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[rsandell]

Well this check has been there since before version 2.0 (2016) so what has changed now to cause problems? The check looks very explicit to me.

[rsandell]

And it looks kinda correct to me that if a domain isn't modifiable you shouldn't be able to add any credentials to it. For example you can't add an object to an unmodifiable list.

[dwnusbaum]

@rsandell FWIW, separately (BEE-51772), Allan claimed that while present, the check in the old code was not functional, and #481 changed that behavior:

Before that change, the 400 was already happening but silent. Now the credentials is not created anymore..

[Dohbedoh]

Well this check has been there since before version 2.0 (2016) so what has changed now to cause problems? The check looks very explicit to me.

Since the change from , we return early within the condition:

• https://github.com/jenkinsci/credentials-plugin/blob/1371.vfee6b_095f0a_3/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java#L602-L604.

Before that change, the HTTP 400 response was generated but we did not return so the rest of the method got executed as well:

And it looks kinda correct to me that if a domain isn't modifiable you shouldn't be able to add any credentials to it.

Seems to me that this condition was not effective all along..

And it looks kinda correct to me that if a domain isn't modifiable you shouldn't be able to add any credentials to it. For example you can't add an object to an unmodifiable list.

My understanding of the javadoc from this method and the usage of that method is that when a CredentialsStore is "domain-unmodifiable", you simply cannot create/delete/modify domains. But you can manage credentials within the read only domain(s). The workaround for that regression for example, is to add credentials from Manage Jenkins > Credentials for root credentials or Folder's Credentials page for folder credentials.

[car-roll]

I've been going back and forth on this. At first my "feeling" was that if a domain is used to categorize credentials, and we are setting that domain to unmodifiable, my expectation is that I should not be adding additional credentials to that domain. But like Alan said, the javadoc says something entirely different:
Identifies whether this CredentialsStore supports making changes to the **list of domains** or whether it only supports **a fixed set of domains** (which may only be one domain).

It may have been misapplication of this particular check? I see there is a check if you can add a credential to a store, but really nothing else for domain.