Skip to content

Add base_publisher_url to file/release events. #15872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions tests/unit/forklift/test_legacy.py
Original file line number Diff line number Diff line change
Expand Up @@ -3255,6 +3255,9 @@ def test_upload_succeeds_creates_release(
identity.username if test_with_user else "OpenID created token"
),
"canonical_version": release.canonical_version,
"base_publisher_url": (
f"{identity.publisher.publisher_url()}" if not test_with_user else None
),
"publisher_url": (
f"{identity.publisher.publisher_url()}/commit/somesha"
if not test_with_user
Expand All @@ -3269,6 +3272,9 @@ def test_upload_succeeds_creates_release(
identity.username if test_with_user else "OpenID created token"
),
"canonical_version": release.canonical_version,
"base_publisher_url": (
f"{identity.publisher.publisher_url()}" if not test_with_user else None
),
"publisher_url": (
f"{identity.publisher.publisher_url()}/commit/somesha"
if not test_with_user
Expand Down
12 changes: 12 additions & 0 deletions tests/unit/oidc/models/test_activestate.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,18 @@ def test_publisher_name(self):

assert publisher.publisher_name == "ActiveState"

def test_base_publisher_url(self):
org_name = "fakeorg"
project_name = "fakeproject"
publisher = ActiveStatePublisher(
organization=org_name, activestate_project_name=project_name
)

assert (
publisher.base_publisher_url()
== f"https://platform.activestate.com/{org_name}/{project_name}"
)

def test_publisher_url(self):
org_name = "fakeorg"
project_name = "fakeproject"
Expand Down
1 change: 1 addition & 0 deletions tests/unit/oidc/models/test_github.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ def test_github_publisher_computed_properties(self):
assert getattr(publisher, claim_name) is not None

assert str(publisher) == "fakeworkflow.yml"
assert publisher.base_publisher_url() == "https://github.com/fakeowner/fakerepo"
assert publisher.publisher_url() == "https://github.com/fakeowner/fakerepo"
assert (
publisher.publisher_url({"sha": "somesha"})
Expand Down
1 change: 1 addition & 0 deletions tests/unit/oidc/models/test_gitlab.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ def test_gitlab_publisher_computed_properties(self):
assert getattr(publisher, claim_name) is not None

assert str(publisher) == "subfolder/fakeworkflow.yml"
assert publisher.base_publisher_url() == "https://gitlab.com/fakeowner/fakerepo"
assert publisher.publisher_url() == "https://gitlab.com/fakeowner/fakerepo"
assert (
publisher.publisher_url({"sha": "somesha"})
Expand Down
5 changes: 5 additions & 0 deletions tests/unit/oidc/models/test_google.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ def test_publisher_name(self):

assert publisher.publisher_name == "Google"

def test_base_publisher_url(self):
publisher = google.GooglePublisher(email="[email protected]")

assert publisher.base_publisher_url() is None

def test_publisher_url(self):
publisher = google.GooglePublisher(email="[email protected]")

Expand Down
11 changes: 6 additions & 5 deletions tests/unit/packaging/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -759,17 +759,18 @@ def test_trusted_published_all(self, db_session):
assert release.trusted_published

@pytest.mark.parametrize(
"url, publisher_url, expected",
"url, base_publisher_url, expected",
[
("xpto.com", "https://pub/url/", False), # Totally different
("https://pub/", "https://pub/url/", False), # Missing parts
("https://pub/url/", "https://pub/url/", True), # Exactly the same
("https://pub/url/", "https://pub/url/", True), # Exactly the same with /
("https://pub/url", "https://pub/url", True), # The same without /
("https://pub/url/blah.md", "https://pub/url/", True), # Additonal parts
("https://pub/url", "https://pub/url/", True), # Missing trailing slash
("https://pub/url/", "https://pub/url", True), # Extratrailing slash
],
)
def test_is_url_verified(self, db_session, url, publisher_url, expected):
def test_is_url_verified(self, db_session, url, base_publisher_url, expected):
project = DBProjectFactory.create()
release = DBReleaseFactory.create(project=project)
release_file = DBFileFactory.create(
Expand All @@ -780,12 +781,12 @@ def test_is_url_verified(self, db_session, url, publisher_url, expected):
DBFileEventFactory.create(
source=release_file,
tag="fake:event",
additional={"publisher_url": publisher_url},
additional={"base_publisher_url": base_publisher_url},
)
DBFileEventFactory.create(
source=release_file,
tag="fake:event",
additional={"publisher_url": publisher_url},
additional={"base_publisher_url": base_publisher_url},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this second event?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To catch the bug that this commit fixed: 7f93c42. It was failing because it doesn't work when there are multiple events found.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see!! Awesome! Thanks!

)

assert project.is_verified_url(url) is expected
Expand Down
10 changes: 10 additions & 0 deletions warehouse/forklift/legacy.py
Original file line number Diff line number Diff line change
Expand Up @@ -800,6 +800,11 @@ def file_upload(request):
request.user.username if request.user else "OpenID created token"
),
"canonical_version": release.canonical_version,
"base_publisher_url": (
request.oidc_publisher.base_publisher_url(request.oidc_claims)
if request.oidc_publisher
else None
),
"publisher_url": (
request.oidc_publisher.publisher_url(request.oidc_claims)
if request.oidc_publisher
Expand Down Expand Up @@ -1120,6 +1125,11 @@ def file_upload(request):
request.user.username if request.user else "OpenID created token"
),
"canonical_version": release.canonical_version,
"base_publisher_url": (
request.oidc_publisher.base_publisher_url(request.oidc_claims)
if request.oidc_publisher
else None
),
"publisher_url": (
request.oidc_publisher.publisher_url(request.oidc_claims)
if request.oidc_publisher
Expand Down
10 changes: 10 additions & 0 deletions warehouse/oidc/models/_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,16 @@ def publisher_name(self) -> str: # pragma: no cover
# Only concrete subclasses are constructed.
raise NotImplementedError

def base_publisher_url(
self, claims: SignedClaims | None = None
) -> str | None: # pragma: no cover
"""
NOTE: This is **NOT** a `@property` because we pass `claims` to it.
When calling, make sure to use `publisher_url()`
"""
# Only concrete subclasses are constructed.
raise NotImplementedError

def publisher_url(
self, claims: SignedClaims | None = None
) -> str | None: # pragma: no cover
Expand Down
5 changes: 4 additions & 1 deletion warehouse/oidc/models/activestate.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,14 @@ def publisher_name(self) -> str:
def project(self) -> str:
return self.activestate_project_name

def publisher_url(self, claims: SignedClaims | None = None) -> str:
def base_publisher_url(self, claims: SignedClaims | None = None) -> str:
return urllib.parse.urljoin(
_ACTIVESTATE_URL, f"{self.organization}/{self.activestate_project_name}"
)

def publisher_url(self, claims: SignedClaims | None = None) -> str:
return self.base_publisher_url(claims)

def stored_claims(self, claims=None):
return {}

Expand Down
5 changes: 4 additions & 1 deletion warehouse/oidc/models/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,11 @@ def job_workflow_ref(self):
def sub(self):
return f"repo:{self.repository}"

def base_publisher_url(self, claims=None):
return f"https://github.com/{self.repository}"

def publisher_url(self, claims=None):
base = f"https://github.com/{self.repository}"
base = self.base_publisher_url(claims)
sha = claims.get("sha") if claims else None

if sha:
Expand Down
5 changes: 4 additions & 1 deletion warehouse/oidc/models/gitlab.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,11 @@ def ci_config_ref_uri(self):
def publisher_name(self):
return "GitLab"

def base_publisher_url(self, claims=None):
return f"https://gitlab.com/{self.project_path}"

def publisher_url(self, claims=None):
base = f"https://gitlab.com/{self.project_path}"
base = self.base_publisher_url(claims)
return f"{base}/commit/{claims['sha']}" if claims else base

def stored_claims(self, claims=None):
Expand Down
3 changes: 3 additions & 0 deletions warehouse/oidc/models/google.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ def __lookup_no_sub__(klass, signed_claims: SignedClaims) -> Query | None:
def publisher_name(self):
return "Google"

def base_publisher_url(self, claims=None):
return None

def publisher_url(self, claims=None):
return None

Expand Down
2 changes: 1 addition & 1 deletion warehouse/packaging/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ def is_verified_url(self, url):
.filter(Project.id == self.id)
.filter(
literal(url).ilike(
File.Event.additional.op("->>")("publisher_url") + "%"
File.Event.additional.op("->>")("base_publisher_url") + "%"
)
)
.first()
Expand Down