Permalink
Browse files

ProjectAuthenticator -> QueueItemAuthenticator

As Jesse pointed out, contextual information is often useful in authenticating the build, which means we need to take Queue.Item (initially AbstractBuild was the parameter, which provided the context, and I failed to accommodate that in transition to AbstractProject.)

To still allow Queue.Tasks to provide a meaningful value fallback to Queue.Task.getDefaultAuthentication() as opposed to hard code it to ACL.SYSTEM. This allow plugins like remote-terminal-access to supply a meaningful secure value without forcing a configuration change to the user.
  • Loading branch information...
kohsuke committed Jun 12, 2013
1 parent ec91933 commit e3a1a78f6d71bfdd9560607ef4ef93ca7296ff67
@@ -39,7 +39,6 @@
import hudson.Util;
import hudson.cli.declarative.CLIMethod;
import hudson.cli.declarative.CLIResolver;
import hudson.matrix.MatrixConfiguration;
import hudson.model.Cause.LegacyCodeCause;
import hudson.model.Cause.RemoteCause;
import hudson.model.Cause.UserIdCause;
@@ -90,9 +89,6 @@
import jenkins.scm.DefaultSCMCheckoutStrategyImpl;
import jenkins.scm.SCMCheckoutStrategy;
import jenkins.scm.SCMCheckoutStrategyDescriptor;
import jenkins.security.ProjectAuthenticator;
import jenkins.security.ProjectAuthenticatorConfiguration;
import jenkins.security.ProjectAuthenticatorConfiguration;
import jenkins.util.TimeDuration;
import net.sf.json.JSONObject;
import org.acegisecurity.Authentication;
@@ -113,6 +109,7 @@
import org.kohsuke.stapler.export.Exported;
import org.kohsuke.stapler.interceptor.RequirePOST;
import javax.annotation.Nonnull;
import javax.servlet.ServletException;
import java.io.File;
import java.io.IOException;
@@ -1178,13 +1175,10 @@ public final Task getOwnerTask() {
return this;
}
/**
* Let the identity determined by {@link ProjectAuthenticator}.
*
* @since 1.520
*/
public Authentication getIdentity() {
return ProjectAuthenticatorConfiguration.get().authenticate(this);
@Nonnull
public Authentication getDefaultAuthentication() {
// backward compatible behaviour.
return ACL.SYSTEM;
}
/**
@@ -239,7 +239,7 @@ public void run() {
}
}
final SecurityContext savedContext = ACL.impersonate(Tasks.getIdentityOf(task));
final SecurityContext savedContext = ACL.impersonate(workUnit.context.item.authenticate());
try {
setName(threadName + " : executing " + executable.toString());
if (LOGGER.isLoggable(FINE))
@@ -327,7 +327,7 @@ public CauseOfBlockage canTake(Queue.BuildableItem item) {
if(l==null && getMode()== Mode.EXCLUSIVE)
return CauseOfBlockage.fromMessage(Messages._Node_BecauseNodeIsReserved(getNodeName())); // this node is reserved for tasks that are tied to it
Authentication identity = item.task.getIdentity();
Authentication identity = item.authenticate();
if (!getACL().hasPermission(identity,AbstractProject.BUILD)) {
// doesn't have a permission
// TODO: does it make more sense to define a separate permission?
@@ -62,6 +62,7 @@
import hudson.model.queue.CauseOfBlockage.BecauseLabelIsOffline;
import hudson.model.queue.CauseOfBlockage.BecauseNodeIsBusy;
import hudson.model.queue.WorkUnitContext;
import hudson.security.ACL;
import hudson.triggers.SafeTimerTask;
import hudson.triggers.Trigger;
import hudson.util.OneShotEvent;
@@ -99,10 +100,14 @@
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nonnull;
import javax.servlet.ServletException;
import jenkins.model.Jenkins;
import jenkins.security.QueueItemAuthenticator;
import jenkins.security.QueueItemAuthenticatorConfiguration;
import org.acegisecurity.AccessDeniedException;
import org.acegisecurity.Authentication;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.HttpResponses;
import org.kohsuke.stapler.export.Exported;
@@ -1282,6 +1287,28 @@ public Api getApi() {
* @since 1.377
*/
Collection<? extends SubTask> getSubTasks();
/**
* This method allows the task to provide the default fallback authentication object to be used
* when {@link QueueItemAuthenticator} fails to authenticate the build.
*
* <p>
* When the task execution touches other objects inside Jenkins, the access control is performed
* based on whether this {@link Authentication} is allowed to use them. Implementers, if you are unsure,
* consider returning the identity of the user who created the task, or
* {@link ACL#SYSTEM} to bypass the access control and run as the super user, which has been
* the traditional behaviour.)

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jun 12, 2013

Member

Imbalanced parentheses.

@jglick

jglick Jun 12, 2013

Member

Imbalanced parentheses.

*
* <p>
* This method was added to an interface after it was created, so plugins built against
* older versions of Jenkins may not have this method implemented. Called {@link Tasks#_getDefaultAuthenticationOf(Task)}
* to avoid {@link AbstractMethodError}.

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jun 12, 2013

Member

Until we can depend on Java 8 and get default implementations, maybe it would be simpler and safer to just define a new extension interface and use instanceof.

@jglick

jglick Jun 12, 2013

Member

Until we can depend on Java 8 and get default implementations, maybe it would be simpler and safer to just define a new extension interface and use instanceof.

This comment has been minimized.

Show comment
Hide comment
@kohsuke

kohsuke Jun 12, 2013

Member

Given what we started, I think we should stick to the way it is. There's some benefit in what we do, in terms of the discoverability and etc.

@kohsuke

kohsuke Jun 12, 2013

Member

Given what we started, I think we should stick to the way it is. There's some benefit in what we do, in terms of the discoverability and etc.

*
* @since 1.520
* @see QueueItemAuthenticator
* @see Tasks#getDefaultAuthenticationOf(Task)
*/
@Nonnull Authentication getDefaultAuthentication();
}
/**
@@ -1512,6 +1539,27 @@ public HttpResponse doCancelQueue() throws IOException, ServletException {
return HttpResponses.forwardToPreviousPage();
}
/**
* Returns the identity that this task carries when it runs, for the purpose of access control.
*
* When the task execution touches other objects inside Jenkins, the access control is performed
* based on whether this {@link Authentication} is allowed to use them. Implementers, if you are unsure,
* return the identity of the user who queued the task, or {@link ACL#SYSTEM} to bypass the access control

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jun 12, 2013

Member

Speaking to “implementers” here may be misleading, as this method is concrete and I see no use case for overriding it.

@jglick

jglick Jun 12, 2013

Member

Speaking to “implementers” here may be misleading, as this method is concrete and I see no use case for overriding it.

* and run as the super user.
*
* @since 1.520
*/
@Nonnull
public Authentication authenticate() {
for (QueueItemAuthenticator auth : QueueItemAuthenticatorConfiguration.get().getAuthenticators()) {
Authentication a = auth.authenticate(this);
if (a!=null)
return a;
}
return Tasks.getDefaultAuthenticationOf(task);
}
/**
* Participates in the cancellation logic to set the {@link #future} accordingly.
*/
@@ -62,7 +62,7 @@ public Object getSameNodeConstraint() {
/**
* This default implementation is the historical behaviour, but this is no longer desirable. Please override.
* See {@link SubTask#getIdentity()} for the contract.
* See {@link Task#getIdentity()} for the contract.
*/
public Authentication getIdentity() {

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jun 12, 2013

Member

I think this method is orphaned and should be deleted, right?

@jglick

jglick Jun 12, 2013

Member

I think this method is orphaned and should be deleted, right?

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jun 12, 2013

Member

(BTW using @Override even on interface implementations defends against this kind of refactoring mistake.)

@jglick

jglick Jun 12, 2013

Member

(BTW using @Override even on interface implementations defends against this kind of refactoring mistake.)

This comment has been minimized.

Show comment
Hide comment
@kohsuke

kohsuke Jun 12, 2013

Member

Exccept that IntelliJ complains when I do that because this usage was technically from -source 1.7

@kohsuke

kohsuke Jun 12, 2013

Member

Exccept that IntelliJ complains when I do that because this usage was technically from -source 1.7

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jun 12, 2013

Member

1.6 you mean. Actually JDK 6 javac accepts it even with -source 1.5: this was treated as a bug fix in the compiler, not a language change. So if IntelliJ complains when using -source 1.5 then that is a bug in IntelliJ, unless it is forking a Maven build to a JDK 5 installation.

@jglick

jglick Jun 12, 2013

Member

1.6 you mean. Actually JDK 6 javac accepts it even with -source 1.5: this was treated as a bug fix in the compiler, not a language change. So if IntelliJ complains when using -source 1.5 then that is a bug in IntelliJ, unless it is forking a Maven build to a JDK 5 installation.

return ACL.SYSTEM;
@@ -26,8 +26,6 @@
import hudson.model.Label;
import hudson.model.Node;
import hudson.model.ResourceList;
import hudson.security.ACL;
import org.acegisecurity.Authentication;
/**
* Partial default implementation of {@link SubTask} to avoid
@@ -55,8 +53,4 @@ public Object getSameNodeConstraint() {
public ResourceList getResourceList() {
return new ResourceList();
}
public Authentication getIdentity() {
return getOwnerTask().getIdentity();
}
}
@@ -67,9 +67,6 @@
* See {@link SubTask#getSameNodeConstraint()}
* <li>
* Label constraint. {@link SubTask}s can specify that it can be only run on nodes that has the label.
* <li>
* Permission constraint. {@link SubTask}s have {@linkplain SubTask#getIdentity() identities} that need to have
* permissions to build on the node.
* </ul>
*
* <p>
@@ -137,10 +134,8 @@ public boolean canAccept(WorkChunk c) {
if (c.assignedLabel!=null && !c.assignedLabel.contains(node))
return false; // label mismatch
for (SubTask task : c) {
if (!nodeAcl.hasPermission(task.getIdentity(), AbstractProject.BUILD))
return false; // tasks don't have a permission to run on this node
}
if (!nodeAcl.hasPermission(item.authenticate(), AbstractProject.BUILD))
return false; // tasks don't have a permission to run on this node
return true;
}
@@ -30,7 +30,6 @@
import hudson.model.Queue.Executable;
import hudson.model.Queue.Task;
import hudson.model.ResourceList;
import org.acegisecurity.Authentication;
import java.io.IOException;
import java.util.Collection;
@@ -119,8 +118,4 @@ public final Task getOwnerTask() {
public Object getSameNodeConstraint() {
return base.getSameNodeConstraint();
}
public Authentication getIdentity() {
return base.getIdentity();
}
}
@@ -86,17 +86,4 @@
* colocation constraint.
*/
Object getSameNodeConstraint();
/**
* Returns the identity that this task carries when it runs, for the purpose of access control.
*
* When the task execution touches other objects inside Jenkins, the access control is performed
* based on whether this {@link Authentication} is allowed to use them. Implementers, if you are unsure,
* return the identity of the user who queued the task, or {@link ACL#SYSTEM} to bypass the access control
* and run as the super user.
*
* @since 1.520
* @see Tasks#getIdentityOf(SubTask)
*/
@Nonnull Authentication getIdentity();
}
@@ -90,13 +90,13 @@ public static Task getOwnerTaskOf(SubTask t) {
* A pointless function to work around what appears to be a HotSpot problem. See JENKINS-5756 and bug 6933067
* on BugParade for more details.
*/
private static Authentication _getIdentityOf(SubTask t) {
return t.getIdentity();
private static Authentication _getDefaultAuthenticationOf(Task t) {
return t.getDefaultAuthentication();
}
public static Authentication getIdentityOf(SubTask t) {
public static Authentication getDefaultAuthenticationOf(Task t) {
try {
return _getIdentityOf(t);
return _getDefaultAuthenticationOf(t);
} catch (AbstractMethodError e) {
return ACL.SYSTEM;
}

This file was deleted.

Oops, something went wrong.

This file was deleted.

Oops, something went wrong.

This file was deleted.

Oops, something went wrong.
Oops, something went wrong.

0 comments on commit e3a1a78

Please sign in to comment.