Fixes converter not working for certain edge cases.
Currently, the converter framework is not robust enough to handle
all policy cases. For example, is_admin context breaks.
This patch makes the converter more robust. The converter was changed
to use oslo_policy's shell tool for figuring out which roles are
permitted for a given rule. The shell tool can be found here:
https://github.com/openstack/oslo.policy/blob/master/oslo_policy/shell.py
Because the shell tool is intended to be used as a CLI tool, it was
adapted from oslo policy to better work within Patrole.
implements blueprint: oslo-policy-converter
Change-Id: Ia0fe9113e2be44e609b0edbb4c6facd1425f28b5
diff --git a/tests/resources/admin_rbac_policy.json b/tests/resources/admin_rbac_policy.json
new file mode 100644
index 0000000..7828921
--- /dev/null
+++ b/tests/resources/admin_rbac_policy.json
@@ -0,0 +1,6 @@
+{
+ "admin_rule": "role:admin",
+ "is_admin_rule": "is_admin:True",
+ "alt_admin_rule": "is_admin:True or (role:admin and is_project_admin:True)",
+ "non_admin_rule": "role:Member"
+}
diff --git a/tests/custom_rbac_policy.json b/tests/resources/custom_rbac_policy.json
similarity index 100%
rename from tests/custom_rbac_policy.json
rename to tests/resources/custom_rbac_policy.json
diff --git a/tests/test_rbac_role_converter.py b/tests/test_rbac_role_converter.py
index 942d7d0..dadab88 100644
--- a/tests/test_rbac_role_converter.py
+++ b/tests/test_rbac_role_converter.py
@@ -29,7 +29,11 @@
current_directory = os.path.dirname(os.path.realpath(__file__))
self.custom_policy_file = os.path.join(current_directory,
+ 'resources',
'custom_rbac_policy.json')
+ self.admin_policy_file = os.path.join(current_directory,
+ 'resources',
+ 'admin_rbac_policy.json')
def test_custom_policy(self):
default_roles = ['zero', 'one', 'two', 'three', 'four',
@@ -37,11 +41,8 @@
CONF.set_override('rbac_roles', default_roles, group='rbac',
enforce_type=True)
- self.converter = rbac_role_converter.RbacPolicyConverter(
- "custom",
- self.custom_policy_file
- )
- self.roles_dict = self.converter.rules
+ converter = rbac_role_converter.RbacPolicyConverter(
+ None, "test", self.custom_policy_file)
expected = {
'policy_action_1': ['two', 'four', 'six', 'eight'],
@@ -55,13 +56,57 @@
fake_rule = 'fake_rule'
- self.assertFalse(fake_rule in self.roles_dict.keys())
+ for role in default_roles:
+ self.assertRaises(KeyError, converter.allowed, fake_rule, role)
- for rule in expected.keys():
- self.assertTrue(rule in self.roles_dict.keys())
- expected_roles = expected[rule]
- unexpected_roles = set(default_roles) - set(expected[rule])
- for role in expected_roles:
- self.assertTrue(role in self.roles_dict[rule])
- for role in unexpected_roles:
- self.assertFalse(role in self.roles_dict[rule])
+ for rule, role_list in expected.items():
+ for role in role_list:
+ self.assertTrue(converter.allowed(rule, role))
+ for role in set(default_roles) - set(role_list):
+ self.assertFalse(converter.allowed(rule, role))
+
+ def test_admin_policy_file_with_admin_role(self):
+ default_roles = ['admin', 'Member']
+ CONF.set_override('rbac_roles', default_roles, group='rbac',
+ enforce_type=True)
+
+ converter = rbac_role_converter.RbacPolicyConverter(
+ None, "test", self.admin_policy_file)
+
+ role = 'admin'
+ allowed_rules = [
+ 'admin_rule'
+ ]
+ disallowed_rules = [
+ 'is_admin_rule', 'alt_admin_rule', 'non_admin_rule']
+
+ for rule in allowed_rules:
+ allowed = converter.allowed(rule, role)
+ self.assertTrue(allowed)
+
+ for rule in disallowed_rules:
+ allowed = converter.allowed(rule, role)
+ self.assertFalse(allowed)
+
+ def test_admin_policy_file_with_member_role(self):
+ default_roles = ['admin', 'Member']
+ CONF.set_override('rbac_roles', default_roles, group='rbac',
+ enforce_type=True)
+
+ converter = rbac_role_converter.RbacPolicyConverter(
+ None, "test", self.admin_policy_file)
+
+ role = 'Member'
+ allowed_rules = [
+ 'non_admin_rule'
+ ]
+ disallowed_rules = [
+ 'admin_rule', 'is_admin_rule', 'alt_admin_rule']
+
+ for rule in allowed_rules:
+ allowed = converter.allowed(rule, role)
+ self.assertTrue(allowed)
+
+ for rule in disallowed_rules:
+ allowed = converter.allowed(rule, role)
+ self.assertFalse(allowed)
diff --git a/tests/test_rbac_rule_validation.py b/tests/test_rbac_rule_validation.py
index a8e2be3..edc442e 100644
--- a/tests/test_rbac_rule_validation.py
+++ b/tests/test_rbac_rule_validation.py
@@ -25,31 +25,42 @@
class RBACRuleValidationTest(base.TestCase):
@mock.patch('patrole_tempest_plugin.rbac_auth.RbacAuthority')
def test_RBAC_rv_happy_path(self, mock_auth):
- decorator = rbac_rv.action("", "", "")
+ decorator = rbac_rv.action("", "")
mock_function = mock.Mock()
+ mock_args = mock.MagicMock(**{
+ 'auth_provider.credentials.tenant_id': 'tenant_id'
+ })
wrapper = decorator(mock_function)
- wrapper()
+ wrapper((mock_args))
self.assertTrue(mock_function.called)
@mock.patch('patrole_tempest_plugin.rbac_auth.RbacAuthority')
def test_RBAC_rv_forbidden(self, mock_auth):
- decorator = rbac_rv.action("", "", "")
+ decorator = rbac_rv.action("", "")
mock_function = mock.Mock()
mock_function.side_effect = exceptions.Forbidden
wrapper = decorator(mock_function)
- self.assertRaises(exceptions.Forbidden, wrapper)
+ mock_args = mock.MagicMock(**{
+ 'auth_provider.credentials.tenant_id': 'tenant_id'
+ })
+
+ self.assertRaises(exceptions.Forbidden, wrapper, mock_args)
@mock.patch('patrole_tempest_plugin.rbac_auth.RbacAuthority')
def test_RBAC_rv_rbac_action_failed(self, mock_auth):
- decorator = rbac_rv.action("", "", "")
+ decorator = rbac_rv.action("", "")
mock_function = mock.Mock()
mock_function.side_effect = rbac_exceptions.RbacActionFailed
+ mock_args = mock.MagicMock(**{
+ 'auth_provider.credentials.tenant_id': 'tenant_id'
+ })
+
wrapper = decorator(mock_function)
- self.assertRaises(exceptions.Forbidden, wrapper)
+ self.assertRaises(exceptions.Forbidden, wrapper, mock_args)
@mock.patch('patrole_tempest_plugin.rbac_auth.RbacAuthority')
def test_RBAC_rv_not_allowed(self, mock_auth):
- decorator = rbac_rv.action("", "", "")
+ decorator = rbac_rv.action("", "")
mock_function = mock.Mock()
wrapper = decorator(mock_function)
@@ -58,25 +69,33 @@
mock_permission.get_permission.return_value = False
mock_auth.return_value = mock_permission
- self.assertRaises(rbac_exceptions.RbacOverPermission, wrapper)
+ mock_args = mock.MagicMock(**{
+ 'auth_provider.credentials.tenant_id': 'tenant_id'
+ })
+
+ self.assertRaises(rbac_exceptions.RbacOverPermission, wrapper,
+ mock_args)
@mock.patch('patrole_tempest_plugin.rbac_auth.RbacAuthority')
def test_RBAC_rv_forbidden_not_allowed(self, mock_auth):
- decorator = rbac_rv.action("", "", "")
+ decorator = rbac_rv.action("", "")
mock_function = mock.Mock()
mock_function.side_effect = exceptions.Forbidden
+ mock_args = mock.MagicMock(**{
+ 'auth_provider.credentials.tenant_id': 'tenant_id'
+ })
wrapper = decorator(mock_function)
mock_permission = mock.Mock()
mock_permission.get_permission.return_value = False
mock_auth.return_value = mock_permission
- self.assertIsNone(wrapper())
+ self.assertIsNone(wrapper(mock_args))
@mock.patch('patrole_tempest_plugin.rbac_auth.RbacAuthority')
def test_RBAC_rv_rbac_action_failed_not_allowed(self, mock_auth):
- decorator = rbac_rv.action("", "", "")
+ decorator = rbac_rv.action("", "")
mock_function = mock.Mock()
mock_function.side_effect = rbac_exceptions.RbacActionFailed
@@ -86,4 +105,8 @@
mock_permission.get_permission.return_value = False
mock_auth.return_value = mock_permission
- self.assertIsNone(wrapper())
+ mock_args = mock.MagicMock(**{
+ 'auth_provider.credentials.tenant_id': 'tenant_id'
+ })
+
+ self.assertIsNone(wrapper(mock_args))