From eecb710104a5a30aca4db586442d97a774b3915f Mon Sep 17 00:00:00 2001 From: Dessalines Date: Thu, 9 Dec 2021 16:15:51 -0500 Subject: [PATCH] A few fixes for private sites: - Added a check_registration_application function. - Only send a verification email if its been changed. - VerifyEmail now returns LoginResponse. - Deleting the old tokens after a successful email verify. - If port is missing on email config, display a better error message. --- crates/api/src/local_user.rs | 52 ++++++++++---- crates/api/src/site.rs | 7 +- crates/api_common/src/lib.rs | 32 +++++++-- crates/api_common/src/person.rs | 3 - crates/api_crud/src/site/read.rs | 6 -- crates/api_crud/src/user/create.rs | 68 +++++++++---------- .../db_schema/src/impls/email_verification.rs | 9 ++- .../src/impls/registration_application.rs | 16 ++++- crates/db_schema/src/newtypes.rs | 4 +- crates/utils/src/email.rs | 6 ++ .../up.sql | 3 +- 11 files changed, 137 insertions(+), 69 deletions(-) diff --git a/crates/api/src/local_user.rs b/crates/api/src/local_user.rs index e4878683a..36e59fdda 100644 --- a/crates/api/src/local_user.rs +++ b/crates/api/src/local_user.rs @@ -6,6 +6,7 @@ use captcha::{gen, Difficulty}; use chrono::Duration; use lemmy_api_common::{ blocking, + check_registration_application, get_local_user_view_from_jwt, is_admin, password_length_check, @@ -97,9 +98,7 @@ impl Perform for Login { return Err(LemmyError::from_message("email_not_verified")); } - if site.require_application && !local_user_view.local_user.accepted_application { - return Err(LemmyError::from_message("registration_application_pending")); - } + check_registration_application(&site, &local_user_view, context.pool()).await?; // Return the jwt Ok(LoginResponse { @@ -188,14 +187,21 @@ impl Perform for SaveUserSettings { let email = if let Some(email) = &email_deref { let site = blocking(context.pool(), Site::read_simple).await??; if site.require_email_verification { - send_verification_email( - local_user_view.local_user.id, - email, - &local_user_view.person.name, - context.pool(), - &context.settings(), - ) - .await?; + 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) @@ -918,7 +924,7 @@ impl Perform for GetUnreadCount { #[async_trait::async_trait(?Send)] impl Perform for VerifyEmail { - type Response = VerifyEmailResponse; + type Response = LoginResponse; async fn perform( &self, @@ -953,6 +959,26 @@ impl Perform for VerifyEmail { send_email_verification_success(&local_user_view, &context.settings())?; - Ok(VerifyEmailResponse {}) + blocking(context.pool(), move |conn| { + EmailVerification::delete_old_tokens_for_local_user(conn, local_user_id) + }) + .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, + }) } } diff --git a/crates/api/src/site.rs b/crates/api/src/site.rs index c653a212c..702988af5 100644 --- a/crates/api/src/site.rs +++ b/crates/api/src/site.rs @@ -38,6 +38,7 @@ use lemmy_db_schema::{ }; use lemmy_db_views::{ comment_view::{CommentQueryBuilder, CommentView}, + local_user_view::LocalUserView, post_view::{PostQueryBuilder, PostView}, registration_application_view::{ RegistrationApplicationQueryBuilder, @@ -662,7 +663,11 @@ impl Perform for ApproveRegistrationApplication { .require_email_verification; if require_email_verification && data.approve { - send_application_approved_email(&local_user_view, &context.settings())?; + 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())?; } // Read the view diff --git a/crates/api_common/src/lib.rs b/crates/api_common/src/lib.rs index 94e7b1d8e..9ad53e7b3 100644 --- a/crates/api_common/src/lib.rs +++ b/crates/api_common/src/lib.rs @@ -14,6 +14,7 @@ use lemmy_db_schema::{ password_reset_request::PasswordResetRequest, person_block::PersonBlock, post::{Post, PostRead, PostReadForm}, + registration_application::RegistrationApplication, secret::Secret, site::Site, }, @@ -426,8 +427,8 @@ pub async fn send_verification_email( let body = format!( concat!( "Please click the link below to verify your email address ", - "for the account @{}@{}. Ignore this email if the account isn't yours.\n\n", - "" + "for the account @{}@{}. Ignore this email if the account isn't yours.

", + "Verify your email" ), username, settings.hostname, verify_link ); @@ -441,7 +442,7 @@ pub fn send_email_verification_success( settings: &Settings, ) -> Result<(), LemmyError> { let email = &local_user_view.local_user.email.to_owned().expect("email"); - let subject = &format!("Email verified for {}", local_user_view.person.name); + let subject = &format!("Email verified for {}", local_user_view.person.actor_id); let html = "Your email has been verified."; send_email(subject, email, &local_user_view.person.name, html, settings) } @@ -452,8 +453,8 @@ pub fn send_application_approved_email( ) -> Result<(), LemmyError> { let email = &local_user_view.local_user.email.to_owned().expect("email"); let subject = &format!( - "Registration to {} approved for {}", - settings.hostname, local_user_view.person.name + "Registration approved for {}", + local_user_view.person.actor_id ); let html = &format!( "Your registration application has been approved. Welcome to {}!", @@ -461,3 +462,24 @@ pub fn send_application_approved_email( ); send_email(subject, email, &local_user_view.person.name, html, settings) } + +pub async fn check_registration_application( + site: &Site, + local_user_view: &LocalUserView, + pool: &DbPool, +) -> Result<(), LemmyError> { + if site.require_application && !local_user_view.local_user.accepted_application { + // Fetch the registration, see if its denied + let local_user_id = local_user_view.local_user.id; + let registration = blocking(pool, move |conn| { + RegistrationApplication::find_by_local_user_id(conn, local_user_id) + }) + .await??; + if registration.deny_reason.is_some() { + return Err(LemmyError::from_message("registration_denied")); + } else { + return Err(LemmyError::from_message("registration_application_pending")); + } + } + Ok(()) +} diff --git a/crates/api_common/src/person.rs b/crates/api_common/src/person.rs index 47c26591e..ff77e41d2 100644 --- a/crates/api_common/src/person.rs +++ b/crates/api_common/src/person.rs @@ -293,6 +293,3 @@ 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/read.rs b/crates/api_crud/src/site/read.rs index f84fca5e3..06146b96f 100644 --- a/crates/api_crud/src/site/read.rs +++ b/crates/api_crud/src/site/read.rs @@ -147,12 +147,6 @@ impl PerformCrud for GetSite { person_blocks, }) } else { - // If the site is setup, private, and there is no auth, return an error - if let Some(site_view) = site_view.to_owned() { - if site_view.site.private_instance { - return Err(LemmyError::from_message("instance_is_private")); - } - } None }; diff --git a/crates/api_crud/src/user/create.rs b/crates/api_crud/src/user/create.rs index 2160661ac..b8739e346 100644 --- a/crates/api_crud/src/user/create.rs +++ b/crates/api_crud/src/user/create.rs @@ -257,43 +257,41 @@ impl PerformCrud for Register { .map_err(|e| e.with_message("community_moderator_already_exists"))?; } - // Log the user in directly if email verification is not required - let login_response = if email_verification { - send_verification_email( - inserted_local_user.id, - // we check at the beginning of this method that email is set - &inserted_local_user.email.expect("email was provided"), - &inserted_person.name, - context.pool(), - &context.settings(), - ) - .await?; - LoginResponse { - jwt: None, - verify_email_sent: true, - registration_created: false, - } - } else if require_application { - LoginResponse { - jwt: None, - verify_email_sent: false, - registration_created: true, - } - } else { - LoginResponse { - jwt: Some( - Claims::jwt( - inserted_local_user.id.0, - &context.secret().jwt_secret, - &context.settings().hostname, - )? - .into(), - ), - verify_email_sent: false, - registration_created: false, - } + let mut login_response = LoginResponse { + jwt: None, + registration_created: false, + verify_email_sent: false, }; + // Log the user in directly if email verification and application aren't required + if !require_application && !email_verification { + login_response.jwt = Some( + Claims::jwt( + inserted_local_user.id.0, + &context.secret().jwt_secret, + &context.settings().hostname, + )? + .into(), + ); + } else { + if email_verification { + send_verification_email( + inserted_local_user.id, + // we check at the beginning of this method that email is set + &inserted_local_user.email.expect("email was provided"), + &inserted_person.name, + context.pool(), + &context.settings(), + ) + .await?; + login_response.verify_email_sent = true; + } + + if require_application { + login_response.registration_created = true; + } + } + Ok(login_response) } } diff --git a/crates/db_schema/src/impls/email_verification.rs b/crates/db_schema/src/impls/email_verification.rs index 69f2136fe..78277fa1f 100644 --- a/crates/db_schema/src/impls/email_verification.rs +++ b/crates/db_schema/src/impls/email_verification.rs @@ -1,4 +1,4 @@ -use crate::{source::email_verification::*, traits::Crud}; +use crate::{newtypes::LocalUserId, source::email_verification::*, traits::Crud}; use diesel::{insert_into, result::Error, ExpressionMethods, PgConnection, QueryDsl, RunQueryDsl}; impl Crud for EmailVerification { @@ -36,4 +36,11 @@ impl EmailVerification { .filter(verification_token.eq(token)) .first::(conn) } + pub fn delete_old_tokens_for_local_user( + conn: &PgConnection, + local_user_id_: LocalUserId, + ) -> Result { + use crate::schema::email_verification::dsl::*; + diesel::delete(email_verification.filter(local_user_id.eq(local_user_id_))).execute(conn) + } } diff --git a/crates/db_schema/src/impls/registration_application.rs b/crates/db_schema/src/impls/registration_application.rs index 91f43574b..5147dbb7a 100644 --- a/crates/db_schema/src/impls/registration_application.rs +++ b/crates/db_schema/src/impls/registration_application.rs @@ -1,5 +1,5 @@ -use crate::{source::registration_application::*, traits::Crud}; -use diesel::{insert_into, result::Error, PgConnection, QueryDsl, RunQueryDsl}; +use crate::{newtypes::LocalUserId, source::registration_application::*, traits::Crud}; +use diesel::{insert_into, result::Error, ExpressionMethods, PgConnection, QueryDsl, RunQueryDsl}; impl Crud for RegistrationApplication { type Form = RegistrationApplicationForm; @@ -28,3 +28,15 @@ impl Crud for RegistrationApplication { diesel::delete(registration_application.find(id_)).execute(conn) } } + +impl RegistrationApplication { + pub fn find_by_local_user_id( + conn: &PgConnection, + local_user_id_: LocalUserId, + ) -> Result { + use crate::schema::registration_application::dsl::*; + registration_application + .filter(local_user_id.eq(local_user_id_)) + .first::(conn) + } +} diff --git a/crates/db_schema/src/newtypes.rs b/crates/db_schema/src/newtypes.rs index 9219d77f9..b863a2500 100644 --- a/crates/db_schema/src/newtypes.rs +++ b/crates/db_schema/src/newtypes.rs @@ -43,7 +43,9 @@ impl fmt::Display for CommentId { )] pub struct CommunityId(pub i32); -#[derive(Debug, Copy, Clone, Hash, Eq, PartialEq, Serialize, Deserialize, DieselNewType)] +#[derive( + Debug, Copy, Clone, Hash, Eq, PartialEq, Default, Serialize, Deserialize, DieselNewType, +)] pub struct LocalUserId(pub i32); #[derive(Debug, Copy, Clone, Hash, Eq, PartialEq, Serialize, Deserialize, DieselNewType)] diff --git a/crates/utils/src/email.rs b/crates/utils/src/email.rs index 78a1f06c5..d9ac1710d 100644 --- a/crates/utils/src/email.rs +++ b/crates/utils/src/email.rs @@ -29,6 +29,12 @@ pub fn send_email( let (smtp_server, smtp_port) = { let email_and_port = email_config.smtp_server.split(':').collect::>(); + if email_and_port.len() == 1 { + return Err(LemmyError::from_message( + "email.smtp_server needs a port, IE smtp.xxx.com:465", + )); + } + ( email_and_port[0], email_and_port[1] diff --git a/migrations/2021-11-23-132840_email_verification/up.sql b/migrations/2021-11-23-132840_email_verification/up.sql index b6606a42e..29a20e00c 100644 --- a/migrations/2021-11-23-132840_email_verification/up.sql +++ b/migrations/2021-11-23-132840_email_verification/up.sql @@ -11,5 +11,4 @@ create table email_verification ( local_user_id int references local_user(id) on update cascade on delete cascade not null, email text not null, verification_token text not null - -); \ No newline at end of file +);