diff --git a/src/controllers.rs b/src/controllers.rs index d1437c8f69b..eca318ce86f 100644 --- a/src/controllers.rs +++ b/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; diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index 1cf8ded7ef4..655946d2351 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/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 { 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; diff --git a/src/controllers/krate/downloads.rs b/src/controllers/krate/downloads.rs index 155d7126746..7dc95fd22f7 100644 --- a/src/controllers/krate/downloads.rs +++ b/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; diff --git a/src/controllers/krate/follow.rs b/src/controllers/krate/follow.rs index d0e86df18a1..93bd2097aa2 100644 --- a/src/controllers/krate/follow.rs +++ b/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::*; diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 9e02718f8c3..0390038d198 100644 --- a/src/controllers/krate/metadata.rs +++ b/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, diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 527d3588bf7..09dce9934aa 100644 --- a/src/controllers/krate/publish.rs +++ b/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::{ diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 50aaa6c27ee..2ad799bd4ce 100644 --- a/src/controllers/krate/search.rs +++ b/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; diff --git a/src/controllers/team.rs b/src/controllers/team.rs index f62b9821e0e..a0a98b4a224 100644 --- a/src/controllers/team.rs +++ b/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; diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 65391eb9e45..f6f48681f7f 100644 --- a/src/controllers/user/me.rs +++ b/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}; @@ -118,7 +117,7 @@ pub fn update_user(req: &mut dyn Request) -> AppResult { // 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)] @@ -132,17 +131,17 @@ pub fn update_user(req: &mut dyn Request) -> AppResult { } 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, _>(|| { @@ -158,7 +157,7 @@ pub fn update_user(req: &mut dyn Request) -> AppResult { .set(&new_email) .returning(emails::token) .get_result::(&*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); @@ -197,7 +196,7 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult { // 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(|| { @@ -207,7 +206,7 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult { .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() diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index b2e9cb598d5..bb3a4ce225d 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -1,4 +1,4 @@ -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::models::{OwnerKind, User}; use crate::schema::{crate_owners, crates, users}; diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 500d18048da..0316f1c9fd7 100644 --- a/src/controllers/user/session.rs +++ b/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 { 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 { .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::(req.app(), "/user", token)?; let user = ghuser.save_to_database(&token.secret(), &*req.db_conn()?)?; diff --git a/src/controllers/version/deprecated.rs b/src/controllers/version/deprecated.rs index 9246a9b8a1f..e9667150765 100644 --- a/src/controllers/version/deprecated.rs +++ b/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::*; diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index 9b985944a4f..0212bba5c8a 100644 --- a/src/controllers/version/metadata.rs +++ b/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}; diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index e67d65f4666..9262678eaaa 100644 --- a/src/controllers/version/yank.rs +++ b/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; diff --git a/src/middleware/log_request.rs b/src/middleware/log_request.rs index ff1631adc5c..2b31527f199 100644 --- a/src/middleware/log_request.rs +++ b/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 { diff --git a/src/tests/user.rs b/src/tests/user.rs index fde2c94a39b..756f6d2447f 100644 --- a/src/tests/user.rs +++ b/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")); } @@ -475,7 +475,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"), "{:?}", @@ -484,7 +484,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"), @@ -510,7 +510,7 @@ fn test_other_users_cannot_change_my_email() { another_user_model.id, Some("pineapple@pineapples.pineapple"), ) - .bad_with_status(200); + .bad_with_status(400); assert!( json.errors[0] .detail diff --git a/src/util/errors.rs b/src/util/errors.rs index aca5bdef5d3..5e9da0c9318 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -8,6 +8,27 @@ use diesel::result::Error as DieselError; use crate::util::json_response; +mod http; + +/// Returns an error with status 200 and the provided description as JSON +/// +/// This is for backwards compatibility with cargo endpoints. For all other +/// endpoints, use helpers like `bad_request` or `server_error` which set a +/// correct status code. +pub fn cargo_err(error: &S) -> Box { + Box::new(http::Ok(error.to_string())) +} + +// The following are intended to be used for errors being sent back to the Ember +// frontend, not to cargo as cargo does not handle non-200 response codes well +// (see ), but Ember requires +// non-200 response codes for its stores to work properly. + +/// Returns an error with status 500 and the provided description as JSON +pub fn server_error(error: &S) -> Box { + Box::new(http::ServerError(error.to_string())) +} + #[derive(Serialize)] struct StringError { detail: String, @@ -21,37 +42,11 @@ struct Bad { // AppError trait pub trait AppError: Send + fmt::Display + fmt::Debug + 'static { - fn description(&self) -> &str; - fn cause(&self) -> Option<&(dyn AppError)> { - None - } - /// Generate an HTTP response for the error - fn response(&self) -> Option; - - /// Fallback logic for generating a cargo friendly response - /// - /// This behavior is deprecated and no new calls or impls should be added. - fn fallback_response(&self) -> Option { - if self.fallback_with_description_as_bad_200() { - Some(json_response(&Bad { - errors: vec![StringError { - detail: self.description().to_string(), - }], - })) - } else { - self.cause().and_then(AppError::response) - } - } - - /// Determines if the `fallback_response` method should send the description as a status 200 - /// error to cargo, or send the cause response (if applicable). /// - /// This is only to be used by the `fallback_response` method. If your error type impls - /// `response`, then there is no need to impl this method. - fn fallback_with_description_as_bad_200(&self) -> bool { - false - } + /// If none is returned, the error will bubble up the middleware stack + /// where it is eventually logged and turned into a status 500 response. + fn response(&self) -> Option; fn get_type_id(&self) -> TypeId { TypeId::of::() @@ -81,15 +76,6 @@ impl dyn AppError { } impl AppError for Box { - fn description(&self) -> &str { - (**self).description() - } - fn cause(&self) -> Option<&dyn AppError> { - (**self).cause() - } - fn fallback_with_description_as_bad_200(&self) -> bool { - (**self).fallback_with_description_as_bad_200() - } fn response(&self) -> Option { (**self).response() } @@ -155,18 +141,9 @@ impl ChainError for Option { } impl AppError for ChainedError { - fn description(&self) -> &str { - self.error.description() - } - fn cause(&self) -> Option<&dyn AppError> { - Some(&*self.cause) - } fn response(&self) -> Option { self.error.response() } - fn fallback_with_description_as_bad_200(&self) -> bool { - self.error.fallback_with_description_as_bad_200() - } } impl fmt::Display for ChainedError { @@ -179,11 +156,8 @@ impl fmt::Display for ChainedError { // Error impls impl AppError for E { - fn description(&self) -> &str { - Error::description(self) - } fn response(&self) -> Option { - self.fallback_response() + None } } @@ -196,35 +170,20 @@ impl From for Box { // Concrete errors #[derive(Debug)] -struct ConcreteAppError { +struct InternalAppError { description: String, - detail: Option, - cause: Option>, - cargo_err: bool, } -impl fmt::Display for ConcreteAppError { +impl fmt::Display for InternalAppError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.description)?; - if let Some(ref s) = self.detail { - write!(f, " ({})", s)?; - } Ok(()) } } -impl AppError for ConcreteAppError { - fn description(&self) -> &str { - &self.description - } - fn cause(&self) -> Option<&dyn AppError> { - self.cause.as_ref().map(|c| &**c) - } +impl AppError for InternalAppError { fn response(&self) -> Option { - self.fallback_response() - } - fn fallback_with_description_as_bad_200(&self) -> bool { - self.cargo_err + None } } @@ -232,10 +191,6 @@ impl AppError for ConcreteAppError { pub struct NotFound; impl AppError for NotFound { - fn description(&self) -> &str { - "not found" - } - fn response(&self) -> Option { let mut response = json_response(&Bad { errors: vec![StringError { @@ -257,10 +212,6 @@ impl fmt::Display for NotFound { pub struct Unauthorized; impl AppError for Unauthorized { - fn description(&self) -> &str { - "unauthorized" - } - fn response(&self) -> Option { let mut response = json_response(&Bad { errors: vec![StringError { @@ -282,10 +233,6 @@ impl fmt::Display for Unauthorized { struct BadRequest(String); impl AppError for BadRequest { - fn description(&self) -> &str { - self.0.as_ref() - } - fn response(&self) -> Option { let mut response = json_response(&Bad { errors: vec![StringError { @@ -304,20 +251,8 @@ impl fmt::Display for BadRequest { } pub fn internal(error: &S) -> Box { - Box::new(ConcreteAppError { - description: error.to_string(), - detail: None, - cause: None, - cargo_err: false, - }) -} - -pub fn cargo_err(error: &S) -> Box { - Box::new(ConcreteAppError { + Box::new(InternalAppError { description: error.to_string(), - detail: None, - cause: None, - cargo_err: true, }) } @@ -335,23 +270,11 @@ pub fn bad_request(error: &S) -> Box { #[derive(Debug)] pub struct AppErrToStdErr(pub Box); -impl Error for AppErrToStdErr { - fn description(&self) -> &str { - self.0.description() - } -} +impl Error for AppErrToStdErr {} impl fmt::Display for AppErrToStdErr { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0)?; - - let mut err = &*self.0; - while let Some(cause) = err.cause() { - err = cause; - write!(f, "\nCaused by: {}", err)?; - } - - Ok(()) + self.0.fmt(f) } } @@ -367,10 +290,6 @@ pub(crate) fn std_error_no_send(e: Box) -> Box { pub struct ReadOnlyMode; impl AppError for ReadOnlyMode { - fn description(&self) -> &str { - "tried to write in read only mode" - } - fn response(&self) -> Option { let mut response = json_response(&Bad { errors: vec![StringError { @@ -396,10 +315,6 @@ pub struct TooManyRequests { } impl AppError for TooManyRequests { - fn description(&self) -> &str { - "too many requests" - } - fn response(&self) -> Option { const HTTP_DATE_FORMAT: &str = "%a, %d %b %Y %H:%M:%S GMT"; let retry_after = self.retry_after.format(HTTP_DATE_FORMAT); @@ -427,3 +342,61 @@ impl fmt::Display for TooManyRequests { "Too many requests".fmt(f) } } + +#[test] +fn chain_error_internal() { + assert_eq!( + None::<()> + .chain_error(|| internal("inner")) + .chain_error(|| internal("middle")) + .chain_error(|| internal("outer")) + .unwrap_err() + .to_string(), + "outer caused by middle caused by inner" + ); + assert_eq!( + Err::<(), _>(internal("inner")) + .chain_error(|| internal("outer")) + .unwrap_err() + .to_string(), + "outer caused by inner" + ); + + // Don't do this, the user will see a generic 500 error instead of the intended message + assert_eq!( + Err::<(), _>(cargo_err("inner")) + .chain_error(|| internal("outer")) + .unwrap_err() + .to_string(), + "outer caused by inner" + ); + assert_eq!( + Err::<(), _>(Unauthorized) + .chain_error(|| internal("outer")) + .unwrap_err() + .to_string(), + "outer caused by must be logged in to perform that action" + ); +} + +#[test] +fn chain_error_user_facing() { + // Do this rarely, the user will only see the outer error + assert_eq!( + Err::<(), _>(cargo_err("inner")) + .chain_error(|| cargo_err("outer")) + .unwrap_err() + .to_string(), + "outer caused by inner" // never logged + ); + + // The outer error is sent as a response to the client. + // The inner error never bubbles up to the logging middleware + assert_eq!( + Err::<(), _>(std::io::Error::from(std::io::ErrorKind::PermissionDenied)) + .chain_error(|| cargo_err("outer")) + .unwrap_err() + .to_string(), + "outer caused by permission denied" // never logged + ); +} diff --git a/src/util/errors/http.rs b/src/util/errors/http.rs new file mode 100644 index 00000000000..2ee59fe15cc --- /dev/null +++ b/src/util/errors/http.rs @@ -0,0 +1,45 @@ +use std::fmt; + +use conduit::Response; + +use super::{AppError, Bad, StringError}; +use crate::util::json_response; + +#[derive(Debug)] +pub(super) struct Ok(pub(super) String); +#[derive(Debug)] +pub(super) struct ServerError(pub(super) String); + +impl AppError for Ok { + fn response(&self) -> Option { + Some(json_response(&Bad { + errors: vec![StringError { + detail: self.0.clone(), + }], + })) + } +} + +impl fmt::Display for Ok { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.fmt(f) + } +} + +impl AppError for ServerError { + fn response(&self) -> Option { + let mut response = json_response(&Bad { + errors: vec![StringError { + detail: self.0.clone(), + }], + }); + response.status = (500, "Internal Server Error"); + Some(response) + } +} + +impl fmt::Display for ServerError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.fmt(f) + } +}