Auto merge of rust-lang#1622 - integer32llc:invalid_emails_from_github, r=sgrif

Invalid emails from GitHub

This supersedes rust-lang#1251.

Author: bors

src/controllers/user/me.rs

@@ -3,6 +3,7 @@ use crate::controllers::prelude::*;
 use crate::controllers::helpers::Paginate;
 use crate::email;
 use crate::util::bad_request;
+use crate::util::errors::CargoError;
 
 use crate::models::{Email, Follow, NewEmail, User, Version};
 use crate::schema::{crates, emails, follows, users, versions};
@@ -127,7 +128,7 @@ pub fn update_user(req: &mut dyn Request) -> CargoResult<Response> {
         return Err(human("empty email rejected"));
     }
 
-    conn.transaction(|| {
+    conn.transaction::<_, Box<dyn CargoError>, _>(|| {
         update(users.filter(gh_login.eq(&user.gh_login)))
             .set(email.eq(user_email))
             .execute(&*conn)?;
@@ -146,8 +147,9 @@ pub fn update_user(req: &mut dyn Request) -> CargoResult<Response> {
             .get_result::<String>(&*conn)
             .map_err(|_| human("Error in creating token"))?;
 
-        crate::email::send_user_confirm_email(user_email, &user.gh_login, &token)
-            .map_err(|_| bad_request("Email could not be sent"))
+        crate::email::send_user_confirm_email(user_email, &user.gh_login, &token);
+
+        Ok(())
     })?;
 
     #[derive(Serialize)]
@@ -199,7 +201,7 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> CargoResult<Response>
             .get_result::<Email>(&*conn)
             .map_err(|_| bad_request("Email could not be found"))?;
 
-        email::send_user_confirm_email(&email.email, &user.gh_login, &email.token)
+        email::try_send_user_confirm_email(&email.email, &user.gh_login, &email.token)
             .map_err(|_| bad_request("Error in sending email"))
     })?;
 

src/controllers/user/session.rs

@@ -4,7 +4,7 @@ use crate::github;
 use conduit_cookie::RequestSession;
 use rand::{thread_rng, Rng};
 
-use crate::models::NewUser;
+use crate::models::{NewUser, User};
 
 /// Handles the `GET /authorize_url` route.
 ///
@@ -84,37 +84,76 @@ pub fn github_access_token(req: &mut dyn Request) -> CargoResult<Response> {
         }
     }
 
-    #[derive(Deserialize)]
-    struct GithubUser {
-        email: Option<String>,
-        name: Option<String>,
-        login: String,
-        id: i32,
-        avatar_url: Option<String>,
-    }
-
     // Fetch the access token from github using the code we just got
     let token = req.app().github.exchange(code).map_err(|s| human(&s))?;
 
     let ghuser = github::github::<GithubUser>(req.app(), "/user", &token)?;
+    let user = ghuser.save_to_database(&token.access_token, &*req.db_conn()?)?;
 
-    let user = NewUser::new(
-        ghuser.id,
-        &ghuser.login,
-        ghuser.email.as_ref().map(|s| &s[..]),
-        ghuser.name.as_ref().map(|s| &s[..]),
-        ghuser.avatar_url.as_ref().map(|s| &s[..]),
-        &token.access_token,
-    )
-    .create_or_update(&*req.db_conn()?)?;
     req.session()
         .insert("user_id".to_string(), user.id.to_string());
     req.mut_extensions().insert(user);
     super::me::me(req)
 }
 
+#[derive(Deserialize)]
+struct GithubUser {
+    email: Option<String>,
+    name: Option<String>,
+    login: String,
+    id: i32,
+    avatar_url: Option<String>,
+}
+
+impl GithubUser {
+    fn save_to_database(&self, access_token: &str, conn: &PgConnection) -> QueryResult<User> {
+        Ok(NewUser::new(
+            self.id,
+            &self.login,
+            self.email.as_ref().map(|s| &s[..]),
+            self.name.as_ref().map(|s| &s[..]),
+            self.avatar_url.as_ref().map(|s| &s[..]),
+            access_token,
+        )
+        .create_or_update(conn)?)
+    }
+}
+
 /// Handles the `GET /logout` route.
 pub fn logout(req: &mut dyn Request) -> CargoResult<Response> {
     req.session().remove(&"user_id".to_string());
     Ok(req.json(&true))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use dotenv::dotenv;
+    use std::env;
+
+    fn pg_connection() -> PgConnection {
+        let _ = dotenv();
+        let database_url =
+            env::var("TEST_DATABASE_URL").expect("TEST_DATABASE_URL must be set to run tests");
+        PgConnection::establish(&database_url).unwrap()
+    }
+
+    #[test]
+    fn gh_user_with_invalid_email_doesnt_fail() {
+        let conn = pg_connection();
+        let gh_user = GithubUser {
+            email: Some("String.Format(\"{0}.{1}@live.com\", FirstName, LastName)".into()),
+            name: Some("My Name".into()),
+            login: "github_user".into(),
+            id: -1,
+            avatar_url: None,
+        };
+        let result = gh_user.save_to_database("arbitrary_token", &conn);
+
+        assert!(
+            result.is_ok(),
+            "Creating a User from a GitHub user failed when it shouldn't have, {:?}",
+            result
+        );
+    }
+}

