Improve OIDC error messages (pypi#14308)

* Add more detailed OIDC errors

* Update tests

tests/unit/oidc/models/test_core.py

@@ -11,7 +11,9 @@
 # limitations under the License.
 
 import pretend
+import pytest
 
+from warehouse.oidc import errors
 from warehouse.oidc.models import _core
 
 
@@ -40,12 +42,14 @@ def test_check_claim_invariant():
 
 
 class TestOIDCPublisher:
-    def test_lookup_by_claims_default_none(self):
-        assert (
-            _core.OIDCPublisher.lookup_by_claims(pretend.stub(), pretend.stub()) is None
-        )
+    def test_lookup_by_claims_raises(self):
+        with pytest.raises(errors.InvalidPublisherError) as e:
+            _core.OIDCPublisher.lookup_by_claims(pretend.stub(), pretend.stub())
+        assert str(e.value) == "All lookup strategies exhausted"
 
     def test_oidc_publisher_not_default_verifiable(self):
         publisher = _core.OIDCPublisher(projects=[])
 
-        assert not publisher.verify_claims(signed_claims={})
+        with pytest.raises(errors.InvalidPublisherError) as e:
+            publisher.verify_claims(signed_claims={})
+        assert str(e.value) == "No required verifiable claims"

tests/unit/oidc/models/test_github.py

@@ -14,6 +14,7 @@
 import pytest
 
 from tests.common.db.oidc import GitHubPublisherFactory, PendingGitHubPublisherFactory
+from warehouse.oidc import errors
 from warehouse.oidc.models import _core, github
 
 
@@ -124,7 +125,9 @@ def test_github_publisher_unaccounted_claims(self, monkeypatch):
         }
         signed_claims["fake-claim"] = "fake"
         signed_claims["another-fake-claim"] = "also-fake"
