From 23b3b12f35997c5c8fa02838553463c6937de3c1 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 24 2020 15:08:07 +0000
Subject: [PATCH 1/2] move admin force usage to assert_policy


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

---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index e344688..dc5c2ab 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -990,13 +990,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)
-        else:
-            pkg_name = pkg and pkg['name'] or pkginfo
-            logger.info("Package list add %s/%s policy overriden by %s" % (
-                tag['name'], pkg_name, context.session.user_data['name']))
+        assert_policy('package_list', policy_data, force=force)
     if not pkg:
         pkg = lookup_package(pkginfo, create=True)
     # validate arches before running callbacks
@@ -1075,11 +1069,7 @@ def _direct_pkglist_remove(taginfo, pkginfo, force=False, policy=False):
         context.session.assertLogin()
         policy_data = {'tag': tag['id'], 'action': 'remove', 'package': pkg['id'], 'force': force}
         # don't check policy for admins using force
-        if not (force and context.session.hasPerm('admin')):
-            assert_policy('package_list', policy_data)
-        else:
-            logger.info("Package list %s/%s remove policy overriden by %s" % (
-                tag['name'], pkg['name'], context.session.user_data['name']))
+        assert_policy('package_list', policy_data, force=force)
 
     user = get_user(context.session.user_id)
     koji.plugin.run_callbacks(
@@ -1111,11 +1101,7 @@ def pkglist_unblock(taginfo, pkginfo, force=False):
     context.session.assertLogin()
     policy_data = {'tag': tag['id'], 'action': 'unblock', 'package': pkg['id'], 'force': force}
     # don't check policy for admins using force
-    if not (force and context.session.hasPerm('admin')):
-        assert_policy('package_list', policy_data)
-    else:
-        logger.info("Package list %s/%s unblock policy overriden by %s" % (
-            tag['name'], pkg['name'], context.session.user_data['name']))
+    assert_policy('package_list', policy_data, force=force)
     user = get_user(context.session.user_id)
     koji.plugin.run_callbacks(
         'prePackageListChange', action='unblock', tag=tag, package=pkg, user=user)
@@ -9670,7 +9656,7 @@ class PolicyTest(koji.policy.BaseSimpleTest):
             return result.lower() in ('yes', 'true', 'allow')
 
 
-def check_policy(name, data, default='deny', strict=False):
+def check_policy(name, data, default='deny', strict=False, force=False):
     """Check data against the named policy
 
     This assumes the policy actions consist of:
@@ -9681,6 +9667,7 @@ def check_policy(name, data, default='deny', strict=False):
         access: True if the policy result is allow, false otherwise
         reason: reason for the access
     If strict is True, will raise ActionNotAllowed if the action is not 'allow'
+    If force is True, policy will pass, but action will be logged
     """
     ruleset = context.policy.get(name)
     if not ruleset:
@@ -9711,6 +9698,9 @@ def check_policy(name, data, default='deny', strict=False):
     if result != 'deny':
         reason = 'error in policy'
         logger.error("Invalid action in policy %s, rule: %s", name, lastrule)
+    if force and context.session.assertPerm('admin'):
+        logger.info("Policy %s overriden by force: %s" % (name, context.session.user_data["name"]))
+        return True, "overriden by force"
     if not strict:
         return False, reason
     err_str = "policy violation (%s)" % name
@@ -9721,7 +9711,7 @@ def check_policy(name, data, default='deny', strict=False):
     raise koji.ActionNotAllowed(err_str)
 
 
-def assert_policy(name, data, default='deny'):
+def assert_policy(name, data, default='deny', force=False):
     """Enforce the named policy
 
     This assumes the policy actions consist of:
@@ -9729,7 +9719,7 @@ def assert_policy(name, data, default='deny'):
         deny
     Raises ActionNotAllowed if policy result is not allow
     """
-    check_policy(name, data, default=default, strict=True)
+    check_policy(name, data, default=default, strict=True, force=force)
 
 
 def rpmdiff(basepath, rpmlist, hashes):
@@ -10571,14 +10561,11 @@ class RootExports(object):
         Retagging is not allowed unless force is true. (retagging changes the order
         of entries will affect which build is the latest)
         """
-        if force:
-            context.session.assertPerm('admin')
-        else:
-            context.session.assertPerm('tag')
-            tag_id = get_tag(tag, strict=True)['id']
-            build_id = get_build(build, strict=True)['id']
-            policy_data = {'tag': tag_id, 'build': build_id, 'fromtag': None, 'operation': 'tag'}
-            assert_policy('tag', policy_data)
+        context.session.assertPerm('tag')
+        tag_id = get_tag(tag, strict=True)['id']
+        build_id = get_build(build, strict=True)['id']
+        policy_data = {'tag': tag_id, 'build': build_id, 'fromtag': None, 'operation': 'tag'}
+        assert_policy('tag', policy_data, force=force)
         _tag_build(tag, build, force=force)
         if notify:
             tag_notification(True, tag, None, build, context.session.user_id)
@@ -10641,12 +10628,8 @@ class RootExports(object):
         else:
             policy_data['operation'] = 'move'
         # don't check policy for admins using force
-        if not (force and context.session.hasPerm('admin')):
-            assert_policy('tag', policy_data)
-            # XXX - we're running this check twice, here and in host.tagBuild (called by the task)
-        else:
-            logger.info("Tag policy %s/%s overriden by %s" % (
-                tag['name'], build['nvr'], context.session.user_data['name']))
+        # XXX - we're running this check twice, here and in host.tagBuild (called by the task)
+        assert_policy('tag', policy_data, force=force)
         # spawn the tagging task
         return make_task('tagBuild', [tag_id, build_id, force, fromtag_id], priority=10)
 
@@ -10664,11 +10647,7 @@ class RootExports(object):
         policy_data['operation'] = 'untag'
         try:
             # don't check policy for admins using force
-            if not (force and context.session.hasPerm('admin')):
-                assert_policy('tag', policy_data)
-            else:
-                logger.info("Untag policy %s/%s overriden by %s" % (
-                    tag, build, context.session.user_data['name']))
+            assert_policy('tag', policy_data, force=force)
             _untag_build(tag, build, strict=strict, force=force)
             tag_notification(True, None, tag, build, user_id)
         except Exception:
@@ -10683,14 +10662,11 @@ class RootExports(object):
 
         Unlike tagBuild, this does not create a task
         No return value"""
-        if force:
-            context.session.assertPerm('admin')
-        else:
-            context.session.assertPerm('tag')
-            tag_id = get_tag(tag, strict=True)['id']
-            build_id = get_build(build, strict=True)['id']
-            policy_data = {'tag': None, 'build': build_id, 'fromtag': tag_id, 'operation': 'untag'}
-            assert_policy('tag', policy_data)
+        context.session.assertPerm('tag')
+        tag_id = get_tag(tag, strict=True)['id']
+        build_id = get_build(build, strict=True)['id']
+        policy_data = {'tag': None, 'build': build_id, 'fromtag': tag_id, 'operation': 'untag'}
+        assert_policy('tag', policy_data, force=force)
         _untag_build(tag, build, strict=strict, force=force)
         if notify:
             tag_notification(True, None, tag, build, context.session.user_id)
@@ -10740,15 +10716,11 @@ class RootExports(object):
         # policy check
         policy_data = {'tag': tag2, 'fromtag': tag1, 'operation': 'move'}
         # don't check policy for admins using force
-        if not (force and context.session.hasPerm('admin')):
-            for build in build_list:
-                policy_data['build'] = build['id']
-                assert_policy('tag', policy_data)
-                # XXX - we're running this check twice, here and in host.tagBuild (called by the
-                # task)
-        else:
-            logger.info("Tag move policy %s/%s overriden by %s" % (
-                tag2, package, context.session.user_data['name']))
+        for build in build_list:
+            policy_data['build'] = build['id']
+            assert_policy('tag', policy_data)
+            # XXX - we're running this check twice, here and in host.tagBuild (called by the
+            # task)
 
         wait_on = []
         tasklist = []
@@ -13924,12 +13896,7 @@ class HostExports(object):
         else:
             policy_data['operation'] = 'move'
         # don't check policy for admins using force
-        perms = koji.auth.get_user_perms(user_id)
-        if not force or 'admin' not in perms:
-            assert_policy('tag', policy_data)
-        if force and 'admin' in perms:
-            logger.info("Tag build %s/%s policy overriden by %s" % (
-                tag, build['nvr'], context.session.user_data['name']))
+        assert_policy('tag', policy_data, force=force)
         # package list check
         pkgs = readPackageList(tagID=tag_id, pkgID=pkg_id, inherit=True)
         pkg_error = None
@@ -13940,7 +13907,7 @@ class HostExports(object):
         if pkg_error:
             if force and context.session.hasPerm('admin'):
                 pkglist_add(tag_id, pkg_id, force=True, block=False)
-                logger.info("Package added %s/%s by %s" % (
+                logger.info("Package added %s/%s by %s by force" % (
                     tag, build['nvr'], context.session.user_data['name']))
             else:
                 raise koji.TagError(pkg_error)
diff --git a/tests/test_hub/test_pkglist.py b/tests/test_hub/test_pkglist.py
index 54a21d7..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})
+            '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})
+            '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})
+            '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})
+            '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})
+            '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})
+            '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})
+            '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,
@@ -447,7 +447,8 @@ class TestPkglistBlock(unittest.TestCase):
             mock.call(112233),
         ])
         # force + admin
-        assert_policy.assert_not_called()
+        assert_policy.assert_called_once_with('package_list',
+            {'tag': 1, 'action': 'add', 'package': 'pkg', 'force': True}, force=True)
 
         self.assertEqual(self.run_callbacks.call_count, 2)
         self.run_callbacks.assert_has_calls([

From 92c8d0407ffcb4bda354f9f2857079c7a456529c Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 01 2020 10:55:00 +0000
Subject: [PATCH 2/2] add refusal reason to log


---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index dc5c2ab..d27859a 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -9699,7 +9699,10 @@ def check_policy(name, data, default='deny', strict=False, force=False):
         reason = 'error in policy'
         logger.error("Invalid action in policy %s, rule: %s", name, lastrule)
     if force and context.session.assertPerm('admin'):
-        logger.info("Policy %s overriden by force: %s" % (name, context.session.user_data["name"]))
+        msg = "Policy %s overriden by force: %s" % (name, context.session.user_data["name"])
+        if reason:
+            msg += ": %s" % reason
+        logger.info(msg)
         return True, "overriden by force"
     if not strict:
         return False, reason

