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

Allow SecurityRealm#getUserIdStrategy to be customizable #493

Conversation

eva-mueller-coremedia
Copy link
Contributor

@eva-mueller-coremedia eva-mueller-coremedia commented Dec 23, 2024

Allow SecurityRealm#getUserIdStrategy to be customizable. This way, the plugin can be aligned with the ID strategy of the configured Identity Provider.

This is necessary, when the configured Identity Provider works with case-sensitive usernames. The Jenkins default is case-insensitive. The default leads to unwanted behaviour, when the configured Identity Provider has case-sensitive usernames, i.e., if a user with name test-user logs in, then a new log-in by the user Test-User will return the data of user test-user.

Testing done

  • Extend PluginTest.java to cover the case-sensitive and case-insensitive case.

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

This way, the plugin can be aligned with the ID strategy of the configured Identity Provider.
@eva-mueller-coremedia eva-mueller-coremedia requested a review from a team as a code owner December 23, 2024 20:41
Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.05%. Comparing base (84359ac) to head (11877ae).
Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #493      +/-   ##
============================================
+ Coverage     71.73%   72.05%   +0.32%     
- Complexity      222      226       +4     
============================================
  Files            17       17              
  Lines          1033     1045      +12     
  Branches        148      148              
============================================
+ Hits            741      753      +12     
  Misses          201      201              
  Partials         91       91              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

There is also the group strategy to cover.
There is code with all of this tested and ready that would conflict, so would recommend waiting for that rather than duplicating

@eva-mueller-coremedia
Copy link
Contributor Author

Hi @jtnord - thanks for having a look at this PR.

There is also the group strategy to cover.
There is code with all of this tested and ready that would conflict, so would recommend waiting for that rather than duplicating

Unfortunately, I do not understand your remark. Do you mean: There is a not-yet-pushed branch which will cover the customizable ID strategy?

@jtnord
Copy link
Member

jtnord commented Dec 23, 2024

Hi @jtnord - thanks for having a look at this PR.

There is also the group strategy to cover.
There is code with all of this tested and ready that would conflict, so would recommend waiting for that rather than duplicating

Unfortunately, I do not understand your remark. Do you mean: There is a not-yet-pushed branch which will cover the customizable ID strategy?

there is code that covers the id strategy changes for both users and groups that has not been pushed but will be shortly

@eva-mueller-coremedia
Copy link
Contributor Author

Hi @jtnord - thanks for having a look at this PR.

There is also the group strategy to cover.
There is code with all of this tested and ready that would conflict, so would recommend waiting for that rather than duplicating

Unfortunately, I do not understand your remark. Do you mean: There is a not-yet-pushed branch which will cover the customizable ID strategy?

there is code that covers the id strategy changes for both users and groups that has not been pushed but will be shortly

Ok. Good to know. Looking forward to see the feature soon.

I assume, I can close this PR, right?!

@eva-mueller-coremedia
Copy link
Contributor Author

Already implemented but not yet pushed.

@eva-mueller-coremedia eva-mueller-coremedia deleted the custom-user-id-strategy branch December 24, 2024 10:12
@jtnord
Copy link
Member

jtnord commented Jan 6, 2025

@eva-mueller-coremedia do you have a Jenkins Jira ID?

@eva-mueller-coremedia
Copy link
Contributor Author

@jtnord No, I have no Jenkins Jira ID

@jtnord
Copy link
Member

jtnord commented Jan 6, 2025

@jtnord No, I have no Jenkins Jira ID

@eva-mueller-coremedia would you mind signing up at https://accounts.jenkins.io/signup?

@eva-mueller-coremedia
Copy link
Contributor Author

@jtnord No, I have no Jenkins Jira ID

@eva-mueller-coremedia would you mind signing up at accounts.jenkins.io/signup?

Done... My user name is eva_mueller

@jtnord
Copy link
Member

jtnord commented Jan 6, 2025

@jtnord No, I have no Jenkins Jira ID

@eva-mueller-coremedia would you mind signing up at accounts.jenkins.io/signup?

Done... My user name is eva_mueller

@eva-mueller-coremedia can you perform a login on Jira, this is needed so that Jira sees your account. Appologies, I did not realize this was a necessary step.

@eva-mueller-coremedia
Copy link
Contributor Author

Successfully logged in...

@eva-mueller-coremedia
Copy link
Contributor Author

@jtnord - thanks for sharing. Looking forward to see the solution in a release! Thanks a lot.

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

Successfully merging this pull request may close these issues.

2 participants