Cleanup backend json error structs

src/controllers/util.rs

@@ -2,7 +2,7 @@ use super::prelude::*;
 
 use crate::middleware::current_user::TrustedUserId;
 use crate::models::{ApiToken, User};
-use crate::util::errors::{internal, AppError, AppResult, ChainError, Unauthorized};
+use crate::util::errors::{forbidden, internal, AppResult, ChainError};
 
 #[derive(Debug)]
 pub struct AuthenticatedUser {
@@ -26,7 +26,7 @@ impl AuthenticatedUser {
 }
 
 impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
-    /// Obtain `AuthenticatedUser` for the request or return an `Unauthorized` error
+    /// Obtain `AuthenticatedUser` for the request or return an `Forbidden` error
     fn authenticate(&self, conn: &PgConnection) -> AppResult<AuthenticatedUser> {
         if let Some(id) = self.extensions().find::<TrustedUserId>() {
             // A trusted user_id was provided by a signed cookie (or a test `MockCookieUser`)
@@ -43,11 +43,10 @@ impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
                         token_id: Some(token.id),
                     })
                     .chain_error(|| internal("invalid token"))
-                    .chain_error(|| Box::new(Unauthorized) as Box<dyn AppError>)
+                    .chain_error(forbidden)
             } else {
                 // Unable to authenticate the user
-                Err(internal("no cookie session or auth header found"))
-                    .chain_error(|| Box::new(Unauthorized) as Box<dyn AppError>)
+                Err(internal("no cookie session or auth header found")).chain_error(forbidden)
             }
         }
     }

src/github.rs

@@ -8,7 +8,7 @@ use serde::de::DeserializeOwned;
 use std::str;
 
 use crate::app::App;
-use crate::util::errors::{cargo_err, internal, AppError, AppResult, NotFound};
+use crate::util::errors::{cargo_err, internal, not_found, AppError, AppResult};
 
 /// Does all the nonsense for sending a GET to Github. Doesn't handle parsing
 /// because custom error-code handling may be desirable. Use
@@ -46,7 +46,7 @@ fn handle_error_response(app: &App, error: &reqwest::Error) -> Box<dyn AppError>
              https://{}/login",
             app.config.domain_name,
         )),
-        Some(Status::NOT_FOUND) => Box::new(NotFound),
+        Some(Status::NOT_FOUND) => not_found(),
         _ => internal(&format_args!(
             "didn't get a 200 result from github: {}",
             error

src/router.rs

@@ -178,7 +178,7 @@ impl Handler for R404 {
                 req.mut_extensions().insert(m.params.clone());
                 m.handler.call(req)
             }
-            Err(..) => Ok(NotFound.response().unwrap()),
+            Err(..) => Ok(NotFound.into()),
         }
     }
 }
@@ -187,7 +187,7 @@ impl Handler for R404 {
 mod tests {
     use super::*;
     use crate::util::errors::{
-        bad_request, cargo_err, internal, AppError, ChainError, NotFound, Unauthorized,
+        bad_request, cargo_err, forbidden, internal, not_found, AppError, ChainError,
     };
     use crate::util::EndpointResult;
 
@@ -209,7 +209,7 @@ mod tests {
             StatusCode::BAD_REQUEST
         );
         assert_eq!(
-            C(|_| err(Unauthorized)).call(&mut req).unwrap().status(),
+            C(|_| Err(forbidden())).call(&mut req).unwrap().status(),
             StatusCode::FORBIDDEN
         );
         assert_eq!(
@@ -220,7 +220,7 @@ mod tests {
             StatusCode::NOT_FOUND
         );
         assert_eq!(
-            C(|_| err(NotFound)).call(&mut req).unwrap().status(),
+            C(|_| Err(not_found())).call(&mut req).unwrap().status(),
             StatusCode::NOT_FOUND
         );
 

src/util/errors.rs

@@ -16,22 +16,22 @@ use std::any::{Any, TypeId};
 use std::error::Error;
 use std::fmt;
 
-use chrono::NaiveDateTime;
-use conduit::{header, StatusCode};
 use diesel::result::Error as DieselError;
 
-use crate::util::{json_response, AppResponse};
+use crate::util::AppResponse;
 
 pub(super) mod concrete;
-mod http;
+mod json;
+
+pub(crate) use json::{NotFound, ReadOnlyMode, TooManyRequests};
 
 /// 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<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
-    Box::new(http::Ok(error.to_string()))
+    Box::new(json::Ok(error.to_string()))
 }
 
 // The following are intended to be used for errors being sent back to the Ember
@@ -41,30 +41,20 @@ pub fn cargo_err<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
 
 /// Return an error with status 400 and the provided description as JSON
 pub fn bad_request<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
-    Box::new(http::BadRequest(error.to_string()))
+    Box::new(json::BadRequest(error.to_string()))
 }
 
-/// Returns an error with status 500 and the provided description as JSON
-pub fn server_error<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
-    Box::new(http::ServerError(error.to_string()))
+pub fn forbidden() -> Box<dyn AppError> {
+    Box::new(json::Forbidden)
 }
 
-#[derive(Serialize)]
-struct StringError<'a> {
-    detail: &'a str,
-}
-#[derive(Serialize)]
-struct Bad<'a> {
-    errors: Vec<StringError<'a>>,
+pub fn not_found() -> Box<dyn AppError> {
+    Box::new(json::NotFound)
 }
 
