From b3be8705b8f1c3ef17185dbdd393c0e4cfd651ba Mon Sep 17 00:00:00 2001
From: Lukas Holecek <lholecek@redhat.com>
Date: Jul 16 2018 12:20:20 +0000
Subject: [PATCH 1/2] Make YAML parsing type-safe


Policy attributes are parsed with strict types and policy files can only
contain policies as top level objects.

---

diff --git a/greenwave/policies.py b/greenwave/policies.py
index 9a4366f..cde6454 100644
--- a/greenwave/policies.py
+++ b/greenwave/policies.py
@@ -1,10 +1,17 @@
 # SPDX-License-Identifier: GPL-2.0+
 
 from fnmatch import fnmatch
-import yaml
 import logging
 import greenwave.resources
 
+from greenwave.safe_yaml import (
+    SafeYAMLChoice,
+    SafeYAMLList,
+    SafeYAMLObject,
+    SafeYAMLString,
+    SafeYAMLError,
+)
+
 log = logging.getLogger(__name__)
 
 
@@ -12,26 +19,6 @@ class DisallowedRuleError(RuntimeError):
     pass
 
 
-def validate_policies(policies, disallowed_rules=None):
-    disallowed_rules = disallowed_rules or []
-    for policy in policies:
-        if not isinstance(policy, Policy):
-            raise RuntimeError('Policies are not configured properly as policy %s '
-                               'is not an instance of Policy' % policy)
-        for required_attribute in ['decision_context', 'product_versions', 'subject_type']:
-            if not hasattr(policy, required_attribute):
-                raise RuntimeError('Policies are not configured properly as policy %s '
-                                   'is missing attribute %s' % (policy.id, required_attribute))
-        for rule in policy.rules:
-            if not isinstance(rule, Rule):
-                raise RuntimeError('Policies are not configured properly as rule %s '
-                                   'is not an instance of Rule' % rule)
-            for disallowed_rule in disallowed_rules:
-                if isinstance(rule, disallowed_rule):
-                    raise DisallowedRuleError('Policies are not configured properly as rule %s '
-                                              'is an instance of %s' % (rule, disallowed_rule))
-
-
 def subject_type_identifier_to_item(subject_type, subject_identifier):
     """
     Greenwave < 0.8 included an "item" key in the "unsatisfied_requirements".
@@ -160,18 +147,23 @@ class TestResultFailed(RuleNotSatisfied):
 
 
 class InvalidGatingYaml(RuleNotSatisfied):
+    """
+    Remote policy parsing failed.
+    """
 
-    def __init__(self, subject_type, subject_identifier, test_case_name):
+    def __init__(self, subject_type, subject_identifier, test_case_name, details):
         self.subject_type = subject_type
         self.subject_identifier = subject_identifier
         self.test_case_name = test_case_name
+        self.details = details
 
     def to_json(self):
         return {
             'type': 'invalid-gating-yaml',
             'testcase': self.test_case_name,
             'subject_type': self.subject_type,
-            'subject_identifier': self.subject_identifier
+            'subject_identifier': self.subject_identifier,
+            'details': self.details
         }
 
 
@@ -234,27 +226,30 @@ def summarize_answers(answers):
     Returns:
         str: Human-readable summary.
     """
-    if len(answers) == 0:
+    if not answers:
         return 'no tests are required'
-    if all(answer.is_satisfied for answer in answers):
-        return 'all required tests passed'
+
     failure_count = len([answer for answer in answers if isinstance(answer, TestResultFailed)])
     missing_count = len([answer for answer in answers if isinstance(answer, TestResultMissing)])
-    invalid_gating_yaml = any(answer for answer in answers
-                              if isinstance(answer, InvalidGatingYaml))
+
     if failure_count and missing_count:
         return '{} of {} required tests failed, {} result{} missing'.format(
             failure_count, len(answers), missing_count, 's' if missing_count > 1 else '')
-    elif failure_count:
+
+    if failure_count > 0:
         return '{} of {} required tests failed'.format(failure_count, len(answers))
-    elif missing_count:
+
+    if missing_count > 0:
         return '{} of {} required test results missing'.format(missing_count, len(answers))
-    elif invalid_gating_yaml:
-        return 'misconfigured gating.yaml file'
+
+    if all(answer.is_satisfied for answer in answers):
+        return 'all required tests passed'
+
+    assert False, 'Unexpected unsatisfied result'
     return 'inexplicable result'
 
 
