Case insensitive secrets + improved secret error messages (#453)

This PR permits case insensitive Base32 or hex secrets (in case of Motp
codes).
I have also improved error messages in case of a bad secret value.

Author: replydev

Cargo.lock

@@ -53,3 +53,4 @@ crossterm = "0.27.0"
 url = "2.5.2"
 color-eyre = "0.6.3"
 enum_dispatch = "0.3.13"
+derive_builder = "0.20.0"

Cargo.toml

@@ -1,12 +1,12 @@
 use clap::{value_parser, Args};
-use color_eyre::eyre::{eyre, ErrReport};
+use color_eyre::eyre::{ErrReport, Result};
 
 use zeroize::Zeroize;
 
 use crate::otp::{
     from_otp_uri::FromOtpUri,
     otp_algorithm::OTPAlgorithm,
-    otp_element::{OTPDatabase, OTPElement},
+    otp_element::{OTPDatabase, OTPElement, OTPElementBuilder},
     otp_type::OTPType,
 };
 
@@ -72,9 +72,6 @@ impl SubcommandExecutor for AddArgs {
         } else {
             get_from_args(self)?
         };
-        if !otp_element.valid_secret() {
-            return Err(eyre!("Invalid secret."));
-        }
 
         database.add_element(otp_element);
         Ok(database)
@@ -83,19 +80,19 @@ impl SubcommandExecutor for AddArgs {
 
 fn get_from_args(matches: AddArgs) -> color_eyre::Result<OTPElement> {
     let secret = rpassword::prompt_password("Insert the secret: ").map_err(ErrReport::from)?;
-    Ok(map_args_to_code(secret, matches))
+    map_args_to_code(secret, matches).map_err(ErrReport::from)
 }
 
-fn map_args_to_code(secret: String, matches: AddArgs) -> OTPElement {
-    OTPElement {
-        secret,
-        issuer: matches.issuer,
-        label: matches.label.unwrap(),
-        digits: matches.digits,
-        type_: matches.otp_type,
-        algorithm: matches.algorithm,
-        period: matches.period,
-        counter: matches.counter,
-        pin: matches.pin,
-    }
+fn map_args_to_code(secret: String, matches: AddArgs) -> Result<OTPElement> {
+    OTPElementBuilder::default()
+        .secret(secret)
+        .issuer(matches.issuer)
+        .label(matches.label.unwrap())
+        .digits(matches.digits)
+        .type_(matches.otp_type)
+        .algorithm(matches.algorithm)
+        .period(matches.period)
+        .counter(matches.counter)
+        .pin(matches.pin)
+        .build()
 }

src/arguments/add.rs

@@ -13,6 +13,12 @@ pub enum OTPAlgorithm {
     Md5,
 }
 
+impl Default for OTPAlgorithm {
+    fn default() -> Self {
+        Self::Sha1
+    }
+}
+
 impl fmt::Display for OTPAlgorithm {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         let to_string = match self {

src/otp/otp_algorithm.rs

@@ -1,4 +1,5 @@
-use color_eyre::eyre::ErrReport;
+use color_eyre::eyre::{eyre, ErrReport};
+use derive_builder::Builder;
 use std::{fs::File, io::Write, vec};
 
 use crate::crypto::cryptography::{argon_derive_key, encrypt_string_with_key, gen_salt};
@@ -126,17 +127,30 @@ impl OTPDatabase {
     }
 }
 
-#[derive(Serialize, Deserialize, Clone, PartialEq, Eq, Debug, Hash, Zeroize, ZeroizeOnDrop)]
+#[derive(
+    Serialize, Deserialize, Builder, Clone, PartialEq, Eq, Debug, Hash, Zeroize, ZeroizeOnDrop,
+)]
+#[builder(
+    setter(into),
+    build_fn(validate = "Self::validate", error = "ErrReport")
+)]
 pub struct OTPElement {
+    #[builder(setter(custom))]
     pub secret: String,
     pub issuer: String,
     pub label: String,
+    #[builder(default = "6")]
     pub digits: u64,
     #[serde(rename = "type")]
+    #[builder(setter(custom), default)]
     pub type_: OTPType,
+    #[builder(default)]
     pub algorithm: OTPAlgorithm,
+    #[builder(default = "30")]
     pub period: u64,
+    #[builder(setter(into), default)]
     pub counter: Option<u64>,
+    #[builder(setter(into), default)]
     pub pin: Option<String>,
 }
 
