From ba627bd5375cdcf8af3febadee635378089981a1 Mon Sep 17 00:00:00 2001
From: Lukas Holecek <lholecek@redhat.com>
Date: Apr 23 2018 08:51:52 +0000
Subject: Validate "results" filter for waivers


Fixes constructing invalid SQL query ("... WHERE () AND ...").

Additionally returns status code 400 if a dict in "results" contains
unknown key. This can help users to fix some mistakes quickly.

Fixes #162

---

diff --git a/tests/test_api_v10.py b/tests/test_api_v10.py
index 21dbdcf..ad3eccf 100644
--- a/tests/test_api_v10.py
+++ b/tests/test_api_v10.py
@@ -324,6 +324,40 @@ def test_filtering_waivers_by_subject_without_testcase(client, session):
     assert res_data['data'][0]['testcase'] == 'testcase1'
 
 
+@pytest.mark.parametrize("results", [
+    [{'item': {'subject.test1': 'subject1'}}], # Unexpected key
+    [{'subject': 'subject1'}], # Unexpected key type
+])
+def test_filtering_waivers_with_bad_key(client, session, results):
+    param = json.dumps(results)
+    r = client.get('/api/v1.0/waivers/?results=%s' % param)
+    res_data = json.loads(r.get_data(as_text=True))
+    assert r.status_code == 400
+    assert "'results' parameter should be a list of dictionaries with subject and testcase" \
+        in res_data.get('message')
+
+
+@pytest.mark.parametrize("results", [
+    [],
+    [{}],
+])
+def test_filtering_waivers_with_empty_results(client, session, results):
+    create_waiver(session, subject={'subject.test1': 'subject1'},
+                  testcase='testcase1', username='foo-1', product_version='foo-1')
+    param = json.dumps(results)
+    r = client.get('/api/v1.0/waivers/?results=%s' % param)
+    res_data = json.loads(r.get_data(as_text=True))
+    assert r.status_code == 200
+    assert len(res_data['data']) == 1
+
+
+def test_filtering_waivers_with_invalid_json(client, session):
+    r = client.get('/api/v1.0/waivers/?results=[')
+    res_data = json.loads(r.get_data(as_text=True))
+    assert r.status_code == 400
+    assert "'results' parameter should be in JSON format" in res_data.get('message')
+
+
 def test_filtering_waivers_by_product_version(client, session):
     create_waiver(session, subject={'subject.test1': 'subject1'},
                   testcase='testcase1', username='foo-1', product_version='release-1')
diff --git a/waiverdb/api_v1.py b/waiverdb/api_v1.py
index 10d6bd1..41d7155 100644
--- a/waiverdb/api_v1.py
+++ b/waiverdb/api_v1.py
@@ -34,6 +34,19 @@ def get_resultsdb_result(result_id):
     return response.json()
 
 
+def _validate_results_filter(results):
+    expected = {
+        'subject': dict,
+        'testcase': str,
+    }
+    for item in results:
+        for k, v in item.items():
+            if not (k in expected and isinstance(v, expected[k])):
+                raise BadRequest(
+                    ("'results' parameter should be a list of dictionaries with"
+                     " subject and testcase"))
+
+
 # RP contains request parsers (reqparse.RequestParser).
 #    Parsers are added in each 'resource section' for better readability
 RP = {}
@@ -114,16 +127,11 @@ class WaiversResource(Resource):
         query = Waiver.query.order_by(Waiver.timestamp.desc())
 
         if args['results']:
-            results = json.loads(args['results'])
-            for d in results:
-                if d.get('subject', None):
-                    if not isinstance(d.get('subject', None), dict):
-                        raise BadRequest("'results' parameter should be a list \
-                                          of dictionaries with subject and testcase")
-                if d.get('testcase', None):
-                    if not isinstance(d.get('testcase', None), str):
-                        raise BadRequest("'results' parameter should be a list \
-                                      of dictionaries with subject and testcase")
+            try:
+                results = json.loads(args['results'])
+            except json.JSONDecodeError:
+                raise BadRequest("'results' parameter should be in JSON format")
+            _validate_results_filter(results)
             query = Waiver.by_results(query, results)
         if args['product_version']:
             query = query.filter(Waiver.product_version == args['product_version'])
@@ -368,17 +376,10 @@ class GetWaiversBySubjectsAndTestcases(Resource):
             raise UnsupportedMediaType('No JSON payload in request')
         data = request.get_json()
         query = Waiver.query.order_by(Waiver.timestamp.desc())
-        if data.get('results'):
-            for d in data['results']:
-                if d.get('subject', None):
-                    if not isinstance(d.get('subject', None), dict):
-                        raise BadRequest("'results' parameter should be a list \
-                                          of dictionaries with subject and testcase")
-                if d.get('testcase', None):
-                    if not isinstance(d.get('testcase', None), str):
-                        raise BadRequest("'results' parameter should be a list \
-                                          of dictionaries with subject and testcase")
-            query = Waiver.by_results(query, data['results'])
+        if 'results' in data:
+            results = data['results']
+            _validate_results_filter(results)
+            query = Waiver.by_results(query, results)
         if 'product_version' in data:
             query = query.filter(Waiver.product_version == data['product_version'])
         if 'username' in data:
diff --git a/waiverdb/models/waivers.py b/waiverdb/models/waivers.py
index c0a2a39..59381b4 100644
--- a/waiverdb/models/waivers.py
+++ b/waiverdb/models/waivers.py
@@ -36,13 +36,27 @@ class Waiver(db.Model):
 
     @classmethod
     def by_results(cls, query, results):
-        return query.filter(or_(*[
-            and_(
-                cls.subject == d['subject'],
-                cls.testcase == d['testcase']
-            ) if d.get('testcase', None) else
-            and_(
-                cls.subject == d['subject']
-            ) if d.get('subject', None) else
-            and_() for d in results
-        ]))
+        """
+        Filter ``query`` by matching with at least one filter in ``results``.
+
+        If ``results`` is empty, ``query`` is not filtered.
+
+        Args:
+            query (flask_sqlalchemy.BaseQuery)
+            results (list): each item should be dict containing
+                "subject" (dict) and "testcase" (str), both optional
+
+        Returns:
+            Filtered query.
+        """
+        clauses = []
+        for result in results:
+            subject = result.get('subject', None)
+            testcase = result.get('testcase', None)
+            if subject or testcase:
+                clauses.append(and_(
+                    not subject or cls.subject == subject,
+                    not testcase or cls.testcase == testcase
+                ))
+
+        return query.filter(or_(*clauses))

