From 53cb4cbe659a85943120e9c0c1fa628337403e37 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 30 Jan 2024 14:31:27 +0100 Subject: [PATCH 1/5] controllers/krate/owners: Use "400 Bad Request" and "403 Forbidden" for errors --- src/controllers/krate/owners.rs | 14 +++++++++----- src/tests/owners.rs | 8 ++++---- src/tests/team.rs | 10 +++++----- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 3491e7f53b..701d0d2817 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -4,7 +4,7 @@ use crate::auth::AuthCheck; use crate::controllers::prelude::*; use crate::models::token::EndpointScope; use crate::models::{Crate, Owner, Rights, Team, User}; -use crate::util::errors::crate_not_found; +use crate::util::errors::{bad_request, crate_not_found, custom}; use crate::views::EncodableOwner; use tokio::runtime::Handle; @@ -121,12 +121,16 @@ fn modify_owners( Rights::Full => {} // Yes! Rights::Publish => { - return Err(cargo_err( + return Err(custom( + StatusCode::FORBIDDEN, "team members don't have permission to modify owners", )); } Rights::None => { - return Err(cargo_err("only owners have permission to modify owners")); + return Err(custom( + StatusCode::FORBIDDEN, + "only owners have permission to modify owners", + )); } } @@ -136,7 +140,7 @@ fn modify_owners( let login_test = |owner: &Owner| owner.login().to_lowercase() == *login.to_lowercase(); if owners.iter().any(login_test) { - return Err(cargo_err(format_args!("`{login}` is already an owner"))); + return Err(bad_request(format_args!("`{login}` is already an owner"))); } let msg = krate.owner_add(app, conn, user, login)?; msgs.push(msg); @@ -147,7 +151,7 @@ fn modify_owners( krate.owner_remove(conn, login)?; } if User::owning(&krate, conn)?.is_empty() { - return Err(cargo_err( + return Err(bad_request( "cannot remove all individual owners of a crate. \ Team member don't have permission to modify owners, so \ at least one individual owner is required.", diff --git a/src/tests/owners.rs b/src/tests/owners.rs index 54637b1892..adea28368d 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -171,7 +171,7 @@ fn owners_can_remove_self() { // Deleting yourself when you're the only owner isn't allowed. let response = token.remove_named_owner("owners_selfremove", username); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.json(), json!({ "errors": [{ "detail": "cannot remove all individual owners of a crate. Team member don't have permission to modify owners, so at least one individual owner is required." }] }) @@ -189,7 +189,7 @@ fn owners_can_remove_self() { // After you delete yourself, you no longer have permissions to manage the crate. let response = token.remove_named_owner("owners_selfremove", username); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.json(), json!({ "errors": [{ "detail": "only owners have permission to modify owners" }] }) @@ -210,7 +210,7 @@ fn modify_multiple_owners() { // Deleting all owners is not allowed. let response = token.remove_named_owners("owners_multiple", &[username, "user2", "user3"]); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.json(), json!({ "errors": [{ "detail": "cannot remove all individual owners of a crate. Team member don't have permission to modify owners, so at least one individual owner is required." }] }) @@ -228,7 +228,7 @@ fn modify_multiple_owners() { // Adding multiple users fails if one of them already is an owner. let response = token.add_named_owners("owners_multiple", &["user2", username]); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.json(), json!({ "errors": [{ "detail": "`foo` is already an owner" }] }) diff --git a/src/tests/team.rs b/src/tests/team.rs index d7f8072b28..ba32c9db83 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -215,7 +215,7 @@ fn remove_team_as_named_owner() { // Removing the individual owner is not allowed, since team members don't // have permission to manage ownership let response = token_on_both_teams.remove_named_owner("foo_remove_team", username); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.json(), json!({ "errors": [{ "detail": "cannot remove all individual owners of a crate. Team member don't have permission to modify owners, so at least one individual owner is required." }] }) @@ -255,7 +255,7 @@ fn remove_team_as_team_owner() { let response = token_on_one_team.remove_named_owner("foo_remove_team_owner", "github:test-org:all"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.json(), json!({ "errors": [{ "detail": "team members don't have permission to modify owners" }] }) @@ -265,7 +265,7 @@ fn remove_team_as_team_owner() { let token_org_owner = user_org_owner.db_new_token("arbitrary token name"); let response = token_org_owner.remove_named_owner("foo_remove_team_owner", "github:test-org:all"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.json(), json!({ "errors": [{ "detail": "only owners have permission to modify owners" }] }) @@ -386,7 +386,7 @@ fn add_owners_as_org_owner() { let token_org_owner = user_org_owner.db_new_token("arbitrary token name"); let response = token_org_owner.add_named_owner("foo_add_owner", "arbitrary_username"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.json(), json!({ "errors": [{ "detail": "only owners have permission to modify owners" }] }) @@ -411,7 +411,7 @@ fn add_owners_as_team_owner() { let token_on_one_team = user_on_one_team.db_new_token("arbitrary token name"); let response = token_on_one_team.add_named_owner("foo_add_owner", "arbitrary_username"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.json(), json!({ "errors": [{ "detail": "team members don't have permission to modify owners" }] }) From 531750750a241c7d0610d3004a3dfa35cf778861 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 30 Jan 2024 14:42:30 +0100 Subject: [PATCH 2/5] models/owner: Use "400 Bad Request" for errors --- src/models/owner.rs | 8 ++++---- src/models/team.rs | 4 ++-- src/tests/routes/crates/owners/add.rs | 4 ++-- src/tests/routes/crates/owners/remove.rs | 4 ++-- src/tests/team.rs | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/models/owner.rs b/src/models/owner.rs index 1191380ffc..79abb5f8e7 100644 --- a/src/models/owner.rs +++ b/src/models/owner.rs @@ -2,7 +2,7 @@ use diesel::pg::Pg; use diesel::prelude::*; use crate::app::App; -use crate::util::errors::{cargo_err, AppResult}; +use crate::util::errors::{bad_request, AppResult}; use crate::models::{Crate, Team, User}; use crate::schema::crate_owners; @@ -74,7 +74,7 @@ impl Owner { User::find_by_login(conn, name) .optional()? .map(Owner::User) - .ok_or_else(|| cargo_err(format_args!("could not find user with login `{name}`"))) + .ok_or_else(|| bad_request(format_args!("could not find user with login `{name}`"))) } } @@ -89,12 +89,12 @@ impl Owner { Team::find_by_login(conn, name) .optional()? .map(Owner::Team) - .ok_or_else(|| cargo_err(format_args!("could not find team with login `{name}`"))) + .ok_or_else(|| bad_request(format_args!("could not find team with login `{name}`"))) } else { User::find_by_login(conn, name) .optional()? .map(Owner::User) - .ok_or_else(|| cargo_err(format_args!("could not find user with login `{name}`"))) + .ok_or_else(|| bad_request(format_args!("could not find user with login `{name}`"))) } } diff --git a/src/models/team.rs b/src/models/team.rs index dedcae2fb4..c7e4a46bf9 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -1,7 +1,7 @@ use diesel::prelude::*; use crate::app::App; -use crate::util::errors::{cargo_err, AppResult}; +use crate::util::errors::{bad_request, cargo_err, AppResult}; use crates_io_github::GitHubError; use oauth2::AccessToken; @@ -150,7 +150,7 @@ impl Team { let team = Handle::current() .block_on(app.github.team_by_name(org_name, team_name, &token)) .map_err(|_| { - cargo_err(format_args!( + bad_request(format_args!( "could not find the github team {org_name}/{team_name}" )) })?; diff --git a/src/tests/routes/crates/owners/add.rs b/src/tests/routes/crates/owners/add.rs index bdeb72f269..b8ca96b3fc 100644 --- a/src/tests/routes/crates/owners/add.rs +++ b/src/tests/routes/crates/owners/add.rs @@ -338,7 +338,7 @@ fn test_unknown_user() { let body = serde_json::to_vec(&json!({ "owners": ["unknown"] })).unwrap(); let response = cookie.put::<()>("/api/v1/crates/foo/owners", body); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_display_snapshot!(response.text(), @r###"{"errors":[{"detail":"could not find user with login `unknown`"}]}"###); } @@ -350,6 +350,6 @@ fn test_unknown_team() { let body = serde_json::to_vec(&json!({ "owners": ["github:unknown:unknown"] })).unwrap(); let response = cookie.put::<()>("/api/v1/crates/foo/owners", body); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_display_snapshot!(response.text(), @r###"{"errors":[{"detail":"could not find the github team unknown/unknown"}]}"###); } diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index fc64b1b0fb..90d2067581 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -49,7 +49,7 @@ fn test_unknown_user() { let body = serde_json::to_vec(&json!({ "owners": ["unknown"] })).unwrap(); let response = cookie.delete_with_body::<()>("/api/v1/crates/foo/owners", body); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_display_snapshot!(response.text(), @r###"{"errors":[{"detail":"could not find user with login `unknown`"}]}"###); } @@ -61,6 +61,6 @@ fn test_unknown_team() { let body = serde_json::to_vec(&json!({ "owners": ["github:unknown:unknown"] })).unwrap(); let response = cookie.delete_with_body::<()>("/api/v1/crates/foo/owners", body); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_display_snapshot!(response.text(), @r###"{"errors":[{"detail":"could not find team with login `github:unknown:unknown`"}]}"###); } diff --git a/src/tests/team.rs b/src/tests/team.rs index ba32c9db83..e95efa15a4 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -79,7 +79,7 @@ fn add_nonexistent_team() { let response = token.add_named_owner("foo_add_nonexistent", "github:test-org:this-does-not-exist"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.json(), json!({ "errors": [{ "detail": "could not find the github team test-org/this-does-not-exist" }] }) From a1b443c0ca27dfb01341dbf1f0e2a165b71ecfb0 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 30 Jan 2024 15:13:07 +0100 Subject: [PATCH 3/5] models/team: Use "400 Bad Request" for errors --- src/models/team.rs | 6 +++--- src/tests/team.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/models/team.rs b/src/models/team.rs index c7e4a46bf9..96ce200e0d 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -99,7 +99,7 @@ impl Team { // unwrap is documented above as part of the calling contract let org = chunks.next().unwrap(); let team = chunks.next().ok_or_else(|| { - cargo_err( + bad_request( "missing github team argument; \ format is github:org:team", ) @@ -113,7 +113,7 @@ impl Team { req_user, ) } - _ => Err(cargo_err( + _ => Err(bad_request( "unknown organization handler, \ only 'github:org:team' is supported", )), @@ -140,7 +140,7 @@ impl Team { } if let Some(c) = org_name.chars().find(|c| !is_allowed_char(*c)) { - return Err(cargo_err(format_args!( + return Err(bad_request(format_args!( "organization cannot contain special \ characters like {c}" ))); diff --git a/src/tests/team.rs b/src/tests/team.rs index e95efa15a4..d863de1510 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -29,7 +29,7 @@ fn not_github() { }); let response = token.add_named_owner("foo_not_github", "dropbox:foo:foo"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.json(), json!({ "errors": [{ "detail": "unknown organization handler, only 'github:org:team' is supported" }] }) @@ -45,7 +45,7 @@ fn weird_name() { }); let response = token.add_named_owner("foo_weird_name", "github:foo/../bar:wut"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.json(), json!({ "errors": [{ "detail": "organization cannot contain special characters like /" }] }) @@ -62,7 +62,7 @@ fn one_colon() { }); let response = token.add_named_owner("foo_one_colon", "github:foo"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( response.json(), json!({ "errors": [{ "detail": "missing github team argument; format is github:org:team" }] }) From 35439764fdd5e4ac458d46ecf6f8a2fc90bc3c13 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 30 Jan 2024 15:13:55 +0100 Subject: [PATCH 4/5] models/team: Use "403 Forbidden" for permission errors --- src/models/team.rs | 6 ++++-- src/tests/team.rs | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/models/team.rs b/src/models/team.rs index 96ce200e0d..7555abd2a5 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -1,7 +1,8 @@ use diesel::prelude::*; +use http::StatusCode; use crate::app::App; -use crate::util::errors::{bad_request, cargo_err, AppResult}; +use crate::util::errors::{bad_request, custom, AppResult}; use crates_io_github::GitHubError; use oauth2::AccessToken; @@ -158,7 +159,8 @@ impl Team { let org_id = team.organization.id; if !Handle::current().block_on(can_add_team(app, org_id, team.id, req_user))? { - return Err(cargo_err( + return Err(custom( + StatusCode::FORBIDDEN, "only members of a team or organization owners can add it as an owner", )); } diff --git a/src/tests/team.rs b/src/tests/team.rs index d863de1510..85d724b7fc 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -190,7 +190,7 @@ fn add_team_as_non_member() { }); let response = token.add_named_owner("foo_team_non_member", "github:test-org:core"); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::FORBIDDEN); assert_eq!( response.json(), json!({ "errors": [{ "detail": "only members of a team or organization owners can add it as an owner" }] }) From 0213f73200815930f65034cf0ebcc8049b467a2a Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 30 Jan 2024 15:30:43 +0100 Subject: [PATCH 5/5] controllers: Remove obsolete `cargo_err()` import --- src/controllers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers.rs b/src/controllers.rs index 1440bc6718..9a85951939 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -21,7 +21,7 @@ mod prelude { use crate::controllers::util::RequestPartsExt; pub use crate::middleware::app::RequestApp; pub use crate::tasks::spawn_blocking; - pub use crate::util::errors::{cargo_err, AppResult, BoxedAppError}; + pub use crate::util::errors::{AppResult, BoxedAppError}; pub use crate::util::BytesRequest; use indexmap::IndexMap;