-/// Generates a response with the provided status and description as JSON
-fn json_error(detail: &str, status: StatusCode) -> AppResponse {
-    let mut response = json_response(&Bad {
-        errors: vec![StringError { detail }],
-    });
-    *response.status_mut() = status;
-    response
+/// Returns an error with status 500 and the provided description as JSON
+pub fn server_error<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
+    Box::new(json::ServerError(error.to_string()))
 }
 
 // =============================================================================
@@ -99,7 +89,7 @@ impl dyn AppError {
 
     fn try_convert(err: &(dyn Error + Send + 'static)) -> Option<Box<Self>> {
         match err.downcast_ref() {
-            Some(DieselError::NotFound) => Some(Box::new(NotFound)),
+            Some(DieselError::NotFound) => Some(not_found()),
             Some(DieselError::DatabaseError(_, info))
                 if info.message().ends_with("read-only transaction") =>
             {
@@ -217,45 +207,6 @@ impl AppError for InternalAppError {
     }
 }
 
-// TODO: The remaining can probably move under `http`
-
-#[derive(Debug, Clone, Copy)]
-pub struct NotFound;
-
-impl From<NotFound> for AppResponse {
-    fn from(_: NotFound) -> AppResponse {
-        json_error("Not Found", StatusCode::NOT_FOUND)
-    }
-}
-
-impl AppError for NotFound {
-    fn response(&self) -> Option<AppResponse> {
-        Some(NotFound.into())
-    }
-}
-
-impl fmt::Display for NotFound {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "Not Found".fmt(f)
-    }
-}
-
-#[derive(Debug, Clone, Copy)]
-pub struct Unauthorized;
-
-impl AppError for Unauthorized {
-    fn response(&self) -> Option<AppResponse> {
-        let detail = "must be logged in to perform that action";
-        Some(json_error(detail, StatusCode::FORBIDDEN))
-    }
-}
-
-impl fmt::Display for Unauthorized {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "must be logged in to perform that action".fmt(f)
-    }
-}
-
 pub fn internal<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
     Box::new(InternalAppError {
         description: error.to_string(),
@@ -277,59 +228,6 @@ pub(crate) fn std_error(e: Box<dyn AppError>) -> Box<dyn Error + Send> {
     Box::new(AppErrToStdErr(e))
 }
 
-#[derive(Debug, Clone, Copy)]
-pub struct ReadOnlyMode;
-
-impl AppError for ReadOnlyMode {
-    fn response(&self) -> Option<AppResponse> {
-        let detail = "Crates.io is currently in read-only mode for maintenance. \
-                      Please try again later.";
-        Some(json_error(detail, StatusCode::SERVICE_UNAVAILABLE))
-    }
-}
-
-impl fmt::Display for ReadOnlyMode {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "Tried to write in read only mode".fmt(f)
-    }
-}
-
-#[derive(Debug, Clone, Copy)]
-pub struct TooManyRequests {
-    pub retry_after: NaiveDateTime,
-}
-
-impl AppError for TooManyRequests {
-    fn response(&self) -> Option<AppResponse> {
-        use std::convert::TryInto;
-
-        const HTTP_DATE_FORMAT: &str = "%a, %d %b %Y %H:%M:%S GMT";
-        let retry_after = self.retry_after.format(HTTP_DATE_FORMAT);
-
-        let detail = format!(
-            "You have published too many crates in a \
-             short period of time. Please try again after {} or email \
-             [email protected] to have your limit increased.",
-            retry_after
-        );
-        let mut response = json_error(&detail, StatusCode::TOO_MANY_REQUESTS);
-        response.headers_mut().insert(
-            header::RETRY_AFTER,
-            retry_after
-                .to_string()
-                .try_into()
-                .expect("HTTP_DATE_FORMAT contains invalid char"),
-        );
-        Some(response)
-    }
-}
-
-impl fmt::Display for TooManyRequests {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "Too many requests".fmt(f)
-    }
-}
-
 #[test]
 fn chain_error_internal() {
     assert_eq!(
@@ -358,7 +256,7 @@ fn chain_error_internal() {
         "outer caused by inner"
     );
     assert_eq!(
-        Err::<(), _>(Unauthorized)
+        Err::<(), _>(forbidden())
             .chain_error(|| internal("outer"))
             .unwrap_err()
             .to_string(),