From b41dce6c349aafe8c2389564c1f7b151918e578c Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 05 2019 12:12:35 +0000
Subject: [PATCH 1/3] createEmptyBuild errors for non-existent user


Until now, error was raised from postgres insert. Now is user's
existence checked before running insert.

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

---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index 51255f2..22fde61 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -5105,6 +5105,9 @@ def new_build(data):
         if not name:
             raise koji.GenericError("No name or package id provided for build")
         data['pkg_id'] = new_package(name, strict=False)
+    if data.get('owner'):
+        # check, that user exists
+        get_user(data['owner'], strict=True)
     for f in ('version', 'release', 'epoch'):
         if f not in data:
             raise koji.GenericError("No %s value for build" % f)
diff --git a/tests/test_hub/test_new_build.py b/tests/test_hub/test_new_build.py
new file mode 100644
index 0000000..f7033e7
--- /dev/null
+++ b/tests/test_hub/test_new_build.py
@@ -0,0 +1,161 @@
+from __future__ import absolute_import
+import mock
+try:
+    import unittest2 as unittest
+except ImportError:
+    import unittest
+
+import koji
+import kojihub
+
+IP = kojihub.InsertProcessor
+
+class TestNewBuild(unittest.TestCase):
+    def setUp(self):
+        self.get_rpm = mock.patch('kojihub.get_rpm').start()
+        self.get_external_repo_id = mock.patch('kojihub.get_external_repo_id').start()
+        self.nextval = mock.patch('kojihub.nextval').start()
+        self.Savepoint = mock.patch('kojihub.Savepoint').start()
+        self.InsertProcessor = mock.patch('kojihub.InsertProcessor',
+                side_effect=self.getInsert).start()
+        self.inserts = []
+        self.insert_execute = mock.MagicMock()
+        self.lookup_package = mock.patch('kojihub.lookup_package').start()
+        self.new_package = mock.patch('kojihub.new_package').start()
+        self.get_user = mock.patch('kojihub.get_user').start()
+        self.get_build = mock.patch('kojihub.get_build').start()
+        self.recycle_build = mock.patch('kojihub.recycle_build').start()
+        self.context = mock.patch('kojihub.context').start()
+        self._singleValue = mock.patch('kojihub._singleValue').start()
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def getInsert(self, *args, **kwargs):
+        insert = IP(*args, **kwargs)
+        insert.execute = self.insert_execute
+        self.inserts.append(insert)
+        return insert
+
+    def test_valid(self):
+        self.context.session.user_id = 123456
+        self.get_build.return_value = None
+        self._singleValue.return_value = 65 # free build id
+        self.new_package.return_value = 54
+        data = {
+            'name': 'test_name',
+            'version': 'test_version',
+            'release': 'test_release',
+            'epoch': 'test_epoch',
+            'owner': 'test_owner',
+            'extra': {'extra_key': 'extra_value'},
+        }
+
+        kojihub.new_build(data)
+
+        self.assertEqual(len(self.inserts), 1)
+        insert = self.inserts[0]
+        self.assertEqual(insert.table, 'build')
+        self.assertEqual(insert.data, {
+            'completion_time': 'NOW',
+            'epoch': 'test_epoch',
+            'extra': '{"extra_key": "extra_value"}',
+            'id': 65,
+            'owner': 'test_owner',
+            'pkg_id': 54,
+            'release': 'test_release',
+            'source': None,
+            'start_time': 'NOW',
+            'state': 1,
+            'task_id': None,
+            'version': 'test_version',
+            'volume_id': 0
+        })
+
+    def test_empty_data(self):
+        with self.assertRaises(koji.GenericError):
+            kojihub.new_build({})
+        self.assertEqual(len(self.inserts), 0)
+
+    def test_wrong_pkg_id(self):
+        self.lookup_package.side_effect = koji.GenericError
+        data = {
+            'pkg_id': 444,
+            'name': 'test_name',
+            'version': 'test_version',
+            'release': 'test_release',
+            'epoch': 'test_epoch',
+            'owner': 'test_owner',
+            'extra': {'extra_key': 'extra_value'},
+        }
+
+        with self.assertRaises(koji.GenericError):
+            kojihub.new_build(data)
+
+        self.assertEqual(len(self.inserts), 0)
+
+    def test_missing_pkg_id_name(self):
+        data = {
+            'version': 'test_version',
+            'release': 'test_release',
+            'epoch': 'test_epoch',
+            'owner': 'test_owner',
+            'extra': {'extra_key': 'extra_value'},
+        }
+
+        with self.assertRaises(koji.GenericError):
+            kojihub.new_build(data)
+
+        self.assertEqual(len(self.inserts), 0)
+
+    def test_wrong_owner(self):
+        self.get_user.side_effect = koji.GenericError
+        data = {
+            'owner': 123456,
+            'name': 'test_name',
+            'version': 'test_version',
+            'release': 'test_release',
+            'epoch': 'test_epoch',
+            'extra': {'extra_key': 'extra_value'},
+        }
+
+        with self.assertRaises(koji.GenericError):
+            kojihub.new_build(data)
+
+        self.assertEqual(len(self.inserts), 0)
+
+    def test_missing_vre(self):
+        self.get_user.side_effect = koji.GenericError
+        data = {
+            'name': 'test_name',
+            'version': 'test_version',
+            'release': 'test_release',
+            'epoch': 'test_epoch',
+        }
+
+        for item in ('version', 'release', 'epoch'):
+            d = data.copy()
+            del d[item]
+            with self.assertRaises(koji.GenericError):
+                kojihub.new_build(d)
+
+        self.assertEqual(len(self.inserts), 0)
+
+    def test_wrong_extra(self):
+        # extra dict contains unserializable data
+        class CantDoJSON(object):
+            pass
+
+        data = {
+            'owner': 123456,
+            'name': 'test_name',
+            'version': 'test_version',
+            'release': 'test_release',
+            'epoch': 'test_epoch',
+            'extra': {'extra_key': CantDoJSON()},
+        }
+
+        with self.assertRaises(koji.GenericError):
+            kojihub.new_build(data)
+
+        self.assertEqual(len(self.inserts), 0)

