Merge "Honor independent pipeline requirements for non-live changes"

15 files changed, 167 insertions, 12 deletions

diff --git a/doc/source/config/pipeline.rst b/doc/source/config/pipeline.rst
index f4d7cce69..41bedfbf2 100644
--- a/doc/source/config/pipeline.rst
+++ b/doc/source/config/pipeline.rst
@@ -266,6 +266,17 @@ success, the pipeline reports back to Gerrit with ``Verified`` vote of
       type of the connection will dictate which options are available.
       See :ref:`drivers`.
 
+   .. attr:: allow-other-connections
+      :default: true
+
+      If this is set to `false` then any change enqueued into the
+      pipeline (whether it is enqueued to run jobs or merely as a
+      dependency) must be from one of the connections specified in the
+      pipeline configuration (this includes any trigger, reporter, or
+      source requirement).  When used in conjuctions with
+      :attr:`pipeline.require`, this can ensure that pipeline
+      requirements are exhaustive.
+
    .. attr:: supercedes
 
       The name of a pipeline, or a list of names, that this pipeline
diff --git a/releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml b/releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml
new file mode 100644
index 000000000..052d5b255
--- /dev/null
+++ b/releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml
@@ -0,0 +1,29 @@
+---
+features:
+  - |
+    A new pipeline attribute,
+    :attr:`pipeline.allow-other-connections`, has been added
+    to ensure that only changes from connections which
+    are mentioned in the pipeline configuration (such as triggers,
+    reporters, or pipeline requirements) are enqueued.
+security:
+  - |
+    Non-live items are now subject to pipeline requirements for
+    independent pipelines.
+
+    Previously, an optimization for independent pipelines skipped
+    checking that a change met the pipeline requirements.  If an
+    independent pipeline is intended only to run reviewed code, this
+    could allow running unreviewed code by updating dependent changes.
+
+    Now both non-live and live items are subject to pipeline
+    requirements in all pipeline managers.
+
+  - |
+    The new `allow-other-connections` pipeline configuration option
+    may now be used to ensure that only changes from connections which
+    are mentioned in the pipeline configuration (such as triggers,
+    reporters, or pipeline requirements) are enqueued.  This allows
+    the construction of a pipeline where, for example, code review
+    requirements are strictly enforced, even for dependencies which
+    are not normally directly enqueued.
diff --git a/tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml b/tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml
new file mode 100644
index 000000000..f679dceae
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml
@@ -0,0 +1,2 @@
+- hosts: all
+  tasks: []
diff --git a/tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml b/tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml
new file mode 100644
index 000000000..494cd3cc7
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml
@@ -0,0 +1,41 @@
+- pipeline:
+    name: trusted-check
+    manager: independent
+    allow-other-connections: false
+    require:
+      gerrit:
+        approval:
+          - Code-Review: 2
+    trigger:
+      gerrit:
+        - event: patchset-created
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- job:
+    name: base
+    parent: null
+    run: playbooks/base.yaml
+    nodeset:
+      nodes:
+        - label: ubuntu-xenial
+          name: controller
+
+- job:
+    name: check-job
+
+- project:
+    name: org/project
+    trusted-check:
+      jobs:
+        - check-job
+
+- project:
+    name: gh/project
+    trusted-check:
+      jobs:
+        - check-job
diff --git a/tests/fixtures/config/requirements/trusted-check/git/gh_project/README b/tests/fixtures/config/requirements/trusted-check/git/gh_project/README
new file mode 100644
index 000000000..9daeafb98
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/git/gh_project/README
@@ -0,0 +1 @@
+test
diff --git a/tests/fixtures/config/requirements/trusted-check/git/org_project/README b/tests/fixtures/config/requirements/trusted-check/git/org_project/README
new file mode 100644
index 000000000..9daeafb98
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/git/org_project/README
@@ -0,0 +1 @@
+test
diff --git a/tests/fixtures/config/requirements/trusted-check/main.yaml b/tests/fixtures/config/requirements/trusted-check/main.yaml
new file mode 100644
index 000000000..c8deb36c1
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/main.yaml
@@ -0,0 +1,11 @@
+- tenant:
+    name: tenant-one
+    source:
+      gerrit:
+        config-projects:
+          - common-config
+        untrusted-projects:
+          - org/project
+      github:
+        untrusted-projects:
+          - gh/project
diff --git a/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml b/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml
index 3fcc43ce6..975b9713e 100644
--- a/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml
+++ b/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml
@@ -4,7 +4,7 @@
     require:
       gerrit:
         approval:
