From 76352587fabaf84036e448c75e358975c62b3311 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 08 2020 10:48:14 +0000
Subject: [PATCH 1/9] editSideTag API call


New API call for editing basic info on sidetags. Needs to be applied
with proper policies.

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

---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index 5932415..6b32e31 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -990,8 +990,11 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force,
         action = 'block'
     if policy:
         context.session.assertLogin()
-        policy_data = {'tag': tag_id, 'action': action, 'package': pkginfo, 'force': force}
-        assert_policy('package_list', policy_data, force=force)
+        policy_data = {'tag': tag_id, 'action': action, 'package': pkginfo,
+                       'force' : force, 'extra': tag['extra']}
+        # don't check policy for admins using force
+        if not (force and context.session.hasPerm('admin')):
+            assert_policy('package_list', policy_data)
     if not pkg:
         pkg = lookup_package(pkginfo, create=True)
     # validate arches before running callbacks
@@ -1068,7 +1071,8 @@ def _direct_pkglist_remove(taginfo, pkginfo, force=False, policy=False):
     pkg = lookup_package(pkginfo, strict=True)
     if policy:
         context.session.assertLogin()
-        policy_data = {'tag': tag['id'], 'action': 'remove', 'package': pkg['id'], 'force': force}
+        policy_data = {'tag': tag['id'], 'action': 'remove', 'package': pkg['id'],
+                       'force' : force, 'extra': tag['extra']}
         # don't check policy for admins using force
         assert_policy('package_list', policy_data, force=force)
 
@@ -1100,7 +1104,8 @@ def pkglist_unblock(taginfo, pkginfo, force=False):
     tag = get_tag(taginfo, strict=True)
     pkg = lookup_package(pkginfo, strict=True)
     context.session.assertLogin()
-    policy_data = {'tag': tag['id'], 'action': 'unblock', 'package': pkg['id'], 'force': force}
+    policy_data = {'tag': tag['id'], 'action': 'unblock', 'package': pkg['id'],
+                   'force' : force, 'extra': tag['extra']}
     # don't check policy for admins using force
     assert_policy('package_list', policy_data, force=force)
     user = get_user(context.session.user_id)
diff --git a/plugins/cli/sidetag_cli.py b/plugins/cli/sidetag_cli.py
index cc55157..6ec5a9d 100644
--- a/plugins/cli/sidetag_cli.py
+++ b/plugins/cli/sidetag_cli.py
@@ -93,3 +93,29 @@ def handle_list_sidetags(options, session, args):
 
     for tag in session.listSideTags(basetag=opts.basetag, user=user):
         print(tag["name"])
+
+
+@export_cli
+def handle_edit_sidetag(options, session, args):
+    "Edit sidetag"
+    usage = _("usage: %(prog)s edit-sidetag [options]")
+    usage += _("\n(Specify the --help global option for a list of other help options)")
+    parser = ArgumentParser(usage=usage)
+    parser.add_argument("sidetag", help="name of sidetag")
+    parser.add_argument("--debuginfo", action="store_true", default=None,
+                        help=_("Generate debuginfo repository"))
+    parser.add_argument("--no-debuginfo", action="store_false", dest="debuginfo")
+    parser.add_argument("-b", "--block", action="append", help="block package")
+    parser.add_argument("-u", "--unblock", action="append", help="unblock package")
+
+    opts = parser.parse_args(args)
+
+    activate_session(session, options)
+
+    kwargs = {
+        'block_pkgs': opts.block,
+        'unblock_pkgs': opts.unblock,
+    }
+    if opts.debuginfo is not None:
+        kwargs['debuginfo'] = opts.debuginfo
+    session.editSideTag(opts.sidetag, **kwargs)
diff --git a/plugins/hub/sidetag_hub.py b/plugins/hub/sidetag_hub.py
index 6b29e1b..5c90e48 100644
--- a/plugins/hub/sidetag_hub.py
+++ b/plugins/hub/sidetag_hub.py
@@ -18,6 +18,7 @@ from kojihub import (  # noqa: F402
     get_tag,
     get_user,
     nextval
+    _edit_tag,
 )
 
 CONFIG_FILE = "/etc/koji-hub/plugins/sidetag.conf"
@@ -170,6 +171,49 @@ def listSideTags(basetag=None, user=None, queryOpts=None):
     return query.execute()
 
 
