Merge "Make upgrades more robust with policy overrides"

2 files changed, 204 insertions, 60 deletions

diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py
index fdd48bb..fe430bc 100644
--- a/oslo_policy/policy.py
+++ b/oslo_policy/policy.py
@@ -575,66 +575,11 @@ class Enforcer(object):
                                                         force_reload, False)
 
             for default in self.registered_rules.values():
-                if default.deprecated_rule:
-                    deprecated_msg = (
-                        'Policy "%(old_name)s":"%(old_check_str)s" was '
-                        'deprecated in %(release)s in favor of "%(name)s":'
-                        '"%(check_str)s". Reason: %(reason)s. Either ensure '
-                        'your deployment is ready for the new default or '
-                        'copy/paste the deprecated policy into your policy '
-                        'file and maintain it manually.' % {
-                            'old_name': default.deprecated_rule.name,
-                            'old_check_str': default.deprecated_rule.check_str,
-                            'release': default.deprecated_since,
-                            'name': default.name,
-                            'check_str': default.check_str,
-                            'reason': default.deprecated_reason
-                        }
-                    )
-                    if default.deprecated_rule.name != default.name and (
-                            default.deprecated_rule.name in self.rules):
-                        # Print a warning because the actual policy name is
-                        # changing. If deployers are relying on an override for
-                        # foo:bar and it's getting renamed to foo:create_bar
-                        # then they need to be able to see that before they
-                        # roll out the next release.
-                        warnings.warn(deprecated_msg)
-                    if (default.deprecated_rule.check_str !=
-                            default.check_str and default.name not in
-                            self.rules):
-                        # In this case, the default check_str is changing. We
-                        # need to let operators know that this is going to
-                        # change. If they don't want to override it, they are
-                        # going to have to make sure the right infrastructure
-                        # exists before they upgrade. This overrides the new
-                        # check with an OrCheck that combines the new and old
-                        # check_str attributes from the new and deprecated
-                        # policies. This will make it so that deployments don't
-                        # break on upgrade, but they receive log messages
-                        # telling them stuff is going to change if they don't
-                        # maintain the policy manually or add infrastructure to
-                        # their deployment to support the new policy.
-                        default.check = _parser.parse_rule(
-                            default.check_str + ' or ' +
-                            default.deprecated_rule.check_str
-                        )
-                        warnings.warn(deprecated_msg)
-                if default.deprecated_for_removal and (
-                        default.name in self.file_rules):
-                    # If a policy is going to be removed altogether, then we
-                    # need to make sure we let operators know so they can clean
-                    # up their policy files, if they are overriding it.
-                    warnings.warn(
-                        'Policy "%(policy)s":"%(check_str)s" was '
-                        'deprecated for removal in %(release)s. Reason: '
-                        '%(reason)s. Its value may be silently ignored in '
-                        'the future.' % {
-                            'policy': default.name,
-                            'check_str': default.check_str,
-                            'release': default.deprecated_since,
-                            'reason': default.deprecated_reason
-                        }
-                    )
+                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
 
@@ -668,6 +613,87 @@ class Enforcer(object):
 
         return not violation
 
+    def _emit_deprecated_for_removal_warning(self, default):
+        # If the policy is being removed completely, we need to let operators
+        # know that the policy is going to be silently ignored in the future
+        # and they can remove it from their overrides since it isn't being
+        # replaced by another policy.
+        if default.name in self.file_rules:
+            warnings.warn(
+                'Policy "%(policy)s":"%(check_str)s" was deprecated for '
+                'removal in %(release)s. Reason: %(reason)s. Its value may be '
+                'silently ignored in the future.' % {
+                    'policy': default.name,
+                    'check_str': default.check_str,
+                    'release': default.deprecated_since,
+                    'reason': default.deprecated_reason
+                }
+            )
+
+    def _handle_deprecated_rule(self, default):
+        """Handle cases where a policy rule has been deprecated.
+
+        :param default: an instance of RuleDefault that contains an instance of
+                        DeprecatedRule
+        """
+
+        deprecated_rule = default.deprecated_rule
+
+        deprecated_msg = (
+            'Policy "%(old_name)s":"%(old_check_str)s" was deprecated in '
+            '%(release)s in favor of "%(name)s":"%(check_str)s". Reason: '
+            '%(reason)s. Either ensure your deployment is ready for the new '
+            'default or copy/paste the deprecated policy into your policy '
+            'file and maintain it manually.' % {
+                'old_name': deprecated_rule.name,
+                'old_check_str': deprecated_rule.check_str,
+                'release': default.deprecated_since,
+                'name': default.name,
+                'check_str': default.check_str,
+                'reason': default.deprecated_reason
+            }
+        )
+
+        # Print a warning because the actual policy name is changing. If
+        # operators are relying on an override for foo:bar and it's getting
+        # renamed to foo:create_bar then they need to be able to see that
+        # before they roll out the next release. If the policy name is in
+        # self.file_rules, we know that it's being overridden.
+        if deprecated_rule.name != default.name and (
+                deprecated_rule.name in self.file_rules):
+
+            warnings.warn(deprecated_msg)
+
+            # If the deprecated policy is being overridden and doesn't match
+            # the default deprecated policy, override the new policy's default
+            # with the old check string. This should prevents unwanted exposure
+            # to APIs on upgrade.
+            if (self.file_rules[deprecated_rule.name].check
+                    != _parser.parse_rule(deprecated_rule.check_str)):
+                if default.name not in self.file_rules.keys():
+                    self.rules[default.name] = 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
+        # override it, they are going to have to make sure the right
+        # infrastructure exists before they upgrade. This overrides the new
+        # check with an OrCheck that combines the new and old check_str
+        # attributes from the new and deprecated policies. This will make it so
+        # that deployments don't break on upgrade, but they receive log
+        # messages telling them stuff is going to change if they don't maintain
+        # the policy manually or add infrastructure to their deployment to
+        # support the new policy.
+        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
+            )
+            warnings.warn(deprecated_msg)
+
     def _undefined_check(self, check):
         '''Check if a RuleCheck references an undefined rule.'''
         if isinstance(check, RuleCheck):
diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py
index 2285381..8eaca5e 100644
--- a/oslo_policy/tests/test_policy.py
+++ b/oslo_policy/tests/test_policy.py
@@ -1235,6 +1235,124 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase):
             deprecated_since='N'
         )
 
+    def test_override_deprecated_policy_with_old_name(self):
+        # Simulate an operator overriding a policy
+        rules = jsonutils.dumps({'foo:bar': 'role:bazz'})
+        self.create_config_file('policy.json', rules)
+
+        # Deprecate the policy name and check string in favor of something
+        # better.
+        deprecated_rule = policy.DeprecatedRule(
+            name='foo:bar',
+            check_str='role:fizz'
+        )
+        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='"role:bang" is a better default',
+            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('foo:create_bar', {}, {'roles': ['fizz']})
+        )
+        self.assertFalse(
+            self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bang']})
+        )
+        self.assertTrue(
+            self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bazz']})
+        )
+
+    def test_override_deprecated_policy_with_new_name(self):
+        # Simulate an operator overriding a policy using the new policy name
+        rules = jsonutils.dumps({'foo:create_bar': 'role:bazz'})
+        self.create_config_file('policy.json', rules)
+
+        # Deprecate the policy name and check string in favor of something
+        # better.
+        deprecated_rule = policy.DeprecatedRule(
+            name='foo:bar',
+            check_str='role:fizz'
+        )
+        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='"role:bang" is a better default',
+            deprecated_since='N'
+        )]
+        self.enforcer.register_defaults(rule_list)
+
+        # Make sure the override supplied by the operator is being used in
+        # place of either default value.
+        self.assertFalse(
+            self.enforcer.enforce('foo:create_bar', {}, {'roles': ['fizz']})
+        )
+        self.assertFalse(
+            self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bang']})
+        )
+        self.assertTrue(
+            self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bazz']})
+        )
+
+    def test_override_both_new_and_old_policy(self):
+        # Simulate an operator overriding a policy using both the the new and
+        # old policy names. The following doesn't make a whole lot of sense
+        # because the overrides are conflicting, but we want to make sure that
+        # oslo.policy uses foo:create_bar instead of foo:bar.
+        rules_dict = {
+            'foo:create_bar': 'role:bazz',
+            'foo:bar': 'role:wee'
+        }
+        rules = jsonutils.dumps(rules_dict)
+        self.create_config_file('policy.json', rules)
+
+        # Deprecate the policy name and check string in favor of something
+        # better.
+        deprecated_rule = policy.DeprecatedRule(
+            name='foo:bar',
+            check_str='role:fizz'
+        )
+        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='"role:bang" is a better default',
+            deprecated_since='N'
+        )]
+        self.enforcer.register_defaults(rule_list)
+
+        # The default check string for the old policy name foo:bar should fail
+        self.assertFalse(
+            self.enforcer.enforce('foo:create_bar', {}, {'roles': ['fizz']})
+        )
+
+        # The default check string for the new policy name foo:create_bar
+        # should fail
+        self.assertFalse(
+            self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bang']})
+        )
+
+        # The override for the old policy name foo:bar should fail
+        self.assertFalse(
+            self.enforcer.enforce('foo:create_bar', {}, {'roles': ['wee']})
+        )
+
+        # The override for foo:create_bar should pass
+        self.assertTrue(
+            self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bazz']})
+        )
+
 
 class DocumentedRuleDefaultTestCase(base.PolicyBaseTestCase):