Merge #1274: Overhaul core Tracker: review whitelist functionality (part 2)

d0e6936 refactor: simplify loop with map (Jose Celano)
d49aebd refactor: [#1270] inline udp tracker scrape invoke fn (Jose Celano)
af28429 refactor: [#1270] inline udp tracker announce invoke fn (Jose Celano)
6651343  refactor: [#1270] inline http tracker scrape invoke fn (Jose Celano)
096d503 refactor: [#1270] inline http tracker announce invoke fn (Jose Celano)
dbee7ad refactor: [#1270] extract fn to new package udp-protocol (Jose Celano)
da1353b refactor: [#1270] return errorin core announce and scrape handler (Jose Celano)

Pull request description:

  Overhaul core Tracker: review whitelist functionality (part 2).

  - [x] Return an error in the announce handler if the tracker is running in `listed` mode and the infohash is not whitelisted. This is already implemented in the higher delivery layer but it should be also validated in the tracker core (domain layer).
  - [x] Change scrape handler signature to return an error even if it's not needed now. That will allow us to return an error in the future whiteout breaking changes.
  - [x] Move `packages::udp_tracker_core::peer_builder::from_request` to new package `udp-protocol`
  - [x] Inline `invoke` function in handlers (`http-tracker-core` and `udp-tracker-core`) when possible.

ACKs for top commit:
  josecelano:
    ACK d0e6936

Tree-SHA512: b4e9356291b35b1cc6a20a5b28a6c0b9136baafbb9bc71638bd83932709970f3ee8bee91a81cc09a775244fd3e774131d238cf4f52920bcc141fed4161b86b03

Author: josecelano

.github/workflows/deployment.yaml

@@ -58,6 +58,7 @@ jobs:
           cargo publish -p bittorrent-http-protocol
           cargo publish -p bittorrent-tracker-client
           cargo publish -p bittorrent-tracker-core
+          cargo publish -p bittorrent-udp-protocol
           cargo publish -p torrust-tracker
           cargo publish -p torrust-tracker-api-client
           cargo publish -p torrust-tracker-client

Cargo.lock

@@ -43,6 +43,7 @@ bittorrent-http-protocol = { version = "3.0.0-develop", path = "packages/http-pr
 bittorrent-primitives = "0.1.0"
 bittorrent-tracker-client = { version = "3.0.0-develop", path = "packages/tracker-client" }
 bittorrent-tracker-core = { version = "3.0.0-develop", path = "packages/tracker-core" }
+bittorrent-udp-protocol = { version = "3.0.0-develop", path = "packages/udp-protocol" }
 bloom = "0.3.2"
 blowfish = "0"
 camino = { version = "1", features = ["serde", "serde1"] }

Cargo.toml

@@ -183,6 +183,17 @@ impl fmt::Display for Event {
     }
 }
 
+impl From<aquatic_udp_protocol::request::AnnounceEvent> for Event {
+    fn from(value: aquatic_udp_protocol::request::AnnounceEvent) -> Self {
+        match value {
+            AnnounceEvent::Started => Self::Started,
+            AnnounceEvent::Stopped => Self::Stopped,
+            AnnounceEvent::Completed => Self::Completed,
+            AnnounceEvent::None => panic!("can't convert announce event from aquatic for None variant"),
+        }
+    }
+}
+
 /// Whether the `announce` response should be in compact mode or not.
 ///
 /// Depending on the value of this param, the tracker will return a different

packages/http-protocol/src/v1/requests/announce.rs

@@ -55,10 +55,26 @@ impl From<PeerIpResolutionError> for Error {
     }
 }
 
+impl From<bittorrent_tracker_core::error::AnnounceError> for Error {
+    fn from(err: bittorrent_tracker_core::error::AnnounceError) -> Self {
+        Error {
+            failure_reason: format!("Tracker announce error: {err}"),
+        }
+    }
+}
+
+impl From<bittorrent_tracker_core::error::ScrapeError> for Error {
+    fn from(err: bittorrent_tracker_core::error::ScrapeError) -> Self {
+        Error {
+            failure_reason: format!("Tracker scrape error: {err}"),
+        }
+    }
+}
+
 impl From<bittorrent_tracker_core::error::WhitelistError> for Error {
     fn from(err: bittorrent_tracker_core::error::WhitelistError) -> Self {
         Error {
-            failure_reason: format!("Tracker error: {err}"),
+            failure_reason: format!("Tracker whitelist error: {err}"),
         }
     }
 }

packages/http-protocol/src/v1/responses/error.rs

@@ -101,12 +101,17 @@ use torrust_tracker_primitives::swarm_metadata::SwarmMetadata;
 
 use super::torrent::repository::in_memory::InMemoryTorrentRepository;
 use super::torrent::repository::persisted::DatabasePersistentTorrentRepository;
+use crate::error::AnnounceError;
+use crate::whitelist::authorization::WhitelistAuthorization;
 
 /// Handles `announce` requests from `BitTorrent` clients.
 pub struct AnnounceHandler {
     /// The tracker configuration.
     config: Core,
 
+    /// Service for authorizing access to whitelisted torrents.
+    whitelist_authorization: Arc<WhitelistAuthorization>,
+
     /// Repository for in-memory torrent data.
     in_memory_torrent_repository: Arc<InMemoryTorrentRepository>,
 
@@ -119,10 +124,12 @@ impl AnnounceHandler {
     #[must_use]
     pub fn new(
         config: &Core,
+        whitelist_authorization: &Arc<WhitelistAuthorization>,
         in_memory_torrent_repository: &Arc<InMemoryTorrentRepository>,
         db_torrent_repository: &Arc<DatabasePersistentTorrentRepository>,
     ) -> Self {
         Self {
+            whitelist_authorization: whitelist_authorization.clone(),
             config: config.clone(),
             in_memory_torrent_repository: in_memory_torrent_repository.clone(),
             db_torrent_repository: db_torrent_repository.clone(),
@@ -143,27 +150,23 @@ impl AnnounceHandler {
     /// # Returns
     ///
     /// An `AnnounceData` struct containing the list of peers, swarm statistics, and tracker policy.
-    pub fn announce(
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if the tracker is running in `listed` mode and the
+    /// torrent is not whitelisted.
+    pub async fn announce(
         &self,
         info_hash: &InfoHash,
         peer: &mut peer::Peer,
         remote_client_ip: &IpAddr,
         peers_wanted: &PeersWanted,
-    ) -> AnnounceData {
+    ) -> Result<AnnounceData, AnnounceError> {
         // code-review: maybe instead of mutating the peer we could just return
         // a tuple with the new peer and the announce data: (Peer, AnnounceData).
         // It could even be a different struct: `StoredPeer` or `PublicPeer`.
 
-        // code-review: in the `scrape` function we perform an authorization check.
-        // We check if the torrent is whitelisted. Should we also check authorization here?
-        // I think so because the `Tracker` has the responsibility for checking authentication and authorization.
-        // The `Tracker` has delegated that responsibility to the handlers
-        // (because we want to return a friendly error response) but that does not mean we should
-        // double-check authorization at this domain level too.
-        // I would propose to return a `Result<AnnounceData, Error>` here.
-        // Besides, regarding authentication the `Tracker` is also responsible for authentication but
-        // we are actually handling authentication at the handlers level. So I would extract that
-        // responsibility into another authentication service.
+        self.whitelist_authorization.authorize(info_hash).await?;
 
         tracing::debug!("Before: {peer:?}");
         peer.change_ip(&assign_ip_address_to_peer(remote_client_ip, self.config.net.external_ip));
@@ -175,11 +178,11 @@ impl AnnounceHandler {
             .in_memory_torrent_repository
             .get_peers_for(info_hash, peer, peers_wanted.limit());
 
-        AnnounceData {
+        Ok(AnnounceData {
             peers,
             stats,
             policy: self.config.announce_policy,
-        }
+        })
     }
 
     /// Updates the torrent data in memory, persists statistics if needed, and
@@ -461,8 +464,10 @@ mod tests {
 
                     let mut peer = sample_peer();
 
-                    let announce_data =
-                        announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
+                    let announce_data = announce_handler
+                        .announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
+                        .await
+                        .unwrap();
 
                     assert_eq!(announce_data.peers, vec![]);
                 }
@@ -472,16 +477,21 @@ mod tests {
                     let (announce_handler, _scrape_handler) = public_tracker();
 
                     let mut previously_announced_peer = sample_peer_1();
-                    announce_handler.announce(
-                        &sample_info_hash(),
-                        &mut previously_announced_peer,
-                        &peer_ip(),
-                        &PeersWanted::AsManyAsPossible,
-                    );
+                    announce_handler
+                        .announce(
+                            &sample_info_hash(),
+                            &mut previously_announced_peer,
+                            &peer_ip(),
+                            &PeersWanted::AsManyAsPossible,
+                        )
+                        .await
+                        .unwrap();
 
                     let mut peer = sample_peer_2();
-                    let announce_data =
-                        announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
+                    let announce_data = announce_handler
+                        .announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
+                        .await
+                        .unwrap();
 
                     assert_eq!(announce_data.peers, vec![Arc::new(previously_announced_peer)]);
                 }
@@ -491,24 +501,32 @@ mod tests {
                     let (announce_handler, _scrape_handler) = public_tracker();
 
                     let mut previously_announced_peer_1 = sample_peer_1();
-                    announce_handler.announce(
-                        &sample_info_hash(),
-                        &mut previously_announced_peer_1,
-                        &peer_ip(),
-                        &PeersWanted::AsManyAsPossible,
-                    );
+                    announce_handler
+                        .announce(
+                            &sample_info_hash(),
+                            &mut previously_announced_peer_1,
+                            &peer_ip(),
+                            &PeersWanted::AsManyAsPossible,
+                        )
+                        .await
+                        .unwrap();
 
                     let mut previously_announced_peer_2 = sample_peer_2();
-                    announce_handler.announce(
-                        &sample_info_hash(),
-                        &mut previously_announced_peer_2,
-                        &peer_ip(),
-                        &PeersWanted::AsManyAsPossible,
-                    );
+                    announce_handler
+                        .announce(
+                            &sample_info_hash(),
+                            &mut previously_announced_peer_2,
+                            &peer_ip(),
+                            &PeersWanted::AsManyAsPossible,
+                        )
+                        .await
+                        .unwrap();
 
                     let mut peer = sample_peer_3();
-                    let announce_data =
-                        announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::only(1));
+                    let announce_data = announce_handler
+                        .announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::only(1))
+                        .await
+                        .unwrap();
 
                     // It should return only one peer. There is no guarantee on
                     // which peer will be returned.
@@ -530,8 +548,10 @@ mod tests {
 
                         let mut peer = seeder();
 
-                        let announce_data =
-                            announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
+                        let announce_data = announce_handler
+                            .announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
+                            .await
+                            .unwrap();
 
                         assert_eq!(announce_data.stats.complete, 1);
                     }
@@ -542,8 +562,10 @@ mod tests {
 
                         let mut peer = leecher();
 
-                        let announce_data =
-                            announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
+                        let announce_data = announce_handler
+                            .announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
+                            .await
+                            .unwrap();
 
                         assert_eq!(announce_data.stats.incomplete, 1);
                     }
@@ -554,20 +576,26 @@ mod tests {
 
                         // We have to announce with "started" event because peer does not count if peer was not previously known
                         let mut started_peer = started_peer();
-                        announce_handler.announce(
-                            &sample_info_hash(),
-                            &mut started_peer,
-                            &peer_ip(),
-                            &PeersWanted::AsManyAsPossible,
-                        );
+                        announce_handler
+                            .announce(
+                                &sample_info_hash(),
+                                &mut started_peer,
+                                &peer_ip(),
+                                &PeersWanted::AsManyAsPossible,
+                            )
+                            .await
+                            .unwrap();
 
                         let mut completed_peer = completed_peer();
-                        let announce_data = announce_handler.announce(
-                            &sample_info_hash(),
-                            &mut completed_peer,
-                            &peer_ip(),
-                            &PeersWanted::AsManyAsPossible,
-                        );
+                        let announce_data = announce_handler
+                            .announce(
+                                &sample_info_hash(),
+                                &mut completed_peer,
+                                &peer_ip(),
+                                &PeersWanted::AsManyAsPossible,
+                            )
+                            .await
+                            .unwrap();
 
                         assert_eq!(announce_data.stats.downloaded, 1);
                     }
@@ -590,10 +618,12 @@ mod tests {
             use crate::torrent::manager::TorrentsManager;
             use crate::torrent::repository::in_memory::InMemoryTorrentRepository;
             use crate::torrent::repository::persisted::DatabasePersistentTorrentRepository;
+            use crate::whitelist::authorization::WhitelistAuthorization;
+            use crate::whitelist::repository::in_memory::InMemoryWhitelist;
 
             #[tokio::test]
             async fn it_should_persist_the_number_of_completed_peers_for_all_torrents_into_the_database() {
-                let mut config = configuration::ephemeral_listed();
+                let mut config = configuration::ephemeral_public();
 
                 config.core.tracker_policy.persistent_torrent_completed_stat = true;
 
@@ -605,8 +635,11 @@ mod tests {
                     &in_memory_torrent_repository,
                     &db_torrent_repository,
                 ));
+                let in_memory_whitelist = Arc::new(InMemoryWhitelist::default());
+                let whitelist_authorization = Arc::new(WhitelistAuthorization::new(&config.core, &in_memory_whitelist.clone()));
                 let announce_handler = Arc::new(AnnounceHandler::new(
                     &config.core,
+                    &whitelist_authorization,
                     &in_memory_torrent_repository,
                     &db_torrent_repository,
                 ));
@@ -616,11 +649,17 @@ mod tests {
                 let mut peer = sample_peer();
 
                 peer.event = AnnounceEvent::Started;
-                let announce_data = announce_handler.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
+                let announce_data = announce_handler
+                    .announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
+                    .await
+                    .unwrap();
                 assert_eq!(announce_data.stats.downloaded, 0);
 
                 peer.event = AnnounceEvent::Completed;
-                let announce_data = announce_handler.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
+                let announce_data = announce_handler
+                    .announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
+                    .await
+                    .unwrap();
                 assert_eq!(announce_data.stats.downloaded, 1);
 
                 // Remove the newly updated torrent from memory

packages/tracker-core/src/announce_handler.rs

@@ -15,6 +15,22 @@ use torrust_tracker_located_error::LocatedError;
 use super::authentication::key::ParseKeyError;
 use super::databases;
 
+/// Errors related to announce requests.
+#[derive(thiserror::Error, Debug, Clone)]
+pub enum AnnounceError {
+    /// Wraps errors related to torrent whitelisting.
+    #[error("Whitelist error: {0}")]
+    Whitelist(#[from] WhitelistError),
+}
+
+/// Errors related to scrape requests.
+#[derive(thiserror::Error, Debug, Clone)]
+pub enum ScrapeError {
+    /// Wraps errors related to torrent whitelisting.
+    #[error("Whitelist error: {0}")]
+    Whitelist(#[from] WhitelistError),
+}
+
 /// Errors related to torrent whitelisting.
 ///
 /// This error is returned when an operation involves a torrent that is not