PR#3028: Add limits on name values - koji

koji

#3028 Add limits on name values

Merged 2 years ago by tkopecek. Opened 2 years ago by jcupova.

jcupova/koji issue-2117 into master

Add limits on name values

Jana Cupova • 2 years ago

74ac826

docs/source/hub_conf.rst

file added

+51

		`@@ -0,0 +1,51 @@`
		`+ hub.conf`
		`+ --------`
		`+ hub.conf is a standard .ini-like configuration file. Its main section is`
		+ called ``[hub]`` and contains the following options. They can occur anywhere.
		`+`
		`+ Incomplete document`
		`+ ^^^^^^^^^^^^^^^^^^^`
		`+`
		`+ This document is a stub and does not cover all options.`
		+ Work to complete this document is tracked in `Issue 3073 <https://pagure.io/koji/issue/3073>`_
		`+`
		+ The old :doc:`Server HOW TO <server_howto>` doc also describes some hub configuration options.
		`+`
		`+ Name verification`
		`+ ^^^^^^^^^^^^^^^^^`
		`+ Currently we have two groups for name verification:`
		`+ - internal names`
		`+ - user names`
		`+`
		`+ Group internal names is currently used for:`
		`+ - archive type`
		`+ - btype`
		`+ - channel`
		`+ - external repo`
		`+ - group`
		`+ - host`
		`+ - kerberos`
		`+ - permission`
		`+ - tag`
		`+ - target`
		`+ - volume`
		`+`
		`+ Group user names is currently used for:`
		`+ - user`
		`+ - host`
		`+`
		`+ Host names are listed in both groups because hosts always have an associated user entry.`
		`+`
		`+ .. glossary::`
		`+ MaxNameLengthInternal = 256`
		`+ Set length of internal names. By default there is allowed length set up to 256.`
		`+ When length is set up to 0, length verifying is disabled.`
		`+`
		`+ RegexNameInternal = ^[A-Za-z0-9/_.+-]+$`
		`+ Set regex for verify an internal names. When regex string is empty, verifying`
		`+ is disabled.`
		`+`
		`+ RegexUserName = ^[A-Za-z0-9/_.@-]+$`
		`+ Set regex for verify a user name and kerberos. User name and kerberos have`
		`+ in default set up allowed '@' and '/' chars on top of basic name regex`
		`+ for internal names. When regex string is empty, verifying is disabled.`

docs/source/index.rst

file modified

		`@@ -41,6 +41,7 @@`
		`signing`
		`database_howto`
		`kojid_conf`
		`+ hub_conf`
		`using_the_koji_build_system`
		`setting_rpm_macros`
		`profiles`

docs/source/server_howto.rst

file modified

		`@@ -777,6 +777,8 @@`
		``/etc/koji-hub/hub.conf.d/secret.conf`` for sensitive values. Typical usecase
		for separate config is :doc:`policy <defining_hub_policies>` configuration file.

		+ Doc page about hub options in :doc:`Hub conf <hub_conf>`. (Currently in progress).
		`+`
		`Authentication Configuration`
		`----------------------------`

hub/hub.conf

file modified

		`@@ -136,3 +136,8 @@`
		`## OfflineMessage as the fault string).`
		`## If LockOut is True, the server will report a ServerOffline fault for all non-admin`
		`## requests.`
		`+`
		`+ ## Determines rules for names (tag, user, ...)`
		`+ # MaxNameLengthInternal = 256`
		`+ # RegexNameInternal = ^[A-Za-z0-9/_.+-]+$`
		`+ # RegexUserName = ^[A-Za-z0-9/_.@-]+$`

hub/kojihub.py

file modified

