Merge "followup: Include spec/discussion references"
diff --git a/doc/source/framework/overview.rst b/doc/source/framework/overview.rst
index d862770..4902f7b 100644
--- a/doc/source/framework/overview.rst
+++ b/doc/source/framework/overview.rst
@@ -26,9 +26,11 @@
"Inconsistent" (or failing) cases include:
* Expected result is ``False`` and the test passes. This results in an
- ``RbacOverPermission`` exception getting thrown.
+ :class:`~rbac_exceptions.RbacOverPermissionException` exception
+ getting thrown.
* Expected result is ``True`` and the test fails. This results in a
- ``Forbidden`` exception getting thrown.
+ :class:`~rbac_exceptions.RbacOverPermissionException` exception
+ getting thrown.
For example, a 200 from the API call and a ``False`` result from
``oslo.policy`` or a 403 from the API call and a ``True`` result from
diff --git a/etc/patrole.conf.sample b/etc/patrole.conf.sample
index 8e7931b..6de073d 100644
--- a/etc/patrole.conf.sample
+++ b/etc/patrole.conf.sample
@@ -45,7 +45,7 @@
#
# YAML definition: allowed
# test run: not allowed
-# test result: fail (under-permission, e.g. Forbidden exception)
+# test result: fail (under-permission)
#
# YAML definition: not allowed
# test run: allowed
diff --git a/patrole_tempest_plugin/config.py b/patrole_tempest_plugin/config.py
index ee7a6c5..4dc27b9 100644
--- a/patrole_tempest_plugin/config.py
+++ b/patrole_tempest_plugin/config.py
@@ -55,7 +55,7 @@
YAML definition: allowed
test run: not allowed
-test result: fail (under-permission, e.g. Forbidden exception)
+test result: fail (under-permission)
YAML definition: not allowed
test run: allowed
diff --git a/patrole_tempest_plugin/rbac_exceptions.py b/patrole_tempest_plugin/rbac_exceptions.py
index e75b8ec..980672a 100644
--- a/patrole_tempest_plugin/rbac_exceptions.py
+++ b/patrole_tempest_plugin/rbac_exceptions.py
@@ -41,8 +41,27 @@
message = "RBAC resource setup failed"
-class RbacOverPermission(exceptions.TempestException):
- message = "Action performed that should not be permitted"
+class RbacOverPermissionException(exceptions.TempestException):
+ """Raised when the expected result is failure but the actual result is
+ pass.
+ """
+ message = "Unauthorized action was allowed to be performed"
+
+
+class RbacUnderPermissionException(exceptions.TempestException):
+ """Raised when the expected result is pass but the actual result is
+ failure.
+ """
+ message = "Authorized action was not allowed to be performed"
+
+
+class RbacExpectedWrongException(exceptions.TempestException):
+ """Raised when the expected exception does not match the actual exception
+ raised, when both are instances of Forbidden or NotFound, indicating
+ the test provides a wrong argument to `expected_error_codes`.
+ """
+ message = ("Expected %(expected)s to be raised but %(actual)s was raised "
+ "instead. Actual exception: %(exception)s")
class RbacInvalidService(exceptions.TempestException):
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 23a210a..2f6759d 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -64,9 +64,9 @@
1) If *expected* is True and the test passes (*actual*), this is a success.
2) If *expected* is True and the test fails (*actual*), this results in a
- `Forbidden` exception failure.
+ ``RbacUnderPermissionException`` exception failure.
3) If *expected* is False and the test passes (*actual*), this results in
- an `OverPermission` exception failure.
+ an ``RbacOverPermissionException`` exception failure.
4) If *expected* is False and the test fails (*actual*), this is a success.
As such, negative and positive testing can be applied using this decorator.
@@ -124,8 +124,10 @@
})
:raises NotFound: If ``service`` is invalid.
- :raises Forbidden: For item (2) above.
- :raises RbacOverPermission: For item (3) above.
+ :raises RbacUnderPermissionException: For item (2) above.
+ :raises RbacOverPermissionException: For item (3) above.
+ :raises RbacExpectedWrongException: When a 403 is expected but a 404
+ is raised instead or vice versa.
Examples::
@@ -193,7 +195,10 @@
rbac_exceptions.RbacMalformedResponse) as e:
test_status = 'Denied'
if irregular_msg:
- LOG.warning(irregular_msg.format(rule, service))
+ LOG.warning(irregular_msg,
+ test_func.__name__,
+ ', '.join(rules),
+ service)
if allowed:
msg = ("Role %s was not allowed to perform the following "
"actions: %s. Expected allowed actions: %s. "
@@ -202,17 +207,28 @@
sorted(set(rules) - set(disallowed_rules)),
sorted(disallowed_rules)))
LOG.error(msg)
- raise exceptions.Forbidden(
+ raise rbac_exceptions.RbacUnderPermissionException(
"%s Exception was: %s" % (msg, e))
- except Exception as e:
- with excutils.save_and_reraise_exception():
- exc_info = sys.exc_info()
- error_details = six.text_type(exc_info[1])
- msg = ("An unexpected exception has occurred during test: "
- "%s. Exception was: %s" % (test_func.__name__,
- error_details))
- test_status = 'Error, %s' % (error_details)
- LOG.error(msg)
+ except Exception as actual_exception:
+ if _check_for_expected_mismatch_exception(expected_exception,
+ actual_exception):
+ LOG.error('Expected and actual exceptions do not match. '
+ 'Expected: %s. Actual: %s.',
+ expected_exception,
+ actual_exception.__class__)
+ raise rbac_exceptions.RbacExpectedWrongException(
+ expected=expected_exception,
+ actual=actual_exception.__class__,
+ exception=actual_exception)
+ else:
+ with excutils.save_and_reraise_exception():
+ exc_info = sys.exc_info()
+ error_details = six.text_type(exc_info[1])
+ msg = ("An unexpected exception has occurred during "
+ "test: %s. Exception was: %s" % (
+ test_func.__name__, error_details))
+ test_status = 'Error, %s' % (error_details)
+ LOG.error(msg)
else:
if not allowed:
msg = (
@@ -222,13 +238,13 @@
)
)
LOG.error(msg)
- raise rbac_exceptions.RbacOverPermission(msg)
+ raise rbac_exceptions.RbacOverPermissionException(msg)
finally:
if CONF.patrole_log.enable_reporting:
RBACLOG.info(
- "[Service]: %s, [Test]: %s, [Rule]: %s, "
+ "[Service]: %s, [Test]: %s, [Rules]: %s, "
"[Expected]: %s, [Actual]: %s",
- service, test_func.__name__, rule,
+ service, test_func.__name__, ', '.join(rules),
"Allowed" if allowed else "Denied",
test_status)
@@ -237,7 +253,6 @@
def _prepare_multi_policy(rule, rules, exp_error_code, exp_error_codes):
-
if exp_error_codes:
if not rules:
msg = ("The `rules` list must be provided if using the "
@@ -332,16 +347,16 @@
is_allowed = authority.allowed(rule, role)
if is_allowed:
- LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule,
+ LOG.debug("[Policy action]: %s, [Role]: %s is allowed!", rule,
role)
else:
- LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
+ LOG.debug("[Policy action]: %s, [Role]: %s is NOT allowed!",
rule, role)
return is_allowed
-def _get_exception_type(expected_error_code=403):
+def _get_exception_type(expected_error_code=_DEFAULT_ERROR_CODE):
"""Dynamically calculate the expected exception to be caught.
Dynamically calculate the expected exception to be caught by the test case.
@@ -370,9 +385,10 @@
expected_exception = exceptions.Forbidden
elif expected_error_code == 404:
expected_exception = exceptions.NotFound
- irregular_msg = ("NotFound exception was caught for policy action "
- "{0}. The service {1} throws a 404 instead of a 403, "
- "which is irregular.")
+ irregular_msg = ("NotFound exception was caught for test %s. Expected "
+ "policies which may have caused the error: %s. The "
+ "service %s throws a 404 instead of a 403, which is "
+ "irregular.")
return expected_exception, irregular_msg
@@ -411,3 +427,12 @@
formatted_target_data[user_attribute] = attr_value
return formatted_target_data
+
+
+def _check_for_expected_mismatch_exception(expected_exception,
+ actual_exception):
+ permission_exceptions = (exceptions.Forbidden, exceptions.NotFound)
+ if isinstance(actual_exception, permission_exceptions):
+ if not isinstance(actual_exception, expected_exception.__class__):
+ return True
+ return False
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 2ae860c..2e275dc 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -64,7 +64,7 @@
"""
mock_authority.PolicyAuthority.return_value.allowed.return_value = True
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action])
def test_policy(*args):
pass
@@ -83,7 +83,7 @@
mock_authority.PolicyAuthority.return_value.allowed.return_value =\
False
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action])
def test_policy(*args):
raise exceptions.Forbidden()
@@ -95,22 +95,24 @@
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
def test_rule_validation_forbidden_negative(self, mock_authority,
mock_log):
- """Test Forbidden error is thrown and have permission fails.
+ """Test RbacUnderPermissionException error is thrown and have
+ permission fails.
Negative test case: if Forbidden is thrown and the user should be
- allowed to perform the action, then the Forbidden exception should be
- raised.
+ allowed to perform the action, then the RbacUnderPermissionException
+ exception should be raised.
"""
mock_authority.PolicyAuthority.return_value.allowed.return_value = True
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action])
def test_policy(*args):
raise exceptions.Forbidden()
test_re = ("Role Member was not allowed to perform the following "
"actions: \[%s\].*" % (mock.sentinel.action))
- self.assertRaisesRegex(exceptions.Forbidden, test_re, test_policy,
- self.mock_test_args)
+ self.assertRaisesRegex(
+ rbac_exceptions.RbacUnderPermissionException, test_re, test_policy,
+ self.mock_test_args)
self.assertRegex(mock_log.error.mock_calls[0][1][0], test_re)
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
@@ -125,7 +127,7 @@
mock_authority.PolicyAuthority.return_value.allowed.return_value =\
False
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action])
def test_policy(*args):
raise rbac_exceptions.RbacMalformedResponse()
@@ -143,14 +145,15 @@
"""
mock_authority.PolicyAuthority.return_value.allowed.return_value = True
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action])
def test_policy(*args):
raise rbac_exceptions.RbacMalformedResponse()
test_re = ("Role Member was not allowed to perform the following "
"actions: \[%s\].*" % (mock.sentinel.action))
- self.assertRaisesRegex(exceptions.Forbidden, test_re, test_policy,
- self.mock_test_args)
+ self.assertRaisesRegex(
+ rbac_exceptions.RbacUnderPermissionException, test_re, test_policy,
+ self.mock_test_args)
self.assertRegex(mock_log.error.mock_calls[0][1][0], test_re)
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
@@ -165,7 +168,7 @@
mock_authority.PolicyAuthority.return_value.allowed.return_value =\
False
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action])
def test_policy(*args):
raise rbac_exceptions.RbacConflictingPolicies()
@@ -184,14 +187,15 @@
"""
mock_authority.PolicyAuthority.return_value.allowed.return_value = True
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action])
def test_policy(*args):
raise rbac_exceptions.RbacConflictingPolicies()
test_re = ("Role Member was not allowed to perform the following "
"actions: \[%s\].*" % (mock.sentinel.action))
- self.assertRaisesRegex(exceptions.Forbidden, test_re, test_policy,
- self.mock_test_args)
+ self.assertRaisesRegex(
+ rbac_exceptions.RbacUnderPermissionException, test_re, test_policy,
+ self.mock_test_args)
self.assertRegex(mock_log.error.mock_calls[0][1][0], test_re)
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
@@ -206,22 +210,20 @@
2) Test have permission and 404 is expected but 403 is thrown throws
exception.
"""
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action,
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action],
expected_error_code=404)
def test_policy(*args):
raise exceptions.Forbidden('Test message')
- error_msg = ("An unexpected exception has occurred during test: "
- "test_policy. Exception was: Forbidden\nDetails: Test "
- "message")
+ error_re = r'Expected .* to be raised but .* was raised instead'
for allowed in [True, False]:
mock_authority.PolicyAuthority.return_value.allowed.\
return_value = allowed
- self.assertRaisesRegex(exceptions.Forbidden, 'Test message',
- test_policy, self.mock_test_args)
- self.assertIn(error_msg, mock_log.error.mock_calls[0][1][0])
- mock_log.error.reset_mock()
+ self.assertRaisesRegex(
+ rbac_exceptions.RbacExpectedWrongException, error_re,
+ test_policy, self.mock_test_args)
+ self.assertTrue(mock_log.error.called)
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
@@ -236,14 +238,16 @@
In both cases, a LOG.warning is called with the "irregular message"
that signals to user that a 404 was expected and caught.
"""
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action,
+ policy_names = ['foo:bar']
+
+ @rbac_rv.action(mock.sentinel.service, rules=policy_names,
expected_error_code=404)
def test_policy(*args):
raise exceptions.NotFound()
expected_errors = [
("Role Member was not allowed to perform the following "
- "actions: \[%s\].*" % (mock.sentinel.action)),
+ "actions: \['%s'\].*" % policy_names[0]),
None
]
@@ -254,18 +258,21 @@
error_re = expected_errors[pos]
if error_re:
- self.assertRaisesRegex(exceptions.Forbidden, error_re,
- test_policy, self.mock_test_args)
+ self.assertRaisesRegex(
+ rbac_exceptions.RbacUnderPermissionException, error_re,
+ test_policy, self.mock_test_args)
self.assertRegex(mock_log.error.mock_calls[0][1][0], error_re)
else:
test_policy(self.mock_test_args)
mock_log.error.assert_not_called()
mock_log.warning.assert_called_with(
- "NotFound exception was caught for policy action {0}. The "
- "service {1} throws a 404 instead of a 403, which is "
- "irregular.".format(mock.sentinel.action,
- mock.sentinel.service))
+ "NotFound exception was caught for test %s. Expected policies "
+ "which may have caused the error: %s. The service %s throws a "
+ "404 instead of a 403, which is irregular.",
+ test_policy.__name__,
+ ', '.join(policy_names),
+ mock.sentinel.service)
mock_log.warning.reset_mock()
mock_log.error.reset_mock()
@@ -274,7 +281,7 @@
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
def test_rule_validation_overpermission_negative(self, mock_authority,
mock_log):
- """Test that OverPermission is correctly handled.
+ """Test that RbacOverPermissionException is correctly handled.
Tests that case where no exception is thrown but the Patrole framework
says that the role should not be allowed to perform the policy action.
@@ -282,11 +289,11 @@
mock_authority.PolicyAuthority.return_value.allowed.return_value =\
False
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action])
def test_policy_expect_forbidden(*args):
pass
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action,
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action],
expected_error_code=404)
def test_policy_expect_not_found(*args):
pass
@@ -295,7 +302,7 @@
test_policy_expect_forbidden, test_policy_expect_not_found):
error_re = ".*OverPermission: .* \[%s\]$" % mock.sentinel.action
- self.assertRaisesRegex(rbac_exceptions.RbacOverPermission,
+ self.assertRaisesRegex(rbac_exceptions.RbacOverPermissionException,
error_re, test_policy, self.mock_test_args)
self.assertRegex(mock_log.error.mock_calls[0][1][0], error_re)
mock_log.error.reset_mock()
@@ -324,9 +331,10 @@
def test_get_exception_type_404(self, _):
"""Test that getting a 404 exception type returns NotFound."""
expected_exception = exceptions.NotFound
- expected_irregular_msg = ("NotFound exception was caught for policy "
- "action {0}. The service {1} throws a 404 "
- "instead of a 403, which is irregular.")
+ expected_irregular_msg = (
+ "NotFound exception was caught for test %s. Expected policies "
+ "which may have caused the error: %s. The service %s throws a "
+ "404 instead of a 403, which is irregular.")
actual_exception, actual_irregular_msg = \
rbac_rv._get_exception_type(404)
@@ -336,7 +344,7 @@
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
def test_get_exception_type_403(self, _):
- """Test that getting a 404 exception type returns Forbidden."""
+ """Test that getting a 403 exception type returns Forbidden."""
expected_exception = exceptions.Forbidden
expected_irregular_msg = None
@@ -371,6 +379,12 @@
mock_log.error.reset_mock()
+
+class RBACRuleValidationLoggingTest(BaseRBACRuleValidationTest):
+ """Test class for validating the RBAC log, dedicated to just logging
+ Patrole RBAC validation work flows.
+ """
+
@mock.patch.object(rbac_rv, 'RBACLOG', autospec=True)
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
def test_rbac_report_logging_disabled(self, mock_authority, mock_rbaclog):
@@ -382,7 +396,7 @@
mock_authority.PolicyAuthority.return_value.allowed.return_value = True
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action])
def test_policy(*args):
pass
@@ -399,17 +413,19 @@
fixtures.ConfPatcher(enable_reporting=True, group='patrole_log'))
mock_authority.PolicyAuthority.return_value.allowed.return_value = True
+ policy_names = ['foo:bar', 'baz:qux']
- @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ @rbac_rv.action(mock.sentinel.service, rules=policy_names)
def test_policy(*args):
pass
test_policy(self.mock_test_args)
mock_rbaclog.info.assert_called_once_with(
- "[Service]: %s, [Test]: %s, [Rule]: %s, "
+ "[Service]: %s, [Test]: %s, [Rules]: %s, "
"[Expected]: %s, [Actual]: %s",
- mock.sentinel.service, 'test_policy',
- mock.sentinel.action,
+ mock.sentinel.service,
+ 'test_policy',
+ ', '.join(policy_names),
"Allowed",
"Allowed")
@@ -449,7 +465,8 @@
def test_rule_validation_multi_policy_overpermission_failure(
self, mock_authority, mock_log):
"""Test that when expected result is unauthorized and test passes that
- the overall evaluation results in an OverPermission getting raised.
+ the overall evaluation results in an RbacOverPermissionException
+ getting raised.
"""
rules = [
@@ -467,8 +484,9 @@
allowed_list)
error_re = ".*OverPermission: .* \[%s\]$" % fail_on_action
- self.assertRaisesRegex(rbac_exceptions.RbacOverPermission,
- error_re, test_policy, self.mock_test_args)
+ self.assertRaisesRegex(
+ rbac_exceptions.RbacOverPermissionException, error_re,
+ test_policy, self.mock_test_args)
mock_log.debug.assert_any_call(
"%s: Expecting %d to be raised for policy name: %s",
'test_policy', 403, fail_on_action)
@@ -518,7 +536,7 @@
self, mock_authority, mock_log):
"""Test that when the expected result is authorized and the test
fails (with a Forbidden error code) that the overall evaluation
- results a Forbidden getting raised.
+ results in a RbacUnderPermissionException getting raised.
"""
# NOTE: Avoid mock.sentinel here due to weird sorting with them.
@@ -536,8 +554,9 @@
"actions: %s. Expected allowed actions: %s. Expected "
"disallowed actions: []." % (rules, rules)).replace(
'[', '\[').replace(']', '\]')
- self.assertRaisesRegex(exceptions.Forbidden, error_re, test_policy,
- self.mock_test_args)
+ self.assertRaisesRegex(
+ rbac_exceptions.RbacUnderPermissionException, error_re,
+ test_policy, self.mock_test_args)
self.assertRegex(mock_log.error.mock_calls[0][1][0], error_re)
self._assert_policy_authority_called_with(rules, mock_authority)
@@ -545,7 +564,7 @@
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
def test_rule_validation_multi_actions_forbidden(
self, mock_authority, mock_log):
- """Test that when the expected result is forbidden because
+ """Test that when the expected result is Forbidden because
two of the actions fail and the first action specifies 403,
verify that the overall evaluation results in success.
"""
@@ -583,8 +602,8 @@
"""
rules = [
- mock.sentinel.action1, mock.sentinel.action2,
- mock.sentinel.action3, mock.sentinel.action4
+ 'mock.sentinel.action1', 'mock.sentinel.action2',
+ 'mock.sentinel.action3', 'mock.sentinel.action4'
]
exp_ecodes = [403, 404, 403, 403]
@@ -603,8 +622,8 @@
mock_log.error.assert_not_called()
self._assert_policy_authority_called_with(rules, mock_authority)
- _do_test([True, False, False, True], mock.sentinel.action2)
- _do_test([True, False, True, False], mock.sentinel.action2)
+ _do_test([True, False, False, True], 'mock.sentinel.action2')
+ _do_test([True, False, True, False], 'mock.sentinel.action2')
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
def test_prepare_multi_policy_allowed_usages(self, mock_log):