Merge #1109

1109: Gh login cant be unique r=sgrif

So I can't deploy to production right now because of the unique index added to users.gh_login.

The values on production for gh_login aren't unique, and here's why:

- Someone can sign up for github with username whatever.
- They can sign in to crates.io. We now get their gh_id, say, 3 (which *is* unique) and their gh_login, whatever. Their user account gets crates.io id 10.
- They rename or delete their github account, which leaves the username 'whatever' available again. (If they renamed their github account, their gh_id stays stable and we'd migrate their account to their new username next time they log in)
- Someone else signs up for github with username wHaTeVeR.
- They can sign in to crates.io. We now get their gh_id, 9000, and their gh_login, wHaTeVeR. Their user account gets crates.io id 10000.

We can't get rid of the first whatever account, because they might sign in again with a renamed account but the same id, and they should be able to still access the crates they own and all that. 

All we know is that the account with the latest crates.io id for that lower(gh_login) is the last valid account we know of for that username (and should match the account that you click through to github, as long as they haven't renamed or deleted their github account since).

r? @sgrif

Author: bors-voyager[bot]

migrations/2017-09-26-200549_users_lowercase_index/down.sql

@@ -1,3 +1 @@
-DROP INDEX lower_gh_login;
-ALTER TABLE users DROP CONSTRAINT unique_gh_login;
-CREATE INDEX index_users_gh_login ON users (gh_login);
+-- This migration intentionally left blank; see corresponding up.sql

migrations/2017-09-26-200549_users_lowercase_index/up.sql

@@ -1,3 +1,9 @@
-DROP INDEX IF EXISTS index_users_gh_login;
-ALTER TABLE users ADD CONSTRAINT unique_gh_login UNIQUE(gh_login);
-CREATE UNIQUE INDEX lower_gh_login ON users (lower(gh_login));
+-- This migration was merged into the master branch but could not be deployed to production
+-- because production gh_login isn't actually unique. Later, this migration was commented out
+-- so that it will be a no-op on production, and a new migration was added to correct the database
+-- of anyone who ran this migration locally.
+--
+-- DROP INDEX IF EXISTS index_users_gh_login;
+-- ALTER TABLE users ADD CONSTRAINT unique_gh_login UNIQUE(gh_login);
+-- CREATE UNIQUE INDEX lower_gh_login ON users (lower(gh_login));
+SELECT 1;

migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/down.sql

@@ -0,0 +1 @@
+-- This file intentionally left blank; see the corresponding up.sql

migrations/2017-10-06-234455_fix_local_dbs_unique_gh_login/up.sql

@@ -0,0 +1,7 @@
+-- This migration should have no effect on production.
+-- Its sole purpose is to fix the local database of anyone who ran 2017-09-26-200549 locally--
+-- that migration can't be run on production because lower(gh_login) isn't unique on production.
+
+CREATE INDEX IF NOT EXISTS index_users_gh_login ON users (gh_login);
+ALTER TABLE users DROP CONSTRAINT IF EXISTS unique_gh_login;
+DROP INDEX IF EXISTS lower_gh_login;

migrations/2017-10-09-135625_add_lower_gh_login_index/down.sql

@@ -0,0 +1,2 @@
+DROP INDEX IF EXISTS lower_gh_login;
+CREATE INDEX index_users_gh_login ON users (gh_login);

migrations/2017-10-09-135625_add_lower_gh_login_index/up.sql

@@ -0,0 +1,2 @@
+DROP INDEX IF EXISTS index_users_gh_login;
+CREATE INDEX lower_gh_login ON users (lower(gh_login));

src/tests/user.rs

@@ -81,20 +81,46 @@ fn show() {
 }
 
 #[test]
-fn show_case_insensitive() {
+fn show_latest_user_case_insensitively() {
     let (_b, app, middle) = ::app();
     {
         let conn = t!(app.diesel_database.get());
 
+        // Please do not delete or modify the setup of this test in order to get it to pass.
+        // This setup mimics how GitHub works. If someone abandons a GitHub account, the username is
+        // available for anyone to take. We need to support having multiple user accounts
+        // with the same gh_login in crates.io. `gh_id` is stable across renames, so that field
+        // should be used for uniquely identifying GitHub accounts whenever possible. For the
+        // crates.io/user/:username pages, the best we can do is show the last crates.io account
+        // created with that username.
         t!(
-            NewUser::new(1, "foobar", Some("[email protected]"), None, None, "bar")
-                .create_or_update(&conn)
+            NewUser::new(
+                1,
+                "foobar",
+                Some("[email protected]"),
+                Some("I was first then deleted my github account"),
+                None,
+                "bar"
+            ).create_or_update(&conn)
+        );
+        t!(
+            NewUser::new(
+                2,
+                "FOOBAR",
+                Some("[email protected]"),
+                Some("I was second, I took the foobar username on github"),
+                None,
+                "bar"
+            ).create_or_update(&conn)
         );
     }
     let mut req = ::req(app.clone(), Method::Get, "api/v1/users/fOObAr");
     let mut response = ok_resp!(middle.call(&mut req));
     let json: UserShowPublicResponse = ::json(&mut response);
-    assert_eq!("foobar", json.user.login);
+    assert_eq!(
+        "I was second, I took the foobar username on github",
+        json.user.name.unwrap()
+    );
 }
 
 #[test]

src/user/mod.rs

@@ -456,12 +456,13 @@ pub fn me(req: &mut Request) -> CargoResult<Response> {
 
 /// Handles the `GET /users/:user_id` route.
 pub fn show(req: &mut Request) -> CargoResult<Response> {
-    use self::users::dsl::{gh_login, users};
+    use self::users::dsl::{gh_login, id, users};
 
     let name = &req.params()["user_id"].to_lowercase();
     let conn = req.db_conn()?;
     let user = users
         .filter(::lower(gh_login).eq(name))
+        .order(id.desc())
         .first::<User>(&*conn)?;
 
     #[derive(Serialize)]