From 68e5d95d251ded79a019f681a2a6c1665aef751b Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Sat, 8 May 2021 22:28:08 -0400 Subject: [PATCH 1/2] admin: Specifically return 404 for user not found - Modify err_code to accept an expr for err_code - Add get_user_or_404, properly returning 404 instead of a generic 400 for cases where user is not found - Use get_user_or_404 where appropriate. --- src/api/admin.rs | 22 +++++++++++++++------- src/error.rs | 4 ++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/api/admin.rs b/src/api/admin.rs index 34875d7a..c8eec93a 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -4,7 +4,7 @@ use serde_json::Value; use std::{env, time::Duration}; use rocket::{ - http::{Cookie, Cookies, SameSite}, + http::{Cookie, Cookies, SameSite, Status}, request::{self, FlashMessage, Form, FromRequest, Outcome, Request}, response::{content::Html, Flash, Redirect}, Route, @@ -279,6 +279,14 @@ struct InviteData { email: String, } +fn get_user_or_404(uuid: &str, conn: &DbConn) -> ApiResult { + if let Some(user) = User::find_by_uuid(uuid, conn) { + Ok(user) + } else { + err_code!("User doesn't exist", Status::NotFound.code); + } +} + #[post("/invite", data = "")] fn invite_user(data: Json, _token: AdminToken, conn: DbConn) -> JsonResult { let data: InviteData = data.into_inner(); @@ -352,20 +360,20 @@ fn users_overview(_token: AdminToken, conn: DbConn) -> ApiResult> { #[get("/users/")] fn get_user_json(uuid: String, _token: AdminToken, conn: DbConn) -> JsonResult { - let user = User::find_by_uuid(&uuid, &conn).map_res("User doesn't exist")?; + let user = get_user_or_404(&uuid, &conn)?; Ok(Json(user.to_json(&conn))) } #[post("/users//delete")] fn delete_user(uuid: String, _token: AdminToken, conn: DbConn) -> EmptyResult { - let user = User::find_by_uuid(&uuid, &conn).map_res("User doesn't exist")?; + let user = get_user_or_404(&uuid, &conn)?; user.delete(&conn) } #[post("/users//deauth")] fn deauth_user(uuid: String, _token: AdminToken, conn: DbConn) -> EmptyResult { - let mut user = User::find_by_uuid(&uuid, &conn).map_res("User doesn't exist")?; + let mut user = get_user_or_404(&uuid, &conn)?; Device::delete_all_by_user(&user.uuid, &conn)?; user.reset_security_stamp(); @@ -374,7 +382,7 @@ fn deauth_user(uuid: String, _token: AdminToken, conn: DbConn) -> EmptyResult { #[post("/users//disable")] fn disable_user(uuid: String, _token: AdminToken, conn: DbConn) -> EmptyResult { - let mut user = User::find_by_uuid(&uuid, &conn).map_res("User doesn't exist")?; + let mut user = get_user_or_404(&uuid, &conn)?; Device::delete_all_by_user(&user.uuid, &conn)?; user.reset_security_stamp(); user.enabled = false; @@ -384,7 +392,7 @@ fn disable_user(uuid: String, _token: AdminToken, conn: DbConn) -> EmptyResult { #[post("/users//enable")] fn enable_user(uuid: String, _token: AdminToken, conn: DbConn) -> EmptyResult { - let mut user = User::find_by_uuid(&uuid, &conn).map_res("User doesn't exist")?; + let mut user = get_user_or_404(&uuid, &conn)?; user.enabled = true; user.save(&conn) @@ -392,7 +400,7 @@ fn enable_user(uuid: String, _token: AdminToken, conn: DbConn) -> EmptyResult { #[post("/users//remove-2fa")] fn remove_2fa(uuid: String, _token: AdminToken, conn: DbConn) -> EmptyResult { - let mut user = User::find_by_uuid(&uuid, &conn).map_res("User doesn't exist")?; + let mut user = get_user_or_404(&uuid, &conn)?; TwoFactor::delete_all_by_user(&user.uuid, &conn)?; user.totp_recover = None; user.save(&conn) diff --git a/src/error.rs b/src/error.rs index 52275cf1..08a499af 100644 --- a/src/error.rs +++ b/src/error.rs @@ -217,11 +217,11 @@ macro_rules! err { #[macro_export] macro_rules! err_code { - ($msg:expr, $err_code: literal) => {{ + ($msg:expr, $err_code: expr) => {{ error!("{}", $msg); return Err(crate::error::Error::new($msg, $msg).with_code($err_code)); }}; - ($usr_msg:expr, $log_value:expr, $err_code: literal) => {{ + ($usr_msg:expr, $log_value:expr, $err_code: expr) => {{ error!("{}. {}", $usr_msg, $log_value); return Err(crate::error::Error::new($usr_msg, $log_value).with_code($err_code)); }}; From e60bdc7efe247e6b93c7c99b1a44e7147cddbf31 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Sat, 8 May 2021 22:29:41 -0400 Subject: [PATCH 2/2] admin: Make invite_user error codes more specific - Return 409 Conflict for when a user with that email already exists - Return 500 InternalServerError for everything else --- src/api/admin.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/api/admin.rs b/src/api/admin.rs index c8eec93a..58be0ff6 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -292,19 +292,24 @@ fn invite_user(data: Json, _token: AdminToken, conn: DbConn) -> Json let data: InviteData = data.into_inner(); let email = data.email.clone(); if User::find_by_mail(&data.email, &conn).is_some() { - err!("User already exists") + err_code!("User already exists", Status::Conflict.code) } let mut user = User::new(email); - if CONFIG.mail_enabled() { - mail::send_invite(&user.email, &user.uuid, None, None, &CONFIG.invitation_org_name(), None)?; - } else { - let invitation = Invitation::new(data.email); - invitation.save(&conn)?; - } + // TODO: After try_blocks is stabilized, this can be made more readable + // See: https://github.com/rust-lang/rust/issues/31436 + (|| { + if CONFIG.mail_enabled() { + mail::send_invite(&user.email, &user.uuid, None, None, &CONFIG.invitation_org_name(), None)?; + } else { + let invitation = Invitation::new(data.email); + invitation.save(&conn)?; + } + + user.save(&conn) + })().map_err(|e| e.with_code(Status::InternalServerError.code))?; - user.save(&conn)?; Ok(Json(user.to_json(&conn))) }