diff --git a/tests/unit/oidc/models/test_core.py b/tests/unit/oidc/models/test_core.py index 3918cf35ca88..bc477722d5ea 100644 --- a/tests/unit/oidc/models/test_core.py +++ b/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" diff --git a/tests/unit/oidc/models/test_github.py b/tests/unit/oidc/models/test_github.py index c3e0a773a9b4..3283b9698a30 100644 --- a/tests/unit/oidc/models/test_github.py +++ b/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"]) diff --git a/tests/unit/oidc/models/test_google.py b/tests/unit/oidc/models/test_google.py index a8f452a7f6d0..0f708db1fd91 100644 --- a/tests/unit/oidc/models/test_google.py +++ b/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": "fake@example.com", "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": "fake@example.com", "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: diff --git a/tests/unit/oidc/test_services.py b/tests/unit/oidc/test_services.py index 1831099d9e04..091843902464 100644 --- a/tests/unit/oidc/test_services.py +++ b/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"], ), ] diff --git a/tests/unit/oidc/test_utils.py b/tests/unit/oidc/test_utils.py index 8c4adc984452..4698735f7a9b 100644 --- a/tests/unit/oidc/test_utils.py +++ b/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( diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 5efae12bad4f..d6433af7b37e 100644 --- a/tests/unit/oidc/test_views.py +++ b/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") diff --git a/warehouse/oidc/errors.py b/warehouse/oidc/errors.py new file mode 100644 index 000000000000..30ea940ba852 --- /dev/null +++ b/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 diff --git a/warehouse/oidc/models/_core.py b/warehouse/oidc/models/_core.py index 47fd8a061113..684f2dc2a6b4 100644 --- a/warehouse/oidc/models/_core.py +++ b/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 diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index b2aafd0e08f3..c5635dc7c176 100644 --- a/warehouse/oidc/services.py +++ b/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) diff --git a/warehouse/oidc/utils.py b/warehouse/oidc/utils.py index d7334dd64fe5..8cdef771214a 100644 --- a/warehouse/oidc/utils.py +++ b/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) diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index e72d42c06701..2a326671f1d7 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -24,6 +24,7 @@ from warehouse.events.tags import EventTag from warehouse.macaroons import caveats from warehouse.macaroons.interfaces import IMacaroonService +from warehouse.oidc.errors import InvalidPublisherError from warehouse.oidc.interfaces import IOIDCPublisherService from warehouse.oidc.models import PendingOIDCPublisher from warehouse.packaging.interfaces import IProjectService @@ -106,8 +107,8 @@ def _invalid(errors): ) # First, try to find a pending publisher. - pending_publisher = oidc_service.find_publisher(claims, pending=True) - if pending_publisher is not None: + try: + pending_publisher = oidc_service.find_publisher(claims, pending=True) factory = ProjectFactory(request) # If the project already exists, this pending publisher is no longer @@ -160,17 +161,22 @@ def _invalid(errors): project_name=stale_publisher.project_name, ) request.db.delete(stale_publisher) + except InvalidPublisherError: + # If the claim set isn't valid for a pending publisher, it's OK, we + # will try finding a regular publisher + pass # We either don't have a pending OIDC publisher, or we *did* # have one and we've just converted it. Either way, look for a full publisher # to actually do the macaroon minting with. - publisher = oidc_service.find_publisher(claims, pending=False) - if not publisher: + try: + publisher = oidc_service.find_publisher(claims, pending=False) + except InvalidPublisherError as e: return _invalid( errors=[ { "code": "invalid-publisher", - "description": "valid token, but no corresponding publisher", + "description": f"valid token, but no corresponding publisher ({e})", } ] )