-        assert not publisher.verify_claims(signed_claims=signed_claims)
+        with pytest.raises(errors.InvalidPublisherError) as e:
+            publisher.verify_claims(signed_claims=signed_claims)
+        assert str(e.value) == "Check failed for required claim 'sub'"
         assert sentry_sdk.capture_message.calls == [
             pretend.call(
                 "JWT for GitHubPublisher has unaccounted claims: "
@@ -160,7 +163,9 @@ def test_github_publisher_missing_claims(self, monkeypatch):
         signed_claims.pop("sub")
         assert "sub" not in signed_claims
         assert publisher.__required_verifiable_claims__
-        assert not publisher.verify_claims(signed_claims=signed_claims)
+        with pytest.raises(errors.InvalidPublisherError) as e:
+            publisher.verify_claims(signed_claims=signed_claims)
+        assert str(e.value) == "Missing claim 'sub'"
         assert sentry_sdk.capture_message.calls == [
             pretend.call("JWT for GitHubPublisher is missing claim: sub")
         ]
@@ -185,7 +190,9 @@ def test_github_publisher_missing_optional_claims(self, monkeypatch):
         signed_claims["ref"] = "ref"
         signed_claims["job_workflow_ref"] = publisher.job_workflow_ref + "@ref"
         assert publisher.__required_verifiable_claims__
-        assert not publisher.verify_claims(signed_claims=signed_claims)
+        with pytest.raises(errors.InvalidPublisherError) as e:
+            publisher.verify_claims(signed_claims=signed_claims)
+        assert str(e.value) == "Check failed for optional claim 'environment'"
         assert sentry_sdk.capture_message.calls == []
 
     @pytest.mark.parametrize("environment", [None, "some-environment"])

tests/unit/oidc/models/test_google.py

@@ -15,6 +15,7 @@
 import pytest
 
 from tests.common.db.oidc import GooglePublisherFactory, PendingGooglePublisherFactory
+from warehouse.oidc import errors
 from warehouse.oidc.models import _core, google
 
 
@@ -84,7 +85,9 @@ def test_google_publisher_unaccounted_claims(self, monkeypatch):
         }
         signed_claims["fake-claim"] = "fake"
         signed_claims["another-fake-claim"] = "also-fake"
-        assert not publisher.verify_claims(signed_claims=signed_claims)
+        with pytest.raises(errors.InvalidPublisherError) as e:
+            publisher.verify_claims(signed_claims=signed_claims)
+        assert str(e.value) == "Check failed for required claim 'email'"
         assert sentry_sdk.capture_message.calls == [
             pretend.call(
                 "JWT for GooglePublisher has unaccounted claims: "
@@ -118,7 +121,9 @@ def test_google_publisher_missing_claims(self, monkeypatch):
         signed_claims.pop("email")
         assert "email" not in signed_claims
         assert publisher.__required_verifiable_claims__
-        assert not publisher.verify_claims(signed_claims=signed_claims)
+        with pytest.raises(errors.InvalidPublisherError) as e:
+            publisher.verify_claims(signed_claims=signed_claims)
+        assert str(e.value) == "Missing claim 'email'"
         assert sentry_sdk.capture_message.calls == [
             pretend.call("JWT for GooglePublisher is missing claim: email")
         ]
@@ -139,7 +144,13 @@ def test_google_publisher_email_verified(self, email_verified, valid):
             "email": "[email protected]",
             "email_verified": email_verified,
         }
-        assert publisher.verify_claims(signed_claims=signed_claims) is valid
+        if valid:
+            # Does not raise
+            publisher.verify_claims(signed_claims=signed_claims)
+        else:
+            with pytest.raises(errors.InvalidPublisherError) as e:
+                publisher.verify_claims(signed_claims=signed_claims)
+            assert str(e.value) == "Check failed for required claim 'email_verified'"
 
     @pytest.mark.parametrize(
         ("expected_sub", "actual_sub", "valid"),
@@ -164,7 +175,13 @@ def test_google_publisher_sub_is_optional(self, expected_sub, actual_sub, valid)
             "email": "[email protected]",
             "email_verified": True,
         }
-        assert publisher.verify_claims(signed_claims=signed_claims) is valid
+        if valid:
+            # Does not raise
+            publisher.verify_claims(signed_claims=signed_claims)
+        else:
+            with pytest.raises(errors.InvalidPublisherError) as e:
+                publisher.verify_claims(signed_claims=signed_claims)
+            assert str(e.value) == "Check failed for optional claim 'sub'"
 
 
 class TestPendingGooglePublisher:

tests/unit/oidc/test_services.py

@@ -21,7 +21,7 @@
 import warehouse.utils.exceptions
 
 from tests.common.db.oidc import GitHubPublisherFactory, PendingGitHubPublisherFactory
-from warehouse.oidc import interfaces, services
+from warehouse.oidc import errors, interfaces, services
 
 
 def test_oidc_publisher_service_factory():
@@ -230,13 +230,14 @@ def test_find_publisher_issuer_lookup_fails(self, monkeypatch):
             ),
         )
 
-        find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: None)
+        find_publisher_by_issuer = pretend.raiser(errors.InvalidPublisherError("foo"))
         monkeypatch.setattr(
             services, "find_publisher_by_issuer", find_publisher_by_issuer
         )
 
         claims = pretend.stub()
-        assert service.find_publisher(claims) is None
+        with pytest.raises(errors.InvalidPublisherError):
+            service.find_publisher(claims)
         assert service.metrics.increment.calls == [
             pretend.call(
                 "warehouse.oidc.find_publisher.attempt",
@@ -260,21 +261,26 @@ def test_find_publisher_verify_claims_fails(self, monkeypatch):
             ),
         )
 
-        publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: False))
+        publisher = pretend.stub(
+            verify_claims=pretend.call_recorder(
+                pretend.raiser(errors.InvalidPublisherError)
+            )
+        )
         find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher)
         monkeypatch.setattr(
             services, "find_publisher_by_issuer", find_publisher_by_issuer
         )
 
         claims = pretend.stub()
