Sending an email to a user to confirm their email address #1045

natboehm · 2017-09-06T14:31:58Z

This PR addresses issue #808, allow editing your own email address. Following the discussion on the issue, this implements the second of three parts, the ability to send a confirmation email to a user for verification. For a full explanation of the issue, see this comment I left which should fully explain the changes made, as well as parts 1 and 3.

In short, I added two tables to the database: one for the user's email, the other for a confirmation token associated with the email. Once the user clicks the link sent, the associated token is deleted from the token table and email_verified is set to true in the email table. It is indicated to the user if they have not verified their email on the account settings page, a message and email resend button being displayed. Sending emails requires Mailgun to be added to the crates.io Heroku account.

This has applications for issue #924 in that we should now be able to send an email to a user, if they have added their email. I know there was a lot of discussion on that issue regarding the ability to send users an email to be added as an owner.

carols10cents

Overall this is great!! This is a lot of work with a whole bunch of pieces, and it's really close!! Just a few polishing things :)

carols10cents · 2017-09-11T18:07:16Z

app/components/email-input.js

+        let email = this.get('user.email');
+        let verified = this.get('user.email_verified');
+
+        if (email != null && !verified) {


I think we could shorten this if/else to just return (email != null && !verified);, what do you think?

carols10cents · 2017-09-11T18:27:31Z

app/templates/error.hbs

@@ -1,5 +0,0 @@
-<h1>Something Went Wrong!</h1>


Is this template not used anywhere anymore? Looks like it was removed in 63facf5, was that intentional? I don't know enough about ember, but it kinda looks like this template is used for generic errors? ex: #240

carols10cents · 2017-09-11T18:54:33Z

src/lib.rs

 #![deny(warnings)]
-#![deny(missing_debug_implementations, missing_copy_implementations)]


Could we put this deny back please :) I'm happy to help fixing any errors!

carols10cents · 2017-09-11T19:01:39Z

src/lib.rs

@@ -1,11 +1,9 @@
 //! This crate implements the backend server for https://crates.io/
 //!
 //! All implemented routes are defined in the [middleware](fn.middleware.html) function and
-//! implemented in the [category](category/index.html), [keyword](keyword/index.html),


Hm, I'm not sure why heroku would have a problem with the category link in this doc comment? (This was changed in ae3683b). There's still the category::index, category::show, and category::slugs functions that go with routes...?

carols10cents · 2017-09-11T19:11:24Z

src/user/mod.rs

+}
+
+fn send_user_confirm_email(email: &str, user_name: &str, token: &str) -> CargoResult<()> {
+    // perhaps use crate lettre and heroku service Mailgun


I think these comments can be removed since you are using lettre and mailgun :)

carols10cents · 2017-09-11T19:32:26Z

src/user/mod.rs

+        smtp_password: env::var("MAILGUN_SMTP_PASSWORD").unwrap_or_else(|_| {
+            String::from("Not found")
+        }),
+        smtp_server: env::var("MAILGUN_SMTP_SERVER").unwrap_or_else(|_| String::from("Not found")),


Ooh another thing we should add is some commented-out placeholders for these environment variables in .env.sample, explaining that these are for sending emails and can be optionally configured locally if you want to try out sending emails but you need a mailgun account, and then probably some longer docs in CONTRIBUTING.md?

Are you up for adding these docs? I'm also happy to help!

carols10cents · 2017-09-11T19:53:17Z

src/user/mod.rs

+        Err(_) => (None, false),
+    };
+
+    let user = User {


wdyt about doing:

let user = User { email: email, ..user_info };

here, to make it clearer that only the email is different? Does that work?

carols10cents · 2017-09-11T19:58:06Z

src/user/mod.rs

-    let user = users.filter(id.eq(user_id)).first::<User>(&*conn)?;
+
+    let user_info = users.filter(id.eq(u_id)).first::<User>(&*conn)?;
+    let email_result = emails.filter(user_id.eq(u_id)).first::<Email>(&*conn);


I wonder if we can get the User and the Email in one query? Maybe not worth it...

carols10cents · 2017-09-11T20:20:35Z

src/user/mod.rs

+            ).as_str(),
+        )
+        .build()
+        .expect("Failed to build confirm email message");


So if this expect gets an Err, this would crash the server, which normally would make me go 😱 🙀 😱 🙀 😱 🙀 but looking at lettre's code, the only way building an email would fail is if:

no from address was set, and we're definitely calling from with something we control

the to address is empty string, and we reject empty emails before calling send_user_confirm_email in update_user

I'm trying to figure out if it's possible that we get Some("") from github and therefore would insert an empty string into the email table and then get an empty email here? I'm going to try some testing and see what happens.

