openstack/python-openstackclient
Code Issues Proposed changes

Bypass user and group verification in RemoveRole

Browse Source 
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
This commit is contained in:
Lance Bragstad 2020-07-09 17:07:52 -05:00
parent a8aad9fec8
commit e246732670
3 changed files with 285 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
openstackclient/identity/v3/role.py
View File

@ -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)

242
openstackclient/tests/unit/identity/v3/test_role.py
View File

@ -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,

7
releasenotes/notes/bug-2006635-3110f7a87a186e62.yaml Normal file
View File

@ -0,0 +1,7 @@
---
fixes:
- |
You can now remove role assignments from keystone that reference non-existent
users or groups.
[Bug `2006635 <https://storyboard.openstack.org/#!/story/2006635>`_]