diff --git a/bodhi-server/tests/services/test_updates.py b/bodhi-server/tests/services/test_updates.py index 51b95d977d..40dfcf9ab5 100644 --- a/bodhi-server/tests/services/test_updates.py +++ b/bodhi-server/tests/services/test_updates.py @@ -306,10 +306,12 @@ def test_new_rpm_update_koji_error(self, *args): assert up['errors'][0]['description'] == "Koji error getting build: bodhi-2.0.0-2.fc17" @mock.patch('bodhi.server.services.updates.handle_side_and_related_tags_task') + @mock.patch('bodhi.server.services.updates.log.debug') @mock.patch.dict('bodhi.server.validators.config', {'acl_system': 'dummy'}) @unused_mock_patch(**mock_uuid4_version1) @pytest.mark.parametrize('rawhide_workflow', (True, False)) - def test_new_rpm_update_from_tag(self, handle_side_and_related_tags_task, rawhide_workflow): + def test_new_rpm_update_from_tag(self, log_debug, + handle_side_and_related_tags_task, rawhide_workflow): """Test creating an update using builds from a Koji tag.""" # We don't want the new update to obsolete the existing one. self.db.delete(Update.query.one()) @@ -332,6 +334,7 @@ def test_new_rpm_update_from_tag(self, handle_side_and_related_tags_task, rawhid r = self.app.post_json('/updates/', update) msg = mockpub.call_args.args[0] + log_debug.assert_any_call('guest owns f17-build-side-7777 side-tag') up = r.json_body assert up['title'] == 'gnome-backgrounds-3.0-1.fc17' assert up['builds'][0]['nvr'] == 'gnome-backgrounds-3.0-1.fc17' @@ -395,14 +398,17 @@ def test_new_rpm_update_from_tag(self, handle_side_and_related_tags_task, rawhid ) @mock.patch('bodhi.server.services.updates.handle_side_and_related_tags_task') + @mock.patch('bodhi.server.services.updates.log.warning') + @mock.patch('bodhi.server.services.updates.log.debug') @mock.patch.dict('bodhi.server.validators.config', {'acl_system': 'dummy'}) @unused_mock_patch(**mock_uuid4_version1) @pytest.mark.parametrize('rawhide_workflow', (True, False)) - def test_new_rpm_update_from_tag_wrong_owner(self, handle_side_and_related_tags_task, + def test_new_rpm_update_from_tag_wrong_owner(self, log_debug, log_warning, + handle_side_and_related_tags_task, rawhide_workflow): """ Test creating an update using builds from a Koji tag fails if user doesn't own - the side-tag. + the side-tag and doesn't have rights to all builds in side-tag. """ # We don't want the new update to obsolete the existing one. self.db.delete(Update.query.one()) @@ -429,10 +435,58 @@ def test_new_rpm_update_from_tag_wrong_owner(self, handle_side_and_related_tags_ with mock.patch('bodhi.server.buildsys.DevBuildsys.getTag', self.mock_getTag): with mock.patch('bodhi.server.models.Release.mandatory_days_in_testing', 0): r = app.post_json('/updates/', update, status=403) + log_warning.assert_called_once_with('mattia does not own f17-build-side-7777 side-tag') + log_debug.assert_any_call('Using builds validation method') assert r.json_body['errors'][0]['description'] == ( "mattia does not have commit access to gnome-backgrounds" ) + @mock.patch('bodhi.server.services.updates.handle_side_and_related_tags_task') + @mock.patch('bodhi.server.services.updates.log.warning') + @mock.patch('bodhi.server.services.updates.log.debug') + @mock.patch.dict('bodhi.server.validators.config', {'acl_system': 'dummy'}) + @unused_mock_patch(**mock_uuid4_version1) + @pytest.mark.parametrize('rawhide_workflow', (True, False)) + def test_new_rpm_update_from_tag_wrong_owner_with_rights(self, log_debug, log_warning, + handle_side_and_related_tags_task, + rawhide_workflow): + """ + Test creating an update using builds from a Koji tag succeed if user doesn't own + the side-tag BUT does have rights to all builds in side-tag. + """ + # We don't want the new update to obsolete the existing one. + self.db.delete(Update.query.one()) + + if rawhide_workflow: + # We need a release that isn't composed by bodhi + release = Release.query.one() + release.composed_by_bodhi = False + self.db.commit() + + update = self.get_update(builds=None, from_tag='f17-build-side-7777') + + user = User(name='ralph') + self.db.add(user) + self.db.commit() + group = self.db.query(Group).filter_by(name='packager').one() + user.groups.append(group) + + with mock.patch('bodhi.server.Session.remove'): + app = TestApp(main({}, testing='ralph', session=self.db, **self.app_settings)) + + update['csrf_token'] = app.get('/csrf').json_body['csrf_token'] + + with mock.patch('bodhi.server.buildsys.DevBuildsys.getTag', self.mock_getTag): + with mock.patch('bodhi.server.models.Release.mandatory_days_in_testing', 0): + # we don't use mock_sends here as we want to do some custom checking + with mock.patch('bodhi.server.models.notifications.publish') as mockpub: + r = app.post_json('/updates/', update) + + log_warning.assert_called_once_with('ralph does not own f17-build-side-7777 side-tag') + log_debug.assert_any_call('Using builds validation method') + up = r.json_body + assert up['title'] == 'gnome-backgrounds-3.0-1.fc17' + @mock.patch('bodhi.server.services.updates.handle_side_and_related_tags_task') @mock.patch.dict('bodhi.server.validators.config', {'acl_system': 'dummy'}) @unused_mock_patch(**mock_uuid4_version1) diff --git a/bodhi-server/tests/test_validators.py b/bodhi-server/tests/test_validators.py index f5543dd68a..638e8f540c 100644 --- a/bodhi-server/tests/test_validators.py +++ b/bodhi-server/tests/test_validators.py @@ -404,8 +404,9 @@ def test_validate_acls_sidetag(self, mock_gpcfp): return_value=False) @mock.patch('bodhi.server.models.Package.get_pkg_committers_from_pagure', return_value=(['guest'], [])) + @mock.patch('bodhi.server.validators.log.warning') @mock.patch.dict('bodhi.server.validators.config', {'acl_system': 'pagure'}) - def test_validate_acls_sidetag_wrong_owner(self, mock_gpcfp, mock_access): + def test_validate_acls_sidetag_wrong_owner(self, log_warning, mock_gpcfp, mock_access): """Test that a user can submit updates only for sidetags they owns.""" mock_request = self.get_mock_request(sidetag=True) mock_request.validated['sidetag_owner'] = 'mattia' @@ -417,13 +418,15 @@ def test_validate_acls_sidetag_wrong_owner(self, mock_gpcfp, mock_access): }] assert mock_request.errors == error mock_gpcfp.assert_not_called() + log_warning.assert_called_once_with('guest does not own f33-build-side-0000 side-tag') @mock.patch('bodhi.server.models.Package.hascommitaccess', return_value=False) @mock.patch('bodhi.server.models.Package.get_pkg_committers_from_pagure', return_value=(['guest'], [])) + @mock.patch('bodhi.server.validators.log.warning') @mock.patch.dict('bodhi.server.validators.config', {'acl_system': 'pagure'}) - def test_validate_acls_sidetag_owner_not_set(self, mock_gpcfp, mock_access): + def test_validate_acls_sidetag_owner_not_set(self, log_warning, mock_gpcfp, mock_access): """If side-tag update, sidetag_owner must be present in request.""" mock_request = self.get_mock_request(sidetag=True) mock_request.validated['sidetag_owner'] = None @@ -435,6 +438,9 @@ def test_validate_acls_sidetag_owner_not_set(self, mock_gpcfp, mock_access): }] assert mock_request.errors == error mock_gpcfp.assert_not_called() + log_warning.assert_called_once_with( + 'Update appear to be from side-tag, but we cannot determine the side-tag owner' + ) @mock.patch('bodhi.server.models.Package.get_pkg_committers_from_pagure', return_value=(['guest'], [])) @@ -451,8 +457,10 @@ def test_validate_acls_sidetag_update_can_view_edit_page(self, mock_gpcfp): return_value=False) @mock.patch('bodhi.server.models.Package.get_pkg_committers_from_pagure', return_value=(['guest'], [])) + @mock.patch('bodhi.server.validators.log.warning') @mock.patch.dict('bodhi.server.validators.config', {'acl_system': 'pagure'}) - def test_validate_acls_sidetag_update_cannot_view_edit_page(self, mock_gpcfp, mock_access): + def test_validate_acls_sidetag_update_cannot_view_edit_page(self, log_warning, mock_gpcfp, + mock_access): """Test that a user can display the edit form.""" user = self.db.query(models.User).filter_by(id=2).one() self.db.flush() @@ -467,6 +475,7 @@ def test_validate_acls_sidetag_update_cannot_view_edit_page(self, mock_gpcfp, mo }] assert mock_request.errors == error mock_gpcfp.assert_not_called() + log_warning.assert_called_once_with('guest does not own f33-build-side-0000 side-tag') class TestValidateBugFeedback(BasePyTestCase): diff --git a/news/PR5764.feature b/news/PR5764.feature new file mode 100644 index 0000000000..a26dd3832f --- /dev/null +++ b/news/PR5764.feature @@ -0,0 +1 @@ +A packager can now edit a side-tag update even if the side-tag is not owned by them, provided they have commit rights on all packages included in the side-tag