feat: Add more details to ParamValueInvalid and introduce SaltInvalid error (#713)

Szymongib · web-flow · commit 67f2cb48e2ae · 2021-08-25T13:12:02.000-06:00
diff --git a/password-hash/src/errors.rs b/password-hash/src/errors.rs
@@ -33,7 +33,7 @@ pub enum Error {
     ParamNameInvalid,
 
     /// Invalid parameter value.
-    ParamValueInvalid,
+    ParamValueInvalid(InvalidValue),
 
     /// Maximum number of parameters exceeded.
     ParamsMaxExceeded,
@@ -50,11 +50,8 @@ pub enum Error {
     /// Password hash string too long.
     PhcStringTooLong,
 
-    /// Salt too short.
-    SaltTooShort,
-
-    /// Salt too long.
-    SaltTooLong,
+    /// Salt invalid.
+    SaltInvalid(InvalidValue),
 
     /// Invalid algorithm version.
     Version,
@@ -70,14 +67,13 @@ impl fmt::Display for Error {
             Self::OutputTooLong => f.write_str("PHF output too long (max 64-bytes)"),
             Self::ParamNameDuplicated => f.write_str("duplicate parameter"),
             Self::ParamNameInvalid => f.write_str("invalid parameter name"),
-            Self::ParamValueInvalid => f.write_str("invalid parameter value"),
+            Self::ParamValueInvalid(val_err) => write!(f, "invalid parameter value: {}", val_err),
             Self::ParamsMaxExceeded => f.write_str("maximum number of parameters reached"),
             Self::Password => write!(f, "invalid password"),
             Self::PhcStringInvalid => write!(f, "password hash string invalid"),
             Self::PhcStringTooShort => write!(f, "password hash string too short"),
             Self::PhcStringTooLong => write!(f, "password hash string too long"),
-            Self::SaltTooShort => write!(f, "salt too short"),
-            Self::SaltTooLong => write!(f, "salt too long"),
+            Self::SaltInvalid(val_err) => write!(f, "salt invalid: {}", val_err),
             Self::Version => write!(f, "invalid algorithm version"),
         }
     }
@@ -97,3 +93,25 @@ impl From<base64ct::InvalidLengthError> for Error {
         Error::B64(B64Error::InvalidLength)
     }
 }
+
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+#[non_exhaustive]
+pub enum InvalidValue {
+    ToLong,
+    ToShort,
+    NotProvided,
+    InvalidChar,
+    InvalidFormat,
+}
+
+impl fmt::Display for InvalidValue {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> core::result::Result<(), fmt::Error> {
+        match self {
+            Self::ToLong => f.write_str("value to long"),
+            Self::ToShort => f.write_str("value to short"),
+            Self::NotProvided => f.write_str("required value not provided"),
+            Self::InvalidChar => f.write_str("contains invalid character"),
+            Self::InvalidFormat => f.write_str("value format is invalid"),
+        }
+    }
+}
diff --git a/password-hash/src/params.rs b/password-hash/src/params.rs
@@ -1,5 +1,6 @@
 //! Algorithm parameters.
 
+use crate::errors::InvalidValue;
 use crate::{
     value::{Decimal, Value},
     Error, Ident, Result,
@@ -56,7 +57,9 @@ impl ParamsString {
         value: impl TryInto<Value<'a>>,
     ) -> Result<()> {
         let name = name.try_into().map_err(|_| Error::ParamNameInvalid)?;
-        let value = value.try_into().map_err(|_| Error::ParamValueInvalid)?;
+        let value = value
+            .try_into()
+            .map_err(|_| Error::ParamValueInvalid(InvalidValue::InvalidFormat))?;
         self.add(name, value)
     }
 
@@ -162,11 +165,11 @@ impl FromStr for ParamsString {
             // Validate value
             param
                 .next()
-                .ok_or(Error::ParamValueInvalid)
+                .ok_or(Error::ParamValueInvalid(InvalidValue::NotProvided))
                 .and_then(Value::try_from)?;
 
             if param.next().is_some() {
-                return Err(Error::ParamValueInvalid);
+                return Err(Error::ParamValueInvalid(InvalidValue::NotProvided));
             }
         }
 
diff --git a/password-hash/src/salt.rs b/password-hash/src/salt.rs
@@ -6,6 +6,7 @@ use core::{
     fmt, str,
 };
 
+use crate::errors::InvalidValue;
 #[cfg(feature = "rand_core")]
 use rand_core::{CryptoRng, RngCore};
 
@@ -99,14 +100,17 @@ impl<'a> Salt<'a> {
         let length = input.as_bytes().len();
 
         if length < Self::MIN_LENGTH {
-            return Err(Error::SaltTooShort);
+            return Err(Error::SaltInvalid(InvalidValue::ToShort));
         }
 
         if length > Self::MAX_LENGTH {
-            return Err(Error::SaltTooLong);
+            return Err(Error::SaltInvalid(InvalidValue::ToLong));
         }
 
-        input.try_into().map(Self)
+        input.try_into().map(Self).map_err(|e| match e {
+            Error::ParamValueInvalid(value_err) => Error::SaltInvalid(value_err),
+            err => err,
+        })
     }
 
     /// Attempt to decode a B64-encoded [`Salt`], writing the decoded result
@@ -183,7 +187,7 @@ impl SaltString {
 
     /// Create a new [`SaltString`].
     pub fn new(s: &str) -> Result<Self> {
-        // Assert `s` parses successifully as a `Salt`
+        // Assert `s` parses successfully as a `Salt`
         Salt::new(s)?;
 
         let length = s.as_bytes().len();
@@ -196,7 +200,7 @@ impl SaltString {
                 length: length as u8,
             })
         } else {
-            Err(Error::SaltTooLong)
+            Err(Error::SaltInvalid(InvalidValue::ToLong))
         }
     }
 
@@ -257,6 +261,7 @@ impl<'a> From<&'a SaltString> for Salt<'a> {
 #[cfg(test)]
 mod tests {
     use super::{Error, Salt};
+    use crate::errors::InvalidValue;
 
     #[test]
     fn new_with_valid_min_length_input() {
@@ -276,14 +281,21 @@ mod tests {
     fn reject_new_too_short() {
         for &too_short in &["", "a", "ab", "abc"] {
             let err = Salt::new(too_short).err().unwrap();
-            assert_eq!(err, Error::SaltTooShort);
+            assert_eq!(err, Error::SaltInvalid(InvalidValue::ToShort));
         }
     }
 
     #[test]
     fn reject_new_too_long() {
         let s = "01234567891123456789212345678931234567894123456785234567896234567";
         let err = Salt::new(s).err().unwrap();
-        assert_eq!(err, Error::SaltTooLong);
+        assert_eq!(err, Error::SaltInvalid(InvalidValue::ToLong));
+    }
+
+    #[test]
+    fn reject_new_invalid_char() {
+        let s = "01234_abcd";
+        let err = Salt::new(s).err().unwrap();
+        assert_eq!(err, Error::SaltInvalid(InvalidValue::InvalidChar));
     }
 }
diff --git a/password-hash/src/value.rs b/password-hash/src/value.rs
@@ -13,6 +13,7 @@
 //!
 //! [1]: https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md
 
+use crate::errors::InvalidValue;
 use crate::{Encoding, Error, Result};
 use core::{convert::TryFrom, fmt, str};
 
@@ -50,7 +51,7 @@ impl<'a> Value<'a> {
     /// the PHC string format's rules.
     pub fn new(input: &'a str) -> Result<Self> {
         if input.as_bytes().len() > Self::MAX_LENGTH {
-            return Err(Error::ParamValueInvalid);
+            return Err(Error::ParamValueInvalid(InvalidValue::ToLong));
         }
 
         // Check that the characters are permitted in a PHC parameter value.
@@ -124,26 +125,26 @@ impl<'a> Value<'a> {
 
         // Empty strings aren't decimals
         if value.is_empty() {
-            return Err(Error::ParamValueInvalid);
+            return Err(Error::ParamValueInvalid(InvalidValue::NotProvided));
         }
 
         // Ensure all characters are digits
         for c in value.chars() {
             if !matches!(c, '0'..='9') {
-                return Err(Error::ParamValueInvalid);
+                return Err(Error::ParamValueInvalid(InvalidValue::InvalidChar));
             }
         }
 
         // Disallow leading zeroes
         if value.starts_with('0') && value.len() > 1 {
-            return Err(Error::ParamValueInvalid);
+            return Err(Error::ParamValueInvalid(InvalidValue::InvalidFormat));
         }
 
         value.parse().map_err(|_| {
             // In theory a value overflow should be the only potential error here.
             // When `ParseIntError::kind` is stable it might be good to double check:
             // <https://github.com/rust-lang/rust/issues/22639>
-            Error::ParamValueInvalid
+            Error::ParamValueInvalid(InvalidValue::InvalidFormat)
         })
     }
 
@@ -193,7 +194,7 @@ impl<'a> fmt::Display for Value<'a> {
 fn assert_valid_value(input: &str) -> Result<()> {
     for c in input.chars() {
         if !is_char_valid(c) {
-            return Err(Error::ParamValueInvalid);
+            return Err(Error::ParamValueInvalid(InvalidValue::InvalidChar));
         }
     }
 
@@ -207,7 +208,7 @@ fn is_char_valid(c: char) -> bool {
 
 #[cfg(test)]
 mod tests {
-    use super::{Error, Value};
+    use super::{Error, InvalidValue, Value};
     use core::convert::TryFrom;
 
     // Invalid value examples
@@ -236,21 +237,27 @@ mod tests {
     fn reject_decimal_with_leading_zero() {
         let value = Value::new("01").unwrap();
         let err = u32::try_from(value).err().unwrap();
-        assert!(matches!(err, Error::ParamValueInvalid));
+        assert!(matches!(
+            err,
+            Error::ParamValueInvalid(InvalidValue::InvalidFormat)
+        ));
     }
 
     #[test]
     fn reject_overlong_decimal() {
         let value = Value::new("4294967296").unwrap();
         let err = u32::try_from(value).err().unwrap();
-        assert_eq!(err, Error::ParamValueInvalid);
+        assert_eq!(err, Error::ParamValueInvalid(InvalidValue::InvalidFormat));
     }
 
     #[test]
     fn reject_negative() {
         let value = Value::new("-1").unwrap();
         let err = u32::try_from(value).err().unwrap();
-        assert!(matches!(err, Error::ParamValueInvalid));
+        assert!(matches!(
+            err,
+            Error::ParamValueInvalid(InvalidValue::InvalidChar)
+        ));
     }
 
     //
@@ -278,18 +285,21 @@ mod tests {
     #[test]
     fn reject_invalid_char() {
         let err = Value::new(INVALID_CHAR).err().unwrap();
-        assert!(matches!(err, Error::ParamValueInvalid));
+        assert!(matches!(
+            err,
+            Error::ParamValueInvalid(InvalidValue::InvalidChar)
+        ));
     }
 
     #[test]
     fn reject_too_long() {
         let err = Value::new(INVALID_TOO_LONG).err().unwrap();
-        assert_eq!(err, Error::ParamValueInvalid);
+        assert_eq!(err, Error::ParamValueInvalid(InvalidValue::ToLong));
     }
 
     #[test]
     fn reject_invalid_char_and_too_long() {
         let err = Value::new(INVALID_CHAR_AND_TOO_LONG).err().unwrap();
-        assert_eq!(err, Error::ParamValueInvalid);
+        assert_eq!(err, Error::ParamValueInvalid(InvalidValue::ToLong));
     }
 }
