From 6a5b3c26860afe7b658bace0457af9fe70ee902d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 29 Sep 2023 17:05:24 +0200 Subject: [PATCH 01/14] [PM-3434] Password generator --- .../bitwarden/src/tool/generators/password.rs | 90 +++++++++++++++++-- 1 file changed, 84 insertions(+), 6 deletions(-) diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index 0a3874082..bbdcac393 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -1,4 +1,5 @@ use crate::error::Result; +use rand::seq::SliceRandom; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -21,10 +22,10 @@ pub struct PasswordGeneratorRequest { pub length: Option, pub avoid_ambiguous: Option, // TODO: Should we rename this to include_all_characters? - pub min_lowercase: Option, - pub min_uppercase: Option, - pub min_number: Option, - pub min_special: Option, + pub min_lowercase: Option, + pub min_uppercase: Option, + pub min_number: Option, + pub min_special: Option, } /// Passphrase generator request. @@ -40,8 +41,85 @@ pub struct PassphraseGeneratorRequest { pub include_number: Option, } -pub(super) fn password(_input: PasswordGeneratorRequest) -> Result { - Ok("pa11w0rd".to_string()) +const DEFAULT_PASSWORD_LENGTH: u8 = 16; + +const UPPER_CHARS_AMBIGUOUS: [char; 2] = ['I', 'O']; +const UPPER_CHARS: [char; 24] = [ + 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'J', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'U', + 'V', 'W', 'X', 'Y', 'Z', +]; +const LOWER_CHARS_AMBIGUOUS: [char; 1] = ['l']; +const LOWER_CHARS: [char; 25] = [ + 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', + 'u', 'v', 'w', 'x', 'y', 'z', +]; +const NUMBER_CHARS_AMBIGUOUS: [char; 2] = ['0', '1']; +const NUMBER_CHARS: [char; 8] = ['2', '3', '4', '5', '6', '7', '8', '9']; +const SPECIAL_CHARS: [char; 8] = ['!', '@', '#', '$', '%', '^', '&', '*']; + +pub(super) fn password(input: PasswordGeneratorRequest) -> Result { + // Generate all character dictionaries + fn gen_chars(chars: &[char], ambiguous: &[char], avoid_ambiguous: bool) -> Vec { + let mut chars = chars.to_vec(); + if !avoid_ambiguous { + chars.extend_from_slice(ambiguous); + } + chars + } + let avoid_ambiguous = input.avoid_ambiguous.unwrap_or(false); + let lower_chars = gen_chars(&LOWER_CHARS, &LOWER_CHARS_AMBIGUOUS, avoid_ambiguous); + let upper_chars = gen_chars(&UPPER_CHARS, &UPPER_CHARS_AMBIGUOUS, avoid_ambiguous); + let number_chars = gen_chars(&NUMBER_CHARS, &NUMBER_CHARS_AMBIGUOUS, avoid_ambiguous); + let all_chars = lower_chars + .iter() + .chain(&upper_chars) + .chain(&number_chars) + .chain(&SPECIAL_CHARS) + .collect::>(); + + // We always have to have at least one character type + let lowercase = input.lowercase || (!input.uppercase && !input.numbers && !input.special); + + // Sanitize the minimum values + fn get_minimum(min: Option, enabled: bool) -> u8 { + if enabled { + // Make sure there's at least one + u8::max(min.unwrap_or(1), 1) + } else { + 0 + } + } + let min_lowercase = get_minimum(input.min_lowercase, lowercase); + let min_uppercase = get_minimum(input.min_uppercase, input.uppercase); + let min_number = get_minimum(input.min_number, input.numbers); + let min_special = get_minimum(input.min_special, input.special); + + // Sanitize the length value + let min_length = min_lowercase + min_uppercase + min_number + min_special; + let length = u8::max(input.length.unwrap_or(DEFAULT_PASSWORD_LENGTH), min_length); + + // Generate the minimum chars of each type, then generate the rest to fill the expected length + let mut chars = Vec::with_capacity(length as usize); + let mut rand = rand::thread_rng(); + + for _ in 0..min_lowercase { + chars.push(*lower_chars.choose(&mut rand).expect("slice is not empty")); + } + for _ in 0..min_uppercase { + chars.push(*upper_chars.choose(&mut rand).expect("slice is not empty")); + } + for _ in 0..min_number { + chars.push(*number_chars.choose(&mut rand).expect("slice is not empty")); + } + for _ in 0..min_special { + chars.push(*SPECIAL_CHARS.choose(&mut rand).expect("slice is not empty")); + } + for _ in min_length..length { + chars.push(**all_chars.choose(&mut rand).expect("slice is not empty")); + } + + chars.shuffle(&mut rand); + Ok(chars.iter().collect()) } pub(super) fn passphrase(_input: PassphraseGeneratorRequest) -> Result { From f02164710fd4108db245b62a074b626ec204920f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 2 Oct 2023 13:53:59 +0200 Subject: [PATCH 02/14] Extract charset logic to a separate struct and add tests --- .../bitwarden/src/tool/generators/password.rs | 181 ++++++++++++++---- 1 file changed, 139 insertions(+), 42 deletions(-) diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index bbdcac393..605d040e2 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -1,4 +1,4 @@ -use crate::error::Result; +use crate::error::{Error, Result}; use rand::seq::SliceRandom; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -43,85 +43,182 @@ pub struct PassphraseGeneratorRequest { const DEFAULT_PASSWORD_LENGTH: u8 = 16; -const UPPER_CHARS_AMBIGUOUS: [char; 2] = ['I', 'O']; -const UPPER_CHARS: [char; 24] = [ +const UPPER_CHARS_AMBIGUOUS: &[char] = &['I', 'O']; +const UPPER_CHARS: &[char] = &[ 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'J', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', ]; -const LOWER_CHARS_AMBIGUOUS: [char; 1] = ['l']; -const LOWER_CHARS: [char; 25] = [ +const LOWER_CHARS_AMBIGUOUS: &[char] = &['l']; +const LOWER_CHARS: &[char] = &[ 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', ]; -const NUMBER_CHARS_AMBIGUOUS: [char; 2] = ['0', '1']; -const NUMBER_CHARS: [char; 8] = ['2', '3', '4', '5', '6', '7', '8', '9']; -const SPECIAL_CHARS: [char; 8] = ['!', '@', '#', '$', '%', '^', '&', '*']; +const NUMBER_CHARS_AMBIGUOUS: &[char] = &['0', '1']; +const NUMBER_CHARS: &[char] = &['2', '3', '4', '5', '6', '7', '8', '9']; +const SPECIAL_CHARS: &[char] = &['!', '@', '#', '$', '%', '^', '&', '*']; -pub(super) fn password(input: PasswordGeneratorRequest) -> Result { - // Generate all character dictionaries - fn gen_chars(chars: &[char], ambiguous: &[char], avoid_ambiguous: bool) -> Vec { - let mut chars = chars.to_vec(); - if !avoid_ambiguous { - chars.extend_from_slice(ambiguous); +struct PasswordGeneratorCharSet { + lower: Vec, + upper: Vec, + number: Vec, + special: Vec, + all: Vec, +} + +impl PasswordGeneratorCharSet { + fn new(lower: bool, upper: bool, number: bool, special: bool, avoid_ambiguous: bool) -> Self { + fn chars( + enabled: bool, + chars: &[char], + ambiguous: &[char], + avoid_ambiguous: bool, + ) -> Vec { + if !enabled { + return Vec::new(); + } + let mut chars = chars.to_vec(); + if !avoid_ambiguous { + chars.extend_from_slice(ambiguous); + } + chars + } + let lower = chars(lower, LOWER_CHARS, LOWER_CHARS_AMBIGUOUS, avoid_ambiguous); + let upper = chars(upper, UPPER_CHARS, UPPER_CHARS_AMBIGUOUS, avoid_ambiguous); + let number = chars( + number, + NUMBER_CHARS, + NUMBER_CHARS_AMBIGUOUS, + avoid_ambiguous, + ); + let special = chars(special, SPECIAL_CHARS, &[], avoid_ambiguous); + let all = lower + .iter() + .chain(&upper) + .chain(&number) + .chain(&special) + .copied() + .collect(); + + Self { + lower, + upper, + number, + special, + all, } - chars } - let avoid_ambiguous = input.avoid_ambiguous.unwrap_or(false); - let lower_chars = gen_chars(&LOWER_CHARS, &LOWER_CHARS_AMBIGUOUS, avoid_ambiguous); - let upper_chars = gen_chars(&UPPER_CHARS, &UPPER_CHARS_AMBIGUOUS, avoid_ambiguous); - let number_chars = gen_chars(&NUMBER_CHARS, &NUMBER_CHARS_AMBIGUOUS, avoid_ambiguous); - let all_chars = lower_chars - .iter() - .chain(&upper_chars) - .chain(&number_chars) - .chain(&SPECIAL_CHARS) - .collect::>(); - - // We always have to have at least one character type - let lowercase = input.lowercase || (!input.uppercase && !input.numbers && !input.special); - - // Sanitize the minimum values +} + +pub(super) fn password(input: PasswordGeneratorRequest) -> Result { + // We always have to have at least one character set enabled + if !input.lowercase || !input.uppercase && !input.numbers && !input.special { + return Err(Error::Internal( + "At least one character set must be enabled", + )); + } + + // Generate all character dictionaries + let chars = PasswordGeneratorCharSet::new( + input.lowercase, + input.uppercase, + input.numbers, + input.special, + input.avoid_ambiguous.unwrap_or(false), + ); + + // Make sure the minimum values are zero when the character + // set is disabled, and at least one when it's enabled fn get_minimum(min: Option, enabled: bool) -> u8 { if enabled { - // Make sure there's at least one u8::max(min.unwrap_or(1), 1) } else { 0 } } - let min_lowercase = get_minimum(input.min_lowercase, lowercase); + let min_lowercase = get_minimum(input.min_lowercase, input.lowercase); let min_uppercase = get_minimum(input.min_uppercase, input.uppercase); let min_number = get_minimum(input.min_number, input.numbers); let min_special = get_minimum(input.min_special, input.special); - // Sanitize the length value + // Check that the minimum lengths aren't larger than the password length let min_length = min_lowercase + min_uppercase + min_number + min_special; - let length = u8::max(input.length.unwrap_or(DEFAULT_PASSWORD_LENGTH), min_length); + let length = input.length.unwrap_or(DEFAULT_PASSWORD_LENGTH); + if min_length > length { + return Err(Error::Internal( + "Password length can't be less than the sum of the minimums", + )); + } // Generate the minimum chars of each type, then generate the rest to fill the expected length - let mut chars = Vec::with_capacity(length as usize); + let mut buf = Vec::with_capacity(length as usize); let mut rand = rand::thread_rng(); for _ in 0..min_lowercase { - chars.push(*lower_chars.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.lower.choose(&mut rand).expect("slice is not empty")); } for _ in 0..min_uppercase { - chars.push(*upper_chars.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.upper.choose(&mut rand).expect("slice is not empty")); } for _ in 0..min_number { - chars.push(*number_chars.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.number.choose(&mut rand).expect("slice is not empty")); } for _ in 0..min_special { - chars.push(*SPECIAL_CHARS.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.special.choose(&mut rand).expect("slice is not empty")); } for _ in min_length..length { - chars.push(**all_chars.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.all.choose(&mut rand).expect("slice is not empty")); } - chars.shuffle(&mut rand); - Ok(chars.iter().collect()) + buf.shuffle(&mut rand); + Ok(buf.iter().collect()) } pub(super) fn passphrase(_input: PassphraseGeneratorRequest) -> Result { Ok("correct-horse-battery-staple".to_string()) } + +#[cfg(test)] +mod test { + use super::*; + + // We convert the slices to Strings to be able to use `contains` + // This wouldn't work if the character sets were ordered differently, but that's not the case for us + fn to_string(chars: &[char]) -> String { + chars.iter().collect() + } + + #[test] + fn test_password_characters() { + // All characters excluding ambiguous + let set = PasswordGeneratorCharSet::new(true, true, true, true, true); + assert_eq!(set.lower, LOWER_CHARS); + assert_eq!(set.upper, UPPER_CHARS); + assert_eq!(set.number, NUMBER_CHARS); + assert_eq!(set.special, SPECIAL_CHARS); + + // All characters including ambiguous + let set = PasswordGeneratorCharSet::new(true, true, true, true, false); + assert!(to_string(&set.lower).contains(&to_string(LOWER_CHARS))); + assert!(to_string(&set.lower).contains(&to_string(LOWER_CHARS_AMBIGUOUS))); + assert!(to_string(&set.upper).contains(&to_string(UPPER_CHARS))); + assert!(to_string(&set.upper).contains(&to_string(UPPER_CHARS_AMBIGUOUS))); + assert!(to_string(&set.number).contains(&to_string(NUMBER_CHARS))); + assert!(to_string(&set.number).contains(&to_string(NUMBER_CHARS_AMBIGUOUS))); + assert_eq!(set.special, SPECIAL_CHARS); + + // Only lowercase + let set = PasswordGeneratorCharSet::new(true, false, false, false, true); + assert_eq!(set.lower, LOWER_CHARS); + assert_eq!(set.upper, Vec::new()); + assert_eq!(set.number, Vec::new()); + assert_eq!(set.special, Vec::new()); + + // Only uppercase including ambiguous + let set = PasswordGeneratorCharSet::new(false, true, false, false, false); + assert_eq!(set.lower, Vec::new()); + assert!(to_string(&set.upper).contains(&to_string(UPPER_CHARS))); + assert!(to_string(&set.upper).contains(&to_string(UPPER_CHARS_AMBIGUOUS))); + assert_eq!(set.number, Vec::new()); + assert_eq!(set.special, Vec::new()); + } +} From 0e9d04bb1ffc1cdca948a2ecc9e00aacfdf8ad57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 2 Oct 2023 13:57:16 +0200 Subject: [PATCH 03/14] Fix condition --- crates/bitwarden/src/tool/generators/password.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index 605d040e2..517f57099 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -111,7 +111,7 @@ impl PasswordGeneratorCharSet { pub(super) fn password(input: PasswordGeneratorRequest) -> Result { // We always have to have at least one character set enabled - if !input.lowercase || !input.uppercase && !input.numbers && !input.special { + if !input.lowercase && !input.uppercase && !input.numbers && !input.special { return Err(Error::Internal( "At least one character set must be enabled", )); From 4b2cc9be6a24b8e8ef15ee886b6516e448a31826 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 2 Oct 2023 14:20:23 +0200 Subject: [PATCH 04/14] Test password generation and split tests into functions --- .../bitwarden/src/tool/generators/password.rs | 77 +++++++++++++++++-- 1 file changed, 70 insertions(+), 7 deletions(-) diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index 517f57099..ebe2f3bf9 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -187,16 +187,20 @@ mod test { chars.iter().collect() } + fn count(chars: &[char], pass: &str) -> usize { + pass.chars().filter(|c| chars.contains(c)).count() + } + #[test] - fn test_password_characters() { - // All characters excluding ambiguous + fn test_password_characters_all() { let set = PasswordGeneratorCharSet::new(true, true, true, true, true); assert_eq!(set.lower, LOWER_CHARS); assert_eq!(set.upper, UPPER_CHARS); assert_eq!(set.number, NUMBER_CHARS); assert_eq!(set.special, SPECIAL_CHARS); - - // All characters including ambiguous + } + #[test] + fn test_password_characters_all_ambiguous() { let set = PasswordGeneratorCharSet::new(true, true, true, true, false); assert!(to_string(&set.lower).contains(&to_string(LOWER_CHARS))); assert!(to_string(&set.lower).contains(&to_string(LOWER_CHARS_AMBIGUOUS))); @@ -205,14 +209,17 @@ mod test { assert!(to_string(&set.number).contains(&to_string(NUMBER_CHARS))); assert!(to_string(&set.number).contains(&to_string(NUMBER_CHARS_AMBIGUOUS))); assert_eq!(set.special, SPECIAL_CHARS); - - // Only lowercase + } + #[test] + fn test_password_characters_lower() { let set = PasswordGeneratorCharSet::new(true, false, false, false, true); assert_eq!(set.lower, LOWER_CHARS); assert_eq!(set.upper, Vec::new()); assert_eq!(set.number, Vec::new()); assert_eq!(set.special, Vec::new()); - + } + #[test] + fn test_password_characters_upper_ambiguous() { // Only uppercase including ambiguous let set = PasswordGeneratorCharSet::new(false, true, false, false, false); assert_eq!(set.lower, Vec::new()); @@ -221,4 +228,60 @@ mod test { assert_eq!(set.number, Vec::new()); assert_eq!(set.special, Vec::new()); } + + #[test] + fn test_password_gen_all_characters() { + let pass = password(PasswordGeneratorRequest { + lowercase: true, + uppercase: true, + numbers: true, + special: true, + length: Some(100), + avoid_ambiguous: Some(true), + min_lowercase: Some(1), + min_uppercase: Some(1), + min_number: Some(1), + min_special: Some(1), + }) + .unwrap(); + + assert_eq!(pass.len(), 100); + + assert!(count(LOWER_CHARS, &pass) > 1); + assert!(count(UPPER_CHARS, &pass) > 1); + assert!(count(NUMBER_CHARS, &pass) > 1); + assert!(count(SPECIAL_CHARS, &pass) > 1); + + assert_eq!(count(LOWER_CHARS_AMBIGUOUS, &pass), 0); + assert_eq!(count(UPPER_CHARS_AMBIGUOUS, &pass), 0); + assert_eq!(count(NUMBER_CHARS_AMBIGUOUS, &pass), 0); + } + + #[test] + fn test_password_gen_some_characters() { + let pass = password(PasswordGeneratorRequest { + lowercase: true, + uppercase: false, + numbers: false, + special: true, + length: Some(10), + avoid_ambiguous: Some(true), + min_lowercase: Some(9), + min_uppercase: None, + min_number: None, + min_special: Some(1), + }) + .unwrap(); + + assert_eq!(pass.len(), 10); + + assert_eq!(count(LOWER_CHARS, &pass), 9); + assert_eq!(count(UPPER_CHARS, &pass), 0); + assert_eq!(count(NUMBER_CHARS, &pass), 0); + assert_eq!(count(SPECIAL_CHARS, &pass), 1); + + assert_eq!(count(LOWER_CHARS_AMBIGUOUS, &pass), 0); + assert_eq!(count(UPPER_CHARS_AMBIGUOUS, &pass), 0); + assert_eq!(count(NUMBER_CHARS_AMBIGUOUS, &pass), 0); + } } From ac5a453dfe8c81af60efd115fc728e5bc47e0244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 10 Oct 2023 17:18:54 +0200 Subject: [PATCH 05/14] Test password --- Cargo.lock | 1 + crates/bitwarden/Cargo.toml | 1 + .../bitwarden/src/tool/generators/password.rs | 124 +++++++++--------- 3 files changed, 65 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 814fe5318..4e65a50dd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -330,6 +330,7 @@ dependencies = [ "num-traits", "pbkdf2", "rand 0.8.5", + "rand_chacha 0.3.1", "reqwest", "rsa", "schemars", diff --git a/crates/bitwarden/Cargo.toml b/crates/bitwarden/Cargo.toml index 345b1f9cb..1f89c7aae 100644 --- a/crates/bitwarden/Cargo.toml +++ b/crates/bitwarden/Cargo.toml @@ -59,5 +59,6 @@ bitwarden-api-identity = { path = "../bitwarden-api-identity", version = "=0.2.1 bitwarden-api-api = { path = "../bitwarden-api-api", version = "=0.2.1" } [dev-dependencies] +rand_chacha = "0.3.1" tokio = { version = "1.28.2", features = ["rt", "macros"] } wiremock = "0.5.18" diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index ebe2f3bf9..3698e6ec8 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -1,5 +1,5 @@ use crate::error::{Error, Result}; -use rand::seq::SliceRandom; +use rand::{seq::SliceRandom, RngCore}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -110,6 +110,13 @@ impl PasswordGeneratorCharSet { } pub(super) fn password(input: PasswordGeneratorRequest) -> Result { + password_with_rng(rand::thread_rng(), input) +} + +pub(super) fn password_with_rng( + mut rng: impl RngCore, + input: PasswordGeneratorRequest, +) -> Result { // We always have to have at least one character set enabled if !input.lowercase && !input.uppercase && !input.numbers && !input.special { return Err(Error::Internal( @@ -151,25 +158,24 @@ pub(super) fn password(input: PasswordGeneratorRequest) -> Result { // Generate the minimum chars of each type, then generate the rest to fill the expected length let mut buf = Vec::with_capacity(length as usize); - let mut rand = rand::thread_rng(); for _ in 0..min_lowercase { - buf.push(*chars.lower.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.lower.choose(&mut rng).expect("slice is not empty")); } for _ in 0..min_uppercase { - buf.push(*chars.upper.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.upper.choose(&mut rng).expect("slice is not empty")); } for _ in 0..min_number { - buf.push(*chars.number.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.number.choose(&mut rng).expect("slice is not empty")); } for _ in 0..min_special { - buf.push(*chars.special.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.special.choose(&mut rng).expect("slice is not empty")); } for _ in min_length..length { - buf.push(*chars.all.choose(&mut rand).expect("slice is not empty")); + buf.push(*chars.all.choose(&mut rng).expect("slice is not empty")); } - buf.shuffle(&mut rand); + buf.shuffle(&mut rng); Ok(buf.iter().collect()) } @@ -179,6 +185,8 @@ pub(super) fn passphrase(_input: PassphraseGeneratorRequest) -> Result { #[cfg(test)] mod test { + use rand::SeedableRng; + use super::*; // We convert the slices to Strings to be able to use `contains` @@ -187,10 +195,6 @@ mod test { chars.iter().collect() } - fn count(chars: &[char], pass: &str) -> usize { - pass.chars().filter(|c| chars.contains(c)).count() - } - #[test] fn test_password_characters_all() { let set = PasswordGeneratorCharSet::new(true, true, true, true, true); @@ -230,58 +234,56 @@ mod test { } #[test] - fn test_password_gen_all_characters() { - let pass = password(PasswordGeneratorRequest { - lowercase: true, - uppercase: true, - numbers: true, - special: true, - length: Some(100), - avoid_ambiguous: Some(true), - min_lowercase: Some(1), - min_uppercase: Some(1), - min_number: Some(1), - min_special: Some(1), - }) - .unwrap(); - - assert_eq!(pass.len(), 100); + fn test_password_gen() { + let mut rng = rand_chacha::ChaCha8Rng::from_seed([0u8; 32]); - assert!(count(LOWER_CHARS, &pass) > 1); - assert!(count(UPPER_CHARS, &pass) > 1); - assert!(count(NUMBER_CHARS, &pass) > 1); - assert!(count(SPECIAL_CHARS, &pass) > 1); - - assert_eq!(count(LOWER_CHARS_AMBIGUOUS, &pass), 0); - assert_eq!(count(UPPER_CHARS_AMBIGUOUS, &pass), 0); - assert_eq!(count(NUMBER_CHARS_AMBIGUOUS, &pass), 0); - } - - #[test] - fn test_password_gen_some_characters() { - let pass = password(PasswordGeneratorRequest { - lowercase: true, - uppercase: false, - numbers: false, - special: true, - length: Some(10), - avoid_ambiguous: Some(true), - min_lowercase: Some(9), - min_uppercase: None, - min_number: None, - min_special: Some(1), - }) + let pass = password_with_rng( + &mut rng, + PasswordGeneratorRequest { + lowercase: true, + uppercase: true, + numbers: true, + special: true, + ..Default::default() + }, + ) .unwrap(); + assert_eq!(pass, "xfZPr&wXCiFta8DM"); - assert_eq!(pass.len(), 10); - - assert_eq!(count(LOWER_CHARS, &pass), 9); - assert_eq!(count(UPPER_CHARS, &pass), 0); - assert_eq!(count(NUMBER_CHARS, &pass), 0); - assert_eq!(count(SPECIAL_CHARS, &pass), 1); + let pass = password_with_rng( + &mut rng, + PasswordGeneratorRequest { + lowercase: true, + uppercase: true, + numbers: false, + special: false, + length: Some(20), + avoid_ambiguous: Some(false), + min_lowercase: Some(1), + min_uppercase: Some(1), + min_number: None, + min_special: None, + }, + ) + .unwrap(); + assert_eq!(pass, "jvpFStaIdRUoENAeTmJw"); - assert_eq!(count(LOWER_CHARS_AMBIGUOUS, &pass), 0); - assert_eq!(count(UPPER_CHARS_AMBIGUOUS, &pass), 0); - assert_eq!(count(NUMBER_CHARS_AMBIGUOUS, &pass), 0); + let pass = password_with_rng( + &mut rng, + PasswordGeneratorRequest { + lowercase: false, + uppercase: false, + numbers: true, + special: true, + length: Some(5), + avoid_ambiguous: Some(true), + min_lowercase: None, + min_uppercase: None, + min_number: Some(3), + min_special: Some(2), + }, + ) + .unwrap(); + assert_eq!(pass, "^878%"); } } From e19ec3180215ef2db993d89ab86a3bd65e4b0347 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Wed, 18 Oct 2023 18:41:20 +0200 Subject: [PATCH 06/14] Add API documentation and switch to HashSet in tests --- .../src/tool/generators/client_generator.rs | 24 +++++++ .../bitwarden/src/tool/generators/password.rs | 69 +++++++++++++++---- 2 files changed, 79 insertions(+), 14 deletions(-) diff --git a/crates/bitwarden/src/tool/generators/client_generator.rs b/crates/bitwarden/src/tool/generators/client_generator.rs index 0384eec6a..e74a408dd 100644 --- a/crates/bitwarden/src/tool/generators/client_generator.rs +++ b/crates/bitwarden/src/tool/generators/client_generator.rs @@ -11,6 +11,30 @@ pub struct ClientGenerator<'a> { } impl<'a> ClientGenerator<'a> { + /// Generates a random password. + /// A passphrase is a combination of random words separated by a character. + /// An example of passphrase is `correct horse battery staple`. + /// + /// By default, the password contains lowercase 16 characters, but the character + /// sets and password length can be customized using the `input` parameter. + /// + /// # Examples + /// + /// ``` + /// use bitwarden::{Client, tool::PasswordGeneratorRequest, error::Result}; + /// async fn test() -> Result<()> { + /// let input = PasswordGeneratorRequest { + /// lowercase: true, + /// uppercase: true, + /// numbers: true, + /// length: Some(20), + /// ..Default::default() + /// }; + /// let password = Client::new(None).generator().password(input).await.unwrap(); + /// println!("{}", password); + /// Ok(()) + /// } + /// ``` pub async fn password(&self, input: PasswordGeneratorRequest) -> Result { password(input) } diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index 3698e6ec8..08ade786c 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -3,31 +3,69 @@ use rand::{seq::SliceRandom, RngCore}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -/// Password generator request. If all options are false, the default is to +/// Password generator request options. If all options are false, the default is to /// generate a password with: /// - lowercase /// - uppercase /// - numbers /// /// The default length is 16. -#[derive(Serialize, Deserialize, Debug, JsonSchema, Default)] +#[derive(Serialize, Deserialize, Debug, JsonSchema)] #[serde(rename_all = "camelCase", deny_unknown_fields)] #[cfg_attr(feature = "mobile", derive(uniffi::Record))] pub struct PasswordGeneratorRequest { + /// When set to true, the generated password will contain lowercase characters (a-z). pub lowercase: bool, + /// When set to true, the generated password will contain uppercase characters (A-Z). pub uppercase: bool, + /// When set to true, the generated password will contain numbers (0-9). pub numbers: bool, + /// When set to true, the generated password will contain special characters. + /// The supported characters are: ! @ # $ % ^ & * pub special: bool, + /// The length of the generated password. + /// Note that the password length must be greater than the sum of all the minimums. + /// The default value when unset is 16. pub length: Option, + /// When set to true, the generated password will not contain ambiguous characters. + /// The ambiguous characters are: I, O, l, 0, 1 pub avoid_ambiguous: Option, // TODO: Should we rename this to include_all_characters? + + /// The minimum number of lowercase characters in the generated password. + /// When set, the value must be between 1 and 9. This value is ignored is lowercase is false pub min_lowercase: Option, + /// The minimum number of uppercase characters in the generated password. + /// When set, the value must be between 1 and 9. This value is ignored is uppercase is false pub min_uppercase: Option, + /// The minimum number of numbers in the generated password. + /// When set, the value must be between 1 and 9. This value is ignored is numbers is false pub min_number: Option, + /// The minimum number of special characters in the generated password. + /// When set, the value must be between 1 and 9. This value is ignored is special is false pub min_special: Option, } +// We need to implement this manually so we can set one character set to true. +// Otherwise the default implementation will fail to generate a password. +impl Default for PasswordGeneratorRequest { + fn default() -> Self { + Self { + lowercase: true, + uppercase: false, + numbers: false, + special: false, + length: None, + avoid_ambiguous: None, + min_lowercase: None, + min_uppercase: None, + min_number: None, + min_special: None, + } + } +} + /// Passphrase generator request. /// /// The default separator is `-` and default number of words is 3. @@ -109,6 +147,8 @@ impl PasswordGeneratorCharSet { } } +/// Implementation of the random password generator. This is not accessible to the public API. +/// See [`ClientGenerator::password`](crate::ClientGenerator::password) for the API function. pub(super) fn password(input: PasswordGeneratorRequest) -> Result { password_with_rng(rand::thread_rng(), input) } @@ -185,14 +225,15 @@ pub(super) fn passphrase(_input: PassphraseGeneratorRequest) -> Result { #[cfg(test)] mod test { + use std::collections::HashSet; + use rand::SeedableRng; use super::*; - // We convert the slices to Strings to be able to use `contains` - // This wouldn't work if the character sets were ordered differently, but that's not the case for us - fn to_string(chars: &[char]) -> String { - chars.iter().collect() + // We convert the slices to HashSets to be able to use `is_subset` + fn to_set(chars: &[char]) -> HashSet { + chars.iter().copied().collect() } #[test] @@ -206,12 +247,12 @@ mod test { #[test] fn test_password_characters_all_ambiguous() { let set = PasswordGeneratorCharSet::new(true, true, true, true, false); - assert!(to_string(&set.lower).contains(&to_string(LOWER_CHARS))); - assert!(to_string(&set.lower).contains(&to_string(LOWER_CHARS_AMBIGUOUS))); - assert!(to_string(&set.upper).contains(&to_string(UPPER_CHARS))); - assert!(to_string(&set.upper).contains(&to_string(UPPER_CHARS_AMBIGUOUS))); - assert!(to_string(&set.number).contains(&to_string(NUMBER_CHARS))); - assert!(to_string(&set.number).contains(&to_string(NUMBER_CHARS_AMBIGUOUS))); + assert!(to_set(&set.lower).is_superset(&to_set(LOWER_CHARS))); + assert!(to_set(&set.lower).is_superset(&to_set(LOWER_CHARS_AMBIGUOUS))); + assert!(to_set(&set.upper).is_superset(&to_set(UPPER_CHARS))); + assert!(to_set(&set.upper).is_superset(&to_set(UPPER_CHARS_AMBIGUOUS))); + assert!(to_set(&set.number).is_superset(&to_set(NUMBER_CHARS))); + assert!(to_set(&set.number).is_superset(&to_set(NUMBER_CHARS_AMBIGUOUS))); assert_eq!(set.special, SPECIAL_CHARS); } #[test] @@ -227,8 +268,8 @@ mod test { // Only uppercase including ambiguous let set = PasswordGeneratorCharSet::new(false, true, false, false, false); assert_eq!(set.lower, Vec::new()); - assert!(to_string(&set.upper).contains(&to_string(UPPER_CHARS))); - assert!(to_string(&set.upper).contains(&to_string(UPPER_CHARS_AMBIGUOUS))); + assert!(to_set(&set.upper).is_superset(&to_set(UPPER_CHARS))); + assert!(to_set(&set.upper).is_superset(&to_set(UPPER_CHARS_AMBIGUOUS))); assert_eq!(set.number, Vec::new()); assert_eq!(set.special, Vec::new()); } From 7a7a269a0c9fcde23c9b6f3f57983bc8f03874e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 6 Nov 2023 13:13:45 +0100 Subject: [PATCH 07/14] Test formatting --- crates/bitwarden/src/tool/generators/password.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index 08ade786c..cea7adab4 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -244,6 +244,7 @@ mod test { assert_eq!(set.number, NUMBER_CHARS); assert_eq!(set.special, SPECIAL_CHARS); } + #[test] fn test_password_characters_all_ambiguous() { let set = PasswordGeneratorCharSet::new(true, true, true, true, false); @@ -255,6 +256,7 @@ mod test { assert!(to_set(&set.number).is_superset(&to_set(NUMBER_CHARS_AMBIGUOUS))); assert_eq!(set.special, SPECIAL_CHARS); } + #[test] fn test_password_characters_lower() { let set = PasswordGeneratorCharSet::new(true, false, false, false, true); @@ -263,6 +265,7 @@ mod test { assert_eq!(set.number, Vec::new()); assert_eq!(set.special, Vec::new()); } + #[test] fn test_password_characters_upper_ambiguous() { // Only uppercase including ambiguous From f758ada19af759ef10f6c5b23ba75bfe7f8e8d28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 6 Nov 2023 15:42:58 +0100 Subject: [PATCH 08/14] Make password infallible by moving validation outside of it, moved CharSets to BTreeSets and implemented Distribution on them --- .../src/tool/generators/client_generator.rs | 5 +- .../bitwarden/src/tool/generators/password.rs | 475 ++++++++++-------- crates/bw/src/main.rs | 2 +- 3 files changed, 283 insertions(+), 199 deletions(-) diff --git a/crates/bitwarden/src/tool/generators/client_generator.rs b/crates/bitwarden/src/tool/generators/client_generator.rs index e74a408dd..fcfb39a71 100644 --- a/crates/bitwarden/src/tool/generators/client_generator.rs +++ b/crates/bitwarden/src/tool/generators/client_generator.rs @@ -27,7 +27,7 @@ impl<'a> ClientGenerator<'a> { /// lowercase: true, /// uppercase: true, /// numbers: true, - /// length: Some(20), + /// length: 20, /// ..Default::default() /// }; /// let password = Client::new(None).generator().password(input).await.unwrap(); @@ -36,7 +36,8 @@ impl<'a> ClientGenerator<'a> { /// } /// ``` pub async fn password(&self, input: PasswordGeneratorRequest) -> Result { - password(input) + let charset = input.validate_options()?; + Ok(password(charset)) } pub async fn passphrase(&self, input: PassphraseGeneratorRequest) -> Result { diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index cea7adab4..24752f704 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -1,5 +1,5 @@ use crate::error::{Error, Result}; -use rand::{seq::SliceRandom, RngCore}; +use rand::{distributions::Distribution, seq::SliceRandom, RngCore}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -26,12 +26,11 @@ pub struct PasswordGeneratorRequest { /// The length of the generated password. /// Note that the password length must be greater than the sum of all the minimums. - /// The default value when unset is 16. - pub length: Option, + pub length: u8, /// When set to true, the generated password will not contain ambiguous characters. /// The ambiguous characters are: I, O, l, 0, 1 - pub avoid_ambiguous: Option, // TODO: Should we rename this to include_all_characters? + pub avoid_ambiguous: bool, // TODO: Should we rename this to include_all_characters? /// The minimum number of lowercase characters in the generated password. /// When set, the value must be between 1 and 9. This value is ignored is lowercase is false @@ -47,6 +46,8 @@ pub struct PasswordGeneratorRequest { pub min_special: Option, } +const DEFAULT_PASSWORD_LENGTH: u8 = 16; + // We need to implement this manually so we can set one character set to true. // Otherwise the default implementation will fail to generate a password. impl Default for PasswordGeneratorRequest { @@ -56,8 +57,8 @@ impl Default for PasswordGeneratorRequest { uppercase: false, numbers: false, special: false, - length: None, - avoid_ambiguous: None, + length: DEFAULT_PASSWORD_LENGTH, + avoid_ambiguous: false, min_lowercase: None, min_uppercase: None, min_number: None, @@ -79,144 +80,182 @@ pub struct PassphraseGeneratorRequest { pub include_number: Option, } -const DEFAULT_PASSWORD_LENGTH: u8 = 16; - const UPPER_CHARS_AMBIGUOUS: &[char] = &['I', 'O']; -const UPPER_CHARS: &[char] = &[ - 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'J', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'U', - 'V', 'W', 'X', 'Y', 'Z', -]; const LOWER_CHARS_AMBIGUOUS: &[char] = &['l']; -const LOWER_CHARS: &[char] = &[ - 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', - 'u', 'v', 'w', 'x', 'y', 'z', -]; const NUMBER_CHARS_AMBIGUOUS: &[char] = &['0', '1']; -const NUMBER_CHARS: &[char] = &['2', '3', '4', '5', '6', '7', '8', '9']; const SPECIAL_CHARS: &[char] = &['!', '@', '#', '$', '%', '^', '&', '*']; -struct PasswordGeneratorCharSet { - lower: Vec, - upper: Vec, - number: Vec, - special: Vec, - all: Vec, -} +// We don't want the validated struct to be accessible, yet at the same time it needs to be public +// to be used as a return type, so we define it in a private module to make it innaccessible. +mod private { + use std::collections::BTreeSet; + + use rand::prelude::Distribution; -impl PasswordGeneratorCharSet { - fn new(lower: bool, upper: bool, number: bool, special: bool, avoid_ambiguous: bool) -> Self { - fn chars( - enabled: bool, - chars: &[char], - ambiguous: &[char], - avoid_ambiguous: bool, - ) -> Vec { - if !enabled { - return Vec::new(); + // Note: We are using a BTreeSet to have consistent ordering between runs. This is not + // important during normal execution, but it's necessary for the tests to be repeatable. + #[derive(Clone, Default)] + pub struct CharSet(pub(super) BTreeSet); + impl CharSet { + pub fn include(self, other: impl IntoIterator) -> Self { + self.include_if(true, other) + } + pub fn include_if( + mut self, + predicate: bool, + other: impl IntoIterator, + ) -> Self { + if predicate { + self.0.extend(other); } - let mut chars = chars.to_vec(); - if !avoid_ambiguous { - chars.extend_from_slice(ambiguous); + self + } + pub fn exclude_if<'a>( + self, + predicate: bool, + other: impl IntoIterator, + ) -> Self { + if predicate { + let other: BTreeSet<_> = other.into_iter().copied().collect(); + Self(self.0.difference(&other).copied().collect()) + } else { + self } - chars } - let lower = chars(lower, LOWER_CHARS, LOWER_CHARS_AMBIGUOUS, avoid_ambiguous); - let upper = chars(upper, UPPER_CHARS, UPPER_CHARS_AMBIGUOUS, avoid_ambiguous); - let number = chars( - number, - NUMBER_CHARS, - NUMBER_CHARS_AMBIGUOUS, - avoid_ambiguous, + } + impl<'a> IntoIterator for &'a CharSet { + type Item = char; + type IntoIter = std::iter::Copied>; + fn into_iter(self) -> Self::IntoIter { + self.0.iter().copied() + } + } + impl Distribution for CharSet { + fn sample(&self, rng: &mut R) -> char { + let idx = rng.gen_range(0..self.0.len()); + *self.0.iter().nth(idx).expect("Valid index") + } + } + + pub struct PasswordGeneratorOptions { + pub(super) lower: (CharSet, usize), + pub(super) upper: (CharSet, usize), + pub(super) number: (CharSet, usize), + pub(super) special: (CharSet, usize), + pub(super) all: (CharSet, usize), + + pub(super) length: usize, + } +} +use private::{CharSet, PasswordGeneratorOptions}; + +impl PasswordGeneratorRequest { + // TODO: Add password generator policy checks + pub fn validate_options(self) -> Result { + // We always have to have at least one character set enabled + if !self.lowercase && !self.uppercase && !self.numbers && !self.special { + return Err(Error::Internal( + "At least one character set must be enabled", + )); + } + + // Make sure the minimum values are zero when the character + // set is disabled, and at least one when it's enabled + fn get_minimum(min: Option, enabled: bool) -> usize { + if enabled { + usize::max(min.unwrap_or(1) as usize, 1) + } else { + 0 + } + } + + let length = self.length as usize; + let min_lowercase = get_minimum(self.min_lowercase, self.lowercase); + let min_uppercase = get_minimum(self.min_uppercase, self.uppercase); + let min_number = get_minimum(self.min_number, self.numbers); + let min_special = get_minimum(self.min_special, self.special); + + // Check that the minimum lengths aren't larger than the password length + let minimum_length = min_lowercase + min_uppercase + min_number + min_special; + if minimum_length > length { + return Err(Error::Internal( + "Password length can't be less than the sum of the minimums", + )); + } + + let lower = ( + CharSet::default() + .include_if(self.lowercase, 'a'..='z') + .exclude_if(self.avoid_ambiguous, LOWER_CHARS_AMBIGUOUS), + min_lowercase, ); - let special = chars(special, SPECIAL_CHARS, &[], avoid_ambiguous); - let all = lower - .iter() - .chain(&upper) - .chain(&number) - .chain(&special) - .copied() - .collect(); - Self { + let upper = ( + CharSet::default() + .include_if(self.uppercase, 'A'..='Z') + .exclude_if(self.avoid_ambiguous, UPPER_CHARS_AMBIGUOUS), + min_uppercase, + ); + + let number = ( + CharSet::default() + .include_if(self.numbers, '0'..='9') + .exclude_if(self.avoid_ambiguous, NUMBER_CHARS_AMBIGUOUS), + min_number, + ); + + let special = ( + CharSet::default().include_if(self.special, SPECIAL_CHARS.iter().copied()), + min_special, + ); + + let all = ( + CharSet::default() + .include(&lower.0) + .include(&upper.0) + .include(&number.0) + .include(&special.0), + length - minimum_length, + ); + + Ok(PasswordGeneratorOptions { lower, upper, number, special, all, - } + length, + }) } } /// Implementation of the random password generator. This is not accessible to the public API. /// See [`ClientGenerator::password`](crate::ClientGenerator::password) for the API function. -pub(super) fn password(input: PasswordGeneratorRequest) -> Result { +pub(super) fn password(input: PasswordGeneratorOptions) -> String { password_with_rng(rand::thread_rng(), input) } -pub(super) fn password_with_rng( - mut rng: impl RngCore, - input: PasswordGeneratorRequest, -) -> Result { - // We always have to have at least one character set enabled - if !input.lowercase && !input.uppercase && !input.numbers && !input.special { - return Err(Error::Internal( - "At least one character set must be enabled", - )); - } +pub(super) fn password_with_rng(mut rng: impl RngCore, input: PasswordGeneratorOptions) -> String { + let mut buf: Vec = Vec::with_capacity(input.length); - // Generate all character dictionaries - let chars = PasswordGeneratorCharSet::new( - input.lowercase, - input.uppercase, - input.numbers, - input.special, - input.avoid_ambiguous.unwrap_or(false), - ); - - // Make sure the minimum values are zero when the character - // set is disabled, and at least one when it's enabled - fn get_minimum(min: Option, enabled: bool) -> u8 { - if enabled { - u8::max(min.unwrap_or(1), 1) - } else { - 0 - } - } - let min_lowercase = get_minimum(input.min_lowercase, input.lowercase); - let min_uppercase = get_minimum(input.min_uppercase, input.uppercase); - let min_number = get_minimum(input.min_number, input.numbers); - let min_special = get_minimum(input.min_special, input.special); - - // Check that the minimum lengths aren't larger than the password length - let min_length = min_lowercase + min_uppercase + min_number + min_special; - let length = input.length.unwrap_or(DEFAULT_PASSWORD_LENGTH); - if min_length > length { - return Err(Error::Internal( - "Password length can't be less than the sum of the minimums", - )); - } + let (set, qty) = &input.all; + buf.extend(set.sample_iter(&mut rng).take(*qty)); - // Generate the minimum chars of each type, then generate the rest to fill the expected length - let mut buf = Vec::with_capacity(length as usize); + let (set, qty) = &input.upper; + buf.extend(set.sample_iter(&mut rng).take(*qty)); - for _ in 0..min_lowercase { - buf.push(*chars.lower.choose(&mut rng).expect("slice is not empty")); - } - for _ in 0..min_uppercase { - buf.push(*chars.upper.choose(&mut rng).expect("slice is not empty")); - } - for _ in 0..min_number { - buf.push(*chars.number.choose(&mut rng).expect("slice is not empty")); - } - for _ in 0..min_special { - buf.push(*chars.special.choose(&mut rng).expect("slice is not empty")); - } - for _ in min_length..length { - buf.push(*chars.all.choose(&mut rng).expect("slice is not empty")); - } + let (set, qty) = &input.lower; + buf.extend(set.sample_iter(&mut rng).take(*qty)); + + let (set, qty) = &input.number; + buf.extend(set.sample_iter(&mut rng).take(*qty)); + + let (set, qty) = &input.special; + buf.extend(set.sample_iter(&mut rng).take(*qty)); buf.shuffle(&mut rng); - Ok(buf.iter().collect()) + + buf.iter().collect() } pub(super) fn passphrase(_input: PassphraseGeneratorRequest) -> Result { @@ -225,109 +264,153 @@ pub(super) fn passphrase(_input: PassphraseGeneratorRequest) -> Result { #[cfg(test)] mod test { - use std::collections::HashSet; + use std::collections::BTreeSet; use rand::SeedableRng; use super::*; - // We convert the slices to HashSets to be able to use `is_subset` - fn to_set(chars: &[char]) -> HashSet { - chars.iter().copied().collect() + // We convert the slices to BTreeSets to be able to use `is_subset` + fn ref_to_set<'a>(chars: impl IntoIterator) -> BTreeSet { + chars.into_iter().copied().collect() + } + fn to_set(chars: impl IntoIterator) -> BTreeSet { + chars.into_iter().collect() } #[test] - fn test_password_characters_all() { - let set = PasswordGeneratorCharSet::new(true, true, true, true, true); - assert_eq!(set.lower, LOWER_CHARS); - assert_eq!(set.upper, UPPER_CHARS); - assert_eq!(set.number, NUMBER_CHARS); - assert_eq!(set.special, SPECIAL_CHARS); + fn test_password_gen_all_charsets_enabled() { + let mut rng = rand_chacha::ChaCha8Rng::from_seed([0u8; 32]); + + let options = PasswordGeneratorRequest { + lowercase: true, + uppercase: true, + numbers: true, + special: true, + avoid_ambiguous: false, + ..Default::default() + } + .validate_options() + .unwrap(); + + assert_eq!(to_set(&options.lower.0), to_set('a'..='z')); + assert_eq!(to_set(&options.upper.0), to_set('A'..='Z')); + assert_eq!(to_set(&options.number.0), to_set('0'..='9')); + assert_eq!(to_set(&options.special.0), ref_to_set(SPECIAL_CHARS)); + + let pass = password_with_rng(&mut rng, options); + assert_eq!(pass, "Z!^B5r%hUa23dFM@"); } #[test] - fn test_password_characters_all_ambiguous() { - let set = PasswordGeneratorCharSet::new(true, true, true, true, false); - assert!(to_set(&set.lower).is_superset(&to_set(LOWER_CHARS))); - assert!(to_set(&set.lower).is_superset(&to_set(LOWER_CHARS_AMBIGUOUS))); - assert!(to_set(&set.upper).is_superset(&to_set(UPPER_CHARS))); - assert!(to_set(&set.upper).is_superset(&to_set(UPPER_CHARS_AMBIGUOUS))); - assert!(to_set(&set.number).is_superset(&to_set(NUMBER_CHARS))); - assert!(to_set(&set.number).is_superset(&to_set(NUMBER_CHARS_AMBIGUOUS))); - assert_eq!(set.special, SPECIAL_CHARS); + fn test_password_gen_only_letters_enabled() { + let mut rng = rand_chacha::ChaCha8Rng::from_seed([0u8; 32]); + + let options = PasswordGeneratorRequest { + lowercase: true, + uppercase: true, + numbers: false, + special: false, + avoid_ambiguous: false, + ..Default::default() + } + .validate_options() + .unwrap(); + + assert_eq!(to_set(&options.lower.0), to_set('a'..='z')); + assert_eq!(to_set(&options.upper.0), to_set('A'..='Z')); + assert_eq!(to_set(&options.number.0), to_set([])); + assert_eq!(to_set(&options.special.0), to_set([])); + + let pass = password_with_rng(&mut rng, options); + assert_eq!(pass, "NQiFrGufQMiNUAmj"); } #[test] - fn test_password_characters_lower() { - let set = PasswordGeneratorCharSet::new(true, false, false, false, true); - assert_eq!(set.lower, LOWER_CHARS); - assert_eq!(set.upper, Vec::new()); - assert_eq!(set.number, Vec::new()); - assert_eq!(set.special, Vec::new()); + fn test_password_gen_only_numbers_and_lower_enabled_no_ambiguous() { + let mut rng = rand_chacha::ChaCha8Rng::from_seed([0u8; 32]); + + let options = PasswordGeneratorRequest { + lowercase: true, + uppercase: false, + numbers: true, + special: false, + avoid_ambiguous: true, + ..Default::default() + } + .validate_options() + .unwrap(); + + assert!(to_set(&options.lower.0).is_subset(&to_set('a'..='z'))); + assert!(to_set(&options.lower.0).is_disjoint(&ref_to_set(LOWER_CHARS_AMBIGUOUS))); + + assert!(to_set(&options.number.0).is_subset(&to_set('0'..='9'))); + assert!(to_set(&options.number.0).is_disjoint(&ref_to_set(NUMBER_CHARS_AMBIGUOUS))); + + assert_eq!(to_set(&options.upper.0), to_set([])); + assert_eq!(to_set(&options.special.0), to_set([])); + + let pass = password_with_rng(&mut rng, options); + assert_eq!(pass, "mnjabfz5ct272prf"); } #[test] - fn test_password_characters_upper_ambiguous() { - // Only uppercase including ambiguous - let set = PasswordGeneratorCharSet::new(false, true, false, false, false); - assert_eq!(set.lower, Vec::new()); - assert!(to_set(&set.upper).is_superset(&to_set(UPPER_CHARS))); - assert!(to_set(&set.upper).is_superset(&to_set(UPPER_CHARS_AMBIGUOUS))); - assert_eq!(set.number, Vec::new()); - assert_eq!(set.special, Vec::new()); + fn test_password_gen_only_upper_and_special_enabled_no_ambiguous() { + let mut rng = rand_chacha::ChaCha8Rng::from_seed([0u8; 32]); + + let options = PasswordGeneratorRequest { + lowercase: false, + uppercase: true, + numbers: false, + special: true, + avoid_ambiguous: true, + ..Default::default() + } + .validate_options() + .unwrap(); + + assert!(to_set(&options.upper.0).is_subset(&to_set('A'..='Z'))); + assert!(to_set(&options.upper.0).is_disjoint(&ref_to_set(UPPER_CHARS_AMBIGUOUS))); + + assert_eq!(to_set(&options.special.0), ref_to_set(SPECIAL_CHARS)); + + assert_eq!(to_set(&options.lower.0), to_set([])); + assert_eq!(to_set(&options.number.0), to_set([])); + + let pass = password_with_rng(&mut rng, options); + assert_eq!(pass, "B*GBQANS%UZPQD!K"); } #[test] - fn test_password_gen() { + fn test_password_gen_minimum_limits() { let mut rng = rand_chacha::ChaCha8Rng::from_seed([0u8; 32]); - let pass = password_with_rng( - &mut rng, - PasswordGeneratorRequest { - lowercase: true, - uppercase: true, - numbers: true, - special: true, - ..Default::default() - }, - ) - .unwrap(); - assert_eq!(pass, "xfZPr&wXCiFta8DM"); - - let pass = password_with_rng( - &mut rng, - PasswordGeneratorRequest { - lowercase: true, - uppercase: true, - numbers: false, - special: false, - length: Some(20), - avoid_ambiguous: Some(false), - min_lowercase: Some(1), - min_uppercase: Some(1), - min_number: None, - min_special: None, - }, - ) - .unwrap(); - assert_eq!(pass, "jvpFStaIdRUoENAeTmJw"); - - let pass = password_with_rng( - &mut rng, - PasswordGeneratorRequest { - lowercase: false, - uppercase: false, - numbers: true, - special: true, - length: Some(5), - avoid_ambiguous: Some(true), - min_lowercase: None, - min_uppercase: None, - min_number: Some(3), - min_special: Some(2), - }, - ) + let options = PasswordGeneratorRequest { + lowercase: true, + uppercase: true, + numbers: true, + special: true, + avoid_ambiguous: false, + length: 24, + min_lowercase: Some(5), + min_uppercase: Some(5), + min_number: Some(5), + min_special: Some(5), + } + .validate_options() .unwrap(); - assert_eq!(pass, "^878%"); + + assert_eq!(to_set(&options.lower.0), to_set('a'..='z')); + assert_eq!(to_set(&options.upper.0), to_set('A'..='Z')); + assert_eq!(to_set(&options.number.0), to_set('0'..='9')); + assert_eq!(to_set(&options.special.0), ref_to_set(SPECIAL_CHARS)); + + assert_eq!(options.lower.1, 5); + assert_eq!(options.upper.1, 5); + assert_eq!(options.number.1, 5); + assert_eq!(options.special.1, 5); + + let pass = password_with_rng(&mut rng, options); + assert_eq!(pass, "236q5!a#R%PG5rI%k1!*@uRt"); } } diff --git a/crates/bw/src/main.rs b/crates/bw/src/main.rs index 73e64a4aa..71ec66880 100644 --- a/crates/bw/src/main.rs +++ b/crates/bw/src/main.rs @@ -199,7 +199,7 @@ async fn process_commands() -> Result<()> { uppercase: args.uppercase, numbers: args.numbers, special: args.special, - length: Some(args.length), + length: args.length, ..Default::default() }) .await?; From 472536f0f8976fe5e84c6cacfb47d7af77b3c212 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 6 Nov 2023 16:57:08 +0100 Subject: [PATCH 09/14] Update comments --- .../src/tool/generators/client_generator.rs | 5 +---- .../bitwarden/src/tool/generators/password.rs | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/crates/bitwarden/src/tool/generators/client_generator.rs b/crates/bitwarden/src/tool/generators/client_generator.rs index fcfb39a71..f0ef78b61 100644 --- a/crates/bitwarden/src/tool/generators/client_generator.rs +++ b/crates/bitwarden/src/tool/generators/client_generator.rs @@ -12,11 +12,8 @@ pub struct ClientGenerator<'a> { impl<'a> ClientGenerator<'a> { /// Generates a random password. - /// A passphrase is a combination of random words separated by a character. - /// An example of passphrase is `correct horse battery staple`. /// - /// By default, the password contains lowercase 16 characters, but the character - /// sets and password length can be customized using the `input` parameter. + /// The character sets and password length can be customized using the `input` parameter. /// /// # Examples /// diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index 24752f704..7d9bba43f 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -92,14 +92,19 @@ mod private { use rand::prelude::Distribution; - // Note: We are using a BTreeSet to have consistent ordering between runs. This is not - // important during normal execution, but it's necessary for the tests to be repeatable. + /// A set of characters used to generate a password. This set is backed by a BTreeSet + /// to have consistent ordering between runs. This is notimportant during normal execution, + /// but it's necessary for the tests to be repeatable. + /// To create an instance, use [`CharSet::default()`](CharSet::default) #[derive(Clone, Default)] - pub struct CharSet(pub(super) BTreeSet); + pub struct CharSet(BTreeSet); impl CharSet { + /// Includes the given characters in the set. Any duplicate items will be ignored pub fn include(self, other: impl IntoIterator) -> Self { self.include_if(true, other) } + + /// Includes the given characters in the set if the predicate is true. Any duplicate items will be ignored pub fn include_if( mut self, predicate: bool, @@ -110,6 +115,8 @@ mod private { } self } + + /// Excludes the given characters from the set. Any missing items will be ignored pub fn exclude_if<'a>( self, predicate: bool, @@ -137,6 +144,8 @@ mod private { } } + /// Represents a set of valid options to generate a password with. + /// To get an instance of it, use [`PasswordGeneratorRequest::validate_options`](PasswordGeneratorRequest::validate_options) pub struct PasswordGeneratorOptions { pub(super) lower: (CharSet, usize), pub(super) upper: (CharSet, usize), @@ -150,8 +159,10 @@ mod private { use private::{CharSet, PasswordGeneratorOptions}; impl PasswordGeneratorRequest { - // TODO: Add password generator policy checks + /// Validates the request and returns an immutable struct with valid options to use with [`password`](password). pub fn validate_options(self) -> Result { + // TODO: Add password generator policy checks + // We always have to have at least one character set enabled if !self.lowercase && !self.uppercase && !self.numbers && !self.special { return Err(Error::Internal( From 0038e6f63d62d4be3b52ad20cbbcd834e38d4dbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 6 Nov 2023 17:57:05 +0100 Subject: [PATCH 10/14] Remove reference to innaccessible struct --- crates/bitwarden/src/tool/generators/password.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index 7d9bba43f..caa38298f 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -159,7 +159,7 @@ mod private { use private::{CharSet, PasswordGeneratorOptions}; impl PasswordGeneratorRequest { - /// Validates the request and returns an immutable struct with valid options to use with [`password`](password). + /// Validates the request and returns an immutable struct with valid options to use with the password generator. pub fn validate_options(self) -> Result { // TODO: Add password generator policy checks From eaa4582cdf852ea124f59aca7eb7ad0b7cc52155 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 6 Nov 2023 17:59:27 +0100 Subject: [PATCH 11/14] Add minimum password length --- crates/bitwarden/src/tool/generators/password.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index caa38298f..c301bddce 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -170,6 +170,12 @@ impl PasswordGeneratorRequest { )); } + if self.length < 4 { + return Err(Error::Internal( + "A password must be at least 4 characters long", + )); + } + // Make sure the minimum values are zero when the character // set is disabled, and at least one when it's enabled fn get_minimum(min: Option, enabled: bool) -> usize { From 705220dfd1bb4886cdf47fb76b86ca262c9bc528 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 4 Dec 2023 12:11:32 +0100 Subject: [PATCH 12/14] Change input --- .../src/tool/generators/client_generator.rs | 3 +- .../bitwarden/src/tool/generators/password.rs | 135 ++++++++---------- 2 files changed, 64 insertions(+), 74 deletions(-) diff --git a/crates/bitwarden/src/tool/generators/client_generator.rs b/crates/bitwarden/src/tool/generators/client_generator.rs index f0ef78b61..937e07182 100644 --- a/crates/bitwarden/src/tool/generators/client_generator.rs +++ b/crates/bitwarden/src/tool/generators/client_generator.rs @@ -33,8 +33,7 @@ impl<'a> ClientGenerator<'a> { /// } /// ``` pub async fn password(&self, input: PasswordGeneratorRequest) -> Result { - let charset = input.validate_options()?; - Ok(password(charset)) + password(input) } pub async fn passphrase(&self, input: PassphraseGeneratorRequest) -> Result { diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index c301bddce..80c01e93a 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -1,8 +1,11 @@ -use crate::error::{Error, Result}; +use std::collections::BTreeSet; + use rand::{distributions::Distribution, seq::SliceRandom, RngCore}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use crate::error::{Error, Result}; + /// Password generator request options. If all options are false, the default is to /// generate a password with: /// - lowercase @@ -85,82 +88,69 @@ const LOWER_CHARS_AMBIGUOUS: &[char] = &['l']; const NUMBER_CHARS_AMBIGUOUS: &[char] = &['0', '1']; const SPECIAL_CHARS: &[char] = &['!', '@', '#', '$', '%', '^', '&', '*']; -// We don't want the validated struct to be accessible, yet at the same time it needs to be public -// to be used as a return type, so we define it in a private module to make it innaccessible. -mod private { - use std::collections::BTreeSet; +/// A set of characters used to generate a password. This set is backed by a BTreeSet +/// to have consistent ordering between runs. This is not important during normal execution, +/// but it's necessary for the tests to be repeatable. +/// To create an instance, use [`CharSet::default()`](CharSet::default) +#[derive(Clone, Default)] +struct CharSet(BTreeSet); +impl CharSet { + /// Includes the given characters in the set. Any duplicate items will be ignored + pub fn include(self, other: impl IntoIterator) -> Self { + self.include_if(true, other) + } - use rand::prelude::Distribution; - - /// A set of characters used to generate a password. This set is backed by a BTreeSet - /// to have consistent ordering between runs. This is notimportant during normal execution, - /// but it's necessary for the tests to be repeatable. - /// To create an instance, use [`CharSet::default()`](CharSet::default) - #[derive(Clone, Default)] - pub struct CharSet(BTreeSet); - impl CharSet { - /// Includes the given characters in the set. Any duplicate items will be ignored - pub fn include(self, other: impl IntoIterator) -> Self { - self.include_if(true, other) + /// Includes the given characters in the set if the predicate is true. Any duplicate items will be ignored + pub fn include_if(mut self, predicate: bool, other: impl IntoIterator) -> Self { + if predicate { + self.0.extend(other); } + self + } - /// Includes the given characters in the set if the predicate is true. Any duplicate items will be ignored - pub fn include_if( - mut self, - predicate: bool, - other: impl IntoIterator, - ) -> Self { - if predicate { - self.0.extend(other); - } + /// Excludes the given characters from the set. Any missing items will be ignored + pub fn exclude_if<'a>( + self, + predicate: bool, + other: impl IntoIterator, + ) -> Self { + if predicate { + let other: BTreeSet<_> = other.into_iter().copied().collect(); + Self(self.0.difference(&other).copied().collect()) + } else { self } - - /// Excludes the given characters from the set. Any missing items will be ignored - pub fn exclude_if<'a>( - self, - predicate: bool, - other: impl IntoIterator, - ) -> Self { - if predicate { - let other: BTreeSet<_> = other.into_iter().copied().collect(); - Self(self.0.difference(&other).copied().collect()) - } else { - self - } - } } - impl<'a> IntoIterator for &'a CharSet { - type Item = char; - type IntoIter = std::iter::Copied>; - fn into_iter(self) -> Self::IntoIter { - self.0.iter().copied() - } +} +impl<'a> IntoIterator for &'a CharSet { + type Item = char; + type IntoIter = std::iter::Copied>; + fn into_iter(self) -> Self::IntoIter { + self.0.iter().copied() } - impl Distribution for CharSet { - fn sample(&self, rng: &mut R) -> char { - let idx = rng.gen_range(0..self.0.len()); - *self.0.iter().nth(idx).expect("Valid index") - } +} +impl Distribution for CharSet { + fn sample(&self, rng: &mut R) -> char { + let idx = rng.gen_range(0..self.0.len()); + *self.0.iter().nth(idx).expect("Valid index") } +} - /// Represents a set of valid options to generate a password with. - /// To get an instance of it, use [`PasswordGeneratorRequest::validate_options`](PasswordGeneratorRequest::validate_options) - pub struct PasswordGeneratorOptions { - pub(super) lower: (CharSet, usize), - pub(super) upper: (CharSet, usize), - pub(super) number: (CharSet, usize), - pub(super) special: (CharSet, usize), - pub(super) all: (CharSet, usize), +/// Represents a set of valid options to generate a password with. +/// To get an instance of it, use [`PasswordGeneratorRequest::validate_options`](PasswordGeneratorRequest::validate_options) +struct PasswordGeneratorOptions { + pub(super) lower: (CharSet, usize), + pub(super) upper: (CharSet, usize), + pub(super) number: (CharSet, usize), + pub(super) special: (CharSet, usize), + pub(super) all: (CharSet, usize), - pub(super) length: usize, - } + pub(super) length: usize, } -use private::{CharSet, PasswordGeneratorOptions}; impl PasswordGeneratorRequest { /// Validates the request and returns an immutable struct with valid options to use with the password generator. - pub fn validate_options(self) -> Result { + fn validate_options(self) -> Result { // TODO: Add password generator policy checks // We always have to have at least one character set enabled @@ -248,26 +238,27 @@ impl PasswordGeneratorRequest { /// Implementation of the random password generator. This is not accessible to the public API. /// See [`ClientGenerator::password`](crate::ClientGenerator::password) for the API function. -pub(super) fn password(input: PasswordGeneratorOptions) -> String { - password_with_rng(rand::thread_rng(), input) +pub(super) fn password(input: PasswordGeneratorRequest) -> Result { + let options = input.validate_options()?; + Ok(password_with_rng(rand::thread_rng(), options)) } -pub(super) fn password_with_rng(mut rng: impl RngCore, input: PasswordGeneratorOptions) -> String { - let mut buf: Vec = Vec::with_capacity(input.length); +fn password_with_rng(mut rng: impl RngCore, options: PasswordGeneratorOptions) -> String { + let mut buf: Vec = Vec::with_capacity(options.length); - let (set, qty) = &input.all; + let (set, qty) = &options.all; buf.extend(set.sample_iter(&mut rng).take(*qty)); - let (set, qty) = &input.upper; + let (set, qty) = &options.upper; buf.extend(set.sample_iter(&mut rng).take(*qty)); - let (set, qty) = &input.lower; + let (set, qty) = &options.lower; buf.extend(set.sample_iter(&mut rng).take(*qty)); - let (set, qty) = &input.number; + let (set, qty) = &options.number; buf.extend(set.sample_iter(&mut rng).take(*qty)); - let (set, qty) = &input.special; + let (set, qty) = &options.special; buf.extend(set.sample_iter(&mut rng).take(*qty)); buf.shuffle(&mut rng); From 24a875f058ebe0705ffafcd5e8c59aaa804aa6ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 4 Dec 2023 19:39:47 +0100 Subject: [PATCH 13/14] Review comments --- .../bitwarden/src/tool/generators/password.rs | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index 7bf109f91..d2e52841d 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -17,13 +17,13 @@ use crate::error::{Error, Result}; #[serde(rename_all = "camelCase", deny_unknown_fields)] #[cfg_attr(feature = "mobile", derive(uniffi::Record))] pub struct PasswordGeneratorRequest { - /// When set to true, the generated password will contain lowercase characters (a-z). + /// Include lowercase characters (a-z). pub lowercase: bool, - /// When set to true, the generated password will contain uppercase characters (A-Z). + /// Include uppercase characters (A-Z). pub uppercase: bool, - /// When set to true, the generated password will contain numbers (0-9). + /// Include numbers (0-9). pub numbers: bool, - /// When set to true, the generated password will contain special characters. + /// Include special characters. /// The supported characters are: ! @ # $ % ^ & * pub special: bool, @@ -233,20 +233,16 @@ pub(super) fn password(input: PasswordGeneratorRequest) -> Result { fn password_with_rng(mut rng: impl RngCore, options: PasswordGeneratorOptions) -> String { let mut buf: Vec = Vec::with_capacity(options.length); - let (set, qty) = &options.all; - buf.extend(set.sample_iter(&mut rng).take(*qty)); - - let (set, qty) = &options.upper; - buf.extend(set.sample_iter(&mut rng).take(*qty)); - - let (set, qty) = &options.lower; - buf.extend(set.sample_iter(&mut rng).take(*qty)); - - let (set, qty) = &options.number; - buf.extend(set.sample_iter(&mut rng).take(*qty)); - - let (set, qty) = &options.special; - buf.extend(set.sample_iter(&mut rng).take(*qty)); + let opts = [ + &options.all, + &options.upper, + &options.lower, + &options.number, + &options.special, + ]; + for (set, qty) in opts { + buf.extend(set.sample_iter(&mut rng).take(*qty)); + } buf.shuffle(&mut rng); From bb8ba170c6acf8846fff9e8e97496e080b02c9b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 5 Dec 2023 13:46:30 +0100 Subject: [PATCH 14/14] Review comments --- .../bitwarden/src/tool/generators/password.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/crates/bitwarden/src/tool/generators/password.rs b/crates/bitwarden/src/tool/generators/password.rs index d2e52841d..cf986495c 100644 --- a/crates/bitwarden/src/tool/generators/password.rs +++ b/crates/bitwarden/src/tool/generators/password.rs @@ -6,13 +6,7 @@ use serde::{Deserialize, Serialize}; use crate::error::{Error, Result}; -/// Password generator request options. If all options are false, the default is to -/// generate a password with: -/// - lowercase -/// - uppercase -/// - numbers -/// -/// The default length is 16. +/// Password generator request options. #[derive(Serialize, Deserialize, Debug, JsonSchema)] #[serde(rename_all = "camelCase", deny_unknown_fields)] #[cfg_attr(feature = "mobile", derive(uniffi::Record))] @@ -23,8 +17,7 @@ pub struct PasswordGeneratorRequest { pub uppercase: bool, /// Include numbers (0-9). pub numbers: bool, - /// Include special characters. - /// The supported characters are: ! @ # $ % ^ & * + /// Include special characters: ! @ # $ % ^ & * pub special: bool, /// The length of the generated password. @@ -51,14 +44,12 @@ pub struct PasswordGeneratorRequest { const DEFAULT_PASSWORD_LENGTH: u8 = 16; -// We need to implement this manually so we can set one character set to true. -// Otherwise the default implementation will fail to generate a password. impl Default for PasswordGeneratorRequest { fn default() -> Self { Self { lowercase: true, - uppercase: false, - numbers: false, + uppercase: true, + numbers: true, special: false, length: DEFAULT_PASSWORD_LENGTH, avoid_ambiguous: false,