diff --git a/crates/arr-api/src/health.rs b/crates/arr-api/src/health.rs index 05ddc65..f0fec51 100644 --- a/crates/arr-api/src/health.rs +++ b/crates/arr-api/src/health.rs @@ -181,21 +181,21 @@ async fn probe_prowlarr(state: &AppState) -> Check { } } -/// A live `WebUI` answers `/api/v2/app/version`, and answers `403` when the -/// probe carries no session. Both are a running daemon, which is all this -/// lamp claims; whether arr's credentials work shows up on the first grab. +/// The probe is a real torrent list through the configured client, not a bare +/// reachability check. +/// +/// A `WebUI` that wants credentials answers `403` to anything unauthenticated, +/// so "the port is open" is true of an instance arr cannot use at all. Reading +/// that as healthy once left the lamp green while every grab and every reaper +/// tick failed on a rejected password, which is the one thing this lamp exists +/// to catch. async fn probe_qbit(state: &AppState) -> Check { - let base = state.upstreams().qbittorrent_url.trim_end_matches('/'); - let request = state.http().get(format!("{base}/api/v2/app/version")); - match request.send().await { - Ok(response) - if response.status().is_success() - || response.status() == reqwest::StatusCode::FORBIDDEN => - { - Check::ok() - } - Ok(response) => Check::unreachable(format!("http {}", response.status().as_u16())), - Err(err) => Check::unreachable(describe(err)), + let Some(qbit) = state.qbit() else { + return Check::unconfigured("no qBittorrent client is configured"); + }; + match qbit.list_torrents().await { + Ok(_) => Check::ok(), + Err(error) => Check::unreachable(error.to_string()), } } diff --git a/crates/arr-api/src/lib.rs b/crates/arr-api/src/lib.rs index 36dff92..ad787b0 100644 --- a/crates/arr-api/src/lib.rs +++ b/crates/arr-api/src/lib.rs @@ -191,8 +191,8 @@ mod tests { let qbit = MockServer::start().await; Mock::given(method("GET")) - .and(path("/api/v2/app/version")) - .respond_with(ResponseTemplate::new(403).set_body_string("Forbidden")) + .and(path("/api/v2/torrents/info")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) .mount(&qbit) .await; @@ -237,6 +237,7 @@ mod tests { .with_tmdb_api_key(Some("key".into())), ) .expect("state") + .with_qbit(arr_dl::QbitClient::new(&qbit.uri()).expect("client")) // The subtitle binaries default to `PATH`; pin them to something // every machine has so this test stays about the classic upstreams. .with_syncer(arr_subs::Syncer::new().with_binary("sh")) @@ -253,7 +254,9 @@ mod tests { #[tokio::test] async fn a_missing_tmdb_key_is_unconfigured_not_an_outage() { let (prowlarr, qbit) = upstreams_up().await; - let state = AppState::new(Upstreams::new(prowlarr.uri(), qbit.uri())).expect("state"); + let state = AppState::new(Upstreams::new(prowlarr.uri(), qbit.uri())) + .expect("state") + .with_qbit(arr_dl::QbitClient::new(&qbit.uri()).expect("client")); let body = report(state).await; assert_eq!(body["tmdb"]["status"], "unconfigured"); @@ -266,8 +269,9 @@ mod tests { // Port 1 is privileged and nothing binds it, so the probe gets a // refused connection immediately instead of waiting out the timeout. - let state = - AppState::new(Upstreams::new("http://127.0.0.1:1".into(), qbit.uri())).expect("state"); + let state = AppState::new(Upstreams::new("http://127.0.0.1:1".into(), qbit.uri())) + .expect("state") + .with_qbit(arr_dl::QbitClient::new(&qbit.uri()).expect("client")); let body = report(state).await; assert_eq!(body["status"], "degraded"); @@ -307,7 +311,8 @@ mod tests { .with_tmdb_url(tmdb.uri()) .with_tmdb_api_key(Some("super-secret".into())), ) - .expect("state"); + .expect("state") + .with_qbit(arr_dl::QbitClient::new(&qbit.uri()).expect("client")); let body = report(state).await; assert_eq!(body["tmdb"]["status"], "unreachable"); diff --git a/crates/arr-dl/src/lib.rs b/crates/arr-dl/src/lib.rs index 5688dd1..9d555fe 100644 --- a/crates/arr-dl/src/lib.rs +++ b/crates/arr-dl/src/lib.rs @@ -479,10 +479,11 @@ impl QbitClient { path: &str, build: impl Fn() -> reqwest::RequestBuilder, ) -> Result { - if self.credentials.is_some() && self.session.read().await.is_none() { - self.login().await?; - } - + // Deliberately no login before the first call. An instance that + // bypasses authentication for arr's address answers everything, and + // logging in first would turn a stale or wrong password into a hard + // failure on a call that would have succeeded. Authentication is + // established only when qBittorrent actually asks for it, below. for attempt in 0..2 { let session = self.session.read().await.clone(); let mut request = build().header(REFERER, &self.origin); @@ -496,7 +497,7 @@ impl QbitClient { if self.credentials.is_none() { return Err(Error::Unauthenticated); } - // The session expired or the client restarted under us. + // No session yet, or the one held expired. self.login().await?; continue; } @@ -668,7 +669,7 @@ mod tests { use std::path::PathBuf; use serde_json::json; - use wiremock::matchers::{body_string_contains, method, path, query_param}; + use wiremock::matchers::{body_string_contains, header, method, path, query_param}; use wiremock::{Mock, MockServer, ResponseTemplate}; use super::{AddTorrent, Error, QbitClient, TorrentSource, TorrentState}; @@ -688,7 +689,7 @@ mod tests { } #[tokio::test] - async fn logs_in_once_and_reuses_the_session() { + async fn logs_in_once_on_the_first_403_and_reuses_the_session() { let server = MockServer::start().await; Mock::given(method("POST")) .and(path("/api/v2/auth/login")) @@ -702,8 +703,17 @@ mod tests { .await; Mock::given(method("GET")) .and(path("/api/v2/torrents/info")) + .and(header("cookie", "SID=abc123")) .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) .expect(2) + .with_priority(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/api/v2/torrents/info")) + .respond_with(ResponseTemplate::new(403).set_body_string("Forbidden")) + .expect(1) + .with_priority(2) .mount(&server) .await; @@ -713,9 +723,37 @@ mod tests { assert!(client.list_torrents().await.unwrap().is_empty()); } + /// An instance that bypasses authentication for arr's address answers + /// every call, so a stale password configured alongside it must not turn + /// a working call into a failure. Nothing here ever reaches `auth/login`. + #[tokio::test] + async fn a_bypassed_instance_is_never_logged_into() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/api/v2/auth/login")) + .respond_with(ResponseTemplate::new(200).set_body_string("Fails.")) + .expect(0) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/api/v2/torrents/info")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .mount(&server) + .await; + + let client = + QbitClient::with_credentials(&server.uri(), "user".into(), "stale".into()).unwrap(); + assert!(client.list_torrents().await.unwrap().is_empty()); + } + #[tokio::test] async fn a_wrong_password_is_a_login_error_not_a_retry_loop() { let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/api/v2/torrents/info")) + .respond_with(ResponseTemplate::new(403).set_body_string("Forbidden")) + .mount(&server) + .await; Mock::given(method("POST")) .and(path("/api/v2/auth/login")) .respond_with(ResponseTemplate::new(200).set_body_string("Fails."))