Don't modify 'Rule.check'

There are two big issues with this. Firstly, as noted in bug #1914095,
we're not copying the provided 'Rule' object which means if we create an
enforcer and the rule is modified, then a second enforcer in the same
process will ultimately end up using the same modified 'Rule' object
will be used. Secondly, while the 'check' attribute is modified the
'check_str' attribute is not. This is immensely confusing for people
debugging.

Resolve both issues by not modifying this check at all and instead build
the combined check and store this.

Change-Id: Iff85a9134f4db7c0355da2858deb8a704d044634
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

2 files changed, 33 insertions, 38 deletions

diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py
index 892e5d4..16b7a75 100644
--- a/oslo_policy/policy.py
+++ b/oslo_policy/policy.py
@@ -638,11 +638,15 @@ class Enforcer(object):
             for default in self.registered_rules.values():
                 if default.deprecated_for_removal:
                     self._emit_deprecated_for_removal_warning(default)
-                elif default.deprecated_rule:
-                    self._handle_deprecated_rule(default)
 
-                if default.name not in self.rules:
-                    self.rules[default.name] = default.check
+                if default.name in self.rules:
+                    continue
+
+                check = default.check
+                if default.deprecated_rule:
+                    check = self._handle_deprecated_rule(default)
+
+                self.rules[default.name] = check
 
             # Detect and log obvious incorrect rule definitions
             if self._need_check_rule:
@@ -699,11 +703,6 @@ class Enforcer(object):
                         DeprecatedRule
         """
 
-        # Short-circuit the rule deprecation logic if we've already processed
-        # it for this particular rule.
-        if default._deprecated_rule_handled:
-            return
-
         deprecated_rule = default.deprecated_rule
         deprecated_reason = (
             deprecated_rule.deprecated_reason or default.deprecated_reason)
@@ -749,10 +748,7 @@ class Enforcer(object):
                     str(self.file_rules[deprecated_rule.name].check)
                     != 'rule:%s' % default.name):
                 if default.name not in self.file_rules.keys():
-                    self.rules[default.name] = self.file_rules[
-                        deprecated_rule.name
-                    ].check
-                    default._deprecated_rule_handled = True
+                    return self.file_rules[deprecated_rule.name].check
 
         # In this case, the default check string is changing. We need to let
         # operators know that this is going to change. If they don't want to
@@ -770,14 +766,18 @@ class Enforcer(object):
                 and deprecated_rule.check_str != default.check_str
                 and default.name not in self.file_rules):
 
-            default.check = OrCheck([_parser.parse_rule(cs) for cs in
-                                     [default.check_str,
-                                      deprecated_rule.check_str]])
-            default._deprecated_rule_handled = True
             if not (self.suppress_deprecation_warnings
                     or self.suppress_default_change_warnings):
                 warnings.warn(deprecated_msg)
 
+            return OrCheck([
+                _parser.parse_rule(cs) for cs in [
+                    default.check_str, deprecated_rule.check_str,
+                ]
+            ])
+
+        return default.check
+
     def _undefined_check(self, check):
         '''Check if a RuleCheck references an undefined rule.'''
         if isinstance(check, RuleCheck):
@@ -1195,7 +1195,6 @@ class RuleDefault(object):
         self.deprecated_for_removal = deprecated_for_removal
         self.deprecated_reason = deprecated_reason
         self.deprecated_since = deprecated_since
-        self._deprecated_rule_handled = False
 
         if self.deprecated_rule:
             if not isinstance(self.deprecated_rule, DeprecatedRule):
diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py
index b97aae3..200e91c 100644
--- a/oslo_policy/tests/test_policy.py
+++ b/oslo_policy/tests/test_policy.py
@@ -788,20 +788,15 @@ class EnforcerTest(base.PolicyBaseTestCase):
         rule_original = policy.RuleDefault(
             name='test',
             check_str='role:owner',)
-        rule_original._deprecated_rule_handled = False
         self.enforcer.register_default(rule_original)
         self.enforcer.registered_rules['test'].check_str = 'role:admin'
         self.enforcer.registered_rules['test'].check = 'role:admin'
-        self.enforcer.registered_rules['test']._deprecated_rule_handled = True
         self.assertEqual(self.enforcer.registered_rules['test'].check_str,
                          'role:admin')
         self.assertEqual(self.enforcer.registered_rules['test'].check,
                          'role:admin')
-        self.assertTrue(
-            self.enforcer.registered_rules['test']._deprecated_rule_handled)
         self.assertEqual(rule_original.check_str, 'role:owner')
         self.assertEqual(rule_original.check.__str__(), 'role:owner')
-        self.assertFalse(rule_original._deprecated_rule_handled)
 
     def test_non_reversible_check(self):
         self.create_config_file('policy.json',
@@ -1783,27 +1778,28 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase):
             deprecated_reason='"role:bang" is a better default',
             deprecated_since='N'
         )
+        check = rule.check
 
         enforcer = policy.Enforcer(self.conf)
         enforcer.register_defaults([rule])
-        registered_default_rule = enforcer.registered_rules['foo:create_bar']
-        # Check that rule deprecation handling hasn't been done, yet
-        self.assertFalse(registered_default_rule._deprecated_rule_handled)
 
-        # Loading the rules will modify the rule check string to logically OR
-        # the new value with the deprecated value
-        enforcer.load_rules()
-        self.assertTrue(registered_default_rule._deprecated_rule_handled)
+        # Check that rule processing hasn't been done, yet
+        self.assertEqual({}, enforcer.rules)
 
-        # Make sure the original value is used instead of instantiating new
-        # OrCheck objects whenever we perform subsequent reloads
-        expected_check = policy.OrCheck([_parser.parse_rule(cs) for cs in
-                                         [rule.check_str,
-                                          deprecated_rule.check_str]])
+        # Load the rules
         enforcer.load_rules()
-        self.assertEqual(registered_default_rule.check.__str__(),
-                         expected_check.__str__())
-        self.assertTrue(registered_default_rule._deprecated_rule_handled)
+
+        # Loading the rules will store a version of the rule check string
+        # logically ORed with the check string of the deprecated value. Make
+        # sure this is happening but that the original rule check is unchanged
+        expected_check = policy.OrCheck([
+            _parser.parse_rule(cs) for cs in
+            [rule.check_str, deprecated_rule.check_str]
+        ])
+        self.assertIn('foo:create_bar', enforcer.rules)
+        self.assertEqual(
+            str(enforcer.rules['foo:create_bar']), str(expected_check))
+        self.assertEqual(check, rule.check)
 
 
 class DocumentedRuleDefaultTestCase(base.PolicyBaseTestCase):