Better upload errors when using API tokens (pypi#11885)

tests/unit/macaroons/test_caveats.py

@@ -19,7 +19,9 @@
 import pytest
 
 from pymacaroons.exceptions import MacaroonInvalidSignatureException
+from pyramid.security import Allowed
 
+from warehouse.errors import WarehouseDenied
 from warehouse.macaroons.caveats import (
     Caveat,
     ExpiryCaveat,
@@ -48,6 +50,9 @@ class TestV1Caveat:
             ("invalid json", False),
             ('{"version": 2}', False),
             ('{"permissions": null, "version": 1}', False),
+            ('{"permissions": null, "version": 2}', False),
+            ('{"permissions": "user", "version": 2}', False),
+            ('{"permissions": "", "version": 2}', False),
         ],
     )
     def test_verify_invalid_predicates(self, predicate, result):
@@ -217,7 +222,7 @@ def test_creation(self):
 
     def test_verify_invalid_signature(self, monkeypatch):
         verify = pretend.call_recorder(
-            pretend.raiser(MacaroonInvalidSignatureException)
+            pretend.raiser(MacaroonInvalidSignatureException("Signatures do not match"))
         )
         macaroon = pretend.stub()
         context = pretend.stub()
@@ -227,35 +232,75 @@ def test_verify_invalid_signature(self, monkeypatch):
         verifier = Verifier(macaroon, context, principals, permission)
 
         monkeypatch.setattr(verifier.verifier, "verify", verify)
-        assert verifier.verify(key) is False
+        status = verifier.verify(key)
+        assert not status
+        assert status.msg == "Signatures do not match"
+        assert verify.calls == [pretend.call(macaroon, key)]
+
+    def test_verify_generic_exception(self, monkeypatch):
+        verify = pretend.call_recorder(pretend.raiser(ValueError))
+        macaroon = pretend.stub()
+        context = pretend.stub()
+        principals = pretend.stub()
+        permission = pretend.stub()
+        key = pretend.stub()
+        verifier = Verifier(macaroon, context, principals, permission)
+
+        monkeypatch.setattr(verifier.verifier, "verify", verify)
+        status = verifier.verify(key)
+        assert not status
+        assert status.msg == "malformed macaroon"
+        assert verify.calls == [pretend.call(macaroon, key)]
+
+    def test_verify_inner_verifier_returns_false(self, monkeypatch):
+        verify = pretend.call_recorder(lambda macaroon, key: False)
+        macaroon = pretend.stub()
+        context = pretend.stub()
+        principals = pretend.stub()
+        permission = pretend.stub()
+        key = pretend.stub()
+        verifier = Verifier(macaroon, context, principals, permission)
+
+        monkeypatch.setattr(verifier.verifier, "verify", verify)
+        status = verifier.verify(key)
+        assert not status
+        assert status.msg == "unknown error"
         assert verify.calls == [pretend.call(macaroon, key)]
 
     @pytest.mark.parametrize(
-        ["caveats", "valid"],
+        ["caveats", "expected_status"],
         [
             # Both V1 and expiry present and valid.
             (
                 [
                     {"permissions": "user", "version": 1},
                     {"exp": int(time.time()) + 3600, "nbf": int(time.time()) - 1},
                 ],
-                True,
+                Allowed("signature and caveats OK"),
             ),
             # V1 only present and valid.
-            ([{"permissions": "user", "version": 1}], True),
+            (
+                [{"permissions": "user", "version": 1}],
+                Allowed("signature and caveats OK"),
+            ),
             # V1 and expiry present but V1 invalid.
-            ([{"permissions": "bad", "version": 1}], False),
+            (
+                [{"permissions": "bad", "version": 1}],
+                WarehouseDenied(
+                    "invalid permissions format", reason="invalid_api_token"
+                ),
+            ),
             # V1 and expiry present but expiry invalid.
             (
                 [
                     {"permissions": "user", "version": 1},
                     {"exp": int(time.time()) + 1, "nbf": int(time.time()) + 3600},
                 ],
-                False,
+                WarehouseDenied("token is expired", reason="invalid_api_token"),
             ),
         ],
     )
