From 7dd8d49072e2bcf61223e7c6a10c21f185fa211e Mon Sep 17 00:00:00 2001 From: Rustin170506 <29879298+Rustin170506@users.noreply.github.com> Date: Wed, 11 Sep 2024 21:47:19 +0800 Subject: [PATCH] Reuse the code to perform version yank update Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com> --- src/controllers/version/metadata.rs | 94 +++++++++++++++++------------ src/controllers/version/yank.rs | 69 ++------------------- 2 files changed, 62 insertions(+), 101 deletions(-) diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index dee6b77ff42..791f141f03f 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -7,7 +7,7 @@ use axum::extract::Path; use axum::Json; use crates_io_worker::BackgroundJob; -use diesel::{ExpressionMethods, RunQueryDsl}; +use diesel::{ExpressionMethods, QueryDsl, RunQueryDsl}; use diesel_async::async_connection_wrapper::AsyncConnectionWrapper; use http::request::Parts; use http::StatusCode; @@ -142,14 +142,52 @@ fn apply_yank_update( ) -> AppResult<()> { // Try to update the yank state first, to avoid unnecessary checks. update_version_yank_state(version, update_data)?; + perform_version_yank_update(state, req, conn, version, krate)?; - // Add authentication check + Ok(()) +} + +fn update_version_yank_state(version: &mut Version, update_data: &VersionUpdate) -> AppResult<()> { + match (update_data.yanked, &update_data.yank_message) { + (Some(true), Some(message)) => { + version.yanked = true; + version.yank_message = Some(message.clone()); + } + (Some(yanked), None) => { + version.yanked = yanked; + version.yank_message = None; + } + (Some(false), Some(_)) => { + return Err(bad_request("Cannot set yank message when unyanking")); + } + (None, Some(message)) => { + if version.yanked { + version.yank_message = Some(message.clone()); + } else { + return Err(bad_request( + "Cannot update yank message for a version that is not yanked", + )); + } + } + // If both yanked and yank_message are None, do nothing. + // This function only cares about updating the yanked state and yank message. + (None, None) => {} + } + Ok(()) +} + +pub fn perform_version_yank_update( + state: &AppState, + req: &Parts, + conn: &mut impl Conn, + version: &Version, + krate: &Crate, +) -> AppResult<()> { let auth = AuthCheck::default() .with_endpoint_scope(EndpointScope::Yank) .for_crate(&krate.name) .check(req, conn)?; - // Add rate limiting check state .rate_limiter .check_rate_limit(auth.user_id(), LimitedAction::YankUnyank, conn)?; @@ -158,7 +196,6 @@ fn apply_yank_update( let user = auth.user(); let owners = krate.owners(conn)?; - // Check user rights if Handle::current().block_on(user.rights(state, &owners))? < Rights::Publish { if user.is_admin { warn!( @@ -168,19 +205,32 @@ fn apply_yank_update( } else { return Err(custom( StatusCode::FORBIDDEN, - "must already be an owner to update version", + "must already be an owner to yank or unyank", )); } } - diesel::update(&*version) + // Check if the yanked state or yank message has changed + let (yanked, yank_message) = crate::schema::versions::table + .find(version.id) + .select(( + crate::schema::versions::yanked, + crate::schema::versions::yank_message, + )) + .first::<(bool, Option)>(conn)?; + + if yanked == version.yanked && yank_message == version.yank_message { + // No changes, return early + return Ok(()); + } + + diesel::update(version) .set(( crate::schema::versions::yanked.eq(version.yanked), crate::schema::versions::yank_message.eq(&version.yank_message), )) .execute(conn)?; - // Add version owner action let action = if version.yanked { VersionAction::Yank } else { @@ -188,38 +238,8 @@ fn apply_yank_update( }; insert_version_owner_action(conn, version.id, user.id, api_token_id, action)?; - // Enqueue jobs jobs::enqueue_sync_to_index(&krate.name, conn)?; UpdateDefaultVersion::new(krate.id).enqueue(conn)?; Ok(()) } - -fn update_version_yank_state(version: &mut Version, update_data: &VersionUpdate) -> AppResult<()> { - match (update_data.yanked, &update_data.yank_message) { - (Some(true), Some(message)) => { - version.yanked = true; - version.yank_message = Some(message.clone()); - } - (Some(yanked), None) => { - version.yanked = yanked; - version.yank_message = None; - } - (Some(false), Some(_)) => { - return Err(bad_request("Cannot set yank message when unyanking")); - } - (None, Some(message)) => { - if version.yanked { - version.yank_message = Some(message.clone()); - } else { - return Err(bad_request( - "Cannot update yank message for a version that is not yanked", - )); - } - } - // If both yanked and yank_message are None, do nothing. - // This function only cares about updating the yanked state and yank message. - (None, None) => {} - } - Ok(()) -} diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index 59226522d20..9ed44910fd1 100644 --- a/src/controllers/version/yank.rs +++ b/src/controllers/version/yank.rs @@ -1,26 +1,15 @@ //! Endpoints for yanking and unyanking specific versions of crates +use super::metadata::perform_version_yank_update; use super::version_and_crate; use crate::app::AppState; -use crate::auth::AuthCheck; use crate::controllers::helpers::ok_true; -use crate::models::token::EndpointScope; -use crate::models::Rights; -use crate::models::{insert_version_owner_action, VersionAction}; -use crate::rate_limiter::LimitedAction; -use crate::schema::versions; use crate::tasks::spawn_blocking; -use crate::util::errors::{custom, version_not_found, AppResult}; -use crate::worker::jobs; -use crate::worker::jobs::UpdateDefaultVersion; +use crate::util::errors::{version_not_found, AppResult}; use axum::extract::Path; use axum::response::Response; -use crates_io_worker::BackgroundJob; -use diesel::prelude::*; use diesel_async::async_connection_wrapper::AsyncConnectionWrapper; use http::request::Parts; -use http::StatusCode; -use tokio::runtime::Handle; /// Handles the `DELETE /crates/:crate_id/:version/yank` route. /// This does not delete a crate version, it makes the crate @@ -66,57 +55,9 @@ async fn modify_yank( let conn = state.db_write().await?; spawn_blocking(move || { let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - - let auth = AuthCheck::default() - .with_endpoint_scope(EndpointScope::Yank) - .for_crate(&crate_name) - .check(&req, conn)?; - - state - .rate_limiter - .check_rate_limit(auth.user_id(), LimitedAction::YankUnyank, conn)?; - - let (version, krate) = version_and_crate(conn, &crate_name, &version)?; - let api_token_id = auth.api_token_id(); - let user = auth.user(); - let owners = krate.owners(conn)?; - - if Handle::current().block_on(user.rights(&state, &owners))? < Rights::Publish { - if user.is_admin { - let action = if yanked { "yanking" } else { "unyanking" }; - warn!( - "Admin {} is {action} {}@{}", - user.gh_login, krate.name, version.num - ); - } else { - return Err(custom( - StatusCode::FORBIDDEN, - "must already be an owner to yank or unyank", - )); - } - } - - if version.yanked == yanked { - // The crate is already in the state requested, nothing to do - return ok_true(); - } - - diesel::update(&version) - .set(versions::yanked.eq(yanked)) - .execute(conn)?; - - let action = if yanked { - VersionAction::Yank - } else { - VersionAction::Unyank - }; - - insert_version_owner_action(conn, version.id, user.id, api_token_id, action)?; - - jobs::enqueue_sync_to_index(&krate.name, conn)?; - - UpdateDefaultVersion::new(krate.id).enqueue(conn)?; - + let (mut version, krate) = version_and_crate(conn, &crate_name, &version)?; + version.yanked = yanked; + perform_version_yank_update(&state, &req, conn, &version, &krate)?; ok_true() }) .await