dev: improve announce ip logic test

Author: da2ce7

src/core/torrent/mod.rs

@@ -33,6 +33,7 @@ pub mod repository;
 use std::time::Duration;
 
 use aquatic_udp_protocol::AnnounceEvent;
+use log::debug;
 use serde::{Deserialize, Serialize};
 
 use super::peer::{self, Peer};
@@ -138,6 +139,7 @@ impl Entry {
     ///
     /// It filters out the input peer, typically because we want to return this
     /// list of peers to that client peer.
+    ///
     #[must_use]
     pub fn get_all_peers_for_peer(&self, client: &Peer) -> Vec<&peer::Peer> {
         self.peers
@@ -154,6 +156,9 @@ impl Entry {
     /// list of peers to that client peer.
     #[must_use]
     pub fn get_peers_for_peer(&self, client: &Peer, limit: usize) -> Vec<&peer::Peer> {
+        debug!("In db: {:?}", self.peers);
+        debug!("client: {client:?}");
+
         self.peers
             .values()
             // Take peers which are not the client peer

src/servers/http/v1/handlers/announce.rs

@@ -77,6 +77,11 @@ async fn handle(
    See https://github.com/torrust/torrust-tracker/discussions/240.
 */
 
+/* code-review (da2ce7): The IP that is supplied with the announce request is completely dropped
+   in favour of the IP supplied by the `client_ip_sources`.
+   Is this intended behavior, is the IP in the client announce request completely untrusted?
+*/
+
 async fn handle_announce(
     tracker: &Arc<Tracker>,
     announce_request: &Announce,

tests/servers/http/asserts.rs

@@ -4,6 +4,7 @@ use reqwest::Response;
 
 use super::responses::announce::{Announce, Compact, DeserializedCompact};
 use super::responses::scrape;
+use crate::servers::http::responses::announce::DictionaryPeer;
 use crate::servers::http::responses::error::Error;
 
 pub fn assert_bencoded_error(response_text: &String, expected_failure_reason: &str, location: &'static Location<'static>) {
@@ -22,10 +23,11 @@ pub fn assert_bencoded_error(response_text: &String, expected_failure_reason: &s
     );
 }
 
+#[allow(dead_code)]
 pub async fn assert_empty_announce_response(response: Response) {
     assert_eq!(response.status(), 200);
     let announce_response: Announce = serde_bencode::from_str(&response.text().await.unwrap()).unwrap();
-    assert!(announce_response.peers.is_empty());
+    assert_eq!(announce_response.peers, Vec::<DictionaryPeer>::new());
 }
 
 pub async fn assert_announce_response(response: Response, expected_announce_response: &Announce) {

tests/servers/http/v1/contract.rs

@@ -96,8 +96,8 @@ mod for_all_config_modes {
         use crate::common::fixtures::{invalid_info_hashes, PeerBuilder};
         use crate::servers::http::asserts::{
             assert_announce_response, assert_bad_announce_request_error_response, assert_cannot_parse_query_param_error_response,
-            assert_cannot_parse_query_params_error_response, assert_compact_announce_response, assert_empty_announce_response,
-            assert_is_announce_response, assert_missing_query_params_for_announce_request_error_response,
+            assert_cannot_parse_query_params_error_response, assert_compact_announce_response, assert_is_announce_response,
+            assert_missing_query_params_for_announce_request_error_response,
         };
         use crate::servers::http::client::Client;
         use crate::servers::http::requests::announce::{Compact, QueryBuilder};
@@ -497,26 +497,71 @@ mod for_all_config_modes {
             test_env.stop().await;
         }
 
+        /// code-review (da2ce7): is this really intended behavior?
         #[tokio::test]
-        async fn should_consider_two_peers_to_be_the_same_when_they_have_the_same_peer_id_even_if_the_ip_is_different() {
+        async fn it_should_drop_the_client_supplied_ip_and_ignore_peers_with_the_same_ip() {
             let test_env = running_test_environment(configuration::ephemeral_mode_public()).await;
 
-            let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap();
-            let peer = PeerBuilder::default().build();
+            let announce_policy = test_env.tracker.get_announce_policy();
 
-            // Add a peer
-            test_env.add_torrent_peer(&info_hash, &peer).await;
+            let info_hash = InfoHash::from([0; 20]);
+            let id_a = peer::Id(*b"-qB00000000000000000");
+            let id_b = peer::Id(*b"-qB00000000000000001");
 
-            let announce_query = QueryBuilder::default()
-                .with_info_hash(&info_hash)
-                .with_peer_id(&peer.peer_id)
-                .query();
+            let addr_a = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(126, 0, 0, 1)), 8080);
+            let addr_b = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(126, 0, 0, 2)), 8080);
+            let addr_c = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(126, 0, 0, 3)), 8080);
+
+            let peer_a = PeerBuilder::default().with_peer_id(&id_a).with_peer_addr(&addr_a).build();
+            let peer_b = PeerBuilder::default().with_peer_id(&id_b).with_peer_addr(&addr_b).build();
+
+            // Add both Peers into DB.
+            test_env.add_torrent_peer(&info_hash, &peer_a).await;
+            test_env.add_torrent_peer(&info_hash, &peer_b).await;
+
+            // The first query will overwrite the IP of the peer, no matter what ip we use.
+            {
+                let announce = QueryBuilder::default()
+                    .with_info_hash(&info_hash)
+                    .with_peer_id(&id_a)
+                    .with_peer_addr(&addr_c.ip()) // different ip, but this is erased.
+                    .query();
+
+                let response = Client::new(*test_env.bind_address()).announce(&announce).await;
+
+                let response_assert = Announce {
+                    complete: 2,
+                    incomplete: 0,
+                    interval: announce_policy.interval,
+                    min_interval: announce_policy.interval_min,
+                    peers: vec![peer_b.into()], // peer_b still has it's original ip.
+                };
+
+                println!("Query 1");
+                assert_announce_response(response, &response_assert).await;
+            }
+
+            // The Seconds Query will result with no listed peers, as both peer id's are now
+            // associated with the same client ip. i.e (0.0.0.0).
+            {
+                let announce = QueryBuilder::default()
+                    .with_info_hash(&info_hash)
+                    .with_peer_id(&id_b) // now we use peer b
+                    .query();
 
-            assert_ne!(peer.peer_addr.ip(), announce_query.peer_addr);
+                let response = Client::new(*test_env.bind_address()).announce(&announce).await;
 
-            let response = Client::new(*test_env.bind_address()).announce(&announce_query).await;
+                let response_assert = Announce {
+                    complete: 2,
+                    incomplete: 0,
+                    interval: announce_policy.interval,
+                    min_interval: announce_policy.interval_min,
+                    peers: vec![], // peer_a now has host ip.
+                };
 
-            assert_empty_announce_response(response).await;
+                println!("Query 2");
+                assert_announce_response(response, &response_assert).await;
+            }
 
             test_env.stop().await;
         }