From e3e21e64aff4498a6d0ecdc9da6d48fca096f35e Mon Sep 17 00:00:00 2001 From: Carey Hoffman Date: Fri, 4 Aug 2023 14:19:21 -0700 Subject: [PATCH] Update ActiveState code to use actor --- tests/common/db/oidc.py | 23 +- tests/unit/oidc/models/test_activestate.py | 206 ++++++++---------- tests/unit/oidc/test_utils.py | 73 +++---- tests/unit/oidc/test_views.py | 12 +- ...fafda38_add_activestate_oidc_publisher.py} | 63 ++---- warehouse/oidc/models/_core.py | 1 - warehouse/oidc/models/activestate.py | 118 ++++++---- warehouse/oidc/views.py | 4 +- 8 files changed, 244 insertions(+), 256 deletions(-) rename warehouse/migrations/versions/{e8c7bb1b94c6_add_activestate_oidc_publisher.py => bb4dbfafda38_add_activestate_oidc_publisher.py} (63%) diff --git a/tests/common/db/oidc.py b/tests/common/db/oidc.py index 5692e27d9846..f3019fa44fb2 100644 --- a/tests/common/db/oidc.py +++ b/tests/common/db/oidc.py @@ -95,13 +95,11 @@ class Meta: model = ActiveStatePublisher id = factory.Faker("uuid4", cast_to=None) - sub = factory.Faker("pystr", max_chars=12) - organization_id = factory.Faker("uuid4") - organization_url_name = factory.Faker("pystr", max_chars=12) - project_id = factory.Faker("uuid4") - activestate_project_name = factory.Faker("pystr", max_chars=12) - user_id = factory.Faker("uuid4") - branch_id = factory.Faker("uuid4") + organization = factory.Faker("pystr", max_chars=12) + project = factory.Faker("pystr", max_chars=12) + actor = factory.Faker("pystr", max_chars=12) + actor_id = factory.Faker("uuid4") + ingredient = factory.Faker("pystr", max_chars=12) class PendingActiveStatePublisherFactory(WarehouseFactory): @@ -109,11 +107,10 @@ class Meta: model = PendingActiveStatePublisher id = factory.Faker("uuid4", cast_to=None) - sub = factory.Faker("pystr", max_chars=12) project_name = factory.Faker("pystr", max_chars=12) - organization_id = factory.Faker("uuid4") - organization_url_name = factory.Faker("pystr", max_chars=12) - project_id = factory.Faker("uuid4") - activestate_project_name = factory.Faker("pystr", max_chars=12) - user_id = factory.Faker("uuid4") + organization = factory.Faker("pystr", max_chars=12) + project = factory.Faker("pystr", max_chars=12) + actor = factory.Faker("pystr", max_chars=12) + actor_id = factory.Faker("uuid4") added_by = factory.SubFactory(UserFactory) + ingredient = factory.Faker("pystr", max_chars=12) diff --git a/tests/unit/oidc/models/test_activestate.py b/tests/unit/oidc/models/test_activestate.py index 3e07c351afea..f36b991719da 100644 --- a/tests/unit/oidc/models/test_activestate.py +++ b/tests/unit/oidc/models/test_activestate.py @@ -25,17 +25,18 @@ PendingActiveStatePublisher, ) -ORG_ID = "00000000-0000-1000-8000-000000000000" -PROJECT_ID = "00000000-0000-1000-8000-000000000001" -USER_ID = "00000000-0000-1000-8000-000000000002" -BRANCH_ID = "00000000-0000-1000-8000-000000000003" +ORG_URL_NAME = "fakeorg" +PROJECT_NAME = "fakeproject" +ACTOR_ID = "00000000-0000-1000-8000-000000000002" +ACTOR = "fakeuser" +INGREDIENT = "fakeingredientname" # This follows the format of the subject that ActiveState sends us. We don't # validate the format when verifying the JWT. That should happen when the # Publisher is configured. We just need to make sure that the subject matches # # Technically, the branch should only be present if a branch was provided in the JWT # claims -SUBJECT = "org:fake_org_id:project:fake_project_id:branch_id:fake_branch_id" +SUBJECT = f"org:{ORG_URL_NAME}:project:{PROJECT_NAME}" def test_lookup_strategies(): @@ -48,27 +49,30 @@ def test_lookup_strategies(): def new_signed_claims( sub: str = SUBJECT, - organization_id: str = ORG_ID, - org_url_name: str = "fakeorg", - project_id: str = PROJECT_ID, - project_name: str = "fakeproject", + actor: str = ACTOR, + actor_id: str = ACTOR_ID, + ingredient: str = INGREDIENT, + organization: str = ORG_URL_NAME, + org_id: str = "fakeorgid", + project: str = PROJECT_NAME, + project_id: str = "fakeprojectid", project_path: str = "fakeorg/fakeproject", - user_id: str = USER_ID, project_visibility: str = "public", branch_id: str | None = None, ) -> SignedClaims: - project_name = "fakeproject" - org_url_name = "fakeorg" claims = SignedClaims( { "sub": sub, - "organization_id": organization_id, - "organization_url_name": org_url_name, + "actor": actor, + "actor_id": actor_id, + "ingredient": ingredient, + "organization_id": org_id, + "organization": organization, + "project_visibility": project_visibility, "project_id": project_id, - "project_name": project_name, "project_path": project_path, - "user_id": user_id, - "project_visibility": project_visibility, + "project": project, + "builder": "pypi-publisher", } ) if branch_id: @@ -85,9 +89,7 @@ def test_publisher_name(self): def test_publisher_url(self): org_name = "fakeorg" project_name = "fakeproject" - publisher = ActiveStatePublisher( - organization_url_name=org_name, activestate_project_name=project_name - ) + publisher = ActiveStatePublisher(organization=org_name, project=project_name) assert ( publisher.publisher_url() @@ -97,9 +99,7 @@ def test_publisher_url(self): def test_stringifies_as_project_url(self): org_name = "fakeorg" project_name = "fakeproject" - publisher = ActiveStatePublisher( - organization_url_name=org_name, activestate_project_name=project_name - ) + publisher = ActiveStatePublisher(organization=org_name, project=project_name) assert ( str(publisher) @@ -109,9 +109,11 @@ def test_stringifies_as_project_url(self): def test_activestate_publisher_all_known_claims(self): assert ActiveStatePublisher.all_known_claims() == { # verifiable claims - "organization_id", - "project_id", - "user_id", + "organization", + "project", + "actor_id", + "actor", + "builder", "sub", # optional verifiable claims "branch_id", @@ -123,15 +125,14 @@ def test_activestate_publisher_all_known_claims(self): "aud", # unchecked claims "project_visibility", - "project_name", "project_path", - "organization_url_name", + "ingredient", + "organization_id", + "project_id", } def test_activestate_publisher_unaccounted_claims(self, monkeypatch): - publisher = ActiveStatePublisher( - sub=SUBJECT, - ) + publisher = ActiveStatePublisher() scope = pretend.stub() sentry_sdk = pretend.stub( @@ -161,24 +162,28 @@ def test_activestate_publisher_unaccounted_claims(self, monkeypatch): @pytest.mark.parametrize( ("claim_to_drop", "valid"), [ - ("organization_id", False), - ("project_id", False), - ("user_id", False), - ("branch_id", True), - ("organization_url_name", True), + ("organization", False), + ("project", False), + ("actor_id", False), + ("actor", False), + ("builder", False), + ("ingredient", True), + ("organization_id", True), + ("project_id", True), ("project_visibility", True), - ("project_name", True), ("project_path", True), + ("branch_id", True), ], ) def test_activestate_publisher_missing_claims( self, monkeypatch, claim_to_drop: str, valid: bool ): publisher = ActiveStatePublisher( - sub=SUBJECT, - organization_id=ORG_ID, - project_id=PROJECT_ID, - user_id=USER_ID, + organization=ORG_URL_NAME, + project=PROJECT_NAME, + actor_id=ACTOR_ID, + actor=ACTOR, + ingredient=INGREDIENT, ) scope = pretend.stub() @@ -211,88 +216,64 @@ def test_activestate_publisher_missing_claims( @pytest.mark.parametrize( ("expect", "actual", "valid"), [ - (ORG_ID, ORG_ID, True), - (ORG_ID, PROJECT_ID, False), + (ORG_URL_NAME, ORG_URL_NAME, True), + (ORG_URL_NAME, PROJECT_NAME, False), ], ) def test_activestate_publisher_org_id_verified( self, expect: str, actual: str, valid: bool ): publisher = ActiveStatePublisher( - sub=SUBJECT, - organization_id=actual, - project_id=PROJECT_ID, - user_id=USER_ID, + organization=actual, + project=PROJECT_NAME, + actor_id=ACTOR_ID, + actor=ACTOR, + ingredient=INGREDIENT, ) - signed_claims = new_signed_claims(organization_id=expect) + signed_claims = new_signed_claims(organization=expect) assert publisher.verify_claims(signed_claims=signed_claims) is valid @pytest.mark.parametrize( ("expect", "actual", "valid"), [ - (BRANCH_ID, BRANCH_ID, True), - (BRANCH_ID, PROJECT_ID, False), - # If it's configured in the publisher, it must be present in the claim - (BRANCH_ID, None, False), - # If it's not configured in the publisher, we don't care what it is - # in the claim - (None, None, True), - (None, PROJECT_ID, True), - ], - ) - def test_activestate_publisher_branch_id_verified( - self, expect: str, actual: str, valid: bool - ): - publisher = ActiveStatePublisher( - sub=SUBJECT, - organization_id=ORG_ID, - project_id=PROJECT_ID, - user_id=USER_ID, - branch_id=expect, - ) - - signed_claims = new_signed_claims(branch_id=actual) - assert publisher.verify_claims(signed_claims=signed_claims) is valid - - @pytest.mark.parametrize( - ("expect", "actual", "valid"), - [ - (PROJECT_ID, PROJECT_ID, True), - (PROJECT_ID, ORG_ID, False), + (PROJECT_NAME, PROJECT_NAME, True), + (PROJECT_NAME, ORG_URL_NAME, False), ], ) def test_activestate_publisher_project_id_verified( self, expect: str, actual: str, valid: bool ): publisher = ActiveStatePublisher( - sub=SUBJECT, - organization_id=ORG_ID, - project_id=actual, - user_id=USER_ID, + organization=ORG_URL_NAME, + project=actual, + actor_id=ACTOR_ID, + actor=ACTOR, + ingredient=INGREDIENT, ) - signed_claims = new_signed_claims(project_id=expect) + signed_claims = new_signed_claims(project=expect) assert publisher.verify_claims(signed_claims=signed_claims) is valid @pytest.mark.parametrize( ("expect", "actual", "valid"), [ - (USER_ID, USER_ID, True), - (USER_ID, ORG_ID, False), + (ACTOR_ID, ACTOR_ID, True), + (ACTOR_ID, ORG_URL_NAME, False), ], ) def test_activestate_publisher_user_id_verified( self, expect: str, actual: str, valid: bool ): publisher = ActiveStatePublisher( - sub=SUBJECT, - organization_id=ORG_ID, - project_id=PROJECT_ID, - user_id=actual, + organization=ORG_URL_NAME, + project=PROJECT_NAME, + actor_id=actual, + actor=ACTOR, + ingredient=INGREDIENT, ) - signed_claims = new_signed_claims(user_id=expect) + signed_claims = new_signed_claims(actor_id=expect) assert publisher.verify_claims(signed_claims=signed_claims) is valid @pytest.mark.parametrize( @@ -300,39 +281,34 @@ def test_activestate_publisher_user_id_verified( [ # Both present: must match. ( - f"org:{ORG_ID}:project:{PROJECT_ID}", - f"org:{ORG_ID}:project:{PROJECT_ID}", - True, - ), - # Both present, with branch id: must match. - ( - f"org:{ORG_ID}:project:{PROJECT_ID}:branch_id:{BRANCH_ID}", - f"org:{ORG_ID}:project:{PROJECT_ID}:branch_id:{BRANCH_ID}", + f"org:{ORG_URL_NAME}:project:{PROJECT_NAME}", + f"org:{ORG_URL_NAME}:project:{PROJECT_NAME}", True, ), - # sub configured without branch id, claim has branch id: must fail. + # Wrong value, project, must fail. ( - f"org:{ORG_ID}:project:{PROJECT_ID}", - f"org:{ORG_ID}:project:{PROJECT_ID}:branch_id:{BRANCH_ID}", + f"org:{ORG_URL_NAME}:project:{PROJECT_NAME}", + f"org:{ORG_URL_NAME}:project:{ORG_URL_NAME}", False, ), - # sub configured with branch id, claim missing branch id: must fail. + # Wrong value, org_id, must fail. ( - f"org:{ORG_ID}:project:{PROJECT_ID}:branch_id:{BRANCH_ID}", - f"org:{ORG_ID}:project:{PROJECT_ID}", + f"org:{ORG_URL_NAME}:project:{PROJECT_NAME}", + f"org:{PROJECT_NAME}:project:{PROJECT_NAME}", False, ), - # Wrong format for sub to expect from ActiveState: must fail. + # Just nonsenes, must fail. ( - f"org:{ORG_ID}:project:{PROJECT_ID}", - f"org:{ORG_ID}:project:{ORG_ID}", + f"org:{ORG_URL_NAME}:project:{PROJECT_NAME}", + "Nonsense", False, ), ], ) def test_activestate_publisher_sub(self, expected: str, actual: str, valid: bool): check = ActiveStatePublisher.__required_verifiable_claims__["sub"] - assert check(expected, actual, pretend.stub()) is valid + signed_claims = new_signed_claims(sub=actual) + assert check(expected, actual, signed_claims) is valid class TestPendingActiveStatePublisher: @@ -343,8 +319,10 @@ def test_reify_does_not_exist_yet(self, db_request): assert ( db_request.db.query(ActiveStatePublisher) .filter_by( - organization_id=pending_publisher.organization_id, - sub=pending_publisher.sub, + organization=pending_publisher.organization, + project=pending_publisher.project, + actor_id=pending_publisher.actor_id, + actor=pending_publisher.actor, ) .one_or_none() is None @@ -353,16 +331,16 @@ def test_reify_does_not_exist_yet(self, db_request): assert isinstance(publisher, ActiveStatePublisher) assert pending_publisher in db_request.db.deleted - assert publisher.organization_id == pending_publisher.organization_id + assert publisher.organization == pending_publisher.organization assert publisher.sub == pending_publisher.sub def test_reify_already_exists(self, db_request): existing_publisher: ActiveStatePublisher = ActiveStatePublisherFactory.create() pending_publisher = PendingActiveStatePublisherFactory.create( - organization_id=existing_publisher.organization_id, - project_id=existing_publisher.project_id, - branch_id=existing_publisher.branch_id, - sub=existing_publisher.sub, + organization=existing_publisher.organization, + project=existing_publisher.project, + actor_id=existing_publisher.actor_id, + actor=existing_publisher.actor, ) publisher = pending_publisher.reify(db_request.db) diff --git a/tests/unit/oidc/test_utils.py b/tests/unit/oidc/test_utils.py index 0429bd828906..074bf4da34e0 100644 --- a/tests/unit/oidc/test_utils.py +++ b/tests/unit/oidc/test_utils.py @@ -109,31 +109,31 @@ def test_find_publisher_by_issuer_google(db_request, sub, expected_id): @pytest.mark.parametrize( - "expected_id, sub, organization_id, project_id, user_id, branch_id", + "expected_id, sub, organization, project, actor_id, actor", [ ( uuid.UUID("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"), - "org:00000000-1000-8000-0000-000000000001:project:00000000-1000-8000-0000-000000000002", # noqa - "00000000-1000-8000-0000-000000000001", - "00000000-1000-8000-0000-000000000002", + "org:fakeorg1:project:fakeproject1", + "fakeorg1", + "fakeproject1", "00000000-1000-8000-0000-000000000003", - None, + "fakeuser1", ), ( uuid.UUID("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"), - "org:00000000-1000-8000-0000-000000000004:project:00000000-1000-8000-0000-000000000005", # noqa - "00000000-1000-8000-0000-000000000004", - "00000000-1000-8000-0000-000000000005", + "org:fakeorg2:project:fakeproject2", + "fakeorg2", + "fakeproject2", "00000000-1000-8000-0000-000000000006", - None, + "fakeuser2", ), ( uuid.UUID("cccccccc-cccc-cccc-cccc-cccccccccccc"), - "org:00000000-1000-8000-0000-000000000007:project:00000000-1000-8000-0000-000000000008:branch_id:00000000-1000-8000-0000-000000000010", # noqa - "00000000-1000-8000-0000-000000000007", - "00000000-1000-8000-0000-000000000008", + "org:fakeorg3:project:fakeproject3", + "fakeorg3", + "fakeproject3", "00000000-1000-8000-0000-000000000009", - "00000000-1000-8000-0000-000000000010", + "fakeuser3", ), ], ) @@ -141,48 +141,41 @@ def test_find_publisher_by_issuer_activestate( db_request, sub: str, expected_id: str, - organization_id: str, - project_id: str, - user_id: str, - branch_id: str | None, + organization: str, + project: str, + actor_id: str, + actor: str, ): ActiveStatePublisherFactory( id="aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", - organization_id="00000000-1000-8000-0000-000000000001", - project_id="00000000-1000-8000-0000-000000000002", - user_id="00000000-1000-8000-0000-000000000003", - sub="org:00000000-1000-8000-0000-000000000001:project:00000000-1000-8000-0000-000000000002", # noqa + organization="fakeorg1", + project="fakeproject1", + actor_id="00000000-1000-8000-0000-000000000003", + actor="fakeuser1", ) ActiveStatePublisherFactory( id="bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", - organization_id="00000000-1000-8000-0000-000000000004", - project_id="00000000-1000-8000-0000-000000000005", - user_id="00000000-1000-8000-0000-000000000006", - sub="org:00000000-1000-8000-0000-000000000004:project:00000000-1000-8000-0000-000000000005", # noqa + organization="fakeorg2", + project="fakeproject2", + actor_id="00000000-1000-8000-0000-000000000006", + actor="fakeuser2", ) ActiveStatePublisherFactory( id="cccccccc-cccc-cccc-cccc-cccccccccccc", - organization_id="00000000-1000-8000-0000-000000000007", - project_id="00000000-1000-8000-0000-000000000008", - user_id="00000000-1000-8000-0000-000000000009", - branch_id="00000000-1000-8000-0000-000000000010", - sub="org:00000000-1000-8000-0000-000000000007:project:00000000-1000-8000-0000-000000000008:branch_id:00000000-1000-8000-0000-000000000010", # noqa + organization="fakeorg3", + project="fakeproject3", + actor_id="00000000-1000-8000-0000-000000000009", + actor="fakeuser3", ) signed_claims = { "sub": sub, - "organization_id": organization_id, - "organization_url_name": "fakeOrg", - "project_id": project_id, - "project_name": "fakername1", - "project_path": "fakeOrg/fakername1", - "user_id": user_id, - "project_visibility": "public", + "organization": organization, + "project": project, + "actor_id": actor_id, + "actor": actor_id, } - if branch_id: - signed_claims["branch_id"] = branch_id - assert ( utils.find_publisher_by_issuer( db_request.db, diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index ae015f7bf202..789094da705b 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -18,7 +18,11 @@ import pytest from tests.common.db.accounts import UserFactory -from tests.common.db.oidc import OIDCPublisherFactory, PendingOIDCPublisherFactory +from tests.common.db.oidc import ( + OIDCPublisherFactory, + PendingGitHubPublisherFactory, + PendingOIDCPublisherFactory, +) from tests.common.db.packaging import ProjectFactory from warehouse.events.tags import EventTag from warehouse.macaroons import caveats @@ -290,7 +294,7 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( monkeypatch.setattr(views, "time", time) user = UserFactory.create() - pending_publisher = PendingPublisherFactory.create( + pending_publisher = PendingGitHubPublisherFactory.create( project_name="does-not-exist", added_by=user, ) @@ -301,7 +305,7 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( emailed_users = [] for project_name in ["does_not_exist", "does-not-exist", "dOeS-NoT-ExISt"]: user = UserFactory.create() - PendingPublisherFactory.create( + PendingGitHubPublisherFactory.create( project_name=project_name, added_by=user, ) @@ -345,6 +349,8 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( oidc_service = db_request.find_service(IOIDCPublisherService, name="github") + # This currently fails because it can't find the Publisher in the OIDC_SERVICE + # since there is no way to get a PendingPubliser by jwt fields. resp = views.mint_token(oidc_service, db_request) assert resp["success"] assert resp["token"].startswith("pypi-") diff --git a/warehouse/migrations/versions/e8c7bb1b94c6_add_activestate_oidc_publisher.py b/warehouse/migrations/versions/bb4dbfafda38_add_activestate_oidc_publisher.py similarity index 63% rename from warehouse/migrations/versions/e8c7bb1b94c6_add_activestate_oidc_publisher.py rename to warehouse/migrations/versions/bb4dbfafda38_add_activestate_oidc_publisher.py index 7c7abf48f2bf..20148459f61b 100644 --- a/warehouse/migrations/versions/e8c7bb1b94c6_add_activestate_oidc_publisher.py +++ b/warehouse/migrations/versions/bb4dbfafda38_add_activestate_oidc_publisher.py @@ -10,19 +10,20 @@ # See the License for the specific language governing permissions and # limitations under the License. """ -add ActiveState OIDC publisher +Add ActiveState OIDC publisher -Revision ID: e8c7bb1b94c6 -Revises: 4a0276f260c7 -Create Date: 2023-06-22 22:57:53.695035 +Revision ID: bb4dbfafda38 +Revises: a2af745511e0 +Create Date: 2023-08-05 00:33:10.339771 """ import sqlalchemy as sa from alembic import op +from sqlalchemy.dialects import postgresql -revision = "e8c7bb1b94c6" -down_revision = "4a0276f260c7" +revision = "bb4dbfafda38" +down_revision = "a2af745511e0" # Note: It is VERY important to ensure that a migration does not lock for a # long period of time and to ensure that each individual migration does @@ -51,53 +52,41 @@ def upgrade(): # ### commands auto generated by Alembic - please adjust! ### op.create_table( "activestate_oidc_publishers", - sa.Column("id", sa.UUID(), nullable=False), - sa.Column("sub", sa.VARCHAR(), nullable=False), - sa.Column("organization_id", sa.VARCHAR(), nullable=False), - sa.Column("organization_url_name", sa.VARCHAR(), nullable=False), - sa.Column("project_id", sa.VARCHAR(), nullable=False), - sa.Column("activestate_project_name", sa.VARCHAR(), nullable=False), - sa.Column("user_id", sa.VARCHAR(), nullable=False), - sa.Column("branch_id", sa.VARCHAR(), nullable=True), + sa.Column("id", postgresql.UUID(as_uuid=True), nullable=False), + sa.Column("organization", sa.VARCHAR(), nullable=False), + sa.Column("project", sa.VARCHAR(), nullable=False), + sa.Column("actor", sa.VARCHAR(), nullable=False), + sa.Column("actor_id", sa.VARCHAR(), nullable=False), + sa.Column("ingredient", sa.VARCHAR(), nullable=True), sa.ForeignKeyConstraint( ["id"], ["oidc_publishers.id"], ), sa.PrimaryKeyConstraint("id"), sa.UniqueConstraint( - "organization_id", - "organization_url_name", - "project_id", - "user_id", - "branch_id", - name="_activestate_oidc_publisher_uc", + "organization", "project", "actor_id", name="_activestate_oidc_publisher_uc" ), ) op.create_table( "pending_activestate_oidc_publishers", - sa.Column("id", sa.UUID(), nullable=False), - sa.Column("sub", sa.VARCHAR(), nullable=False), - sa.Column("organization_id", sa.VARCHAR(), nullable=False), - sa.Column("organization_url_name", sa.VARCHAR(), nullable=False), - sa.Column("project_id", sa.VARCHAR(), nullable=False), - sa.Column("activestate_project_name", sa.VARCHAR(), nullable=False), - sa.Column("user_id", sa.VARCHAR(), nullable=False), - sa.Column("branch_id", sa.VARCHAR(), nullable=True), + sa.Column("id", postgresql.UUID(as_uuid=True), nullable=False), + sa.Column("organization", sa.VARCHAR(), nullable=False), + sa.Column("project", sa.VARCHAR(), nullable=False), + sa.Column("actor", sa.VARCHAR(), nullable=False), + sa.Column("actor_id", sa.VARCHAR(), nullable=False), + sa.Column("ingredient", sa.VARCHAR(), nullable=True), sa.ForeignKeyConstraint( ["id"], ["pending_oidc_publishers.id"], ), sa.PrimaryKeyConstraint("id"), sa.UniqueConstraint( - "organization_id", - "organization_url_name", - "project_id", - "user_id", - "branch_id", + "organization", + "project", + "actor_id", name="_pending_activestate_oidc_publisher_uc", ), ) - op.drop_index("project_name_ultranormalized", table_name="projects") # ### end Alembic commands ### # Disable the ActiveState OIDC provider by default op.execute( @@ -115,12 +104,6 @@ def upgrade(): def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.create_index( - "project_name_ultranormalized", - "projects", - [sa.text("ultranormalize_name(name)")], - unique=False, - ) op.drop_table("pending_activestate_oidc_publishers") op.drop_table("activestate_oidc_publishers") # ### end Alembic commands ### diff --git a/warehouse/oidc/models/_core.py b/warehouse/oidc/models/_core.py index aeb9fe0cdac6..71264257699e 100644 --- a/warehouse/oidc/models/_core.py +++ b/warehouse/oidc/models/_core.py @@ -138,7 +138,6 @@ def lookup_by_claims(cls, session, signed_claims: SignedClaims): # We might not build a query if we know the claim set can't # satisfy it. If that's the case, then we skip. continue - if publisher := query.with_session(session).one_or_none(): return publisher raise InvalidPublisherError("All lookup strategies exhausted") diff --git a/warehouse/oidc/models/activestate.py b/warehouse/oidc/models/activestate.py index 521839a4e29d..4d880b6c3810 100644 --- a/warehouse/oidc/models/activestate.py +++ b/warehouse/oidc/models/activestate.py @@ -12,17 +12,18 @@ from typing import Any +import urllib.parse as urlparse from sqlalchemy import VARCHAR, Column, ForeignKey, UniqueConstraint from sqlalchemy.dialects.postgresql import UUID from sqlalchemy.orm import Query from warehouse.oidc.interfaces import SignedClaims +import warehouse.oidc.models._core as oidccore from warehouse.oidc.models._core import ( CheckClaimCallable, OIDCPublisher, PendingOIDCPublisher, - check_claim_binary, ) ACTIVESTATE_URL = "https://platform.activestate.com" @@ -31,12 +32,28 @@ # branch_id is optional, so we need to verify but it's ok if it's missing -def _check_branch_id_optional( +def _check_sub( ground_truth: str, signed_claim: str, _all_signed_claims: SignedClaims -): - if ground_truth: - return ground_truth == signed_claim - return True +) -> bool: + # We expect a string formatted as follows: + # repo:ORG/REPO[:OPTIONAL-STUFF] + # where :OPTIONAL-STUFF is a concatenation of other job context + # metadata. We currently lack the ground context to verify that + # additional metadata, so we limit our verification to just the ORG/REPO + # component. + + # Defensive: GitHub should never give us an empty subject. + if not signed_claim: + return False + + components = signed_claim.split(":") + if len(components) < 2: + return False + + return ( + f"{components[0]}:{components[1]}:{components[2]}:{components[3]}" + == ground_truth + ) class ActiveStatePublisherMixin: @@ -44,54 +61,72 @@ class ActiveStatePublisherMixin: Common functionality for both pending and concrete ActiveState OIDC publishers. """ - sub = Column(VARCHAR, nullable=False) - organization_id = Column(VARCHAR, nullable=False) - organization_url_name = Column(VARCHAR, nullable=False) - project_id = Column(VARCHAR, nullable=False) - activestate_project_name = Column(VARCHAR, nullable=False) - user_id = Column(VARCHAR, nullable=False) - branch_id = Column(VARCHAR) + organization = Column(VARCHAR, nullable=False) + project = Column(VARCHAR, nullable=False) + actor = Column(VARCHAR, nullable=False) + # actor_username is obstained from the user while configuring the publisher + # We'll make an api call to ActiveState to get the actor_id + actor_id = Column(VARCHAR, nullable=False) + ingredient = Column(VARCHAR, nullable=True) __required_verifiable_claims__: dict[str, CheckClaimCallable[Any]] = { - "sub": check_claim_binary(str.__eq__), - "organization_id": check_claim_binary(str.__eq__), - "project_id": check_claim_binary(str.__eq__), - "user_id": check_claim_binary(str.__eq__), + "sub": _check_sub, + "organization": oidccore.check_claim_binary(str.__eq__), + "project": oidccore.check_claim_binary(str.__eq__), + "actor_id": oidccore.check_claim_binary(str.__eq__), + "actor": oidccore.check_claim_binary(str.__eq__), + # This is the name of the builder in the ActiveState Platform that + # publishes things to PyPI. + "builder": oidccore.check_claim_invariant("pypi-publisher"), } __optional_verifiable_claims__: dict[str, CheckClaimCallable[Any]] = { - "branch_id": _check_branch_id_optional, + # Should we check that the pypi projectname matches this ingredient + # name? + "ingredient": oidccore.check_claim_binary(str.__eq__), } __unchecked_claims__ = { - "organization_url_name", + "organization_id", + "project_id", "project_visibility", - "project_name", "project_path", + "branch_id", } @staticmethod def __lookup_all__(klass, signed_claims: SignedClaims): return Query(klass).filter_by( - organization_id=signed_claims["organization_id"], - project_id=signed_claims["project_id"], - user_id=signed_claims["user_id"], - sub=signed_claims["sub"], + organization=signed_claims["organization"], + project=signed_claims["project"], + actor_id=signed_claims["actor_id"], ) __lookup_strategies__ = [ __lookup_all__, ] + @property + def sub(self) -> str: + return f"org:{self.organization}:project:{self.project}" + + @property + def builder(self) -> str: + return "pypi-publisher" + @property def publisher_name(self) -> str: return "ActiveState" + @property + def activestate_project_path(self) -> str: + return f"{self.organization}/{self.project}" + def publisher_url(self, claims: SignedClaims | None = None) -> str: - return f"{ACTIVESTATE_URL}/{self.organization_url_name}/{self.activestate_project_name}" # noqa + return urlparse.urljoin(ACTIVESTATE_URL, self.activestate_project_path) def __str__(self) -> str: - return f"{ACTIVESTATE_URL}/{self.organization_url_name}/{self.activestate_project_name}" # noqa + return self.publisher_url() class ActiveStatePublisher(ActiveStatePublisherMixin, OIDCPublisher): @@ -99,11 +134,9 @@ class ActiveStatePublisher(ActiveStatePublisherMixin, OIDCPublisher): __mapper_args__ = {"polymorphic_identity": "activestate_oidc_publishers"} __table_args__ = ( UniqueConstraint( - "organization_id", - "organization_url_name", - "project_id", - "user_id", - "branch_id", + "organization", + "project", + "actor_id", name="_activestate_oidc_publisher_uc", ), ) @@ -116,11 +149,9 @@ class PendingActiveStatePublisher(ActiveStatePublisherMixin, PendingOIDCPublishe __mapper_args__ = {"polymorphic_identity": "pending_activestate_oidc_publishers"} __table_args__ = ( UniqueConstraint( - "organization_id", - "organization_url_name", - "project_id", - "user_id", - "branch_id", + "organization", + "project", + "actor_id", name="_pending_activestate_oidc_publisher_uc", ), ) @@ -138,18 +169,19 @@ def reify(self, session): maybe_publisher = ( session.query(ActiveStatePublisher) .filter( - ActiveStatePublisher.organization_id == self.organization_id, - ActiveStatePublisher.project_id == self.project_id, - ActiveStatePublisher.branch_id == self.branch_id, + ActiveStatePublisher.organization == self.organization, + ActiveStatePublisher.project == self.project, + ActiveStatePublisher.actor_id == self.actor_id, + ActiveStatePublisher.actor == self.actor, ) .one_or_none() ) publisher = maybe_publisher or ActiveStatePublisher( - sub=self.sub, - organization_id=self.organization_id, - project_id=self.project_id, - branch_id=self.branch_id, + organization=self.organization, + project=self.project, + actor_id=self.actor_id, + actor=self.actor, ) session.delete(self) diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index d23bde6c7176..0bc9d441e118 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -29,7 +29,7 @@ from warehouse.macaroons.services import DatabaseMacaroonService from warehouse.oidc.interfaces import IOIDCPublisherService from warehouse.oidc.models import OIDCPublisher, PendingOIDCPublisher -from warehouse.oidc.services import OIDCPublisherService +from warehouse.oidc.services import NullOIDCPublisherService from warehouse.packaging.interfaces import IProjectService from warehouse.packaging.models import ProjectFactory from warehouse.rate_limiting.interfaces import IRateLimiter @@ -117,7 +117,7 @@ def mint_token_from_oidc_github(request: Request): return mint_token(oidc_service, request) -def mint_token(oidc_service: OIDCPublisherService, request: Request) -> JsonRespone: +def mint_token(oidc_service: NullOIDCPublisherService, request: Request) -> JsonRespone: unverified_jwt: str try: payload = TokenPayload.parse_raw(request.body)