+@export
+def editSideTag(sidetag, debuginfo=None, block_pkgs=None, unblock_pkgs=None):
+    """Restricted ability to modify sidetags, parent tag must have:
+    sidetag_debuginfo_allowed: 1
+    sidetag_package_list_allowed: 1
+    in extra, if modifying functions should work. For blocking/unblocking
+    further policy must be compatible with these operations.
+
+    :param sidetag: sidetag id or name
+    :type sidetag: int or str
+    :param debuginfo: set or disable debuginfo repo generation
+    :type debuginfo: bool
+    :param block_pkgs: package names to be blocked in sidetag
+    :type block_pkgs: list of str
+    :param unblock_pkgs: package names to be unblocked in sidetag
+    :type unblock_pkgs: list of str
+    """
+
+    context.session.assertLogin()
+    user = get_user(context.session.user_id, strict=True)
+    tag = get_tag(sidetag, strict=True)
+
+    if not sidetag["extra"].get("sidetag"):
+        raise koji.GenericError("Not a sidetag: %(name)s" % sidetag)
+    if sidetag["extra"].get("sidetag_user_id") != user["id"]:
+        if not context.session.hasPerm("admin"):
+            raise koji.ActionNotAllowed("This is not your sidetag")
+
+    parent_id = getInheritanceData(sidetag)[0]['parent_id']
+    parent = get_tag(parent_id)
+
+    if debuginfo is not None and not parent['extra'].get('sidetag_debuginfo_allowed'):
+        raise koji.GenericError("Debuginfo setting is not allowed in parent tag.")
+    if (block_pkgs or unblock_pkgs) and not parent['extra'].get('sidetag_package_list_allowed'):
+        raise koji.GenericError("Package un/blocking is not allowed in parent tag.")
+
+    if debuginfo is not None:
+        _edit_tag(sidetag, extra={'with_debuginfo': bool(debuginfo)})
+    for pkg in block_pkgs:
+        pkglist_block(sidetag, pkg)
+    for pkg in unblock_pkgs:
+        pkglist_unblock(sidetag, pkg)
+
 def handle_sidetag_untag(cbtype, *args, **kws):
     """Remove a side tag when its last build is untagged
 
diff --git a/tests/test_hub/test_pkglist.py b/tests/test_hub/test_pkglist.py
index f9f6c2a..7ab2642 100644
--- a/tests/test_hub/test_pkglist.py
+++ b/tests/test_hub/test_pkglist.py
@@ -58,7 +58,7 @@ class TestPkglistBlock(unittest.TestCase):
     @mock.patch('kojihub.lookup_package')
     def test_pkglist_unblock(self, lookup_package, get_tag, assert_policy,
             readPackageList, _pkglist_add, _pkglist_remove):
-        tag = {'id': 1, 'name': 'tag'}
+        tag = {'id': 1, 'name': 'tag', 'extra': {}}
         pkg = {'id': 2, 'name': 'package', 'owner_id': 3}
         get_tag.return_value = tag
         lookup_package.return_value = pkg
@@ -73,7 +73,7 @@ class TestPkglistBlock(unittest.TestCase):
         get_tag.assert_called_once_with('tag', strict=True)
         lookup_package.assert_called_once_with('pkg', strict=True)
         assert_policy.assert_called_once_with('package_list', {'tag': tag['id'],
-            'action': 'unblock', 'package': pkg['id'], 'force': False}, force=False)
+            'action': 'unblock', 'package': pkg['id'], 'force': False, 'extra': {}}, force=False)
         self.assertEqual(readPackageList.call_count, 2)
         readPackageList.assert_has_calls([
             mock.call(tag['id'], pkgID=pkg['id'], inherit=True),
@@ -96,7 +96,7 @@ class TestPkglistBlock(unittest.TestCase):
     def test_pkglist_unblock_inherited(self, lookup_package, get_tag, assert_policy,
             readPackageList, _pkglist_add, _pkglist_remove):
         tag_id, pkg_id, owner_id = 1, 2, 3
-        get_tag.return_value = {'id': tag_id, 'name': 'tag'}
+        get_tag.return_value = {'id': tag_id, 'name': 'tag', 'extra': {}}
         lookup_package.return_value = {'id': pkg_id, 'name': 'pkg'}
         readPackageList.return_value = {pkg_id: {
             'blocked': True,
@@ -109,7 +109,7 @@ class TestPkglistBlock(unittest.TestCase):
         get_tag.assert_called_once_with('tag', strict=True)
         lookup_package.assert_called_once_with('pkg', strict=True)
         assert_policy.assert_called_once_with('package_list', {'tag': tag_id,
-            'action': 'unblock', 'package': pkg_id, 'force': False}, force=False)
+            'action': 'unblock', 'package': pkg_id, 'force': False, 'extra': {}}, force=False)
         readPackageList.assert_called_once_with(tag_id, pkgID=pkg_id, inherit=True)
         _pkglist_add.assert_called_once_with(tag_id, pkg_id, owner_id, False, '')
         _pkglist_remove.assert_not_called()
@@ -123,7 +123,7 @@ class TestPkglistBlock(unittest.TestCase):
     def test_pkglist_unblock_not_present(self, lookup_package, get_tag, assert_policy,
             readPackageList, _pkglist_add, _pkglist_remove):
         tag_id, pkg_id = 1, 2
-        get_tag.return_value = {'id': tag_id, 'name': 'tag'}
+        get_tag.return_value = {'id': tag_id, 'name': 'tag', 'extra': {}}
         lookup_package.return_value = {'id': pkg_id, 'name': 'pkg'}
         readPackageList.return_value = {}
 
@@ -133,7 +133,7 @@ class TestPkglistBlock(unittest.TestCase):
         get_tag.assert_called_once_with('tag', strict=True)
         lookup_package.assert_called_once_with('pkg', strict=True)
         assert_policy.assert_called_once_with('package_list', {'tag': tag_id,
-            'action': 'unblock', 'package': pkg_id, 'force': False}, force=False)
+            'action': 'unblock', 'package': pkg_id, 'force': False, 'extra': {}}, force=False)
         readPackageList.assert_called_once_with(tag_id, pkgID=pkg_id, inherit=True)
         _pkglist_add.assert_not_called()
         _pkglist_remove.assert_not_called()
@@ -147,7 +147,7 @@ class TestPkglistBlock(unittest.TestCase):
     def test_pkglist_unblock_not_blocked(self, lookup_package, get_tag, assert_policy,
             readPackageList, _pkglist_add, _pkglist_remove):
         tag_id, pkg_id, owner_id = 1, 2, 3
-        get_tag.return_value = {'id': tag_id, 'name': 'tag'}
+        get_tag.return_value = {'id': tag_id, 'name': 'tag', 'extra': {}}
         lookup_package.return_value = {'id': pkg_id, 'name': 'pkg'}
         readPackageList.return_value = {pkg_id: {
             'blocked': False,
@@ -162,7 +162,7 @@ class TestPkglistBlock(unittest.TestCase):
         get_tag.assert_called_once_with('tag', strict=True)
         lookup_package.assert_called_once_with('pkg', strict=True)
         assert_policy.assert_called_once_with('package_list', {'tag': tag_id,
-            'action': 'unblock', 'package': pkg_id, 'force': False}, force=False)
+            'action': 'unblock', 'package': pkg_id, 'force': False, 'extra': {}}, force=False)
         readPackageList.assert_called_once_with(tag_id, pkgID=pkg_id, inherit=True)
         _pkglist_add.assert_not_called()
         _pkglist_remove.assert_not_called()
@@ -200,7 +200,7 @@ class TestPkglistBlock(unittest.TestCase):
         force=False
         update=False
         policy=True
-        tag = {'id': 1, 'name': 'tag'}
+        tag = {'id': 1, 'name': 'tag', 'extra': {}}
         pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3}
         users = [
             {'id': 3, 'name': 'user'},
@@ -224,7 +224,7 @@ class TestPkglistBlock(unittest.TestCase):
             mock.call(112233),
         ])
         assert_policy.assert_called_once_with('package_list', {'tag': tag['id'],
-            'action': 'add', 'package': pkg['name'], 'force': False}, force=False)
+            'action': 'add', 'package': pkg['name'], 'force': False, 'extra': {}}, force=False)
         self.assertEqual(self.run_callbacks.call_count, 2)
         self.run_callbacks.assert_has_calls([
             mock.call('prePackageListChange', action='add', tag=tag,
@@ -313,7 +313,7 @@ class TestPkglistBlock(unittest.TestCase):
         force=False
         update=False
         policy=True
-        tag = {'id': 1, 'name': 'tag'}
+        tag = {'id': 1, 'name': 'tag', 'extra': {}}
         pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3}
         users = [
             {'id': 3, 'name': 'user'},
@@ -341,7 +341,7 @@ class TestPkglistBlock(unittest.TestCase):
             mock.call(112233),
         ])
         assert_policy.assert_called_once_with('package_list', {'tag': tag['id'],
-            'action': 'add', 'package': pkg['name'], 'force': False}, force=False)
+            'action': 'add', 'package': pkg['name'], 'force': False, 'extra': {}}, force=False)
         self.assertEqual(self.run_callbacks.call_count, 2)
         self.run_callbacks.assert_has_calls([
             mock.call('prePackageListChange', action='add', tag=tag,
@@ -370,7 +370,7 @@ class TestPkglistBlock(unittest.TestCase):
         force=False
         update=False
         policy=True
-        tag = {'id': 1, 'name': 'tag'}
+        tag = {'id': 1, 'name': 'tag', 'extra': {}}
         pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3}
         users = [
             {'id': 3, 'name': 'user',},
@@ -398,7 +398,7 @@ class TestPkglistBlock(unittest.TestCase):
             mock.call(112233),
         ])
         assert_policy.assert_called_once_with('package_list', {'tag': tag['id'],
-            'action': 'add', 'package': pkg['name'], 'force': False}, force=False)
+            'action': 'add', 'package': pkg['name'], 'force': False, 'extra': {}}, force=False)
         self.run_callbacks.assert_called_once_with(
                 'prePackageListChange', action='add', tag=tag,
                 package=pkg, owner=user['id'], block=block,
@@ -420,7 +420,7 @@ class TestPkglistBlock(unittest.TestCase):
         force=True
         update=False
         policy=True
-        tag = {'id': 1, 'name': 'tag'}
+        tag = {'id': 1, 'name': 'tag', 'extra': {}}
         pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3}
         users = [
             {'id': 3, 'name': 'user',},

From a143c440ddcb2f61fb69021007fba6ab81ae7603 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 08 2020 10:48:14 +0000
Subject: [PATCH 2/9] remove un/block part


---

diff --git a/plugins/cli/sidetag_cli.py b/plugins/cli/sidetag_cli.py
index 6ec5a9d..32d4323 100644
--- a/plugins/cli/sidetag_cli.py
+++ b/plugins/cli/sidetag_cli.py
@@ -105,17 +105,12 @@ def handle_edit_sidetag(options, session, args):
     parser.add_argument("--debuginfo", action="store_true", default=None,
                         help=_("Generate debuginfo repository"))
     parser.add_argument("--no-debuginfo", action="store_false", dest="debuginfo")
-    parser.add_argument("-b", "--block", action="append", help="block package")
-    parser.add_argument("-u", "--unblock", action="append", help="unblock package")
 
     opts = parser.parse_args(args)
 
+    if opts.debuginfo is None:
+        parser.error("--debuginfo or --no-debuginfo must be specified")
+
     activate_session(session, options)
 
-    kwargs = {
-        'block_pkgs': opts.block,
-        'unblock_pkgs': opts.unblock,
-    }
-    if opts.debuginfo is not None:
-        kwargs['debuginfo'] = opts.debuginfo
-    session.editSideTag(opts.sidetag, **kwargs)
+    session.editSideTag(opts.sidetag, debuginfo=opts.debuginfo)
diff --git a/plugins/hub/sidetag_hub.py b/plugins/hub/sidetag_hub.py
index 5c90e48..5e60670 100644
--- a/plugins/hub/sidetag_hub.py
+++ b/plugins/hub/sidetag_hub.py
@@ -172,10 +172,9 @@ def listSideTags(basetag=None, user=None, queryOpts=None):
 
 
 @export
-def editSideTag(sidetag, debuginfo=None, block_pkgs=None, unblock_pkgs=None):
+def editSideTag(sidetag, debuginfo=None):
     """Restricted ability to modify sidetags, parent tag must have:
     sidetag_debuginfo_allowed: 1
