From 9ed51dd2c4d2e0db1b74c50981e20d8fe846ee59 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Thu, 9 Mar 2017 08:05:16 -0500 Subject: [PATCH] Use the database to generate API tokens There's a ton of code which is setting this field in misleading ways, and it seems really odd to pass it to `find_or_insert` when it's completely ignored in the update case. We can just use a database default for this, and simplify everything a bit. --- .../down.sql | 2 + .../up.sql | 11 +++ src/bin/update-downloads.rs | 2 +- src/tests/all.rs | 13 ++-- src/tests/user.rs | 31 ++++---- src/user/mod.rs | 78 ++++++++++++++----- 6 files changed, 92 insertions(+), 45 deletions(-) create mode 100644 migrations/20170309122510_generate_api_token_with_sql/down.sql create mode 100644 migrations/20170309122510_generate_api_token_with_sql/up.sql diff --git a/migrations/20170309122510_generate_api_token_with_sql/down.sql b/migrations/20170309122510_generate_api_token_with_sql/down.sql new file mode 100644 index 00000000000..929f38a2087 --- /dev/null +++ b/migrations/20170309122510_generate_api_token_with_sql/down.sql @@ -0,0 +1,2 @@ +ALTER TABLE users ALTER COLUMN api_token DROP DEFAULT; +DROP FUNCTION random_string(int4); diff --git a/migrations/20170309122510_generate_api_token_with_sql/up.sql b/migrations/20170309122510_generate_api_token_with_sql/up.sql new file mode 100644 index 00000000000..73c2ed989cd --- /dev/null +++ b/migrations/20170309122510_generate_api_token_with_sql/up.sql @@ -0,0 +1,11 @@ +CREATE FUNCTION random_string(int4) RETURNS text AS $$ + SELECT (array_to_string(array( + SELECT substr( + 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789', + floor(random() * 62)::int4 + 1, + 1 + ) FROM generate_series(1, $1) + ), '')) +$$ LANGUAGE SQL; + +ALTER TABLE users ALTER COLUMN api_token SET DEFAULT random_string(32); diff --git a/src/bin/update-downloads.rs b/src/bin/update-downloads.rs index e8016345504..23b5122b4d6 100644 --- a/src/bin/update-downloads.rs +++ b/src/bin/update-downloads.rs @@ -148,7 +148,7 @@ mod test { fn user(conn: &postgres::transaction::Transaction) -> User{ User::find_or_insert(conn, 2, "login", None, None, None, - "access_token", "api_token").unwrap() + "access_token").unwrap() } fn crate_downloads(tx: &postgres::transaction::Transaction, id: i32, expected: usize) { diff --git a/src/tests/all.rs b/src/tests/all.rs index 4b6cd7dda34..61e618c8262 100755 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -180,8 +180,8 @@ fn user(login: &str) -> User { email: None, name: None, avatar: None, - gh_access_token: User::new_api_token(), // just randomize it - api_token: User::new_api_token(), + gh_access_token: "some random token".into(), + api_token: "some random token".into(), } } @@ -209,12 +209,15 @@ fn mock_user(req: &mut Request, u: User) -> User { u.email.as_ref().map(|s| &s[..]), u.name.as_ref().map(|s| &s[..]), u.avatar.as_ref().map(|s| &s[..]), - &u.gh_access_token, - &u.api_token).unwrap(); - req.mut_extensions().insert(u.clone()); + &u.gh_access_token).unwrap(); + sign_in_as(req, &u); return u; } +fn sign_in_as(req: &mut Request, user: &User) { + req.mut_extensions().insert(user.clone()); +} + fn mock_crate(req: &mut Request, krate: Crate) -> (Crate, Version) { mock_crate_vers(req, krate, &semver::Version::parse("1.0.0").unwrap()) } diff --git a/src/tests/user.rs b/src/tests/user.rs index 76a54b66ad8..f329e29e063 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -1,4 +1,4 @@ -use conduit::{Handler, Request, Method}; +use conduit::{Handler, Method}; use cargo_registry::Model; use cargo_registry::krate::EncodableCrate; @@ -37,20 +37,20 @@ fn user_insert() { let conn = t!(app.database.get()); let tx = t!(conn.transaction()); - let user = t!(User::find_or_insert(&tx, 1, "foo", None, None, None, "bar", "baz")); - assert_eq!(t!(User::find_by_api_token(&tx, "baz")), user); + let user = t!(User::find_or_insert(&tx, 1, "foo", None, None, None, "bar")); + assert_eq!(t!(User::find_by_api_token(&tx, &user.api_token)), user); assert_eq!(t!(User::find(&tx, user.id)), user); assert_eq!(t!(User::find_or_insert(&tx, 1, "foo", None, None, None, - "bar", "api")), user); + "bar")), user); let user2 = t!(User::find_or_insert(&tx, 1, "foo", None, None, None, - "baz", "api")); + "baz")); assert!(user != user2); assert_eq!(user.id, user2.id); assert_eq!(user2.gh_access_token, "baz"); let user3 = t!(User::find_or_insert(&tx, 1, "bar", None, None, None, - "baz", "api")); + "baz")); assert!(user != user3); assert_eq!(user.id, user3.id); assert_eq!(user3.gh_login, "bar"); @@ -63,8 +63,7 @@ fn me() { let response = t_resp!(middle.call(&mut req)); assert_eq!(response.status.0, 403); - let user = ::user("foo"); - ::mock_user(&mut req, user.clone()); + let user = ::mock_user(&mut req, ::user("foo")); let mut response = ok_resp!(middle.call(&mut req)); let json: MeResponse = ::json(&mut response); assert_eq!(json.user.email, user.email); @@ -77,8 +76,8 @@ fn show() { { let conn = t!(app.diesel_database.get()); - t!(NewUser::new(1, "foo", Some("foo@bar.com"), None, None, "bar", "baz").create_or_update(&conn)); - t!(NewUser::new(2, "bar", Some("bar@baz.com"), None, None, "bar", "baz").create_or_update(&conn)); + t!(NewUser::new(1, "foo", Some("foo@bar.com"), None, None, "bar").create_or_update(&conn)); + t!(NewUser::new(2, "bar", Some("bar@baz.com"), None, None, "bar").create_or_update(&conn)); } let mut req = ::req(app.clone(), Method::Get, "/api/v1/users/foo"); @@ -97,16 +96,12 @@ fn show() { fn reset_token() { let (_b, app, middle) = ::app(); let mut req = ::req(app, Method::Put, "/me/reset_token"); - let user = { - let tx = RequestTransaction::tx(&mut req as &mut Request); - User::find_or_insert(tx.unwrap(), 1, "foo", None, - None, None, "bar", "baz").unwrap() - }; - ::mock_user(&mut req, user.clone()); + let user = User::find_or_insert(req.tx().unwrap(), 1, "foo", None, + None, None, "bar").unwrap(); + ::sign_in_as(&mut req, &user); ok_resp!(middle.call(&mut req)); - let tx = RequestTransaction::tx(&mut req as &mut Request); - let u2 = User::find(tx.unwrap(), user.id).unwrap(); + let u2 = User::find(req.tx().unwrap(), user.id).unwrap(); assert!(u2.api_token != user.api_token); } diff --git a/src/user/mod.rs b/src/user/mod.rs index 8980e658fbd..3fc9cfc19df 100644 --- a/src/user/mod.rs +++ b/src/user/mod.rs @@ -44,7 +44,6 @@ pub struct NewUser<'a> { pub name: Option<&'a str>, pub gh_avatar: Option<&'a str>, pub gh_access_token: &'a str, - pub api_token: &'a str, } impl<'a> NewUser<'a> { @@ -53,8 +52,7 @@ impl<'a> NewUser<'a> { email: Option<&'a str>, name: Option<&'a str>, gh_avatar: Option<&'a str>, - gh_access_token: &'a str, - api_token: &'a str) -> Self { + gh_access_token: &'a str) -> Self { NewUser { gh_id: gh_id, gh_login: gh_login, @@ -62,7 +60,6 @@ impl<'a> NewUser<'a> { name: name, gh_avatar: gh_avatar, gh_access_token: gh_access_token, - api_token: api_token, } } @@ -134,8 +131,7 @@ impl User { email: Option<&str>, name: Option<&str>, avatar: Option<&str>, - access_token: &str, - api_token: &str) -> CargoResult { + access_token: &str) -> CargoResult { // TODO: this is racy, but it looks like any other solution is... // interesting! For now just do the racy thing which will report // more errors than it needs to. @@ -159,13 +155,12 @@ impl User { None => {} } let stmt = conn.prepare("INSERT INTO users - (email, gh_access_token, api_token, + (email, gh_access_token, gh_login, name, gh_avatar, gh_id) - VALUES ($1, $2, $3, $4, $5, $6, $7) + VALUES ($1, $2, $3, $4, $5, $6) RETURNING *")?; let rows = stmt.query(&[&email, &access_token, - &api_token, &login, &name, &avatar, @@ -175,11 +170,6 @@ impl User { })?)) } - /// Generates a new crates.io API token. - pub fn new_api_token() -> String { - thread_rng().gen_ascii_chars().take(32).collect() - } - /// Converts this `User` model into an `EncodableUser` for JSON serialization. pub fn encodable(self) -> EncodableUser { let User { id, email, api_token: _, gh_access_token: _, @@ -300,8 +290,6 @@ pub fn github_access_token(req: &mut Request) -> CargoResult { let (handle, resp) = http::github(req.app(), "/user", &token)?; let ghuser: GithubUser = http::parse_github_response(handle, resp)?; - // Into the database! - let api_token = User::new_api_token(); let user = User::find_or_insert(req.tx()?, ghuser.id, &ghuser.login, @@ -311,8 +299,7 @@ pub fn github_access_token(req: &mut Request) -> CargoResult { .map(|s| &s[..]), ghuser.avatar_url.as_ref() .map(|s| &s[..]), - &token.access_token, - &api_token)?; + &token.access_token)?; req.session().insert("user_id".to_string(), user.id.to_string()); req.mut_extensions().insert(user); me(req) @@ -328,10 +315,12 @@ pub fn logout(req: &mut Request) -> CargoResult { pub fn reset_token(req: &mut Request) -> CargoResult { let user = req.user()?; - let token = User::new_api_token(); let conn = req.tx()?; - conn.execute("UPDATE users SET api_token = $1 WHERE id = $2", - &[&token, &user.id])?; + let rows = conn.query("UPDATE users SET api_token = DEFAULT \ + WHERE id = $1 RETURNING api_token", &[&user.id])?; + let token = rows.iter().next() + .map(|r| r.get("api_token")) + .chain_error(|| NotFound)?; #[derive(RustcEncodable)] struct R { api_token: String } @@ -424,3 +413,50 @@ pub fn updates(req: &mut Request) -> CargoResult { struct Meta { more: bool } Ok(req.json(&R{ versions: versions, crates: crates, meta: Meta { more: more } })) } + +#[cfg(test)] +mod tests { + use super::*; + use diesel::pg::PgConnection; + use dotenv::dotenv; + use std::env; + + fn connection() -> PgConnection { + let _ = dotenv(); + let database_url = env::var("TEST_DATABASE_URL") + .expect("TEST_DATABASE_URL must be set to run tests"); + let conn = PgConnection::establish(&database_url).unwrap(); + conn.begin_test_transaction().unwrap(); + conn + } + + #[test] + fn new_users_have_different_api_tokens() { + let conn = connection(); + let user1 = NewUser::new(1, "foo", None, None, None, "foo") + .create_or_update(&conn).unwrap(); + let user2 = NewUser::new(2, "bar", None, None, None, "bar") + .create_or_update(&conn).unwrap(); + + assert_ne!(user1.id, user2.id); + assert_ne!(user1.api_token, user2.api_token); + assert_eq!(32, user1.api_token.len()); + } + + #[test] + fn updating_existing_user_doesnt_change_api_token() { + let conn = connection(); + let user_after_insert = NewUser::new(1, "foo", None, None, None, "foo") + .create_or_update(&conn).unwrap(); + let original_token = user_after_insert.api_token; + NewUser::new(1, "bar", None, None, None, "bar_token") + .create_or_update(&conn).unwrap(); + let mut users = users::table.load::(&conn).unwrap(); + assert_eq!(1, users.len()); + let user = users.pop().unwrap(); + + assert_eq!("bar", user.gh_login); + assert_eq!("bar_token", user.gh_access_token); + assert_eq!(original_token, user.api_token); + } +}