Auto merge of #1929 - jtgeibel:fix-some-error-responses, r=smarnach

Error response refactoring step 2

This is a continuation of #1920.  This series captures several changes:

* The controller prelude is split to provide helpers for either cargo or frontend endpoints.  Eventually this will be unnecessary, but I see it as a temporary guard rail to encourage the correct usage.
* Several user and session frontend endpoints are updated to return a response with the correct HTTP status.  Tests were updated as necessary.
* A `util::errors::http` module is added for concrete error types associated with HTTP status codes.  There are more types to be moved here, but for review purposes that can be done in another PR.
* Once `cargo_err` was refactored to use `http::Ok`, all of the description and fallback logic in the `AppError` trait and `ConcreteAppError` type was no longer necessary.

Here is what I have in mind for the next steps in this refactor:

* Add a `not_found` helper that allows a 404 status with an error message for the client.  (TBD if this can be merged with the existing `NotFound` type or if that functionality should remain independent.)
* Finish moving types to `http` as applicable.
* Review remaing existing usage of helpers within controllers.

After that groundwork, my final step is to investigate the ability to sanitize error response for cargo at the router level so that `cargo_err` can be removed.  It isn't really practical currently to use the correct helper type from within models, since model functionality can be used from either type of endpoint (or both).

The router seems like a strange place for that behavior, but it's where the [error to response conversion](https://github.com/rust-lang/crates.io/blob/4a1542cb17ae50c2bfd0f7e85876d82617e30415/src/router.rs#L139) already happens, and the middleware layer only sees a generic `dyn Error`, not our `AppError`.

With a central place to sanitize errors for old cargo versions, we could even enable it only for old (and empty) user agents.

r? @smarnach

Author: bors

src/controllers.rs

