From 3e124f685a63932c5ec32f694ac2e992b3626f5c Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Mon, 14 Oct 2019 08:27:08 +0000 Subject: [PATCH] fix editUser api for multiple kerberos support --- hub/kojihub.py | 54 +++++++++++++++++++++++------ koji/auth.py | 18 +++++++--- tests/test_hub/test_edit_user.py | 59 +++++++++++++++++++++++++------- 3 files changed, 105 insertions(+), 26 deletions(-) diff --git a/hub/kojihub.py b/hub/kojihub.py index 0bc3e843..3dca29f0 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 66bae3e3..145a5bda 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 c2dba357..db6fc217 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')