From ce50f26d4ea18de7b210dc1a673bca81dfa80564 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 21 Aug 2023 17:25:53 +0100 Subject: [PATCH] feat: [#257] improve bad request error message for uploading Add a custom error for each case so that the Bad Request response constains a descriptive error message. --- src/errors.rs | 6 +- src/models/torrent.rs | 12 +--- src/models/torrent_file.rs | 1 + src/services/torrent.rs | 77 +++++++++++++++------ src/web/api/v1/contexts/torrent/errors.rs | 61 ++++++++++++++++ src/web/api/v1/contexts/torrent/handlers.rs | 73 ++++++++----------- src/web/api/v1/contexts/torrent/mod.rs | 1 + src/web/api/v1/responses.rs | 3 +- 8 files changed, 154 insertions(+), 80 deletions(-) create mode 100644 src/web/api/v1/contexts/torrent/errors.rs diff --git a/src/errors.rs b/src/errors.rs index 01b64d2d..c3cd08ea 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -97,8 +97,8 @@ pub enum ServiceError { #[display(fmt = "Torrent title is too short.")] InvalidTorrentTitleLength, - #[display(fmt = "Bad request.")] - BadRequest, + #[display(fmt = "Some mandatory metadata fields are missing.")] + MissingMandatoryMetadataFields, #[display(fmt = "Selected category does not exist.")] InvalidCategory, @@ -223,7 +223,7 @@ pub fn http_status_code_for_service_error(error: &ServiceError) -> StatusCode { ServiceError::InvalidTorrentPiecesLength => StatusCode::BAD_REQUEST, ServiceError::InvalidFileType => StatusCode::BAD_REQUEST, ServiceError::InvalidTorrentTitleLength => StatusCode::BAD_REQUEST, - ServiceError::BadRequest => StatusCode::BAD_REQUEST, + ServiceError::MissingMandatoryMetadataFields => StatusCode::BAD_REQUEST, ServiceError::InvalidCategory => StatusCode::BAD_REQUEST, ServiceError::InvalidTag => StatusCode::BAD_REQUEST, ServiceError::Unauthorized => StatusCode::FORBIDDEN, diff --git a/src/models/torrent.rs b/src/models/torrent.rs index 194b0c10..66a9616d 100644 --- a/src/models/torrent.rs +++ b/src/models/torrent.rs @@ -2,7 +2,6 @@ use serde::{Deserialize, Serialize}; use super::torrent_tag::TagId; use crate::errors::ServiceError; -use crate::models::torrent_file::Torrent; #[allow(clippy::module_name_repetitions)] pub type TorrentId = i64; @@ -23,13 +22,6 @@ pub struct TorrentListing { pub leechers: i64, } -#[allow(clippy::module_name_repetitions)] -#[derive(Debug)] -pub struct AddTorrentRequest { - pub metadata: Metadata, - pub torrent: Torrent, -} - #[derive(Debug, Deserialize)] pub struct Metadata { pub title: String, @@ -43,10 +35,10 @@ impl Metadata { /// /// # Errors /// - /// This function will return an `BadRequest` error if the `title` or the `category` is empty. + /// This function will return an error if the any of the mandatory metadata fields are missing. pub fn verify(&self) -> Result<(), ServiceError> { if self.title.is_empty() || self.category.is_empty() { - Err(ServiceError::BadRequest) + Err(ServiceError::MissingMandatoryMetadataFields) } else { Ok(()) } diff --git a/src/models/torrent_file.rs b/src/models/torrent_file.rs index 16080481..125a457e 100644 --- a/src/models/torrent_file.rs +++ b/src/models/torrent_file.rs @@ -229,6 +229,7 @@ impl Torrent { #[must_use] pub fn info_hash(&self) -> String { + // todo: return an InfoHash struct from_bytes(&self.calculate_info_hash_as_bytes()).to_lowercase() } diff --git a/src/services/torrent.rs b/src/services/torrent.rs index e8e6cef9..ec3d63a8 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -11,11 +11,12 @@ use crate::errors::ServiceError; use crate::models::category::CategoryId; use crate::models::info_hash::InfoHash; use crate::models::response::{DeletedTorrentResponse, TorrentResponse, TorrentsResponse}; -use crate::models::torrent::{AddTorrentRequest, TorrentId, TorrentListing}; +use crate::models::torrent::{Metadata, TorrentId, TorrentListing}; use crate::models::torrent_file::{DbTorrentInfo, Torrent, TorrentFile}; use crate::models::torrent_tag::{TagId, TorrentTag}; use crate::models::user::UserId; use crate::tracker::statistics_importer::StatisticsImporter; +use crate::utils::parse_torrent; use crate::{tracker, AsCSV}; const MIN_TORRENT_TITLE_LENGTH: usize = 3; @@ -34,6 +35,14 @@ pub struct Index { torrent_listing_generator: Arc, } +pub struct AddTorrentRequest { + pub title: String, + pub description: String, + pub category: String, + pub tags: Vec, + pub torrent_buffer: Vec, +} + /// User request to generate a torrent listing. #[derive(Debug, Deserialize)] pub struct ListingRequest { @@ -101,46 +110,75 @@ impl Index { /// * Unable to insert the torrent into the database. /// * Unable to add the torrent to the whitelist. /// * Torrent title is too short. - pub async fn add_torrent(&self, mut torrent_request: AddTorrentRequest, user_id: UserId) -> Result { + /// + /// # Panics + /// + /// This function will panic if: + /// + /// * Unable to parse the torrent info-hash. + pub async fn add_torrent( + &self, + add_torrent_form: AddTorrentRequest, + user_id: UserId, + ) -> Result<(TorrentId, InfoHash), ServiceError> { + let metadata = Metadata { + title: add_torrent_form.title, + description: add_torrent_form.description, + category: add_torrent_form.category, + tags: add_torrent_form.tags, + }; + + metadata.verify()?; + + let mut torrent = + parse_torrent::decode_torrent(&add_torrent_form.torrent_buffer).map_err(|_| ServiceError::InvalidTorrentFile)?; + + // Make sure that the pieces key has a length that is a multiple of 20 + if let Some(pieces) = torrent.info.pieces.as_ref() { + if pieces.as_ref().len() % 20 != 0 { + return Err(ServiceError::InvalidTorrentPiecesLength); + } + } + let _user = self.user_repository.get_compact(&user_id).await?; - torrent_request.torrent.set_announce_urls(&self.configuration).await; + torrent.set_announce_urls(&self.configuration).await; - if torrent_request.metadata.title.len() < MIN_TORRENT_TITLE_LENGTH { + if metadata.title.len() < MIN_TORRENT_TITLE_LENGTH { return Err(ServiceError::InvalidTorrentTitleLength); } let category = self .category_repository - .get_by_name(&torrent_request.metadata.category) + .get_by_name(&metadata.category) .await .map_err(|_| ServiceError::InvalidCategory)?; - let torrent_id = self.torrent_repository.add(&torrent_request, user_id, category).await?; + let torrent_id = self.torrent_repository.add(&torrent, &metadata, user_id, category).await?; + let info_hash: InfoHash = torrent + .info_hash() + .parse() + .expect("the parsed torrent should have a valid info hash"); drop( self.tracker_statistics_importer - .import_torrent_statistics(torrent_id, &torrent_request.torrent.info_hash()) + .import_torrent_statistics(torrent_id, &torrent.info_hash()) .await, ); // We always whitelist the torrent on the tracker because even if the tracker mode is `public` // it could be changed to `private` later on. - if let Err(e) = self - .tracker_service - .whitelist_info_hash(torrent_request.torrent.info_hash()) - .await - { + if let Err(e) = self.tracker_service.whitelist_info_hash(torrent.info_hash()).await { // If the torrent can't be whitelisted somehow, remove the torrent from database drop(self.torrent_repository.delete(&torrent_id).await); return Err(e); } self.torrent_tag_repository - .link_torrent_to_tags(&torrent_id, &torrent_request.metadata.tags) + .link_torrent_to_tags(&torrent_id, &metadata.tags) .await?; - Ok(torrent_id) + Ok((torrent_id, info_hash)) } /// Gets a torrent from the Index. @@ -436,18 +474,13 @@ impl DbTorrentRepository { /// This function will return an error there is a database error. pub async fn add( &self, - torrent_request: &AddTorrentRequest, + torrent: &Torrent, + metadata: &Metadata, user_id: UserId, category: Category, ) -> Result { self.database - .insert_torrent_and_get_id( - &torrent_request.torrent, - user_id, - category.category_id, - &torrent_request.metadata.title, - &torrent_request.metadata.description, - ) + .insert_torrent_and_get_id(torrent, user_id, category.category_id, &metadata.title, &metadata.description) .await } diff --git a/src/web/api/v1/contexts/torrent/errors.rs b/src/web/api/v1/contexts/torrent/errors.rs new file mode 100644 index 00000000..9bf24d48 --- /dev/null +++ b/src/web/api/v1/contexts/torrent/errors.rs @@ -0,0 +1,61 @@ +use axum::response::{IntoResponse, Response}; +use derive_more::{Display, Error}; +use hyper::StatusCode; + +use crate::web::api::v1::responses::{json_error_response, ErrorResponseData}; + +#[derive(Debug, Display, PartialEq, Eq, Error)] +pub enum Request { + #[display(fmt = "torrent title bytes are nota valid UTF8 string.")] + TitleIsNotValidUtf8, + + #[display(fmt = "torrent description bytes are nota valid UTF8 string.")] + DescriptionIsNotValidUtf8, + + #[display(fmt = "torrent category bytes are nota valid UTF8 string.")] + CategoryIsNotValidUtf8, + + #[display(fmt = "torrent tags arrays bytes are nota valid UTF8 string array.")] + TagsArrayIsNotValidUtf8, + + #[display(fmt = "torrent tags string is not a valid JSON.")] + TagsArrayIsNotValidJson, + + #[display(fmt = "upload torrent request header `content-type` should be `application/x-bittorrent`.")] + InvalidFileType, + + #[display(fmt = "cannot write uploaded torrent bytes (binary file) into memory.")] + CannotWriteChunkFromUploadedBinary, + + #[display(fmt = "cannot read a chunk of bytes from the uploaded torrent file. Review the request body size limit.")] + CannotReadChunkFromUploadedBinary, + + #[display(fmt = "provided path param for Info-hash is not valid.")] + InvalidInfoHashParam, +} + +impl IntoResponse for Request { + fn into_response(self) -> Response { + json_error_response( + http_status_code_for_handler_error(&self), + &ErrorResponseData { error: self.to_string() }, + ) + } +} + +#[must_use] +pub fn http_status_code_for_handler_error(error: &Request) -> StatusCode { + #[allow(clippy::match_same_arms)] + match error { + Request::TitleIsNotValidUtf8 => StatusCode::BAD_REQUEST, + Request::DescriptionIsNotValidUtf8 => StatusCode::BAD_REQUEST, + Request::CategoryIsNotValidUtf8 => StatusCode::BAD_REQUEST, + Request::TagsArrayIsNotValidUtf8 => StatusCode::BAD_REQUEST, + Request::TagsArrayIsNotValidJson => StatusCode::BAD_REQUEST, + Request::InvalidFileType => StatusCode::BAD_REQUEST, + Request::InvalidInfoHashParam => StatusCode::BAD_REQUEST, + // Internal errors processing the request + Request::CannotWriteChunkFromUploadedBinary => StatusCode::INTERNAL_SERVER_ERROR, + Request::CannotReadChunkFromUploadedBinary => StatusCode::INTERNAL_SERVER_ERROR, + } +} diff --git a/src/web/api/v1/contexts/torrent/handlers.rs b/src/web/api/v1/contexts/torrent/handlers.rs index 2621ff9c..31cd29ca 100644 --- a/src/web/api/v1/contexts/torrent/handlers.rs +++ b/src/web/api/v1/contexts/torrent/handlers.rs @@ -10,14 +10,14 @@ use axum::Json; use serde::Deserialize; use uuid::Uuid; +use super::errors; use super::forms::UpdateTorrentInfoForm; use super::responses::{new_torrent_response, torrent_file_response}; use crate::common::AppData; use crate::errors::ServiceError; use crate::models::info_hash::InfoHash; -use crate::models::torrent::{AddTorrentRequest, Metadata}; use crate::models::torrent_tag::TagId; -use crate::services::torrent::ListingRequest; +use crate::services::torrent::{AddTorrentRequest, ListingRequest}; use crate::services::torrent_file::generate_random_torrent; use crate::utils::parse_torrent; use crate::web::api::v1::auth::get_optional_logged_in_user; @@ -43,15 +43,13 @@ pub async fn upload_torrent_handler( Err(error) => return error.into_response(), }; - let torrent_request = match get_torrent_request_from_payload(multipart).await { + let add_torrent_form = match build_add_torrent_request_from_payload(multipart).await { Ok(torrent_request) => torrent_request, Err(error) => return error.into_response(), }; - let info_hash = torrent_request.torrent.info_hash().clone(); - - match app_data.torrent_service.add_torrent(torrent_request, user_id).await { - Ok(torrent_id) => new_torrent_response(torrent_id, &info_hash).into_response(), + match app_data.torrent_service.add_torrent(add_torrent_form, user_id).await { + Ok(torrent_ids) => new_torrent_response(torrent_ids.0, &torrent_ids.1.to_hex_string()).into_response(), Err(error) => error.into_response(), } } @@ -69,7 +67,7 @@ impl InfoHashParam { /// /// # Errors /// -/// Returns `ServiceError::BadRequest` if the torrent info-hash is invalid. +/// Returns an error if the torrent info-hash is invalid. #[allow(clippy::unused_async)] pub async fn download_torrent_handler( State(app_data): State>, @@ -77,7 +75,7 @@ pub async fn download_torrent_handler( Path(info_hash): Path, ) -> Response { let Ok(info_hash) = InfoHash::from_str(&info_hash.lowercase()) else { - return ServiceError::BadRequest.into_response(); + return errors::Request::InvalidInfoHashParam.into_response(); }; let opt_user_id = match get_optional_logged_in_user(maybe_bearer_token, app_data.clone()).await { @@ -127,7 +125,7 @@ pub async fn get_torrent_info_handler( Path(info_hash): Path, ) -> Response { let Ok(info_hash) = InfoHash::from_str(&info_hash.lowercase()) else { - return ServiceError::BadRequest.into_response(); + return errors::Request::InvalidInfoHashParam.into_response(); }; let opt_user_id = match get_optional_logged_in_user(maybe_bearer_token, app_data.clone()).await { @@ -158,7 +156,7 @@ pub async fn update_torrent_info_handler( extract::Json(update_torrent_info_form): extract::Json, ) -> Response { let Ok(info_hash) = InfoHash::from_str(&info_hash.lowercase()) else { - return ServiceError::BadRequest.into_response(); + return errors::Request::InvalidInfoHashParam.into_response(); }; let user_id = match app_data.auth.get_user_id_from_bearer_token(&maybe_bearer_token).await { @@ -199,7 +197,7 @@ pub async fn delete_torrent_handler( Path(info_hash): Path, ) -> Response { let Ok(info_hash) = InfoHash::from_str(&info_hash.lowercase()) else { - return ServiceError::BadRequest.into_response(); + return errors::Request::InvalidInfoHashParam.into_response(); }; let user_id = match app_data.auth.get_user_id_from_bearer_token(&maybe_bearer_token).await { @@ -231,11 +229,11 @@ impl UuidParam { /// /// # Errors /// -/// Returns `ServiceError::BadRequest` if the torrent info-hash is invalid. +/// Returns an error if the torrent info-hash is invalid. #[allow(clippy::unused_async)] pub async fn create_random_torrent_handler(State(_app_data): State>, Path(uuid): Path) -> Response { let Ok(uuid) = Uuid::parse_str(&uuid.value()) else { - return ServiceError::BadRequest.into_response(); + return errors::Request::InvalidInfoHashParam.into_response(); }; let torrent = generate_random_torrent(uuid); @@ -259,7 +257,7 @@ pub async fn create_random_torrent_handler(State(_app_data): State> /// - The multipart content is invalid. /// - The torrent file pieces key has a length that is not a multiple of 20. /// - The binary data cannot be decoded as a torrent file. -async fn get_torrent_request_from_payload(mut payload: Multipart) -> Result { +async fn build_add_torrent_request_from_payload(mut payload: Multipart) -> Result { let torrent_buffer = vec![0u8]; let mut torrent_cursor = Cursor::new(torrent_buffer); @@ -277,69 +275,56 @@ async fn get_torrent_request_from_payload(mut payload: Multipart) -> Result { let data = field.bytes().await.unwrap(); if data.is_empty() { continue; }; - description = String::from_utf8(data.to_vec()).map_err(|_| ServiceError::BadRequest)?; + description = String::from_utf8(data.to_vec()).map_err(|_| errors::Request::DescriptionIsNotValidUtf8)?; } "category" => { let data = field.bytes().await.unwrap(); if data.is_empty() { continue; }; - category = String::from_utf8(data.to_vec()).map_err(|_| ServiceError::BadRequest)?; + category = String::from_utf8(data.to_vec()).map_err(|_| errors::Request::CategoryIsNotValidUtf8)?; } "tags" => { let data = field.bytes().await.unwrap(); if data.is_empty() { continue; }; - let string_data = String::from_utf8(data.to_vec()).map_err(|_| ServiceError::BadRequest)?; - tags = serde_json::from_str(&string_data).map_err(|_| ServiceError::BadRequest)?; + let string_data = String::from_utf8(data.to_vec()).map_err(|_| errors::Request::TagsArrayIsNotValidUtf8)?; + tags = serde_json::from_str(&string_data).map_err(|_| errors::Request::TagsArrayIsNotValidJson)?; } "torrent" => { let content_type = field.content_type().unwrap(); if content_type != "application/x-bittorrent" { - return Err(ServiceError::InvalidFileType); + return Err(errors::Request::InvalidFileType); } - while let Some(chunk) = field.chunk().await.map_err(|_| (ServiceError::BadRequest))? { - torrent_cursor.write_all(&chunk)?; + while let Some(chunk) = field + .chunk() + .await + .map_err(|_| (errors::Request::CannotReadChunkFromUploadedBinary))? + { + torrent_cursor + .write_all(&chunk) + .map_err(|_| (errors::Request::CannotWriteChunkFromUploadedBinary))?; } } _ => {} } } - let fields = Metadata { + Ok(AddTorrentRequest { title, description, category, tags, - }; - - fields.verify()?; - - let position = usize::try_from(torrent_cursor.position()).map_err(|_| ServiceError::InvalidTorrentFile)?; - let inner = torrent_cursor.get_ref(); - - let torrent = parse_torrent::decode_torrent(&inner[..position]).map_err(|_| ServiceError::InvalidTorrentFile)?; - - // Make sure that the pieces key has a length that is a multiple of 20 - // code-review: I think we could put this inside the service. - if let Some(pieces) = torrent.info.pieces.as_ref() { - if pieces.as_ref().len() % 20 != 0 { - return Err(ServiceError::InvalidTorrentPiecesLength); - } - } - - Ok(AddTorrentRequest { - metadata: fields, - torrent, + torrent_buffer: torrent_cursor.into_inner(), }) } diff --git a/src/web/api/v1/contexts/torrent/mod.rs b/src/web/api/v1/contexts/torrent/mod.rs index 6553ba7f..82536cb8 100644 --- a/src/web/api/v1/contexts/torrent/mod.rs +++ b/src/web/api/v1/contexts/torrent/mod.rs @@ -330,6 +330,7 @@ //! //! Refer to the [`DeletedTorrentResponse`](crate::models::response::DeletedTorrentResponse) //! struct for more information about the response attributes. +pub mod errors; pub mod forms; pub mod handlers; pub mod responses; diff --git a/src/web/api/v1/responses.rs b/src/web/api/v1/responses.rs index 3adb7442..397862df 100644 --- a/src/web/api/v1/responses.rs +++ b/src/web/api/v1/responses.rs @@ -39,7 +39,8 @@ impl IntoResponse for database::Error { } } -fn json_error_response(status_code: StatusCode, error_response_data: &ErrorResponseData) -> Response { +#[must_use] +pub fn json_error_response(status_code: StatusCode, error_response_data: &ErrorResponseData) -> Response { ( status_code, [(header::CONTENT_TYPE, "application/json")],