Skip to content

Commit

Permalink
[SM-1174] Access Token Login State by Default in bws (#930)
Browse files Browse the repository at this point in the history
## 🎟️ Tracking

https://bitwarden.atlassian.net/browse/SM-1174

## 📔 Objective

This PR switches `bws` to use access token login state by default. This
will help prevent many users from running into rate limiting issues. The
two related config keys in a profile:

- `state_dir` -> this is the same as it has been; a custom directory for
where state files will be stored
- `state_opt_out` -> this is a new key. If it's `true` or `1`, state
files will not be used

**Note**: the `gnu` build failures are already happening on `main` and
have been brought up internally

## ⏰ Reminders before review

- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
  team

## 🦮 Reviewer guidelines

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- 👍 (`:+1:`) or similar for great changes
- 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info
- ❓ (`:question:`) for questions
- 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry
that's not quite a confirmed
  issue and could potentially benefit from discussion
- 🎨 (`:art:`) for suggestions / improvements
- ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or
concerns needing attention
- 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or
indications of technical debt
- ⛏ (`:pick:`) for minor or nitpick changes
  • Loading branch information
coltonhurst authored Aug 15, 2024
1 parent 8e9e8f1 commit 9e4df97
Show file tree
Hide file tree
Showing 6 changed files with 105 additions and 14 deletions.
3 changes: 2 additions & 1 deletion crates/bws/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub(crate) const PROFILE_KEY_VAR_NAME: &str = "BWS_PROFILE";
pub(crate) const SERVER_URL_KEY_VAR_NAME: &str = "BWS_SERVER_URL";

pub(crate) const DEFAULT_CONFIG_FILENAME: &str = "config";
pub(crate) const DEFAULT_CONFIG_DIRECTORY: &str = ".bws";
pub(crate) const DEFAULT_CONFIG_DIRECTORY: &str = ".config/bws";

#[allow(non_camel_case_types)]
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, ValueEnum, Debug)]
Expand All @@ -20,6 +20,7 @@ pub(crate) enum ProfileKey {
server_api,
server_identity,
state_dir,
state_opt_out,
}

#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, ValueEnum, Debug)]
Expand Down
9 changes: 8 additions & 1 deletion crates/bws/src/command/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use clap::CommandFactory;
use clap_complete::Shell;
use color_eyre::eyre::{bail, Result};

use crate::{config, Cli, ProfileKey};
use crate::{config, util, Cli, ProfileKey};

pub(crate) fn completions(shell: Option<Shell>) -> Result<()> {
let Some(shell) = shell.or_else(Shell::from_env) else {
Expand Down Expand Up @@ -48,6 +48,13 @@ pub(crate) fn config(
(None, None) => bail!("Missing `name` and `value`"),
(None, Some(_)) => bail!("Missing `value`"),
(Some(_), None) => bail!("Missing `name`"),
(Some(ProfileKey::state_opt_out), Some(value)) => {
if util::string_to_bool(value.as_str()).is_err() {
bail!("Profile key \"state_opt_out\" must be \"true\" or \"false\"");
} else {
(ProfileKey::state_opt_out, value)
}
}
(Some(name), Some(value)) => (name, value),
};

Expand Down
3 changes: 3 additions & 0 deletions crates/bws/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub(crate) struct Profile {
pub server_api: Option<String>,
pub server_identity: Option<String>,
pub state_dir: Option<String>,
pub state_opt_out: Option<String>,
}

impl ProfileKey {
Expand All @@ -30,6 +31,7 @@ impl ProfileKey {
ProfileKey::server_api => p.server_api = Some(value),
ProfileKey::server_identity => p.server_identity = Some(value),
ProfileKey::state_dir => p.state_dir = Some(value),
ProfileKey::state_opt_out => p.state_opt_out = Some(value),
}
}
}
Expand Down Expand Up @@ -118,6 +120,7 @@ impl Profile {
server_api: None,
server_identity: None,
state_dir: None,
state_opt_out: None,
})
}
pub(crate) fn api_url(&self) -> Result<String> {
Expand Down
29 changes: 25 additions & 4 deletions crates/bws/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use bitwarden::{
use bitwarden_cli::install_color_eyre;
use clap::{CommandFactory, Parser};
use color_eyre::eyre::{bail, Result};
use config::Profile;
use log::error;
use render::OutputSettings;

Expand All @@ -15,6 +16,7 @@ mod command;
mod config;
mod render;
mod state;
mod util;

use crate::cli::*;

Expand Down Expand Up @@ -84,10 +86,19 @@ async fn process_commands() -> Result<()> {
})
.transpose()?;

let state_file = state::get_state_file(
profile.and_then(|p| p.state_dir).map(Into::into),
access_token_obj.access_token_id.to_string(),
)?;
let state_file = match get_state_opt_out(&profile) {
true => None,
false => match state::get_state_file(
profile.and_then(|p| p.state_dir).map(Into::into),
access_token_obj.access_token_id.to_string(),
) {
Ok(state_file) => Some(state_file),
Err(e) => {
eprintln!("Warning: {}\nRetrieving the state file failed. Attempting to continue without using state. Please set \"state_dir\" in your config file to avoid authentication limits.", e);
None
}
},
};

let client = bitwarden::Client::new(settings);

Expand Down Expand Up @@ -150,3 +161,13 @@ fn get_config_profile(
};
Ok(profile)
}

