Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
crates.io token scopes #2947
crates.io token scopes #2947
Changes from 3 commits
39b738a
d0ee1db
d4cad47
4caaba6
905a789
8e5a9ad
804b6ee
5c43b7d
cffcaf8
0db28df
04b48cb
f5f12a7
7cede63
826ebc0
67d07c6
48d8cf4
fb55797
1d9527e
6aebe17
2804940
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 propose an additional legacy scope. Removing an endpoint from this scope is not considered a breaking change. During the implementation stage, endpoints can be promoted to their own scope or make cookie-only. Ideally, this scope goes away entirely before landing in production.
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 propose that this be replaced by a
legacy
scope.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.
During the deployment, if the
legacy
scope remains, then I think we should migrate existing tokens to only include the default scopes required by cargo. If there are any token-based users of legacy endpoints, then I think they should have to explicitly opt-in.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.
Hmm, this would break everyone using the token to authenticate to endpoints not used by Cargo. Even though we're not providing stability guarantees for them I'd be wary of blindly breaking them. At least I'd like some stats on which endpoints are accessed with the cookie and which are used with tokens.
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.
If I follow this correctly, I think it may be clearer to phrase this as:
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 first sentence is spot on! I'm not so sure if I want to commit to having a "edit token" functionality in the RFC though.
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.
An alternative would be to allow only characters that are allowed in crate names in addition to
,
and*
. Since none of the characters in crate names have a special meaning in regular expressions, and,
and*
are not allowed in crate names, this would remove any ambiguities.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.
True. I'm not sure changing this matters that much though.
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.
Yeah, this is more of an implementation commment – what I suggested is easier to translate into code, but it doesn't really matter.
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 the example below, I think "All other characters" should be changed to "All other non-alphanumeric characters". In practice, I believe
-
and_
are the only characters that matter right now (and_
matches a literal '_' character anyway). I think it makes sense to reject a crate scope if it contains unexpected characters, but that's an implementation detail.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.
In my opinion, legacy scopes should disappear if possible. I know it's not possible, but let's not add new features to them.
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.
Hmm, I don't think it's good to artificially restrict features, but I personally don't care much about it either way.