diff options
| -rw-r--r-- | .pre-commit-config.yaml | 6 | ||||
| -rw-r--r-- | lower-constraints.txt | 3 | ||||
| -rw-r--r-- | oslo_policy/policy.py | 20 | ||||
| -rw-r--r-- | oslo_policy/shell.py | 4 | ||||
| -rw-r--r-- | oslo_policy/tests/test_policy.py | 55 | ||||
| -rw-r--r-- | releasenotes/notes/bug-1913718-f1b46bbff3231d98.yaml | 7 | ||||
| -rw-r--r-- | releasenotes/notes/fix-bug-1914095-fa71d81c9639ba94.yaml | 8 | ||||
| -rw-r--r-- | test-requirements.txt | 9 | ||||
| -rw-r--r-- | tox.ini | 6 |
9 files changed, 98 insertions, 20 deletions
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9d94556..e56ba97 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,9 +1,9 @@ +--- # We from the Oslo project decided to pin repos based on the # commit hash instead of the version tag to prevend arbitrary # code from running in developer's machines. To update to a # newer version, run `pre-commit autoupdate` and then replace # the newer versions with their commit hash. - default_language_version: python: python3 @@ -28,8 +28,8 @@ repos: - id: check-yaml files: .*\.(yaml|yml)$ - repo: https://gitlab.com/pycqa/flake8 - rev: 181bb46098dddf7e2d45319ea654b4b4d58c2840 # 3.8.3 + rev: 181bb46098dddf7e2d45319ea654b4b4d58c2840 # 3.8.3 hooks: - id: flake8 additional_dependencies: - - hacking>=3.0.1,<3.1.0 + - hacking>=3.2.0,<3.3.0 diff --git a/lower-constraints.txt b/lower-constraints.txt index c83f09b..2d9d8ef 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -4,7 +4,7 @@ Babel==2.3.4 bandit==1.4.0 coverage==4.0 debtcollector==1.2.0 -docutils==0.11 +docutils==0.12 dulwich==0.15.0 extras==1.0.0 fixtures==3.0.0 @@ -38,6 +38,7 @@ requests==2.14.2 requests-mock==1.2.0 requestsexceptions==1.2.0 rfc3986==0.3.1 +Sphinx==2.0.0 stestr==2.0.0 smmap==0.9.0 snowballstemmer==1.2.1 diff --git a/oslo_policy/policy.py b/oslo_policy/policy.py index f21ebe9..903f65b 100644 --- a/oslo_policy/policy.py +++ b/oslo_policy/policy.py @@ -221,7 +221,7 @@ by setting the ``policy_default_rule`` configuration setting to the desired rule name. """ -import collections +import collections.abc import copy import logging import os @@ -698,6 +698,11 @@ 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_msg = ( @@ -742,6 +747,7 @@ class Enforcer(object): self.rules[default.name] = self.file_rules[ deprecated_rule.name ].check + default._deprecated_rule_handled = True # 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 @@ -762,6 +768,7 @@ class Enforcer(object): 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) @@ -953,9 +960,9 @@ class Enforcer(object): # a method on RequestContext objects that converts attributes of the # context object to policy values. However, ``to_policy_values()`` # doesn't actually return a dictionary, it's a subclass of - # collections.MutableMapping, which behaves like a dictionary but + # collections.abc.MutableMapping, which behaves like a dictionary but # doesn't pass the type check. - elif not isinstance(creds, collections.MutableMapping): + elif not isinstance(creds, collections.abc.MutableMapping): msg = ( 'Expected type oslo_context.context.RequestContext, dict, or ' 'the output of ' @@ -1094,7 +1101,11 @@ class Enforcer(object): if default.name in self.registered_rules: raise DuplicatePolicyError(default.name) - self.registered_rules[default.name] = default + # NOTE Always make copy of registered rule because policy engine + # update these rules in many places (one example is + # self._handle_deprecated_rule() ). This will avoid any conflict + # in rule object values when running tests in parallel. + self.registered_rules[default.name] = copy.deepcopy(default) def register_defaults(self, defaults): """Registers a list of RuleDefaults. @@ -1180,6 +1191,7 @@ 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/shell.py b/oslo_policy/shell.py index 7ffb5ea..da1bf1e 100644 --- a/oslo_policy/shell.py +++ b/oslo_policy/shell.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -import collections +import collections.abc import sys from oslo_serialization import jsonutils @@ -59,7 +59,7 @@ def flatten(d, parent_key=''): items = [] for k, v in d.items(): new_key = parent_key + '.' + k if parent_key else k - if isinstance(v, collections.MutableMapping): + if isinstance(v, collections.abc.MutableMapping): items.extend(flatten(v, new_key).items()) else: items.append((new_key, v)) diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py index f27f6b1..513b43d 100644 --- a/oslo_policy/tests/test_policy.py +++ b/oslo_policy/tests/test_policy.py @@ -784,6 +784,25 @@ class EnforcerTest(base.PolicyBaseTestCase): policy.RuleDefault(name='owner', check_str='role:owner')) + def test_enforcer_does_not_modify_original_registered_rule(self): + 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', jsonutils.dumps( @@ -1749,6 +1768,42 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase): enforcer.enforce('foo:create_bar', {}, {'roles': ['baz']}) ) + def test_deprecation_logic_is_only_performed_once_per_rule(self): + deprecated_rule = policy.DeprecatedRule( + name='foo:create_bar', + check_str='role:fizz' + ) + rule = 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' + ) + + 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) + + # 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]]) + enforcer.load_rules() + self.assertEqual(registered_default_rule.check.__str__(), + expected_check.__str__()) + self.assertTrue(registered_default_rule._deprecated_rule_handled) + class DocumentedRuleDefaultTestCase(base.PolicyBaseTestCase): diff --git a/releasenotes/notes/bug-1913718-f1b46bbff3231d98.yaml b/releasenotes/notes/bug-1913718-f1b46bbff3231d98.yaml new file mode 100644 index 0000000..509d6af --- /dev/null +++ b/releasenotes/notes/bug-1913718-f1b46bbff3231d98.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + [`bug 1913718 <https://launchpad.net/bugs/1913718>`_] + The `Enforcer()` object now only processes deprecated rules once at load or + enforcement time, improving performance for users that make extensive use + of policy enforcement. diff --git a/releasenotes/notes/fix-bug-1914095-fa71d81c9639ba94.yaml b/releasenotes/notes/fix-bug-1914095-fa71d81c9639ba94.yaml new file mode 100644 index 0000000..17f2c6f --- /dev/null +++ b/releasenotes/notes/fix-bug-1914095-fa71d81c9639ba94.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + This fixes the Bug# 1914095. Policy engine has bug of modifying the + registered rule original object which caused issue when there are + multiple policy objects are processing rules in parallel. + With this fix. policy engine will make copies of all the registered rules + and process accordingly. diff --git a/test-requirements.txt b/test-requirements.txt index fb0d03a..3717b43 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,16 +1,11 @@ # The order of packages is significant, because pip processes them in the order # of appearance. Changing the order has an impact on the overall integration # process, which may cause wedges in the gate later. -hacking>=3.0.1,<3.1.0 # Apache-2.0 + oslotest>=3.2.0 # Apache-2.0 requests-mock>=1.2.0 # Apache-2.0 stestr>=2.0.0 # Apache-2.0 -oslo.context>=2.22.0 # Apache-2.0 +sphinx>=2.0.0,!=2.1.0 # BSD # computes code coverage percentages coverage!=4.4,>=4.0 # Apache-2.0 - -# Bandit security code scanner -bandit>=1.6.0,<1.7.0 # Apache-2.0 - -pre-commit>=2.6.0 # MIT @@ -6,7 +6,7 @@ ignore_basepython_conflict = true [testenv] basepython = python3 deps = - -c{env:UPPER_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} -r{toxinidir}/test-requirements.txt -r{toxinidir}/requirements.txt -r{toxinidir}/doc/requirements.txt @@ -14,7 +14,8 @@ commands = stestr run --slowest {posargs} [testenv:pep8] deps = - -r{toxinidir}/test-requirements.txt + pre-commit>=2.6.0 # MIT + bandit>=1.6.0,<1.7.0 # Apache-2.0 commands = pre-commit run -a # Run security linter @@ -66,4 +67,3 @@ deps = -c{toxinidir}/lower-constraints.txt -r{toxinidir}/test-requirements.txt -r{toxinidir}/requirements.txt - -r{toxinidir}/doc/requirements.txt |