From 9c12c2f595c38f77ca0bf458ddc705e0b2c8dbe2 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 31 2017 12:36:07 +0000 Subject: [PATCH 1/5] don't send notifications to disabled users or hosts Related: https://pagure.io/koji/issue/613 --- diff --git a/hub/kojihub.py b/hub/kojihub.py index cbad388..41efdbe 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -7252,9 +7252,18 @@ def get_notification_recipients(build, tag_id, state): notifications on the package or tag (or both), as well as the package owner for this tag and the user who submitted the build. The list will not contain duplicates. + + Only active 'human' users will be in this list. """ - clauses = [] + clauses = [ + 'users.id = build_notifications.user_id', + 'users.status = %s' % koji.USER_STATUS['NORMAL'], + 'users.usertype = %s' % koji.USERTYPES['NORMAL'], + ] + + if not build and tag_id: + raise koji.GenericError('Invalid call') if build: package_id = build['package_id'] @@ -7268,7 +7277,7 @@ def get_notification_recipients(build, tag_id, state): if state != koji.BUILD_STATES['COMPLETE']: clauses.append('success_only = FALSE') - query = QueryProcessor(columns=('email',), tables=['build_notifications'], + query = QueryProcessor(columns=('email',), tables=['build_notifications', 'users'], clauses=clauses, values=locals(), opts={'asList':True}) emails = [result[0] for result in query.execute()] @@ -7276,7 +7285,7 @@ def get_notification_recipients(build, tag_id, state): email_domain = context.opts['EmailDomain'] notify_on_success = context.opts['NotifyOnSuccess'] - if notify_on_success is True or state != koji.BUILD_STATES['COMPLETE']: + if build and (notify_on_success is True or state != koji.BUILD_STATES['COMPLETE']): # user who submitted the build emails.append('%s@%s' % (build['owner_name'], email_domain)) @@ -7287,12 +7296,14 @@ def get_notification_recipients(build, tag_id, state): # If the package list has changed very recently it is possible we # will get no result. if pkgdata and not pkgdata['blocked']: - emails.append('%s@%s' % (pkgdata['owner_name'], email_domain)) + owner = get_user(pkgdata['owner_id'], strict=True) + if owner['status'] == koji.USER_STATUS['NORMAL'] and \ + owner['usertype'] == koji.USERTYPES['NORMAL']: + emails.append('%s@%s' % (owner['name'], email_domain)) #FIXME - if tag_id is None, we don't have a good way to get the package owner. # using all package owners from all tags would be way overkill. - emails_uniq = dict([(x, 1) for x in emails]).keys() - return emails_uniq + return set(emails) def tag_notification(is_successful, tag_id, from_id, build_id, user_id, ignore_success=False, failure_msg=''): if context.opts.get('DisableNotifications'): diff --git a/tests/test_hub/test_notifications.py b/tests/test_hub/test_notifications.py new file mode 100644 index 0000000..4c47b0e --- /dev/null +++ b/tests/test_hub/test_notifications.py @@ -0,0 +1,146 @@ +import mock +import unittest + +import koji +import kojihub + +QP = kojihub.QueryProcessor + +class TestGetNotificationRecipients(unittest.TestCase): + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = mock.MagicMock() + self.queries.append(query) + return query + + def setUp(self): + self.context = mock.patch('kojihub.context').start() + self.context.opts = { + 'EmailDomain': 'test.domain.com', + 'NotifyOnSuccess': True, + } + + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + + def tearDown(self): + mock.patch.stopall() + + + @mock.patch('kojihub.get_user') + @mock.patch('kojihub.readPackageList') + def test_get_notification_recipients(self, readPackageList, get_user): + # without build / tag_id + build = None + tag_id = None + state = koji.BUILD_STATES['CANCELED'] + + emails = kojihub.get_notification_recipients(build, tag_id, state) + self.assertEqual(emails, set([])) + + # only query to watchers + self.assertEqual(len(self.queries), 1) + q = self.queries[0] + self.assertEqual(q.columns, ('email',)) + self.assertEqual(q.tables, ['build_notifications', 'users']) + self.assertEqual(q.clauses, ['users.id = build_notifications.user_id', + 'users.status = 0', + 'users.usertype = 0', + 'package_id IS NULL', + 'tag_id IS NULL', + 'success_only = FALSE']) + self.assertEqual(q.values['state'], state) + self.assertEqual(q.values['build'], build) + self.assertEqual(q.values['tag_id'], tag_id) + readPackageList.assert_not_called() + + + ### with build without tag + build = {'package_id': 12345, 'owner_name': 'owner_name'} + self.queries = [] + + emails = kojihub.get_notification_recipients(build, tag_id, state) + self.assertEqual(emails, set(['owner_name@test.domain.com'])) + + # there should be only query to watchers + self.assertEqual(len(self.queries), 1) + q = self.queries[0] + self.assertEqual(q.columns, ('email',)) + self.assertEqual(q.tables, ['build_notifications', 'users']) + self.assertEqual(q.clauses, ['users.id = build_notifications.user_id', + 'users.status = 0', + 'users.usertype = 0', + 'package_id = %(package_id)i OR package_id IS NULL', + 'tag_id IS NULL', + 'success_only = FALSE']) + self.assertEqual(q.values['package_id'], build['package_id']) + self.assertEqual(q.values['state'], state) + self.assertEqual(q.values['build'], build) + self.assertEqual(q.values['tag_id'], tag_id) + readPackageList.assert_not_called() + + ### with tag without build makes no sense + build = None + tag_id = 123 + self.queries = [] + + with self.assertRaises(koji.GenericError): + kojihub.get_notification_recipients(build, tag_id, state) + self.assertEqual(self.queries, []) + readPackageList.assert_not_called() + + + ### with tag and build + build = {'package_id': 12345, 'owner_name': 'owner_name'} + tag_id = 123 + self.queries = [] + readPackageList.return_value = {12345: {'blocked': False, 'owner_id': 'owner_id'}} + get_user.return_value = { + 'id': 'owner_id', + 'name': 'pkg_owner_name', + 'status': koji.USER_STATUS['NORMAL'], + 'usertype': koji.USERTYPES['NORMAL'] + } + + emails = kojihub.get_notification_recipients(build, tag_id, state) + self.assertEqual(emails, set(['owner_name@test.domain.com', 'pkg_owner_name@test.domain.com'])) + + + # there should be only query to watchers + self.assertEqual(len(self.queries), 1) + q = self.queries[0] + self.assertEqual(q.columns, ('email',)) + self.assertEqual(q.tables, ['build_notifications', 'users']) + self.assertEqual(q.clauses, ['users.id = build_notifications.user_id', + 'users.status = 0', + 'users.usertype = 0', + 'package_id = %(package_id)i OR package_id IS NULL', + 'tag_id = %(tag_id)i OR tag_id IS NULL', + 'success_only = FALSE']) + self.assertEqual(q.values['package_id'], build['package_id']) + self.assertEqual(q.values['state'], state) + self.assertEqual(q.values['build'], build) + self.assertEqual(q.values['tag_id'], tag_id) + readPackageList.assert_called_once_with(pkgID=build['package_id'], tagID=tag_id, inherit=True) + get_user.asssert_called_once_with('owner_id', strict=True) + + # blocked package owner + get_user.return_value = { + 'id': 'owner_id', + 'name': 'pkg_owner_name', + 'status': koji.USER_STATUS['BLOCKED'], + 'usertype': koji.USERTYPES['NORMAL'] + } + emails = kojihub.get_notification_recipients(build, tag_id, state) + self.assertEqual(emails, set(['owner_name@test.domain.com'])) + + # package owner is machine + get_user.return_value = { + 'id': 'owner_id', + 'name': 'pkg_owner_name', + 'status': koji.USER_STATUS['NORMAL'], + 'usertype': koji.USERTYPES['HOST'] + } + emails = kojihub.get_notification_recipients(build, tag_id, state) + self.assertEqual(emails, set(['owner_name@test.domain.com'])) From a8e2844ad834bf6d75bd9fb0f2591f046b02dac2 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 31 2017 12:36:07 +0000 Subject: [PATCH 2/5] make query more secure --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 41efdbe..b057c20 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -7256,10 +7256,12 @@ def get_notification_recipients(build, tag_id, state): Only active 'human' users will be in this list. """ + users_status = koji.USER_STATUS['NORMAL'] + users_usertypes = [koji.USERTYPES['NORMAL'], koji.USERTYPES['GROUP']] clauses = [ 'users.id = build_notifications.user_id', - 'users.status = %s' % koji.USER_STATUS['NORMAL'], - 'users.usertype = %s' % koji.USERTYPES['NORMAL'], + 'users.status = %(users_status)i', + 'users.usertype IN %(users_usertype)s', ] if not build and tag_id: diff --git a/tests/test_hub/test_notifications.py b/tests/test_hub/test_notifications.py index 4c47b0e..5ff9bb3 100644 --- a/tests/test_hub/test_notifications.py +++ b/tests/test_hub/test_notifications.py @@ -45,8 +45,8 @@ class TestGetNotificationRecipients(unittest.TestCase): self.assertEqual(q.columns, ('email',)) self.assertEqual(q.tables, ['build_notifications', 'users']) self.assertEqual(q.clauses, ['users.id = build_notifications.user_id', - 'users.status = 0', - 'users.usertype = 0', + 'users.status = %(users_status)i', + 'users.usertype IN %(users_usertype)s', 'package_id IS NULL', 'tag_id IS NULL', 'success_only = FALSE']) @@ -69,8 +69,8 @@ class TestGetNotificationRecipients(unittest.TestCase): self.assertEqual(q.columns, ('email',)) self.assertEqual(q.tables, ['build_notifications', 'users']) self.assertEqual(q.clauses, ['users.id = build_notifications.user_id', - 'users.status = 0', - 'users.usertype = 0', + 'users.status = %(users_status)i', + 'users.usertype IN %(users_usertype)s', 'package_id = %(package_id)i OR package_id IS NULL', 'tag_id IS NULL', 'success_only = FALSE']) @@ -113,8 +113,8 @@ class TestGetNotificationRecipients(unittest.TestCase): self.assertEqual(q.columns, ('email',)) self.assertEqual(q.tables, ['build_notifications', 'users']) self.assertEqual(q.clauses, ['users.id = build_notifications.user_id', - 'users.status = 0', - 'users.usertype = 0', + 'users.status = %(users_status)i', + 'users.usertype IN %(users_usertype)s', 'package_id = %(package_id)i OR package_id IS NULL', 'tag_id = %(tag_id)i OR tag_id IS NULL', 'success_only = FALSE']) From 9e13423b5498e5442bedbfcc1380f22d63871b5f Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 31 2017 12:36:07 +0000 Subject: [PATCH 3/5] use explicit JOIN --- diff --git a/hub/kojihub.py b/hub/kojihub.py index b057c20..5e794b9 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -7256,12 +7256,12 @@ def get_notification_recipients(build, tag_id, state): Only active 'human' users will be in this list. """ + joins = ['JOIN users ON build_notifications.user_id = users.id'] users_status = koji.USER_STATUS['NORMAL'] users_usertypes = [koji.USERTYPES['NORMAL'], koji.USERTYPES['GROUP']] clauses = [ - 'users.id = build_notifications.user_id', - 'users.status = %(users_status)i', - 'users.usertype IN %(users_usertype)s', + 'status = %(users_status)i', + 'usertype IN %(users_usertype)s', ] if not build and tag_id: @@ -7279,8 +7279,8 @@ def get_notification_recipients(build, tag_id, state): if state != koji.BUILD_STATES['COMPLETE']: clauses.append('success_only = FALSE') - query = QueryProcessor(columns=('email',), tables=['build_notifications', 'users'], - clauses=clauses, values=locals(), + query = QueryProcessor(columns=('email',), tables=['build_notifications'], + joins=joins, clauses=clauses, values=locals(), opts={'asList':True}) emails = [result[0] for result in query.execute()] From 7cef8b9ef5dd1277833ceaa57fd6248c79b5303e Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 31 2017 12:36:07 +0000 Subject: [PATCH 4/5] fix tests --- diff --git a/tests/test_hub/test_notifications.py b/tests/test_hub/test_notifications.py index 5ff9bb3..793a884 100644 --- a/tests/test_hub/test_notifications.py +++ b/tests/test_hub/test_notifications.py @@ -43,13 +43,13 @@ class TestGetNotificationRecipients(unittest.TestCase): self.assertEqual(len(self.queries), 1) q = self.queries[0] self.assertEqual(q.columns, ('email',)) - self.assertEqual(q.tables, ['build_notifications', 'users']) - self.assertEqual(q.clauses, ['users.id = build_notifications.user_id', - 'users.status = %(users_status)i', - 'users.usertype IN %(users_usertype)s', + self.assertEqual(q.tables, ['build_notifications']) + self.assertEqual(q.clauses, [ 'status = %(users_status)i', + 'usertype IN %(users_usertype)s', 'package_id IS NULL', 'tag_id IS NULL', 'success_only = FALSE']) + self.assertEqual(q.joins, ['JOIN users ON build_notifications.user_id = users.id']) self.assertEqual(q.values['state'], state) self.assertEqual(q.values['build'], build) self.assertEqual(q.values['tag_id'], tag_id) @@ -67,13 +67,13 @@ class TestGetNotificationRecipients(unittest.TestCase): self.assertEqual(len(self.queries), 1) q = self.queries[0] self.assertEqual(q.columns, ('email',)) - self.assertEqual(q.tables, ['build_notifications', 'users']) - self.assertEqual(q.clauses, ['users.id = build_notifications.user_id', - 'users.status = %(users_status)i', - 'users.usertype IN %(users_usertype)s', + self.assertEqual(q.tables, ['build_notifications']) + self.assertEqual(q.clauses, ['status = %(users_status)i', + 'usertype IN %(users_usertype)s', 'package_id = %(package_id)i OR package_id IS NULL', 'tag_id IS NULL', 'success_only = FALSE']) + self.assertEqual(q.joins, ['JOIN users ON build_notifications.user_id = users.id']) self.assertEqual(q.values['package_id'], build['package_id']) self.assertEqual(q.values['state'], state) self.assertEqual(q.values['build'], build) @@ -111,13 +111,13 @@ class TestGetNotificationRecipients(unittest.TestCase): self.assertEqual(len(self.queries), 1) q = self.queries[0] self.assertEqual(q.columns, ('email',)) - self.assertEqual(q.tables, ['build_notifications', 'users']) - self.assertEqual(q.clauses, ['users.id = build_notifications.user_id', - 'users.status = %(users_status)i', - 'users.usertype IN %(users_usertype)s', + self.assertEqual(q.tables, ['build_notifications']) + self.assertEqual(q.clauses, ['status = %(users_status)i', + 'usertype IN %(users_usertype)s', 'package_id = %(package_id)i OR package_id IS NULL', 'tag_id = %(tag_id)i OR tag_id IS NULL', 'success_only = FALSE']) + self.assertEqual(q.joins, ['JOIN users ON build_notifications.user_id = users.id']) self.assertEqual(q.values['package_id'], build['package_id']) self.assertEqual(q.values['state'], state) self.assertEqual(q.values['build'], build) From 7fa2a48a2b2f01281d7cfd7952476e647f2d4c29 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 31 2017 12:37:28 +0000 Subject: [PATCH 5/5] return xmlrpc-compatible list instead of set --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 5e794b9..eb909db 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -7305,7 +7305,7 @@ def get_notification_recipients(build, tag_id, state): #FIXME - if tag_id is None, we don't have a good way to get the package owner. # using all package owners from all tags would be way overkill. - return set(emails) + return list(set(emails)) def tag_notification(is_successful, tag_id, from_id, build_id, user_id, ignore_success=False, failure_msg=''): if context.opts.get('DisableNotifications'): diff --git a/tests/test_hub/test_notifications.py b/tests/test_hub/test_notifications.py index 793a884..74573c4 100644 --- a/tests/test_hub/test_notifications.py +++ b/tests/test_hub/test_notifications.py @@ -37,7 +37,7 @@ class TestGetNotificationRecipients(unittest.TestCase): state = koji.BUILD_STATES['CANCELED'] emails = kojihub.get_notification_recipients(build, tag_id, state) - self.assertEqual(emails, set([])) + self.assertEqual(emails, []) # only query to watchers self.assertEqual(len(self.queries), 1) @@ -61,7 +61,7 @@ class TestGetNotificationRecipients(unittest.TestCase): self.queries = [] emails = kojihub.get_notification_recipients(build, tag_id, state) - self.assertEqual(emails, set(['owner_name@test.domain.com'])) + self.assertEqual(emails, ['owner_name@test.domain.com']) # there should be only query to watchers self.assertEqual(len(self.queries), 1) @@ -104,7 +104,7 @@ class TestGetNotificationRecipients(unittest.TestCase): } emails = kojihub.get_notification_recipients(build, tag_id, state) - self.assertEqual(emails, set(['owner_name@test.domain.com', 'pkg_owner_name@test.domain.com'])) + self.assertEqual(emails, ['owner_name@test.domain.com', 'pkg_owner_name@test.domain.com']) # there should be only query to watchers @@ -133,7 +133,7 @@ class TestGetNotificationRecipients(unittest.TestCase): 'usertype': koji.USERTYPES['NORMAL'] } emails = kojihub.get_notification_recipients(build, tag_id, state) - self.assertEqual(emails, set(['owner_name@test.domain.com'])) + self.assertEqual(emails, ['owner_name@test.domain.com']) # package owner is machine get_user.return_value = { @@ -143,4 +143,4 @@ class TestGetNotificationRecipients(unittest.TestCase): 'usertype': koji.USERTYPES['HOST'] } emails = kojihub.get_notification_recipients(build, tag_id, state) - self.assertEqual(emails, set(['owner_name@test.domain.com'])) + self.assertEqual(emails, ['owner_name@test.domain.com'])