From e9cafb37bac5bd470b71c16bd8b87e44030691c3 Mon Sep 17 00:00:00 2001 From: mprahl Date: Dec 01 2019 16:47:54 +0000 Subject: [PATCH 1/3] Use advisory IDs as the Event search_key values in test_views.py For some reason, these were using the advisory names instead of the advisory IDs, which is not what Freshmaker does in practice. This change is in preparation for adding the ability to inherit the advisory associated with a "dependent" Freshmaker event during a manual rebuild submission. --- diff --git a/tests/test_views.py b/tests/test_views.py index 649715f..985d9a4 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -115,12 +115,12 @@ class TestViews(helpers.ModelsTestCase): self.client = app.test_client() def _init_data(self): - event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000001", "RHSA-2018-101", events.TestingEvent) + event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000001", "101", events.TestingEvent) build = models.ArtifactBuild.create(db.session, event, "ed", "module", 1234) build.build_args = '{"key": "value"}' models.ArtifactBuild.create(db.session, event, "mksh", "module", 1235) models.ArtifactBuild.create(db.session, event, "bash", "module", 1236) - models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000002", "RHSA-2018-102", events.TestingEvent) + models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000002", "102", events.TestingEvent) db.session.commit() db.session.expire_all() @@ -146,7 +146,7 @@ class TestViews(helpers.ModelsTestCase): self.assertIn(build_id, [b['build_id'] for b in builds]) def test_query_builds_order_by_default(self): - event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", "RHSA-2018-103", events.TestingEvent) + event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", "103", events.TestingEvent) build9 = models.ArtifactBuild.create(db.session, event, "make", "module", 1237) build9.id = 9 db.session.commit() @@ -161,7 +161,7 @@ class TestViews(helpers.ModelsTestCase): self.assertEqual(id, build['id']) def test_query_builds_order_by_id_asc(self): - event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", "RHSA-2018-103", events.TestingEvent) + event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", "103", events.TestingEvent) build9 = models.ArtifactBuild.create(db.session, event, "make", "module", 1237) build9.id = 9 db.session.commit() @@ -176,7 +176,7 @@ class TestViews(helpers.ModelsTestCase): self.assertEqual(id, build['id']) def test_query_builds_order_by_build_id_desc(self): - event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", "RHSA-2018-103", events.TestingEvent) + event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", "103", events.TestingEvent) build9 = models.ArtifactBuild.create(db.session, event, "make", "module", 1237) build9.id = 9 db.session.commit() @@ -285,20 +285,20 @@ class TestViews(helpers.ModelsTestCase): self.assertEqual(len(builds), 0) def test_query_build_by_event_search_key(self): - resp = self.client.get('/api/1/builds/?event_search_key=RHSA-2018-101') + resp = self.client.get('/api/1/builds/?event_search_key=101') builds = json.loads(resp.get_data(as_text=True))['items'] self.assertEqual(len(builds), 3) - resp = self.client.get('/api/1/builds/?event_search_key=RHSA-2018-102') + resp = self.client.get('/api/1/builds/?event_search_key=102') builds = json.loads(resp.get_data(as_text=True))['items'] self.assertEqual(len(builds), 0) def test_query_build_by_event_type_id_and_search_key(self): - resp = self.client.get('/api/1/builds/?event_type_id=%s&event_search_key=RHSA-2018-101' % models.EVENT_TYPES[events.TestingEvent]) + resp = self.client.get('/api/1/builds/?event_type_id=%s&event_search_key=101' % models.EVENT_TYPES[events.TestingEvent]) builds = json.loads(resp.get_data(as_text=True))['items'] self.assertEqual(len(builds), 3) - resp = self.client.get('/api/1/builds/?event_type_id=%s&event_search_key=RHSA-2018-102' % models.EVENT_TYPES[events.TestingEvent]) + resp = self.client.get('/api/1/builds/?event_type_id=%s&event_search_key=102' % models.EVENT_TYPES[events.TestingEvent]) builds = json.loads(resp.get_data(as_text=True))['items'] self.assertEqual(len(builds), 0) @@ -307,7 +307,7 @@ class TestViews(helpers.ModelsTestCase): data = json.loads(resp.get_data(as_text=True)) self.assertEqual(data['id'], 1) self.assertEqual(data['message_id'], '2017-00000000-0000-0000-0000-000000000001') - self.assertEqual(data['search_key'], 'RHSA-2018-101') + self.assertEqual(data['search_key'], '101') self.assertEqual(data['event_type_id'], models.EVENT_TYPES[events.TestingEvent]) self.assertEqual(len(data['builds']), 3) @@ -356,10 +356,10 @@ class TestViews(helpers.ModelsTestCase): self.assertEqual(evs[0]['message_id'], '2017-00000000-0000-0000-0000-000000000001') def test_query_event_by_search_key(self): - resp = self.client.get('/api/1/events/?search_key=RHSA-2018-101') + resp = self.client.get('/api/1/events/?search_key=101') evs = json.loads(resp.get_data(as_text=True))['items'] self.assertEqual(len(evs), 1) - self.assertEqual(evs[0]['search_key'], 'RHSA-2018-101') + self.assertEqual(evs[0]['search_key'], '101') def test_query_event_by_state_name(self): models.Event.create(db.session, @@ -520,8 +520,8 @@ class TestViews(helpers.ModelsTestCase): 'msg': 'Found 1 images which are handled by Freshmaker for defined content_sets.'}) def test_dependencies(self): - event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", "RHSA-2018-103", events.TestingEvent) - event1 = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000004", "RHSA-2018-104", events.TestingEvent) + event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", "103", events.TestingEvent) + event1 = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000004", "104", events.TestingEvent) db.session.commit() event.add_event_dependency(db.session, event1) db.session.commit() @@ -549,7 +549,7 @@ class TestViewsMultipleFilterValues(helpers.ModelsTestCase): def _init_data(self): event = models.Event.create( db.session, "2017-00000000-0000-0000-0000-000000000001", - "RHSA-2018-101", events.TestingEvent) + "101", events.TestingEvent) event.state = EventState.BUILDING.value build = models.ArtifactBuild.create(db.session, event, "ed", "module", 1234) build.build_args = '{"key": "value"}' @@ -557,11 +557,11 @@ class TestViewsMultipleFilterValues(helpers.ModelsTestCase): models.ArtifactBuild.create(db.session, event, "bash", "module", 1236) event2 = models.Event.create( db.session, "2017-00000000-0000-0000-0000-000000000002", - "RHSA-2018-102", events.GitModuleMetadataChangeEvent) + "102", events.GitModuleMetadataChangeEvent) event2.state = EventState.SKIPPED.value event3 = models.Event.create( db.session, "2017-00000000-0000-0000-0000-000000000003", - "RHSA-2018-103", events.MBSModuleStateChangeEvent) + "103", events.MBSModuleStateChangeEvent) event3.state = EventState.FAILED.value db.session.commit() db.session.expire_all() @@ -704,7 +704,7 @@ class TestManualTriggerRebuild(ViewBaseTest): from_advisory_id, publish): models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", - "RHSA-2018-103", events.TestingEvent) + "103", events.TestingEvent) db.session.commit() time.return_value = 123 from_advisory_id.return_value = ErrataAdvisory( @@ -722,7 +722,7 @@ class TestManualTriggerRebuild(ViewBaseTest): # Other fields are predictible. self.assertEqual(data['requested_rebuilds'], ["foo-1-1"]) assert add_dependency.call_count == 1 - assert "RHSA-2018-103" == add_dependency.call_args[0][1].search_key + assert "103" == add_dependency.call_args[0][1].search_key publish.assert_called_once_with( 'manual.rebuild', {'msg_id': 'manual_rebuild_123', u'errata_id': 1, @@ -734,7 +734,7 @@ class TestPatchAPI(ViewBaseTest): event = models.Event.create( db.session, '2017-00000000-0000-0000-0000-000000000003', - 'RHSA-2018-103', + '103', events.TestingEvent, # Tests that admins can cancel any event, regardless of the requester requester='tom_hanks', @@ -765,7 +765,7 @@ class TestPatchAPI(ViewBaseTest): event = models.Event.create( db.session, '2017-00000000-0000-0000-0000-000000000003', - 'RHSA-2018-103', + '123', events.TestingEvent, requester='tom_hanks', ) @@ -779,7 +779,7 @@ class TestPatchAPI(ViewBaseTest): event = models.Event.create( db.session, '2017-00000000-0000-0000-0000-000000000003', - 'RHSA-2018-103', + '103', events.TestingEvent, requester='han_solo', ) From 3f7638772ae7d5847d7e86365f6b9e976cc38a36 Mon Sep 17 00:00:00 2001 From: mprahl Date: Dec 01 2019 16:47:54 +0000 Subject: [PATCH 2/3] Allow inheriting the advisory ID from the dependent Freshmaker event When submitting a manual rebuild, you can specify a dependent Freshmaker event. If this is set, the `errata_id` value must match the advisory ID associated with the dependent Freshmaker event. If it is not set, the advisory ID is inherited. As part of this, some additional input sanitization was added. More input sanitization will come in a future commit. --- diff --git a/freshmaker/views.py b/freshmaker/views.py index 56a8dde..f0eaf0a 100644 --- a/freshmaker/views.py +++ b/freshmaker/views.py @@ -392,23 +392,51 @@ class BuildAPI(MethodView): :jsonparam string errata_id: The ID of Errata advisory to rebuild - artifacts for. + artifacts for. If this is not set, freshmaker_event_id must be set. :jsonparam list container_images: When set, defines list of NVRs of leaf container images which should be rebuild in the newly created Event. :jsonparam bool dry_run: When True, the Event will be handled in the DRY_RUN mode. - :jsonparam bool freshmaker_event_id: When set, defines the event - which will be used as dependant event. Successfull builds from - this Event will be reused in the newly created Event instead of - building all the artifacts from scratch. - :statuscode 200: New even generated. - :statuscode 400: Errata with this ID is not found. + :jsonparam bool freshmaker_event_id: When set, it defines the event + which will be used as the dependant event. Successfull builds from + this event will be reused in the newly created event instead of + building all the artifacts from scratch. If errata_id is not + provided, it will be inherited from this Freshmaker event. + :statuscode 200: A new event was created. + :statuscode 400: The provided input is invalid. """ data = request.get_json(force=True) - if 'errata_id' not in data: + for key in ('errata_id', 'freshmaker_event_id'): + if data.get(key) and not isinstance(data[key], int): + return json_error(400, 'Bad Request', f'"{key}" must be an integer.') + + if not data.get('errata_id') and not data.get('freshmaker_event_id'): return json_error( - 400, 'Bad Request', 'Missing errata_id in request') + 400, + 'Bad Request', + 'You must at least provide "errata_id" or "freshmaker_event_id" in the request.', + ) + + dependent_event = None + if data.get('freshmaker_event_id'): + dependent_event = models.Event.get_by_event_id( + db.session, data.get('freshmaker_event_id'), + ) + if not dependent_event: + return json_error( + 400, 'Bad Request', 'The provided "freshmaker_event_id" is invalid.', + ) + + if not data.get('errata_id'): + data['errata_id'] = int(dependent_event.search_key) + elif int(dependent_event.search_key) != data['errata_id']: + return json_error( + 400, + 'Bad Request', + 'The provided "errata_id" doesn\'t match the Advisory ID associated with the ' + 'input "freshmaker_event_id".', + ) # Use the shared code to parse the POST data and generate right # event based on the data. Currently it generates just @@ -416,10 +444,6 @@ class BuildAPI(MethodView): parser = FreshmakerManualRebuildParser() event = parser.parse_post_data(data) - dependent_event = None - if event.freshmaker_event_id: - dependent_event = models.Event.get_by_event_id(db.session, event.freshmaker_event_id) - # Store the event into database, so it gets the ID which we can return # to client sending this POST request. The client can then use the ID # to check for the event status. diff --git a/tests/test_views.py b/tests/test_views.py index 985d9a4..2fb8c56 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -708,10 +708,10 @@ class TestManualTriggerRebuild(ViewBaseTest): db.session.commit() time.return_value = 123 from_advisory_id.return_value = ErrataAdvisory( - 123, 'name', 'REL_PREP', ['rpm']) + 103, 'name', 'REL_PREP', ['rpm']) payload = { - 'errata_id': 1, + 'errata_id': 103, 'container_images': ['foo-1-1'], 'freshmaker_event_id': 1, } @@ -725,9 +725,92 @@ class TestManualTriggerRebuild(ViewBaseTest): assert "103" == add_dependency.call_args[0][1].search_key publish.assert_called_once_with( 'manual.rebuild', - {'msg_id': 'manual_rebuild_123', u'errata_id': 1, + {'msg_id': 'manual_rebuild_123', u'errata_id': 103, 'container_images': ["foo-1-1"], 'freshmaker_event_id': 1}) + @patch('freshmaker.messaging.publish') + @patch('freshmaker.parsers.internal.manual_rebuild.ErrataAdvisory.' + 'from_advisory_id') + @patch('freshmaker.parsers.internal.manual_rebuild.time.time') + @patch('freshmaker.models.Event.add_event_dependency') + def test_dependent_manual_rebuild_on_existing_event_no_errata_id( + self, add_dependency, time, from_advisory_id, publish, + ): + models.Event.create( + db.session, '2017-00000000-0000-0000-0000-000000000003', '1', events.TestingEvent, + ) + db.session.commit() + from_advisory_id.return_value = ErrataAdvisory(1, 'name', 'REL_PREP', ['rpm']) + + payload = { + 'container_images': ['foo-1-1'], + 'freshmaker_event_id': 1, + } + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.json['search_key'], '1') + + def test_dependent_manual_rebuild_on_existing_event_errata_id_mismatch(self): + models.Event.create( + db.session, '2017-00000000-0000-0000-0000-000000000003', '1', events.TestingEvent, + ) + db.session.commit() + + payload = { + 'container_images': ['foo-1-1'], + 'errata_id': 2, + 'freshmaker_event_id': 1, + } + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual( + resp.json['message'], + 'The provided "errata_id" doesn\'t match the Advisory ID associated with the input ' + '"freshmaker_event_id".', + ) + + def test_dependent_manual_rebuild_on_existing_event_invalid_dependent(self): + payload = { + 'container_images': ['foo-1-1'], + 'freshmaker_event_id': 1, + } + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], 'The provided "freshmaker_event_id" is invalid.') + + def test_manual_rebuild_missing_errata_id(self): + payload = {'container_images': ['foo-1-1']} + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual( + resp.json['message'], + 'You must at least provide "errata_id" or "freshmaker_event_id" in the request.', + ) + + def test_manual_rebuild_invalid_type_errata_id(self): + payload = {'errata_id': '123'} + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], '"errata_id" must be an integer.') + + def test_manual_rebuild_invalid_type_freshmaker_event_id(self): + payload = {'freshmaker_event_id': '123'} + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], '"freshmaker_event_id" must be an integer.') + class TestPatchAPI(ViewBaseTest): def test_patch_event_cancel(self): From 5aff07e34704a59855ca16e78b57c8fd4ca95f22 Mon Sep 17 00:00:00 2001 From: mprahl Date: Dec 01 2019 16:47:54 +0000 Subject: [PATCH 3/3] Sanitize the input on the POST API endpoint --- diff --git a/freshmaker/views.py b/freshmaker/views.py index f0eaf0a..3cb4718 100644 --- a/freshmaker/views.py +++ b/freshmaker/views.py @@ -411,6 +411,18 @@ class BuildAPI(MethodView): if data.get(key) and not isinstance(data[key], int): return json_error(400, 'Bad Request', f'"{key}" must be an integer.') + container_images = data.get('container_images', []) + if ( + not isinstance(container_images, list) or + any(not isinstance(image, str) for image in container_images) + ): + return json_error( + 400, 'Bad Request', '"container_images" must be an array of strings.', + ) + + if not isinstance(data.get('dry_run', False), bool): + return json_error(400, 'Bad Request', '"dry_run" must be a boolean.') + if not data.get('errata_id') and not data.get('freshmaker_event_id'): return json_error( 400, diff --git a/tests/test_views.py b/tests/test_views.py index 2fb8c56..05f5f19 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -811,6 +811,22 @@ class TestManualTriggerRebuild(ViewBaseTest): self.assertEqual(resp.status_code, 400) self.assertEqual(resp.json['message'], '"freshmaker_event_id" must be an integer.') + def test_manual_rebuild_invalid_type_container_images(self): + payload = {'container_images': '123'} + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], '"container_images" must be an array of strings.') + + def test_manual_rebuild_invalid_type_dry_run(self): + payload = {'dry_run': '123'} + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], '"dry_run" must be a boolean.') + class TestPatchAPI(ViewBaseTest): def test_patch_event_cancel(self):