-    sidetag_package_list_allowed: 1
     in extra, if modifying functions should work. For blocking/unblocking
     further policy must be compatible with these operations.
 
@@ -183,10 +182,6 @@ def editSideTag(sidetag, debuginfo=None, block_pkgs=None, unblock_pkgs=None):
     :type sidetag: int or str
     :param debuginfo: set or disable debuginfo repo generation
     :type debuginfo: bool
-    :param block_pkgs: package names to be blocked in sidetag
-    :type block_pkgs: list of str
-    :param unblock_pkgs: package names to be unblocked in sidetag
-    :type unblock_pkgs: list of str
     """
 
     context.session.assertLogin()
@@ -204,15 +199,10 @@ def editSideTag(sidetag, debuginfo=None, block_pkgs=None, unblock_pkgs=None):
 
     if debuginfo is not None and not parent['extra'].get('sidetag_debuginfo_allowed'):
         raise koji.GenericError("Debuginfo setting is not allowed in parent tag.")
-    if (block_pkgs or unblock_pkgs) and not parent['extra'].get('sidetag_package_list_allowed'):
-        raise koji.GenericError("Package un/blocking is not allowed in parent tag.")
 
     if debuginfo is not None:
         _edit_tag(sidetag, extra={'with_debuginfo': bool(debuginfo)})
-    for pkg in block_pkgs:
-        pkglist_block(sidetag, pkg)
-    for pkg in unblock_pkgs:
-        pkglist_unblock(sidetag, pkg)
+
 
 def handle_sidetag_untag(cbtype, *args, **kws):
     """Remove a side tag when its last build is untagged