+63 -15

		`@@ -2290,6 +2290,8 @@`
		`if host is None:`
		`raise koji.GenericError('host does not exist: %s' % hostname)`
		`host_id = host['id']`
		`+ if create:`
		`+ verify_name_internal(channel_name)`
		`channel_id = get_channel_id(channel_name, create=create)`
		`if channel_id is None:`
		`raise koji.GenericError('channel does not exist: %s' % channel_name)`
		`@@ -2344,6 +2346,7 @@`
		`context.session.assertPerm('admin')`
		`if not isinstance(new, str):`
		`raise koji.GenericError("new channel name must be a string")`
		`+ verify_name_internal(new)`
		`cinfo = get_channel(old, strict=True)`
		`dup_check = get_channel(new, strict=False)`
		`if dup_check:`
		`@@ -2376,6 +2379,7 @@`
		`if kw.get('name'):`
		`if not isinstance(kw['name'], str):`
		`raise koji.GenericError("new channel name must be a string")`
		`+ verify_name_internal(kw['name'])`
		`dup_check = get_channel(kw['name'], strict=False)`
		`if dup_check:`
		`raise koji.GenericError("channel %(name)s already exists (id=%(id)i)" % dup_check)`
		`@@ -2431,6 +2435,7 @@`
		`context.session.assertPerm('admin')`
		`if not isinstance(channel_name, str):`
		`raise koji.GenericError("Channel name must be a string")`
		`+ verify_name_internal(channel_name)`
		`dup_check = get_channel(channel_name, strict=False)`
		`if dup_check:`
		`raise koji.GenericError("channel %(name)s already exists (id=%(id)i)" % dup_check)`
		`@@ -3058,14 +3063,6 @@`
		`insert.execute()`


		`- def _validate_build_target_name(name):`
		`- """ A helper function that validates a build target name. """`
		`- max_name_length = 256`
		`- if len(name) > max_name_length:`
		`- raise koji.GenericError("Build target name %s is too long. Max length "`
		`- "is %s characters" % (name, max_name_length))`
		`-`
		`-`
		`def create_build_target(name, build_tag, dest_tag):`
		`"""Create a new build target"""`

		`@@ -3075,7 +3072,7 @@`

		`def _create_build_target(name, build_tag, dest_tag):`
		`"""Create a new build target(no access check)"""`
		`- _validate_build_target_name(name)`
		`+ verify_name_internal(name)`

		`# Does a target with this name already exist?`
		`if get_build_targets(info=name):`
		`@@ -3111,7 +3108,7 @@`

		`def _edit_build_target(buildTargetInfo, name, build_tag, dest_tag):`
		`"""Edit build target parameters, w/ no access checks"""`
		`- _validate_build_target_name(name)`
		`+ verify_name_internal(name)`

		`target = lookup_build_target(buildTargetInfo)`
		`if not target:`
		`@@ -3343,11 +3340,7 @@`
		`def _create_tag(name, parent=None, arches=None, perm=None, locked=False, maven_support=False,`
		`maven_include_all=False, extra=None):`
		`"""Create a new tag, without access check"""`
		`-`
		`- max_name_length = 256`
		`- if len(name) > max_name_length:`
		`- raise koji.GenericError("Tag name %s is too long. Max length is %s characters",`
		`- name, max_name_length)`
		`+ verify_name_internal(name)`

		`arches = koji.parse_arches(arches, strict=True, allow_none=True)`

		`@@ -3549,6 +3542,7 @@`

		`name = kwargs.get('name')`
		`if name and tag['name'] != name:`
		`+ verify_name_internal(name)`
		`# attempt to update tag name`
		`# XXX - I'm not sure we should allow this sort of renaming anyway.`
		`# while I can see the convenience, it is an untracked change (granted`
		`@@ -3714,6 +3708,7 @@`
		`of the new repo."""`

		`context.session.assertPerm('admin')`
		`+ verify_name_internal(name)`

		`if get_external_repos(info=name):`
		`raise koji.GenericError('An external repo named "%s" already exists' % name)`
		`@@ -3792,6 +3787,7 @@`
		`repo_id = repo['id']`

		`if name and name != repo['name']:`
		`+ verify_name_internal(name)`
		`existing_id = _singleValue("""SELECT id FROM external_repo WHERE name = %(name)s""",`
		`locals(), strict=False)`
		`if existing_id is not None:`
		`@@ -4127,6 +4123,7 @@`
		`"""Edit information for an existing user."""`
		`user = get_user(userInfo, strict=True)`
		`if name and user['name'] != name:`
		`+ verify_name_user(name=name)`
		`# attempt to update user name`
		`values = {`
		`'name': name,`
		`@@ -4151,6 +4148,7 @@`
		`if old:`
		`removed.add(old)`
		`if new:`
		`+ verify_name_user(krb=new)`
		`added.add(new)`
		`dups = added & removed`
		`if dups:`
		`@@ -4820,6 +4818,7 @@`
		`def add_btype(name):`
		`"""Add a new btype with the given name"""`
		`context.session.assertPerm('admin')`
		`+ verify_name_internal(name)`
		`data = {'name': name}`
		`if list_btypes(data):`
		`raise koji.GenericError("btype already exists")`
		`@@ -5734,6 +5733,7 @@`
		`def add_volume(name, strict=True):`
		`"""Add a new storage volume in the database"""`
		`context.session.assertPerm('admin')`
		`+ verify_name_internal(name)`
		`voldir = koji.pathinfo.volumedir(name)`
		`if not os.path.isdir(voldir):`
		`raise koji.GenericError('please create the volume directory first')`
		`@@ -7317,6 +7317,7 @@`
		`:param str extensions: space-separated list of descriptions, eg. "yaml yml"`
		`"""`
		`context.session.assertPerm('admin')`
		`+ verify_name_internal(name)`
		`data = {'name': name,`
		`'description': description,`
		`'extensions': extensions,`
		`@@ -8794,6 +8795,7 @@`
		`def new_group(name):`
		`"""Add a user group to the database"""`
		`context.session.assertPerm('admin')`
		`+ verify_name_internal(name)`
		`if get_user(name):`
		`raise koji.GenericError('user/group already exists: %s' % name)`
		`return context.session.createUser(name, usertype=koji.USERTYPES['GROUP'])`
		`@@ -12405,6 +12407,8 @@`
		`def grantPermission(self, userinfo, permission, create=False):`
		`"""Grant a permission to a user"""`
		`context.session.assertPerm('admin')`
		`+ if create:`
		`+ verify_name_internal(permission)`
		`user_id = get_user(userinfo, strict=True)['id']`
		`perm = lookup_perm(permission, strict=(not create), create=create)`
		`perm_id = perm['id']`
		`@@ -12442,6 +12446,7 @@`
		`exists.`
		`"""`
		`context.session.assertPerm('admin')`
		`+ verify_name_user(username, krb_principal)`
		`if get_user(username):`
		`raise koji.GenericError('user already exists: %s' % username)`
		`if krb_principal and get_user_by_krb_principal(krb_principal):`
		`@@ -12965,6 +12970,8 @@`
		`from the HostPrincipalFormat setting (if available).`
		`"""`
		`context.session.assertPerm('host')`
		`+ verify_host_name(hostname)`
		`+`
		`# validate arches`
		`arches = " ".join(arches)`
		`arches = koji.parse_arches(arches, strict=True)`
		`@@ -15373,3 +15380,44 @@`
		`context.session.id, context.session.callnum,`
		`time.time() - start, size, fn)`
		`return ret`
		`+`
		`+`
		`+ def verify_name_internal(name):`
		`+ if not isinstance(name, str):`
		`+ raise koji.GenericError("Name should be string")`
		`+ max_name_length_internal = context.opts['MaxNameLengthInternal']`
		`+ if max_name_length_internal != 0 and len(name) > max_name_length_internal:`
		`+ raise koji.GenericError("Name %s is too long. Max length is %s characters"`
		`+ % (name, max_name_length_internal))`
		`+ if context.opts.get('RegexNameInternal.compiled'):`
		`+ regex_name_internal_compiled = context.opts['RegexNameInternal.compiled']`
		`+ if not regex_name_internal_compiled.match(name):`
		`+ raise koji.GenericError("Name %s does not match RegexNameInternal value" % name)`
		`+`
		`+`
		`+ def verify_name_user(name=None, krb=None):`
		`+ if name and not isinstance(name, str):`
		`+ raise koji.GenericError("Name should be string")`
		`+ if krb and not isinstance(krb, str):`
		`+ raise koji.GenericError("Kerberos principal should be string")`
		`+ max_name_length_internal = context.opts['MaxNameLengthInternal']`
		`+`
		`+ if max_name_length_internal != 0:`
		`+ if name and len(name) > max_name_length_internal:`
		`+ raise koji.GenericError("Name %s is too long. Max length is %s characters"`
		`+ % (name, max_name_length_internal))`
		`+ if krb and len(krb) > max_name_length_internal:`
		`+ raise koji.GenericError("Kerberos principal %s is too long. Max length is "`
		`+ "%s characters" % (krb, max_name_length_internal))`
		`+ if context.opts.get('RegexUserName.compiled'):`
		`+ regex_user_name_compiled = context.opts['RegexUserName.compiled']`
		`+ if (name is not None) and (not regex_user_name_compiled.match(name)):`
		`+ raise koji.GenericError("Name %s does not match RegexUserName value" % name)`
		`+ if (krb is not None) and (not regex_user_name_compiled.match(krb)):`
		`+ raise koji.GenericError("Kerberos principal %s does not match RegexUserName "`
		`+ "value" % krb)`
		`+`
		`+`
		`+ def verify_host_name(name):`
		`+ verify_name_internal(name)`
		`+ verify_name_user(name)`

hub/kojixmlrpc.py

file modified

		`@@ -28,6 +28,7 @@`
		`import threading`
		`import time`
		`import traceback`
		`+ import re`

		`import koji`
		`import koji.auth`
		`@@ -467,6 +468,10 @@`
		`['LockOut', 'boolean', False],`
		`['ServerOffline', 'boolean', False],`
		`['OfflineMessage', 'string', None],`
		`+`
		`+ ['MaxNameLengthInternal', 'integer', 256],`
		`+ ['RegexNameInternal', 'string', r'^[A-Za-z0-9/_.+-]+$'],`
		`+ ['RegexUserName', 'string', r'^[A-Za-z0-9/_.@-]+$']`
		`]`
		`opts = {}`
		`for name, dtype, default in cfgmap:`
		`@@ -495,6 +500,10 @@`
		`if opts.get('KojiDir') is not None:`
		`koji.BASEDIR = opts['KojiDir']`
		`koji.pathinfo.topdir = opts['KojiDir']`
		`+ if opts['RegexNameInternal'] != '':`
		`+ opts['RegexNameInternal.compiled'] = re.compile(opts['RegexNameInternal'])`
		`+ if opts['RegexUserName'] != '':`
		`+ opts['RegexUserName.compiled'] = re.compile(opts['RegexUserName'])`
		`return opts`

koji/__init__.py

file modified

		`@@ -431,6 +431,14 @@`
		`faultCode = 1023`


		`+ class NameValidationError(GenericError):`
		`+ """Raised when name validation fails`
		`+`
		`+ Currently not used, set up this error class into verify_name_internal, verify_name_user`
		`+ instead of GenericError for Koji 1.31."""`
		`+ faultCode = 1024`
		`+`
		`+`
		`class MultiCallInProgress(object):`
		`"""`
		`Placeholder class to be returned by method calls when in the process of`

tests/test_hub/test_add_archivetype.py

file modified

+16 -2

		`@@ -1,4 +1,5 @@`
		`import unittest`
		`+`
		`import mock`

		`import koji`
		`@@ -9,17 +10,19 @@`

		`class TestAddArchiveType(unittest.TestCase):`

		`+ @mock.patch('kojihub.verify_name_internal')`
		`@mock.patch('kojihub._multiRow')`
		`@mock.patch('kojihub.get_archive_type')`
		`@mock.patch('kojihub.InsertProcessor')`
		`- def test_add_archive_type(self, InsertProcessor, get_archive_type,`
		`- _multiRow):`
		`+ def test_add_archive_type(self, InsertProcessor, get_archive_type, _multiRow,`
		`+ verify_name_internal):`
		`# Not sure why mock can't patch kojihub.context, so we do this`
		`session = kojihub.context.session = mock.MagicMock()`
		`mocks = [InsertProcessor, get_archive_type, session]`
		`# It seems MagicMock will not automatically handle attributes that`
		`# start with "assert"`
		`session.assertPerm = mock.MagicMock()`
		`+ verify_name_internal.return_value = None`

		`# expected case`
		`get_archive_type.return_value = None`
		`@@ -47,3 +50,14 @@`
		`kojihub.add_archive_type('deb', 'Debian package', 'deb')`
		`InsertProcessor.assert_not_called()`
		`session.assertPerm.assert_called_with('admin')`
		`+`
		`+ # name is longer as expected`
		`+ new_archive_type = 'new-archive-type+'`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.add_archive_type(new_archive_type, 'Debian package', 'deb')`
		`+`
		`+ # not except regex rules`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.add_archive_type(new_archive_type, 'Debian package', 'deb')`

