From f90438dbd98b00818d4aced7f5d27b8b7fd0dbab Mon Sep 17 00:00:00 2001 From: Owain Davies <116417456+OTheDev@users.noreply.github.com> Date: Mon, 6 May 2024 23:48:52 +0700 Subject: [PATCH] Use service and username to identify each password (#15) Refactored the password manager to utilize a service-username pair (represented by the `UserIdentity` struct), rather than an ID string, to identify each password. This is more aligned with real-world user behavior, as users tend to associate a password with a username and a particular target platform, not with an arbitrary ID. This change makes the password manager easier to use. --- README.md | 2 +- src/lib.rs | 2 +- src/main.rs | 196 +++++++++++++++++++++++++++---------------- src/manager.rs | 128 +++++++++++++++++++++------- src/manager/error.rs | 4 +- src/manager/tests.rs | 143 ++++++++++++++++++++----------- 6 files changed, 318 insertions(+), 157 deletions(-) diff --git a/README.md b/README.md index ebe6b8a..d02f464 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ Rudimentary command-line tool and Rust library for managing passwords. ## Password Database Passwords are encrypted and stored in a SQLite database where each password is -identified by a unique ID string. +uniquely identified by a **service** name and an optional **username**. ## Security diff --git a/src/lib.rs b/src/lib.rs index e5fc7fc..e9b5cfa 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,4 +7,4 @@ mod db; mod manager; pub use manager::error::{Error, Result}; -pub use manager::PwdManager; +pub use manager::{PwdManager, UserIdentity}; diff --git a/src/main.rs b/src/main.rs index b3267d0..1f9d0ce 100644 --- a/src/main.rs +++ b/src/main.rs @@ -11,7 +11,7 @@ use dialoguer::{theme::ColorfulTheme, Confirm, Input, Password, Select}; use pwdg::{PwdGen, PwdGenOptions}; use pwdm::{ Error::{IncorrectMasterPassword, WeakPassword}, - PwdManager, + PwdManager, UserIdentity, }; use std::path::{Path, PathBuf}; @@ -164,16 +164,55 @@ fn select_action() -> Result { Ok(SELECTIONS[selection]) } +fn get_identity() -> Result> { + let service: String = Input::new() + .with_prompt(format!("Enter {} ('b' to go back)", "Service".cyan())) + .interact_text()?; + if service == "b" { + return Ok(UserAction::Back); + } + + let username: String = Input::new() + .with_prompt(format!( + "Enter {} (or press Enter for None, 'b' to go back)", + "Username".cyan() + )) + .allow_empty(true) + .interact_text()?; + if username == "b" { + return Ok(UserAction::Back); + } + + let uid = UserIdentity { + service, + username: if username.is_empty() { + None + } else { + Some(username) + }, + }; + Ok(UserAction::ContinueWithMessage(uid)) +} + fn match_action( selection: Action, pwdgen: &PwdGen, pwd_manager: &mut PwdManager, ) -> Result> { match selection { - Action::Add => add_password(pwd_manager, pwdgen), - Action::Get => get_password(pwd_manager), - Action::Delete => delete_password(pwd_manager), - Action::Update => update_password(pwd_manager, pwdgen), + Action::Add | Action::Get | Action::Delete | Action::Update => { + match get_identity()? { + UserAction::Back => Ok(UserAction::Back), + UserAction::ContinueWithMessage(uid) => match selection { + Action::Add => add_password(pwd_manager, pwdgen, &uid), + Action::Get => get_password(pwd_manager, &uid), + Action::Delete => delete_password(pwd_manager, &uid), + Action::Update => update_password(pwd_manager, pwdgen, &uid), + _ => unreachable!(), + }, + _ => unreachable!(), + } + } Action::List => { list_passwords(pwd_manager)?; Ok(UserAction::Continue) @@ -211,88 +250,99 @@ where fn add_password( pwd_manager: &PwdManager, pwdgen: &PwdGen, + uid: &UserIdentity, ) -> Result> { - do_action( - "Enter ID", - |id| { - if pwd_manager.get_password(id)?.is_none() { - let password: String = generate_password(pwdgen, "Enter password")?; - pwd_manager.add_password(id, &password)?; - println!("Password added."); - } else { - println!("{}", "Password exists.".red()); - } - Ok(()) - }, - false, - ) + if pwd_manager.get_password(uid)?.is_none() { + let password: String = generate_password(pwdgen, "Enter password")?; + pwd_manager.add_password(uid, &password)?; + Ok(UserAction::ContinueWithMessage("Password added.".into())) + } else { + println!("{}", "Password exists.".red()); + Ok(UserAction::Continue) + } } -fn get_password(pwd_manager: &PwdManager) -> Result> { - do_action( - "Enter ID", - |id| { - match pwd_manager.get_password(id)? { - Some(password) => println!("Password: {}", password), - None => print_no_password_found_for_id(id), - } - Ok(()) - }, - false, - ) +fn get_password( + pwd_manager: &PwdManager, + uid: &UserIdentity, +) -> Result> { + match pwd_manager.get_password(uid)? { + Some(password) => Ok(UserAction::ContinueWithMessage(format!( + "{}: {}", + "Password".cyan(), + password + ))), + None => { + print_no_password_found_for_id(uid); + Ok(UserAction::Continue) + } + } } -fn delete_password(pwd_manager: &PwdManager) -> Result> { - do_action( - "Enter ID", - |id| { - if pwd_manager.get_password(id)?.is_none() { - print_no_password_found_for_id(id); - } else if Confirm::new() - .with_prompt(format!( - "Are you sure you want to delete password for ID {}", - id - )) - .interact()? - { - pwd_manager.delete_password(id)?; - println!("Password deleted."); - } - Ok(()) - }, - false, - ) +fn delete_password( + pwd_manager: &PwdManager, + uid: &UserIdentity, +) -> Result> { + if pwd_manager.get_password(uid)?.is_none() { + print_no_password_found_for_id(uid); + return Ok(UserAction::Continue); + } else if Confirm::new() + .with_prompt(format!( + "Are you sure you want to delete the password for {}", + uid + )) + .interact()? + { + pwd_manager.delete_password(uid)?; + return Ok(UserAction::ContinueWithMessage("Password deleted.".into())); + } + Ok(UserAction::Continue) } fn update_password( pwd_manager: &PwdManager, pwdgen: &PwdGen, + uid: &UserIdentity, ) -> Result> { - do_action( - "Enter ID", - |id| { - if pwd_manager.get_password(id)?.is_none() { - print_no_password_found_for_id(id); - } else { - let new_password: String = - generate_password(pwdgen, "Enter new password")?; - pwd_manager.update_password(id, &new_password)?; - println!("Password updated."); - } - Ok(()) - }, - false, - ) + if pwd_manager.get_password(uid)?.is_none() { + print_no_password_found_for_id(uid); + return Ok(UserAction::Continue); + } + let new_password: String = generate_password(pwdgen, "Enter new password")?; + pwd_manager.update_password(uid, &new_password)?; + Ok(UserAction::ContinueWithMessage("Password updated.".into())) } fn list_passwords(pwd_manager: &PwdManager) -> Result<()> { - let ids = pwd_manager.list_passwords()?; - if ids.is_empty() { + let uids = pwd_manager.list_passwords()?; + if uids.is_empty() { println!("No passwords stored."); } else { - println!("Stored passwords:"); - for id in ids { - println!("- {}", id); + let max_service_len = + uids.iter().map(|uid| uid.service.len()).max().unwrap_or(0); + let max_username_len = uids + .iter() + .filter_map(|uid| uid.username.as_ref().map(String::len)) + .max() + .unwrap_or(0); + println!( + "\n{0:, +} + +impl PartialEq for UserIdentity { + fn eq(&self, other: &Self) -> bool { + self.service == other.service && self.username == other.username + } +} + +impl std::fmt::Display for UserIdentity { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match &self.username { + Some(username) => { + write!(f, "Service: {}, Username: {}", self.service, username) + } + None => write!(f, "Service: {}", self.service), + } + } +} + /// Password manager struct. pub struct PwdManager { conn: Connection, @@ -72,7 +95,9 @@ impl PwdManager { tx.execute( "CREATE TABLE passwords ( - id TEXT PRIMARY KEY, + id INTEGER PRIMARY KEY AUTOINCREMENT, + service TEXT NOT NULL, + username TEXT, ciphertext BLOB NOT NULL CHECK(length(ciphertext) > 0), nonce BLOB NOT NULL, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, @@ -81,6 +106,16 @@ impl PwdManager { [], )?; + // In SQLite, NULL values are considered different from all other NULL + // values, so we use a little trick with ifnull(). + tx.execute( + r#" + CREATE UNIQUE INDEX idx_username_service ON passwords + (ifnull(username, ''), service); + "#, + [], + )?; + tx.execute( "CREATE TRIGGER update_time AFTER UPDATE ON passwords @@ -166,7 +201,7 @@ impl PwdManager { } /// Adds a password to the database. - pub fn add_password(&self, id: &str, password: &str) -> Result<()> { + pub fn add_password(&self, uid: &UserIdentity, password: &str) -> Result<()> { if password.is_empty() { return Err(Error::EmptyPassword); } @@ -174,15 +209,23 @@ impl PwdManager { let (ciphertext, nonce) = self.cipher.encrypt(password)?; match self.conn.execute( - "INSERT INTO passwords (id, ciphertext, nonce) VALUES (?1, ?2, ?3)", - params![id, &ciphertext[..], &nonce[..]], + " + INSERT INTO passwords (service, username, ciphertext, nonce) + VALUES (?1, ?2, ?3, ?4) + ", + params![ + &uid.service, + uid.username.as_deref(), + &ciphertext[..], + &nonce[..] + ], ) { Ok(_) => Ok(()), Err(err) => match err { rusqlite::Error::SqliteFailure(error, _) if error.code == rusqlite::ErrorCode::ConstraintViolation => { - Err(Error::DuplicateId(id.to_string())) + Err(Error::DuplicateId(uid.clone())) } _ => Err(Error::Sqlite(err)), }, @@ -190,10 +233,11 @@ impl PwdManager { } /// Removes a password by its ID from the database. - pub fn delete_password(&self, id: &str) -> Result<()> { - let changes = self - .conn - .execute("DELETE FROM passwords WHERE id = ?1", params![id])?; + pub fn delete_password(&self, uid: &UserIdentity) -> Result<()> { + let changes = self.conn.execute( + "DELETE FROM passwords WHERE service = ?1 AND username IS ?2", + params![&uid.service, uid.username.as_deref()], + )?; if changes == 0 { Err(Error::PasswordNotFound) @@ -203,47 +247,71 @@ impl PwdManager { } /// Fetches all password IDs sorted in ascending order. - pub fn list_passwords(&self) -> Result> { - let mut stmt = self - .conn - .prepare("SELECT id FROM passwords ORDER BY id ASC")?; - let rows = stmt.query_map([], |row| row.get(0))?; - - let mut ids = Vec::new(); - for id_result in rows { - let id: String = id_result?; - ids.push(id); + pub fn list_passwords(&self) -> Result> { + let mut stmt = self.conn.prepare( + "SELECT service, username FROM passwords + ORDER BY service ASC, username ASC + ", + )?; + + let rows = stmt.query_map([], |row| { + let service: String = row.get("service")?; + let username: Option = row.get("username")?; + let uid = UserIdentity { service, username }; + Ok(uid) + })?; + + let mut service_username_pairs = Vec::new(); + for row_result in rows { + let row = row_result?; + service_username_pairs.push(row); } - Ok(ids) + Ok(service_username_pairs) } /// Updates a password by its ID. - pub fn update_password(&self, id: &str, new_password: &str) -> Result<()> { + pub fn update_password( + &self, + uid: &UserIdentity, + new_password: &str, + ) -> Result<()> { if new_password.is_empty() { return Err(Error::EmptyPassword); } - if self.get_password(id)?.is_none() { + if self.get_password(uid)?.is_none() { return Err(Error::PasswordNotFound); } let (ciphertext, nonce) = self.cipher.encrypt(new_password)?; self.conn.execute( - "UPDATE passwords SET ciphertext = ?1, nonce = ?2 WHERE id = ?3", - params![&ciphertext[..], &nonce[..], id], + " + UPDATE passwords SET ciphertext = ?1, nonce = ?2 + WHERE service = ?3 AND username IS ?4 + ", + params![ + &ciphertext[..], + &nonce[..], + &uid.service, + uid.username.as_deref() + ], )?; Ok(()) } /// Retrieves a password by its ID. - pub fn get_password(&self, id: &str) -> Result> { - let mut stmt = self - .conn - .prepare("SELECT ciphertext, nonce FROM passwords WHERE id = ?")?; + pub fn get_password(&self, uid: &UserIdentity) -> Result> { + let mut stmt = self.conn.prepare( + " + SELECT ciphertext, nonce FROM passwords + WHERE service = ?1 AND username IS ?2 + ", + )?; - let mut rows = stmt.query(params![id])?; + let mut rows = + stmt.query(params![&uid.service, uid.username.as_deref()])?; if let Some(row) = rows.next()? { let ciphertext: Vec = row.get(0)?; @@ -292,7 +360,7 @@ impl PwdManager { tx.prepare("SELECT id, ciphertext, nonce FROM passwords")?; let rows = stmt.query_map([], |row| { - let id: String = row.get(0)?; + let id: i64 = row.get(0)?; let ciphertext: Vec = row.get(1)?; let nonce: Vec = row.get(2)?; Ok((id, ciphertext, nonce)) diff --git a/src/manager/error.rs b/src/manager/error.rs index a951803..b0eaf83 100644 --- a/src/manager/error.rs +++ b/src/manager/error.rs @@ -18,8 +18,8 @@ pub enum Error { #[error("Argon2PasswordHash error: {0}")] Argon2PasswordHash(argon2::password_hash::Error), - #[error("Duplicate id '{0}': A password with this id already exists")] - DuplicateId(String), + #[error("Duplicate user identity: A password with this uid already exists")] + DuplicateId(crate::manager::UserIdentity), #[error("The password provided is empty")] EmptyPassword, diff --git a/src/manager/tests.rs b/src/manager/tests.rs index c987531..eff773b 100644 --- a/src/manager/tests.rs +++ b/src/manager/tests.rs @@ -14,6 +14,20 @@ fn setup_db() -> (PwdManager, NamedTempFile) { (manager, temp_file) } +fn initial_identity() -> UserIdentity { + UserIdentity { + service: "test_service".to_string(), + username: Some("test_username".to_string()), + } +} + +fn nonexistent_identity() -> UserIdentity { + UserIdentity { + service: "nonexistent_service".to_string(), + username: Some("nonexistent_username".to_string()), + } +} + #[test] fn test_new_pwd_manager() { let (manager, _temp_file) = setup_db(); @@ -23,73 +37,76 @@ fn test_new_pwd_manager() { #[test] fn test_add_and_get_password() { let (manager, _temp_file) = setup_db(); - manager.add_password("test_id", "test_password").unwrap(); - let retrieved_password = manager.get_password("test_id").unwrap(); + let uid = initial_identity(); + + manager.add_password(&uid, "test_password").unwrap(); + let retrieved_password = manager.get_password(&uid).unwrap(); assert_eq!(retrieved_password, Some("test_password".to_string())); } #[test] fn test_update_password() { let (manager, _temp_file) = setup_db(); - manager.add_password("test_id", "test_password").unwrap(); - manager.update_password("test_id", "new_password").unwrap(); - let retrieved_password = manager.get_password("test_id").unwrap(); + let uid = initial_identity(); + + manager.add_password(&uid, "test_password").unwrap(); + manager.update_password(&uid, "new_password").unwrap(); + let retrieved_password = manager.get_password(&uid).unwrap(); assert_eq!(retrieved_password, Some("new_password".to_string())); } #[test] fn test_delete_password() { let (manager, _temp_file) = setup_db(); - manager.add_password("test_id", "test_password").unwrap(); - manager.delete_password("test_id").unwrap(); - let retrieved_password = manager.get_password("test_id").unwrap(); + let uid = initial_identity(); + + manager.add_password(&uid, "test_password").unwrap(); + manager.delete_password(&uid).unwrap(); + let retrieved_password = manager.get_password(&uid).unwrap(); assert!(retrieved_password.is_none()); } #[test] fn test_get_nonexistent_password() { let (manager, _temp_file) = setup_db(); - assert_eq!(manager.get_password("nonexistent_id").unwrap(), None); + assert_eq!(manager.get_password(&nonexistent_identity()).unwrap(), None); } #[test] fn test_update_nonexistent_password() { let (manager, _temp_file) = setup_db(); assert!(manager - .update_password("nonexistent_id", "new_password") + .update_password(&nonexistent_identity(), "new_password") .is_err()); } #[test] fn test_delete_nonexistent_password() { let (manager, _temp_file) = setup_db(); - assert!(manager.delete_password("nonexistent_id").is_err()); + assert!(manager.delete_password(&nonexistent_identity()).is_err()); } #[test] fn test_long_password() { let (manager, _temp_file) = setup_db(); + let uid = initial_identity(); + let long_password = "p".repeat(1000); - manager - .add_password("long_pass_id", &long_password) - .unwrap(); - assert_eq!( - manager.get_password("long_pass_id").unwrap(), - Some(long_password) - ); + manager.add_password(&uid, &long_password).unwrap(); + assert_eq!(manager.get_password(&uid).unwrap(), Some(long_password)); } #[test] fn test_special_character_password() { let (manager, _temp_file) = setup_db(); + let uid = initial_identity(); + let special_password = "p@ssw0rd!$"; - manager - .add_password("special_char_id", special_password) - .unwrap(); + manager.add_password(&uid, special_password).unwrap(); assert_eq!( - manager.get_password("special_char_id").unwrap(), + manager.get_password(&uid).unwrap(), Some(special_password.to_string()) ); } @@ -97,10 +114,11 @@ fn test_special_character_password() { #[test] fn test_duplicate_password_addition() { let (manager, _temp_file) = setup_db(); - let duplicate_id = "duplicate_id"; - manager.add_password(duplicate_id, "password").unwrap(); - match manager.add_password(duplicate_id, "password") { - Err(Error::DuplicateId(id)) => assert_eq!(duplicate_id, id), + let uid = initial_identity(); + + manager.add_password(&uid, "password").unwrap(); + match manager.add_password(&uid, "password") { + Err(Error::DuplicateId(id)) => assert_eq!(&uid, &id), _ => panic!("Expected DuplicateId error"), } } @@ -108,15 +126,19 @@ fn test_duplicate_password_addition() { #[test] fn test_add_empty_password() { let (manager, _temp_file) = setup_db(); - let result = manager.add_password("empty_password_id", ""); + let uid = initial_identity(); + + let result = manager.add_password(&uid, ""); assert!(result.is_err()); } #[test] fn test_update_to_empty_password() { let (manager, _temp_file) = setup_db(); - manager.add_password("test_id", "test_password").unwrap(); - let result = manager.update_password("test_id", ""); + let uid = initial_identity(); + + manager.add_password(&uid, "test_password").unwrap(); + let result = manager.update_password(&uid, ""); assert!(result.is_err()); } @@ -160,27 +182,39 @@ fn test_database_file_opening_failure() { #[test] fn test_list_passwords() { let (manager, _temp_file) = setup_db(); - manager.add_password("test_id1", "test_password").unwrap(); - manager.add_password("test_id2", "test_password").unwrap(); + + let id1 = UserIdentity { + service: "test_service_1".to_string(), + username: Some("test_username_1".to_string()), + }; + let id2 = UserIdentity { + service: "test_service_2".to_string(), + username: Some("test_username_2".to_string()), + }; + + manager.add_password(&id1, "test_password").unwrap(); + manager.add_password(&id2, "test_password").unwrap(); let ids = manager.list_passwords().unwrap(); - assert_eq!(ids, vec!["test_id1", "test_id2"]); + assert_eq!(ids, vec![id1, id2]); } #[test] fn test_add_update_long_password() { let (manager, _temp_file) = setup_db(); + let uid = initial_identity(); + let long_password = "p@ssw0Rd".repeat(100); - manager.add_password("long_id", &long_password).unwrap(); - let retrieved_password = manager.get_password("long_id").unwrap(); + manager.add_password(&uid, &long_password).unwrap(); + let retrieved_password = manager.get_password(&uid).unwrap(); assert_eq!(retrieved_password, Some(long_password.clone())); let updated_long_password = "q".repeat(10_000); manager - .update_password("long_id", &updated_long_password) + .update_password(&uid, &updated_long_password) .unwrap(); - let updated_retrieved_password = manager.get_password("long_id").unwrap(); + let updated_retrieved_password = manager.get_password(&uid).unwrap(); assert_eq!(updated_retrieved_password, Some(updated_long_password)); } @@ -239,7 +273,9 @@ fn test_cipher_decrypt_on_invalid_encryption() { #[test] fn test_update_master_password() { let (mut manager, _temp_file) = setup_db(); - manager.add_password("test_id", "test_password").unwrap(); + let uid = initial_identity(); + + manager.add_password(&uid, "test_password").unwrap(); let new_master_password = "new_master_password"; manager.update_master_password(new_master_password).unwrap(); @@ -260,13 +296,18 @@ fn test_update_master_password() { // Verify encryption/decryption assert_eq!( - manager.get_password("test_id").unwrap(), + manager.get_password(&uid).unwrap(), Some("test_password".to_string()) ); - manager.add_password("new_id", "new_password").unwrap(); + let new_uid = UserIdentity { + service: "new_service".to_string(), + username: Some("new_username".to_string()), + }; + + manager.add_password(&new_uid, "new_password").unwrap(); assert_eq!( - manager.get_password("new_id").unwrap(), + manager.get_password(&new_uid).unwrap(), Some("new_password".to_string()) ); } @@ -383,33 +424,35 @@ fn test_created_at_and_updated_at() { cast(strftime('%s', created_at) as integer), cast(strftime('%s', updated_at) as integer) FROM passwords - WHERE id = ? + WHERE service = ?1 AND username = ?2 "; - let get_times = |id: &str| { + let get_times = |uid: &UserIdentity| { manager .conn - .query_row(QUERY, rusqlite::params![id], |row| { - Ok((row.get::<_, i64>(0)?, row.get::<_, i64>(1)?)) - }) + .query_row( + QUERY, + rusqlite::params![&uid.service, uid.username.as_deref()], + |row| Ok((row.get::<_, i64>(0)?, row.get::<_, i64>(1)?)), + ) .unwrap() }; - let id = "test_id"; + let uid = initial_identity(); let password = "test_password"; - manager.add_password(id, password).unwrap(); + manager.add_password(&uid, password).unwrap(); // Check timestamps on creation - let times_0 = get_times(id); + let times_0 = get_times(&uid); assert_eq!(times_0.0, times_0.1); std::thread::sleep(std::time::Duration::from_secs(2)); // Update the password let new_password = "new_password"; - manager.update_password(id, new_password).unwrap(); + manager.update_password(&uid, new_password).unwrap(); // Check timestamps on update - let times_1 = get_times(id); + let times_1 = get_times(&uid); assert!( times_1.1 > times_1.0, "updated_at should not be earlier than created_at"