From 500051abd493325500dfdda5e83f0d012a408905 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Thu, 27 Dec 2018 07:29:15 -0700 Subject: [PATCH 1/4] Add the ability to flag a user as deleted in GH When a user deletes their account in GitHub we don't receive any notification that this has happened. If another user creates an account with the same name, we will link to this new user on any crates owned by the old user, which can make it appear that the new user is an owner and lead to social engineering situations we don't want to permit. The front-end is pretty coupled to the (false) concept that a user always has a GH login, and that we can display it. Ideally I'd like to move what we currently call "login" to the "id" field, but we have at least 2 places today where we use the actual user id in the URL, so we'll have to change those before we can make that happen. For now I've just introduced a `display_name` field (which is confusingly named since it's usually the same as `login`, not `name`, but I couldn't come up with a better name). When a user deletes their account (or renames it, we have no way of knowing which one happened), we will no longer report their old github name, and will instead report `deleted_123` where 123 is *our* user ID for them. This is then used in the users route to look them up, instead of their old GH login, which may be reused by another user. Since we don't necessarily want to allow crawling of user profiles, this can only be used to access deleted accounts. Attempting to visit /users/deleted_123 for a user ID that isn't deleted will 404. Additionally, since we can't generally tell if a user deleted their account or renamed it, logging back into crates.io (which will update gh_login with the new value) automatically marks them as undeleted. This isn't something we can reasonably automate detection of, but when we do find out that it's happened, we can now manually flag a user. We *can* detect this when a new user is created with the same gh_login as an existing user, which will be handled in a followup PR. Fixes #1585 --- app/components/user-avatar.js | 2 +- app/components/user-link.js | 2 +- app/models/user.js | 1 + app/templates/me/index.hbs | 2 +- app/templates/user.hbs | 2 +- .../down.sql | 1 + .../up.sql | 1 + src/controllers/user/other.rs | 19 ++++++-- src/models/user.rs | 48 ++++++++++++++++--- src/schema.rs | 6 +++ src/tests/user.rs | 22 +++++++++ src/views/mod.rs | 14 ++++++ 12 files changed, 105 insertions(+), 15 deletions(-) create mode 100644 migrations/2018-12-27-132130_add_deleted_to_users/down.sql create mode 100644 migrations/2018-12-27-132130_add_deleted_to_users/up.sql diff --git a/app/components/user-avatar.js b/app/components/user-avatar.js index ee6b8249b56..49cf8ae819c 100644 --- a/app/components/user-avatar.js +++ b/app/components/user-avatar.js @@ -21,7 +21,7 @@ export default Component.extend({ height: readOnly('width'), alt: computed('user', function() { - return `${this.get('user.name')} (${this.get('user.login')})`; + return `${this.get('user.name')} (${this.get('user.display_name')})`; }), src: computed('size', 'user', function() { diff --git a/app/components/user-link.js b/app/components/user-link.js index 814fbb7f6f7..04dec067d44 100644 --- a/app/components/user-link.js +++ b/app/components/user-link.js @@ -6,7 +6,7 @@ export default Component.extend({ attributeBindings: ['title', 'href'], tagName: 'a', - title: readOnly('user.login'), + title: readOnly('user.display_name'), // TODO replace this with a link to a native crates.io profile // page when they exist. diff --git a/app/models/user.js b/app/models/user.js index 9bab18b285d..cd76c8aa28e 100644 --- a/app/models/user.js +++ b/app/models/user.js @@ -5,6 +5,7 @@ export default DS.Model.extend({ email_verified: DS.attr('boolean'), email_verification_sent: DS.attr('boolean'), name: DS.attr('string'), + display_name: DS.attr('string'), login: DS.attr('string'), avatar: DS.attr('string'), url: DS.attr('string'), diff --git a/app/templates/me/index.hbs b/app/templates/me/index.hbs index ffe434749d4..d07e397e4de 100644 --- a/app/templates/me/index.hbs +++ b/app/templates/me/index.hbs @@ -15,7 +15,7 @@
Name
{{ model.user.name }}
GitHub Account
-
{{ model.user.login }}
+
{{ model.user.display_name }}
diff --git a/app/templates/user.hbs b/app/templates/user.hbs index 69ce4ce34da..88f1b6b5e31 100644 --- a/app/templates/user.hbs +++ b/app/templates/user.hbs @@ -1,7 +1,7 @@
{{user-avatar user=model.user size='medium' data-test-avatar=true}}

- {{ model.user.login }} + {{ model.user.display_name }}

