-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(crates-io): list owner request unauthorized #16008
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
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the contribution!
Can we add a test for cargo owner --list
demonstrating the problematic behavior? The test should pass in a standalone commit.
The second commit will contain both the fix and the test change diff, which the diff shows the behavior change.
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.
This PR isn't priority for me; I just noticed the issue while reading the code. Determining how to add a test for a presently untested code path is well beyond the investment I have in this getting fixed.
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.
That is understandable. Just call that out that this PR doesn't fix #16007 because of #16008 (comment)
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.
cargo/src/cargo/ops/registry/owner.rs
Lines 39 to 46 in 113761f
let (mut registry, _) = super::registry( | |
gctx, | |
&source_ids, | |
opts.token.as_ref().map(Secret::as_deref), | |
opts.reg_or_index.as_ref(), | |
true, | |
Some(operation), | |
)?; |
This code path super::registry
still requires a token. This may need a bit more refactor
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.
Nope, there is only a single instance in the entire codebase of something delegating to get
. add_owners
delegates to put
which includes the authorization.
get
is private, and list_owners
is the only thing that calls it.
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 code path I shown above unconditionally called before hitting get
even for cargo owner --list
. Granted, this PR standalone is good enough for fixing the issue in the crates-io
crate.
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 just saw the token_required
bit and how the read is being conflated with the owners
add/remove operation
.
I've caught up to you now. It's only the "back half" of the fix, and assumes that we don't throw a TokenMissing
when we create the registry.
Thanks for your patience. ❤️ (Sorry, not an expert in this codebase, but you already knew that. 😜)
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 am fine merging this as-is. Let me edit the title a bit to reflect that.
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.
See #16007
What would be left after this PR for that issue? |
Ah, my answer is in #16007 (comment) |
Problem
list_owners calls
self.get
:cargo/crates/crates-io/lib.rs
Lines 259 to 262 in 113761f
Which, surprisingly, is configured to send
Auth::Authorized
:cargo/crates/crates-io/lib.rs
Lines 377 to 380 in 113761f
But the endpoint does not require auth:
https://crates.io/api/v1/crates/serde/owners
Important note:
list_owners
is the only one that actually calls.get
.Part of #16007