-
Notifications
You must be signed in to change notification settings - Fork 164
[JENKINS-47113] Populate the authorities after a successful authentication to Github #87
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
[JENKINS-47113] Populate the authorities after a successful authentication to Github #87
Conversation
- add failing test to reproduce the case - after using the github auth, we retrieve the groups normally but for next auth with ApiToken, we do not receive the groups
- add fireLoggedIn after GithubAuth - add fireAuthenticated after GithubToken
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
// | ||
// wc = j.createWebClient(); | ||
// // in case of GithubToken there is no session so we retrieve the same result | ||
// makeRequestWithAuthCodeAndVerify(encodeBasic(aliceLogin, aliceApiRestToken), "alice", Arrays.asList("authenticated", "org-a", "org-a*team-b")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the commented out code be deleted? It looks like that code is in testBob_usingGithubLogin
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the same. In such case the commented code is related to the TODO in the use of GitHub token that should or should not trigger the loggedIn event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok that makes sense.
|
||
@Issue("JENKINS-47113") | ||
@Test | ||
public void testGroupPopulation() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider moving this stuff to an @Before
method and adding @Test
to testAlice_usingGithubToken
and testBob_usingGithubLogin
, and maybe renaming those methods to testLoginUsingGithubApiToken
and testLoginUsingGithubOauth
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For that point I followed some previous comments from Jesse, telling me it's better to have a single test in order to avoid double Jenkins startup.
But in this plugin, there's not a lot of tests, so you're right I think, will do the change 👍
SecurityListener.fireAuthenticated(new GithubOAuthUserDetails(token.getName(), github.getAuthorities())); | ||
|
||
//TODO do we want to trigger a loggedIn event when using GitHub token ? | ||
// SecurityListener.fireLoggedIn(token.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SecurityListener#loggedIn
JavaDoc says:
Fired when a user has logged in via the web UI.
So the question is whether this code is called from the UI, right? I'll do some local testing, would be good to decide either way and remove the TODO before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the use of GitHub login, it's when we use the process showing you the GitHub login page etc. It's the case of "Web UI" authentication.
But in the case of GitHub token, we are in the same case as for the API Token in core, it's not trough Web UI but by REST calls, hence my TODO.
FTR I proposed changes in the JavaDoc of loggedIn
in #3074 from "via the web UI" to "notion of cache / storage of authentication"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the link to the proposed JavaDoc changes. I looked through other SecurityRealm
implementations (listed here), but didn't find any that called loggedIn
for a token-based authentication. The main reason to call fireLoggedIn would be to populate the last granted authorities, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially yes, but as you mentionned it's not a behavior that is really desired. I will remove the comments 👍
- add a TODO on the GithubAPITest that use Ignore but should use Assume
What happens to the granted authorities when a user's access is revoked from an organization or team but they don't log in through the web UI? |
…just using Github token
This is acting as a cache, so previous access remains. The user record in Jenkins can be deleted, this will remove the associations (even if the user might reappear due to e.g. occurring in SCM). |
Are you referring to Think of the following scenario from an organization using GitHub Enterprise:
I do not think this change will be accepted if it doesn't properly match the user's state of access with GitHub. Idea: Why not store the GitHub API token generated from a user login during the OAuth process with the user record? This could then be used to determine authorizations and even take advantage of the caches provided for GitHub organizations and teams in the plugin. When the Jenkins API token is used; it could use the cached authorization token from the user login to query GitHub directly for organizations and teams. |
The case you mentionned currently did not work either. With current version if you use Github Token or Login you will have the correct authorization but with Jenkins API Token, you will simply be
From my PoV this will be a security issue. Storing token that could be retrieved is a bad idea from a security perspective. |
Yes if using a Jenkins API token, but a user is not granted additional authorization that they don't have. Granting authorizations which have been revoked in GitHub is not desired behavior. The case I mentioned works if the user is using a GitHub personal access token to interact with the Jenkins API.
I don't really see much difference from plugins configuring tokens for authorization with GitHub. Other GitHub applications store this token to interact with GitHub on the user's behalf (of which the user consents). As long as the stored token is secured using the credentials API (or at the very least |
The credentials api is not a match for storage of such details. I would recommend storing the oauth token in Secret or using a similar mechanism with a realm specific key derived from the instance master key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess? Do not understand well enough to review seriously.
* https://developer.github.com/v3/orgs/ | ||
* https://developer.github.com/v3/orgs/teams/ | ||
*/ | ||
private static class MockGithubServlet extends DefaultServlet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can also try using WireMock. Cf. discussion in jenkinsci/jenkins-test-harness#39 and linked PRs.
From out-of-band discussion: I suspect that the root problem here is that JENKINS-20064 should never have been using
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking because of security issue. Authorizations should not be granted to a user when authorized access is revoked in GitHub.
@samrocketman you're right, I will propose a modification in that way: i.e. storing the oauth token in Secret as mentioned by Stephen and not triggering the loggedIn event for the moment. Ideally we desire to have such event triggered for audit / log purpose. |
@Wadeck I appreciate you opening this change and starting this dialogue. I agree the Jenkins token was a bit of a black sheep. Hopefully, this gets resolved and other forms of auth in Jenkins benefit as well (such as SSH keys). |
…r in a UserProperty. - also the loggedIn event trigger is removed to avoid cache problem with LastGrantedAuthorities
@samrocketman new version proposed: we store the access token in a @stephenc can you just check the usage of @jglick I investigated your point and I agree the implementation of |
I shall review this evening after work hours. I'll also build and play with it. Thanks! |
Sure, I'll get that information for you this evening after work. |
Expand the following details to see plugin versions Jenkins version and plugins versions
By this point the versions of Jenkins and plugins are out of date. I can try upgrading and cut a new build of your pull request. |
I'll try to be as exact as possible. Even with the latest build and latest version of Jenkins and plugins, the issue still remains. Versions of Jenkins and plugins tested SetupFrom the this page https://github.com/samrocketman/jenkins-bootstrap-github-oauth/releases/tag/pr-87-30d61ed Download your choice of RPM or DEB. It contains a packaged version of Jenkins and every plugin I had installed during testing. Download the Provision your own VM which supports RPM or DEB packages. It should have at least 6GB of RAM and 2 CPUs are recommended. Alternatively, if you prefer vagrant then follow the instructions in the jenkins bootstrap README. If you install the package Jenkins can be started as a service with the following command:
Configure JenkinsWhen Jenkins is listening on http://localhost:8080/ or wherever you set up Jenkins. The following needs to be done.
Testing Jenkins CLIAssuming you're using http://localhost:8080/ for Jenkins run the following commands to test Jenkins CLI.
|
@samrocketman after lots of efforts I reproduce your problem. I used a GitHub account with not sufficiant amount of org/team to be able to reproduce initially, so using local Jenkins, Vagrant one or even within a VM, the results is always the same ~15s to have the response. The root cause seems to be a simple timeout on the CLI. When you are using a GitHub account with logs of org / groups, the current implement will loop on each org / then each team to verify the user belongs to them. I will try to find a way to improve the performance (caching ?) without having the same problem as we encountered with |
That makes sense. My account is a member of many organizations and teams. Strangely, when I use the GitHub personal access token it seems to work. Good investigation @Wadeck. |
@samrocketman after a bit more analysis on the code, the root cause must be found between I suspect the code in the I'll keep you informed ;) (and yes also for me, with my GH token very quick and with API Token timeout) |
@Wadeck that being said. Would you like me to merge this and you open a separate PR with the fix? As far as I can tell your code didn't actually break anything. |
@samrocketman yeah good idea, in order to separate things. |
@oleg-nenashev would you mind reviewing again since you have a blocking review? |
When this one is merged, @samrocketman you will be able to redo your testing process on #92 as it corrects the problem (at least for me) |
I'll check this out this evening after work. It will give other reviewers a chance to look before end of day. Thanks for your hard work on this @Wadeck. I know long PR reviews with lots of comments like this one can be cumbersome. I thank you for your patience. |
This pull request also resolves JENKINS-40204. |
@oleg-nenashev @jglick @stephenc @Wadeck @daniel-beck @dwnusbaum Congratulations, you have earned a 👾 sticker as prize for participating in this pull request! A lot of issues were resolved. |
An unfortunate side effect of this change is now every time my oauth token is updated the SCM Sync configuration plugin (https://plugins.jenkins.io/scm-sync-configuration) commits a change because the token is stored in the user's config.xml. |
@barryhagan does it impact the data integrity or behavior of the plugin by not saving the change ? If it's "just" some noise in the git repository, I see two possibilities:
WDYT ? In any case, you can file a ticket in JIRA for that plugin mentionning this PR. |
@Wadeck, it is a noise problem. The scm-sync-configuration plugin is doing file diffs on the config.xml and committing the changes to a github repo. It does not inspect individual persisted properties, nor should it be doing that. In my opinion, the github-oauth-plugin should not be using a user property to store this type of cache data since that ends up in config.xml and the value changes each time a user authenticates. Transient cache data does not belong in a config file. I would suggest having it store the oauth token as a Secret in a different file in Obviously it was convenient to use the user property interface to store this, and there are no rules about what can be stored that way, but I just don't think this data belongs in the user config file. The behavior of scm-sync-configuration is a symptom. |
I disagree. Jenkins API tokens are stored in the same manner per user. The same thing would happen if one changes their Jenkins API token. This data is specific to users and, for now, should remain. How frequent is it actually changing the API token? This theoretically shouldn't change all that often per login session. Otherwise tools like Travis CI wouldn't be able vote back to repositories using stored tokens from the user via the authorized app. Needs more investigation. |
I totally agree that Jenkins API tokens should be stored this way. They don't change unless you intend to change them. They are effectively a password. The Github access token is being implicitly updated each time there is an oauth flow to create a session cookie for a user. That will cause numerous config changes on an even moderately active installation and now my configuration commit history is a mess. Updating the stored access token on every session login is not really necessary, but it certainly simplifies the lifetime management of that data. I've reverted to the previous version for now. |
Creating an issue is probably the best course of action. This way additional work can be tracked. There's articles on how to manage tokens better long term https://stackoverflow.com/questions/26902600/whats-the-lifetime-of-github-oauth-api-access-token Would like an issue, though. |
when the user logged in using Github login, we trigger the loggedIn event in order to populate the LastGrantedAuthoritiesSee JENKINS-47113.
Proposed changelog entries
Submitter checklist
* Use the
Internal:
prefix if the change has no user-visible impact (API, test frameworks, etc.)Desired reviewers
@reviewbybees @daniel-beck