Fix ActiveState model tests, OIDC utils tests

Author: th3coop

tests/common/db/oidc.py

@@ -95,14 +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 = factory.Faker("pystr", max_chars=12)
-    project_id = factory.Faker("uuid4")
     project = factory.Faker("pystr", max_chars=12)
     actor = factory.Faker("pystr", max_chars=12)
     actor_id = factory.Faker("uuid4")
-    branch_id = factory.Faker("uuid4")
+    ingredient = factory.Faker("pystr", max_chars=12)
 
 
 class PendingActiveStatePublisherFactory(WarehouseFactory):
@@ -111,10 +108,9 @@ class Meta:
 
     id = factory.Faker("uuid4", cast_to=None)
     project_name = factory.Faker("pystr", max_chars=12)
-    organization_id = factory.Faker("uuid4")
     organization = factory.Faker("pystr", max_chars=12)
-    project_id = factory.Faker("uuid4")
     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)

tests/unit/oidc/models/test_activestate.py

@@ -28,13 +28,15 @@
 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"
+SUBJECT = f"org:{ORG_URL_NAME}:project:{PROJECT_NAME}"
 
 
 def test_lookup_strategies():
@@ -47,8 +49,9 @@ def test_lookup_strategies():
 
 def new_signed_claims(
     sub: str = SUBJECT,
-    actor: str = "fakeuser",
+    actor: str = ACTOR,
     actor_id: str = ACTOR_ID,
+    ingredient: str = INGREDIENT,
     organization: str = ORG_URL_NAME,
     org_id: str = "fakeorgid",
     project: str = PROJECT_NAME,
@@ -62,14 +65,14 @@ def new_signed_claims(
             "sub": sub,
             "actor": actor,
             "actor_id": actor_id,
+            "ingredient": ingredient,
             "organization_id": org_id,
             "organization": organization,
             "project_visibility": project_visibility,
             "project_id": project_id,
             "project_path": project_path,
-            "project_name": project,
-            "builder": "fakebuilder",
-            "ingredient": "fakeingredient",
+            "project": project,
+            "builder": "pypi-publisher",
         }
     )
     if branch_id:
@@ -109,6 +112,8 @@ def test_activestate_publisher_all_known_claims(self):
             "organization",
             "project",
             "actor_id",
+            "actor",
+            "builder",
             "sub",
             # optional verifiable claims
             "branch_id",
@@ -120,15 +125,14 @@ def test_activestate_publisher_all_known_claims(self):
             "aud",
             # unchecked claims
             "project_visibility",
-            "project_name",
             "project_path",
-            "organization",
+            "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,21 +165,25 @@ def test_activestate_publisher_unaccounted_claims(self, monkeypatch):
             ("organization", False),
             ("project", False),
             ("actor_id", False),
-            ("branch_id", True),
-            ("organization", True),
+            ("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=ORG_URL_NAME,
             project=PROJECT_NAME,
             actor_id=ACTOR_ID,
+            actor=ACTOR,
+            ingredient=INGREDIENT,
         )
 
         scope = pretend.stub()
@@ -216,10 +224,11 @@ def test_activestate_publisher_org_id_verified(
         self, expect: str, actual: str, valid: bool
     ):
         publisher = ActiveStatePublisher(
-            sub=SUBJECT,
             organization=actual,
             project=PROJECT_NAME,
             actor_id=ACTOR_ID,
+            actor=ACTOR,
+            ingredient=INGREDIENT,
         )
 
         signed_claims = new_signed_claims(organization=expect)
@@ -236,10 +245,11 @@ def test_activestate_publisher_project_id_verified(
         self, expect: str, actual: str, valid: bool
     ):
         publisher = ActiveStatePublisher(
-            sub=SUBJECT,
             organization=ORG_URL_NAME,
             project=actual,
             actor_id=ACTOR_ID,
+            actor=ACTOR,
+            ingredient=INGREDIENT,
         )
 
         signed_claims = new_signed_claims(project=expect)
@@ -256,10 +266,11 @@ def test_activestate_publisher_user_id_verified(
         self, expect: str, actual: str, valid: bool
     ):
         publisher = ActiveStatePublisher(
-            sub=SUBJECT,
             organization=ORG_URL_NAME,
             project=PROJECT_NAME,
             actor_id=actual,
+            actor=ACTOR,
+            ingredient=INGREDIENT,
         )
 
         signed_claims = new_signed_claims(actor_id=expect)
@@ -296,7 +307,8 @@ def test_activestate_publisher_user_id_verified(
     )
     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:

tests/unit/oidc/test_utils.py