-    def test_verify(self, monkeypatch, caveats, valid):
+    def test_verify(self, monkeypatch, caveats, expected_status):
         key = os.urandom(32)
         m = pymacaroons.Macaroon(
             location="fakelocation",
@@ -276,4 +321,6 @@ def test_verify(self, monkeypatch, caveats, valid):
         permission = pretend.stub()
 
         verifier = Verifier(deserialized_macaroon, context, principals, permission)
-        assert verifier.verify(key) is valid
+        status = verifier.verify(key)
+        assert bool(status) is bool(expected_status)
+        assert status.msg == expected_status.msg

tests/unit/macaroons/test_security_policy.py

@@ -235,7 +235,7 @@ def test_permits_invalid_macaroon(self, monkeypatch):
         result = policy.permits(pretend.stub(), pretend.stub(), pretend.stub())
 
         assert result == Denied("")
-        assert result.s == "Invalid API Token: InvalidMacaroonError('foo')"
+        assert result.s == "Invalid API Token: foo"
 
     def test_permits_valid_macaroon(self, monkeypatch):
         macaroon_service = pretend.stub(

tests/unit/macaroons/test_services.py

@@ -22,6 +22,7 @@
 
 from pymacaroons.exceptions import MacaroonDeserializationException
 
+from warehouse.errors import WarehouseDenied
 from warehouse.macaroons import services
 from warehouse.macaroons.models import Macaroon
 
@@ -137,7 +138,9 @@ def test_verify_unprefixed_macaroon(self, macaroon_service):
             version=pymacaroons.MACAROON_V2,
         ).serialize()
 
-        with pytest.raises(services.InvalidMacaroonError):
+        with pytest.raises(
+            services.InvalidMacaroonError, match="malformed or nonexistent macaroon"
+        ):
             macaroon_service.verify(
                 raw_macaroon, pretend.stub(), pretend.stub(), pretend.stub()
             )
@@ -151,7 +154,9 @@ def test_verify_no_macaroon(self, macaroon_service):
         ).serialize()
         raw_macaroon = f"pypi-{raw_macaroon}"
 
-        with pytest.raises(services.InvalidMacaroonError):
+        with pytest.raises(
+            services.InvalidMacaroonError, match="deleted or nonexistent macaroon"
+        ):
             macaroon_service.verify(
                 raw_macaroon, pretend.stub(), pretend.stub(), pretend.stub()
             )
@@ -162,15 +167,17 @@ def test_verify_invalid_macaroon(self, monkeypatch, user_service, macaroon_servi
             "fake location", user.id, "fake description", [{"permissions": "user"}]
         )
 
-        verifier_obj = pretend.stub(verify=pretend.call_recorder(lambda k: False))
+        verifier_obj = pretend.stub(
+            verify=pretend.call_recorder(lambda k: WarehouseDenied("foo"))
+        )
         verifier_cls = pretend.call_recorder(lambda *a: verifier_obj)
         monkeypatch.setattr(services, "Verifier", verifier_cls)
 
         context = pretend.stub()
         principals = pretend.stub()
         permissions = pretend.stub()
 
