openstack/oslo.policy
Code Issues Proposed changes

Don't modify 'Rule.check'

Browse Source 
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>
This commit is contained in:
Stephen Finucane 2021-02-04 17:29:11 +00:00
parent d3185debdb
commit d933ca88ba
2 changed files with 33 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
oslo_policy/policy.py
View File

@ -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):

36
oslo_policy/tests/test_policy.py
View File

@ -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):