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

Allow username and password to be passed into register with flags #401

Merged
merged 1 commit into from
Aug 23, 2024
Merged

Allow username and password to be passed into register with flags #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 16 additions & 1 deletion src/cli/cbenv_register.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@ impl Command for CbEnvRegister {
SyntaxShape::String,
"the connection string to use for this cluster",
)
.named(
"username",
SyntaxShape::String,
"the username to use for the registered cluster",
None,
Westwooo marked this conversation as resolved.
Show resolved Hide resolved
)
.named(
"password",
SyntaxShape::String,
"the password to use with the registered cluster",
None,
)
.named(
"display_name",
SyntaxShape::String,
Expand Down Expand Up @@ -150,7 +162,10 @@ fn clusters_register(
None
};

let (username, password) = get_username_and_password()?;
let username_flag = call.get_flag(engine_state, stack, "username")?;
let password_flag = call.get_flag(engine_state, stack, "password")?;

let (username, password) = get_username_and_password(username_flag, password_flag)?;

let cluster = RemoteCluster::new(
RemoteClusterResources {
Expand Down
2 changes: 1 addition & 1 deletion src/cli/credentials_create.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ fn credentials_create(
active_cluster.password().to_string(),
)
} else {
get_username_and_password()?
get_username_and_password(None, None)?
};

let payload = CredentialsCreateRequest::new(name.clone(), password.clone(), read, write);
Expand Down
44 changes: 29 additions & 15 deletions src/cli/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,21 +444,35 @@ pub fn get_active_cluster<'a>(
}
}

pub fn get_username_and_password() -> Result<(String, String), ShellError> {
println!("Please enter username:");
let username = match read_input() {
Some(user) => user,
None => {
return Err(generic_error("Username required", None, None));
}
};

let password = match rpassword::prompt_password("Password: ") {
Ok(p) => p,
Err(_) => {
return Err(generic_error("Password required", None, None));
}
};
pub fn get_username_and_password(
user_flag: Option<String>,
password_flag: Option<String>,
) -> Result<(String, String), ShellError> {
let username = user_flag.map_or_else(
|| {
println!("Please enter username:");
read_input().ok_or_else(|| generic_error("Username required", None, None))
},
Ok,
)?;

let password = password_flag.map_or_else(
|| match rpassword::prompt_password("Password: ") {
Ok(p) => {
if p.is_empty() {
Copy link

Choose a reason for hiding this comment

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

Doesn't look like the original code had this check, is it needed?
I assume there could be cases where we don't want to include a password at all (e.g. when using certs). Does this check get avoided for those cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code didn't have this check as I thought that prompt_password returned an error on am empty password, but it does not.

Even when using certs cbshell needs the username and password to perform any doc ops. Also when we register a cluster through the config file we return an error if there is no username and password, so I think it should be the same when registering a cluster through this command.

The other place this is used is when creating credentials, where we obviously need a username and password. I think if there was something that used wither certs or username/password it'd be the responsibility of the caller to call this func if they require a username and password, and don't if a cert has been supplied.

Copy link

Choose a reason for hiding this comment

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

Thanks for the context, makes sense.

Err(generic_error("Password required", None, None))
} else {
Ok(p)
}
}
Err(e) => Err(generic_error(
format!("Failed to parse password: {}", e),
None,
None,
)),
},
Ok,
)?;

Ok((username, password))
}
Expand Down
Loading