Refactor add action to unlock vault after user input

Refactored the action structure to delay vault unlocking until after user input is received in "add" operations.

src/actions/add.rs

@@ -1,9 +1,8 @@
 use anyhow::{bail, Context};
 use clap::ArgMatches;
 use clipboard::{ClipboardContext, ClipboardProvider};
-use crate::actions::{Action, copy_to_clipboard, ItemType, UnlockingAction};
+use crate::actions::{Action, copy_to_clipboard, ItemType, unlock, unlock_totp_vault};
 use crate::{crypto, ui};
-use crate::vault::entities::Credential;
 use crate::vault::vault_trait::Vault;
 
 pub struct AddAction {
@@ -45,63 +44,65 @@ impl AddAction {
             Ok(ui::ask_password("Enter password to save: "))
         }
     }
-    fn save(&self, creds: &Credential, vault: &mut Box<dyn Vault>) -> anyhow::Result<()> {
-        vault.save_one_credential(creds.clone());
-        println!("Saved.");
-        Ok(())
+    fn get_vault(&self) -> anyhow::Result<Box<dyn Vault>> {
+        if self.is_totp {
+            unlock_totp_vault()
+        } else {
+            unlock()
+        }
     }
-    fn add_credential(&self, vault: &mut Box<dyn Vault>) -> anyhow::Result<(), anyhow::Error> {
+    fn add_credential(&self) -> anyhow::Result<(), anyhow::Error> {
         let password = self.get_password().context(format!(
             "Failed to get password {}",
             if self.clipboard { "from clipboard" } else { "" }
         ))?;
 
         let creds = ui::ask_credentials(&password);
-        self.save(&creds, vault)
-            .context("failed to save")?;
+        let mut vault = self.get_vault().context("Failed to unlock vault")?;
+        vault.save_one_credential(creds.clone());
         if !self.clipboard {
             copy_to_clipboard(&password);
             println!("Password - also copied to clipboard: {}", password);
         };
         Ok(())
     }
-    fn add_payment(&self, vault: &mut Box<dyn Vault>) -> anyhow::Result<()> {
+    fn add_payment(&self) -> anyhow::Result<()> {
         let payment = ui::ask_payment_info();
         println!("Saving...");
+        let mut vault = self.get_vault().context("Failed to unlock vault")?;
         vault.save_payment(payment);
         println!("Payment saved.");
         Ok(())
     }
-    fn add_note(&self, vault: &mut Box<dyn Vault>) -> anyhow::Result<()> {
+    fn add_note(&self) -> anyhow::Result<()> {
         let note = ui::ask_note_info();
         println!("Saving...");
+        let mut vault = self.get_vault().context("Failed to unlock vault")?;
         vault.save_note(&note);
         println!("Note saved.");
         Ok(())
     }
-    fn add_totp(&self, vault: &mut Box<dyn Vault>) -> anyhow::Result<()> {
+    fn add_totp(&self) -> anyhow::Result<()> {
         let totp = ui::ask_totp_info();
         println!("Saving...");
+        let mut vault = self.get_vault().context("Failed to unlock vault")?;
         vault.save_totp(&totp);
         println!("TOTP saved.");
         Ok(())
     }
-}
-
-impl Action for AddAction {}
 
-impl UnlockingAction for AddAction {
-    fn is_totp_vault(&self) -> bool {
-        self.is_totp
+    fn add(&self) -> anyhow::Result<()> {
+        match self.item_type {
+            ItemType::Credential => self.add_credential(),
+            ItemType::Payment => self.add_payment(),
+            ItemType::Note => self.add_note(),
+            ItemType::Totp => self.add_totp(),
+        }
     }
+}
 
-    fn run_with_vault(&self, vault: &mut Box<dyn Vault>) -> anyhow::Result<()> {
-        match self.item_type {
-            ItemType::Credential => self.add_credential(vault)?,
-            ItemType::Payment => self.add_payment(vault)?,
-            ItemType::Note => self.add_note(vault)?,
-            ItemType::Totp => self.add_totp(vault)?
-        };
-        Ok(())
+impl Action for AddAction {
+    fn run(&self) -> anyhow::Result<()> {
+        self.add().context("Failed to add item")
     }
 }

src/actions/delete.rs

@@ -1,5 +1,5 @@
 use clap::ArgMatches;
-use crate::actions::{Action, handle_matches, ItemType, MatchHandlerTemplate, UnlockingAction};
+use crate::actions::{handle_matches, ItemType, MatchHandlerTemplate, UnlockingAction};
 use crate::ui;
 use crate::vault::entities::{Credential, Note, PaymentCard, Totp};
 use crate::vault::vault_trait::Vault;
@@ -206,8 +206,6 @@ fn delete_totp(vault: &mut Box<dyn Vault>) {
     handle_matches(totps, &mut Box::new(DeleteTotpTemplate { vault }));
 }
 
-impl Action for DeleteAction {}
-
 impl UnlockingAction for DeleteAction {
     fn is_totp_vault(&self) -> bool {
         self.is_totp

src/actions/mod.rs

@@ -12,7 +12,6 @@ use anyhow::{bail, Context};
 use clap::ArgMatches;
 use log::{debug, info};
 use std::io;
-use crate::vault::entities::{Credential};
 use crate::vault::keepass_vault::KeepassVault;
 use crate::vault::vault_trait::Vault;
 use anyhow::Result;
@@ -49,13 +48,47 @@ pub trait Action {
     }
 }
 
-pub trait UnlockingAction: Action {
+
+fn get_vault_properties() -> (String, String, Option<String>) {
+    let stored_password = keychain::get_master_password();
+    let master_pwd = stored_password.unwrap_or_else(|_| ui::ask_master_password(None));
+    let filepath = store::get_vault_path();
+    let keyfile_path = store::get_keyfile_path();
+    (master_pwd, filepath, keyfile_path)
+}
+
+fn unlock() -> Result<Box<dyn Vault>> {
+    let (master_pwd, filepath, keyfile_path) = get_vault_properties();
+    get_vault(&master_pwd, &filepath, keyfile_path)
+}
+
+fn unlock_totp_vault() -> Result<Box<dyn Vault>> {
+    let stored_password = keychain::get_totp_master_password();
+    let master_pwd = stored_password.unwrap_or_else(|_| ui::ask_totp_master_password());
+    let filepath = store::get_totp_vault_path();
+    let keyfile_path = store::get_totp_keyfile_path();
+    get_vault(&master_pwd, &filepath, keyfile_path)
+}
+
+fn get_vault(password: &str, filepath: &str, keyfile_path: Option<String>) -> anyhow::Result<Box<dyn Vault>> {
+    // we could return some other Vault implementation here
+    let vault = KeepassVault::new(password, filepath, keyfile_path);
+    match vault {
+        Ok(v) => Ok(Box::new(v)),
+        Err(e) => {
+            bail!("Incorrect password? {}", e.message);
+        }
+    }
+}
+
+
+pub trait UnlockingAction {
     fn execute(&self) {
         info!("Unlocking vault...");
         let result = if self.is_totp_vault() {
-            self.unlock_totp_vault()
+            unlock_totp_vault()
         } else {
-            self.unlock()
+            unlock()
         };
         match result {
             Ok(mut vault) => {
@@ -82,31 +115,6 @@ pub trait UnlockingAction: Action {
         Ok(())
     }
 
-    fn get_vault(&self, password: &str, filepath: &str, keyfile_path: Option<String>) -> anyhow::Result<Box<dyn Vault>> {
-        // we could return some other Vault implementation here
-        let vault = KeepassVault::new(password, filepath, keyfile_path);
-        match vault {
-            Ok(v) => Ok(Box::new(v)),
-            Err(e) => {
-                bail!("Incorrect password? {}", e.message);
-            }
-        }
-    }
-    fn unlock(&self) -> Result<Box<dyn Vault>> {
-        let stored_password = keychain::get_master_password();
-        let master_pwd = stored_password.unwrap_or_else(|_| ui::ask_master_password(None));
-        let filepath = store::get_vault_path();
-        let keyfile_path = store::get_keyfile_path();
-        self.get_vault(&master_pwd, &filepath, keyfile_path)
-    }
-
-    fn unlock_totp_vault(&self) -> Result<Box<dyn Vault>> {
-        let stored_password = keychain::get_totp_master_password();
-        let master_pwd = stored_password.unwrap_or_else(|_| ui::ask_totp_master_password());
-        let filepath = store::get_totp_vault_path();
-        let keyfile_path = store::get_totp_keyfile_path();
-        self.get_vault(&master_pwd, &filepath, keyfile_path)
-    }
 }
 
 pub fn copy_to_clipboard(value: &String) {
@@ -171,8 +179,6 @@ impl ExportAction {
     }
 }
 
-impl Action for ExportAction {}
-
 impl UnlockingAction for ExportAction {
     fn run_with_vault(&self, vault: &mut Box<dyn Vault>) -> anyhow::Result<()> {
         match self.export_csv(vault) {
@@ -204,9 +210,6 @@ fn push_from_csv(vault: &mut Box<dyn Vault>, file_path: &str) -> anyhow::Result<
     Ok(num_imported.try_into().unwrap())
 }
 
-
-impl Action for ImportCsvAction {}
-
 impl UnlockingAction for ImportCsvAction {
     fn run_with_vault(&self, vault: &mut Box<dyn Vault>) -> anyhow::Result<()> {
         match push_from_csv(vault, &self.file_path) {
@@ -217,7 +220,7 @@ impl UnlockingAction for ImportCsvAction {
     }
 }
 
-pub struct GeneratePasswordAction {}
+pub struct GeneratePasswordAction;
 
 impl Action for GeneratePasswordAction {
     fn run(&self) -> anyhow::Result<()> {
@@ -239,8 +242,6 @@ impl Action for LockAction {
 
 pub struct UnlockAction {}
 
-impl Action for UnlockAction {}
-
 impl UnlockingAction for UnlockAction {
     fn run_with_vault(&self, vault: &mut Box<dyn Vault>) -> anyhow::Result<()> {
         keychain::save_master_password(&vault.get_master_password())?;

src/actions/show.rs

@@ -1,6 +1,6 @@
 use clap::ArgMatches;
 use log::debug;
-use crate::actions::{Action, copy_to_clipboard, handle_matches, ItemType, MatchHandlerTemplate, UnlockingAction};
+use crate::actions::{copy_to_clipboard, handle_matches, ItemType, MatchHandlerTemplate, UnlockingAction};
 use crate::ui;
 use crate::vault::entities::{Credential, Note, PaymentCard, Totp};
 use crate::vault::vault_trait::Vault;
@@ -138,7 +138,7 @@ impl MatchHandlerTemplate for ShowTotpTemplate {
 
     fn handle_many_matches(&mut self, matches: Vec<Self::ItemType>) {
         ui::show_totp_table(&matches);
-        
+
         match ui::ask_index(
             "To see the code for one of these OTP authorizers, please enter a row number from the table above, or press q to exit:",
             matches.len() as i16 - 1,
@@ -208,8 +208,6 @@ impl ShowAction {
     }
 }
 
-impl Action for ShowAction {}
-
 impl UnlockingAction for ShowAction {
     fn is_totp_vault(&self) -> bool {
         self.is_totp

src/main.rs

@@ -115,13 +115,13 @@ fn main() {
     }
 
     let action = match matches.subcommand() {
-        // TODO: AddAction should unlock the vault only after the info has been collected from the user
-        Some(("add", sub_matches)) => VaultAction::UnlockingAction(Box::new(AddAction::new(sub_matches))),
+        Some(("add", sub_matches)) => VaultAction::Action(Box::new(AddAction::new(sub_matches))),
         Some(("show", sub_matches)) => VaultAction::UnlockingAction(Box::new(ShowAction::new(sub_matches))),
         Some(("delete", sub_matches)) => VaultAction::UnlockingAction(Box::new(DeleteAction::new(sub_matches))),
         Some(("csv", sub_matches)) => VaultAction::UnlockingAction(Box::new(ImportCsvAction::new(sub_matches))),
         Some(("lock", _)) => VaultAction::Action(Box::new(LockAction {})),
-        // TODO: does unlock need to unlock the VAULT?
+        // Unlock first asks for the master password, then  opens the keepass DB using this password 
+        // to verify that it's OK and finally saves it to the keychain
         Some(("unlock", _)) => VaultAction::UnlockingAction(Box::new(UnlockAction {})),
         Some(("export", sub_matches)) => VaultAction::UnlockingAction(Box::new(ExportAction::new(sub_matches))),
         _ => {

src/ui.rs

@@ -5,7 +5,6 @@ use std::io::Write;
 use std::cmp::min;
 use uuid::Uuid;
 use crate::vault::entities::{Address, Credential, Expiry, Note, PaymentCard, Totp};
-use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS};
 
 pub fn ask(question: &str) -> String {
     print!("{} ", question);

src/vault/keepass_vault.rs

@@ -4,7 +4,6 @@ use keepass_ng::{db::Entry, db::Node, Database, DatabaseKey, error::DatabaseOpen
 use std::fs::{File, OpenOptions};
 use std::path::Path;
 use std::str::FromStr;
-use keepass_ng::db::{TOTP, TOTPAlgorithm};
 use keepass_ng::error::TOTPError;
 use log::{debug, error};
 use uuid::Uuid;
@@ -310,7 +309,7 @@ impl KeepassVault {
     }
 
     fn create_totp_entry(&mut self, parent_uuid: &Uuid, totp: &Totp) -> Result<Option<Uuid>, Error> {
-        Ok(self.db.create_new_entry(parent_uuid.clone(), 0).map(|node| {
+        Ok(self.db.create_new_entry(*parent_uuid, 0).map(|node| {
             node.borrow_mut().as_any_mut().downcast_mut::<Entry>().map(|entry| {
                 entry.set_title(Some(&totp.label));
                 entry.set_otp(&totp.url);