From 5668436073eee5dfea9f1f2119553ac7f1318f9b Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 08 2020 10:48:14 +0000
Subject: [PATCH 3/9] introduce is_sidetag_owner policy


---

diff --git a/plugins/hub/sidetag_hub.py b/plugins/hub/sidetag_hub.py
index 5e60670..8bdad2c 100644
--- a/plugins/hub/sidetag_hub.py
+++ b/plugins/hub/sidetag_hub.py
@@ -4,6 +4,7 @@
 import sys
 
 import koji
+import koji.policy
 from koji.context import context
 from koji.plugin import callback, export
 sys.path.insert(0, "/usr/share/koji-hub/")
@@ -13,18 +14,31 @@ from kojihub import (  # noqa: F402
     _create_tag,
     _delete_build_target,
     _delete_tag,
+    _edit_tag,
     assert_policy,
     get_build_target,
+    getInheritanceData,
     get_tag,
     get_user,
-    nextval
-    _edit_tag,
+    nextval,
+    policy_get_user
 )
 
 CONFIG_FILE = "/etc/koji-hub/plugins/sidetag.conf"
 CONFIG = None
 
 
+class SidetagOwner(koji.policy.MatchTest):
+    """Checks, if user is a real owner of sidetag"""
+    name = 'is_sidetag_owner'
+
+    def run(self, data):
+        user = policy_get_user(data)
+        tag = get_tag(data['tag'])
+        return (tag['extra'].get('sidetag') and
+                tag['extra'].get('sidetag_user_id') == user['id'])
+
+
 @export
 def createSideTag(basetag, debuginfo=False):
     """Create a side tag.
@@ -186,7 +200,7 @@ def editSideTag(sidetag, debuginfo=None):
 
     context.session.assertLogin()
     user = get_user(context.session.user_id, strict=True)
-    tag = get_tag(sidetag, strict=True)
+    sidetag = get_tag(sidetag, strict=True)
 
     if not sidetag["extra"].get("sidetag"):
         raise koji.GenericError("Not a sidetag: %(name)s" % sidetag)

From 4b5a16008d37293b9fef48f33b04334e324361c0 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 08 2020 10:48:51 +0000
Subject: [PATCH 4/9] revert main hub changes


---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index 6b32e31..bdc435d 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -990,8 +990,7 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force,
         action = 'block'
     if policy:
         context.session.assertLogin()
-        policy_data = {'tag': tag_id, 'action': action, 'package': pkginfo,
-                       'force' : force, 'extra': tag['extra']}
+        policy_data = {'tag': tag_id, 'action': action, 'package': pkginfo, 'force': force}
         # don't check policy for admins using force
         if not (force and context.session.hasPerm('admin')):
             assert_policy('package_list', policy_data)
@@ -1071,8 +1070,7 @@ def _direct_pkglist_remove(taginfo, pkginfo, force=False, policy=False):
     pkg = lookup_package(pkginfo, strict=True)
     if policy:
         context.session.assertLogin()
-        policy_data = {'tag': tag['id'], 'action': 'remove', 'package': pkg['id'],
-                       'force' : force, 'extra': tag['extra']}
+        policy_data = {'tag': tag['id'], 'action': 'remove', 'package': pkg['id'], 'force': force}
         # don't check policy for admins using force
         assert_policy('package_list', policy_data, force=force)
 
@@ -1104,8 +1102,7 @@ def pkglist_unblock(taginfo, pkginfo, force=False):
     tag = get_tag(taginfo, strict=True)
     pkg = lookup_package(pkginfo, strict=True)
     context.session.assertLogin()
-    policy_data = {'tag': tag['id'], 'action': 'unblock', 'package': pkg['id'],
-                   'force' : force, 'extra': tag['extra']}
+    policy_data = {'tag': tag['id'], 'action': 'unblock', 'package': pkg['id'], 'force': force}
     # don't check policy for admins using force
     assert_policy('package_list', policy_data, force=force)
     user = get_user(context.session.user_id)
diff --git a/tests/test_hub/test_pkglist.py b/tests/test_hub/test_pkglist.py
index 7ab2642..8c8b18e 100644
--- a/tests/test_hub/test_pkglist.py
+++ b/tests/test_hub/test_pkglist.py
@@ -58,7 +58,7 @@ class TestPkglistBlock(unittest.TestCase):
     @mock.patch('kojihub.lookup_package')
     def test_pkglist_unblock(self, lookup_package, get_tag, assert_policy,
             readPackageList, _pkglist_add, _pkglist_remove):
-        tag = {'id': 1, 'name': 'tag', 'extra': {}}
+        tag = {'id': 1, 'name': 'tag'}
         pkg = {'id': 2, 'name': 'package', 'owner_id': 3}
         get_tag.return_value = tag
         lookup_package.return_value = pkg
@@ -96,7 +96,7 @@ class TestPkglistBlock(unittest.TestCase):
     def test_pkglist_unblock_inherited(self, lookup_package, get_tag, assert_policy,
             readPackageList, _pkglist_add, _pkglist_remove):
         tag_id, pkg_id, owner_id = 1, 2, 3
-        get_tag.return_value = {'id': tag_id, 'name': 'tag', 'extra': {}}
+        get_tag.return_value = {'id': tag_id, 'name': 'tag'}
         lookup_package.return_value = {'id': pkg_id, 'name': 'pkg'}
         readPackageList.return_value = {pkg_id: {
             'blocked': True,
@@ -123,7 +123,7 @@ class TestPkglistBlock(unittest.TestCase):
     def test_pkglist_unblock_not_present(self, lookup_package, get_tag, assert_policy,
             readPackageList, _pkglist_add, _pkglist_remove):
         tag_id, pkg_id = 1, 2
-        get_tag.return_value = {'id': tag_id, 'name': 'tag', 'extra': {}}
+        get_tag.return_value = {'id': tag_id, 'name': 'tag'}
         lookup_package.return_value = {'id': pkg_id, 'name': 'pkg'}
         readPackageList.return_value = {}
 
@@ -147,7 +147,7 @@ class TestPkglistBlock(unittest.TestCase):
     def test_pkglist_unblock_not_blocked(self, lookup_package, get_tag, assert_policy,
             readPackageList, _pkglist_add, _pkglist_remove):
         tag_id, pkg_id, owner_id = 1, 2, 3
-        get_tag.return_value = {'id': tag_id, 'name': 'tag', 'extra': {}}
+        get_tag.return_value = {'id': tag_id, 'name': 'tag'}
         lookup_package.return_value = {'id': pkg_id, 'name': 'pkg'}
         readPackageList.return_value = {pkg_id: {
             'blocked': False,
@@ -200,7 +200,7 @@ class TestPkglistBlock(unittest.TestCase):
         force=False
         update=False
         policy=True
-        tag = {'id': 1, 'name': 'tag', 'extra': {}}
+        tag = {'id': 1, 'name': 'tag'}
         pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3}
         users = [
             {'id': 3, 'name': 'user'},
@@ -313,7 +313,7 @@ class TestPkglistBlock(unittest.TestCase):
         force=False
         update=False
         policy=True
-        tag = {'id': 1, 'name': 'tag', 'extra': {}}
+        tag = {'id': 1, 'name': 'tag'}
         pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3}
         users = [
             {'id': 3, 'name': 'user'},
@@ -370,7 +370,7 @@ class TestPkglistBlock(unittest.TestCase):
         force=False
         update=False
         policy=True
-        tag = {'id': 1, 'name': 'tag', 'extra': {}}
+        tag = {'id': 1, 'name': 'tag'}
         pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3}
         users = [
             {'id': 3, 'name': 'user',},
@@ -420,7 +420,7 @@ class TestPkglistBlock(unittest.TestCase):
         force=True
         update=False
         policy=True
-        tag = {'id': 1, 'name': 'tag', 'extra': {}}
+        tag = {'id': 1, 'name': 'tag'}
         pkg = {'id': 2, 'name': 'pkg', 'owner_id': 3}
         users = [
             {'id': 3, 'name': 'user',},

From 39d45e550fd84265f6386c8c182e176ccb85e138 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 08 2020 10:48:51 +0000
Subject: [PATCH 5/9] add is_sidetag policy test


---

diff --git a/plugins/hub/sidetag_hub.py b/plugins/hub/sidetag_hub.py
index 8bdad2c..add40e7 100644
--- a/plugins/hub/sidetag_hub.py
+++ b/plugins/hub/sidetag_hub.py
@@ -28,7 +28,16 @@ CONFIG_FILE = "/etc/koji-hub/plugins/sidetag.conf"
 CONFIG = None
 
 
-class SidetagOwner(koji.policy.MatchTest):
+class SidetagTest(koji.policy.MatchTest):
+    """Checks, if tag is a sidetag"""
+    name = 'is_sidetag'
+
+    def run(self, data):
+        tag = get_tag(data['tag'])
+        return bool(tag['extra'].get('sidetag'))
+
+
+class SidetagOwnerTest(koji.policy.MatchTest):
     """Checks, if user is a real owner of sidetag"""
     name = 'is_sidetag_owner'
 

From f603231450c38ff7522e8b56c5971edc39e67a9c Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 08 2020 10:48:51 +0000
Subject: [PATCH 6/9] simplify checks


---

diff --git a/docs/source/plugins.rst b/docs/source/plugins.rst
index 28bbf7c..c1e9d09 100644
--- a/docs/source/plugins.rst
+++ b/docs/source/plugins.rst
@@ -93,6 +93,14 @@ Example for `/etc/koji-hub/hub.conf`:
         # forbid everything else
         all :: deny
 
+    package_list =
+        # allow blocking for owners in their sidetags
+        match action block && is_sidetag_owner :: allow
+        all :: deny
+
+There are two special policy tests `is_sidetag` and `is_sidetag_owner` with
+expectable behaviour.
+
 Now Sidetag Koji plugin should be installed.  To verify that, run
 `koji list-api` command -- it should now display `createSideTag`
 as one of available API calls.
diff --git a/plugins/hub/sidetag_hub.py b/plugins/hub/sidetag_hub.py
index add40e7..a887dbb 100644
--- a/plugins/hub/sidetag_hub.py
+++ b/plugins/hub/sidetag_hub.py
@@ -28,13 +28,29 @@ CONFIG_FILE = "/etc/koji-hub/plugins/sidetag.conf"
 CONFIG = None
 
 
+def is_sidetag(taginfo, raise_error=False):
+    """Check, that given tag is sidetag"""
+    result = bool(taginfo['extra'].get('sidetag'))
+    if not result and raise_error:
+        raise koji.GenericError("Not a sidetag: %(name)s" % taginfo)
+
+
+def is_sidetag_owner(taginfo, user, raise_error=False):
+    """Check, that given user is owner of the sidetag"""
+    result = (taginfo['extra'].get('sidetag') and
+              taginfo['extra'].get('sidetag_user_id') == user['id'])
+    if not result and raise_error:
+        raise koji.ActionNotAllowed("This is not your sidetag")
+
+
+# Policy tests
 class SidetagTest(koji.policy.MatchTest):
     """Checks, if tag is a sidetag"""
     name = 'is_sidetag'
 
     def run(self, data):
         tag = get_tag(data['tag'])
-        return bool(tag['extra'].get('sidetag'))
+        return is_sidetag(tag)
 
 
 class SidetagOwnerTest(koji.policy.MatchTest):
@@ -44,10 +60,10 @@ class SidetagOwnerTest(koji.policy.MatchTest):
     def run(self, data):
         user = policy_get_user(data)
         tag = get_tag(data['tag'])
-        return (tag['extra'].get('sidetag') and
-                tag['extra'].get('sidetag_user_id') == user['id'])
+        return is_sidetag_owner(tag, user)
 
 
+# API calls
 @export
 def createSideTag(basetag, debuginfo=False):
     """Create a side tag.
