Skip to content
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

Trusted publishing: surface pending publisher name collisions #16260

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

twm
Copy link
Contributor

@twm twm commented Jul 11, 2024

Provide up-front validation of the project name within a pending publisher by:

  • Extracting the validation logic from ProjectService.create_project to a new ProjectService.check_project_name routine
  • Update ProjectService.create_project to call check_project_name
  • Thread ProjectService into the forms in place of ProjectFactory.

Fixes #16226.

@di
Copy link
Member

di commented Jul 11, 2024

Hey @twm, FYI our general policy is to not review draft PRs unless explicitly asked -- let us know if you have any questions here, otherwise we will wait for this to become ready to review.

@twm
Copy link
Contributor Author

twm commented Jul 13, 2024

Hi @di, thanks for approving the build! Some connectivity issues are making it difficult to get things working locally so it's very helpful. I'll remove the draft status when I'm ready for review!

@twm twm force-pushed the 16226-oidc-name-validation branch 2 times, most recently from db324f3 to 2ab2a55 Compare July 17, 2024 05:08
Copy link
Contributor Author

@twm twm left a comment

Choose a reason for hiding this comment

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

Hi @di, this is ready for review now!

warehouse/packaging/services.py Outdated Show resolved Hide resolved
warehouse/packaging/services.py Outdated Show resolved Hide resolved
@twm twm marked this pull request as ready for review July 17, 2024 18:45
@twm twm requested a review from a team as a code owner July 17, 2024 18:45
@twm twm force-pushed the 16226-oidc-name-validation branch from ed79c6b to 7033c15 Compare July 17, 2024 18:47
@twm twm mentioned this pull request Aug 2, 2024
@twm twm force-pushed the 16226-oidc-name-validation branch 2 times, most recently from 4065562 to 9cdb4ab Compare August 6, 2024 06:32
@twm twm force-pushed the 16226-oidc-name-validation branch 10 times, most recently from a8ede89 to 2182f86 Compare August 20, 2024 17:59
Comment on lines +51 to +52
CREATE INDEX pending_project_name_ultranormalized
ON pending_oidc_publishers (ultranormalize_name(project_name));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted against CREATE INDEX CONCURRENTLY here because the original migration didn't use it and I expect that there are orders of magnitude fewer pending_oidc_publishers rows than projects rows.

@twm twm force-pushed the 16226-oidc-name-validation branch from 2182f86 to 79daaed Compare August 20, 2024 18:13
@twm
Copy link
Contributor Author

twm commented Aug 20, 2024

Hi @di, this is ready again. I'd appreciate a review within a few days if you can, as I keep having to rebase through conflicts.

@twm twm force-pushed the 16226-oidc-name-validation branch from 79daaed to 0787590 Compare August 20, 2024 18:59
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

Just one last comment, otherwise LGTM!

Comment on lines 589 to 592
or_(
func.normalize_pep426_name(PendingOIDCPublisher.project_name)
== func.normalize_pep426_name(project.name),
func.ultranormalize_name(PendingOIDCPublisher.project_name)
== func.ultranormalize_name(project.name),
),
Copy link
Member

Choose a reason for hiding this comment

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

I think the 'ultranormalized' set will include all of the 'normalized' matches as well? This or_ condition might not be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it safer to match on both since there is no formal relationship between the functions that guarantees that to be the case, but I will drop it per your preference.

@twm twm force-pushed the 16226-oidc-name-validation branch 2 times, most recently from 0c19353 to 13bd1d3 Compare September 22, 2024 18:27
@twm twm requested a review from di September 22, 2024 18:36
@twm
Copy link
Contributor Author

twm commented Sep 22, 2024

Hi @di, sorry for losing track of this. I've addressed your comment and merged forward.

warehouse/packaging/services.py Outdated Show resolved Hide resolved
@twm twm force-pushed the 16226-oidc-name-validation branch 2 times, most recently from ac2cafd to 18bfa3f Compare September 24, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trusted publishing: pending publisher should warn about ultranormalized name collision
3 participants