Skip to content

Commit 509f360

Browse files
josecelanojosecelano
committed
refactor: [torrust#521]: validate tracker API config before it starts
It shows a more friendly error message and it's easier to test.
1 parent 1033a7d commit 509f360

File tree

4 files changed

+192
-53
lines changed

4 files changed

+192
-53
lines changed

‎packages/configuration/src/http_tracker.rs

+13-41
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,15 @@ use thiserror::Error;
1414

1515
use crate::HttpTracker;
1616

17-
/// Errors that can occur when validating the plan configuration.
17+
/// Errors that can occur when validating the plain configuration.
1818
#[derive(Error, Debug, PartialEq)]
1919
pub enum ValidationError {
2020
/// Missing SSL cert path.
21-
#[error("missing SSL cert path, got: {ssl_cert_path}")]
22-
MissingSslCertPath { ssl_cert_path: String },
21+
#[error("missing SSL cert path")]
22+
MissingSslCertPath,
2323
/// Missing SSL key path.
24-
#[error("missing SSL key path, got: {ssl_key_path}")]
25-
MissingSslKeyPath { ssl_key_path: String },
24+
#[error("missing SSL key path")]
25+
MissingSslKeyPath,
2626
}
2727

2828
/// Configuration for each HTTP tracker.
@@ -50,31 +50,23 @@ impl TryFrom<HttpTracker> for Config {
5050
match config.ssl_cert_path.clone() {
5151
Some(ssl_cert_path) => {
5252
if ssl_cert_path.is_empty() {
53-
Err(ValidationError::MissingSslCertPath {
54-
ssl_cert_path: String::new(),
55-
})
53+
Err(ValidationError::MissingSslCertPath)
5654
} else {
5755
Ok(())
5856
}
5957
}
60-
None => Err(ValidationError::MissingSslCertPath {
61-
ssl_cert_path: String::new(),
62-
}),
58+
None => Err(ValidationError::MissingSslCertPath),
6359
}?;
6460

6561
match config.ssl_key_path.clone() {
6662
Some(ssl_key_path) => {
6763
if ssl_key_path.is_empty() {
68-
Err(ValidationError::MissingSslKeyPath {
69-
ssl_key_path: String::new(),
70-
})
64+
Err(ValidationError::MissingSslKeyPath)
7165
} else {
7266
Ok(())
7367
}
7468
}
75-
None => Err(ValidationError::MissingSslKeyPath {
76-
ssl_key_path: String::new(),
77-
}),
69+
None => Err(ValidationError::MissingSslKeyPath),
7870
}?;
7971
}
8072

@@ -117,12 +109,7 @@ mod tests {
117109
ssl_key_path: Some("./localhost.key".to_string()),
118110
};
119111

120-
assert_eq!(
121-
Config::try_from(plain_config),
122-
Err(ValidationError::MissingSslCertPath {
123-
ssl_cert_path: String::new()
124-
})
125-
);
112+
assert_eq!(Config::try_from(plain_config), Err(ValidationError::MissingSslCertPath));
126113
}
127114

128115
#[test]
@@ -135,12 +122,7 @@ mod tests {
135122
ssl_key_path: Some("./localhost.key".to_string()),
136123
};
137124

138-
assert_eq!(
139-
Config::try_from(plain_config),
140-
Err(ValidationError::MissingSslCertPath {
141-
ssl_cert_path: String::new()
142-
})
143-
);
125+
assert_eq!(Config::try_from(plain_config), Err(ValidationError::MissingSslCertPath));
144126
}
145127

146128
#[test]
@@ -153,12 +135,7 @@ mod tests {
153135
ssl_key_path: None,
154136
};
155137

156-
assert_eq!(
157-
Config::try_from(plain_config),
158-
Err(ValidationError::MissingSslKeyPath {
159-
ssl_key_path: String::new()
160-
})
161-
);
138+
assert_eq!(Config::try_from(plain_config), Err(ValidationError::MissingSslKeyPath));
162139
}
163140

164141
#[test]
@@ -171,12 +148,7 @@ mod tests {
171148
ssl_key_path: Some(String::new()),
172149
};
173150

