From aaa836ea9b111b640f238054520565f9cf9422a1 Mon Sep 17 00:00:00 2001 From: Mike McLean 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 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 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 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 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(