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.

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

feat: Use credentials plugin for managing authentication credentials #325

Merged
merged 6 commits into from
Nov 16, 2021

Conversation

slide
Copy link
Member

@slide slide commented Oct 22, 2021

This switches the MailAccount class to use credentials instead of directly storing the username/password. There is an upgrade path from the old settings to using credentials.

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

  • 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
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Hacktoberfest submission

This switches the MailAccount class to use credentials instead of directly storing the username/password. There is an upgrade path from the old settings to using credentials.
@slide
Copy link
Member Author

slide commented Oct 22, 2021

I'll fix up the tests

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks great overall, and I think users will be delighted to have this functionality. I just see one minor compatibility problem, after which I'm happy to merge and release this.

The changes to src/test/resources/hudson/plugins/emailext/ExtendedEmailPublisherDescriptorTest/persistedConfigurationBeforeDefaultAddress/hudson.plugins.emailext.ExtendedEmailPublisher.xml and src/test/resources/hudson/plugins/emailext/ExtendedEmailPublisherDescriptorTest/persistedConfigurationBeforeJCasC/hudson.plugins.emailext.ExtendedEmailPublisher.xml don't seem quite right. These tests are intended to verify that existing serialized state from old versions can still be loaded. Since the plugin should always remain compatible, these files shouldn't ever be changed. So I would suggest reverting the changes to these files.

Speaking of which, since this PR changes the serialization format and adds a new readResolve method, it would be good to write a similar test. It could be modeled after persistedConfigurationBeforeDefaultAddress and persistedConfigurationBeforeJCasC. If you look at those tests, you can see they use @LocalData with a comment stating "Local data created using Email Extension 2.71 with the following code" and then a bunch of commented out code. The same could be done here using local data from the current released version (with the old username and password). The commented out code isn't strictly necessary, but it is a nice touch. Once you have the hudson.plugins.emailext.ExtendedEmailPublisher.xml and jenkins.model.JenkinsLocationConfiguration.xml files from the old version, you can stick them in src/test/resources like the ones for persistedConfigurationBeforeDefaultAddress and persistedConfigurationBeforeJCasC and then write new tests with @LocalData. Some test cases to consider:

  • Loading older state without a corresponding credential should create a new migrated credential
  • Loading older state with a corresponding credential should use the existing credential

@slide
Copy link
Member Author

slide commented Oct 26, 2021

I will get this done soon, thanks for the review.

@slide
Copy link
Member Author

slide commented Oct 26, 2021

For the two tests you mention, the first case is actually tested via the existing tests (with some updates) since the data is in the older format. I just need to update the test to check for the credential ID (I should check for the text that is used in the update process in readResolve).

I will add a test for the second case soon.

@basil basil merged commit 3ed0b44 into jenkinsci:master Nov 16, 2021
@slide slide deleted the credentials_plugin branch November 16, 2021 22:38
@slide
Copy link
Member Author

slide commented Nov 16, 2021

Thanks!

@basil
Copy link
Member

basil commented Nov 16, 2021

Released in 2.85.

@MarcusUA
Copy link

This change has introduced strange behavior, it migrated creds to Global Credentials (login is correct for sure), but it fails to use them, and uses root with no password:

DEBUG: getProvider() returning javax.mail.Provider[TRANSPORT,smtp,com.sun.mail.smtp.SMTPTransport,Oracle]
DEBUG SMTP: need username and password for authentication
DEBUG SMTP: protocolConnect returning false, host=smtp.office365.com, user=root, password=
DEBUG SMTP: useEhlo true, useAuth true
DEBUG SMTP: trying to connect to host "smtp.office365.com", port 587, isSSL false

After I created new creds entry with correct user, it started to work, but still acting strange:

Sending email to: redacted@redacted.com
DEBUG: getProvider() returning javax.mail.Provider[TRANSPORT,smtp,com.sun.mail.smtp.SMTPTransport,Oracle]
DEBUG SMTP: need username and password for authentication
DEBUG SMTP: protocolConnect returning false, host=smtp.office365.com, user=root, password=
DEBUG SMTP: useEhlo true, useAuth true
DEBUG SMTP: trying to connect to host "smtp.office365.com", port 587, isSSL false

but few lines later is same log:

DEBUG SMTP: protocolConnect login, host=smtp.office365.com, user=redacted@redacted.com, password=
DEBUG SMTP: Attempt to authenticate using mechanisms: LOGIN PLAIN DIGEST-MD5 NTLM XOAUTH2
DEBUG SMTP: Using mechanism LOGIN
DEBUG SMTP: AUTH LOGIN command trace suppressed
DEBUG SMTP: AUTH LOGIN succeeded

@slide
Copy link
Member Author

slide commented Nov 19, 2021

Can you submit an issue in jira and attach your config.xml for the emailext plugin (make sure to remove any sensitive info).

@MCMicS
Copy link

MCMicS commented Nov 23, 2021

Also have same issue after Update plugin.
I've create an issue and try to reproduce it again on other machine

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

@slide
Copy link
Member Author

slide commented Nov 23, 2021

Just to clarify, you had existing username/password stored for your connection, you upgraded the plugin and restarted Jenkins and then started a build and the creds were wrong?

@MCMicS
Copy link

MCMicS commented Nov 23, 2021

Yes i had valid credentials. After update it was migrated to credentials. After restart the serve and started a build no mail was sent

@matthiasklein
Copy link

Also have the same problem after updating the plugin.

I had to create the credentials again to make them work. The automatically converted ones did not work.

@cfischer-oo-software
Copy link

cfischer-oo-software commented Dec 8, 2021

Is there any solution or workaround available?

Using 2.85. Rollback to 2.84 results in other errors when sending email.

I'm reading "had to create the credentials again to make them work", but: In the Jenkins System Configuration ("E-mail Notification") I can enter username/password only, a cannot assign any saved credential. Re-entering username/password does not change anything. The log of a simple freestyle test job sending an email shows:

10:53:26 DEBUG: getProvider() returning javax.mail.Provider[TRANSPORT,smtp,com.sun.mail.smtp.SMTPTransport,Oracle]
10:53:26 DEBUG SMTP: need username and password for authentication
10:53:26 DEBUG SMTP: protocolConnect returning false, host=[the-servername], user=[not the value specified in system configuration], password=null
10:53:26 AuthenticationFailedException message: failed to connect, no password specified?

Sending the test email from System Configuration succeeds.

Any help is greatly appreciated.

@arnisjuraga
Copy link

@cfischer-oo-software did you resolve this issue?
I have exactly the same problem. using version 2.88.

Now I am upgrading to 2.89 to see if it helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants