From b93f51c1aa85ca4735118668d1f890ca0fbe941d Mon Sep 17 00:00:00 2001
From: Zane Bitter <zbitter@redhat.com>
Date: Thu, 12 Dec 2019 11:03:30 -0500
Subject: [PATCH] Don't use string processing to combine deprecated rules

Constructing a policy string by sticking ' or ' between the new and
deprecated check_str values is error-prone. Construct the policy
programmatically instead by parsing the check_str values separately and
combining them into an OrCheck.

Change-Id: Ia2ee05aa08326c6daa214a7b1156baa6efe43dc0
Closes-Bug: #1856207
---
 oslo_policy/policy.py            |  7 ++--
 oslo_policy/tests/test_policy.py | 64 ++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py
index 3429ea4b..61a3597d 100644
--- a/oslo_policy/policy.py
+++ b/oslo_policy/policy.py
@@ -699,10 +699,9 @@ class Enforcer(object):
         if (deprecated_rule.check_str != default.check_str
                 and default.name not in self.file_rules):
 
-            default.check = _parser.parse_rule(
-                default.check_str + ' or ' +
-                deprecated_rule.check_str
-            )
+            default.check = OrCheck([_parser.parse_rule(cs) for cs in
+                                     [default.check_str,
+                                      deprecated_rule.check_str]])
             if not self.suppress_deprecation_warnings:
                 warnings.warn(deprecated_msg)
 
diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py
index 88d363ae..bc41054d 100644
--- a/oslo_policy/tests/test_policy.py
+++ b/oslo_policy/tests/test_policy.py
@@ -1190,6 +1190,70 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase):
             enforcer.load_rules()
             mock_warn.assert_called_once_with(expected_msg)
 
+        self.assertTrue(
+            enforcer.enforce('foo:create_bar', {}, {'roles': ['bang']})
+        )
+        self.assertTrue(
+            enforcer.enforce('foo:create_bar', {}, {'roles': ['fizz']})
+        )
+        self.assertFalse(
+            enforcer.enforce('foo:create_bar', {}, {'roles': ['baz']})
+        )
+
+    def test_deprecate_an_empty_policy_check_string(self):
+        deprecated_rule = policy.DeprecatedRule(
+            name='foo:create_bar',
+            check_str=''
+        )
+
+        rule_list = [policy.DocumentedRuleDefault(
+            name='foo:create_bar',
+            check_str='role:bang',
+            description='Create a bar.',
+            operations=[{'path': '/v1/bars', 'method': 'POST'}],
+            deprecated_rule=deprecated_rule,
+            deprecated_reason='because of reasons',
+            deprecated_since='N'
+        )]
+        enforcer = policy.Enforcer(self.conf)
+        enforcer.register_defaults(rule_list)
+
+        with mock.patch('warnings.warn') as mock_warn:
+            enforcer.load_rules()
+            mock_warn.assert_called_once()
+
+        enforcer.enforce('foo:create_bar', {}, {'roles': ['bang']},
+                         do_raise=True)
+        enforcer.enforce('foo:create_bar', {}, {'roles': ['fizz']},
+                         do_raise=True)
+
+    def test_deprecate_replace_with_empty_policy_check_string(self):
+        deprecated_rule = policy.DeprecatedRule(
+            name='foo:create_bar',
+            check_str='role:fizz'
+        )
+
+        rule_list = [policy.DocumentedRuleDefault(
+            name='foo:create_bar',
+            check_str='',
+            description='Create a bar.',
+            operations=[{'path': '/v1/bars', 'method': 'POST'}],
+            deprecated_rule=deprecated_rule,
+            deprecated_reason='because of reasons',
+            deprecated_since='N'
+        )]
+        enforcer = policy.Enforcer(self.conf)
+        enforcer.register_defaults(rule_list)
+
+        with mock.patch('warnings.warn') as mock_warn:
+            enforcer.load_rules()
+            mock_warn.assert_called_once()
+
+        enforcer.enforce('foo:create_bar', {}, {'roles': ['fizz']},
+                         do_raise=True)
+        enforcer.enforce('foo:create_bar', {}, {'roles': ['bang']},
+                         do_raise=True)
+
     def test_deprecate_a_policy_name(self):
         deprecated_rule = policy.DeprecatedRule(
             name='foo:bar',