From 2e491191e56dba338e7604cbc0c45b7658d4ee7a Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Fri, 25 Oct 2024 18:27:48 +0900 Subject: [PATCH] Fix ignored --user-domain in role assignment list Fix the wrong value assignment which made the --user-domain option ignored. Unit tests are updated to verify usage of domain options to avoid further regressions. Also drop the redundant look up of domain id to avoid unnecessary API call. Closes-Bug: #2085604 Change-Id: I5112b8e831fb26eb6544615277f0d3fe4f15dc5a --- openstackclient/identity/common.py | 56 ++-- .../identity/v3/role_assignment.py | 2 +- .../unit/identity/v3/test_role_assignment.py | 261 ++++++++++++++++++ 3 files changed, 284 insertions(+), 35 deletions(-) diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py index b8a5b885fe..9f4402f5b5 100644 --- a/openstackclient/identity/common.py +++ b/openstackclient/identity/common.py @@ -184,13 +184,6 @@ def _get_token_resource(client, resource, parsed_name, parsed_domain=None): return parsed_name -def _get_domain_id_if_requested(identity_client, domain_name_or_id): - if not domain_name_or_id: - return None - domain = find_domain(identity_client, domain_name_or_id) - return domain.id - - def find_domain(identity_client, name_or_id): return _find_identity_resource( identity_client.domains, name_or_id, domains.Domain @@ -198,48 +191,43 @@ def find_domain(identity_client, name_or_id): def find_group(identity_client, name_or_id, domain_name_or_id=None): - domain_id = _get_domain_id_if_requested(identity_client, domain_name_or_id) - if not domain_id: + if domain_name_or_id is None: return _find_identity_resource( identity_client.groups, name_or_id, groups.Group ) - else: - domain_id = find_domain(identity_client, domain_id).id - return _find_identity_resource( - identity_client.groups, - name_or_id, - groups.Group, - domain_id=domain_id, - ) + + domain_id = find_domain(identity_client, domain_name_or_id).id + return _find_identity_resource( + identity_client.groups, + name_or_id, + groups.Group, + domain_id=domain_id, + ) def find_project(identity_client, name_or_id, domain_name_or_id=None): - domain_id = _get_domain_id_if_requested(identity_client, domain_name_or_id) - if not domain_id: + if domain_name_or_id is None: return _find_identity_resource( identity_client.projects, name_or_id, projects.Project ) - else: - domain_id = find_domain(identity_client, domain_id).id - return _find_identity_resource( - identity_client.projects, - name_or_id, - projects.Project, - domain_id=domain_id, - ) + domain_id = find_domain(identity_client, domain_name_or_id).id + return _find_identity_resource( + identity_client.projects, + name_or_id, + projects.Project, + domain_id=domain_id, + ) def find_user(identity_client, name_or_id, domain_name_or_id=None): - domain_id = _get_domain_id_if_requested(identity_client, domain_name_or_id) - if not domain_id: + if domain_name_or_id is None: return _find_identity_resource( identity_client.users, name_or_id, users.User ) - else: - domain_id = find_domain(identity_client, domain_id).id - return _find_identity_resource( - identity_client.users, name_or_id, users.User, domain_id=domain_id - ) + domain_id = find_domain(identity_client, domain_name_or_id).id + return _find_identity_resource( + identity_client.users, name_or_id, users.User, domain_id=domain_id + ) def _find_identity_resource( diff --git a/openstackclient/identity/v3/role_assignment.py b/openstackclient/identity/v3/role_assignment.py index 7d4dfcdb06..12e1527b67 100644 --- a/openstackclient/identity/v3/role_assignment.py +++ b/openstackclient/identity/v3/role_assignment.py @@ -148,7 +148,7 @@ class ListRoleAssignment(command.Lister): user_domain_id = None if parsed_args.user_domain: - project_domain_id = _find_sdk_id( + user_domain_id = _find_sdk_id( identity_client.find_domain, name_or_id=parsed_args.user_domain, ) diff --git a/openstackclient/tests/unit/identity/v3/test_role_assignment.py b/openstackclient/tests/unit/identity/v3/test_role_assignment.py index ec7c7da560..65dad0d775 100644 --- a/openstackclient/tests/unit/identity/v3/test_role_assignment.py +++ b/openstackclient/tests/unit/identity/v3/test_role_assignment.py @@ -165,7 +165,75 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): arglist = ['--user', self.user.name] verifylist = [ ('user', self.user.name), + ('user_domain', None), ('group', None), + ('group_domain', None), + ('system', None), + ('domain', None), + ('project', None), + ('project_domain', None), + ('role', None), + ('effective', None), + ('inherited', False), + ('names', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # In base command class Lister in cliff, abstract method take_action() + # returns a tuple containing the column names and an iterable + # containing the data to be listed. + columns, data = self.cmd.take_action(parsed_args) + + self.identity_sdk_client.find_user.assert_called_with( + name_or_id=self.user.name, ignore_missing=False, domain_id=None + ) + self.identity_sdk_client.role_assignments.assert_called_with( + role_id=None, + user_id=self.user.id, + group_id=None, + scope_project_id=None, + scope_domain_id=None, + scope_system=None, + effective=None, + include_names=None, + inherited_to=None, + ) + + self.assertEqual(self.columns, columns) + datalist = ( + ( + self.role.id, + self.user.id, + '', + '', + self.domain.id, + '', + False, + ), + ( + self.role.id, + self.user.id, + '', + self.project.id, + '', + '', + False, + ), + ) + self.assertEqual(datalist, tuple(data)) + + def test_role_assignment_list_user_with_domain(self): + self.identity_sdk_client.role_assignments.return_value = [ + self.assignment_with_domain_id_and_user_id, + self.assignment_with_project_id_and_user_id, + ] + + arglist = ['--user', self.user.name, '--user-domain', self.domain.name] + verifylist = [ + ('user', self.user.name), + ('user_domain', self.domain.name), + ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), @@ -181,6 +249,14 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): # containing the data to be listed. columns, data = self.cmd.take_action(parsed_args) + self.identity_sdk_client.find_domain.assert_called_with( + name_or_id=self.domain.name, ignore_missing=False + ) + self.identity_sdk_client.find_user.assert_called_with( + name_or_id=self.user.name, + ignore_missing=False, + domain_id=self.domain.id, + ) self.identity_sdk_client.role_assignments.assert_called_with( role_id=None, user_id=self.user.id, @@ -230,10 +306,13 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): arglist = ['--user', self.user.id] verifylist = [ ('user', self.user.id), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -290,10 +369,13 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): arglist = ['--group', self.group.name] verifylist = [ ('user', None), + ('user_domain', None), ('group', self.group.name), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -306,6 +388,85 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): # containing the data to be listed. columns, data = self.cmd.take_action(parsed_args) + self.identity_sdk_client.find_group.assert_called_with( + name_or_id=self.group.name, ignore_missing=False, domain_id=None + ) + self.identity_sdk_client.role_assignments.assert_called_with( + role_id=None, + user_id=None, + group_id=self.group.id, + scope_project_id=None, + scope_domain_id=None, + scope_system=None, + effective=None, + include_names=None, + inherited_to=None, + ) + + self.assertEqual(self.columns, columns) + datalist = ( + ( + self.role.id, + '', + self.group.id, + '', + self.domain.id, + '', + False, + ), + ( + self.role.id, + '', + self.group.id, + self.project.id, + '', + '', + False, + ), + ) + self.assertEqual(datalist, tuple(data)) + + def test_role_assignment_list_group_with_domain(self): + self.identity_sdk_client.role_assignments.return_value = [ + self.assignment_with_domain_id_and_group_id, + self.assignment_with_project_id_and_group_id, + ] + + arglist = [ + '--group', + self.group.name, + '--group-domain', + self.domain.name, + ] + verifylist = [ + ('user', None), + ('user_domain', None), + ('group', self.group.name), + ('group_domain', self.domain.name), + ('system', None), + ('domain', None), + ('project', None), + ('project_domain', None), + ('role', None), + ('effective', None), + ('inherited', False), + ('names', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # In base command class Lister in cliff, abstract method take_action() + # returns a tuple containing the column names and an iterable + # containing the data to be listed. + columns, data = self.cmd.take_action(parsed_args) + + self.identity_sdk_client.find_domain.assert_called_with( + name_or_id=self.domain.name, ignore_missing=False + ) + self.identity_sdk_client.find_group.assert_called_with( + name_or_id=self.group.name, + ignore_missing=False, + domain_id=self.domain.id, + ) self.identity_sdk_client.role_assignments.assert_called_with( role_id=None, user_id=None, @@ -350,10 +511,13 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): arglist = ['--domain', self.domain.name] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', self.domain.name), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -410,10 +574,13 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): arglist = ['--project', self.project.name] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', self.project.name), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -426,6 +593,85 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): # containing the data to be listed. columns, data = self.cmd.take_action(parsed_args) + self.identity_sdk_client.find_project.assert_called_with( + name_or_id=self.project.name, ignore_missing=False, domain_id=None + ) + self.identity_sdk_client.role_assignments.assert_called_with( + role_id=None, + user_id=None, + group_id=None, + scope_project_id=self.project.id, + scope_domain_id=None, + scope_system=None, + effective=None, + include_names=None, + inherited_to=None, + ) + + self.assertEqual(self.columns, columns) + datalist = ( + ( + self.role.id, + self.user.id, + '', + self.project.id, + '', + '', + False, + ), + ( + self.role.id, + '', + self.group.id, + self.project.id, + '', + '', + False, + ), + ) + self.assertEqual(datalist, tuple(data)) + + def test_role_assignment_list_project_with_domain(self): + self.identity_sdk_client.role_assignments.return_value = [ + self.assignment_with_project_id_and_user_id, + self.assignment_with_project_id_and_group_id, + ] + + arglist = [ + '--project', + self.project.name, + '--project-domain', + self.domain.name, + ] + verifylist = [ + ('user', None), + ('user_domain', None), + ('group', None), + ('group_domain', None), + ('system', None), + ('domain', None), + ('project', self.project.name), + ('project_domain', self.domain.name), + ('role', None), + ('effective', None), + ('inherited', False), + ('names', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # In base command class Lister in cliff, abstract method take_action() + # returns a tuple containing the column names and an iterable + # containing the data to be listed. + columns, data = self.cmd.take_action(parsed_args) + + self.identity_sdk_client.find_domain.assert_called_with( + name_or_id=self.domain.name, ignore_missing=False + ) + self.identity_sdk_client.find_project.assert_called_with( + name_or_id=self.project.name, + ignore_missing=False, + domain_id=self.domain.id, + ) self.identity_sdk_client.role_assignments.assert_called_with( role_id=None, user_id=None, @@ -476,10 +722,13 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): ] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -529,10 +778,13 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): arglist = ['--effective'] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', True), ('inherited', False), @@ -611,10 +863,13 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): arglist = ['--inherited'] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', True), @@ -707,10 +962,13 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): arglist = ['--names'] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', None), ('effective', None), ('inherited', False), @@ -799,10 +1057,13 @@ class TestRoleAssignmentList(identity_fakes.TestIdentityv3): ] verifylist = [ ('user', None), + ('user_domain', None), ('group', None), + ('group_domain', None), ('system', None), ('domain', None), ('project', None), + ('project_domain', None), ('role', role_2.name), ('effective', None), ('inherited', False),