Merge rbac_auth with rbac_rule_validation
Currently, rbac_auth doesn't do much: It decentralizes logic
that can be easily merged into rbac_rule_validation without
doing anything authentication-related. All rbac_auth does is:
1) Construct RbacPolicyParser and check whether a given role
is allowed to perform a given policy action.
2) Dump some info to LOG
3) Catch some exceptions
Thus, there's no justification for keeping rbac_auth. It doesn't
provide a high-enough level of abstraction to warrant being used.
It should be removed and its logic inserted in rbac_rule_validation.
Change-Id: I756175ea28ec11f24150f46d5ae4c2f64499a0ea
Closes-Bug: #1681459
diff --git a/patrole_tempest_plugin/rbac_auth.py b/patrole_tempest_plugin/rbac_auth.py
deleted file mode 100644
index ae497f3..0000000
--- a/patrole_tempest_plugin/rbac_auth.py
+++ /dev/null
@@ -1,49 +0,0 @@
-# Copyright 2017 AT&T Corporation.
-# All Rights Reserved.
-#
-# Licensed under the Apache License, Version 2.0 (the "License"); you may
-# not use this file except in compliance with the License. You may obtain
-# a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-# License for the specific language governing permissions and limitations
-# under the License.
-
-import testtools
-
-from oslo_log import log as logging
-
-from tempest import config
-
-from patrole_tempest_plugin import rbac_exceptions
-from patrole_tempest_plugin import rbac_policy_parser
-
-LOG = logging.getLogger(__name__)
-CONF = config.CONF
-
-
-class RbacAuthority(object):
- def __init__(self, project_id, user_id, service, extra_target_data):
- self.policy_parser = rbac_policy_parser.RbacPolicyParser(
- project_id, user_id, service, extra_target_data=extra_target_data)
-
- def get_permission(self, rule_name, role):
- try:
- is_allowed = self.policy_parser.allowed(rule_name, role)
- if is_allowed:
- LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule_name,
- role)
- else:
- LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
- rule_name, role)
- return is_allowed
- except rbac_exceptions.RbacParsingException as e:
- if CONF.rbac.strict_policy_check:
- raise e
- else:
- raise testtools.TestCase.skipException(str(e))
- return False
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 6a5ed5e..5a6bc74 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -15,6 +15,7 @@
import logging
import sys
+import testtools
import six
@@ -22,8 +23,8 @@
from tempest.lib import exceptions
from tempest import test
-from patrole_tempest_plugin import rbac_auth
from patrole_tempest_plugin import rbac_exceptions
+from patrole_tempest_plugin import rbac_policy_parser
CONF = config.CONF
LOG = logging.getLogger(__name__)
@@ -62,18 +63,15 @@
:raises RbacOverPermission: for bullet (3) above.
"""
def decorator(func):
+ role = CONF.rbac.rbac_test_role
+
def wrapper(*args, **kwargs):
- try:
- caller_ref = None
- if args and isinstance(args[0], test.BaseTestCase):
- caller_ref = args[0]
- project_id = caller_ref.auth_provider.credentials.project_id
- user_id = caller_ref.auth_provider.credentials.user_id
- except AttributeError as e:
- msg = ("{0}: project_id/user_id not found in "
- "cls.auth_provider.credentials".format(e))
- LOG.error(msg)
- raise rbac_exceptions.RbacResourceSetupFailed(msg)
+ if args and isinstance(args[0], test.BaseTestCase):
+ test_obj = args[0]
+ else:
+ raise rbac_exceptions.RbacResourceSetupFailed(
+ '`rbac_rule_validation` decorator can only be applied to '
+ 'an instance of `tempest.test.BaseTestCase`.')
if admin_only:
LOG.info("As admin_only is True, only admin role should be "
@@ -81,33 +79,28 @@
"check for policy action {0}.".format(rule))
allowed = CONF.rbac.rbac_test_role == 'admin'
else:
- authority = rbac_auth.RbacAuthority(
- project_id, user_id, service,
- _format_extra_target_data(caller_ref, extra_target_data))
- allowed = authority.get_permission(
- rule, CONF.rbac.rbac_test_role)
+ allowed = _is_authorized(test_obj, service, rule,
+ extra_target_data)
expected_exception, irregular_msg = _get_exception_type(
expected_error_code)
try:
- func(*args)
+ func(*args, **kwargs)
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))
+ "%s RbacInvalidService was: %s" % (msg, e))
except (expected_exception, rbac_exceptions.RbacActionFailed) as e:
if irregular_msg:
LOG.warning(irregular_msg.format(rule, service))
if allowed:
msg = ("Role %s was not allowed to perform %s." %
- (CONF.rbac.rbac_test_role, rule))
+ (role, rule))
LOG.error(msg)
raise exceptions.Forbidden(
- "%s exception was: %s" %
- (msg, e))
+ "%s exception was: %s" % (msg, e))
except Exception as e:
exc_info = sys.exc_info()
error_details = exc_info[1].__str__()
@@ -119,21 +112,58 @@
else:
if not allowed:
LOG.error("Role %s was allowed to perform %s" %
- (CONF.rbac.rbac_test_role, rule))
+ (role, rule))
raise rbac_exceptions.RbacOverPermission(
"OverPermission: Role %s was allowed to perform %s" %
- (CONF.rbac.rbac_test_role, rule))
+ (role, rule))
finally:
- caller_ref.rbac_utils.switch_role(caller_ref,
- toggle_rbac_role=False)
- return wrapper
+ test_obj.rbac_utils.switch_role(test_obj,
+ toggle_rbac_role=False)
+
+ _wrapper = testtools.testcase.attr(role)(wrapper)
+ return _wrapper
return decorator
+def _is_authorized(test_obj, service, rule_name, extra_target_data):
+ try:
+ project_id = test_obj.auth_provider.credentials.project_id
+ user_id = test_obj.auth_provider.credentials.user_id
+ except AttributeError as e:
+ msg = ("{0}: project_id/user_id not found in "
+ "cls.auth_provider.credentials".format(e))
+ LOG.error(msg)
+ raise rbac_exceptions.RbacResourceSetupFailed(msg)
+
+ try:
+ role = CONF.rbac.rbac_test_role
+ formatted_target_data = _format_extra_target_data(
+ test_obj, extra_target_data)
+ policy_parser = rbac_policy_parser.RbacPolicyParser(
+ project_id, user_id, service,
+ extra_target_data=formatted_target_data)
+ is_allowed = policy_parser.allowed(rule_name, role)
+
+ if is_allowed:
+ LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule_name,
+ role)
+ else:
+ LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
+ rule_name, role)
+ return is_allowed
+ except rbac_exceptions.RbacParsingException as e:
+ if CONF.rbac.strict_policy_check:
+ raise e
+ else:
+ raise testtools.TestCase.skipException(str(e))
+ return False
+
+
def _get_exception_type(expected_error_code):
expected_exception = None
irregular_msg = None
supported_error_codes = [403, 404]
+
if expected_error_code == 403:
expected_exception = exceptions.Forbidden
elif expected_error_code == 404:
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 174945e..41af3b2 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -19,7 +19,6 @@
from tempest import test
from tempest.tests import base
-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
@@ -43,8 +42,9 @@
self.addCleanup(CONF.clear_override, 'rbac_test_role', group='rbac')
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_rule_validation_have_permission_no_exc(self, mock_auth, mock_log):
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_rule_validation_have_permission_no_exc(self, mock_policy,
+ mock_log):
"""Test that having permission and no exception thrown is success.
Positive test case success scenario.
@@ -54,9 +54,7 @@
mock_function = mock.Mock()
wrapper = decorator(mock_function)
- mock_permission = mock.Mock()
- mock_permission.get_permission.return_value = True
- mock_auth.return_value = mock_permission
+ mock_policy.RbacPolicyParser.return_value.allowed.return_value = True
result = wrapper(self.mock_args)
@@ -65,8 +63,8 @@
mock_log.error.assert_not_called()
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_rule_validation_lack_permission_throw_exc(self, mock_auth,
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_rule_validation_lack_permission_throw_exc(self, mock_policy,
mock_log):
"""Test that having no permission and exception thrown is success.
@@ -78,9 +76,7 @@
mock_function.side_effect = exceptions.Forbidden
wrapper = decorator(mock_function)
- mock_permission = mock.Mock()
- mock_permission.get_permission.return_value = False
- mock_auth.return_value = mock_permission
+ mock_policy.RbacPolicyParser.return_value.allowed.return_value = False
result = wrapper(self.mock_args)
@@ -89,8 +85,8 @@
mock_log.error.assert_not_called()
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_rule_validation_forbidden_negative(self, mock_auth, mock_log):
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_rule_validation_forbidden_negative(self, mock_policy, mock_log):
"""Test Forbidden error is thrown and have permission fails.
Negative test case: if Forbidden is thrown and the user should be
@@ -102,9 +98,7 @@
mock_function.side_effect = exceptions.Forbidden
wrapper = decorator(mock_function)
- mock_permission = mock.Mock()
- mock_permission.get_permission.return_value = True
- mock_auth.return_value = mock_permission
+ mock_policy.RbacPolicyParser.return_value.allowed.return_value = True
e = self.assertRaises(exceptions.Forbidden, wrapper, self.mock_args)
self.assertIn(
@@ -114,8 +108,8 @@
" perform sentinel.action.")
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_rule_validation_rbac_action_failed_positive(self, mock_auth,
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_rule_validation_rbac_action_failed_positive(self, mock_policy,
mock_log):
"""Test RbacActionFailed error is thrown without permission passes.
@@ -127,9 +121,7 @@
mock_function.side_effect = rbac_exceptions.RbacActionFailed
wrapper = decorator(mock_function)
- mock_permission = mock.Mock()
- mock_permission.get_permission.return_value = False
- mock_auth.return_value = mock_permission
+ mock_policy.RbacPolicyParser.return_value.allowed.return_value = False
result = wrapper(self.mock_args)
@@ -138,8 +130,8 @@
mock_log.warning.assert_not_called()
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_rule_validation_rbac_action_failed_negative(self, mock_auth,
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_rule_validation_rbac_action_failed_negative(self, mock_policy,
mock_log):
"""Test RbacActionFailed error is thrown with permission fails.
@@ -151,9 +143,7 @@
mock_function.side_effect = rbac_exceptions.RbacActionFailed
wrapper = decorator(mock_function)
- mock_permission = mock.Mock()
- mock_permission.get_permission.return_value = True
- mock_auth.return_value = mock_permission
+ mock_policy.RbacPolicyParser.return_value.allowed.return_value = True
e = self.assertRaises(exceptions.Forbidden, wrapper, self.mock_args)
self.assertIn(
@@ -164,8 +154,9 @@
" perform sentinel.action.")
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_expect_not_found_but_raises_forbidden(self, mock_auth, mock_log):
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_expect_not_found_but_raises_forbidden(self, mock_policy,
+ mock_log):
"""Test that expecting 404 but getting 403 works for all scenarios.
Tests the following scenarios:
@@ -186,9 +177,8 @@
"NotFound, which was not thrown."
for permission in [True, False]:
- mock_permission = mock.Mock()
- mock_permission.get_permission.return_value = permission
- mock_auth.return_value = mock_permission
+ mock_policy.RbacPolicyParser.return_value.allowed.return_value =\
+ permission
e = self.assertRaises(exceptions.Forbidden, wrapper,
self.mock_args)
@@ -197,8 +187,8 @@
mock_log.error.reset_mock()
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_expect_not_found_and_raise_not_found(self, mock_auth, mock_log):
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_expect_not_found_and_raise_not_found(self, mock_policy, mock_log):
"""Test that expecting 404 and getting 404 works for all scenarios.
Tests the following scenarios:
@@ -220,9 +210,8 @@
]
for pos, permission in enumerate([True, False]):
- mock_permission = mock.Mock()
- mock_permission.get_permission.return_value = permission
- mock_auth.return_value = mock_permission
+ mock_policy.RbacPolicyParser.return_value.allowed.return_value =\
+ permission
expected_error = expected_errors[pos]
@@ -245,8 +234,8 @@
mock_log.error.reset_mock()
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_rule_validation_overpermission_negative(self, mock_auth,
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_rule_validation_overpermission_negative(self, mock_policy,
mock_log):
"""Test that OverPermission is correctly handled.
@@ -258,9 +247,7 @@
mock_function = mock.Mock()
wrapper = decorator(mock_function)
- mock_permission = mock.Mock()
- mock_permission.get_permission.return_value = False
- mock_auth.return_value = mock_permission
+ mock_policy.RbacPolicyParser.return_value.allowed.return_value = False
e = self.assertRaises(rbac_exceptions.RbacOverPermission, wrapper,
self.mock_args)
@@ -270,7 +257,7 @@
mock_log.error.assert_called_once_with(
"Role Member was allowed to perform sentinel.action")
- @mock.patch.object(rbac_auth, 'rbac_policy_parser', autospec=True)
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
def test_invalid_policy_rule_throws_parsing_exception(
self, mock_rbac_policy_parser):
"""Test that invalid policy action causes test to be skipped."""
@@ -295,8 +282,8 @@
mock.sentinel.project_id, mock.sentinel.user_id,
mock.sentinel.service, extra_target_data={})
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_get_exception_type_404(self, mock_auth):
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_get_exception_type_404(self, mock_policy):
"""Test that getting a 404 exception type returns NotFound."""
expected_exception = exceptions.NotFound
expected_irregular_msg = ("NotFound exception was caught for policy "
@@ -309,8 +296,8 @@
self.assertEqual(expected_exception, actual_exception)
self.assertEqual(expected_irregular_msg, actual_irregular_msg)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_get_exception_type_403(self, mock_auth):
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_get_exception_type_403(self, mock_policy):
"""Test that getting a 404 exception type returns Forbidden."""
expected_exception = exceptions.Forbidden
expected_irregular_msg = None
@@ -322,8 +309,9 @@
self.assertEqual(expected_irregular_msg, actual_irregular_msg)
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_exception_thrown_when_type_is_not_int(self, mock_auth, mock_log):
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_exception_thrown_when_type_is_not_int(self, mock_policy,
+ mock_log):
"""Test that non-integer exception type raises error."""
self.assertRaises(rbac_exceptions.RbacInvalidErrorCode,
rbac_rv._get_exception_type, "403")
@@ -333,8 +321,8 @@
"codes: [403, 404]")
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
- @mock.patch.object(rbac_auth, 'RbacAuthority', autospec=True)
- def test_exception_thrown_when_type_is_403_or_404(self, mock_auth,
+ @mock.patch.object(rbac_rv, 'rbac_policy_parser', autospec=True)
+ def test_exception_thrown_when_type_is_403_or_404(self, mock_policy,
mock_log):
"""Test that unsupported exceptions throw error."""
invalid_exceptions = [200, 400, 500]
diff --git a/releasenotes/notes/merge-rbac-auth-with-rbac-rule-validation-5d7c286788a95ee9.yaml b/releasenotes/notes/merge-rbac-auth-with-rbac-rule-validation-5d7c286788a95ee9.yaml
new file mode 100644
index 0000000..b96c73a
--- /dev/null
+++ b/releasenotes/notes/merge-rbac-auth-with-rbac-rule-validation-5d7c286788a95ee9.yaml
@@ -0,0 +1,7 @@
+---
+features:
+ - |
+ Merges `rbac_auth` with `rbac_rule_validation`, because `rbac_auth`
+ decentralized logic from `rbac_rule_validation` without providing any
+ authentication-related utility. This change facilitates code maintenance
+ and code readability.