From 58a3741db9b7b51dc65517bf9468ffde1c2fca64 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Fri, 7 Feb 2025 11:52:12 +0000 Subject: [PATCH 1/4] refactor: [#1250] invert dependency between http-protocol and tracker-core pacakges The `tracker-core` should not depend on higher levels layers like the `http-protocol` package. --- Cargo.lock | 2 +- packages/http-protocol/Cargo.toml | 1 + packages/http-protocol/src/v1/responses/error.rs | 8 ++++++++ packages/tracker-core/Cargo.toml | 1 - packages/tracker-core/src/error.rs | 9 --------- 5 files changed, 10 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d868f745..228640f8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -542,6 +542,7 @@ version = "3.0.0-develop" dependencies = [ "aquatic_udp_protocol", "bittorrent-primitives", + "bittorrent-tracker-core", "derive_more", "multimap", "percent-encoding", @@ -596,7 +597,6 @@ name = "bittorrent-tracker-core" version = "3.0.0-develop" dependencies = [ "aquatic_udp_protocol", - "bittorrent-http-protocol", "bittorrent-primitives", "chrono", "derive_more", diff --git a/packages/http-protocol/Cargo.toml b/packages/http-protocol/Cargo.toml index 05b69d20..2d0cabf5 100644 --- a/packages/http-protocol/Cargo.toml +++ b/packages/http-protocol/Cargo.toml @@ -17,6 +17,7 @@ version.workspace = true [dependencies] aquatic_udp_protocol = "0" bittorrent-primitives = "0.1.0" +bittorrent-tracker-core = { version = "3.0.0-develop", path = "../tracker-core" } derive_more = { version = "1", features = ["as_ref", "constructor", "from"] } multimap = "0" percent-encoding = "2" diff --git a/packages/http-protocol/src/v1/responses/error.rs b/packages/http-protocol/src/v1/responses/error.rs index f939ce29..cdf27e00 100644 --- a/packages/http-protocol/src/v1/responses/error.rs +++ b/packages/http-protocol/src/v1/responses/error.rs @@ -55,6 +55,14 @@ impl From for Error { } } +impl From for Error { + fn from(err: bittorrent_tracker_core::error::Error) -> Self { + Error { + failure_reason: format!("Tracker error: {err}"), + } + } +} + #[cfg(test)] mod tests { diff --git a/packages/tracker-core/Cargo.toml b/packages/tracker-core/Cargo.toml index aeea30a3..96505a7b 100644 --- a/packages/tracker-core/Cargo.toml +++ b/packages/tracker-core/Cargo.toml @@ -16,7 +16,6 @@ version.workspace = true [dependencies] aquatic_udp_protocol = "0" -bittorrent-http-protocol = { version = "3.0.0-develop", path = "../http-protocol" } bittorrent-primitives = "0.1.0" chrono = { version = "0", default-features = false, features = ["clock"] } derive_more = { version = "1", features = ["as_ref", "constructor", "from"] } diff --git a/packages/tracker-core/src/error.rs b/packages/tracker-core/src/error.rs index 1d0e974e..6fdb5b62 100644 --- a/packages/tracker-core/src/error.rs +++ b/packages/tracker-core/src/error.rs @@ -8,7 +8,6 @@ //! use std::panic::Location; -use bittorrent_http_protocol::v1::responses; use bittorrent_primitives::info_hash::InfoHash; use torrust_tracker_located_error::LocatedError; @@ -54,11 +53,3 @@ pub enum PeerKeyError { source: LocatedError<'static, databases::error::Error>, }, } - -impl From for responses::error::Error { - fn from(err: Error) -> Self { - responses::error::Error { - failure_reason: format!("Tracker error: {err}"), - } - } -} From 3bb4a13127b72c8b8fa16b27d89d6f1d21d02af4 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Fri, 7 Feb 2025 12:21:24 +0000 Subject: [PATCH 2/4] refactor: [#1250] remove unused errors in tracker core --- packages/tracker-core/src/error.rs | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/packages/tracker-core/src/error.rs b/packages/tracker-core/src/error.rs index 6fdb5b62..6c2e3d4c 100644 --- a/packages/tracker-core/src/error.rs +++ b/packages/tracker-core/src/error.rs @@ -1,11 +1,4 @@ -//! Error returned by the core `Tracker`. -//! -//! Error | Context | Description -//! ---|---|--- -//! `PeerKeyNotValid` | Authentication | The supplied key is not valid. It may not be registered or expired. -//! `PeerNotAuthenticated` | Authentication | The peer did not provide the authentication key. -//! `TorrentNotWhitelisted` | Authorization | The action cannot be perform on a not-whitelisted torrent (it only applies for trackers running in `listed` or `private_listed` modes). -//! +//! Errors returned by the core tracker. use std::panic::Location; use bittorrent_primitives::info_hash::InfoHash; @@ -14,20 +7,9 @@ use torrust_tracker_located_error::LocatedError; use super::authentication::key::ParseKeyError; use super::databases; -/// Authentication or authorization error returned by the core `Tracker` +/// Authorization errors returned by the core tracker. #[derive(thiserror::Error, Debug, Clone)] pub enum Error { - // Authentication errors - #[error("The supplied key: {key:?}, is not valid: {source}")] - PeerKeyNotValid { - key: super::authentication::Key, - source: LocatedError<'static, dyn std::error::Error + Send + Sync>, - }, - - #[error("The peer is not authenticated, {location}")] - PeerNotAuthenticated { location: &'static Location<'static> }, - - // Authorization errors #[error("The torrent: {info_hash}, is not whitelisted, {location}")] TorrentNotWhitelisted { info_hash: InfoHash, @@ -35,7 +17,7 @@ pub enum Error { }, } -/// Errors related to peers keys. +/// Peers keys errors returned by the core tracker. #[allow(clippy::module_name_repetitions)] #[derive(thiserror::Error, Debug, Clone)] pub enum PeerKeyError { From 0811d67b28a793401895fbf0f91b5e940c2624e8 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Fri, 7 Feb 2025 12:23:59 +0000 Subject: [PATCH 3/4] refactor: [#1250] rename enum --- packages/http-protocol/src/v1/responses/error.rs | 4 ++-- packages/tracker-core/src/error.rs | 4 ++-- packages/tracker-core/src/whitelist/authorization.rs | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/http-protocol/src/v1/responses/error.rs b/packages/http-protocol/src/v1/responses/error.rs index cdf27e00..8a6b4cf5 100644 --- a/packages/http-protocol/src/v1/responses/error.rs +++ b/packages/http-protocol/src/v1/responses/error.rs @@ -55,8 +55,8 @@ impl From for Error { } } -impl From for Error { - fn from(err: bittorrent_tracker_core::error::Error) -> Self { +impl From for Error { + fn from(err: bittorrent_tracker_core::error::WhitelistError) -> Self { Error { failure_reason: format!("Tracker error: {err}"), } diff --git a/packages/tracker-core/src/error.rs b/packages/tracker-core/src/error.rs index 6c2e3d4c..56b6e1a1 100644 --- a/packages/tracker-core/src/error.rs +++ b/packages/tracker-core/src/error.rs @@ -7,9 +7,9 @@ use torrust_tracker_located_error::LocatedError; use super::authentication::key::ParseKeyError; use super::databases; -/// Authorization errors returned by the core tracker. +/// Whitelist errors returned by the core tracker. #[derive(thiserror::Error, Debug, Clone)] -pub enum Error { +pub enum WhitelistError { #[error("The torrent: {info_hash}, is not whitelisted, {location}")] TorrentNotWhitelisted { info_hash: InfoHash, diff --git a/packages/tracker-core/src/whitelist/authorization.rs b/packages/tracker-core/src/whitelist/authorization.rs index cb5f4acb..66f90922 100644 --- a/packages/tracker-core/src/whitelist/authorization.rs +++ b/packages/tracker-core/src/whitelist/authorization.rs @@ -6,7 +6,7 @@ use torrust_tracker_configuration::Core; use tracing::instrument; use super::repository::in_memory::InMemoryWhitelist; -use crate::error::Error; +use crate::error::WhitelistError; pub struct WhitelistAuthorization { /// Core tracker configuration. @@ -32,7 +32,7 @@ impl WhitelistAuthorization { /// Will return an error if the tracker is running in `listed` mode /// and the infohash is not whitelisted. #[instrument(skip(self, info_hash), err)] - pub async fn authorize(&self, info_hash: &InfoHash) -> Result<(), Error> { + pub async fn authorize(&self, info_hash: &InfoHash) -> Result<(), WhitelistError> { if !self.is_listed() { return Ok(()); } @@ -41,7 +41,7 @@ impl WhitelistAuthorization { return Ok(()); } - Err(Error::TorrentNotWhitelisted { + Err(WhitelistError::TorrentNotWhitelisted { info_hash: *info_hash, location: Location::caller(), }) @@ -89,7 +89,7 @@ mod tests { use torrust_tracker_configuration::Core; use crate::core_tests::sample_info_hash; - use crate::error::Error; + use crate::error::WhitelistError; use crate::whitelist::authorization::tests::the_whitelist_authorization_for_announce_and_scrape_actions::{ initialize_whitelist_authorization_and_dependencies_with, initialize_whitelist_authorization_with, }; @@ -121,7 +121,7 @@ mod tests { let result = whitelist_authorization.authorize(&sample_info_hash()).await; - assert!(matches!(result.unwrap_err(), Error::TorrentNotWhitelisted { .. })); + assert!(matches!(result.unwrap_err(), WhitelistError::TorrentNotWhitelisted { .. })); } } From ca4ead502ae94b9bb8c93a6d115fb010153e11dc Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Fri, 7 Feb 2025 12:59:35 +0000 Subject: [PATCH 4/4] test: [#1250] add tests for tracker core errors --- packages/tracker-core/src/error.rs | 83 ++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/packages/tracker-core/src/error.rs b/packages/tracker-core/src/error.rs index 56b6e1a1..515510b8 100644 --- a/packages/tracker-core/src/error.rs +++ b/packages/tracker-core/src/error.rs @@ -35,3 +35,86 @@ pub enum PeerKeyError { source: LocatedError<'static, databases::error::Error>, }, } + +#[cfg(test)] +mod tests { + + mod whitelist_error { + + use crate::core_tests::sample_info_hash; + use crate::error::WhitelistError; + + #[test] + fn torrent_not_whitelisted() { + let err = WhitelistError::TorrentNotWhitelisted { + info_hash: sample_info_hash(), + location: std::panic::Location::caller(), + }; + + let err_msg = format!("{err}"); + + assert!( + err_msg.contains(&format!("The torrent: {}, is not whitelisted", sample_info_hash())), + "Error message did not contain expected text: {err_msg}" + ); + } + } + + mod peer_key_error { + use torrust_tracker_located_error::Located; + + use crate::databases::driver::Driver; + use crate::error::PeerKeyError; + use crate::{authentication, databases}; + + #[test] + fn duration_overflow() { + let seconds_valid = 100; + + let err = PeerKeyError::DurationOverflow { seconds_valid }; + + let err_msg = format!("{err}"); + + assert!( + err_msg.contains(&format!("Invalid peer key duration: {seconds_valid}")), + "Error message did not contain expected text: {err_msg}" + ); + } + + #[test] + fn parsing_from_string() { + let err = authentication::key::ParseKeyError::InvalidKeyLength; + + let err = PeerKeyError::InvalidKey { + key: "INVALID KEY".to_string(), + source: Located(err).into(), + }; + + let err_msg = format!("{err}"); + + assert!( + err_msg.contains(&"Invalid key: INVALID KEY".to_string()), + "Error message did not contain expected text: {err_msg}" + ); + } + + #[test] + fn persisting_into_database() { + let err = databases::error::Error::InsertFailed { + location: std::panic::Location::caller(), + driver: Driver::Sqlite3, + }; + + let err = PeerKeyError::DatabaseError { + source: Located(err).into(), + }; + + let err_msg = format!("{err}"); + + assert!( + err_msg.contains(&"Can't persist key".to_string()), + "Error message did not contain expected text: {err}" + ); + } + } +}