Remove assumptions about release blobs from rule evaluation (mozilla-…

…releng#3065)

`evaluateRules` can't reliably call `blob.getApplicationVersion`, because that
method is specific to certain types of blobs.  Instead, we change the
`blob.shouldServeUpdate` method to return a tristate instead of a boolean: `No`
means no update should be served.  `Maybe` means the update can be served but
the pin version should be checked.  `Yes` means the update should be served.

In the `Maybe` case, `evaluateRules` can look for a release corresponding to
the pin version and return that instead if found.

Closes mozilla-releng#2985

src/auslib/AUS.py

@@ -3,11 +3,10 @@
 from random import randint
 from urllib.parse import urlparse
 
-from auslib.blobs.base import createBlob
-from auslib.errors import BadDataError
+from auslib.blobs.base import ServeUpdate, createBlob
 from auslib.global_state import cache, dbo
 from auslib.services import releases
-from auslib.util.versions import MozillaVersion, PinVersion
+from auslib.util.versions import PinVersion
 
 
 class ForceResult(object):
@@ -132,49 +131,41 @@ def get_blob(mapping):
             return blob
 
         blob = get_blob(mapping)
-        if not blob or not blob.shouldServeUpdate(updateQuery):
+        if not blob:
+            return None, None, eval_metadata
+        candidate = blob.shouldServeUpdate(updateQuery)
+        if not candidate:
             return None, None, eval_metadata
 
-        version_pin = updateQuery.get("pin")
-        if version_pin is not None:
-            try:
-                version_pin = PinVersion(version_pin)
-            except ValueError:
-                raise BadDataError(f"Version Pin String '{version_pin}' is invalid.")
-            blob_version = blob.getApplicationVersion(updateQuery["buildTarget"], updateQuery["locale"])
-            if blob_version:
-                blob_version = MozillaVersion(blob_version)
-                # We should only pay attention to the pin version if we would otherwise return
-                # something newer. If we unconditionally returned the pinned version, we would do
-                # incorrect things like skipping watersheds.
-                if blob_version > version_pin:
-                    pin_mapping = dbo.pinnable_releases.getPinMapping(updateQuery["product"], getFallbackChannel(updateQuery["channel"]), str(version_pin))
-                    # Note that we fall back to serving the original update if the pin is not found
-                    # in the pin table, even if the version that we will serve is past the pinned
-                    # version. This is because, if there is something wrong with the update pin,
-                    # we would rather default to keeping the user up-to-date.
-                    # The alternative would mean that setting invalid pin would be a subtle problem
-                    # that would potentially be difficult to recognize. For example:
-                    #  - A sysadmin installs Firefox ESR 120.0 and sets the pin to '120.15.'.
-                    #  - Every time that a new minor version of 120 is available, the sysadmin
-                    #    updates to it promptly.
-                    #  - Eventually, the sysadmin updates to 120.14, but 120.15 is never released.
-                    #    The next version is ESR 134.0.
-                    #  - The relevant update rule now points to ESR 134, but it isn't returned as
-                    #    an available update so, as far as the sysadmin knows, the pin is working
-                    #    as expected.
-                    #  - A loaner laptop is given out that still has ESR 120.0 installed.
-                    #  - When the loaner laptop asks for updates, Balrog sees that the newest
-                    #    version is ESR 134.0, but that would be newer than the '120.15' pin,
-                    #    so no update is returned.
-                    #  - The loaner laptop never updates and stays at ESR 120.0.
-                    # This situation hides the problem from the sysadmin and leaves some
-                    # installations vulnerable.
-                    if pin_mapping is not None:
-                        mapping = pin_mapping
-                        blob = get_blob(mapping)
-                        if not blob or not blob.shouldServeUpdate(updateQuery):
-                            return None, None, eval_metadata
+        if candidate == ServeUpdate.Maybe:
+            version_pin = PinVersion(updateQuery.get("pin"))
+            pin_mapping = dbo.pinnable_releases.getPinMapping(updateQuery["product"], getFallbackChannel(updateQuery["channel"]), str(version_pin))
+            # Note that we fall back to serving the original update if the pin is not found
+            # in the pin table, even if the version that we will serve is past the pinned
+            # version. This is because, if there is something wrong with the update pin,
+            # we would rather default to keeping the user up-to-date.
+            # The alternative would mean that setting invalid pin would be a subtle problem
+            # that would potentially be difficult to recognize. For example:
+            #  - A sysadmin installs Firefox ESR 120.0 and sets the pin to '120.15.'.
+            #  - Every time that a new minor version of 120 is available, the sysadmin
+            #    updates to it promptly.
+            #  - Eventually, the sysadmin updates to 120.14, but 120.15 is never released.
+            #    The next version is ESR 134.0.
+            #  - The relevant update rule now points to ESR 134, but it isn't returned as
+            #    an available update so, as far as the sysadmin knows, the pin is working
+            #    as expected.
+            #  - A loaner laptop is given out that still has ESR 120.0 installed.
+            #  - When the loaner laptop asks for updates, Balrog sees that the newest
+            #    version is ESR 134.0, but that would be newer than the '120.15' pin,
+            #    so no update is returned.
+            #  - The loaner laptop never updates and stays at ESR 120.0.
+            # This situation hides the problem from the sysadmin and leaves some
+            # installations vulnerable.
+            if pin_mapping is not None:
+                mapping = pin_mapping
+                blob = get_blob(mapping)
+                if not blob or not blob.shouldServeUpdate(updateQuery):
+                    return None, None, eval_metadata
 
         self.log.debug("Returning release %s", mapping)
         return blob, rule["update_type"], eval_metadata

src/auslib/blobs/apprelease.py

@@ -1,13 +1,13 @@
 import itertools
 
 from auslib.AUS import getFallbackChannel, isForbiddenUrl, isSpecialURL
-from auslib.blobs.base import XMLBlob, createBlob
+from auslib.blobs.base import ServeUpdate, XMLBlob, createBlob
 from auslib.errors import BadDataError, BlobValidationError
 from auslib.global_state import dbo
 from auslib.services import releases
 from auslib.util.comparison import has_operator, strip_operator
 from auslib.util.rulematching import matchBuildID, matchChannel, matchVersion
-from auslib.util.versions import MozillaVersion, decrement_version, increment_version
+from auslib.util.versions import MozillaVersion, PinVersion, decrement_version, increment_version
 
 
 class ReleaseBlobBase(XMLBlob):
@@ -218,20 +218,29 @@ def shouldServeUpdate(self, updateQuery):
         releaseVersion = self.getApplicationVersion(buildTarget, locale)
         if not releaseVersion:
             self.log.debug("Matching rule has no application version, will not serve update.")
-            return False
+            return ServeUpdate.No
         releaseVersion = MozillaVersion(releaseVersion)
         queryVersion = MozillaVersion(updateQuery["version"])
         if queryVersion > releaseVersion:
             self.log.debug("Matching rule has older version than request, will not serve update.")
-            return False
+            return ServeUpdate.No
         elif releaseVersion == queryVersion:
             if updateQuery["buildID"] >= self.getBuildID(updateQuery["buildTarget"], updateQuery["locale"]):
                 self.log.debug("Matching rule has older buildid than request, will not serve update.")
-                return False
+                return ServeUpdate.No
         if updateQuery["buildTarget"] not in self["platforms"].keys():
-            return False
+            return ServeUpdate.No
 
-        return True
+        version_pin = updateQuery.get("pin")
+        if version_pin is not None:
+            try:
+                version_pin = PinVersion(version_pin)
+            except ValueError:
+                raise BadDataError(f"Version Pin String '{version_pin}' is invalid.")
+            if releaseVersion > version_pin:
+                return ServeUpdate.Maybe
+
+        return ServeUpdate.Yes
 
     def containsForbiddenDomain(self, product, allowlistedDomains):
         """Returns True if the blob contains any file URLs that contain a
@@ -1201,7 +1210,7 @@ def __init__(self, **kwargs):
 
     def shouldServeUpdate(self, updateQuery):
         # desupport messages should always be returned
-        return True
+        return ServeUpdate.Yes
 
     def getInnerHeaderXML(self, updateQuery, update_type, allowlistedDomains, specialForceHosts):
         return ""

src/auslib/blobs/base.py

@@ -1,3 +1,4 @@
+import enum
 import json
 import logging
 from os import path
@@ -132,6 +133,15 @@ def merge_dicts(ancestor, left, right):
     return result
 
 
+class ServeUpdate(enum.Enum):
+    No = 0
+    Maybe = enum.auto()
+    Yes = enum.auto()
+
+    def __bool__(self):
+        return bool(self.value)
+
+
 class Blob(dict):
     jsonschema = None
 
@@ -180,9 +190,9 @@ def getJSON(self):
 
     def shouldServeUpdate(self, updateQuery):
         """Should be implemented by subclasses. In the event that it's not,
-        False is the safest thing to return (it will fail closed instead of
+        No is the safest thing to return (it will fail closed instead of
         failing open)."""
-        return False
+        return ServeUpdate.No
 
     def containsForbiddenDomain(self, product, allowlistedDomains):
         raise NotImplementedError()

src/auslib/blobs/gmp.py

@@ -1,5 +1,5 @@
 from auslib.AUS import isForbiddenUrl
-from auslib.blobs.base import XMLBlob
+from auslib.blobs.base import ServeUpdate, XMLBlob
 from auslib.errors import BadDataError
 from auslib.util.hashes import getHashLen
 
@@ -48,7 +48,7 @@ def getPlatformData(self, vendor, platform):
     def shouldServeUpdate(self, updateQuery):
         # GMP updates should always be returned. It is the responsibility
         # of the client to decide whether or not any action needs to be taken.
-        return True
+        return ServeUpdate.Yes
 
     def getInnerHeaderXML(self, updateQuery, update_type, allowlistedDomains, specialForceHosts):
         return "    <addons>"

src/auslib/blobs/guardian.py

@@ -1,5 +1,5 @@
 from auslib.AUS import isForbiddenUrl
-from auslib.blobs.base import GenericBlob
+from auslib.blobs.base import GenericBlob, ServeUpdate
 from auslib.util.versions import LooseVersion
 
 
@@ -12,11 +12,11 @@ def containsForbiddenDomain(self, product, allowlistedDomains):
 
     def shouldServeUpdate(self, updateQuery):
         if updateQuery["buildTarget"] not in self.get("platforms", {}):
-            return False
+            return ServeUpdate.No
         if LooseVersion(updateQuery["version"]) >= LooseVersion(self["version"]):
-            return False
+            return ServeUpdate.No
 
-        return True
+        return ServeUpdate.Yes
 
     def getResponse(self, updateQuery, allowlistedDomains):
         url = self.get("platforms", {}).get(updateQuery["buildTarget"], {}).get("fileUrl")

src/auslib/blobs/superblob.py

@@ -1,4 +1,4 @@
-from auslib.blobs.base import XMLBlob
+from auslib.blobs.base import ServeUpdate, XMLBlob
 
 
 class SuperBlob(XMLBlob):
@@ -23,7 +23,7 @@ def getResponseBlobs(self):
 
     def shouldServeUpdate(self, updateQuery):
         # Since a superblob update will always be returned.
-        return True
+        return ServeUpdate.Yes
 
     def containsForbiddenDomain(self, product, allowlistedDomains):
         # Since SuperBlobs don't have any URLs

src/auslib/blobs/systemaddons.py

@@ -1,5 +1,5 @@
 from auslib.AUS import isForbiddenUrl
-from auslib.blobs.base import XMLBlob
+from auslib.blobs.base import ServeUpdate, XMLBlob
 from auslib.errors import BadDataError
 
 
@@ -33,7 +33,7 @@ def shouldServeUpdate(self, updateQuery):
         # SystemAddon updates should always be returned. It is the responsibility
         # of the client to decide whether or not any action needs to be taken,
         # similar to GMP
-        return True
+        return ServeUpdate.Yes
 
     # If there are are no updates, we have a special response for SystemAddons
     # blobs. We return <updates></updates>, without the addons tags.

tests/blobs/test_guardian.py

@@ -1,5 +1,6 @@
 import pytest
 
+from auslib.blobs.base import ServeUpdate
 from auslib.blobs.guardian import GuardianBlob
 
 
@@ -37,7 +38,15 @@ def testContainsForbiddenDomain(guardianblob, allowlistedDomains, expected):
 
 
 @pytest.mark.parametrize(
-    "version,expected", [("0.5.0.0", True), ("0.8.0.0", True), ("0.99.99.99", True), ("1.0.0.0", False), ("1.0.5.0", False), ("2.0.0.0", False)]
+    "version,expected",
+    [
+        ("0.5.0.0", ServeUpdate.Yes),
+        ("0.8.0.0", ServeUpdate.Yes),
+        ("0.99.99.99", ServeUpdate.Yes),
+        ("1.0.0.0", ServeUpdate.No),
+        ("1.0.5.0", ServeUpdate.No),
+        ("2.0.0.0", ServeUpdate.No),
+    ],
 )
 def testShouldServeUpdateVariousVersions(guardianblob, version, expected):
     updateQuery = {"product": "Guardian", "version": version, "buildTarget": "WINNT_x86_64", "channel": "release"}

tests/web/test_client.py

@@ -2729,6 +2729,24 @@ def setUp(self):
 """
             ),
         )
