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

Enable background threads to impersonate SYSTEM #200

Merged
merged 1 commit into from Feb 6, 2015

Conversation

Projects
None yet
7 participants
@scoheb
Copy link
Contributor

commented Feb 6, 2015

Jenkins plugin listeners and background threads should normally run as the SYSTEM user.

This commit allows the EventThreads to impersonate ACL.SYSTEM.

Addresses regression in [JENKINS-23152]

Change-Id: Ia0f5a223f70497ad44529c390aff0c711e414f67

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

*/
@Override
public void run() {
ACL.impersonate(ACL.SYSTEM);

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 6, 2015

Member

ACL.impersonate(ACL.SYSTEM); will set the thread context to SYSTEM "forever" the normal pattern to use is

ACL.SYSTEM.impersonate(new Runnable() {
  public void run() {
    ...
  }
});

This comment has been minimized.

Copy link
@rinrinne

rinrinne Feb 6, 2015

Member

@rsandell Seems his code is the same as hudson.model.Executor.

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 6, 2015

Member

Yes, I don't think its harmful as it's written now.
@reviewbybees ?

This comment has been minimized.

Copy link
@stephenc

stephenc Feb 6, 2015

Member

Always safer to restore the old context... though since this is not a thread pool it's more of a stylistic issue... given the amount of copy&paste example taking in Jenkins plugins, I would think it is safer to always to it the "right" way or else give a comment saying why you don't need to

This comment has been minimized.

Copy link
@jglick

jglick Feb 6, 2015

Member

Agreed with @stephenc, it is safer to restore the former value just in case this code is copied or refactored somehow.

SecurityContext old = ACL.impersonate(ACL.SYSTEM);
try {
    super.run();
} finally {
    SecurityContextHolder.setSecurityContext(old);
}

This comment has been minimized.

Copy link
@scoheb

scoheb Feb 6, 2015

Author Contributor

Thanks. Will use your snippet Jesse.

*/
public class GerritHandlerLifecycle extends GerritHandler {
public class JenkinsAwareGerritHandler extends GerritHandler implements Coordinator {

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 6, 2015

Member

Renaming public classes like this will break backwards compatibility quite severely.

Although a rename is probably in order here the old class should probably be kept but deprecated.

This comment has been minimized.

Copy link
@hugares

hugares Feb 6, 2015

Member

I agree that renaming public class can break backward compatibility but I would be surprised to find one plugin that depends on Gerrit-Trigger and use that class. For this one, I am not convinced that we need to keep the old one.

This comment has been minimized.

Copy link
@hugares

hugares Feb 6, 2015

Member

You did not need to add "implements Coordinator", GerritHandler already implement that interface

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 6, 2015

Member

I sincerely doubt that usage too, but if a rename can be avoided I think we should.
But if @scoheb thinks it becomes too messy too keep the original class with my suggested workarounds then go ahead and keep the rename.

This comment has been minimized.

Copy link
@scoheb

scoheb Feb 6, 2015

Author Contributor

I kept the rename

GlobalMatrixAuthorizationStrategy auth = new GlobalMatrixAuthorizationStrategy();
j.jenkins.setAuthorizationStrategy(auth);
j.jenkins.setCrumbIssuer(null);
HudsonPrivateSecurityRealm realm = new HudsonPrivateSecurityRealm(false);

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 6, 2015

Member

There is a security realm mock in the test harness prepared for these kinds of things that is a bit easier/cleaner to use.

pom.xml Outdated
@@ -61,7 +61,7 @@
<dependency>
<groupId>com.sonymobile.tools.gerrit</groupId>
<artifactId>gerrit-events</artifactId>
<version>2.4.2</version>
<version>2.5.1</version>

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 6, 2015

Member

released as 2.6.0

@scoheb scoheb force-pushed the scoheb:impersonating-threads branch from db4f1c3 to 778ff92 Feb 6, 2015

Scott Hebert
Enable background threads to impersonate SYSTEM
Jenkins plugin listeners and  background threads should normally run as the SYSTEM user.

This commit allows the EventThreads to impersonate ACL.SYSTEM

Addresses regression in [JENKINS-23152]

Change-Id: Ia0f5a223f70497ad44529c390aff0c711e414f67

@scoheb scoheb force-pushed the scoheb:impersonating-threads branch from 778ff92 to 1740c39 Feb 6, 2015

@rsandell

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

Looks good, just waiting to hear from Jenkins and then I'll try a new release.

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2015

rsandell added a commit that referenced this pull request Feb 6, 2015

Merge pull request #200 from scoheb/impersonating-threads
Enable background threads to impersonate SYSTEM

@rsandell rsandell merged commit 84c16b0 into jenkinsci:master Feb 6, 2015

1 check passed

Jenkins This pull request looks good
Details
@rsandell

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

There is a risk that the same issue will appear when onCompleted etc is called with a different security context like with this plugin https://wiki.jenkins-ci.org/display/JENKINS/Authorize+Project+plugin

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