From 8dd5f1990183b7bb9ca44e98c2e9e6e6b1bd82af Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Fri, 5 Oct 2018 21:29:25 +0100 Subject: [PATCH] Remove invalid exception RbacConflictingPolicies This patch set drops RbacConflictingPolicies from Patrole because its intention is 1) flawed and 2) the way it is being used is invalid. First, there is no documented case where conflicting policies exists in OpenStack, so the intention behind the exception class is flawed. Second, the exception is only used in one test in test_server_rbac. It catches a ServerFault (500) and re-raises the exception is a pseudo-Forbidden exception. This is very bad because if Nova raises a 500 on a policy exception then this is a Nova bug. There is no valid use case where a ServerFault from Nova should ever be treated as a successful test scenario. Change-Id: I495b10ccf7b0df523b2551c49ea4db07e5bcaf79 --- patrole_tempest_plugin/rbac_exceptions.py | 5 --- .../rbac_rule_validation.py | 1 - .../tests/api/compute/test_server_rbac.py | 16 ++----- .../tests/unit/test_rbac_rule_validation.py | 42 ------------------- 4 files changed, 4 insertions(+), 60 deletions(-) diff --git a/patrole_tempest_plugin/rbac_exceptions.py b/patrole_tempest_plugin/rbac_exceptions.py index 3958e171..9130fb6a 100644 --- a/patrole_tempest_plugin/rbac_exceptions.py +++ b/patrole_tempest_plugin/rbac_exceptions.py @@ -20,11 +20,6 @@ class BasePatroleException(exceptions.TempestException): message = "An unknown RBAC exception occurred" -class RbacConflictingPolicies(BasePatroleException): - message = ("Conflicting policies preventing this action from being " - "performed.") - - class RbacMalformedResponse(BasePatroleException): message = ("The response body is missing the expected %(attribute)s due " "to policy enforcement failure.") diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py index d3b057cd..828a226d 100644 --- a/patrole_tempest_plugin/rbac_rule_validation.py +++ b/patrole_tempest_plugin/rbac_rule_validation.py @@ -198,7 +198,6 @@ def action(service, test_status = ('Error, %s' % (msg)) LOG.error(msg) except (expected_exception, - rbac_exceptions.RbacConflictingPolicies, rbac_exceptions.RbacMalformedResponse) as actual_exception: caught_exception = actual_exception test_status = 'Denied' diff --git a/patrole_tempest_plugin/tests/api/compute/test_server_rbac.py b/patrole_tempest_plugin/tests/api/compute/test_server_rbac.py index cae31f62..92ff87f8 100644 --- a/patrole_tempest_plugin/tests/api/compute/test_server_rbac.py +++ b/patrole_tempest_plugin/tests/api/compute/test_server_rbac.py @@ -21,9 +21,7 @@ from tempest import config from tempest.lib.common.utils import data_utils from tempest.lib.common.utils import test_utils from tempest.lib import decorators -from tempest.lib import exceptions -from patrole_tempest_plugin import rbac_exceptions from patrole_tempest_plugin import rbac_rule_validation from patrole_tempest_plugin.tests.api.compute import rbac_base as base @@ -182,13 +180,7 @@ class ComputeServersRbacTest(base.BaseV2ComputeRbacTest): def test_update_server(self): new_name = data_utils.rand_name(self.__class__.__name__ + '-server') with self.rbac_utils.override_role(self): - try: - self.servers_client.update_server(self.server['id'], - name=new_name) - waiters.wait_for_server_status(self.servers_client, - self.server['id'], 'ACTIVE') - except exceptions.ServerFault as e: - # Some other policy may have blocked it. - LOG.info("ServerFault exception caught. Some other policy " - "blocked updating of server") - raise rbac_exceptions.RbacConflictingPolicies(e) + self.servers_client.update_server(self.server['id'], + name=new_name) + waiters.wait_for_server_status(self.servers_client, + self.server['id'], 'ACTIVE') 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 17720473..c4cee5a8 100644 --- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py +++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py @@ -166,48 +166,6 @@ class RBACRuleValidationTest(BaseRBACRuleValidationTest): self.mock_test_args) self.assertRegex(mock_log.error.mock_calls[0][1][0], test_re) - @mock.patch.object(rbac_rv, 'LOG', autospec=True) - @mock.patch.object(rbac_rv, 'policy_authority', autospec=True) - def test_rule_validation_rbac_conflicting_policies_positive( - self, mock_authority, mock_log): - """Test RbacConflictingPolicies error is thrown without permission - passes. - - Positive test case: if RbacConflictingPolicies is thrown and the user - is not allowed to perform the action, then this is a success. - """ - mock_authority.PolicyAuthority.return_value.allowed.return_value =\ - False - - @rbac_rv.action(mock.sentinel.service, rules=[mock.sentinel.action]) - def test_policy(*args): - raise rbac_exceptions.RbacConflictingPolicies() - - mock_log.error.assert_not_called() - - @mock.patch.object(rbac_rv, 'LOG', autospec=True) - @mock.patch.object(rbac_rv, 'policy_authority', autospec=True) - def test_rule_validation_rbac_conflicting_policies_negative(self, - mock_authority, - mock_log): - """Test RbacConflictingPolicies error is thrown with permission fails. - - Negative test case: if RbacConflictingPolicies is thrown and the user - is allowed to perform the action, then this is an expected failure. - """ - mock_authority.PolicyAuthority.return_value.allowed.return_value = True - - @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( - 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) @mock.patch.object(rbac_rv, 'policy_authority', autospec=True) def test_expect_not_found_but_raises_forbidden(self, mock_authority,