From 5ee83427ae1f49b35931d323cbd9a1a73d1c13ee Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 1 Dec 2020 21:22:08 +0100 Subject: [PATCH 1/5] tests/util: Implement `Response::json()` helper method --- src/tests/util.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/tests/util.rs b/src/tests/util.rs index df389df231f..2f3a55b5eb8 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -33,6 +33,7 @@ use cargo_registry::{ App, Config, }; use diesel::PgConnection; +use serde_json::Value; use std::{marker::PhantomData, rc::Rc, sync::Arc, time::Duration}; use swirl::Runner; @@ -587,6 +588,11 @@ where } } + #[track_caller] + pub fn json(mut self) -> Value { + crate::json(&mut self.response) + } + /// Assert that the response is good and deserialize the message #[track_caller] pub fn good(mut self) -> T { From 4235e72b4216cdb92d6ef9e2cb051a164197f1a9 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 3 Dec 2020 12:26:02 +0100 Subject: [PATCH 2/5] models/dependency: Extract `WILDCARD_ERROR_MESSAGE` constant This will allow us to use the constant in tests --- src/models/dependency.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/models/dependency.rs b/src/models/dependency.rs index 05b6d4b8faa..d05e1f6bbc3 100644 --- a/src/models/dependency.rs +++ b/src/models/dependency.rs @@ -7,6 +7,11 @@ use crate::models::{Crate, Version}; use crate::schema::*; use crate::views::{EncodableCrateDependency, EncodableDependency}; +pub const WILDCARD_ERROR_MESSAGE: &str = "wildcard (`*`) dependency constraints are not allowed \ + on crates.io. See https://doc.rust-lang.org/cargo/faq.html#can-\ + libraries-use--as-a-version-for-their-dependencies for more \ + information"; + #[derive(Identifiable, Associations, Debug, Queryable, QueryableByName)] #[belongs_to(Version)] #[belongs_to(Crate)] @@ -91,12 +96,7 @@ pub fn add_dependencies( .first(&*conn) .map_err(|_| cargo_err(&format_args!("no known crate named `{}`", &*dep.name)))?; if dep.version_req == semver::VersionReq::parse("*").unwrap() { - return Err(cargo_err( - "wildcard (`*`) dependency constraints are not allowed \ - on crates.io. See https://doc.rust-lang.org/cargo/faq.html#can-\ - libraries-use--as-a-version-for-their-dependencies for more \ - information", - )); + return Err(cargo_err(WILDCARD_ERROR_MESSAGE)); } // If this dependency has an explicit name in `Cargo.toml` that From bd3ad9620bbc5102c16a3604d1fc660018e99b5e Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 3 Dec 2020 12:30:34 +0100 Subject: [PATCH 3/5] controllers/krate/publish: Extract `MISSING_RIGHTS_ERROR_MESSAGE` constant This will allow us to use the constant in tests --- src/controllers/krate/publish.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 956e666890a..ad30051ba22 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -16,6 +16,12 @@ use crate::render; use crate::util::{read_fill, read_le_u32, Maximums}; use crate::views::{EncodableCrateUpload, GoodCrate, PublishWarnings}; +pub 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 \ + publishing."; + /// Handles the `PUT /crates/new` route. /// Used by `cargo publish` to publish a new crate or to publish a new version of an /// existing crate. @@ -98,12 +104,7 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult { let owners = krate.owners(&conn)?; if user.rights(req.app(), &owners)? < Rights::Publish { - return Err(cargo_err( - "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 \ - publishing.", - )); + return Err(cargo_err(MISSING_RIGHTS_ERROR_MESSAGE)); } if krate.name != *name { From 231de0728818f6c8499a15963e571b3692e70e98 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 3 Dec 2020 12:47:14 +0100 Subject: [PATCH 4/5] controllers/krate/publish: Extract `missing_metadata_error_message` function This will allow us to use the message generator function in tests --- src/controllers/krate/publish.rs | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index ad30051ba22..0972391493f 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -261,13 +261,30 @@ fn parse_new_headers(req: &mut dyn RequestExt) -> AppResult String { + format!( + "missing or empty metadata fields: {}. Please \ + see https://doc.rust-lang.org/cargo/reference/manifest.html for \ + how to upload metadata", + missing.join(", ") + ) +} + +#[cfg(test)] +mod tests { + use super::missing_metadata_error_message; + + #[test] + fn missing_metadata_error_message_test() { + assert_eq!(missing_metadata_error_message(&["a"]), "missing or empty metadata fields: a. Please see https://doc.rust-lang.org/cargo/reference/manifest.html for how to upload metadata"); + assert_eq!(missing_metadata_error_message(&["a", "b"]), "missing or empty metadata fields: a, b. Please see https://doc.rust-lang.org/cargo/reference/manifest.html for how to upload metadata"); + assert_eq!(missing_metadata_error_message(&["a", "b", "c"]), "missing or empty metadata fields: a, b, c. Please see https://doc.rust-lang.org/cargo/reference/manifest.html for how to upload metadata"); + } +} From 191e1e55cb833c3485c27cd6f3717b62aaaaa1d1 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 3 Dec 2020 12:52:31 +0100 Subject: [PATCH 5/5] tests/krate/publish: Use `assert_eq!()` to check response payload content --- src/tests/krate/publish.rs | 266 ++++++++++++++----------------------- 1 file changed, 99 insertions(+), 167 deletions(-) diff --git a/src/tests/krate/publish.rs b/src/tests/krate/publish.rs index 09f3ba7bc20..87f1f0b232f 100644 --- a/src/tests/krate/publish.rs +++ b/src/tests/krate/publish.rs @@ -1,6 +1,10 @@ use crate::builders::{CrateBuilder, DependencyBuilder, PublishBuilder}; use crate::new_category; use crate::util::{RequestHelper, TestApp}; +use cargo_registry::controllers::krate::publish::{ + missing_metadata_error_message, MISSING_RIGHTS_ERROR_MESSAGE, +}; +use cargo_registry::models::dependency::WILDCARD_ERROR_MESSAGE; use cargo_registry::models::krate::MAX_NAME_LENGTH; use cargo_registry::schema::{api_tokens, emails, versions_published_by}; use cargo_registry::views::GoodCrate; @@ -52,15 +56,11 @@ fn new_wrong_token() { // Try to publish without a token let crate_to_publish = PublishBuilder::new("foo"); - let json = anon - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::FORBIDDEN); - assert!( - json.errors[0] - .detail - .contains("must be logged in to perform that action"), - "{:?}", - json.errors + let response = anon.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::FORBIDDEN); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) ); // Try to publish with the wrong token (by changing the token in the database) @@ -72,15 +72,11 @@ fn new_wrong_token() { }); let crate_to_publish = PublishBuilder::new("foo"); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::FORBIDDEN); - assert!( - json.errors[0] - .detail - .contains("must be logged in to perform that action"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::FORBIDDEN); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "must be logged in to perform that action" }] }) ); } @@ -235,15 +231,11 @@ fn reject_new_crate_with_alternative_registry_dependency() { DependencyBuilder::new("dep").registry("https://server.example/path/to/registry"); let crate_to_publish = PublishBuilder::new("depends-on-alt-registry").dependency(dependency); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - assert!( - json.errors[0] - .detail - .contains("Cross-registry dependencies are not permitted on crates.io."), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "Dependency `dep` is hosted on another registry. Cross-registry dependencies are not permitted on crates.io." }] }) ); } @@ -262,13 +254,11 @@ fn new_krate_with_wildcard_dependency() { .version("1.0.0") .dependency(dependency); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - assert!( - json.errors[0].detail.contains("dependency constraints"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": WILDCARD_ERROR_MESSAGE }] }) ); } @@ -303,15 +293,11 @@ fn new_krate_wrong_user() { let another_user = app.db_new_user("another").db_new_token("bar"); let crate_to_publish = PublishBuilder::new("foo_wrong").version("2.0.0"); - let json = another_user - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - assert!( - json.errors[0] - .detail - .contains("this crate exists but you don't seem to be an owner."), - "{:?}", - json.errors + let response = another_user.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": MISSING_RIGHTS_ERROR_MESSAGE }] }) ); } @@ -322,15 +308,11 @@ fn new_krate_too_big() { let files = [("foo_big-1.0.0/big", &[b'a'; 2000] as &[_])]; let builder = PublishBuilder::new("foo_big").files(&files); - let json = user - .enqueue_publish(builder) - .bad_with_status(StatusCode::OK); - assert!( - json.errors[0] - .detail - .contains("uploaded tarball is malformed or too large when decompressed"), - "{:?}", - json.errors + let response = user.enqueue_publish(builder); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "uploaded tarball is malformed or too large when decompressed" }] }) ); } @@ -359,13 +341,11 @@ fn new_krate_wrong_files() { let files = [("foo-1.0.0/a", data), ("bar-1.0.0/a", data)]; let builder = PublishBuilder::new("foo").files(&files); - let json = user - .enqueue_publish(builder) - .bad_with_status(StatusCode::OK); - assert!( - json.errors[0].detail.contains("invalid tarball uploaded"), - "{:?}", - json.errors + let response = user.enqueue_publish(builder); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "invalid tarball uploaded" }] }) ); } @@ -380,16 +360,11 @@ fn new_krate_gzip_bomb() { .version("1.1.0") .files_with_io(&mut [("foo-1.1.0/a", &mut body, len)]); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - - assert!( - json.errors[0] - .detail - .contains("too large when decompressed"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "uploaded tarball is malformed or too large when decompressed" }] }) ); } @@ -405,14 +380,11 @@ fn new_krate_duplicate_version() { }); let crate_to_publish = PublishBuilder::new("foo_dupe").version("1.0.0"); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - - assert!( - json.errors[0].detail.contains("already uploaded"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "crate version `1.0.0` is already uploaded" }] }) ); } @@ -427,14 +399,11 @@ fn new_crate_similar_name() { }); let crate_to_publish = PublishBuilder::new("foo_similar").version("1.1.0"); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - - assert!( - json.errors[0].detail.contains("previously named"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "crate was previously named `Foo_similar`" }] }) ); } @@ -449,14 +418,11 @@ fn new_crate_similar_name_hyphen() { }); let crate_to_publish = PublishBuilder::new("foo-bar-hyphen").version("1.1.0"); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - - assert!( - json.errors[0].detail.contains("previously named"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "crate was previously named `foo_bar_hyphen`" }] }) ); } @@ -471,14 +437,11 @@ fn new_crate_similar_name_underscore() { }); let crate_to_publish = PublishBuilder::new("foo_bar_underscore").version("1.1.0"); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - - assert!( - json.errors[0].detail.contains("previously named"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "crate was previously named `foo-bar-underscore`" }] }) ); } @@ -547,15 +510,11 @@ fn new_krate_dependency_missing() { let dependency = DependencyBuilder::new("bar_missing"); let crate_to_publish = PublishBuilder::new("foo_missing").dependency(dependency); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - assert!( - json.errors[0] - .detail - .contains("no known crate named `bar_missing`"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "no known crate named `bar_missing`" }] }) ); } @@ -580,16 +539,11 @@ fn new_krate_without_any_email_fails() { let crate_to_publish = PublishBuilder::new("foo_no_email"); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - - assert!( - json.errors[0] - .detail - .contains("A verified email address is required to publish crates to crates.io"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "A verified email address is required to publish crates to crates.io. Visit https://crates.io/me to set and verify your email address." }] }) ); } @@ -606,16 +560,11 @@ fn new_krate_with_unverified_email_fails() { let crate_to_publish = PublishBuilder::new("foo_unverified_email"); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - - assert!( - json.errors[0] - .detail - .contains("A verified email address is required to publish crates to crates.io"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "A verified email address is required to publish crates to crates.io. Visit https://crates.io/me to set and verify your email address." }] }) ); } @@ -820,15 +769,11 @@ fn author_license_and_description_required() { .unset_description() .unset_authors(); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - assert!( - json.errors[0].detail.contains("author") - && json.errors[0].detail.contains("description") - && json.errors[0].detail.contains("license"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": missing_metadata_error_message(&["description", "license", "authors"]) }] }) ); let crate_to_publish = PublishBuilder::new("foo_metadata") @@ -837,15 +782,11 @@ fn author_license_and_description_required() { .unset_authors() .author(""); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - assert!( - json.errors[0].detail.contains("author") - && json.errors[0].detail.contains("description") - && !json.errors[0].detail.contains("license"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": missing_metadata_error_message(&["description", "authors"]) }] }) ); let crate_to_publish = PublishBuilder::new("foo_metadata") @@ -854,15 +795,11 @@ fn author_license_and_description_required() { .license_file("foo") .unset_description(); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - assert!( - !json.errors[0].detail.contains("author") - && json.errors[0].detail.contains("description") - && !json.errors[0].detail.contains("license"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": missing_metadata_error_message(&["description"]) }] }) ); } @@ -885,16 +822,11 @@ fn new_krate_tarball_with_hard_links() { let crate_to_publish = PublishBuilder::new("foo").version("1.1.0").tarball(tarball); - let json = token - .enqueue_publish(crate_to_publish) - .bad_with_status(StatusCode::OK); - - assert!( - json.errors[0] - .detail - .contains("too large when decompressed"), - "{:?}", - json.errors + let response = token.enqueue_publish(crate_to_publish); + response.assert_status(StatusCode::OK); + assert_eq!( + response.json(), + json!({ "errors": [{ "detail": "uploaded tarball is malformed or too large when decompressed" }] }) ); }