Skip to content

Commit da1353b

Browse files
josecelanojosecelano
committed
refactor: [torrust#1270] return errorin core announce and scrape handler
- In the announce handler, it returns an error when the tracker is running in `listed` mode and the infohash is not whitelisted. This was done only in the delivery layers but not in the domain. - In the scrape handler, it does not return any errors for now, but It will allow us in the future to return errors whithout making breaking changes.
1 parent 1a5ad31 commit da1353b

File tree

16 files changed

+271
-135
lines changed

16 files changed

+271
-135
lines changed

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

+17-1
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,26 @@ impl From<PeerIpResolutionError> for Error {
5555
}
5656
}
5757

58+
impl From<bittorrent_tracker_core::error::AnnounceError> for Error {
59+
fn from(err: bittorrent_tracker_core::error::AnnounceError) -> Self {
60+
Error {
61+
failure_reason: format!("Tracker announce error: {err}"),
62+
}
63+
}
64+
}
65+
66+
impl From<bittorrent_tracker_core::error::ScrapeError> for Error {
67+
fn from(err: bittorrent_tracker_core::error::ScrapeError) -> Self {
68+
Error {
69+
failure_reason: format!("Tracker scrape error: {err}"),
70+
}
71+
}
72+
}
73+
5874
impl From<bittorrent_tracker_core::error::WhitelistError> for Error {
5975
fn from(err: bittorrent_tracker_core::error::WhitelistError) -> Self {
6076
Error {
61-
failure_reason: format!("Tracker error: {err}"),
77+
failure_reason: format!("Tracker whitelist error: {err}"),
6278
}
6379
}
6480
}

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

+96-57
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::AnnounceError;
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, AnnounceError> {
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,10 +618,12 @@ 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() {
596-
let mut config = configuration::ephemeral_listed();
626+
let mut config = configuration::ephemeral_public();
597627

598628
config.core.tracker_policy.persistent_torrent_completed_stat = true;
599629

@@ -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/error.rs

+16
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,22 @@ use torrust_tracker_located_error::LocatedError;
1515
use super::authentication::key::ParseKeyError;
1616
use super::databases;
1717

18+
/// Errors related to announce requests.
19+
#[derive(thiserror::Error, Debug, Clone)]
20+
pub enum AnnounceError {
21+
/// Wraps errors related to torrent whitelisting.
22+
#[error("Whitelist error: {0}")]
23+
Whitelist(#[from] WhitelistError),
24+
}
25+
26+
/// Errors related to scrape requests.
27+
#[derive(thiserror::Error, Debug, Clone)]
28+
pub enum ScrapeError {
29+
/// Wraps errors related to torrent whitelisting.
30+
#[error("Whitelist error: {0}")]
31+
Whitelist(#[from] WhitelistError),
32+
}
33+
1834
/// Errors related to torrent whitelisting.
1935
///
2036
/// This error is returned when an operation involves a torrent that is not

0 commit comments

Comments
 (0)