[Jenkins-39822] GitHub plugin functional tests broken against 1.651+ (#…

…157)

* [JENKINS-39822] Fix SECURITY-170 issues

* [JENKINS-39822] Make sure there is always BuildData on testNoBuildRevision

* [JENKINS-39822] Use conditional on plugin version

* This way we change behaviour only if needed

* [JENKINS-39822] Conditionally handle SECURITY-170

* [JENKINS-39822] Invoke build getting into account git plugin version

* Git plugin 2.4.1+ does not include BuildData if checkout fails,
resulting in testNoBuildRevision failing

* [JENKINS-39822] Fix style

* [JENKINS-39822] Added javadoc and more clarifying comments

* [JENKINS-39822] Fix codacy warning

src/test/java/com/cloudbees/jenkins/GitHubCommitNotifierTest.java

@@ -6,11 +6,13 @@
 import hudson.model.AbstractBuild;
 import hudson.model.Build;
 import hudson.model.BuildListener;
+import hudson.model.Cause;
 import hudson.model.FreeStyleProject;
 import hudson.model.Result;
 import hudson.plugins.git.GitSCM;
 import hudson.plugins.git.Revision;
 import hudson.plugins.git.util.BuildData;
+import hudson.util.VersionNumber;
 import org.eclipse.jgit.lib.ObjectId;
 import org.jenkinsci.plugins.github.config.GitHubPluginConfig;
 import org.jenkinsci.plugins.github.test.GHMockRule;
@@ -96,7 +98,8 @@ public void testNoBuildRevision() throws Exception {
         FreeStyleProject prj = jRule.createFreeStyleProject();
         prj.setScm(new GitSCM("http://non.existent.git.repo.nowhere/repo.git"));
         prj.getPublishersList().add(new GitHubCommitNotifier());
-        Build b = prj.scheduleBuild2(0).get();
+        //Git plugin 2.4.1 + does not include BuildData if checkout fails, so we add it if needed
+        Build b = safelyGenerateBuild(prj);
         jRule.assertBuildStatus(Result.FAILURE, b);
         jRule.assertLogContains(BuildDataHelper_NoLastRevisionError(), b);
     }
@@ -139,6 +142,16 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         github.service().verify(1, postRequestedFor(urlPathMatching(".*/" + SOME_SHA)));
     }
 
+    private Build safelyGenerateBuild(FreeStyleProject prj) throws InterruptedException, java.util.concurrent.ExecutionException {
+        Build b;
+        if (jRule.getPluginManager().getPlugin("git").getVersionNumber().isNewerThan(new VersionNumber("2.4.0"))) {
+            b = prj.scheduleBuild2(0, new Cause.UserIdCause(), new BuildData()).get();
+        } else {
+            b = prj.scheduleBuild2(0).get();
+        }
+        return b;
+    }
+
     @TestExtension
     public static final FixedGHRepoNameTestContributor CONTRIBUTOR = new FixedGHRepoNameTestContributor();
 

src/test/java/org/jenkinsci/plugins/github/common/ExpandableMessageTest.java

@@ -4,7 +4,10 @@
 import hudson.model.AbstractBuild;
 import hudson.model.BuildListener;
 import hudson.model.FreeStyleProject;
+import hudson.model.ParameterDefinition;
 import hudson.model.ParametersAction;
+import hudson.model.ParametersDefinitionProperty;
+import hudson.model.StringParameterDefinition;
 import hudson.model.StringParameterValue;
 import org.junit.Rule;
 import org.junit.Test;
@@ -43,6 +46,11 @@ public void shouldExpandEnvAndBuildVars() throws Exception {
         ));
 
         FreeStyleProject job = jRule.createFreeStyleProject();
+        //Due to SECURITY-170 (jenkins versions 1.651.2+ and 2.3+) only build parameters that have been
+        //explicitly defined in a job's configuration will be available by default at build time. So if
+        //the test is running on such environment the appropriate parameter definitions must be added to
+        // the job
+        handleSecurity170(job);
         job.getBuildersList().add(expander);
 
         job.scheduleBuild2(0, new ParametersAction(new StringParameterValue(CUSTOM_BUILD_PARAM, CUSTOM_PARAM_VAL)))
@@ -52,6 +60,7 @@ public void shouldExpandEnvAndBuildVars() throws Exception {
                 startsWith(format(MSG_FORMAT, job.getFullName(), CUSTOM_PARAM_VAL, job.getFullName())));
     }
 
+
     public static String asVar(String name) {
         return format("${%s}", name);
     }
@@ -60,6 +69,15 @@ public static String asTokenVar(String name) {
         return format(DEFAULT_TOKEN_TEMPLATE, name);
     }
 
+    private static void handleSecurity170(FreeStyleProject job) throws IOException {
+        ParametersActionHelper parametersActionHelper = new ParametersActionHelper();
+        if (parametersActionHelper.getAbletoInspect() && parametersActionHelper.getHasSafeParameterConfig()) {
+            ParameterDefinition paramDef = new StringParameterDefinition(CUSTOM_BUILD_PARAM, "", "");
+            ParametersDefinitionProperty paramsDef = new ParametersDefinitionProperty(paramDef);
+            job.addProperty(paramsDef);
+        }
+    }
+
     private static class MessageExpander extends TestBuilder {
         private ExpandableMessage message;
         private String result;

src/test/java/org/jenkinsci/plugins/github/common/ParametersActionHelper.java

@@ -0,0 +1,61 @@
+package org.jenkinsci.plugins.github.common;
+
+import hudson.model.ParametersAction;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+
+/**
+ * Helper class to check if the environment includes SECURITY-170 fix
+ *
+ * @see <a href=https://wiki.jenkins-ci.org/display/JENKINS/Plugins+affected+by+fix+for+SECURITY-170</a>
+ */
+public class ParametersActionHelper {
+
+    private static final Class<ParametersAction> actionClass = ParametersAction.class;
+
+    private boolean hasSafeParameterConfig = false;
+    private boolean abletoInspect = true;
+    private static final String UNDEFINED_PARAMETERS_FIELD_NAME = "KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME";
+    private static final String SAFE_PARAMETERS_FIELD_NAME = "SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME";
+
+    public ParametersActionHelper() {
+        try {
+            for (Field field : actionClass.getDeclaredFields()) {
+                if (Modifier.isStatic(field.getModifiers()) && isSafeParamsField(field)) {
+                    this.hasSafeParameterConfig = true;
+                    break;
+                }
+            }
+        } catch (Exception e) {
+            this.abletoInspect = false;
+        }
+    }
+
+    /**
+     * Method to check if the fix for SECURITY-170 is present
+     *
+     * @return true if the SECURITY-170 fix is present, false otherwise
+     */
+    public boolean getHasSafeParameterConfig() {
+        return hasSafeParameterConfig;
+    }
+
+    /**
+     * Method to check if this class has been able to determine the existence of SECURITY-170 fix
+     *
+     * @return true if the check for SECURITY-170 has been executed (whatever the result) false otherwise
+     */
+    public boolean getAbletoInspect() {
+        return abletoInspect;
+    }
+
+    private boolean isSafeParamsField(Field field) {
+        String fieldName = field.getName();
+        return UNDEFINED_PARAMETERS_FIELD_NAME.equals(fieldName)
+                                                 || SAFE_PARAMETERS_FIELD_NAME.equals(fieldName);
+    }
+
+
+
+}