@@ -118,11 +134,9 @@ def removeSideTag(sidetag):
     sidetag = get_tag(sidetag, strict=True)
 
     # sanity/access
-    if not sidetag["extra"].get("sidetag"):
-        raise koji.GenericError("Not a sidetag: %(name)s" % sidetag)
-    if sidetag["extra"].get("sidetag_user_id") != user["id"]:
-        if not context.session.hasPerm("admin"):
-            raise koji.ActionNotAllowed("This is not your sidetag")
+    is_sidetag(sidetag, raise_error=True)
+    is_sidetag_owner(sidetag, user, raise_error=True)
+
     _remove_sidetag(sidetag)
 
 
@@ -211,11 +225,9 @@ def editSideTag(sidetag, debuginfo=None):
     user = get_user(context.session.user_id, strict=True)
     sidetag = get_tag(sidetag, strict=True)
 
-    if not sidetag["extra"].get("sidetag"):
-        raise koji.GenericError("Not a sidetag: %(name)s" % sidetag)
-    if sidetag["extra"].get("sidetag_user_id") != user["id"]:
-        if not context.session.hasPerm("admin"):
-            raise koji.ActionNotAllowed("This is not your sidetag")
+    # sanity/access
+    is_sidetag(sidetag, raise_error=True)
+    is_sidetag_owner(sidetag, user, raise_error=True)
 
     parent_id = getInheritanceData(sidetag)[0]['parent_id']
     parent = get_tag(parent_id)
