Merge "Throw skipException for invalid policy actions."
diff --git a/patrole_tempest_plugin/rbac_auth.py b/patrole_tempest_plugin/rbac_auth.py
index e4e35b1..687c0a8 100644
--- a/patrole_tempest_plugin/rbac_auth.py
+++ b/patrole_tempest_plugin/rbac_auth.py
@@ -13,8 +13,11 @@
# License for the specific language governing permissions and limitations
# under the License.
+import testtools
+
from oslo_log import log as logging
+from patrole_tempest_plugin import rbac_exceptions
from patrole_tempest_plugin import rbac_policy_parser
LOG = logging.getLogger(__name__)
@@ -22,20 +25,19 @@
class RbacAuthority(object):
def __init__(self, tenant_id, user_id, service=None):
- self.converter = rbac_policy_parser.RbacPolicyParser(
+ self.policy_parser = rbac_policy_parser.RbacPolicyParser(
tenant_id, user_id, service)
def get_permission(self, rule_name, role):
try:
- is_allowed = self.converter.allowed(rule_name, role)
+ is_allowed = self.policy_parser.allowed(rule_name, role)
if is_allowed:
- LOG.debug("[API]: %s, [Role]: %s is allowed!", rule_name, role)
+ LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule_name,
+ role)
else:
- LOG.debug("[API]: %s, [Role]: %s is NOT allowed!",
+ LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
rule_name, role)
return is_allowed
- except KeyError:
- LOG.debug("[API]: %s, [Role]: %s is NOT allowed!",
- rule_name, role)
- return False
+ except rbac_exceptions.RbacParsingException as e:
+ raise testtools.TestCase.skipException(str(e))
return False
diff --git a/patrole_tempest_plugin/rbac_exceptions.py b/patrole_tempest_plugin/rbac_exceptions.py
index ee42e19..967342b 100644
--- a/patrole_tempest_plugin/rbac_exceptions.py
+++ b/patrole_tempest_plugin/rbac_exceptions.py
@@ -30,3 +30,7 @@
class RbacInvalidService (exceptions.TempestException):
message = "Attempted to test an invalid service"
+
+
+class RbacParsingException (exceptions.TempestException):
+ message = "Attempted to test an invalid policy file or action"
diff --git a/patrole_tempest_plugin/rbac_policy_parser.py b/patrole_tempest_plugin/rbac_policy_parser.py
index 9926613..62113ac 100644
--- a/patrole_tempest_plugin/rbac_policy_parser.py
+++ b/patrole_tempest_plugin/rbac_policy_parser.py
@@ -90,7 +90,7 @@
policy_data = policy_data[:-2] + "\n}"
# Otherwise raise an exception.
else:
- raise rbac_exceptions.RbacResourceSetupFailed(
+ raise rbac_exceptions.RbacParsingException(
'Policy file for service: {0}, {1} not found.'
.format(service, self.path))
@@ -172,8 +172,12 @@
rule = self.rules[apply_rule]
return rule(target, access_data, o)
except KeyError as e:
- LOG.debug("{0} not found in policy file.".format(apply_rule))
- return False
+ message = "Policy action: {0} not found in policy file: {1}."\
+ .format(apply_rule, self.path)
+ LOG.debug(message)
+ raise rbac_exceptions.RbacParsingException(message)
except Exception as e:
- LOG.debug("Exception: {0} for rule: {1}.".format(e, apply_rule))
- return False
+ message = "Unknown exception: {0} for policy action: {1} in "\
+ "policy file: {2}.".format(e, apply_rule, self.path)
+ LOG.debug(message)
+ raise rbac_exceptions.RbacParsingException(message)
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 463adce..a6490e7 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -47,11 +47,11 @@
try:
func(*args)
except rbac_exceptions.RbacInvalidService as e:
- msg = ("%s is not a valid service." % service)
- LOG.error(msg)
- raise exceptions.NotFound(
- "%s RbacInvalidService was: %s" %
- (msg, e))
+ msg = ("%s is not a valid service." % service)
+ LOG.error(msg)
+ raise exceptions.NotFound(
+ "%s RbacInvalidService was: %s" %
+ (msg, e))
except exceptions.Forbidden as e:
if allowed:
msg = ("Role %s was not allowed to perform %s." %
diff --git a/patrole_tempest_plugin/tests/unit/test_rbac_policy_parser.py b/patrole_tempest_plugin/tests/unit/test_rbac_policy_parser.py
index edc63b8..8583eb5 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_policy_parser.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_policy_parser.py
@@ -82,7 +82,7 @@
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
- converter = rbac_policy_parser.RbacPolicyParser(
+ parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test", self.custom_policy_file)
expected = {
@@ -95,24 +95,16 @@
'policy_action_6': ['eight'],
}
- fake_rule = 'fake_rule'
-
- for role in default_roles:
- self.assertFalse(converter.allowed(fake_rule, role))
- m_log.debug.assert_called_once_with(
- "{0} not found in policy file.".format('fake_rule'))
- m_log.debug.reset_mock()
-
for rule, role_list in expected.items():
for role in role_list:
- self.assertTrue(converter.allowed(rule, role))
+ self.assertTrue(parser.allowed(rule, role))
for role in set(default_roles) - set(role_list):
- self.assertFalse(converter.allowed(rule, role))
+ self.assertFalse(parser.allowed(rule, role))
def test_admin_policy_file_with_admin_role(self):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
- converter = rbac_policy_parser.RbacPolicyParser(
+ parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test", self.admin_policy_file)
role = 'admin'
@@ -122,17 +114,17 @@
disallowed_rules = ['non_admin_rule']
for rule in allowed_rules:
- allowed = converter.allowed(rule, role)
+ allowed = parser.allowed(rule, role)
self.assertTrue(allowed)
for rule in disallowed_rules:
- allowed = converter.allowed(rule, role)
+ allowed = parser.allowed(rule, role)
self.assertFalse(allowed)
def test_admin_policy_file_with_member_role(self):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
- converter = rbac_policy_parser.RbacPolicyParser(
+ parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test", self.admin_policy_file)
role = 'Member'
@@ -143,17 +135,17 @@
'admin_rule', 'is_admin_rule', 'alt_admin_rule']
for rule in allowed_rules:
- allowed = converter.allowed(rule, role)
+ allowed = parser.allowed(rule, role)
self.assertTrue(allowed)
for rule in disallowed_rules:
- allowed = converter.allowed(rule, role)
+ allowed = parser.allowed(rule, role)
self.assertFalse(allowed)
def test_admin_policy_file_with_context_is_admin(self):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
- converter = rbac_policy_parser.RbacPolicyParser(
+ parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test", self.alt_admin_policy_file)
role = 'fake_admin'
@@ -161,11 +153,11 @@
disallowed_rules = ['admin_rule']
for rule in allowed_rules:
- allowed = converter.allowed(rule, role)
+ allowed = parser.allowed(rule, role)
self.assertTrue(allowed)
for rule in disallowed_rules:
- allowed = converter.allowed(rule, role)
+ allowed = parser.allowed(rule, role)
self.assertFalse(allowed)
role = 'super_admin'
@@ -173,11 +165,11 @@
disallowed_rules = ['non_admin_rule']
for rule in allowed_rules:
- allowed = converter.allowed(rule, role)
+ allowed = parser.allowed(rule, role)
self.assertTrue(allowed)
for rule in disallowed_rules:
- allowed = converter.allowed(rule, role)
+ allowed = parser.allowed(rule, role)
self.assertFalse(allowed)
def test_tenant_user_policy(self):
@@ -189,28 +181,28 @@
"""
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
- converter = rbac_policy_parser.RbacPolicyParser(
+ parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test", self.tenant_policy_file)
# Check whether Member role can perform expected actions.
allowed_rules = ['rule1', 'rule2', 'rule3', 'rule4']
for rule in allowed_rules:
- allowed = converter.allowed(rule, 'Member')
+ allowed = parser.allowed(rule, 'Member')
self.assertTrue(allowed)
disallowed_rules = ['admin_tenant_rule', 'admin_user_rule']
for disallowed_rule in disallowed_rules:
- self.assertFalse(converter.allowed(disallowed_rule, 'Member'))
+ self.assertFalse(parser.allowed(disallowed_rule, 'Member'))
# Check whether admin role can perform expected actions.
allowed_rules.extend(disallowed_rules)
for rule in allowed_rules:
- allowed = converter.allowed(rule, 'admin')
+ allowed = parser.allowed(rule, 'admin')
self.assertTrue(allowed)
# Check whether _try_rule is called with the correct target dictionary.
with mock.patch.object(
- converter, '_try_rule', return_value=True, autospec=True) \
+ parser, '_try_rule', return_value=True, autospec=True) \
as mock_try_rule:
expected_target = {
@@ -230,7 +222,7 @@
}
for rule in allowed_rules:
- allowed = converter.allowed(rule, 'Member')
+ allowed = parser.allowed(rule, 'Member')
self.assertTrue(allowed)
mock_try_rule.assert_called_once_with(
rule, expected_target, expected_access_data, mock.ANY)
@@ -265,3 +257,41 @@
m_log.debug.assert_called_once_with(
"{0} is NOT a valid service.".format(str(service)))
+
+ @mock.patch.object(rbac_policy_parser, 'LOG', autospec=True)
+ def test_invalid_policy_rule_throws_rbac_parsing_exception(self, m_log):
+ test_tenant_id = mock.sentinel.tenant_id
+ test_user_id = mock.sentinel.user_id
+
+ parser = rbac_policy_parser.RbacPolicyParser(
+ test_tenant_id, test_user_id, "test", self.custom_policy_file)
+
+ fake_rule = 'fake_rule'
+ expected_message = "Policy action: {0} not found in policy file: {1}."\
+ .format(fake_rule, self.custom_policy_file)
+
+ e = self.assertRaises(rbac_exceptions.RbacParsingException,
+ parser.allowed, fake_rule, None)
+ self.assertIn(expected_message, str(e))
+ m_log.debug.assert_called_once_with(expected_message)
+
+ @mock.patch.object(rbac_policy_parser, 'LOG', autospec=True)
+ def test_unknown_exception_throws_rbac_parsing_exception(self, m_log):
+ test_tenant_id = mock.sentinel.tenant_id
+ test_user_id = mock.sentinel.user_id
+
+ parser = rbac_policy_parser.RbacPolicyParser(
+ test_tenant_id, test_user_id, "test", self.custom_policy_file)
+ parser.rules = mock.MagicMock(
+ **{'__getitem__.return_value.side_effect': Exception(
+ mock.sentinel.error)})
+
+ expected_message = "Unknown exception: {0} for policy action: {1} in "\
+ "policy file: {2}.".format(mock.sentinel.error,
+ mock.sentinel.rule,
+ self.custom_policy_file)
+
+ e = self.assertRaises(rbac_exceptions.RbacParsingException,
+ parser.allowed, mock.sentinel.rule, None)
+ self.assertIn(expected_message, str(e))
+ m_log.debug.assert_called_once_with(expected_message)
diff --git a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
index 1e78a7d..eeb06cc 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -13,7 +13,9 @@
# under the License.
import mock
+import testtools
+from patrole_tempest_plugin import rbac_auth
from patrole_tempest_plugin import rbac_exceptions
from patrole_tempest_plugin import rbac_rule_validation as rbac_rv
@@ -29,7 +31,10 @@
self.mock_args = mock.Mock(spec=test.BaseTestCase)
self.mock_args.auth_provider = mock.Mock()
self.mock_args.rbac_utils = mock.Mock()
- self.mock_args.auth_provider.credentials.tenant_id = 'tenant_id'
+ self.mock_args.auth_provider.credentials.tenant_id = \
+ mock.sentinel.tenant_id
+ self.mock_args.auth_provider.credentials.user_id = \
+ mock.sentinel.user_id
@mock.patch('patrole_tempest_plugin.rbac_auth.RbacAuthority')
def test_RBAC_rv_happy_path(self, mock_auth):
@@ -98,3 +103,22 @@
mock_auth.return_value = mock_permission
self.assertIsNone(wrapper(self.mock_args))
+
+ @mock.patch.object(rbac_auth, 'rbac_policy_parser', autospec=True)
+ def test_invalid_policy_rule_throws_skip_exception(
+ self, mock_rbac_policy_parser):
+ mock_rbac_policy_parser.RbacPolicyParser.return_value.allowed.\
+ side_effect = rbac_exceptions.RbacParsingException
+
+ decorator = rbac_rv.action(mock.sentinel.service,
+ mock.sentinel.policy_rule)
+ wrapper = decorator(mock.Mock())
+
+ e = self.assertRaises(testtools.TestCase.skipException, wrapper,
+ self.mock_args)
+ self.assertEqual('Attempted to test an invalid policy file or action',
+ str(e))
+
+ mock_rbac_policy_parser.RbacPolicyParser.assert_called_once_with(
+ mock.sentinel.tenant_id, mock.sentinel.user_id,
+ mock.sentinel.service)