From 44030f8eccc73b48fc3da898e22c24bbf0bacd8d Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 3 Jun 2024 19:25:58 +0100 Subject: [PATCH] feat: [#618] allow users to change the password TODO: - Validate new password. - Extract duplicate code for pass validation. - Tests --- src/app.rs | 5 ++ src/common.rs | 3 + src/databases/database.rs | 3 + src/databases/mysql.rs | 17 +++++ src/databases/sqlite.rs | 17 +++++ src/services/authentication.rs | 9 +++ src/services/user.rs | 67 ++++++++++++++++++- src/web/api/server/signals.rs | 2 +- src/web/api/server/v1/contexts/user/forms.rs | 2 +- .../api/server/v1/contexts/user/handlers.rs | 15 +++-- 10 files changed, 131 insertions(+), 9 deletions(-) diff --git a/src/app.rs b/src/app.rs index 2272dd4f..93934f6e 100644 --- a/src/app.rs +++ b/src/app.rs @@ -120,6 +120,10 @@ pub async fn run(configuration: Configuration, api_version: &Version) -> Running user_repository.clone(), user_profile_repository.clone(), )); + let profile_service = Arc::new(user::ProfileService::new( + configuration.clone(), + user_authentication_repository.clone(), + )); let ban_service = Arc::new(user::BanService::new( user_repository.clone(), user_profile_repository.clone(), @@ -164,6 +168,7 @@ pub async fn run(configuration: Configuration, api_version: &Version) -> Running settings_service, torrent_index, registration_service, + profile_service, ban_service, )); diff --git a/src/common.rs b/src/common.rs index 7e08e125..2ce59f7d 100644 --- a/src/common.rs +++ b/src/common.rs @@ -49,6 +49,7 @@ pub struct AppData { pub settings_service: Arc, pub torrent_service: Arc, pub registration_service: Arc, + pub profile_service: Arc, pub ban_service: Arc, } @@ -85,6 +86,7 @@ impl AppData { settings_service: Arc, torrent_service: Arc, registration_service: Arc, + profile_service: Arc, ban_service: Arc, ) -> AppData { AppData { @@ -118,6 +120,7 @@ impl AppData { settings_service, torrent_service, registration_service, + profile_service, ban_service, } } diff --git a/src/databases/database.rs b/src/databases/database.rs index ce3843b0..aed86ecb 100644 --- a/src/databases/database.rs +++ b/src/databases/database.rs @@ -131,6 +131,9 @@ pub trait Database: Sync + Send { /// Add new user and return the newly inserted `user_id`. async fn insert_user_and_get_id(&self, username: &str, email: &str, password: &str) -> Result; + /// Change user's password. + async fn change_user_password(&self, user_id: i64, new_password: &str) -> Result<(), Error>; + /// Get `User` from `user_id`. async fn get_user_from_id(&self, user_id: i64) -> Result; diff --git a/src/databases/mysql.rs b/src/databases/mysql.rs index beec1a80..14d69ea2 100644 --- a/src/databases/mysql.rs +++ b/src/databases/mysql.rs @@ -114,6 +114,23 @@ impl Database for Mysql { } } + /// Change user's password. + async fn change_user_password(&self, user_id: i64, new_password: &str) -> Result<(), database::Error> { + query("UPDATE torrust_user_authentication SET password_hash = ? WHERE user_id = ?") + .bind(new_password) + .bind(user_id) + .execute(&self.pool) + .await + .map_err(|_| database::Error::Error) + .and_then(|v| { + if v.rows_affected() > 0 { + Ok(()) + } else { + Err(database::Error::UserNotFound) + } + }) + } + async fn get_user_from_id(&self, user_id: i64) -> Result { query_as::<_, User>("SELECT * FROM torrust_users WHERE user_id = ?") .bind(user_id) diff --git a/src/databases/sqlite.rs b/src/databases/sqlite.rs index a70beac6..ceb6e410 100644 --- a/src/databases/sqlite.rs +++ b/src/databases/sqlite.rs @@ -115,6 +115,23 @@ impl Database for Sqlite { } } + /// Change user's password. + async fn change_user_password(&self, user_id: i64, new_password: &str) -> Result<(), database::Error> { + query("UPDATE torrust_user_authentication SET password_hash = ? WHERE user_id = ?") + .bind(new_password) + .bind(user_id) + .execute(&self.pool) + .await + .map_err(|_| database::Error::Error) + .and_then(|v| { + if v.rows_affected() > 0 { + Ok(()) + } else { + Err(database::Error::UserNotFound) + } + }) + } + async fn get_user_from_id(&self, user_id: i64) -> Result { query_as::<_, User>("SELECT * FROM torrust_users WHERE user_id = ?") .bind(user_id) diff --git a/src/services/authentication.rs b/src/services/authentication.rs index 170cac82..39c73255 100644 --- a/src/services/authentication.rs +++ b/src/services/authentication.rs @@ -183,6 +183,15 @@ impl DbUserAuthenticationRepository { pub async fn get_user_authentication_from_id(&self, user_id: &UserId) -> Result { self.database.get_user_authentication_from_id(*user_id).await } + + /// It changes the user's password. + /// + /// # Errors + /// + /// 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 diff --git a/src/services/user.rs b/src/services/user.rs index dc397c17..5b0c7364 100644 --- a/src/services/user.rs +++ b/src/services/user.rs @@ -10,6 +10,7 @@ use log::{debug, info}; use mockall::automock; use pbkdf2::password_hash::rand_core::OsRng; +use super::authentication::DbUserAuthenticationRepository; use crate::config::{Configuration, EmailOnSignup}; use crate::databases::database::{Database, Error}; use crate::errors::ServiceError; @@ -17,7 +18,7 @@ use crate::mailer; use crate::mailer::VerifyClaims; use crate::models::user::{UserCompact, UserId, UserProfile, Username}; use crate::utils::validation::validate_email_address; -use crate::web::api::server::v1::contexts::user::forms::RegistrationForm; +use crate::web::api::server::v1::contexts::user::forms::{ChangePasswordForm, RegistrationForm}; /// Since user email could be optional, we need a way to represent "no email" /// in the database. This function returns the string that should be used for @@ -186,6 +187,70 @@ impl RegistrationService { } } +pub struct ProfileService { + configuration: Arc, + user_authentication_repository: Arc, +} + +impl ProfileService { + #[must_use] + pub fn new(configuration: Arc, user_repository: Arc) -> Self { + Self { + configuration, + user_authentication_repository: user_repository, + } + } + + /// It registers a new user. + /// + /// # Errors + /// + /// This function will return a: + /// + /// * `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. + /// * An error if unable to successfully hash the password. + /// * An error if unable to change the password in the database. + pub async fn change_password(&self, user_id: UserId, change_password_form: &ChangePasswordForm) -> Result<(), ServiceError> { + info!("changing user password for user ID: {user_id}"); + + let settings = self.configuration.settings.read().await; + + // todo: + // - Validate current password + // - Remove duplicate code for password validation and hashing + + if change_password_form.password != change_password_form.confirm_password { + return Err(ServiceError::PasswordsDontMatch); + } + + let password_length = change_password_form.password.len(); + + if password_length <= settings.auth.min_password_length { + return Err(ServiceError::PasswordTooShort); + } + + if password_length >= settings.auth.max_password_length { + return Err(ServiceError::PasswordTooLong); + } + + let salt = SaltString::generate(&mut OsRng); + + // Argon2 with default params (Argon2id v19) + let argon2 = Argon2::default(); + + // Hash password to PHC string ($argon2id$v=19$...) + let new_password_hash = argon2 + .hash_password(change_password_form.password.as_bytes(), &salt)? + .to_string(); + + self.user_authentication_repository.change_password(user_id, &new_password_hash).await?; + + Ok(()) + } +} + pub struct BanService { user_repository: Arc>, user_profile_repository: Arc, diff --git a/src/web/api/server/signals.rs b/src/web/api/server/signals.rs index 872d6094..5979f58a 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(90))); + handle.graceful_shutdown(Some(Duration::from_secs(2))); println!("!! shuting down in 90 seconds !!"); diff --git a/src/web/api/server/v1/contexts/user/forms.rs b/src/web/api/server/v1/contexts/user/forms.rs index fdcfd0cb..28238539 100644 --- a/src/web/api/server/v1/contexts/user/forms.rs +++ b/src/web/api/server/v1/contexts/user/forms.rs @@ -27,7 +27,7 @@ pub struct JsonWebToken { #[derive(Clone, Debug, Deserialize, Serialize)] pub struct ChangePasswordForm { + pub current_password: String, pub password: String, - pub new_password: String, pub confirm_password: String, } diff --git a/src/web/api/server/v1/contexts/user/handlers.rs b/src/web/api/server/v1/contexts/user/handlers.rs index e6b36ae5..54b3f229 100644 --- a/src/web/api/server/v1/contexts/user/handlers.rs +++ b/src/web/api/server/v1/contexts/user/handlers.rs @@ -10,7 +10,6 @@ use serde::Deserialize; use super::forms::{ChangePasswordForm, JsonWebToken, LoginForm, RegistrationForm}; use super::responses::{self}; use crate::common::AppData; -use crate::errors::ServiceError; use crate::web::api::server::v1::extractors::user_id::ExtractLoggedInUser; use crate::web::api::server::v1::responses::OkResponseData; @@ -133,13 +132,17 @@ pub async fn renew_token_handler( /// - The user account is not found. #[allow(clippy::unused_async)] pub async fn change_password_handler( - State(_app_data): State>, + State(app_data): State>, + ExtractLoggedInUser(user_id): ExtractLoggedInUser, extract::Json(change_password_form): extract::Json, ) -> Response { - - println!("change pass form: {change_password_form:#?}"); - - ServiceError::AccountNotFound.into_response() + match app_data.profile_service.change_password(user_id, &change_password_form).await { + Ok(()) => Json(OkResponseData { + data: format!("Password changed for user with ID: {user_id}"), + }) + .into_response(), + Err(error) => error.into_response(), + } } /// It bans a user from the index.