Use the database to generate API tokens

sgrif · sgrif · commit 9ed51dd2c4d2 · 2017-03-09T08:05:16.000-05:00
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.
diff --git a/migrations/20170309122510_generate_api_token_with_sql/down.sql 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,16 +52,14 @@ 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,
             email: email,
             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<User> {
+                          access_token: &str) -> CargoResult<User> {
         // 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<Response> {
     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<Response> {
                                         .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<Response> {
 pub fn reset_token(req: &mut Request) -> CargoResult<Response> {
     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<Response> {
     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::<User>(&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);
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+ALTER TABLE users ALTER COLUMN api_token DROP DEFAULT;`
	`2`	`+DROP FUNCTION random_string(int4);`
Original file line number	Diff line number	Diff line change
`@@ -148,7 +148,7 @@ mod test {`
`148`	`148`
`149`	`149`	`fn user(conn: &postgres::transaction::Transaction) -> User{`
`150`	`150`	`User::find_or_insert(conn, 2, "login", None, None, None,`
`151`		`- "access_token", "api_token").unwrap()`
	`151`	`+ "access_token").unwrap()`
`152`	`152`	`}`
`153`	`153`
`154`	`154`	`fn crate_downloads(tx: &postgres::transaction::Transaction, id: i32, expected: usize) {`