-        with pytest.raises(services.InvalidMacaroonError):
+        with pytest.raises(services.InvalidMacaroonError, match="foo"):
             macaroon_service.verify(raw_macaroon, context, principals, permissions)
         assert verifier_cls.calls == [
             pretend.call(mock.ANY, context, principals, permissions)
@@ -180,7 +187,9 @@ def test_deserialize_raw_macaroon_when_none(self, macaroon_service):
         raw_macaroon = pretend.stub()
         macaroon_service._extract_raw_macaroon = pretend.call_recorder(lambda a: None)
 
-        with pytest.raises(services.InvalidMacaroonError):
+        with pytest.raises(
+            services.InvalidMacaroonError, match="malformed or nonexistent macaroon"
+        ):
             macaroon_service._deserialize_raw_macaroon(raw_macaroon)
 
         assert macaroon_service._extract_raw_macaroon.calls == [

warehouse/macaroons/caveats.py

@@ -15,6 +15,9 @@
 
 import pymacaroons
 
+from pyramid.security import Allowed
+
+from warehouse.errors import WarehouseDenied
 from warehouse.packaging.models import Project
 
 
@@ -58,22 +61,23 @@ def verify_projects(self, projects) -> bool:
     def verify(self, predicate) -> bool:
         try:
             data = json.loads(predicate)
-        except ValueError:
-            self.failure_reason = "malformatted predicate"
+            version = data["version"]
+            permissions = data["permissions"]
+        except (KeyError, ValueError, TypeError):
             return False
 
-        if data.get("version") != 1:
-            self.failure_reason = "invalid version in predicate"
+        if version != 1:
             return False
 
-        permissions = data.get("permissions")
         if permissions is None:
-            self.failure_reason = "invalid permissions in predicate"
             return False
 
         if permissions == "user":
             # User-scoped tokens behave exactly like a user's normal credentials.
             return True
+        elif not isinstance(permissions, dict):
+            self.failure_reason = "invalid permissions format"
+            return False
 
         projects = permissions.get("projects")
         if projects is None:
@@ -90,7 +94,6 @@ def verify(self, predicate):
             expiry = data["exp"]
             not_before = data["nbf"]
         except (KeyError, ValueError, TypeError):
-            self.failure_reason = "malformatted predicate"
             return False
 
         if not expiry or not not_before:
@@ -111,7 +114,6 @@ def verify(self, predicate):
             data = json.loads(predicate)
             project_ids = data["project_ids"]
         except (KeyError, ValueError, TypeError):
-            self.failure_reason = "malformatted predicate"
             return False
 
         if not project_ids:
@@ -125,7 +127,6 @@ def verify(self, predicate):
             return False
 
         if str(self.verifier.context.id) not in project_ids:
-            self.failure_reason = "current project does not matched scoped project IDs"
             return False
 
         return True
@@ -145,9 +146,35 @@ def verify(self, key):
         self.verifier.satisfy_general(ProjectIDsCaveat(self))
 
         try:
-            return self.verifier.verify(self.macaroon, key)
-        except (
-            pymacaroons.exceptions.MacaroonInvalidSignatureException,
-            Exception,  # https://github.com/ecordell/pymacaroons/issues/50
-        ):
-            return False
+            result = self.verifier.verify(self.macaroon, key)
+        except pymacaroons.exceptions.MacaroonInvalidSignatureException as exc:
+            failure_reasons = []
+            for cb in self.verifier.callbacks:
+                failure_reason = getattr(cb, "failure_reason", None)
+                if failure_reason is not None:
+                    failure_reasons.append(failure_reason)
+
+            # If we have a more detailed set of failure reasons, use them.
+            # Otherwise, use whatever the exception gives us.
+            if len(failure_reasons) > 0:
+                return WarehouseDenied(
+                    ", ".join(failure_reasons), reason="invalid_api_token"
+                )
+            else:
+                return WarehouseDenied(str(exc), reason="invalid_api_token")
+        except Exception:
+            # The pymacaroons `verify` API with leak exceptions raised during caveat
+            # verification, which *normally* indicate a deserialization error
+            # (i.e., a malformed caveat body).
+            # When this happens, we don't want to display a random stringified
+            # Python exception to the user, so instead we emit a generic error.
+            # See https://github.com/ecordell/pymacaroons/issues/50
+            return WarehouseDenied("malformed macaroon", reason="invalid_api_token")
+
+        # NOTE: We should never hit this case, since pymacaroons *should* always either
+        # raise on failure *or* return true. But there's nothing stopping that from
+        # silently breaking in the future, so we check the result defensively here.
+        if not result:
+            return WarehouseDenied("unknown error", reason="invalid_api_token")
+        else:
+            return Allowed("signature and caveats OK")

warehouse/macaroons/security_policy.py

@@ -153,7 +153,7 @@ def permits(self, context, principals, permission):
                 macaroon_service.verify(macaroon, context, principals, permission)
             except InvalidMacaroonError as exc:
                 return WarehouseDenied(
-                    f"Invalid API Token: {exc!r}", reason="invalid_api_token"
+                    f"Invalid API Token: {exc}", reason="invalid_api_token"
                 )
 
             # If our Macaroon is verified, and for a valid permission then we'll pass

warehouse/macaroons/services.py

@@ -121,11 +121,12 @@ def verify(self, raw_macaroon, context, principals, permission):
             raise InvalidMacaroonError("deleted or nonexistent macaroon")
 
         verifier = Verifier(m, context, principals, permission)
-        if verifier.verify(dm.key):
+        verified = verifier.verify(dm.key)
+        if verified:
             dm.last_used = datetime.datetime.now()
             return True
 
-        raise InvalidMacaroonError("invalid macaroon")
+        raise InvalidMacaroonError(verified.msg)
 
     def create_macaroon(self, location, user_id, description, caveats):
         """