Fix HTTP status codes for 2 non-cargo user endpoints

Author: jtgeibel

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/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::user::{UserNoEmailType, ALL_COLUMNS};
@@ -122,7 +121,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)]
@@ -136,17 +135,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>, _>(|| {
@@ -166,7 +165,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);
 
@@ -213,7 +212,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(|| {
@@ -223,7 +222,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"))
     })?;
 
     #[derive(Serialize)]

src/tests/user.rs

@@ -466,7 +466,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"),
         "{:?}",
@@ -475,7 +475,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"),
@@ -501,7 +501,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

src/util/errors.rs

@@ -8,6 +8,30 @@ 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<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
+    Box::new(ConcreteAppError {
+        description: error.to_string(),
+        cargo_err: true,
+    })
+}
+
+// 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 <https://github.com/rust-lang/cargo/issues/3995>), 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<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
+    Box::new(http::ServerError(error.to_string()))
+}
+
 #[derive(Serialize)]
 struct StringError {
     detail: String,
@@ -305,13 +329,6 @@ pub fn internal<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
     })
 }
 
-pub fn cargo_err<S: ToString + ?Sized>(error: &S) -> Box<dyn AppError> {
-    Box::new(ConcreteAppError {
-        description: error.to_string(),
-        cargo_err: true,
-    })
-}
-
 /// This is 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 <https://github.com/rust-lang/cargo/issues/3995>), but Ember requires

src/util/errors/http.rs

@@ -0,0 +1,31 @@
+use std::fmt;
+
+use conduit::Response;
+
+use super::{AppError, Bad, StringError};
+use crate::util::json_response;
+
+#[derive(Debug)]
+pub(super) struct ServerError(pub(super) String);
+
+impl AppError for ServerError {
+    fn description(&self) -> &str {
+        self.0.as_ref()
+    }
+
+    fn response(&self) -> Option<Response> {
+        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)
+    }
+}