Refactor role list subcommand for identity v3 api

Currently parts of user list and group list command are actually
functioning as role listing, which is quite counter intuitive and
misleading.

This refactor change move role related logic to a single place of role
list command. It now allows role grants listing for user/group +
domain/project combinations.

If no user or group specified, it will list all roles in the system,
which is the default behaviour.

Change-Id: I4ced6df4b76f018d01000d28b4281ad9f252ffcc

openstackclient/identity/v3/group.py

@@ -176,23 +176,6 @@ class ListGroup(lister.Lister):
             nargs='?',
             help='Name or ID of group to list [required with --role]',
         )
-        parser.add_argument(
-            '--role',
-            action='store_true',
-            default=False,
-            help='List the roles assigned to <group>',
-        )
-        domain_or_project = parser.add_mutually_exclusive_group()
-        domain_or_project.add_argument(
-            '--domain',
-            metavar='<domain>',
-            help='Filter list by <domain> [Only valid with --role]',
-        )
-        domain_or_project.add_argument(
-            '--project',
-            metavar='<project>',
-            help='Filter list by <project> [Only valid with --role]',
-        )
         parser.add_argument(
             '--long',
             action='store_true',
@@ -205,65 +188,12 @@ class ListGroup(lister.Lister):
         self.log.debug('take_action(%s)', parsed_args)
         identity_client = self.app.client_manager.identity
 
-        if parsed_args.role:
-            # List roles belonging to group
-
-            # Group is required here, bail if it is not supplied
-            if not parsed_args.group:
-                sys.stderr.write('Error: Group must be specified')
-                # TODO(dtroyer): This lists the commands...I want it to
-                # show the help for _this_ command.
-                self.app.DeferredHelpAction(
-                    self.app.parser,
-                    self.app.parser,
-                    None,
-                    None,
-                )
-                return ([], [])
-
-            group = utils.find_resource(
-                identity_client.groups,
-                parsed_args.group,
-            )
-
-            if parsed_args.domain:
-                columns = ('ID', 'Name', 'Domain', 'Group')
-                domain = utils.find_resource(
-                    identity_client.domains,
-                    parsed_args.domain,
-                )
-                data = identity_client.roles.list(
-                    group=group,
-                    domain=domain,
-                )
-                for group_role in data:
-                    group_role.group = group.name
-                    group_role.domain = domain.name
-            elif parsed_args.project:
-                columns = ('ID', 'Name', 'Project', 'Group')
-                project = utils.find_resource(
-                    identity_client.projects,
-                    parsed_args.project,
-                )
-                data = identity_client.roles.list(
-                    group=group,
-                    project=project,
-                )
-                for group_role in data:
-                    group_role.group = group.name
-                    group_role.project = project.name
-            else:
-                # TODO(dtroyer): raise exception here, this really is an error
-                sys.stderr.write("Error: Must specify --domain or --project "
-                                 "with --role\n")
-                return ([], [])
+        # List groups
+        if parsed_args.long:
+            columns = ('ID', 'Name', 'Domain ID', 'Description')
         else:
-            # List groups
-            if parsed_args.long:
-                columns = ('ID', 'Name', 'Domain ID', 'Description')
-            else:
-                columns = ('ID', 'Name')
-            data = identity_client.groups.list()
+            columns = ('ID', 'Name')
+        data = identity_client.groups.list()
 
         return (columns,
                 (utils.get_item_properties(

openstackclient/identity/v3/role.py

@@ -192,10 +192,110 @@ class ListRole(lister.Lister):
 
     log = logging.getLogger(__name__ + '.ListRole')
 
+    def get_parser(self, prog_name):
+        parser = super(ListRole, self).get_parser(prog_name)
+        domain_or_project = parser.add_mutually_exclusive_group()
+        domain_or_project.add_argument(
+            '--domain',
+            metavar='<domain>',
+            help='Filter role list by <domain>',
+        )
+        domain_or_project.add_argument(
+            '--project',
+            metavar='<project>',
+            help='Filter role list by <project>',
+        )
+        user_or_group = parser.add_mutually_exclusive_group()
+        user_or_group.add_argument(
+            '--user',
+            metavar='<user>',
+            help='Name or ID of user to list roles asssigned to',
+        )
+        user_or_group.add_argument(
+            '--group',
+            metavar='<group>',
+            help='Name or ID of group to list roles asssigned to',
+        )
+        return parser
+
     def take_action(self, parsed_args):
-        self.log.debug('take_action(%s)', parsed_args)
-        columns = ('ID', 'Name')
-        data = self.app.client_manager.identity.roles.list()
+        self.log.debug('take_action(%s)' % parsed_args)
+        identity_client = self.app.client_manager.identity
+
+        if parsed_args.user:
+            user = utils.find_resource(
+                identity_client.users,
+                parsed_args.user,
+            )
+        elif parsed_args.group:
+            group = utils.find_resource(
+                identity_client.groups,
+                parsed_args.group,
+            )
+
+        if parsed_args.domain:
+            domain = utils.find_resource(
+                identity_client.domains,
+                parsed_args.domain,
+            )
+        elif parsed_args.project:
+            project = utils.find_resource(
+                identity_client.projects,
+                parsed_args.project,
+            )
+
+        # no user or group specified, list all roles in the system
+        if not parsed_args.user and not parsed_args.group:
+            columns = ('ID', 'Name')
+            data = identity_client.roles.list()
+        elif parsed_args.user and parsed_args.domain:
+            columns = ('ID', 'Name', 'Domain', 'User')
+            data = identity_client.roles.list(
+                user=user,
+                domain=domain,
+            )
+            for user_role in data:
+                user_role.user = user.name
+                user_role.domain = domain.name
+        elif parsed_args.user and parsed_args.project:
+            columns = ('ID', 'Name', 'Project', 'User')
+            data = identity_client.roles.list(
+                user=user,
+                project=project,
+            )
+            for user_role in data:
+                user_role.user = user.name
+                user_role.project = project.name
+        elif parsed_args.user:
+            columns = ('ID', 'Name')
+            data = identity_client.roles.list(
+                user=user,
+                domain='default',
+            )
+        elif parsed_args.group and parsed_args.domain:
+            columns = ('ID', 'Name', 'Domain', 'Group')
+            data = identity_client.roles.list(
+                group=group,
+                domain=domain,
+            )
+            for group_role in data:
+                group_role.group = group.name
+                group_role.domain = domain.name
+        elif parsed_args.group and parsed_args.project:
+            columns = ('ID', 'Name', 'Project', 'Group')
+            data = identity_client.roles.list(
+                group=group,
+                project=project,
+            )
+            for group_role in data:
+                group_role.group = group.name
+                group_role.project = project.name
+        else:
+            sys.stderr.write("Error: If a user or group is specified, either "
+                             "--domain or --project must also be specified to "
+                             "list role grants.\n")
+            return ([], [])
+
         return (columns,
                 (utils.get_item_properties(
                     s, columns,

openstackclient/identity/v3/user.py

@@ -17,7 +17,6 @@
 
 import logging
 import six
-import sys
 
 from cliff import command
 from cliff import lister
@@ -161,23 +160,6 @@ class ListUser(lister.Lister):
             nargs='?',
             help='Name or ID of user to list [required with --role]',
         )
-        parser.add_argument(
-            '--role',
-            action='store_true',
-            default=False,
-            help='List the roles assigned to <user>',
-        )
-        domain_or_project = parser.add_mutually_exclusive_group()
-        domain_or_project.add_argument(
-            '--domain',
-            metavar='<domain>',
-            help='Filter list by <domain> [Only valid with --role]',
-        )
-        domain_or_project.add_argument(
-            '--project',
-            metavar='<project>',
-            help='Filter list by <project> [Only valid with --role]',
-        )
         parser.add_argument(
             '--long',
             action='store_true',
@@ -187,70 +169,15 @@ class ListUser(lister.Lister):
         return parser
 
     def take_action(self, parsed_args):
-        self.log.debug('take_action(%s)', parsed_args)
-        identity_client = self.app.client_manager.identity
+        self.log.debug('take_action(%s)' % parsed_args)
 
-        if parsed_args.role:
-            # List roles belonging to user
-
-            # User is required here, bail if it is not supplied
-            if not parsed_args.user:
-                sys.stderr.write('Error: User must be specified')
-                return ([], [])
-
-            user = utils.find_resource(
-                identity_client.users,
-                parsed_args.user,
-            )
-
-            # List a user's roles
-            if not parsed_args.domain and not parsed_args.project:
-                columns = ('ID', 'Name')
-                data = identity_client.roles.list(
-                    user=user,
-                    domain='default',
-                )
-            # List a user's roles on a domain
-            elif parsed_args.user and parsed_args.domain:
-                columns = ('ID', 'Name', 'Domain', 'User')
-                domain = utils.find_resource(
-                    identity_client.domains,
-                    parsed_args.domain,
-                )
-                data = identity_client.roles.list(
-                    user=user,
-                    domain=domain,
-                )
-                for user_role in data:
-                    user_role.user = user.name
-                    user_role.domain = domain.name
-            # List a user's roles on a project
-            elif parsed_args.user and parsed_args.project:
-                columns = ('ID', 'Name', 'Project', 'User')
-                project = utils.find_resource(
-                    identity_client.projects,
-                    parsed_args.project,
-                )
-                data = identity_client.roles.list(
-                    user=user,
-                    project=project,
-                )
-                for user_role in data:
-                    user_role.user = user.name
-                    user_role.project = project.name
-            else:
-                # TODO(dtroyer): raise exception here, this really is an error
-                sys.stderr.write("Error: Must specify --domain or --project "
-                                 "with --role\n")
-                return ([], [])
+        # List users
+        if parsed_args.long:
+            columns = ('ID', 'Name', 'Project Id', 'Domain Id',
+                       'Description', 'Email', 'Enabled')
         else:
-            # List users
-            if parsed_args.long:
-                columns = ('ID', 'Name', 'Project Id', 'Domain Id',
-                           'Description', 'Email', 'Enabled')
-            else:
-                columns = ('ID', 'Name')
-            data = self.app.client_manager.identity.users.list()
+            columns = ('ID', 'Name')
+        data = self.app.client_manager.identity.users.list()
 
         return (columns,
                 (utils.get_item_properties(

openstackclient/tests/identity/v3/test_role.py

@@ -296,6 +296,27 @@ class TestRoleList(TestRole):
             ),
         ]
 
+        self.domains_mock.get.return_value = fakes.FakeResource(
+            None,
+            copy.deepcopy(identity_fakes.DOMAIN),
+            loaded=True,
+        )
+        self.projects_mock.get.return_value = fakes.FakeResource(
+            None,
+            copy.deepcopy(identity_fakes.PROJECT),
+            loaded=True,
+        )
+        self.users_mock.get.return_value = fakes.FakeResource(
+            None,
+            copy.deepcopy(identity_fakes.USER),
+            loaded=True,
+        )
+        self.groups_mock.get.return_value = fakes.FakeResource(
+            None,
+            copy.deepcopy(identity_fakes.GROUP),
+            loaded=True,
+        )
+
         # Get the command object to test
         self.cmd = role.ListRole(self.app, None)
 
@@ -317,6 +338,172 @@ class TestRoleList(TestRole):
         ), )
         self.assertEqual(tuple(data), datalist)
 
+    def test_user_list_user(self):
+        arglist = [
+            '--user', identity_fakes.user_id,
+        ]
+        verifylist = [
+            ('user', identity_fakes.user_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # DisplayCommandBase.take_action() returns two tuples
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'domain': 'default',
+            'user': self.users_mock.get(),
+        }
+        # RoleManager.list(user=, group=, domain=, project=, **kwargs)
+        self.roles_mock.list.assert_called_with(
+            **kwargs
+        )
+
+        collist = ('ID', 'Name')
+        self.assertEqual(columns, collist)
+        datalist = ((
+            identity_fakes.role_id,
+            identity_fakes.role_name,
+        ), )
+        self.assertEqual(tuple(data), datalist)
+
+    def test_role_list_domain_user(self):
+        arglist = [
+            '--domain', identity_fakes.domain_name,
+            '--user', identity_fakes.user_id,
+        ]
+        verifylist = [
+            ('domain', identity_fakes.domain_name),
+            ('user', identity_fakes.user_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # DisplayCommandBase.take_action() returns two tuples
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'domain': self.domains_mock.get(),
+            'user': self.users_mock.get(),
+        }
+        # RoleManager.list(user=, group=, domain=, project=, **kwargs)
+        self.roles_mock.list.assert_called_with(
+            **kwargs
+        )
+
+        collist = ('ID', 'Name', 'Domain', 'User')
+        self.assertEqual(columns, collist)
+        datalist = ((
+            identity_fakes.role_id,
+            identity_fakes.role_name,
+            identity_fakes.domain_name,
+            identity_fakes.user_name,
+        ), )
+        self.assertEqual(tuple(data), datalist)
+
+    def test_role_list_domain_group(self):
+        arglist = [
+            '--domain', identity_fakes.domain_name,
+            '--group', identity_fakes.group_id,
+        ]
+        verifylist = [
+            ('domain', identity_fakes.domain_name),
+            ('group', identity_fakes.group_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # DisplayCommandBase.take_action() returns two tuples
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'domain': self.domains_mock.get(),
+            'group': self.groups_mock.get(),
+        }
+        # RoleManager.list(user=, group=, domain=, project=, **kwargs)
+        self.roles_mock.list.assert_called_with(
+            **kwargs
+        )
+
+        collist = ('ID', 'Name', 'Domain', 'Group')
+        self.assertEqual(columns, collist)
+        datalist = ((
+            identity_fakes.role_id,
+            identity_fakes.role_name,
+            identity_fakes.domain_name,
+            identity_fakes.group_name,
+        ), )
+        self.assertEqual(tuple(data), datalist)
+
+    def test_role_list_project_user(self):
+        arglist = [
+            '--project', identity_fakes.project_name,
+            '--user', identity_fakes.user_id,
+        ]
+        verifylist = [
+            ('project', identity_fakes.project_name),
+            ('user', identity_fakes.user_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # DisplayCommandBase.take_action() returns two tuples
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'project': self.projects_mock.get(),
+            'user': self.users_mock.get(),
+        }
+        # RoleManager.list(user=, group=, domain=, project=, **kwargs)
+        self.roles_mock.list.assert_called_with(
+            **kwargs
+        )
+
+        collist = ('ID', 'Name', 'Project', 'User')
+        self.assertEqual(columns, collist)
+        datalist = ((
+            identity_fakes.role_id,
+            identity_fakes.role_name,
+            identity_fakes.project_name,
+            identity_fakes.user_name,
+        ), )
+        self.assertEqual(tuple(data), datalist)
+
+    def test_role_list_project_group(self):
+        arglist = [
+            '--project', identity_fakes.project_name,
+            '--group', identity_fakes.group_id,
+        ]
+        verifylist = [
+            ('project', identity_fakes.project_name),
+            ('group', identity_fakes.group_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # DisplayCommandBase.take_action() returns two tuples
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'project': self.projects_mock.get(),
+            'group': self.groups_mock.get(),
+        }
+        # RoleManager.list(user=, group=, domain=, project=, **kwargs)
+        self.roles_mock.list.assert_called_with(
+            **kwargs
+        )
+
+        collist = ('ID', 'Name', 'Project', 'Group')
+        self.assertEqual(columns, collist)
+        datalist = ((
+            identity_fakes.role_id,
+            identity_fakes.role_name,
+            identity_fakes.project_name,
+            identity_fakes.group_name,
+        ), )
+        self.assertEqual(tuple(data), datalist)
+
 
 class TestRoleRemove(TestRole):
 

openstackclient/tests/identity/v3/test_user.py

@@ -476,33 +476,6 @@ class TestUserList(TestUser):
     def setUp(self):
         super(TestUserList, self).setUp()
 
-        self.domains_mock.get.return_value = fakes.FakeResource(
-            None,
-            copy.deepcopy(identity_fakes.DOMAIN),
-            loaded=True,
-        )
-
-        self.projects_mock.get.return_value = fakes.FakeResource(
-            None,
-            copy.deepcopy(identity_fakes.PROJECT),
-            loaded=True,
-        )
-        self.projects_mock.list.return_value = [
-            fakes.FakeResource(
-                None,
-                copy.deepcopy(identity_fakes.PROJECT),
-                loaded=True,
-            ),
-        ]
-
-        self.roles_mock.list.return_value = [
-            fakes.FakeResource(
-                None,
-                copy.deepcopy(identity_fakes.ROLE),
-                loaded=True,
-            ),
-        ]
-
         self.users_mock.get.return_value = fakes.FakeResource(
             None,
             copy.deepcopy(identity_fakes.USER),
@@ -537,154 +510,6 @@ class TestUserList(TestUser):
         ), )
         self.assertEqual(tuple(data), datalist)
 
-    def test_user_list_project(self):
-        arglist = [
-            '--project', identity_fakes.project_id,
-        ]
-        verifylist = [
-            ('project', identity_fakes.project_id),
-        ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        # DisplayCommandBase.take_action() returns two tuples
-        columns, data = self.cmd.take_action(parsed_args)
-
-        self.users_mock.list.assert_called_with()
-
-        collist = ('ID', 'Name')
-        self.assertEqual(columns, collist)
-        datalist = ((
-            identity_fakes.user_id,
-            identity_fakes.user_name,
-        ), )
-        self.assertEqual(tuple(data), datalist)
-
-    def test_user_list_domain(self):
-        arglist = [
-            '--domain', identity_fakes.domain_id,
-        ]
-        verifylist = [
-            ('domain', identity_fakes.domain_id),
-        ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        # DisplayCommandBase.take_action() returns two tuples
-        columns, data = self.cmd.take_action(parsed_args)
-
-        self.users_mock.list.assert_called_with()
-
-        collist = ('ID', 'Name')
-        self.assertEqual(columns, collist)
-        datalist = ((
-            identity_fakes.user_id,
-            identity_fakes.user_name,
-        ), )
-        self.assertEqual(tuple(data), datalist)
-
-    def test_user_list_role_user(self):
-        arglist = [
-            '--role',
-            identity_fakes.user_id,
-        ]
-        verifylist = [
-            ('role', True),
-            ('user', identity_fakes.user_id),
-        ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        # DisplayCommandBase.take_action() returns two tuples
-        columns, data = self.cmd.take_action(parsed_args)
-
-        # Set expected values
-        kwargs = {
-            'domain': 'default',
-            'user': self.users_mock.get(),
-        }
-        # RoleManager.list(user=, group=, domain=, project=, **kwargs)
-        self.roles_mock.list.assert_called_with(
-            **kwargs
-        )
-
-        collist = ('ID', 'Name')
-        self.assertEqual(columns, collist)
-        datalist = ((
-            identity_fakes.role_id,
-            identity_fakes.role_name,
-        ), )
-        self.assertEqual(tuple(data), datalist)
-
-    def test_user_list_role_domain(self):
-        arglist = [
-            '--domain', identity_fakes.domain_name,
-            '--role',
-            identity_fakes.user_id,
-        ]
-        verifylist = [
-            ('domain', identity_fakes.domain_name),
-            ('role', True),
-            ('user', identity_fakes.user_id),
-        ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        # DisplayCommandBase.take_action() returns two tuples
-        columns, data = self.cmd.take_action(parsed_args)
-
-        # Set expected values
-        kwargs = {
-            'domain': self.domains_mock.get(),
-            'user': self.users_mock.get(),
-        }
-        # RoleManager.list(user=, group=, domain=, project=, **kwargs)
-        self.roles_mock.list.assert_called_with(
-            **kwargs
-        )
-
-        collist = ('ID', 'Name', 'Domain', 'User')
-        self.assertEqual(columns, collist)
-        datalist = ((
-            identity_fakes.role_id,
-            identity_fakes.role_name,
-            identity_fakes.domain_name,
-            identity_fakes.user_name,
-        ), )
-        self.assertEqual(tuple(data), datalist)
-
-    def test_user_list_role_project(self):
-        arglist = [
-            '--project', identity_fakes.project_name,
-            '--role',
-            identity_fakes.user_id,
-        ]
-        verifylist = [
-            ('project', identity_fakes.project_name),
-            ('role', True),
-            ('user', identity_fakes.user_id),
-        ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        # DisplayCommandBase.take_action() returns two tuples
-        columns, data = self.cmd.take_action(parsed_args)
-
-        # Set expected values
-        kwargs = {
-            'project': self.projects_mock.get(),
-            'user': self.users_mock.get(),
-        }
-        # RoleManager.list(user=, group=, domain=, project=, **kwargs)
-        self.roles_mock.list.assert_called_with(
-            **kwargs
-        )
-
-        collist = ('ID', 'Name', 'Project', 'User')
-        self.assertEqual(columns, collist)
-        datalist = ((
-            identity_fakes.role_id,
-            identity_fakes.role_name,
-            identity_fakes.project_name,
-            identity_fakes.user_name,
-        ), )
-        self.assertEqual(tuple(data), datalist)
-
     def test_user_list_long(self):
         arglist = [
             '--long',