+        dbo.releases.t.insert().execute(
+            name="desupport",
+            product="b",
+            data_version=1,
+            data=createBlob(
+                """
+{
+    "name": "desupport",
+    "schema_version": 50,
+    "detailsUrl": "http://example.com/desupport",
+    "displayVersion": "1"
+}
+"""
+            ),
+        )
+        dbo.rules.t.insert().execute(
+            priority=100, backgroundRate=100, mapping="desupport", update_type="minor", product="b", data_version=1, osVersion="obsolete"
+        )
 
     def testUnpinnedRecent(self):
         ret = self.client.get("/update/6/b/3.0/30000101000030/p/l/c/a/a/a/a/update.xml")
@@ -2859,3 +2877,16 @@ def testBrokenPin(self):
         self.assertEqual(ret.status_code, 400)
         error_message = ret.get_data(as_text=True)
         self.assertEqual(error_message, "Version Pin String '2' is invalid.")
+
+    def testPinWithDesupport(self):
+        ret = self.client.get("/update/6/b/3.0/30000101000020/p/l/c/obsolete/a/a/a/update.xml?pin=4.")
+        self.assertEqual(ret.status_code, 200)
+        self.assertUpdateTextEqual(
+            ret,
+            """<?xml version="1.0"?>
+<updates>
+
+    <update type="minor" unsupported="true" detailsURL="http://example.com/desupport" displayVersion="1">
+</update>
+</updates>""",
+        )