-
Notifications
You must be signed in to change notification settings - Fork 602
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
base: main
Are you sure you want to change the base?
Conversation
55220c9
to
a666f2e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9904 +/- ##
==========================================
+ Coverage 89.22% 89.44% +0.22%
==========================================
Files 295 296 +1
Lines 30839 31181 +342
==========================================
+ Hits 27516 27890 +374
+ Misses 3323 3291 -32 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Going to review momentarily, but I wanted to wrestle with the open question first:
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? |
There was a problem hiding this 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.
🫥
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)); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
c6a8a83
to
87621e0
Compare
There was a problem hiding this 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.
} | ||
|
||
#[tokio::test(flavor = "multi_thread")] | ||
async fn test_happy_path_old_crate() -> anyhow::Result<()> { |
There was a problem hiding this comment.
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.
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
Please feel free to take the discussion to the #wg-securing-software-repos channel if you want! |
87621e0
to
6e45353
Compare
I've rebased this on top of #9912 now and integrated the |
6e45353
to
e24216e
Compare
e24216e
to
726f8c1
Compare
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:
crates-admin
tool.Related:
DeleteCrateFromStorage
background job #9549