From 63a686d390e441216718a3d64408336409890791 Mon Sep 17 00:00:00 2001 From: Richard Schwab Date: Tue, 13 Aug 2024 22:18:26 +0200 Subject: [PATCH] Approve applications in transaction (#4970) * Implement tests for registration application count and list api * Use transaction when approving applications to ensure consistent approval state --- Cargo.lock | 1 + crates/api/Cargo.toml | 1 + .../site/registration_applications/approve.rs | 56 ++- .../site/registration_applications/list.rs | 3 +- .../src/site/registration_applications/mod.rs | 2 + .../site/registration_applications/tests.rs | 428 ++++++++++++++++++ .../registration_applications/unread_count.rs | 3 +- crates/api_crud/src/site/create.rs | 4 +- 8 files changed, 474 insertions(+), 24 deletions(-) create mode 100644 crates/api/src/site/registration_applications/tests.rs diff --git a/Cargo.lock b/Cargo.lock index d2d40d5eb..a53843f6e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2817,6 +2817,7 @@ dependencies = [ "elementtree", "hound", "lemmy_api_common", + "lemmy_api_crud", "lemmy_db_schema", "lemmy_db_views", "lemmy_db_views_actor", diff --git a/crates/api/Cargo.toml b/crates/api/Cargo.toml index 9506049ee..87879f6cd 100644 --- a/crates/api/Cargo.toml +++ b/crates/api/Cargo.toml @@ -43,3 +43,4 @@ serial_test = { workspace = true } tokio = { workspace = true } elementtree = "1.2.3" pretty_assertions = { workspace = true } +lemmy_api_crud = { workspace = true } diff --git a/crates/api/src/site/registration_applications/approve.rs b/crates/api/src/site/registration_applications/approve.rs index 823af54c4..dcde78117 100644 --- a/crates/api/src/site/registration_applications/approve.rs +++ b/crates/api/src/site/registration_applications/approve.rs @@ -1,4 +1,5 @@ -use actix_web::web::{Data, Json}; +use activitypub_federation::config::Data; +use actix_web::web::Json; use lemmy_api_common::{ context::LemmyContext, site::{ApproveRegistrationApplication, RegistrationApplicationResponse}, @@ -10,10 +11,13 @@ use lemmy_db_schema::{ registration_application::{RegistrationApplication, RegistrationApplicationUpdateForm}, }, traits::Crud, - utils::diesel_string_update, + utils::{diesel_string_update, get_conn}, }; use lemmy_db_views::structs::{LocalUserView, RegistrationApplicationView}; -use lemmy_utils::{error::LemmyResult, LemmyErrorType}; +use lemmy_utils::{ + error::{LemmyError, LemmyResult}, + LemmyErrorType, +}; pub async fn approve_registration_application( data: Json, @@ -25,34 +29,46 @@ pub async fn approve_registration_application( // Only let admins do this is_admin(&local_user_view)?; - // Update the registration with reason, admin_id - let deny_reason = diesel_string_update(data.deny_reason.as_deref()); - let app_form = RegistrationApplicationUpdateForm { - admin_id: Some(Some(local_user_view.person.id)), - deny_reason, - }; + let pool = &mut context.pool(); + let conn = &mut get_conn(pool).await?; + let tx_data = data.clone(); + let approved_user_id = conn + .build_transaction() + .run(|conn| { + Box::pin(async move { + // Update the registration with reason, admin_id + let deny_reason = diesel_string_update(tx_data.deny_reason.as_deref()); + let app_form = RegistrationApplicationUpdateForm { + admin_id: Some(Some(local_user_view.person.id)), + deny_reason, + }; - let registration_application = - RegistrationApplication::update(&mut context.pool(), app_id, &app_form).await?; + let registration_application = + RegistrationApplication::update(&mut conn.into(), app_id, &app_form).await?; - // Update the local_user row - let local_user_form = LocalUserUpdateForm { - accepted_application: Some(data.approve), - ..Default::default() - }; + // Update the local_user row + let local_user_form = LocalUserUpdateForm { + accepted_application: Some(tx_data.approve), + ..Default::default() + }; - let approved_user_id = registration_application.local_user_id; - LocalUser::update(&mut context.pool(), approved_user_id, &local_user_form).await?; + let approved_user_id = registration_application.local_user_id; + LocalUser::update(&mut conn.into(), approved_user_id, &local_user_form).await?; + + Ok::<_, LemmyError>(approved_user_id) + }) as _ + }) + .await?; if data.approve { let approved_local_user_view = LocalUserView::read(&mut context.pool(), approved_user_id) .await? .ok_or(LemmyErrorType::CouldntFindLocalUser)?; - if approved_local_user_view.local_user.email.is_some() { + // Email sending may fail, but this won't revert the application approval send_application_approved_email(&approved_local_user_view, context.settings()).await?; } - } + }; // Read the view let registration_application = RegistrationApplicationView::read(&mut context.pool(), app_id) diff --git a/crates/api/src/site/registration_applications/list.rs b/crates/api/src/site/registration_applications/list.rs index df86b11d5..877e83796 100644 --- a/crates/api/src/site/registration_applications/list.rs +++ b/crates/api/src/site/registration_applications/list.rs @@ -1,4 +1,5 @@ -use actix_web::web::{Data, Json, Query}; +use activitypub_federation::config::Data; +use actix_web::web::{Json, Query}; use lemmy_api_common::{ context::LemmyContext, site::{ListRegistrationApplications, ListRegistrationApplicationsResponse}, diff --git a/crates/api/src/site/registration_applications/mod.rs b/crates/api/src/site/registration_applications/mod.rs index e904f597d..c9a63cdef 100644 --- a/crates/api/src/site/registration_applications/mod.rs +++ b/crates/api/src/site/registration_applications/mod.rs @@ -1,4 +1,6 @@ pub mod approve; pub mod get; pub mod list; +#[cfg(test)] +mod tests; pub mod unread_count; diff --git a/crates/api/src/site/registration_applications/tests.rs b/crates/api/src/site/registration_applications/tests.rs new file mode 100644 index 000000000..062fa550f --- /dev/null +++ b/crates/api/src/site/registration_applications/tests.rs @@ -0,0 +1,428 @@ +use crate::site::registration_applications::{ + approve::approve_registration_application, + list::list_registration_applications, + unread_count::get_unread_registration_application_count, +}; +use activitypub_federation::config::Data; +use actix_web::web::{Json, Query}; +use lemmy_api_common::{ + context::LemmyContext, + site::{ + ApproveRegistrationApplication, + EditSite, + GetUnreadRegistrationApplicationCountResponse, + ListRegistrationApplicationsResponse, + }, +}; +use lemmy_api_crud::site::update::update_site; +use lemmy_db_schema::{ + newtypes::InstanceId, + source::{ + instance::Instance, + local_site::{LocalSite, LocalSiteInsertForm}, + local_site_rate_limit::{LocalSiteRateLimit, LocalSiteRateLimitInsertForm}, + local_user::{LocalUser, LocalUserInsertForm, LocalUserUpdateForm}, + person::{Person, PersonInsertForm}, + registration_application::{RegistrationApplication, RegistrationApplicationInsertForm}, + site::{Site, SiteInsertForm}, + }, + traits::Crud, + utils::DbPool, + RegistrationMode, +}; +use lemmy_db_views::structs::LocalUserView; +use lemmy_utils::{error::LemmyResult, LemmyErrorType, CACHE_DURATION_API}; +use serial_test::serial; + +#[allow(clippy::unwrap_used)] +async fn create_test_site(context: &Data) -> LemmyResult<(Instance, LocalUserView)> { + let pool = &mut context.pool(); + + let inserted_instance = Instance::read_or_create(pool, "my_domain.tld".to_string()) + .await + .expect("Create test instance"); + + let admin_person = Person::create( + pool, + &PersonInsertForm::test_form(inserted_instance.id, "admin"), + ) + .await?; + LocalUser::create( + pool, + &LocalUserInsertForm::test_form_admin(admin_person.id), + vec![], + ) + .await?; + + let admin_local_user_view = LocalUserView::read_person(pool, admin_person.id) + .await? + .unwrap(); + + let site_form = SiteInsertForm::builder() + .name("test site".to_string()) + .instance_id(inserted_instance.id) + .build(); + let site = Site::create(pool, &site_form).await.unwrap(); + + // Create a local site, since this is necessary for determining if email verification is + // required + let local_site_form = LocalSiteInsertForm::builder() + .site_id(site.id) + .require_email_verification(Some(true)) + .application_question(Some(".".to_string())) + .registration_mode(Some(RegistrationMode::RequireApplication)) + .site_setup(Some(true)) + .build(); + let local_site = LocalSite::create(pool, &local_site_form).await.unwrap(); + + // Required to have a working local SiteView when updating the site to change email verification + // requirement or registration mode + let rate_limit_form = LocalSiteRateLimitInsertForm::builder() + .local_site_id(local_site.id) + .build(); + LocalSiteRateLimit::create(pool, &rate_limit_form) + .await + .unwrap(); + + Ok((inserted_instance, admin_local_user_view)) +} + +async fn signup( + pool: &mut DbPool<'_>, + instance_id: InstanceId, + name: &str, + email: Option<&str>, +) -> LemmyResult<(LocalUser, RegistrationApplication)> { + let person_insert_form = PersonInsertForm::test_form(instance_id, name); + let person = Person::create(pool, &person_insert_form).await?; + + let local_user_insert_form = match email { + Some(email) => LocalUserInsertForm { + email: Some(email.to_string()), + email_verified: Some(false), + ..LocalUserInsertForm::test_form(person.id) + }, + None => LocalUserInsertForm::test_form(person.id), + }; + + let local_user = LocalUser::create(pool, &local_user_insert_form, vec![]).await?; + + let application_insert_form = RegistrationApplicationInsertForm { + local_user_id: local_user.id, + answer: "x".to_string(), + }; + let application = RegistrationApplication::create(pool, &application_insert_form).await?; + + Ok((local_user, application)) +} + +#[allow(clippy::unwrap_used)] +async fn get_application_statuses( + context: &Data, + admin: LocalUserView, +) -> LemmyResult<( + Json, + Json, + Json, +)> { + let application_count = + get_unread_registration_application_count(context.reset_request_count(), admin.clone()).await?; + + let unread_applications = list_registration_applications( + Query::from_query("unread_only=true").unwrap(), + context.reset_request_count(), + admin.clone(), + ) + .await?; + + let all_applications = list_registration_applications( + Query::from_query("unread_only=false").unwrap(), + context.reset_request_count(), + admin, + ) + .await?; + + Ok((application_count, unread_applications, all_applications)) +} + +#[allow(clippy::indexing_slicing)] +#[allow(clippy::unwrap_used)] +#[tokio::test] +#[serial] +async fn test_application_approval() -> LemmyResult<()> { + let context = LemmyContext::init_test_context().await; + let pool = &mut context.pool(); + + let (instance, admin_local_user_view) = create_test_site(&context).await?; + + // Non-unread counts unfortunately are duplicated due to different types (i64 vs usize) + let mut expected_total_applications = 0; + let mut expected_unread_applications = 0u8; + + let (local_user_with_email, app_with_email) = + signup(pool, instance.id, "user_w_email", Some("lemmy@localhost")).await?; + + let (application_count, unread_applications, all_applications) = + get_application_statuses(&context, admin_local_user_view.clone()).await?; + + // When email verification is required and the email is not verified the application should not + // be visible to admins + assert_eq!( + application_count.registration_applications, + i64::from(expected_unread_applications), + ); + assert_eq!( + unread_applications.registration_applications.len(), + usize::from(expected_unread_applications), + ); + assert_eq!( + all_applications.registration_applications.len(), + expected_total_applications, + ); + + LocalUser::update( + pool, + local_user_with_email.id, + &LocalUserUpdateForm { + email_verified: Some(true), + ..Default::default() + }, + ) + .await?; + + expected_total_applications += 1; + expected_unread_applications += 1; + + let (application_count, unread_applications, all_applications) = + get_application_statuses(&context, admin_local_user_view.clone()).await?; + + // When email verification is required and the email is verified the application should be + // visible to admins + assert_eq!( + application_count.registration_applications, + i64::from(expected_unread_applications), + ); + assert_eq!( + unread_applications.registration_applications.len(), + usize::from(expected_unread_applications), + ); + assert!( + !unread_applications.registration_applications[0] + .creator_local_user + .accepted_application + ); + assert_eq!( + all_applications.registration_applications.len(), + expected_total_applications, + ); + + let approval = approve_registration_application( + Json(ApproveRegistrationApplication { + id: app_with_email.id, + approve: true, + deny_reason: None, + }), + context.reset_request_count(), + admin_local_user_view.clone(), + ) + .await; + // Approval should be processed up until email sending is attempted + assert!(approval.is_err_and(|e| e.error_type == LemmyErrorType::NoEmailSetup)); + + expected_unread_applications -= 1; + + let (application_count, unread_applications, all_applications) = + get_application_statuses(&context, admin_local_user_view.clone()).await?; + + // When the application is approved it should only be returned for unread queries + assert_eq!( + application_count.registration_applications, + i64::from(expected_unread_applications), + ); + assert_eq!( + unread_applications.registration_applications.len(), + usize::from(expected_unread_applications), + ); + assert_eq!( + all_applications.registration_applications.len(), + expected_total_applications, + ); + assert!( + all_applications.registration_applications[0] + .creator_local_user + .accepted_application + ); + + let (_local_user, app_with_email_2) = signup( + pool, + instance.id, + "user_w_email_2", + Some("lemmy2@localhost"), + ) + .await?; + let (application_count, unread_applications, all_applications) = + get_application_statuses(&context, admin_local_user_view.clone()).await?; + + // Email not verified, so application still not visible + assert_eq!( + application_count.registration_applications, + i64::from(expected_unread_applications), + ); + assert_eq!( + unread_applications.registration_applications.len(), + usize::from(expected_unread_applications), + ); + assert_eq!( + all_applications.registration_applications.len(), + expected_total_applications, + ); + + update_site( + Json(EditSite { + require_email_verification: Some(false), + ..Default::default() + }), + context.reset_request_count(), + admin_local_user_view.clone(), + ) + .await?; + + // TODO: There is probably a better way to ensure cache invalidation + tokio::time::sleep(CACHE_DURATION_API).await; + + expected_total_applications += 1; + expected_unread_applications += 1; + + let (application_count, unread_applications, all_applications) = + get_application_statuses(&context, admin_local_user_view.clone()).await?; + + // After disabling email verification the application should now be visible + assert_eq!( + application_count.registration_applications, + i64::from(expected_unread_applications), + ); + assert_eq!( + unread_applications.registration_applications.len(), + usize::from(expected_unread_applications), + ); + assert_eq!( + all_applications.registration_applications.len(), + expected_total_applications, + ); + + approve_registration_application( + Json(ApproveRegistrationApplication { + id: app_with_email_2.id, + approve: false, + deny_reason: None, + }), + context.reset_request_count(), + admin_local_user_view.clone(), + ) + .await?; + + expected_unread_applications -= 1; + + let (application_count, unread_applications, all_applications) = + get_application_statuses(&context, admin_local_user_view.clone()).await?; + + // Denied applications should not be marked as unread + assert_eq!( + application_count.registration_applications, + i64::from(expected_unread_applications), + ); + assert_eq!( + unread_applications.registration_applications.len(), + usize::from(expected_unread_applications), + ); + assert_eq!( + all_applications.registration_applications.len(), + expected_total_applications, + ); + + signup(pool, instance.id, "user_wo_email", None).await?; + + expected_total_applications += 1; + expected_unread_applications += 1; + + let (application_count, unread_applications, all_applications) = + get_application_statuses(&context, admin_local_user_view.clone()).await?; + + // New user without email should immediately be visible + assert_eq!( + application_count.registration_applications, + i64::from(expected_unread_applications), + ); + assert_eq!( + unread_applications.registration_applications.len(), + usize::from(expected_unread_applications), + ); + assert_eq!( + all_applications.registration_applications.len(), + expected_total_applications, + ); + + signup(pool, instance.id, "user_w_email_3", None).await?; + + expected_total_applications += 1; + expected_unread_applications += 1; + + let (application_count, unread_applications, all_applications) = + get_application_statuses(&context, admin_local_user_view.clone()).await?; + + // New user with email should immediately be visible + assert_eq!( + application_count.registration_applications, + i64::from(expected_unread_applications), + ); + assert_eq!( + unread_applications.registration_applications.len(), + usize::from(expected_unread_applications), + ); + assert_eq!( + all_applications.registration_applications.len(), + expected_total_applications, + ); + + update_site( + Json(EditSite { + registration_mode: Some(RegistrationMode::Open), + ..Default::default() + }), + context.reset_request_count(), + admin_local_user_view.clone(), + ) + .await?; + + // TODO: There is probably a better way to ensure cache invalidation + tokio::time::sleep(CACHE_DURATION_API).await; + + let (application_count, unread_applications, all_applications) = + get_application_statuses(&context, admin_local_user_view.clone()).await?; + + // TODO: At this time applications do not get approved when switching to open registration, so the + // numbers will not change. See https://github.com/LemmyNet/lemmy/issues/4969 + // expected_application_count = 0; + // expected_unread_applications_len = 0; + + // When applications are not required all previous applications should become approved but still + // visible + assert_eq!( + application_count.registration_applications, + i64::from(expected_unread_applications), + ); + assert_eq!( + unread_applications.registration_applications.len(), + usize::from(expected_unread_applications), + ); + assert_eq!( + all_applications.registration_applications.len(), + expected_total_applications, + ); + + LocalSite::delete(pool).await?; + // Instance deletion cascades cleanup of all created persons + Instance::delete(pool, instance.id).await?; + + Ok(()) +} diff --git a/crates/api/src/site/registration_applications/unread_count.rs b/crates/api/src/site/registration_applications/unread_count.rs index a12ecb1d3..5cc391ed0 100644 --- a/crates/api/src/site/registration_applications/unread_count.rs +++ b/crates/api/src/site/registration_applications/unread_count.rs @@ -1,4 +1,5 @@ -use actix_web::web::{Data, Json}; +use activitypub_federation::config::Data; +use actix_web::web::Json; use lemmy_api_common::{ context::LemmyContext, site::GetUnreadRegistrationApplicationCountResponse, diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index 6b1909966..9dcd1595a 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -1,6 +1,6 @@ use crate::site::{application_question_check, site_default_post_listing_type_check}; -use activitypub_federation::http_signatures::generate_actor_keypair; -use actix_web::web::{Data, Json}; +use activitypub_federation::{config::Data, http_signatures::generate_actor_keypair}; +use actix_web::web::Json; use lemmy_api_common::{ context::LemmyContext, site::{CreateSite, SiteResponse},