[JENKINS-72209] Fix initially missing authorities
This should fix the following issues:
- https://issues.jenkins.io/browse/JENKINS-63296
- https://issues.jenkins.io/browse/JENKINS-72209
- https://issues.jenkins.io/browse/JENKINS-72268
- https://issues.jenkins.io/browse/JENKINS-73060
Unlike https://github.com/jenkinsci/github-oauth-plugin/pull/256, I don't believe this will cause a performance regression.
In https://github.com/jenkinsci/github-oauth-plugin/pull/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.
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:
- Log out
- Log in via Github
- Go to http://$JENKINS_URL/user/$username/
- Check that you have Groups
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
- [X] Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
- [X] Ensure that the pull request title represents the desired changelog entry
- [X] Please describe what you did
- [X] Link to relevant issues in GitHub or Jira
- [X] Link to relevant pull requests, esp. upstream and downstream changes
- [X] Ensure you have provided tests that demonstrate the feature works or the issue is fixed
Unlike #256, I don't believe this will cause a performance regression.
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 gh checks were simply masking this out since the rest of the cached information is likely still correct.
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 getGitHub but it doesn't really look that heavy to me.
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 loadRepository. This would only be an issue if Github Commiter Authorization Strategy is used and Use GitHub repository permissions is enabled and a user is trying to access a job that's on a private repo the user is not part of.
@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:
authorizationStrategy:
github:
...
useRepositoryPermissions: true
security:
...
queueItemAuthenticator:
authenticators:
- global:
strategy: "triggeringUsersAuthorizationStrategy"
Have you tried using this PR or #256 to see if it fixes it for you?
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 triggeringUsersAuthorizationStrategy in my setup.
Here is my setup if you need more clarification:
https://github.com/lprimak/infra/blob/main/scripts/jenkins/downloaded-config.yaml
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!