Skip to content

Commit

Permalink
Merge #1274: Overhaul core Tracker: review whitelist functionality (p…
Browse files Browse the repository at this point in the history 
…art 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
  • Loading branch information
@josecelano
josecelano committed Feb 17, 2025
2 parents 1a5ad31 + d0e6936 commit 467d50a
Show file tree
Hide file tree
Showing 27 changed files with 1,189 additions and 271 deletions.
1 change: 1 addition & 0 deletions .github/workflows/deployment.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
Expand Down
11 changes: 11 additions & 0 deletions packages/http-protocol/src/v1/requests/announce.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 17 additions & 1 deletion packages/http-protocol/src/v1/responses/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}"),
}
}
}
Expand Down
153 changes: 96 additions & 57 deletions packages/tracker-core/src/announce_handler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>,

Expand All @@ -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(),
Expand All @@ -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));
Expand All @@ -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
Expand Down Expand Up @@ -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![]);
}
Expand All @@ -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)]);
}
Expand All @@ -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.
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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;

Expand All @@ -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,
));
Expand All @@ -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
Expand Down
16 changes: 16 additions & 0 deletions packages/tracker-core/src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 467d50a

Please sign in to comment.