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

Jira Bearer Authentication #521

Merged
merged 29 commits into from
May 25, 2023

Conversation

elia-bracci-hs
Copy link
Contributor

@elia-bracci-hs elia-bracci-hs commented Mar 27, 2023

Hi team!

The intent of this PR is to allow JiraRestService to work with Bearer Token authentication!

To do that I implemented new methods and classes:

This implementation could be a starting point for #497

@elia-bracci-hs
Copy link
Contributor Author

Hi @rantoniuk, can you please review my code? Thank you in advance!

Copy link
Contributor

@rantoniuk rantoniuk left a comment

Choose a reason for hiding this comment

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

Hi!

Thank you for contributing, really great to see this incoming PR. I made some small comments about moving files to better places, but apart from that I think this PR is not yet ready for review:

  1. the test case is actually not testing anything at the moment. You don't do any assertions and notice in the PR checks menu that the code coverage dropped:
    Line: 59.00% (-0.23% against target branch)
  2. I don't see any changes to config.jelly, so I'm not sure how this new constructor is actually used and how the site should be configured via UI in this case? Maybe the related credentialsId need to be updated as well.

Take a look at:

Hope that helps!

@elia-bracci-hs
Copy link
Contributor Author

elia-bracci-hs commented Mar 27, 2023

I updated JiraSite class and config.jelly with new boolean variable useBearerAuth. Furthermore, I have some considerations:

  • I implemented JiraSessionFactory class: let me say, that's not a real factory but I think that this pattern can fit the use case
  • JiraRestServiceBearerAuthTest has been created and adapted with new JiraRestService constructor
  • I added useBearerAuth in config.jelly and JiraSite builder (used by form doValidate). However, it seems that constructor used by builder pattern is an old one: do I have to update it too? Basically, it's not clear to me how to grant backward compatibility for JiraSite constructor... If I update the builder one, I have to update all deprecated constructor and adjust JiraSiteTest

@rantoniuk rantoniuk added the feature Release Drafter label label Mar 28, 2023
@elia-bracci-hs
Copy link
Contributor Author

I updated JiraSite class and config.jelly with new boolean variable useBearerAuth. Furthermore, I have some considerations:

  • I implemented JiraSessionFactory class: let me say, that's not a real factory but I think that this pattern can fit the use case
  • JiraRestServiceBearerAuthTest has been created and adapted with new JiraRestService constructor
  • I added useBearerAuth in config.jelly and JiraSite builder (used by form doValidate). However, it seems that constructor used by builder pattern is an old one: do I have to update it too? Basically, it's not clear to me how to grant backward compatibility for JiraSite constructor... If I update the builder one, I have to update all deprecated constructor and adjust JiraSiteTest

I updated JiraSite builder by removing builder method for useBearerAuth and added call to setUseBearerAuth method.

@rantoniuk
Copy link
Contributor

I updated JiraSite class and config.jelly with new boolean variable useBearerAuth. Furthermore, I have some considerations:

  • I implemented JiraSessionFactory class: let me say, that's not a real factory but I think that this pattern can fit the use case
  • JiraRestServiceBearerAuthTest has been created and adapted with new JiraRestService constructor
  • I added useBearerAuth in config.jelly and JiraSite builder (used by form doValidate). However, it seems that constructor used by builder pattern is an old one: do I have to update it too? Basically, it's not clear to me how to grant backward compatibility for JiraSite constructor... If I update the builder one, I have to update all deprecated constructor and adjust JiraSiteTest

That looks great!

I would love you to update everything but that's out of scope for a single PR and would make a mess to review. I propose to do it "in the old way" now and handle the deprecations in another PR when this is merged and you still have power to contribute :)

@elia-bracci-hs
Copy link
Contributor Author

I would love you to update everything but that's out of scope for a single PR and would make a mess to review. I propose to do it "in the old way" now and handle the deprecations in another PR when this is merged and you still have power to contribute :)

Got it 👍 In that case, how do you suggest to proceed?

@elia-bracci-hs
Copy link
Contributor Author

elia-bracci-hs commented Mar 28, 2023

Jenkins job stuck 🤔

@rantoniuk
Copy link
Contributor

When you feel your change is ready, let me know. Note that you can use the incremental build to do a test run in your sandbox instance to see if it works as expected (link is in the checks panel)

@elia-bracci-hs
Copy link
Contributor Author

elia-bracci-hs commented Mar 30, 2023

I was able to test my code on local Jenkins instance and it worked at the first attempt 😎 I tested both bearer token and basic authentication. The only concern I have is that when credentials are unauthorised/bad, I got 401/403 error on Jenkins console but Jenkins jobs succeed anyway. I think that this behaviour is not due to my changes. In the next comment you can see some tests screenshots. I was able to e.g. update Jira ticket workflow both with basic and bearer (Unfortunately, I changed the JiraSite configuration in the second test, without changing job comment 🤣 ). I'll upload further screenshots soon

