From 8c598897825562e55d30eb5e935e6640047583cf Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 14 2019 06:28:03 +0000 Subject: [PATCH 1/6] hub: [getUser] support dict userInfo --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 724df28..f776ba7 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3666,7 +3666,11 @@ def get_external_repo_list(tag_info, event=None): def get_user(userInfo=None, strict=False, krb_princs=False): """Return information about a user. - :param userInfo: either a str (Kerberos principal or name) or an int (user id) + :param userInfo: a str (Kerberos principal or name) or an int (user id) + or a dict: + - id: User's ID + - name: User's name + - krb_principal: Kerberos principal :param strict: whether raising Error when no user found :param krb_princs: whether show krb_principals in result :return: a dict as user's information: @@ -3682,14 +3686,41 @@ def get_user(userInfo=None, strict=False, krb_princs=False): # not logged in raise koji.GenericError("No user provided") fields = ['id', 'name', 'status', 'usertype'] - data = {'info': userInfo} - if isinstance(userInfo, six.integer_types): - clauses = ['id = %(info)i'] - elif isinstance(userInfo, str): + if isinstance(userInfo, dict): + data = userInfo + elif isinstance(userInfo, six.integer_types): + data = {'id': userInfo} + elif isinstance(userInfo, six.string_types): + data = {'info': userInfo} clauses = ['krb_principal = %(info)s OR name = %(info)s'] else: raise koji.GenericError('invalid type for userInfo: %s' % type(userInfo)) + if isinstance(data, dict) and not data.get('info'): + clauses = [] + uid = data.get('id') + if uid is not None: + if isinstance(uid, six.integer_types): + clauses.append('users.id = %(id)i') + else: + raise koji.GenericError('invalid type for userid: %s' + % type(uid)) + username = data.get('name') + if username: + if isinstance(username, six.string_types): + clauses.append('users.name = %(name)s') + else: + raise koji.GenericError('invalid type for username: %s' + % type(username)) + krb_principal = data.get('krb_principal') + if krb_principal: + if isinstance(krb_principal, six.string_types): + clauses.append('user_krb_principals.krb_principal' + ' = %(krb_principal)s') + else: + raise koji.GenericError('invalid type for krb_principal: %s' + % type(krb_principal)) + query = QueryProcessor(tables=['users'], columns=fields, joins=['LEFT JOIN user_krb_principals' ' ON users.id = user_krb_principals.user_id'], @@ -3785,16 +3816,8 @@ def get_user_by_krb_principal(krb_principal, strict=False, krb_princs=False): if not isinstance(krb_principal, str): raise koji.GenericError("invalid type for krb_principal: %s" % type(krb_principal)) - fields = ['user_id'] - data = {'krb_principal': krb_principal} - clauses = ['krb_principal = %(krb_principal)s'] - query = QueryProcessor(tables=['users_krb_principals'], columns=fields, - clauses=clauses, values=data) - princ_item = query.executeOne() - if not princ_item and strict: - raise koji.GenericError("Cannot find user with kerberos principal: %s" - % krb_principal) - return get_user(data, strict=strict, krb_princ=krb_princs) + return get_user({'krb_principal': krb_principal}, strict=strict, + krb_princs=krb_princs) def find_build_id(X, strict=False): From 997e4d07fd7e20c975160802843b425fafec007d Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 14 2019 06:28:03 +0000 Subject: [PATCH 2/6] fix typos in auth.py --- diff --git a/koji/auth.py b/koji/auth.py index 9d992ce..66bae3e 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -707,6 +707,7 @@ class Session(object): def removeKrbPrincipal(self, name, krb_principal): select = """SELECT id FROM users JOIN user_krb_principals + ON users.id = user_krb_principals.user_id WHERE name = %(name)s AND krb_principal = %(krb_principal)s""" cursor = context.cnx.cursor() @@ -720,7 +721,7 @@ class Session(object): else: user_id = r[0] delete = """DELETE FROM user_krb_principals - WHERE user_id = (%(user_id)i + WHERE user_id = %(user_id)i AND krb_principal = %(krb_principal)s""" cursor.execute(delete, locals()) context.cnx.commit() From d96c90e8d93fa05bbc2b57ff264ad87c2f1a9533 Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 14 2019 06:28:03 +0000 Subject: [PATCH 3/6] hub: [get_group_members] return field is not named as alias --- diff --git a/hub/kojihub.py b/hub/kojihub.py index f776ba7..4aa2916 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -8059,14 +8059,23 @@ def get_group_members(group): if not ginfo or ginfo['usertype'] != koji.USERTYPES['GROUP']: raise koji.GenericError("Not a group: %s" % group) group_id = ginfo['id'] - fields = ('id', 'name', 'usertype', 'array_remove(array_agg(krb_principal)' - ', NULL) AS krb_principals') - q = """SELECT %s FROM user_groups - JOIN users ON user_groups.user_id = users.id - LEFT JOIN user_krb_principals ON users.id = user_krb_principals.user_id - WHERE active = TRUE AND group_id = %%(group_id)i - GROUP BY users.id""" % ','.join(fields) - return _multiRow(q, locals(), fields) + columns = ('id', 'name', 'usertype', 'array_remove(array_agg(krb_principal)' + ', NULL)') + aliases = ('id', 'name', 'usertype', 'krb_principals') + joins = ['JOIN users ON user_groups.user_id = users.id', + 'LEFT JOIN user_krb_principals' + ' ON users.id = user_krb_principals.user_id'] + clauses = [eventCondition(None), 'group_id = %(group_id)i'] + + query = QueryProcessor(tables=['user_groups'], + columns=columns, + aliases=aliases, + joins=joins, + clauses=clauses, + values=locals(), + opts={'group': 'users.id'}, + enable_group=True) + return query.iterate() def set_user_status(user, status): context.session.assertPerm('admin') diff --git a/tests/test_hub/test_user_groups.py b/tests/test_hub/test_user_groups.py index 42ad16c..98e5c93 100644 --- a/tests/test_hub/test_user_groups.py +++ b/tests/test_hub/test_user_groups.py @@ -287,7 +287,7 @@ class TestGrouplist(unittest.TestCase): self.context.session.assertPerm.side_effect = None self.get_user.side_effect = get_user2 kojihub.get_group_members(group) - self.assertEqual(len(self.queries), 0) + self.assertEqual(len(self.queries), 1) self.assertEqual(len(self.inserts), 0) self.assertEqual(len(self.updates), 0) - _multiRow.assert_called_once() + _multiRow.assert_not_called() From 5b905c738df540b81ca699133617f2eb53c7b802 Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 14 2019 08:30:04 +0000 Subject: [PATCH 4/6] fix editUser api for multiple kerberos support --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 4aa2916..ff0ee9d 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3732,22 +3732,28 @@ def get_user(userInfo=None, strict=False, krb_princs=False): user['krb_principals'] = list_user_krb_principals(user['id']) return user -def edit_user(userInfo, name=None, krb_principal=None): +def edit_user(userInfo, name=None, krb_principal_mappings=None): """Edit information for an existing user. userInfo specifies the user to edit fields changes are provided as keyword arguments: name: rename the user - krb_principal: change user's kerberos principal + krb_principal_mappings: change user's kerberos principal, it is a list + contains krb_principal pair(s) + - old: krb_principal to modify, None and '' + indicates adding a new krb_principal + - new: new value of krb_principal, None and '' + indicates removing the old krb_principal """ context.session.assertPerm('admin') - _edit_user(userInfo, name=name, krb_principal=krb_principal) + _edit_user(userInfo, name=name, + krb_principal_mappings=krb_principal_mappings) -def _edit_user(userInfo, name=None, krb_principal=None): +def _edit_user(userInfo, name=None, krb_principal_mappings=None): """Edit information for an existing user.""" - user = get_user(userInfo, strict=True) + user = get_user(userInfo, strict=True, krb_princs=True) if name and user['name'] != name: # attempt to update user name values = { @@ -3759,14 +3765,42 @@ def _edit_user(userInfo, name=None, krb_principal=None): if id is not None: # new name is taken raise koji.GenericError("Name %s already taken by user %s" % (name, id)) - update = UpdateProcessor('users', values={'userID': user['id']}, clauses=['id = %(userID)i']) + update = UpdateProcessor('users', + values={'userID': user['id']}, + clauses=['id = %(userID)i']) update.set(name=name) update.execute() - if krb_principal and user['krb_principal'] != krb_principal: + if krb_principal_mappings: + added = set() + removed = set() + for pairs in krb_principal_mappings: + old = pairs.get('old') + new = pairs.get('new') + if old: + removed.add(old) + if new: + added.add(new) + dups = added & removed + if dups: + raise koji.GenericError("There are some conflicts between added" + " and removed Kerberos principals: %s" + % ', '.join(dups)) + currents = set(user.get('krb_principals')) + dups = added & currents + if dups: + raise koji.GenericError("Cannot add existing Kerberos" + " principals: %s" % ', '.join(dups)) + unable_removed = removed - currents + if unable_removed: + raise koji.GenericError("Cannot remove non-existent Kerberos" + " principals: %s" + % ', '.join(unable_removed)) + # attempt to update kerberos principal - update = UpdateProcessor('users', values={'userID': user['id']}, clauses=['id = %(userID)i']) - update.set(krb_principal=krb_principal) - update.execute() + for r in removed: + context.session.removeKrbPrincipal(user['id'], krb_principal=r) + for a in added: + context.session.setKrbPrincipal(user['id'], krb_principal=a) def list_user_krb_principals(user_info=None): diff --git a/koji/auth.py b/koji/auth.py index 66bae3e..145a5bd 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -689,7 +689,12 @@ class Session(object): def setKrbPrincipal(self, name, krb_principal, krb_princ_check=True): if krb_princ_check: self.checkKrbPrincipal(krb_principal) - select = """SELECT id FROM users WHERE name = %(name)s""" + select = """SELECT id FROM users WHERE %s""" + if isinstance(name, six.integer_types): + user_condition = 'id = %(name)i' + else: + user_condition = 'name = %(name)s' + select = select % user_condition cursor = context.cnx.cursor() cursor.execute(select, locals()) r = cursor.fetchone() @@ -708,15 +713,20 @@ class Session(object): select = """SELECT id FROM users JOIN user_krb_principals ON users.id = user_krb_principals.user_id - WHERE name = %(name)s - AND krb_principal = %(krb_principal)s""" + WHERE %s + AND krb_principal = %%(krb_principal)s""" + if isinstance(name, six.integer_types): + user_condition = 'id = %(name)i' + else: + user_condition = 'name = %(name)s' + select = select % user_condition cursor = context.cnx.cursor() cursor.execute(select, locals()) r = cursor.fetchone() if not r: context.cnx.rollback() raise koji.AuthError( - 'could not automatically remove Kerberos Principal:' + 'cannot remove Kerberos Principal:' ' %(krb_principal)s with user %(name)s' % locals()) else: user_id = r[0] diff --git a/tests/test_hub/test_edit_user.py b/tests/test_hub/test_edit_user.py index c2dba35..db6fc21 100644 --- a/tests/test_hub/test_edit_user.py +++ b/tests/test_hub/test_edit_user.py @@ -30,11 +30,11 @@ class TestEditUser(unittest.TestCase): def test_edit(self): self.get_user.return_value = {'id': 333, - 'name': 'user', - 'krb_principal': 'krb'} + 'name': 'user', + 'krb_principals': ['krb']} self._singleValue.return_value = None - kojihub._edit_user('user', name='newuser', krb_principal='krb') + kojihub._edit_user('user', name='newuser') # check the update self.assertEqual(len(self.updates), 1) update = self.updates[0] @@ -43,18 +43,53 @@ class TestEditUser(unittest.TestCase): self.assertEqual(update.values, {'userID': 333}) self.assertEqual(update.clauses, ['id = %(userID)i']) - kojihub._edit_user('user', name='user', krb_principal='newkrb') - # check the insert/update - self.assertEqual(len(self.updates), 2) - update = self.updates[1] - self.assertEqual(update.table, 'users') - self.assertEqual(update.data, {'krb_principal': 'newkrb'}) - self.assertEqual(update.values, {'userID': 333}) - self.assertEqual(update.clauses, ['id = %(userID)i']) + kojihub._edit_user('user', krb_principal_mappings=[{'old': 'krb', + 'new': 'newkrb'}]) + self.context.session.removeKrbPrincipal. \ + assert_called_once_with(333, krb_principal='krb') + self.context.session.setKrbPrincipal. \ + assert_called_once_with(333, krb_principal='newkrb') + + self.context.reset_mock() + with self.assertRaises(koji.GenericError) as cm: + kojihub._edit_user('user', + krb_principal_mappings=[{'old': 'krb', + 'new': 'newkrb'}, + {'old': 'newkrb', + 'new': 'newnewkrb'} + ]) + self.assertEqual(cm.exception.args[0], + 'There are some conflicts between added and removed' + ' Kerberos principals: newkrb') + self.context.session.removeKrbPrincipal.assert_not_called() + self.context.session.setKrbPrincipal.assert_not_called() + + self.context.reset_mock() + with self.assertRaises(koji.GenericError) as cm: + kojihub._edit_user('user', + krb_principal_mappings=[{'old': 'otherkrb', + 'new': 'newkrb'}]) + self.assertEqual(cm.exception.args[0], + 'Cannot remove non-existent Kerberos principals:' + ' otherkrb') + self.context.session.removeKrbPrincipal.assert_not_called() + self.context.session.setKrbPrincipal.assert_not_called() + + self.context.reset_mock() + with self.assertRaises(koji.GenericError) as cm: + kojihub._edit_user('user', + krb_principal_mappings=[{'old': None, + 'new': 'krb'}]) + self.assertEqual(cm.exception.args[0], + 'Cannot add existing Kerberos principals: krb') + self.context.session.removeKrbPrincipal.assert_not_called() + self.context.session.setKrbPrincipal.assert_not_called() + self._singleValue.reset_mock() self._singleValue.return_value = 2 with self.assertRaises(koji.GenericError) as cm: kojihub._edit_user('user', name='newuser') self._singleValue.assert_called_once() - self.assertEqual(cm.exception.args[0], 'Name newuser already taken by user 2') + self.assertEqual(cm.exception.args[0], + 'Name newuser already taken by user 2') From a468af66dafd49b36616a0a884bb3db14497f8fe Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 14 2019 08:30:31 +0000 Subject: [PATCH 5/6] cli: fix edit-user command for multiple kerberos support --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index 0d572bf..413cbdf 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -2064,7 +2064,14 @@ def handle_edit_user(goptions, session, args): usage += _("\n(Specify the --help global option for a list of other help options)") parser = OptionParser(usage=usage) parser.add_option("--rename", help=_("Rename the user")) - parser.add_option("--krb", help=_("Change kerberos principal of the user")) + parser.add_option("--edit-krb", action="append", default=[], + metavar="OLD=NEW", + help=_("Change kerberos principal of the user")) + parser.add_option("--add-krb", action="append", default=[], metavar="KRB", + help=_("Add kerberos principal of the user")) + parser.add_option("--remove-krb", action="append", default=[], + metavar="KRB", + help=_("Remove kerberos principal of the user")) (options, args) = parser.parse_args(args) if len(args) < 1: parser.error(_("You must specify the username of the user to edit")) @@ -2072,7 +2079,15 @@ def handle_edit_user(goptions, session, args): parser.error(_("This command only accepts one argument (username)")) activate_session(session, goptions) user = args[0] - session.editUser(user, options.rename, options.krb) + princ_mappings = [] + for p in options.edit_krb: + old, new = p.split('=', 1) + princ_mappings.append({'old': arg_filter(old), 'new': arg_filter(old)}) + for a in options.add_krb: + princ_mappings.append({'old': None, 'new': arg_filter(a)}) + for r in options.remove_krb: + princ_mappings.append({'old': arg_filter(r), 'new': None}) + session.editUser(user, options.rename, princ_mappings) def handle_list_signed(goptions, session, args): diff --git a/tests/test_cli/test_edit_user.py b/tests/test_cli/test_edit_user.py index 1dcd497..843f1b3 100644 --- a/tests/test_cli/test_edit_user.py +++ b/tests/test_cli/test_edit_user.py @@ -20,10 +20,11 @@ class TestEditUser(unittest.TestCase): def test_handle_edit_user(self, activate_session_mock, stdout): user = 'user' rename = 'user2' - krb_principal = 'krb' args = [user] args.append('--rename=' + rename) - args.append('--krb=' + krb_principal) + args.append('--add-krb=addedkrb') + args.append('--remove-krb=removedkrb') + args.append('--edit-krb=oldkrb=newkrb') options = mock.MagicMock() # Mock out the xmlrpc server @@ -38,7 +39,10 @@ class TestEditUser(unittest.TestCase): self.assertMultiLineEqual(actual, expected) # Finally, assert that things were called as we expected. activate_session_mock.assert_called_once_with(session, options) - session.editUser.assert_called_once_with(user, rename, krb_principal) + session.editUser.assert_called_once_with(user, rename, + [{'new': 'oldkrb', 'old': 'oldkrb'}, + {'new': 'addedkrb', 'old': None}, + {'new': None, 'old': 'removedkrb'}]) self.assertEqual(rv, None) stdout.seek(0) @@ -67,9 +71,11 @@ class TestEditUser(unittest.TestCase): (Specify the --help global option for a list of other help options) Options: - -h, --help show this help message and exit - --rename=RENAME Rename the user - --krb=KRB Change kerberos principal of the user + -h, --help show this help message and exit + --rename=RENAME Rename the user + --edit-krb=OLD=NEW Change kerberos principal of the user + --add-krb=KRB Add kerberos principal of the user + --remove-krb=KRB Remove kerberos principal of the user """ % progname expected_stderr = '' self.assertMultiLineEqual(actual_stdout, expected_stdout) From be26c7392a0aad1cd19041b96f40c2857e8a332d Mon Sep 17 00:00:00 2001 From: Yuming Zhu Date: Oct 15 2019 06:31:56 +0000 Subject: [PATCH 6/6] fix unitest import for el6 --- diff --git a/tests/test_cli/test_edit_user.py b/tests/test_cli/test_edit_user.py index 843f1b3..7d9768e 100644 --- a/tests/test_cli/test_edit_user.py +++ b/tests/test_cli/test_edit_user.py @@ -3,7 +3,10 @@ import mock import os import six import sys -import unittest +try: + import unittest2 as unittest +except ImportError: + import unittest from koji_cli.commands import handle_edit_user diff --git a/tests/test_hub/test_edit_user.py b/tests/test_hub/test_edit_user.py index db6fc21..65fc11d 100644 --- a/tests/test_hub/test_edit_user.py +++ b/tests/test_hub/test_edit_user.py @@ -1,5 +1,8 @@ import mock -import unittest +try: + import unittest2 as unittest +except ImportError: + import unittest import koji import kojihub