From d60624af6a8c78c891de84cf00d7012da468bf1d Mon Sep 17 00:00:00 2001 From: Dessalines Date: Sun, 12 Dec 2021 13:14:30 -0500 Subject: [PATCH] Adding fixes from comments. --- crates/api/src/local_user.rs | 65 +++++++++--------------- crates/api/src/site.rs | 11 ++-- crates/api_common/src/lib.rs | 22 +++++++- crates/api_common/src/person.rs | 3 ++ crates/api_crud/src/site/update.rs | 35 +++++++++++-- crates/db_schema/src/impls/local_user.rs | 14 +++++ src/main.rs | 4 +- 7 files changed, 101 insertions(+), 53 deletions(-) diff --git a/crates/api/src/local_user.rs b/crates/api/src/local_user.rs index 36e59fdda..781a581a7 100644 --- a/crates/api/src/local_user.rs +++ b/crates/api/src/local_user.rs @@ -183,32 +183,30 @@ impl Perform for SaveUserSettings { let matrix_user_id = diesel_option_overwrite(&data.matrix_user_id); let bot_account = data.bot_account; let email_deref = data.email.as_deref().map(|e| e.to_owned()); + let email = diesel_option_overwrite(&email_deref); - let email = if let Some(email) = &email_deref { - let site = blocking(context.pool(), Site::read_simple).await??; - if site.require_email_verification { - if let Some(previous_email) = local_user_view.local_user.email { - // Only send the verification email if there was an email change - if previous_email.ne(email) { - send_verification_email( - local_user_view.local_user.id, - email, - &local_user_view.person.name, - context.pool(), - &context.settings(), - ) - .await?; - } - } - // Fine to return None here, because the actual email is also in the email_verification - // table, and gets set in the function below. - None - } else { - diesel_option_overwrite(&email_deref) + if let Some(Some(email)) = &email { + let previous_email = local_user_view.local_user.email.unwrap_or_default(); + // Only send the verification email if there was an email change + if previous_email.ne(email) { + send_verification_email( + local_user_view.local_user.id, + email, + &local_user_view.person.name, + context.pool(), + &context.settings(), + ) + .await?; } - } else { - None - }; + } + + // When the site requires email, make sure email is not Some(None). IE, an overwrite to a None value + if let Some(email) = &email { + let site_fut = blocking(context.pool(), Site::read_simple); + if email.is_none() && site_fut.await??.require_email_verification { + return Err(LemmyError::from_message("email_required")); + } + } if let Some(Some(bio)) = &bio { if bio.chars().count() > 300 { @@ -924,7 +922,7 @@ impl Perform for GetUnreadCount { #[async_trait::async_trait(?Send)] impl Perform for VerifyEmail { - type Response = LoginResponse; + type Response = VerifyEmailResponse; async fn perform( &self, @@ -964,21 +962,6 @@ impl Perform for VerifyEmail { }) .await??; - let site = blocking(context.pool(), Site::read_simple).await??; - check_registration_application(&site, &local_user_view, context.pool()).await?; - - // Return the jwt - Ok(LoginResponse { - jwt: Some( - Claims::jwt( - local_user_view.local_user.id.0, - &context.secret().jwt_secret, - &context.settings().hostname, - )? - .into(), - ), - verify_email_sent: false, - registration_created: false, - }) + Ok(VerifyEmailResponse {}) } } diff --git a/crates/api/src/site.rs b/crates/api/src/site.rs index 702988af5..172e79be6 100644 --- a/crates/api/src/site.rs +++ b/crates/api/src/site.rs @@ -658,16 +658,15 @@ impl Perform for ApproveRegistrationApplication { }) .await??; - let require_email_verification = blocking(context.pool(), Site::read_simple) - .await?? - .require_email_verification; - - if require_email_verification && data.approve { + if data.approve { let approved_local_user_view = blocking(context.pool(), move |conn| { LocalUserView::read(conn, approved_user_id) }) .await??; - send_application_approved_email(&approved_local_user_view, &context.settings())?; + + if approved_local_user_view.local_user.email.is_some() { + send_application_approved_email(&approved_local_user_view, &context.settings())?; + } } // Read the view diff --git a/crates/api_common/src/lib.rs b/crates/api_common/src/lib.rs index 9ad53e7b3..cd854a6d9 100644 --- a/crates/api_common/src/lib.rs +++ b/crates/api_common/src/lib.rs @@ -468,7 +468,10 @@ pub async fn check_registration_application( local_user_view: &LocalUserView, pool: &DbPool, ) -> Result<(), LemmyError> { - if site.require_application && !local_user_view.local_user.accepted_application { + if site.require_application + && !local_user_view.local_user.accepted_application + && !local_user_view.person.admin + { // Fetch the registration, see if its denied let local_user_id = local_user_view.local_user.id; let registration = blocking(pool, move |conn| { @@ -483,3 +486,20 @@ pub async fn check_registration_application( } Ok(()) } + +/// TODO this check should be removed after https://github.com/LemmyNet/lemmy/issues/868 is done. +pub async fn check_private_instance_and_federation_enabled( + pool: &DbPool, + settings: &Settings, +) -> Result<(), LemmyError> { + let site_opt = blocking(pool, Site::read_simple).await?; + + if let Ok(site) = site_opt { + if site.private_instance && settings.federation.enabled { + return Err(LemmyError::from_message( + "Cannot have both private instance and federation enabled.", + )); + } + } + Ok(()) +} diff --git a/crates/api_common/src/person.rs b/crates/api_common/src/person.rs index ff77e41d2..47c26591e 100644 --- a/crates/api_common/src/person.rs +++ b/crates/api_common/src/person.rs @@ -293,3 +293,6 @@ pub struct GetUnreadCountResponse { pub struct VerifyEmail { pub token: String, } + +#[derive(Debug, Serialize, Deserialize, Clone)] +pub struct VerifyEmailResponse {} diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index ad833fb16..bcd6a1a32 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -11,7 +11,10 @@ use lemmy_db_schema::{ diesel_option_overwrite, diesel_option_overwrite_to_url, naive_now, - source::site::{Site, SiteForm}, + source::{ + local_user::LocalUser, + site::{Site, SiteForm}, + }, traits::Crud, }; use lemmy_db_views::site_view::SiteView; @@ -68,11 +71,35 @@ impl PerformCrud for EditSite { private_instance: data.private_instance, }; - let update_site = move |conn: &'_ _| Site::update(conn, 1, &site_form); - blocking(context.pool(), update_site) + let update_site = blocking(context.pool(), move |conn| { + Site::update(conn, 1, &site_form) + }) + .await? + .map_err(LemmyError::from) + .map_err(|e| e.with_message("couldnt_update_site"))?; + + // TODO can't think of a better way to do this. + // If the server suddenly requires email verification, or required applications, no old users + // will be able to log in. It really only wants this to be a requirement for NEW signups. + // So if it was set from false, to true, you need to update all current users columns to be verified. + + if !found_site.require_application && update_site.require_application { + blocking(context.pool(), move |conn| { + LocalUser::set_all_users_registration_applications_accepted(conn) + }) .await? .map_err(LemmyError::from) - .map_err(|e| e.with_message("couldnt_update_site"))?; + .map_err(|e| e.with_message("couldnt_set_all_registrations_accepted"))?; + } + + if !found_site.require_email_verification && update_site.require_email_verification { + blocking(context.pool(), move |conn| { + LocalUser::set_all_users_email_verified(conn) + }) + .await? + .map_err(LemmyError::from) + .map_err(|e| e.with_message("couldnt_set_all_email_verified"))?; + } let site_view = blocking(context.pool(), SiteView::read).await??; diff --git a/crates/db_schema/src/impls/local_user.rs b/crates/db_schema/src/impls/local_user.rs index c7696155b..833d6bdb9 100644 --- a/crates/db_schema/src/impls/local_user.rs +++ b/crates/db_schema/src/impls/local_user.rs @@ -89,6 +89,20 @@ impl LocalUser { )) .get_result::(conn) } + + pub fn set_all_users_email_verified(conn: &PgConnection) -> Result, Error> { + diesel::update(local_user) + .set(email_verified.eq(true)) + .get_results::(conn) + } + + pub fn set_all_users_registration_applications_accepted( + conn: &PgConnection, + ) -> Result, Error> { + diesel::update(local_user) + .set(accepted_application.eq(true)) + .get_results::(conn) + } } impl Crud for LocalUser { diff --git a/src/main.rs b/src/main.rs index cf29e7790..252d37a38 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,7 +9,7 @@ use diesel::{ }; use doku::json::{AutoComments, Formatting}; use lemmy_api::match_websocket_operation; -use lemmy_api_common::blocking; +use lemmy_api_common::{blocking, check_private_instance_and_federation_enabled}; use lemmy_api_crud::match_websocket_operation_crud; use lemmy_apub_lib::activity_queue::create_activity_queue; use lemmy_db_schema::{get_database_url_from_env, source::secret::Secret}; @@ -103,6 +103,8 @@ async fn main() -> Result<(), LemmyError> { let activity_queue = queue_manager.queue_handle().clone(); + check_private_instance_and_federation_enabled(&pool, &settings).await?; + let chat_server = ChatServer::startup( pool.clone(), rate_limiter.clone(),