From 8cab4f2d4c84413e0c59dedfe67e6578561bef2f Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 08:33:07 +0800 Subject: [PATCH 01/13] controllers/version/yank: Extract `authenticate()` fn --- src/controllers/version/metadata.rs | 19 +++++++++++-------- src/controllers/version/yank.rs | 13 +++++++++++-- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index 86a040a001c..b84ccd289c6 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -16,7 +16,7 @@ use serde_json::Value; use tokio::runtime::Handle; use crate::app::AppState; -use crate::auth::AuthCheck; +use crate::auth::{AuthCheck, Authentication}; use crate::models::token::EndpointScope; use crate::models::{ insert_version_owner_action, Crate, Dependency, Rights, Version, VersionAction, @@ -132,12 +132,13 @@ pub async fn update( let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); validate_yank_update(&update_request.version, &version)?; + let auth = authenticate(&req, conn, &krate.name)?; perform_version_yank_update( &state, - &req, conn, &mut version, &krate, + &auth, update_request.version.yanked, update_request.version.yank_message, )?; @@ -166,22 +167,24 @@ fn validate_yank_update(update_data: &VersionUpdate, version: &Version) -> AppRe Ok(()) } +pub fn authenticate(req: &Parts, conn: &mut impl Conn, name: &str) -> AppResult { + AuthCheck::default() + .with_endpoint_scope(EndpointScope::Yank) + .for_crate(name) + .check(req, conn) +} + pub fn perform_version_yank_update( state: &AppState, - req: &Parts, conn: &mut impl Conn, version: &mut Version, krate: &Crate, + auth: &Authentication, yanked: Option, yank_message: Option, ) -> AppResult<()> { use diesel::RunQueryDsl; - let auth = AuthCheck::default() - .with_endpoint_scope(EndpointScope::Yank) - .for_crate(&krate.name) - .check(req, conn)?; - state .rate_limiter .check_rate_limit(auth.user_id(), LimitedAction::YankUnyank, conn)?; diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index e18805f2ffa..67b61c63201 100644 --- a/src/controllers/version/yank.rs +++ b/src/controllers/version/yank.rs @@ -1,6 +1,6 @@ //! Endpoints for yanking and unyanking specific versions of crates -use super::metadata::perform_version_yank_update; +use super::metadata::{authenticate, perform_version_yank_update}; use super::version_and_crate; use crate::app::AppState; use crate::controllers::helpers::ok_true; @@ -56,7 +56,16 @@ async fn modify_yank( let (mut version, krate) = version_and_crate(&mut conn, &crate_name, &version).await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - perform_version_yank_update(&state, &req, conn, &mut version, &krate, Some(yanked), None)?; + let auth = authenticate(&req, conn, &crate_name)?; + perform_version_yank_update( + &state, + conn, + &mut version, + &krate, + &auth, + Some(yanked), + None, + )?; ok_true() }) .await From 66f71fb9f072bc30fe0df05de31f2cafe45c6a12 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 6 Nov 2024 21:30:37 +0800 Subject: [PATCH 02/13] auth: Implement async fns for migrating `AuthCheck::check()` to async --- src/auth.rs | 133 ++++++++++++++++++++++++++++++++++++++++++++ src/models/token.rs | 38 +++++++++++++ src/models/user.rs | 7 +++ 3 files changed, 178 insertions(+) diff --git a/src/auth.rs b/src/auth.rs index 089d74d18d0..3b4f1fd4a9a 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -10,6 +10,7 @@ use crate::util::errors::{ }; use crate::util::token::HashedToken; use chrono::Utc; +use diesel_async::AsyncPgConnection; use http::header; use http::request::Parts; @@ -94,6 +95,47 @@ impl AuthCheck { Ok(auth) } + #[instrument(name = "auth.async_check", skip_all)] + pub async fn async_check( + &self, + parts: &Parts, + conn: &mut AsyncPgConnection, + ) -> AppResult { + let auth = async_authenticate(parts, conn).await?; + + if let Some(token) = auth.api_token() { + if !self.allow_token { + let error_message = + "API Token authentication was explicitly disallowed for this API"; + parts.request_log().add("cause", error_message); + + return Err(forbidden( + "this action can only be performed on the crates.io website", + )); + } + + if !self.endpoint_scope_matches(token.endpoint_scopes.as_ref()) { + let error_message = "Endpoint scope mismatch"; + parts.request_log().add("cause", error_message); + + return Err(forbidden( + "this token does not have the required permissions to perform this action", + )); + } + + if !self.crate_scope_matches(token.crate_scopes.as_ref()) { + let error_message = "Crate scope mismatch"; + parts.request_log().add("cause", error_message); + + return Err(forbidden( + "this token does not have the required permissions to perform this action", + )); + } + } + + Ok(auth) + } + fn endpoint_scope_matches(&self, token_scopes: Option<&Vec>) -> bool { match (&token_scopes, &self.endpoint_scope) { // The token is a legacy token. @@ -193,6 +235,32 @@ fn authenticate_via_cookie( Ok(Some(CookieAuthentication { user })) } +#[instrument(skip_all)] +async fn async_authenticate_via_cookie( + parts: &Parts, + conn: &mut AsyncPgConnection, +) -> AppResult> { + let user_id_from_session = parts + .session() + .get("user_id") + .and_then(|s| s.parse::().ok()); + + let Some(id) = user_id_from_session else { + return Ok(None); + }; + + let user = User::async_find(conn, id).await.map_err(|err| { + parts.request_log().add("cause", err); + internal("user_id from cookie not found in database") + })?; + + ensure_not_locked(&user)?; + + parts.request_log().add("uid", id); + + Ok(Some(CookieAuthentication { user })) +} + #[instrument(skip_all)] fn authenticate_via_token( parts: &Parts, @@ -230,6 +298,45 @@ fn authenticate_via_token( Ok(Some(TokenAuthentication { user, token })) } +#[instrument(skip_all)] +async fn async_authenticate_via_token( + parts: &Parts, + conn: &mut AsyncPgConnection, +) -> AppResult> { + let maybe_authorization = parts + .headers() + .get(header::AUTHORIZATION) + .and_then(|h| h.to_str().ok()); + + let Some(header_value) = maybe_authorization else { + return Ok(None); + }; + + let token = + HashedToken::parse(header_value).map_err(|_| InsecurelyGeneratedTokenRevoked::boxed())?; + + let token = ApiToken::async_find_by_api_token(conn, &token) + .await + .map_err(|e| { + let cause = format!("invalid token caused by {e}"); + parts.request_log().add("cause", cause); + + forbidden("authentication failed") + })?; + + let user = User::async_find(conn, token.user_id).await.map_err(|err| { + parts.request_log().add("cause", err); + internal("user_id from token not found in database") + })?; + + ensure_not_locked(&user)?; + + parts.request_log().add("uid", token.user_id); + parts.request_log().add("tokenid", token.id); + + Ok(Some(TokenAuthentication { user, token })) +} + #[instrument(skip_all)] fn authenticate(parts: &Parts, conn: &mut impl Conn) -> AppResult { controllers::util::verify_origin(parts)?; @@ -253,6 +360,32 @@ fn authenticate(parts: &Parts, conn: &mut impl Conn) -> AppResult AppResult { + controllers::util::verify_origin(parts)?; + + match async_authenticate_via_cookie(parts, conn).await { + Ok(None) => {} + Ok(Some(auth)) => return Ok(Authentication::Cookie(auth)), + Err(err) => return Err(err), + } + + match async_authenticate_via_token(parts, conn).await { + Ok(None) => {} + Ok(Some(auth)) => return Ok(Authentication::Token(auth)), + Err(err) => return Err(err), + } + + // Unable to authenticate the user + let cause = "no cookie session or auth header found"; + parts.request_log().add("cause", cause); + + return Err(forbidden("this action requires authentication")); +} + fn ensure_not_locked(user: &User) -> AppResult<()> { if let Some(reason) = &user.account_lock_reason { let still_locked = user diff --git a/src/models/token.rs b/src/models/token.rs index 77f558a2d1a..3a11715dede 100644 --- a/src/models/token.rs +++ b/src/models/token.rs @@ -1,6 +1,7 @@ mod scopes; use chrono::NaiveDateTime; +use diesel_async::AsyncPgConnection; pub use self::scopes::{CrateScope, EndpointScope}; use crate::models::User; @@ -92,6 +93,43 @@ impl ApiToken { .or_else(|_| tokens.select(ApiToken::as_select()).first(conn)) .map_err(Into::into) } + + pub async fn async_find_by_api_token( + conn: &mut AsyncPgConnection, + token: &HashedToken, + ) -> QueryResult { + use diesel::{dsl::now, update}; + use diesel_async::scoped_futures::ScopedFutureExt; + use diesel_async::{AsyncConnection, RunQueryDsl}; + + let tokens = api_tokens::table + .filter(api_tokens::revoked.eq(false)) + .filter( + api_tokens::expired_at + .is_null() + .or(api_tokens::expired_at.gt(now)), + ) + .filter(api_tokens::token.eq(token)); + + // If the database is in read only mode, we can't update last_used_at. + // Try updating in a new transaction, if that fails, fall back to reading + let token = conn + .transaction(|conn| { + async move { + update(tokens) + .set(api_tokens::last_used_at.eq(now.nullable())) + .returning(ApiToken::as_returning()) + .get_result(conn) + .await + } + .scope_boxed() + }) + .await; + let Ok(_) = token else { + return tokens.select(ApiToken::as_select()).first(conn).await; + }; + token + } } #[derive(Debug)] diff --git a/src/models/user.rs b/src/models/user.rs index bd895f173ce..88a7307debe 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -1,4 +1,5 @@ use chrono::NaiveDateTime; +use diesel_async::AsyncPgConnection; use secrecy::SecretString; use crate::app::App; @@ -34,6 +35,12 @@ impl User { users::table.find(id).first(conn) } + pub async fn async_find(conn: &mut AsyncPgConnection, id: i32) -> QueryResult { + use diesel_async::RunQueryDsl; + + users::table.find(id).first(conn).await + } + pub fn find_by_login(conn: &mut impl Conn, login: &str) -> QueryResult { use diesel::RunQueryDsl; From b821110215e817fde3e80a4ee7b137cf70bcca75 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 22:21:36 +0800 Subject: [PATCH 03/13] controllers/version/yank: Convert `authenticate()` to async --- src/controllers/version/metadata.rs | 14 ++++++++++---- src/controllers/version/yank.rs | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index b84ccd289c6..e91297d692d 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -9,6 +9,7 @@ use axum::Json; use crates_io_database::schema::{crates, dependencies}; use crates_io_worker::BackgroundJob; use diesel_async::async_connection_wrapper::AsyncConnectionWrapper; +use diesel_async::AsyncPgConnection; use http::request::Parts; use http::StatusCode; use serde::Deserialize; @@ -128,11 +129,11 @@ pub async fn update( let mut conn = state.db_write().await?; let (mut version, krate) = version_and_crate(&mut conn, &crate_name, &version).await?; + validate_yank_update(&update_request.version, &version)?; + let auth = authenticate(&req, &mut conn, &krate.name).await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - validate_yank_update(&update_request.version, &version)?; - let auth = authenticate(&req, conn, &krate.name)?; perform_version_yank_update( &state, conn, @@ -167,11 +168,16 @@ fn validate_yank_update(update_data: &VersionUpdate, version: &Version) -> AppRe Ok(()) } -pub fn authenticate(req: &Parts, conn: &mut impl Conn, name: &str) -> AppResult { +pub async fn authenticate( + req: &Parts, + conn: &mut AsyncPgConnection, + name: &str, +) -> AppResult { AuthCheck::default() .with_endpoint_scope(EndpointScope::Yank) .for_crate(name) - .check(req, conn) + .async_check(req, conn) + .await } pub fn perform_version_yank_update( diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index 67b61c63201..51bc588d1ca 100644 --- a/src/controllers/version/yank.rs +++ b/src/controllers/version/yank.rs @@ -54,9 +54,9 @@ async fn modify_yank( let mut conn = state.db_write().await?; let (mut version, krate) = version_and_crate(&mut conn, &crate_name, &version).await?; + let auth = authenticate(&req, &mut conn, &crate_name).await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = authenticate(&req, conn, &crate_name)?; perform_version_yank_update( &state, conn, From 6a5296f57bbab9a369e641a6e47cbbbc1bcc8efb Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 23:37:13 +0800 Subject: [PATCH 04/13] controllers/krate/search: Change to use `async_check()` --- src/controllers/krate/search.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index f67635b3b8b..dd2597c185f 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -6,10 +6,12 @@ use axum::Json; use diesel::dsl::{exists, sql, InnerJoinQuerySource, LeftJoinQuerySource}; use diesel::sql_types::{Array, Bool, Text}; use diesel_async::async_connection_wrapper::AsyncConnectionWrapper; +use diesel_async::AsyncPgConnection; use diesel_full_text_search::*; use http::request::Parts; use serde_json::Value; use std::cell::OnceCell; +use tokio::runtime::Handle; use crate::app::AppState; use crate::controllers::helpers::Paginate; @@ -22,7 +24,6 @@ use crate::controllers::helpers::pagination::{Page, Paginated, PaginationOptions use crate::models::krate::ALL_COLUMNS; use crate::sql::{array_agg, canon_crate_name, lower}; use crate::tasks::spawn_blocking; -use crate::util::diesel::Conn; use crate::util::RequestUtils; /// Handles the `GET /crates` route. @@ -303,12 +304,14 @@ impl<'a> FilterParams<'a> { .as_deref() } - fn authed_user_id(&self, req: &Parts, conn: &mut impl Conn) -> AppResult { + fn authed_user_id(&self, req: &Parts, conn: &mut AsyncPgConnection) -> AppResult { if let Some(val) = self._auth_user_id.get() { return Ok(*val); } - let user_id = AuthCheck::default().check(req, conn)?.user_id(); + let user_id = Handle::current() + .block_on(AuthCheck::default().async_check(req, conn))? + .user_id(); // This should not fail, because of the `get()` check above let _ = self._auth_user_id.set(user_id); @@ -319,7 +322,7 @@ impl<'a> FilterParams<'a> { fn make_query( &'a self, req: &Parts, - conn: &mut impl Conn, + conn: &mut AsyncPgConnection, ) -> AppResult> { let mut query = crates::table.into_boxed(); From 7466b090b3a09a7243d39831444c39f358a9f0ec Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 23:40:03 +0800 Subject: [PATCH 05/13] controllers/krate/publish: Change to use `async_check()` --- src/controllers/krate/publish.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 88d686c9887..4e7367a3c94 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -81,17 +81,17 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult = &mut conn.into(); + let (existing_crate, auth) = { + use diesel_async::RunQueryDsl; // this query should only be used for the endpoint scope calculation // since a race condition there would only cause `publish-new` instead of // `publish-update` to be used. let existing_crate: Option = Crate::by_name(&metadata.name) - .first::(conn) + .first::(&mut conn) + .await .optional()?; let endpoint_scope = match existing_crate { @@ -102,7 +102,15 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult = &mut conn.into(); let api_token_id = auth.api_token_id(); let user = auth.user(); From 632f58fee92e3b242fc30844637b995542577ce6 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 23:51:01 +0800 Subject: [PATCH 06/13] controllers/krate/follow: Change to use `async_check()` --- src/controllers/krate/follow.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/controllers/krate/follow.rs b/src/controllers/krate/follow.rs index d13d5614859..38a4220f13b 100644 --- a/src/controllers/krate/follow.rs +++ b/src/controllers/krate/follow.rs @@ -34,13 +34,16 @@ pub async fn follow( Path(crate_name): Path, req: Parts, ) -> AppResult { - let conn = app.db_write().await?; + let mut conn = app.db_write().await?; + let user_id = AuthCheck::default() + .async_check(&req, &mut conn) + .await? + .user_id(); spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - 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) @@ -58,13 +61,16 @@ pub async fn unfollow( Path(crate_name): Path, req: Parts, ) -> AppResult { - let conn = app.db_write().await?; + let mut conn = app.db_write().await?; + let user_id = AuthCheck::default() + .async_check(&req, &mut conn) + .await? + .user_id(); spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let user_id = AuthCheck::default().check(&req, conn)?.user_id(); let follow = follow_target(&crate_name, conn, user_id)?; diesel::delete(&follow).execute(conn)?; @@ -79,7 +85,11 @@ pub async fn following( Path(crate_name): Path, req: Parts, ) -> AppResult> { - let conn = app.db_read_prefer_primary().await?; + let mut conn = app.db_read_prefer_primary().await?; + let user_id = AuthCheck::only_cookie() + .async_check(&req, &mut conn) + .await? + .user_id(); spawn_blocking(move || { use diesel::RunQueryDsl; @@ -87,7 +97,6 @@ pub async fn following( use diesel::dsl::exists; - 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::(conn)?; From 42aa6204d8f60037c348c9c5710742a60c1481df Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 23:51:10 +0800 Subject: [PATCH 07/13] controllers/krate/owners: Change to use `async_check()` --- src/controllers/krate/owners.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 5abc2a86ed1..245d3b0e6d6 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -130,17 +130,17 @@ async fn modify_owners( )); } - let conn = app.db_write().await?; + let mut conn = app.db_write().await?; + let auth = AuthCheck::default() + .with_endpoint_scope(EndpointScope::ChangeOwners) + .for_crate(&crate_name) + .async_check(&parts, &mut conn) + .await?; spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = AuthCheck::default() - .with_endpoint_scope(EndpointScope::ChangeOwners) - .for_crate(&crate_name) - .check(&parts, conn)?; - let user = auth.user(); // The set of emails to send out after invite processing is complete and From 395a3be264a6905294062256c3fab0a3178c1bff Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 23:51:20 +0800 Subject: [PATCH 08/13] controllers/user/me: Change to use `async_check()` --- src/controllers/user/me.rs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 06aafedf705..36804c9d25a 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -21,14 +21,16 @@ use crate::views::{EncodableMe, EncodablePrivateUser, EncodableVersion, OwnedCra /// Handles the `GET /me` route. pub async fn me(app: AppState, req: Parts) -> AppResult> { - let conn = app.db_read_prefer_primary().await?; + let mut conn = app.db_read_prefer_primary().await?; + let user_id = AuthCheck::only_cookie() + .async_check(&req, &mut conn) + .await? + .user_id(); spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let user_id = AuthCheck::only_cookie().check(&req, conn)?.user_id(); - let (user, verified, email, verification_sent): (User, Option, Option, bool) = users::table .find(user_id) @@ -67,11 +69,13 @@ pub async fn me(app: AppState, req: Parts) -> AppResult> { /// Handles the `GET /me/updates` route. pub async fn updates(app: AppState, req: Parts) -> AppResult> { - let conn = app.db_read_prefer_primary().await?; + let mut conn = app.db_read_prefer_primary().await?; + let auth = AuthCheck::only_cookie() + .async_check(&req, &mut conn) + .await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = AuthCheck::only_cookie().check(&req, conn)?; let user = auth.user(); let followed_crates = Follow::belonging_to(user).select(follows::crate_id); @@ -145,7 +149,11 @@ pub async fn update_email_notifications(app: AppState, req: BytesRequest) -> App .map(|c| (c.id, c.email_notifications)) .collect(); - let conn = app.db_write().await?; + let mut conn = app.db_write().await?; + let user_id = AuthCheck::default() + .async_check(&parts, &mut conn) + .await? + .user_id(); spawn_blocking(move || { use diesel::RunQueryDsl; @@ -153,8 +161,6 @@ pub async fn update_email_notifications(app: AppState, req: BytesRequest) -> App use diesel::pg::upsert::excluded; - let user_id = AuthCheck::default().check(&parts, conn)?.user_id(); - // Build inserts from existing crates belonging to the current user let to_insert = CrateOwner::by_owner_kind(OwnerKind::User) .filter(crate_owners::owner_id.eq(user_id)) From 862312ff4591dd183bd57e421afbb6d75a06bbfd Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 23:51:29 +0800 Subject: [PATCH 09/13] controllers/user/update: Change to use `async_check()` --- src/controllers/user/update.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/controllers/user/update.rs b/src/controllers/user/update.rs index deba7359579..147564b5541 100644 --- a/src/controllers/user/update.rs +++ b/src/controllers/user/update.rs @@ -33,13 +33,13 @@ pub async fn update_user( req: Parts, Json(user_update): Json, ) -> AppResult { - let conn = state.db_write().await?; + let mut conn = state.db_write().await?; + let auth = AuthCheck::default().async_check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = AuthCheck::default().check(&req, conn)?; let user = auth.user(); // need to check if current user matches user to be updated @@ -120,13 +120,13 @@ pub async fn regenerate_token_and_send( Path(param_user_id): Path, req: Parts, ) -> AppResult { - let conn = state.db_write().await?; + let mut conn = state.db_write().await?; + let auth = AuthCheck::default().async_check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = AuthCheck::default().check(&req, conn)?; let user = auth.user(); // need to check if current user matches user to be updated From 5ef9e7c529cbff4a1d73ae7f4557af1e99d52222 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 23:59:37 +0800 Subject: [PATCH 10/13] controllers/token: Change to use `async_check()` --- src/controllers/token.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/controllers/token.rs b/src/controllers/token.rs index 5b17c3de40d..665a9500928 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -41,13 +41,15 @@ pub async fn list( Query(params): Query, req: Parts, ) -> AppResult> { - let conn = app.db_read_prefer_primary().await?; + let mut conn = app.db_read_prefer_primary().await?; + let auth = AuthCheck::only_cookie() + .async_check(&req, &mut conn) + .await?; spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = AuthCheck::only_cookie().check(&req, conn)?; let user = auth.user(); let tokens: Vec = ApiToken::belonging_to(user) @@ -92,13 +94,13 @@ pub async fn new( return Err(bad_request("name must have a value")); } - let conn = app.db_write().await?; + let mut conn = app.db_write().await?; + let auth = AuthCheck::default().async_check(&parts, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = AuthCheck::default().check(&parts, conn)?; if auth.api_token_id().is_some() { return Err(bad_request( "cannot use an API token to create a new API token", @@ -175,13 +177,13 @@ pub async fn new( /// Handles the `GET /me/tokens/:id` route. pub async fn show(app: AppState, Path(id): Path, req: Parts) -> AppResult> { - let conn = app.db_write().await?; + let mut conn = app.db_write().await?; + let auth = AuthCheck::default().async_check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = AuthCheck::default().check(&req, conn)?; let user = auth.user(); let token = ApiToken::belonging_to(user) .find(id) @@ -195,13 +197,13 @@ pub async fn show(app: AppState, Path(id): Path, req: Parts) -> AppResult, req: Parts) -> AppResult> { - let conn = app.db_write().await?; + let mut conn = app.db_write().await?; + let auth = AuthCheck::default().async_check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - 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)) @@ -214,13 +216,13 @@ pub async fn revoke(app: AppState, Path(id): Path, req: Parts) -> AppResult /// Handles the `DELETE /tokens/current` route. pub async fn revoke_current(app: AppState, req: Parts) -> AppResult { - let conn = app.db_write().await?; + let mut conn = app.db_write().await?; + let auth = AuthCheck::default().async_check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = AuthCheck::default().check(&req, conn)?; let api_token_id = auth .api_token_id() .ok_or_else(|| bad_request("token not provided"))?; From 9fe0f5e60c63f3b80f75c481a7bd68d1104a17cf Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 7 Nov 2024 23:50:52 +0800 Subject: [PATCH 11/13] controllers/crate_owner_invitation: Change to use `async_check()` --- src/controllers/crate_owner_invitation.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index 09cc57cfddb..c94aaee4605 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/src/controllers/crate_owner_invitation.rs @@ -27,11 +27,13 @@ use tokio::runtime::Handle; /// Handles the `GET /api/v1/me/crate_owner_invitations` route. pub async fn list(app: AppState, req: Parts) -> AppResult> { - let conn = app.db_read().await?; + let mut conn = app.db_read().await?; + let auth = AuthCheck::only_cookie() + .async_check(&req, &mut conn) + .await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - let auth = AuthCheck::only_cookie().check(&req, conn)?; let user_id = auth.user_id(); let PrivateListResponse { @@ -69,12 +71,13 @@ pub async fn list(app: AppState, req: Parts) -> AppResult> { /// Handles the `GET /api/private/crate_owner_invitations` route. pub async fn private_list(app: AppState, req: Parts) -> AppResult> { - let conn = app.db_read().await?; + let mut conn = app.db_read().await?; + let auth = AuthCheck::only_cookie() + .async_check(&req, &mut conn) + .await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - 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()) } else if let Some(id) = req.query().get("invitee_id").and_then(|i| i.parse().ok()) { @@ -284,11 +287,11 @@ pub async fn handle_invite(state: AppState, req: BytesRequest) -> AppResult = &mut conn.into(); - let auth = AuthCheck::default().check(&parts, conn)?; let user_id = auth.user_id(); let config = &state.config; From 4d0447418a39dd23fd05362c0dbe92b61f468c39 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Fri, 8 Nov 2024 00:14:35 +0800 Subject: [PATCH 12/13] auth: Remove obsolete blocking fns --- src/auth.rs | 124 ---------------------------------------------------- 1 file changed, 124 deletions(-) diff --git a/src/auth.rs b/src/auth.rs index 3b4f1fd4a9a..8be2ccccea0 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -4,7 +4,6 @@ use crate::middleware::log_request::RequestLogExt; use crate::middleware::session::RequestSession; use crate::models::token::{CrateScope, EndpointScope}; use crate::models::{ApiToken, User}; -use crate::util::diesel::Conn; use crate::util::errors::{ account_locked, forbidden, internal, AppResult, InsecurelyGeneratedTokenRevoked, }; @@ -58,43 +57,6 @@ impl AuthCheck { } } - #[instrument(name = "auth.check", skip_all)] - pub fn check(&self, parts: &Parts, conn: &mut impl Conn) -> AppResult { - let auth = authenticate(parts, conn)?; - - if let Some(token) = auth.api_token() { - if !self.allow_token { - let error_message = - "API Token authentication was explicitly disallowed for this API"; - parts.request_log().add("cause", error_message); - - return Err(forbidden( - "this action can only be performed on the crates.io website", - )); - } - - if !self.endpoint_scope_matches(token.endpoint_scopes.as_ref()) { - let error_message = "Endpoint scope mismatch"; - parts.request_log().add("cause", error_message); - - return Err(forbidden( - "this token does not have the required permissions to perform this action", - )); - } - - if !self.crate_scope_matches(token.crate_scopes.as_ref()) { - let error_message = "Crate scope mismatch"; - parts.request_log().add("cause", error_message); - - return Err(forbidden( - "this token does not have the required permissions to perform this action", - )); - } - } - - Ok(auth) - } - #[instrument(name = "auth.async_check", skip_all)] pub async fn async_check( &self, @@ -209,32 +171,6 @@ impl Authentication { } } -#[instrument(skip_all)] -fn authenticate_via_cookie( - parts: &Parts, - conn: &mut impl Conn, -) -> AppResult> { - let user_id_from_session = parts - .session() - .get("user_id") - .and_then(|s| s.parse::().ok()); - - let Some(id) = user_id_from_session else { - return Ok(None); - }; - - let user = User::find(conn, id).map_err(|err| { - parts.request_log().add("cause", err); - internal("user_id from cookie not found in database") - })?; - - ensure_not_locked(&user)?; - - parts.request_log().add("uid", id); - - Ok(Some(CookieAuthentication { user })) -} - #[instrument(skip_all)] async fn async_authenticate_via_cookie( parts: &Parts, @@ -261,43 +197,6 @@ async fn async_authenticate_via_cookie( Ok(Some(CookieAuthentication { user })) } -#[instrument(skip_all)] -fn authenticate_via_token( - parts: &Parts, - conn: &mut impl Conn, -) -> AppResult> { - let maybe_authorization = parts - .headers() - .get(header::AUTHORIZATION) - .and_then(|h| h.to_str().ok()); - - let Some(header_value) = maybe_authorization else { - return Ok(None); - }; - - let token = - HashedToken::parse(header_value).map_err(|_| InsecurelyGeneratedTokenRevoked::boxed())?; - - let token = ApiToken::find_by_api_token(conn, &token).map_err(|e| { - let cause = format!("invalid token caused by {e}"); - parts.request_log().add("cause", cause); - - forbidden("authentication failed") - })?; - - let user = User::find(conn, token.user_id).map_err(|err| { - parts.request_log().add("cause", err); - internal("user_id from token not found in database") - })?; - - ensure_not_locked(&user)?; - - parts.request_log().add("uid", token.user_id); - parts.request_log().add("tokenid", token.id); - - Ok(Some(TokenAuthentication { user, token })) -} - #[instrument(skip_all)] async fn async_authenticate_via_token( parts: &Parts, @@ -337,29 +236,6 @@ async fn async_authenticate_via_token( Ok(Some(TokenAuthentication { user, token })) } -#[instrument(skip_all)] -fn authenticate(parts: &Parts, conn: &mut impl Conn) -> AppResult { - controllers::util::verify_origin(parts)?; - - match authenticate_via_cookie(parts, conn) { - Ok(None) => {} - Ok(Some(auth)) => return Ok(Authentication::Cookie(auth)), - Err(err) => return Err(err), - } - - match authenticate_via_token(parts, conn) { - Ok(None) => {} - Ok(Some(auth)) => return Ok(Authentication::Token(auth)), - Err(err) => return Err(err), - } - - // Unable to authenticate the user - let cause = "no cookie session or auth header found"; - parts.request_log().add("cause", cause); - - return Err(forbidden("this action requires authentication")); -} - #[instrument(skip_all)] async fn async_authenticate( parts: &Parts, From 5fcdaf46dad779b98ce87986cd454d3586ae41b1 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Fri, 8 Nov 2024 00:20:00 +0800 Subject: [PATCH 13/13] auth: Remove obsolete `async_` prefixes from fns We don't use blocking check implementations anymore, so we remove these temporary prefixes. --- src/auth.rs | 19 ++++++++----------- src/controllers/crate_owner_invitation.rs | 10 +++------- src/controllers/krate/follow.rs | 12 +++--------- src/controllers/krate/owners.rs | 2 +- src/controllers/krate/publish.rs | 2 +- src/controllers/krate/search.rs | 2 +- src/controllers/token.rs | 12 +++++------- src/controllers/user/me.rs | 8 +++----- src/controllers/user/update.rs | 4 ++-- src/controllers/version/metadata.rs | 2 +- 10 files changed, 28 insertions(+), 45 deletions(-) diff --git a/src/auth.rs b/src/auth.rs index 8be2ccccea0..755f7110eac 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -57,13 +57,13 @@ impl AuthCheck { } } - #[instrument(name = "auth.async_check", skip_all)] - pub async fn async_check( + #[instrument(name = "auth.check", skip_all)] + pub async fn check( &self, parts: &Parts, conn: &mut AsyncPgConnection, ) -> AppResult { - let auth = async_authenticate(parts, conn).await?; + let auth = authenticate(parts, conn).await?; if let Some(token) = auth.api_token() { if !self.allow_token { @@ -172,7 +172,7 @@ impl Authentication { } #[instrument(skip_all)] -async fn async_authenticate_via_cookie( +async fn authenticate_via_cookie( parts: &Parts, conn: &mut AsyncPgConnection, ) -> AppResult> { @@ -198,7 +198,7 @@ async fn async_authenticate_via_cookie( } #[instrument(skip_all)] -async fn async_authenticate_via_token( +async fn authenticate_via_token( parts: &Parts, conn: &mut AsyncPgConnection, ) -> AppResult> { @@ -237,19 +237,16 @@ async fn async_authenticate_via_token( } #[instrument(skip_all)] -async fn async_authenticate( - parts: &Parts, - conn: &mut AsyncPgConnection, -) -> AppResult { +async fn authenticate(parts: &Parts, conn: &mut AsyncPgConnection) -> AppResult { controllers::util::verify_origin(parts)?; - match async_authenticate_via_cookie(parts, conn).await { + match authenticate_via_cookie(parts, conn).await { Ok(None) => {} Ok(Some(auth)) => return Ok(Authentication::Cookie(auth)), Err(err) => return Err(err), } - match async_authenticate_via_token(parts, conn).await { + match authenticate_via_token(parts, conn).await { Ok(None) => {} Ok(Some(auth)) => return Ok(Authentication::Token(auth)), Err(err) => return Err(err), diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index c94aaee4605..cffb8d7ad15 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/src/controllers/crate_owner_invitation.rs @@ -28,9 +28,7 @@ use tokio::runtime::Handle; /// Handles the `GET /api/v1/me/crate_owner_invitations` route. pub async fn list(app: AppState, req: Parts) -> AppResult> { let mut conn = app.db_read().await?; - let auth = AuthCheck::only_cookie() - .async_check(&req, &mut conn) - .await?; + let auth = AuthCheck::only_cookie().check(&req, &mut conn).await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); @@ -72,9 +70,7 @@ pub async fn list(app: AppState, req: Parts) -> AppResult> { /// Handles the `GET /api/private/crate_owner_invitations` route. pub async fn private_list(app: AppState, req: Parts) -> AppResult> { let mut conn = app.db_read().await?; - let auth = AuthCheck::only_cookie() - .async_check(&req, &mut conn) - .await?; + let auth = AuthCheck::only_cookie().check(&req, &mut conn).await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); @@ -288,7 +284,7 @@ pub async fn handle_invite(state: AppState, req: BytesRequest) -> AppResult = &mut conn.into(); diff --git a/src/controllers/krate/follow.rs b/src/controllers/krate/follow.rs index 38a4220f13b..9d1c6447cd7 100644 --- a/src/controllers/krate/follow.rs +++ b/src/controllers/krate/follow.rs @@ -35,10 +35,7 @@ pub async fn follow( req: Parts, ) -> AppResult { let mut conn = app.db_write().await?; - let user_id = AuthCheck::default() - .async_check(&req, &mut conn) - .await? - .user_id(); + let user_id = AuthCheck::default().check(&req, &mut conn).await?.user_id(); spawn_blocking(move || { use diesel::RunQueryDsl; @@ -62,10 +59,7 @@ pub async fn unfollow( req: Parts, ) -> AppResult { let mut conn = app.db_write().await?; - let user_id = AuthCheck::default() - .async_check(&req, &mut conn) - .await? - .user_id(); + let user_id = AuthCheck::default().check(&req, &mut conn).await?.user_id(); spawn_blocking(move || { use diesel::RunQueryDsl; @@ -87,7 +81,7 @@ pub async fn following( ) -> AppResult> { let mut conn = app.db_read_prefer_primary().await?; let user_id = AuthCheck::only_cookie() - .async_check(&req, &mut conn) + .check(&req, &mut conn) .await? .user_id(); spawn_blocking(move || { diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 245d3b0e6d6..179643a81e3 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -134,7 +134,7 @@ async fn modify_owners( let auth = AuthCheck::default() .with_endpoint_scope(EndpointScope::ChangeOwners) .for_crate(&crate_name) - .async_check(&parts, &mut conn) + .check(&parts, &mut conn) .await?; spawn_blocking(move || { use diesel::RunQueryDsl; diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 4e7367a3c94..9b1cf49e7d6 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -102,7 +102,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult FilterParams<'a> { } let user_id = Handle::current() - .block_on(AuthCheck::default().async_check(req, conn))? + .block_on(AuthCheck::default().check(req, conn))? .user_id(); // This should not fail, because of the `get()` check above diff --git a/src/controllers/token.rs b/src/controllers/token.rs index 665a9500928..2cef5883c96 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -42,9 +42,7 @@ pub async fn list( req: Parts, ) -> AppResult> { let mut conn = app.db_read_prefer_primary().await?; - let auth = AuthCheck::only_cookie() - .async_check(&req, &mut conn) - .await?; + let auth = AuthCheck::only_cookie().check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; @@ -95,7 +93,7 @@ pub async fn new( } let mut conn = app.db_write().await?; - let auth = AuthCheck::default().async_check(&parts, &mut conn).await?; + let auth = AuthCheck::default().check(&parts, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; @@ -178,7 +176,7 @@ pub async fn new( /// Handles the `GET /me/tokens/:id` route. pub async fn show(app: AppState, Path(id): Path, req: Parts) -> AppResult> { let mut conn = app.db_write().await?; - let auth = AuthCheck::default().async_check(&req, &mut conn).await?; + let auth = AuthCheck::default().check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; @@ -198,7 +196,7 @@ pub async fn show(app: AppState, Path(id): Path, req: Parts) -> AppResult, req: Parts) -> AppResult> { let mut conn = app.db_write().await?; - let auth = AuthCheck::default().async_check(&req, &mut conn).await?; + let auth = AuthCheck::default().check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; @@ -217,7 +215,7 @@ pub async fn revoke(app: AppState, Path(id): Path, req: Parts) -> AppResult /// Handles the `DELETE /tokens/current` route. pub async fn revoke_current(app: AppState, req: Parts) -> AppResult { let mut conn = app.db_write().await?; - let auth = AuthCheck::default().async_check(&req, &mut conn).await?; + let auth = AuthCheck::default().check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 36804c9d25a..dc4fb71f389 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -23,7 +23,7 @@ use crate::views::{EncodableMe, EncodablePrivateUser, EncodableVersion, OwnedCra pub async fn me(app: AppState, req: Parts) -> AppResult> { let mut conn = app.db_read_prefer_primary().await?; let user_id = AuthCheck::only_cookie() - .async_check(&req, &mut conn) + .check(&req, &mut conn) .await? .user_id(); spawn_blocking(move || { @@ -70,9 +70,7 @@ pub async fn me(app: AppState, req: Parts) -> AppResult> { /// Handles the `GET /me/updates` route. pub async fn updates(app: AppState, req: Parts) -> AppResult> { let mut conn = app.db_read_prefer_primary().await?; - let auth = AuthCheck::only_cookie() - .async_check(&req, &mut conn) - .await?; + let auth = AuthCheck::only_cookie().check(&req, &mut conn).await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); @@ -151,7 +149,7 @@ pub async fn update_email_notifications(app: AppState, req: BytesRequest) -> App let mut conn = app.db_write().await?; let user_id = AuthCheck::default() - .async_check(&parts, &mut conn) + .check(&parts, &mut conn) .await? .user_id(); spawn_blocking(move || { diff --git a/src/controllers/user/update.rs b/src/controllers/user/update.rs index 147564b5541..ca607eecfc0 100644 --- a/src/controllers/user/update.rs +++ b/src/controllers/user/update.rs @@ -34,7 +34,7 @@ pub async fn update_user( Json(user_update): Json, ) -> AppResult { let mut conn = state.db_write().await?; - let auth = AuthCheck::default().async_check(&req, &mut conn).await?; + let auth = AuthCheck::default().check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; @@ -121,7 +121,7 @@ pub async fn regenerate_token_and_send( req: Parts, ) -> AppResult { let mut conn = state.db_write().await?; - let auth = AuthCheck::default().async_check(&req, &mut conn).await?; + let auth = AuthCheck::default().check(&req, &mut conn).await?; spawn_blocking(move || { use diesel::RunQueryDsl; diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index e91297d692d..2ae82dc3030 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -176,7 +176,7 @@ pub async fn authenticate( AuthCheck::default() .with_endpoint_scope(EndpointScope::Yank) .for_crate(name) - .async_check(req, conn) + .check(req, conn) .await }