diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index b185c9fa669..f1c0d688e26 100644 --- a/src/controllers/user/me.rs +++ b/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 { return Err(human("empty email rejected")); } - conn.transaction(|| { + conn.transaction::<_, Box, _>(|| { 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 { .get_result::(&*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 .get_result::(&*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")) })?; diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index db038a87bcf..a0a07633948 100644 --- a/src/controllers/user/session.rs +++ b/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 { } } - #[derive(Deserialize)] - struct GithubUser { - email: Option, - name: Option, - login: String, - id: i32, - avatar_url: Option, - } - // 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::(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, + name: Option, + login: String, + id: i32, + avatar_url: Option, +} + +impl GithubUser { + fn save_to_database(&self, access_token: &str, conn: &PgConnection) -> QueryResult { + 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 { 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 + ); + } +} diff --git a/src/email.rs b/src/email.rs index b18d0116278..132560f7c1d 100644 --- a/src/email.rs +++ b/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("someone@example.com", "test", "test"); + assert!(result.is_ok()); + } +} diff --git a/src/models/user.rs b/src/models/user.rs index 83ee9b96d5d..5cf4715b30e 100644 --- a/src/models/user.rs +++ b/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); } } diff --git a/src/tests/user.rs b/src/tests/user.rs index b04933d0241..cb37522eb79 100644 --- a/src/tests/user.rs +++ b/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("apple@example.com"), ..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::(&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("banana@example.com"), ..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("test_insert_with_change@example.com"), ..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("banana2@example.com"), ..new_user("potato") };