Skip to content

Commit d08de9f

Browse files
committed
Don't require a valid email address to sign in
Users do not necessarily have a valid email in GitHub. We should not fail most actions if we cannot successfully send them an email. I have purposely left the error case on the `users/email/token/regenerate` endpoint, since that endpoint clearly has an email purpose in mind. I opted to change the signature of the existing name in order to force a compiler error on any code that hasn't been looked at to determine whether it should fail on this or not. Fixes #1142
1 parent 47998b7 commit d08de9f

File tree

1 file changed

+18
-9
lines changed

1 file changed

+18
-9
lines changed

src/user/mod.rs

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use serde_json;
1111
use app::RequestApp;
1212
use db::RequestTransaction;
1313
use controllers::helpers::Paginate;
14-
use util::{bad_request, human, CargoResult, RequestUtils};
14+
use util::{bad_request, human, CargoError, CargoResult, RequestUtils};
1515
use github;
1616
use email;
1717

@@ -89,7 +89,6 @@ impl<'a> NewUser<'a> {
8989
use diesel::dsl::sql;
9090
use diesel::sql_types::Integer;
9191
use diesel::pg::upsert::excluded;
92-
use diesel::NotFound;
9392
use schema::users::dsl::*;
9493

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

130129
if let Some(token) = token {
131-
send_user_confirm_email(user_email, &user.gh_login, &token)
132-
.map_err(|_| NotFound)?;
130+
send_user_confirm_email(user_email, &user.gh_login, &token);
133131
}
134132
}
135133

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

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

563-
send_user_confirm_email(user_email, &user.gh_login, &token)
564-
.map_err(|_| bad_request("Email could not be sent"))
561+
send_user_confirm_email(user_email, &user.gh_login, &token);
562+
563+
Ok(())
565564
})?;
566565

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

574-
fn send_user_confirm_email(email: &str, user_name: &str, token: &str) -> CargoResult<()> {
573+
/// Attempts to send a confirmation email. Swallows all errors.
574+
///
575+
/// This function swallows any errors that occur while attempting to send the
576+
/// email. Some users do not have any valid email set in their GitHub profile.
577+
/// We cannot force them to give us an email address, and that should not stop
578+
/// them from signing in.
579+
fn send_user_confirm_email(email: &str, user_name: &str, token: &str) {
580+
let _ = try_send_user_confirm_email(email, user_name, token);
581+
}
582+
583+
fn try_send_user_confirm_email(email: &str, user_name: &str, token: &str) -> CargoResult<()> {
575584
// Create a URL with token string as path to send to user
576585
// If user clicks on path, look email/user up in database,
577586
// make sure tokens match
@@ -629,7 +638,7 @@ pub fn regenerate_token_and_send(req: &mut Request) -> CargoResult<Response> {
629638
.get_result::<Email>(&*conn)
630639
.map_err(|_| bad_request("Email could not be found"))?;
631640

632-
send_user_confirm_email(&email.email, &user.gh_login, &email.token)
641+
try_send_user_confirm_email(&email.email, &user.gh_login, &email.token)
633642
.map_err(|_| bad_request("Error in sending email"))
634643
})?;
635644

0 commit comments

Comments
 (0)