174-
assert_eq!(
175-
Config::try_from(plain_config),
176-
Err(ValidationError::MissingSslKeyPath {
177-
ssl_key_path: String::new()
178-
})
179-
);
151+
assert_eq!(Config::try_from(plain_config), Err(ValidationError::MissingSslKeyPath));
180152
}
181153
}
182154
}

‎packages/configuration/src/tracker_api.rs

+162
Original file line numberDiff line numberDiff line change
@@ -1 +1,163 @@
1+
//! Validated configuration for the Tracker API service.
2+
//!
3+
//! [``crate::HttpApi``] is a DTO containing the parsed data from the toml
4+
//! file.
5+
//!
6+
//! This configuration is a first level of validation that can be perform
7+
//! statically without running the service.
8+
//!
9+
//! For example, if SSL is enabled you must provide the certificate path. That
10+
//! can be validated. However, this validation does not check if the
11+
//! certificate is valid.
12+
use serde::{Deserialize, Serialize};
13+
use thiserror::Error;
114

15+
use crate::{AccessTokens, HttpApi};
16+
17+
/// Errors that can occur when validating the plain configuration.
18+
#[derive(Error, Debug, PartialEq)]
19+
pub enum ValidationError {
20+
/// Missing SSL cert path.
21+
#[error("missing SSL cert path")]
22+
MissingSslCertPath,
23+
/// Missing SSL key path.
24+
#[error("missing SSL key path")]
25+
MissingSslKeyPath,
26+
}
27+
28+
/// Configuration for each HTTP tracker.
29+
#[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone)]
30+
pub struct Config {
31+
enabled: bool,
32+
bind_address: String, // todo: use SocketAddr
33+
ssl_enabled: bool,
34+
ssl_cert_path: Option<String>, // todo: use Path
35+
ssl_key_path: Option<String>, // todo: use Path
36+
access_tokens: AccessTokens,
37+
}
38+
39+
impl Config {
40+
#[must_use]
41+
pub fn is_enabled(&self) -> bool {
42+
self.enabled
43+
}
44+
}
45+
46+
impl TryFrom<HttpApi> for Config {
47+
type Error = ValidationError;
48+
49+
fn try_from(config: HttpApi) -> Result<Self, Self::Error> {
50+
if config.ssl_enabled {
51+
match config.ssl_cert_path.clone() {
52+
Some(ssl_cert_path) => {
53+
if ssl_cert_path.is_empty() {
54+
Err(ValidationError::MissingSslCertPath)
55+
} else {
56+
Ok(())
57+
}
58+
}
59+
None => Err(ValidationError::MissingSslCertPath),
60+
}?;
61+
62+
match config.ssl_key_path.clone() {
63+
Some(ssl_key_path) => {
64+
if ssl_key_path.is_empty() {
65+
Err(ValidationError::MissingSslKeyPath)
66+
} else {
67+
Ok(())
68+
}
69+
}
70+
None => Err(ValidationError::MissingSslKeyPath),
71+
}?;
72+
}
73+
74+
Ok(Self {
75+
enabled: config.enabled,
76+
bind_address: config.bind_address,
77+
ssl_enabled: config.ssl_enabled,
78+
ssl_cert_path: config.ssl_cert_path,
79+
ssl_key_path: config.ssl_key_path,
80+
access_tokens: config.access_tokens,
81+
})
82+
}
83+
}
84+
85+
impl From<Config> for HttpApi {
86+
fn from(config: Config) -> Self {
87+
Self {
88+
enabled: config.enabled,
89+
bind_address: config.bind_address,
90+
ssl_enabled: config.ssl_enabled,
91+
ssl_cert_path: config.ssl_cert_path,
92+
ssl_key_path: config.ssl_key_path,
93+
access_tokens: config.access_tokens,
94+
}
95+
}
96+
}
97+
98+
#[cfg(test)]
99+
mod tests {
100+
101+
mod when_ssl_is_enabled {
102+
use std::collections::HashMap;
103+
104+
use crate::tracker_api::{Config, ValidationError};
105+
use crate::HttpApi;
106+
107+
#[test]
108+
fn it_should_return_an_error_when_ssl_is_enabled_but_the_cert_path_is_not_provided() {
109+
let plain_config = HttpApi {
110+
enabled: true,
111+
bind_address: "127.0.0.1:1212".to_string(),
112+
ssl_enabled: true,
113+
ssl_cert_path: None,
114+
ssl_key_path: Some("./localhost.key".to_string()),
115+
access_tokens: HashMap::new(),
116+
};
117+
118+
assert_eq!(Config::try_from(plain_config), Err(ValidationError::MissingSslCertPath));
119+
}
120+
121+
#[test]
122+
fn it_should_return_an_error_when_ssl_is_enabled_but_the_cert_path_is_empty() {
123+
let plain_config = HttpApi {
124+
enabled: true,
125+
bind_address: "127.0.0.1:1212".to_string(),
126+
ssl_enabled: true,
127+
ssl_cert_path: Some(String::new()),
128+
ssl_key_path: Some("./localhost.key".to_string()),
129+
access_tokens: HashMap::new(),
130+
};
131+
132+
assert_eq!(Config::try_from(plain_config), Err(ValidationError::MissingSslCertPath));
133+
}
134+
135+
#[test]
136+
fn it_should_return_an_error_when_ssl_is_enabled_but_the_key_path_is_not_provided() {
137+
let plain_config = HttpApi {
138+
enabled: true,
139+
bind_address: "127.0.0.1:1212".to_string(),
140+
ssl_enabled: true,
141+
ssl_cert_path: Some("./localhost.crt".to_string()),
142+
ssl_key_path: None,
143+
access_tokens: HashMap::new(),
144+
};
145+
146+
assert_eq!(Config::try_from(plain_config), Err(ValidationError::MissingSslKeyPath));
147+
}
148+
149+
#[test]
150+
fn it_should_return_an_error_when_ssl_is_enabled_but_the_key_path_is_empty() {
151+
let plain_config = HttpApi {
152+
enabled: true,
153+
bind_address: "127.0.0.1:1212".to_string(),
154+
ssl_enabled: true,
155+
ssl_cert_path: Some("./localhost.crt".to_string()),
156+
ssl_key_path: Some(String::new()),
157+
access_tokens: HashMap::new(),
158+
};
159+
160+
assert_eq!(Config::try_from(plain_config), Err(ValidationError::MissingSslKeyPath));
161+
}
162+
}
163+
}

