From 30a1b6564a7cdad027f2b52c61113b7f47da6ead Mon Sep 17 00:00:00 2001 From: Mike McLean Date: May 21 2019 22:40:05 +0000 Subject: PR#1272: check architecture names for mistakes Merges #1272 https://pagure.io/koji/pull-request/1272 Fixes: #1237 https://pagure.io/koji/issue/1237 Architectures field in build target uses 2 different separators --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index b8a77de..a4d1960 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -195,7 +195,7 @@ def handle_edit_host(options, session, args): if val is not None: vals[key] = val if 'arches' in vals: - vals['arches'] = parse_arches(vals['arches']) + vals['arches'] = koji.parse_arches(vals['arches']) session.multicall = True for host in args: @@ -350,7 +350,7 @@ def handle_add_pkg(goptions, session, args): continue to_add.append(package) if options.extra_arches: - opts['extra_arches'] = parse_arches(options.extra_arches) + opts['extra_arches'] = koji.parse_arches(options.extra_arches) # add the packages print("Adding %i packages to tag %s" % (len(to_add), dsttag['name'])) @@ -477,7 +477,7 @@ def handle_build(options, session, args): source = args[1] opts = {} if build_opts.arch_override: - opts['arch_override'] = parse_arches(build_opts.arch_override) + opts['arch_override'] = koji.parse_arches(build_opts.arch_override) for key in ('skip_tag', 'scratch', 'repo_id', 'fail_fast'): val = getattr(build_opts, key) if val is not None: @@ -4856,7 +4856,7 @@ def handle_add_tag(goptions, session, args): if options.parent: opts['parent'] = options.parent if options.arches: - opts['arches'] = parse_arches(options.arches) + opts['arches'] = koji.parse_arches(options.arches) if options.maven_support: opts['maven_support'] = True if options.include_all: @@ -4898,7 +4898,7 @@ def handle_edit_tag(goptions, session, args): tag = args[0] opts = {} if options.arches: - opts['arches'] = parse_arches(options.arches) + opts['arches'] = koji.parse_arches(options.arches) if options.no_perm: opts['perm_id'] = None elif options.perm: @@ -6270,7 +6270,7 @@ def handle_set_pkg_arches(goptions, session, args): parser.error(_("Please specify an archlist, a tag, and at least one package")) assert False # pragma: no cover activate_session(session, goptions) - arches = parse_arches(args[0]) + arches = koji.parse_arches(args[0]) tag = args[1] for package in args[2:]: #really should implement multicall... diff --git a/cli/koji_cli/lib.py b/cli/koji_cli/lib.py index 8e8bf99..5d77954 100644 --- a/cli/koji_cli/lib.py +++ b/cli/koji_cli/lib.py @@ -20,6 +20,8 @@ except ImportError: # pragma: no cover import koji from koji.util import to_list +# import parse_arches to current namespace for backward compatibility +from koji import parse_arches # for compatibility with plugins based on older version of lib # Use optparse imports directly in new code. @@ -132,26 +134,6 @@ def print_task_recurse(task,depth=0): print_task_recurse(child,depth+1) -def parse_arches(arches, to_list=False): - """Normalize user input for a list of arches. - - This method parses a single comma- or space-separated string of arches and - returns a space-separated string. - - :param str arches: comma- or space-separated string of arches, eg. - "x86_64,ppc64le", or "x86_64 ppc64le" - :param bool to_list: return a list of each arch, instead of a single - string. This is False by default. - :returns: a space-separated string like "x86_64 ppc64le", or a list like - ['x86_64', 'ppc64le']. - """ - arches = arches.replace(',', ' ').split() - if to_list: - return arches - else: - return ' '.join(arches) - - class TaskWatcher(object): def __init__(self,task_id,session,level=0,quiet=False): diff --git a/hub/kojihub.py b/hub/kojihub.py index 466bdcc..4118ad2 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -883,10 +883,11 @@ def _pkglist_remove(tag_id, pkg_id): def _pkglist_add(tag_id, pkg_id, owner, block, extra_arches): #revoke old entry (if present) - _pkglist_remove(tag_id, pkg_id) data = dslice(locals(), ('tag_id', 'owner', 'extra_arches')) data['package_id'] = pkg_id data['blocked'] = block + data['extra_arches'] = koji.parse_arches(data['extra_arches'], strict=True, allow_none=True) + _pkglist_remove(tag_id, pkg_id) insert = InsertProcessor('tag_packages', data=data) insert.make_create() #XXX user_id? insert.execute() @@ -923,6 +924,8 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force, assert_policy('package_list', policy_data) if not pkg: pkg = lookup_package(pkginfo, create=True) + # validate arches before running callbacks + extra_arches = koji.parse_arches(extra_arches, strict=True, allow_none=True) koji.plugin.run_callbacks('prePackageListChange', action=action, tag=tag, package=pkg, owner=owner, block=block, extra_arches=extra_arches, force=force, update=update) # first check to see if package is: @@ -3035,6 +3038,8 @@ def _create_tag(name, parent=None, arches=None, perm=None, locked=False, maven_s raise koji.GenericError("Tag name %s is too long. Max length is %s characters", name, max_name_length) + arches = koji.parse_arches(arches, strict=True, allow_none=True) + if not context.opts.get('EnableMaven') and (maven_support or maven_include_all): raise koji.GenericError("Maven support not enabled") @@ -3215,6 +3220,11 @@ SET name = %(name)s WHERE id = %(tagID)i""" _dml(update, values) + # sanitize architecture names (space-separated string) + arches = kwargs.get('arches') + if arches and tag['arches'] != arches: + kwargs['arches'] = koji.parse_arches(arches, strict=True, allow_none=True) + #check for changes data = tag.copy() changed = False @@ -10947,6 +10957,9 @@ class RootExports(object): from the HostPrincipalFormat setting (if available). """ context.session.assertPerm('admin') + # validate arches + arches = " ".join(arches) + arches = koji.parse_arches(arches, strict=True) if get_host(hostname): raise koji.GenericError('host already exists: %s' % hostname) q = """SELECT id FROM channels WHERE name = 'default'""" @@ -10964,7 +10977,7 @@ class RootExports(object): _dml(insert, dslice(locals(), ('hostID', 'userID', 'hostname'))) insert = InsertProcessor('host_config') - insert.set(host_id=hostID, arches=" ".join(arches)) + insert.set(host_id=hostID, arches=arches) insert.make_create() insert.execute() diff --git a/koji/__init__.py b/koji/__init__.py index dfe35e9..4388e8b 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -1104,6 +1104,37 @@ def canonArch(arch): else: return arch +def parse_arches(arches, to_list=False, strict=False, allow_none=False): + """Normalize user input for a list of arches. + + This method parses a single comma- or space-separated string of arches and + returns a space-separated string. + + Raise an error if arches string contain non-allowed characters. In strict + version allow only space-separated strings (db input). + + :param str arches: comma- or space-separated string of arches, eg. + "x86_64,ppc64le", or "x86_64 ppc64le" + :param bool to_list: return a list of each arch, instead of a single + string. This is False by default. + :param bool allow_none: convert None to "" + :returns: a space-separated string like "x86_64 ppc64le", or a list like + ['x86_64', 'ppc64le']. + """ + if allow_none and arches is None: + arches = '' + if not strict: + arches = arches.replace(',', ' ') + if not re.match(r'^[a-zA-Z0-9_\- ]*$', arches): + raise GenericError("Architecture can be only [a-zA-Z0-9_-]") + + arches = arches.split() + if to_list: + return arches + else: + return ' '.join(arches) + + class POMHandler(xml.sax.handler.ContentHandler): def __init__(self, values, fields): xml.sax.handler.ContentHandler.__init__(self) diff --git a/tests/test_hub/test_create_tag.py b/tests/test_hub/test_create_tag.py index f483211..bf79501 100644 --- a/tests/test_hub/test_create_tag.py +++ b/tests/test_hub/test_create_tag.py @@ -1,8 +1,6 @@ +# coding: utf-8 from __future__ import absolute_import -import copy import mock -import shutil -import tempfile try: import unittest2 as unittest except ImportError: @@ -55,7 +53,7 @@ class TestCreateTag(unittest.TestCase): insert = self.inserts[0] self.assertEqual(insert.table, 'tag_config') values = { - 'arches': None, + 'arches': '', 'create_event': 42, 'creator_id': 23, 'locked': False, @@ -67,3 +65,20 @@ class TestCreateTag(unittest.TestCase): self.assertEqual(insert.data, values) self.assertEqual(insert.rawdata, {}) insert = self.inserts[0] + + def test_invalid_archs(self): + self.get_tag.return_value = None + self.get_tag_id.return_value = 99 + self.context.event_id = 42 + self.context.session.user_id = 23 + + with self.assertRaises(koji.GenericError): + kojihub.create_tag('newtag', arches=u'ěšč') + + with self.assertRaises(koji.GenericError): + kojihub.create_tag('newtag', arches=u'arch1;arch2') + + with self.assertRaises(koji.GenericError): + kojihub.create_tag('newtag', arches=u'arch1,arch2') + + self.assertEqual(len(self.inserts), 0) diff --git a/tests/test_hub/test_edit_tag.py b/tests/test_hub/test_edit_tag.py index cb32059..e207074 100644 --- a/tests/test_hub/test_edit_tag.py +++ b/tests/test_hub/test_edit_tag.py @@ -1,3 +1,4 @@ +# coding: utf-8 from __future__ import absolute_import import mock try: @@ -197,3 +198,40 @@ WHERE id = %(tagID)i""", {'name': 'newtag', 'tagID': 333}) self.get_perm_id.assert_called_once() self._singleValue.assert_called_once() self.assertEqual(cm.exception.args[0], 'Name newtag already taken by tag 2') + + def test_invalid_archs(self): + self.get_tag.return_value = { + 'create_event': 42, + 'creator_id': 23, + 'arches': 'arch1 arch2', + 'locked': True, + 'maven_include_all': True, + 'maven_support': True, + 'perm_id': None, + 'tag_id': 333, + 'name': 'newtag', + 'id': 345, + } + + # valid + kwargs = { + 'name': 'newtag', + 'arches': 'valid_arch', + } + kojihub._edit_tag('tag', **kwargs) + + # invalid 1 + kwargs['arches'] = u'ěšč' + with self.assertRaises(koji.GenericError): + kojihub._edit_tag('tag', **kwargs) + + # invalid 2 + kwargs['arches'] = u'arch1;arch2' + with self.assertRaises(koji.GenericError): + kojihub._edit_tag('tag', **kwargs) + + # invalid 2 + kwargs['arches'] = u'arch1,arch2' + with self.assertRaises(koji.GenericError): + kojihub._edit_tag('tag', **kwargs) + diff --git a/tests/test_lib/test_parse_arches.py b/tests/test_lib/test_parse_arches.py new file mode 100644 index 0000000..a5aaeac --- /dev/null +++ b/tests/test_lib/test_parse_arches.py @@ -0,0 +1,31 @@ +# coding: utf-8 +from __future__ import absolute_import +try: + import unittest2 as unittest +except ImportError: + import unittest +import koji + +class TestParseArchesString(unittest.TestCase): + def test_parse_valid_arches(self): + r = koji.parse_arches('i386', to_list=True) + self.assertEqual(['i386'], r) + + r = koji.parse_arches('i386 x86_64', to_list=True) + self.assertEqual(['i386', 'x86_64'], r) + + r = koji.parse_arches('i386 x86_64 ', to_list=True) + self.assertEqual(['i386', 'x86_64'], r) + + r = koji.parse_arches('i386,x86_64', to_list=True) + self.assertEqual(['i386', 'x86_64'], r) + + def test_parse_invalid_arches(self): + with self.assertRaises(koji.GenericError): + koji.parse_arches(u'ěšč') + + with self.assertRaises(koji.GenericError): + koji.parse_arches(u'i386;x86_64') + + with self.assertRaises(koji.GenericError): + koji.parse_arches(u'i386,x86_64', strict=True)