@@ -212,11 +226,50 @@ impl OTPElement {
         let s = (value % exponential).to_string();
         Ok("0".repeat(self.digits as usize - s.chars().count()) + s.as_str())
     }
+}
 
-    pub fn valid_secret(&self) -> bool {
-        match self.type_ {
-            OTPType::Motp => hex::decode(&self.secret).is_ok(),
-            _ => BASE32_NOPAD.decode(self.secret.as_bytes()).is_ok(),
+impl OTPElementBuilder {
+    /// Makes the secret insertion case insensitive
+    pub fn secret<VALUE: Into<String>>(&mut self, value: VALUE) -> &mut Self {
+        self.secret = Some(value.into().to_uppercase());
+        self
+    }
+
+    /// Makes the secret insertion case insensitive
+    pub fn type_<VALUE: Into<OTPType>>(&mut self, value: VALUE) -> &mut Self {
+        let otp_type: OTPType = value.into();
+
+        if otp_type == OTPType::Motp {
+            // Motp codes must be lowercase
+            self.secret = self.secret.as_ref().map(|s| s.to_lowercase())
+        } else {
+            // Base32 codes must be uppercase
+            self.secret = self.secret.as_ref().map(|s| s.to_uppercase())
+        }
+
+        self.type_ = Some(otp_type);
+        self
+    }
+
+    /// Check if the OTPElement is valid
+    fn validate(&self) -> Result<(), ErrReport> {
+        if self.secret.is_none() {
+            return Err(eyre!("Secret must be set",));
+        }
+
+        if self.secret.as_ref().unwrap().is_empty() {
+            return Err(eyre!("Secret must not be empty",));
+        }
+
+        // Validate secret encoding
+        match self.type_.unwrap_or_default() {
+            OTPType::Motp => hex::decode(self.secret.as_ref().unwrap())
+                .map(|_| {})
+                .map_err(|e| eyre!("Invalid hex secret: {e}")),
+            _ => BASE32_NOPAD
+                .decode(self.secret.as_ref().unwrap().as_bytes())
+                .map(|_| {})
+                .map_err(|e| eyre!("Invalid BASE32 secret: {e}")),
         }
     }
 }
@@ -230,11 +283,12 @@ fn get_label(issuer: &str, label: &str) -> String {
 #[cfg(test)]
 mod test {
     use crate::otp::otp_element::OTPAlgorithm::Sha1;
-    use crate::otp::otp_element::OTPElement;
     use crate::otp::otp_element::OTPType::Totp;
+    use crate::otp::otp_element::{OTPElement, OTPElementBuilder};
 
     use crate::otp::from_otp_uri::FromOtpUri;
     use crate::otp::otp_error::OtpError;
+    use crate::otp::otp_type::OTPType;
 
     #[test]
     fn test_serialization_otp_uri_full_element() {
@@ -315,4 +369,58 @@ mod test {
         // Assert
         assert_eq!(Err(OtpError::InvalidDigits), result)
     }
+
+    #[test]
+    fn test_lowercase_secret() {
+        // Arrange / Act
+        let result = OTPElementBuilder::default()
+            .secret("aa")
+            .label("label")
+            .issuer("")
+            .build();
+
+        // Assert
+        assert_eq!("AA", result.unwrap().secret);
+    }
+
+    #[test]
+    fn test_invalid_secret_base32() {
+        let result = OTPElementBuilder::default()
+            .secret("aaa")
+            .label("label")
+            .issuer("")
+            .build();
+
+        assert_eq!(
+            "Invalid BASE32 secret: invalid length at 2",
+            result.unwrap_err().to_string()
+        )
+    }
+
+    #[test]
+    fn valid_hex_secret() {
+        let result = OTPElementBuilder::default()
+            .secret("aAAf")
+            .label("label")
+            .issuer("")
+            .type_(OTPType::Motp)
+            .build();
+
+        assert_eq!("aaaf", result.unwrap().secret)
+    }
+
+    #[test]
+    fn invalid_secret_hex() {
+        let result = OTPElementBuilder::default()
+            .secret("aaa")
+            .label("label")
+            .issuer("")
+            .type_(OTPType::Motp)
+            .build();
+
+        assert_eq!(
+            "Invalid hex secret: Odd number of digits",
+            result.unwrap_err().to_string()
+        )
+    }
 }

src/otp/otp_element.rs

@@ -24,6 +24,12 @@ pub enum OTPType {
     Motp,
 }
 
+impl Default for OTPType {
+    fn default() -> Self {
+        Self::Totp
+    }
+}
+
 impl fmt::Display for OTPType {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         let to_string = match self {