Update overpermission/underpermission rbac exceptions
This patchset updates rbac_exceptions by bringing the concept
of under-permission and over-permission together. An over-permission
occurs when an unauthorized role is allowed to perform an action
and an under-permission occurs when an authorized role is not
allowed to perform an action. Both of these are important failure
scenarios.
Current Patrole has an RbacOverPermission Exception but uses
a "Forbidden" as a pseudonym for the under-permission version
but this is not ideal for the following reasons:
* Patrole can expect a 404 Not Found due to Neutron policy enforcement [0]
* The naming is inconsistent with RbacOverPermission
* It should have a Patrole wrapper exception (NotFound is used directly
from Tempest)
So, this patchset:
* renames RbacOverPermission to RbacOverPermissionException
* replaces Forbidden exception with RbacUnderPermissionException
* updates documentation, docstrings and unit tests
In addition, this patchset introduces a new exception called
RbacExpectedWrongException which is raised when the expected
exception does not match the actual exception and both are instances
of 403 and 404, which means that the RBAC test uses the wrong
expected_error_codes.
Change-Id: I681610448cbe0269f02c34ea6afaaaf29c306121
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 7d48870..115e23e 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.
@@ -123,8 +123,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::
@@ -201,17 +203,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 = (
@@ -221,7 +234,7 @@
)
)
LOG.error(msg)
- raise rbac_exceptions.RbacOverPermission(msg)
+ raise rbac_exceptions.RbacOverPermissionException(msg)
finally:
if CONF.patrole_log.enable_reporting:
RBACLOG.info(
@@ -236,7 +249,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 "
@@ -410,3 +422,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..23aa458 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -95,11 +95,12 @@
@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
@@ -109,8 +110,9 @@
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)
@@ -149,8 +151,9 @@
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)
@@ -190,8 +193,9 @@
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)
@@ -211,17 +215,15 @@
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)
@@ -254,8 +256,9 @@
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)
@@ -274,7 +277,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.
@@ -295,7 +298,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()
@@ -336,7 +339,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
@@ -449,7 +452,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 +471,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 +523,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 +541,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 +551,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.
"""