From aaa836ea9b111b640f238054520565f9cf9422a1 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Mar 28 2024 23:57:06 +0000
Subject: [PATCH 1/5] address issues with list-users


Fixes: https://pagure.io/koji/issue/4050

---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index 7553890..9d70f85 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -8171,8 +8171,9 @@ def anon_handle_list_users(goptions, session, args):
     """[admin] List of users"""
     usage = "usage: %prog list-users [options]"
     parser = OptionParser(usage=get_usage_str(usage))
-    parser.add_option("--usertype", help="List users that have a given usertype "
-                                         "(e.g. NORMAL, HOST, GROUP)")
+    parser.add_option("--usertype", default="NORMAL",
+                      help='List users that have a given usertype (e.g. NORMAL, HOST, GROUP). '
+                      'Use "any" to get all types')
     parser.add_option("--prefix", help="List users that have a given prefix")
     parser.add_option("--perm", help="List users that have a given permission")
     parser.add_option("--inherited-perm", action='store_true', default=False,
@@ -8187,30 +8188,26 @@ def anon_handle_list_users(goptions, session, args):
 
     activate_session(session, goptions)
 
-    if options.usertype:
-        if options.usertype.upper() in koji.USERTYPES.keys():
-            usertype = koji.USERTYPES[options.usertype.upper()]
-        else:
-            error("Usertype %s doesn't exist" % options.usertype)
-    elif options.perm:
+    utype = options.usertype.upper()
+    if utype in koji.USERTYPES.keys():
+        usertype = koji.USERTYPES[utype]
+    elif utype in ("ANY", "ALL"):
         usertype = None
     else:
-        usertype = koji.USERTYPES['NORMAL']
+        error("Invalid usertype: %s" % options.usertype)
+
+    kwargs = {'userType': usertype}
 
     if options.prefix:
-        prefix = options.prefix
-    else:
-        prefix = None
+        kwargs['prefix'] = options.prefix
 
     if options.perm:
-        if options.perm in [p['name'] for p in session.getAllPerms()]:
-            perm = options.perm
-        else:
-            error("Permission %s does not exists" % options.perm)
-    else:
-        perm = None
+        if options.perm not in [p['name'] for p in session.getAllPerms()]:
+            error("Invalid permission: %s" % options.perm)
+        kwargs['perm'] = options.perm
+        if options.inherited_perm:
+            kwargs['inherited_perm'] = options.inherited_perm
 
-    users_list = session.listUsers(userType=usertype, prefix=prefix, perm=perm,
-                                   inherited_perm=options.inherited_perm)
+    users_list = session.listUsers(**kwargs)
     for user in users_list:
         print(user['name'])
diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py
index 1b53ea4..88fb94d 100644
--- a/kojihub/kojihub.py
+++ b/kojihub/kojihub.py
@@ -13119,28 +13119,37 @@ class RootExports(object):
 
     def listUsers(self, userType=koji.USERTYPES['NORMAL'], prefix=None, queryOpts=None, perm=None,
                   inherited_perm=False):
-        """List all users in the system.
-        userType can be an integer value from koji.USERTYPES (defaults to 0, i.e. normal users).
-        Returns a list of maps with the following keys:
+        """List users in the system
 
+        :param int|list|None userType: filter by type, defaults to normal users
+        :param str prefix: only users whose name starts with prefix. optional
+        :param dict queryOpts: query options. optional
+        :param str perm: only users that have this permission. optional
+        :param bool inherited_perm: consider inherited permissions. default: False
+        :returns: a list of matching user entries
+
+        The userType option can either by a single integer or a list. These integer values
+        correspond to the values from koji.USERTYPES (defaults to 0, i.e. normal users).
+
+        The entries in the returned list have the following keys:
         - id
         - name
         - status
         - usertype
         - krb_principals
-        - permissions
 
-        If no users of the specified type exist, return an empty list."""
+        If no users match, the list will be empty.
+        """
         if inherited_perm and not perm:
             raise koji.GenericError('inherited_perm option must be used with perm option')
         joins = []
-        if userType is None:
-            userType = list(koji.USERTYPES.values())
-        elif isinstance(userType, int):
-            userType = [userType]
-        else:
-            raise koji.ParameterError("userType must be integer or None")
-        clauses = ['usertype IN %(userType)s']
+        clauses = []
+        if userType is not None:
+            if isinstance(userType, int):
+                userType = [userType]
+            else:
+                raise koji.ParameterError("userType must be integer or None")
+            clauses.append('usertype IN %(userType)s')
         fields = [
             ('users.id', 'id'),
             ('users.name', 'name'),
@@ -13149,21 +13158,17 @@ class RootExports(object):
             ('array_agg(krb_principal)', 'krb_principals'),
         ]
         if perm:
-            fields.extend([
-                ('permissions.name', 'permission_name'),
-                ('permissions.id', 'permission_id'),
-            ])
-            clauses.extend(['user_perms.active AND permissions.name = %(perm)s'])
+            perm_id = get_perm_id(perm, strict=True)
+            clauses.extend(['user_perms.active AND user_perms.perm_id = %(perm_id)s'])
             if inherited_perm:
-                joins.extend(['LEFT JOIN user_groups ON user_id = users.id AND '
-                              'user_groups.active IS TRUE',
-                              'LEFT JOIN user_perms ON users.id = user_perms.user_id AND '
-                              'user_perms.active IS TRUE OR group_id =user_perms.user_id',
-                              'LEFT JOIN permissions ON perm_id = permissions.id'])
+                joins.extend([
+                    'LEFT JOIN user_groups ON user_id = users.id AND user_groups.active IS TRUE',
+                    # the active condition for user_groups must be in the join, otherwise we will
+                    # filter out users that never had a group
+                    'LEFT JOIN user_perms ON users.id = user_perms.user_id '
+                    'OR group_id = user_perms.user_id'])
             else:
-                joins.extend(['LEFT JOIN user_perms ON users.id = user_perms.user_id AND '
-                              'user_perms.active IS TRUE',
-                              'LEFT JOIN permissions ON perm_id = permissions.id'])
+                joins.append('LEFT JOIN user_perms ON users.id = user_perms.user_id')
         joins.append('LEFT JOIN user_krb_principals ON users.id = user_krb_principals.user_id')
         if prefix:
             clauses.append("users.name ilike %(prefix)s || '%%'")
@@ -13171,7 +13176,7 @@ class RootExports(object):
             queryOpts = {}
         if not queryOpts.get('group'):
             if perm:
-                queryOpts['group'] = 'users.id,permissions.id'
+                queryOpts['group'] = 'users.id,user_perms.perm_id'
             else:
                 queryOpts['group'] = 'users.id'
         else:

From 699ec1fb1450532ecbb92694eec72566f2aa2792 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Mar 28 2024 23:57:06 +0000
Subject: [PATCH 2/5] adjust help


---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index 9d70f85..c4eb79a 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -8177,7 +8177,7 @@ def anon_handle_list_users(goptions, session, args):
     parser.add_option("--prefix", help="List users that have a given prefix")
     parser.add_option("--perm", help="List users that have a given permission")
     parser.add_option("--inherited-perm", action='store_true', default=False,
-                      help="List of users that inherited specific perm")
+                      help="Consider inherited permissions")
     (options, args) = parser.parse_args(args)
 
     if len(args) > 0:

