Skip to content

Commit e40777f

Browse files
josecelanojosecelano
committed
refactor: [torrust#1270] return error in tracker core announce handler
when the tracker is running in `listed` mode and the client is trying to announce a torrents whose infohash is not whitelisted.
1 parent 1a5ad31 commit e40777f

File tree

11 files changed

+184
-92
lines changed

11 files changed

+184
-92
lines changed

‎packages/tracker-core/src/announce_handler.rs

+95-56
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,17 @@ use torrust_tracker_primitives::swarm_metadata::SwarmMetadata;
101101

102102
use super::torrent::repository::in_memory::InMemoryTorrentRepository;
103103
use super::torrent::repository::persisted::DatabasePersistentTorrentRepository;
104+
use crate::error::WhitelistError;
105+
use crate::whitelist::authorization::WhitelistAuthorization;
104106

105107
/// Handles `announce` requests from `BitTorrent` clients.
106108
pub struct AnnounceHandler {
107109
/// The tracker configuration.
108110
config: Core,
109111

112+
/// Service for authorizing access to whitelisted torrents.
113+
whitelist_authorization: Arc<WhitelistAuthorization>,
114+
110115
/// Repository for in-memory torrent data.
111116
in_memory_torrent_repository: Arc<InMemoryTorrentRepository>,
112117

@@ -119,10 +124,12 @@ impl AnnounceHandler {
119124
#[must_use]
120125
pub fn new(
121126
config: &Core,
127+
whitelist_authorization: &Arc<WhitelistAuthorization>,
122128
in_memory_torrent_repository: &Arc<InMemoryTorrentRepository>,
123129
db_torrent_repository: &Arc<DatabasePersistentTorrentRepository>,
124130
) -> Self {
125131
Self {
132+
whitelist_authorization: whitelist_authorization.clone(),
126133
config: config.clone(),
127134
in_memory_torrent_repository: in_memory_torrent_repository.clone(),
128135
db_torrent_repository: db_torrent_repository.clone(),
@@ -143,27 +150,23 @@ impl AnnounceHandler {
143150
/// # Returns
144151
///
145152
/// An `AnnounceData` struct containing the list of peers, swarm statistics, and tracker policy.
146-
pub fn announce(
153+
///
154+
/// # Errors
155+
///
156+
/// Returns an error if the tracker is running in `listed` mode and the
157+
/// torrent is not whitelisted.
158+
pub async fn announce(
147159
&self,
148160
info_hash: &InfoHash,
149161
peer: &mut peer::Peer,
150162
remote_client_ip: &IpAddr,
151163
peers_wanted: &PeersWanted,
152-
) -> AnnounceData {
164+
) -> Result<AnnounceData, WhitelistError> {
153165
// code-review: maybe instead of mutating the peer we could just return
154166
// a tuple with the new peer and the announce data: (Peer, AnnounceData).
155167
// It could even be a different struct: `StoredPeer` or `PublicPeer`.
156168

157-
// code-review: in the `scrape` function we perform an authorization check.
158-
// We check if the torrent is whitelisted. Should we also check authorization here?
159-
// I think so because the `Tracker` has the responsibility for checking authentication and authorization.
160-
// The `Tracker` has delegated that responsibility to the handlers
161-
// (because we want to return a friendly error response) but that does not mean we should
162-
// double-check authorization at this domain level too.
163-
// I would propose to return a `Result<AnnounceData, Error>` here.
164-
// Besides, regarding authentication the `Tracker` is also responsible for authentication but
165-
// we are actually handling authentication at the handlers level. So I would extract that
166-
// responsibility into another authentication service.
169+
self.whitelist_authorization.authorize(info_hash).await?;
167170

168171
tracing::debug!("Before: {peer:?}");
169172
peer.change_ip(&assign_ip_address_to_peer(remote_client_ip, self.config.net.external_ip));
@@ -175,11 +178,11 @@ impl AnnounceHandler {
175178
.in_memory_torrent_repository
176179
.get_peers_for(info_hash, peer, peers_wanted.limit());
177180

178-
AnnounceData {
181+
Ok(AnnounceData {
179182
peers,
180183
stats,
181184
policy: self.config.announce_policy,
182-
}
185+
})
183186
}
184187

185188
/// Updates the torrent data in memory, persists statistics if needed, and
@@ -461,8 +464,10 @@ mod tests {
461464

462465
let mut peer = sample_peer();
463466

464-
let announce_data =
465-
announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
467+
let announce_data = announce_handler
468+
.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
469+
.await
470+
.unwrap();
466471

467472
assert_eq!(announce_data.peers, vec![]);
468473
}
@@ -472,16 +477,21 @@ mod tests {
472477
let (announce_handler, _scrape_handler) = public_tracker();
473478

474479
let mut previously_announced_peer = sample_peer_1();
475-
announce_handler.announce(
476-
&sample_info_hash(),
477-
&mut previously_announced_peer,
478-
&peer_ip(),
479-
&PeersWanted::AsManyAsPossible,
480-
);
480+
announce_handler
481+
.announce(
482+
&sample_info_hash(),
483+
&mut previously_announced_peer,
484+
&peer_ip(),
485+
&PeersWanted::AsManyAsPossible,
486+
)
487+
.await
488+
.unwrap();
481489

482490
let mut peer = sample_peer_2();
483-
let announce_data =
484-
announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
491+
let announce_data = announce_handler
492+
.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
493+
.await
494+
.unwrap();
485495

486496
assert_eq!(announce_data.peers, vec![Arc::new(previously_announced_peer)]);
487497
}
@@ -491,24 +501,32 @@ mod tests {
491501
let (announce_handler, _scrape_handler) = public_tracker();
492502

493503
let mut previously_announced_peer_1 = sample_peer_1();
494-
announce_handler.announce(
495-
&sample_info_hash(),
496-
&mut previously_announced_peer_1,
497-
&peer_ip(),
498-
&PeersWanted::AsManyAsPossible,
499-
);
504+
announce_handler
505+
.announce(
506+
&sample_info_hash(),
507+
&mut previously_announced_peer_1,
508+
&peer_ip(),
509+
&PeersWanted::AsManyAsPossible,
510+
)
511+
.await
512+
.unwrap();
500513

501514
let mut previously_announced_peer_2 = sample_peer_2();
502-
announce_handler.announce(
503-
&sample_info_hash(),
504-
&mut previously_announced_peer_2,
505-
&peer_ip(),
506-
&PeersWanted::AsManyAsPossible,
507-
);
515+
announce_handler
516+
.announce(
517+
&sample_info_hash(),
518+
&mut previously_announced_peer_2,
519+
&peer_ip(),
520+
&PeersWanted::AsManyAsPossible,
521+
)
522+
.await
523+
.unwrap();
508524

509525
let mut peer = sample_peer_3();
510-
let announce_data =
511-
announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::only(1));
526+
let announce_data = announce_handler
527+
.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::only(1))
528+
.await
529+
.unwrap();
512530

513531
// It should return only one peer. There is no guarantee on
514532
// which peer will be returned.
@@ -530,8 +548,10 @@ mod tests {
530548

531549
let mut peer = seeder();
532550

533-
let announce_data =
534-
announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
551+
let announce_data = announce_handler
552+
.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
553+
.await
554+
.unwrap();
535555

536556
assert_eq!(announce_data.stats.complete, 1);
537557
}
@@ -542,8 +562,10 @@ mod tests {
542562

543563
let mut peer = leecher();
544564

545-
let announce_data =
546-
announce_handler.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
565+
let announce_data = announce_handler
566+
.announce(&sample_info_hash(), &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
567+
.await
568+
.unwrap();
547569

548570
assert_eq!(announce_data.stats.incomplete, 1);
549571
}
@@ -554,20 +576,26 @@ mod tests {
554576

555577
// We have to announce with "started" event because peer does not count if peer was not previously known
556578
let mut started_peer = started_peer();
557-
announce_handler.announce(
558-
&sample_info_hash(),
559-
&mut started_peer,
560-
&peer_ip(),
561-
&PeersWanted::AsManyAsPossible,
562-
);
579+
announce_handler
580+
.announce(
581+
&sample_info_hash(),
582+
&mut started_peer,
583+
&peer_ip(),
584+
&PeersWanted::AsManyAsPossible,
585+
)
586+
.await
587+
.unwrap();
563588

564589
let mut completed_peer = completed_peer();
565-
let announce_data = announce_handler.announce(
566-
&sample_info_hash(),
567-
&mut completed_peer,
568-
&peer_ip(),
569-
&PeersWanted::AsManyAsPossible,
570-
);
590+
let announce_data = announce_handler
591+
.announce(
592+
&sample_info_hash(),
593+
&mut completed_peer,
594+
&peer_ip(),
595+
&PeersWanted::AsManyAsPossible,
596+
)
597+
.await
598+
.unwrap();
571599

572600
assert_eq!(announce_data.stats.downloaded, 1);
573601
}
@@ -590,6 +618,8 @@ mod tests {
590618
use crate::torrent::manager::TorrentsManager;
591619
use crate::torrent::repository::in_memory::InMemoryTorrentRepository;
592620
use crate::torrent::repository::persisted::DatabasePersistentTorrentRepository;
621+
use crate::whitelist::authorization::WhitelistAuthorization;
622+
use crate::whitelist::repository::in_memory::InMemoryWhitelist;
593623

594624
#[tokio::test]
595625
async fn it_should_persist_the_number_of_completed_peers_for_all_torrents_into_the_database() {
@@ -605,8 +635,11 @@ mod tests {
605635
&in_memory_torrent_repository,
606636
&db_torrent_repository,
607637
));
638+
let in_memory_whitelist = Arc::new(InMemoryWhitelist::default());
639+
let whitelist_authorization = Arc::new(WhitelistAuthorization::new(&config.core, &in_memory_whitelist.clone()));
608640
let announce_handler = Arc::new(AnnounceHandler::new(
609641
&config.core,
642+
&whitelist_authorization,
610643
&in_memory_torrent_repository,
611644
&db_torrent_repository,
612645
));
@@ -616,11 +649,17 @@ mod tests {
616649
let mut peer = sample_peer();
617650

618651
peer.event = AnnounceEvent::Started;
619-
let announce_data = announce_handler.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
652+
let announce_data = announce_handler
653+
.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
654+
.await
655+
.unwrap();
620656
assert_eq!(announce_data.stats.downloaded, 0);
621657

622658
peer.event = AnnounceEvent::Completed;
623-
let announce_data = announce_handler.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
659+
let announce_data = announce_handler
660+
.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
661+
.await
662+
.unwrap();
624663
assert_eq!(announce_data.stats.downloaded, 1);
625664

626665
// Remove the newly updated torrent from memory

‎packages/tracker-core/src/lib.rs

+26-14
Original file line numberDiff line numberDiff line change
@@ -191,21 +191,27 @@ mod tests {
191191

192192
// Announce a "complete" peer for the torrent
193193
let mut complete_peer = complete_peer();
194-
announce_handler.announce(
195-
&info_hash,
196-
&mut complete_peer,
197-
&IpAddr::V4(Ipv4Addr::new(126, 0, 0, 10)),
198-
&PeersWanted::AsManyAsPossible,
199-
);
194+
announce_handler
195+
.announce(
196+
&info_hash,
197+
&mut complete_peer,
198+
&IpAddr::V4(Ipv4Addr::new(126, 0, 0, 10)),
199+
&PeersWanted::AsManyAsPossible,
200+
)
201+
.await
202+
.unwrap();
200203

201204
// Announce an "incomplete" peer for the torrent
202205
let mut incomplete_peer = incomplete_peer();
203-
announce_handler.announce(
204-
&info_hash,
205-
&mut incomplete_peer,
206-
&IpAddr::V4(Ipv4Addr::new(126, 0, 0, 11)),
207-
&PeersWanted::AsManyAsPossible,
208-
);
206+
announce_handler
207+
.announce(
208+
&info_hash,
209+
&mut incomplete_peer,
210+
&IpAddr::V4(Ipv4Addr::new(126, 0, 0, 11)),
211+
&PeersWanted::AsManyAsPossible,
212+
)
213+
.await
214+
.unwrap();
209215

210216
// Scrape
211217
let scrape_data = scrape_handler.scrape(&vec![info_hash]).await;
@@ -245,11 +251,17 @@ mod tests {
245251
let info_hash = "3b245504cf5f11bbdbe1201cea6a6bf45aee1bc0".parse::<InfoHash>().unwrap(); // DevSkim: ignore DS173237
246252

247253
let mut peer = incomplete_peer();
248-
announce_handler.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
254+
announce_handler
255+
.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
256+
.await
257+
.unwrap();
249258

250259
// Announce twice to force non zeroed swarm metadata
251260
let mut peer = complete_peer();
252-
announce_handler.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible);
261+
announce_handler
262+
.announce(&info_hash, &mut peer, &peer_ip(), &PeersWanted::AsManyAsPossible)
263+
.await
264+
.unwrap();
253265

254266
let scrape_data = scrape_handler.scrape(&vec![info_hash]).await;
255267

‎packages/tracker-core/src/test_helpers.rs

+1
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ pub(crate) mod tests {
177177

178178
let announce_handler = Arc::new(AnnounceHandler::new(
179179
&config.core,
180+
&whitelist_authorization,
180181
&in_memory_torrent_repository,
181182
&db_torrent_repository,
182183
));

‎packages/tracker-core/tests/integration.rs

+11-8
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ impl Container {
7676
));
7777
let announce_handler = Arc::new(AnnounceHandler::new(
7878
config,
79+
&whitelist_authorization,
7980
&in_memory_torrent_repository,
8081
&db_torrent_repository,
8182
));
@@ -102,21 +103,23 @@ async fn test_announce_and_scrape_requests() {
102103

103104
// First announce: download started
104105
peer.event = AnnounceEvent::Started;
105-
let announce_data =
106-
container
107-
.announce_handler
108-
.announce(&info_hash, &mut peer, &remote_client_ip(), &PeersWanted::AsManyAsPossible);
106+
let announce_data = container
107+
.announce_handler
108+
.announce(&info_hash, &mut peer, &remote_client_ip(), &PeersWanted::AsManyAsPossible)
109+
.await
110+
.unwrap();
109111

110112
// NOTICE: you don't get back the peer making the request.
111113
assert_eq!(announce_data.peers.len(), 0);
112114
assert_eq!(announce_data.stats.downloaded, 0);
113115

114116
// Second announce: download completed
115117
peer.event = AnnounceEvent::Completed;
116-
let announce_data =
117-
container
118-
.announce_handler
119-
.announce(&info_hash, &mut peer, &remote_client_ip(), &PeersWanted::AsManyAsPossible);
118+
let announce_data = container
119+
.announce_handler
120+
.announce(&info_hash, &mut peer, &remote_client_ip(), &PeersWanted::AsManyAsPossible)
121+
.await
122+
.unwrap();
120123

121124
assert_eq!(announce_data.peers.len(), 0);
122125
assert_eq!(announce_data.stats.downloaded, 1);

‎src/bootstrap/app.rs

+1
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@ pub fn initialize_app_container(configuration: &Configuration) -> AppContainer {
129129

130130
let announce_handler = Arc::new(AnnounceHandler::new(
131131
&configuration.core,
132+
&whitelist_authorization,
132133
&in_memory_torrent_repository,
133134
&db_torrent_repository,
134135
));

0 commit comments

Comments
 (0)