From 7d50a171cfef486a1a9a3516d49a10af9b8b71a6 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 19 Sep 2023 15:49:48 +0100 Subject: [PATCH 01/11] refactor: reorganize test mods for torrent contract --- .../web/api/v1/contexts/torrent/contract.rs | 232 +++++++++--------- 1 file changed, 121 insertions(+), 111 deletions(-) diff --git a/tests/e2e/web/api/v1/contexts/torrent/contract.rs b/tests/e2e/web/api/v1/contexts/torrent/contract.rs index 3e577b37..e592ab76 100644 --- a/tests/e2e/web/api/v1/contexts/torrent/contract.rs +++ b/tests/e2e/web/api/v1/contexts/torrent/contract.rs @@ -451,154 +451,164 @@ mod for_authenticated_users { use torrust_index_backend::utils::parse_torrent::decode_torrent; use torrust_index_backend::web::api; - use uuid::Uuid; - use crate::common::asserts::assert_json_error_response; use crate::common::client::Client; - use crate::common::contexts::torrent::fixtures::{random_torrent, TestTorrent}; - use crate::common::contexts::torrent::forms::UploadTorrentMultipartForm; - use crate::common::contexts::torrent::responses::UploadedTorrentResponse; use crate::e2e::environment::TestEnv; use crate::e2e::web::api::v1::contexts::torrent::asserts::{build_announce_url, get_user_tracker_key}; use crate::e2e::web::api::v1::contexts::torrent::steps::upload_random_torrent_to_index; use crate::e2e::web::api::v1::contexts::user::steps::new_logged_in_user; - #[tokio::test] - async fn it_should_allow_authenticated_users_to_upload_new_torrents() { - let mut env = TestEnv::new(); - env.start(api::Version::V1).await; + mod uploading_a_torrent { - if !env.provides_a_tracker() { - println!("test skipped. It requires a tracker to be running."); - return; - } + use torrust_index_backend::web::api; + use uuid::Uuid; - let uploader = new_logged_in_user(&env).await; - let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + use crate::common::asserts::assert_json_error_response; + use crate::common::client::Client; + use crate::common::contexts::torrent::fixtures::{random_torrent, TestTorrent}; + use crate::common::contexts::torrent::forms::UploadTorrentMultipartForm; + use crate::common::contexts::torrent::responses::UploadedTorrentResponse; + use crate::e2e::environment::TestEnv; + use crate::e2e::web::api::v1::contexts::user::steps::new_logged_in_user; - let test_torrent = random_torrent(); - let info_hash = test_torrent.file_info_hash().clone(); + #[tokio::test] + async fn it_should_allow_authenticated_users_to_upload_new_torrents() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; - let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); + if !env.provides_a_tracker() { + println!("test skipped. It requires a tracker to be running."); + return; + } - let response = client.upload_torrent(form.into()).await; + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); - let uploaded_torrent_response: UploadedTorrentResponse = serde_json::from_str(&response.body).unwrap(); + let test_torrent = random_torrent(); + let info_hash = test_torrent.file_info_hash().clone(); - assert_eq!( - uploaded_torrent_response.data.info_hash.to_lowercase(), - info_hash.to_lowercase() - ); - assert!(response.is_json_and_ok()); - } + let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); - #[tokio::test] - async fn it_should_not_allow_uploading_a_torrent_with_a_non_existing_category() { - let mut env = TestEnv::new(); - env.start(api::Version::V1).await; + let response = client.upload_torrent(form.into()).await; - let uploader = new_logged_in_user(&env).await; - let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + let uploaded_torrent_response: UploadedTorrentResponse = serde_json::from_str(&response.body).unwrap(); - let mut test_torrent = random_torrent(); + assert_eq!( + uploaded_torrent_response.data.info_hash.to_lowercase(), + info_hash.to_lowercase() + ); + assert!(response.is_json_and_ok()); + } + + #[tokio::test] + async fn it_should_not_allow_uploading_a_torrent_with_a_non_existing_category() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; - test_torrent.index_info.category = "non-existing-category".to_string(); + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); - let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); + let mut test_torrent = random_torrent(); - let response = client.upload_torrent(form.into()).await; + test_torrent.index_info.category = "non-existing-category".to_string(); - assert_eq!(response.status, 400); - } + let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); - #[tokio::test] - async fn it_should_not_allow_uploading_a_torrent_with_a_title_that_already_exists() { - let mut env = TestEnv::new(); - env.start(api::Version::V1).await; + let response = client.upload_torrent(form.into()).await; - if !env.provides_a_tracker() { - println!("test skipped. It requires a tracker to be running."); - return; + assert_eq!(response.status, 400); } - let uploader = new_logged_in_user(&env).await; - let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + #[tokio::test] + async fn it_should_not_allow_uploading_a_torrent_with_a_title_that_already_exists() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; - // Upload the first torrent - let first_torrent = random_torrent(); - let first_torrent_title = first_torrent.index_info.title.clone(); - let form: UploadTorrentMultipartForm = first_torrent.index_info.into(); - let _response = client.upload_torrent(form.into()).await; - - // Upload the second torrent with the same title as the first one - let mut second_torrent = random_torrent(); - second_torrent.index_info.title = first_torrent_title; - let form: UploadTorrentMultipartForm = second_torrent.index_info.into(); - let response = client.upload_torrent(form.into()).await; - - assert_json_error_response(&response, "This torrent title has already been used."); - } + if !env.provides_a_tracker() { + println!("test skipped. It requires a tracker to be running."); + return; + } - #[tokio::test] - async fn it_should_not_allow_uploading_a_torrent_with_a_info_hash_that_already_exists() { - let mut env = TestEnv::new(); - env.start(api::Version::V1).await; + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); - if !env.provides_a_tracker() { - println!("test skipped. It requires a tracker to be running."); - return; + // Upload the first torrent + let first_torrent = random_torrent(); + let first_torrent_title = first_torrent.index_info.title.clone(); + let form: UploadTorrentMultipartForm = first_torrent.index_info.into(); + let _response = client.upload_torrent(form.into()).await; + + // Upload the second torrent with the same title as the first one + let mut second_torrent = random_torrent(); + second_torrent.index_info.title = first_torrent_title; + let form: UploadTorrentMultipartForm = second_torrent.index_info.into(); + let response = client.upload_torrent(form.into()).await; + + assert_json_error_response(&response, "This torrent title has already been used."); } - let uploader = new_logged_in_user(&env).await; - let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + #[tokio::test] + async fn it_should_not_allow_uploading_a_torrent_with_a_info_hash_that_already_exists() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; - // Upload the first torrent - let first_torrent = random_torrent(); - let mut first_torrent_clone = first_torrent.clone(); - let first_torrent_title = first_torrent.index_info.title.clone(); - let form: UploadTorrentMultipartForm = first_torrent.index_info.into(); - let _response = client.upload_torrent(form.into()).await; - - // Upload the second torrent with the same info-hash as the first one. - // We need to change the title otherwise the torrent will be rejected - // because of the duplicate title. - first_torrent_clone.index_info.title = format!("{first_torrent_title}-clone"); - let form: UploadTorrentMultipartForm = first_torrent_clone.index_info.into(); - let response = client.upload_torrent(form.into()).await; - - assert_eq!(response.status, 400); - } + if !env.provides_a_tracker() { + println!("test skipped. It requires a tracker to be running."); + return; + } - #[tokio::test] - async fn it_should_not_allow_uploading_a_torrent_whose_canonical_info_hash_already_exists() { - let mut env = TestEnv::new(); - env.start(api::Version::V1).await; + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); - if !env.provides_a_tracker() { - println!("test skipped. It requires a tracker to be running."); - return; + // Upload the first torrent + let first_torrent = random_torrent(); + let mut first_torrent_clone = first_torrent.clone(); + let first_torrent_title = first_torrent.index_info.title.clone(); + let form: UploadTorrentMultipartForm = first_torrent.index_info.into(); + let _response = client.upload_torrent(form.into()).await; + + // Upload the second torrent with the same info-hash as the first one. + // We need to change the title otherwise the torrent will be rejected + // because of the duplicate title. + first_torrent_clone.index_info.title = format!("{first_torrent_title}-clone"); + let form: UploadTorrentMultipartForm = first_torrent_clone.index_info.into(); + let response = client.upload_torrent(form.into()).await; + + assert_eq!(response.status, 400); } - let uploader = new_logged_in_user(&env).await; - let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + #[tokio::test] + async fn it_should_not_allow_uploading_a_torrent_whose_canonical_info_hash_already_exists() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; - let id1 = Uuid::new_v4(); + if !env.provides_a_tracker() { + println!("test skipped. It requires a tracker to be running."); + return; + } - // Upload the first torrent - let first_torrent = TestTorrent::with_custom_info_dict_field(id1, "data", "custom 01"); - let first_torrent_title = first_torrent.index_info.title.clone(); - let form: UploadTorrentMultipartForm = first_torrent.index_info.into(); - let _response = client.upload_torrent(form.into()).await; - - // Upload the second torrent with the same canonical info-hash as the first one. - // We need to change the title otherwise the torrent will be rejected - // because of the duplicate title. - let mut torrent_with_the_same_canonical_info_hash = TestTorrent::with_custom_info_dict_field(id1, "data", "custom 02"); - torrent_with_the_same_canonical_info_hash.index_info.title = format!("{first_torrent_title}-clone"); - let form: UploadTorrentMultipartForm = torrent_with_the_same_canonical_info_hash.index_info.into(); - let response = client.upload_torrent(form.into()).await; - - assert_eq!(response.status, 400); + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + + let id1 = Uuid::new_v4(); + + // Upload the first torrent + let first_torrent = TestTorrent::with_custom_info_dict_field(id1, "data", "custom 01"); + let first_torrent_title = first_torrent.index_info.title.clone(); + let form: UploadTorrentMultipartForm = first_torrent.index_info.into(); + let _response = client.upload_torrent(form.into()).await; + + // Upload the second torrent with the same canonical info-hash as the first one. + // We need to change the title otherwise the torrent will be rejected + // because of the duplicate title. + let mut torrent_with_the_same_canonical_info_hash = + TestTorrent::with_custom_info_dict_field(id1, "data", "custom 02"); + torrent_with_the_same_canonical_info_hash.index_info.title = format!("{first_torrent_title}-clone"); + let form: UploadTorrentMultipartForm = torrent_with_the_same_canonical_info_hash.index_info.into(); + let response = client.upload_torrent(form.into()).await; + + assert_eq!(response.status, 400); + } } #[tokio::test] From a302c227fe585469fb39e9c92243809eba98dcf0 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 19 Sep 2023 17:03:41 +0100 Subject: [PATCH 02/11] test: [#276] add more tests for torrent upload --- tests/common/client.rs | 4 +- tests/common/contexts/torrent/fixtures.rs | 2 + .../web/api/v1/contexts/torrent/contract.rs | 162 +++++++++++++++++- 3 files changed, 165 insertions(+), 3 deletions(-) diff --git a/tests/common/client.rs b/tests/common/client.rs index 05ffa17b..97216bfa 100644 --- a/tests/common/client.rs +++ b/tests/common/client.rs @@ -253,7 +253,7 @@ impl Http { .bearer_auth(token) .send() .await - .unwrap(), + .expect("failed to send multipart request with token"), None => reqwest::Client::builder() .build() .unwrap() @@ -261,7 +261,7 @@ impl Http { .multipart(form) .send() .await - .unwrap(), + .expect("failed to send multipart request without token"), }; TextResponse::from(response).await } diff --git a/tests/common/contexts/torrent/fixtures.rs b/tests/common/contexts/torrent/fixtures.rs index 3ee8c84a..223f8419 100644 --- a/tests/common/contexts/torrent/fixtures.rs +++ b/tests/common/contexts/torrent/fixtures.rs @@ -18,10 +18,12 @@ use crate::common::contexts::category::fixtures::software_category_name; /// Information about a torrent that is going to be added to the index. #[derive(Clone)] pub struct TorrentIndexInfo { + // Metadata pub title: String, pub description: String, pub category: String, pub tags: Option>, + // Other fields pub torrent_file: BinaryFile, pub name: String, } diff --git a/tests/e2e/web/api/v1/contexts/torrent/contract.rs b/tests/e2e/web/api/v1/contexts/torrent/contract.rs index e592ab76..a300cbc1 100644 --- a/tests/e2e/web/api/v1/contexts/torrent/contract.rs +++ b/tests/e2e/web/api/v1/contexts/torrent/contract.rs @@ -23,7 +23,8 @@ mod for_guests { use crate::common::client::Client; use crate::common::contexts::category::fixtures::software_predefined_category_id; use crate::common::contexts::torrent::asserts::assert_expected_torrent_details; - use crate::common::contexts::torrent::fixtures::TestTorrent; + use crate::common::contexts::torrent::fixtures::{random_torrent, TestTorrent}; + use crate::common::contexts::torrent::forms::UploadTorrentMultipartForm; use crate::common::contexts::torrent::requests::InfoHash; use crate::common::contexts::torrent::responses::{ Category, File, TorrentDetails, TorrentDetailsResponse, TorrentListResponse, @@ -426,6 +427,22 @@ mod for_guests { assert_eq!(response.status, 404); } + #[tokio::test] + async fn it_should_not_allow_guests_to_upload_torrents() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; + + let client = Client::unauthenticated(&env.server_socket_addr().unwrap()); + + let test_torrent = random_torrent(); + + let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); + + let response = client.upload_torrent(form.into()).await; + + assert_eq!(response.status, 401); + } + #[tokio::test] async fn it_should_not_allow_guests_to_delete_torrents() { let mut env = TestEnv::new(); @@ -500,6 +517,149 @@ mod for_authenticated_users { assert!(response.is_json_and_ok()); } + mod it_should_guard_that_torrent_metadata { + use torrust_index_backend::web::api; + + use crate::common::client::Client; + use crate::common::contexts::torrent::fixtures::random_torrent; + use crate::common::contexts::torrent::forms::UploadTorrentMultipartForm; + use crate::e2e::environment::TestEnv; + use crate::e2e::web::api::v1::contexts::user::steps::new_logged_in_user; + + #[tokio::test] + async fn contains_a_non_empty_category_name() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; + + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + + let mut test_torrent = random_torrent(); + + test_torrent.index_info.category = String::new(); + + let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); + + let response = client.upload_torrent(form.into()).await; + + assert_eq!(response.status, 400); + } + + #[tokio::test] + async fn contains_a_non_empty_title() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; + + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + + let mut test_torrent = random_torrent(); + + test_torrent.index_info.title = String::new(); + + let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); + + let response = client.upload_torrent(form.into()).await; + + assert_eq!(response.status, 400); + } + + #[tokio::test] + async fn title_has_at_least_3_chars() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; + + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + + let mut test_torrent = random_torrent(); + + test_torrent.index_info.title = "12".to_string(); + + let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); + + let response = client.upload_torrent(form.into()).await; + + assert_eq!(response.status, 400); + } + } + + mod it_should_guard_that_the_torrent_file { + + use torrust_index_backend::web::api; + + use crate::common::client::Client; + use crate::common::contexts::torrent::fixtures::random_torrent; + use crate::common::contexts::torrent::forms::UploadTorrentMultipartForm; + use crate::e2e::environment::TestEnv; + use crate::e2e::web::api::v1::contexts::user::steps::new_logged_in_user; + + #[tokio::test] + #[should_panic] + async fn contains_a_bencoded_dictionary_with_the_info_key_in_order_to_calculate_the_original_info_hash() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; + + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + + let mut test_torrent = random_torrent(); + + // Make the random torrent invalid by changing the bytes of the torrent file + let minimal_bencoded_value = b"de".to_vec(); + test_torrent.index_info.torrent_file.contents = minimal_bencoded_value; + + let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); + + let _response = client.upload_torrent(form.into()).await; + } + + #[tokio::test] + async fn contains_a_valid_metainfo_file() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; + + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + + let mut test_torrent = random_torrent(); + + // Make the random torrent invalid by changing the bytes of the torrent file. + // It's a valid bencoded format but an invalid torrent. It contains + // a `info` otherwise the test to validate the `info` key would fail. + // cspell:disable-next-line + let minimal_bencoded_value_with_info_key = b"d4:infod6:custom6:customee".to_vec(); + test_torrent.index_info.torrent_file.contents = minimal_bencoded_value_with_info_key; + + let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); + + let response = client.upload_torrent(form.into()).await; + + assert_eq!(response.status, 400); + } + + #[tokio::test] + async fn pieces_key_has_a_length_that_is_a_multiple_of_20() { + let mut env = TestEnv::new(); + env.start(api::Version::V1).await; + + let uploader = new_logged_in_user(&env).await; + let client = Client::authenticated(&env.server_socket_addr().unwrap(), &uploader.token); + + let mut test_torrent = random_torrent(); + + // cspell:disable-next-line + let torrent_with_19_pieces = b"d4:infod6:lengthi2e4:name42:torrent-with-invalid-pieces-key-length.txt12:piece lengthi16384e6:pieces19:\x3F\x78\x68\x50\xE3\x87\x55\x0F\xDA\xB8\x36\xED\x7E\x6D\xC8\x81\xDE\x23\x00ee"; + test_torrent.index_info.torrent_file.contents = torrent_with_19_pieces.to_vec(); + + let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); + + let response = client.upload_torrent(form.into()).await; + + assert_eq!(response.status, 400); + } + } + #[tokio::test] async fn it_should_not_allow_uploading_a_torrent_with_a_non_existing_category() { let mut env = TestEnv::new(); From ca6e97cdcef954100d3288403493a6f19f36c4da Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 20 Sep 2023 13:31:42 +0100 Subject: [PATCH 03/11] refactor: [#276] move metadata format validation to Metadata struct And other minor changes. --- src/databases/mysql.rs | 2 +- src/databases/sqlite.rs | 2 +- src/errors.rs | 13 ++++ src/models/torrent.rs | 62 ++++++++++++++-- src/models/torrent_file.rs | 18 ++--- src/services/torrent.rs | 73 +++++++++++-------- src/utils/parse_torrent.rs | 2 +- src/web/api/v1/contexts/torrent/handlers.rs | 12 ++- .../web/api/v1/contexts/torrent/contract.rs | 5 +- 9 files changed, 134 insertions(+), 55 deletions(-) diff --git a/src/databases/mysql.rs b/src/databases/mysql.rs index f793e5cb..f7dff1ff 100644 --- a/src/databases/mysql.rs +++ b/src/databases/mysql.rs @@ -436,7 +436,7 @@ impl Database for Mysql { title: &str, description: &str, ) -> Result { - let info_hash = torrent.info_hash_hex(); + let info_hash = torrent.canonical_info_hash_hex(); let canonical_info_hash = torrent.canonical_info_hash(); // open pool connection diff --git a/src/databases/sqlite.rs b/src/databases/sqlite.rs index 980e7a3b..ec8d2c01 100644 --- a/src/databases/sqlite.rs +++ b/src/databases/sqlite.rs @@ -426,7 +426,7 @@ impl Database for Sqlite { title: &str, description: &str, ) -> Result { - let info_hash = torrent.info_hash_hex(); + let info_hash = torrent.canonical_info_hash_hex(); let canonical_info_hash = torrent.canonical_info_hash(); // open pool connection diff --git a/src/errors.rs b/src/errors.rs index 25ea007d..29eb12f0 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -5,6 +5,7 @@ use derive_more::{Display, Error}; use hyper::StatusCode; use crate::databases::database; +use crate::models::torrent::MetadataError; pub type ServiceResult = Result; @@ -204,6 +205,18 @@ impl From for ServiceError { } } +impl From for ServiceError { + fn from(e: MetadataError) -> Self { + eprintln!("{e}"); + match e { + MetadataError::MissingTorrentTitle | MetadataError::MissingTorrentCategoryName => { + ServiceError::MissingMandatoryMetadataFields + } + MetadataError::InvalidTorrentTitleLength => ServiceError::InvalidTorrentTitleLength, + } + } +} + #[must_use] pub fn http_status_code_for_service_error(error: &ServiceError) -> StatusCode { #[allow(clippy::match_same_arms)] diff --git a/src/models/torrent.rs b/src/models/torrent.rs index 150d2bba..b86eb519 100644 --- a/src/models/torrent.rs +++ b/src/models/torrent.rs @@ -1,7 +1,9 @@ +use derive_more::{Display, Error}; use serde::{Deserialize, Serialize}; use super::torrent_tag::TagId; -use crate::errors::ServiceError; + +const MIN_TORRENT_TITLE_LENGTH: usize = 3; #[allow(clippy::module_name_repetitions)] pub type TorrentId = i64; @@ -24,6 +26,18 @@ pub struct TorrentListing { pub comment: Option, } +#[derive(Debug, Display, PartialEq, Eq, Error)] +pub enum MetadataError { + #[display(fmt = "Missing mandatory torrent title")] + MissingTorrentTitle, + + #[display(fmt = "Missing mandatory torrent category name")] + MissingTorrentCategoryName, + + #[display(fmt = "Torrent title is too short.")] + InvalidTorrentTitleLength, +} + #[derive(Debug, Deserialize)] pub struct Metadata { pub title: String, @@ -33,16 +47,48 @@ pub struct Metadata { } impl Metadata { - /// Returns the verify of this [`Metadata`]. + /// Create a new struct. + /// + /// # Errors + /// + /// This function will return an error if the metadata fields do not have a + /// valid format. + pub fn new(title: &str, description: &str, category: &str, tag_ids: &[TagId]) -> Result { + Self::validate_format(title, description, category, tag_ids)?; + + Ok(Self { + title: title.to_owned(), + description: description.to_owned(), + category: category.to_owned(), + tags: tag_ids.to_vec(), + }) + } + + /// It validates the format of the metadata fields. + /// + /// It does not validate domain rules, like: + /// + /// - Duplicate titles. + /// - Non-existing categories. + /// - ... /// /// # Errors /// - /// 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::MissingMandatoryMetadataFields) - } else { - Ok(()) + /// This function will return an error if any of the metadata fields does + /// not have a valid format. + fn validate_format(title: &str, _description: &str, category: &str, _tag_ids: &[TagId]) -> Result<(), MetadataError> { + if title.is_empty() { + return Err(MetadataError::MissingTorrentTitle); } + + if category.is_empty() { + return Err(MetadataError::MissingTorrentCategoryName); + } + + if title.len() < MIN_TORRENT_TITLE_LENGTH { + return Err(MetadataError::InvalidTorrentTitleLength); + } + + Ok(()) } } diff --git a/src/models/torrent_file.rs b/src/models/torrent_file.rs index aba633c5..7045c147 100644 --- a/src/models/torrent_file.rs +++ b/src/models/torrent_file.rs @@ -133,13 +133,13 @@ impl Torrent { } #[must_use] - pub fn info_hash_hex(&self) -> String { - from_bytes(&self.calculate_info_hash_as_bytes()).to_lowercase() + pub fn canonical_info_hash(&self) -> InfoHash { + self.calculate_info_hash_as_bytes().into() } #[must_use] - pub fn canonical_info_hash(&self) -> InfoHash { - self.calculate_info_hash_as_bytes().into() + pub fn canonical_info_hash_hex(&self) -> String { + self.canonical_info_hash().to_hex_string() } #[must_use] @@ -389,7 +389,7 @@ mod tests { httpseeds: None, }; - assert_eq!(torrent.info_hash_hex(), "79fa9e4a2927804fe4feab488a76c8c2d3d1cdca"); + assert_eq!(torrent.canonical_info_hash_hex(), "79fa9e4a2927804fe4feab488a76c8c2d3d1cdca"); } mod infohash_should_be_calculated_for { @@ -430,7 +430,7 @@ mod tests { httpseeds: None, }; - assert_eq!(torrent.info_hash_hex(), "79fa9e4a2927804fe4feab488a76c8c2d3d1cdca"); + assert_eq!(torrent.canonical_info_hash_hex(), "79fa9e4a2927804fe4feab488a76c8c2d3d1cdca"); } #[test] @@ -469,7 +469,7 @@ mod tests { httpseeds: None, }; - assert_eq!(torrent.info_hash_hex(), "aa2aca91ab650c4d249c475ca3fa604f2ccb0d2a"); + assert_eq!(torrent.canonical_info_hash_hex(), "aa2aca91ab650c4d249c475ca3fa604f2ccb0d2a"); } #[test] @@ -504,7 +504,7 @@ mod tests { httpseeds: None, }; - assert_eq!(torrent.info_hash_hex(), "ccc1cf4feb59f3fa85c96c9be1ebbafcfe8a9cc8"); + assert_eq!(torrent.canonical_info_hash_hex(), "ccc1cf4feb59f3fa85c96c9be1ebbafcfe8a9cc8"); } #[test] @@ -539,7 +539,7 @@ mod tests { httpseeds: None, }; - assert_eq!(torrent.info_hash_hex(), "d3a558d0a19aaa23ba6f9f430f40924d10fefa86"); + assert_eq!(torrent.canonical_info_hash_hex(), "d3a558d0a19aaa23ba6f9f430f40924d10fefa86"); } } } diff --git a/src/services/torrent.rs b/src/services/torrent.rs index 71c8fb48..f6864c06 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -20,8 +20,6 @@ use crate::tracker::statistics_importer::StatisticsImporter; use crate::utils::parse_torrent; use crate::{tracker, AsCSV}; -const MIN_TORRENT_TITLE_LENGTH: usize = 3; - pub struct Index { configuration: Arc, tracker_statistics_importer: Arc, @@ -128,22 +126,34 @@ impl Index { /// * Unable to parse the torrent info-hash. pub async fn add_torrent( &self, - add_torrent_form: AddTorrentRequest, + add_torrent_req: AddTorrentRequest, user_id: UserId, ) -> Result { - let metadata = Metadata { - title: add_torrent_form.title, - description: add_torrent_form.description, - category: add_torrent_form.category, - tags: add_torrent_form.tags, - }; + // Authorization: only authenticated users ere allowed to upload torrents + + let _user = self.user_repository.get_compact(&user_id).await?; + + // Validate and build metadata + + let metadata = Metadata::new( + &add_torrent_req.title, + &add_torrent_req.description, + &add_torrent_req.category, + &add_torrent_req.tags, + )?; + + let category = self + .category_repository + .get_by_name(&metadata.category) + .await + .map_err(|_| ServiceError::InvalidCategory)?; - metadata.verify()?; + // Validate and build torrent file - let original_info_hash = parse_torrent::calculate_info_hash(&add_torrent_form.torrent_buffer); + let original_info_hash = parse_torrent::calculate_info_hash(&add_torrent_req.torrent_buffer); let mut torrent = - parse_torrent::decode_torrent(&add_torrent_form.torrent_buffer).map_err(|_| ServiceError::InvalidTorrentFile)?; + parse_torrent::decode_torrent(&add_torrent_req.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() { @@ -152,22 +162,12 @@ impl Index { } } - let _user = self.user_repository.get_compact(&user_id).await?; - torrent.set_announce_urls(&self.configuration).await; - if metadata.title.len() < MIN_TORRENT_TITLE_LENGTH { - return Err(ServiceError::InvalidTorrentTitleLength); - } - - let category = self - .category_repository - .get_by_name(&metadata.category) - .await - .map_err(|_| ServiceError::InvalidCategory)?; - let canonical_info_hash = torrent.canonical_info_hash(); + // Canonical InfoHash Group checks + let original_info_hashes = self .torrent_info_hash_repository .get_canonical_info_hash_group(&canonical_info_hash) @@ -194,35 +194,44 @@ impl Index { return Err(ServiceError::CanonicalInfoHashAlreadyExists); } - // First time a torrent with this original infohash is uploaded. + // Store the torrent into the database let torrent_id = self .torrent_repository .add(&original_info_hash, &torrent, &metadata, user_id, category) .await?; - let info_hash = torrent.canonical_info_hash(); + + self.torrent_tag_repository + .link_torrent_to_tags(&torrent_id, &metadata.tags) + .await?; + + // Secondary task: import torrent statistics from the tracker drop( self.tracker_statistics_importer - .import_torrent_statistics(torrent_id, &torrent.info_hash_hex()) + .import_torrent_statistics(torrent_id, &torrent.canonical_info_hash_hex()) .await, ); + // Secondary task: whitelist torrent on the tracker + // 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.info_hash_hex()).await { + if let Err(e) = self + .tracker_service + .whitelist_info_hash(torrent.canonical_info_hash_hex()) + .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, &metadata.tags) - .await?; + // Build response Ok(AddTorrentResponse { torrent_id, - info_hash: info_hash.to_string(), + info_hash: torrent.canonical_info_hash_hex(), original_info_hash: original_info_hash.to_string(), }) } diff --git a/src/utils/parse_torrent.rs b/src/utils/parse_torrent.rs index 21a219d5..5644cd8d 100644 --- a/src/utils/parse_torrent.rs +++ b/src/utils/parse_torrent.rs @@ -99,7 +99,7 @@ mod tests { // The infohash is not the original infohash of the torrent file, // but the infohash of the info dictionary without the custom keys. assert_eq!( - torrent.info_hash_hex(), + torrent.canonical_info_hash_hex(), "8aa01a4c816332045ffec83247ccbc654547fedf".to_string() ); } diff --git a/src/web/api/v1/contexts/torrent/handlers.rs b/src/web/api/v1/contexts/torrent/handlers.rs index 6c0754f0..9ddf345d 100644 --- a/src/web/api/v1/contexts/torrent/handlers.rs +++ b/src/web/api/v1/contexts/torrent/handlers.rs @@ -100,7 +100,11 @@ pub async fn download_torrent_handler( return ServiceError::InternalServerError.into_response(); }; - torrent_file_response(bytes, &format!("{}.torrent", torrent.info.name), &torrent.info_hash_hex()) + torrent_file_response( + bytes, + &format!("{}.torrent", torrent.info.name), + &torrent.canonical_info_hash_hex(), + ) } } @@ -307,7 +311,11 @@ pub async fn create_random_torrent_handler(State(_app_data): State> return ServiceError::InternalServerError.into_response(); }; - torrent_file_response(bytes, &format!("{}.torrent", torrent.info.name), &torrent.info_hash_hex()) + torrent_file_response( + bytes, + &format!("{}.torrent", torrent.info.name), + &torrent.canonical_info_hash_hex(), + ) } /// Extracts the [`TorrentRequest`] from the multipart form payload. diff --git a/tests/e2e/web/api/v1/contexts/torrent/contract.rs b/tests/e2e/web/api/v1/contexts/torrent/contract.rs index a300cbc1..5bd59d2f 100644 --- a/tests/e2e/web/api/v1/contexts/torrent/contract.rs +++ b/tests/e2e/web/api/v1/contexts/torrent/contract.rs @@ -396,7 +396,10 @@ mod for_guests { // The returned torrent info-hash should be the same as the first torrent assert_eq!(response.status, 200); - assert_eq!(torrent.info_hash_hex(), first_torrent_canonical_info_hash.to_hex_string()); + assert_eq!( + torrent.canonical_info_hash_hex(), + first_torrent_canonical_info_hash.to_hex_string() + ); } #[tokio::test] From a46d3007f08342196c55b7738ae759da5dbfeb22 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 20 Sep 2023 14:10:37 +0100 Subject: [PATCH 04/11] refactor: [#276] extract function Torrent::validate_and_build_metadata --- src/databases/database.rs | 2 +- src/databases/mysql.rs | 2 +- src/databases/sqlite.rs | 2 +- src/errors.rs | 4 +- src/models/torrent.rs | 25 ++++++----- src/services/torrent.rs | 47 ++++++++++++--------- src/web/api/v1/contexts/torrent/handlers.rs | 2 +- 7 files changed, 44 insertions(+), 40 deletions(-) diff --git a/src/databases/database.rs b/src/databases/database.rs index 9205d843..462e722b 100644 --- a/src/databases/database.rs +++ b/src/databases/database.rs @@ -196,7 +196,7 @@ pub trait Database: Sync + Send { original_info_hash: &InfoHash, torrent: &Torrent, uploader_id: UserId, - category_id: i64, + category_id: CategoryId, title: &str, description: &str, ) -> Result; diff --git a/src/databases/mysql.rs b/src/databases/mysql.rs index f7dff1ff..9eaef8a7 100644 --- a/src/databases/mysql.rs +++ b/src/databases/mysql.rs @@ -432,7 +432,7 @@ impl Database for Mysql { original_info_hash: &InfoHash, torrent: &Torrent, uploader_id: UserId, - category_id: i64, + category_id: CategoryId, title: &str, description: &str, ) -> Result { diff --git a/src/databases/sqlite.rs b/src/databases/sqlite.rs index ec8d2c01..43d475ca 100644 --- a/src/databases/sqlite.rs +++ b/src/databases/sqlite.rs @@ -422,7 +422,7 @@ impl Database for Sqlite { original_info_hash: &InfoHash, torrent: &Torrent, uploader_id: UserId, - category_id: i64, + category_id: CategoryId, title: &str, description: &str, ) -> Result { diff --git a/src/errors.rs b/src/errors.rs index 29eb12f0..e8ef6712 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -209,9 +209,7 @@ impl From for ServiceError { fn from(e: MetadataError) -> Self { eprintln!("{e}"); match e { - MetadataError::MissingTorrentTitle | MetadataError::MissingTorrentCategoryName => { - ServiceError::MissingMandatoryMetadataFields - } + MetadataError::MissingTorrentTitle => ServiceError::MissingMandatoryMetadataFields, MetadataError::InvalidTorrentTitleLength => ServiceError::InvalidTorrentTitleLength, } } diff --git a/src/models/torrent.rs b/src/models/torrent.rs index b86eb519..1c2d10cc 100644 --- a/src/models/torrent.rs +++ b/src/models/torrent.rs @@ -1,6 +1,7 @@ use derive_more::{Display, Error}; use serde::{Deserialize, Serialize}; +use super::category::CategoryId; use super::torrent_tag::TagId; const MIN_TORRENT_TITLE_LENGTH: usize = 3; @@ -28,12 +29,9 @@ pub struct TorrentListing { #[derive(Debug, Display, PartialEq, Eq, Error)] pub enum MetadataError { - #[display(fmt = "Missing mandatory torrent title")] + #[display(fmt = "Missing mandatory torrent title.")] MissingTorrentTitle, - #[display(fmt = "Missing mandatory torrent category name")] - MissingTorrentCategoryName, - #[display(fmt = "Torrent title is too short.")] InvalidTorrentTitleLength, } @@ -42,7 +40,7 @@ pub enum MetadataError { pub struct Metadata { pub title: String, pub description: String, - pub category: String, + pub category_id: CategoryId, pub tags: Vec, } @@ -53,13 +51,13 @@ impl Metadata { /// /// This function will return an error if the metadata fields do not have a /// valid format. - pub fn new(title: &str, description: &str, category: &str, tag_ids: &[TagId]) -> Result { - Self::validate_format(title, description, category, tag_ids)?; + pub fn new(title: &str, description: &str, category_id: CategoryId, tag_ids: &[TagId]) -> Result { + Self::validate_format(title, description, category_id, tag_ids)?; Ok(Self { title: title.to_owned(), description: description.to_owned(), - category: category.to_owned(), + category_id, tags: tag_ids.to_vec(), }) } @@ -76,15 +74,16 @@ impl Metadata { /// /// This function will return an error if any of the metadata fields does /// not have a valid format. - fn validate_format(title: &str, _description: &str, category: &str, _tag_ids: &[TagId]) -> Result<(), MetadataError> { + fn validate_format( + title: &str, + _description: &str, + _category_id: CategoryId, + _tag_ids: &[TagId], + ) -> Result<(), MetadataError> { if title.is_empty() { return Err(MetadataError::MissingTorrentTitle); } - if category.is_empty() { - return Err(MetadataError::MissingTorrentCategoryName); - } - if title.len() < MIN_TORRENT_TITLE_LENGTH { return Err(MetadataError::InvalidTorrentTitleLength); } diff --git a/src/services/torrent.rs b/src/services/torrent.rs index f6864c06..7ae61848 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -7,7 +7,7 @@ use serde_derive::{Deserialize, Serialize}; use super::category::DbCategoryRepository; use super::user::DbUserRepository; use crate::config::Configuration; -use crate::databases::database::{Category, Database, Error, Sorting}; +use crate::databases::database::{Database, Error, Sorting}; use crate::errors::ServiceError; use crate::models::category::CategoryId; use crate::models::info_hash::InfoHash; @@ -38,7 +38,7 @@ pub struct Index { pub struct AddTorrentRequest { pub title: String, pub description: String, - pub category: String, + pub category_name: String, pub tags: Vec, pub torrent_buffer: Vec, } @@ -130,23 +130,9 @@ impl Index { user_id: UserId, ) -> Result { // Authorization: only authenticated users ere allowed to upload torrents - let _user = self.user_repository.get_compact(&user_id).await?; - // Validate and build metadata - - let metadata = Metadata::new( - &add_torrent_req.title, - &add_torrent_req.description, - &add_torrent_req.category, - &add_torrent_req.tags, - )?; - - let category = self - .category_repository - .get_by_name(&metadata.category) - .await - .map_err(|_| ServiceError::InvalidCategory)?; + let metadata = self.validate_and_build_metadata(&add_torrent_req).await?; // Validate and build torrent file @@ -198,7 +184,7 @@ impl Index { let torrent_id = self .torrent_repository - .add(&original_info_hash, &torrent, &metadata, user_id, category) + .add(&original_info_hash, &torrent, &metadata, user_id, metadata.category_id) .await?; self.torrent_tag_repository @@ -236,6 +222,27 @@ impl Index { }) } + async fn validate_and_build_metadata(&self, add_torrent_req: &AddTorrentRequest) -> Result { + if add_torrent_req.category_name.is_empty() { + return Err(ServiceError::MissingMandatoryMetadataFields); + } + + let category = self + .category_repository + .get_by_name(&add_torrent_req.category_name) + .await + .map_err(|_| ServiceError::InvalidCategory)?; + + let metadata = Metadata::new( + &add_torrent_req.title, + &add_torrent_req.description, + category.category_id, + &add_torrent_req.tags, + )?; + + Ok(metadata) + } + /// Gets a torrent from the Index. /// /// # Errors @@ -533,14 +540,14 @@ impl DbTorrentRepository { torrent: &Torrent, metadata: &Metadata, user_id: UserId, - category: Category, + category_id: CategoryId, ) -> Result { self.database .insert_torrent_and_get_id( original_info_hash, torrent, user_id, - category.category_id, + category_id, &metadata.title, &metadata.description, ) diff --git a/src/web/api/v1/contexts/torrent/handlers.rs b/src/web/api/v1/contexts/torrent/handlers.rs index 9ddf345d..bab51443 100644 --- a/src/web/api/v1/contexts/torrent/handlers.rs +++ b/src/web/api/v1/contexts/torrent/handlers.rs @@ -396,7 +396,7 @@ async fn build_add_torrent_request_from_payload(mut payload: Multipart) -> Resul Ok(AddTorrentRequest { title, description, - category, + category_name: category, tags, torrent_buffer: torrent_cursor.into_inner(), }) From 329485fe748d6c6f0740805efa2032cbd977a0db Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 20 Sep 2023 14:36:54 +0100 Subject: [PATCH 05/11] refactor: [#276] extract fn parse_torrent::decode_and_validate_torrent_file --- src/errors.rs | 11 +++++++++++ src/services/torrent.rs | 18 ++++-------------- src/utils/parse_torrent.rs | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index e8ef6712..b6f3385d 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -6,6 +6,7 @@ use hyper::StatusCode; use crate::databases::database; use crate::models::torrent::MetadataError; +use crate::utils::parse_torrent::MetainfoFileDataError; pub type ServiceResult = Result; @@ -215,6 +216,16 @@ impl From for ServiceError { } } +impl From for ServiceError { + fn from(e: MetainfoFileDataError) -> Self { + eprintln!("{e}"); + match e { + MetainfoFileDataError::InvalidBencodeData => ServiceError::InvalidTorrentFile, + MetainfoFileDataError::InvalidTorrentPiecesLength => ServiceError::InvalidTorrentTitleLength, + } + } +} + #[must_use] pub fn http_status_code_for_service_error(error: &ServiceError) -> StatusCode { #[allow(clippy::match_same_arms)] diff --git a/src/services/torrent.rs b/src/services/torrent.rs index 7ae61848..dd13331f 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -17,7 +17,7 @@ use crate::models::torrent_file::{DbTorrent, 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::utils::parse_torrent::decode_and_validate_torrent_file; use crate::{tracker, AsCSV}; pub struct Index { @@ -134,20 +134,10 @@ impl Index { let metadata = self.validate_and_build_metadata(&add_torrent_req).await?; - // Validate and build torrent file - - let original_info_hash = parse_torrent::calculate_info_hash(&add_torrent_req.torrent_buffer); - - let mut torrent = - parse_torrent::decode_torrent(&add_torrent_req.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 (mut torrent, original_info_hash) = decode_and_validate_torrent_file(&add_torrent_req.torrent_buffer)?; + // Customize the announce URLs with the linked tracker URL + // and remove others if the torrent is private. torrent.set_announce_urls(&self.configuration).await; let canonical_info_hash = torrent.canonical_info_hash(); diff --git a/src/utils/parse_torrent.rs b/src/utils/parse_torrent.rs index 5644cd8d..e04c2549 100644 --- a/src/utils/parse_torrent.rs +++ b/src/utils/parse_torrent.rs @@ -1,5 +1,6 @@ use std::error; +use derive_more::{Display, Error}; use serde::{self, Deserialize, Serialize}; use serde_bencode::value::Value; use serde_bencode::{de, Error}; @@ -8,6 +9,43 @@ use sha1::{Digest, Sha1}; use crate::models::info_hash::InfoHash; use crate::models::torrent_file::Torrent; +#[derive(Debug, Display, PartialEq, Eq, Error)] +pub enum MetainfoFileDataError { + #[display(fmt = "Torrent data could not be decoded from the bencoded format.")] + InvalidBencodeData, + + #[display(fmt = "Torrent has an invalid pieces key length. It should be a multiple of 20.")] + InvalidTorrentPiecesLength, +} + +/// It decodes and validate an array of bytes containing a torrent file. +/// +/// It returns a tuple containing the decoded torrent and the original info hash. +/// The original info-hash migth not match the new one in the `Torrent` because +/// the info dictionary might have been modified. For example, ignoring some +/// non-standard fields. +/// +/// # Errors +/// +/// This function will return an error if +/// +/// - The torrent file is not a valid bencoded file. +/// - The pieces key has a length that is not a multiple of 20. +pub fn decode_and_validate_torrent_file(bytes: &[u8]) -> Result<(Torrent, InfoHash), MetainfoFileDataError> { + let original_info_hash = calculate_info_hash(bytes); + + let torrent = decode_torrent(bytes).map_err(|_| MetainfoFileDataError::InvalidBencodeData)?; + + // 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(MetainfoFileDataError::InvalidTorrentPiecesLength); + } + } + + Ok((torrent, original_info_hash)) +} + /// Decode a Torrent from Bencoded Bytes. /// /// # Errors From 4cc97c78214db737cd4ac662b9bc501761eae83e Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 20 Sep 2023 16:05:13 +0100 Subject: [PATCH 06/11] feat!: [#276] upload torrent returns 400 instead of empty response when the original info-hash of the upload torrent cannot be calculated. The API should not return empty responses, so "panic" should not be use for common problems that we should notify to the user. --- src/errors.rs | 12 +++--- src/services/torrent.rs | 2 +- src/utils/parse_torrent.rs | 40 ++++++++++++------- .../web/api/v1/contexts/torrent/contract.rs | 8 ++-- 4 files changed, 38 insertions(+), 24 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index b6f3385d..c92a0361 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -6,7 +6,7 @@ use hyper::StatusCode; use crate::databases::database; use crate::models::torrent::MetadataError; -use crate::utils::parse_torrent::MetainfoFileDataError; +use crate::utils::parse_torrent::DecodeTorrentFileError; pub type ServiceResult = Result; @@ -216,12 +216,14 @@ impl From for ServiceError { } } -impl From for ServiceError { - fn from(e: MetainfoFileDataError) -> Self { +impl From for ServiceError { + fn from(e: DecodeTorrentFileError) -> Self { eprintln!("{e}"); match e { - MetainfoFileDataError::InvalidBencodeData => ServiceError::InvalidTorrentFile, - MetainfoFileDataError::InvalidTorrentPiecesLength => ServiceError::InvalidTorrentTitleLength, + DecodeTorrentFileError::InvalidTorrentPiecesLength => ServiceError::InvalidTorrentTitleLength, + DecodeTorrentFileError::CannotBencodeInfoDict + | DecodeTorrentFileError::InvalidInfoDictionary + | DecodeTorrentFileError::InvalidBencodeData => ServiceError::InvalidTorrentFile, } } } diff --git a/src/services/torrent.rs b/src/services/torrent.rs index dd13331f..cb40de57 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -129,7 +129,7 @@ impl Index { add_torrent_req: AddTorrentRequest, user_id: UserId, ) -> Result { - // Authorization: only authenticated users ere allowed to upload torrents + // Guard that the users exists let _user = self.user_repository.get_compact(&user_id).await?; let metadata = self.validate_and_build_metadata(&add_torrent_req).await?; diff --git a/src/utils/parse_torrent.rs b/src/utils/parse_torrent.rs index e04c2549..69e69011 100644 --- a/src/utils/parse_torrent.rs +++ b/src/utils/parse_torrent.rs @@ -10,12 +10,18 @@ use crate::models::info_hash::InfoHash; use crate::models::torrent_file::Torrent; #[derive(Debug, Display, PartialEq, Eq, Error)] -pub enum MetainfoFileDataError { +pub enum DecodeTorrentFileError { #[display(fmt = "Torrent data could not be decoded from the bencoded format.")] InvalidBencodeData, + #[display(fmt = "Torrent info dictionary key could not be decoded from the bencoded format.")] + InvalidInfoDictionary, + #[display(fmt = "Torrent has an invalid pieces key length. It should be a multiple of 20.")] InvalidTorrentPiecesLength, + + #[display(fmt = "Cannot bencode the parsed `info` dictionary again to generate the info-hash.")] + CannotBencodeInfoDict, } /// It decodes and validate an array of bytes containing a torrent file. @@ -31,15 +37,15 @@ pub enum MetainfoFileDataError { /// /// - The torrent file is not a valid bencoded file. /// - The pieces key has a length that is not a multiple of 20. -pub fn decode_and_validate_torrent_file(bytes: &[u8]) -> Result<(Torrent, InfoHash), MetainfoFileDataError> { - let original_info_hash = calculate_info_hash(bytes); +pub fn decode_and_validate_torrent_file(bytes: &[u8]) -> Result<(Torrent, InfoHash), DecodeTorrentFileError> { + let original_info_hash = calculate_info_hash(bytes)?; - let torrent = decode_torrent(bytes).map_err(|_| MetainfoFileDataError::InvalidBencodeData)?; + let torrent = decode_torrent(bytes).map_err(|_| DecodeTorrentFileError::InvalidBencodeData)?; // 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(MetainfoFileDataError::InvalidTorrentPiecesLength); + return Err(DecodeTorrentFileError::InvalidTorrentPiecesLength); } } @@ -77,30 +83,34 @@ pub fn encode_torrent(torrent: &Torrent) -> Result, Error> { } #[derive(Serialize, Deserialize, Debug, PartialEq)] -struct MetainfoFile { +struct ParsedInfoDictFromMetainfoFile { pub info: Value, } /// Calculates the `InfoHash` from a the torrent file binary data. /// -/// # Panics +/// # Errors +/// +/// This function will return an error if: /// -/// This function will panic if the torrent file is not a valid bencoded file -/// or if the info dictionary cannot be bencoded. -#[must_use] -pub fn calculate_info_hash(bytes: &[u8]) -> InfoHash { +/// - The torrent file is not a valid bencoded torrent file containing an `info` +/// dictionary key. +/// - The original torrent info-hash cannot be bencoded from the parsed `info` +/// dictionary is not a valid bencoded dictionary. +pub fn calculate_info_hash(bytes: &[u8]) -> Result { // Extract the info dictionary - let metainfo: MetainfoFile = serde_bencode::from_bytes(bytes).expect("Torrent file cannot be parsed from bencoded format"); + let metainfo: ParsedInfoDictFromMetainfoFile = + serde_bencode::from_bytes(bytes).map_err(|_| DecodeTorrentFileError::InvalidInfoDictionary)?; // Bencode the info dictionary - let info_dict_bytes = serde_bencode::to_bytes(&metainfo.info).expect("Info dictionary cannot by bencoded"); + let info_dict_bytes = serde_bencode::to_bytes(&metainfo.info).map_err(|_| DecodeTorrentFileError::CannotBencodeInfoDict)?; // Calculate the SHA-1 hash of the bencoded info dictionary let mut hasher = Sha1::new(); hasher.update(&info_dict_bytes); let result = hasher.finalize(); - InfoHash::from_bytes(&result) + Ok(InfoHash::from_bytes(&result)) } #[cfg(test)] @@ -117,7 +127,7 @@ mod tests { "tests/fixtures/torrents/6c690018c5786dbbb00161f62b0712d69296df97_with_custom_info_dict_key.torrent", ); - let original_info_hash = super::calculate_info_hash(&std::fs::read(torrent_path).unwrap()); + let original_info_hash = super::calculate_info_hash(&std::fs::read(torrent_path).unwrap()).unwrap(); assert_eq!( original_info_hash, diff --git a/tests/e2e/web/api/v1/contexts/torrent/contract.rs b/tests/e2e/web/api/v1/contexts/torrent/contract.rs index 5bd59d2f..601e0478 100644 --- a/tests/e2e/web/api/v1/contexts/torrent/contract.rs +++ b/tests/e2e/web/api/v1/contexts/torrent/contract.rs @@ -317,7 +317,8 @@ mod for_guests { // Download let response = client.download_torrent(&test_torrent.file_info_hash()).await; - let downloaded_torrent_info_hash = calculate_info_hash(&response.bytes); + let downloaded_torrent_info_hash = + calculate_info_hash(&response.bytes).expect("failed to calculate info-hash of the downloaded torrent"); assert_eq!( downloaded_torrent_info_hash.to_hex_string(), @@ -598,7 +599,6 @@ mod for_authenticated_users { use crate::e2e::web::api::v1::contexts::user::steps::new_logged_in_user; #[tokio::test] - #[should_panic] async fn contains_a_bencoded_dictionary_with_the_info_key_in_order_to_calculate_the_original_info_hash() { let mut env = TestEnv::new(); env.start(api::Version::V1).await; @@ -614,7 +614,9 @@ mod for_authenticated_users { let form: UploadTorrentMultipartForm = test_torrent.index_info.into(); - let _response = client.upload_torrent(form.into()).await; + let response = client.upload_torrent(form.into()).await; + + assert_eq!(response.status, 400); } #[tokio::test] From bbadd59a4af9139cbe32f5b3465d94add5bfdd2c Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 20 Sep 2023 16:24:27 +0100 Subject: [PATCH 07/11] refactor: [#276] extract fn Torrent::customize_announcement_info_for --- src/models/torrent_file.rs | 21 +++++++++++++-------- src/services/torrent.rs | 11 ++++++++--- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/models/torrent_file.rs b/src/models/torrent_file.rs index 7045c147..98e57b92 100644 --- a/src/models/torrent_file.rs +++ b/src/models/torrent_file.rs @@ -4,7 +4,6 @@ use serde_bytes::ByteBuf; use sha1::{Digest, Sha1}; use super::info_hash::InfoHash; -use crate::config::Configuration; use crate::utils::hex::{from_bytes, into_bytes}; #[derive(PartialEq, Debug, Clone, Serialize, Deserialize)] @@ -101,19 +100,25 @@ impl Torrent { } } - /// Sets the announce url to the tracker url and removes all other trackers - /// if the torrent is private. - pub async fn set_announce_urls(&mut self, cfg: &Configuration) { - let settings = cfg.settings.read().await; + /// Sets the announce url to the tracker url. + pub fn set_announce_to(&mut self, tracker_url: &str) { + self.announce = Some(tracker_url.to_owned()); + } - self.announce = Some(settings.tracker.url.clone()); + /// Removes all other trackers if the torrent is private. + pub fn reset_announce_list_if_private(&mut self) { + if self.is_private() { + self.announce_list = None; + } + } - // if torrent is private, remove all other trackers + fn is_private(&self) -> bool { if let Some(private) = self.info.private { if private == 1 { - self.announce_list = None; + return true; } } + false } /// It calculates the info hash of the torrent file. diff --git a/src/services/torrent.rs b/src/services/torrent.rs index cb40de57..7d59320b 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -136,9 +136,7 @@ impl Index { let (mut torrent, original_info_hash) = decode_and_validate_torrent_file(&add_torrent_req.torrent_buffer)?; - // Customize the announce URLs with the linked tracker URL - // and remove others if the torrent is private. - torrent.set_announce_urls(&self.configuration).await; + self.customize_announcement_info_for(&mut torrent).await; let canonical_info_hash = torrent.canonical_info_hash(); @@ -233,6 +231,13 @@ impl Index { Ok(metadata) } + async fn customize_announcement_info_for(&self, torrent: &mut Torrent) { + let settings = self.configuration.settings.read().await; + let tracker_url = settings.tracker.url.clone(); + torrent.set_announce_to(&tracker_url); + torrent.reset_announce_list_if_private(); + } + /// Gets a torrent from the Index. /// /// # Errors From 6a75c54b3b73f58d54f0aa93100769bd5b5385a4 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 20 Sep 2023 16:33:24 +0100 Subject: [PATCH 08/11] refactor: [#276] extract fn Torrent::canonical_info_hash_group_checks --- src/services/torrent.rs | 61 +++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/src/services/torrent.rs b/src/services/torrent.rs index 7d59320b..429fea38 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -140,34 +140,9 @@ impl Index { let canonical_info_hash = torrent.canonical_info_hash(); - // Canonical InfoHash Group checks - - let original_info_hashes = self - .torrent_info_hash_repository - .get_canonical_info_hash_group(&canonical_info_hash) + self.canonical_info_hash_group_checks(&original_info_hash, &canonical_info_hash) .await?; - if !original_info_hashes.is_empty() { - // Torrent with the same canonical infohash was already uploaded - debug!("Canonical infohash found: {:?}", canonical_info_hash.to_hex_string()); - - if let Some(original_info_hash) = original_info_hashes.find(&original_info_hash) { - // The exact original infohash was already uploaded - debug!("Original infohash found: {:?}", original_info_hash.to_hex_string()); - - return Err(ServiceError::InfoHashAlreadyExists); - } - - // A new original infohash is being uploaded with a canonical infohash that already exists. - debug!("Original infohash not found: {:?}", original_info_hash.to_hex_string()); - - // Add the new associated original infohash to the canonical one. - self.torrent_info_hash_repository - .add_info_hash_to_canonical_info_hash_group(&original_info_hash, &canonical_info_hash) - .await?; - return Err(ServiceError::CanonicalInfoHashAlreadyExists); - } - // Store the torrent into the database let torrent_id = self @@ -231,6 +206,40 @@ impl Index { Ok(metadata) } + async fn canonical_info_hash_group_checks( + &self, + original_info_hash: &InfoHash, + canonical_info_hash: &InfoHash, + ) -> Result<(), ServiceError> { + let original_info_hashes = self + .torrent_info_hash_repository + .get_canonical_info_hash_group(canonical_info_hash) + .await?; + + if !original_info_hashes.is_empty() { + // Torrent with the same canonical infohash was already uploaded + debug!("Canonical infohash found: {:?}", canonical_info_hash.to_hex_string()); + + if let Some(original_info_hash) = original_info_hashes.find(original_info_hash) { + // The exact original infohash was already uploaded + debug!("Original infohash found: {:?}", original_info_hash.to_hex_string()); + + return Err(ServiceError::InfoHashAlreadyExists); + } + + // A new original infohash is being uploaded with a canonical infohash that already exists. + debug!("Original infohash not found: {:?}", original_info_hash.to_hex_string()); + + // Add the new associated original infohash to the canonical one. + self.torrent_info_hash_repository + .add_info_hash_to_canonical_info_hash_group(original_info_hash, canonical_info_hash) + .await?; + return Err(ServiceError::CanonicalInfoHashAlreadyExists); + } + + Ok(()) + } + async fn customize_announcement_info_for(&self, torrent: &mut Torrent) { let settings = self.configuration.settings.read().await; let tracker_url = settings.tracker.url.clone(); From fbb42adcba97e1f7928031d5e73f58237712c52b Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 20 Sep 2023 16:50:57 +0100 Subject: [PATCH 09/11] feat!: [#276] do not persist uploaded torrent if it cannot persit tags When you upload a torrent the transaction wraps all the persisted data. Before this change, if tags can't be stored in the database the rest of the indexed data is stored anyway. --- src/databases/database.rs | 6 ++---- src/databases/mysql.rs | 29 ++++++++++++++++++++++------- src/databases/sqlite.rs | 29 ++++++++++++++++++++++------- src/services/torrent.rs | 22 +++------------------- 4 files changed, 49 insertions(+), 37 deletions(-) diff --git a/src/databases/database.rs b/src/databases/database.rs index 462e722b..0d6e8c3e 100644 --- a/src/databases/database.rs +++ b/src/databases/database.rs @@ -7,7 +7,7 @@ use crate::databases::sqlite::Sqlite; use crate::models::category::CategoryId; use crate::models::info_hash::InfoHash; use crate::models::response::TorrentsResponse; -use crate::models::torrent::TorrentListing; +use crate::models::torrent::{Metadata, TorrentListing}; use crate::models::torrent_file::{DbTorrent, Torrent, TorrentFile}; use crate::models::torrent_tag::{TagId, TorrentTag}; use crate::models::tracker_key::TrackerKey; @@ -196,9 +196,7 @@ pub trait Database: Sync + Send { original_info_hash: &InfoHash, torrent: &Torrent, uploader_id: UserId, - category_id: CategoryId, - title: &str, - description: &str, + metadata: &Metadata, ) -> Result; /// Get `Torrent` from `InfoHash`. diff --git a/src/databases/mysql.rs b/src/databases/mysql.rs index 9eaef8a7..b57ad077 100644 --- a/src/databases/mysql.rs +++ b/src/databases/mysql.rs @@ -12,7 +12,7 @@ use crate::databases::database::{Category, Database, Driver, Sorting, TorrentCom use crate::models::category::CategoryId; use crate::models::info_hash::InfoHash; use crate::models::response::TorrentsResponse; -use crate::models::torrent::TorrentListing; +use crate::models::torrent::{Metadata, TorrentListing}; use crate::models::torrent_file::{DbTorrent, DbTorrentAnnounceUrl, DbTorrentFile, Torrent, TorrentFile}; use crate::models::torrent_tag::{TagId, TorrentTag}; use crate::models::tracker_key::TrackerKey; @@ -432,9 +432,7 @@ impl Database for Mysql { original_info_hash: &InfoHash, torrent: &Torrent, uploader_id: UserId, - category_id: CategoryId, - title: &str, - description: &str, + metadata: &Metadata, ) -> Result { let info_hash = torrent.canonical_info_hash_hex(); let canonical_info_hash = torrent.canonical_info_hash(); @@ -471,7 +469,7 @@ impl Database for Mysql { ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, UTC_TIMESTAMP())", ) .bind(uploader_id) - .bind(category_id) + .bind(metadata.category_id) .bind(info_hash.to_lowercase()) .bind(torrent.file_size()) .bind(torrent.info.name.to_string()) @@ -585,11 +583,28 @@ impl Database for Mysql { return Err(e); } + // Insert tags + + for tag_id in &metadata.tags { + let insert_torrent_tag_result = query("INSERT INTO torrust_torrent_tag_links (torrent_id, tag_id) VALUES (?, ?)") + .bind(torrent_id) + .bind(tag_id) + .execute(&mut tx) + .await + .map_err(|err| database::Error::ErrorWithText(err.to_string())); + + // rollback transaction on error + if let Err(e) = insert_torrent_tag_result { + drop(tx.rollback().await); + return Err(e); + } + } + let insert_torrent_info_result = query(r#"INSERT INTO torrust_torrent_info (torrent_id, title, description) VALUES (?, ?, NULLIF(?, ""))"#) .bind(torrent_id) - .bind(title) - .bind(description) + .bind(metadata.title.clone()) + .bind(metadata.description.clone()) .execute(&mut tx) .await .map_err(|e| match e { diff --git a/src/databases/sqlite.rs b/src/databases/sqlite.rs index 43d475ca..8e12f9be 100644 --- a/src/databases/sqlite.rs +++ b/src/databases/sqlite.rs @@ -12,7 +12,7 @@ use crate::databases::database::{Category, Database, Driver, Sorting, TorrentCom use crate::models::category::CategoryId; use crate::models::info_hash::InfoHash; use crate::models::response::TorrentsResponse; -use crate::models::torrent::TorrentListing; +use crate::models::torrent::{Metadata, TorrentListing}; use crate::models::torrent_file::{DbTorrent, DbTorrentAnnounceUrl, DbTorrentFile, Torrent, TorrentFile}; use crate::models::torrent_tag::{TagId, TorrentTag}; use crate::models::tracker_key::TrackerKey; @@ -422,9 +422,7 @@ impl Database for Sqlite { original_info_hash: &InfoHash, torrent: &Torrent, uploader_id: UserId, - category_id: CategoryId, - title: &str, - description: &str, + metadata: &Metadata, ) -> Result { let info_hash = torrent.canonical_info_hash_hex(); let canonical_info_hash = torrent.canonical_info_hash(); @@ -461,7 +459,7 @@ impl Database for Sqlite { ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, strftime('%Y-%m-%d %H:%M:%S',DATETIME('now', 'utc')))", ) .bind(uploader_id) - .bind(category_id) + .bind(metadata.category_id) .bind(info_hash.to_lowercase()) .bind(torrent.file_size()) .bind(torrent.info.name.to_string()) @@ -575,11 +573,28 @@ impl Database for Sqlite { return Err(e); } + // Insert tags + + for tag_id in &metadata.tags { + let insert_torrent_tag_result = query("INSERT INTO torrust_torrent_tag_links (torrent_id, tag_id) VALUES (?, ?)") + .bind(torrent_id) + .bind(tag_id) + .execute(&mut tx) + .await + .map_err(|err| database::Error::ErrorWithText(err.to_string())); + + // rollback transaction on error + if let Err(e) = insert_torrent_tag_result { + drop(tx.rollback().await); + return Err(e); + } + } + let insert_torrent_info_result = query(r#"INSERT INTO torrust_torrent_info (torrent_id, title, description) VALUES (?, ?, NULLIF(?, ""))"#) .bind(torrent_id) - .bind(title) - .bind(description) + .bind(metadata.title.clone()) + .bind(metadata.description.clone()) .execute(&mut tx) .await .map_err(|e| match e { diff --git a/src/services/torrent.rs b/src/services/torrent.rs index 429fea38..7419cfd9 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -138,20 +138,12 @@ impl Index { self.customize_announcement_info_for(&mut torrent).await; - let canonical_info_hash = torrent.canonical_info_hash(); - - self.canonical_info_hash_group_checks(&original_info_hash, &canonical_info_hash) + self.canonical_info_hash_group_checks(&original_info_hash, &torrent.canonical_info_hash()) .await?; - // Store the torrent into the database - let torrent_id = self .torrent_repository - .add(&original_info_hash, &torrent, &metadata, user_id, metadata.category_id) - .await?; - - self.torrent_tag_repository - .link_torrent_to_tags(&torrent_id, &metadata.tags) + .add(&original_info_hash, &torrent, &metadata, user_id) .await?; // Secondary task: import torrent statistics from the tracker @@ -544,17 +536,9 @@ impl DbTorrentRepository { torrent: &Torrent, metadata: &Metadata, user_id: UserId, - category_id: CategoryId, ) -> Result { self.database - .insert_torrent_and_get_id( - original_info_hash, - torrent, - user_id, - category_id, - &metadata.title, - &metadata.description, - ) + .insert_torrent_and_get_id(original_info_hash, torrent, user_id, metadata) .await } From f19e801c66a4f7da2e636edcff0419453e6aa701 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 20 Sep 2023 17:25:41 +0100 Subject: [PATCH 10/11] refactor: [#276] extract fn Torrent::import_torrent_statistics_from_tracker --- src/services/torrent.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/services/torrent.rs b/src/services/torrent.rs index 7419cfd9..40a42179 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -146,13 +146,10 @@ impl Index { .add(&original_info_hash, &torrent, &metadata, user_id) .await?; - // Secondary task: import torrent statistics from the tracker + // Synchronous secondary tasks - drop( - self.tracker_statistics_importer - .import_torrent_statistics(torrent_id, &torrent.canonical_info_hash_hex()) - .await, - ); + self.import_torrent_statistics_from_tracker(torrent_id, &torrent.canonical_info_hash()) + .await; // Secondary task: whitelist torrent on the tracker @@ -239,6 +236,14 @@ impl Index { torrent.reset_announce_list_if_private(); } + async fn import_torrent_statistics_from_tracker(&self, torrent_id: TorrentId, canonical_info_hash: &InfoHash) { + drop( + self.tracker_statistics_importer + .import_torrent_statistics(torrent_id, &canonical_info_hash.to_hex_string()) + .await, + ); + } + /// Gets a torrent from the Index. /// /// # Errors From 275231a6a9b9b82279c035a32ac0b2ec2c6f6d50 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 20 Sep 2023 17:31:55 +0100 Subject: [PATCH 11/11] doc: [#276] add comment for upload torrent secondary tasks --- src/services/torrent.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/services/torrent.rs b/src/services/torrent.rs index 40a42179..0a1f6b94 100644 --- a/src/services/torrent.rs +++ b/src/services/torrent.rs @@ -148,13 +148,16 @@ impl Index { // Synchronous secondary tasks + // code-review: consider moving this to a background task self.import_torrent_statistics_from_tracker(torrent_id, &torrent.canonical_info_hash()) .await; - // Secondary task: whitelist torrent on the tracker - - // We always whitelist the torrent on the tracker because even if the tracker mode is `public` - // it could be changed to `private` later on. + // We always whitelist the torrent on the tracker because + // even if the tracker mode is `public` it could be changed to `private` + // later on. + // + // code-review: maybe we should consider adding a new feature to + // whitelist all torrents from the admin panel if that change happens. if let Err(e) = self .tracker_service .whitelist_info_hash(torrent.canonical_info_hash_hex())