From cbbba37909ecf68770653a6ec4053a4314c284f2 Mon Sep 17 00:00:00 2001 From: Sanchith Hegde <22217505+SanchithHegde@users.noreply.github.com> Date: Tue, 6 Dec 2022 15:19:46 +0530 Subject: [PATCH] refactor: extract email validation and PII utils to `common_utils` crate (#72) --- Cargo.lock | 7 +- crates/common_utils/Cargo.toml | 7 ++ crates/common_utils/src/errors.rs | 22 +++- crates/common_utils/src/lib.rs | 2 + crates/{router => common_utils}/src/pii.rs | 80 ++++++++------ crates/common_utils/src/validation.rs | 102 ++++++++++++++++++ crates/router/Cargo.toml | 8 +- .../src/connector/braintree/transformers.rs | 4 +- crates/router/src/core/errors.rs | 12 +-- crates/router/src/lib.rs | 9 +- crates/router/src/utils.rs | 7 +- crates/router/src/utils/ext_traits.rs | 98 ----------------- 12 files changed, 202 insertions(+), 156 deletions(-) rename crates/{router => common_utils}/src/pii.rs (75%) create mode 100644 crates/common_utils/src/validation.rs diff --git a/Cargo.lock b/Cargo.lock index 530485506b..9e107e0a17 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -897,11 +897,16 @@ version = "0.1.0" dependencies = [ "bytes", "error-stack", + "fake", "masking", + "once_cell", + "proptest", + "regex", "router_env", "serde", "serde_json", "serde_urlencoded", + "thiserror", "time", ] @@ -2625,7 +2630,6 @@ dependencies = [ "dyn-clone", "encoding_rs", "error-stack", - "fake", "fred", "futures", "hex", @@ -2637,7 +2641,6 @@ dependencies = [ "mime", "nanoid", "once_cell", - "proptest", "rand", "redis_interface", "regex", diff --git a/crates/common_utils/Cargo.toml b/crates/common_utils/Cargo.toml index 51e2b6e2d6..09b9f1e422 100644 --- a/crates/common_utils/Cargo.toml +++ b/crates/common_utils/Cargo.toml @@ -7,11 +7,18 @@ edition = "2021" [dependencies] bytes = "1.2.1" error-stack = "0.2.1" +once_cell = "1.16.0" +regex = "1.7.0" serde = { version = "1.0.145", features = ["derive"] } serde_json = "1.0.85" serde_urlencoded = "0.7.1" +thiserror = "1.0.37" time = { version = "0.3.17", features = ["serde", "serde-well-known", "std"] } # First party crates masking = { version = "0.1.0", path = "../masking" } router_env = { version = "0.1.0", path = "../router_env", features = ["log_extra_implicit_fields", "log_custom_entries_to_extra"] } + +[dev-dependencies] +fake = "2.5.0" +proptest = "1.0.0" \ No newline at end of file diff --git a/crates/common_utils/src/errors.rs b/crates/common_utils/src/errors.rs index 97ef47f89d..61d4cbacc9 100644 --- a/crates/common_utils/src/errors.rs +++ b/crates/common_utils/src/errors.rs @@ -1,9 +1,8 @@ -//! -//! errors and error specific types for universal use +//! Errors and error specific types for universal use /// Custom Result /// A custom datatype that wraps the error variant into a report, allowing -/// error_stack::Report specific extendability +/// error_stack::Report specific extendability /// /// Effectively, equivalent to `Result>` /// @@ -38,3 +37,20 @@ macro_rules! impl_error_type { } impl_error_type!(ParsingError, "Parsing error"); + +/// Validation errors. +#[allow(missing_docs)] // Only to prevent warnings about struct fields not being documented +#[derive(Debug, thiserror::Error)] +pub enum ValidationError { + /// The provided input is missing a required field. + #[error("Missing required field: {field_name}")] + MissingRequiredField { field_name: String }, + + /// An incorrect value was provided for the field specified by `field_name`. + #[error("Incorrect value provided for field: {field_name}")] + IncorrectValueProvided { field_name: &'static str }, + + /// An invalid input was provided. + #[error("{message}")] + InvalidValue { message: String }, +} diff --git a/crates/common_utils/src/lib.rs b/crates/common_utils/src/lib.rs index cf55b253cc..d20570b28c 100644 --- a/crates/common_utils/src/lib.rs +++ b/crates/common_utils/src/lib.rs @@ -16,6 +16,8 @@ pub mod custom_serde; pub mod errors; pub mod ext_traits; +pub mod pii; +pub mod validation; /// Date-time utilities. pub mod date_time { diff --git a/crates/router/src/pii.rs b/crates/common_utils/src/pii.rs similarity index 75% rename from crates/router/src/pii.rs rename to crates/common_utils/src/pii.rs index 84ae8a8a8f..c6e7800ace 100644 --- a/crates/router/src/pii.rs +++ b/crates/common_utils/src/pii.rs @@ -1,14 +1,13 @@ -//! //! Personal Identifiable Information protection. -//! use std::{convert::AsRef, fmt}; -#[doc(inline)] -pub use masking::*; +use masking::{Strategy, WithType}; -use crate::utils::validate_email; +use crate::validation::validate_email; +/// Card number +#[derive(Debug)] pub struct CardNumber; impl Strategy for CardNumber @@ -22,32 +21,42 @@ where return WithType::fmt(val, f); } - write!(f, "{}{}", &val_str[..6], "*".repeat(val_str.len() - 6)) + f.write_str(&format!( + "{}{}", + &val_str[..6], + "*".repeat(val_str.len() - 6) + )) } } -//pub struct PhoneNumber; +/* +/// Phone number +#[derive(Debug)] +pub struct PhoneNumber; -//impl Strategy for PhoneNumber -//where -//T: AsRef, -//{ -//fn fmt(val: &T, f: &mut fmt::Formatter<'_>) -> fmt::Result { -//let val_str: &str = val.as_ref(); +impl Strategy for PhoneNumber +where + T: AsRef, +{ + fn fmt(val: &T, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let val_str: &str = val.as_ref(); -//if val_str.len() < 10 || val_str.len() > 12 { -//return WithType::fmt(val, f); -//} + if val_str.len() < 10 || val_str.len() > 12 { + return WithType::fmt(val, f); + } -//f.write_str(&format!( -//"{}{}{}", -//&val_str[..2], -//"*".repeat(val_str.len() - 5), -//&val_str[(val_str.len() - 3)..] -//)) -//} -//} + f.write_str(&format!( + "{}{}{}", + &val_str[..2], + "*".repeat(val_str.len() - 5), + &val_str[(val_str.len() - 3)..] + )) + } +} +*/ +/// Email address +#[derive(Debug)] pub struct Email; impl Strategy for Email @@ -62,14 +71,17 @@ where return WithType::fmt(val, f); } - if let Some((a, b)) = val_str.split_once('@') { - write!(f, "{}@{}", "*".repeat(a.len()), b) - } else { - WithType::fmt(val, f) + let parts: Vec<&str> = val_str.split('@').collect(); + if parts.len() != 2 { + return WithType::fmt(val, f); } + + f.write_str(&format!("{}@{}", "*".repeat(parts[0].len()), parts[1])) } } +/// IP address +#[derive(Debug)] pub struct IpAddress; impl Strategy for IpAddress @@ -90,13 +102,15 @@ where } } - write!(f, "{}.**.**.**", segments[0]) + f.write_str(&format!("{}.**.**.**", segments[0])) } } #[cfg(test)] mod pii_masking_strategy_tests { - use super::{CardNumber, Email, IpAddress, Secret}; + use masking::Secret; + + use super::{CardNumber, Email, IpAddress}; #[test] fn test_valid_card_number_masking() { @@ -110,7 +124,8 @@ mod pii_masking_strategy_tests { assert_eq!("123456****", &format!("{:?}", secret)); } - /* #[test] + /* + #[test] fn test_valid_phone_number_masking() { let secret: Secret = Secret::new("9922992299".to_string()); assert_eq!("99*****299", &format!("{}", secret)); @@ -123,7 +138,8 @@ mod pii_masking_strategy_tests { let secret: Secret = Secret::new("9922992299229922".to_string()); assert_eq!("*** alloc::string::String ***", &format!("{}", secret)); - } */ + } + */ #[test] fn test_valid_email_masking() { diff --git a/crates/common_utils/src/validation.rs b/crates/common_utils/src/validation.rs new file mode 100644 index 0000000000..e8ac7dbe1f --- /dev/null +++ b/crates/common_utils/src/validation.rs @@ -0,0 +1,102 @@ +//! Custom validations for some shared types. + +use error_stack::report; +use once_cell::sync::Lazy; +use regex::Regex; +use router_env::logger; + +use crate::errors::{CustomResult, ValidationError}; + +/// Performs a simple validation against a provided email address. +pub fn validate_email(email: &str) -> CustomResult<(), ValidationError> { + #[deny(clippy::invalid_regex)] + static EMAIL_REGEX: Lazy> = Lazy::new(|| { + match Regex::new( + r"^(?i)[a-z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?(?:\.[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?)*$", + ) { + Ok(regex) => Some(regex), + Err(error) => { + logger::error!(?error); + None + } + } + }); + let email_regex = match EMAIL_REGEX.as_ref() { + Some(regex) => Ok(regex), + None => Err(report!(ValidationError::InvalidValue { + message: "Invalid regex expression".into() + })), + }?; + + const EMAIL_MAX_LENGTH: usize = 319; + if email.is_empty() || email.chars().count() > EMAIL_MAX_LENGTH { + return Err(report!(ValidationError::InvalidValue { + message: "Email address is either empty or exceeds maximum allowed length".into() + })); + } + + if !email_regex.is_match(email) { + return Err(report!(ValidationError::InvalidValue { + message: "Invalid email address format".into() + })); + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use fake::{faker::internet::en::SafeEmail, Fake}; + use proptest::{ + prop_assert, + strategy::{Just, NewTree, Strategy}, + test_runner::TestRunner, + }; + + use super::*; + + #[derive(Debug)] + struct ValidEmail; + + impl Strategy for ValidEmail { + type Tree = Just; + type Value = String; + + fn new_tree(&self, _runner: &mut TestRunner) -> NewTree { + Ok(Just(SafeEmail().fake())) + } + } + + #[test] + fn test_validate_email() { + let result = validate_email("abc@example.com"); + assert!(result.is_ok()); + + let result = validate_email("abc+123@example.com"); + assert!(result.is_ok()); + + let result = validate_email(""); + assert!(result.is_err()); + } + + proptest::proptest! { + /// Example of unit test + #[test] + fn proptest_valid_fake_email(email in ValidEmail) { + prop_assert!(validate_email(&email).is_ok()); + } + + /// Example of unit test + #[test] + fn proptest_invalid_data_email(email in "\\PC*") { + prop_assert!(validate_email(&email).is_err()); + } + + // TODO: make maybe unit test working + // minimal failing input: email = "+@a" + // #[test] + // fn proptest_invalid_email(email in "[.+]@(.+)") { + // prop_assert!(validate_email(&email).is_err()); + // } + } +} diff --git a/crates/router/Cargo.toml b/crates/router/Cargo.toml index f656e0ac93..06bf5394bb 100644 --- a/crates/router/Cargo.toml +++ b/crates/router/Cargo.toml @@ -75,15 +75,13 @@ router_env = { version = "0.1.0", path = "../router_env", features = ["log_extra router_env = { version = "0.1.0", path = "../router_env", default-features = false, features = ["vergen"] } [dev-dependencies] +actix-http = "3.2.2" awc = { version = "3.0.1", features = ["rustls"] } +derive_deref = "1.1.1" +rand = "0.8.5" time = { version = "0.3.14", features = ["macros"] } tokio = "1.21.2" toml = "0.5.9" -derive_deref = "1.1.1" -actix-http = "3.2.2" -proptest = "1.0" -fake = "2.5" -rand = "0.8" [[bin]] name = "router" diff --git a/crates/router/src/connector/braintree/transformers.rs b/crates/router/src/connector/braintree/transformers.rs index 39d740cd72..c100296876 100644 --- a/crates/router/src/connector/braintree/transformers.rs +++ b/crates/router/src/connector/braintree/transformers.rs @@ -40,7 +40,7 @@ pub struct Card { } impl TryFrom<&types::PaymentsAuthorizeRouterData> for BraintreePaymentsRequest { - type Error = error_stack::Report; + type Error = error_stack::Report; fn try_from(item: &types::PaymentsAuthorizeRouterData) -> Result { match item.request.payment_method_data { api::PaymentMethod::Card(ref ccard) => { @@ -62,7 +62,7 @@ impl TryFrom<&types::PaymentsAuthorizeRouterData> for BraintreePaymentsRequest { transaction: braintree_payment_request, }) } - _ => Err(errors::ValidateError.into()), + _ => Err(errors::ConnectorError::RequestEncodingFailed.into()), } } } diff --git a/crates/router/src/core/errors.rs b/crates/router/src/core/errors.rs index d677ef5825..0c1067bfbb 100644 --- a/crates/router/src/core/errors.rs +++ b/crates/router/src/core/errors.rs @@ -5,7 +5,7 @@ pub(crate) mod utils; use std::fmt::Display; use actix_web::{body::BoxBody, http::StatusCode, HttpResponse, ResponseError}; -pub use common_utils::errors::{CustomResult, ParsingError}; +pub use common_utils::errors::{CustomResult, ParsingError, ValidationError}; use config::ConfigError; use error_stack; pub use redis_interface::errors::RedisError; @@ -409,16 +409,6 @@ error_to_process_tracker_error!( ProcessTrackerError::EValidationError(error_stack::Report) ); -#[derive(Debug, thiserror::Error)] -pub enum ValidationError { - #[error("Missing required field: {field_name}")] - MissingRequiredField { field_name: String }, - #[error("Incorrect value provided for field: {field_name}")] - IncorrectValueProvided { field_name: &'static str }, - #[error("{message}")] - InvalidValue { message: String }, -} - #[derive(Debug, thiserror::Error)] pub enum WebhooksFlowError { #[error("Merchant webhook config not found")] diff --git a/crates/router/src/lib.rs b/crates/router/src/lib.rs index 75d8fb3954..1cfd8cc072 100644 --- a/crates/router/src/lib.rs +++ b/crates/router/src/lib.rs @@ -27,7 +27,6 @@ pub mod core; pub mod cors; pub mod db; pub mod env; -pub mod pii; pub mod routes; pub mod scheduler; @@ -61,6 +60,14 @@ pub mod headers { pub const X_API_VERSION: &str = "X-ApiVersion"; } +pub mod pii { + //! Personal Identifiable Information protection. + + pub(crate) use common_utils::pii::{CardNumber, Email, IpAddress}; + #[doc(inline)] + pub use masking::*; +} + pub fn mk_app( state: AppState, request_body_limit: usize, diff --git a/crates/router/src/utils.rs b/crates/router/src/utils.rs index c64b2df699..e5836829a9 100644 --- a/crates/router/src/utils.rs +++ b/crates/router/src/utils.rs @@ -5,11 +5,14 @@ mod fp_utils; #[cfg(feature = "kv_store")] pub(crate) mod storage_partitioning; -pub(crate) use common_utils::ext_traits::{ByteSliceExt, BytesExt, Encode, StringExt, ValueExt}; +pub(crate) use common_utils::{ + ext_traits::{ByteSliceExt, BytesExt, Encode, StringExt, ValueExt}, + validation::validate_email, +}; use nanoid::nanoid; pub(crate) use self::{ - ext_traits::{validate_address, validate_email, OptionExt, ValidateCall}, + ext_traits::{validate_address, OptionExt, ValidateCall}, fp_utils::when, }; use crate::consts; diff --git a/crates/router/src/utils/ext_traits.rs b/crates/router/src/utils/ext_traits.rs index 612544ae85..43bd298781 100644 --- a/crates/router/src/utils/ext_traits.rs +++ b/crates/router/src/utils/ext_traits.rs @@ -1,11 +1,8 @@ use common_utils::ext_traits::ValueExt; use error_stack::{report, IntoReport, Report, ResultExt}; -use once_cell::sync::Lazy; -use regex::Regex; use crate::{ core::errors::{self, ApiErrorResponse, CustomResult, RouterResult}, - logger, types::api::AddressDetails, utils::when, }; @@ -132,42 +129,6 @@ where } } -pub fn validate_email(email: &str) -> CustomResult<(), errors::ValidationError> { - #[deny(clippy::invalid_regex)] - static EMAIL_REGEX: Lazy> = Lazy::new(|| { - match Regex::new( - r"^(?i)[a-z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?(?:\.[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?)*$", - ) { - Ok(regex) => Some(regex), - Err(error) => { - logger::error!(?error); - None - } - } - }); - let email_regex = match EMAIL_REGEX.as_ref() { - Some(regex) => Ok(regex), - None => Err(report!(errors::ValidationError::InvalidValue { - message: "Invalid regex expression".into() - })), - }?; - - const EMAIL_MAX_LENGTH: usize = 319; - if email.is_empty() || email.chars().count() > EMAIL_MAX_LENGTH { - return Err(report!(errors::ValidationError::InvalidValue { - message: "Email address is either empty or exceeds maximum allowed length".into() - })); - } - - if !email_regex.is_match(email) { - return Err(report!(errors::ValidationError::InvalidValue { - message: "Invalid email address format".into() - })); - } - - Ok(()) -} - pub fn validate_address(address: &serde_json::Value) -> CustomResult<(), errors::ValidationError> { if let Err(err) = serde_json::from_value::(address.clone()) { return Err(report!(errors::ValidationError::InvalidValue { @@ -176,62 +137,3 @@ pub fn validate_address(address: &serde_json::Value) -> CustomResult<(), errors: } Ok(()) } - -#[cfg(test)] -mod tests { - use fake::{faker::internet::en::SafeEmail, Fake}; - use proptest::{ - prop_assert, - strategy::{Just, NewTree, Strategy}, - test_runner::TestRunner, - }; - - use super::*; - - #[derive(Debug)] - struct ValidEmail; - - impl Strategy for ValidEmail { - type Tree = Just; - type Value = String; - - fn new_tree(&self, _runner: &mut TestRunner) -> NewTree { - Ok(Just(SafeEmail().fake())) - } - } - - #[test] - fn test_validate_email() { - let result = validate_email("abc@example.com"); - assert!(result.is_ok()); - - let result = validate_email("abc+123@example.com"); - assert!(result.is_ok()); - - let result = validate_email(""); - assert!(result.is_err()); - } - - proptest::proptest! { - /// Example of unit test - /// Kind of test: output-based testing - #[test] - fn proptest_valid_fake_email(email in ValidEmail) { - prop_assert!(validate_email(&email).is_ok()); - } - - /// Example of unit test - /// Kind of test: output-based testing - #[test] - fn proptest_invalid_data_email(email in "\\PC*") { - prop_assert!(validate_email(&email).is_err()); - } - - // TODO: make maybe unit test working - // minimal failing input: email = "+@a" - // #[test] - // fn proptest_invalid_email(email in "[.+]@(.+)") { - // prop_assert!(validate_email(&email).is_err()); - // } - } -}