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

[JENKINS-33944] User-scoped credentials cannot be looked up #45

Merged
merged 2 commits into from Apr 4, 2016

Conversation

@armfergom
Copy link
Contributor

commented Mar 31, 2016

See description of the issue in JENKINS-33944.

Work included:

  • Fix issue.
  • Implement test.
  • Migrate test suite to new JTH.
  • Add extra test cases (covering cases not related to JENKINS-33944)

@reviewbybees esp. @stephenc

@reviewbybees

This comment has been minimized.

Copy link

commented Mar 31, 2016

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.

boolean needImpersonation = user.equals(User.current());
SecurityContext old = needImpersonation ? null : ACL.impersonate(user.impersonate());
boolean needImpersonation = !user.equals(User.current());
SecurityContext old = needImpersonation ? ACL.impersonate(user.impersonate()) : null;

This comment has been minimized.

Copy link
@jglick

jglick Mar 31, 2016

Member

Maybe I am being blinded, but is this just a no-op, equivalent to renaming the variable?

This comment has been minimized.

Copy link
@jglick

jglick Mar 31, 2016

Member

Never mind, used below in the finally block.

jglick referenced this pull request Mar 31, 2016
…ore of other users

- Not really considered as a security issue as the user could not view the actual secrets of the credentials, but better to lock it down in any case
@jglick

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

From 3876043 I guess. Review by @stephenc is mandatory.

@armfergom

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2016

@jglick correct, that's the commit that introduced what, IMHO, is a regression. Forgot to mention @stephenc for the review. Thanks!

@stephenc stephenc merged commit 590caa7 into jenkinsci:master Apr 4, 2016
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@stephenc

This comment has been minimized.

Copy link
Member

commented Apr 4, 2016

@armfergom can you update the wiki page changelog with details of the 1.27 release

@armfergom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2016

@stephenc sure thing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.