From 0b67146a16fa0505cabd1a85a3e91bffb79d47c8 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 07 2020 12:47:19 +0000 Subject: Emit user in PackageListChange messages Fixes: https://pagure.io/koji/issue/1035 --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 6f205b8..ad9b7d9 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -966,8 +966,11 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force, 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) + user = get_user(context.session.user_id) + koji.plugin.run_callbacks('prePackageListChange', action=action, + tag=tag, package=pkg, owner=owner, + block=block, extra_arches=extra_arches, + force=force, update=update, user=user) # first check to see if package is: # already present (via inheritance) # blocked @@ -1012,8 +1015,10 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force, _pkglist_add(tag_id, pkg['id'], owner, block, extra_arches) elif changed_owner: _pkglist_owner_add(tag_id, pkg['id'], owner) - koji.plugin.run_callbacks('postPackageListChange', action=action, tag=tag, package=pkg, owner=owner, - block=block, extra_arches=extra_arches, force=force, update=update) + koji.plugin.run_callbacks('postPackageListChange', action=action, + tag=tag, package=pkg, owner=owner, + block=block, extra_arches=extra_arches, + force=force, update=update, user=user) def pkglist_remove(taginfo, pkginfo, force=False): """Remove package from the list for tag @@ -1036,9 +1041,10 @@ def _direct_pkglist_remove(taginfo, pkginfo, force=False, policy=False): #don't check policy for admins using force if not (force and context.session.hasPerm('admin')): assert_policy('package_list', policy_data) - koji.plugin.run_callbacks('prePackageListChange', action='remove', tag=tag, package=pkg) + user = get_user(context.session.user_id) + koji.plugin.run_callbacks('prePackageListChange', action='remove', tag=tag, package=pkg, user=user) _pkglist_remove(tag['id'], pkg['id']) - koji.plugin.run_callbacks('postPackageListChange', action='remove', tag=tag, package=pkg) + koji.plugin.run_callbacks('postPackageListChange', action='remove', tag=tag, package=pkg, user=user) def pkglist_block(taginfo, pkginfo, force=False): @@ -1058,7 +1064,8 @@ def pkglist_unblock(taginfo, pkginfo, force=False): #don't check policy for admins using force if not (force and context.session.hasPerm('admin')): assert_policy('package_list', policy_data) - koji.plugin.run_callbacks('prePackageListChange', action='unblock', tag=tag, package=pkg) + user = get_user(context.session.user_id) + koji.plugin.run_callbacks('prePackageListChange', action='unblock', tag=tag, package=pkg, user=user) tag_id = tag['id'] pkg_id = pkg['id'] pkglist = readPackageList(tag_id, pkgID=pkg_id, inherit=True) @@ -1078,7 +1085,7 @@ def pkglist_unblock(taginfo, pkginfo, force=False): pkglist = readPackageList(tag_id, pkgID=pkg_id, inherit=True) if pkg_id not in pkglist or pkglist[pkg_id]['blocked']: _pkglist_add(tag_id, pkg_id, previous['owner_id'], False, previous['extra_arches']) - koji.plugin.run_callbacks('postPackageListChange', action='unblock', tag=tag, package=pkg) + koji.plugin.run_callbacks('postPackageListChange', action='unblock', tag=tag, package=pkg, user=user) def pkglist_setowner(taginfo, pkginfo, owner, force=False): """Set the owner for package in tag""" diff --git a/plugins/hub/protonmsg.py b/plugins/hub/protonmsg.py index fdc1aab..3f164d7 100644 --- a/plugins/hub/protonmsg.py +++ b/plugins/hub/protonmsg.py @@ -158,7 +158,8 @@ def prep_package_list_change(cbtype, *args, **kws): props = {'type': cbtype[4:], 'tag': kws['tag']['name'], 'package': kws['package']['name'], - 'action': kws['action']} + 'action': kws['action'], + 'user': kws['user']['name']} queue_msg(address, props, kws) @convert_datetime diff --git a/tests/test_hub/test_dist_repo.py b/tests/test_hub/test_dist_repo.py index bc641e5..f685908 100644 --- a/tests/test_hub/test_dist_repo.py +++ b/tests/test_hub/test_dist_repo.py @@ -89,6 +89,7 @@ class TestDistRepo(unittest.TestCase): @mock.patch('kojihub.make_task') def test_DistRepo(self, make_task, dist_repo_init): session = kojihub.context.session = mock.MagicMock() + session.user_id = 123 # It seems MagicMock will not automatically handle attributes that # start with "assert" session.assertPerm = mock.MagicMock() @@ -210,7 +211,8 @@ class TestDistRepoMove(unittest.TestCase): def test_distRepoMove(self): - kojihub.context.session = mock.MagicMock() + session = kojihub.context.session = mock.MagicMock() + session.user_id = 123 exports = kojihub.HostExports() exports.distRepoMove(self.rinfo['id'], self.uploadpath, self.arch) # check result diff --git a/tests/test_hub/test_pkglist.py b/tests/test_hub/test_pkglist.py index 731abb6..1865908 100644 --- a/tests/test_hub/test_pkglist.py +++ b/tests/test_hub/test_pkglist.py @@ -8,12 +8,26 @@ except ImportError: import koji import kojihub +def get_user_factory(data): + def get_user(userInfo, strict=False): + if isinstance(userInfo, int): + key = 'id' + else: + key = 'name' + for ui in data: + if ui[key] == userInfo: + return ui + if strict: + raise koji.GenericError(user_id) + return get_user + class TestPkglistBlock(unittest.TestCase): def setUp(self): self.context = mock.patch('kojihub.context').start() # It seems MagicMock will not automatically handle attributes that # start with "assert" self.context.session.assertLogin = mock.MagicMock() + self.context.session.user_id = 112233 self.run_callbacks = mock.patch('koji.plugin.run_callbacks').start() def tearDown(self): @@ -59,8 +73,8 @@ class TestPkglistBlock(unittest.TestCase): _pkglist_remove.assert_called_once_with(tag['id'], pkg['id']) self.assertEqual(self.run_callbacks.call_count, 2) self.run_callbacks.assert_has_calls([ - mock.call('prePackageListChange', action='unblock', tag=tag, package=pkg), - mock.call('postPackageListChange', action='unblock', tag=tag, package=pkg), + mock.call('prePackageListChange', action='unblock', tag=tag, package=pkg, user=None), + mock.call('postPackageListChange', action='unblock', tag=tag, package=pkg, user=None), ]) @mock.patch('kojihub._pkglist_remove') @@ -178,10 +192,14 @@ class TestPkglistBlock(unittest.TestCase): policy=True tag = {'id': 1, 'name': 'tag'} pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3} - user = {'id': 3, 'name': 'user'} + users = [ + {'id': 3, 'name': 'user'}, + {'id': 112233, 'name': 'user'}, + ] + user = users[0] get_tag.return_value = tag lookup_package.return_value = pkg - get_user.return_value = user + get_user.side_effect = get_user_factory(users) readPackageList.return_value = {} @@ -191,17 +209,22 @@ class TestPkglistBlock(unittest.TestCase): get_tag.assert_called_once_with(tag['name'], strict=True) lookup_package.assert_called_once_with(pkg['name'], strict=False) - get_user.assert_called_once_with(user['name'], strict=True) + get_user.assert_has_calls([ + mock.call(user['name'], strict=True), + mock.call(112233), + ]) assert_policy.assert_called_once_with('package_list', {'tag': tag['id'], 'action': 'add', 'package': pkg['name'], 'force': False}) self.assertEqual(self.run_callbacks.call_count, 2) self.run_callbacks.assert_has_calls([ mock.call('prePackageListChange', action='add', tag=tag, package=pkg, owner=user['id'], block=block, - extra_arches=extra_arches, force=force, update=update), + extra_arches=extra_arches, force=force, update=update, + user=users[1]), mock.call('postPackageListChange', action='add', tag=tag, package=pkg, owner=user['id'], block=block, - extra_arches=extra_arches, force=force, update=update), + extra_arches=extra_arches, force=force, update=update, + user=users[1]), ]) _pkglist_add.assert_called_once_with(tag['id'], pkg['id'], user['id'], block, extra_arches) @@ -282,10 +305,14 @@ class TestPkglistBlock(unittest.TestCase): policy=True tag = {'id': 1, 'name': 'tag'} pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3} - user = {'id': 3, 'name': 'user'} + users = [ + {'id': 3, 'name': 'user'}, + {'id': 112233, 'name': 'user1'}, + ] + user = users[0] get_tag.return_value = tag lookup_package.side_effect = [None, pkg] - get_user.return_value = user + get_user.side_effect = get_user_factory(users) readPackageList.return_value = {} @@ -299,17 +326,22 @@ class TestPkglistBlock(unittest.TestCase): mock.call(pkg['name'], strict=False), mock.call(pkg['name'], create=True), ) - get_user.assert_called_once_with(user['name'], strict=True) + get_user.assert_has_calls([ + mock.call(user['name'], strict=True), + mock.call(112233), + ]) assert_policy.assert_called_once_with('package_list', {'tag': tag['id'], 'action': 'add', 'package': pkg['name'], 'force': False}) self.assertEqual(self.run_callbacks.call_count, 2) self.run_callbacks.assert_has_calls([ mock.call('prePackageListChange', action='add', tag=tag, package=pkg, owner=user['id'], block=block, - extra_arches=extra_arches, force=force, update=update), + extra_arches=extra_arches, force=force, update=update, + user=users[1]), mock.call('postPackageListChange', action='add', tag=tag, package=pkg, owner=user['id'], block=block, - extra_arches=extra_arches, force=force, update=update), + extra_arches=extra_arches, force=force, update=update, + user=users[1]), ]) _pkglist_add.assert_called_once_with(tag['id'], pkg['id'], user['id'], block, extra_arches) @@ -330,10 +362,14 @@ class TestPkglistBlock(unittest.TestCase): policy=True tag = {'id': 1, 'name': 'tag'} pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3} - user = {'id': 3, 'name': 'user'} + users = [ + {'id': 3, 'name': 'user',}, + {'id': 112233, 'name': 'user1'}, + ] + user = users[0] get_tag.return_value = tag lookup_package.return_value = pkg - get_user.return_value = user + get_user.side_effect = get_user_factory(users) readPackageList.return_value = {pkg['id']: { 'owner_id': pkg['owner_id'], 'blocked': True, @@ -347,13 +383,17 @@ class TestPkglistBlock(unittest.TestCase): get_tag.assert_called_once_with(tag['name'], strict=True) lookup_package.assert_called_once_with(pkg['name'], strict=False) - get_user.assert_called_once_with(user['name'], strict=True) + get_user.assert_has_calls([ + mock.call(user['name'], strict=True), + mock.call(112233), + ]) assert_policy.assert_called_once_with('package_list', {'tag': tag['id'], 'action': 'add', 'package': pkg['name'], 'force': False}) self.run_callbacks.assert_called_once_with( 'prePackageListChange', action='add', tag=tag, package=pkg, owner=user['id'], block=block, - extra_arches=extra_arches, force=force, update=update) + extra_arches=extra_arches, force=force, update=update, + user=users[1]) _pkglist_add.assert_not_called() @mock.patch('kojihub._pkglist_add') @@ -372,10 +412,14 @@ class TestPkglistBlock(unittest.TestCase): policy=True tag = {'id': 1, 'name': 'tag'} pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3} - user = {'id': 3, 'name': 'user'} + users = [ + {'id': 3, 'name': 'user',}, + {'id': 112233, 'name': 'user1'}, + ] + user = users[0] get_tag.return_value = tag lookup_package.return_value = pkg - get_user.return_value = user + get_user.side_effect = get_user_factory(users) readPackageList.return_value = {pkg['id']: { 'owner_id': pkg['owner_id'], 'blocked': True, @@ -388,7 +432,10 @@ class TestPkglistBlock(unittest.TestCase): get_tag.assert_called_once_with(tag['name'], strict=True) lookup_package.assert_called_once_with(pkg['name'], strict=False) - get_user.assert_called_once_with(user['name'], strict=True) + get_user.assert_has_calls([ + mock.call(user['name'], strict=True), + mock.call(112233), + ]) # force + admin assert_policy.assert_not_called() @@ -396,10 +443,12 @@ class TestPkglistBlock(unittest.TestCase): self.run_callbacks.assert_has_calls([ mock.call('prePackageListChange', action='add', tag=tag, package=pkg, owner=user['id'], block=block, - extra_arches=extra_arches, force=force, update=update), + extra_arches=extra_arches, force=force, update=update, + user=users[1]), mock.call('postPackageListChange', action='add', tag=tag, package=pkg, owner=user['id'], block=block, - extra_arches=extra_arches, force=force, update=update), + extra_arches=extra_arches, force=force, update=update, + user=users[1]), ]) _pkglist_add.assert_called_once_with(tag['id'], pkg['id'], user['id'], block, extra_arches) diff --git a/tests/test_plugins/test_protonmsg.py b/tests/test_plugins/test_protonmsg.py index 394b0d7..04d63ab 100644 --- a/tests/test_plugins/test_protonmsg.py +++ b/tests/test_plugins/test_protonmsg.py @@ -44,9 +44,10 @@ class TestProtonMsg(unittest.TestCase): package={'name': 'test-pkg'}, owner=1, block=False, extra_arches='i386 x86_64', - force=False, update=False) + force=False, update=False, + user={'name': 'username'}) self.assertMsg('package.add', type='PackageListChange', tag='test-tag', - package='test-pkg', action='add') + package='test-pkg', action='add', user='username') def test_prep_package_list_change_update(self): protonmsg.prep_package_list_change('postPackageListChange', @@ -54,9 +55,10 @@ class TestProtonMsg(unittest.TestCase): package={'name': 'test-pkg'}, owner=1, block=False, extra_arches='i386 x86_64', - force=False, update=False) + force=False, update=False, + user={'name': 'username'}) self.assertMsg('package.update', type='PackageListChange', tag='test-tag', - package='test-pkg', action='update') + package='test-pkg', action='update', user='username') def test_prep_package_list_change_block(self): protonmsg.prep_package_list_change('postPackageListChange', @@ -64,23 +66,26 @@ class TestProtonMsg(unittest.TestCase): package={'name': 'test-pkg'}, owner=1, block=False, extra_arches='i386 x86_64', - force=False, update=False) + force=False, update=False, + user={'name': 'username'}) self.assertMsg('package.block', type='PackageListChange', tag='test-tag', - package='test-pkg', action='block') + package='test-pkg', action='block', user='username') def test_prep_package_list_change_unblock(self): protonmsg.prep_package_list_change('postPackageListChange', action='unblock', tag={'name': 'test-tag'}, - package={'name': 'test-pkg'}) + package={'name': 'test-pkg'}, + user={'name': 'username'}) self.assertMsg('package.unblock', type='PackageListChange', tag='test-tag', - package='test-pkg', action='unblock') + package='test-pkg', action='unblock', user='username') def test_prep_package_list_change_remove(self): protonmsg.prep_package_list_change('postPackageListChange', action='remove', tag={'name': 'test-tag'}, - package={'name': 'test-pkg'}) + package={'name': 'test-pkg'}, + user={'name': 'username'}) self.assertMsg('package.remove', type='PackageListChange', tag='test-tag', - package='test-pkg', action='remove') + package='test-pkg', action='remove', user='username') def test_prep_task_state_change(self): info = {'id': 5678,