tests/test_hub/test_add_btype.py

file modified

+15 -1

		`@@ -1,4 +1,5 @@`
		`import unittest`
		`+`
		`import mock`

		`import koji`
		`@@ -9,15 +10,17 @@`

		`class TestAddBType(unittest.TestCase):`

		`+ @mock.patch('kojihub.verify_name_internal')`
		`@mock.patch('kojihub.list_btypes')`
		`@mock.patch('kojihub.InsertProcessor')`
		`- def test_add_btype(self, InsertProcessor, list_btypes):`
		`+ def test_add_btype(self, InsertProcessor, list_btypes, verify_name_internal):`
		`# Not sure why mock can't patch kojihub.context, so we do this`
		`session = kojihub.context.session = mock.MagicMock()`
		`mocks = [InsertProcessor, list_btypes, session]`
		`# It seems MagicMock will not automatically handle attributes that`
		`# start with "assert"`
		`session.assertPerm = mock.MagicMock()`
		`+ verify_name_internal.return_value = None`

		`# expected case`
		`list_btypes.return_value = None`
		`@@ -43,3 +46,14 @@`
		`kojihub.add_btype('new_btype')`
		`InsertProcessor.assert_not_called()`
		`session.assertPerm.assert_called_with('admin')`
		`+`
		`+ # name is longer as expected`
		`+ new_btype = 'new-btype+'`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.add_btype(new_btype)`
		`+`
		`+ # not except regex rules`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.add_btype(new_btype)`

tests/test_hub/test_add_channel.py

file modified

+19 -2

		`@@ -32,9 +32,11 @@`
		`self.inserts.append(insert)`
		`return insert`

		`+ @mock.patch('kojihub.verify_name_internal')`
		`@mock.patch('kojihub.get_channel')`
		`@mock.patch('kojihub._singleValue')`
		`- def test_add_channel_exists(self, _singleValue, get_channel):`
		`+ def test_add_channel_exists(self, _singleValue, get_channel, verify_name_internal):`
		`+ verify_name_internal.return_value = None`
		`get_channel.return_value = {'id': 123, 'name': self.channel_name}`
		`with self.assertRaises(koji.GenericError):`
		`self.exports.addChannel(self.channel_name)`
		`@@ -42,11 +44,13 @@`
		`_singleValue.assert_not_called()`
		`self.assertEqual(len(self.inserts), 0)`

		`+ @mock.patch('kojihub.verify_name_internal')`
		`@mock.patch('kojihub.get_channel')`
		`@mock.patch('kojihub._singleValue')`
		`- def test_add_channel_valid(self, _singleValue, get_channel):`
		`+ def test_add_channel_valid(self, _singleValue, get_channel, verify_name_internal):`
		`get_channel.return_value = {}`
		`_singleValue.side_effect = [12]`
		`+ verify_name_internal.return_value = None`

		`r = self.exports.addChannel(self.channel_name, description=self.description)`
		`self.assertEqual(r, 12)`
		`@@ -63,3 +67,16 @@`
		`_singleValue.assert_has_calls([`
		`mock.call("SELECT nextval('channels_id_seq')", strict=True)`
		`])`
		`+`
		`+ @mock.patch('kojihub.verify_name_internal')`
		`+ def test_add_channel_wrong_format(self, verify_name_internal):`
		`+ # name is longer as expected`
		`+ channel_name = 'test-channel+'`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.addChannel(channel_name)`
		`+`
		`+ # not except regex rules`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.addChannel(channel_name)`

tests/test_hub/test_add_host.py

file modified

+37 -10

		`@@ -1,6 +1,7 @@`
		`- import mock`
		`import unittest`

		`+ import mock`
		`+`
		`import koji`
		`import kojihub`

		`@@ -39,10 +40,12 @@`
		`def tearDown(self):`
		`mock.patch.stopall()`

		`+ @mock.patch('kojihub.verify_host_name')`
		`@mock.patch('kojihub._dml')`
		`@mock.patch('kojihub.get_host')`
		`@mock.patch('kojihub._singleValue')`
		`- def test_add_host_exists(self, _singleValue, get_host, _dml):`
		`+ def test_add_host_exists(self, _singleValue, get_host, _dml, verify_host_name):`
		`+ verify_host_name.return_value = None`
		`get_host.return_value = {'id': 123}`
		`with self.assertRaises(koji.GenericError):`
		`self.exports.addHost('hostname', ['i386', 'x86_64'])`
		`@@ -50,10 +53,12 @@`
		`get_host.assert_called_once_with('hostname')`
		`_singleValue.assert_not_called()`

		`+ @mock.patch('kojihub.verify_host_name')`
		`@mock.patch('kojihub._dml')`
		`@mock.patch('kojihub.get_host')`
		`@mock.patch('kojihub._singleValue')`
		`- def test_add_host_valid(self, _singleValue, get_host, _dml):`
		`+ def test_add_host_valid(self, _singleValue, get_host, _dml, verify_host_name):`
		`+ verify_host_name.return_value = None`
		`get_host.return_value = {}`
		`_singleValue.side_effect = [333, 12]`
		`self.context.session.createUser.return_value = 456`
		`@@ -63,22 +68,25 @@`

		`self.context.session.assertPerm.assert_called_once_with('host')`
		`kojihub.get_host.assert_called_once_with('hostname')`
		`- self.context.session.createUser.assert_called_once_with('hostname',`
		`- usertype=koji.USERTYPES['HOST'], krb_principal='-hostname-')`
		`+ self.context.session.createUser.assert_called_once_with(`
		`+ 'hostname', usertype=koji.USERTYPES['HOST'], krb_principal='-hostname-')`
		`self.assertEqual(_singleValue.call_count, 2)`
		`_singleValue.assert_has_calls([`
		`mock.call("SELECT id FROM channels WHERE name = 'default'"),`
		`mock.call("SELECT nextval('host_id_seq')", strict=True)`
		`])`
		`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'})`
		`+ _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.verify_host_name')`
		`@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):`
		`+ def test_add_host_wrong_user(self, _singleValue, get_host, _dml, get_user, verify_host_name):`
		`+ verify_host_name.return_value = None`
		`get_user.return_value = {`
		`'id': 1,`
		`'name': 'hostname',`
		`@@ -94,11 +102,14 @@`
		`self.assertEqual(len(self.inserts), 0)`
		`self.assertEqual(len(self.updates), 0)`

		`+ @mock.patch('kojihub.verify_host_name')`
		`@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):`
		`+ def test_add_host_wrong_user_forced(self, _singleValue, get_host, _dml, get_user,`
		`+ verify_host_name):`
		`+ verify_host_name.return_value = None`
		`get_user.return_value = {`
		`'id': 123,`
		`'name': 'hostname',`
		`@@ -120,11 +131,14 @@`
		`self.assertEqual(update.clauses, ['id = %(userID)i'])`
		`self.assertEqual(update.data, {'usertype': koji.USERTYPES['HOST']})`

		`+ @mock.patch('kojihub.verify_host_name')`
		`@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):`
		`+ def test_add_host_superwrong_user_forced(self, _singleValue, get_host, _dml, get_user,`
		`+ verify_host_name):`
		`+ verify_host_name.return_value = None`
		`get_user.return_value = {`
		`'id': 123,`
		`'name': 'hostname',`
		`@@ -141,3 +155,16 @@`
		`_singleValue.assert_called()`
		`self.assertEqual(len(self.inserts), 0)`
		`self.assertEqual(len(self.updates), 0)`
		`+`
		`+ @mock.patch('kojihub.verify_host_name')`
		`+ def test_add_host_wrong_format(self, verify_host_name):`
		`+ # name is longer as expected`
		`+ hostname = 'host-name+'`
		`+ verify_host_name.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.addHost(hostname, ['i386', 'x86_64'], force=True)`
		`+`
		`+ # not except regex rules`
		`+ verify_host_name.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.addHost(hostname, ['i386', 'x86_64'], force=True)`