-class Rule(yaml.YAMLObject):
+class Rule(SafeYAMLObject):
     """
     An individual rule within a policy. A policy consists of multiple rules.
     When the policy is evaluated, each rule returns an answer
@@ -279,26 +274,16 @@ class Rule(yaml.YAMLObject):
         """
         raise NotImplementedError()
 
-    def to_json(self):
-        """ Return a dict representation of this rule.
-
-        Returns:
-            dict: A representation of this Rule as a dict for an API response.
-        """
-        raise NotImplementedError()
-
 
-def handle_misconfigured_gating_yaml(subject_type, subject_identifier, waivers):
-    if any((d['testcase'] == 'invalid-gating-yaml' and d['subject']['type'] == subject_type and
-            d['subject']['item'] == subject_identifier) for d in waivers):
-        return []
-    else:
-        return InvalidGatingYaml(subject_type, subject_identifier, 'invalid-gating-yaml')
+def waives_invalid_gating_yaml(waiver, subject_type, subject_identifier):
+    return (waiver['testcase'] == 'invalid-gating-yaml' and
+            waiver['subject']['type'] == subject_type and
+            waiver['subject']['item'] == subject_identifier)
 
 
 class RemoteRule(Rule):
     yaml_tag = '!RemoteRule'
-    yaml_loader = yaml.SafeLoader
+    safe_yaml_attributes = {}
 
     def check(self, subject_type, subject_identifier, results, waivers):
         if subject_type != 'koji_build':
@@ -313,26 +298,16 @@ class RemoteRule(Rule):
             return []
 
         try:
-            policies = yaml.safe_load_all(response)
-            # policies is a generator, so listifying it
-            policies = list(policies)
-        except yaml.parser.ParserError as e:
-            # if the yaml file is malformed we skip these policies
-            log.warning("Error parsing gating.yaml for package %s: %s", pkg_name, e)
-            return handle_misconfigured_gating_yaml(subject_type, subject_identifier, waivers)
-        # policies in dist-git are always about a package
-        for policy in policies:
-            policy.subject_type = 'koji_build'
-            # Attribute 'id' in remote policy is optional.
-            policy_id = getattr(policy, 'id', 'untitled')
-            # Prefix the id for better error reporting.
-            policy.id = 'dist-git-gating-policy-{}-{}'.format(policy_id, pkg_name)
-        try:
-            validate_policies(policies, [RemoteRule])
-        except DisallowedRuleError:
-            log.warning('Policies are not configured properly as there is a policy '
-                        'that is an instance of RemoteRule')
-            return handle_misconfigured_gating_yaml(subject_type, subject_identifier, waivers)
+            policies = RemotePolicy.safe_load_all(response)
+        except SafeYAMLError as e:
+            if any(waives_invalid_gating_yaml(waiver, subject_type, subject_identifier)
+                    for waiver in waivers):
+                return []
+            return [
+                InvalidGatingYaml(
+                    subject_type, subject_identifier, 'invalid-gating-yaml', str(e))
+            ]
+
         answers = []
         for policy in policies:
             response = policy.check(subject_identifier, results, waivers)
@@ -354,7 +329,11 @@ class PassingTestCaseRule(Rule):
     a non-passing result with a waiver.
     """
     yaml_tag = '!PassingTestCaseRule'
-    yaml_loader = yaml.SafeLoader
+
+    safe_yaml_attributes = {
+        'test_case_name': SafeYAMLString(),
+        'scenario': SafeYAMLString(optional=True),
+    }
 
     def check(self, subject_type, subject_identifier, results, waivers):
         matching_results = [
@@ -363,7 +342,7 @@ class PassingTestCaseRule(Rule):
             w for w in waivers if (w['testcase'] == self.test_case_name and w['waived'] is True)]
 
         # Rules may optionally specify a scenario to limit applicability.
-        if self._scenario:
+        if self.scenario:
             matching_results = [r for r in matching_results if self.scenario in
                                 r['data'].get('scenario', [])]
 
@@ -371,9 +350,9 @@ class PassingTestCaseRule(Rule):
         if not matching_results:
             if not matching_waivers:
                 return TestResultMissing(subject_type, subject_identifier, self.test_case_name,
-                                         self._scenario)
+                                         self.scenario)
             return TestResultMissingWaived(
-                subject_type, subject_identifier, self.test_case_name, self._scenario)
+                subject_type, subject_identifier, self.test_case_name, self.scenario)
 
         # If we find multiple matching results, we always use the first one which
         # will be the latest chronologically, because ResultsDB always returns
@@ -389,21 +368,13 @@ class PassingTestCaseRule(Rule):
                w['waived'] for w in waivers):
             return TestResultPassed(self.test_case_name, matching_result['id'])
         return TestResultFailed(subject_type, subject_identifier, self.test_case_name,
-                                self._scenario, matching_result['id'])
-
-    @property
-    def _scenario(self):
-        return getattr(self, 'scenario', None)
-
-    def __repr__(self):
-        return "%s(test_case_name=%r, scenario=%r)" % (
-            self.__class__.__name__, self.test_case_name, self._scenario)
+                                self.scenario, matching_result['id'])
 
     def to_json(self):
         return {
             'rule': self.__class__.__name__,
             'test_case_name': self.test_case_name,
-            'scenario': self._scenario,
+            'scenario': self.scenario,
         }
 
 
@@ -415,9 +386,10 @@ class PackageSpecificRule(Rule):
     This intermediary class should be considered abstract, and not used directly.
     """
 
-    def __init__(self, test_case_name, repos):
-        self.test_case_name = test_case_name
-        self.repos = repos
+    safe_yaml_attributes = {
+        'test_case_name': SafeYAMLString(),
+        'repos': SafeYAMLList(str),
+    }
 
     def check(self, subject_type, subject_identifier, results, waivers):
         """ Check that the subject passes testcase for the given results, but
@@ -445,10 +417,6 @@ class PackageSpecificRule(Rule):
         rule.test_case_name = self.test_case_name
         return rule.check(subject_type, subject_identifier, results, waivers)
 
-    def __repr__(self):
-        return "%s(test_case_name=%s, repos=%r)" % (
-            self.__class__.__name__, self.test_case_name, self.repos)
-
     def to_json(self):
         return {
             'rule': self.__class__.__name__,
@@ -459,18 +427,27 @@ class PackageSpecificRule(Rule):
 
 class FedoraAtomicCi(PackageSpecificRule):
     yaml_tag = '!FedoraAtomicCi'
-    yaml_loader = yaml.SafeLoader
 
 
 class PackageSpecificBuild(PackageSpecificRule):
     yaml_tag = '!PackageSpecificBuild'
-    yaml_loader = yaml.SafeLoader
 
 
-class Policy(yaml.YAMLObject):
-    yaml_tag = '!Policy'
-    yaml_loader = yaml.SafeLoader
-    blacklist = []
+class Policy(SafeYAMLObject):
+    root_yaml_tag = '!Policy'
+
+    safe_yaml_attributes = {
+        'id': SafeYAMLString(),
+        'product_versions': SafeYAMLList(str),
+        'decision_context': SafeYAMLString(),
+        # TODO: Handle brew-build value better.
+        'subject_type': SafeYAMLChoice(
+            'koji_build', 'bodhi_update', 'compose', 'brew-build'),
+        'rules': SafeYAMLList(Rule),
+        'blacklist': SafeYAMLList(str, optional=True),
+        'relevance_key': SafeYAMLString(optional=True),
+        'relevance_value': SafeYAMLString(optional=True),
+    }
 
     def applies_to(self, decision_context, product_version, subject_type):
         return (decision_context == self.decision_context and
@@ -492,22 +469,29 @@ class Policy(yaml.YAMLObject):
                 answers.append(response)
         return answers
 
-    def __repr__(self):
-        return "%s(id=%r, product_versions=%r, decision_context=%r, subject_type=%r, rules=%r)" % (
-            self.__class__.__name__, self.id, self.product_versions, self.decision_context,
-            self.subject_type, self.rules)
-
-    def to_json(self):
-        return {
-            'id': self.id,
-            'product_versions': self.product_versions,
-            'decision_context': self.decision_context,
-            'subject_type': self.subject_type,
-            'rules': [rule.to_json() for rule in self.rules],
-            'blacklist': self.blacklist,
-            'relevance_key': getattr(self, 'relevance_key', None),
-            'relevance_value': getattr(self, 'relevance_value', None),
-        }
-
     def _applies_to_product_version(self, product_version):
         return any(fnmatch(product_version, version) for version in self.product_versions)
+
+    @property
+    def safe_yaml_label(self):
+        return 'Policy {!r}'.format(self.id or 'untitled')
+
+
+class RemotePolicy(Policy):
+    root_yaml_tag = '!Policy'
+
+    safe_yaml_attributes = {
+        'id': SafeYAMLString(optional=True),
+        'product_versions': SafeYAMLList(str),
+        'decision_context': SafeYAMLString(),
+        'rules': SafeYAMLList(Rule),
+        'blacklist': SafeYAMLList(str, optional=True),
+    }
+
+    subject_type = 'koji_build'
+
+    def validate(self):
+        for rule in self.rules:
+            if isinstance(rule, RemoteRule):
+                raise SafeYAMLError('RemoteRule is not allowed in remote policies')
+        super().validate()
diff --git a/greenwave/safe_yaml.py b/greenwave/safe_yaml.py
new file mode 100644
index 0000000..6b407ee
--- /dev/null
+++ b/greenwave/safe_yaml.py
@@ -0,0 +1,214 @@
+# SPDX-License-Identifier: GPL-2.0+
+"""
+Provides a way of defining type-safe YAML parsing.
+"""
+import yaml
+
+
+class SafeYAMLError(RuntimeError):
+    """
+    Exception raised when an unexpected type is found in YAML.
+    or validation fails (see SafeYAMLObject.validate()).
+    """
+    pass
+
+
+class SafeYAMLAttribute(object):
+    """
+    Base class for SafeYAMLObject attributes (in SafeYAMLObject.safe_yaml_attributes dict).
+    """
+    def __init__(self, optional=False):
+        self.optional = optional
+
+    def from_yaml(self, loader, node):
+        raise NotImplementedError()
+
+    def to_json(self, value):
+        raise NotImplementedError()
+
+    @property
+    def default_value(self):
+        raise NotImplementedError()
+
+
+class SafeYAMLString(SafeYAMLAttribute):
+    """
+    YAML object attribute representing a string value.
+    """
+    def from_yaml(self, loader, node):
+        value = loader.construct_scalar(node)
+        return str(value)
+
+    def to_json(self, value):
+        return value
+
+    @property
+    def default_value(self):
+        return None
+
+
+class SafeYAMLChoice(SafeYAMLAttribute):
+    """
+    YAML object attribute with only specific values allowed.
+    """
+    def __init__(self, *values, **kwargs):
+        super().__init__(**kwargs)
+        self.values = values
+
+    def from_yaml(self, loader, node):
+        value = loader.construct_scalar(node)
+        if value not in self.values:
+            raise SafeYAMLError(
+                'Value must be one of: {}'.format(', '.join(self.values)))
+        return str(value)
+
+    def to_json(self, value):
+        return value
+
+    @property
+    def default_value(self):
+        return self.values[0]
+
+
+class SafeYAMLList(SafeYAMLAttribute):
+    """
+    YAML object attribute represeting a list of values.
+    """
+    def __init__(self, item_type, **kwargs):
+        super().__init__(**kwargs)
+        self.item_type = item_type
+
+    def from_yaml(self, loader, node):
+        values = loader.construct_sequence(node)
+        for value in values:
+            if not isinstance(value, self.item_type):
+                raise SafeYAMLError(
+                    'Expected list of {} objects'.format(self.item_type.__name__))
+        return values
+
+    @property
+    def default_value(self):
+        return []
+
+    def to_json(self, values):
+        return [self._item_to_json(value) for value in values]
+
+    def _item_to_json(self, value):
+        if isinstance(value, SafeYAMLObject):
+            return value.to_json()
+        return value
+
+
+class SafeYAMLObjectMetaclass(yaml.YAMLObjectMetaclass):
+    """
+    The metaclass for SafeYAMLObject.
+
+    Enabled YAML loader to accept only root objects of specific type.
+    """
+    def __init__(cls, name, bases, kwds):
+        super().__init__(name, bases, kwds)
+
+        tag = getattr(cls, 'root_yaml_tag', None)
+        if tag:
+            class Loader(cls.yaml_loader):
+                def get_node(self):
+                    node = super().get_node()
+
+                    if node.tag != tag:
+                        raise SafeYAMLError('Missing {} tag'.format(tag))
+
+                    if not isinstance(node, yaml.MappingNode):
+                        raise SafeYAMLError('Expected mapping for {} tagged object'.format(tag))
+
+                    return node
+
+            Loader.add_constructor(tag, cls.from_yaml)
+            cls.yaml_loader = Loader
+
+
+class SafeYAMLObject(yaml.YAMLObject, metaclass=SafeYAMLObjectMetaclass):
+    """
+    Base class for safer YAML map objects.
+
+    Allows to specify attribute types and whether these are optional.
+
+    Optionally, set class attribute root_yaml_tag to YAML tag name. This will
+    be used to verify the root object has this tag.
+
+    Define class attribute safe_yaml_attributes which is dict mapping attribute
+    name to a SafeYAMLAttribute object.
+    """
+    yaml_loader = yaml.SafeLoader
+
+    @classmethod
+    def __new__(cls, *args, **kwargs):
+        result = super().__new__(*args, **kwargs)
+
+        for attribute_name, yaml_attribute in cls.safe_yaml_attributes.items():
+            value = yaml_attribute.default_value
+            setattr(result, attribute_name, value)
+
+        return result
+
+    @classmethod
+    def from_yaml(cls, loader, node):
+        nodes = {
+            name_node.value: value_node
+            for name_node, value_node in node.value
+        }
+        result = cls()
+
+        for attribute_name, yaml_attribute in cls.safe_yaml_attributes.items():
+            child_node = nodes.get(attribute_name)
+            if child_node is None:
+                if not yaml_attribute.optional:
+                    msg = '{}: Attribute {!r} is required'.format(
+                        result.safe_yaml_label, attribute_name)
+                    raise SafeYAMLError(msg)
+                value = yaml_attribute.default_value
+            else:
+                try:
+                    value = yaml_attribute.from_yaml(loader, child_node)
+                except (SafeYAMLError, yaml.YAMLError) as e:
+                    msg = '{}: Attribute {!r}: {}'.format(
+                        result.safe_yaml_label, attribute_name, str(e))
+                    raise SafeYAMLError(msg)
+            setattr(result, attribute_name, value)
+
+        try:
+            result.validate()
+        except SafeYAMLError as e:
+            msg = '{}: {}'.format(result.safe_yaml_label, str(e))
+            raise SafeYAMLError(msg)
+
+        return result
+
+    @classmethod
+    def safe_load_all(cls, file_or_content):
+        """
+        Load objects from file or a data.
+
+        :raises: SafeYAMLError: if root object tag doesn't match yaml_tag,
+            attributes don't match their types or parsing fails.
+        """
+        try:
+            values = yaml.load_all(file_or_content, Loader=cls.yaml_loader)
+            values = list(values)
+        except yaml.YAMLError as e:
+            raise SafeYAMLError('YAML Parser Error: {}'.format(e))
+
+        return values
+
+    @property
+    def safe_yaml_label(self):
+        return 'YAML object {}'.format(self.yaml_tag)
+
+    def validate(self):
+        pass
+
+    def to_json(self):
+        return {
+            attribute_name: yaml_attribute.to_json(
+                getattr(self, attribute_name, None))
+            for attribute_name, yaml_attribute in self.safe_yaml_attributes.items()
+        }
diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py
index 25d9d20..e0c93eb 100644
--- a/greenwave/tests/test_policies.py
+++ b/greenwave/tests/test_policies.py
@@ -4,15 +4,20 @@
 import pytest
 import mock
 
+from textwrap import dedent
+
 from greenwave.app_factory import create_app
 from greenwave.policies import (
     summarize_answers,
+    Policy,
+    RemotePolicy,
     RuleSatisfied,
     TestResultMissing,
     TestResultFailed,
     InvalidGatingYaml
 )
 from greenwave.utils import load_policies
+from greenwave.safe_yaml import SafeYAMLError
 
 
 def test_summarize_answers():
@@ -180,9 +185,9 @@ subject_type: bodhi_update
 rules:
   - !PassingTestCaseRule {test_case_name: dist.abicheck}
         """)
