Skip to content

Don't require a valid email address to sign in #1251

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Don't require a valid email address to sign in #1251

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 18 additions & 9 deletions src/user/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use serde_json;
use app::RequestApp;
use db::RequestTransaction;
use controllers::helpers::Paginate;
use util::{bad_request, human, CargoResult, RequestUtils};
use util::{bad_request, human, CargoError, CargoResult, RequestUtils};
use github;
use email;

Expand Down Expand Up @@ -89,7 +89,6 @@ impl<'a> NewUser<'a> {
use diesel::dsl::sql;
use diesel::sql_types::Integer;
use diesel::pg::upsert::excluded;
use diesel::NotFound;
use schema::users::dsl::*;

conn.transaction(|| {
Expand Down Expand Up @@ -128,8 +127,7 @@ impl<'a> NewUser<'a> {
.optional()?;

if let Some(token) = token {
send_user_confirm_email(user_email, &user.gh_login, &token)
.map_err(|_| NotFound)?;
send_user_confirm_email(user_email, &user.gh_login, &token);
Copy link
Member

Choose a reason for hiding this comment

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

If there isn't an email, we shouldn't even be in this block because of the if let Some(user_email) = user.email.as_ref() { above, right? And if the email is present but not valid, we don't find that out synchronously. Emails get put in mailgun's queue and then their dashboard shows email sending success/failure.

Copy link
Member

Choose a reason for hiding this comment

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

So what I think what we were seeing here is that the email address is being rejected on our end (probably in lettre) before we even generate an email and give it to mailgun. For instance, if somebody manages to put invalid as their email address in GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

As a more specific example, here is the email address that was giving one user issues: String.Format(\"{0}.{1}@live.com\", FirstName, LastName). After looking at the profile a dozen times, I only just now realized that this is the email address on their profile page.

Copy link
Member

Choose a reason for hiding this comment

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

That example would make a great test case and would certainly clarify this PR that it's not sending the email that fails, but that there's verification of an email address's validity in lettre that fails and returns an error. That wasn't at all clear from the original PR.

}
}

Expand Down Expand Up @@ -541,7 +539,7 @@ pub fn update_user(req: &mut Request) -> CargoResult<Response> {
return Err(human("empty email rejected"));
}

conn.transaction(|| {
conn.transaction::<_, Box<CargoError>, _>(|| {
update(users.filter(gh_login.eq(&user.gh_login)))
.set(email.eq(user_email))
.execute(&*conn)?;
Expand All @@ -560,8 +558,9 @@ pub fn update_user(req: &mut Request) -> CargoResult<Response> {
.get_result::<String>(&*conn)
.map_err(|_| human("Error in creating token"))?;

send_user_confirm_email(user_email, &user.gh_login, &token)
.map_err(|_| bad_request("Email could not be sent"))
send_user_confirm_email(user_email, &user.gh_login, &token);

Ok(())
})?;

#[derive(Serialize)]
Expand All @@ -571,7 +570,17 @@ pub fn update_user(req: &mut Request) -> CargoResult<Response> {
Ok(req.json(&R { ok: true }))
}

fn send_user_confirm_email(email: &str, user_name: &str, token: &str) -> CargoResult<()> {
/// Attempts to send a confirmation email. Swallows all errors.
///
/// This function swallows any errors that occur while attempting to send the
/// email. Some users do not have any valid email set in their GitHub profile.
/// We cannot force them to give us an email address, and that should not stop
/// them from signing in.
fn send_user_confirm_email(email: &str, user_name: &str, token: &str) {
let _ = try_send_user_confirm_email(email, user_name, token);
}

fn try_send_user_confirm_email(email: &str, user_name: &str, token: &str) -> CargoResult<()> {
// Create a URL with token string as path to send to user
// If user clicks on path, look email/user up in database,
// make sure tokens match
Expand Down Expand Up @@ -629,7 +638,7 @@ pub fn regenerate_token_and_send(req: &mut Request) -> CargoResult<Response> {
.get_result::<Email>(&*conn)
.map_err(|_| bad_request("Email could not be found"))?;

send_user_confirm_email(&email.email, &user.gh_login, &email.token)
try_send_user_confirm_email(&email.email, &user.gh_login, &email.token)
.map_err(|_| bad_request("Error in sending email"))
})?;

Expand Down