@@ -241,8 +253,7 @@ def handle_sidetag_untag(cbtype, *args, **kws):
     if not tag:
         # also shouldn't happen, but just in case
         return
-    if not tag["extra"].get("sidetag"):
-        # not a side tag
+    if not is_sidetag(tag):
         return
     # is the tag now empty?
     query = QueryProcessor(

From fa4a4a6339bd40dfb3d62c2f8dbaab1d72140645 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 08 2020 10:48:51 +0000
Subject: [PATCH 7/9] edit rpm macros


---

diff --git a/plugins/cli/sidetag_cli.py b/plugins/cli/sidetag_cli.py
index 32d4323..0443c76 100644
--- a/plugins/cli/sidetag_cli.py
+++ b/plugins/cli/sidetag_cli.py
@@ -105,12 +105,31 @@ def handle_edit_sidetag(options, session, args):
     parser.add_argument("--debuginfo", action="store_true", default=None,
                         help=_("Generate debuginfo repository"))
     parser.add_argument("--no-debuginfo", action="store_false", dest="debuginfo")
+    parser.add_argument("--rpm-macro", action="append", default=[], metavar="key=value",
+                        help=_("Set tag-specific rpm macros"))
+    parser.add_argument("--remove-rpm-macro", action="append", default=[], metavar="key",
+                        help=_("Remove rpm macros"))
 
     opts = parser.parse_args(args)
 
-    if opts.debuginfo is None:
-        parser.error("--debuginfo or --no-debuginfo must be specified")
+    if opts.debuginfo is None and not opts.add_rpm_macro and not opts.remove_rpm_macros:
+        parser.error("At least one option needs to be specified")
 
     activate_session(session, options)
 
-    session.editSideTag(opts.sidetag, debuginfo=opts.debuginfo)
+    kwargs = {}
+    if opts.debuginfo is not None:
+        kwargs['debuginfo'] = opts.debuginfo
+
+    if options.add_rpm_macro:
+        rpm_macros = {]
+        for xopt in opts.add_rpm_macro:
+            key, value = xopt.split('=', 1)
+            value = arg_filter(value)
+            rpm_macros[key] = value
+        kwargs['rpm_macros'] = rpm_macros
+
+    if opts.remove_rpm_macros:
+        kwargs['remove_rpm_macros'] = opts.remove_rpm_macros
+
+    session.editSideTag(opts.sidetag, **kwargs)
diff --git a/plugins/hub/sidetag_hub.py b/plugins/hub/sidetag_hub.py
index a887dbb..dc3ee4f 100644
--- a/plugins/hub/sidetag_hub.py
+++ b/plugins/hub/sidetag_hub.py
@@ -209,9 +209,10 @@ def listSideTags(basetag=None, user=None, queryOpts=None):
 
 
 @export
-def editSideTag(sidetag, debuginfo=None):
+def editSideTag(sidetag, debuginfo=None, rpm_macros=None, remove_rpm_macros=None):
     """Restricted ability to modify sidetags, parent tag must have:
     sidetag_debuginfo_allowed: 1
+    sidetag_rpm_macros_allowed: 1
     in extra, if modifying functions should work. For blocking/unblocking
     further policy must be compatible with these operations.
 
@@ -219,6 +220,10 @@ def editSideTag(sidetag, debuginfo=None):
     :type sidetag: int or str
     :param debuginfo: set or disable debuginfo repo generation
     :type debuginfo: bool
+    :param rpm_macros: add/update rpms macros in extra
+    :type rpm_macros: dict
+    :param remove_rpm_macros: remove rpm macros from extra
+    :type remove_rpm_macros: list of str
     """
 
     context.session.assertLogin()
@@ -235,8 +240,19 @@ def editSideTag(sidetag, debuginfo=None):
     if debuginfo is not None and not parent['extra'].get('sidetag_debuginfo_allowed'):
         raise koji.GenericError("Debuginfo setting is not allowed in parent tag.")
 
+    if ((rpm_macros is not None or remove_rpm_macros is not None)
+        and not parent['extra'].get('sidetag_rpm_macros_allowed')):
+        raise koji.GenericError("RPM macros change is not allowed in parent tag.")
+
+    kwargs = {'extra': {}}
     if debuginfo is not None:
-        _edit_tag(sidetag, extra={'with_debuginfo': bool(debuginfo)})
+        kwargs['extra']['with_debuginfo'] = bool(debuginfo)
+    for macro, value in rpm_macros.items():
+        kwargs['extra']['rpm.macro.%s' % macro] = value
+    if remove_rpm_macros:
+        kwargs['remove_extra'] = ['rpm.macro.%s' % m for m in remove_rpm_macros]
+
+    _edit_tag(sidetag, **kwargs)
 
 
 def handle_sidetag_untag(cbtype, *args, **kws):

From 40c02699a69b3e6ece55320d86f8504d32f47195 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 08 2020 10:48:51 +0000
Subject: [PATCH 8/9] fixes


---

diff --git a/plugins/cli/sidetag_cli.py b/plugins/cli/sidetag_cli.py
index 0443c76..f44ac40 100644
--- a/plugins/cli/sidetag_cli.py
+++ b/plugins/cli/sidetag_cli.py
@@ -10,7 +10,7 @@ from argparse import ArgumentParser
 import koji
 from koji.plugin import export_cli
 from koji_cli.commands import anon_handle_wait_repo
-from koji_cli.lib import _, activate_session
+from koji_cli.lib import _, activate_session, arg_filter
 
 
 @export_cli
@@ -106,13 +106,13 @@ def handle_edit_sidetag(options, session, args):
                         help=_("Generate debuginfo repository"))
     parser.add_argument("--no-debuginfo", action="store_false", dest="debuginfo")
     parser.add_argument("--rpm-macro", action="append", default=[], metavar="key=value",
-                        help=_("Set tag-specific rpm macros"))
+                        dest="rpm_macros", help=_("Set tag-specific rpm macros"))
     parser.add_argument("--remove-rpm-macro", action="append", default=[], metavar="key",
-                        help=_("Remove rpm macros"))
+                        dest="remove_rpm_macros", help=_("Remove rpm macros"))
 
     opts = parser.parse_args(args)
 
