Merge pull request #5889 from Turbo87/auth-conn

auth: Reuse existing database connections

Author: Turbo87

src/auth.rs

@@ -1,6 +1,5 @@
 use crate::controllers;
 use crate::controllers::util::RequestPartsExt;
-use crate::middleware::app::RequestApp;
 use crate::middleware::log_request::CustomMetadataRequestExt;
 use crate::middleware::session::RequestSession;
 use crate::models::token::{CrateScope, EndpointScope};
@@ -56,8 +55,12 @@ impl AuthCheck {
         }
     }
 
-    pub fn check<T: RequestPartsExt>(&self, request: &T) -> AppResult<Authentication> {
-        let auth = authenticate(request)?;
+    pub fn check<T: RequestPartsExt>(
+        &self,
+        request: &T,
+        conn: &PgConnection,
+    ) -> AppResult<Authentication> {
+        let auth = authenticate(request, conn)?;
 
         if let Some(token) = auth.api_token() {
             if !self.allow_token {
@@ -203,18 +206,16 @@ fn authenticate_via_token<T: RequestPartsExt>(
     Ok(Some(TokenAuthentication { user, token }))
 }
 
-fn authenticate<T: RequestPartsExt>(req: &T) -> AppResult<Authentication> {
+fn authenticate<T: RequestPartsExt>(req: &T, conn: &PgConnection) -> AppResult<Authentication> {
     controllers::util::verify_origin(req)?;
 
-    let conn = req.app().db_write()?;
-
-    match authenticate_via_cookie(req, &conn) {
+    match authenticate_via_cookie(req, conn) {
         Ok(None) => {}
         Ok(Some(auth)) => return Ok(Authentication::Cookie(auth)),
         Err(err) => return Err(err),
     }
 
-    match authenticate_via_token(req, &conn) {
+    match authenticate_via_token(req, conn) {
         Ok(None) => {}
         Ok(Some(auth)) => return Ok(Authentication::Token(auth)),
         Err(err) => return Err(err),

src/controllers/crate_owner_invitation.rs

@@ -18,7 +18,8 @@ use std::collections::{HashMap, HashSet};
 /// Handles the `GET /api/v1/me/crate_owner_invitations` route.
 pub async fn list(req: ConduitRequest) -> AppResult<Json<Value>> {
     conduit_compat(move || {
-        let auth = AuthCheck::only_cookie().check(&req)?;
+        let conn = req.app().db_write()?;
+        let auth = AuthCheck::only_cookie().check(&req, &conn)?;
         let user_id = auth.user_id();
 
         let PrivateListResponse {
@@ -57,7 +58,8 @@ pub async fn list(req: ConduitRequest) -> AppResult<Json<Value>> {
 /// Handles the `GET /api/private/crate_owner_invitations` route.
 pub async fn private_list(req: ConduitRequest) -> AppResult<Json<PrivateListResponse>> {
     conduit_compat(move || {
-        let auth = AuthCheck::only_cookie().check(&req)?;
+        let conn = req.app().db_write()?;
+        let auth = AuthCheck::only_cookie().check(&req, &conn)?;
 
         let filter = if let Some(crate_name) = req.query().get("crate_name") {
             ListFilter::CrateName(crate_name.clone())
@@ -263,18 +265,19 @@ pub async fn handle_invite(mut req: ConduitRequest) -> AppResult<Json<Value>> {
 
         let crate_invite = crate_invite.crate_owner_invite;
 
-        let auth = AuthCheck::default().check(&req)?;
+        let state = req.app();
+        let conn = state.db_write()?;
+
+        let auth = AuthCheck::default().check(&req, &conn)?;
         let user_id = auth.user_id();
 
-        let state = req.app();
-        let conn = &*state.db_write()?;
         let config = &state.config;
 
-        let invitation = CrateOwnerInvitation::find_by_id(user_id, crate_invite.crate_id, conn)?;
+        let invitation = CrateOwnerInvitation::find_by_id(user_id, crate_invite.crate_id, &conn)?;
         if crate_invite.accepted {
-            invitation.accept(conn, config)?;
+            invitation.accept(&conn, config)?;
         } else {
-            invitation.decline(conn)?;
+            invitation.decline(&conn)?;
         }
 
         Ok(Json(json!({ "crate_owner_invitation": crate_invite })))

src/controllers/krate/follow.rs

@@ -18,8 +18,8 @@ fn follow_target(crate_name: &str, conn: &DieselPooledConn<'_>, user_id: i32) ->
 /// Handles the `PUT /crates/:crate_id/follow` route.
 pub async fn follow(Path(crate_name): Path<String>, req: ConduitRequest) -> AppResult<Response> {
     conduit_compat(move || {
-        let user_id = AuthCheck::default().check(&req)?.user_id();
         let conn = req.app().db_write()?;
+        let user_id = AuthCheck::default().check(&req, &conn)?.user_id();
         let follow = follow_target(&crate_name, &conn, user_id)?;
         diesel::insert_into(follows::table)
             .values(&follow)
@@ -34,8 +34,8 @@ pub async fn follow(Path(crate_name): Path<String>, req: ConduitRequest) -> AppR
 /// Handles the `DELETE /crates/:crate_id/follow` route.
 pub async fn unfollow(Path(crate_name): Path<String>, req: ConduitRequest) -> AppResult<Response> {
     conduit_compat(move || {
-        let user_id = AuthCheck::default().check(&req)?.user_id();
         let conn = req.app().db_write()?;
+        let user_id = AuthCheck::default().check(&req, &conn)?.user_id();
         let follow = follow_target(&crate_name, &conn, user_id)?;
         diesel::delete(&follow).execute(&*conn)?;
 
@@ -52,8 +52,8 @@ pub async fn following(
     conduit_compat(move || {
         use diesel::dsl::exists;
 
-        let user_id = AuthCheck::only_cookie().check(&req)?.user_id();
         let conn = req.app().db_read_prefer_primary()?;
+        let user_id = AuthCheck::only_cookie().check(&req, &conn)?.user_id();
         let follow = follow_target(&crate_name, &conn, user_id)?;
         let following =
             diesel::select(exists(follows::table.find(follow.id()))).get_result::<bool>(&*conn)?;

src/controllers/krate/owners.rs

@@ -103,15 +103,15 @@ fn modify_owners<B: Read>(
     req: &mut Request<B>,
     add: bool,
 ) -> AppResult<Json<Value>> {
-    let auth = AuthCheck::default()
-        .with_endpoint_scope(EndpointScope::ChangeOwners)
-        .for_crate(crate_name)
-        .check(req)?;
-
     let logins = parse_owners_request(req)?;
     let app = req.app();
 
     let conn = app.db_write()?;
+    let auth = AuthCheck::default()
+        .with_endpoint_scope(EndpointScope::ChangeOwners)
+        .for_crate(crate_name)
+        .check(req, &conn)?;
+
     let user = auth.user();
 
     conn.transaction(|| {

src/controllers/krate/publish.rs

@@ -82,7 +82,7 @@ pub async fn publish(mut req: ConduitRequest) -> AppResult<Json<GoodCrate>> {
         let auth = AuthCheck::default()
             .with_endpoint_scope(endpoint_scope)
             .for_crate(&new_crate.name)
-            .check(&req)?;
+            .check(&req, &conn)?;
 
         let api_token_id = auth.api_token_id();
         let user = auth.user();

src/controllers/krate/search.rs

@@ -115,6 +115,8 @@ pub async fn search(req: ConduitRequest) -> AppResult<Json<Value>> {
             );
         }
 
+        let conn = req.app().db_read()?;
+
         if let Some(kws) = params.get("all_keywords") {
             // Calculating the total number of results with filters is not supported yet.
             supports_seek = false;
@@ -188,7 +190,7 @@ pub async fn search(req: ConduitRequest) -> AppResult<Json<Value>> {
             // Calculating the total number of results with filters is not supported yet.
             supports_seek = false;
 
-            let user_id = AuthCheck::default().check(&req)?.user_id();
+            let user_id = AuthCheck::default().check(&req, &conn)?.user_id();
 
             query = query.filter(
                 crates::id.eq_any(
@@ -250,7 +252,6 @@ pub async fn search(req: ConduitRequest) -> AppResult<Json<Value>> {
             .limit_page_numbers()
             .enable_seek(supports_seek)
             .gather(&req)?;
-        let conn = req.app().db_read()?;
 
         let (explicit_page, seek) = match pagination.page.clone() {
             Page::Numeric(_) => (true, None),

src/controllers/token.rs

@@ -11,8 +11,8 @@ use serde_json as json;
 /// Handles the `GET /me/tokens` route.
 pub async fn list(req: ConduitRequest) -> AppResult<Json<Value>> {
     conduit_compat(move || {
-        let auth = AuthCheck::only_cookie().check(&req)?;
         let conn = req.app().db_read_prefer_primary()?;
+        let auth = AuthCheck::only_cookie().check(&req, &conn)?;
         let user = auth.user();
 
         let tokens: Vec<ApiToken> = ApiToken::belonging_to(user)
@@ -48,14 +48,15 @@ pub async fn new(mut req: ConduitRequest) -> AppResult<Json<Value>> {
             return Err(bad_request("name must have a value"));
         }
 
-        let auth = AuthCheck::default().check(&req)?;
+        let conn = req.app().db_write()?;
+
+        let auth = AuthCheck::default().check(&req, &conn)?;
         if auth.api_token_id().is_some() {
             return Err(bad_request(
                 "cannot use an API token to create a new API token",
             ));
         }
 
-        let conn = req.app().db_write()?;
         let user = auth.user();
 
         let max_token_per_user = 500;
@@ -77,8 +78,8 @@ pub async fn new(mut req: ConduitRequest) -> AppResult<Json<Value>> {
 /// Handles the `DELETE /me/tokens/:id` route.
 pub async fn revoke(Path(id): Path<i32>, req: ConduitRequest) -> AppResult<Json<Value>> {
     conduit_compat(move || {
-        let auth = AuthCheck::default().check(&req)?;
         let conn = req.app().db_write()?;
+        let auth = AuthCheck::default().check(&req, &conn)?;
         let user = auth.user();
         diesel::update(ApiToken::belonging_to(user).find(id))
             .set(api_tokens::revoked.eq(true))
@@ -92,12 +93,12 @@ pub async fn revoke(Path(id): Path<i32>, req: ConduitRequest) -> AppResult<Json<
 /// Handles the `DELETE /tokens/current` route.
 pub async fn revoke_current(req: ConduitRequest) -> AppResult<Response> {
     conduit_compat(move || {
-        let auth = AuthCheck::default().check(&req)?;
+        let conn = req.app().db_write()?;
+        let auth = AuthCheck::default().check(&req, &conn)?;
         let api_token_id = auth
             .api_token_id()
             .ok_or_else(|| bad_request("token not provided"))?;
 
-        let conn = req.app().db_write()?;
         diesel::update(api_tokens::table.filter(api_tokens::id.eq(api_token_id)))
             .set(api_tokens::revoked.eq(true))
             .execute(&*conn)?;

src/controllers/user/me.rs

@@ -15,8 +15,8 @@ use crate::views::{EncodableMe, EncodablePrivateUser, EncodableVersion, OwnedCra
 /// Handles the `GET /me` route.
 pub async fn me(req: ConduitRequest) -> AppResult<Json<EncodableMe>> {
     conduit_compat(move || {
-        let user_id = AuthCheck::only_cookie().check(&req)?.user_id();
         let conn = req.app().db_read_prefer_primary()?;
+        let user_id = AuthCheck::only_cookie().check(&req, &conn)?.user_id();
 
         let (user, verified, email, verification_sent): (User, Option<bool>, Option<String>, bool) =
             users::table
@@ -59,7 +59,8 @@ pub async fn updates(req: ConduitRequest) -> AppResult<Json<Value>> {
     conduit_compat(move || {
         use diesel::dsl::any;
 
-        let auth = AuthCheck::only_cookie().check(&req)?;
+        let conn = req.app().db_read_prefer_primary()?;
+        let auth = AuthCheck::only_cookie().check(&req, &conn)?;
         let user = auth.user();
 
         let followed_crates = Follow::belonging_to(user).select(follows::crate_id);
@@ -74,7 +75,6 @@ pub async fn updates(req: ConduitRequest) -> AppResult<Json<Value>> {
                 users::all_columns.nullable(),
             ))
             .pages_pagination(PaginationOptions::builder().gather(&req)?);
-        let conn = req.app().db_read_prefer_primary()?;
         let data: Paginated<(Version, String, Option<User>)> = query.load(&conn)?;
         let more = data.next_page_params().is_some();
         let versions = data.iter().map(|(v, _, _)| v).cloned().collect::<Vec<_>>();
@@ -107,8 +107,10 @@ pub async fn update_user(
         use self::emails::user_id;
         use diesel::insert_into;
 
-        let auth = AuthCheck::default().check(&req)?;
+        let state = req.app().clone();
+        let conn = state.db_write()?;
 
+        let auth = AuthCheck::default().check(&req, &conn)?;
         let user = auth.user();
 
         // need to check if current user matches user to be updated
@@ -138,8 +140,6 @@ pub async fn update_user(
             return Err(bad_request("empty email rejected"));
         }
 
-        let state = req.app();
-        let conn = state.db_write()?;
         conn.transaction::<_, BoxedAppError, _>(|| {
             let new_email = NewEmail {
                 user_id: user.id,
@@ -203,10 +203,10 @@ pub async fn regenerate_token_and_send(
         use diesel::dsl::sql;
         use diesel::update;
 
-        let auth = AuthCheck::default().check(&req)?;
-
         let state = req.app();
         let conn = state.db_write()?;
+
+        let auth = AuthCheck::default().check(&req, &conn)?;
         let user = auth.user();
 
         // need to check if current user matches user to be updated
@@ -249,8 +249,8 @@ pub async fn update_email_notifications(mut req: ConduitRequest) -> AppResult<Re
                 .map(|c| (c.id, c.email_notifications))
                 .collect();
 
-        let user_id = AuthCheck::default().check(&req)?.user_id();
         let conn = req.app().db_write()?;
+        let user_id = AuthCheck::default().check(&req, &conn)?.user_id();
 
         // Build inserts from existing crates belonging to the current user
         let to_insert = CrateOwner::by_owner_kind(OwnerKind::User)

src/controllers/version/yank.rs

@@ -48,13 +48,14 @@ fn modify_yank<B>(
         return Err(cargo_err(&format_args!("invalid semver: {version}")));
     }
 
+    let state = req.app();
+    let conn = state.db_write()?;
+
     let auth = AuthCheck::default()
         .with_endpoint_scope(EndpointScope::Yank)
         .for_crate(crate_name)
-        .check(req)?;
+        .check(req, &conn)?;
 
-    let state = req.app();
-    let conn = state.db_write()?;
     let (version, krate) = version_and_crate(&conn, crate_name, version)?;
     let api_token_id = auth.api_token_id();
     let user = auth.user();