From e24673267093de85beee753860cda1fb224ce4bc Mon Sep 17 00:00:00 2001 From: Lance Bragstad Date: Thu, 9 Jul 2020 17:07:52 -0500 Subject: [PATCH] Bypass user and group verification in RemoveRole Keystone let's users remove role assignments that reference non-existent users and groups. This is nice when keystone backs to an identity store like LDAP and users or groups are removed. Previously, openstackclient would validate the user and group existed in keystone before sending the request to delete the role assignment. This commit updates the code to bypass that validation so that users can use IDs to forcibly cleanup role assignments. Change-Id: I102b41677736bbe37a82abaa3c5b3e1faf2475d5 Story: 2006635 Task: 36848 --- openstackclient/identity/v3/role.py | 68 ++--- .../tests/unit/identity/v3/test_role.py | 242 ++++++++++++++++++ .../notes/bug-2006635-3110f7a87a186e62.yaml | 7 + 3 files changed, 285 insertions(+), 32 deletions(-) create mode 100644 releasenotes/notes/bug-2006635-3110f7a87a186e62.yaml diff --git a/openstackclient/identity/v3/role.py b/openstackclient/identity/v3/role.py index 980ebf1165..a674564fe0 100644 --- a/openstackclient/identity/v3/role.py +++ b/openstackclient/identity/v3/role.py @@ -64,59 +64,61 @@ def _add_identity_and_resource_options_to_parser(parser): def _process_identity_and_resource_options(parsed_args, - identity_client_manager): + identity_client_manager, + validate_actor_existence=True): + + def _find_user(): + try: + return common.find_user( + identity_client_manager, + parsed_args.user, + parsed_args.user_domain + ).id + except exceptions.CommandError: + if not validate_actor_existence: + return parsed_args.user + raise + + def _find_group(): + try: + return common.find_group( + identity_client_manager, + parsed_args.group, + parsed_args.group_domain + ).id + except exceptions.CommandError: + if not validate_actor_existence: + return parsed_args.group + raise + kwargs = {} if parsed_args.user and parsed_args.system: - kwargs['user'] = common.find_user( - identity_client_manager, - parsed_args.user, - parsed_args.user_domain, - ).id + kwargs['user'] = _find_user() kwargs['system'] = parsed_args.system elif parsed_args.user and parsed_args.domain: - kwargs['user'] = common.find_user( - identity_client_manager, - parsed_args.user, - parsed_args.user_domain, - ).id + kwargs['user'] = _find_user() kwargs['domain'] = common.find_domain( identity_client_manager, parsed_args.domain, ).id elif parsed_args.user and parsed_args.project: - kwargs['user'] = common.find_user( - identity_client_manager, - parsed_args.user, - parsed_args.user_domain, - ).id + kwargs['user'] = _find_user() kwargs['project'] = common.find_project( identity_client_manager, parsed_args.project, parsed_args.project_domain, ).id elif parsed_args.group and parsed_args.system: - kwargs['group'] = common.find_group( - identity_client_manager, - parsed_args.group, - parsed_args.group_domain, - ).id + kwargs['group'] = _find_group() kwargs['system'] = parsed_args.system elif parsed_args.group and parsed_args.domain: - kwargs['group'] = common.find_group( - identity_client_manager, - parsed_args.group, - parsed_args.group_domain, - ).id + kwargs['group'] = _find_group() kwargs['domain'] = common.find_domain( identity_client_manager, parsed_args.domain, ).id elif parsed_args.group and parsed_args.project: - kwargs['group'] = common.find_group( - identity_client_manager, - parsed_args.group, - parsed_args.group_domain, - ).id + kwargs['group'] = _find_group() kwargs['project'] = common.find_project( identity_client_manager, parsed_args.project, @@ -340,7 +342,9 @@ class RemoveRole(command.Command): ) kwargs = _process_identity_and_resource_options( - parsed_args, self.app.client_manager.identity) + parsed_args, self.app.client_manager.identity, + validate_actor_existence=False + ) identity_client.roles.revoke(role.id, **kwargs) diff --git a/openstackclient/tests/unit/identity/v3/test_role.py b/openstackclient/tests/unit/identity/v3/test_role.py index c1e91f9a0f..774b2c2b5f 100644 --- a/openstackclient/tests/unit/identity/v3/test_role.py +++ b/openstackclient/tests/unit/identity/v3/test_role.py @@ -19,6 +19,7 @@ from unittest import mock from osc_lib import exceptions from osc_lib import utils +from openstackclient.identity import common from openstackclient.identity.v3 import role from openstackclient.tests.unit import fakes from openstackclient.tests.unit.identity.v3 import fakes as identity_fakes @@ -846,6 +847,47 @@ class TestRoleRemove(TestRole): ) self.assertIsNone(result) + @mock.patch.object(common, 'find_user') + def test_role_remove_non_existent_user_system(self, find_mock): + # Simulate the user not being in keystone, the client should gracefully + # handle this exception and send the request to remove the role since + # keystone supports removing role assignments with non-existent actors + # (e.g., users or groups). + find_mock.side_effect = exceptions.CommandError + + arglist = [ + '--user', identity_fakes.user_id, + '--system', 'all', + identity_fakes.role_name + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', identity_fakes.user_id), + ('group', None), + ('system', 'all'), + ('domain', None), + ('project', None), + ('role', identity_fakes.role_name), + ('inherited', self._is_inheritance_testcase()), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = { + 'user': identity_fakes.user_id, + 'system': 'all', + 'os_inherit_extension_inherited': self._is_inheritance_testcase(), + } + # RoleManager.revoke(role, user=, group=, domain=, project=) + self.roles_mock.revoke.assert_called_with( + identity_fakes.role_id, + **kwargs + ) + self.assertIsNone(result) + def test_role_remove_user_domain(self): arglist = [ '--user', identity_fakes.user_name, @@ -879,6 +921,46 @@ class TestRoleRemove(TestRole): ) self.assertIsNone(result) + @mock.patch.object(common, 'find_user') + def test_role_remove_non_existent_user_domain(self, find_mock): + # Simulate the user not being in keystone, the client the gracefully + # handle this exception and send the request to remove the role since + # keystone will validate. + find_mock.side_effect = exceptions.CommandError + + arglist = [ + '--user', identity_fakes.user_id, + '--domain', identity_fakes.domain_name, + identity_fakes.role_name + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', identity_fakes.user_id), + ('group', None), + ('system', None), + ('domain', identity_fakes.domain_name), + ('project', None), + ('role', identity_fakes.role_name), + ('inherited', self._is_inheritance_testcase()), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = { + 'user': identity_fakes.user_id, + 'domain': identity_fakes.domain_id, + 'os_inherit_extension_inherited': self._is_inheritance_testcase(), + } + # RoleManager.revoke(role, user=, group=, domain=, project=) + self.roles_mock.revoke.assert_called_with( + identity_fakes.role_id, + **kwargs + ) + self.assertIsNone(result) + def test_role_remove_user_project(self): arglist = [ '--user', identity_fakes.user_name, @@ -912,6 +994,46 @@ class TestRoleRemove(TestRole): ) self.assertIsNone(result) + @mock.patch.object(common, 'find_user') + def test_role_remove_non_existent_user_project(self, find_mock): + # Simulate the user not being in keystone, the client the gracefully + # handle this exception and send the request to remove the role since + # keystone will validate. + find_mock.side_effect = exceptions.CommandError + + arglist = [ + '--user', identity_fakes.user_id, + '--project', identity_fakes.project_name, + identity_fakes.role_name + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', identity_fakes.user_id), + ('group', None), + ('system', None), + ('domain', None), + ('project', identity_fakes.project_name), + ('role', identity_fakes.role_name), + ('inherited', self._is_inheritance_testcase()), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = { + 'user': identity_fakes.user_id, + 'project': identity_fakes.project_id, + 'os_inherit_extension_inherited': self._is_inheritance_testcase(), + } + # RoleManager.revoke(role, user=, group=, domain=, project=) + self.roles_mock.revoke.assert_called_with( + identity_fakes.role_id, + **kwargs + ) + self.assertIsNone(result) + def test_role_remove_group_system(self): arglist = [ '--group', identity_fakes.group_name, @@ -947,6 +1069,46 @@ class TestRoleRemove(TestRole): ) self.assertIsNone(result) + @mock.patch.object(common, 'find_group') + def test_role_remove_non_existent_group_system(self, find_mock): + # Simulate the user not being in keystone, the client the gracefully + # handle this exception and send the request to remove the role since + # keystone will validate. + find_mock.side_effect = exceptions.CommandError + + arglist = [ + '--group', identity_fakes.group_id, + '--system', 'all', + identity_fakes.role_name + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', None), + ('group', identity_fakes.group_id), + ('system', 'all'), + ('domain', None), + ('project', None), + ('role', identity_fakes.role_name), + ('inherited', self._is_inheritance_testcase()), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = { + 'group': identity_fakes.group_id, + 'system': 'all', + 'os_inherit_extension_inherited': self._is_inheritance_testcase(), + } + # RoleManager.revoke(role, user=, group=, domain=, project=) + self.roles_mock.revoke.assert_called_with( + identity_fakes.role_id, + **kwargs + ) + self.assertIsNone(result) + def test_role_remove_group_domain(self): arglist = [ '--group', identity_fakes.group_name, @@ -981,6 +1143,46 @@ class TestRoleRemove(TestRole): ) self.assertIsNone(result) + @mock.patch.object(common, 'find_group') + def test_role_remove_non_existent_group_domain(self, find_mock): + # Simulate the user not being in keystone, the client the gracefully + # handle this exception and send the request to remove the role since + # keystone will validate. + find_mock.side_effect = exceptions.CommandError + + arglist = [ + '--group', identity_fakes.group_id, + '--domain', identity_fakes.domain_name, + identity_fakes.role_name + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', None), + ('group', identity_fakes.group_id), + ('system', None), + ('domain', identity_fakes.domain_name), + ('project', None), + ('role', identity_fakes.role_name), + ('inherited', self._is_inheritance_testcase()), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = { + 'group': identity_fakes.group_id, + 'domain': identity_fakes.domain_id, + 'os_inherit_extension_inherited': self._is_inheritance_testcase(), + } + # RoleManager.revoke(role, user=, group=, domain=, project=) + self.roles_mock.revoke.assert_called_with( + identity_fakes.role_id, + **kwargs + ) + self.assertIsNone(result) + def test_role_remove_group_project(self): arglist = [ '--group', identity_fakes.group_name, @@ -1014,6 +1216,46 @@ class TestRoleRemove(TestRole): ) self.assertIsNone(result) + @mock.patch.object(common, 'find_group') + def test_role_remove_non_existent_group_project(self, find_mock): + # Simulate the user not being in keystone, the client the gracefully + # handle this exception and send the request to remove the role since + # keystone will validate. + find_mock.side_effect = exceptions.CommandError + + arglist = [ + '--group', identity_fakes.group_id, + '--project', identity_fakes.project_name, + identity_fakes.role_name + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', None), + ('group', identity_fakes.group_id), + ('system', None), + ('domain', None), + ('project', identity_fakes.project_name), + ('role', identity_fakes.role_name), + ('inherited', self._is_inheritance_testcase()), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = { + 'group': identity_fakes.group_id, + 'project': identity_fakes.project_id, + 'os_inherit_extension_inherited': self._is_inheritance_testcase(), + } + # RoleManager.revoke(role, user=, group=, domain=, project=) + self.roles_mock.revoke.assert_called_with( + identity_fakes.role_id, + **kwargs + ) + self.assertIsNone(result) + def test_role_remove_domain_role_on_group_domain(self): self.roles_mock.get.return_value = fakes.FakeResource( None, diff --git a/releasenotes/notes/bug-2006635-3110f7a87a186e62.yaml b/releasenotes/notes/bug-2006635-3110f7a87a186e62.yaml new file mode 100644 index 0000000000..ecc58c8a3b --- /dev/null +++ b/releasenotes/notes/bug-2006635-3110f7a87a186e62.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + You can now remove role assignments from keystone that reference non-existent + users or groups. + + [Bug `2006635 `_]