From 0a4adba1a53c25ad75a10525130d832138f4fc68 Mon Sep 17 00:00:00 2001
From: Ben Nemec <bnemec@redhat.com>
Date: Fri, 13 Sep 2019 20:04:56 +0000
Subject: [PATCH] Fix reference cycle caused by deprecated sample override

In the sample policy generator, we create a rule that maps the
deprecated name of a policy to the new rule name. For example:

identity:old_rule: rule:identity:new_rule

However, in the policy code, if we see an override of a deprecated
name and no override for the new name, we apply the value of the
deprecated name to the new name. In the above case, this results
in us creating a rule that looks like:

identity:new_rule: rule:identity:new_rule

which is a circular reference and nonsense.

To fix this, I added a check to the deprecated rule logic that looks
for instances where the old override is just a reference to the new
rule. If that's the case, then we don't need to do anything because
it's already doing the right thing.

Change-Id: Ifd14993bc84e83c13abab3456fbf670c06e5806f
Closes-Bug: 1843931
(cherry picked from commit 82a2c8d8b71896b1f8b7f33c560681367ae76755)
---
 oslo_policy/policy.py            |  8 +++++++-
 oslo_policy/tests/test_policy.py | 33 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py
index dd53ba7f..2b19eda0 100644
--- a/oslo_policy/policy.py
+++ b/oslo_policy/policy.py
@@ -668,8 +668,14 @@ class Enforcer(object):
             # the default deprecated policy, override the new policy's default
             # with the old check string. This should prevents unwanted exposure
             # to APIs on upgrade.
+            # There's one exception to this: When we generate a sample policy,
+            # we set the deprecated rule name to reference the new rule. If we
+            # see that the deprecated override rule is just the new rule, then
+            # we shouldn't mess with it.
             if (self.file_rules[deprecated_rule.name].check
-                    != _parser.parse_rule(deprecated_rule.check_str)):
+                    != _parser.parse_rule(deprecated_rule.check_str) and
+                    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
diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py
index 9a7f6a41..a39e8256 100644
--- a/oslo_policy/tests/test_policy.py
+++ b/oslo_policy/tests/test_policy.py
@@ -1452,6 +1452,39 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase):
             self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bazz']})
         )
 
+    def test_override_deprecated_policy_with_new_rule(self):
+        # Simulate an operator overriding a deprecated policy with a reference
+        # to the new policy, as done by the sample policy generator.
+        rules = jsonutils.dumps({'old_rule': 'rule:new_rule'})
+        self.create_config_file('policy.json', rules)
+
+        # Deprecate the policy name in favor of something better.
+        deprecated_rule = policy.DeprecatedRule(
+            name='old_rule',
+            check_str='role:bang'
+        )
+        rule_list = [policy.DocumentedRuleDefault(
+            name='new_rule',
+            check_str='role:bang',
+            description='Replacement for old_rule.',
+            operations=[{'path': '/v1/bars', 'method': 'POST'}],
+            deprecated_rule=deprecated_rule,
+            deprecated_reason='"old_rule" is a bad name',
+            deprecated_since='N'
+        )]
+        self.enforcer.register_defaults(rule_list)
+
+        # Make sure the override supplied by the operator using the old policy
+        # name is used in favor of the old or new default.
+        self.assertFalse(
+            self.enforcer.enforce('new_rule', {}, {'roles': ['fizz']})
+        )
+        self.assertTrue(
+            self.enforcer.enforce('new_rule', {}, {'roles': ['bang']})
+        )
+        # Verify that we didn't overwrite the new rule.
+        self.assertEqual('bang', self.enforcer.rules['new_rule'].match)
+
 
 class DocumentedRuleDefaultTestCase(base.PolicyBaseTestCase):