From 95e50190b578254782bd47663a1f4c71b5238d8b Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 4 Jun 2024 12:07:24 +0100 Subject: [PATCH] feat: [#618] check current password before changing it This is an extra security check. Before changing the password the user must provide the current one. --- src/errors.rs | 3 +++ src/services/authentication.rs | 14 +++++------ src/services/user.rs | 19 +++++++++++---- src/web/api/server/signals.rs | 2 +- .../e2e/web/api/v1/contexts/user/contract.rs | 23 +++++++++++++++++++ 5 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 9723b4f7..00c79ecf 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -30,6 +30,8 @@ pub enum ServiceError { #[display(fmt = "Invalid username/email or password")] WrongPasswordOrUsername, + #[display(fmt = "Invalid password")] + InvalidPassword, #[display(fmt = "Username not found")] UsernameNotFound, #[display(fmt = "User not found")] @@ -273,6 +275,7 @@ pub fn http_status_code_for_service_error(error: &ServiceError) -> StatusCode { ServiceError::EmailInvalid => StatusCode::BAD_REQUEST, ServiceError::NotAUrl => StatusCode::BAD_REQUEST, ServiceError::WrongPasswordOrUsername => StatusCode::FORBIDDEN, + ServiceError::InvalidPassword => StatusCode::FORBIDDEN, ServiceError::UsernameNotFound => StatusCode::NOT_FOUND, ServiceError::UserNotFound => StatusCode::NOT_FOUND, ServiceError::AccountNotFound => StatusCode::NOT_FOUND, diff --git a/src/services/authentication.rs b/src/services/authentication.rs index 39c73255..cd9dd9d3 100644 --- a/src/services/authentication.rs +++ b/src/services/authentication.rs @@ -65,7 +65,7 @@ impl Service { .await .map_err(|_| ServiceError::InternalServerError)?; - verify_password(password.as_bytes(), &user_authentication)?; + verify_password(password.as_bytes(), &user_authentication).map_err(|_| ServiceError::WrongPasswordOrUsername)?; let settings = self.configuration.settings.read().await; @@ -191,7 +191,7 @@ impl DbUserAuthenticationRepository { /// It returns an error if there is a database error. pub async fn change_password(&self, user_id: UserId, password_hash: &str) -> Result<(), Error> { self.database.change_user_password(user_id, password_hash).await - } + } } /// Verify if the user supplied and the database supplied passwords match @@ -199,27 +199,27 @@ impl DbUserAuthenticationRepository { /// # Errors /// /// This function will return an error if unable to parse password hash from the stored user authentication value. -/// This function will return a `ServiceError::WrongPasswordOrUsername` if unable to match the password with either `argon2id` or `pbkdf2-sha256`. -fn verify_password(password: &[u8], user_authentication: &UserAuthentication) -> Result<(), ServiceError> { +/// This function will return a `ServiceError::InvalidPassword` if unable to match the password with either `argon2id` or `pbkdf2-sha256`. +pub fn verify_password(password: &[u8], user_authentication: &UserAuthentication) -> Result<(), ServiceError> { // wrap string of the hashed password into a PasswordHash struct for verification let parsed_hash = PasswordHash::new(&user_authentication.password_hash)?; match parsed_hash.algorithm.as_str() { "argon2id" => { if Argon2::default().verify_password(password, &parsed_hash).is_err() { - return Err(ServiceError::WrongPasswordOrUsername); + return Err(ServiceError::InvalidPassword); } Ok(()) } "pbkdf2-sha256" => { if Pbkdf2.verify_password(password, &parsed_hash).is_err() { - return Err(ServiceError::WrongPasswordOrUsername); + return Err(ServiceError::InvalidPassword); } Ok(()) } - _ => Err(ServiceError::WrongPasswordOrUsername), + _ => Err(ServiceError::InvalidPassword), } } diff --git a/src/services/user.rs b/src/services/user.rs index d260fc45..f18ecc52 100644 --- a/src/services/user.rs +++ b/src/services/user.rs @@ -17,6 +17,7 @@ use crate::errors::ServiceError; use crate::mailer; use crate::mailer::VerifyClaims; use crate::models::user::{UserCompact, UserId, UserProfile, Username}; +use crate::services::authentication::verify_password; use crate::utils::validation::validate_email_address; use crate::web::api::server::v1::contexts::user::forms::{ChangePasswordForm, RegistrationForm}; @@ -100,7 +101,7 @@ impl RegistrationService { max_password_length: settings.auth.max_password_length, }; - validate_password( + validate_password_constrains( ®istration_form.password, ®istration_form.confirm_password, &password_constraints, @@ -196,6 +197,7 @@ impl ProfileService { /// /// This function will return a: /// + /// * `ServiceError::InvalidPassword` if the current password supplied is invalid. /// * `ServiceError::PasswordsDontMatch` if the supplied passwords do not match. /// * `ServiceError::PasswordTooShort` if the supplied password is too short. /// * `ServiceError::PasswordTooLong` if the supplied password is too long. @@ -206,14 +208,19 @@ impl ProfileService { let settings = self.configuration.settings.read().await; - // todo: guard that current password matches the one provided in change password form + let user_authentication = self + .user_authentication_repository + .get_user_authentication_from_id(&user_id) + .await?; + + verify_password(change_password_form.current_password.as_bytes(), &user_authentication)?; let password_constraints = PasswordConstraints { min_password_length: settings.auth.min_password_length, max_password_length: settings.auth.max_password_length, }; - validate_password( + validate_password_constrains( &change_password_form.password, &change_password_form.confirm_password, &password_constraints, @@ -413,7 +420,11 @@ struct PasswordConstraints { pub max_password_length: usize, } -fn validate_password(password: &str, confirm_password: &str, password_rules: &PasswordConstraints) -> Result<(), ServiceError> { +fn validate_password_constrains( + password: &str, + confirm_password: &str, + password_rules: &PasswordConstraints, +) -> Result<(), ServiceError> { if password != confirm_password { return Err(ServiceError::PasswordsDontMatch); } diff --git a/src/web/api/server/signals.rs b/src/web/api/server/signals.rs index 5979f58a..872d6094 100644 --- a/src/web/api/server/signals.rs +++ b/src/web/api/server/signals.rs @@ -23,7 +23,7 @@ pub async fn graceful_shutdown(handle: axum_server::Handle, rx_halt: tokio::sync shutdown_signal_with_message(rx_halt, message).await; info!("Sending graceful shutdown signal"); - handle.graceful_shutdown(Some(Duration::from_secs(2))); + handle.graceful_shutdown(Some(Duration::from_secs(90))); println!("!! shuting down in 90 seconds !!"); diff --git a/tests/e2e/web/api/v1/contexts/user/contract.rs b/tests/e2e/web/api/v1/contexts/user/contract.rs index 14ea0490..3124fc28 100644 --- a/tests/e2e/web/api/v1/contexts/user/contract.rs +++ b/tests/e2e/web/api/v1/contexts/user/contract.rs @@ -118,6 +118,29 @@ mod authentication { assert_successful_login_response(&response, &logged_in_user.username); } + #[tokio::test] + async fn it_should_fail_changing_the_password_if_the_user_does_not_provide_the_current_password() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; + + let logged_in_user = new_logged_in_user(&env).await; + + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &logged_in_user.token); + + let response = client + .change_password( + Username::new(logged_in_user.username.clone()), + ChangePasswordForm { + current_password: "INVALID PASSWORD".to_string(), + password: VALID_PASSWORD.to_string(), + confirm_password: VALID_PASSWORD.to_string(), + }, + ) + .await; + + assert_eq!(response.status, 403); + } + #[tokio::test] async fn it_should_allow_a_logged_in_user_to_verify_an_authentication_token() { let mut env = TestEnv::new();