Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement email verification (fixes #219) #1956

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,8 @@ mod tests {
let inserted_person = Person::create(&conn, &new_person).unwrap();

let local_user_form = LocalUserForm {
person_id: inserted_person.id,
password_encrypted: "123456".to_string(),
person_id: Some(inserted_person.id),
password_encrypted: Some("123456".to_string()),
..LocalUserForm::default()
};

Expand Down
80 changes: 69 additions & 11 deletions crates/api/src/local_user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use lemmy_db_schema::{
source::{
comment::Comment,
community::Community,
email_verification::EmailVerification,
local_user::{LocalUser, LocalUserForm},
moderator::*,
password_reset_request::*,
Expand Down Expand Up @@ -54,6 +55,7 @@ use lemmy_utils::{
LemmyError,
};
use lemmy_websocket::{
email::send_verification_email,
messages::{CaptchaItem, SendAllMessage},
LemmyContext,
UserOperation,
Expand Down Expand Up @@ -88,13 +90,18 @@ impl Perform for Login {
return Err(ApiError::err_plain("password_incorrect").into());
}

let site = blocking(context.pool(), Site::read_simple).await??;
if site.require_email_verification && !local_user_view.local_user.email_verified {
return Err(ApiError::err_plain("email_not_verified").into());
}

// Return the jwt
Ok(LoginResponse {
jwt: Claims::jwt(
jwt: Some(Claims::jwt(
local_user_view.local_user.id.0,
&context.secret().jwt_secret,
&context.settings().hostname,
)?,
)?),
})
}
}
Expand Down Expand Up @@ -159,12 +166,29 @@ impl Perform for SaveUserSettings {

let avatar = diesel_option_overwrite_to_url(&data.avatar)?;
let banner = diesel_option_overwrite_to_url(&data.banner)?;
let email = diesel_option_overwrite(&data.email);
let bio = diesel_option_overwrite(&data.bio);
let display_name = diesel_option_overwrite(&data.display_name);
let matrix_user_id = diesel_option_overwrite(&data.matrix_user_id);
let bot_account = data.bot_account;

let email = if let Some(email) = &data.email {
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,
)
.await?;
None
} else {
diesel_option_overwrite(&data.email)
}
} else {
None
};

if let Some(Some(bio)) = &bio {
if bio.chars().count() > 300 {
return Err(ApiError::err_plain("bio_length_overflow").into());
Expand Down Expand Up @@ -222,9 +246,9 @@ impl Perform for SaveUserSettings {
.map_err(|e| ApiError::err("user_already_exists", e))?;

let local_user_form = LocalUserForm {
person_id,
person_id: Some(person_id),
email,
password_encrypted,
password_encrypted: Some(password_encrypted),
show_nsfw: data.show_nsfw,
show_bot_accounts: data.show_bot_accounts,
show_scores: data.show_scores,
Expand All @@ -236,6 +260,7 @@ impl Perform for SaveUserSettings {
show_read_posts: data.show_read_posts,
show_new_post_notifs: data.show_new_post_notifs,
send_notifications_to_email: data.send_notifications_to_email,
email_verified: None,
};

let local_user_res = blocking(context.pool(), move |conn| {
Expand All @@ -259,11 +284,11 @@ impl Perform for SaveUserSettings {

// Return the jwt
Ok(LoginResponse {
jwt: Claims::jwt(
jwt: Some(Claims::jwt(
updated_local_user.id.0,
&context.secret().jwt_secret,
&context.settings().hostname,
)?,
)?),
})
}
}
Expand Down Expand Up @@ -307,11 +332,11 @@ impl Perform for ChangePassword {

// Return the jwt
Ok(LoginResponse {
jwt: Claims::jwt(
jwt: Some(Claims::jwt(
updated_local_user.id.0,
&context.secret().jwt_secret,
&context.settings().hostname,
)?,
)?),
})
}
}
Expand Down Expand Up @@ -775,11 +800,11 @@ impl Perform for PasswordChange {

// Return the jwt
Ok(LoginResponse {
jwt: Claims::jwt(
jwt: Some(Claims::jwt(
updated_local_user.id.0,
&context.secret().jwt_secret,
&context.settings().hostname,
)?,
)?),
})
}
}
Expand Down Expand Up @@ -860,3 +885,36 @@ impl Perform for GetUnreadCount {
Ok(res)
}
}

#[async_trait::async_trait(?Send)]
impl Perform for VerifyEmail {
type Response = ();

async fn perform(
&self,
context: &Data<LemmyContext>,
_websocket_id: Option<usize>,
) -> Result<Self::Response, LemmyError> {
let token = self.token.clone();
let verification = blocking(context.pool(), move |conn| {
EmailVerification::read_for_token(conn, &token)
})
.await?
.map_err(|e| ApiError::err("token_not_found", e))?;

let form = LocalUserForm {
// necessary in case this is a new signup
email_verified: Some(true),
// necessary in case email of an existing user was changed
email: Some(Some(verification.email)),
..LocalUserForm::default()
};
let local_user_id = verification.local_user_id;
blocking(context.pool(), move |conn| {
LocalUser::update(conn, local_user_id, &form)
})
.await??;

Ok(())
}
}
10 changes: 9 additions & 1 deletion crates/api_common/src/person.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub struct Register {
pub password: String,
pub password_verify: String,
pub show_nsfw: bool,
/// email is mandatory if email verification is enabled on the server
pub email: Option<String>,
pub captcha_uuid: Option<String>,
pub captcha_answer: Option<String>,
Expand Down Expand Up @@ -77,7 +78,9 @@ pub struct ChangePassword {

#[derive(Serialize, Deserialize)]
pub struct LoginResponse {
pub jwt: String,
/// This is None in response to `Register` if email verification is enabled, and in response to
/// `DeleteAccount`.
pub jwt: Option<String>,
}

#[derive(Serialize, Deserialize)]
Expand Down Expand Up @@ -278,3 +281,8 @@ pub struct GetUnreadCountResponse {
pub mentions: i64,
pub private_messages: i64,
}

#[derive(Serialize, Deserialize)]
pub struct VerifyEmail {
pub token: String,
}
1 change: 1 addition & 0 deletions crates/api_common/src/site.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ pub struct EditSite {
pub open_registration: Option<bool>,
pub enable_nsfw: Option<bool>,
pub community_creation_admin_only: Option<bool>,
pub require_email_verification: Option<bool>,
pub auth: String,
}

Expand Down
2 changes: 1 addition & 1 deletion crates/api_crud/src/site/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ impl PerformCrud for CreateSite {
enable_downvotes: data.enable_downvotes,
open_registration: data.open_registration,
enable_nsfw: data.enable_nsfw,
updated: None,
community_creation_admin_only: data.community_creation_admin_only,
..SiteForm::default()
};

let create_site = move |conn: &'_ _| Site::create(conn, &site_form);
Expand Down
8 changes: 6 additions & 2 deletions crates/api_crud/src/site/read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ impl PerformCrud for GetSite {
captcha_answer: None,
honeypot: None,
};
let login_response = register.perform(context, websocket_id).await?;
let admin_jwt = register
.perform(context, websocket_id)
.await?
.jwt
.expect("jwt is returned from registration on newly created site");
info!("Admin {} created", setup.admin_username);

let create_site = CreateSite {
Expand All @@ -58,7 +62,7 @@ impl PerformCrud for GetSite {
open_registration: setup.open_registration,
enable_nsfw: setup.enable_nsfw,
community_creation_admin_only: setup.community_creation_admin_only,
auth: login_response.jwt,
auth: admin_jwt,
};
create_site.perform(context, websocket_id).await?;
info!("Site {} created", setup.site_name);
Expand Down
1 change: 1 addition & 0 deletions crates/api_crud/src/site/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ impl PerformCrud for EditSite {
open_registration: data.open_registration,
enable_nsfw: data.enable_nsfw,
community_creation_admin_only: data.community_creation_admin_only,
require_email_verification: data.require_email_verification,
};

let update_site = move |conn: &'_ _| Site::update(conn, 1, &site_form);
Expand Down
50 changes: 29 additions & 21 deletions crates/api_crud/src/user/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ use lemmy_db_schema::{
site::Site,
},
traits::{Crud, Followable, Joinable},
ListingType,
SortType,
};
use lemmy_db_views_actor::person_view::PersonViewSafe;
use lemmy_utils::{
Expand All @@ -36,7 +34,7 @@ use lemmy_utils::{
ConnectionId,
LemmyError,
};
use lemmy_websocket::{messages::CheckCaptcha, LemmyContext};
use lemmy_websocket::{email::send_verification_email, messages::CheckCaptcha, LemmyContext};

#[async_trait::async_trait(?Send)]
impl PerformCrud for Register {
Expand All @@ -49,16 +47,24 @@ impl PerformCrud for Register {
) -> Result<LoginResponse, LemmyError> {
let data: &Register = self;

// no email verification if the site is not setup yet
let mut email_verification = false;

// Make sure site has open registration
if let Ok(site) = blocking(context.pool(), Site::read_simple).await? {
if !site.open_registration {
return Err(ApiError::err_plain("registration_closed").into());
}
email_verification = site.require_email_verification;
}
Comment on lines +50 to 59
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of mut, you could probably do let email_verification = if let ...


password_length_check(&data.password)?;
honeypot_check(&data.honeypot)?;

if email_verification && data.email.is_none() {
return Err(ApiError::err_plain("email_required").into());
}

// Make sure passwords match
if data.password != data.password_verify {
return Err(ApiError::err_plain("passwords_dont_match").into());
Expand Down Expand Up @@ -124,22 +130,13 @@ impl PerformCrud for Register {
.map_err(|e| ApiError::err("user_already_exists", e))?;

// Create the local user
// TODO some of these could probably use the DB defaults
let local_user_form = LocalUserForm {
person_id: inserted_person.id,
person_id: Some(inserted_person.id),
email: Some(data.email.to_owned()),
password_encrypted: data.password.to_owned(),
password_encrypted: Some(data.password.to_owned()),
show_nsfw: Some(data.show_nsfw),
show_bot_accounts: Some(true),
theme: Some("browser".into()),
default_sort_type: Some(SortType::Active as i16),
default_listing_type: Some(ListingType::Subscribed as i16),
lang: Some("browser".into()),
show_avatars: Some(true),
show_scores: Some(true),
show_read_posts: Some(true),
show_new_post_notifs: Some(false),
send_notifications_to_email: Some(false),
email_verified: Some(!email_verification),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this too. If email_verification is false, this will incorrectly get set to true, when no email has been actually verified. This should just be Some(false)

..LocalUserForm::default()
};

let inserted_local_user = match blocking(context.pool(), move |conn| {
Expand Down Expand Up @@ -228,13 +225,24 @@ impl PerformCrud for Register {
.map_err(|e| ApiError::err("community_moderator_already_exists", e))?;
}

// Return the jwt
Ok(LoginResponse {
jwt: Claims::jwt(
// Log the user in directly if email verification is not required
let jwt = 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,
)
.await?;
None
Comment on lines +228 to +238
Copy link
Member

@dessalines dessalines Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'll have to remember to create a translation string for "Your application has been sent" or something like that, for when jwt is None on a register.

The other possibility is to add an optional application_sent: bool field to LoginResponse. That might be more clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm doing the registration_application stuff, and I also need to add a registration_sent: bool for that too, so you might as well add one here for verify_email_sent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this now on my PR, so don't worry about this one.

} else {
Some(Claims::jwt(
inserted_local_user.id.0,
&context.secret().jwt_secret,
&context.settings().hostname,
)?,
})
)?)
};
Ok(LoginResponse { jwt })
}
}
4 changes: 1 addition & 3 deletions crates/api_crud/src/user/delete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ impl PerformCrud for DeleteAccount {
})
.await??;

Ok(LoginResponse {
jwt: data.auth.to_owned(),
})
Ok(LoginResponse { jwt: None })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never really made sense in the first place, and now we'll get conflicting responses. Could you create DeleteAccountResponse { success: bool } below here.

}
}
1 change: 1 addition & 0 deletions crates/db_schema/src/aggregates/site_aggregates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ mod tests {
enable_nsfw: None,
updated: None,
community_creation_admin_only: Some(false),
require_email_verification: None,
};

Site::create(&conn, &site_form).unwrap();
Expand Down
Loading