From 2cfa19af24c5c75e24b12ac120be291871333641 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 10 2020 11:21:20 +0000 Subject: [PATCH 1/6] add-host work even if host already tried to log in Fixes: https://pagure.io/koji/issue/1874 --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index c083734..36819fb 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -187,6 +187,8 @@ def handle_add_host(goptions, session, args): parser = OptionParser(usage=get_usage_str(usage)) parser.add_option("--krb-principal", help=_("set a non-default kerberos principal for the host")) + parser.add_option("--force", default=False, action="store_true", + help=_("if existing used is a regular user, convert it to a host")) (options, args) = parser.parse_args(args) if len(args) < 2: parser.error(_("Please specify a hostname and at least one arch")) @@ -194,15 +196,13 @@ def handle_add_host(goptions, session, args): activate_session(session, goptions) id = session.getHost(host) if id: - print("%s is already in the database" % host) - return 1 + error("%s is already in the database" % host) else: - kwargs = {} + kwargs = {'force': options.force} if options.krb_principal is not None: kwargs['krb_principal'] = options.krb_principal id = session.addHost(host, args[1:], **kwargs) - if id: - print("%s added: id %d" % (host, id)) + print("%s added: id %d" % (host, id)) def handle_edit_host(options, session, args): diff --git a/hub/kojihub.py b/hub/kojihub.py index c1bf31d..71cbe14 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -12112,7 +12112,7 @@ class RootExports(object): arch=taskInfo['arch'], channel=channel['name'], priority=taskInfo['priority']) - def addHost(self, hostname, arches, krb_principal=None): + def addHost(self, hostname, arches, krb_principal=None, force=False): """ Add a builder host to the database. @@ -12120,6 +12120,7 @@ class RootExports(object): :param list arches: list of architectures this builder supports. :param str krb_principal: (optional) a non-default kerberos principal for the host. + :param bool force: override user type :returns: new host id If krb_principal is not given then that field will be generated @@ -12137,9 +12138,24 @@ class RootExports(object): fmt = context.opts.get('HostPrincipalFormat') if fmt: krb_principal = fmt % hostname - # users entry - userID = context.session.createUser(hostname, usertype=koji.USERTYPES['HOST'], - krb_principal=krb_principal) + # builder user can already exist, if host tried to log in before adding into db + user = get_user(userInfo={'name': hostname, 'krb_principals': [krb_principal]}) + if user: + if user['usertype'] != koji.USERTYPES['HOST']: + if force and user['usertype'] == koji.USERTYPES['NORMAL']: + # override usertype in this special case + update = UpdateProcessor('users', + values={'userID': user['id']}, + clauses=['id = %(userID)i']) + update.set(usertype=koji.USERTYPES['HOST']) + update.execute() + else: + raise koji.GenericError( + 'user %s already exists and it is not a host' % hostname) + userID = user['id'] + else: + userID = context.session.createUser(hostname, usertype=koji.USERTYPES['HOST'], + krb_principal=krb_principal) # host entry hostID = _singleValue("SELECT nextval('host_id_seq')", strict=True) insert = "INSERT INTO host (id, user_id, name) VALUES (%(hostID)i, %(userID)i, " \ diff --git a/tests/test_cli/test_add_host.py b/tests/test_cli/test_add_host.py index 94260ad..5c8faa7 100644 --- a/tests/test_cli/test_add_host.py +++ b/tests/test_cli/test_add_host.py @@ -8,6 +8,7 @@ try: except ImportError: import unittest +import koji from koji_cli.commands import handle_add_host class TestAddHost(unittest.TestCase): @@ -24,7 +25,7 @@ class TestAddHost(unittest.TestCase): krb_principal = '--krb-principal=krb' arguments = [host] + arches arguments.append(krb_principal) - kwargs = {'krb_principal': 'krb'} + kwargs = {'krb_principal': 'krb', 'force': False} options = mock.MagicMock() # Mock out the xmlrpc server @@ -69,12 +70,12 @@ class TestAddHost(unittest.TestCase): # Finally, assert that things were called as we expected. activate_session_mock.assert_called_once_with(session, options) session.getHost.assert_called_once_with(host) - session.addHost.assert_called_once_with(host, arches) + session.addHost.assert_called_once_with(host, arches, force=False) self.assertNotEqual(rv, 1) - @mock.patch('sys.stdout', new_callable=six.StringIO) + @mock.patch('sys.stderr', new_callable=six.StringIO) @mock.patch('koji_cli.commands.activate_session') - def test_handle_add_host_dupl(self, activate_session_mock, stdout): + def test_handle_add_host_dupl(self, activate_session_mock, stderr): host = 'host' host_id = 1 arches = ['arch1', 'arch2'] @@ -89,15 +90,15 @@ class TestAddHost(unittest.TestCase): # Run it and check immediate output # args: host, arch1, arch2, --krb-principal=krb # expected: failed, host already exists - rv = handle_add_host(options, session, arguments) - actual = stdout.getvalue() + with self.assertRaises(SystemExit): + handle_add_host(options, session, arguments) + actual = stderr.getvalue() expected = 'host is already in the database\n' self.assertMultiLineEqual(actual, expected) # Finally, assert that things were called as we expected. activate_session_mock.assert_called_once_with(session, options) session.getHost.assert_called_once_with(host) session.addHost.assert_not_called() - self.assertEqual(rv, 1) @mock.patch('sys.stdout', new_callable=six.StringIO) @mock.patch('sys.stderr', new_callable=six.StringIO) @@ -141,18 +142,19 @@ class TestAddHost(unittest.TestCase): krb_principal = '--krb-principal=krb' arguments = [host] + arches arguments.append(krb_principal) - kwargs = {'krb_principal': 'krb'} + kwargs = {'krb_principal': 'krb', 'force': False} options = mock.MagicMock() # Mock out the xmlrpc server session = mock.MagicMock() session.getHost.return_value = None - session.addHost.return_value = None + session.addHost.side_effect = koji.GenericError # Run it and check immediate output # args: host, arch1, arch2, --krb-principal=krb # expected: failed - handle_add_host(options, session, arguments) + with self.assertRaises(koji.GenericError): + handle_add_host(options, session, arguments) actual = stdout.getvalue() expected = '' self.assertMultiLineEqual(actual, expected) diff --git a/tests/test_hub/test_add_host.py b/tests/test_hub/test_add_host.py index fa1978a..cb5e7ee 100644 --- a/tests/test_hub/test_add_host.py +++ b/tests/test_hub/test_add_host.py @@ -77,3 +77,77 @@ class TestAddHost(unittest.TestCase): self.assertEqual(_dml.call_count, 1) _dml.assert_called_once_with("INSERT INTO host (id, user_id, name) VALUES (%(hostID)i, %(userID)i, %(hostname)s)", {'hostID': 12, 'userID': 456, 'hostname': 'hostname'}) + + @mock.patch('kojihub.get_user') + @mock.patch('kojihub._dml') + @mock.patch('kojihub.get_host') + @mock.patch('kojihub._singleValue') + def test_add_host_wrong_user(self, _singleValue, get_host, _dml, get_user): + get_user.return_value = { + 'id': 1, + 'name': 'hostname', + 'usertype': koji.USERTYPES['NORMAL'] + } + get_host.return_value = {} + with self.assertRaises(koji.GenericError): + self.exports.addHost('hostname', ['i386', 'x86_64']) + _dml.assert_not_called() + get_user.assert_called_once_with(userInfo={ + 'name': 'hostname', + 'krb_principals': ['-hostname-']}) + get_host.assert_called_once_with('hostname') + _singleValue.assert_called_once() + self.assertEqual(len(self.inserts), 0) + self.assertEqual(len(self.updates), 0) + + @mock.patch('kojihub.get_user') + @mock.patch('kojihub._dml') + @mock.patch('kojihub.get_host') + @mock.patch('kojihub._singleValue') + def test_add_host_wrong_user_forced(self, _singleValue, get_host, _dml, get_user): + get_user.return_value = { + 'id': 123, + 'name': 'hostname', + 'usertype': koji.USERTYPES['NORMAL'] + } + get_host.return_value = {} + + self.exports.addHost('hostname', ['i386', 'x86_64'], force=True) + + _dml.assert_called_once() + get_user.assert_called_once_with(userInfo={ + 'name': 'hostname', + 'krb_principals': ['-hostname-']}) + get_host.assert_called_once_with('hostname') + _singleValue.assert_called() + self.assertEqual(len(self.inserts), 2) + self.assertEqual(len(self.updates), 1) + update = self.updates[0] + self.assertEqual(update.values, {'userID': 123}) + self.assertEqual(update.table, 'users') + self.assertEqual(update.clauses, ['id = %(userID)i']) + self.assertEqual(update.data, {'usertype': koji.USERTYPES['HOST']}) + + @mock.patch('kojihub.get_user') + @mock.patch('kojihub._dml') + @mock.patch('kojihub.get_host') + @mock.patch('kojihub._singleValue') + def test_add_host_superwrong_user_forced(self, _singleValue, get_host, _dml, get_user): + get_user.return_value = { + 'id': 123, + 'name': 'hostname', + 'usertype': koji.USERTYPES['GROUP'] + } + get_host.return_value = {} + + with self.assertRaises(koji.GenericError): + self.exports.addHost('hostname', ['i386', 'x86_64'], force=True) + + _dml.assert_not_called() + get_user.assert_called_once_with(userInfo={ + 'name': 'hostname', + 'krb_principals': ['-hostname-']}) + get_host.assert_called_once_with('hostname') + _singleValue.assert_called() + self.assertEqual(len(self.inserts), 0) + self.assertEqual(len(self.updates), 0) From 698fada9d4f5eec598cd6768053235b63dee9684 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 11 2020 13:44:45 +0000 Subject: [PATCH 2/6] fix get_user params --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 71cbe14..8bf77d2 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -12139,7 +12139,7 @@ class RootExports(object): if fmt: krb_principal = fmt % hostname # builder user can already exist, if host tried to log in before adding into db - user = get_user(userInfo={'name': hostname, 'krb_principals': [krb_principal]}) + user = get_user(userInfo={'name': hostname, 'krb_principal': krb_principal}) if user: if user['usertype'] != koji.USERTYPES['HOST']: if force and user['usertype'] == koji.USERTYPES['NORMAL']: From cc45c23315fc027d0a91aea28b2c8f15a391ac4f Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 11 2020 13:56:51 +0000 Subject: [PATCH 3/6] fix test --- diff --git a/tests/test_hub/test_add_host.py b/tests/test_hub/test_add_host.py index cb5e7ee..b020cce 100644 --- a/tests/test_hub/test_add_host.py +++ b/tests/test_hub/test_add_host.py @@ -94,7 +94,7 @@ class TestAddHost(unittest.TestCase): _dml.assert_not_called() get_user.assert_called_once_with(userInfo={ 'name': 'hostname', - 'krb_principals': ['-hostname-']}) + 'krb_principal': '-hostname-'}) get_host.assert_called_once_with('hostname') _singleValue.assert_called_once() self.assertEqual(len(self.inserts), 0) @@ -117,7 +117,7 @@ class TestAddHost(unittest.TestCase): _dml.assert_called_once() get_user.assert_called_once_with(userInfo={ 'name': 'hostname', - 'krb_principals': ['-hostname-']}) + 'krb_principal': '-hostname-'}) get_host.assert_called_once_with('hostname') _singleValue.assert_called() self.assertEqual(len(self.inserts), 2) @@ -146,7 +146,7 @@ class TestAddHost(unittest.TestCase): _dml.assert_not_called() get_user.assert_called_once_with(userInfo={ 'name': 'hostname', - 'krb_principals': ['-hostname-']}) + 'krb_principal': '-hostname-'}) get_host.assert_called_once_with('hostname') _singleValue.assert_called() self.assertEqual(len(self.inserts), 0) From 49416f0f0c49f359e32940a44c78126f9cce1f2e Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 19 2020 14:45:46 +0000 Subject: [PATCH 4/6] query on krb only if it is requested --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 8bf77d2..0d8a01a 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -12139,7 +12139,10 @@ class RootExports(object): if fmt: krb_principal = fmt % hostname # builder user can already exist, if host tried to log in before adding into db - user = get_user(userInfo={'name': hostname, 'krb_principal': krb_principal}) + userinfo = {'name': hostname} + if krb_principal: + userinfo['krb_principal'] = krb_principal + user = get_user(userInfo=userinfo) if user: if user['usertype'] != koji.USERTYPES['HOST']: if force and user['usertype'] == koji.USERTYPES['NORMAL']: From 7a72c46adfe0855506dfbb9ec454c8490674185b Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 24 2020 11:14:57 +0000 Subject: [PATCH 5/6] check krb_principal before it is rewritten --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 0d8a01a..803ef28 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -12134,10 +12134,6 @@ class RootExports(object): raise koji.GenericError('host already exists: %s' % hostname) q = """SELECT id FROM channels WHERE name = 'default'""" default_channel = _singleValue(q) - if krb_principal is None: - fmt = context.opts.get('HostPrincipalFormat') - if fmt: - krb_principal = fmt % hostname # builder user can already exist, if host tried to log in before adding into db userinfo = {'name': hostname} if krb_principal: @@ -12157,6 +12153,10 @@ class RootExports(object): 'user %s already exists and it is not a host' % hostname) userID = user['id'] else: + if krb_principal is None: + fmt = context.opts.get('HostPrincipalFormat') + if fmt: + krb_principal = fmt % hostname userID = context.session.createUser(hostname, usertype=koji.USERTYPES['HOST'], krb_principal=krb_principal) # host entry From 89df7a9d0a3965b88cfe8406e07a3b8e0bccbf26 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 25 2020 12:47:18 +0000 Subject: [PATCH 6/6] fix tests --- diff --git a/tests/test_hub/test_add_host.py b/tests/test_hub/test_add_host.py index b020cce..61c8a38 100644 --- a/tests/test_hub/test_add_host.py +++ b/tests/test_hub/test_add_host.py @@ -92,9 +92,7 @@ class TestAddHost(unittest.TestCase): with self.assertRaises(koji.GenericError): self.exports.addHost('hostname', ['i386', 'x86_64']) _dml.assert_not_called() - get_user.assert_called_once_with(userInfo={ - 'name': 'hostname', - 'krb_principal': '-hostname-'}) + get_user.assert_called_once_with(userInfo={'name': 'hostname'}) get_host.assert_called_once_with('hostname') _singleValue.assert_called_once() self.assertEqual(len(self.inserts), 0) @@ -115,9 +113,7 @@ class TestAddHost(unittest.TestCase): self.exports.addHost('hostname', ['i386', 'x86_64'], force=True) _dml.assert_called_once() - get_user.assert_called_once_with(userInfo={ - 'name': 'hostname', - 'krb_principal': '-hostname-'}) + get_user.assert_called_once_with(userInfo={'name': 'hostname'}) get_host.assert_called_once_with('hostname') _singleValue.assert_called() self.assertEqual(len(self.inserts), 2) @@ -144,9 +140,7 @@ class TestAddHost(unittest.TestCase): self.exports.addHost('hostname', ['i386', 'x86_64'], force=True) _dml.assert_not_called() - get_user.assert_called_once_with(userInfo={ - 'name': 'hostname', - 'krb_principal': '-hostname-'}) + get_user.assert_called_once_with(userInfo={'name': 'hostname'}) get_host.assert_called_once_with('hostname') _singleValue.assert_called() self.assertEqual(len(self.inserts), 0)