From 4ffaa9343103fe692d26be8faa21d477abb9c7cf Mon Sep 17 00:00:00 2001 From: Nutomic Date: Tue, 21 May 2024 20:46:49 +0200 Subject: [PATCH 1/2] Dont allow reusing password reset token, use normal rate limit (#4719) * Dont allow reusing password reset token, use normal rate limit * fix --- .../local_user/change_password_after_reset.rs | 2 +- crates/api/src/local_user/reset_password.rs | 10 -- crates/api_common/src/utils.rs | 2 +- .../src/impls/password_reset_request.rs | 128 +++++++----------- crates/utils/src/error.rs | 1 - src/api_routes_http.rs | 6 +- 6 files changed, 54 insertions(+), 95 deletions(-) diff --git a/crates/api/src/local_user/change_password_after_reset.rs b/crates/api/src/local_user/change_password_after_reset.rs index 9d693a750..f86421796 100644 --- a/crates/api/src/local_user/change_password_after_reset.rs +++ b/crates/api/src/local_user/change_password_after_reset.rs @@ -19,7 +19,7 @@ pub async fn change_password_after_reset( ) -> LemmyResult> { // Fetch the user_id from the token let token = data.token.clone(); - let local_user_id = PasswordResetRequest::read_from_token(&mut context.pool(), &token) + let local_user_id = PasswordResetRequest::read_and_delete(&mut context.pool(), &token) .await? .ok_or(LemmyErrorType::TokenNotFound)? .local_user_id; diff --git a/crates/api/src/local_user/reset_password.rs b/crates/api/src/local_user/reset_password.rs index edb5adee6..b6b113c07 100644 --- a/crates/api/src/local_user/reset_password.rs +++ b/crates/api/src/local_user/reset_password.rs @@ -6,7 +6,6 @@ use lemmy_api_common::{ utils::send_password_reset_email, SuccessResponse, }; -use lemmy_db_schema::source::password_reset_request::PasswordResetRequest; use lemmy_db_views::structs::{LocalUserView, SiteView}; use lemmy_utils::error::{LemmyErrorType, LemmyResult}; @@ -21,15 +20,6 @@ pub async fn reset_password( .await? .ok_or(LemmyErrorType::IncorrectLogin)?; - // Check for too many attempts (to limit potential abuse) - let recent_resets_count = PasswordResetRequest::get_recent_password_resets_count( - &mut context.pool(), - local_user_view.local_user.id, - ) - .await?; - if recent_resets_count >= 3 { - Err(LemmyErrorType::PasswordResetLimitReached)? - } let site_view = SiteView::read_local(&mut context.pool()) .await? .ok_or(LemmyErrorType::LocalSiteNotSetup)?; diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 4ab587928..7db4c56ea 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -440,7 +440,7 @@ pub async fn send_password_reset_email( // Insert the row after successful send, to avoid using daily reset limit while // email sending is broken. let local_user_id = user.local_user.id; - PasswordResetRequest::create_token(pool, local_user_id, token.clone()).await?; + PasswordResetRequest::create(pool, local_user_id, token.clone()).await?; Ok(()) } diff --git a/crates/db_schema/src/impls/password_reset_request.rs b/crates/db_schema/src/impls/password_reset_request.rs index 4fad632f5..c92cb0867 100644 --- a/crates/db_schema/src/impls/password_reset_request.rs +++ b/crates/db_schema/src/impls/password_reset_request.rs @@ -1,49 +1,22 @@ use crate::{ diesel::OptionalExtension, newtypes::LocalUserId, - schema::password_reset_request::dsl::{local_user_id, password_reset_request, published, token}, + schema::password_reset_request::dsl::{password_reset_request, published, token}, source::password_reset_request::{PasswordResetRequest, PasswordResetRequestForm}, - traits::Crud, utils::{get_conn, DbPool}, }; use diesel::{ + delete, dsl::{insert_into, now, IntervalDsl}, result::Error, sql_types::Timestamptz, ExpressionMethods, IntoSql, - QueryDsl, }; use diesel_async::RunQueryDsl; -#[async_trait] -impl Crud for PasswordResetRequest { - type InsertForm = PasswordResetRequestForm; - type UpdateForm = PasswordResetRequestForm; - type IdType = i32; - - async fn create(pool: &mut DbPool<'_>, form: &PasswordResetRequestForm) -> Result { - let conn = &mut get_conn(pool).await?; - insert_into(password_reset_request) - .values(form) - .get_result::(conn) - .await - } - async fn update( - pool: &mut DbPool<'_>, - password_reset_request_id: i32, - form: &PasswordResetRequestForm, - ) -> Result { - let conn = &mut get_conn(pool).await?; - diesel::update(password_reset_request.find(password_reset_request_id)) - .set(form) - .get_result::(conn) - .await - } -} - impl PasswordResetRequest { - pub async fn create_token( + pub async fn create( pool: &mut DbPool<'_>, from_local_user_id: LocalUserId, token_: String, @@ -52,30 +25,21 @@ impl PasswordResetRequest { local_user_id: from_local_user_id, token: token_.into(), }; - - Self::create(pool, &form).await - } - pub async fn read_from_token(pool: &mut DbPool<'_>, token_: &str) -> Result, Error> { let conn = &mut get_conn(pool).await?; - password_reset_request + insert_into(password_reset_request) + .values(form) + .get_result::(conn) + .await + } + + pub async fn read_and_delete(pool: &mut DbPool<'_>, token_: &str) -> Result, Error> { + let conn = &mut get_conn(pool).await?; + delete(password_reset_request) .filter(token.eq(token_)) .filter(published.gt(now.into_sql::() - 1.days())) - .first(conn) - .await - .optional() - } - - pub async fn get_recent_password_resets_count( - pool: &mut DbPool<'_>, - user_id: LocalUserId, - ) -> Result { - let conn = &mut get_conn(pool).await?; - password_reset_request - .filter(local_user_id.eq(user_id)) - .filter(published.gt(now.into_sql::() - 1.days())) - .count() .get_result(conn) .await + .optional() } } @@ -94,62 +58,64 @@ mod tests { traits::Crud, utils::build_db_pool_for_tests, }; + use lemmy_utils::error::LemmyResult; use pretty_assertions::assert_eq; use serial_test::serial; #[tokio::test] #[serial] - async fn test_crud() { + async fn test_password_reset() -> LemmyResult<()> { let pool = &build_db_pool_for_tests().await; let pool = &mut pool.into(); - let inserted_instance = Instance::read_or_create(pool, "my_domain.tld".to_string()) - .await - .unwrap(); - + // Setup + let inserted_instance = Instance::read_or_create(pool, "my_domain.tld".to_string()).await?; let new_person = PersonInsertForm::builder() .name("thommy prw".into()) .public_key("pubkey".to_string()) .instance_id(inserted_instance.id) .build(); - - let inserted_person = Person::create(pool, &new_person).await.unwrap(); - + let inserted_person = Person::create(pool, &new_person).await?; let new_local_user = LocalUserInsertForm::builder() .person_id(inserted_person.id) .password_encrypted("pass".to_string()) .build(); + let inserted_local_user = LocalUser::create(pool, &new_local_user, vec![]).await?; - let inserted_local_user = LocalUser::create(pool, &new_local_user, vec![]) - .await - .unwrap(); - + // Create password reset token let token = "nope"; - let inserted_password_reset_request = - PasswordResetRequest::create_token(pool, inserted_local_user.id, token.to_string()) - .await - .unwrap(); + PasswordResetRequest::create(pool, inserted_local_user.id, token.to_string()).await?; - let expected_password_reset_request = PasswordResetRequest { - id: inserted_password_reset_request.id, - local_user_id: inserted_local_user.id, - token: token.to_string().into(), - published: inserted_password_reset_request.published, - }; - - let read_password_reset_request = PasswordResetRequest::read_from_token(pool, token) - .await - .unwrap() + // Read it and verify + let read_password_reset_request = PasswordResetRequest::read_and_delete(pool, token) + .await? .unwrap(); - let num_deleted = Person::delete(pool, inserted_person.id).await.unwrap(); - Instance::delete(pool, inserted_instance.id).await.unwrap(); - - assert_eq!(expected_password_reset_request, read_password_reset_request); assert_eq!( - expected_password_reset_request, - inserted_password_reset_request + inserted_password_reset_request.id, + read_password_reset_request.id ); + assert_eq!( + inserted_password_reset_request.local_user_id, + read_password_reset_request.local_user_id + ); + assert_eq!( + inserted_password_reset_request.token, + read_password_reset_request.token + ); + assert_eq!( + inserted_password_reset_request.published, + read_password_reset_request.published + ); + + // Cannot reuse same token again + let read_password_reset_request = PasswordResetRequest::read_and_delete(pool, token).await?; + assert!(read_password_reset_request.is_none()); + + // Cleanup + let num_deleted = Person::delete(pool, inserted_person.id).await?; + Instance::delete(pool, inserted_instance.id).await?; assert_eq!(1, num_deleted); + Ok(()) } } diff --git a/crates/utils/src/error.rs b/crates/utils/src/error.rs index 79d37cd08..53fba7c9e 100644 --- a/crates/utils/src/error.rs +++ b/crates/utils/src/error.rs @@ -161,7 +161,6 @@ pub enum LemmyErrorType { PermissiveRegex, InvalidRegex, CaptchaIncorrect, - PasswordResetLimitReached, CouldntCreateAudioCaptcha, InvalidUrlScheme, CouldntSendWebmention, diff --git a/src/api_routes_http.rs b/src/api_routes_http.rs index 6bfe0d727..1702ceb2a 100644 --- a/src/api_routes_http.rs +++ b/src/api_routes_http.rs @@ -278,6 +278,11 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimitCell) { .wrap(rate_limit.register()) .route(web::post().to(login)), ) + .service( + web::resource("/user/password_reset") + .wrap(rate_limit.register()) + .route(web::post().to(reset_password)), + ) .service( // Handle captcha separately web::resource("/user/get_captcha") @@ -318,7 +323,6 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimitCell) { // TODO Account actions. I don't like that they're in /user maybe /accounts .route("/logout", web::post().to(logout)) .route("/delete_account", web::post().to(delete_account)) - .route("/password_reset", web::post().to(reset_password)) .route( "/password_change", web::post().to(change_password_after_reset), From 6b46a7053535e3841ef5dcb5e8d2e80cbff976f8 Mon Sep 17 00:00:00 2001 From: Nutomic Date: Tue, 21 May 2024 20:47:06 +0200 Subject: [PATCH 2/2] Extra logging to debug duplicate activities (ref #4609) (#4726) * Extra logging to debug duplicate activities (ref #4609) * Fix logging for api tests * fmt --- api_tests/prepare-drone-federation-test.sh | 6 +++--- crates/federate/src/lib.rs | 5 +++++ crates/federate/src/worker.rs | 19 +++++++++---------- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/api_tests/prepare-drone-federation-test.sh b/api_tests/prepare-drone-federation-test.sh index 147a553ab..136f8ddfc 100755 --- a/api_tests/prepare-drone-federation-test.sh +++ b/api_tests/prepare-drone-federation-test.sh @@ -3,13 +3,13 @@ # it is expected that this script is called by run-federation-test.sh script. set -e -if [ -n "$LEMMY_LOG_LEVEL" ]; +if [ -z "$LEMMY_LOG_LEVEL" ]; then - LEMMY_LOG_LEVEL=warn + LEMMY_LOG_LEVEL=info fi export RUST_BACKTRACE=1 -#export RUST_LOG="warn,lemmy_server=$LEMMY_LOG_LEVEL,lemmy_federate=$LEMMY_LOG_LEVEL,lemmy_api=$LEMMY_LOG_LEVEL,lemmy_api_common=$LEMMY_LOG_LEVEL,lemmy_api_crud=$LEMMY_LOG_LEVEL,lemmy_apub=$LEMMY_LOG_LEVEL,lemmy_db_schema=$LEMMY_LOG_LEVEL,lemmy_db_views=$LEMMY_LOG_LEVEL,lemmy_db_views_actor=$LEMMY_LOG_LEVEL,lemmy_db_views_moderator=$LEMMY_LOG_LEVEL,lemmy_routes=$LEMMY_LOG_LEVEL,lemmy_utils=$LEMMY_LOG_LEVEL,lemmy_websocket=$LEMMY_LOG_LEVEL" +export RUST_LOG="warn,lemmy_server=$LEMMY_LOG_LEVEL,lemmy_federate=$LEMMY_LOG_LEVEL,lemmy_api=$LEMMY_LOG_LEVEL,lemmy_api_common=$LEMMY_LOG_LEVEL,lemmy_api_crud=$LEMMY_LOG_LEVEL,lemmy_apub=$LEMMY_LOG_LEVEL,lemmy_db_schema=$LEMMY_LOG_LEVEL,lemmy_db_views=$LEMMY_LOG_LEVEL,lemmy_db_views_actor=$LEMMY_LOG_LEVEL,lemmy_db_views_moderator=$LEMMY_LOG_LEVEL,lemmy_routes=$LEMMY_LOG_LEVEL,lemmy_utils=$LEMMY_LOG_LEVEL,lemmy_websocket=$LEMMY_LOG_LEVEL" export LEMMY_TEST_FAST_FEDERATION=1 # by default, the persistent federation queue has delays in the scale of 30s-5min diff --git a/crates/federate/src/lib.rs b/crates/federate/src/lib.rs index a4dc49536..fc5bd2387 100644 --- a/crates/federate/src/lib.rs +++ b/crates/federate/src/lib.rs @@ -13,6 +13,7 @@ use tokio::{ time::sleep, }; use tokio_util::sync::CancellationToken; +use tracing::info; mod util; mod worker; @@ -44,6 +45,10 @@ async fn start_stop_federation_workers( let pool2 = &mut DbPool::Pool(&pool); let process_index = opts.process_index - 1; let local_domain = federation_config.settings().get_hostname_without_port()?; + info!( + "Starting federation workers for process count {} and index {}", + opts.process_count, process_index + ); loop { let mut total_count = 0; let mut dead_count = 0; diff --git a/crates/federate/src/worker.rs b/crates/federate/src/worker.rs index ff2a68e3c..f6701a8d1 100644 --- a/crates/federate/src/worker.rs +++ b/crates/federate/src/worker.rs @@ -34,6 +34,7 @@ use std::{ }; use tokio::{sync::mpsc::UnboundedSender, time::sleep}; use tokio_util::sync::CancellationToken; +use tracing::{debug, info, trace, warn}; /// Check whether to save state to db every n sends if there's no failures (during failures state is saved after every attempt) /// This determines the batch size for loop_batch. After a batch ends and SAVE_STATE_EVERY_TIME has passed, the federation_queue_state is updated in the DB. @@ -105,6 +106,7 @@ impl InstanceWorker { &mut self, pool: &mut DbPool<'_>, ) -> Result<(), anyhow::Error> { + debug!("Starting federation worker for {}", self.instance.domain); let save_state_every = chrono::Duration::from_std(SAVE_STATE_EVERY_TIME).expect("not negative"); self.update_communities(pool).await?; @@ -176,15 +178,14 @@ impl InstanceWorker { .await .context("failed reading activity from db")? else { - tracing::debug!("{}: {:?} does not exist", self.instance.domain, id); + debug!("{}: {:?} does not exist", self.instance.domain, id); self.state.last_successful_id = Some(id); continue; }; if let Err(e) = self.send_retry_loop(pool, &ele.0, &ele.1).await { - tracing::warn!( + warn!( "sending {} errored internally, skipping activity: {:?}", - ele.0.ap_id, - e + ele.0.ap_id, e ); } if self.stop.is_cancelled() { @@ -211,7 +212,7 @@ impl InstanceWorker { .await .context("failed figuring out inbox urls")?; if inbox_urls.is_empty() { - tracing::debug!("{}: {:?} no inboxes", self.instance.domain, activity.id); + trace!("{}: {:?} no inboxes", self.instance.domain, activity.id); self.state.last_successful_id = Some(activity.id); self.state.last_successful_published_time = Some(activity.published); return Ok(()); @@ -229,16 +230,14 @@ impl InstanceWorker { SendActivityTask::prepare(&object, actor.as_ref(), inbox_urls, &self.context).await?; for task in requests { // usually only one due to shared inbox - tracing::debug!("sending out {}", task); + trace!("sending out {}", task); while let Err(e) = task.sign_and_send(&self.context).await { self.state.fail_count += 1; self.state.last_retry = Some(Utc::now()); let retry_delay: Duration = federate_retry_sleep_duration(self.state.fail_count); - tracing::info!( + info!( "{}: retrying {:?} attempt {} with delay {retry_delay:.2?}. ({e})", - self.instance.domain, - activity.id, - self.state.fail_count + self.instance.domain, activity.id, self.state.fail_count ); self.save_and_send_state(pool).await?; tokio::select! {