Skip to content

[JENKINS-56229] Use the correct access token when impersonating a user#109

Merged
samrocketman merged 2 commits into
jenkinsci:masterfrom
agentgonzo:JENKINS-56229
Apr 12, 2019
Merged

[JENKINS-56229] Use the correct access token when impersonating a user#109
samrocketman merged 2 commits into
jenkinsci:masterfrom
agentgonzo:JENKINS-56229

Conversation

@agentgonzo
Copy link
Copy Markdown

The token retrieved will be for the caller of the user.impersonate()
method (ie, could be SYSTEM), rather than that of the target of the
impersonation.

@reviewbybees

The token retrieved will be for the caller of the user.impersonate()
method (ie, could be SYSTEM), rather than that of the target of the
impersonation.
Copy link
Copy Markdown
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

This PR seems to correct a bug that was not discovered before due to lack of impersonation called on GH-oauth I imagine.

@samrocketman Could you please run your usual tests and determine if this PR is breaking expected behavior?

Thank you in advance

Comment thread src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java Outdated
Co-Authored-By: agentgonzo <sarch@cloudbees.com>
@jtnord
Copy link
Copy Markdown
Member

jtnord commented Mar 18, 2019

@samrocketman are you still maintaining this plugin?

@alexanderrtaylor
Copy link
Copy Markdown

@samrocketman Is there anything we can do to help here? It is still showing you as the maintainer so I wanted to check if this could be approved and merged

@samrocketman
Copy link
Copy Markdown
Member

Hello, I’ve been away a few months due to a cross country move. I’m getting back into the open source groove. I will be testing this change this week.

@Wadeck
Copy link
Copy Markdown
Contributor

Wadeck commented Apr 11, 2019

@samrocketman welcome back 🚢 ✈️ 🚌 🚗 🚴

@samrocketman samrocketman merged commit 6c9f6d9 into jenkinsci:master Apr 12, 2019
samrocketman added a commit that referenced this pull request Apr 12, 2019
This octomerge simplifies merging multiple pull requests.  They've all
been tested in conjunction and don't cause any upgrade issues.
@samrocketman
Copy link
Copy Markdown
Member

This has been released in github-oauth 0.32

@agentgonzo
Copy link
Copy Markdown
Author

Thanks @samrocketman !!!

@samrocketman
Copy link
Copy Markdown
Member

samrocketman commented Apr 16, 2019

Thanks for the fix. I verified it worked by using a freestyle job and impersonating to a user with a system groovy script.

import jenkins.model.Jenkins

Jenkins.instance.as(User.get('samrocketman')).with { ctx ->
    try {
        // access Jenkins job only user has access
    } finally {
        ctx.close()
    }
}

Before the fix it would throw an error that anonymous does not have read permissions. After, behaves as expected.

@agentgonzo
Copy link
Copy Markdown
Author

Before the fix it would throw an error that anonymous does not have read permissions. After, behaves as expected.

That was what we saw too

@samrocketman
Copy link
Copy Markdown
Member

@agentgonzo mind taking a look at https://issues.jenkins-ci.org/browse/JENKINS-57154 ?

@samrocketman
Copy link
Copy Markdown
Member

@Wadeck @agentgonzo
I've discovered this PR is the root cause of https://issues.jenkins-ci.org/browse/JENKINS-57154

@samrocketman
Copy link
Copy Markdown
Member

Reverting this change seems to fix things locally on my test machine. Impersonation is still broken without it, though.

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.

6 participants