@@ -115,80 +115,73 @@ def test_find_publisher_by_issuer_google(db_request, sub, expected_id):
 
 
 @pytest.mark.parametrize(
-    "expected_id, sub, organization_id, project_id, actor_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",
         ),
     ],
 )
 def test_find_publisher_by_issuer_activestate(
     db_request,
     sub: str,
     expected_id: str,
-    organization_id: str,
-    project_id: str,
+    organization: str,
+    project: str,
     actor_id: str,
-    branch_id: str | None,
+    actor: str,
 ):
     ActiveStatePublisherFactory(
         id="aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
-        organization_id="00000000-1000-8000-0000-000000000001",
-        project_id="00000000-1000-8000-0000-000000000002",
+        organization="fakeorg1",
+        project="fakeproject1",
         actor_id="00000000-1000-8000-0000-000000000003",
-        sub="org:00000000-1000-8000-0000-000000000001:project:00000000-1000-8000-0000-000000000002",  # noqa
+        actor="fakeuser1",
     )
     ActiveStatePublisherFactory(
         id="bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb",
-        organization_id="00000000-1000-8000-0000-000000000004",
-        project_id="00000000-1000-8000-0000-000000000005",
+        organization="fakeorg2",
+        project="fakeproject2",
         actor_id="00000000-1000-8000-0000-000000000006",
-        sub="org:00000000-1000-8000-0000-000000000004:project:00000000-1000-8000-0000-000000000005",  # noqa
+        actor="fakeuser2",
     )
     ActiveStatePublisherFactory(
         id="cccccccc-cccc-cccc-cccc-cccccccccccc",
-        organization_id="00000000-1000-8000-0000-000000000007",
-        project_id="00000000-1000-8000-0000-000000000008",
+        organization="fakeorg3",
+        project="fakeproject3",
         actor_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
+        actor="fakeuser3",
     )
 
     signed_claims = {
         "sub": sub,
-        "organization_id": organization_id,
-        "organization": "fakeOrg",
-        "project_id": project_id,
-        "project_name": "fakername1",
-        "project_path": "fakeOrg/fakername1",
+        "organization": organization,
+        "project": project,
         "actor_id": actor_id,
-        "project_visibility": "public",
+        "actor": actor_id,
     }
 
-    if branch_id:
-        signed_claims["branch_id"] = branch_id
-
     assert (
         utils.find_publisher_by_issuer(
             db_request.db,

warehouse/oidc/models/_core.py

@@ -133,9 +133,9 @@ 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
+
         return None
 
     @classmethod

warehouse/oidc/models/activestate.py

@@ -50,7 +50,10 @@ def _check_sub(
     if len(components) < 2:
         return False
 
-    return f"{components[0]}:{components[1]}" == ground_truth
+    return (
+        f"{components[0]}:{components[1]}:{components[2]}:{components[3]}"
+        == ground_truth
+    )
 
 
 class ActiveStatePublisherMixin:
@@ -72,25 +75,23 @@ class ActiveStatePublisherMixin:
         "project": oidccore.check_claim_binary(str.__eq__),
         "actor_id": oidccore.check_claim_binary(str.__eq__),
         "actor": oidccore.check_claim_binary(str.__eq__),
-        "ingredient": 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]] = {
+        # Should we check that the pypi projectname matches this ingredient
+        # name?
         "ingredient": oidccore.check_claim_binary(str.__eq__),
     }
 
     __unchecked_claims__ = {
         "organization_id",
-        "project_visibility",
         "project_id",
+        "project_visibility",
         "project_path",
         "branch_id",
-        # Should we check that the pypi projectname matches this ingredient
-        # name?
-        "ingredient",
     }
 
     @staticmethod
@@ -99,7 +100,6 @@ def __lookup_all__(klass, signed_claims: SignedClaims):
             organization=signed_claims["organization"],
             project=signed_claims["project"],
             actor_id=signed_claims["actor_id"],
-            actor=signed_claims["actor"],
         )
 
     __lookup_strategies__ = [
@@ -110,6 +110,10 @@ def __lookup_all__(klass, signed_claims: SignedClaims):
     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"
@@ -168,15 +172,16 @@ def reify(self, session):
                 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=self.organization,
             project=self.project,
             actor_id=self.actor_id,
+            actor=self.actor,
         )
 
         session.delete(self)

warehouse/oidc/utils.py

@@ -66,7 +66,6 @@ def find_publisher_by_issuer(session, issuer_url, signed_claims, *, pending=Fals
         # This indicates a logic error, since we shouldn't have verified
         # claims for an issuer that we don't recognize and support.
         return None
-
     return publisher_cls.lookup_by_claims(session, signed_claims)