From a0112b858c1022314e2c7f67f1cfd25b9b837f71 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Mar 22 2022 14:52:39 +0000 Subject: comments for grant/revoke permissions Related: https://pagure.io/koji/issue/3295 --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index da673aa..12ef06d 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -2445,6 +2445,8 @@ def handle_grant_permission(goptions, session, args): help="Create this permission if the permission does not exist") parser.add_option("--description", help="Add description about new permission") + parser.add_option("--comment", + help="Additional comment which will be seen in the history.") (options, args) = parser.parse_args(args) if len(args) < 2: parser.error("Please specify a permission and at least one user") @@ -2462,6 +2464,8 @@ def handle_grant_permission(goptions, session, args): kwargs['create'] = True if options.description: kwargs['description'] = options.description + if options.comment: + kwargs['label'] = options.comment if options.description and not options.new: parser.error("Option new must be specified with option description.") for user in users: @@ -2472,6 +2476,8 @@ def handle_revoke_permission(goptions, session, args): "[admin] Revoke a permission from a user" usage = "usage: %prog revoke-permission [ ...]" parser = OptionParser(usage=get_usage_str(usage)) + parser.add_option("--comment", + help="Additional comment which will be seen in the history.") (options, args) = parser.parse_args(args) if len(args) < 2: parser.error("Please specify a permission and at least one user") @@ -2484,8 +2490,11 @@ def handle_revoke_permission(goptions, session, args): if user is None: parser.error("No such user: %s" % n) users.append(user) + kwargs = {} + if options.comment: + kwargs['label'] = options.comment for user in users: - session.revokePermission(user['name'], perm) + session.revokePermission(user['name'], perm, **kwargs) def handle_edit_permission(goptions, session, args): @@ -4670,6 +4679,8 @@ def _print_histline(entry, **kwargs): parts.insert(1, "(eid %i)" % event_id) if who: parts.append(who % x) + if x['create_event_label']: + parts.append("(%s)" % x['create_event_label']) if create and x['active']: parts.append("[still active]") print(' '.join(parts)) diff --git a/hub/kojihub.py b/hub/kojihub.py index f487443..d45ceac 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -7992,12 +7992,16 @@ def query_history(tables=None, **kwargs): "LEFT OUTER JOIN events AS ev2 ON ev2.id = revoke_event", "users AS creator ON creator.id = creator_id", "LEFT OUTER JOIN users AS revoker ON revoker.id = revoker_id", + "LEFT OUTER JOIN event_labels AS ev1_labels ON ev1_labels.event_id = create_event", + "LEFT OUTER JOIN event_labels AS ev2_labels ON ev2_labels.event_id = revoke_event", ] common_joined_fields = { 'creator.name': 'creator_name', 'revoker.name': 'revoker_name', 'EXTRACT(EPOCH FROM ev1.time) AS create_ts': 'create_ts', 'EXTRACT(EPOCH FROM ev2.time) AS revoke_ts': 'revoke_ts', + 'ev1_labels.label': 'create_event_label', + 'ev2_labels.label': 'revoke_event_label', } table_fields = { 'user_perms': ['user_id', 'perm_id'], @@ -9007,7 +9011,7 @@ def assert_cg(cg, user=None): raise koji.AuthError("Content generator access required (%s)" % cg['name']) -def get_event(): +def get_event(label=None): """Get an event id for this transaction We cache the result in context, so subsequent calls in the same transaction will @@ -9020,6 +9024,11 @@ def get_event(): return context.event_id event_id = _singleValue("SELECT get_event()") context.event_id = event_id + if label: + # there could be more than one label per event + insert = InsertProcessor('event_labels') + insert.set(event_id=event_id, label=label) + insert.execute() return event_id @@ -9195,9 +9204,15 @@ class InsertProcessor(object): """Set rawdata via keyword args""" self.rawdata.update(kwargs) - def make_create(self, event_id=None, user_id=None): + def make_create(self, event_id=None, user_id=None, label=None): if event_id is None: - event_id = get_event() + event_id = get_event(label) + elif label: + update = UpdateProcessor('event_labels', + clauses=['id=%(event_id)'], + values={'event_id': event_id}) + update.set(label=label) + update.execute() if user_id is None: context.session.assertLogin() user_id = context.session.user_id @@ -9284,10 +9299,16 @@ class UpdateProcessor(object): """Set rawdata via keyword args""" self.rawdata.update(kwargs) - def make_revoke(self, event_id=None, user_id=None): + def make_revoke(self, event_id=None, user_id=None, label=None): """Add standard revoke options to the update""" if event_id is None: - event_id = get_event() + event_id = get_event(label=label) + elif label: + update = UpdateProcessor('event_labels', + clauses=['id=%(event_id)'], + values={'event_id': event_id}) + update.set(label=label) + update.execute() if user_id is None: context.session.assertLogin() user_id = context.session.user_id @@ -12437,8 +12458,19 @@ class RootExports(object): getUser = staticmethod(get_user) editUser = staticmethod(edit_user) - def grantPermission(self, userinfo, permission, create=False, description=None): - """Grant a permission to a user""" + def grantPermission(self, userinfo, permission, create=False, description=None, label=None): + """Grant a permission to a user + + :param userInfo: a str (Kerberos principal or name) or an int (user id) + or a dict: + - id: User's ID + - name: User's name + - krb_principal: Kerberos principal + :param str permission: Permission name + :param bool create: Create permission if it doesn't exist + :param str description: In case of creating new permission, description can be specified + :param str label: Reason/comment for grant operation + """ context.session.assertPerm('admin') if create: verify_name_internal(permission) @@ -12457,10 +12489,10 @@ class RootExports(object): (userinfo, perm['name'])) insert = InsertProcessor('user_perms') insert.set(user_id=user_id, perm_id=perm_id) - insert.make_create() + insert.make_create(label=label) insert.execute() - def revokePermission(self, userinfo, permission): + def revokePermission(self, userinfo, permission, label=None): """Revoke a permission from a user""" context.session.assertPerm('admin') user_id = get_user(userinfo, strict=True)['id'] @@ -12471,7 +12503,7 @@ class RootExports(object): (userinfo, perm['name'])) update = UpdateProcessor('user_perms', values=locals(), clauses=["user_id = %(user_id)i", "perm_id = %(perm_id)i"]) - update.make_revoke() + update.make_revoke(label=label) update.execute() def editPermission(self, permission, description): diff --git a/tests/test_cli/test_list_history.py b/tests/test_cli/test_list_history.py index 656b5a4..87d1afd 100644 --- a/tests/test_cli/test_list_history.py +++ b/tests/test_cli/test_list_history.py @@ -232,12 +232,14 @@ class TestListHistory(utils.CliTestCase): 'channel_id': 1, 'channels.name': 'default', 'create_event': 3, + 'create_event_label': None, 'create_ts': 1612355089.887727, 'creator_id': 1, 'creator_name': 'kojiadmin', 'host.name': 'kojibuilder', 'host_id': 1, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None}] @@ -260,12 +262,14 @@ class TestListHistory(utils.CliTestCase): 'channel_id': 1, 'channels.name': 'default', 'create_event': 3, + 'create_event_label': None, 'create_ts': 1612355089.887727, 'creator_id': 1, 'creator_name': 'kojiadmin', 'host.name': 'kojibuilder', 'host_id': 1, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None}] @@ -285,12 +289,14 @@ class TestListHistory(utils.CliTestCase): 'channel_id': 1, 'channels.name': 'default', 'create_event': 3, + 'create_event_label': None, 'create_ts': 1612355089.887727, 'creator_id': 1, 'creator_name': 'kojiadmin', 'host.name': 'kojibuilder', 'host_id': 1, 'revoke_event': 8, + 'revoke_event_label': None, 'revoke_ts': 1612355099.887727, 'revoker_id': 1, 'revoker_name': 'kojiadmin'}] @@ -328,6 +334,7 @@ class TestListHistory(utils.CliTestCase): 'capacity': 2.0, 'comment': None, 'create_event': 2, + 'create_event_label': None, 'create_ts': 1612355089.886359, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -336,6 +343,7 @@ class TestListHistory(utils.CliTestCase): 'host.name': 'kojibuilder', 'host_id': 1, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None}] @@ -357,6 +365,7 @@ class TestListHistory(utils.CliTestCase): 'capacity': 2.0, 'comment': None, 'create_event': 2, + 'create_event_label': None, 'create_ts': 1612355089.886359, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -365,6 +374,7 @@ class TestListHistory(utils.CliTestCase): 'host.name': 'kojibuilder', 'host_id': 1, 'revoke_event': 3, + 'revoke_event_label': None, 'revoke_ts': 1612355099.886359, 'revoker_id': 1, 'revoker_name': 'kojiadmin'}] @@ -403,6 +413,7 @@ class TestListHistory(utils.CliTestCase): 'build.state': 1, 'build_id': 6, 'create_event': 585, + 'create_event_label': None, 'create_ts': 1613744957.14465, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -410,6 +421,7 @@ class TestListHistory(utils.CliTestCase): 'name': 'test-build', 'release': '11', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -433,6 +445,7 @@ class TestListHistory(utils.CliTestCase): 'build.state': 1, 'build_id': 6, 'create_event': 585, + 'create_event_label': None, 'create_ts': 1613744957.14465, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -440,6 +453,7 @@ class TestListHistory(utils.CliTestCase): 'name': 'test-build', 'release': '11', 'revoke_event': 590, + 'revoke_event_label': None, 'revoke_ts': 1613744967.14465, 'revoker_id': 1, 'revoker_name': 'kojiadmin', @@ -483,6 +497,7 @@ class TestListHistory(utils.CliTestCase): 'build.state': 1, 'build_id': 4, 'create_event': 424, + 'create_event_label': None, 'create_ts': 1613736474.42776, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -490,6 +505,7 @@ class TestListHistory(utils.CliTestCase): 'name': 'pkg-name', 'release': '11', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -499,6 +515,7 @@ class TestListHistory(utils.CliTestCase): 'tag_package_owners': [ {'active': True, 'create_event': 418, + 'create_event_label': None, 'create_ts': 1613736220.79199, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -507,6 +524,7 @@ class TestListHistory(utils.CliTestCase): 'package.name': 'pkg-name', 'package_id': 4, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -516,6 +534,7 @@ class TestListHistory(utils.CliTestCase): {'active': True, 'blocked': False, 'create_event': 418, + 'create_event_label': None, 'create_ts': 1613736220.79199, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -523,6 +542,7 @@ class TestListHistory(utils.CliTestCase): 'package.name': 'pkg-name', 'package_id': 4, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -560,6 +580,7 @@ class TestListHistory(utils.CliTestCase): {'active': True, 'arches': 'x86_64', 'create_event': 6, + 'create_event_label': None, 'create_ts': 1612872591.313584, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -569,6 +590,7 @@ class TestListHistory(utils.CliTestCase): 'perm_id': None, 'permission.name': None, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -577,11 +599,13 @@ class TestListHistory(utils.CliTestCase): 'tag_extra': [ {'active': True, 'create_event': 6, + 'create_event_label': None, 'create_ts': 1612872591.313584, 'creator_id': 1, 'creator_name': 'kojiadmin', 'key': 'mock.package_manager', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -620,12 +644,14 @@ class TestListHistory(utils.CliTestCase): 'build_target.name': 'build-target-test-tag', 'build_target_id': 1, 'create_event': 8, + 'create_event_label': None, 'create_ts': 1612872656.231499, 'creator_id': 1, 'creator_name': 'kojiadmin', 'dest_tag': 3, 'dest_tag.name': 'destination-test-tag', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None}], @@ -636,12 +662,14 @@ class TestListHistory(utils.CliTestCase): 'channel_id': 1, 'channels.name': 'default', 'create_event': 3, + 'create_event_label': None, 'create_ts': 1612355089.887727, 'creator_id': 1, 'creator_name': 'kojiadmin', 'host.name': 'kojibuilder', 'host_id': 1, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None}], @@ -653,6 +681,7 @@ class TestListHistory(utils.CliTestCase): 'capacity': 2.0, 'comment': None, 'create_event': 2, + 'create_event_label': None, 'create_ts': 1612355089.886359, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -661,6 +690,7 @@ class TestListHistory(utils.CliTestCase): 'host.name': 'kojibuilder', 'host_id': 1, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None}], @@ -670,6 +700,7 @@ class TestListHistory(utils.CliTestCase): 'active': True, 'arches': '', 'create_event': 5, + 'create_event_label': None, 'create_ts': 1612871243.593475, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -679,6 +710,7 @@ class TestListHistory(utils.CliTestCase): 'perm_id': None, 'permission.name': None, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -689,11 +721,13 @@ class TestListHistory(utils.CliTestCase): '_revoked_by': None, 'active': True, 'create_event': 6, + 'create_event_label': None, 'create_ts': 1612872591.313584, 'creator_id': 1, 'creator_name': 'kojiadmin', 'key': 'mock.package_manager', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -705,6 +739,7 @@ class TestListHistory(utils.CliTestCase): '_revoked_by': True, 'active': None, 'create_event': 16, + 'create_event_label': None, 'create_ts': 1613545870.370125, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -716,6 +751,7 @@ class TestListHistory(utils.CliTestCase): 'pkg_filter': '', 'priority': 1, 'revoke_event': 31, + 'revoke_event_label': None, 'revoke_ts': 1613545887.513565, 'revoker_id': 1, 'revoker_name': 'kojiadmin', @@ -726,6 +762,7 @@ class TestListHistory(utils.CliTestCase): '_revoked_by': None, 'active': True, 'create_event': 9, + 'create_event_label': None, 'create_ts': 1612872778.934647, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -734,6 +771,7 @@ class TestListHistory(utils.CliTestCase): 'package.name': 'koji', 'package_id': 1, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -745,6 +783,7 @@ class TestListHistory(utils.CliTestCase): 'active': True, 'blocked': False, 'create_event': 9, + 'create_event_label': None, 'create_ts': 1612872778.934647, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -752,6 +791,7 @@ class TestListHistory(utils.CliTestCase): 'package.name': 'koji', 'package_id': 1, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -762,12 +802,14 @@ class TestListHistory(utils.CliTestCase): '_revoked_by': None, 'active': True, 'create_event': 1, + 'create_event_label': None, 'create_ts': 1612355089.882428, 'creator_id': 1, 'creator_name': 'kojiadmin', 'perm_id': 1, 'permission.name': 'admin', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -815,6 +857,7 @@ class TestListHistory(utils.CliTestCase): 'tag_package_owners': [ {'active': True, 'create_event': 9, + 'create_event_label': None, 'create_ts': 1613730799.934647, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -823,6 +866,7 @@ class TestListHistory(utils.CliTestCase): 'package.name': 'koji', 'package_id': 1, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -832,12 +876,14 @@ class TestListHistory(utils.CliTestCase): 'user_perms': [ {'active': True, 'create_event': 1, + 'create_event_label': None, 'create_ts': 1612355089.882428, 'creator_id': 1, 'creator_name': 'kojiadmin', 'perm_id': 1, 'permission.name': 'admin', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -845,12 +891,14 @@ class TestListHistory(utils.CliTestCase): 'user_id': 1}, {'active': None, 'create_event': 6, + 'create_event_label': None, 'create_ts': 1613730777.437744, 'creator_id': 1, 'creator_name': 'kojiadmin', 'perm_id': 4, 'permission.name': 'dist-repo', 'revoke_event': 7, + 'revoke_event_label': None, 'revoke_ts': 1613730790.797031, 'revoker_id': 1, 'revoker_name': 'kojiadmin', @@ -889,12 +937,14 @@ class TestListHistory(utils.CliTestCase): 'user_perms': [ {'active': True, 'create_event': 20, + 'create_event_label': None, 'create_ts': 1613730777.437744, 'creator_id': 1, 'creator_name': 'kojiadmin', 'perm_id': 4, 'permission.name': 'dist-repo', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -915,12 +965,14 @@ class TestListHistory(utils.CliTestCase): 'user_perms': [ {'active': None, 'create_event': 25, + 'create_event_label': None, 'create_ts': 1613730787.437744, 'creator_id': 1, 'creator_name': 'kojiadmin', 'perm_id': 4, 'permission.name': 'dist-repo', 'revoke_event': 27, + 'revoke_event_label': None, 'revoke_ts': 1613730797.797031, 'revoker_id': 1, 'revoker_name': 'kojiadmin', @@ -955,10 +1007,12 @@ class TestListHistory(utils.CliTestCase): 'cg_id': 1, 'content_generator.name': 'test-cg', 'create_event': 425, + 'create_event_label': None, 'create_ts': 1613736494.11504, 'creator_id': 1, 'creator_name': 'kojiadmin', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -979,10 +1033,12 @@ class TestListHistory(utils.CliTestCase): 'cg_id': 2, 'content_generator.name': 'test-cg', 'create_event': 430, + 'create_event_label': None, 'create_ts': 1613736499.11504, 'creator_id': 1, 'creator_name': 'kojiadmin', 'revoke_event': 440, + 'revoke_event_label': None, 'revoke_ts': 1613736510.11504, 'revoker_id': 1, 'revoker_name': 'kojiadmin', @@ -1010,12 +1066,14 @@ class TestListHistory(utils.CliTestCase): 'external_repo_config': [ {'active': True, 'create_event': 342, + 'create_event_label': None, 'create_ts': 1613736061.68861, 'creator_id': 1, 'creator_name': 'kojiadmin', 'external_repo.name': 'external-repo-test', 'external_repo_id': 5, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -1024,6 +1082,7 @@ class TestListHistory(utils.CliTestCase): 'tag_external_repos': [ {'active': True, 'create_event': 343, + 'create_event_label': None, 'create_ts': 1613736061.72882, 'creator_id': 1, 'creator_name': 'kojiadmin', @@ -1032,6 +1091,7 @@ class TestListHistory(utils.CliTestCase): 'merge_mode': 'koji', 'priority': 5, 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None, @@ -1073,12 +1133,14 @@ class TestListHistory(utils.CliTestCase): 'build_target.name': 'test-build-target', 'build_target_id': 5, 'create_event': 420, + 'create_event_label': None, 'create_ts': 1613736230.7615, 'creator_id': 1, 'creator_name': 'kojiadmin', 'dest_tag': 11, 'dest_tag.name': 'destination-test-tag', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None}] @@ -1101,12 +1163,14 @@ class TestListHistory(utils.CliTestCase): 'build_target.name': 'test-build-target', 'build_target_id': 4, 'create_event': 417, + 'create_event_label': None, 'create_ts': 1613736220.7615, 'creator_id': 1, 'creator_name': 'kojiadmin', 'dest_tag': 10, 'dest_tag.name': 'destination-test-tag', 'revoke_event': 420, + 'revoke_event_label': None, 'revoke_ts': 1613736225.7615, 'revoker_id': 1, 'revoker_name': 'kojiadmin'}] @@ -1136,11 +1200,13 @@ class TestListHistory(utils.CliTestCase): 'tag_extra': [ {'active': True, 'create_event': 586, + 'create_event_label': None, 'create_ts': 1613744977.02986, 'creator_id': 1, 'creator_name': 'kojiadmin', 'key': 'extra-key-test', 'revoke_event': None, + 'revoke_event_label': None, 'revoke_ts': None, 'revoker_id': None, 'revoker_name': None,