Merge ~alexmurray/review-tools:validate-parsed-plugs-slots-json-for-bare-scalars into review-tools:master

[Alex Murray]

many: when parsing plugs/slots json validate for bare scalars

The store will raise an error if the provided plugs/slots json contains bare
scalars - instead these need to be strings. So when parsing these from the user,
validate them against this condition so we can warn the user before they go and
try to update the declaration in the store using something that would ultimately
fail.

Signed-off-by: Alex Murray <email address hidden>

[Evan Caville (evancaville)]

LGTM - thanks for adding this in!

review: Approve

diff --git a/bin/snap-review b/bin/snap-review
index 1b9ebc0..37c9fea 100755
--- a/bin/snap-review
+++ b/bin/snap-review
@@ -18,6 +18,8 @@ from reviewtools.common import (
     verify_override_state,
 )
 
+from reviewtools.sr_declaration import validate_json
+
 
 def print_findings(results, description):
     """
@@ -211,13 +213,21 @@ def main():
     if args.plugs:
         if overrides is None:
             overrides = {}
-        with open(args.plugs, "r") as plugs_file:
-            overrides["snap_decl_plugs"] = json.loads(plugs_file.read())
+        try:
+            with open(args.plugs, "r") as plugs_file:
+                overrides["snap_decl_plugs"] = json.loads(plugs_file.read())
+                validate_json(overrides["snap_decl_plugs"])
+        except Exception as e:
+            error("Failed to read plugs: %s" % e, output_type=error_output_type)
     if args.slots:
         if overrides is None:
             overrides = {}
-        with open(args.slots, "r") as slots_file:
-            overrides["snap_decl_slots"] = json.loads(slots_file.read())
+        try:
+            with open(args.slots, "r") as slots_file:
+                overrides["snap_decl_slots"] = json.loads(slots_file.read())
+                validate_json(overrides["snap_decl_slots"])
+        except Exception as e:
+            error("Failed to read slots: %s" % e, output_type=error_output_type)
     if args.allow_classic:
         if overrides is None:
             overrides = {}
diff --git a/bin/snap-verify-declaration b/bin/snap-verify-declaration
index b596387..152c6c7 100755
--- a/bin/snap-verify-declaration
+++ b/bin/snap-verify-declaration
@@ -63,14 +63,16 @@ def main():
         try:
             with open(args.plugs, "r") as plugs_file:
                 snap_decl["plugs"] = json.loads(plugs_file.read())
+                sr_declaration.validate_json(snap_decl["plugs"])
         except Exception as e:
-            error("Could not read plugs: %s" % e, output_type=error_output_type)
+            error("Failed to read plugs: %s" % e, output_type=error_output_type)
     if args.slots:
         try:
             with open(args.slots, "r") as slots_file:
                 snap_decl["slots"] = json.loads(slots_file.read())
+                sr_declaration.validate_json(snap_decl["slots"])
         except Exception as e:
-            error("Could not read slots: %s" % e, output_type=error_output_type)
+            error("Failed to read slots: %s" % e, output_type=error_output_type)
 
     try:
         review = sr_declaration.verify_snap_declaration(snap_decl)
diff --git a/reviewtools/sr_declaration.py b/reviewtools/sr_declaration.py
index 2978593..5af713a 100644
--- a/reviewtools/sr_declaration.py
+++ b/reviewtools/sr_declaration.py
@@ -1476,6 +1476,32 @@ class SnapReviewDeclaration(SnapReview):
         self._verify_declaration_apps_hooks("hooks")
 
 
+# check there are no bare boolean / integer etc values in the provided json data
+# since the store will fail to accept them
+def validate_json(data, path=[]):
+    def validate_value(value, key, path):
+        if (
+            isinstance(value, bool)
+            or isinstance(value, int)
+            or isinstance(value, float)
+        ):
+            raise ValueError(
+                "bare %s value %s for '%s' is not allowed - this should be a string \"%s\" instead"
+                % (str(type(value)), value, ":".join(path + [key]), str(value).lower())
+            )
+        if isinstance(value, dict):
+            validate_json(value, path + [key])
+        if isinstance(value, list):
+            validate_json(value, path + [key])
+
+    if isinstance(data, dict):
+        for key, value in data.items():
+            validate_value(value, key, path)
+    if isinstance(data, list):
+        for i, value in enumerate(data):
+            validate_value(value, str(i), path)
+
+
 #
 # Helper functions
 #
diff --git a/reviewtools/tests/test_sr_declaration.py b/reviewtools/tests/test_sr_declaration.py
index 21b1f4e..cef3967 100644
--- a/reviewtools/tests/test_sr_declaration.py
+++ b/reviewtools/tests/test_sr_declaration.py
@@ -18,6 +18,7 @@ from reviewtools.sr_declaration import (
     SnapReviewDeclaration,
     SnapDeclarationException,
     verify_snap_declaration,
+    validate_json,
 )
 import reviewtools.sr_tests as sr_tests
 from unittest import TestCase
@@ -6297,7 +6298,14 @@ slots:
         slots = {"iface": {"interface": "upower-observe"}}
         self.set_test_snap_yaml("slots", slots)
         self.set_test_snap_yaml("type", "app")
-        overrides = {"snap_decl_slots": {"upower-observe": {"allow-connection": True, "allow-auto-connection": True}}}
+        overrides = {
+            "snap_decl_slots": {
+                "upower-observe": {
+                    "allow-connection": True,
+                    "allow-auto-connection": True,
+                }
+            }
+        }
         c = SnapReviewDeclaration(self.test_name, overrides=overrides)
         self._use_test_base_declaration(c)
 
@@ -9051,3 +9059,17 @@ class TestSnapDeclarationVerify(TestCase):
 
                 pprint.pprint(r)
                 raise
+
+
+class TestSnapDeclarationValidateJson(TestCase):
+    def test_validate_json(self):
+        # test validation of bare scalar values in parsed data
+        for decl in [
+            {"foo": 1},
+            {"foo": [1]},
+            {"foo": [{"bar": 1}]},
+            {"foo": [{"bar": [{"baz": True}]}]},
+            {"foo": [{"bar": [{"baz": [1.0]}]}]},
+        ]:
+            with self.assertRaises(ValueError):
+                validate_json(decl)
diff --git a/tests/test-snap-verify-declaration.sh b/tests/test-snap-verify-declaration.sh
index 40b1dd3..3d1d5ad 100755
--- a/tests/test-snap-verify-declaration.sh
+++ b/tests/test-snap-verify-declaration.sh
@@ -107,6 +107,17 @@ EOM
 run --plugs="./plugs"
 run --json --plugs="./plugs"
 
+# invalid (bare scalars)
+cat > "./plugs" <<EOM
+{
+  "mir": {
+    "allow-auto-connection": true
+  }
+}
+EOM
+run --plugs="./plugs"
+run --json --plugs="./plugs"
+
 
 cd "$origdir"
 echo
diff --git a/tests/test-snap-verify-declaration.sh.expected b/tests/test-snap-verify-declaration.sh.expected
index 026401b..0149682 100644
--- a/tests/test-snap-verify-declaration.sh.expected
+++ b/tests/test-snap-verify-declaration.sh.expected
@@ -135,7 +135,7 @@ Running: snap-verify-declaration --json --plugs=./plugs
 }
 
 Running: snap-verify-declaration --plugs=./plugs
-ERROR: Could not read plugs: Expecting property name enclosed in double quotes: line 2 column 1 (char 2)
+ERROR: Failed to read plugs: Expecting property name enclosed in double quotes: line 2 column 1 (char 2)
 
 Running: snap-verify-declaration --json --plugs=./plugs
 {
@@ -143,7 +143,7 @@ Running: snap-verify-declaration --json --plugs=./plugs
     "error": {
       "msg": {
         "manual_review": true,
-        "text": "Could not read plugs: Expecting property name enclosed in double quotes: line 2 column 1 (char 2)"
+        "text": "Failed to read plugs: Expecting property name enclosed in double quotes: line 2 column 1 (char 2)"
       }
     },
     "info": {},
@@ -204,3 +204,20 @@ Running: snap-verify-declaration --json --plugs=./plugs
   "warn": {}
 }
 
+Running: snap-verify-declaration --plugs=./plugs
+ERROR: Failed to read plugs: bare <class 'bool'> value True for 'mir:allow-auto-connection' is not allowed - this should be a string "true" instead
+
+Running: snap-verify-declaration --json --plugs=./plugs
+{
+  "runtime-errors": {
+    "error": {
+      "msg": {
+        "manual_review": true,
+        "text": "Failed to read plugs: bare <class 'bool'> value True for 'mir:allow-auto-connection' is not allowed - this should be a string \"true\" instead"
+      }
+    },
+    "info": {},
+    "warn": {}
+  }
+}
+