From bb1e0fe53d1f953bdc0b4ab3076c731e01546da7 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Mar 28 2024 23:57:06 +0000
Subject: [PATCH 3/5] warn if hub does not support --usertype any


---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index c4eb79a..29a47c3 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -8209,5 +8209,7 @@ def anon_handle_list_users(goptions, session, args):
             kwargs['inherited_perm'] = options.inherited_perm
 
     users_list = session.listUsers(**kwargs)
+    if not users_list and session.hub_version < (1, 34, 1) and usertype is None:
+        warn('This hub does not support querying for all usertypes')
     for user in users_list:
         print(user['name'])

From ed1a5545ac68c4ef76b8ae49c85f2a4f3bb2e0ed Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Mar 28 2024 23:57:06 +0000
Subject: [PATCH 4/5] fix unit tests


---

diff --git a/tests/test_cli/test_list_users.py b/tests/test_cli/test_list_users.py
index aed416a..2eb1516 100644
--- a/tests/test_cli/test_list_users.py
+++ b/tests/test_cli/test_list_users.py
@@ -46,8 +46,7 @@ testuser
 """
         self.assertMultiLineEqual(actual, expected)
         self.assertEqual(rv, None)
-        self.session.listUsers.assert_called_once_with(
-            inherited_perm=False, perm=None, userType=koji.USERTYPES['NORMAL'], prefix=None)
+        self.session.listUsers.assert_called_once_with(userType=koji.USERTYPES['NORMAL'])
         self.session.getAllPerms.assert_not_called()
 
     @mock.patch('sys.stdout', new_callable=StringIO)
@@ -65,8 +64,8 @@ testuser
 """
         self.assertMultiLineEqual(actual, expected)
         self.assertEqual(rv, None)
