From 1309c0c0ea570138c5ea04fc29a4f460c9aa90c4 Mon Sep 17 00:00:00 2001 From: Brian Rosmaita Date: Mon, 26 Nov 2018 17:12:51 -0500 Subject: [PATCH] Enhance test to prevent JSON parsing regression Change I43782d245d7652ba69613b26fe598ac79ec19929 added a policy file parsing optimization that had the affect of allowing some strictly speaking invalid JSON policy files to be accepted. Enhance the test for bad JSON to look for this to prevent formerly acceptable policy files from becoming invalid if the code is refactored at some point. Also updates two related comments that had gotten out of sync with the code. Change-Id: I6a269b91436cac29bd72e11dbdc51ee74feca028 --- oslo_policy/tests/test_policy.py | 39 ++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py index d5e66869..986ec637 100644 --- a/oslo_policy/tests/test_policy.py +++ b/oslo_policy/tests/test_policy.py @@ -121,8 +121,7 @@ class RulesTestCase(test_base.BaseTestCase): @mock.patch.object(_parser, 'parse_rule', lambda x: x) def test_load_json_invalid_exc(self): - # When the JSON isn't valid, ValueError is raised on load_json. - # Note the trailing , in the exemplar is invalid JSON. + # When the JSON isn't valid, ValueError is raised on load. exemplar = """{ "admin_or_owner": [["role:admin"], ["project_id:%(project_id)s"]], "default": [ @@ -130,6 +129,38 @@ class RulesTestCase(test_base.BaseTestCase): self.assertRaises(ValueError, policy.Rules.load, exemplar, 'default') + # However, since change I43782d245d7652ba69613b26fe598ac79ec19929, + # policy.Rules.load() first tries loading with the really fast + # jsonutils.loads(), and if that fails, it tries loading with + # yaml.safe_load(). Since YAML is a superset of JSON, some strictly + # invalid JSON can be parsed correctly by policy.Rules.load() without + # raising an exception. But that means that since 1.17.0, we've been + # accepting (strictly speaking) illegal JSON policy files, and for + # backward compatibility, we should continue to do so. Thus the + # following are here to prevent regressions: + + # JSON requires double quotes, but the YAML parser doesn't care + bad_but_acceptable = """{ + 'admin_or_owner': [["role:admin"], ["project_id:%(project_id)s"]], + 'default': [] +}""" + self.assertTrue(policy.Rules.load(bad_but_acceptable, 'default')) + + # JSON does not allow bare keys, but the YAML parser doesn't care + bad_but_acceptable = """{ + admin_or_owner: [["role:admin"], ["project_id:%(project_id)s"]], + default: [] +}""" + self.assertTrue(policy.Rules.load(bad_but_acceptable, 'default')) + + # JSON is picky about commas, but the YAML parser is more forgiving + # (Note the trailing , in the exemplar is invalid JSON.) + bad_but_acceptable = """{ + admin_or_owner: [["role:admin"], ["project_id:%(project_id)s"]], + default: [], +}""" + self.assertTrue(policy.Rules.load(bad_but_acceptable, 'default')) + @mock.patch.object(_parser, 'parse_rule', lambda x: x) def test_load_empty_data(self): result = policy.Rules.load('', 'default') @@ -155,8 +186,8 @@ default: [] @mock.patch.object(_parser, 'parse_rule', lambda x: x) def test_load_yaml_invalid_exc(self): - # When the JSON isn't valid, ValueError is raised on load(). - # Note the trailing , in the exemplar is invalid JSON. + # When the JSON is seriously invalid, ValueError is raised on load(). + # (See test_load_json_invalid_exc for what 'seriously invalid' means.) exemplar = """{ # Define a custom rule. admin_or_owner: role:admin or project_id:%(project_id)s