Adjust error status codes for ownership change endpoints

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;
 

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.",

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}`")))
         }
     }
 

src/models/team.rs

@@ -1,7 +1,8 @@
 use diesel::prelude::*;
+use http::StatusCode;
 
 use crate::app::App;
-use crate::util::errors::{cargo_err, AppResult};
+use crate::util::errors::{bad_request, custom, AppResult};
 
 use crates_io_github::GitHubError;
 use oauth2::AccessToken;
@@ -99,7 +100,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 +114,7 @@ impl Team {
                     req_user,
                 )
             }
-            _ => Err(cargo_err(
+            _ => Err(bad_request(
                 "unknown organization handler, \
                  only 'github:org:team' is supported",
             )),
@@ -140,7 +141,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}"
             )));
@@ -150,15 +151,16 @@ 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}"
                 ))
             })?;
 
         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",
             ));
         }

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" }] })

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"}]}"###);
 }

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`"}]}"###);
 }

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" }] })
@@ -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" }] })
@@ -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" }] })
@@ -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" }] })