#615 don't send notifications to disabled users or hosts
Merged 6 years ago by mikem. Opened 6 years ago by tkopecek.
tkopecek/koji issue613  into  master

file modified
+19 -6
@@ -7252,9 +7252,20 @@ 

      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 = []

+     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 = [

+         'status = %(users_status)i',

+         'usertype IN %(users_usertype)s',

+     ]

+ 

+     if not build and tag_id:

+         raise koji.GenericError('Invalid call')

  

      if build:

          package_id = build['package_id']
@@ -7269,14 +7280,14 @@ 

          clauses.append('success_only = FALSE')

  

      query = QueryProcessor(columns=('email',), tables=['build_notifications'],

-                            clauses=clauses, values=locals(),

+                            joins=joins, clauses=clauses, values=locals(),

                             opts={'asList':True})

      emails = [result[0] for result in query.execute()]

  

      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 +7298,14 @@ 

              # 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 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'):

@@ -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, [])

+ 

+         # 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'])

+         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)

+         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, ['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'])

+         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)

+         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, ['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'])

+         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)

+         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, ['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, ['owner_name@test.domain.com'])

please avoid constructing queries this way. Use the safer parameter method whenever possible.

for now, let's also include groups

or perhaps simply exclude hosts

1 new commit added

  • make query more secure
6 years ago

rebased onto 002060860e552b83e0fad5ca6388b5cfa642cd21

6 years ago

It seems strange to me to have users.id = build_notifications.user_id as a clause rather than a join condition. I guess they are technically equivalent here, but it seems like it would be clearer to have an explicit join as we do elsewhere.

rebased onto 90f760944fb9fb15c0cdd3dd28c720fe7b8e9829

6 years ago

We don't want a left join here

rebased onto 9fc3c16d2f53ac4327c922e9b25562be35fdec00

6 years ago

1 new commit added

  • fix tests
6 years ago

This changes the return type of this function.
While tag_notification simply loops over the result, build_notification passes it verbatim as an option to the buildNotification task. This will break because set objects cannot be represented in xmlrpc.
Probably just return list(set(emails))

rebased onto 9c12c2f

6 years ago

Commit 882f513 fixes this pull-request

Pull-Request has been merged by mikem@redhat.com

6 years ago