@elia-bracci-hs
Copy link
Contributor Author

Screenshot 2023-03-30 at 12 26 15
Screenshot 2023-03-30 at 12 26 29
Screenshot 2023-03-30 at 12 26 48

@elia-bracci-hs
Copy link
Contributor Author

@rantoniuk I think that the PR can be merged 👍 I'm confident with the changes and I tested it. Just a note, from the previous screen I changed useBearerAuth checkbox title from Use Bearer authentication to Use Bearer authentication instead of Basic authentication

@elia-bracci-hs
Copy link
Contributor Author

Hi @rantoniuk 😄 Any update on that side? Am I missing something?

@faminepq
Copy link

@rantoniuk any update, pls?

@fmauri-sumologic
Copy link

Hi @rantoniuk , any update?

@rantoniuk
Copy link
Contributor

@EliaBracciSumo sorry for the delay. I'd like to test your changes against our Jira Cloud instance if all looks good. Could you briefly explain how would you configure the plugin after your changes?

I took a quick try to test it already by:

  • spinning up a clean docker-based LTS instance and installing the plugin from incrementals
  • then configured a System credential with a freshly generated PAT token
  • then I validated it works by hitting the Validate Settings button with the Use Bearer authentication instead of Basic authentication unchecked -> the result was success
  • then I checked the Bearer checkbox and I tried to validate again - the validation failed

Am I missing something? If you wish to try it out against the Jira Cloud instance, let me know and I will create an account for you for tests.

@elia-bracci-hs
Copy link
Contributor Author

elia-bracci-hs commented May 9, 2023

@EliaBracciSumo sorry for the delay. I'd like to test your changes against our Jira Cloud instance if all looks good. Could you briefly explain how would you configure the plugin after your changes?

I took a quick try to test it already by:

  • spinning up a clean docker-based LTS instance and installing the plugin from incrementals
  • then configured a System credential with a freshly generated PAT token
  • then I validated it works by hitting the Validate Settings button with the Use Bearer authentication instead of Basic authentication unchecked -> the result was success
  • then I checked the Bearer checkbox and I tried to validate again - the validation failed

Am I missing something? If you wish to try it out against the Jira Cloud instance, let me know and I will create an account for you for tests.

Bearer credentials with Use Bearer authentication instead of Basic authentication checked:
Screenshot 2023-05-09 at 15 01 09
Bearer credentials with Use Bearer authentication instead of Basic authentication unchecked:
Screenshot 2023-05-09 at 15 02 16
🤔
I think that create an account for my tests could be a nice idea 👍

Which versions of the plugin did you install?

@elia-bracci-hs
Copy link
Contributor Author

@rantoniuk have you tried to use PAT with curl?

@rantoniuk
Copy link
Contributor

@rantoniuk have you tried to use PAT with curl?

I didn't because I know PAT method worked before with the Basic auth.

🤔 I think that create an account for my tests could be a nice idea 👍

Please try to access https://jenkins-jira-plugin.atlassian.net/ with your Atlassian account, you should be able to request access.

Which versions of the plugin did you install?

See my previous comment, there is a link to incrementals build (so the version built from this PR).

@elia-bracci-hs
Copy link
Contributor Author

elia-bracci-hs commented May 9, 2023

@rantoniuk you are right. https://community.atlassian.com/t5/Jira-questions/Jira-on-prem-cloud-API-Personal-Access-Token-PAT/qaq-p/2030384 Jira cloud doesn't allow to create PAT but just API token and this one can be used only with Basic Auth. So to recap:

  • on-premise Jira: PAT with Bearer Auth
  • cloud Jira: API token with Basic Auth

Since my update works fine with on-premise Jira, maybe I can put a not on the checkbox I added saying Note: Bearer authentication doesn't work with Jira Cloud, use Basic instead. WDYT?

@rantoniuk
Copy link
Contributor

Yes, that makes sense. I would re-phrase it the other way: "Note: Bearer authentication is only supported in Jira Server, for Jira Cloud leave this unchecked"

@elia-bracci-hs elia-bracci-hs requested a review from rantoniuk May 12, 2023 08:15
@elia-bracci-hs elia-bracci-hs requested a review from rantoniuk May 13, 2023 08:33
@elia-bracci-hs elia-bracci-hs requested a review from rantoniuk May 18, 2023 20:20
@rantoniuk rantoniuk merged commit 851f553 into jenkinsci:master May 25, 2023
@elia-bracci-hs elia-bracci-hs deleted the ebracci-jira-bearer-auth branch May 25, 2023 15:53
@elia-bracci-hs
Copy link
Contributor Author

@rantoniuk thank you for all the support!! In the next days I will proceed (ASAP) with the two pending tasks (repo cleaning and missing apache library)

@rantoniuk rantoniuk linked an issue May 31, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Release Drafter label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to use a Authorization: Bearer instead of Basic
4 participants