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-34281] More general fix of CpsFlowExecution.suspendAll ACL bug #11

Merged
merged 1 commit into from Aug 3, 2016

Conversation

Projects
None yet
7 participants
@jglick
Member

jglick commented Aug 3, 2016

Supersedes jenkinsci/workflow-cps-plugin#19 (harmless to have both).

From the system log while using 1.14.1, the following was found:

… WARNING o.j.p.w.flow.FlowExecutionList$1#computeNext: Failed to load Owner[…/…:null]. Unregisteringjava.io.IOException: no such WorkflowJob …
    at org.jenkinsci.plugins.workflow.job.WorkflowRun$Owner.run(WorkflowRun.java:701)
    at org.jenkinsci.plugins.workflow.job.WorkflowRun$Owner.get(WorkflowRun.java:713)
    at org.jenkinsci.plugins.workflow.flow.FlowExecutionList$1.computeNext(FlowExecutionList.java:63)
    at org.jenkinsci.plugins.workflow.flow.FlowExecutionList$1.computeNext(FlowExecutionList.java:55)
    at com.google.common.collect.AbstractIterator.tryToComputeNext(AbstractIterator.java:143)
    at com.google.common.collect.AbstractIterator.hasNext(AbstractIterator.java:138)
    at org.jenkinsci.plugins.workflow.support.steps.input.InputAction.loadExecutions(InputAction.java:57)
    at org.jenkinsci.plugins.workflow.support.steps.input.InputAction.getExecutions(InputAction.java:133)
    at com.cloudbees.workflow.rest.external.RunExt.isPendingInput(RunExt.java:386)
    at com.cloudbees.workflow.rest.external.RunExt.createOld(RunExt.java:327)
    at com.cloudbees.workflow.rest.external.RunExt.create(RunExt.java:303)
    at com.cloudbees.workflow.rest.external.JobExt.create(JobExt.java:126)
    at com.cloudbees.workflow.rest.endpoints.JobAPI.doRuns(JobAPI.java:68)

for a job which did exist. The most plausible explanation seems to be that the named job was not visible as the user browsing the stage view in this HTTP request.

The earlier case was reported here. I am no longer able to reproduce that, unfortunately, and am unclear on the mechanism: a given Owner in FlowExecutionList.runningTasks was either

  • created by starting a build during this session, in which case the WorkflowRun run field would be set in the initializer; or
  • deserialized in runningTasks, in which case FlowExecutionList.ItemListenerImpl.onLoaded should have forced run to be resolved and cached (as SYSTEM) during startup, before any HTTP request servicing began

In either case, run() should immediately return the cached field, ignoring Jenkins.getAuthentication(). Nonetheless, I suspect there are some corner cases where this does not happen, and there are many other places where FlowExecutionOwner.get() may be called (which probably are running as SYSTEM but I am not positive), and so it is safer to be sure that SYSTEM is used to resolve the job if it exists. Harmless to wrap in impersonate even if we are already running as SYSTEM.

@reviewbybees

[JENKINS-34281] More general fix of CpsFlowExecution.suspendAll ACL b…
…ug which could help with InputAction.loadExecutions as well.
@reviewbybees

This comment has been minimized.

reviewbybees commented Aug 3, 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.

@abayer

This comment has been minimized.

Member

abayer commented Aug 3, 2016

🐝

@svanoort

This comment has been minimized.

Member

svanoort commented Aug 3, 2016

🐝

1 similar comment
@aheritier

This comment has been minimized.

Member

aheritier commented Aug 3, 2016

🐝

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Aug 3, 2016

Smells like a hack, but 🐝

@kohsuke

This comment has been minimized.

Member

kohsuke commented Aug 3, 2016

But is this safe? Our access control model generally rely on not giving access to the object in question. If the user doesn't have access to that job, how come he can request its runs?

@jglick

This comment has been minimized.

Member

jglick commented Aug 3, 2016

is this safe?

I think so. A FlowExecutionOwner is not something that would be passed around in requests, and it is game over if you can construct or deserialize arbitrary objects from a script. Anyway the normal case is that the run field was either initialized in the constructor, or set soon after startup, so you would already be able to get a return value from .get() regardless of your authentication.

In the suspected case with loadExecutions, the HTTP call is being done as a given user—but InputAction is merely scanning every running build in the system looking for the one associated with itself. If you could find the InputAction, you already had a handle on that job.

@jglick jglick merged commit baf8f40 into jenkinsci:master Aug 3, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:Owner-ACL-JENKINS-34281 branch Aug 3, 2016

jglick added a commit to jenkinsci/pipeline-plugin that referenced this pull request Aug 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment