Merge "Use oslo_policy.policy.Rules.load to load rules"
diff --git a/patrole_tempest_plugin/config.py b/patrole_tempest_plugin/config.py
index 54d2491..56a786b 100644
--- a/patrole_tempest_plugin/config.py
+++ b/patrole_tempest_plugin/config.py
@@ -31,7 +31,7 @@
assumes Patrole is on the same host as the policy files. The paths should be
ordered by precedence, with high-priority paths before low-priority paths. All
the paths that are found to contain the service's policy file will be used and
-all policy files will be merged.
+all policy files will be merged. Allowed ``json`` or ``yaml`` formats.
"""),
cfg.BoolOpt('test_custom_requirements',
default=False,
diff --git a/patrole_tempest_plugin/policy_authority.py b/patrole_tempest_plugin/policy_authority.py
index 27786ae..2a49b6c 100644
--- a/patrole_tempest_plugin/policy_authority.py
+++ b/patrole_tempest_plugin/policy_authority.py
@@ -16,7 +16,6 @@
import collections
import copy
import glob
-import json
import os
from oslo_log import log as logging
@@ -112,7 +111,7 @@
if CONF.patrole.custom_policy_files:
self.discover_policy_files()
- self.rules = policy.Rules.load(self._get_policy_data(), 'default')
+ self.rules = self.get_rules()
self.project_id = project_id
self.user_id = user_id
self.extra_target_data = extra_target_data
@@ -170,81 +169,60 @@
is_admin=is_admin_context)
return is_allowed
- def _get_policy_data(self):
- file_policy_data = {}
- mgr_policy_data = {}
- policy_data = {}
-
+ def get_rules(self):
+ rules = policy.Rules()
# Check whether policy file exists and attempt to read it.
for path in self.policy_files[self.service]:
try:
with open(path, 'r') as fp:
- for k, v in json.load(fp).items():
- if k not in file_policy_data:
- file_policy_data[k] = v
- else:
- # If the policy name and rule are the same, no
- # ambiguity, so no reason to warn.
- if v != file_policy_data[k]:
- LOG.warning(
- "The same policy name: %s was found in "
- "multiple policies files for service %s. "
- "This can lead to policy rule ambiguity. "
- "Using rule: %s", k, self.service,
- file_policy_data[k])
- except (IOError, ValueError) as e:
- msg = "Failed to read policy file for service. "
- if isinstance(e, IOError):
- msg += "Please check that policy path exists."
- else:
- msg += "JSON may be improperly formatted."
- LOG.debug(msg)
+ for k, v in policy.Rules.load(fp.read()).items():
+ if k not in rules:
+ rules[k] = v
+ # If the policy name and rule are the same, no
+ # ambiguity, so no reason to warn.
+ elif str(v) != str(rules[k]):
+ msg = ("The same policy name: %s was found in "
+ "multiple policies files for service %s. "
+ "This can lead to policy rule ambiguity. "
+ "Using rule: %s; Rule from file: %s")
+ LOG.warning(msg, k, self.service, rules[k], v)
+ except (ValueError, IOError):
+ LOG.warning("Failed to read policy file '%s' for service %s.",
+ path, self.service)
# Check whether policy actions are defined in code. Nova and Keystone,
# for example, define their default policy actions in code.
mgr = stevedore.named.NamedExtensionManager(
'oslo.policy.policies',
names=[self.service],
- on_load_failure_callback=None,
invoke_on_load=True,
warn_on_missing_entrypoint=False)
if mgr:
policy_generator = {plc.name: plc.obj for plc in mgr}
- if policy_generator and self.service in policy_generator:
+ if self.service in policy_generator:
for rule in policy_generator[self.service]:
- mgr_policy_data[rule.name] = str(rule.check)
+ if rule.name not in rules:
+ rules[rule.name] = rule.check
+ elif str(rule.check) != str(rules[rule.name]):
+ msg = ("The same policy name: %s was found in the "
+ "policies files and in the code for service "
+ "%s. This can lead to policy rule ambiguity. "
+ "Using rule: %s; Rule from code: %s")
+ LOG.warning(msg, rule.name, self.service,
+ rules[rule.name], rule.check)
- # If data from both file and code exist, combine both together.
- if file_policy_data and mgr_policy_data:
- # Add the policy actions from code first.
- for action, rule in mgr_policy_data.items():
- policy_data[action] = rule
- # Overwrite with any custom policy actions defined in policy.json.
- for action, rule in file_policy_data.items():
- policy_data[action] = rule
- elif file_policy_data:
- policy_data = file_policy_data
- elif mgr_policy_data:
- policy_data = mgr_policy_data
- else:
- error_message = (
+ if not rules:
+ msg = (
'Policy files for {0} service were not found among the '
'registered in-code policies or in any of the possible policy '
- 'files: {1}.'.format(self.service,
- [loc % self.service for loc in
- CONF.patrole.custom_policy_files])
- )
- raise rbac_exceptions.RbacParsingException(error_message)
+ 'files: {1}.'.format(
+ self.service,
+ [loc % self.service
+ for loc in CONF.patrole.custom_policy_files]))
+ raise rbac_exceptions.RbacParsingException(msg)
- try:
- policy_data = json.dumps(policy_data)
- except (TypeError, ValueError):
- error_message = 'Policy files for {0} service are invalid.'.format(
- self.service)
- raise rbac_exceptions.RbacParsingException(error_message)
-
- return policy_data
+ return rules
def _is_admin_context(self, role):
"""Checks whether a role has admin context.
diff --git a/patrole_tempest_plugin/tests/unit/resources/custom_rbac_policy.yaml b/patrole_tempest_plugin/tests/unit/resources/custom_rbac_policy.yaml
new file mode 100644
index 0000000..444bd2e
--- /dev/null
+++ b/patrole_tempest_plugin/tests/unit/resources/custom_rbac_policy.yaml
@@ -0,0 +1,13 @@
+---
+even_rule: role:two or role:four or role:six or role:eight
+odd_rule: role:one or role:three or role:five or role:seven or role:nine
+zero_rule: role:zero
+prime_rule: role:one or role:two or role:three or role:five or role:seven
+all_rule: ''
+
+policy_action_1: rule:even_rule
+policy_action_2: rule:odd_rule
+policy_action_3: rule:zero_rule
+policy_action_4: rule:prime_rule
+policy_action_5: rule:all_rule
+policy_action_6: role:eight
diff --git a/patrole_tempest_plugin/tests/unit/test_policy_authority.py b/patrole_tempest_plugin/tests/unit/test_policy_authority.py
index b2af1c6..624c0c5 100644
--- a/patrole_tempest_plugin/tests/unit/test_policy_authority.py
+++ b/patrole_tempest_plugin/tests/unit/test_policy_authority.py
@@ -13,7 +13,6 @@
# License for the specific language governing permissions and limitations
# under the License.
-import json
import mock
import os
@@ -61,11 +60,14 @@
self.tenant_policy_file = os.path.join(current_directory,
'resources',
'tenant_rbac_policy.json')
- self.conf_policy_path = os.path.join(
+ self.conf_policy_path_json = os.path.join(
current_directory, 'resources', '%s.json')
+ self.conf_policy_path_yaml = os.path.join(
+ current_directory, 'resources', '%s.yaml')
+
self.useFixture(fixtures.ConfPatcher(
- custom_policy_files=[self.conf_policy_path], group='patrole'))
+ custom_policy_files=[self.conf_policy_path_json], group='patrole'))
self.useFixture(fixtures.ConfPatcher(
api_v3=True, api_v2=False, group='identity-feature-enabled'))
@@ -74,13 +76,18 @@
if attr in dir(policy_authority.PolicyAuthority):
delattr(policy_authority.PolicyAuthority, attr)
- def _get_fake_policy_rule(self, name, rule):
- fake_rule = mock.Mock(check=rule, __name__='foo')
- fake_rule.name = name
- return fake_rule
+ @staticmethod
+ def _get_fake_policies(rules):
+ fake_rules = []
+ rules = policy_authority.policy.Rules.from_dict(rules)
+ for name, check in rules.items():
+ fake_rule = mock.Mock(check=check, __name__='foo')
+ fake_rule.name = name
+ fake_rules.append(fake_rule)
+ return fake_rules
@mock.patch.object(policy_authority, 'LOG', autospec=True)
- def test_custom_policy(self, m_log):
+ def _test_custom_policy(self, *args):
default_roles = ['zero', 'one', 'two', 'three', 'four',
'five', 'six', 'seven', 'eight', 'nine']
@@ -105,6 +112,16 @@
for role in set(default_roles) - set(role_list):
self.assertFalse(authority.allowed(rule, role))
+ def test_custom_policy_json(self):
+ # The CONF.patrole.custom_policy_files has a path to JSON file by
+ # default, so we don't need to use ConfPatcher here.
+ self._test_custom_policy()
+
+ def test_custom_policy_yaml(self):
+ self.useFixture(fixtures.ConfPatcher(
+ custom_policy_files=[self.conf_policy_path_yaml], group='patrole'))
+ self._test_custom_policy()
+
def test_admin_policy_file_with_admin_role(self):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
@@ -303,15 +320,12 @@
m_log.debug.assert_called_once_with(expected_message)
@mock.patch.object(policy_authority, 'stevedore', autospec=True)
- def test_get_policy_data_from_file_and_from_code(self, mock_stevedore):
- fake_policy_rules = [
- self._get_fake_policy_rule('code_policy_action_1',
- 'rule:code_rule_1'),
- self._get_fake_policy_rule('code_policy_action_2',
- 'rule:code_rule_2'),
- self._get_fake_policy_rule('code_policy_action_3',
- 'rule:code_rule_3'),
- ]
+ def test_get_rules_from_file_and_from_code(self, mock_stevedore):
+ fake_policy_rules = self._get_fake_policies({
+ 'code_policy_action_1': 'rule:code_rule_1',
+ 'code_policy_action_2': 'rule:code_rule_2',
+ 'code_policy_action_3': 'rule:code_rule_3',
+ })
mock_manager = mock.Mock(obj=fake_policy_rules, __name__='foo')
mock_manager.configure_mock(name='tenant_rbac_policy')
@@ -324,10 +338,10 @@
authority = policy_authority.PolicyAuthority(
test_tenant_id, test_user_id, "tenant_rbac_policy")
- policy_data = authority._get_policy_data()
- self.assertIsInstance(policy_data, str)
+ rules = authority.get_rules()
+ self.assertIsInstance(rules, policy_authority.policy.Rules)
- actual_policy_data = json.loads(policy_data)
+ actual_policy_data = {k: str(v) for k, v in rules.items()}
expected_policy_data = {
"code_policy_action_1": "rule:code_rule_1",
"code_policy_action_2": "rule:code_rule_2",
@@ -336,23 +350,22 @@
"rule2": "tenant_id:%(tenant_id)s",
"rule3": "project_id:%(project_id)s",
"rule4": "user_id:%(user_id)s",
- "admin_tenant_rule": "role:admin and tenant_id:%(tenant_id)s",
- "admin_user_rule": "role:admin and user_id:%(user_id)s"
+ "admin_tenant_rule": "(role:admin and tenant_id:%(tenant_id)s)",
+ "admin_user_rule": "(role:admin and user_id:%(user_id)s)"
}
self.assertEqual(expected_policy_data, actual_policy_data)
@mock.patch.object(policy_authority, 'stevedore', autospec=True)
- def test_get_policy_data_from_file_and_from_code_with_overwrite(
+ def test_get_rules_from_file_and_from_code_with_overwrite(
self, mock_stevedore):
# The custom policy file should overwrite default rules rule1 and rule2
# that are defined in code.
- fake_policy_rules = [
- self._get_fake_policy_rule('rule1', 'rule:code_rule_1'),
- self._get_fake_policy_rule('rule2', 'rule:code_rule_2'),
- self._get_fake_policy_rule('code_policy_action_3',
- 'rule:code_rule_3'),
- ]
+ fake_policy_rules = self._get_fake_policies({
+ 'rule1': 'rule:code_rule_1',
+ 'rule2': 'rule:code_rule_2',
+ 'code_policy_action_3': 'rule:code_rule_3',
+ })
mock_manager = mock.Mock(obj=fake_policy_rules, __name__='foo')
mock_manager.configure_mock(name='tenant_rbac_policy')
@@ -365,24 +378,24 @@
authority = policy_authority.PolicyAuthority(
test_tenant_id, test_user_id, 'tenant_rbac_policy')
- policy_data = authority._get_policy_data()
- self.assertIsInstance(policy_data, str)
+ rules = authority.get_rules()
+ self.assertIsInstance(rules, policy_authority.policy.Rules)
- actual_policy_data = json.loads(policy_data)
+ actual_policy_data = {k: str(v) for k, v in rules.items()}
expected_policy_data = {
"code_policy_action_3": "rule:code_rule_3",
"rule1": "tenant_id:%(network:tenant_id)s",
"rule2": "tenant_id:%(tenant_id)s",
"rule3": "project_id:%(project_id)s",
"rule4": "user_id:%(user_id)s",
- "admin_tenant_rule": "role:admin and tenant_id:%(tenant_id)s",
- "admin_user_rule": "role:admin and user_id:%(user_id)s"
+ "admin_tenant_rule": "(role:admin and tenant_id:%(tenant_id)s)",
+ "admin_user_rule": "(role:admin and user_id:%(user_id)s)"
}
self.assertEqual(expected_policy_data, actual_policy_data)
@mock.patch.object(policy_authority, 'stevedore', autospec=True)
- def test_get_policy_data_cannot_find_policy(self, mock_stevedore):
+ def test_get_rules_cannot_find_policy(self, mock_stevedore):
mock_stevedore.named.NamedExtensionManager.return_value = None
e = self.assertRaises(rbac_exceptions.RbacParsingException,
policy_authority.PolicyAuthority,
@@ -395,51 +408,21 @@
[CONF.patrole.custom_policy_files[0] % 'test_service']))
self.assertIn(expected_error, str(e))
- @mock.patch.object(policy_authority, 'json', autospec=True)
+ @mock.patch.object(policy_authority.policy, 'parse_file_contents',
+ autospec=True)
@mock.patch.object(policy_authority, 'stevedore', autospec=True)
- def test_get_policy_data_without_valid_policy(self, mock_stevedore,
- mock_json):
- test_policy_action = mock.Mock(check='rule:bar', __name__='foo')
- test_policy_action.configure_mock(name='foo')
-
- test_policy = mock.Mock(obj=[test_policy_action], __name__='foo')
- test_policy.configure_mock(name='test_service')
-
- mock_stevedore.named.NamedExtensionManager\
- .return_value = [test_policy]
-
- mock_json.dumps.side_effect = ValueError
-
- e = self.assertRaises(rbac_exceptions.RbacParsingException,
- policy_authority.PolicyAuthority,
- None, None, 'test_service')
-
- expected_error = "Policy files for {0} service are invalid."\
- .format("test_service")
- self.assertIn(expected_error, str(e))
-
- mock_stevedore.named.NamedExtensionManager.assert_called_once_with(
- 'oslo.policy.policies',
- names=['test_service'],
- on_load_failure_callback=None,
- invoke_on_load=True,
- warn_on_missing_entrypoint=False)
-
- @mock.patch.object(policy_authority, 'json', autospec=True)
- @mock.patch.object(policy_authority, 'stevedore', autospec=True)
- def test_get_policy_data_from_file_not_json(self, mock_stevedore,
- mock_json):
+ def test_get_rules_without_valid_policy(self, mock_stevedore,
+ mock_parse_file_contents):
mock_stevedore.named.NamedExtensionManager.return_value = None
- mock_json.loads.side_effect = ValueError
+ mock_parse_file_contents.side_effect = ValueError
e = self.assertRaises(rbac_exceptions.RbacParsingException,
policy_authority.PolicyAuthority,
None, None, 'tenant_rbac_policy')
expected_error = (
'Policy files for {0} service were not found among the registered '
- 'in-code policies or in any of the possible policy files: {1}.'
- .format('tenant_rbac_policy', [CONF.patrole.custom_policy_files[0]
- % 'tenant_rbac_policy']))
+ 'in-code policies or in any of the possible policy files:'
+ .format('tenant_rbac_policy'))
self.assertIn(expected_error, str(e))
def test_discover_policy_files(self):
@@ -451,11 +434,11 @@
dir(policy_authority.PolicyAuthority))
self.assertIn('policy_files', dir(policy_parser))
self.assertIn('tenant_rbac_policy', policy_parser.policy_files)
- self.assertEqual([self.conf_policy_path % 'tenant_rbac_policy'],
+ self.assertEqual([self.conf_policy_path_json % 'tenant_rbac_policy'],
policy_parser.policy_files['tenant_rbac_policy'])
@mock.patch.object(policy_authority, 'policy', autospec=True)
- @mock.patch.object(policy_authority.PolicyAuthority, '_get_policy_data',
+ @mock.patch.object(policy_authority.PolicyAuthority, 'get_rules',
autospec=True)
@mock.patch.object(policy_authority, 'clients', autospec=True)
@mock.patch.object(policy_authority, 'os', autospec=True)
@@ -494,7 +477,8 @@
expected_error = (
'Policy files for {0} service were not found among the registered '
'in-code policies or in any of the possible policy files: {1}.'
- .format('test_service', [self.conf_policy_path % 'test_service']))
+ .format('test_service',
+ [self.conf_policy_path_json % 'test_service']))
e = self.assertRaises(rbac_exceptions.RbacParsingException,
policy_authority.PolicyAuthority,
diff --git a/releasenotes/notes/yaml-policy-file-support-278d3edf64f98d69.yaml b/releasenotes/notes/yaml-policy-file-support-278d3edf64f98d69.yaml
new file mode 100644
index 0000000..e333377
--- /dev/null
+++ b/releasenotes/notes/yaml-policy-file-support-278d3edf64f98d69.yaml
@@ -0,0 +1,7 @@
+---
+features:
+- |
+ Patrole now supports parsing custom YAML policy files, the new policy file
+ extension since Ocata. The function ``_get_policy_data`` has been renamed to
+ ``get_rules`` and been changed to re-use ``oslo_policy.policy.Rules.load``
+ function.