tests/test_hub/test_add_host_to_channel.py

file modified

+23 -2

		`@@ -1,6 +1,7 @@`
		`- import mock`
		`import unittest`

		`+ import mock`
		`+`
		`import koji`
		`import kojihub`

		`@@ -91,17 +92,20 @@`
		`get_channel_id.assert_called_once_with(cname, create=False)`
		`self.assertEqual(len(self.inserts), 0)`

		`+ @mock.patch('kojihub.verify_name_internal')`
		`@mock.patch('kojihub.get_channel')`
		`@mock.patch('kojihub.list_channels')`
		`@mock.patch('kojihub.get_channel_id')`
		`@mock.patch('kojihub.get_host')`
		`- def test_no_channel_create(self, get_host, get_channel_id, list_channels, get_channel):`
		`+ def test_no_channel_create(self, get_host, get_channel_id, list_channels, get_channel,`
		`+ verify_name_internal):`
		`name = 'hostname'`
		`cname = 'channel_name'`
		`get_host.return_value = {'id': 123, 'name': name}`
		`get_channel_id.return_value = 456`
		`list_channels.return_value = [{'id': 1, 'name': 'default'}]`
		`get_channel.return_value = {'enabled': True}`
		`+ verify_name_internal.return_value = None`

		`kojihub.add_host_to_channel(name, cname, create=True)`

		`@@ -142,3 +146,20 @@`
		`get_channel_id.assert_called_once_with(cname, create=False)`
		`list_channels.assert_called_once_with(123)`
		`self.assertEqual(len(self.inserts), 0)`
		`+`
		`+ @mock.patch('kojihub.verify_name_internal')`
		`+ @mock.patch('kojihub.get_host')`
		`+ def test_channel_wrong_format(self, get_host, verify_name_internal):`
		`+ name = 'hostname'`
		`+ channel_name = 'test-channel+'`
		`+ get_host.return_value = {'id': 123, 'name': name}`
		`+`
		`+ # name is longer as expected`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.add_host_to_channel(name, channel_name, create=True)`
		`+`
		`+ # not except regex rules`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.add_host_to_channel(name, channel_name, create=True)`

tests/test_hub/test_add_volume.py

file added

+30

		`@@ -0,0 +1,30 @@`
		`+ import unittest`
		`+`
		`+ import mock`
		`+`
		`+ import koji`
		`+ import kojihub`
		`+`
		`+`
		`+ class TestAddVolume(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start()`
		`+ self.context = mock.patch('kojihub.context').start()`
		`+ # It seems MagicMock will not automatically handle attributes that`
		`+ # start with "assert"`
		`+ self.context.session.assertPerm = mock.MagicMock()`
		`+ self.context.session.assertLogin = mock.MagicMock()`
		`+`
		`+ def test_add_volume_wrong_format(self):`
		`+ volume_name = 'volume-name+'`
		`+`
		`+ # name is longer as expected`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.add_volume(volume_name)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.add_volume(volume_name)`

tests/test_hub/test_create_build_target.py

file added

+82

		`@@ -0,0 +1,82 @@`
		`+ # coding: utf-8`
		`+ import unittest`
		`+`
		`+ import mock`
		`+`
		`+ import koji`
		`+ import kojihub`
		`+`
		`+`
		`+ class TestCreateBuildTarget(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self.get_build_targets = mock.patch('kojihub.get_build_targets').start()`
		`+ self.get_tag = mock.patch('kojihub.get_tag').start()`
		`+ self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start()`
		`+ self.context = mock.patch('kojihub.context').start()`
		`+ # It seems MagicMock will not automatically handle attributes that`
		`+ # start with "assert"`
		`+ self.context.session.assertPerm = mock.MagicMock()`
		`+ self.context.session.assertLogin = mock.MagicMock()`
		`+ self.tag_name = 'test-tag'`
		`+ self.destination_tag_name = 'dest-test-tag'`
		`+ self.target_name = 'test_target'`
		`+ self.build_target_info = {`
		`+ 'build_tag': 2,`
		`+ 'build_tag_name': self.tag_name,`
		`+ 'dest_tag': 3,`
		`+ 'dest_tag_name': self.destination_tag_name,`
		`+ 'id': 1,`
		`+ 'name': self.target_name`
		`+ }`
		`+ self.tag_info = {'arches': '',`
		`+ 'extra': {},`
		`+ 'id': 1,`
		`+ 'locked': False,`
		`+ 'maven_include_all': False,`
		`+ 'maven_support': False,`
		`+ 'name': self.tag_name,`
		`+ 'perm': None,`
		`+ 'perm_id': None}`
		`+`
		`+ def tearDown(self):`
		`+ mock.patch.stopall()`
		`+`
		`+ def test_target_wrong_format(self):`
		`+ target_name = 'test-target+'`
		`+`
		`+ # name is longer as expected`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.create_build_target(target_name, self.tag_name, self.destination_tag_name)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.create_build_target(target_name, self.tag_name, self.destination_tag_name)`
		`+`
		`+ def test_target_exists(self):`
		`+ self.get_build_targets.return_value = self.build_target_info`
		`+ self.verify_name_internal.return_value = None`
		`+ expected = "A build target with the name '%s' already exists" % self.target_name`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.create_build_target(self.target_name, self.tag_name, self.destination_tag_name)`
		`+ self.assertEqual(expected, str(cm.exception))`
		`+`
		`+ def test_tag_not_exists(self):`
		`+ self.get_build_targets.return_value = None`
		`+ self.verify_name_internal.return_value = None`
		`+ self.get_tag.return_value = None`
		`+ expected = "build tag '%s' does not exist" % self.tag_name`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.create_build_target(self.target_name, self.tag_name, self.destination_tag_name)`
		`+ self.assertEqual(expected, str(cm.exception))`
		`+`
		`+ def test_dest_tag_not_exists(self):`
		`+ self.get_build_targets.return_value = None`
		`+ self.verify_name_internal.return_value = None`
		`+ self.get_tag.side_effect = [self.tag_info, None]`
		`+ expected = "destination tag '%s' does not exist" % self.destination_tag_name`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.create_build_target(self.target_name, self.tag_name, self.destination_tag_name)`
		`+ self.assertEqual(expected, str(cm.exception))`

tests/test_hub/test_create_external_repo.py

file added

+47

		`@@ -0,0 +1,47 @@`
		`+ # coding: utf-8`
		`+ import unittest`
		`+`
		`+ import mock`
		`+`
		`+ import koji`
		`+ import kojihub`
		`+`
		`+`
		`+ class TestCreateExternalRepo(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self.get_external_repos = mock.patch('kojihub.get_external_repos').start()`
		`+ self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start()`
		`+ self.context = mock.patch('kojihub.context').start()`
		`+ # It seems MagicMock will not automatically handle attributes that`
		`+ # start with "assert"`
		`+ self.context.session.assertPerm = mock.MagicMock()`
		`+ self.context.session.assertLogin = mock.MagicMock()`
		`+ self.repo_url = 'http://path_to_ext_repo.com'`
		`+ self.repo_name = 'test-repo'`
		`+ self.repo_info = {'id': 1, 'name': self.repo_name, 'url': self.repo_url}`
		`+`
		`+ def tearDown(self):`
		`+ mock.patch.stopall()`
		`+`
		`+ def test_create_external_repo_wrong_format(self):`
		`+ repo_name = 'test-repo+'`
		`+`
		`+ # name is longer as expected`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.create_external_repo(repo_name, self.repo_url)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.create_external_repo(repo_name, self.repo_url)`
		`+`
		`+ def test_create_external_repo_exists(self):`
		`+ expected = 'An external repo named "%s" already exists' % self.repo_name`
		`+`
		`+ self.verify_name_internal.return_value = None`
		`+ self.get_external_repos.return_value = self.repo_info`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.create_external_repo(self.repo_name, self.repo_url)`
		`+ self.assertEqual(expected, str(cm.exception))`

tests/test_hub/test_create_tag.py

file modified