-    if opts.debuginfo is None and not opts.add_rpm_macro and not opts.remove_rpm_macros:
+    if opts.debuginfo is None and not opts.rpm_macros and not opts.remove_rpm_macros:
         parser.error("At least one option needs to be specified")
 
     activate_session(session, options)
@@ -121,9 +121,9 @@ def handle_edit_sidetag(options, session, args):
     if opts.debuginfo is not None:
         kwargs['debuginfo'] = opts.debuginfo
 
-    if options.add_rpm_macro:
-        rpm_macros = {]
-        for xopt in opts.add_rpm_macro:
+    if opts.rpm_macros:
+        rpm_macros = {}
+        for xopt in opts.rpm_macros:
             key, value = xopt.split('=', 1)
             value = arg_filter(value)
             rpm_macros[key] = value
diff --git a/plugins/hub/sidetag_hub.py b/plugins/hub/sidetag_hub.py
index dc3ee4f..13cf878 100644
--- a/plugins/hub/sidetag_hub.py
+++ b/plugins/hub/sidetag_hub.py
@@ -17,7 +17,7 @@ from kojihub import (  # noqa: F402
     _edit_tag,
     assert_policy,
     get_build_target,
-    getInheritanceData,
+    readInheritanceData,
     get_tag,
     get_user,
     nextval,
@@ -234,25 +234,26 @@ def editSideTag(sidetag, debuginfo=None, rpm_macros=None, remove_rpm_macros=None
     is_sidetag(sidetag, raise_error=True)
     is_sidetag_owner(sidetag, user, raise_error=True)
 
-    parent_id = getInheritanceData(sidetag)[0]['parent_id']
+    parent_id = readInheritanceData(sidetag['id'])[0]['parent_id']
     parent = get_tag(parent_id)
 
     if debuginfo is not None and not parent['extra'].get('sidetag_debuginfo_allowed'):
         raise koji.GenericError("Debuginfo setting is not allowed in parent tag.")
 
-    if ((rpm_macros is not None or remove_rpm_macros is not None)
-        and not parent['extra'].get('sidetag_rpm_macros_allowed')):
+    if (rpm_macros is not None or remove_rpm_macros is not None) \
+            and not parent['extra'].get('sidetag_rpm_macros_allowed'):
         raise koji.GenericError("RPM macros change is not allowed in parent tag.")
 
     kwargs = {'extra': {}}
     if debuginfo is not None:
         kwargs['extra']['with_debuginfo'] = bool(debuginfo)
-    for macro, value in rpm_macros.items():
-        kwargs['extra']['rpm.macro.%s' % macro] = value
-    if remove_rpm_macros:
+    if rpm_macros is not None:
+        for macro, value in rpm_macros.items():
+            kwargs['extra']['rpm.macro.%s' % macro] = value
+    if remove_rpm_macros is not None:
         kwargs['remove_extra'] = ['rpm.macro.%s' % m for m in remove_rpm_macros]
 
-    _edit_tag(sidetag, **kwargs)
+    _edit_tag(sidetag['id'], **kwargs)
 
 
 def handle_sidetag_untag(cbtype, *args, **kws):

From b0ef16f29313dc816ad8f303db50d53403269014 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 08 2020 10:55:33 +0000
Subject: [PATCH 9/9] rebase fixes


---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index bdc435d..5932415 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -991,9 +991,7 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force,
     if policy:
         context.session.assertLogin()
         policy_data = {'tag': tag_id, 'action': action, 'package': pkginfo, 'force': force}
-        # don't check policy for admins using force
-        if not (force and context.session.hasPerm('admin')):
-            assert_policy('package_list', policy_data)
+        assert_policy('package_list', policy_data, force=force)
     if not pkg:
         pkg = lookup_package(pkginfo, create=True)
     # validate arches before running callbacks
diff --git a/tests/test_hub/test_pkglist.py b/tests/test_hub/test_pkglist.py
index 8c8b18e..f9f6c2a 100644
--- a/tests/test_hub/test_pkglist.py
+++ b/tests/test_hub/test_pkglist.py
@@ -73,7 +73,7 @@ class TestPkglistBlock(unittest.TestCase):
         get_tag.assert_called_once_with('tag', strict=True)
         lookup_package.assert_called_once_with('pkg', strict=True)
         assert_policy.assert_called_once_with('package_list', {'tag': tag['id'],
-            'action': 'unblock', 'package': pkg['id'], 'force': False, 'extra': {}}, force=False)
+            'action': 'unblock', 'package': pkg['id'], 'force': False}, force=False)
         self.assertEqual(readPackageList.call_count, 2)
         readPackageList.assert_has_calls([
             mock.call(tag['id'], pkgID=pkg['id'], inherit=True),
@@ -109,7 +109,7 @@ class TestPkglistBlock(unittest.TestCase):
         get_tag.assert_called_once_with('tag', strict=True)
         lookup_package.assert_called_once_with('pkg', strict=True)
         assert_policy.assert_called_once_with('package_list', {'tag': tag_id,
-            'action': 'unblock', 'package': pkg_id, 'force': False, 'extra': {}}, force=False)
+            'action': 'unblock', 'package': pkg_id, 'force': False}, force=False)
         readPackageList.assert_called_once_with(tag_id, pkgID=pkg_id, inherit=True)
         _pkglist_add.assert_called_once_with(tag_id, pkg_id, owner_id, False, '')
         _pkglist_remove.assert_not_called()
