Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Feb 17, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
da1353b
refactor: [#1270] return errorin core announce and scrape handler
josecelano Feb 17, 2025
dbee7ad
refactor: [#1270] extract fn to new package udp-protocol
josecelano Feb 17, 2025
096d503
refactor: [#1270] inline http tracker announce invoke fn
josecelano Feb 17, 2025
6651343
refactor: [#1270] inline http tracker scrape invoke fn
josecelano Feb 17, 2025
af28429
refactor: [#1270] inline udp tracker announce invoke fn
josecelano Feb 17, 2025
d49aebd
refactor: [#1270] inline udp tracker scrape invoke fn
josecelano Feb 17, 2025
d0e6936
refactor: simplify loop with map
josecelano Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/deployment.yaml
View file
Open in desktop
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
Open in desktop

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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