Revert "Downgrading to a warning at @abayer’s request."

This reverts commit f3774c3.

Author: jglick

src/main/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinder.java

@@ -25,12 +25,9 @@
 package org.jenkinsci.plugins.workflow.multibranch;
 
 import edu.umd.cs.findbugs.annotations.NonNull;
+import hudson.AbortException;
 import hudson.Extension;
 import hudson.Functions;
-import hudson.MarkupText;
-import hudson.console.ConsoleAnnotationDescriptor;
-import hudson.console.ConsoleAnnotator;
-import hudson.console.ConsoleNote;
 import hudson.model.Action;
 import hudson.model.Descriptor;
 import hudson.model.DescriptorVisibilityFilter;
@@ -46,6 +43,7 @@
 import jenkins.scm.api.SCMRevision;
 import jenkins.scm.api.SCMRevisionAction;
 import jenkins.scm.api.SCMSource;
+import jenkins.util.SystemProperties;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition;
 import org.jenkinsci.plugins.workflow.flow.FlowDefinition;
@@ -61,7 +59,11 @@
 class SCMBinder extends FlowDefinition {
 
     /** Kill switch for JENKINS-33273 in case of problems. */
-    static /* not final */ boolean USE_HEAVYWEIGHT_CHECKOUT = Boolean.getBoolean(SCMBinder.class.getName() + ".USE_HEAVYWEIGHT_CHECKOUT"); // TODO 2.4+ use SystemProperties
+    static /* not final */ boolean USE_HEAVYWEIGHT_CHECKOUT = SystemProperties.getBoolean(SCMBinder.class.getName() + ".USE_HEAVYWEIGHT_CHECKOUT");
+
+    /** Kill switch for making this as strict as {@link ReadTrustedStep} about untrusted modifications. */
+    static /* not final */ boolean IGNORE_UNTRUSTED_EDITS = SystemProperties.getBoolean(SCMBinder.class.getName() + ".IGNORE_UNTRUSTED_EDITS");
+
     private String scriptPath = WorkflowBranchProjectFactory.SCRIPT;
 
     public Object readResolve() {
@@ -111,10 +113,10 @@ public SCMBinder(String scriptPath) {
                         listener.error("Could not do lightweight checkout, falling back to heavyweight").println(Functions.printThrowable(x).trim());
                     }
                     if (script != null) {
-                        if (!rev.equals(tip)) {
-                            // Print a warning in builds where an untrusted contributor has tried to edit Jenkinsfile.
-                            // If we fail to check this (e.g., due to heavyweight checkout), a warning will still be printed to the log
-                            // by the SCM, but that is less apparent.
+                        if (!IGNORE_UNTRUSTED_EDITS && !rev.equals(tip)) {
+                            // Make a best effort to abort builds where an untrusted contributor has tried to edit Jenkinsfile.
+                            // If we fail to check this (e.g., due to heavyweight checkout), a warning will be printed to the log
+                            // and the build will continue with the trusted variant, which is safe but confusing.
                             SCMFileSystem tipFS = SCMFileSystem.of(scmSource, head, tip);
                             if (tipFS != null) {
                                 String tipScript = null;
@@ -124,9 +126,7 @@ public SCMBinder(String scriptPath) {
                                     listener.error("Could not compare lightweight checkout of trusted revision").println(Functions.printThrowable(x).trim());
                                 }
                                 if (tipScript != null && !script.equals(tipScript)) {
-                                    listener.annotate(new WarningNote());
-                                    listener.getLogger().println(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis(scriptPath));
-                                    // TODO JENKINS-45970 consider aborting instead, at least optionally
+                                    throw new AbortException(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis(scriptPath));
                                 }
                             }
                         }
@@ -165,22 +165,4 @@ public SCMBinder(String scriptPath) {
 
     }
 
-    // TODO seems there is no general-purpose ConsoleNote which simply wraps markup in specified HTML
-    @SuppressWarnings("rawtypes")
-    public static class WarningNote extends ConsoleNote {
-
-        @Override public ConsoleAnnotator annotate(Object context, MarkupText text, int charPos) {
-            text.addMarkup(0, text.length(), "<span class='warning-inline'>", "</span>");
-            return null;
-        }
-
-        @Extension public static final class DescriptorImpl extends ConsoleAnnotationDescriptor {
-            @NonNull
-            @Override public String getDisplayName() {
-                return "Multibranch warnings";
-            }
-        }
-
-    }
-
 }

src/test/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinderTest.java

@@ -237,7 +237,8 @@ public static void assertRevisionAction(WorkflowRun build) {
 
     @Test public void untrustedRevisions() throws Exception {
         sampleGitRepo.init();
-        sampleGitRepo.write("Jenkinsfile", "node {checkout scm; echo readFile('file')}");
+        String masterJenkinsfile = "node {checkout scm; echo readFile('file')}";
+        sampleGitRepo.write("Jenkinsfile", masterJenkinsfile);
         sampleGitRepo.write("file", "initial content");
         sampleGitRepo.git("add", "Jenkinsfile");
         sampleGitRepo.git("commit", "--all", "--message=flow");
@@ -262,8 +263,20 @@ public static void assertRevisionAction(WorkflowRun build) {
         assertNotNull(b);
         assertEquals(1, b.getNumber());
         assertRevisionAction(b);
-        r.assertBuildStatusSuccess(b);
+        r.assertBuildStatus(Result.FAILURE, b);
         r.assertLogContains(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis("Jenkinsfile"), b);
+        r.assertLogContains("not trusting", b);
+        SCMBinder.IGNORE_UNTRUSTED_EDITS = true;
+        try {
+            b = r.buildAndAssertSuccess(p);
+            r.assertLogContains("subsequent content", b);
+            r.assertLogContains("not trusting", b);
+        } finally {
+            SCMBinder.IGNORE_UNTRUSTED_EDITS = false;
+        }
+        sampleGitRepo.write("Jenkinsfile", masterJenkinsfile);
+        sampleGitRepo.git("commit", "--all", "--message=meekly submitting");
+        b = r.buildAndAssertSuccess(p);
         r.assertLogContains("subsequent content", b);
         r.assertLogContains("not trusting", b);
     }