-        assert service.find_publisher(claims) is None
+        with pytest.raises(errors.InvalidPublisherError):
+            service.find_publisher(claims)
         assert service.metrics.increment.calls == [
             pretend.call(
                 "warehouse.oidc.find_publisher.attempt",
                 tags=["publisher:fakepublisher"],
             ),
             pretend.call(
-                "warehouse.oidc.find_publisher.invalid_claims",
+                "warehouse.oidc.find_publisher.publisher_not_found",
                 tags=["publisher:fakepublisher"],
             ),
         ]

tests/unit/oidc/test_utils.py

@@ -18,17 +18,15 @@
 from pyramid.authorization import Authenticated
 
 from tests.common.db.oidc import GitHubPublisherFactory, GooglePublisherFactory
-from warehouse.oidc import utils
+from warehouse.oidc import errors, utils
 from warehouse.utils.security_policy import principals_for
 
 
 def test_find_publisher_by_issuer_bad_issuer_url():
-    assert (
+    with pytest.raises(errors.InvalidPublisherError):
         utils.find_publisher_by_issuer(
             pretend.stub(), "https://fake-issuer.url", pretend.stub()
         )
-        is None
-    )
 
 
 @pytest.mark.parametrize(

tests/unit/oidc/test_views.py

@@ -23,7 +23,7 @@
 from warehouse.events.tags import EventTag
 from warehouse.macaroons import caveats
 from warehouse.macaroons.interfaces import IMacaroonService
-from warehouse.oidc import views
+from warehouse.oidc import errors, views
 from warehouse.oidc.interfaces import IOIDCPublisherService
 from warehouse.rate_limiting.interfaces import IRateLimiter
 
@@ -162,9 +162,12 @@ def test_mint_token_from_trusted_publisher_verify_jwt_signature_fails():
 
 def test_mint_token_from_trusted_publisher_lookup_fails():
     claims = pretend.stub()
+    message = "some message"
     oidc_service = pretend.stub(
         verify_jwt_signature=pretend.call_recorder(lambda token: claims),
-        find_publisher=pretend.call_recorder(lambda claims, **kw: None),
+        find_publisher=pretend.call_recorder(
+            pretend.raiser(errors.InvalidPublisherError(message))
+        ),
     )
     request = pretend.stub(
         response=pretend.stub(status=None),
@@ -180,13 +183,15 @@ def test_mint_token_from_trusted_publisher_lookup_fails():
         "errors": [
             {
                 "code": "invalid-publisher",
-                "description": "valid token, but no corresponding publisher",
+                "description": (
+                    f"valid token, but no corresponding publisher ({message})"
+                ),
             }
         ],
     }
 
     assert request.find_service.calls == [
-        pretend.call(IOIDCPublisherService, name="github")
+        pretend.call(IOIDCPublisherService, name="github"),
     ]
     assert oidc_service.verify_jwt_signature.calls == [pretend.call("faketoken")]
     assert oidc_service.find_publisher.calls == [
@@ -396,11 +401,15 @@ def test_mint_token_from_oidc_no_pending_publisher_ok(
     # NOTE: Can't set __str__ using pretend.stub()
     monkeypatch.setattr(publisher.__class__, "__str__", lambda s: "fakespecifier")
 
+    def _find_publisher(claims, pending=False):
+        if pending:
+            raise errors.InvalidPublisherError
+        else:
+            return publisher
+
     oidc_service = pretend.stub(
         verify_jwt_signature=pretend.call_recorder(lambda token: claims_in_token),
-        find_publisher=pretend.call_recorder(
-            lambda claims, pending=False: publisher if not pending else None
-        ),
+        find_publisher=pretend.call_recorder(_find_publisher),
     )
 
     db_macaroon = pretend.stub(description="fakemacaroon")

warehouse/oidc/errors.py

@@ -0,0 +1,15 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+class InvalidPublisherError(Exception):
+    pass

warehouse/oidc/models/_core.py

@@ -23,6 +23,7 @@
 
 from warehouse import db
 from warehouse.macaroons.models import Macaroon
+from warehouse.oidc.errors import InvalidPublisherError
 from warehouse.oidc.interfaces import SignedClaims
 from warehouse.packaging.models import Project
 
@@ -136,7 +137,7 @@ def lookup_by_claims(cls, session, signed_claims: SignedClaims):
 
             if publisher := query.with_session(session).one_or_none():
                 return publisher
-        return None
+        raise InvalidPublisherError("All lookup strategies exhausted")
 
     @classmethod
     def all_known_claims(cls) -> set[str]:
@@ -160,7 +161,7 @@ def verify_claims(self, signed_claims: SignedClaims):
         # Defensive programming: treat the absence of any claims to verify
         # as a failure rather than trivially valid.
         if not self.__required_verifiable_claims__:
-            return False
+            raise InvalidPublisherError("No required verifiable claims")
 
         # All claims should be accounted for.
         # The presence of an unaccounted claim is not an error, only a warning
@@ -189,10 +190,12 @@ def verify_claims(self, signed_claims: SignedClaims):
                         f"JWT for {self.__class__.__name__} is missing claim: "
                         f"{claim_name}"
                     )
-                return False
+                raise InvalidPublisherError(f"Missing claim {claim_name!r}")
 
             if not check(getattr(self, claim_name), signed_claim, signed_claims):
-                return False
+                raise InvalidPublisherError(
+                    f"Check failed for required claim {claim_name!r}"
+                )
 
         # Check optional verifiable claims
         for claim_name, check in self.__optional_verifiable_claims__.items():
@@ -203,7 +206,9 @@ def verify_claims(self, signed_claims: SignedClaims):
             signed_claim = signed_claims.get(claim_name)
 
             if not check(getattr(self, claim_name), signed_claim, signed_claims):
-                return False
+                raise InvalidPublisherError(
+                    f"Check failed for optional claim {claim_name!r}"
+                )
 
         return True
 

warehouse/oidc/services.py

@@ -21,6 +21,7 @@
 from zope.interface import implementer
 
 from warehouse.metrics.interfaces import IMetricsService
+from warehouse.oidc.errors import InvalidPublisherError
 from warehouse.oidc.interfaces import IOIDCPublisherService, SignedClaims
 from warehouse.oidc.models import OIDCPublisher, PendingOIDCPublisher
 from warehouse.oidc.utils import find_publisher_by_issuer
@@ -288,29 +289,22 @@ def find_publisher(
             tags=metrics_tags,
         )
 
-        publisher = find_publisher_by_issuer(
-            self.db, self.issuer_url, signed_claims, pending=pending
-        )
-        if publisher is None:
-            self.metrics.increment(
-                "warehouse.oidc.find_publisher.publisher_not_found",
-                tags=metrics_tags,
+        try:
+            publisher = find_publisher_by_issuer(
+                self.db, self.issuer_url, signed_claims, pending=pending
             )
-            return None
-
-        if not publisher.verify_claims(signed_claims):
+            publisher.verify_claims(signed_claims)
             self.metrics.increment(
-                "warehouse.oidc.find_publisher.invalid_claims",
+                "warehouse.oidc.find_publisher.ok",
                 tags=metrics_tags,
             )
-            return None
-        else:
+            return publisher
+        except InvalidPublisherError as e:
             self.metrics.increment(
-                "warehouse.oidc.find_publisher.ok",
+                "warehouse.oidc.find_publisher.publisher_not_found",
                 tags=metrics_tags,
             )
-
-        return publisher
+            raise e
 
     def reify_pending_publisher(self, pending_publisher, project):
         new_publisher = pending_publisher.reify(self.db)

warehouse/oidc/utils.py

@@ -16,6 +16,7 @@
 
 from pyramid.authorization import Authenticated
 
+from warehouse.oidc.errors import InvalidPublisherError
 from warehouse.oidc.interfaces import SignedClaims
 from warehouse.oidc.models import (
     GitHubPublisher,
@@ -52,7 +53,7 @@ def find_publisher_by_issuer(session, issuer_url, signed_claims, *, pending=Fals
     except KeyError:
         # This indicates a logic error, since we shouldn't have verified
         # claims for an issuer that we don't recognize and support.
-        return None
+        raise InvalidPublisherError(f"Issuer {issuer_url!r} is unsupported")
 
     return publisher_cls.lookup_by_claims(session, signed_claims)