‎share/default/config/tracker.development.sqlite3.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ ssl_key_path = ""
2727
bind_address = "127.0.0.1:1212"
2828
enabled = true
2929
ssl_cert_path = ""
30-
ssl_enabled = false
30+
ssl_enabled = true
3131
ssl_key_path = ""
3232

3333
[http_api.access_tokens]

‎src/app.rs

+16-11
Original file line numberDiff line numberDiff line change
@@ -98,17 +98,22 @@ pub async fn start(config: &Configuration, tracker: Arc<core::Tracker>) -> Vec<J
9898
}
9999

100100
// Start HTTP API
101-
if config.http_api.enabled {
102-
if let Some(job) = tracker_apis::start_job(
103-
&config.http_api,
104-
tracker.clone(),
105-
registar.give_form(),
106-
servers::apis::Version::V1,
107-
)
108-
.await
109-
{
110-
jobs.push(job);
111-
};
101+
match torrust_tracker_configuration::tracker_api::Config::try_from(config.http_api.clone()) {
102+
Ok(tracker_api_config) => {
103+
if tracker_api_config.is_enabled() {
104+
if let Some(job) = tracker_apis::start_job(
105+
&tracker_api_config.into(),
106+
tracker.clone(),
107+
registar.give_form(),
108+
servers::apis::Version::V1,
109+
)
110+
.await
111+
{
112+
jobs.push(job);
113+
};
114+
}
115+
}
116+
Err(err) => panic!("Invalid Tracker API configuration: {err}"),
112117
}
113118

114119
// Start runners to remove torrents without peers, every interval

0 commit comments

Comments
 (0)