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.

Sign up for GitHub

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

Jump to bottom

Check empty input for login #11145

Merged
merged 3 commits into from
Oct 7, 2022
Merged

Check empty input for login #11145

merged 3 commits into from
Oct 7, 2022

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Sep 26, 2022 •
edited
Loading

What does this PR try to resolve?

close #11144
Check empty input for login.

How should we test and review this PR?

  • unit test
  • cargo login and enter

@rust-highfive
Copy link

r? @weihanglo

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2022
@Rustin170506 Rustin170506 force-pushed the rustin-patch-login branch 2 times, most recently from 1cc9205 to 73a8c1b Compare September 26, 2022 03:04
weihanglo
weihanglo reviewed Sep 26, 2022
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

What I am going to say might sound stupid: Is it possible that a registry accepts empty tokens to login?

For some reason, I know that some websites permit login with empty passwords. I cannot guarantee there isn't a private registry does the same thing at this moment.

@Rustin170506
Copy link
Member Author

Is it possible that a registry accepts empty tokens to login?

I think the empty password makes sense, but the empty token doesn't make much sense.

@Rustin170506

This comment was marked as outdated.

Sign in to view
@weihanglo
Copy link
Member

Is it possible that a registry accepts empty tokens to login?

I think the empty password makes sense, but the empty token doesn't make much sense.

Agree with you that empty token seems like a weird existence. This patch IMO is with a good intent. I just want to make sure no one's compilation is broken with it.

@arlosi
Copy link
Contributor

arlosi commented Sep 27, 2022

This only affects cargo login. Users who needed to send an empty token (seems really unlikely) could still manually use the credentials file.

@weihanglo weihanglo added the T-cargo Team: Cargo label Sep 27, 2022
@weihanglo
Copy link
Member

Supposedly because the scenario of sending an empty token is rare, and cargo login should be used mostly in interactive sessions, I propose to merge this. This will prevent people from adding empty tokens by accident.

The risk is that someone need to pass an empty token via CLI cannot work anymore. As @arlosi said, people could still add empty tokens through credentials file if they intend to do so.

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Sep 27, 2022 •
edited by Eh2406
Loading

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Sep 27, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 27, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@Rustin170506
Check empty input for login
06bd857 
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@Rustin170506
Add tests
b3da66f 
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@Rustin170506
Also check the token argument
ff575b2 
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Oct 7, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Oct 7, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 7, 2022

📌 Commit ff575b2 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 7, 2022
@bors
Copy link
Contributor

bors commented Oct 7, 2022

⌛ Testing commit ff575b2 with merge 882c5dd...

@bors
Copy link
Contributor

bors commented Oct 7, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 882c5dd to master...

@bors bors merged commit 882c5dd into rust-lang:master Oct 7, 2022
@Rustin170506 Rustin170506 deleted the rustin-patch-login branch October 8, 2022 02:19
weihanglo added a commit to weihanglo/rust that referenced this pull request Oct 11, 2022
@weihanglo
Update cargo
1ff4f52 
9 commits in 3cdf1ab25dc4fe56f890e8c7330d53a23ad905d3..b8f30cb23c4e5f20854a4f683325782b7cff9837
2022-10-07 17:34:03 +0000 to 2022-10-10 19:16:06 +0000

- Add more doc comments for three modules (rust-lang/cargo#11207)
- docs: fix (rust-lang/cargo#11208)
- Add completions for `cargo remove` (rust-lang/cargo#11204)
- Config file loaded via CLI takes priority over env vars (rust-lang/cargo#11077)
- Use `#[default]` when possible (rust-lang/cargo#11197)
- Implement RFC 3289: source replacement ambiguity (rust-lang/cargo#10907)
- Use correct version of cargo in test (rust-lang/cargo#11193)
- Check empty input for login (rust-lang/cargo#11145)
- Add retry support to sparse registries (rust-lang/cargo#11069)
@weihanglo weihanglo mentioned this pull request Oct 11, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2022
@bors
Auto merge of rust-lang#102934 - weihanglo:update-cargo, r=ehuss
7e8d64e 
Update cargo

9 commits in 3cdf1ab25dc4fe56f890e8c7330d53a23ad905d3..b8f30cb23c4e5f20854a4f683325782b7cff9837 2022-10-07 17:34:03 +0000 to 2022-10-10 19:16:06 +0000

- Add more doc comments for three modules (rust-lang/cargo#11207)
- docs: fix (rust-lang/cargo#11208)
- Add completions for `cargo remove` (rust-lang/cargo#11204)
- Config file loaded via CLI takes priority over env vars (rust-lang/cargo#11077)
- Use `#[default]` when possible (rust-lang/cargo#11197)
- Implement RFC 3289: source replacement ambiguity (rust-lang/cargo#10907)
- Use correct version of cargo in test (rust-lang/cargo#11193)
- Check empty input for login (rust-lang/cargo#11145)
- Add retry support to sparse registries (rust-lang/cargo#11069)
@andrewvyrros andrewvyrros mentioned this pull request Feb 7, 2023
Closed
@ehuss ehuss added this to the 1.66.0 milestone Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Rustin170506 @rust-highfive @weihanglo @arlosi @rfcbot @bors @ehuss