diff --git a/migrations/2017-09-23-182408_move_tokens_to_emails_table/down.sql b/migrations/2017-09-23-182408_move_tokens_to_emails_table/down.sql new file mode 100644 index 00000000000..69dfbe02223 --- /dev/null +++ b/migrations/2017-09-23-182408_move_tokens_to_emails_table/down.sql @@ -0,0 +1,17 @@ +DROP TRIGGER trigger_emails_reconfirm ON emails; +DROP TRIGGER trigger_emails_set_token_generated_at ON emails; +DROP FUNCTION reconfirm_email_on_email_change(); +DROP FUNCTION emails_set_token_generated_at(); + +CREATE TABLE tokens ( + id SERIAL PRIMARY KEY, + email_id INTEGER NOT NULL UNIQUE REFERENCES emails (id), + token TEXT NOT NULL, + created_at TIMESTAMP NOT NULL DEFAULT NOW() +); + +INSERT INTO tokens (email_id, token, created_at) +SELECT id, token, token_generated_at FROM emails WHERE token_generated_at IS NOT NULL; + +ALTER TABLE emails DROP COLUMN token; +ALTER TABLE emails DROP COLUMN token_generated_at; diff --git a/migrations/2017-09-23-182408_move_tokens_to_emails_table/up.sql b/migrations/2017-09-23-182408_move_tokens_to_emails_table/up.sql new file mode 100644 index 00000000000..9245aee0e2a --- /dev/null +++ b/migrations/2017-09-23-182408_move_tokens_to_emails_table/up.sql @@ -0,0 +1,30 @@ +ALTER TABLE emails ADD COLUMN token TEXT NOT NULL DEFAULT random_string(26); +ALTER TABLE emails ADD COLUMN token_generated_at TIMESTAMP; +UPDATE emails SET token = tokens.token, token_generated_at = tokens.created_at + FROM tokens WHERE tokens.email_id = emails.id; +DROP TABLE tokens; + +CREATE FUNCTION emails_set_token_generated_at() RETURNS trigger AS $$ + BEGIN + NEW.token_generated_at := CURRENT_TIMESTAMP; + RETURN NEW; + END +$$ LANGUAGE plpgsql; + +CREATE FUNCTION reconfirm_email_on_email_change() RETURNS trigger AS $$ + BEGIN + IF NEW.email IS DISTINCT FROM OLD.email THEN + NEW.token := random_string(26); + NEW.verified := false; + END IF; + RETURN NEW; + END +$$ LANGUAGE plpgsql; + +CREATE TRIGGER trigger_emails_set_token_generated_at BEFORE +INSERT OR UPDATE OF token ON emails +FOR EACH ROW EXECUTE PROCEDURE emails_set_token_generated_at(); + +CREATE TRIGGER trigger_emails_reconfirm BEFORE UPDATE +ON emails +FOR EACH ROW EXECUTE PROCEDURE reconfirm_email_on_email_change(); diff --git a/migrations/2017-09-23-191545_add_users_to_emails_foreign_key/down.sql b/migrations/2017-09-23-191545_add_users_to_emails_foreign_key/down.sql new file mode 100644 index 00000000000..dc9e007eeee --- /dev/null +++ b/migrations/2017-09-23-191545_add_users_to_emails_foreign_key/down.sql @@ -0,0 +1 @@ +ALTER TABLE emails DROP CONSTRAINT fk_emails_user_id; diff --git a/migrations/2017-09-23-191545_add_users_to_emails_foreign_key/up.sql b/migrations/2017-09-23-191545_add_users_to_emails_foreign_key/up.sql new file mode 100644 index 00000000000..369c6adf35b --- /dev/null +++ b/migrations/2017-09-23-191545_add_users_to_emails_foreign_key/up.sql @@ -0,0 +1 @@ +ALTER TABLE emails ADD CONSTRAINT fk_emails_user_id FOREIGN KEY (user_id) REFERENCES users (id); diff --git a/src/schema.rs b/src/schema.rs index 6c16d5cc658..4dd4ccc40fc 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -437,6 +437,18 @@ table! { /// /// (Automatically generated by Diesel.) verified -> Bool, + /// The `token` column of the `emails` table. + /// + /// Its SQL type is `Text`. + /// + /// (Automatically generated by Diesel.) + token -> Text, + /// The `token_generated_at` column of the `emails` table. + /// + /// Its SQL type is `Nullable`. + /// + /// (Automatically generated by Diesel.) + token_generated_at -> Nullable, } } @@ -558,38 +570,6 @@ table! { } } -table! { - /// Representation of the `tokens` table. - /// - /// (Automatically generated by Diesel.) - tokens (id) { - /// The `id` column of the `tokens` table. - /// - /// Its SQL type is `Int4`. - /// - /// (Automatically generated by Diesel.) - id -> Int4, - /// The `email_id` column of the `tokens` table. - /// - /// Its SQL type is `Int4`. - /// - /// (Automatically generated by Diesel.) - email_id -> Int4, - /// The `token` column of the `tokens` table. - /// - /// Its SQL type is `Varchar`. - /// - /// (Automatically generated by Diesel.) - token -> Varchar, - /// The `created_at` column of the `tokens` table. - /// - /// Its SQL type is `Timestamp`. - /// - /// (Automatically generated by Diesel.) - created_at -> Timestamp, - } -} - table! { /// Representation of the `users` table. /// @@ -784,22 +764,22 @@ table! { } } -joinable!(follows -> users (user_id)); -joinable!(version_authors -> users (user_id)); -joinable!(crates_keywords -> keywords (keyword_id)); -joinable!(crates_categories -> categories (category_id)); joinable!(api_tokens -> users (user_id)); joinable!(crate_downloads -> crates (crate_id)); +joinable!(crate_owner_invitations -> crates (crate_id)); joinable!(crate_owners -> crates (crate_id)); +joinable!(crate_owners -> teams (owner_id)); +joinable!(crate_owners -> users (owner_id)); +joinable!(crates_categories -> categories (category_id)); joinable!(crates_categories -> crates (crate_id)); joinable!(crates_keywords -> crates (crate_id)); +joinable!(crates_keywords -> keywords (keyword_id)); joinable!(dependencies -> crates (crate_id)); -joinable!(follows -> crates (crate_id)); -joinable!(versions -> crates (crate_id)); joinable!(dependencies -> versions (version_id)); +joinable!(emails -> users (user_id)); +joinable!(follows -> crates (crate_id)); +joinable!(follows -> users (user_id)); +joinable!(version_authors -> users (user_id)); joinable!(version_authors -> versions (version_id)); joinable!(version_downloads -> versions (version_id)); -joinable!(crate_owners -> teams (owner_id)); -joinable!(crate_owners -> users (owner_id)); -joinable!(crate_owner_invitations -> crates (crate_id)); -joinable!(tokens -> emails (email_id)); +joinable!(versions -> crates (crate_id)); diff --git a/src/tests/user.rs b/src/tests/user.rs index e1d88d9ed7b..39c237a6dfb 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -4,8 +4,7 @@ use conduit::{Handler, Method}; use cargo_registry::token::ApiToken; use cargo_registry::krate::EncodableCrate; -use cargo_registry::user::{Email, EncodablePrivateUser, EncodablePublicUser, NewEmail, NewUser, - Token, User}; +use cargo_registry::user::{Email, EncodablePrivateUser, EncodablePublicUser, NewUser, User}; use cargo_registry::version::EncodableVersion; use diesel::prelude::*; @@ -658,7 +657,7 @@ fn test_insert_into_email_table_with_email_change() { */ #[test] fn test_confirm_user_email() { - use cargo_registry::schema::{emails, tokens}; + use cargo_registry::schema::emails; #[derive(Deserialize)] struct R { @@ -675,7 +674,7 @@ fn test_confirm_user_email() { let user = { let conn = app.diesel_database.get().unwrap(); let user = NewUser { - email: Some("potato@example.com"), + email: Some("potato2@example.com"), ..::new_user("potato") }; @@ -686,15 +685,10 @@ fn test_confirm_user_email() { let email_token = { let conn = app.diesel_database.get().unwrap(); - let email_info = emails::table - .filter(emails::user_id.eq(user.id)) - .first::(&*conn) - .unwrap(); - let token_info = tokens::table - .filter(tokens::email_id.eq(email_info.id)) - .first::(&*conn) - .unwrap(); - token_info.token + Email::belonging_to(&user) + .select(emails::token) + .first::(&*conn) + .unwrap() }; let mut response = ok_resp!( @@ -707,7 +701,7 @@ fn test_confirm_user_email() { let mut response = ok_resp!(middle.call(req.with_path("/api/v1/me").with_method(Method::Get),)); let r = ::json::(&mut response); - assert_eq!(r.user.email.unwrap(), "potato@example.com"); + assert_eq!(r.user.email.unwrap(), "potato2@example.com"); assert_eq!(r.user.login, "potato"); assert!(r.user.email_verified); assert!(r.user.email_verification_sent); @@ -719,8 +713,9 @@ fn test_confirm_user_email() { */ #[test] fn test_existing_user_email() { - use cargo_registry::schema::{emails, users}; - use diesel::insert; + use cargo_registry::schema::emails; + use chrono::NaiveDateTime; + use diesel::update; #[derive(Deserialize)] struct R { @@ -731,24 +726,17 @@ fn test_existing_user_email() { let mut req = ::req(app.clone(), Method::Get, "/me"); { let conn = app.diesel_database.get().unwrap(); - let user = ::new_user("potahto"); - - // Deliberately not using User::create_or_update since that - // will try to send a verification email; we want to simulate - // a user who already had an email before we added verification. - let user = insert(&user) - .into(users::table) - .get_result::(&*conn) - .unwrap(); - - let email = NewEmail { - user_id: user.id, - email: "potahto@example.com", - verified: false, + let new_user = NewUser { + email: Some("potahto@example.com"), + ..::new_user("potahto") }; - - insert(&email).into(emails::table).execute(&*conn).unwrap(); - + let user = new_user.create_or_update(&conn).unwrap(); + update(Email::belonging_to(&user)) + // Users created before we added verification will have + // `NULL` in the `token_generated_at` column. + .set(emails::token_generated_at.eq(None::)) + .execute(&*conn) + .unwrap(); ::sign_in_as(&mut req, &user); } diff --git a/src/user/mod.rs b/src/user/mod.rs index 68a4193af58..e36f466fdec 100644 --- a/src/user/mod.rs +++ b/src/user/mod.rs @@ -54,54 +54,15 @@ pub struct Email { pub user_id: i32, pub email: String, pub verified: bool, -} - -#[derive(Debug, Insertable, AsChangeset)] -#[table_name = "emails"] -pub struct NewEmail<'a> { - pub user_id: i32, - pub email: &'a str, - pub verified: bool, -} - -#[derive(Debug, Queryable, AsChangeset, Identifiable, Associations)] -#[belongs_to(Email)] -pub struct Token { - pub id: i32, - pub email_id: i32, pub token: String, - pub created_at: NaiveDateTime, + pub token_generated_at: Option, } #[derive(Debug, Insertable, AsChangeset)] -#[table_name = "tokens"] -pub struct NewToken { - email_id: i32, - token: String, -} - -impl NewToken { - fn generate(email_id: i32) -> Self { - NewToken { - token: generate_token(), - email_id, - } - } - - fn regenerate(id: i32, conn: &PgConnection) -> QueryResult { - use diesel::insert; - use diesel::expression::now; - use diesel::pg::upsert::*; - use schema::tokens::dsl::{created_at, email_id, token, tokens}; - - let new_token = Self::generate(id); - insert(&new_token.on_conflict( - email_id, - do_update().set((token.eq(&new_token.token), created_at.eq(now))), - )).into(tokens) - .returning(token) - .get_result(conn) - } +#[table_name = "emails"] +struct NewEmail<'a> { + user_id: i32, + email: &'a str, } impl<'a> NewUser<'a> { @@ -159,21 +120,16 @@ impl<'a> NewUser<'a> { let new_email = NewEmail { user_id: user.id, email: user_email, - verified: false, }; - let email_id = insert(&new_email.on_conflict_do_nothing()) + let token = insert(&new_email.on_conflict_do_nothing()) .into(emails::table) - .returning(emails::id) - .get_result(conn) + .returning(emails::token) + .get_result::(conn) .optional()?; - if let Some(id) = email_id { - let new_token = NewToken::generate(id); - - insert(&new_token).into(tokens::table).execute(conn)?; - - send_user_confirm_email(user_email, &user.gh_login, &new_token.token) + if let Some(token) = token { + send_user_confirm_email(user_email, &user.gh_login, &token) .map_err(|_| NotFound)?; } } @@ -411,38 +367,25 @@ pub fn me(req: &mut Request) -> CargoResult { // perhaps adding `req.mut_extensions().insert(user)` to the // update_user route, however this somehow does not seem to work - use self::users::dsl::{id, users}; - use self::emails::dsl::{emails, user_id}; - use diesel::select; - use diesel::expression::dsl::exists; - - let u_id = req.user()?.id; + let id = req.user()?.id; let conn = req.db_conn()?; - let user_info = users.filter(id.eq(u_id)).first::(&*conn)?; - let email_result = emails.filter(user_id.eq(u_id)).first::(&*conn); - - let (email, verified, verification_sent): (Option, bool, bool) = match email_result { - Ok(email_record) => { - let verification_sent = if email_record.verified { - true - } else { - select(exists(Token::belonging_to(&email_record))) - .get_result(&*conn) - .unwrap_or(false) - }; - ( - Some(email_record.email), - email_record.verified, - verification_sent, - ) - } - Err(_) => (None, false, false), - }; - + let (user, verified, email, verification_sent) = users::table + .find(id) + .left_join(emails::table) + .select(( + users::all_columns, + emails::verified.nullable(), + emails::email.nullable(), + emails::token_generated_at.nullable().is_not_null(), + )) + .first::<(User, Option, Option, bool)>(&*conn)?; + + let verified = verified.unwrap_or(false); + let verification_sent = verified || verification_sent; let user = User { email: email, - ..user_info + ..user }; #[derive(Serialize)] @@ -610,15 +553,13 @@ pub fn update_user(req: &mut Request) -> CargoResult { let new_email = NewEmail { user_id: user.id, email: user_email, - verified: false, }; - let email_id = insert(&new_email.on_conflict(user_id, do_update().set(&new_email))) + let token = insert(&new_email.on_conflict(user_id, do_update().set(&new_email))) .into(emails::table) - .returning(emails::id) - .get_result(&*conn) - .map_err(|_| human("Error in updating email"))?; - let token = NewToken::regenerate(email_id, &conn)?; + .returning(emails::token) + .get_result::(&*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")) @@ -650,34 +591,18 @@ https://crates.io/confirm/{}", /// Handles the `PUT /confirm/:email_token` route pub fn confirm_user_email(req: &mut Request) -> CargoResult { - // to confirm, we must grab the token on the request as part of the URL - // look up the token in the tokens table - // find what user the token belongs to - // on the email table, change 'verified' to true - // delete the token from the tokens table - use diesel::{delete, update}; + use diesel::update; let conn = req.db_conn()?; let req_token = &req.params()["email_token"]; - let token_info = tokens::table - .filter(tokens::token.eq(req_token)) - .first::(&*conn) - .map_err(|_| bad_request("Email token not found."))?; - - let email_info = emails::table - .filter(emails::id.eq(token_info.email_id)) - .first::(&*conn) - .map_err(|_| bad_request("Email belonging to token not found."))?; - - update(emails::table.filter(emails::id.eq(email_info.id))) + let updated_rows = update(emails::table.filter(emails::token.eq(req_token))) .set(emails::verified.eq(true)) - .execute(&*conn) - .map_err(|_| bad_request("Email verification could not be updated"))?; + .execute(&*conn)?; - delete(tokens::table.filter(tokens::id.eq(token_info.id))) - .execute(&*conn) - .map_err(|_| bad_request("Email token could not be deleted"))?; + if updated_rows == 0 { + return Err(bad_request("Email belonging to token not found.")); + } #[derive(Serialize)] struct R { @@ -688,6 +613,9 @@ pub fn confirm_user_email(req: &mut Request) -> CargoResult { /// Handles `PUT /user/:user_id/resend` route pub fn regenerate_token_and_send(req: &mut Request) -> CargoResult { + use diesel::update; + use diesel::expression::sql; + let user = req.user()?; let name = &req.params()["user_id"].parse::().ok().unwrap(); let conn = req.db_conn()?; @@ -698,14 +626,12 @@ pub fn regenerate_token_and_send(req: &mut Request) -> CargoResult { } conn.transaction(|| { - let email_info = emails::table - .filter(emails::user_id.eq(user.id)) - .first::(&*conn) + let email = update(Email::belonging_to(user)) + .set(emails::token.eq(sql("DEFAULT"))) + .get_result::(&*conn) .map_err(|_| bad_request("Email could not be found"))?; - let token = NewToken::regenerate(email_info.id, &conn)?; - - send_user_confirm_email(&email_info.email, &user.gh_login, &token) + send_user_confirm_email(&email.email, &user.gh_login, &email.token) .map_err(|_| bad_request("Error in sending email")) })?; @@ -715,8 +641,3 @@ pub fn regenerate_token_and_send(req: &mut Request) -> CargoResult { } Ok(req.json(&R { ok: true })) } - -fn generate_token() -> String { - let token: String = thread_rng().gen_ascii_chars().take(26).collect(); - token -}