Auto merge of #2077 - jtgeibel:defer-user-auth, r=carols10cents

Defer user authentication until requested by endpoint

Many endpoints do not require user authentication, and by moving the
logic out of the middleware layer these endpoints now avoid obtaining a
database connection from the pool and 1 or 2 queries.

Due to lifetime issues a small middlware component remains.  Further
details are described in the middleware's documentation.

r? @carols10cents

Author: bors

src/controllers.rs

@@ -19,7 +19,6 @@ mod prelude {
     pub use crate::util::errors::{cargo_err, AppError, AppResult, ChainError}; // TODO: Remove cargo_err from here
 
     pub use crate::middleware::app::RequestApp;
-    pub use crate::middleware::current_user::RequestUser;
 
     use std::collections::HashMap;
     use std::io;
@@ -28,6 +27,10 @@ mod prelude {
     use serde::Serialize;
     use url;
 
+    pub trait UserAuthenticationExt {
+        fn authenticate(&self, conn: &PgConnection) -> AppResult<super::util::AuthenticatedUser>;
+    }
+
     pub trait RequestUtils {
         fn redirect(&self, url: String) -> Response;
 
@@ -77,6 +80,7 @@ mod prelude {
 }
 
 pub mod helpers;
+mod util;
 
 pub mod category;
 pub mod crate_owner_invitation;

src/controllers/crate_owner_invitation.rs

@@ -7,7 +7,7 @@ use crate::views::{EncodableCrateOwnerInvitation, InvitationResponse};
 /// Handles the `GET /me/crate_owner_invitations` route.
 pub fn list(req: &mut dyn Request) -> AppResult<Response> {
     let conn = &*req.db_conn()?;
-    let user_id = req.user()?.id;
+    let user_id = req.authenticate(conn)?.user_id();
 
     let crate_owner_invitations = crate_owner_invitations::table
         .filter(crate_owner_invitations::invited_user_id.eq(user_id))
@@ -41,7 +41,7 @@ pub fn handle_invite(req: &mut dyn Request) -> AppResult<Response> {
         serde_json::from_str(&body).map_err(|_| bad_request("invalid json request"))?;
 
     let crate_invite = crate_invite.crate_owner_invite;
-    let user_id = req.user()?.id;
+    let user_id = req.authenticate(conn)?.user_id();
 
     if crate_invite.accepted {
         accept_invite(req, conn, crate_invite, user_id)
@@ -116,8 +116,7 @@ fn decline_invite(
 ) -> AppResult<Response> {
     use diesel::delete;
 
-    let user_id = req.user()?.id;
-
+    let user_id = req.authenticate(conn)?.user_id();
     delete(crate_owner_invitations::table.find((user_id, crate_invite.crate_id))).execute(conn)?;
 
     #[derive(Serialize)]

src/controllers/krate/follow.rs

@@ -8,15 +8,12 @@ use crate::models::{Crate, Follow};
 use crate::schema::*;
 
 fn follow_target(req: &dyn Request, conn: &DieselPooledConn<'_>) -> AppResult<Follow> {
-    let user = req.user()?;
+    let user_id = req.authenticate(conn)?.user_id();
     let crate_name = &req.params()["crate_id"];
     let crate_id = Crate::by_name(crate_name)
         .select(crates::id)
         .first(&**conn)?;
-    Ok(Follow {
-        user_id: user.id,
-        crate_id,
-    })
+    Ok(Follow { user_id, crate_id })
 }
 
 /// Handles the `PUT /crates/:crate_id/follow` route.

src/controllers/krate/owners.rs

@@ -92,9 +92,10 @@ fn parse_owners_request(req: &mut dyn Request) -> AppResult<Vec<String>> {
 fn modify_owners(req: &mut dyn Request, add: bool) -> AppResult<Response> {
     let logins = parse_owners_request(req)?;
     let app = req.app();
-    let user = req.user()?;
     let crate_name = &req.params()["crate_id"];
+
     let conn = req.db_conn()?;
+    let user = req.authenticate(&conn)?.find_user(&conn)?;
 
     conn.transaction(|| {
         let krate = Crate::by_name(crate_name).first::<Crate>(&*conn)?;
@@ -121,13 +122,13 @@ fn modify_owners(req: &mut dyn Request, add: bool) -> AppResult<Response> {
                 if owners.iter().any(login_test) {
                     return Err(cargo_err(&format_args!("`{}` is already an owner", login)));
                 }
-                let msg = krate.owner_add(app, &conn, user, login)?;
+                let msg = krate.owner_add(app, &conn, &user, login)?;
                 msgs.push(msg);
             }
             msgs.join(",")
         } else {
             for login in &logins {
-                krate.owner_remove(app, &conn, user, login)?;
+                krate.owner_remove(app, &conn, &user, login)?;
             }
             if User::owning(&krate, &conn)?.is_empty() {
                 return Err(cargo_err(

src/controllers/krate/publish.rs

@@ -8,7 +8,7 @@ use crate::controllers::cargo_prelude::*;
 use crate::git;
 use crate::models::dependency;
 use crate::models::{
-    insert_version_owner_action, Badge, Category, Keyword, NewCrate, NewVersion, Rights, User,
+    insert_version_owner_action, Badge, Category, Keyword, NewCrate, NewVersion, Rights,
     VersionAction,
 };
 
@@ -39,9 +39,11 @@ pub fn publish(req: &mut dyn Request) -> AppResult<Response> {
     // - Then the .crate tarball length is passed to the upload_crate function where the actual
     //   file is read and uploaded.
 
-    let (new_crate, user) = parse_new_headers(req)?;
+    let new_crate = parse_new_headers(req)?;
 
     let conn = app.diesel_database.get()?;
+    let ids = req.authenticate(&conn)?;
+    let user = ids.find_user(&conn)?;
 
     let verified_email_address = user.verified_email(&conn)?;
     let verified_email_address = verified_email_address.ok_or_else(|| {
@@ -150,7 +152,7 @@ pub fn publish(req: &mut dyn Request) -> AppResult<Response> {
             &conn,
             version.id,
             user.id,
-            req.authentication_source()?.api_token_id(),
+            ids.api_token_id(),
             VersionAction::Publish,
         )?;
 
@@ -223,9 +225,8 @@ pub fn publish(req: &mut dyn Request) -> AppResult<Response> {
 /// Used by the `krate::new` function.
 ///
 /// This function parses the JSON headers to interpret the data and validates
-/// the data during and after the parsing. Returns crate metadata and user
-/// information.
-fn parse_new_headers(req: &mut dyn Request) -> AppResult<(EncodableCrateUpload, User)> {
+/// the data during and after the parsing. Returns crate metadata.
+fn parse_new_headers(req: &mut dyn Request) -> AppResult<EncodableCrateUpload> {
     // Read the json upload request
     let metadata_length = u64::from(read_le_u32(req.body())?);
     req.mut_extensions().insert(metadata_length);
@@ -264,6 +265,5 @@ fn parse_new_headers(req: &mut dyn Request) -> AppResult<(EncodableCrateUpload,
         )));
     }
 
-    let user = req.user()?;
-    Ok((new, user.clone()))
+    Ok(new)
 }

src/controllers/krate/search.rs

@@ -154,11 +154,12 @@ pub fn search(req: &mut dyn Request) -> AppResult<Response> {
             ),
         );
     } else if params.get("following").is_some() {
+        let user_id = req.authenticate(&conn)?.user_id();
         query = query.filter(
             crates::id.eq_any(
                 follows::table
                     .select(follows::crate_id)
-                    .filter(follows::user_id.eq(req.user()?.id)),
+                    .filter(follows::user_id.eq(user_id)),
             ),
         );
     }

src/controllers/token.rs

@@ -1,6 +1,5 @@
 use super::frontend_prelude::*;
 
-use crate::middleware::current_user::AuthenticationSource;
 use crate::models::ApiToken;
 use crate::schema::api_tokens;
 use crate::util::read_fill;
@@ -10,10 +9,13 @@ use serde_json as json;
 
 /// Handles the `GET /me/tokens` route.
 pub fn list(req: &mut dyn Request) -> AppResult<Response> {
-    let tokens = ApiToken::belonging_to(req.user()?)
+    let conn = req.db_conn()?;
+    let user = req.authenticate(&conn)?.find_user(&conn)?;
+
+    let tokens = ApiToken::belonging_to(&user)
         .filter(api_tokens::revoked.eq(false))
         .order(api_tokens::created_at.desc())
-        .load(&*req.db_conn()?)?;
+        .load(&*conn)?;
     #[derive(Serialize)]
     struct R {
         api_tokens: Vec<ApiToken>,
@@ -35,12 +37,6 @@ pub fn new(req: &mut dyn Request) -> AppResult<Response> {
         api_token: NewApiToken,
     }
 
-    if req.authentication_source()? != AuthenticationSource::SessionCookie {
-        return Err(bad_request(
-            "cannot use an API token to create a new API token",
-        ));
-    }
-
     let max_size = 2000;
     let length = req
         .content_length()
@@ -64,11 +60,18 @@ pub fn new(req: &mut dyn Request) -> AppResult<Response> {
         return Err(bad_request("name must have a value"));
     }
 
-    let user = req.user()?;
     let conn = req.db_conn()?;
+    let ids = req.authenticate(&conn)?;
+    let user = ids.find_user(&conn)?;
+
+    if ids.api_token_id().is_some() {
+        return Err(bad_request(
+            "cannot use an API token to create a new API token",
+        ));
+    }
 
     let max_token_per_user = 500;
-    let count = ApiToken::belonging_to(user)
+    let count = ApiToken::belonging_to(&user)
         .count()
         .get_result::<i64>(&*conn)?;
     if count >= max_token_per_user {
@@ -95,9 +98,11 @@ pub fn revoke(req: &mut dyn Request) -> AppResult<Response> {
         .parse::<i32>()
         .map_err(|e| bad_request(&format!("invalid token id: {:?}", e)))?;
 
-    diesel::update(ApiToken::belonging_to(req.user()?).find(id))
+    let conn = req.db_conn()?;
+    let user = req.authenticate(&conn)?.find_user(&conn)?;
+    diesel::update(ApiToken::belonging_to(&user).find(id))
         .set(api_tokens::revoked.eq(true))
-        .execute(&*req.db_conn()?)?;
+        .execute(&*conn)?;
 
     #[derive(Serialize)]
     struct R {}

src/controllers/user/me.rs

@@ -24,8 +24,8 @@ pub fn me(req: &mut dyn Request) -> AppResult<Response> {
     // perhaps adding `req.mut_extensions().insert(user)` to the
     // update_user route, however this somehow does not seem to work
 
-    let user_id = req.user()?.id;
     let conn = req.db_conn()?;
+    let user_id = req.authenticate(&conn)?.user_id();
 
     let (user, verified, email, verification_sent) = users::table
         .find(user_id)
@@ -64,10 +64,10 @@ pub fn me(req: &mut dyn Request) -> AppResult<Response> {
 pub fn updates(req: &mut dyn Request) -> AppResult<Response> {
     use diesel::dsl::any;
 
-    let user = req.user()?;
     let conn = req.db_conn()?;
+    let user = req.authenticate(&conn)?.find_user(&conn)?;
 
-    let followed_crates = Follow::belonging_to(user).select(follows::crate_id);
+    let followed_crates = Follow::belonging_to(&user).select(follows::crate_id);
     let data = versions::table
         .inner_join(crates::table)
         .left_outer_join(users::table)
@@ -118,12 +118,12 @@ pub fn update_user(req: &mut dyn Request) -> AppResult<Response> {
     let mut body = String::new();
 
     req.body().read_to_string(&mut body)?;
-    let user = req.user()?;
-    let name = &req.params()["user_id"];
+    let param_user_id = &req.params()["user_id"];
     let conn = req.db_conn()?;
+    let user = req.authenticate(&conn)?.find_user(&conn)?;
 
     // need to check if current user matches user to be updated
-    if &user.id.to_string() != name {
+    if &user.id.to_string() != param_user_id {
         return Err(bad_request("current user does not match requested user"));
     }
 
@@ -195,19 +195,19 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult<Response> {
     use diesel::dsl::sql;
     use diesel::update;
 
-    let user = req.user()?;
-    let name = &req.params()["user_id"]
+    let param_user_id = req.params()["user_id"]
         .parse::<i32>()
         .chain_error(|| bad_request("invalid user_id"))?;
     let conn = req.db_conn()?;
+    let user = req.authenticate(&conn)?.find_user(&conn)?;
 
     // need to check if current user matches user to be updated
-    if &user.id != name {
+    if user.id != param_user_id {
         return Err(bad_request("current user does not match requested user"));
     }
 
     conn.transaction(|| {
-        let email = update(Email::belonging_to(user))
+        let email = update(Email::belonging_to(&user))
             .set(emails::token.eq(sql("DEFAULT")))
             .get_result::<Email>(&*conn)
             .map_err(|_| bad_request("Email could not be found"))?;
@@ -238,12 +238,12 @@ pub fn update_email_notifications(req: &mut dyn Request) -> AppResult<Response>
         .map(|c| (c.id, c.email_notifications))
         .collect();
 
-    let user = req.user()?;
     let conn = req.db_conn()?;
+    let user_id = req.authenticate(&conn)?.user_id();
 
     // Build inserts from existing crates belonging to the current user
     let to_insert = CrateOwner::by_owner_kind(OwnerKind::User)
-        .filter(owner_id.eq(user.id))
+        .filter(owner_id.eq(user_id))
         .select((crate_id, owner_id, owner_kind, email_notifications))
         .load(&*conn)?
         .into_iter()

src/controllers/util.rs

@@ -0,0 +1,53 @@
+use super::prelude::*;
+
+use crate::middleware::current_user::TrustedUserId;
+use crate::models::{ApiToken, User};
+use crate::util::errors::{internal, AppError, AppResult, ChainError, Unauthorized};
+
+#[derive(Debug)]
+pub struct AuthenticatedUser {
+    user_id: i32,
+    token_id: Option<i32>,
+}
+
+impl AuthenticatedUser {
+    pub fn user_id(&self) -> i32 {
+        self.user_id
+    }
+
+    pub fn api_token_id(&self) -> Option<i32> {
+        self.token_id
+    }
+
+    pub fn find_user(&self, conn: &PgConnection) -> AppResult<User> {
+        User::find(conn, self.user_id())
+            .chain_error(|| internal("user_id from cookie or token not found in database"))
+    }
+}
+
+impl<'a> UserAuthenticationExt for dyn Request + 'a {
+    /// Obtain `AuthenticatedUser` for the request or return an `Unauthorized` error
+    fn authenticate(&self, conn: &PgConnection) -> AppResult<AuthenticatedUser> {
+        if let Some(id) = self.extensions().find::<TrustedUserId>() {
+            // A trusted user_id was provided by a signed cookie (or a test `MockCookieUser`)
+            Ok(AuthenticatedUser {
+                user_id: id.0,
+                token_id: None,
+            })
+        } else {
+            // Otherwise, look for an `Authorization` header on the request
+            if let Some(headers) = self.headers().find("Authorization") {
+                ApiToken::find_by_api_token(conn, headers[0])
+                    .map(|token| AuthenticatedUser {
+                        user_id: token.user_id,
+                        token_id: Some(token.id),
+                    })
+                    // Convert a NotFound (or other database error) into Unauthorized
+                    .map_err(|_| Box::new(Unauthorized) as Box<dyn AppError>)
+            } else {
+                // Unable to authenticate the user
+                Err(Box::new(Unauthorized))
+            }
+        }
+    }
+}

src/controllers/version.rs

@@ -5,15 +5,17 @@ pub mod yank;
 
 use super::prelude::*;
 
+use crate::db::DieselPooledConn;
 use crate::models::{Crate, CrateVersions, Version};
 use crate::schema::versions;
 
-fn version_and_crate(req: &mut dyn Request) -> AppResult<(Version, Crate)> {
+fn version_and_crate(req: &dyn Request) -> AppResult<(DieselPooledConn<'_>, Version, Crate)> {
     let crate_name = &req.params()["crate_id"];
     let semver = &req.params()["version"];
     if semver::Version::parse(semver).is_err() {
         return Err(cargo_err(&format_args!("invalid semver: {}", semver)));
     };
+
     let conn = req.db_conn()?;
     let krate = Crate::by_name(crate_name).first::<Crate>(&*conn)?;
     let version = krate
@@ -26,5 +28,6 @@ fn version_and_crate(req: &mut dyn Request) -> AppResult<(Version, Crate)> {
                 crate_name, semver
             ))
         })?;
-    Ok((version, krate))
+
+    Ok((conn, version, krate))
 }