-          - Verified: -1
+          - email: for-check@example.com
     trigger:
       gerrit:
         - event: patchset-created
@@ -24,7 +24,7 @@
     require:
       gerrit:
         approval:
-          - Verified: 1
+          - email: for-gate@example.com
     trigger:
       gerrit:
         - event: comment-added
diff --git a/tests/unit/test_requirements.py b/tests/unit/test_requirements.py
index 9a32e4b21..9f3b87187 100644
--- a/tests/unit/test_requirements.py
+++ b/tests/unit/test_requirements.py
@@ -453,3 +453,40 @@ class TestRequirementsReject(ZuulTestCase):
         self.fake_gerrit.addEvent(comment)
         self.waitUntilSettled()
         self.assertEqual(len(self.history), 3)
+
+
+class TestRequirementsTrustedCheck(ZuulTestCase):
+    config_file = "zuul-gerrit-github.conf"
+    tenant_config_file = "config/requirements/trusted-check/main.yaml"
+
+    def test_non_live_requirements(self):
+        # Test that pipeline requirements are applied to non-live
+        # changes.
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
+        B.setDependsOn(A, 1)
+        B.addApproval('Code-Review', 2)
+
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([])
+
+        self.fake_gerrit.addEvent(A.addApproval('Code-Review', 2))
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([
+            dict(name='check-job', result='SUCCESS', changes='1,1 2,1')],
+            ordered=False)
+
+    def test_other_connections(self):
+        # Test allow-other-connections: False
+        A = self.fake_github.openFakePullRequest("gh/project", "master", "A")
+        B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
+        B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
+            B.subject, A.url,
+        )
+        B.addApproval('Code-Review', 2)
+
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([])
diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py
index 5c36e6621..3a6544937 100644
--- a/tests/unit/test_scheduler.py
+++ b/tests/unit/test_scheduler.py
@@ -2807,6 +2807,7 @@ class TestScheduler(ZuulTestCase):
         B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B')
         B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
             B.subject, A.data['url'])
+        A.addApproval('Code-Review', 2)
         B.addApproval('Code-Review', 2)
 
         self.executor_server.hold_jobs_in_build = True
diff --git a/tests/unit/test_zuultrigger.py b/tests/unit/test_zuultrigger.py
index 25bfd4c30..f649f4723 100644
--- a/tests/unit/test_zuultrigger.py
+++ b/tests/unit/test_zuultrigger.py
@@ -35,9 +35,10 @@ class TestZuulTriggerParentChangeEnqueued(ZuulTestCase):
         A.addApproval('Code-Review', 2)
         B1.addApproval('Code-Review', 2)
         B2.addApproval('Code-Review', 2)
-        A.addApproval('Verified', 1)    # required by gate
-        B1.addApproval('Verified', -1)  # should go to check
-        B2.addApproval('Verified', 1)   # should go to gate
+        A.addApproval('Verified', 1, username="for-check")   # reqd by check
+        A.addApproval('Verified', 1, username="for-gate")    # reqd by gate
+        B1.addApproval('Verified', 1, username="for-check")  # go to check
+        B2.addApproval('Verified', 1, username="for-gate")   # go to gate
         B1.addApproval('Approved', 1)
         B2.addApproval('Approved', 1)
         B1.setDependsOn(A, 1)
@@ -75,9 +76,9 @@ class TestZuulTriggerParentChangeEnqueued(ZuulTestCase):
             self.scheds.first.sched, "addTriggerEvent", addTriggerEvent
         ):
             C = self.fake_gerrit.addFakeChange('org/project', 'master', 'C')
-            C.addApproval('Verified', -1)
+            C.addApproval('Verified', 1, username="for-check")
             D = self.fake_gerrit.addFakeChange('org/project', 'master', 'D')
-            D.addApproval('Verified', -1)
+            D.addApproval('Verified', 1, username="for-check")
             D.setDependsOn(C, 1)
             self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
 
@@ -108,6 +109,7 @@ class TestZuulTriggerParentChangeEnqueuedGithub(ZuulGithubAppTestCase):
         B1.addReview('derp', 'APPROVED')
         B2.addReview('derp', 'APPROVED')
         A.addLabel('for-gate')    # required by gate
+        A.addLabel('for-check')   # required by check
         B1.addLabel('for-check')  # should go to check
         B2.addLabel('for-gate')   # should go to gate
 
