diff --git a/crates/crates_io_database/src/schema.rs b/crates/crates_io_database/src/schema.rs index 7cec766ca1e..1ca4af91f36 100644 --- a/crates/crates_io_database/src/schema.rs +++ b/crates/crates_io_database/src/schema.rs @@ -428,6 +428,8 @@ diesel::table! { message -> Nullable, /// Date and time when users will be able to create a new crate with the same name available_at -> Timestamptz, + /// The first version that can be used by a new crate with the same name + min_version -> Nullable, } } diff --git a/migrations/2024-11-20-031728_add-min-version-to-deleted-crates/down.sql b/migrations/2024-11-20-031728_add-min-version-to-deleted-crates/down.sql new file mode 100644 index 00000000000..1bfda5f84bd --- /dev/null +++ b/migrations/2024-11-20-031728_add-min-version-to-deleted-crates/down.sql @@ -0,0 +1,2 @@ +ALTER TABLE deleted_crates +DROP COLUMN min_version; diff --git a/migrations/2024-11-20-031728_add-min-version-to-deleted-crates/up.sql b/migrations/2024-11-20-031728_add-min-version-to-deleted-crates/up.sql new file mode 100644 index 00000000000..a108bbad69b --- /dev/null +++ b/migrations/2024-11-20-031728_add-min-version-to-deleted-crates/up.sql @@ -0,0 +1,4 @@ +ALTER TABLE deleted_crates +ADD COLUMN min_version VARCHAR NULL; + +COMMENT ON COLUMN deleted_crates.min_version IS 'The first version that can be used by a new crate with the same name'; diff --git a/src/bin/crates-admin/delete_crate.rs b/src/bin/crates-admin/delete_crate.rs index 77d6859283a..ef0e56665ab 100644 --- a/src/bin/crates-admin/delete_crate.rs +++ b/src/bin/crates-admin/delete_crate.rs @@ -2,7 +2,7 @@ use crate::dialoguer; use anyhow::Context; use chrono::{NaiveDateTime, Utc}; use colored::Colorize; -use crates_io::models::{NewDeletedCrate, User}; +use crates_io::models::{NewDeletedCrate, TopVersions, User}; use crates_io::schema::{crate_downloads, deleted_crates}; use crates_io::worker::jobs; use crates_io::{db, schema::crates}; @@ -86,6 +86,17 @@ pub async fn run(opts: Opts) -> anyhow::Result<()> { if let Some(crate_info) = existing_crates.iter().find(|info| info.name == *name) { let id = crate_info.id; + let min_version = crate_info.top_versions(&mut conn).await?.highest.map( + |semver::Version { major, minor, .. }| { + if major > 0 { + semver::Version::new(major + 1, 0, 0) + } else { + semver::Version::new(0, minor + 1, 0) + } + .to_string() + }, + ); + let created_at = crate_info.created_at.and_utc(); let deleted_crate = NewDeletedCrate::builder(name) .created_at(&created_at) @@ -93,6 +104,7 @@ pub async fn run(opts: Opts) -> anyhow::Result<()> { .deleted_by(deleted_by.id) .maybe_message(opts.message.as_deref()) .available_at(&available_at) + .maybe_min_version(min_version.as_deref()) .build(); info!("{name}: Deleting crate from the database…"); @@ -162,6 +174,20 @@ struct CrateInfo { rev_deps: i64, } +impl CrateInfo { + async fn top_versions(&self, conn: &mut AsyncPgConnection) -> QueryResult { + use crates_io_database::schema::versions::dsl::*; + + Ok(TopVersions::from_date_version_pairs( + versions + .filter(crate_id.eq(self.id)) + .select((created_at, num)) + .load(conn) + .await?, + )) + } +} + impl Display for CrateInfo { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let id = self.id; diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 32d8271e2f2..d3c4091b87c 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -9,7 +9,6 @@ use crate::worker::jobs::{ use axum::body::Bytes; use axum::Json; use cargo_manifest::{Dependency, DepsSet, TargetDepsSet}; -use chrono::{DateTime, SecondsFormat, Utc}; use crates_io_tarball::{process_tarball, TarballError}; use crates_io_worker::BackgroundJob; use diesel::connection::DefaultLoadingMode; @@ -42,6 +41,8 @@ use crate::views::{ EncodableCrate, EncodableCrateDependency, GoodCrate, PublishMetadata, PublishWarnings, }; +mod deleted; + const MISSING_RIGHTS_ERROR_MESSAGE: &str = "this crate exists but you don't seem to be an owner. \ If you believe this is a mistake, perhaps you need \ to accept an invitation to be an owner before \ @@ -87,21 +88,8 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult)> = deleted_crates::table - .filter(canon_crate_name(deleted_crates::name).eq(canon_crate_name(&metadata.name))) - .filter(deleted_crates::available_at.gt(Utc::now())) - .select((deleted_crates::name, deleted_crates::available_at)) - .first(&mut conn) - .await - .optional()?; - - if let Some(deleted_crate) = deleted_crate { - return Err(bad_request(format!( - "A crate with the name `{}` was recently deleted. Reuse of this name will be available after {}.", - deleted_crate.0, - deleted_crate.1.to_rfc3339_opts(SecondsFormat::Secs, true) - ))); - } + // Check that this publish doesn't conflict with any deleted crates. + deleted::validate(&mut conn, &metadata.name, &semver).await?; // this query should only be used for the endpoint scope calculation // since a race condition there would only cause `publish-new` instead of diff --git a/src/controllers/krate/publish/deleted.rs b/src/controllers/krate/publish/deleted.rs new file mode 100644 index 00000000000..436baa7609c --- /dev/null +++ b/src/controllers/krate/publish/deleted.rs @@ -0,0 +1,264 @@ +use chrono::{DateTime, SecondsFormat, Utc}; +use diesel_async::AsyncPgConnection; +use http::StatusCode; +use semver::Version; + +use crate::{ + schema::deleted_crates, + sql::canon_crate_name, + util::{ + diesel::prelude::*, + errors::{custom, AppResult}, + }, +}; + +/// Checks the given crate name and version against the deleted crates table, +/// ensuring that the crate version is allowed to be published. +/// +/// If the crate version cannot be published, a +/// [`crate::util::errors::BoxedAppError`] will be returned with a user-oriented +/// message suitable for being returned directly by a controller. +pub async fn validate( + conn: &mut AsyncPgConnection, + name: &str, + version: &Version, +) -> AppResult<()> { + use diesel_async::RunQueryDsl; + + // Since the same crate may have been deleted multiple times, we need to + // calculate the most restrictive set of conditions that the crate version + // being published must adhere to; specifically: the latest available_at + // time, and the highest min_version. + let mut state = State::default(); + + // To do this, we need to iterate over all the relevant deleted crates. + for (available_at, min_version) in deleted_crates::table + .filter(canon_crate_name(deleted_crates::name).eq(canon_crate_name(name))) + .select((deleted_crates::available_at, deleted_crates::min_version)) + .load::<(DateTime, Option)>(conn) + .await? + { + state.observe_available_at(available_at); + + // We shouldn't really end up with an invalid semver in the + // `min_version` field, so we're going to silently swallow any errors + // for now. + if let Some(Ok(min_version)) = min_version.map(|v| Version::parse(&v)) { + state.observe_min_version(min_version); + } + } + + // Finally, we can check the given name and version against the built up + // state and see if it passes. + state.into_result(name, version, Utc::now()) +} + +#[derive(Default)] +#[cfg_attr(test, derive(Clone))] +struct State { + available_at: Option>, + min_version: Option, +} + +impl State { + fn observe_available_at(&mut self, available_at: DateTime) { + if let Some(current) = self.available_at { + self.available_at = Some(std::cmp::max(current, available_at)); + } else { + self.available_at = Some(available_at); + } + } + + fn observe_min_version(&mut self, min_version: Version) { + if let Some(current) = self.min_version.take() { + self.min_version = Some(std::cmp::max(current, min_version)); + } else { + self.min_version = Some(min_version); + } + } + + fn into_result(self, name: &str, version: &Version, now: DateTime) -> AppResult<()> { + let mut messages = Vec::new(); + + if let Some(available_at) = self.available_at { + if now < available_at { + messages.push(format!( + "Reuse of this name will be available after {}.", + available_at.to_rfc3339_opts(SecondsFormat::Secs, true) + )); + } + } + + if let Some(min_version) = self.min_version { + if version < &min_version { + messages.push(format!("To avoid conflicts with previously published versions of this crate, the minimum version that can be published is {min_version}.")); + } + } + + if messages.is_empty() { + Ok(()) + } else { + Err(custom( + StatusCode::UNPROCESSABLE_ENTITY, + format!( + "A crate with the name `{name}` was previously deleted.\n\n* {}", + messages.join("\n* "), + ), + )) + } + } +} + +#[cfg(test)] +mod tests { + use chrono::TimeDelta; + use insta::assert_snapshot; + + use super::*; + + macro_rules! assert_result_status { + ($result:expr) => {{ + let response = $result.unwrap_err().response(); + assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY); + + String::from_utf8( + axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap() + .into(), + ) + .unwrap() + }}; + } + + macro_rules! assert_result_failed { + ($result:expr) => {{ + let text = assert_result_status!($result); + assert_snapshot!(text); + }}; + ($result:expr, $name:tt) => {{ + let text = assert_result_status!($result); + assert_snapshot!($name, text); + }}; + } + + #[test] + fn empty_state() { + let state = State::default(); + + // Any combination of values should result in Ok, since there are no + // deleted crates. + for (name, version, now) in [ + ("foo", "0.0.0", "2024-11-20T01:00:00Z"), + ("bar", "1.0.0", "1970-01-01T00:00:00Z"), + ] { + assert_ok!(state.clone().into_result( + name, + &Version::parse(version).unwrap(), + now.parse().unwrap() + )); + } + } + + #[tokio::test] + async fn available_at_only() { + let available_at = "2024-11-20T00:00:00Z".parse().unwrap(); + let version = Version::parse("0.0.0").unwrap(); + + let mut state = State::default(); + state.observe_available_at(available_at); + + // There should be no error for a crate after available_at. + assert_ok!(state.clone().into_result( + "foo", + &version, + available_at + TimeDelta::seconds(60) + )); + + // Similarly, a crate actually _at_ available_at should be fine. + assert_ok!(state.clone().into_result("foo", &version, available_at)); + + // But a crate one second earlier should error. + assert_result_failed!(state.into_result( + "foo", + &version, + available_at - TimeDelta::seconds(1) + )); + } + + #[tokio::test] + async fn min_version_only() { + let available_at = "2024-11-20T00:00:00Z".parse().unwrap(); + + let mut state = State::default(); + state.observe_available_at(available_at); + + // Test the versions that we expect to pass. + for (min_version, publish_version) in [ + ("0.1.0", "0.1.0"), + ("0.1.0", "0.1.1"), + ("0.1.0", "0.2.0"), + ("0.1.0", "1.0.0"), + ("1.0.0", "1.0.0"), + ("1.0.0", "1.0.1"), + ("1.0.0", "2.0.0"), + ] { + let mut state = state.clone(); + state.observe_min_version(Version::parse(min_version).unwrap()); + + assert_ok!(state.into_result( + "foo", + &Version::parse(publish_version).unwrap(), + available_at + )); + } + + // Now test the versions that we expect to fail. + for (min_version, publish_version) in [("0.1.0", "0.0.0"), ("1.0.0", "0.1.0")] { + let mut state = state.clone(); + state.observe_min_version(Version::parse(min_version).unwrap()); + + assert_result_failed!( + state.into_result( + "foo", + &Version::parse(publish_version).unwrap(), + available_at, + ), + publish_version + ); + } + } + + #[tokio::test] + async fn multiple_deleted() { + // We won't repeat everything from the above here, but let's make sure + // the most restrictive available_at and min_version are used when + // multiple deleted crates are observed. + let mut state = State::default(); + + let earlier_available_at = "2024-11-20T00:00:00Z".parse().unwrap(); + let later_available_at = "2024-11-21T12:00:00Z".parse().unwrap(); + state.observe_available_at(earlier_available_at); + state.observe_available_at(later_available_at); + state.observe_available_at(earlier_available_at); + + let first_version = Version::parse("0.1.0").unwrap(); + let second_version = Version::parse("1.0.0").unwrap(); + state.observe_min_version(first_version.clone()); + state.observe_min_version(second_version.clone()); + state.observe_min_version(first_version.clone()); + + assert_ok!(state + .clone() + .into_result("foo", &second_version, later_available_at)); + + // Now the bad cases. + for (name, version, now) in [ + ("min_version", &first_version, later_available_at), + ("available_at", &second_version, earlier_available_at), + ("both", &first_version, earlier_available_at), + ] { + assert_result_failed!(state.clone().into_result("foo", version, now), name); + } + } +} diff --git a/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__0.0.0.snap b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__0.0.0.snap new file mode 100644 index 00000000000..17462b0deaf --- /dev/null +++ b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__0.0.0.snap @@ -0,0 +1,5 @@ +--- +source: src/controllers/krate/publish/deleted.rs +expression: text +--- +{"errors":[{"detail":"A crate with the name `foo` was previously deleted.\n\n* To avoid conflicts with previously published versions of this crate, the minimum version that can be published is 0.1.0."}]} diff --git a/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__0.1.0.snap b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__0.1.0.snap new file mode 100644 index 00000000000..a03f258f81f --- /dev/null +++ b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__0.1.0.snap @@ -0,0 +1,5 @@ +--- +source: src/controllers/krate/publish/deleted.rs +expression: text +--- +{"errors":[{"detail":"A crate with the name `foo` was previously deleted.\n\n* To avoid conflicts with previously published versions of this crate, the minimum version that can be published is 1.0.0."}]} diff --git a/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__available_at.snap b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__available_at.snap new file mode 100644 index 00000000000..ed6b371c626 --- /dev/null +++ b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__available_at.snap @@ -0,0 +1,5 @@ +--- +source: src/controllers/krate/publish/deleted.rs +expression: text +--- +{"errors":[{"detail":"A crate with the name `foo` was previously deleted.\n\n* Reuse of this name will be available after 2024-11-21T12:00:00Z."}]} diff --git a/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__available_at_only.snap b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__available_at_only.snap new file mode 100644 index 00000000000..29db0046c83 --- /dev/null +++ b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__available_at_only.snap @@ -0,0 +1,5 @@ +--- +source: src/controllers/krate/publish/deleted.rs +expression: text +--- +{"errors":[{"detail":"A crate with the name `foo` was previously deleted.\n\n* Reuse of this name will be available after 2024-11-20T00:00:00Z."}]} diff --git a/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__both.snap b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__both.snap new file mode 100644 index 00000000000..016f1173ec6 --- /dev/null +++ b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__both.snap @@ -0,0 +1,5 @@ +--- +source: src/controllers/krate/publish/deleted.rs +expression: text +--- +{"errors":[{"detail":"A crate with the name `foo` was previously deleted.\n\n* Reuse of this name will be available after 2024-11-21T12:00:00Z.\n* To avoid conflicts with previously published versions of this crate, the minimum version that can be published is 1.0.0."}]} diff --git a/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__min_version.snap b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__min_version.snap new file mode 100644 index 00000000000..a03f258f81f --- /dev/null +++ b/src/controllers/krate/publish/snapshots/crates_io__controllers__krate__publish__deleted__tests__min_version.snap @@ -0,0 +1,5 @@ +--- +source: src/controllers/krate/publish/deleted.rs +expression: text +--- +{"errors":[{"detail":"A crate with the name `foo` was previously deleted.\n\n* To avoid conflicts with previously published versions of this crate, the minimum version that can be published is 1.0.0."}]} diff --git a/src/models/deleted_crate.rs b/src/models/deleted_crate.rs index a21d667589f..551a89b1967 100644 --- a/src/models/deleted_crate.rs +++ b/src/models/deleted_crate.rs @@ -13,4 +13,5 @@ pub struct NewDeletedCrate<'a> { deleted_by: Option, message: Option<&'a str>, available_at: &'a DateTime, + min_version: Option<&'a str>, } diff --git a/src/tests/krate/publish/deleted_crates.rs b/src/tests/krate/publish/deleted_crates.rs index b9fd2efc7c4..3101a984f4f 100644 --- a/src/tests/krate/publish/deleted_crates.rs +++ b/src/tests/krate/publish/deleted_crates.rs @@ -31,8 +31,70 @@ async fn test_recently_deleted_crate_with_same_name() -> anyhow::Result<()> { let crate_to_publish = PublishBuilder::new("actix-web", "1.0.0"); let response = token.publish_crate(crate_to_publish).await; - assert_eq!(response.status(), StatusCode::BAD_REQUEST); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"A crate with the name `actix_web` was recently deleted. Reuse of this name will be available after 2099-12-25T12:34:56Z."}]}"#); + assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"A crate with the name `actix-web` was previously deleted.\n\n* Reuse of this name will be available after 2099-12-25T12:34:56Z."}]}"#); + assert_that!(app.stored_files().await, empty()); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_deleted_crate_with_same_name_and_version() -> anyhow::Result<()> { + let (app, _, _, token) = TestApp::full().with_token().await; + let mut conn = app.async_db_conn().await; + + let now = Utc::now(); + let created_at = now - Duration::hours(24); + let deleted_at = now - Duration::hours(1); + let available_at = now - Duration::minutes(30); + + let deleted_crate = NewDeletedCrate::builder("actix_web") + .created_at(&created_at) + .deleted_at(&deleted_at) + .available_at(&available_at) + .min_version("2.0.0") + .build(); + + diesel::insert_into(deleted_crates::table) + .values(deleted_crate) + .execute(&mut conn) + .await?; + + let crate_to_publish = PublishBuilder::new("actix-web", "1.0.0"); + let response = token.publish_crate(crate_to_publish).await; + assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"A crate with the name `actix-web` was previously deleted.\n\n* To avoid conflicts with previously published versions of this crate, the minimum version that can be published is 2.0.0."}]}"#); + assert_that!(app.stored_files().await, empty()); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_recently_deleted_crate_with_same_name_and_version() -> anyhow::Result<()> { + let (app, _, _, token) = TestApp::full().with_token().await; + let mut conn = app.async_db_conn().await; + + let now = Utc::now(); + let created_at = now - Duration::hours(24); + let deleted_at = now - Duration::hours(1); + let available_at = "2099-12-25T12:34:56Z".parse()?; + + let deleted_crate = NewDeletedCrate::builder("actix_web") + .created_at(&created_at) + .deleted_at(&deleted_at) + .available_at(&available_at) + .min_version("2.0.0") + .build(); + + diesel::insert_into(deleted_crates::table) + .values(deleted_crate) + .execute(&mut conn) + .await?; + + let crate_to_publish = PublishBuilder::new("actix-web", "1.0.0"); + let response = token.publish_crate(crate_to_publish).await; + assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"A crate with the name `actix-web` was previously deleted.\n\n* Reuse of this name will be available after 2099-12-25T12:34:56Z.\n* To avoid conflicts with previously published versions of this crate, the minimum version that can be published is 2.0.0."}]}"#); assert_that!(app.stored_files().await, empty()); Ok(())