[FIXED JENKINS-41456] Properly validate publishHTML

Specifically, when we see that there's a sole required parameter, the only provided argument by the user is a map, at least one of those map keys doesn't have a corresponding parameter in the describable, and the sole required parameter's type is a describable itself, validate the argument map against that parameter type describable instead of against the parent describable. So for publishHTML, that means realizing `target` is an `HtmlPublisherTarget` and validating the map against that.
jenkinsci · Mar 22, 2017 · 3020057 · 3020057
1 parent 42a9aa4
commit 3020057
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 24 deletions.
diff --git a/...rc/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTArgumentList.java b/...rc/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTArgumentList.java
@@ -1,8 +1,5 @@
 package org.jenkinsci.plugins.pipeline.modeldefinition.ast;
 
-import org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable;
-
-import java.util.List;
 import java.util.Map;
 
 /**

diff --git a/pipeline-model-definition/pom.xml b/pipeline-model-definition/pom.xml
@@ -128,6 +128,12 @@
       </exclusions>
     </dependency>
 
+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>htmlpublisher</artifactId>
+      <scope>test</scope>
+    </dependency>
+
     <dependency>
       <groupId>org.jenkins-ci.main</groupId>
       <artifactId>jenkins-test-harness-tools</artifactId>

diff --git a/.../src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParser.groovy b/.../src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParser.groovy
@@ -271,6 +271,7 @@ class ModelParser implements Parser {
                             errorCollector.error(badKey, Messages.ModelValidatorImpl_InvalidIdentifierInEnv(keyTxt))
                         } else {
                             errorCollector.error(badKey, Messages.ModelParser_InvalidEnvironmentIdentifier(srcTxt))
+
                         }
                     }
                 }

diff --git a/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy b/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy
@@ -235,35 +235,59 @@ class ModelValidatorImpl implements ModelValidator {
                     errorCollector.error(condition, Messages.ModelValidatorImpl_NoNestedWhenAllowed(condition.name))
                     valid = false
                 } else {
-                    valid = validateDescribable(condition, condition.name, condition.args, model, desc, false)
+                    valid = validateDescribable(condition, condition.name, condition.args, model, false)
                 }
             }
         }
 
         return valid
     }
 
+    private boolean isValidStepParameter(DescribableModel<? extends Describable> model,
+                                         String key,
+                                         ModelASTElement keyElement) {
+        def p = model?.getParameter(key);
+        if (p == null) {
+            String possible = EditDistance.findNearest(key, model.getParameters().collect {
+                it.name
+            })
+            errorCollector.error(keyElement, Messages.ModelValidatorImpl_InvalidStepParameter(key, possible))
+            return false
+        }
+        return true
+    }
+
     private boolean validateDescribable(ModelASTElement element, String name,
                                         ModelASTArgumentList args,
                                         DescribableModel<? extends Describable> model,
-                                        Descriptor desc, boolean takesClosure = false) {
+                                        boolean takesClosure = false) {
         boolean valid = true
 
         if (args instanceof ModelASTNamedArgumentList) {
             ModelASTNamedArgumentList argList = (ModelASTNamedArgumentList) args
 
+            boolean soleDescribableMap = false
+
             argList.arguments.each { k, v ->
+                // Check if there is a sole required parameter and it's describable
+                if (model.getParameter(k.key) == null &&
+                    model?.soleRequiredParameter != null &&
+                    Describable.class.isAssignableFrom(model.soleRequiredParameter.erasedType)) {
+                    // Check if the argument list validates as that describable. If it does, note that so
+                    // we can proceed.
+                    soleDescribableMap = true
+                    valid = validateDescribable(element, model.soleRequiredParameter.name, argList,
+                        new DescribableModel<>(model.soleRequiredParameter.erasedType))
+                    return
+                }
 
-                def p = model.getParameter(k.key);
-                if (p == null) {
-                    String possible = EditDistance.findNearest(k.key, model.getParameters().collect {
-                        it.name
-                    })
-                    errorCollector.error(k, Messages.ModelValidatorImpl_InvalidStepParameter(k.key, possible))
+                if (!isValidStepParameter(model, k.key, k)) {
                     valid = false
-                    return;
+                    return
                 }
 
+                def p = model.getParameter(k.key);
+
                 ModelASTKey validateKey = k
 
                 // Check if this is the only required parameter and if so, validate it without the key.
@@ -281,10 +305,14 @@ class ModelValidatorImpl implements ModelValidator {
                     valid = false
                 }
             }
-            model.parameters.each { p ->
-                if (p.isRequired() && !argList.containsKeyName(p.name)) {
-                    errorCollector.error(element, Messages.ModelValidatorImpl_MissingRequiredStepParameter(p.name))
-                    valid = false
+            // Only check for required parameters if we're valid up to this point and we haven't already processed
+            // a sole describable map
+            if (valid && !soleDescribableMap) {
+                model.parameters.each { p ->
+                    if (p.isRequired() && !argList.containsKeyName(p.name)) {
+                        errorCollector.error(element, Messages.ModelValidatorImpl_MissingRequiredStepParameter(p.name))
+                        valid = false
+                    }
                 }
             }
         } else if (args instanceof ModelASTPositionalArgumentList) {
@@ -331,7 +359,7 @@ class ModelValidatorImpl implements ModelValidator {
             // No validation needed for code blocks like expression and script
             return true
         } else {
-            return validateDescribable(step, step.name, step.args, model, desc, lookup.stepTakesClosure(desc))
+            return validateDescribable(step, step.name, step.args, model, lookup.stepTakesClosure(desc))
         }
     }
 
@@ -380,16 +408,13 @@ class ModelValidatorImpl implements ModelValidator {
                         }
                         ModelASTKeyValueOrMethodCallPair kvm = (ModelASTKeyValueOrMethodCallPair) a
 
-                        def p = model.getParameter(kvm.key.key);
-                        if (p == null) {
-                            String possible = EditDistance.findNearest(kvm.key.key, model.getParameters().collect {
-                                it.name
-                            })
-                            errorCollector.error(kvm.key, Messages.ModelValidatorImpl_InvalidStepParameter(kvm.key.key, possible))
+                        if (!isValidStepParameter(model, kvm.key.key, kvm.key)) {
                             valid = false
-                            return;
+                            return
                         }
 
+                        def p = model.getParameter(kvm.key.key);
+
                         if (kvm.value instanceof ModelASTMethodCall) {
                             valid = validateElement((ModelASTMethodCall) kvm.value)
                         } else {

diff --git a/...ition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java b/...ition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java
@@ -25,6 +25,7 @@
 
 import com.cloudbees.hudson.plugins.folder.Folder;
 import com.google.common.base.Predicate;
+import htmlpublisher.HtmlPublisherTarget;
 import hudson.model.Result;
 import hudson.model.Slave;
 import jenkins.plugins.git.GitSCMSource;
@@ -227,6 +228,18 @@ public void parallelPipelineWithFailFast() throws Exception {
                 .go();
     }
 
+    @Issue("JENKINS-41456")
+    @Test
+    public void htmlPublisher() throws Exception {
+        WorkflowRun b = expect("htmlPublisher")
+                .logContains("[Pipeline] { (foo)")
+                .go();
+
+        HtmlPublisherTarget.HTMLBuildAction buildReport = b.getAction(HtmlPublisherTarget.HTMLBuildAction.class);
+        assertNotNull(buildReport);
+        assertEquals("Test Report", buildReport.getHTMLTarget().getReportName());
+    }
+
     @Test
     public void executionModelAction() throws Exception {
         WorkflowRun b = expect("executionModelAction").go();

diff --git a/pipeline-model-definition/src/test/resources/htmlPublisher.groovy b/pipeline-model-definition/src/test/resources/htmlPublisher.groovy
@@ -0,0 +1,46 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2017, CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+pipeline {
+    agent any
+    stages {
+        stage("foo") {
+            steps {
+                sh("mkdir -p tmp")
+                sh('echo "<html><body><h1>HELLO</h1></body></html" > tmp/test.html')
+                publishHTML ([
+                    allowMissing: false,
+                    alwaysLinkToLastBuild: false,
+                    keepAll: true,
+                    reportDir: "tmp",
+                    reportFiles: 'test.html',
+                    reportName: "Test Report"
+                ])
+            }
+        }
+    }
+}
+
+
+
diff --git a/pom.xml b/pom.xml
@@ -170,6 +170,11 @@
         <artifactId>jackson2-api</artifactId>
         <version>2.7.3</version>
       </dependency>
+      <dependency>
+        <groupId>org.jenkins-ci.plugins</groupId>
+        <artifactId>htmlpublisher</artifactId>
+        <version>1.12</version>
+      </dependency>
       <dependency>
         <groupId>org.jenkins-ci.plugins</groupId>
         <artifactId>git</artifactId>