fn get_state_opt_out(profile: &Option<Profile>) -> bool {
if let Some(profile) = profile {
if let Some(state_opt_out) = &profile.state_opt_out {
return util::string_to_bool(state_opt_out).unwrap_or(false);
}
}

false
}
31 changes: 23 additions & 8 deletions crates/bws/src/state.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,32 @@
use std::path::PathBuf;

use color_eyre::eyre::Result;
use color_eyre::eyre::{bail, Result};
use directories::BaseDirs;

use crate::DEFAULT_CONFIG_DIRECTORY;

pub(crate) const DEFAULT_STATE_DIRECTORY: &str = "state";

pub(crate) fn get_state_file(
state_dir: Option<PathBuf>,
access_token_id: String,
) -> Result<Option<PathBuf>> {
if let Some(mut state_dir) = state_dir {
std::fs::create_dir_all(&state_dir)?;
state_dir.push(access_token_id);
) -> Result<PathBuf> {
let mut state_dir = match state_dir {
Some(state_dir) => state_dir,
None => {
if let Some(base_dirs) = BaseDirs::new() {
base_dirs
.home_dir()
.join(DEFAULT_CONFIG_DIRECTORY)
.join(DEFAULT_STATE_DIRECTORY)
} else {
bail!("A valid home directory doesn't exist");
}
}
};

return Ok(Some(state_dir));
}
std::fs::create_dir_all(&state_dir)?;
state_dir.push(access_token_id);

Ok(None)
Ok(state_dir)
}
44 changes: 44 additions & 0 deletions crates/bws/src/util.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
const STRING_TO_BOOL_ERROR_MESSAGE: &str = "Could not convert string to bool";

pub(crate) fn string_to_bool(value: &str) -> Result<bool, &str> {
match value.trim().to_lowercase().as_str() {
"true" | "1" => Ok(true),
"false" | "0" => Ok(false),
_ => Err(STRING_TO_BOOL_ERROR_MESSAGE),
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_string_to_bool_true_true() {
let result = string_to_bool("true");
assert_eq!(result, Ok(true));
}

#[test]
fn test_string_to_bool_one_true() {
let result = string_to_bool("1");
assert_eq!(result, Ok(true));
}

#[test]
fn test_string_to_bool_false_false() {
let result = string_to_bool("false");
assert_eq!(result, Ok(false));
}

#[test]
fn test_string_to_bool_zero_false() {
let result = string_to_bool("0");
assert_eq!(result, Ok(false));
}

#[test]
fn test_string_to_bool_bad_string_errors() {
let result = string_to_bool("hello world");
assert_eq!(result, Err(STRING_TO_BOOL_ERROR_MESSAGE));
}
}

0 comments on commit 9e4df97

Please sign in to comment.