From ff2b568e5f2734c9340bdad8a1e4c255f21fa41f Mon Sep 17 00:00:00 2001 From: Rustin170506 <29879298+Rustin170506@users.noreply.github.com> Date: Fri, 6 Sep 2024 23:05:05 +0800 Subject: [PATCH 1/5] Add `PATCH /crates/:crate/:version` route Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com> --- src/controllers/version/metadata.rs | 162 +++++++++++++++++++++++++++- src/controllers/version/yank.rs | 68 +----------- src/router.rs | 2 +- 3 files changed, 165 insertions(+), 67 deletions(-) diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index 1588648b97..9f18c6712f 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -6,17 +6,43 @@ use axum::extract::Path; use axum::Json; +use crates_io_worker::BackgroundJob; +use diesel::{ + BoolExpressionMethods, ExpressionMethods, PgExpressionMethods, QueryDsl, RunQueryDsl, +}; use diesel_async::async_connection_wrapper::AsyncConnectionWrapper; +use http::request::Parts; +use http::StatusCode; +use serde::Deserialize; use serde_json::Value; +use tokio::runtime::Handle; use crate::app::AppState; -use crate::models::VersionOwnerAction; +use crate::auth::AuthCheck; +use crate::models::token::EndpointScope; +use crate::models::{ + insert_version_owner_action, Crate, Rights, Version, VersionAction, VersionOwnerAction, +}; +use crate::rate_limiter::LimitedAction; +use crate::schema::versions; use crate::tasks::spawn_blocking; -use crate::util::errors::{version_not_found, AppResult}; +use crate::util::diesel::Conn; +use crate::util::errors::{bad_request, custom, version_not_found, AppResult}; use crate::views::{EncodableDependency, EncodableVersion}; +use crate::worker::jobs::{self, UpdateDefaultVersion}; use super::version_and_crate; +#[derive(Deserialize)] +pub struct VersionUpdate { + yanked: Option, + yank_message: Option, +} +#[derive(Deserialize)] +pub struct VersionUpdateRequest { + version: VersionUpdate, +} + /// Handles the `GET /crates/:crate_id/:version/dependencies` route. /// /// This information can be obtained directly from the index. @@ -84,3 +110,135 @@ pub async fn show( }) .await } + +/// Handles the `PATCH /crates/:crate/:version` route. +/// +/// This endpoint allows updating the yanked state of a version, including a yank message. +pub async fn update( + state: AppState, + Path((crate_name, version)): Path<(String, String)>, + req: Parts, + Json(update_request): Json, +) -> AppResult> { + if semver::Version::parse(&version).is_err() { + return Err(version_not_found(&crate_name, &version)); + } + + let conn = state.db_write().await?; + spawn_blocking(move || { + let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); + let (mut version, krate) = version_and_crate(conn, &crate_name, &version)?; + + validate_yank_update(&update_request.version, &version)?; + perform_version_yank_update( + &state, + &req, + conn, + &mut version, + &krate, + update_request.version.yanked, + update_request.version.yank_message, + )?; + + let published_by = version.published_by(conn); + let actions = VersionOwnerAction::by_version(conn, &version)?; + let updated_version = EncodableVersion::from(version, &krate.name, published_by, actions); + Ok(Json(json!({ "version": updated_version }))) + }) + .await +} + +fn validate_yank_update(update_data: &VersionUpdate, version: &Version) -> AppResult<()> { + match (update_data.yanked, &update_data.yank_message) { + (Some(false), Some(_)) => { + return Err(bad_request("Cannot set yank message when unyanking")); + } + (None, Some(_)) => { + if !version.yanked { + return Err(bad_request( + "Cannot update yank message for a version that is not yanked", + )); + } + } + _ => {} + } + Ok(()) +} + +pub fn perform_version_yank_update( + state: &AppState, + req: &Parts, + conn: &mut impl Conn, + version: &mut Version, + krate: &Crate, + yanked: Option, + yank_message: Option, +) -> AppResult<()> { + 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)?; + + 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 version.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", + )); + } + } + + let yanked = yanked.unwrap_or(version.yanked); + // Check if the yanked state or yank message has changed and update if necessary + let updated_cnt = diesel::update( + versions::table.find(version.id).filter( + versions::yanked + .is_distinct_from(yanked) + .or(versions::yank_message.is_distinct_from(&yank_message)), + ), + ) + .set(( + versions::yanked.eq(yanked), + versions::yank_message.eq(&yank_message), + )) + .execute(conn)?; + + // If no rows were updated, return early + if updated_cnt == 0 { + return Ok(()); + } + + // Apply the update to the version + version.yanked = yanked; + version.yank_message = yank_message; + + let action = if version.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)?; + + Ok(()) +} diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index 59226522d2..eb85695d6b 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,8 @@ 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)?; + perform_version_yank_update(&state, &req, conn, &mut version, &krate, Some(yanked), None)?; ok_true() }) .await diff --git a/src/router.rs b/src/router.rs index 397e68f8e2..b74cce82a6 100644 --- a/src/router.rs +++ b/src/router.rs @@ -45,7 +45,7 @@ pub fn build_axum_router(state: AppState) -> Router<()> { .route("/api/v1/crates/:crate_id", get(krate::metadata::show)) .route( "/api/v1/crates/:crate_id/:version", - get(version::metadata::show), + get(version::metadata::show).patch(version::metadata::update), ) .route( "/api/v1/crates/:crate_id/:version/readme", From 5790f16950285b5f0457719d587fe0ee8bd863c5 Mon Sep 17 00:00:00 2001 From: Rustin170506 <29879298+Rustin170506@users.noreply.github.com> Date: Tue, 10 Sep 2024 23:36:22 +0800 Subject: [PATCH 2/5] Add unit tests for the patch API --- ..._yanking__patch_version_yank_unyank-2.snap | 62 ++++++++++++++ ..._yanking__patch_version_yank_unyank-3.snap | 73 ++++++++++++++++ ..._yanking__patch_version_yank_unyank-4.snap | 73 ++++++++++++++++ ..._yanking__patch_version_yank_unyank-5.snap | 84 +++++++++++++++++++ ..._yanking__patch_version_yank_unyank-6.snap | 84 +++++++++++++++++++ ...e__yanking__patch_version_yank_unyank.snap | 62 ++++++++++++++ src/tests/krate/yanking.rs | 67 +++++++++++++++ src/tests/mod.rs | 2 +- .../routes/crates/versions/yank_unyank.rs | 34 +++++++- src/tests/util.rs | 14 ++++ 10 files changed, 553 insertions(+), 2 deletions(-) create mode 100644 src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-2.snap create mode 100644 src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-3.snap create mode 100644 src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-4.snap create mode 100644 src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-5.snap create mode 100644 src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-6.snap create mode 100644 src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank.snap diff --git a/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-2.snap b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-2.snap new file mode 100644 index 0000000000..af1afffa98 --- /dev/null +++ b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-2.snap @@ -0,0 +1,62 @@ +--- +source: src/tests/krate/yanking.rs +expression: json +--- +{ + "version": { + "id": 1, + "crate": "patchable", + "num": "1.0.0", + "dl_path": "/api/v1/crates/patchable/1.0.0/download", + "readme_path": "/api/v1/crates/patchable/1.0.0/readme", + "updated_at": "[datetime]", + "created_at": "[datetime]", + "downloads": 0, + "features": {}, + "yanked": true, + "yank_message": "Yanking reason", + "lib_links": null, + "license": "MIT", + "links": { + "dependencies": "/api/v1/crates/patchable/1.0.0/dependencies", + "version_downloads": "/api/v1/crates/patchable/1.0.0/downloads", + "authors": "/api/v1/crates/patchable/1.0.0/authors" + }, + "crate_size": 151, + "published_by": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "audit_actions": [ + { + "action": "publish", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + } + ], + "checksum": "ddfc395ab340f413ee1d1ed0afce51a7c9df1c99c551fed5aef76edd4abe4048", + "rust_version": null, + "has_lib": false, + "bin_names": [] + } +} diff --git a/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-3.snap b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-3.snap new file mode 100644 index 0000000000..455b8364ca --- /dev/null +++ b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-3.snap @@ -0,0 +1,73 @@ +--- +source: src/tests/krate/yanking.rs +expression: json +--- +{ + "version": { + "id": 1, + "crate": "patchable", + "num": "1.0.0", + "dl_path": "/api/v1/crates/patchable/1.0.0/download", + "readme_path": "/api/v1/crates/patchable/1.0.0/readme", + "updated_at": "[datetime]", + "created_at": "[datetime]", + "downloads": 0, + "features": {}, + "yanked": true, + "yank_message": "Updated reason", + "lib_links": null, + "license": "MIT", + "links": { + "dependencies": "/api/v1/crates/patchable/1.0.0/dependencies", + "version_downloads": "/api/v1/crates/patchable/1.0.0/downloads", + "authors": "/api/v1/crates/patchable/1.0.0/authors" + }, + "crate_size": 151, + "published_by": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "audit_actions": [ + { + "action": "publish", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + } + ], + "checksum": "ddfc395ab340f413ee1d1ed0afce51a7c9df1c99c551fed5aef76edd4abe4048", + "rust_version": null, + "has_lib": false, + "bin_names": [] + } +} diff --git a/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-4.snap b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-4.snap new file mode 100644 index 0000000000..455b8364ca --- /dev/null +++ b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-4.snap @@ -0,0 +1,73 @@ +--- +source: src/tests/krate/yanking.rs +expression: json +--- +{ + "version": { + "id": 1, + "crate": "patchable", + "num": "1.0.0", + "dl_path": "/api/v1/crates/patchable/1.0.0/download", + "readme_path": "/api/v1/crates/patchable/1.0.0/readme", + "updated_at": "[datetime]", + "created_at": "[datetime]", + "downloads": 0, + "features": {}, + "yanked": true, + "yank_message": "Updated reason", + "lib_links": null, + "license": "MIT", + "links": { + "dependencies": "/api/v1/crates/patchable/1.0.0/dependencies", + "version_downloads": "/api/v1/crates/patchable/1.0.0/downloads", + "authors": "/api/v1/crates/patchable/1.0.0/authors" + }, + "crate_size": 151, + "published_by": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "audit_actions": [ + { + "action": "publish", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + } + ], + "checksum": "ddfc395ab340f413ee1d1ed0afce51a7c9df1c99c551fed5aef76edd4abe4048", + "rust_version": null, + "has_lib": false, + "bin_names": [] + } +} diff --git a/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-5.snap b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-5.snap new file mode 100644 index 0000000000..e441d6b90d --- /dev/null +++ b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-5.snap @@ -0,0 +1,84 @@ +--- +source: src/tests/krate/yanking.rs +expression: json +--- +{ + "version": { + "id": 1, + "crate": "patchable", + "num": "1.0.0", + "dl_path": "/api/v1/crates/patchable/1.0.0/download", + "readme_path": "/api/v1/crates/patchable/1.0.0/readme", + "updated_at": "[datetime]", + "created_at": "[datetime]", + "downloads": 0, + "features": {}, + "yanked": false, + "yank_message": null, + "lib_links": null, + "license": "MIT", + "links": { + "dependencies": "/api/v1/crates/patchable/1.0.0/dependencies", + "version_downloads": "/api/v1/crates/patchable/1.0.0/downloads", + "authors": "/api/v1/crates/patchable/1.0.0/authors" + }, + "crate_size": 151, + "published_by": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "audit_actions": [ + { + "action": "publish", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "unyank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + } + ], + "checksum": "ddfc395ab340f413ee1d1ed0afce51a7c9df1c99c551fed5aef76edd4abe4048", + "rust_version": null, + "has_lib": false, + "bin_names": [] + } +} diff --git a/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-6.snap b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-6.snap new file mode 100644 index 0000000000..e441d6b90d --- /dev/null +++ b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank-6.snap @@ -0,0 +1,84 @@ +--- +source: src/tests/krate/yanking.rs +expression: json +--- +{ + "version": { + "id": 1, + "crate": "patchable", + "num": "1.0.0", + "dl_path": "/api/v1/crates/patchable/1.0.0/download", + "readme_path": "/api/v1/crates/patchable/1.0.0/readme", + "updated_at": "[datetime]", + "created_at": "[datetime]", + "downloads": 0, + "features": {}, + "yanked": false, + "yank_message": null, + "lib_links": null, + "license": "MIT", + "links": { + "dependencies": "/api/v1/crates/patchable/1.0.0/dependencies", + "version_downloads": "/api/v1/crates/patchable/1.0.0/downloads", + "authors": "/api/v1/crates/patchable/1.0.0/authors" + }, + "crate_size": 151, + "published_by": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "audit_actions": [ + { + "action": "publish", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "unyank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + } + ], + "checksum": "ddfc395ab340f413ee1d1ed0afce51a7c9df1c99c551fed5aef76edd4abe4048", + "rust_version": null, + "has_lib": false, + "bin_names": [] + } +} diff --git a/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank.snap b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank.snap new file mode 100644 index 0000000000..af1afffa98 --- /dev/null +++ b/src/tests/krate/snapshots/crates_io__tests__krate__yanking__patch_version_yank_unyank.snap @@ -0,0 +1,62 @@ +--- +source: src/tests/krate/yanking.rs +expression: json +--- +{ + "version": { + "id": 1, + "crate": "patchable", + "num": "1.0.0", + "dl_path": "/api/v1/crates/patchable/1.0.0/download", + "readme_path": "/api/v1/crates/patchable/1.0.0/readme", + "updated_at": "[datetime]", + "created_at": "[datetime]", + "downloads": 0, + "features": {}, + "yanked": true, + "yank_message": "Yanking reason", + "lib_links": null, + "license": "MIT", + "links": { + "dependencies": "/api/v1/crates/patchable/1.0.0/dependencies", + "version_downloads": "/api/v1/crates/patchable/1.0.0/downloads", + "authors": "/api/v1/crates/patchable/1.0.0/authors" + }, + "crate_size": 151, + "published_by": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "audit_actions": [ + { + "action": "publish", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + }, + { + "action": "yank", + "user": { + "id": 1, + "login": "foo", + "name": null, + "avatar": null, + "url": "https://github.com/foo" + }, + "time": "[datetime]" + } + ], + "checksum": "ddfc395ab340f413ee1d1ed0afce51a7c9df1c99c551fed5aef76edd4abe4048", + "rust_version": null, + "has_lib": false, + "bin_names": [] + } +} diff --git a/src/tests/krate/yanking.rs b/src/tests/krate/yanking.rs index c8216355cd..1dc3aa8369 100644 --- a/src/tests/krate/yanking.rs +++ b/src/tests/krate/yanking.rs @@ -3,9 +3,11 @@ use crate::schema::publish_limit_buckets; use crate::tests::builders::PublishBuilder; use crate::tests::routes::crates::versions::yank_unyank::YankRequestHelper; use crate::tests::util::{RequestHelper, TestApp}; +use crate::tests::VersionResponse; use chrono::Utc; use diesel::{ExpressionMethods, RunQueryDsl}; use googletest::prelude::*; +use insta::assert_json_snapshot; use std::time::Duration; #[tokio::test(flavor = "multi_thread")] @@ -220,3 +222,68 @@ async fn publish_after_yank_max_version() { let json = anon.show_crate("fyk_max").await; assert_eq!(json.krate.max_version, "2.0.0"); } + +#[tokio::test(flavor = "multi_thread")] +async fn patch_version_yank_unyank() { + let (_, anon, _, token) = TestApp::full().with_token(); + + // Upload a new crate + let crate_to_publish = PublishBuilder::new("patchable", "1.0.0"); + token.publish_crate(crate_to_publish).await.good(); + + // Check initial state + let json = anon.show_version("patchable", "1.0.0").await; + assert!(!json.version.yanked); + assert_eq!(json.version.yank_message, None); + + let assert_json_helper = |json: VersionResponse| { + assert_json_snapshot!(json, { + ".version.created_at" => "[datetime]", + ".version.updated_at" => "[datetime]", + ".version.audit_actions[].time" => "[datetime]", + }); + }; + + // Yank with message + let response = token + .update_yank_status("patchable", "1.0.0", Some(true), Some("Yanking reason")) + .await + .good(); + assert_json_helper(response); + + let json = anon.show_version("patchable", "1.0.0").await; + assert_json_helper(json); + + // Update yank message + let response = token + .update_yank_status("patchable", "1.0.0", None, Some("Updated reason")) + .await + .good(); + assert_json_helper(response); + + let json = anon.show_version("patchable", "1.0.0").await; + assert_json_helper(json); + + // Unyank + let response = token + .update_yank_status("patchable", "1.0.0", Some(false), None) + .await + .good(); + assert_json_helper(response); + + let json = anon.show_version("patchable", "1.0.0").await; + assert_json_helper(json); + + // Attempt to set yank message on unyanked version (should fail) + token + .update_yank_status("patchable", "1.0.0", None, Some("Invalid message")) + .await + .status() + .is_client_error(); + // Attempt to unyank with message (should fail) + token + .update_yank_status("patchable", "1.0.0", Some(false), Some("Invalid message")) + .await + .status() + .is_client_error(); +} diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 7b7abe2c8a..1457059fad 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -56,7 +56,7 @@ pub struct CrateResponse { versions: Option>, keywords: Option>, } -#[derive(Deserialize)] +#[derive(Serialize, Deserialize)] pub struct VersionResponse { version: EncodableVersion, } diff --git a/src/tests/routes/crates/versions/yank_unyank.rs b/src/tests/routes/crates/versions/yank_unyank.rs index f802ddb652..2601138929 100644 --- a/src/tests/routes/crates/versions/yank_unyank.rs +++ b/src/tests/routes/crates/versions/yank_unyank.rs @@ -1,8 +1,9 @@ use crate::tests::builders::{CrateBuilder, PublishBuilder}; use crate::tests::util::{RequestHelper, Response, TestApp}; -use crate::tests::OkBool; +use crate::tests::{OkBool, VersionResponse}; use http::StatusCode; use insta::assert_snapshot; +use serde_json::json; pub trait YankRequestHelper { /// Yank the specified version of the specified crate and run all pending background jobs @@ -10,6 +11,15 @@ pub trait YankRequestHelper { /// Unyank the specified version of the specified crate and run all pending background jobs async fn unyank(&self, krate_name: &str, version: &str) -> Response; + + /// Update the yank status of the specified version of the specified crate with a patch request and run all pending background jobs + async fn update_yank_status( + &self, + krate_name: &str, + version: &str, + yanked: Option, + yank_message: Option<&str>, + ) -> Response; } impl YankRequestHelper for T { @@ -26,6 +36,28 @@ impl YankRequestHelper for T { self.app().run_pending_background_jobs().await; response } + + async fn update_yank_status( + &self, + krate_name: &str, + version: &str, + yanked: Option, + yank_message: Option<&str>, + ) -> Response { + let url = format!("/api/v1/crates/{krate_name}/{version}"); + + let json_body = json!({ + "version": { + "yanked": yanked, + "yank_message": yank_message + } + }); + let body = serde_json::to_string(&json_body).expect("Failed to serialize JSON body"); + + let response = self.patch(&url, body).await; + self.app().run_pending_background_jobs().await; + response + } } #[tokio::test(flavor = "multi_thread")] diff --git a/src/tests/util.rs b/src/tests/util.rs index b6b58432d8..aa096506dd 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -145,6 +145,20 @@ pub trait RequestHelper { self.run(request).await } + /// Issue a PATCH request + async fn patch(&self, path: &str, body: impl Into) -> Response { + let body = body.into(); + let is_json = body.starts_with(b"{") && body.ends_with(b"}"); + + let mut request = self.request_builder(Method::PATCH, path); + *request.body_mut() = body; + if is_json { + request.header(header::CONTENT_TYPE, "application/json"); + } + + self.run(request).await + } + /// Issue a DELETE request async fn delete(&self, path: &str) -> Response { let request = self.request_builder(Method::DELETE, path); From 3739c88899ab7cd362451c544b45c5376999afe6 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 30 Sep 2024 12:39:38 +0200 Subject: [PATCH 3/5] tests/krate/yanking: Fix status code assertions --- src/tests/krate/yanking.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/tests/krate/yanking.rs b/src/tests/krate/yanking.rs index 1dc3aa8369..f88de43cea 100644 --- a/src/tests/krate/yanking.rs +++ b/src/tests/krate/yanking.rs @@ -7,6 +7,7 @@ use crate::tests::VersionResponse; use chrono::Utc; use diesel::{ExpressionMethods, RunQueryDsl}; use googletest::prelude::*; +use http::StatusCode; use insta::assert_json_snapshot; use std::time::Duration; @@ -275,15 +276,14 @@ async fn patch_version_yank_unyank() { assert_json_helper(json); // Attempt to set yank message on unyanked version (should fail) - token + let response = token .update_yank_status("patchable", "1.0.0", None, Some("Invalid message")) - .await - .status() - .is_client_error(); + .await; + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + // Attempt to unyank with message (should fail) - token + let response = token .update_yank_status("patchable", "1.0.0", Some(false), Some("Invalid message")) - .await - .status() - .is_client_error(); + .await; + assert_eq!(response.status(), StatusCode::BAD_REQUEST); } From cccdd7451ceef8f8685f6e7f5691fa562742045c Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 30 Sep 2024 12:46:38 +0200 Subject: [PATCH 4/5] tests/krate/yanking: Add error response body assertions --- src/tests/krate/yanking.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tests/krate/yanking.rs b/src/tests/krate/yanking.rs index f88de43cea..854f2bb0b3 100644 --- a/src/tests/krate/yanking.rs +++ b/src/tests/krate/yanking.rs @@ -8,7 +8,7 @@ use chrono::Utc; use diesel::{ExpressionMethods, RunQueryDsl}; use googletest::prelude::*; use http::StatusCode; -use insta::assert_json_snapshot; +use insta::{assert_json_snapshot, assert_snapshot}; use std::time::Duration; #[tokio::test(flavor = "multi_thread")] @@ -280,10 +280,12 @@ async fn patch_version_yank_unyank() { .update_yank_status("patchable", "1.0.0", None, Some("Invalid message")) .await; assert_eq!(response.status(), StatusCode::BAD_REQUEST); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Cannot update yank message for a version that is not yanked"}]}"#); // Attempt to unyank with message (should fail) let response = token .update_yank_status("patchable", "1.0.0", Some(false), Some("Invalid message")) .await; assert_eq!(response.status(), StatusCode::BAD_REQUEST); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Cannot set yank message when unyanking"}]}"#); } From 45ec3fb3a48423334226be69ce96ae04b0bf9cb2 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 30 Sep 2024 13:18:32 +0200 Subject: [PATCH 5/5] controllers/version/metadata: Fix `yanked` usage This was using the previous `yanked` state for the admin log message, instead of the new state. --- src/controllers/version/metadata.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index 9f18c6712f..41e24edd06 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -187,13 +187,11 @@ pub fn perform_version_yank_update( let user = auth.user(); let owners = krate.owners(conn)?; + let yanked = yanked.unwrap_or(version.yanked); + if Handle::current().block_on(user.rights(state, &owners))? < Rights::Publish { if user.is_admin { - let action = if version.yanked { - "yanking" - } else { - "unyanking" - }; + let action = if yanked { "yanking" } else { "unyanking" }; warn!( "Admin {} is {action} {}@{}", user.gh_login, krate.name, version.num @@ -206,7 +204,6 @@ pub fn perform_version_yank_update( } } - let yanked = yanked.unwrap_or(version.yanked); // Check if the yanked state or yank message has changed and update if necessary let updated_cnt = diesel::update( versions::table.find(version.id).filter( @@ -230,7 +227,7 @@ pub fn perform_version_yank_update( version.yanked = yanked; version.yank_message = yank_message; - let action = if version.yanked { + let action = if yanked { VersionAction::Yank } else { VersionAction::Unyank