From 7c7468e1b39bd7d6628c8259810c6c3b9aa71f33 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 07 2019 13:19:57 +0000
Subject: [PATCH 2/3] convert username to user_id


---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index 22fde61..e944048 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -5106,8 +5106,8 @@ def new_build(data):
             raise koji.GenericError("No name or package id provided for build")
         data['pkg_id'] = new_package(name, strict=False)
     if data.get('owner'):
-        # check, that user exists
-        get_user(data['owner'], strict=True)
+        # check, that user exists (and convert name to id)
+        data['owner'] = get_user(data['owner'], strict=True)['id']
     for f in ('version', 'release', 'epoch'):
         if f not in data:
             raise koji.GenericError("No %s value for build" % f)

From b95019267bc68a1f09878a3c648437064721d45d Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 07 2019 13:25:43 +0000
Subject: [PATCH 3/3] fix test for numeric user_id


---

diff --git a/tests/test_hub/test_new_build.py b/tests/test_hub/test_new_build.py
index f7033e7..fea39c3 100644
--- a/tests/test_hub/test_new_build.py
+++ b/tests/test_hub/test_new_build.py
@@ -38,10 +38,10 @@ class TestNewBuild(unittest.TestCase):
         return insert
 
     def test_valid(self):
-        self.context.session.user_id = 123456
         self.get_build.return_value = None
         self._singleValue.return_value = 65 # free build id
         self.new_package.return_value = 54
+        self.get_user.return_value = {'id': 123}
         data = {
             'name': 'test_name',
             'version': 'test_version',
@@ -61,7 +61,7 @@ class TestNewBuild(unittest.TestCase):
             'epoch': 'test_epoch',
             'extra': '{"extra_key": "extra_value"}',
             'id': 65,
-            'owner': 'test_owner',
+            'owner': 123,
             'pkg_id': 54,
             'release': 'test_release',
             'source': None,

