[JENKINS-37655] Use Credentials API for SMTP authentication

[chriskilding]

Implements JENKINS-37655

To do:

• Basic implementation
• Tests
• Credential tracking
• I18N of relevant messages
• Documentation
• Manual test on a real Jenkins

[chriskilding]

Build is currently held up because the Jenkins plugin parent POM artifact does not exist in Jenkins Incrementals.

[2020-06-09T11:37:07.676Z] org.apache.maven.model.resolution.UnresolvableModelException: Could not find artifact org.jenkins-ci.plugins:plugin:pom:3.55 in incrementals (https://repo.jenkins-ci.org/incrementals/)

[chriskilding]

The current design is a backwards-incompatible replacement. It adds SMTPAuthentication#credentialsId and the surrounding Credentials lookups/UI logic. It removes the SMTPAuthentication#username and SMTPAuthentication#password fields, and their accessors. As such it would mean a major version bump, and anyone using SMTP authentication would need to update the mailer-plugin config on their Jenkins.

That's quite a change so it is worth considering whether backwards compatibility can be retained, and whether this would be advisable:

• Could users get confused between the methods?
• Might they try to use both, not knowing that only one should be picked?
• Is it a good idea to allow users to continue storing SMTP logins in the legacy insecure format?

[kerogers-cloudbees]

To borrow @Vlatombe 's directions for maintaining backwards compatibility when I did this migration with a closed source plugin:

To handle the migration of old data, keep the old fields with transient and deprecate them.

Then you'll need to implement

protected Object readResolve() { 
    // Migration code 
    return this; 
}

in order to create credentials from prior username/password, then set these to null to do it only once.

[chriskilding]

Makes sense.

I've got a potential config data migration coming up in a different plugin and have been thinking about how best to test it. Migrations are a fact of life for many plugins so I'm looking for a testing mechanism that would make migrations a first-class citizen of the test suite, making it easy to write migration tests for any plugin, and indeed giving new developers a one-stop shop to see all the migrations currently active in any plugin. (They shouldn't have to go digging for deprecated or transient fields.)

I didn't see any such mechanism so far, so I've proposed a MigrationTest helper, loosely based on the style of Rails' ActiveRecord Migrations, in the PR for my other plugin. If there's mileage in the idea, we could develop it further, extract it into jenkins-test-harness (or similar), and then use it here too.

Feedback welcomed either below or in jenkinsci/aws-secrets-manager-credentials-provider-plugin#32

[chriskilding]

I'm in the middle of adding the migration code, and have come across a couple of issues:

• While we have the legacy values to populate a new StandardUsernamePasswordCredentials instance in readResolve(), a credential also needs an ID. But inside readResolve() we cannot know what ID the user wants that credential to have.
• Some credentials providers / stores are read-only, and will not allow us to add a new credential in readResolve() anyway.

Advice appreciated

[chriskilding]

I've now added migration code that will attempt to create a credential with a random ID from the legacy username and password fields. It specifically tries to save it to the on-disk SystemCredentialsProvider, because this provider is always present when the credentials plugin is present, and because this provider allows insertion of new credentials.

In the unlikely event that a credential already exists with the generated ID, the migration code retries 10 times (this is arbitrary and can be changed), generating new IDs each time.

Ready for another code review

[chriskilding]

I ignored the return value, as a failure should be expressed by the method throwing an exception. Is this alright?

[chriskilding]

Not sure if this is needed, since I have dedicated migration tests for it now.

[chriskilding]

I've put this here since this legacy constructor should only be called by the migration code in Mailer, and I wanted to avoid constructing credentials in 2 places (Mailer#readResolve and SMTPAuthentication#readResolve). Is this alright?

[chriskilding]

Do I need to do anything extra for credentials tracking here, or does a call to lookupCredentials append to the tracking log automatically?

[chriskilding]

@alecharp would you be able to take a look at this? If I can get a resolution to the questions, I can rebase it and get it ready to merge.

[SamSpiri]

@alecharp please look into this PR. That's a nice feature for us to. We use sendgrid and want to hide the token. We have cloudbees setup ;)

[alecharp]

I'm sorry I haven't looked at this before. This would be a nice addition to the plugin. However, now there are conflicts.

@chriskilding could you re-work on this?

[chriskilding]

Hi @alecharp, I've rebased it and caught up with recent changes.

The 4 failing tests call authentication.getUsername() and authentication.getPassword(), which in turn just reflect the value of the deprecated username and password fields. The thing is, migration moves the values for those fields into a credential, so those deprecated fields will now always be null.

Couple of options to fix:

The first is to remove SMTPAuthentication#getUsername() and SMTPAuthentication#getPassword() entirely. This would be a breaking change to the API.

The second is to provide backwards compatibility, by calling CredentialsProvider.lookupCredentials(<new credential ID>) in those two getters. If a remote credentials provider is in play, the getPassword() lookup could be much slower than it is today as an online lookup to the backing store would be needed. That could surprise calling code that is used to an immediate lookup.

[chriskilding]

@alecharp could you take a look at this?