This is also starting to feel a bit spooky-action-at-a-distance since the code that is ensuring the to email isn't empty is far away from here. This could be an issue for any email we send.... Hmmmm.... I wonder if, when the generic email functionality is extracted into a function, if we should pass the generic function strings for to, from, subject, and body, and then the generic function first checks to make sure to isn't an empty string and returns an Err if so, and then we can call expect and point to the line checking that should be nearby as proof that the expect won't fail?

carols10cents · 2017-09-11T20:24:17Z

src/user/mod.rs

+}
+
+fn generate_token() -> String {
+    let token: String = thread_rng().gen_ascii_chars().take(26).collect();


… generated, preliminary test for functionality added

…ests fail

…, tests passing

…g to what is route we get from

… - added confirm route that submits PUT request to backend with error handling

…confirmed, deleted & displays error message when token not found

…mail has been verified

…and navigating to account settings page. Uses GET request to `/me` to get the latest version of the model upon returning successfully from the PUT request to `/confirm`.

One part uses the env variables if found, the other uses an email stub that prints to the env_logger.

…email` will send output to a file named {message_id}.txt in user's `\tmp` directory

…dified

…ct to be an Option for dealing with development versus production modes, change build_email to return a CargoResult<Email> for better error handling

carols10cents · 2017-09-15T17:54:50Z

bors: r+

1045: Sending an email to a user to confirm their email address r=carols10cents This PR addresses issue #808, allow editing your own email address. Following the discussion on the issue, this implements the second of three parts, the ability to send a confirmation email to a user for verification. For a full explanation of the issue, see [this comment](#808 (comment)) I left which should fully explain the changes made, as well as parts 1 and 3. In short, I added two tables to the database: one for the user's email, the other for a confirmation token associated with the email. Once the user clicks the link sent, the associated token is deleted from the token table and `email_verified` is set to `true` in the email table. It is indicated to the user if they have not verified their email on the account settings page, a message and email resend button being displayed. Sending emails requires Mailgun to be added to the crates.io Heroku account. This has applications for issue #924 in that we should now be able to send an email to a user, if they have added their email. I know there was a lot of discussion on that issue regarding the ability to send users an email to be added as an owner.

bors-voyager · 2017-09-15T19:44:56Z

Timed out

carols10cents · 2017-09-15T21:15:55Z

bors: r+

1045: Sending an email to a user to confirm their email address r=carols10cents This PR addresses issue #808, allow editing your own email address. Following the discussion on the issue, this implements the second of three parts, the ability to send a confirmation email to a user for verification. For a full explanation of the issue, see [this comment](#808 (comment)) I left which should fully explain the changes made, as well as parts 1 and 3. In short, I added two tables to the database: one for the user's email, the other for a confirmation token associated with the email. Once the user clicks the link sent, the associated token is deleted from the token table and `email_verified` is set to `true` in the email table. It is indicated to the user if they have not verified their email on the account settings page, a message and email resend button being displayed. Sending emails requires Mailgun to be added to the crates.io Heroku account. This has applications for issue #924 in that we should now be able to send an email to a user, if they have added their email. I know there was a lot of discussion on that issue regarding the ability to send users an email to be added as an owner.

bors-voyager · 2017-09-15T22:32:52Z

Timed out

carols10cents · 2017-09-15T23:46:13Z

bors: r+

1045: Sending an email to a user to confirm their email address r=carols10cents This PR addresses issue #808, allow editing your own email address. Following the discussion on the issue, this implements the second of three parts, the ability to send a confirmation email to a user for verification. For a full explanation of the issue, see [this comment](#808 (comment)) I left which should fully explain the changes made, as well as parts 1 and 3. In short, I added two tables to the database: one for the user's email, the other for a confirmation token associated with the email. Once the user clicks the link sent, the associated token is deleted from the token table and `email_verified` is set to `true` in the email table. It is indicated to the user if they have not verified their email on the account settings page, a message and email resend button being displayed. Sending emails requires Mailgun to be added to the crates.io Heroku account. This has applications for issue #924 in that we should now be able to send an email to a user, if they have added their email. I know there was a lot of discussion on that issue regarding the ability to send users an email to be added as an owner.

bors-voyager · 2017-09-16T00:23:12Z

Build succeeded

continuous-integration/travis-ci/push

carols10cents added the S-waiting-on-review label Sep 9, 2017

carols10cents requested changes Sep 11, 2017

View reviewed changes

natboehm added 28 commits September 14, 2017 15:02

add database migrations and print schema for email and token tables

aa407fc

create/update seems to add to email table

69cec97

on create/update user, email field is added to new table and token is…

e45871e

… generated, preliminary test for functionality added

add conflict target to migrations

225b365

try to add email and table functionality to edit/add email address, t…

0982c3b

…ests fail

for GET /me route, get email from emails table rather than user table…

40c6b0f

…, tests passing

UserShowResponse should be separated into public and private accordin…

44d56eb

…g to what is route we get from

add email_verified field to private user response

2cf7ad0

delete debug print statements

ed0f060

add function for emailing users, no idea if it works

7f21fd1

preliminary send/confirm user email

5ca9ef8

Add routing info and lettre crate for confirming emails

fa305cf

add error handling for confirming user email

232134e

Add lettre crate

9c1511f

delete unused imports

545ace2

test for when user clicks on link to confirm email

490bcb4

beginnings of route to display success/error of user confirming email…

5112f87

… - added confirm route that submits PUT request to backend with error handling

route to confirm user email token displays success when token found, …

ea2a0b4

…confirmed, deleted & displays error message when token not found

remove console logs and update confirm success page

d5a25ad

displays notification on account settings indicating whether or not e…

d0b7e5b

…mail has been verified

add ability to resend verification emails

6056a5c

fixes bug where user model would not reload between confirming email …

bd46867

…and navigating to account settings page. Uses GET request to `/me` to get the latest version of the model upon returning successfully from the PUT request to `/confirm`.

separates into 'development' and 'production' parts

b712905

One part uses the env variables if found, the other uses an email stub that prints to the env_logger.

For testing purposes when env variables not found `send_user_confirm_…

78b4f44

…email` will send output to a file named {message_id}.txt in user's `\tmp` directory

added return result for fn send_user_confirm_email

e295069

this change is just to get my branch to run on my version of heroku

329f021

add debug statements to email sending

ab0826c

figure out why mail isn't sending, should fail

0d0a2f8

natboehm added 14 commits September 14, 2017 15:06

cargo fmt

b716033

clippy reference/dereference error

ba0a123

Add back deleted deny statement, add debug

1c27fa6

shorten if/else in emailNotVerified

d8bf2e6

separate email logic from user logic, created new src/email.rs file

99410d6

add back app/templates/error.hbs

17a433c

simplify User struct construction as email is the only field being mo…

2073d26

…dified

cargo fmt

66bfc44

Remove 'not found' when getting mailgun env vars, change env var stru…

8256def

…ct to be an Option for dealing with development versus production modes, change build_email to return a CargoResult<Email> for better error handling

add category link back to doc comment

25c3cf6

fix clippy errors

ce858f8

mention how to set up email in contributing docs

af3a66d

Add Mailgun configuration variables to .env + explaination

5a3a2b1

prevent null email from being added to emails table

3530771

natboehm force-pushed the confirm-user-email branch from 1ba4a40 to 3530771 Compare September 14, 2017 19:06

add http data files for email tests

26254f4

carols10cents approved these changes Sep 15, 2017

View reviewed changes

bors-voyager bot merged commit 26254f4 into rust-lang:master Sep 16, 2017

natboehm deleted the confirm-user-email branch September 25, 2017 18:01

Turbo87 removed the S-waiting-on-review label Dec 3, 2017

carols10cents mentioned this pull request Nov 7, 2019

Cleanup: remove email field on User in Rust code #1888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sending an email to a user to confirm their email address #1045

Sending an email to a user to confirm their email address #1045

natboehm commented Sep 6, 2017

carols10cents left a comment

carols10cents Sep 11, 2017

carols10cents Sep 11, 2017

carols10cents Sep 11, 2017

carols10cents Sep 11, 2017

carols10cents Sep 11, 2017

carols10cents Sep 11, 2017

carols10cents Sep 11, 2017

carols10cents Sep 11, 2017

carols10cents Sep 11, 2017

carols10cents Sep 11, 2017

carols10cents commented Sep 15, 2017

bors-voyager bot commented Sep 15, 2017

carols10cents commented Sep 15, 2017

bors-voyager bot commented Sep 15, 2017

carols10cents commented Sep 15, 2017

bors-voyager bot commented Sep 16, 2017

		#![deny(warnings)]
		#![deny(missing_debug_implementations, missing_copy_implementations)]

Sending an email to a user to confirm their email address #1045

Sending an email to a user to confirm their email address #1045

Conversation

natboehm commented Sep 6, 2017

carols10cents left a 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

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

carols10cents commented Sep 15, 2017

bors-voyager bot commented Sep 15, 2017

Timed out

carols10cents commented Sep 15, 2017

bors-voyager bot commented Sep 15, 2017

Timed out

carols10cents commented Sep 15, 2017

bors-voyager bot commented Sep 16, 2017

Build succeeded