Throw skipException for invalid policy actions.
Currently, if an invalid policy action is passed to the
rbac_rule_validation.action decorator, then
rbac_policy_parser.RbacPolicyParser.allowed => false to
rbac_auth.RbacAuthority.get_permission => false to allowed in
rbac_rule_validation. At this point, if the test passes, then
an OverPermission error is thrown, which is nonsensical. If
the test fails, then the test will silently pass. This is very
bad.
Instead, the Patrole framework should be changed to throw a
skipException if the policy action that is passed to the
decorator is invalid, with a detailed error message as to
what happened. The reason why a skipException should be
thrown is because of backwards compatibility: since policy
actions change all the time, they are not
backwards-compatible. Failing the test in a previous version
of, say, Nova, will make adopting Patrole for testing in
earlier OS releases more challenging, because failures might
occur everywhere, since the policy actions probably won't exist
in either the default policy file or a custom policy file.
Also, expecting a failure is not appropriate either, since
there is no perfect correlation between the policy action
being passed in and the logic inside the test. (For example,
it is possible to set the policy action to "foo", then call
"nova list" -- the test will probably pass, causing the
expected failure to fail.)
This patch throws a skipException if an invalid policy action
is passed to the rbac_rule_validation.action decorator.
Change-Id: I7ef87417e1bb05450e9e750bc605aa34d985c835
Implements: blueprint skip-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)