Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions cli/src/cmd_auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,7 @@ impl CmdAuthStatus {
{
let client = Client::new_authenticated_config(
&ClientConfig::default().with_host_and_token(&host_env, &token_env),
)
.expect("client authentication from host/token failed");
)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the new behavior in these cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously this path was in practice infallible, now it can fail if OXIDE_HOST and OXIDE_PROFILE are both set. With the expect we get a somewhat confusing panic here:

$ OXIDE_HOST=a OXIDE_TOKEN=b OXIDE_PROFILE=c cargo run -- auth status
thread 'main' panicked at cli/src/cmd_auth.rs:501:14:
client authentication from host/token failed: HostProfileConflict


spinner.set_message(format!("Checking {}...", host_env));
spinner.enable_steady_tick(Duration::from_millis(100));
Expand All @@ -521,8 +520,7 @@ impl CmdAuthStatus {
let client = Client::new_authenticated_config(
&ClientConfig::default()
.with_host_and_token(&profile_info.host, &profile_info.token),
)
.expect("client authentication from host/token failed");
)?;

spinner.reset();
spinner.set_message(format!("Checking {}...", &profile_info.host));
Expand Down
68 changes: 60 additions & 8 deletions cli/tests/test_auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,14 +184,8 @@ fn test_auth_login_first() {
assert_mode(&config_dir.join("config.toml"), 0o644);
}

fn write_first_creds(dir: &Path) {
fn write_creds(dir: &Path, creds: &str) {
let cred_path = dir.join("credentials.toml");
let creds = "\
[profile.first]\n\
host = \"https://oxide.internal\"\n\
token = \"***-***-***\"\n\
user = \"00000000-0000-0000-0000-000000000000\"\n\
";
write(&cred_path, creds).unwrap();

// On Unix set permissions to 0600 to avoid triggering permissions warning.
Expand All @@ -204,6 +198,19 @@ fn write_first_creds(dir: &Path) {
file.set_permissions(perms).unwrap();
}
}

fn write_first_creds(dir: &Path) {
write_creds(
dir,
"\
[profile.first]\n\
host = \"https://oxide.internal\"\n\
token = \"***-***-***\"\n\
user = \"00000000-0000-0000-0000-000000000000\"\n\
",
);
}

fn write_first_config(dir: &Path) {
let config_path = dir.join("config.toml");
let config = "\
Expand Down Expand Up @@ -540,6 +547,22 @@ fn test_cmd_auth_status_env() {
});
});

let temp_dir = tempfile::tempdir().unwrap();
let temp_dir_path = temp_dir.path();

write_creds(
temp_dir_path,
&format!(
"\
[profile.funky-town]\n\
host = \"{}\"\n\
token = \"oxide-token-good\"\n\
user = \"00000000-0000-0000-0000-000000000000\"\n\
",
server.url("")
),
);

// Validate authenticated credentials
Command::cargo_bin("oxide")
.unwrap()
Expand All @@ -554,7 +577,36 @@ fn test_cmd_auth_status_env() {
server.url("")
));

oxide_mock.assert();
// OXIDE_PROFILE also works, uses creds file
Command::cargo_bin("oxide")
.unwrap()
.env("OXIDE_PROFILE", "funky-town")
.arg("--config-dir")
.arg(temp_dir.path().as_os_str())
.arg("auth")
.arg("status")
.assert()
.success()
.stdout(format!(
"Profile \"funky-town\" ({}) status: Authenticated\n",
server.url(""),
));

// OXIDE_HOST conflicts with OXIDE_PROFILE
Command::cargo_bin("oxide")
.unwrap()
.env("OXIDE_HOST", server.url(""))
.env("OXIDE_TOKEN", "oxide-token-good")
.env("OXIDE_PROFILE", "ignored")
.arg("--config-dir")
.arg(temp_dir.path().as_os_str())
.arg("auth")
.arg("status")
.assert()
.failure()
.stderr(format!("{}\n", oxide::OxideAuthError::HostProfileConflict));

oxide_mock.assert_hits(2);

let oxide_mock = server.current_user_view(|when, then| {
when.into_inner()
Expand Down
6 changes: 6 additions & 0 deletions sdk/src/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ impl Client {
..
} = config;

if std::env::var("OXIDE_HOST").is_ok() && std::env::var("OXIDE_PROFILE").is_ok() {
return Err(OxideAuthError::HostProfileConflict);
}
Comment on lines +191 to +193
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could do something with clap's parsing to examine env vars... but I'm not sure that would be better; just noting it in case it seems worthwhile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about adding OXIDE_PROFILE to just the CLI, which would let easy to do with Arg::env, as you say. This seems like something that would be generally convenient, so I went with including it in the SDK.


let (host, token) = match auth_method {
AuthMethod::DefaultProfile => get_profile_auth(config_dir, None)?,
AuthMethod::Profile(profile) => get_profile_auth(config_dir, Some(profile))?,
Expand Down Expand Up @@ -296,6 +300,8 @@ fn get_profile_auth(
.next()
.ok_or(OxideAuthError::MissingToken(env_host))?
.clone()
} else if let Ok(env_profile) = std::env::var("OXIDE_PROFILE") {
env_profile
} else {
let config_path = config_dir.join("config.toml");
let contents = std::fs::read_to_string(&config_path).map_err(|e| {
Expand Down
2 changes: 2 additions & 0 deletions sdk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ pub enum OxideAuthError {
Login without $OXIDE_HOST set or set $OXIDE_TOKEN."
)]
MissingToken(String),
#[error("Both $OXIDE_HOST and $OXIDE_PROFILE are set, only one may be used")]
HostProfileConflict,
#[error("Parse error for {0}: {1}")]
TomlError(PathBuf, toml::de::Error),
#[error("IO Error: {0}")]
Expand Down