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

Request api token if necessary instead of error after building when publishing a crate #6847

Closed
bjorn3 opened this issue Apr 14, 2019 · 5 comments · Fixed by #6854
Closed
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@bjorn3
Copy link
Member

bjorn3 commented Apr 14, 2019

Describe the problem you are trying to solve

I forgot to login before using cargo publish. It started building the crate to verify it, but when it was finished it gave an error about not being logged in. When I logged in I had to wait for it getting built again.

Describe the solution you'd like

Either error immediately after running cargo publish without logging in first or ask for an api key.

@bjorn3 bjorn3 added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Apr 14, 2019
@fluffysquirrels
Copy link
Contributor

I can take a look at this.

@fluffysquirrels
Copy link
Contributor

fluffysquirrels commented Apr 15, 2019

For my own benefit at least, here is the relevant control flow on the current master:

  • cargo publish CLI command
  • src/cargo/ops/registry.rs
    • fn publish: verifies, calls fn package to make a tarball, calls transmit
    • fn transmit: calls crates_io::Registry.publish, bails if publish is a dry-run
  • src/crates-io/lib.rs: API for crates.io, (other registries too?)
    • struct Registry
    • Registry::publish does the HTTP request to "${HOST}/api/v1/crates/new", when the auth token is missing prints the error message "no upload token found, please run cargo login"

@fluffysquirrels
Copy link
Contributor

So it looks like we could run the auth token check from crates-io/lib.rs' fn Registry::publish in cargo/ops/registry.rs' fn publish when the publish is not a dry-run. We already have a Registry instance at this point, so how about we keep this DRY by creating a utility method on impl Registry to check we have the necessary token?

I'm not sure what automated testing is already done on this part of the code base, I'll take a quick look at that next.

@fluffysquirrels
Copy link
Contributor

grepping for the error message "no upload token found" reveals 2 call sites and 2 tests:

~/Code/cargo % rg -e "no upload token found"
src/crates-io/lib.rs
201:            None => bail!("no upload token found, please run `cargo login`"),
295:                None => bail!("no upload token found, please run `cargo login`"),

tests/testsuite/publish.rs
136:        .with_stderr_contains("[ERROR] no upload token found, please run `cargo login`")

tests/testsuite/alt_registry.rs
422:        .with_stderr_contains("error: no upload token found, please run `cargo login`")

So this error is already tested and the tests should still pass if the error message is returned earlier, so I don't anticipate changing the tests substantially.

@fluffysquirrels
Copy link
Contributor

Hmmm, I wrote code for this and then realised it's not I-nominated yet, as required by the contribution guidelines. I'm asking in Discord about it but until I get a reply the changes are on my branch here: https://github.com/fluffysquirrels/cargo/tree/validate-login

bors added a commit that referenced this issue Apr 17, 2019
Validate registry token before operations that require it.

Fixes #6847 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants