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

Implement DELETE /api/v1/crates/:crate_id endpoint #9904

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Nov 11, 2024

This PR implements the first step of #9352: a new DELETE /api/v1/crates/:crate_id endpoint that can be used by crate owners to delete their crate under certain conditions (see linked RFC)

Unresolved Questions:

Related:

@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Nov 11, 2024
@Turbo87 Turbo87 requested a review from a team November 11, 2024 14:57
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.72%. Comparing base (c865c0a) to head (606fa59).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9904      +/-   ##
==========================================
+ Coverage   89.51%   89.72%   +0.20%     
==========================================
  Files         295      296       +1     
  Lines       31211    31550     +339     
==========================================
+ Hits        27939    28308     +369     
+ Misses       3272     3242      -30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@LawnGnome
Copy link
Contributor

Going to review momentarily, but I wanted to wrestle with the open question first:

Should names of deleted crates be blocked so that they can't be re-used? – This PR does not implement any blocking for now, similar to how it already works for our crates-admin tool.

The downside in punting an unresolved question in an RFC is that eventually you have to resolve it. 😆

That might be a good one to get outside opinions on. NPM you already noted in the RFC. I don't know what PyPI does off hand — I can't find any reference to blocking names after deletion, but I guess we could confirm that with @di et al. Might be one we could take to a relevant OpenSSF channel.

My feeling is probably very slightly towards allowing re-use: at first blush, lockfiles should prevent obvious supply chain attacks, and it opens up the useful use case of a crate author wanting to free a crate name back up that they no longer intend to use. If we go down the road of blocking versions, we'll probably have to do it by blocking any version that is semver-compatible with any version that was previously published, which feels challenging both implement and (more importantly) explain.

@walterhpearce, you probably have feelings here too?

Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

LGTM. I've got a few nits (mostly around handling of the download limit constant), but the core logic to check the RFC conditions and validate that the user has the correct permissions looks solid to me.

🫥

src/controllers/krate.rs Show resolved Hide resolved
src/controllers/krate/delete.rs Outdated Show resolved Hide resolved
src/controllers/krate/delete.rs Show resolved Hide resolved
if downloads > max_downloads {
let msg =
"only crates with less than 100 downloads per month can be deleted after 72 hours";
return Err(custom(StatusCode::UNPROCESSABLE_ENTITY, msg));
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that these rules are public anyway, I wonder if we shouldn't short circuit, but instead report all the conditions that are failing (if there's more than one).

Copy link
Member Author

Choose a reason for hiding this comment

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

I generally agree. it's somewhat related to #3054, which requests something similar for the publish endpoint. I'm not aware of any ergonomic Error implementations for this use case though. Do you have something in mind? I played around with a custom implementation in the past, but it didn't quite compose as well as I had hoped.

Copy link
Contributor

Choose a reason for hiding this comment

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

The really quick and dirty solution here would be something private to this module that represents the error states as enum values, puts them together into a container, and then just spits out a string that can be given to custom().

It feels like we could extend CustomApiError to support multiple detail messages easily enough, though, if we want something more reusable. Something like this (except with tests, and some renaming of types).

Not too bothered either way, but it feels like we should do at least one of them.

src/controllers/krate/delete.rs Outdated Show resolved Hide resolved
src/controllers/krate/delete.rs Outdated Show resolved Hide resolved
src/controllers/krate/delete.rs Outdated Show resolved Hide resolved
src/controllers/krate/delete.rs Outdated Show resolved Hide resolved
src/controllers/krate/delete.rs Show resolved Hide resolved
Copy link
Contributor

@eth3lbert eth3lbert left a comment

Choose a reason for hiding this comment

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

The implementation looks correct to me 👍

I've left a note and a small suggestion, but they're not blockers.

src/controllers/krate/delete.rs Show resolved Hide resolved
}

#[tokio::test(flavor = "multi_thread")]
async fn test_happy_path_old_crate() -> anyhow::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is a per-month condition, I suggest adding a test case that is older than a month.

@di
Copy link

di commented Nov 12, 2024

I don't know what PyPI does off hand — I can't find any reference to blocking names after deletion, but I guess we could confirm that with @di et al.

PyPI sort of does a mix: we immediately release the project name after deletion, but we prevent the reuse of filenames, which has the effect of requiring a new release if someone were to re-use the project name. We also have controls which prevent the creation of new project names that are too similar with existing projects, so sometimes there is an effect of the project name becoming immediately unavailable after deletion due to this.

What we've found is that a common use case for our users is to make a bunch of releases to a project, delete the project to "wipe the slate clean", and then immediately attempt to make a "proper" release. It's also a common use case for a user to delete an unused or unmaintained project in order to return it to the pool of available names for another user to use.

Due to the limits on filename reuse and our general recommendation to enable hash-checking, the risk of a resurrection attack is relatively low.

All that said, we are currently having a discussion about changing this policy, which you might find relevant: https://discuss.python.org/t/pep-763-limiting-deletions-on-pypi/69487

Might be one we could take to a relevant OpenSSF channel.

Please feel free to take the discussion to the #wg-securing-software-repos channel if you want!

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 18, 2024

I've rebased this on top of #9912 now and integrated the INSERT INTO deleted_crates query into the proposed endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
Status: For next meeting
Development

Successfully merging this pull request may close these issues.

4 participants