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-32731] [JENKINS-34650] Docker.groovy is already trusted #75

Merged
merged 3 commits into from Oct 31, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 3 additions & 12 deletions pom.xml
Expand Up @@ -4,7 +4,8 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>2.11</version>
<version>2.17</version>
<relativePath/>
</parent>
<artifactId>docker-workflow</artifactId>
<version>1.10-SNAPSHOT</version>
Expand Down Expand Up @@ -48,20 +49,10 @@
<artifactId>docker-commons</artifactId>
<version>1.5</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1.17</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
<version>2.2</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps</artifactId>
<version>2.7</version>
<version>2.17</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down
12 changes: 0 additions & 12 deletions src/main/java/org/jenkinsci/plugins/docker/workflow/DockerDSL.java
Expand Up @@ -25,9 +25,6 @@

import groovy.lang.Binding;
import hudson.Extension;
import java.io.IOException;
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist;
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.StaticWhitelist;
import org.jenkinsci.plugins.workflow.cps.CpsScript;
import org.jenkinsci.plugins.workflow.cps.GlobalVariable;

Expand All @@ -50,13 +47,4 @@
return docker;
}

@Extension public static class MiscWhitelist extends ProxyWhitelist {
public MiscWhitelist() throws IOException {
super(new StaticWhitelist(
// TODO should docker-commons just get a script-security dependency and mark these things @Whitelisted?
"new org.jenkinsci.plugins.docker.commons.credentials.DockerRegistryEndpoint java.lang.String java.lang.String",
"method org.jenkinsci.plugins.docker.commons.credentials.DockerRegistryEndpoint imageName java.lang.String"));
}
}

}
Expand Up @@ -23,8 +23,6 @@
*/
package org.jenkinsci.plugins.docker.workflow;

import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.Whitelisted;

import javax.annotation.Nonnull;
import java.io.Serializable;

Expand All @@ -42,12 +40,9 @@ public class ImageNameTokens implements Serializable {

private static final long serialVersionUID = 1L;

@Whitelisted
public final String userAndRepo;
@Whitelisted
public final String tag;

Copy link
Member

Choose a reason for hiding this comment

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

Does not seem to be required. I would keep it explicitly

@Whitelisted
public ImageNameTokens(@Nonnull String name) {
int tagIdx = name.lastIndexOf(':');
if (tagIdx != -1) {
Expand Down
Expand Up @@ -34,7 +34,7 @@ class Docker implements Serializable {
public <V> V withRegistry(String url, String credentialsId = null, Closure<V> body) {
node {
script.withEnv(["DOCKER_REGISTRY_URL=${url}"]) {
script.withDockerRegistry(registry: [url: url, credentialsId: credentialsId]) {
Copy link
Member

Choose a reason for hiding this comment

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

Will the original code work after the change?. Better to keep both tests

script.withDockerRegistry([url: url, credentialsId: credentialsId]) {
body()
}
}
Expand All @@ -43,7 +43,7 @@ class Docker implements Serializable {

public <V> V withServer(String uri, String credentialsId = null, Closure<V> body) {
node {
script.withDockerServer(server: [uri: uri, credentialsId: credentialsId]) {
Copy link
Member

Choose a reason for hiding this comment

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

Better to keep the original test as well

script.withDockerServer([uri: uri, credentialsId: credentialsId]) {
body()
}
}
Expand Down