-
Notifications
You must be signed in to change notification settings - Fork 165
[JENKINS-72209] Fix initially missing authorities #299
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
base: master
Are you sure you want to change the base?
Conversation
In jenkinsci#61, use of `gh` was replaced by `getGitHub()` but the check for `gh != null` was not removed. `getGitHub()` already ensures that it's present so there's no need to do the check. The check was causing calls to `loadUser`, etc to return `null` if a token was restored and nothing that transitively calls `getGitHub()` has been called yet. In particular, when a user logs in, their authorities will be empty because GithubSecurityRealm tries to `loadUser` in `loadUserByUsername2` and sets authorities to empty if the user is `null`. Only later, when something transitively calls `getGitHub()`, will the user have the authorities set.
Upon further thought, this might cause the same performance regression as #256. It's a bit hard to tell without knowing the exact setup but it's possible that there's a lack of caching in some part of the authentication flow. The improper |
Hm, looking at the possible flows, I'm not really sure what could be the cause of the performance regression. Logs from the affected user would have been nice to have possibly. I think this PR is slightly better since it doesn't eagerly construct |
In the case of my enterprise setup authorities disappear when Okta SSO is integrated on GitHub. When you log into GitHub you are forced through the SSO flow. Within Jenkins it loses its authorities until you manually log out and log in again. A fix for this might be a required authority to force a new login. e.g. if authority X is missing, then behave as if logged out and go through oauth login flow again. |
Have you tried using this PR or #256 to see if it fixes it for you? |
Hm, regarding the performance issues, I think the only thing that's not cached is a negative result for @lprimak you ran into issues with #256, would you mind testing if this PR gives you the same issues? Alternatively, could you give details about the authorization strategy in your Jenkins setup? Are you using the following:
|
I will try these; I am currently in the process of upgrading (after a couple years of keeping the same plugin set). I will report back for testing with this PR. |
Hi, I just noticed this. I'll try to test it this week. |
Yes, I am using the |
I did a quick test and don't see any issues. LGTM as far as I am concerned. Thank you for checking! |
I apologize for the long delay. I missed the notification somehow :( |
Thanks for checking! |
This should fix the following issues:
Unlike #256, I don't believe this will cause a performance regression.
In #61, use of
gh
was replaced bygetGitHub()
but the check forgh != null
was not removed.getGitHub()
already ensures that it's present so there's no need to do the check.The check was causing calls to
loadUser
, etc to returnnull
if a token was restored and nothing that transitively callsgetGitHub()
has been called yet.In particular, when a user logs in, their authorities will be empty because GithubSecurityRealm tries to
loadUser
inloadUserByUsername2
and sets authorities to empty if the user isnull
. Only later, when something transitively callsgetGitHub()
, will the user have the authorities set.Testing done
I've added a unit test that checks that
loadUser
works after deserialization.I also tested manually on a local Jenkins instance with:
Prior to this PR, Groups would be empty immediately after logging in. Only a few minutes later it might get populated. With this PR, groups will be populated even after logging in immediately.
Submitter checklist