Merge "Add support for handling multiple error codes"
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 69a43ea..7d48870 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -33,11 +33,13 @@
LOG = logging.getLogger(__name__)
_SUPPORTED_ERROR_CODES = [403, 404]
+_DEFAULT_ERROR_CODE = 403
RBACLOG = logging.getLogger('rbac_reporting')
-def action(service, rule='', rules=None, expected_error_code=403,
+def action(service, rule='', rules=None,
+ expected_error_code=_DEFAULT_ERROR_CODE, expected_error_codes=None,
extra_target_data=None):
"""A decorator for verifying OpenStack policy enforcement.
@@ -90,6 +92,25 @@
A 404 should not be provided *unless* the endpoint masks a
``Forbidden`` exception as a ``NotFound`` exception.
+ :param list expected_error_codes: When the ``rules`` list parameter is
+ used, then this list indicates the expected error code to use if one
+ of the rules does not allow the role being tested. This list must
+ coincide with and its elements remain in the same order as the rules
+ in the rules list.
+
+ Example::
+ rules=["api_action1", "api_action2"]
+ expected_error_codes=[404, 403]
+
+ a) If api_action1 fails and api_action2 passes, then the expected
+ error code is 404.
+ b) if api_action2 fails and api_action1 passes, then the expected
+ error code is 403.
+ c) if both api_action1 and api_action2 fail, then the expected error
+ code is the first error seen (404).
+
+ If an error code is missing from the list, it is defaulted to 403.
+
:param dict extra_target_data: Dictionary, keyed with ``oslo.policy``
generic check names, whose values are string literals that reference
nested ``tempest.test.BaseTestCase`` attributes. Used by
@@ -118,7 +139,9 @@
if extra_target_data is None:
extra_target_data = {}
- rules = _prepare_rules(rule, rules)
+ rules, expected_error_codes = _prepare_multi_policy(rule, rules,
+ expected_error_code,
+ expected_error_codes)
def decorator(test_func):
role = CONF.patrole.rbac_test_role
@@ -141,8 +164,18 @@
disallowed_rules.append(rule)
allowed = allowed and _allowed
+ exp_error_code = expected_error_code
+ if disallowed_rules:
+ # Choose the first disallowed rule and expect the error
+ # code corresponding to it.
+ first_error_index = rules.index(disallowed_rules[0])
+ exp_error_code = expected_error_codes[first_error_index]
+ LOG.debug("%s: Expecting %d to be raised for policy name: %s",
+ test_func.__name__, exp_error_code,
+ disallowed_rules[0])
+
expected_exception, irregular_msg = _get_exception_type(
- expected_error_code)
+ exp_error_code)
test_status = 'Allowed'
@@ -202,7 +235,32 @@
return decorator
-def _prepare_rules(rule, rules):
+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 "
+ "`expected_error_codes` list.")
+ raise ValueError(msg)
+ if len(rules) != len(exp_error_codes):
+ msg = ("The `expected_error_codes` list is not the same length "
+ "as the `rules` list.")
+ raise ValueError(msg)
+ if exp_error_code:
+ deprecation_msg = (
+ "The `exp_error_code` argument has been deprecated in favor "
+ "of `exp_error_codes` and will be removed in a future "
+ "version.")
+ versionutils.report_deprecated_feature(LOG, deprecation_msg)
+ LOG.debug("The `exp_error_codes` argument will be used instead of "
+ "`exp_error_code`.")
+ if not isinstance(exp_error_codes, (tuple, list)):
+ exp_error_codes = [exp_error_codes]
+ else:
+ exp_error_codes = []
+ if exp_error_code:
+ exp_error_codes.append(exp_error_code)
+
if rules is None:
rules = []
elif not isinstance(rules, (tuple, list)):
@@ -216,7 +274,18 @@
LOG.debug("The `rules` argument will be used instead of `rule`.")
else:
rules.append(rule)
- return rules
+
+ # Fill in the exp_error_codes if needed. This is needed for the scenarios
+ # where no exp_error_codes array is provided, so the error codes must be
+ # set to the default error code value and there must be the same number
+ # of error codes as rules.
+ num_ecs = len(exp_error_codes)
+ num_rules = len(rules)
+ if (num_ecs < num_rules):
+ for i in range(num_rules - num_ecs):
+ exp_error_codes.append(_DEFAULT_ERROR_CODE)
+
+ return rules, exp_error_codes
def _is_authorized(test_obj, service, rule, extra_target_data):
diff --git a/patrole_tempest_plugin/tests/api/network/test_routers_rbac.py b/patrole_tempest_plugin/tests/api/network/test_routers_rbac.py
index ab85745..812b0c1 100644
--- a/patrole_tempest_plugin/tests/api/network/test_routers_rbac.py
+++ b/patrole_tempest_plugin/tests/api/network/test_routers_rbac.py
@@ -330,7 +330,8 @@
self.routers_client.delete_router(router['id'])
@rbac_rule_validation.action(service="neutron",
- rule="add_router_interface")
+ rules=["get_router", "add_router_interface"],
+ expected_error_codes=[404, 403])
@decorators.idempotent_id('a0627778-d68d-4913-881b-e345360cca19')
def test_add_router_interface(self):
"""Add Router Interface
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 0a4c44b..2ae860c 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -436,7 +436,8 @@
rules = [mock.sentinel.action1, mock.sentinel.action2]
- @rbac_rv.action(mock.sentinel.service, rules=rules)
+ @rbac_rv.action(mock.sentinel.service, rules=rules,
+ expected_error_codes=[403, 403])
def test_policy(*args):
pass
@@ -454,8 +455,10 @@
rules = [
mock.sentinel.action1, mock.sentinel.action2, mock.sentinel.action3
]
+ exp_ecodes = [403, 403, 403]
- @rbac_rv.action(mock.sentinel.service, rules=rules)
+ @rbac_rv.action(mock.sentinel.service, rules=rules,
+ expected_error_codes=exp_ecodes)
def test_policy(*args):
pass
@@ -466,6 +469,9 @@
error_re = ".*OverPermission: .* \[%s\]$" % fail_on_action
self.assertRaisesRegex(rbac_exceptions.RbacOverPermission,
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)
self.assertRegex(mock_log.error.mock_calls[0][1][0], error_re)
mock_log.error.reset_mock()
self._assert_policy_authority_called_with(rules, mock_authority)
@@ -485,21 +491,26 @@
rules = [
mock.sentinel.action1, mock.sentinel.action2, mock.sentinel.action3
]
+ exp_ecodes = [403, 403, 403]
- @rbac_rv.action(mock.sentinel.service, rules=rules)
+ @rbac_rv.action(mock.sentinel.service, rules=rules,
+ expected_error_codes=exp_ecodes)
def test_policy(*args):
raise exceptions.Forbidden()
- def _do_test(allowed_list):
+ def _do_test(allowed_list, fail_on_action):
mock_authority.PolicyAuthority.return_value.allowed.\
side_effect = allowed_list
test_policy(self.mock_test_args)
+ mock_log.debug.assert_called_with(
+ "%s: Expecting %d to be raised for policy name: %s",
+ 'test_policy', 403, fail_on_action)
mock_log.error.assert_not_called()
self._assert_policy_authority_called_with(rules, mock_authority)
- _do_test([True, True, False])
- _do_test([False, True, True])
- _do_test([True, False, True])
+ _do_test([True, True, False], mock.sentinel.action3)
+ _do_test([False, True, True], mock.sentinel.action1)
+ _do_test([True, False, True], mock.sentinel.action2)
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
@@ -513,7 +524,8 @@
# NOTE: Avoid mock.sentinel here due to weird sorting with them.
rules = ['action1', 'action2', 'action3']
- @rbac_rv.action(mock.sentinel.service, rules=rules)
+ @rbac_rv.action(mock.sentinel.service, rules=rules,
+ expected_error_codes=[403, 403, 403])
def test_policy(*args):
raise exceptions.Forbidden()
@@ -528,3 +540,136 @@
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)
+
+ @mock.patch.object(rbac_rv, 'LOG', autospec=True)
+ @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
+ two of the actions fail and the first action specifies 403,
+ verify that the overall evaluation results in success.
+ """
+
+ rules = [
+ mock.sentinel.action1, mock.sentinel.action2, mock.sentinel.action3
+ ]
+ exp_ecodes = [403, 403, 404]
+
+ @rbac_rv.action(mock.sentinel.service, rules=rules,
+ expected_error_codes=exp_ecodes)
+ def test_policy(*args):
+ raise exceptions.Forbidden()
+
+ def _do_test(allowed_list, fail_on_action):
+ mock_authority.PolicyAuthority.return_value.allowed.\
+ side_effect = allowed_list
+ test_policy(self.mock_test_args)
+ mock_log.debug.assert_called_with(
+ "%s: Expecting %d to be raised for policy name: %s",
+ 'test_policy', 403, fail_on_action)
+ mock_log.error.assert_not_called()
+ self._assert_policy_authority_called_with(rules, mock_authority)
+
+ _do_test([False, True, False], mock.sentinel.action1)
+ _do_test([False, False, True], mock.sentinel.action1)
+
+ @mock.patch.object(rbac_rv, 'LOG', autospec=True)
+ @mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
+ def test_rule_validation_multi_actions_notfound(
+ self, mock_authority, mock_log):
+ """Test that when the expected result is not found because
+ two of the actions fail and the first action specifies 404,
+ verify that the overall evaluation results in success.
+ """
+
+ rules = [
+ mock.sentinel.action1, mock.sentinel.action2,
+ mock.sentinel.action3, mock.sentinel.action4
+ ]
+ exp_ecodes = [403, 404, 403, 403]
+
+ @rbac_rv.action(mock.sentinel.service, rules=rules,
+ expected_error_codes=exp_ecodes)
+ def test_policy(*args):
+ raise exceptions.NotFound()
+
+ def _do_test(allowed_list, fail_on_action):
+ mock_authority.PolicyAuthority.return_value.allowed.\
+ side_effect = allowed_list
+ test_policy(self.mock_test_args)
+ mock_log.debug.assert_called_with(
+ "%s: Expecting %d to be raised for policy name: %s",
+ 'test_policy', 404, fail_on_action)
+ 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)
+
+ @mock.patch.object(rbac_rv, 'LOG', autospec=True)
+ def test_prepare_multi_policy_allowed_usages(self, mock_log):
+
+ def _do_test(rule, rules, ecode, ecodes, exp_rules, exp_ecodes):
+ rule_list, ec_list = rbac_rv._prepare_multi_policy(rule, rules,
+ ecode, ecodes)
+ self.assertEqual(rule_list, exp_rules)
+ self.assertEqual(ec_list, exp_ecodes)
+
+ # Validate that using deprecated values: rule and expected_error_code
+ # are converted into rules = [rule] and expected_error_codes =
+ # [expected_error_code]
+ _do_test("rule1", None, 403, None, ["rule1"], [403])
+
+ # Validate that rules = [rule] and expected_error_codes defaults to
+ # 403 when no values are provided.
+ _do_test("rule1", None, None, None, ["rule1"], [403])
+
+ # Validate that `len(rules) == len(expected_error_codes)` works when
+ # both == 1.
+ _do_test(None, ["rule1"], None, [403], ["rule1"], [403])
+
+ # Validate that `len(rules) == len(expected_error_codes)` works when
+ # both are > 1.
+ _do_test(None, ["rule1", "rule2"], None, [403, 404],
+ ["rule1", "rule2"], [403, 404])
+
+ # Validate that when only a default expected_error_code argument is
+ # provided, that default value and other default values (403) are
+ # filled into the expected_error_codes list.
+ # Example:
+ # @rbac_rv.action(service, rules=[<rule>, <rule>])
+ # def test_policy(*args):
+ # ...
+ _do_test(None, ["rule1", "rule2"], 403, None,
+ ["rule1", "rule2"], [403, 403])
+
+ # Validate that the deprecated values are ignored when new values are
+ # provided.
+ _do_test("rule3", ["rule1", "rule2"], 404, [403, 403],
+ ["rule1", "rule2"], [403, 403])
+ mock_log.debug.assert_any_call(
+ "The `rules` argument will be used instead of `rule`.")
+ mock_log.debug.assert_any_call(
+ "The `exp_error_codes` argument will be used instead of "
+ "`exp_error_code`.")
+
+ @mock.patch.object(rbac_rv, 'LOG', autospec=True)
+ def test_prepare_multi_policy_disallowed_usages(self, mock_log):
+
+ def _do_test(rule, rules, ecode, ecodes):
+ rule_list, ec_list = rbac_rv._prepare_multi_policy(rule, rules,
+ ecode, ecodes)
+
+ error_re = ("The `expected_error_codes` list is not the same length"
+ " as the `rules` list.")
+ # When len(rules) > 1 then len(expected_error_codes) must be same len.
+ self.assertRaisesRegex(ValueError, error_re, _do_test,
+ None, ["rule1", "rule2"], None, [403])
+ # When len(expected_error_codes) > 1 len(rules) must be same len.
+ self.assertRaisesRegex(ValueError, error_re, _do_test,
+ None, ["rule1"], None, [403, 404])
+ error_re = ("The `rules` list must be provided if using the "
+ "`expected_error_codes` list.")
+ # When expected_error_codes is provided rules must be as well.
+ self.assertRaisesRegex(ValueError, error_re, _do_test,
+ None, None, None, [404])
diff --git a/releasenotes/notes/multi-policy-support-4e5c8b4e9e25ad9d.yaml b/releasenotes/notes/multi-policy-support-4e5c8b4e9e25ad9d.yaml
index 3d192d9..1f33d8f 100644
--- a/releasenotes/notes/multi-policy-support-4e5c8b4e9e25ad9d.yaml
+++ b/releasenotes/notes/multi-policy-support-4e5c8b4e9e25ad9d.yaml
@@ -7,7 +7,25 @@
expected test result. This allows Patrole to more accurately determine
whether RBAC is configured correctly, since some API endpoints enforce
multiple policies.
+
+ Multiple policy support includes the capability to specify multiple
+ expected error codes, as some components may return different error codes
+ for different roles due to checking multiple policy rules. The
+ ``expected_error_codes`` argument has been added to the
+ ``rbac_rule_validation.action`` decorator, which is a list of error codes
+ expected when the corresponding rule in the ``rules`` list is disallowed
+ to perform the API action. For this reason, the error codes in the
+ ``expected_error_codes`` list must appear in the same order as their
+ corresponding rules in the ``rules`` list. For example:
+
+ expected_error_codes[0] is the error code for the rules[0] rule.
+ expected_error_codes[1] is the error code for the rules[1] rule.
+ ...
+
deprecations:
- |
The ``rule`` argument in the ``rbac_rule_validation.action`` decorator has
been deprecated in favor of ``rules``.
+
+ The ``expected_error_code`` argument in the ``rbac_rule_validation.action``
+ decorator has been deprecated in favor of ``expected_error_codes``.