-        self.session.listUsers.assert_called_once_with(
-            inherited_perm=False, perm=None, userType=koji.USERTYPES['NORMAL'], prefix='koji')
+        self.session.listUsers.assert_called_once_with(userType=koji.USERTYPES['NORMAL'],
+                                                       prefix='koji')
         self.session.getAllPerms.assert_not_called()
 
     @mock.patch('sys.stdout', new_callable=StringIO)
@@ -89,8 +88,7 @@ testhost
 """
         self.assertMultiLineEqual(actual, expected)
         self.assertEqual(rv, None)
-        self.session.listUsers.assert_called_once_with(
-            inherited_perm=False, perm=None, userType=koji.USERTYPES['HOST'], prefix=None)
+        self.session.listUsers.assert_called_once_with(userType=koji.USERTYPES['HOST'])
         self.session.getAllPerms.assert_not_called()
 
     def test_list_users_with_usertype_non_existing(self):
@@ -99,7 +97,7 @@ testhost
             anon_handle_list_users,
             self.options, self.session, arguments,
             stdout='',
-            stderr="Usertype test doesn't exist\n",
+            stderr="Invalid usertype: test\n",
             activate_session=None,
             exit_code=1)
         self.session.listUsers.assert_not_called()
@@ -120,8 +118,8 @@ testhost
 """
         self.assertMultiLineEqual(actual, expected)
         self.assertEqual(rv, None)
-        self.session.listUsers.assert_called_once_with(
-            inherited_perm=False, perm=None, userType=koji.USERTYPES['HOST'], prefix='test')
+        self.session.listUsers.assert_called_once_with(userType=koji.USERTYPES['HOST'],
+                                                       prefix='test')
         self.session.getAllPerms.assert_not_called()
 
     def test_list_users_with_arg(self):
@@ -143,7 +141,7 @@ testhost
         self.assert_system_exit(
             anon_handle_list_users,
             self.options, self.session, arguments,
-            stderr="Permission test-non-exist-perm does not exists\n",
+            stderr="Invalid permission: test-non-exist-perm\n",
             stdout='',
             activate_session=None,
             exit_code=1)
@@ -157,14 +155,15 @@ testhost
         arguments = ['--perm', perm]
         self.session.getAllPerms.return_value = [{'name': 'test-perm'}, {'name': 'test-perm-2'}]
         self.session.listUsers.return_value = []
+        self.session.hub_version = (1, 34, 1)
         rv = anon_handle_list_users(self.options, self.session, arguments)
         actual = stdout.getvalue()
         expected = """"""
         self.assertMultiLineEqual(actual, expected)
         self.assertEqual(rv, None)
         self.activate_session.assert_called_once_with(self.session, self.options)
-        self.session.listUsers.assert_called_once_with(
-            inherited_perm=False, perm=perm, prefix=None, userType=None)
+        self.session.listUsers.assert_called_once_with(perm=perm,
+                                                       userType=koji.USERTYPES['NORMAL'])
         self.session.getAllPerms.assert_called_once_with()
 
     @mock.patch('sys.stdout', new_callable=StringIO)
@@ -193,7 +192,7 @@ testuser1234
         self.assertMultiLineEqual(actual, expected)
         self.assertEqual(rv, None)
         self.session.listUsers.assert_called_once_with(
-            inherited_perm=True, perm=perm, prefix=None, userType=None)
+            inherited_perm=True, perm=perm, userType=koji.USERTYPES['NORMAL'])
         self.session.getAllPerms.assert_called_once_with()
         self.activate_session.assert_called_once_with(self.session, self.options)
 
@@ -220,8 +219,8 @@ testuser1234
 Options:
   -h, --help           show this help message and exit
   --usertype=USERTYPE  List users that have a given usertype (e.g. NORMAL,
-                       HOST, GROUP)
+                       HOST, GROUP). Use "any" to get all types
   --prefix=PREFIX      List users that have a given prefix
   --perm=PERM          List users that have a given permission
