Merge #980: Do not allow invalid tracker keys

e81914b refactor: [#976] concrete errors for parsing keys (Jose Celano)
8d41d18 fix: [#976] do not allow invalid tracker keys (Jose Celano)

Pull request description:

  Do not allow invalid tracker keys. Keys cannot be constructed if they:

  - Are longer than 32 chars.
  - Contain non-alphanumeric ASCII chars.

ACKs for top commit:
  josecelano:
    ACK e81914b

Tree-SHA512: 4916e50c0b853173bd8b28e9cbe7f1b9b707e413ae5f164339427710a1d5482575cb2ba8fee430341503d24a8b8db48ba25fd8de4e0333a8d14f83681783a483

Author: josecelano

src/core/auth.rs

@@ -131,13 +131,37 @@ impl ExpiringKey {
     }
 }
 
-/// A randomly generated token used for authentication.
+/// A token used for authentication.
 ///
-/// It contains lower and uppercase letters and numbers.
-/// It's a 32-char string.
+/// - It contains only ascii alphanumeric chars: lower and uppercase letters and
+///   numbers.
+/// - It's a 32-char string.
 #[derive(Serialize, Deserialize, Debug, Eq, PartialEq, Clone, Display, Hash)]
 pub struct Key(String);
 
+impl Key {
+    /// # Errors
+    ///
+    /// Will return an error is the string represents an invalid key.
+    /// Valid keys can only contain 32 chars including 0-9, a-z and A-Z.
+    pub fn new(value: &str) -> Result<Self, ParseKeyError> {
+        if value.len() != AUTH_KEY_LENGTH {
+            return Err(ParseKeyError::InvalidKeyLength);
+        }
+
+        if !value.chars().all(|c| c.is_ascii_alphanumeric()) {
+            return Err(ParseKeyError::InvalidChars);
+        }
+
+        Ok(Self(value.to_owned()))
+    }
+
+    #[must_use]
+    pub fn value(&self) -> &str {
+        &self.0
+    }
+}
+
 /// Error returned when a key cannot be parsed from a string.
 ///
 /// ```rust,no_run
@@ -151,24 +175,27 @@ pub struct Key(String);
 /// assert_eq!(key.unwrap().to_string(), key_string);
 /// ```
 ///
-/// If the string does not contains a valid key, the parser function will return this error.
-#[derive(Debug, PartialEq, Eq, Display)]
-pub struct ParseKeyError;
+/// If the string does not contains a valid key, the parser function will return
+/// this error.
+#[derive(Debug, Error)]
+pub enum ParseKeyError {
+    #[error("Invalid key length. Key must be have 32 chars")]
+    InvalidKeyLength,
+    #[error("Invalid chars for key. Key can only alphanumeric chars (0-9, a-z, A-Z)")]
+    InvalidChars,
+}
 
 impl FromStr for Key {
     type Err = ParseKeyError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        if s.len() != AUTH_KEY_LENGTH {
-            return Err(ParseKeyError);
-        }
-
+        Key::new(s)?;
         Ok(Self(s.to_string()))
     }
 }
 
-/// Verification error. Error returned when an [`ExpiringKey`] cannot be verified with the [`verify(...)`](crate::core::auth::verify) function.
-///
+/// Verification error. Error returned when an [`ExpiringKey`] cannot be
+/// verified with the [`verify(...)`](crate::core::auth::verify) function.
 #[derive(Debug, Error)]
 #[allow(dead_code)]
 pub enum Error {
@@ -209,6 +236,22 @@ mod tests {
             assert!(key.is_ok());
             assert_eq!(key.unwrap().to_string(), key_string);
         }
+
+        #[test]
+        fn length_should_be_32() {
+            let key = Key::new("");
+            assert!(key.is_err());
+
+            let string_longer_than_32 = "012345678901234567890123456789012"; // DevSkim: ignore  DS173237
+            let key = Key::new(string_longer_than_32);
+            assert!(key.is_err());
+        }
+
+        #[test]
+        fn should_only_include_alphanumeric_chars() {
+            let key = Key::new("%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%");
+            assert!(key.is_err());
+        }
     }
 
     mod expiring_auth_key {

tests/servers/api/v1/asserts.rs

@@ -61,6 +61,12 @@ pub async fn assert_bad_request(response: Response, body: &str) {
     assert_eq!(response.text().await.unwrap(), body);
 }
 
+pub async fn assert_bad_request_with_text(response: Response, text: &str) {
+    assert_eq!(response.status(), 400);
+    assert_eq!(response.headers().get("content-type").unwrap(), "text/plain; charset=utf-8");
+    assert!(response.text().await.unwrap().contains(text));
+}
+
 pub async fn assert_unprocessable_content(response: Response, text: &str) {
     assert_eq!(response.status(), 422);
     assert_eq!(response.headers().get("content-type").unwrap(), "text/plain; charset=utf-8");
@@ -93,20 +99,9 @@ pub async fn assert_invalid_auth_key_get_param(response: Response, invalid_auth_
 }
 
 pub async fn assert_invalid_auth_key_post_param(response: Response, invalid_auth_key: &str) {
-    assert_bad_request(
+    assert_bad_request_with_text(
         response,
-        &format!(
-            "Invalid URL: invalid auth key: string \"{}\", ParseKeyError",
-            &invalid_auth_key
-        ),
-    )
-    .await;
-}
-
-pub async fn _assert_unprocessable_auth_key_param(response: Response, _invalid_value: &str) {
-    assert_unprocessable_content(
-        response,
-        "Failed to deserialize the JSON body into the target type: seconds_valid: invalid type",
+        &format!("Invalid URL: invalid auth key: string \"{}\"", &invalid_auth_key),
     )
     .await;
 }

tests/servers/api/v1/contract/context/auth_key.rs

@@ -136,10 +136,10 @@ async fn should_fail_generating_a_new_auth_key_when_the_provided_key_is_invalid(
     let invalid_keys = [
         // "", it returns 404
         // " ", it returns 404
-        "-1",      // Not a string
-        "invalid", // Invalid string
-        "GQEs2ZNcCm9cwEV9dBpcPB5OwNFWFiR", // Not a 32-char string
-                   // "%QEs2ZNcCm9cwEV9dBpcPB5OwNFWFiRd", // Invalid char. todo: this doesn't fail
+        "-1",                               // Not a string
+        "invalid",                          // Invalid string
+        "GQEs2ZNcCm9cwEV9dBpcPB5OwNFWFiR",  // Not a 32-char string
+        "%QEs2ZNcCm9cwEV9dBpcPB5OwNFWFiRd", // Invalid char.
     ];
 
     for invalid_key in invalid_keys {