@@ -1,3 +1,13 @@
+mod cargo_prelude {
+    pub use super::prelude::*;
+    pub use crate::util::cargo_err;
+}
+
+mod frontend_prelude {
+    pub use super::prelude::*;
+    pub use crate::util::errors::{bad_request, server_error};
+}
+
 mod prelude {
     pub use super::helpers::ok_true;
     pub use diesel::prelude::*;
@@ -6,7 +16,7 @@ mod prelude {
     pub use conduit_router::RequestParams;
 
     pub use crate::db::RequestTransaction;
-    pub use crate::util::{cargo_err, AppResult};
+    pub use crate::util::{cargo_err, AppResult}; // TODO: Remove cargo_err from here
 
     pub use crate::middleware::app::RequestApp;
     pub use crate::middleware::current_user::RequestUser;

src/controllers/crate_owner_invitation.rs

@@ -1,4 +1,4 @@
-use super::prelude::*;
+use super::frontend_prelude::*;
 
 use crate::models::{CrateOwner, CrateOwnerInvitation, OwnerKind};
 use crate::schema::{crate_owner_invitations, crate_owners};
@@ -38,7 +38,7 @@ pub fn handle_invite(req: &mut dyn Request) -> AppResult<Response> {
     let conn = &*req.db_conn()?;
 
     let crate_invite: OwnerInvitation =
-        serde_json::from_str(&body).map_err(|_| cargo_err("invalid json request"))?;
+        serde_json::from_str(&body).map_err(|_| bad_request("invalid json request"))?;
 
     let crate_invite = crate_invite.crate_owner_invite;
 

src/controllers/krate/downloads.rs

@@ -5,7 +5,7 @@
 
 use std::cmp;
 
-use crate::controllers::prelude::*;
+use crate::controllers::frontend_prelude::*;
 
 use crate::models::{Crate, CrateVersions, Version, VersionDownload};
 use crate::schema::version_downloads;

src/controllers/krate/follow.rs

@@ -2,7 +2,7 @@
 
 use diesel::associations::Identifiable;
 
-use crate::controllers::prelude::*;
+use crate::controllers::frontend_prelude::*;
 use crate::db::DieselPooledConn;
 use crate::models::{Crate, Follow};
 use crate::schema::*;

src/controllers/krate/metadata.rs

@@ -4,7 +4,7 @@
 //! index or cached metadata which was extracted (client side) from the
 //! `Cargo.toml` file.
 
-use crate::controllers::prelude::*;
+use crate::controllers::frontend_prelude::*;
 use crate::models::{
     Category, Crate, CrateCategory, CrateKeyword, CrateVersions, Keyword, RecentCrateDownloads,
     User, Version,

src/controllers/krate/publish.rs

@@ -4,7 +4,7 @@ use hex::ToHex;
 use std::sync::Arc;
 use swirl::Job;
 
-use crate::controllers::prelude::*;
+use crate::controllers::cargo_prelude::*;
 use crate::git;
 use crate::models::dependency;
 use crate::models::{

src/controllers/krate/search.rs

@@ -3,8 +3,8 @@
 use diesel::dsl::*;
 use diesel_full_text_search::*;
 
+use crate::controllers::cargo_prelude::*;
 use crate::controllers::helpers::Paginate;
-use crate::controllers::prelude::*;
 use crate::models::{Crate, CrateBadge, CrateOwner, CrateVersions, OwnerKind, Version};
 use crate::schema::*;
 use crate::views::EncodableCrate;

src/controllers/team.rs

@@ -1,4 +1,4 @@
-use crate::controllers::prelude::*;
+use crate::controllers::frontend_prelude::*;
 
 use crate::models::Team;
 use crate::schema::teams;

src/controllers/user/me.rs

@@ -1,10 +1,9 @@
 use std::collections::HashMap;
 
-use crate::controllers::prelude::*;
+use crate::controllers::frontend_prelude::*;
 
 use crate::controllers::helpers::*;
 use crate::email;
-use crate::util::bad_request;
 use crate::util::errors::AppError;
 
 use crate::models::{CrateOwner, Email, Follow, NewEmail, OwnerKind, User, Version};
@@ -117,7 +116,7 @@ pub fn update_user(req: &mut dyn Request) -> AppResult<Response> {
 
     // need to check if current user matches user to be updated
     if &user.id.to_string() != name {
-        return Err(cargo_err("current user does not match requested user"));
+        return Err(bad_request("current user does not match requested user"));
     }
 
     #[derive(Deserialize)]
@@ -131,17 +130,17 @@ pub fn update_user(req: &mut dyn Request) -> AppResult<Response> {
     }
 
     let user_update: UserUpdate =
-        serde_json::from_str(&body).map_err(|_| cargo_err("invalid json request"))?;
+        serde_json::from_str(&body).map_err(|_| bad_request("invalid json request"))?;
 
     if user_update.user.email.is_none() {
-        return Err(cargo_err("empty email rejected"));
+        return Err(bad_request("empty email rejected"));
     }
 
     let user_email = user_update.user.email.unwrap();
     let user_email = user_email.trim();
 
     if user_email == "" {
-        return Err(cargo_err("empty email rejected"));
+        return Err(bad_request("empty email rejected"));
     }
 
     conn.transaction::<_, Box<dyn AppError>, _>(|| {
@@ -157,7 +156,7 @@ pub fn update_user(req: &mut dyn Request) -> AppResult<Response> {
             .set(&new_email)
             .returning(emails::token)
             .get_result::<String>(&*conn)
-            .map_err(|_| cargo_err("Error in creating token"))?;
+            .map_err(|_| server_error("Error in creating token"))?;
 
         crate::email::send_user_confirm_email(user_email, &user.gh_login, &token);
 
@@ -196,7 +195,7 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult<Response> {
 
     // need to check if current user matches user to be updated
     if &user.id != name {
-        return Err(cargo_err("current user does not match requested user"));
+        return Err(bad_request("current user does not match requested user"));
     }
 
     conn.transaction(|| {
@@ -206,7 +205,7 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult<Response> {
             .map_err(|_| bad_request("Email could not be found"))?;
 
         email::try_send_user_confirm_email(&email.email, &user.gh_login, &email.token)
-            .map_err(|_| bad_request("Error in sending email"))
+            .map_err(|_| server_error("Error in sending email"))
     })?;
 
     ok_true()

src/controllers/user/other.rs

@@ -1,4 +1,4 @@
-use crate::controllers::prelude::*;
+use crate::controllers::frontend_prelude::*;
 
 use crate::models::{CrateOwner, OwnerKind, User};
 use crate::schema::{crate_owners, crates, users};

src/controllers/user/session.rs

@@ -1,12 +1,13 @@
-use crate::controllers::prelude::*;
+use crate::controllers::frontend_prelude::*;
 
 use crate::github;
 use conduit_cookie::RequestSession;
+use failure::Fail;
 use oauth2::{prelude::*, AuthorizationCode, TokenResponse};
 
 use crate::models::{NewUser, User};
 use crate::schema::users;
-use crate::util::errors::{AppError, ReadOnlyMode};
+use crate::util::errors::{AppError, ChainError, ReadOnlyMode};
 
 /// Handles the `GET /api/private/session/begin` route.
 ///
@@ -83,7 +84,7 @@ pub fn authorize(req: &mut dyn Request) -> AppResult<Response> {
         let session_state = req.session().remove(&"github_oauth_state".to_string());
         let session_state = session_state.as_ref().map(|a| &a[..]);
         if Some(&state[..]) != session_state {
-            return Err(cargo_err("invalid state parameter"));
+            return Err(bad_request("invalid state parameter"));
         }
     }
 
@@ -94,7 +95,8 @@ pub fn authorize(req: &mut dyn Request) -> AppResult<Response> {
         .app()
         .github
         .exchange_code(code)
-        .map_err(|s| cargo_err(&s))?;
+        .map_err(|e| e.compat())
+        .chain_error(|| server_error("Error obtaining token"))?;
     let token = token.access_token();
     let ghuser = github::github_api::<GithubUser>(req.app(), "/user", token)?;
     let user = ghuser.save_to_database(&token.secret(), &*req.db_conn()?)?;

src/controllers/version/deprecated.rs

@@ -5,7 +5,7 @@
 //! period of time to ensure there are no external users of an endpoint before
 //! it is removed.
 
-use crate::controllers::prelude::*;
+use crate::controllers::frontend_prelude::*;
 
 use crate::models::{Crate, User, Version};
 use crate::schema::*;

src/controllers/version/metadata.rs

@@ -4,7 +4,7 @@
 //! index or cached metadata which was extracted (client side) from the
 //! `Cargo.toml` file.
 
-use crate::controllers::prelude::*;
+use crate::controllers::frontend_prelude::*;
 
 use crate::schema::*;
 use crate::views::{EncodableDependency, EncodablePublicUser, EncodableVersion};

src/controllers/version/yank.rs

@@ -3,7 +3,7 @@
 use swirl::Job;
 
 use super::version_and_crate;
-use crate::controllers::prelude::*;
+use crate::controllers::cargo_prelude::*;
 use crate::git;
 use crate::models::{insert_version_owner_action, Rights, VersionAction};
 use crate::util::AppError;

src/middleware/log_request.rs

@@ -53,7 +53,7 @@ impl Handler for LogRequests {
         );
 
         if let Err(ref e) = res {
-            print!(" error=\"{}\"", e.description());
+            print!(" error=\"{}\"", e.to_string());
         }
 
         if response_time > 1000 {

src/tests/user.rs

@@ -115,7 +115,7 @@ fn access_token_needs_data() {
     let (_, anon) = TestApp::init().empty();
     let json = anon
         .get::<()>("/api/private/session/authorize")
-        .bad_with_status(200); // Change endpoint to 400?
+        .bad_with_status(400);
     assert!(json.errors[0].detail.contains("invalid state"));
 }
 
@@ -486,7 +486,7 @@ fn test_empty_email_not_added() {
 
     let json = user
         .update_email_more_control(model.id, Some(""))
-        .bad_with_status(200);
+        .bad_with_status(400);
     assert!(
         json.errors[0].detail.contains("empty email rejected"),
         "{:?}",
@@ -495,7 +495,7 @@ fn test_empty_email_not_added() {
 
     let json = user
         .update_email_more_control(model.id, None)
-        .bad_with_status(200);
+        .bad_with_status(400);
 
     assert!(
         json.errors[0].detail.contains("empty email rejected"),
@@ -521,7 +521,7 @@ fn test_other_users_cannot_change_my_email() {
             another_user_model.id,
             Some("[email protected]"),
         )
-        .bad_with_status(200);
+        .bad_with_status(400);
     assert!(
         json.errors[0]
             .detail