Implement email verification (fixes #219) #1956

Nutomic · 2021-11-24T21:20:40Z

very much wip

migrations/2021-11-23-132840_email_verification/up.sql

Nutomic · 2021-11-25T17:51:36Z

Updated, works fine now in my testing. It will require some frontend changes though:

Create a frontend page for the email verification and send link to that (instead of raw api link). then also change the verify endpoint to POST
Add error message for email_not_verified on login
When changing email in user settings, the change will only be applied after clicking the verification link, should maybe add a message about that
Add admin checkbox to toggle require_email_verification

dessalines · 2021-11-25T18:04:23Z

Run this to make sure your commit hooks are up to date, it missed an unwrap:

cp .cargo-husky/hooks/pre-commit .git/hooks/

Nutomic · 2021-11-26T12:32:51Z

updated

dessalines · 2021-11-29T16:29:12Z

migrations/2021-11-23-132840_email_verification/up.sql

@@ -0,0 +1,15 @@
+-- use defaults from db for local user init
+alter table local_user alter column theme set default 'browser';
+alter table local_user alter column default_listing_type set default 2;


pub enum ListingType { All, Local, Subscribed, Community, }

I noticed in the DB the default is 1 or Local, but when a new user registers, it uses subscribed. I spose subscribed is the better default here, so this is good.

dessalines · 2021-11-29T16:48:11Z

crates/api_crud/src/user/create.rs

+    // 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;
    }


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

dessalines · 2021-11-29T16:51:48Z

crates/api_crud/src/user/create.rs

+    // 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


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.

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.

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

dessalines · 2021-11-29T16:56:36Z

crates/api_crud/src/user/delete.rs

-    Ok(LoginResponse {
-      jwt: data.auth.to_owned(),
-    })
+    Ok(LoginResponse { jwt: None })


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

dessalines · 2021-11-29T16:59:52Z

crates/db_schema/src/impls/email_verification.rs

+  pub fn read_for_token(conn: &PgConnection, token: &str) -> Result<Self, Error> {
+    use crate::schema::email_verification::dsl::*;
+    email_verification
+      .filter(verification_token.eq(token))


Should also add a time filter to protect this... maybe a week? Example: https://github.com/LemmyNet/lemmy/blob/main/crates/db_schema/src/impls/password_reset_request.rs#L57

dessalines · 2021-12-04T03:32:46Z

crates/db_schema/src/source/local_user.rs

+  pub person_id: Option<PersonId>,
+  pub password_encrypted: Option<String>,


Ignore that above.

dessalines · 2021-11-29T17:09:38Z

crates/websocket/src/email.rs

+  // TODO: link should be replaced with a frontend route once that exists
+  let verify_link = format!(
+    "{}/api/v3/user/verify_email?token={}",


Go ahead and make it {}/verify_email?..., I can start adding that front end soon.

dessalines · 2021-11-29T17:11:14Z

migrations/2021-11-23-132840_email_verification/up.sql

+create table email_verification (
+    id serial primary key,
+    local_user_id int references local_user(id) on update cascade on delete cascade not null,
+    email text not null,


This should be removed. There should only be one email field, on the local_user table.

dessalines · 2021-11-29T17:11:25Z

migrations/2021-11-23-132840_email_verification/up.sql

+    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
+


Add published.

dessalines · 2021-11-29T17:12:01Z

src/api_routes.rs

-          .route("/unread_count", web::get().to(route_get::<GetUnreadCount>)),
+          .route("/unread_count", web::get().to(route_get::<GetUnreadCount>))
+          // TODO: currently GET for easier testing, but should probably be POST
+          .route("/verify_email", web::get().to(route_get::<VerifyEmail>)),


Go ahead and make POST, I can start working on the front end for this soon.

dessalines · 2021-12-02T19:30:58Z

Don't force push to this, because I'll be working off it from another branch.

dessalines · 2021-12-04T01:22:39Z

crates/api_crud/src/user/create.rs

-      show_read_posts: Some(true),
-      show_new_post_notifs: Some(false),
-      send_notifications_to_email: Some(false),
+      email_verified: Some(!email_verification),


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)

dessalines · 2021-12-04T03:32:46Z

crates/db_schema/src/source/local_user.rs

+  pub person_id: Option<PersonId>,
+  pub password_encrypted: Option<String>,


Ignore that above.

dessalines · 2021-12-08T15:57:33Z

I'll close this as I've merged its work and some updates into #1949

Nutomic force-pushed the email-verification branch from 5f48865 to c278055 Compare November 24, 2021 22:39

dessalines reviewed Nov 24, 2021

View reviewed changes

migrations/2021-11-23-132840_email_verification/up.sql Outdated Show resolved Hide resolved

dessalines reviewed Nov 24, 2021

View reviewed changes

migrations/2021-11-23-132840_email_verification/up.sql Show resolved Hide resolved

Implement email verification (fixes #219)

c199215

Nutomic force-pushed the email-verification branch from c278055 to c199215 Compare November 25, 2021 17:50

Nutomic changed the title ~~WIP: Implement email verification (fixes #219)~~ Implement email verification (fixes #219) Nov 25, 2021

Nutomic marked this pull request as ready for review November 25, 2021 17:51

remove unwrap

e43bc67

dessalines reviewed Nov 29, 2021

View reviewed changes

dessalines reviewed Dec 4, 2021

View reviewed changes

dessalines closed this Dec 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement email verification (fixes #219) #1956

Implement email verification (fixes #219) #1956

Nutomic commented Nov 24, 2021

Nutomic commented Nov 25, 2021

dessalines commented Nov 25, 2021

Nutomic commented Nov 26, 2021

dessalines Nov 29, 2021

dessalines Nov 29, 2021

dessalines Nov 29, 2021 •

edited

Loading

dessalines Dec 4, 2021

dessalines Dec 4, 2021

dessalines Nov 29, 2021

dessalines Nov 29, 2021

dessalines Dec 4, 2021

dessalines Nov 29, 2021

dessalines Nov 29, 2021 •

edited

Loading

dessalines Nov 29, 2021

dessalines Nov 29, 2021

dessalines commented Dec 2, 2021

dessalines Dec 4, 2021

dessalines Dec 4, 2021

dessalines commented Dec 8, 2021

		pub person_id: Option<PersonId>,
		pub password_encrypted: Option<String>,

Implement email verification (fixes #219) #1956

Implement email verification (fixes #219) #1956

Conversation

Nutomic commented Nov 24, 2021

Nutomic commented Nov 25, 2021

dessalines commented Nov 25, 2021

Nutomic commented Nov 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines commented Dec 8, 2021

dessalines Nov 29, 2021 •

edited

Loading

dessalines Nov 29, 2021 •

edited

Loading