Enhance some tests for acl validator change and add release note

Signed-off-by: Mattia Verga <mattia.verga@tiscali.it>

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)

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

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