+20 -2

		`@@ -1,7 +1,8 @@`
		`# coding: utf-8`
		`- import mock`
		`import unittest`

		`+ import mock`
		`+`
		`import koji`
		`import kojihub`

		`@@ -18,11 +19,12 @@`

		`def setUp(self):`
		`self.InsertProcessor = mock.patch('kojihub.InsertProcessor',`
		`- side_effect=self.getInsert).start()`
		`+ side_effect=self.getInsert).start()`
		`self.inserts = []`
		`self._dml = mock.patch('kojihub._dml').start()`
		`self.get_tag = mock.patch('kojihub.get_tag').start()`
		`self.get_tag_id = mock.patch('kojihub.get_tag_id').start()`
		`+ self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start()`
		`self.writeInheritanceData = mock.patch('kojihub.writeInheritanceData').start()`
		`self.context = mock.patch('kojihub.context').start()`
		`# It seems MagicMock will not automatically handle attributes that`
		`@@ -34,6 +36,7 @@`
		`mock.patch.stopall()`

		`def test_duplicate(self):`
		`+ self.verify_name_internal.return_value = None`
		`self.get_tag.return_value = {'name': 'duptag'}`
		`with self.assertRaises(koji.GenericError):`
		`kojihub.create_tag('duptag')`
		`@@ -41,6 +44,7 @@`
		`def test_simple_create(self):`
		`self.get_tag.return_value = None`
		`self.get_tag_id.return_value = 99`
		`+ self.verify_name_internal.return_value = None`
		`self.context.event_id = 42`
		`self.context.session.user_id = 23`
		`kojihub.create_tag('newtag')`
		`@@ -66,6 +70,7 @@`
		`def test_invalid_archs(self):`
		`self.get_tag.return_value = None`
		`self.get_tag_id.return_value = 99`
		`+ self.verify_name_internal.return_value = None`
		`self.context.event_id = 42`
		`self.context.session.user_id = 23`

		`@@ -79,3 +84,16 @@`
		`kojihub.create_tag('newtag', arches=u'arch1,arch2')`

		`self.assertEqual(len(self.inserts), 0)`
		`+`
		`+ def test_tag_wrong_format(self):`
		`+ tag_name = 'test-tag+'`
		`+`
		`+ # name is longer as expected`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.create_tag(tag_name)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.create_tag(tag_name)`

tests/test_hub/test_create_user.py

file added

+56

		`@@ -0,0 +1,56 @@`
		`+ import unittest`
		`+`
		`+ import mock`
		`+`
		`+ import koji`
		`+ import kojihub`
		`+`
		`+`
		`+ class TestCreateUser(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self.verify_name_user = mock.patch('kojihub.verify_name_user').start()`
		`+ self.get_user = mock.patch('kojihub.get_user').start()`
		`+ self.get_user_by_krb_principal = mock.patch('kojihub.get_user_by_krb_principal').start()`
		`+ self.exports = kojihub.RootExports()`
		`+ self.context = mock.patch('kojihub.context').start()`
		`+ # It seems MagicMock will not automatically handle attributes that`
		`+ # start with "assert"`
		`+ self.context.session.assertPerm = mock.MagicMock()`
		`+ self.context.session.assertLogin = mock.MagicMock()`
		`+ self.user_name = 'test_user'`
		`+ self.user_info = {'id': 1, 'krb_principals': [], 'name': self.user_name,`
		`+ 'status': 0, 'usertype': 0}`
		`+ self.user_info_krb = {'id': 1, 'krb_principals': ['test_user@fedora.org'],`
		`+ 'name': self.user_name, 'status': 0, 'usertype': 0}`
		`+`
		`+ def test_create_user_wrong_format(self):`
		`+ user_name = 'test-user+'`
		`+`
		`+ # name is longer as expected`
		`+ self.verify_name_user.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.createUser(user_name)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_user.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.createUser(user_name)`
		`+`
		`+ def test_create_user_exists(self):`
		`+ expected = 'user already exists: %s' % self.user_name`
		`+ self.verify_name_user.return_value = None`
		`+ self.get_user.return_value = self.user_info`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ self.exports.createUser(self.user_name)`
		`+ self.assertEqual(expected, str(cm.exception))`
		`+`
		`+ def test_create_user_exists_krb(self):`
		`+ krb_principal = 'test_user@fedora.org'`
		`+ expected = 'user with this Kerberos principal already exists: %s' % krb_principal`
		`+ self.verify_name_user.return_value = None`
		`+ self.get_user.return_value = None`
		`+ self.get_user_by_krb_principal.return_value = self.user_info_krb`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ self.exports.createUser(self.user_name, krb_principal=krb_principal)`
		`+ self.assertEqual(expected, str(cm.exception))`

tests/test_hub/test_edit_build_target.py

file modified

+18

		`@@ -10,11 +10,13 @@`

		`def setUp(self):`
		`self.lookup_build_target = mock.patch('kojihub.lookup_build_target').start()`
		`+ self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start()`
		`self.exports = kojihub.RootExports()`

		`def test_non_exist_build_target(self):`
		`session = kojihub.context.session = mock.MagicMock()`
		`session.assertPerm = mock.MagicMock()`
		`+ self.verify_name_internal.return_value = None`
		`target_name = 'build-target'`
		`name = 'build-target-rename'`
		`build_tag = 'tag'`
		`@@ -25,3 +27,19 @@`
		`self.assertEqual("No such build target: %s" % target_name,`
		`str(cm.exception))`
		`session.assertPerm.called_once_with('target')`
		`+`
		`+ def test_target_wrong_format(self):`
		`+ target_name = 'test-target'`
		`+ name = 'build-target-rename+'`
		`+ build_tag = 'tag'`
		`+ dest_tag = 'dest-tag'`
		`+`
		`+ # name is longer as expected`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.editBuildTarget(target_name, name, build_tag, dest_tag)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.editBuildTarget(target_name, name, build_tag, dest_tag)`

tests/test_hub/test_edit_channel.py

file modified

+27 -3

		`@@ -38,8 +38,9 @@`
		`def tearDown(self):`
		`mock.patch.stopall()`

		`+ @mock.patch('kojihub.verify_name_internal')`
		`@mock.patch('kojihub.get_channel')`
		`- def test_edit_channel_missing(self, get_channel):`
		`+ def test_edit_channel_missing(self, get_channel, verify_name_internal):`
		`expected = 'Invalid type for channelInfo: %s' % self.channel_name`
		`get_channel.side_effect = koji.GenericError(expected)`
		`with self.assertRaises(koji.GenericError) as ex:`
		`@@ -49,8 +50,10 @@`
		`self.assertEqual(self.updates, [])`
		`self.assertEqual(expected, str(ex.exception))`

		`+ @mock.patch('kojihub.verify_name_internal')`
		`@mock.patch('kojihub.get_channel')`
		`- def test_edit_channel_already_exists(self, get_channel):`
		`+ def test_edit_channel_already_exists(self, get_channel, verify_name_internal):`
		`+ verify_name_internal.return_value = None`
		`get_channel.side_effect = [`
		`{`
		`'id': 123,`
		`@@ -73,8 +76,10 @@`
		`self.assertEqual('channel %s already exists (id=124)' % self.channel_name_new,`
		`str(ex.exception))`

		`+ @mock.patch('kojihub.verify_name_internal')`
		`@mock.patch('kojihub.get_channel')`
		`- def test_edit_channel_valid(self, get_channel):`
		`+ def test_edit_channel_valid(self, get_channel, verify_name_internal):`
		`+ verify_name_internal.return_value = None`
		`kojihub.get_channel.side_effect = [{`
		`'id': 123,`
		`'name': self.channel_name,`
		`@@ -97,3 +102,22 @@`
		`self.assertEqual(update.table, 'channels')`
		`self.assertEqual(update.values, values)`
		`self.assertEqual(update.clauses, clauses)`
		`+`
		`+ @mock.patch('kojihub.verify_name_internal')`
		`+ @mock.patch('kojihub.get_channel')`
		`+ def test_edit_channel_wrong_format(self, get_channel, verify_name_internal):`
		`+ channel_name_new = 'test-channel+'`
		`+ get_channel.return_value = {'id': 123,`
		`+ 'name': self.channel_name,`
		`+ 'description': 'description',`
		`+ }`
		`+`
		`+ # name is longer as expected`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.editChannel(self.channel_name, name=channel_name_new)`
		`+`
		`+ # not except regex rules`
		`+ verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.editChannel(self.channel_name, name=channel_name_new)`

tests/test_hub/test_edit_external_repo.py

file added

+39

		`@@ -0,0 +1,39 @@`
		`+ # coding: utf-8`
		`+ import unittest`
		`+`
		`+ import mock`
		`+`
		`+ import koji`
		`+ import kojihub`
		`+`
		`+`
		`+ class TestEditExternalRepo(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self.get_external_repo = mock.patch('kojihub.get_external_repo').start()`
		`+ self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start()`
		`+ self.context = mock.patch('kojihub.context').start()`
		`+ # It seems MagicMock will not automatically handle attributes that`
		`+ # start with "assert"`
		`+ self.context.session.assertPerm = mock.MagicMock()`
		`+ self.context.session.assertLogin = mock.MagicMock()`
		`+ self.repo_url = 'http://path_to_ext_repo.com'`
		`+ self.repo_name = 'test-repo'`
		`+ self.repo_info = {'id': 1, 'name': self.repo_name, 'url': self.repo_url}`
		`+`
		`+ def tearDown(self):`
		`+ mock.patch.stopall()`
		`+`
		`+ def test_edit_external_repo_wrong_format(self):`
		`+ repo_name_new = 'test-repo+'`
		`+ self.get_external_repo.return_value = self.repo_info`
		`+`
		`+ # name is longer as expected`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.edit_external_repo(self.repo_name, repo_name_new, self.repo_url)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.edit_external_repo(self.repo_name, repo_name_new, self.repo_url)`

tests/test_hub/test_edit_tag.py

file modified

+39 -2

		`@@ -1,6 +1,8 @@`
		`# coding: utf-8`
		`- import mock`
		`import unittest`
		`+`
		`+ import mock`
		`+`
		`import koji`
		`import kojihub`

		`@@ -32,6 +34,7 @@`
		`self._singleValue = mock.patch('kojihub._singleValue').start()`
		`self.get_tag = mock.patch('kojihub.get_tag').start()`
		`self.get_perm_id = mock.patch('kojihub.get_perm_id').start()`
		`+ self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start()`
		`self.context = mock.patch('kojihub.context').start()`
		`# It seems MagicMock will not automatically handle attributes that`
		`# start with "assert"`
		`@@ -62,6 +65,7 @@`
		`'exC': 3,`
		`'exD': 4}}`
		`self._singleValue.return_value = None`
		`+ self.verify_name_internal.return_value = None`
		`self.context.event_id = 42`
		`self.context.session.user_id = 23`
		`# no1 invoke`
		`@@ -168,7 +172,8 @@`

		`with self.assertRaises(koji.GenericError) as cm:`
		`kojihub._edit_tag('tag', **kwargs)`
		`- self.assertEqual(cm.exception.args[0], 'Can not both add/update and remove tag-extra: \'exC\'')`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ 'Can not both add/update and remove tag-extra: \'exC\'')`

		`# no3 invoke`
		`kwargs = {`
		`@@ -231,3 +236,35 @@`
		`with self.assertRaises(koji.GenericError):`
		`kojihub._edit_tag('tag', **kwargs)`

		`+ def test_edit_wrong_format_tag(self):`
		`+ tag_name_new = 'new-test-tag+'`
		`+ tag_name = 'tag'`
		`+ self.get_tag.return_value = {'id': 333,`
		`+ 'perm_id': 1,`
		`+ 'name': tag_name,`
		`+ 'arches': 'arch1 arch3',`
		`+ 'locked': False,`
		`+ 'maven_support': False,`
		`+ 'maven_include_all': False,`
		`+ 'extra': {'exA': 1,`
		`+ 'exC': 3,`
		`+ 'exD': 4}}`
		`+ kwargs = {`
		`+ 'perm': None,`
		`+ 'name': tag_name_new,`
		`+ 'arches': 'arch1 arch2',`
		`+ 'locked': True,`
		`+ 'maven_support': False,`
		`+ 'maven_include_all': False,`
		`+ 'extra': {},`
		`+ 'remove_extra': []`
		`+ }`
		`+ # name is longer as expected`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub._edit_tag('tag', **kwargs)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub._edit_tag('tag', **kwargs)`

tests/test_hub/test_edit_user.py

file modified

+17 -4

		`@@ -1,11 +1,13 @@`
		`- import mock`
		`import unittest`

		`+ import mock`
		`+`
		`import koji`
		`import kojihub`

		`UP = kojihub.UpdateProcessor`

		`+`
		`class TestEditUser(unittest.TestCase):`

		`def getUpdate(self, args, *kwargs):`
		`@@ -18,6 +20,7 @@`
		`self.updates = []`
		`self._singleValue = mock.patch('kojihub._singleValue').start()`
		`self.get_user = mock.patch('kojihub.get_user').start()`
		`+ self.verify_name_user = mock.patch('kojihub.verify_name_user').start()`
		`self.context = mock.patch('kojihub.context').start()`
		`self.UpdateProcessor = mock.patch('kojihub.UpdateProcessor',`
		`side_effect=self.getUpdate).start()`
		`@@ -33,6 +36,7 @@`
		`'name': 'user',`
		`'krb_principals': ['krb']}`
		`self._singleValue.return_value = None`
		`+ self.verify_name_user.return_value = None`

		`kojihub._edit_user('user', name='newuser')`
		`# check the update`
		`@@ -43,8 +47,7 @@`
		`self.assertEqual(update.values, {'userID': 333})`
		`self.assertEqual(update.clauses, ['id = %(userID)i'])`

		`- kojihub._edit_user('user', krb_principal_mappings=[{'old': 'krb',`
		`- 'new': 'newkrb'}])`
		`+ 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. \`
		`@@ -85,7 +88,6 @@`
		`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:`
		`@@ -93,3 +95,14 @@`
		`self._singleValue.assert_called_once()`
		`self.assertEqual(cm.exception.args[0],`
		`'Name newuser already taken by user 2')`
		`+`
		`+ # name is longer as expected`
		`+ new_username = 'new-user+'`
		`+ self.verify_name_user.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub._edit_user('user', name=new_username)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_user.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub._edit_user('user', name=new_username)`

tests/test_hub/test_grant_permissions.py

file added

+32

		`@@ -0,0 +1,32 @@`
		`+ import unittest`
		`+`
		`+ import mock`
		`+`
		`+ import koji`
		`+ import kojihub`
		`+`
		`+`
		`+ class TestGrantPermission(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start()`
		`+ self.exports = kojihub.RootExports()`
		`+ self.context = mock.patch('kojihub.context').start()`
		`+ # It seems MagicMock will not automatically handle attributes that`
		`+ # start with "assert"`
		`+ self.context.session.assertPerm = mock.MagicMock()`
		`+ self.context.session.assertLogin = mock.MagicMock()`
		`+ self.user_name = 'test_user'`
		`+`
		`+ def test_grant_permission_wrong_format(self):`
		`+ perms_name = 'test-perms+'`
		`+`
		`+ # name is longer as expected`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.grantPermission(self.user_name, perms_name, create=True)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.grantPermission(self.user_name, perms_name, create=True)`

tests/test_hub/test_user_groups.py

file modified

+19 -6

		`@@ -1,6 +1,7 @@`
		`- import mock`
		`import unittest`

		`+ import mock`
		`+`
		`import koji`
		`import kojihub`

		`@@ -8,6 +9,7 @@`
		`IP = kojihub.InsertProcessor`
		`UP = kojihub.UpdateProcessor`

		`+`
		`class TestGrouplist(unittest.TestCase):`
		`def getQuery(self, args, *kwargs):`
		`query = QP(args, *kwargs)`
		`@@ -31,25 +33,27 @@`
		`def setUp(self):`
		`self.context = mock.patch('kojihub.context').start()`
		`self.get_user = mock.patch('kojihub.get_user').start()`
		`+ self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start()`
		`# It seems MagicMock will not automatically handle attributes that`
		`# start with "assert"`
		`self.context.session.assertPerm = mock.MagicMock()`
		`self.context.session.assertLogin = mock.MagicMock()`

		`self.QueryProcessor = mock.patch('kojihub.QueryProcessor',`
		`- side_effect=self.getQuery).start()`
		`+ side_effect=self.getQuery).start()`
		`self.queries = []`
		`self.InsertProcessor = mock.patch('kojihub.InsertProcessor',`
		`- side_effect=self.getInsert).start()`
		`+ side_effect=self.getInsert).start()`
		`self.inserts = []`
		`self.UpdateProcessor = mock.patch('kojihub.UpdateProcessor',`
		`- side_effect=self.getUpdate).start()`
		`+ side_effect=self.getUpdate).start()`
		`self.updates = []`

		`def tearDown(self):`
		`mock.patch.stopall()`

		`def test_new_group(self):`
		`+ self.verify_name_internal.return_value = None`
		`name = 'test_group'`

		`# insufficient permissions`
		`@@ -74,6 +78,17 @@`
		`self.context.session.assertPerm.assert_called_with('admin')`
		`self.context.session.createUser.called_with(name, usertype=koji.USERTYPES['GROUP'])`

		`+ # name is longer as expected`
		`+ name = 'new-group+'`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.new_group(name)`
		`+`
		`+ # not except regex rules`
		`+ self.verify_name_internal.side_effect = koji.GenericError`
		`+ with self.assertRaises(koji.GenericError):`
		`+ kojihub.new_group(name)`
		`+`
		`def test_add_group_member(self):`
		`group, gid = 'test_group', 1`
		`user, uid = 'username', 2`
		`@@ -160,7 +175,6 @@`
		`self.assertEqual(i.data['group_id'], gid)`
		`self.assertEqual(i.data['user_id'], uid)`

		`-`
		`@mock.patch('kojihub.get_group_members')`
		`def test_drop_group_member(self, get_group_members):`
		`group, gid = 'test_group', 1`
		`@@ -244,7 +258,6 @@`
		`self.assertEqual(u.values['user_id'], uid)`
		`self.assertEqual(u.values['group_id'], gid)`

		`-`
		`@mock.patch('kojihub._multiRow')`
		`def test_get_group_members(self, _multiRow):`
		`group, gid = 'test_group', 1`

tests/test_hub/test_verify_names.py

file added

+86

		`@@ -0,0 +1,86 @@`
		`+ import unittest`
		`+`
		`+ import mock`
		`+ import re`
		`+`
		`+ import koji`
		`+ import kojihub`
		`+`
		`+`
		`+ class TestVerifyNameInternal(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self.context = mock.patch('kojihub.context').start()`
		`+ self.context.session.assertPerm = mock.MagicMock()`
		`+ self.context.opts = {'MaxNameLengthInternal': 15,`
		`+ 'RegexNameInternal.compiled': re.compile('^[A-Za-z0-9/_.+-]+$')}`
		`+`
		`+ def test_verify_name_internal_integer_type(self):`
		`+ expected_error = "Name should be string"`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.verify_name_internal(1234)`
		`+ self.assertEqual(expected_error, str(cm.exception))`
		`+`
		`+ def test_verify_name_internal_longer(self):`
		`+ name = 'test-name-internal-longer'`
		`+ expected_error = "Name %s is too long. Max length is 15 characters" % name`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.verify_name_internal(name)`
		`+ self.assertEqual(expected_error, str(cm.exception))`
		`+`
		`+ def test_verify_name_internal_wrong_chars(self):`
		`+ name = 'test-name@#'`
		`+ expected_error = "Name %s does not match RegexNameInternal value" % name`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.verify_name_internal(name)`
		`+ self.assertEqual(expected_error, str(cm.exception))`
		`+`
		`+`
		`+ class TestVerifyUser(unittest.TestCase):`
		`+ def setUp(self):`
		`+ self.context = mock.patch('kojihub.context').start()`
		`+ self.context.session.assertPerm = mock.MagicMock()`
		`+ self.context.opts = {'MaxNameLengthInternal': 15,`
		`+ 'RegexUserName.compiled': re.compile('^[A-Za-z0-9/_.@-]+$')}`
		`+`
		`+ def test_verify_user_type_name(self):`
		`+ expected_error = "Name should be string"`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.verify_name_user(name=1234)`
		`+ self.assertEqual(expected_error, str(cm.exception))`
		`+`
		`+ def test_verify_user_type_kerberos(self):`
		`+ expected_error = "Kerberos principal should be string"`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.verify_name_user(name='test-name', krb=1234)`
		`+ self.assertEqual(expected_error, str(cm.exception))`
		`+`
		`+ def test_verify_user_name_longer(self):`
		`+ name = 'test-user-longer'`
		`+ expected_error = "Name %s is too long. Max length is 15 characters" % name`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.verify_name_user(name=name)`
		`+ self.assertEqual(expected_error, str(cm.exception))`
		`+`
		`+ def test_verify_user_kerberos_longer(self):`
		`+ name = 'test-user'`
		`+ krb = 'testuser@kerberos-test.com'`
		`+ expected_error = "Kerberos principal %s is too long. Max length is 15 characters" % krb`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.verify_name_user(name=name, krb=krb)`
		`+ self.assertEqual(expected_error, str(cm.exception))`
		`+`
		`+ def test_verify_user_name_wrong_chars(self):`
		`+ name = 'test-name+@#'`
		`+ expected_error = "Name %s does not match RegexUserName value" % name`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.verify_name_user(name=name)`
		`+ self.assertEqual(expected_error, str(cm.exception))`
		`+`
		`+ def test_verify_user_kerberos_wrong_chars(self):`
		`+ name = 'test-name'`
		`+ krb = 'user+@test.com'`
		`+ expected_error = "Kerberos principal %s does not match RegexUserName value" % krb`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.verify_name_user(name=name, krb=krb)`
		`+ self.assertEqual(expected_error, str(cm.exception))`

jcupova commented 2 years ago

Fixes: https://pagure.io/koji/issue/2117

rebased onto 871721e0500c75a881512011744c5381f7d7ac78

2 years ago

mikem commented 2 years ago

Notes from the discussion today:

separate options for name length
clearer name for RegexInternal option
treat max length setting of 0 as no limit
treat blank regex as no regex check
set the default regex values in kojixmlrpc

rebased onto f87fe3f7782e91b2f3e483aa9cc0e6a41a69d0af

2 years ago

jcupova commented 2 years ago

@mikem , @tkopecek all should be correct based on yesterday's discussions.

tkopecek commented on line 8 of hub/hub.conf 2 years ago

I would limit internal values to ASCII only (this one matches also unicode chars)

tkopecek commented 2 years ago

It is also worth some documentation in server setup.

tkopecek commented on line 205 of hub/kojihub.py 2 years ago

regex could be compiled during reading options, so it is not compiled every time

rebased onto 81e24a5c6b7626661cb204a206f0b0f1e0c015d8

2 years ago

tkopecek commented 2 years ago

:thumbsup: Only concern I've seen is MBS which is creating tags based on package names. In such case internal regexp would be the same as external. Not sure what we want to do about it but it should be part of another PR probably.

rebased onto decfa59de5d2afbbc51faa9f161458b076a991bf

2 years ago

ktdreyer commented on line 10 of docs/source/kojihub_conf.rst 2 years ago

What is an "internal" name? Can we just call this a "resource" name?

ktdreyer commented on line 5 of docs/source/kojihub_conf.rst 2 years ago

I think we should remove the second sentence about "They can occur anywhere". I think most people will reasonably know that because that is how configuration files generally work.

tkopecek commented 2 years ago

Next step should be "external" ones - like package names where other rules (more general could be applied) But better name is still a good suggestion.

ktdreyer commented on line 21 of docs/source/kojihub_conf.rst 2 years ago

If "internal" names are different than vs "user" names, why is "user" in this list?

ktdreyer commented on line 24 of docs/source/kojihub_conf.rst 2 years ago

What are the default values if the values are commented out or deleted?

ktdreyer commented on line 178 of hub/kojihub.py 2 years ago

I think we should introduce another error class here, like "NameValidationError", instead of using the basic GenericError class for everything. It makes it difficult for callers to programmatically distinguish this (a user input problem) from anything else.

ktdreyer commented on line 186 of hub/kojihub.py 2 years ago

If a user sees a message that their chosen value "does not comply with the rules", that error message is is vauge and hard to understand at a glance. Would you please change this to "Name %s does not match RegexNameInternal setting %s"? That will make it much easier for the user to see the regex that the hub allows, and adjust their string accordingly.

ktdreyer commented on line 9 of hub/hub.conf 2 years ago

I'm wondering why we give this much control to users over usernames?

Is it a security thing?

Why would users ever enable this setting?

mikem commented 2 years ago

What is an "internal" name? Can we just call this a "resource" name?

Internal means the field originates from Koji and so we can reasonably impose restrictions on it, unlike, say, the NVR fields where the data comes from systems outside of Koji.

I'm open to adjusting the terminology, but I'm not sure "resource" is quite right for what we're distinguishing.

mikem commented 2 years ago

If "internal" names are different than vs "user" names, why is "user" in this list?

user names were identified as not purely internal because they often come directly from external systems (i.e. kerberos). Hence we didn't feel we could apply the same rules to them as, say, tag names.

The inclusion in this list is probably just a typo, perhaps from an earlier version of the patch.

mikem commented 2 years ago

I'm wondering why we give this much control to users over usernames?

Is it a security thing?

Why would users ever enable this setting?

We're giving Koji system administrators control over what a valid username is. An admin might need this setting if their existing set of user names falls outside of our defaults.

mikem commented 2 years ago

I think we should introduce another error class here, like "NameValidationError"

There's a larger issue here of refactoring our exception classes. The tricky part is that adding a new class requires both server and client to know about it. If we add a new exception class, then a client might not know about it and will be unable to convert the Fault.

That's not to say we can't add new exception classes, but it would be better to add them in the client before we start returning them from the hub, if at all possible.

At any rate, I don't think this needs to be dealt with in this PR. No existing exception seems to be a better fit than GenericError for now.

jcupova commented 2 years ago

What are the default values if the values are commented out or deleted?

Default values are set up in hub/kojixmlrpc.py. In hub.conf, there are commented out examples, how user can set up other rules for options.

mikem commented 2 years ago

I'm not sure about the docs placement. It's definitely good to document this, and it does make sense to have a doc page about the hub.conf options, but I'm not sure about starting out with a mostly incomplete doc that only covers the new options. It might be better to, for now, add these in the server howto doc, where some of the other options are currently documented. Later we could have a separate PR to pull out and expand that content into a complete doc.

Also, the config file is hub.conf, not kojihub.conf.

mikem commented 2 years ago

There are a couple places where verify_name_internal is applied for values that are effectively user names.

In new_group, we should check verify_name_user, because groups are implemented as a special type of user.

In addHost, we have a similar situation. Hosts get user entries with the same name, so we should apply user rules here. It might actually make sense to apply both.

mikem commented 2 years ago

I was a little puzzled as to why we reorder the word when we assign RegexNameInternal to the local var name_regex_internal (and similarly RegexUserName → name_regex_username).

I was a little surprised that we store the compiled form in context.opts. I guess it's good to compile at load time so that we'll have a startup error on bad config, but it does make the field somewhat misnamed.

mikem commented 2 years ago

The error messages "...does not comply with the rules" could be more specific. Would be good to indicate which configuration option is causing this.

jcupova commented 2 years ago

I'm not sure about the docs placement. It's definitely good to document this, and it does make sense to have a doc page about the hub.conf options, but I'm not sure about starting out with a mostly incomplete doc that only covers the new options. It might be better to, for now, add these in the server howto doc, where some of the other options are currently documented. Later we could have a separate PR to pull out and expand that content into a complete doc.

Also, the config file is hub.conf, not kojihub.conf.

Mike, I have a plan to continue with this doc page after merge this PR for document hub.conf options, not only this one :) .

rebased onto daee6557e4c9848278f7f9921c101dee4e59e301

2 years ago

jcupova commented 2 years ago

@tkopecek @mikem @ktdreyer all should be fixed, if it is not fixed, I commented here more up.

mikem commented 2 years ago

I see you've renamed the docs file. However the title line and first paragraph still refer to kojihub.conf.

It would be good to
1. clearly indicate that this file is incomplete and a work in progress
2. link it from the portion of server_howto that talks about hub.conf

A few grammar adjustments

is {+a+} standard .ini-like configuration file

contains {+the+} following options

-Verification host names is in the internal names and user name group, because we are using both
-verification there.
+Host names are listed in both groups because hosts always have an associated user entry.

mikem commented 2 years ago

The error messages are better, thank you, but I don't know if we need to include the actual regex. Maybe? On one hand, it might help the user self correct, on the other, it might make the error too busy.

In any case, we don't need to terminate the message with a period. Our other error messages do not do that.

mikem commented 2 years ago

A global in the koji namespace is the wrong place to store instance specific configuration data. Furthermore, it is not thread-safe.

Also, you don't set these values in the opts['Regex*'] == '' cases, which would lead to NameErrors. However, the unit tests pass. This suggests we are missing an important test case.

There is not a 100% ideal solution, but I think if we're going to compile the regexes at startup time, then storing them in opts under an alternate name is probably best. Perhaps RegexNameInternal.compiled or similar?

rebased onto 9c99d2bd993c7f9ca6df5de6e1c7dd96011542f9

2 years ago

rebased onto 409a1175be30c7f73bf694f81e4e3fa0fb5b1924

2 years ago

mikem commented 2 years ago

This seems to access context.opts['RegexNameInternal.compiled'] unconditionally, but it is not set if RegexNameInternal == ''. We can probably just wrap the check in if context.opts['RegexNameInternal]. Similar for RegexUserName.compiled.

mikem commented 2 years ago

I'm not sure the page title is the best place to mark the doc in progress. When the docs are viewed it shows in the nav column.

I think I'd prefer to just have a short section at the top that indicates the status. Perhaps something like:

Incomplete document
^^^^^^^^^^^^^^^^^^^

This document is a stub and does not cover all options.
Work to complete this document is tracked in `Issue 3073 <https://pagure.io/koji/issue/3073>`_

The old :doc:`Server HOWTO <server_howto>` doc also describes some hub configuration options.

mikem commented 2 years ago

/tmp/tmp_zel_vjq/docs/source/server_howto.rst:780: WARNING: unknown document: <hub_conf>

I think you need to include a title for the form of reference you've used.

mikem commented 2 years ago

Also, I've filed #3073 for the follow up doc work

rebased onto 2c30efa104b39ca960de35f5cd65e6687d0b6b72

2 years ago

jcupova commented 2 years ago

@mikem updated and I taken #3073 and added to the next sprint.

mikem commented 2 years ago

The docs changes look good, but I don't see where you addressed this part:

This seems to access context.opts['RegexNameInternal.compiled'] unconditionally, but it is not set if RegexNameInternal == ''. We can probably just wrap the check in if context.opts['RegexNameInternal]. Similar for RegexUserName.compiled.

rebased onto 012ecb524d8936f5b1d030f60a1922e7d2158cbf

2 years ago

jcupova commented 2 years ago

Ha, sorry, now updated with this change also. :-)

mikem commented 2 years ago

:thumbsup:

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

rebased onto 58d216f028bf3c93536f87b885f0625767f27887

2 years ago

rebased onto 89bbf8fd1318bfada695ed00a0c02395b69566dc

2 years ago

rebased onto 81d9eae39e86595e0ceb8836d9342808e63fb88f

2 years ago

rebased onto 198e9119973ef3b228a36ffe2e37d2feb909367b

2 years ago

rebased onto ff49b3af3c7979fab93ba8aaa3049c0d50cc837c

2 years ago

Metadata Update from @jobrauer:
- Pull-request tagged with: testing-done

2 years ago

Metadata Update from @jcupova:
- Pull-request untagged with: testing-done, testing-ready

2 years ago

jcupova commented 2 years ago

In the QE testing was found a problem, when user create for example user by cli/hub, and there is used some regex. After create it, user can see this new user in list of users on webUI and when he click to the user for more info, he can get an error message 'koji.GenericError: Invalid int/label value: xxx', because regex for webUI is not the same as for API/CLI. Because tomorrow is code freeze, we don't want to release it with some quick fix. There could be some next hidden problem with limit on name value.

rebased onto 2890abfceafceefdfd307f54a8ec96c0d5134b0f

2 years ago

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

2 years ago

Metadata Update from @jcupova:
- Pull-request untagged with: testing-done

2 years ago

rebased onto 74ac826

2 years ago

pretty please pagure-ci rebuild

2 years ago