From 535665ad39809e3176b3139bad83e456a5141393 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 27 Jan 2020 13:07:17 -0500 Subject: [PATCH 1/3] Set the TrustedUserId in the extensions when logging in Fixes #2166. In #2077 (d0b9bccc4cf80d3e311696a7a9e62ee469cce892), we changed the type of the authentication data stored in the extensions from `User` to `TrustedUserId`, but we missed this spot :( We're allowed to insert anything we want into the extensions, but if we use a different type than what we use when looking up data in the extensions, it won't find what we inserted. --- src/controllers/user/session.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 9527f5c4e7c..2808aa01899 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -5,6 +5,7 @@ use conduit_cookie::RequestSession; use failure::Fail; use oauth2::{prelude::*, AuthorizationCode, TokenResponse}; +use crate::middleware::current_user::TrustedUserId; use crate::models::{NewUser, User}; use crate::schema::users; use crate::util::errors::ReadOnlyMode; @@ -102,7 +103,8 @@ pub fn authorize(req: &mut dyn Request) -> AppResult { let user = ghuser.save_to_database(&token.secret(), &*req.db_conn()?)?; req.session() .insert("user_id".to_string(), user.id.to_string()); - req.mut_extensions().insert(user); + req.mut_extensions().insert(TrustedUserId(user.id)); + super::me::me(req) } From 30f8dff9a52187be9ad9cba4377a77eff10f3bea Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 27 Jan 2020 13:13:26 -0500 Subject: [PATCH 2/3] Add some more comments --- src/controllers/user/session.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 2808aa01899..2adc97ffb99 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -89,8 +89,7 @@ pub fn authorize(req: &mut dyn Request) -> AppResult { } } - // Fetch the access token from github using the code we just got - + // Fetch the access token from GitHub using the code we just got let code = AuthorizationCode::new(code); let token = req .app() @@ -99,8 +98,12 @@ pub fn authorize(req: &mut dyn Request) -> AppResult { .map_err(|e| e.compat()) .chain_error(|| server_error("Error obtaining token"))?; let token = token.access_token(); + + // Fetch the user info from GitHub using the access token we just got and create a user record let ghuser = github::github_api::(req.app(), "/user", token)?; let user = ghuser.save_to_database(&token.secret(), &*req.db_conn()?)?; + + // Log in by setting a cookie and the middleware authentication req.session() .insert("user_id".to_string(), user.id.to_string()); req.mut_extensions().insert(TrustedUserId(user.id)); From 949d294977e6d7b1323e9ff3efc21b8c8b4448a9 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 27 Jan 2020 13:24:59 -0500 Subject: [PATCH 3/3] Remove a comment that's no longer applicable Because we're only storing the user ID in the extensions rather than the whole `User`, we're always looking up the user in the database, so this comment saying the user lookup isn't preferable is out of date. --- src/controllers/user/me.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 6808fdd754c..dfd1fc1c6b6 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -13,17 +13,6 @@ use crate::views::{EncodableMe, EncodableVersion, OwnedCrate}; /// Handles the `GET /me` route. pub fn me(req: &mut dyn Request) -> AppResult { - // Changed to getting User information from database because in - // src/tests/user.rs, when testing put and get on updating email, - // request seems to be somehow 'cached'. When we try to get a - // request from the /me route with the just updated user (call - // this function) the user is the same as the initial GET request - // and does not seem to get the updated user information from the - // database - // This change is not preferable, we'd rather fix the request, - // perhaps adding `req.mut_extensions().insert(user)` to the - // update_user route, however this somehow does not seem to work - let conn = req.db_conn()?; let user_id = req.authenticate(&conn)?.user_id();