src/email.rs

@@ -57,7 +57,22 @@ fn build_email(
     Ok(email.into())
 }
 
-pub 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
+/// have an invalid email set in their GitHub profile, and we should let them sign in even though
+/// we're trying to silently use their invalid address during signup and can't send them an email.
+/// Use `try_send_user_confirm_email` when the user is directly trying to set their email.
+pub fn send_user_confirm_email(email: &str, user_name: &str, token: &str) {
+    let _ = try_send_user_confirm_email(email, user_name, token);
+}
+
+/// Attempts to send a confirmation email and returns errors.
+///
+/// For use in cases where we want to fail if an email is bad because the user is directly trying
+/// to set their email correctly, as opposed to us silently trying to use the email from their
+/// GitHub profile during signup.
+pub 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
@@ -100,3 +115,24 @@ fn send_email(recipient: &str, subject: &str, body: &str) -> CargoResult<()> {
 
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn sending_to_invalid_email_fails() {
+        let result = send_email(
+            "String.Format(\"{0}.{1}@live.com\", FirstName, LastName)",
+            "test",
+            "test",
+        );
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn sending_to_valid_email_succeeds() {
+        let result = send_email("[email protected]", "test", "test");
+        assert!(result.is_ok());
+    }
+}

src/models/user.rs

@@ -58,7 +58,6 @@ impl<'a> NewUser<'a> {
         use diesel::insert_into;
         use diesel::pg::upsert::excluded;
         use diesel::sql_types::Integer;
-        use diesel::NotFound;
 
         conn.transaction(|| {
             let user = insert_into(users)
@@ -96,8 +95,7 @@ impl<'a> NewUser<'a> {
                     .optional()?;
 
                 if let Some(token) = token {
-                    crate::email::send_user_confirm_email(user_email, &user.gh_login, &token)
-                        .map_err(|_| NotFound)?;
+                    crate::email::send_user_confirm_email(user_email, &user.gh_login, &token);
                 }
             }
 

src/tests/user.rs

@@ -270,10 +270,7 @@ fn test_github_login_does_not_overwrite_email() {
     let mut req = req(Method::Get, "/api/v1/me");
     let user = {
         let conn = app.diesel_database.get().unwrap();
-        let user = NewUser {
-            gh_id: 1,
-            ..new_user("apricot")
-        };
+        let user = new_user("apricot");
 
         let user = user.create_or_update(&conn).unwrap();
         sign_in_as(&mut req, &user);
@@ -299,7 +296,7 @@ fn test_github_login_does_not_overwrite_email() {
     {
         let conn = app.diesel_database.get().unwrap();
         let user = NewUser {
-            gh_id: 1,
+            gh_id: user.gh_id,
             ..new_user("apricot")
         };
 
@@ -449,17 +446,17 @@ fn test_this_user_cannot_change_that_user_email() {
 fn test_insert_into_email_table() {
     let (_b, app, middle) = app();
     let mut req = req(Method::Get, "/me");
-    {
+    let user = {
         let conn = app.diesel_database.get().unwrap();
         let user = NewUser {
-            gh_id: 1,
             email: Some("[email protected]"),
             ..new_user("apple")
         };
 
         let user = user.create_or_update(&conn).unwrap();
         sign_in_as(&mut req, &user);
-    }
+        user
+    };
 
     let mut response = ok_resp!(middle.call(req.with_path("/api/v1/me").with_method(Method::Get),));
     let r = crate::json::<UserShowPrivateResponse>(&mut response);
@@ -472,7 +469,7 @@ fn test_insert_into_email_table() {
     {
         let conn = app.diesel_database.get().unwrap();
         let user = NewUser {
-            gh_id: 1,
+            gh_id: user.gh_id,
             email: Some("[email protected]"),
             ..new_user("apple")
         };
@@ -499,7 +496,6 @@ fn test_insert_into_email_table_with_email_change() {
     let user = {
         let conn = app.diesel_database.get().unwrap();
         let user = NewUser {
-            gh_id: 1,
             email: Some("[email protected]"),
             ..new_user("potato")
         };
@@ -531,7 +527,7 @@ fn test_insert_into_email_table_with_email_change() {
     {
         let conn = app.diesel_database.get().unwrap();
         let user = NewUser {
-            gh_id: 1,
+            gh_id: user.gh_id,
             email: Some("[email protected]"),
             ..new_user("potato")
         };