Limit exception handling to calls within override_role
Motivation: prevents false positives caused by test
exceptions matching the expected exception before or
after the ``override_role`` context is called.
This patchset changes expected_error_codes behavior [0] by checking
errors explicitly outside the override_role context. This is done
by introducing a new function to rbac_rule_validation that is
used for validating that the expected exception isn't raised too
early (before ``override_role`` call) or too late (after
``override_call``) or at all (which is a bad test).
This means that exceptions raised prior to override_role
call result in failure. The same goes for exceptions raised
after override_role -- except for those that are an instance
of BasePatroleException (which is valid for things like
RbacMalformedResponse getting raised intentionally).
The new exception that is introduced is called
RbacOverrideRoleException.
Unit tests are added for all validation scenarios described
above.
[0] https://storyboard.openstack.org/#!/story/2003297
Story: 2003297
Task: 24246
Co-Authored-By: Felipe Monteiro <felipe.monteiro@att.com>
Change-Id: Iae9a58640463093f6dda20d40261b20051be2820
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 32deb9f..a7927fc 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -180,6 +180,7 @@
expected_exception, irregular_msg = _get_exception_type(
exp_error_code)
+ caught_exception = None
test_status = 'Allowed'
try:
@@ -193,13 +194,16 @@
LOG.error(msg)
except (expected_exception,
rbac_exceptions.RbacConflictingPolicies,
- rbac_exceptions.RbacMalformedResponse) as e:
+ rbac_exceptions.RbacMalformedResponse) as actual_exception:
+ caught_exception = actual_exception
test_status = 'Denied'
+
if irregular_msg:
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. "
@@ -209,8 +213,10 @@
sorted(disallowed_rules)))
LOG.error(msg)
raise rbac_exceptions.RbacUnderPermissionException(
- "%s Exception was: %s" % (msg, e))
+ "%s Exception was: %s" % (msg, actual_exception))
except Exception as actual_exception:
+ caught_exception = actual_exception
+
if _check_for_expected_mismatch_exception(expected_exception,
actual_exception):
LOG.error('Expected and actual exceptions do not match. '
@@ -249,6 +255,14 @@
"Allowed" if allowed else "Denied",
test_status)
+ # Sanity-check that ``override_role`` was called to eliminate
+ # false-positives and bad test flows resulting from exceptions
+ # getting raised too early, too late or not at all, within
+ # the scope of an RBAC test.
+ _validate_override_role_called(
+ test_obj,
+ actual_exception=caught_exception)
+
return wrapper
return decorator
@@ -389,7 +403,7 @@
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.")
+ "irregular")
return expected_exception, irregular_msg
@@ -431,8 +445,63 @@
def _check_for_expected_mismatch_exception(expected_exception,
actual_exception):
+ """Checks that ``expected_exception`` matches ``actual_exception``.
+
+ Since Patrole must handle 403/404 it is important that the expected and
+ actual error codes match.
+
+ :param excepted_exception: Expected exception for test.
+ :param actual_exception: Actual exception raised by test.
+ :returns: True if match, else False.
+ :rtype: boolean
+ """
permission_exceptions = (lib_exc.Forbidden, lib_exc.NotFound)
if isinstance(actual_exception, permission_exceptions):
if not isinstance(actual_exception, expected_exception.__class__):
return True
return False
+
+
+def _validate_override_role_called(test_obj, actual_exception):
+ """Validates that :func:`rbac_utils.RbacUtils.override_role` is called
+ during each Patrole test.
+
+ Useful for validating that the expected exception isn't raised too early
+ (before ``override_role`` call) or too late (after ``override_call``) or
+ at all (which is a bad test).
+
+ :param test_obj: An instance or subclass of ``tempest.test.BaseTestCase``.
+ :param actual_exception: Actual exception raised by test.
+ :raises RbacOverrideRoleException: If ``override_role`` isn't called, is
+ called too early, or is called too late.
+ """
+ called = test_obj._validate_override_role_called()
+ base_msg = ('This error is unrelated to RBAC and is due to either '
+ 'an API or override role failure. Exception: %s' %
+ actual_exception)
+
+ if not called:
+ if actual_exception is not None:
+ msg = ('Caught exception (%s) but it was raised before the '
+ '`override_role` context. ' % actual_exception.__class__)
+ else:
+ msg = 'Test missing required `override_role` call. '
+ msg += base_msg
+ LOG.error(msg)
+ raise rbac_exceptions.RbacOverrideRoleException(msg)
+ else:
+ exc_caught_in_ctx = test_obj._validate_override_role_caught_exc()
+ # This block is only executed if ``override_role`` is called. If
+ # an exception is raised and the exception wasn't raised in the
+ # ``override_role`` context and if the exception isn't a valid
+ # exception type (instance of ``BasePatroleException``), then this is
+ # a legitimate error.
+ if (not exc_caught_in_ctx and
+ actual_exception is not None and
+ not isinstance(actual_exception,
+ rbac_exceptions.BasePatroleException)):
+ msg = ('Caught exception (%s) but it was raised after the '
+ '`override_role` context. ' % actual_exception.__class__)
+ msg += base_msg
+ LOG.error(msg)
+ raise rbac_exceptions.RbacOverrideRoleException(msg)