{{#user-link user=model.user data-test-user-link=true}} GitHub profile diff --git a/migrations/2018-12-27-132130_add_deleted_to_users/down.sql b/migrations/2018-12-27-132130_add_deleted_to_users/down.sql new file mode 100644 index 00000000000..cde774d1160 --- /dev/null +++ b/migrations/2018-12-27-132130_add_deleted_to_users/down.sql @@ -0,0 +1 @@ +ALTER TABLE users DROP COLUMN gh_deleted; diff --git a/migrations/2018-12-27-132130_add_deleted_to_users/up.sql b/migrations/2018-12-27-132130_add_deleted_to_users/up.sql new file mode 100644 index 00000000000..747b0468235 --- /dev/null +++ b/migrations/2018-12-27-132130_add_deleted_to_users/up.sql @@ -0,0 +1 @@ +ALTER TABLE users ADD COLUMN gh_deleted BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index cbdf4180010..247744f369b 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -5,15 +5,24 @@ use crate::schema::{crate_owners, crates, users}; use crate::views::EncodablePublicUser; /// Handles the `GET /users/:user_id` route. +#[allow(warnings)] pub fn show(req: &mut dyn Request) -> CargoResult { - use self::users::dsl::{gh_login, id, users}; + use self::users::dsl::{gh_deleted, gh_login, id, users}; let name = &req.params()["user_id"].to_lowercase(); let conn = req.db_conn()?; - let user = users - .filter(crate::lower(gh_login).eq(name)) - .order(id.desc()) - .first::(&*conn)?; + let user = if name.starts_with("deleted_") { + let deleted_id = name["deleted_".len()..].parse::()?; + users + .find(deleted_id) + .filter(gh_deleted) + .first::(&conn)? + } else { + users + .filter(crate::lower(gh_login).eq(name)) + .order(id.desc()) + .first::(&*conn)? + }; #[derive(Serialize)] struct R { diff --git a/src/models/user.rs b/src/models/user.rs index 83ee9b96d5d..fd965c8ff52 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -19,6 +19,7 @@ pub struct User { pub name: Option, pub gh_avatar: Option, pub gh_id: i32, + pub gh_deleted: bool, } #[derive(Insertable, Debug)] @@ -78,6 +79,7 @@ impl<'a> NewUser<'a> { name.eq(excluded(name)), gh_avatar.eq(excluded(gh_avatar)), gh_access_token.eq(excluded(gh_access_token)), + gh_deleted.eq(false), )) .get_result::(conn)?; @@ -179,43 +181,77 @@ impl User { email_verified: bool, email_verification_sent: bool, ) -> EncodablePrivateUser { + let url = self.url().into_owned(); + let display_name = self.display_name().into_owned(); + let login = self.login().into_owned(); let User { id, email, name, - gh_login, gh_avatar, .. } = self; - let url = format!("https://github.com/{}", gh_login); EncodablePrivateUser { id, email, email_verified, email_verification_sent, avatar: gh_avatar, - login: gh_login, + login, name, url: Some(url), + display_name: Some(display_name), } } /// Converts this`User` model into an `EncodablePublicUser` for JSON serialization. pub fn encodable_public(self) -> EncodablePublicUser { + let url = self.url().into_owned(); + let display_name = self.display_name().into_owned(); + let login = self.login().into_owned(); let User { id, name, - gh_login, gh_avatar, .. } = self; - let url = format!("https://github.com/{}", gh_login); EncodablePublicUser { id, avatar: gh_avatar, - login: gh_login, + login, name, url: Some(url), + display_name: Some(display_name), + } + } + + /// Returns a link to this user's GitHub profile, or @ghost if the user + /// has been deleted + fn url(&self) -> Cow { + if self.gh_deleted { + Cow::Borrowed("https://github.com/ghost") + } else { + Cow::Owned(format!("https://github.com/{}", self.gh_login)) + } + } + + /// Returns the user's name for display purposes. Typically the same as + /// gh_login. + fn display_name(&self) -> Cow { + if self.gh_deleted { + Cow::Owned(format!("{} (deleted)", self.gh_login)) + } else { + Cow::Borrowed(&self.gh_login) + } + } + + /// Returns the user's gh_login if they haven't been deleted, or a special + /// identifier if they have. + fn login(&self) -> Cow { + if self.gh_deleted { + Cow::Owned(format!("deleted_{}", self.id)) + } else { + Cow::Borrowed(&self.gh_login) } } } diff --git a/src/schema.rs b/src/schema.rs index e7a7ced1628..10985129f6b 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -747,6 +747,12 @@ table! { /// /// (Automatically generated by Diesel.) gh_id -> Int4, + /// The `gh_deleted` column of the `users` table. + /// + /// Its SQL type is `Bool`. + /// + /// (Automatically generated by Diesel.) + gh_deleted -> Bool, } } diff --git a/src/tests/user.rs b/src/tests/user.rs index d6d0fbb1871..0f9158d2584 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -628,3 +628,25 @@ fn test_existing_user_email() { assert!(!r.user.email_verified); assert!(!r.user.email_verification_sent); } + +#[test] +fn deleted_users_show_this_information() { + use cargo_registry::schema::users; + + let (app, _anon, user) = TestApp::init().with_user(); + + app.db(|conn| { + let user = user.as_model(); + diesel::update(user) + .set(users::gh_deleted.eq(true)) + .execute(conn) + .unwrap(); + }); + let login = format!("deleted_{}", user.as_model().id); + let resp: UserShowPublicResponse = user.get(&format!("/api/v1/users/{}", login)).good(); + let user = resp.user; + + assert_eq!("foo (deleted)", user.display_name.unwrap()); + assert_eq!("https://github.com/ghost", user.url.unwrap()); + assert_eq!(login, user.login); +} diff --git a/src/views/mod.rs b/src/views/mod.rs index bcb9ee4d8a1..897770a5d25 100644 --- a/src/views/mod.rs +++ b/src/views/mod.rs @@ -161,12 +161,25 @@ pub struct EncodableMe { #[derive(Deserialize, Serialize, Debug)] pub struct EncodablePrivateUser { pub id: i32, + + /// The user's login, used to identify the user in URLs. + /// This should not be used for display purposes pub login: String, + + /// The user's display name. Typically the same as login, + /// but changes if the user's GitHub account has been deleted. + /// Use this instead of login for display + pub display_name: Option, + pub email: Option, pub email_verified: bool, pub email_verification_sent: bool, + + /// The user's full name as given to GitHub pub name: Option, pub avatar: Option, + + /// The URL to the user's external profile pub url: Option, } @@ -176,6 +189,7 @@ pub struct EncodablePrivateUser { pub struct EncodablePublicUser { pub id: i32, pub login: String, + pub display_name: Option, pub name: Option, pub avatar: Option, pub url: Option, From 5154183d17b7e664d9b09d2c68c37a070d28f4f1 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Thu, 27 Dec 2018 08:04:15 -0700 Subject: [PATCH 2/4] Add `display_name` to JS fixtures --- mirage/fixtures/users.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mirage/fixtures/users.js b/mirage/fixtures/users.js index 83b1b4365dd..5f14fb96732 100644 --- a/mirage/fixtures/users.js +++ b/mirage/fixtures/users.js @@ -5,6 +5,7 @@ export default [ email: null, id: 303, login: 'blabaere', + display_name: 'blabaere', name: 'Benoît Labaere', url: 'https://github.com/blabaere', }, @@ -13,6 +14,7 @@ export default [ email: 'dnfagnan@gmail.com', id: 2, login: 'thehydroimpulse', + display_name: 'thehydroimpulse', name: 'Daniel Fagnan', url: 'https://github.com/thehydroimpulse', }, @@ -21,6 +23,7 @@ export default [ email: 'iain@fastmail.com', id: 10982, login: 'iain8', + display_name: 'iain8', name: 'Iain Buchanan', url: 'https://github.com/iain8', }, From 5752c51a08712d30101708eb05622eb180ee95bc Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Thu, 27 Dec 2018 08:11:43 -0700 Subject: [PATCH 3/4] Don't allow warnings because we're not adding random `panic!`s anymore --- src/controllers/user/other.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index 247744f369b..9ab891e6194 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -5,7 +5,6 @@ use crate::schema::{crate_owners, crates, users}; use crate::views::EncodablePublicUser; /// Handles the `GET /users/:user_id` route. -#[allow(warnings)] pub fn show(req: &mut dyn Request) -> CargoResult { use self::users::dsl::{gh_deleted, gh_login, id, users}; From 025596cf067f7fde7db4d6a62310d561cbbd37a8 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Thu, 27 Dec 2018 13:50:41 -0700 Subject: [PATCH 4/4] Ensure users whose login starts with `deleted_` can still be looked up --- src/controllers/user/other.rs | 21 ++++++++++----------- src/tests/user.rs | 13 +++++++++++++ 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index 9ab891e6194..3c2e9a3b53c 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -10,19 +10,18 @@ pub fn show(req: &mut dyn Request) -> CargoResult { let name = &req.params()["user_id"].to_lowercase(); let conn = req.db_conn()?; - let user = if name.starts_with("deleted_") { - let deleted_id = name["deleted_".len()..].parse::()?; - users - .find(deleted_id) - .filter(gh_deleted) - .first::(&conn)? - } else { - users - .filter(crate::lower(gh_login).eq(name)) - .order(id.desc()) - .first::(&*conn)? + let mut query = users + .filter(crate::lower(gh_login).eq(name)) + .order(id.desc()) + .into_boxed(); + if name.starts_with("deleted_") { + if let Ok(deleted_id) = name["deleted_".len()..].parse::() { + query = query.or_filter(id.eq(deleted_id).and(gh_deleted)); + } }; + let user = query.first::(&conn)?; + #[derive(Serialize)] struct R { user: EncodablePublicUser, diff --git a/src/tests/user.rs b/src/tests/user.rs index 0f9158d2584..8cada527236 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -650,3 +650,16 @@ fn deleted_users_show_this_information() { assert_eq!("https://github.com/ghost", user.url.unwrap()); assert_eq!(login, user.login); } + +#[test] +fn users_with_names_starting_with_deleted_can_be_looked_up() { + let (app, _) = TestApp::init().empty(); + let user = app.db_new_user("deleted_user"); + + let resp: UserShowPublicResponse = user.get("/api/v1/users/deleted_user").good(); + let user = resp.user; + + assert_eq!("deleted_user", user.display_name.unwrap()); + assert_eq!("https://github.com/deleted_user", user.url.unwrap()); + assert_eq!("deleted_user", user.login); +}