diff --git a/zuul/configloader.py b/zuul/configloader.py
index 08b517618..7f8346382 100644
--- a/zuul/configloader.py
+++ b/zuul/configloader.py
@@ -1292,6 +1292,7 @@ class PipelineParser(object):
 
         pipeline = {vs.Required('name'): str,
                     vs.Required('manager'): manager,
+                    'allow-other-connections': bool,
                     'precedence': precedence,
                     'supercedes': to_list(str),
                     'description': str,
@@ -1331,6 +1332,8 @@ class PipelineParser(object):
         pipeline = model.Pipeline(conf['name'], self.pcontext.tenant)
         pipeline.source_context = conf['_source_context']
         pipeline.start_mark = conf['_start_mark']
+        pipeline.allow_other_connections = conf.get(
+            'allow-other-connections', True)
         pipeline.description = conf.get('description')
         pipeline.supercedes = as_list(conf.get('supercedes', []))
 
@@ -1366,6 +1369,7 @@ class PipelineParser(object):
         # Make a copy to manipulate for backwards compat.
         conf_copy = conf.copy()
 
+        seen_connections = set()
         for conf_key, action in self.reporter_actions.items():
             reporter_set = []
             allowed_reporters = self.pcontext.tenant.allowed_reporters
@@ -1379,6 +1383,7 @@ class PipelineParser(object):
                         reporter_name, pipeline, params)
                     reporter.setAction(conf_key)
                     reporter_set.append(reporter)
+                    seen_connections.add(reporter_name)
             setattr(pipeline, action, reporter_set)
 
         # If merge-conflict actions aren't explicit, use the failure actions
@@ -1423,11 +1428,13 @@ class PipelineParser(object):
             source = self.pcontext.connections.getSource(source_name)
             manager.ref_filters.extend(
                 source.getRequireFilters(require_config))
+            seen_connections.add(source_name)
 
         for source_name, reject_config in conf.get('reject', {}).items():
             source = self.pcontext.connections.getSource(source_name)
             manager.ref_filters.extend(
                 source.getRejectFilters(reject_config))
+            seen_connections.add(source_name)
 
         for connection_name, trigger_config in conf.get('trigger').items():
             if self.pcontext.tenant.allowed_triggers is not None and \
@@ -1439,7 +1446,9 @@ class PipelineParser(object):
             manager.event_filters.extend(
                 trigger.getEventFilters(connection_name,
                                         conf['trigger'][connection_name]))
+            seen_connections.add(connection_name)
 
+        pipeline.connections = list(seen_connections)
         # Pipelines don't get frozen
         return pipeline
 
diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py
index aec996f31..60eb479e0 100644
--- a/zuul/manager/__init__.py
+++ b/zuul/manager/__init__.py
@@ -508,6 +508,12 @@ class PipelineManager(metaclass=ABCMeta):
             log.debug("Change %s is already in pipeline, ignoring" % change)
             return True
 
+        if ((not self.pipeline.allow_other_connections) and
+            (change.project.connection_name not in self.pipeline.connections)):
+            log.debug("Change %s is not from a connection known to %s ",
+                      change, self.pipeline)
+            return False
+
         if not ignore_requirements:
             for f in self.ref_filters:
                 if f.connection_name != change.project.connection_name:
diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py
index aeeff5adc..ca52b37fe 100644
--- a/zuul/manager/independent.py
+++ b/zuul/manager/independent.py
@@ -61,13 +61,15 @@ class IndependentPipelineManager(PipelineManager):
         for needed_change in needed_changes:
             # This differs from the dependent pipeline by enqueuing
             # changes ahead as "not live", that is, not intended to
-            # have jobs run.  Also, pipeline requirements are always
-            # ignored (which is safe because the changes are not
-            # live).
+            # have jobs run.  Pipeline requirements are still in place
+            # in order to avoid unreviewed code being executed in
+            # pipelines that require review.
             if needed_change not in history:
                 r = self.addChange(needed_change, event, quiet=True,
-                                   ignore_requirements=True, live=False,
-                                   change_queue=change_queue, history=history,
+                                   ignore_requirements=ignore_requirements,
+                                   live=False,
+                                   change_queue=change_queue,
+                                   history=history,
                                    dependency_graph=dependency_graph)
                 if not r:
                     return False
diff --git a/zuul/model.py b/zuul/model.py
index ac38c3705..0d789f976 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -438,6 +438,8 @@ class Pipeline(object):
         # reconfigured).  A pipeline requires a tenant in order to
         # reach the currently active layout for that tenant.
         self.tenant = tenant
+        self.allow_other_connections = True
+        self.connections = []
         self.source_context = None
         self.start_mark = None
         self.description = None