Support login tokens for multiple registries #4680

cswindle · Oct 30, 2017

This pull request builds on #4206 to support login using the the registry from alternate registries (RFC 2141). This includes the following changes:

allow credentials to be stored based on the registry
allow passing the registry to run cargo commands against using --registry

Note that this does not include a feature gate on the use of --registry as the publish code blocks publish if we use any features. @alexcrichton, are you happy with this approach, or is there a way that you would recommend this should be relaxed for testing purposes?

rust-highfive · Oct 30, 2017

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · Oct 30, 2017

alexcrichton reviewed Oct 30, 2017

View changes

alexcrichton · Oct 30, 2017

src/bin/login.rs

+                    let config = src.config()?.unwrap();
+                    options.flag_host.clone().unwrap_or(config.api.unwrap())
+                }
+            };
            println!("please visit {}me and paste the API Token below", host);


Hm I think this should only get printed for the crates.io registry, right? We may not want to assume the layout of another registry per se just yet, and instead we could probably just return an error if no token was passed and --registry was passed?

I was wondering if the /me API should be required for publish, I am happy to switch to just return an error stating that the token should be provided.

Yeah this is one where we haven't done much to sketch out the path towards "what does a custom registry need HTTP-wise", so I think it's ok to err on the side of caution for now and just return an error

alexcrichton · Oct 30, 2017

alexcrichton reviewed Oct 30, 2017

View changes

alexcrichton · Oct 30, 2017

src/cargo/ops/registry.rs

+    let (index, token) = match registry {
+        Some(registry) => {
+            let index = Some(config.get_registry_index(&registry)?);
+            let table = config.get_table(&format!("registry.{}", registry))?.map(|t| t.val);


Could this use get_string perhaps? That'll allow overrides via env var (table support isn't implemented there yet)

I did originally try using get_string, but that caused problems if there was a '.' in the registry name (which I think is allowed). I will have a play and see if I can get something working.

Turns out it does just work switching to get_string, so I have made the change.

alexcrichton · Oct 30, 2017

alexcrichton reviewed Oct 30, 2017

View changes

alexcrichton · Oct 30, 2017

src/cargo/util/config.rs

@@ -545,6 +545,14 @@ impl Config {
        }
    }

