Move Koji tag expansion to validate_from_tag().

This finds the latest builds in a tag in the from_tag validator and adds
them to request.validated. This is necessary because the list of builds
is further used in validate_acls() to decide if a user is allowed to
submit an update for the involved builds.

Signed-off-by: Nils Philippsen <nils@redhat.com>

bodhi/server/services/updates.py

@@ -26,7 +26,7 @@
 from sqlalchemy.sql import or_
 from requests import RequestException, Timeout as RequestsTimeout
 
-from bodhi.server import buildsys, log, security
+from bodhi.server import log, security
 from bodhi.server.exceptions import BodhiException, LockedUpdateException
 from bodhi.server.models import (
     Update,
@@ -434,7 +434,8 @@ def new_update(request):
 
     If the ``from_tag`` parameter is specified and ``builds`` is missing or
     empty, the list of builds will be filled with the latest builds in this
-    Koji tag.
+    Koji tag. This is done by validate_from_tag() because the list of builds
+    needs to be available in validate_acls().
 
     Args:
         request (pyramid.request): The current request.
@@ -446,17 +447,12 @@ def new_update(request):
     # it since the models don't care about a csrf argument.
     data.pop('csrf_token')
 
+    # Same here, but it can be missing.
+    data.pop('builds_from_tag', None)
+
     build_nvrs = data.get('builds', [])
     from_tag = data.get('from_tag')
 
-    if from_tag and not build_nvrs:
-        # If from_tag is set and build_nvrs (`builds` in the request) is empty,
-        # fill from latest builds in the submitted Koji tag. This lets users
-        # submit the builds when editing in order to skip this step.
-        koji = buildsys.get_session()
-
-        build_nvrs = [b['nvr'] for b in koji.listTagged(from_tag, latest=True)]
-
     caveats = []
     try:
 

bodhi/server/validators.py

@@ -29,6 +29,7 @@
 import rpm
 
 from bodhi.server.config import config
+from bodhi.server.exceptions import BodhiException
 from . import buildsys, log
 from .models import (
     Build,
@@ -200,7 +201,7 @@ def validate_build_nvrs(request, **kwargs):
         request (pyramid.request.Request): The current request.
         kwargs (dict): The kwargs of the related service definition. Unused.
     """
-    for build in request.validated.get('builds', []):
+    for build in request.validated.get('builds') or []:  # cope with builds being None
         try:
             cache_nvrs(request, build)
         except ValueError:
@@ -1264,7 +1265,10 @@ def validate_request(request, **kwargs):
 
 @postschema_validator
 def validate_from_tag(request: pyramid.request.Request, **kwargs: dict):
-    """Ensure that from_tag exists and is known and valid as a release tag.
+    """Check that an existing from_tag is valid and set `builds`.
+
+    Ensure that `from_tag` is a valid Koji tag and set `builds` to latest
+    builds from this tag if unset.
 
     Args:
         request: The current request.
@@ -1273,8 +1277,33 @@ def validate_from_tag(request: pyramid.request.Request, **kwargs: dict):
     koji_tag = request.validated.get('from_tag')
 
     if koji_tag:
-        koji = buildsys.get_session()
+        koji_client = buildsys.get_session()
+
+        if request.validated.get('builds'):
+            # Builds were specified explicitly, verify that the tag exists in Koji.
+            if not koji_client.getTag(koji_tag):
+                request.errors.add('body', 'from_tag', "The supplied from_tag doesn't exist.")
+                return
 
-        # Verify that the tag exists in Koji.
-        if not koji.getTag(koji_tag):
-            request.errors.add('body', 'from_tag', "The supplied from_tag doesn't exist.")
+            # Flag that `builds` wasn't filled from the Koji tag.
+            request.validated['builds_from_tag'] = False
+        else:
+            # Builds weren't specified explicitly, pull the list of latest NVRs here, as it is
+            # necessary for later validation of ACLs pertaining the respective components.
+            try:
+                request.validated['builds'] = [
+                    b['nvr'] for b in koji_client.listTagged(koji_tag, latest=True)
+                ]
+            except koji.GenericError as e:
+                if "invalid taginfo" in str(e).lower():
+                    request.errors.add('body', 'from_tag', "The supplied from_tag doesn't exist.")
+                else:
+                    raise BodhiException("Encountered error while requesting tagged builds from "
+                                         f"Koji: '{e}'") from e
+            else:  # no Koji error, request.validated['builds'] was filled
+                if not request.validated['builds']:
+                    request.errors.add('body', 'from_tag',
+                                       "The supplied from_tag doesn't contain any builds.")
+                else:
+                    # Flag that `builds` was filled from the Koji tag.
+                    request.validated['builds_from_tag'] = True

bodhi/tests/server/test_validators.py

@@ -22,10 +22,13 @@
 
 from cornice.errors import Errors
 from fedora_messaging import api, testing as fml_testing
+import koji
 from pyramid import exceptions
+import pytest
 
 from bodhi.tests.server.base import BasePyTestCase, BaseTestCase
 from bodhi.server import buildsys, models, validators
+from bodhi.server.exceptions import BodhiException
 
 
 class TestValidateCSRFToken(BaseTestCase):
@@ -667,6 +670,30 @@ def test_empty_from_tag(self):
 class TestValidateFromTag(BasePyTestCase):
     """Test the validate_from_tag() function."""
 
+    class UnknownTagDevBuildsys(buildsys.DevBuildsys):
+
+        def listTagged(self, tag, *args, **kwargs):
+            raise koji.GenericError(f"Invalid tagInfo: {tag!r}")
+
+        def getTag(self, tag, **kwargs):
+            return None
+
+    class NoBuildsDevBuildsys(buildsys.DevBuildsys):
+
+        def listTagged(self, tag, *args, **kwargs):
+            return []
+
+    class UnknownKojiGenericError(buildsys.DevBuildsys):
+
+        def listTagged(self, tag, *args, **kwargs):
+            raise koji.GenericError("foo")
+
+    @classmethod
+    def mock_get_session_for_class(cls, buildsys_cls):
+        def mock_get_session():
+            return buildsys_cls()
+        return mock_get_session
+
     def setup_method(self, method):
         """Sets up the environment for each test method call."""
         super().setup_method(method)
@@ -676,14 +703,91 @@ def setup_method(self, method):
         self.request.errors = Errors()
         self.request.validated = {'from_tag': 'f17-updates-candidate'}
 
-    def test_unknown(self):
-        """An unknown from_tag should add an error to the request."""
-        # DevBuildSys acts as if it doesn't know tags starting with 'epel'.
-        self.request.validated['from_tag'] = 'epel7'
+    # Successful validations
+
+    def test_known_with_builds(self):
+        """Test with known from_tag and with builds set in request.validated.
+
+        This test expects that builds_from_tag is set to False after calling
+        the validator."""
+        self.request.validated['builds'] = ['foo-1-1']
 
         validators.validate_from_tag(self.request)
 
+        assert self.request.validated['builds_from_tag'] == False
+        assert not self.request.errors
+
+    def test_known_without_builds(self):
+        """Test with known from_tag but without builds in request.validated.
+
+        This test expects that builds_from_tag is set to True, and builds are
+        filled after calling the validator."""
+        validators.validate_from_tag(self.request)
+
+        assert self.request.validated['builds_from_tag'] == True
+        assert len(self.request.validated['builds'])
+        assert not self.request.errors
+
+    def test_without_from_tag(self):
+        """Test without from_tag supplied.
+
+        This makes the validator a no-op."""
+        del self.request.validated['from_tag']
+
+        validators.validate_from_tag(self.request)
+
+    # Error conditions
+
+    def test_known_with_empty_builds(self):
+        """Test with known from_tag but with empty builds in request.validated.
+
+        This test expects an appropriate error to be added."""
+        with mock.patch('bodhi.server.validators.buildsys.get_session',
+                        self.mock_get_session_for_class(self.NoBuildsDevBuildsys)):
+            validators.validate_from_tag(self.request)
+
+        assert self.request.errors == [
+            {'location': 'body', 'name': 'from_tag',
+             'description': "The supplied from_tag doesn't contain any builds."}
+        ]
+
+    def test_unknown_with_builds(self):
+        """An unknown from_tag should add an error to the request.
+
+        This test runs with a list of fills in request.validated."""
+        self.request.validated['builds'] = ['foo-1-1']
+
+        with mock.patch('bodhi.server.validators.buildsys.get_session',
+                        self.mock_get_session_for_class(self.UnknownTagDevBuildsys)):
+            validators.validate_from_tag(self.request)
+
         assert self.request.errors == [
             {'location': 'body', 'name': 'from_tag',
              'description': "The supplied from_tag doesn't exist."}
         ]
+
+    def test_unknown_without_builds(self):
+        """An unknown from_tag should add an error to the request.
+
+        This test runs without a list of fills in request.validated."""
+        with mock.patch('bodhi.server.validators.buildsys.get_session',
+                        self.mock_get_session_for_class(self.UnknownTagDevBuildsys)):
+            validators.validate_from_tag(self.request)
+
+        assert self.request.errors == [
+            {'location': 'body', 'name': 'from_tag',
+             'description': "The supplied from_tag doesn't exist."}
+        ]
+
+    def test_unknown_koji_genericerror(self):
+        """An unknown koji.GenericError should be wrapped."""
+        with pytest.raises(BodhiException) as excinfo:
+            with mock.patch('bodhi.server.validators.buildsys.get_session',
+                            self.mock_get_session_for_class(self.UnknownKojiGenericError)):
+                validators.validate_from_tag(self.request)
+
+        assert (str(excinfo.value)
+                == "Encountered error while requesting tagged builds from Koji: 'foo'")
+
+        # check type of wrapped exception
+        assert isinstance(excinfo.value.__cause__, koji.GenericError)