@@ -133,7 +133,7 @@ class TestPkglistBlock(unittest.TestCase):
         get_tag.assert_called_once_with('tag', strict=True)
         lookup_package.assert_called_once_with('pkg', strict=True)
         assert_policy.assert_called_once_with('package_list', {'tag': tag_id,
-            'action': 'unblock', 'package': pkg_id, 'force': False, 'extra': {}}, force=False)
+            'action': 'unblock', 'package': pkg_id, 'force': False}, force=False)
         readPackageList.assert_called_once_with(tag_id, pkgID=pkg_id, inherit=True)
         _pkglist_add.assert_not_called()
         _pkglist_remove.assert_not_called()
@@ -162,7 +162,7 @@ class TestPkglistBlock(unittest.TestCase):
         get_tag.assert_called_once_with('tag', strict=True)
         lookup_package.assert_called_once_with('pkg', strict=True)
         assert_policy.assert_called_once_with('package_list', {'tag': tag_id,
-            'action': 'unblock', 'package': pkg_id, 'force': False, 'extra': {}}, force=False)
+            'action': 'unblock', 'package': pkg_id, 'force': False}, force=False)
         readPackageList.assert_called_once_with(tag_id, pkgID=pkg_id, inherit=True)
         _pkglist_add.assert_not_called()
         _pkglist_remove.assert_not_called()
@@ -224,7 +224,7 @@ class TestPkglistBlock(unittest.TestCase):
             mock.call(112233),
         ])
         assert_policy.assert_called_once_with('package_list', {'tag': tag['id'],
-            'action': 'add', 'package': pkg['name'], 'force': False, 'extra': {}}, force=False)
+            'action': 'add', 'package': pkg['name'], 'force': False}, force=False)
         self.assertEqual(self.run_callbacks.call_count, 2)
         self.run_callbacks.assert_has_calls([
             mock.call('prePackageListChange', action='add', tag=tag,
@@ -341,7 +341,7 @@ class TestPkglistBlock(unittest.TestCase):
             mock.call(112233),
         ])
         assert_policy.assert_called_once_with('package_list', {'tag': tag['id'],
-            'action': 'add', 'package': pkg['name'], 'force': False, 'extra': {}}, force=False)
+            'action': 'add', 'package': pkg['name'], 'force': False}, force=False)
         self.assertEqual(self.run_callbacks.call_count, 2)
         self.run_callbacks.assert_has_calls([
             mock.call('prePackageListChange', action='add', tag=tag,
@@ -398,7 +398,7 @@ class TestPkglistBlock(unittest.TestCase):
             mock.call(112233),
         ])
         assert_policy.assert_called_once_with('package_list', {'tag': tag['id'],
-            'action': 'add', 'package': pkg['name'], 'force': False, 'extra': {}}, force=False)
+            'action': 'add', 'package': pkg['name'], 'force': False}, force=False)
         self.run_callbacks.assert_called_once_with(
                 'prePackageListChange', action='add', tag=tag,
                 package=pkg, owner=user['id'], block=block,