+    /// Gets the index for a registry.
+    pub fn get_registry_index(&self, registry: &str) -> CargoResult<String> {


Perhaps this could bake in returning a Url instead of a String?

Sounds reasonable, I will update.

alexcrichton · Oct 30, 2017

Looking great! Could this also be sure to place the support behind a feature gate? To do something like rustc it means we'd add a -Z unstable-options "feature" which would unlock other unstable flags like --registry

cswindle · Oct 31, 2017

@alexcrichton, I have now made all the changes that you requested as part of your review.

alexcrichton · Oct 31, 2017

alexcrichton reviewed Oct 31, 2017

View changes

Looking great! Lemme know if anything doesn't make sense though.

alexcrichton · Oct 31, 2017

tests/alt-registry.rs

+    // Now perform the actual publish
+    assert_that(p.cargo("publish").masquerade_as_nightly_cargo()
+                 .arg("--registry").arg("alternative").arg("-Zunstable-options"),
+                execs().with_status(101));


Could you add an assertion about stderr here to ensure it prints the right error message?

alexcrichton · Oct 31, 2017

tests/alt-registry.rs

+    // Now perform the actual publish
+    assert_that(p.cargo("publish").masquerade_as_nightly_cargo()
+                 .arg("--registry").arg("alternative").arg("-Zunstable-options"),
+                execs().with_status(0));


Hm could you describe the behavior of this test a bit? Presumably this is sending a POST which I guess is "uploading" a file to the local filesystem?

If that's the case, can this assert that the file was created? I think the binary format is pretty crazy so it's ok to not actually look inside the file, but asserting at least that it exists may be good?

This is just performing an upload to the local registry in the same way that publish_clean currently works (with the only change that we are going to an alternate registry), I am not too keen on adding too much extra checking of the actual registry contents as that will likely be a remote server which we will be receiving the package normally. Are you happy for me to not make this change?

Oh yeah I was just thinking we could assert that a file exists on the filesystem somewhere. Basically assert that the upload happened to the alternative registry here and not the "main registry"

alexcrichton · Oct 31, 2017

tests/login.rs

@@ -7,7 +7,7 @@ extern crate toml;
 use std::io::prelude::*;


This (and I think at least one other file?) I think changed from 644 permissions to 755, mind reverting back?

I will revert those.

alexcrichton · Oct 31, 2017

tests/login.rs

+    [registries.test-reg]
+    index = "http://dummy_index/"
+
+    [registries.test.reg]


This actually reminds me, at some point we're going to want validation on the names of registries I think? Probably for now (to start conservative at least) we probably want to restrict registries to [a-zA-Z0-9_-] probably?

Restrict the identifiers of the registry, that is

In that case I will remove the test for test.reg and remove this entry. I will also add this to the issue list for stabilization.

alexcrichton · Oct 31, 2017

src/cargo/util/config.rs

+        if let Some(registry) = registry {
+            let mut map = HashMap::new();
+            map.insert(key, value);
+            (registry, CV::Table(map, file.path().to_path_buf()))


I think this is building up a TOML file like:

[my-registry-name] token = "..."

right?

That seems fine by me for now (although maybe registry.foo for consistency with specifying the index?), but one thing I'm a little worried about is that we're making a lot of decisions in a bit of an ad-hoc fashion. I think all the conclusions are fine so far, but it'd be good to have a list of "here's the design decisions that were made" when we go for stabilizing custom registries.

Mind opening a tracking issue for custom registries (or appending to one that already exists?). I'm thinking we could track things like the --registry flag, my concern below for the naming of the registry identifiers, the format of the credentials file, the configuration syntax in .cargo/config for custom registries, etc.

Basically I'm thinking we should just track somewhere "here's what we need to be sure about for stabilization" and we can throw concerns/thoughts/implementation details on that as they arise.

I will raise an issue to cover this.

alexcrichton · Oct 31, 2017

Ok looks great! r=me with an assertion that the publish happens to the right registry (asserting that a file exists)

cswindle · Oct 31, 2017

@alexcrichton, I have added the additional test that you asked for.

cswindle · Nov 1, 2017

Fixes #4498 and #3365.

alexcrichton · Nov 1, 2017

@bors: r+

Looks great!

bors · Nov 1, 2017

📌 Commit 4ef8f55 has been approved by alexcrichton

bors · Nov 1, 2017

⌛️ Testing commit 4ef8f55 with merge 551b62b...

bors · Nov 1, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 551b62b to master...

rust-highfive assigned matklad Oct 30, 2017

cswindle referenced this pull request Oct 30, 2017
Merged
Add support for publish to optionally take the index that can be used #4568

alexcrichton referenced this pull request Oct 30, 2017
Closed
Replace `cargo login --host` with `cargo login --index`. #4613

cswindle added some commits Oct 31, 2017

Updated tests based on review comments.

Loading status checks…

3c7a3cb

Sort out remaining file permissions.

Loading status checks…

276e902

Add verification that an crate has been published to the correct loca…
…tion.

Loading status checks…

4ef8f55

cswindle referenced this pull request Nov 1, 2017
Closed
Areas requiring agreement for stabilization of alternative registries #4688
2 of 10 tasks complete

bors merged commit 4ef8f55 into rust-lang:master Nov 1, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors referenced this pull request Nov 1, 2017
Closed
Add a publish-build-info command for reporting build results to crates.io #3627

This was referenced Nov 24, 2017

Closed

`cargo login` should allow for multiple tokens, one per registry, to be saved in ~/.cargo/credentials #3365

Closed

Once we support registry configuration, allow specifying `--registry` in addition to `--index` #4498

aturon referenced this pull request Jan 8, 2018
Closed
Tracking issue for RFC 2141: Add support to Cargo for alternative registries #44931
4 of 11 tasks complete

rust-lang/cargo

Join GitHub today

Support login tokens for multiple registries #4680

Conversation

cswindle commented Oct 30, 2017

dethoter and others added some commits Jun 22, 2017

rust-highfive assigned matklad Oct 30, 2017

This comment has been minimized.

rust-highfive commented Oct 30, 2017

cswindle referenced this pull request Oct 30, 2017

alexcrichton reviewed Oct 30, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 30, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 30, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Oct 30, 2017

alexcrichton referenced this pull request Oct 30, 2017

cswindle added some commits Oct 30, 2017

This comment has been minimized.

cswindle commented Oct 31, 2017

alexcrichton reviewed Oct 31, 2017 View changes

alexcrichton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cswindle added some commits Oct 31, 2017

This comment has been minimized.

alexcrichton commented Oct 31, 2017

This comment has been minimized.

cswindle commented Oct 31, 2017

This comment has been minimized.

cswindle commented Nov 1, 2017

cswindle referenced this pull request Nov 1, 2017

This comment has been minimized.

alexcrichton commented Nov 1, 2017

This comment has been minimized.

bors commented Nov 1, 2017

This comment has been minimized.

bors commented Nov 1, 2017

bors added a commit that referenced this pull request Nov 1, 2017

This comment has been minimized.

bors commented Nov 1, 2017

Hide details View details bors merged commit 4ef8f55 into rust-lang:master Nov 1, 2017 3 checks passed

3 checks passed

bors referenced this pull request Nov 1, 2017

This was referenced Nov 24, 2017

aturon referenced this pull request Jan 8, 2018

alexcrichton reviewed Oct 30, 2017

View changes

alexcrichton reviewed Oct 30, 2017

View changes

alexcrichton reviewed Oct 30, 2017

View changes

alexcrichton reviewed Oct 31, 2017

View changes

bors merged commit `4ef8f55` into rust-lang:master Nov 1, 2017
3 checks passed