From 069c5c773e4cf54dac2e359d526ad968a6369c18 Mon Sep 17 00:00:00 2001
From: Lukas Holecek <hluk@email.cz>
Date: Dec 12 2019 19:13:24 +0000
Subject: [PATCH 1/2] Fix loading RemoteRule attribute in on-demand rules


Signed-off-by: Lukas Holecek <hluk@email.cz>

---

diff --git a/greenwave/policies.py b/greenwave/policies.py
index c4633c2..e7dc800 100644
--- a/greenwave/policies.py
+++ b/greenwave/policies.py
@@ -413,7 +413,9 @@ class Rule(SafeYAMLObject):
         processed_rules = []
         for rule in rules:
             if rule['type'] == 'RemoteRule':
-                processed_rules.append(RemoteRule())
+                temp_rule = RemoteRule()
+                temp_rule.required = rule.get('required', False)
+                processed_rules.append(temp_rule)
             elif rule['type'] == 'PassingTestCaseRule':
                 temp_rule = PassingTestCaseRule()
                 temp_rule.test_case_name = rule['test_case_name']
diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py
index c8d3d09..492b82d 100644
--- a/greenwave/tests/test_policies.py
+++ b/greenwave/tests/test_policies.py
@@ -12,6 +12,7 @@ from greenwave.policies import (
     summarize_answers,
     Policy,
     RemotePolicy,
+    RemoteRule,
     RuleSatisfied,
     TestResultMissing,
     TestResultFailed,
@@ -1074,6 +1075,47 @@ def test_on_demand_policy_match(two_rules):
                 assert isinstance(decision[0], RuleSatisfied)
 
 
+@pytest.mark.parametrize('namespace', ["rpms", ""])
+def test_remote_rule_policy_on_demand_policy_required(namespace):
+    """ Testing the RemoteRule with the koji interaction when on_demand policy is given.
+    In this case we are just mocking koji """
+
+    nvr = 'nethack-1.2.3-1.el9000'
+    subject = create_test_subject('koji_build', nvr)
+
+    serverside_json = {
+        'product_version': 'fedora-26',
+        'id': 'taskotron_release_critical_tasks_with_remoterule',
+        'subject_type': 'koji_build',
+        'subject_identifier': nvr,
+        'rules': [
+            {
+                'type': 'RemoteRule',
+                'required': True
+            },
+        ],
+    }
+
+    app = create_app('greenwave.config.TestingConfig')
+    with app.app_context():
+        with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm:
+            scm.return_value = (namespace, 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb')
+            with mock.patch('greenwave.resources.retrieve_yaml_remote_rule') as f:
+                f.return_value = None
+
+                policy = OnDemandPolicy.create_from_json(serverside_json)
+                assert len(policy.rules) == 1
+                assert isinstance(policy.rules[0], RemoteRule)
+                assert policy.rules[0].required
+
+                results = DummyResultsRetriever()
+                decision = policy.check('fedora-26', subject, results)
+                assert len(decision) == 1
+                assert isinstance(decision[0], MissingGatingYaml)
+                assert not decision[0].is_satisfied
+                assert decision[0].subject.identifier == subject.identifier
+
+
 def test_two_rules_no_duplicate(tmpdir):
     nvr = 'nethack-1.2.3-1.el9000'
     subject = create_test_subject('koji_build', nvr)

From b4042b1426be5d501cf6e8d0c8473131caa3e6fb Mon Sep 17 00:00:00 2001
From: Lukas Holecek <hluk@email.cz>
Date: Dec 12 2019 19:14:07 +0000
Subject: [PATCH 2/2] Make on-demand policies loading more generic


Signed-off-by: Lukas Holecek <hluk@email.cz>

---

diff --git a/functional-tests/test_api_v1.py b/functional-tests/test_api_v1.py
index 30115c2..5890291 100644
--- a/functional-tests/test_api_v1.py
+++ b/functional-tests/test_api_v1.py
@@ -1387,7 +1387,10 @@ def test_cannot_make_decision_without_required_rule_type(
     }
     r = requests_session.post(greenwave_server + 'api/v1.0/decision', json=data)
     assert r.status_code == 400
-    assert ('Key \'type\' is required for every rule') == r.json()['message']
+    assert (
+        "Failed to parse on demand policy: Attribute 'rules': "
+        "Key 'type' is required for each list item"
+    ) == r.json()['message']
 
 
 @pytest.mark.smoke
@@ -1409,7 +1412,10 @@ def test_cannot_make_decision_without_required_rule_testcase_name(
     }
     r = requests_session.post(greenwave_server + 'api/v1.0/decision', json=data)
     assert r.status_code == 400
-    assert ('Key \'test_case_name\' is required if not a RemoteRule') == r.json()['message']
+    assert (
+        "Failed to parse on demand policy: Attribute 'rules': "
+        "Attribute 'test_case_name' is required"
+    ) == r.json()['message']
 
 
 def test_make_a_decision_with_verbose_flag_on_demand_policy(
diff --git a/greenwave/policies.py b/greenwave/policies.py
index e7dc800..8165d56 100644
--- a/greenwave/policies.py
+++ b/greenwave/policies.py
@@ -667,12 +667,15 @@ class Policy(SafeYAMLObject):
         if product_version and not self.matches_product_version(product_version):
             return False
 
-        subject = attributes.get('subject')
-        if subject and subject.type != self.subject_type:
+        if not self.matches_subject_type(**attributes):
             return False
 
         return not self.rules or any(rule.matches(self, **attributes) for rule in self.rules)
 
+    def matches_subject_type(self, **attributes):
+        subject = attributes.get('subject')
+        return not subject or subject.type == self.subject_type
+
     @remove_duplicates
     def check(
             self,
@@ -713,46 +716,25 @@ class Policy(SafeYAMLObject):
 
 
 class OnDemandPolicy(Policy):
-    root_yaml_tag = '!Policy'
-    safe_yaml_attributes = {}
-
-    def __init__(self):
-        self.id = None
-        self.product_versions = None
-        self.subject_type = None
-        self.rules = None
-        self.blacklist = None
-        self.excluded_packages = None
-        self.packages = None
-        self.relevance_key = None
+    root_yaml_tag = None
 
     @classmethod
-    def create_from_json(cls, data_dict):
-        policy = cls()
-        policy.id = data_dict.get('id')
-        policy.product_versions = [data_dict['product_version']]
-        policy.subject_type = data_dict['subject_type']
-        policy.rules = Rule.process_on_demand_rules(data_dict['rules'])
-        policy.blacklist = data_dict.get('blacklist', [])
-        policy.excluded_packages = data_dict.get('excluded_packages', [])
-        policy.packages = data_dict.get('packages', [])
-        policy.relevance_key = data_dict.get('relevance_key')
-
-        # Validate the data before processing.
-        policy.__validate_attributes()  # pylint: disable=protected-access
-        return policy
-
-    def __validate_attributes(self):
-        """ Validates types of the attributes. """
-        list_attributes = ['product_versions', 'rules', 'excluded_packages', 'packages']
-        for attribute in self.__dict__.keys():
-            if attribute in list_attributes and not isinstance(
-                    getattr(self, attribute, None), list):
-                raise TypeError('{} should be a list.'.format(attribute))
-            elif attribute not in list_attributes:
-                if getattr(self, attribute, None) and not isinstance(
-                        getattr(self, attribute, None), str):
-                    raise TypeError('{} should be a string.'.format(attribute))
+    def create_from_json(cls, data):
+        try:
+            data2 = {
+                'id': 'on-demand-policy',
+                'product_versions': [data['product_version']],
+                'decision_context': 'on-demand-policy',
+                'subject_type': 'unused',
+            }
+            data2.update(data)
+            result = cls.from_value(data2)
+            return result
+        except SafeYAMLError as e:
+            raise BadRequest('Failed to parse on demand policy: {}'.format(e))
+
+    def matches_subject_type(self, **attributes):
+        return True
 
 
 class RemotePolicy(Policy):
diff --git a/greenwave/safe_yaml.py b/greenwave/safe_yaml.py
index 429e4a9..6a65bfb 100644
--- a/greenwave/safe_yaml.py
+++ b/greenwave/safe_yaml.py
@@ -4,6 +4,8 @@ Provides a way of defining type-safe YAML parsing.
 """
 import yaml
 
+safe_yaml_tag_to_class = {}
+
 
 class SafeYAMLError(RuntimeError):
     """
@@ -22,6 +24,9 @@ class SafeYAMLAttribute(object):
     def from_yaml(self, loader, node):
         raise NotImplementedError()
 
+    def from_value(self, value):
+        raise NotImplementedError()
+
     def to_json(self, value):
         raise NotImplementedError()
 
@@ -39,6 +44,9 @@ class SafeYAMLDict(SafeYAMLAttribute):
 
     def from_yaml(self, loader, node):
         value = loader.construct_mapping(node)
+        return self.from_value(value)
+
+    def from_value(self, value):
         if isinstance(value, dict):
             return value
 
@@ -63,6 +71,9 @@ class SafeYAMLBool(SafeYAMLAttribute):
     def from_yaml(self, loader, node):
         value = loader.construct_scalar(node)
         value = yaml.safe_load(value)
+        return self.from_value(value)
+
+    def from_value(self, value):
         if isinstance(value, bool):
             return value
 
@@ -86,6 +97,9 @@ class SafeYAMLString(SafeYAMLAttribute):
 
     def from_yaml(self, loader, node):
         value = loader.construct_scalar(node)
+        return self.from_value(value)
+
+    def from_value(self, value):
         return str(value)
 
     def to_json(self, value):
@@ -109,6 +123,29 @@ class SafeYAMLList(SafeYAMLAttribute):
 
     def from_yaml(self, loader, node):
         values = loader.construct_sequence(node)
+        return self._from_value(values)
+
+    def from_value(self, value):
+        results = []
+        for item in value:
+            if not isinstance(item, dict):
+                results.append(item)
+                continue
+
+            item_type = item.get('type')
+            if not item_type:
+                raise SafeYAMLError("Key 'type' is required for each list item")
+
+            cls = safe_yaml_tag_to_class.get(item_type)
+            if cls is None:
+                raise SafeYAMLError(
+                    "Key 'type' for an list item is not valid: {}".format(item_type))
+
+            results.append(cls.from_value(item))
+
+        return self._from_value(results)
+
+    def _from_value(self, values):
         for value in values:
             if not isinstance(value, self.item_type):
                 raise SafeYAMLError(
@@ -137,23 +174,28 @@ class SafeYAMLObjectMetaclass(yaml.YAMLObjectMetaclass):
     def __init__(cls, name, bases, kwds):
         super().__init__(name, bases, kwds)
 
-        tag = getattr(cls, 'root_yaml_tag', None)
-        if tag:
+        root_yaml_tag = getattr(cls, 'root_yaml_tag', None)
+        if root_yaml_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 node.tag != root_yaml_tag:
+                        raise SafeYAMLError('Missing {} tag'.format(root_yaml_tag))
 
                     if not isinstance(node, yaml.MappingNode):
-                        raise SafeYAMLError('Expected mapping for {} tagged object'.format(tag))
+                        raise SafeYAMLError(
+                            'Expected mapping for {} tagged object'.format(root_yaml_tag))
 
                     return node
 
-            Loader.add_constructor(tag, cls.from_yaml)
+            Loader.add_constructor(root_yaml_tag, cls.from_yaml)
             cls.yaml_loader = Loader
 
+        yaml_tag = getattr(cls, 'yaml_tag', None)
+        if yaml_tag:
+            safe_yaml_tag_to_class[yaml_tag.lstrip('!')] = cls
+
 
 class SafeYAMLObject(yaml.YAMLObject, metaclass=SafeYAMLObjectMetaclass):
     """
@@ -228,6 +270,28 @@ class SafeYAMLObject(yaml.YAMLObject, metaclass=SafeYAMLObjectMetaclass):
 
         return values
 
+    @classmethod
+    def from_value(cls, data):
+        result = cls()
+
+        for attribute_name, yaml_attribute in cls.safe_yaml_attributes.items():
+            value = data.get(attribute_name)
+            if value is None:
+                if not yaml_attribute.optional:
+                    msg = 'Attribute {!r} is required'.format(attribute_name)
+                    raise SafeYAMLError(msg)
+                value = yaml_attribute.default_value
+            else:
+                try:
+                    value = yaml_attribute.from_value(value)
+                except SafeYAMLError as e:
+                    msg = 'Attribute {!r}: {}'.format(attribute_name, str(e))
+                    raise SafeYAMLError(msg)
+
+            setattr(result, attribute_name, value)
+
+        return result
+
     @property
     def safe_yaml_label(self):
         return 'YAML object {}'.format(self.yaml_tag)
diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py
index 492b82d..11845d4 100644
--- a/greenwave/tests/test_policies.py
+++ b/greenwave/tests/test_policies.py
@@ -1075,8 +1075,7 @@ def test_on_demand_policy_match(two_rules):
                 assert isinstance(decision[0], RuleSatisfied)
 
 
-@pytest.mark.parametrize('namespace', ["rpms", ""])
-def test_remote_rule_policy_on_demand_policy_required(namespace):
+def test_remote_rule_policy_on_demand_policy_required():
     """ Testing the RemoteRule with the koji interaction when on_demand policy is given.
     In this case we are just mocking koji """
 
@@ -1086,8 +1085,7 @@ def test_remote_rule_policy_on_demand_policy_required(namespace):
     serverside_json = {
         'product_version': 'fedora-26',
         'id': 'taskotron_release_critical_tasks_with_remoterule',
-        'subject_type': 'koji_build',
-        'subject_identifier': nvr,
+        'subject': [{'item': nvr, 'type': 'koji_build'}],
         'rules': [
             {
                 'type': 'RemoteRule',
@@ -1099,7 +1097,7 @@ def test_remote_rule_policy_on_demand_policy_required(namespace):
     app = create_app('greenwave.config.TestingConfig')
     with app.app_context():
         with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm:
-            scm.return_value = (namespace, 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb')
+            scm.return_value = ('rpms', 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb')
             with mock.patch('greenwave.resources.retrieve_yaml_remote_rule') as f:
                 f.return_value = None
 