-    with pytest.raises(RuntimeError) as excinfo:
+    expected_error = "Missing !Policy tag"
+    with pytest.raises(SafeYAMLError, match=expected_error):
         load_policies(tmpdir.strpath)
-    assert 'Policies are not configured properly' in str(excinfo.value)
 
 
 def test_misconfigured_policy_rules(tmpdir):
@@ -197,9 +202,13 @@ subject_type: bodhi_update
 rules:
   - {test_case_name: dist.abicheck}
         """)
-    with pytest.raises(RuntimeError) as excinfo:
+    expected_error = (
+        "Policy 'taskotron_release_critical_tasks': "
+        "Attribute 'rules': "
+        "Expected list of Rule objects"
+    )
+    with pytest.raises(SafeYAMLError, match=expected_error):
         load_policies(tmpdir.strpath)
-    assert 'Policies are not configured properly' in str(excinfo.value)
 
 
 def test_passing_testcasename_with_scenario(tmpdir):
@@ -338,12 +347,12 @@ rules:
                 policy = policies[0]
 
                 results, waivers = [], []
-                expected_error = (
-                    'policy dist-git-gating-policy-untitled-nethack'
-                    ' is missing attribute product_versions'
-                )
-                with pytest.raises(RuntimeError, match=expected_error):
-                    policy.check(nvr, results, waivers)
+                expected_details = "Policy 'untitled': Attribute 'product_versions' is required"
+                decision = policy.check(nvr, results, waivers)
+                assert len(decision) == 1
+                assert isinstance(decision[0], InvalidGatingYaml)
+                assert decision[0].is_satisfied is False
+                assert decision[0].details == expected_details
 
 
 def test_remote_rule_malformed_yaml(tmpdir):
@@ -459,3 +468,223 @@ rules:
                     }]
                     decision = policy.check(nvr, results, waivers)
                     assert len(decision) == 0
+
+
+def test_parse_policies_missing_tag():
+    expected_error = "Missing !Policy tag"
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        Policy.safe_load_all("""---""")
+
+
+def test_parse_policies_unexpected_type():
+    policies = dedent("""
+        --- !Policy
+        42
+    """)
+    expected_error = "Expected mapping for !Policy tagged object"
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        RemotePolicy.safe_load_all(policies)
+
+
+def test_parse_policies_missing_id():
+    expected_error = "Policy 'untitled': Attribute 'id' is required"
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        Policy.safe_load_all(dedent("""
+            --- !Policy
+            product_versions: [fedora-rawhide]
+            decision_context: test
+            subject_type: compose
+            blacklist: []
+            rules:
+              - !PassingTestCaseRule {test_case_name: compose.cloud.all}
+        """))
+
+
+def test_parse_policies_missing_product_versions():
+    expected_error = "Policy 'test': Attribute 'product_versions' is required"
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        Policy.safe_load_all(dedent("""
+            --- !Policy
+            id: test
+            decision_context: test
+            subject_type: compose
+            blacklist: []
+            rules:
+              - !PassingTestCaseRule {test_case_name: compose.cloud.all}
+        """))
+
+
+def test_parse_policies_missing_decision_context():
+    expected_error = "Policy 'test': Attribute 'decision_context' is required"
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        Policy.safe_load_all(dedent("""
+            --- !Policy
+            id: test
+            product_versions: [fedora-rawhide]
+            subject_type: compose
+            blacklist: []
+            rules:
+              - !PassingTestCaseRule {test_case_name: compose.cloud.all}
+        """))
+
+
+def test_parse_policies_invalid_subject_type():
+    expected_error = (
+        r"Policy 'test': Attribute 'subject_type': "
+        "Value must be one of:.*"
+    )
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        Policy.safe_load_all(dedent("""
+            --- !Policy
+            id: test
+            product_versions: [fedora-rawhide]
+            decision_context: test
+            subject_type: bad_subject
+            blacklist: []
+            rules:
+              - !PassingTestCaseRule {test_case_name: compose.cloud.all}
+              - 0
+        """))
+
+
+def test_parse_policies_invalid_rule():
+    expected_error = "Policy 'test': Attribute 'rules': Expected list of Rule objects"
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        Policy.safe_load_all(dedent("""
+            --- !Policy
+            id: test
+            product_versions: [fedora-rawhide]
+            decision_context: test
+            subject_type: compose
+            blacklist: []
+            rules:
+              - !PassingTestCaseRule {test_case_name: compose.cloud.all}
+              - bad_rule
+        """))
+
+
+def test_parse_policies_remote_missing_tag():
+    expected_error = "Missing !Policy tag"
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        RemotePolicy.safe_load_all("""---""")
+
+
+def test_parse_policies_remote_missing_id_is_ok():
+    policies = RemotePolicy.safe_load_all(dedent("""
+        --- !Policy
+        product_versions: [fedora-rawhide]
+        decision_context: test
+        subject_type: koji_build
+        rules:
+          - !PassingTestCaseRule {test_case_name: test.case.name}
+    """))
+    assert len(policies) == 1
+    assert policies[0].id is None
+
+
+def test_parse_policies_remote_missing_subject_type_is_ok():
+    policies = RemotePolicy.safe_load_all(dedent("""
+        --- !Policy
+        product_versions: [fedora-rawhide]
+        decision_context: test
+        rules:
+          - !PassingTestCaseRule {test_case_name: test.case.name}
+    """))
+    assert len(policies) == 1
+    assert policies[0].subject_type == 'koji_build'
+
+
+def test_parse_policies_remote_recursive():
+    expected_error = "Policy 'test': RemoteRule is not allowed in remote policies"
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        RemotePolicy.safe_load_all(dedent("""
+            --- !Policy
+            id: test
+            product_versions: [fedora-rawhide]
+            decision_context: bodhi_update_push_stable_with_remoterule
+            subject_type: koji_build
+            rules:
+              - !RemoteRule {}
+        """))
+
+
+def test_parse_policies_remote_multiple():
+    policies = RemotePolicy.safe_load_all(dedent("""
+        --- !Policy
+        id: test1
+        product_versions: [fedora-rawhide]
+        decision_context: test
+        rules:
+          - !PassingTestCaseRule {test_case_name: test.case.name}
+
+        --- !Policy
+        id: test2
+        product_versions: [fedora-rawhide]
+        decision_context: test
+        rules:
+          - !PassingTestCaseRule {test_case_name: test.case.name}
+    """))
+    assert len(policies) == 2
+    assert policies[0].id == 'test1'
+    assert policies[1].id == 'test2'
+
+
+def test_parse_policies_remote_multiple_missing_tag():
+    expected_error = "Missing !Policy tag"
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        RemotePolicy.safe_load_all(dedent("""
+            --- !Policy
+            id: test1
+            product_versions: [fedora-rawhide]
+            decision_context: test
+            rules:
+              - !PassingTestCaseRule {test_case_name: test.case.name}
+
+            ---
+            id: test2
+            product_versions: [fedora-rawhide]
+            decision_context: test
+            rules:
+              - !PassingTestCaseRule {test_case_name: test.case.name}
+        """))
+
+
+def test_parse_policies_remote_missing_rule_attribute():
+    expected_error = (
+        "Policy 'test': "
+        "Attribute 'rules': "
+        "YAML object !PassingTestCaseRule: "
+        "Attribute 'test_case_name' is required"
+    )
+    with pytest.raises(SafeYAMLError, match=expected_error):
+        RemotePolicy.safe_load_all(dedent("""
+            --- !Policy
+            id: test
+            product_versions: [fedora-rawhide]
+            decision_context: test
+            rules:
+              - !PassingTestCaseRule {test_case: test.case.name}
+        """))
+
+
+def test_policies_to_json():
+    policies = Policy.safe_load_all(dedent("""
+        --- !Policy
+        id: test
+        product_versions: [fedora-rawhide]
+        decision_context: test
+        subject_type: compose
+        blacklist: []
+        rules: []
+    """))
+    assert len(policies) == 1
+    assert policies[0].to_json() == {
+        'id': 'test',
+        'product_versions': ['fedora-rawhide'],
+        'decision_context': 'test',
+        'subject_type': 'compose',
+        'blacklist': [],
+        'rules': [],
+        'relevance_key': None,
+        'relevance_value': None,
+    }
diff --git a/greenwave/utils.py b/greenwave/utils.py
index 66b4417..e32f44a 100644
--- a/greenwave/utils.py
+++ b/greenwave/utils.py
@@ -7,7 +7,6 @@ import os
 import time
 import hashlib
 
-import yaml
 from flask import jsonify, current_app, request
 from flask.config import Config
 from requests import ConnectionError, Timeout
@@ -119,8 +118,8 @@ def load_policies(policies_dir):
     policy_pathnames = glob.glob(os.path.join(policies_dir, '*.yaml'))
     policies = []
     for policy_pathname in policy_pathnames:
-        policies.extend(yaml.safe_load_all(open(policy_pathname, 'r')))
-    greenwave.policies.validate_policies(policies)
+        with open(policy_pathname, 'r') as f:
+            policies.extend(greenwave.policies.Policy.safe_load_all(f))
     log.debug("Loaded %i policies from %s", len(policies), policies_dir)
     return policies
 

From 99a252c610e2dd02420ddbd69c28ab8306bf1fd7 Mon Sep 17 00:00:00 2001
From: Lukas Holecek <lholecek@redhat.com>
Date: Jul 16 2018 12:38:24 +0000
Subject: [PATCH 2/2] Provide API for validating gating.yaml


Example command:

    http localhost:5005/api/v1.0/validate-gating-yaml \
        < ../greenwave-policies/fedora.yaml

Related to #217.

---

diff --git a/functional-tests/test_api_v1.py b/functional-tests/test_api_v1.py
index 3abcf17..4bc47d1 100644
--- a/functional-tests/test_api_v1.py
+++ b/functional-tests/test_api_v1.py
@@ -2,6 +2,8 @@
 
 import json
 
+from textwrap import dedent
+
 from greenwave import __version__
 
 
@@ -826,3 +828,41 @@ def test_make_a_decision_about_brew_build(requests_session, greenwave_server, te
     assert res_data['policies_satisfied'] is True
     assert res_data['applicable_policies'] == ['osci_compose']
     assert res_data['summary'] == 'all required tests passed'
+
+
+def test_validate_gating_yaml_valid(requests_session, greenwave_server):
+    gating_yaml = dedent("""
+        --- !Policy
+        id: "test"
+        product_versions:
+          - fedora-26
+        decision_context: test
+        rules:
+          - !PassingTestCaseRule {test_case_name: test}
+    """)
+    result = requests_session.post(
+        greenwave_server + 'api/v1.0/validate-gating-yaml', data=gating_yaml)
+    assert result.json().get('message') == 'All OK'
+    assert result.status_code == 200
+
+
+def test_validate_gating_yaml_empty(requests_session, greenwave_server):
+    result = requests_session.post(greenwave_server + 'api/v1.0/validate-gating-yaml')
+    assert result.json().get('message') == 'No policies defined'
+    assert result.status_code == 400
+
+
+def test_validate_gating_yaml_missing_tag(requests_session, greenwave_server):
+    gating_yaml = dedent("""
+        ---
+        id: "test"
+        product_versions:
+          - fedora-26
+        decision_context: test
+        rules:
+          - !PassingTestCaseRule {test_case_name: test}
+    """)
+    result = requests_session.post(
+        greenwave_server + 'api/v1.0/validate-gating-yaml', data=gating_yaml)
+    assert result.json().get('message') == "Missing !Policy tag"
+    assert result.status_code == 400
diff --git a/greenwave/api_v1.py b/greenwave/api_v1.py
index 41a9687..9481185 100644
--- a/greenwave/api_v1.py
+++ b/greenwave/api_v1.py
@@ -3,8 +3,9 @@
 from flask import Blueprint, request, current_app, jsonify, url_for, redirect
 from werkzeug.exceptions import BadRequest, NotFound, UnsupportedMediaType, InternalServerError
 from greenwave import __version__
-from greenwave.policies import summarize_answers, RemoteRule
+from greenwave.policies import summarize_answers, RemotePolicy, RemoteRule
 from greenwave.resources import retrieve_results, retrieve_waivers, retrieve_builds_in_update
+from greenwave.safe_yaml import SafeYAMLError
 from greenwave.utils import insert_headers, jsonp
 
 api = (Blueprint('api_v1', __name__))
@@ -372,3 +373,54 @@ def make_decision():
     resp = insert_headers(resp)
     resp.status_code = 200
     return resp
+
+
+@api.route('/validate-gating-yaml', methods=['GET', 'POST'])
+@jsonp
+def validate_gating_yaml_post():
+    """
+    Validates contents of "gating.yaml" file.
+
+    POST data is the file content.
+
+    The response is JSON object containing lists of "errors", "successes" and
+    "messages".
+
+    **Sample response for failed validation**:
+
+    .. sourcecode:: none
+
+       HTTP/1.0 200 OK
+       Content-Length: 52
+       Content-Type: application/json
+       Date: Fri, 22 Jun 2018 11:19:35 GMT
+       Server: Werkzeug/0.12.2 Python/3.6.5
+
+       {
+           "message": "Missing !Policy tag"
+       }
+
+    **Sample response for successful validation**:
+
+    .. sourcecode:: none
+
+       HTTP/1.0 200 OK
+       Content-Length: 38
+       Content-Type: application/json
+       Date: Fri, 22 Jun 2018 11:23:16 GMT
+       Server: Werkzeug/0.12.2 Python/3.6.5
+
+       {
+           "message": "All OK"
+       }
+    """
+    content = request.get_data().decode('utf-8')
+    try:
+        policies = RemotePolicy.safe_load_all(content)
+    except SafeYAMLError as e:
+        raise BadRequest(str(e))
+
+    if not policies:
+        raise BadRequest('No policies defined')
+
+    return jsonify({'message': 'All OK'})

