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
This commit is contained in:
Felipe Monteiro
parent
84227729cf
commit
78fc4895be
|
|
@ -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
|
||||
|
|
@ -15,6 +15,7 @@
|
|||
|
||||
import logging
|
||||
import sys
|
||||
import testtools
|
||||
|
||||
import six
|
||||
|
||||
|
|
@ -22,8 +23,8 @@ from tempest import config
|
|||
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 @@ def action(service, rule, admin_only=False, expected_error_code=403,
|
|||
: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 @@ def action(service, rule, admin_only=False, expected_error_code=403,
|
|||
"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 @@ def action(service, rule, admin_only=False, expected_error_code=403,
|
|||
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:
|
||||
|
|
|
|||
|
|
@ -19,7 +19,6 @@ from tempest.lib import exceptions
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
" 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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
" 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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
"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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
]
|
||||
|
||||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
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 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
"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]
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
Loading…
Reference in New Issue