-  --inherited-perm     List of users that inherited specific perm
+  --inherited-perm     Consider inherited permissions
 """ % self.progname)
diff --git a/tests/test_hub/test_list_users.py b/tests/test_hub/test_list_users.py
index 0295e45..739119e 100644
--- a/tests/test_hub/test_list_users.py
+++ b/tests/test_hub/test_list_users.py
@@ -16,6 +16,7 @@ class TestListUsers(unittest.TestCase):
         self.QueryProcessor = mock.patch('kojihub.kojihub.QueryProcessor',
                                          side_effect=self.getQuery).start()
         self.queries = []
+        self.get_perm_id = mock.patch('kojihub.kojihub.get_perm_id').start()
 
     def tearDown(self):
         mock.patch.stopall()
@@ -44,13 +45,11 @@ class TestListUsers(unittest.TestCase):
         query = self.queries[0]
         self.assertEqual(query.tables, ['users'])
         self.assertEqual(query.joins, [
-            'LEFT JOIN user_perms ON users.id = user_perms.user_id AND user_perms.active IS TRUE',
-            'LEFT JOIN permissions ON perm_id = permissions.id',
+            'LEFT JOIN user_perms ON users.id = user_perms.user_id',
             'LEFT JOIN user_krb_principals ON users.id = user_krb_principals.user_id'])
         self.assertEqual(query.clauses, [
-            'user_perms.active AND permissions.name = %(perm)s',
+            'user_perms.active AND user_perms.perm_id = %(perm_id)s',
             "users.name ilike %(prefix)s || '%%'",
-            'usertype IN %(userType)s',
         ])
 
     def test_valid_userType_none_with_perm_inherited_perm_and_prefix(self):
@@ -61,14 +60,12 @@ class TestListUsers(unittest.TestCase):
         self.assertEqual(query.tables, ['users'])
         self.assertEqual(query.joins, [
             'LEFT JOIN user_groups ON user_id = users.id AND user_groups.active IS TRUE',
-            'LEFT JOIN user_perms ON users.id = user_perms.user_id AND '
-            'user_perms.active IS TRUE OR group_id =user_perms.user_id',
-            'LEFT JOIN permissions ON perm_id = permissions.id',
+            'LEFT JOIN user_perms ON users.id = user_perms.user_id '
+                'OR group_id = user_perms.user_id',
             'LEFT JOIN user_krb_principals ON users.id = user_krb_principals.user_id'])
         self.assertEqual(query.clauses, [
-            'user_perms.active AND permissions.name = %(perm)s',
+            'user_perms.active AND user_perms.perm_id = %(perm_id)s',
             "users.name ilike %(prefix)s || '%%'",
-            'usertype IN %(userType)s',
         ])
 
     def test_inherited_perm_without_perm(self):

From b4af1895d775d884feae53e2ca7e0456d71e8cac Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Mar 28 2024 23:57:06 +0000
Subject: [PATCH 5/5] new cli tests for coverage


---

diff --git a/tests/test_cli/test_list_users.py b/tests/test_cli/test_list_users.py
index 2eb1516..dc7eff2 100644
--- a/tests/test_cli/test_list_users.py
+++ b/tests/test_cli/test_list_users.py
@@ -91,6 +91,42 @@ testhost
         self.session.listUsers.assert_called_once_with(userType=koji.USERTYPES['HOST'])
         self.session.getAllPerms.assert_not_called()
 
+    @mock.patch('sys.stdout', new_callable=StringIO)
+    def test_list_users_with_all_types(self, stdout):
+        arguments = ['--usertype', 'any']
+        self.session.listUsers.return_value = [{
+            'id': 3, 'krb_principals': [],
+            'name': 'kojihost',
+            'status': 0,
+            'usertype': 1},
+            {'id': 5, 'krb_principals': [],
+             'name': 'testhost',
+             'status': 0,
+             'usertype': 1},
+        ]
+        rv = anon_handle_list_users(self.options, self.session, arguments)
+        actual = stdout.getvalue()
+        expected = """kojihost
+testhost
+"""
+        self.assertMultiLineEqual(actual, expected)
+        self.assertEqual(rv, None)
+        self.session.listUsers.assert_called_once_with(userType=None)
+        self.session.getAllPerms.assert_not_called()
+
+    @mock.patch('sys.stderr', new_callable=StringIO)
+    def test_list_users_with_all_types_old_hub(self, stderr):
+        arguments = ['--usertype', 'any']
+        self.session.listUsers.return_value = []
+        self.session.hub_version = (1, 34, 0)
+        rv = anon_handle_list_users(self.options, self.session, arguments)
+        actual = stderr.getvalue()
+        expected = "This hub does not support querying for all usertypes\n"
+        self.assertMultiLineEqual(actual, expected)
+        self.assertEqual(rv, None)
+        self.session.listUsers.assert_called_once_with(userType=None)
+        self.session.getAllPerms.assert_not_called()
+
     def test_list_users_with_usertype_non_existing(self):
         arguments = ['--usertype', 'test']
         self.assert_system_exit(

