From f7a543dfdcfdb398de238d68ae6dd4de5fa92d1a Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Sun, 20 Oct 2024 18:34:51 +0400 Subject: [PATCH 01/12] refactor: Improve errors --- Cargo.lock | 32 +++++++++++++++++++++++++-- Cargo.toml | 1 + src/auth/handlers.rs | 51 ++++++++++++++++++++++++-------------------- src/auth/mod.rs | 15 ------------- src/errors.rs | 51 ++++++++++++++++++++++++++++++++++++++++++++ src/main.rs | 1 + 6 files changed, 111 insertions(+), 40 deletions(-) create mode 100644 src/errors.rs diff --git a/Cargo.lock b/Cargo.lock index ddc48d7..582b180 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -36,7 +36,7 @@ dependencies = [ "brotli", "bytes", "bytestring", - "derive_more", + "derive_more 0.99.17", "encoding_rs", "flate2", "futures-core", @@ -170,7 +170,7 @@ dependencies = [ "bytestring", "cfg-if", "cookie", - "derive_more", + "derive_more 0.99.17", "encoding_rs", "futures-core", "futures-util", @@ -654,6 +654,27 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "derive_more" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4a9b99b9cbbe49445b21764dc0625032a89b145a2642e67603e1c936f5458d05" +dependencies = [ + "derive_more-impl", +] + +[[package]] +name = "derive_more-impl" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb7330aeadfbe296029522e6c40f315320aba36fc43a5b3632f3795348f3bd22" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.59", + "unicode-xid", +] + [[package]] name = "digest" version = "0.10.7" @@ -1264,6 +1285,7 @@ dependencies = [ "anyhow", "bcrypt", "chrono", + "derive_more 1.0.0", "dotenvy", "env_logger", "figment", @@ -2373,6 +2395,12 @@ version = "1.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d4c87d22b6e3f4a18d4d40ef354e97c90fcb14dd91d7dc0aa9d8a1172ebf7202" +[[package]] +name = "unicode-xid" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f962df74c8c05a667b5ee8bcf162993134c104e96440b663c8daa176dc772d8c" + [[package]] name = "unicode_categories" version = "0.1.1" diff --git a/Cargo.toml b/Cargo.toml index 7023423..be28f34 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ actix-web = { version = "4.5.1", features = ["rustls", "cookies"] } anyhow = "1.0.81" bcrypt = "0.15.1" chrono = { version = "0.4.37", features = ["serde"] } +derive_more = { version = "1.0.0", features = ["display", "error"] } dotenvy = "0.15.7" env_logger = "0.11.3" figment = { version = "0.10.17", features = ["env"] } diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index 65134d3..58c9a9d 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -1,9 +1,7 @@ use crate::{ - auth::{ - jwt::{create_tokens, validate_token}, - Error, Result, - }, + auth::jwt::{create_tokens, validate_token}, context::Context, + errors::APIError, models::{SignInCredentials, SignUpCredentials}, }; @@ -25,11 +23,11 @@ use time::OffsetDateTime; async fn register( ctx: Data, Json(user_data): Json, -) -> Result { +) -> Result { log::trace!("Received register request"); let mut user = user_data; - user.password = bcrypt::hash(user.password, bcrypt::DEFAULT_COST)?; + user.password = bcrypt::hash(user.password, bcrypt::DEFAULT_COST).unwrap(); ctx.db.add_user(user).await?; @@ -46,24 +44,26 @@ async fn register( async fn login( ctx: Data, Json(creds): Json, -) -> Result { +) -> Result { log::trace!("Received login request"); - let Ok(user) = ctx.db.get_user_by_creds(&creds).await else { - let _ = bcrypt::hash(&creds.password, bcrypt::DEFAULT_COST)?; - return Ok(HttpResponse::Unauthorized().finish()); - }; + let user = ctx.db.get_user_by_creds(&creds).await.map_err(|_| { + let _ = bcrypt::hash(&creds.password, bcrypt::DEFAULT_COST).unwrap(); + APIError::WrongCredentials + })?; - let utf8_hash = - std::str::from_utf8(&user.password).map_err(|_| Error::Auth)?; + let utf8_hash = std::str::from_utf8(&user.password) + .map_err(|_| APIError::WrongCredentials)?; - if !bcrypt::verify(&creds.password, utf8_hash)? { - return Err(Error::Auth); + if !bcrypt::verify(&creds.password, utf8_hash) + .map_err(|_| APIError::InternalServer)? + { + return Err(APIError::WrongCredentials); } log::trace!("User has been verified"); - let (access_token, refresh_token) = - create_tokens(&ctx.config, &user.email)?; + let (access_token, refresh_token) = create_tokens(&ctx.config, &user.email) + .map_err(|_| APIError::InternalServer)?; let cookie_to_add = |name, token| { Cookie::build(name, token) @@ -83,14 +83,19 @@ async fn login( ) )] #[post("/refresh")] -async fn refresh(ctx: Data, req: HttpRequest) -> Result { +async fn refresh( + ctx: Data, + req: HttpRequest, +) -> Result { let Some(cookie) = req.cookie("refresh_token") else { - return Ok(HttpResponse::Unauthorized().finish()); + return Err(APIError::InvalidToken); }; - let claims = validate_token(&ctx.config, cookie.value())?; - let (access_token, refresh_token) = - create_tokens(&ctx.config, &claims.sub)?; + let claims = validate_token(&ctx.config, cookie.value()) + .map_err(|_| APIError::InvalidToken)?; + + let (access_token, refresh_token) = create_tokens(&ctx.config, &claims.sub) + .map_err(|_| APIError::InternalServer)?; let cookie_to_add = |name, token| { Cookie::build(name, token) @@ -111,7 +116,7 @@ async fn refresh(ctx: Data, req: HttpRequest) -> Result { ) )] #[post("/logout")] -async fn logout() -> Result { +async fn logout() -> Result { // NOTE(granatam): We cannot delete cookies, so we explicitly set its // expiration time to the elapsed time let cookie_to_delete = |name| { diff --git a/src/auth/mod.rs b/src/auth/mod.rs index 8217d2a..0c10a50 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -1,17 +1,2 @@ pub mod handlers; mod jwt; - -#[derive(thiserror::Error, Debug)] -pub enum Error { - #[error("database error: {0}")] - Sqlx(#[from] sqlx::Error), - #[error("bcrypt error: {0}")] - Bcrypt(#[from] bcrypt::BcryptError), - #[error("jwt error: {0}")] - Jwt(#[from] jsonwebtoken::errors::Error), - #[error("auth error")] - Auth, -} -impl actix_web::error::ResponseError for Error {} - -pub type Result = std::result::Result; diff --git a/src/errors.rs b/src/errors.rs new file mode 100644 index 0000000..4c64bee --- /dev/null +++ b/src/errors.rs @@ -0,0 +1,51 @@ +use actix_web::{body::BoxBody, http::StatusCode, HttpResponse, ResponseError}; +use derive_more::{Display, Error}; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Display, Error)] +pub enum APIError { + NotFound, + WrongCredentials, + InvalidToken, + InternalServer, +} + +impl From for APIError { + fn from(value: sqlx::Error) -> Self { + match value { + sqlx::Error::RowNotFound => APIError::NotFound, + _ => APIError::InternalServer, + } + } +} + +#[derive(Serialize, Deserialize)] +struct ErrorBody { + pub error: &'static str, +} + +impl ResponseError for APIError { + fn status_code(&self) -> StatusCode { + match self { + APIError::NotFound => StatusCode::NOT_FOUND, + APIError::WrongCredentials => StatusCode::UNAUTHORIZED, + APIError::InvalidToken => StatusCode::UNAUTHORIZED, + APIError::InternalServer => StatusCode::INTERNAL_SERVER_ERROR, + } + } + + fn error_response(&self) -> HttpResponse { + HttpResponse::build(self.status_code()).json(match self { + APIError::NotFound => ErrorBody { error: "NOT_FOUND" }, + APIError::WrongCredentials => ErrorBody { + error: "WRONG_CREDENTIALS", + }, + APIError::InvalidToken => ErrorBody { + error: "INVALID_TOKEN", + }, + APIError::InternalServer => ErrorBody { + error: "INTERNAL_SERVER", + }, + }) + } +} diff --git a/src/main.rs b/src/main.rs index eae2f74..2745f31 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,6 +2,7 @@ mod auth; mod config; mod context; mod db; +mod errors; mod models; mod routes; From 2b432050f177d3b1a15e8e70d32dd5a9688d346b Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Mon, 21 Oct 2024 13:03:22 +0400 Subject: [PATCH 02/12] refactor: Rename errors in APIError --- src/auth/handlers.rs | 16 ++++++++-------- src/errors.rs | 28 ++++++++++++++-------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index 58c9a9d..46c5272 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -49,21 +49,21 @@ async fn login( let user = ctx.db.get_user_by_creds(&creds).await.map_err(|_| { let _ = bcrypt::hash(&creds.password, bcrypt::DEFAULT_COST).unwrap(); - APIError::WrongCredentials + APIError::WrongCredentialsError })?; let utf8_hash = std::str::from_utf8(&user.password) - .map_err(|_| APIError::WrongCredentials)?; + .map_err(|_| APIError::WrongCredentialsError)?; if !bcrypt::verify(&creds.password, utf8_hash) - .map_err(|_| APIError::InternalServer)? + .map_err(|_| APIError::InternalServerError)? { - return Err(APIError::WrongCredentials); + return Err(APIError::WrongCredentialsError); } log::trace!("User has been verified"); let (access_token, refresh_token) = create_tokens(&ctx.config, &user.email) - .map_err(|_| APIError::InternalServer)?; + .map_err(|_| APIError::InternalServerError)?; let cookie_to_add = |name, token| { Cookie::build(name, token) @@ -88,14 +88,14 @@ async fn refresh( req: HttpRequest, ) -> Result { let Some(cookie) = req.cookie("refresh_token") else { - return Err(APIError::InvalidToken); + return Err(APIError::InvalidTokenError); }; let claims = validate_token(&ctx.config, cookie.value()) - .map_err(|_| APIError::InvalidToken)?; + .map_err(|_| APIError::InvalidTokenError)?; let (access_token, refresh_token) = create_tokens(&ctx.config, &claims.sub) - .map_err(|_| APIError::InternalServer)?; + .map_err(|_| APIError::InternalServerError)?; let cookie_to_add = |name, token| { Cookie::build(name, token) diff --git a/src/errors.rs b/src/errors.rs index 4c64bee..f426653 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -4,17 +4,17 @@ use serde::{Deserialize, Serialize}; #[derive(Debug, Display, Error)] pub enum APIError { - NotFound, - WrongCredentials, - InvalidToken, - InternalServer, + NotFoundError, + WrongCredentialsError, + InvalidTokenError, + InternalServerError, } impl From for APIError { fn from(value: sqlx::Error) -> Self { match value { - sqlx::Error::RowNotFound => APIError::NotFound, - _ => APIError::InternalServer, + sqlx::Error::RowNotFound => APIError::NotFoundError, + _ => APIError::InternalServerError, } } } @@ -27,23 +27,23 @@ struct ErrorBody { impl ResponseError for APIError { fn status_code(&self) -> StatusCode { match self { - APIError::NotFound => StatusCode::NOT_FOUND, - APIError::WrongCredentials => StatusCode::UNAUTHORIZED, - APIError::InvalidToken => StatusCode::UNAUTHORIZED, - APIError::InternalServer => StatusCode::INTERNAL_SERVER_ERROR, + APIError::NotFoundError => StatusCode::NOT_FOUND, + APIError::WrongCredentialsError => StatusCode::UNAUTHORIZED, + APIError::InvalidTokenError => StatusCode::UNAUTHORIZED, + APIError::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR, } } fn error_response(&self) -> HttpResponse { HttpResponse::build(self.status_code()).json(match self { - APIError::NotFound => ErrorBody { error: "NOT_FOUND" }, - APIError::WrongCredentials => ErrorBody { + APIError::NotFoundError => ErrorBody { error: "NOT_FOUND" }, + APIError::WrongCredentialsError => ErrorBody { error: "WRONG_CREDENTIALS", }, - APIError::InvalidToken => ErrorBody { + APIError::InvalidTokenError => ErrorBody { error: "INVALID_TOKEN", }, - APIError::InternalServer => ErrorBody { + APIError::InternalServerError => ErrorBody { error: "INTERNAL_SERVER", }, }) From de5eb21c7eac25e56c086b2ba9f2513ce7d03f5d Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Mon, 21 Oct 2024 13:05:24 +0400 Subject: [PATCH 03/12] chore: Add comment for unwrap --- src/auth/handlers.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index 46c5272..a7c62c7 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -48,6 +48,8 @@ async fn login( log::trace!("Received login request"); let user = ctx.db.get_user_by_creds(&creds).await.map_err(|_| { + // NOTE(t3m8ch): This line will never throw an error because it depends + // on the second argument we have fixed. That's why unwrap is used here. let _ = bcrypt::hash(&creds.password, bcrypt::DEFAULT_COST).unwrap(); APIError::WrongCredentialsError })?; From 5df4454117ef33069ddef215e2249dc76f872c97 Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Mon, 21 Oct 2024 13:08:07 +0400 Subject: [PATCH 04/12] fix: Clippy --- src/auth/handlers.rs | 10 +++++----- src/errors.rs | 20 ++++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index a7c62c7..0e7a42e 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -51,16 +51,16 @@ async fn login( // NOTE(t3m8ch): This line will never throw an error because it depends // on the second argument we have fixed. That's why unwrap is used here. let _ = bcrypt::hash(&creds.password, bcrypt::DEFAULT_COST).unwrap(); - APIError::WrongCredentialsError + APIError::WrongCredentials })?; let utf8_hash = std::str::from_utf8(&user.password) - .map_err(|_| APIError::WrongCredentialsError)?; + .map_err(|_| APIError::WrongCredentials)?; if !bcrypt::verify(&creds.password, utf8_hash) .map_err(|_| APIError::InternalServerError)? { - return Err(APIError::WrongCredentialsError); + return Err(APIError::WrongCredentials); } log::trace!("User has been verified"); @@ -90,11 +90,11 @@ async fn refresh( req: HttpRequest, ) -> Result { let Some(cookie) = req.cookie("refresh_token") else { - return Err(APIError::InvalidTokenError); + return Err(APIError::InvalidToken); }; let claims = validate_token(&ctx.config, cookie.value()) - .map_err(|_| APIError::InvalidTokenError)?; + .map_err(|_| APIError::InvalidToken)?; let (access_token, refresh_token) = create_tokens(&ctx.config, &claims.sub) .map_err(|_| APIError::InternalServerError)?; diff --git a/src/errors.rs b/src/errors.rs index f426653..3e411bb 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -4,16 +4,16 @@ use serde::{Deserialize, Serialize}; #[derive(Debug, Display, Error)] pub enum APIError { - NotFoundError, - WrongCredentialsError, - InvalidTokenError, + NotFound, + WrongCredentials, + InvalidToken, InternalServerError, } impl From for APIError { fn from(value: sqlx::Error) -> Self { match value { - sqlx::Error::RowNotFound => APIError::NotFoundError, + sqlx::Error::RowNotFound => APIError::NotFound, _ => APIError::InternalServerError, } } @@ -27,20 +27,20 @@ struct ErrorBody { impl ResponseError for APIError { fn status_code(&self) -> StatusCode { match self { - APIError::NotFoundError => StatusCode::NOT_FOUND, - APIError::WrongCredentialsError => StatusCode::UNAUTHORIZED, - APIError::InvalidTokenError => StatusCode::UNAUTHORIZED, + APIError::NotFound => StatusCode::NOT_FOUND, + APIError::WrongCredentials => StatusCode::UNAUTHORIZED, + APIError::InvalidToken => StatusCode::UNAUTHORIZED, APIError::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR, } } fn error_response(&self) -> HttpResponse { HttpResponse::build(self.status_code()).json(match self { - APIError::NotFoundError => ErrorBody { error: "NOT_FOUND" }, - APIError::WrongCredentialsError => ErrorBody { + APIError::NotFound => ErrorBody { error: "NOT_FOUND" }, + APIError::WrongCredentials => ErrorBody { error: "WRONG_CREDENTIALS", }, - APIError::InvalidTokenError => ErrorBody { + APIError::InvalidToken => ErrorBody { error: "INVALID_TOKEN", }, APIError::InternalServerError => ErrorBody { From 59eaca15ba887c5887c1d94b6396d199e0a1bdb9 Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Thu, 24 Oct 2024 18:15:53 +0400 Subject: [PATCH 05/12] refactor: Create APIResult --- src/auth/handlers.rs | 13 +++++-------- src/errors.rs | 2 ++ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index 0e7a42e..eb2e56f 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -1,7 +1,7 @@ use crate::{ auth::jwt::{create_tokens, validate_token}, context::Context, - errors::APIError, + errors::{APIError, APIResult}, models::{SignInCredentials, SignUpCredentials}, }; @@ -23,7 +23,7 @@ use time::OffsetDateTime; async fn register( ctx: Data, Json(user_data): Json, -) -> Result { +) -> APIResult { log::trace!("Received register request"); let mut user = user_data; @@ -44,7 +44,7 @@ async fn register( async fn login( ctx: Data, Json(creds): Json, -) -> Result { +) -> APIResult { log::trace!("Received login request"); let user = ctx.db.get_user_by_creds(&creds).await.map_err(|_| { @@ -85,10 +85,7 @@ async fn login( ) )] #[post("/refresh")] -async fn refresh( - ctx: Data, - req: HttpRequest, -) -> Result { +async fn refresh(ctx: Data, req: HttpRequest) -> APIResult { let Some(cookie) = req.cookie("refresh_token") else { return Err(APIError::InvalidToken); }; @@ -118,7 +115,7 @@ async fn refresh( ) )] #[post("/logout")] -async fn logout() -> Result { +async fn logout() -> APIResult { // NOTE(granatam): We cannot delete cookies, so we explicitly set its // expiration time to the elapsed time let cookie_to_delete = |name| { diff --git a/src/errors.rs b/src/errors.rs index 3e411bb..5e16656 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -10,6 +10,8 @@ pub enum APIError { InternalServerError, } +pub type APIResult = Result; + impl From for APIError { fn from(value: sqlx::Error) -> Self { match value { From e7b428430860f5e0c8968821cda8bc2641797f67 Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Thu, 24 Oct 2024 18:19:03 +0400 Subject: [PATCH 06/12] refactor: Use ok_or --- src/auth/handlers.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index eb2e56f..8bb7038 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -86,9 +86,7 @@ async fn login( )] #[post("/refresh")] async fn refresh(ctx: Data, req: HttpRequest) -> APIResult { - let Some(cookie) = req.cookie("refresh_token") else { - return Err(APIError::InvalidToken); - }; + let cookie = req.cookie("refresh_token").ok_or(APIError::InvalidToken)?; let claims = validate_token(&ctx.config, cookie.value()) .map_err(|_| APIError::InvalidToken)?; From 71986fbae319b3ef70f976771b8dbd853a8c54fc Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Thu, 24 Oct 2024 18:21:17 +0400 Subject: [PATCH 07/12] refactor: Change naming for errors --- src/auth/handlers.rs | 26 +++++++++++++------------- src/{errors.rs => error.rs} | 28 ++++++++++++++-------------- src/main.rs | 2 +- 3 files changed, 28 insertions(+), 28 deletions(-) rename src/{errors.rs => error.rs} (53%) diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index 8bb7038..287f968 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -1,7 +1,7 @@ use crate::{ auth::jwt::{create_tokens, validate_token}, context::Context, - errors::{APIError, APIResult}, + error::{ApiError, ApiResult}, models::{SignInCredentials, SignUpCredentials}, }; @@ -23,7 +23,7 @@ use time::OffsetDateTime; async fn register( ctx: Data, Json(user_data): Json, -) -> APIResult { +) -> ApiResult { log::trace!("Received register request"); let mut user = user_data; @@ -44,28 +44,28 @@ async fn register( async fn login( ctx: Data, Json(creds): Json, -) -> APIResult { +) -> ApiResult { log::trace!("Received login request"); let user = ctx.db.get_user_by_creds(&creds).await.map_err(|_| { // NOTE(t3m8ch): This line will never throw an error because it depends // on the second argument we have fixed. That's why unwrap is used here. let _ = bcrypt::hash(&creds.password, bcrypt::DEFAULT_COST).unwrap(); - APIError::WrongCredentials + ApiError::WrongCredentials })?; let utf8_hash = std::str::from_utf8(&user.password) - .map_err(|_| APIError::WrongCredentials)?; + .map_err(|_| ApiError::WrongCredentials)?; if !bcrypt::verify(&creds.password, utf8_hash) - .map_err(|_| APIError::InternalServerError)? + .map_err(|_| ApiError::InternalServerError)? { - return Err(APIError::WrongCredentials); + return Err(ApiError::WrongCredentials); } log::trace!("User has been verified"); let (access_token, refresh_token) = create_tokens(&ctx.config, &user.email) - .map_err(|_| APIError::InternalServerError)?; + .map_err(|_| ApiError::InternalServerError)?; let cookie_to_add = |name, token| { Cookie::build(name, token) @@ -85,14 +85,14 @@ async fn login( ) )] #[post("/refresh")] -async fn refresh(ctx: Data, req: HttpRequest) -> APIResult { - let cookie = req.cookie("refresh_token").ok_or(APIError::InvalidToken)?; +async fn refresh(ctx: Data, req: HttpRequest) -> ApiResult { + let cookie = req.cookie("refresh_token").ok_or(ApiError::InvalidToken)?; let claims = validate_token(&ctx.config, cookie.value()) - .map_err(|_| APIError::InvalidToken)?; + .map_err(|_| ApiError::InvalidToken)?; let (access_token, refresh_token) = create_tokens(&ctx.config, &claims.sub) - .map_err(|_| APIError::InternalServerError)?; + .map_err(|_| ApiError::InternalServerError)?; let cookie_to_add = |name, token| { Cookie::build(name, token) @@ -113,7 +113,7 @@ async fn refresh(ctx: Data, req: HttpRequest) -> APIResult { ) )] #[post("/logout")] -async fn logout() -> APIResult { +async fn logout() -> ApiResult { // NOTE(granatam): We cannot delete cookies, so we explicitly set its // expiration time to the elapsed time let cookie_to_delete = |name| { diff --git a/src/errors.rs b/src/error.rs similarity index 53% rename from src/errors.rs rename to src/error.rs index 5e16656..311f37a 100644 --- a/src/errors.rs +++ b/src/error.rs @@ -3,20 +3,20 @@ use derive_more::{Display, Error}; use serde::{Deserialize, Serialize}; #[derive(Debug, Display, Error)] -pub enum APIError { +pub enum ApiError { NotFound, WrongCredentials, InvalidToken, InternalServerError, } -pub type APIResult = Result; +pub type ApiResult = Result; -impl From for APIError { +impl From for ApiError { fn from(value: sqlx::Error) -> Self { match value { - sqlx::Error::RowNotFound => APIError::NotFound, - _ => APIError::InternalServerError, + sqlx::Error::RowNotFound => ApiError::NotFound, + _ => ApiError::InternalServerError, } } } @@ -26,26 +26,26 @@ struct ErrorBody { pub error: &'static str, } -impl ResponseError for APIError { +impl ResponseError for ApiError { fn status_code(&self) -> StatusCode { match self { - APIError::NotFound => StatusCode::NOT_FOUND, - APIError::WrongCredentials => StatusCode::UNAUTHORIZED, - APIError::InvalidToken => StatusCode::UNAUTHORIZED, - APIError::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR, + ApiError::NotFound => StatusCode::NOT_FOUND, + ApiError::WrongCredentials => StatusCode::UNAUTHORIZED, + ApiError::InvalidToken => StatusCode::UNAUTHORIZED, + ApiError::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR, } } fn error_response(&self) -> HttpResponse { HttpResponse::build(self.status_code()).json(match self { - APIError::NotFound => ErrorBody { error: "NOT_FOUND" }, - APIError::WrongCredentials => ErrorBody { + ApiError::NotFound => ErrorBody { error: "NOT_FOUND" }, + ApiError::WrongCredentials => ErrorBody { error: "WRONG_CREDENTIALS", }, - APIError::InvalidToken => ErrorBody { + ApiError::InvalidToken => ErrorBody { error: "INVALID_TOKEN", }, - APIError::InternalServerError => ErrorBody { + ApiError::InternalServerError => ErrorBody { error: "INTERNAL_SERVER", }, }) diff --git a/src/main.rs b/src/main.rs index 2745f31..fb422a5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,7 +2,7 @@ mod auth; mod config; mod context; mod db; -mod errors; +mod error; mod models; mod routes; From b6da5ef1d8d28a6e3c2e69a31a7f0f3c41a7487c Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Thu, 24 Oct 2024 18:32:31 +0400 Subject: [PATCH 08/12] refactor: Use Display trait for ErrorBody --- src/error.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/error.rs b/src/error.rs index 311f37a..8d28b03 100644 --- a/src/error.rs +++ b/src/error.rs @@ -23,7 +23,7 @@ impl From for ApiError { #[derive(Serialize, Deserialize)] struct ErrorBody { - pub error: &'static str, + pub error: String, } impl ResponseError for ApiError { @@ -37,17 +37,8 @@ impl ResponseError for ApiError { } fn error_response(&self) -> HttpResponse { - HttpResponse::build(self.status_code()).json(match self { - ApiError::NotFound => ErrorBody { error: "NOT_FOUND" }, - ApiError::WrongCredentials => ErrorBody { - error: "WRONG_CREDENTIALS", - }, - ApiError::InvalidToken => ErrorBody { - error: "INVALID_TOKEN", - }, - ApiError::InternalServerError => ErrorBody { - error: "INTERNAL_SERVER", - }, + HttpResponse::build(self.status_code()).json(ErrorBody { + error: format!("{}", self), }) } } From 0f5019dd074c263da446439ed4b695a7a55b9355 Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Thu, 24 Oct 2024 18:41:37 +0400 Subject: [PATCH 09/12] chore: Change comment --- src/auth/handlers.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index 287f968..b07413e 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -48,8 +48,8 @@ async fn login( log::trace!("Received login request"); let user = ctx.db.get_user_by_creds(&creds).await.map_err(|_| { - // NOTE(t3m8ch): This line will never throw an error because it depends - // on the second argument we have fixed. That's why unwrap is used here. + // NOTE(t3m8ch): This call will never end with an error, because it can only + // produce one, when the cost is invalid, which we cannot possibly have. let _ = bcrypt::hash(&creds.password, bcrypt::DEFAULT_COST).unwrap(); ApiError::WrongCredentials })?; From 82fde6dd9a46895d39ea57683ab795bc6d6e8b49 Mon Sep 17 00:00:00 2001 From: Evgeny Mangasaryan Date: Fri, 25 Oct 2024 01:57:24 +0400 Subject: [PATCH 10/12] Multiple changes to the error-handling * doc: Document fundamental error-handling types * feat: Utilize the `Display` trait and make string representation of error variants snake_case * fix: Treat all `sqlx` errors as internal --- src/auth/handlers.rs | 10 +++++++--- src/error.rs | 37 +++++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index b07413e..d2fa2f8 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -27,6 +27,8 @@ async fn register( log::trace!("Received register request"); let mut user = user_data; + // NOTE(evgenymng): This call will never end with an error, because it can + // only produce one, when the cost is invalid, which we cannot possibly have. user.password = bcrypt::hash(user.password, bcrypt::DEFAULT_COST).unwrap(); ctx.db.add_user(user).await?; @@ -50,6 +52,8 @@ async fn login( let user = ctx.db.get_user_by_creds(&creds).await.map_err(|_| { // NOTE(t3m8ch): This call will never end with an error, because it can only // produce one, when the cost is invalid, which we cannot possibly have. + // NOTE(evgenymng): Masquerade as if the user exists and spend time + // calculating hash. let _ = bcrypt::hash(&creds.password, bcrypt::DEFAULT_COST).unwrap(); ApiError::WrongCredentials })?; @@ -58,14 +62,14 @@ async fn login( .map_err(|_| ApiError::WrongCredentials)?; if !bcrypt::verify(&creds.password, utf8_hash) - .map_err(|_| ApiError::InternalServerError)? + .map_err(|_| ApiError::Internal)? { return Err(ApiError::WrongCredentials); } log::trace!("User has been verified"); let (access_token, refresh_token) = create_tokens(&ctx.config, &user.email) - .map_err(|_| ApiError::InternalServerError)?; + .map_err(|_| ApiError::Internal)?; let cookie_to_add = |name, token| { Cookie::build(name, token) @@ -92,7 +96,7 @@ async fn refresh(ctx: Data, req: HttpRequest) -> ApiResult { .map_err(|_| ApiError::InvalidToken)?; let (access_token, refresh_token) = create_tokens(&ctx.config, &claims.sub) - .map_err(|_| ApiError::InternalServerError)?; + .map_err(|_| ApiError::Internal)?; let cookie_to_add = |name, token| { Cookie::build(name, token) diff --git a/src/error.rs b/src/error.rs index 8d28b03..15fc07b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,44 +1,49 @@ -use actix_web::{body::BoxBody, http::StatusCode, HttpResponse, ResponseError}; +use actix_web::{http::StatusCode, HttpResponse, ResponseError}; use derive_more::{Display, Error}; -use serde::{Deserialize, Serialize}; +use serde::Serialize; -#[derive(Debug, Display, Error)] +/// Represents the complete set of error codes the API may produce. +#[derive(Serialize, Clone, Debug, Display, Error)] +// NOTE(evgenymng): Intentionally not making it `Copy`, because we +// will probably need to store some additional information in those variants +// in the future. pub enum ApiError { - NotFound, + #[display("wrong_credentials")] WrongCredentials, + #[display("invlid_token")] InvalidToken, - InternalServerError, + #[display("internal")] + Internal, } +/// A convenient alias for the API handler's return type. pub type ApiResult = Result; impl From for ApiError { - fn from(value: sqlx::Error) -> Self { - match value { - sqlx::Error::RowNotFound => ApiError::NotFound, - _ => ApiError::InternalServerError, - } + fn from(_: sqlx::Error) -> Self { + ApiError::Internal } } -#[derive(Serialize, Deserialize)] +/// Structured error's body. If the API call fails with an error, +/// an object with this structure is sent as a response. +#[derive(Serialize)] struct ErrorBody { - pub error: String, + pub error: ApiError, } impl ResponseError for ApiError { fn status_code(&self) -> StatusCode { match self { - ApiError::NotFound => StatusCode::NOT_FOUND, ApiError::WrongCredentials => StatusCode::UNAUTHORIZED, ApiError::InvalidToken => StatusCode::UNAUTHORIZED, - ApiError::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR, + ApiError::Internal => StatusCode::INTERNAL_SERVER_ERROR, } } - fn error_response(&self) -> HttpResponse { + fn error_response(&self) -> HttpResponse { HttpResponse::build(self.status_code()).json(ErrorBody { - error: format!("{}", self), + error: self.clone(), }) } } From f1ce2d7d3d9d0396324473c0e615d0efd871be72 Mon Sep 17 00:00:00 2001 From: Evgeny Mangasaryan Date: Fri, 25 Oct 2024 02:22:14 +0400 Subject: [PATCH 11/12] fix: Fix typo in the display macro --- src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/error.rs b/src/error.rs index 15fc07b..a45a46e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -10,7 +10,7 @@ use serde::Serialize; pub enum ApiError { #[display("wrong_credentials")] WrongCredentials, - #[display("invlid_token")] + #[display("invalid_token")] InvalidToken, #[display("internal")] Internal, From 0d869f99153bf2dad925eadb1acc04dff68c2453 Mon Sep 17 00:00:00 2001 From: Kudyakov Artem Date: Fri, 25 Oct 2024 06:50:14 +0400 Subject: [PATCH 12/12] refactor: Remove unnecessary logs --- src/auth/handlers.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/auth/handlers.rs b/src/auth/handlers.rs index d2fa2f8..5572de8 100644 --- a/src/auth/handlers.rs +++ b/src/auth/handlers.rs @@ -24,8 +24,6 @@ async fn register( ctx: Data, Json(user_data): Json, ) -> ApiResult { - log::trace!("Received register request"); - let mut user = user_data; // NOTE(evgenymng): This call will never end with an error, because it can // only produce one, when the cost is invalid, which we cannot possibly have. @@ -47,8 +45,6 @@ async fn login( ctx: Data, Json(creds): Json, ) -> ApiResult { - log::trace!("Received login request"); - let user = ctx.db.get_user_by_creds(&creds).await.map_err(|_| { // NOTE(t3m8ch): This call will never end with an error, because it can only // produce one, when the cost is invalid, which we cannot possibly have. @@ -66,7 +62,6 @@ async fn login( { return Err(ApiError::WrongCredentials); } - log::trace!("User has been verified"); let (access_token, refresh_token) = create_tokens(&ctx.config, &user.email) .map_err(|_| ApiError::Internal)?;