From a16f73543dc1c151c71a4944b63634bbeca56613 Mon Sep 17 00:00:00 2001 From: Yoshihiro OKUMURA Date: Fri, 4 Sep 2026 16:31:14 +0900 Subject: [PATCH] fix: stream transfers and close gaps against the Python client A review against the Python client, which shares this client's config and login cache, found the transfers holding whole files in memory and several commands answering differently from their Python counterparts. - Stream uploads and downloads instead of buffering the whole file, so memory no longer scales with file size times concurrency. Uploads still declare a Content-Length rather than going out chunked. - Write a download beside its destination and move it into place once complete, and refuse a destination that cannot be written, so a failed transfer leaves what was already there untouched. - Report the server's own error detail instead of the bare status. - List locked sub-folders in `ls --json --recursive` using the given password and skip only those that cannot be unlocked, instead of failing the whole listing. - Name each folder's laboratory in JSON output and sort its entries, matching the Python client's schema and order. - Ask the server who is logged in for `whoami`, rather than trusting the cached name. - Collapse repeated separators and refuse `..` in remote paths. - Fall back to the API when a laboratory is missing from the cache, so a newly added one no longer needs a fresh login. - Verify the login cache digest on read, as the Python client does. - Accept `-e` as the short form of `--exclude`. - Satisfy clippy and rustfmt across the crate. --- Cargo.lock | 65 +++++-- Cargo.toml | 3 +- README.md | 2 +- src/api/doi.rs | 4 +- src/api/files.rs | 352 ++++++++++++++++++++++++++++++++-- src/api/folders.rs | 19 +- src/api/laboratories.rs | 3 +- src/api/users.rs | 6 +- src/cache/mod.rs | 73 ++++++- src/cli.rs | 2 +- src/commands/chacl.rs | 7 +- src/commands/config.rs | 24 +-- src/commands/cp.rs | 10 +- src/commands/download.rs | 270 +++++++++++--------------- src/commands/file_metadata.rs | 6 +- src/commands/ls.rs | 93 +++++++-- src/commands/metadata.rs | 6 +- src/commands/mkdir.rs | 8 +- src/commands/mv.rs | 10 +- src/commands/rm.rs | 9 +- src/commands/shared.rs | 95 +++++++-- src/commands/upload.rs | 46 ++--- src/commands/whoami.rs | 15 +- src/error.rs | 80 ++++++++ src/main.rs | 2 +- src/settings.rs | 13 +- 26 files changed, 899 insertions(+), 324 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8f7c1b8..5af9bf0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -878,9 +878,9 @@ dependencies = [ [[package]] name = "js-sys" -version = "0.3.100" +version = "0.3.103" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f2025f20d7a4fa7785846e7b63d10a76d3f1cee98ee5cb79ea59703f95e42162" +checksum = "53b44bfcdb3f8d5837a46dae1ca9660a837176eee74a28b229bc626816589102" dependencies = [ "cfg-if", "futures-util", @@ -907,9 +907,9 @@ checksum = "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61ece587de66" [[package]] name = "libredox" -version = "0.1.17" +version = "0.1.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f02ab6bace2054fb888a3c16f990117b579d14a3088e472d63c6011fa185c9d3" +checksum = "c943259e342f1e06ff2da7a83eabdfe7f92ce10262688dbf1895ff0b3e6e4652" dependencies = [ "libc", ] @@ -980,6 +980,7 @@ dependencies = [ "tar", "tempfile", "tokio", + "tokio-util", "unicode-normalization", "zip", ] @@ -1493,12 +1494,14 @@ dependencies = [ "sync_wrapper", "tokio", "tokio-rustls", + "tokio-util", "tower", "tower-http", "tower-service", "url", "wasm-bindgen", "wasm-bindgen-futures", + "wasm-streams", "web-sys", "webpki-roots", ] @@ -1924,6 +1927,19 @@ dependencies = [ "tokio", ] +[[package]] +name = "tokio-util" +version = "0.7.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "494815d09bf52b5548659851081238f0ca39ff638363907596da739561c62c52" +dependencies = [ + "bytes", + "futures-core", + "futures-sink", + "pin-project-lite", + "tokio", +] + [[package]] name = "tower" version = "0.5.3" @@ -2080,9 +2096,9 @@ checksum = "ccf3ec651a847eb01de73ccad15eb7d99f80485de043efb2f370cd654f4ea44b" [[package]] name = "wasip2" -version = "1.0.3+wasi-0.2.9" +version = "1.0.4+wasi-0.2.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "20064672db26d7cdc89c7798c48a0fdfac8213434a1186e5ef29fd560ae223d6" +checksum = "b67efb37e106e55ce722a510d6b5f9c17f083e5fc79afc2badeb12cc313d9487" dependencies = [ "wit-bindgen 0.57.1", ] @@ -2098,9 +2114,9 @@ dependencies = [ [[package]] name = "wasm-bindgen" -version = "0.2.123" +version = "0.2.126" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a254a4b10c19a76f09a27640e7ffbf9bc30bf67e16a3bf28aaefa4920fe81563" +checksum = "4b067c0c11094aef6b7a801c1e34a26affafdf3d051dba08456b868789aaf9a4" dependencies = [ "cfg-if", "once_cell", @@ -2111,9 +2127,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-futures" -version = "0.4.73" +version = "0.4.76" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "54568702fabf5d4849ce2b90fadfa64168a097eaf4b351ce9df8b687a0086aaf" +checksum = "c62df1340f32221cb9c54d6a27b030e3dba64361d4a95bed55f9aacb44da291d" dependencies = [ "js-sys", "wasm-bindgen", @@ -2121,9 +2137,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro" -version = "0.2.123" +version = "0.2.126" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "24a40fc75b0ec6f3746ceb10d36f53a93dcd68a93b11b6445983945d79eba0dc" +checksum = "167ce5e579f6bcf889c4f7175a8a5a585de84e8ff93976ce393efa5f2837aab1" dependencies = [ "quote", "wasm-bindgen-macro-support", @@ -2131,9 +2147,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro-support" -version = "0.2.123" +version = "0.2.126" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "908f34bd9b9ce3d4caf07b72dfab63d61504d156856c6bd3cd87fa350cf3985b" +checksum = "f3997c7839262f4ef12cf90b818d6340c18e80f263f1a94bf157d0ec4420380e" dependencies = [ "bumpalo", "proc-macro2", @@ -2144,9 +2160,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-shared" -version = "0.2.123" +version = "0.2.126" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7acbf7616c27b194bbb550bf77ed0c2c3e5b7fd1260a93082b95fb7f47959b92" +checksum = "dc1b4cb0cc549fcf58d7dfc081778139b3d283a081644e833e84682ad71cea24" dependencies = [ "unicode-ident", ] @@ -2173,6 +2189,19 @@ dependencies = [ "wasmparser", ] +[[package]] +name = "wasm-streams" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "15053d8d85c7eccdbefef60f06769760a563c7f0a9d6902a13d35c7800b0ad65" +dependencies = [ + "futures-util", + "js-sys", + "wasm-bindgen", + "wasm-bindgen-futures", + "web-sys", +] + [[package]] name = "wasmparser" version = "0.244.0" @@ -2187,9 +2216,9 @@ dependencies = [ [[package]] name = "web-sys" -version = "0.3.100" +version = "0.3.103" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6e0871acf327f283dc6da28a1696cdc64fb355ba9f935d052021fa77f35cce69" +checksum = "8622dcb61c0bcc9fffa6938bed81210af2da9a7e4a1a834b2e37a59b6dfb6141" dependencies = [ "js-sys", "wasm-bindgen", diff --git a/Cargo.toml b/Cargo.toml index 7bb6313..69f46b9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,10 +12,11 @@ path = "src/main.rs" [dependencies] clap = { version = "4.5", features = ["derive"] } -reqwest = { version = "0.12", default-features = false, features = ["json", "multipart", "rustls-tls"] } +reqwest = { version = "0.12", default-features = false, features = ["json", "multipart", "rustls-tls", "stream"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.150" tokio = { version = "1.52.3", features = ["full"] } +tokio-util = { version = "0.7", features = ["io"] } futures = "0.3" dirs = "6.0.0" anyhow = "1.0.102" diff --git a/README.md b/README.md index 116aa3d..3cede4b 100644 --- a/README.md +++ b/README.md @@ -143,7 +143,7 @@ Download a file or folder from a remote path. You can also specify a DOI path. mdrs download neurodata:/NIU/Repository/TEST/sample.dat ./ mdrs download -r neurodata:/NIU/Repository/TEST/dataset/ ./ mdrs download -p SHARING_PASSWORD neurodata:/NIU/Repository/PW_Open/Readme.dat ./ -mdrs download -r --exclude /NIU/Repository/TEST/dataset/skip neurodata:/NIU/Repository/TEST/dataset/ ./ +mdrs download -r -e /NIU/Repository/TEST/dataset/skip neurodata:/NIU/Repository/TEST/dataset/ ./ mdrs download -r --skip-if-exists neurodata:/NIU/Repository/TEST/dataset/ ./ # DOI access examples: diff --git a/src/api/doi.rs b/src/api/doi.rs index 6bbbd1c..ca20af9 100644 --- a/src/api/doi.rs +++ b/src/api/doi.rs @@ -1,6 +1,6 @@ use crate::connection::MDRSConnection; +use crate::error::response_error; use crate::models::doi::DoiResponse; -use anyhow::bail; impl MDRSConnection { /// Retrieve the folder associated with a DOI suffix ID (GET v3/doi/{id}/). @@ -10,7 +10,7 @@ impl MDRSConnection { pub async fn retrieve_doi(&self, id: &str) -> Result { let resp = self.get(&format!("v3/doi/{}/", id)).await?; if !resp.status().is_success() { - bail!("DOI lookup failed: {}", resp.status()); + return Err(response_error("DOI lookup failed", resp).await); } Ok(resp.json::().await?) } diff --git a/src/api/files.rs b/src/api/files.rs index 6a6a135..8ebd741 100644 --- a/src/api/files.rs +++ b/src/api/files.rs @@ -1,8 +1,30 @@ use crate::connection::{ApiRequestLimiter, MDRSConnection}; +use crate::error::response_error; pub use crate::models::file::File; -use anyhow::bail; use unicode_normalization::UnicodeNormalization; +/// Read and write size for a streamed transfer. Large enough that the syscalls are not +/// what limits the transfer, small enough that ten of them in flight cost little. +const TRANSFER_CHUNK_BYTES: usize = 64 * 1024; + +/// Name a scratch file beside the destination for a download in progress. +/// +/// Beside it, so the move into place is a rename within one directory and cannot fail +/// half way. Unique, so two transfers heading for the same name cannot clear up after +/// each other. +fn partial_path(dest: &std::path::Path) -> std::path::PathBuf { + use std::sync::atomic::{AtomicU64, Ordering}; + static COUNTER: AtomicU64 = AtomicU64::new(0); + + let mut name = dest.file_name().unwrap_or_default().to_os_string(); + name.push(format!( + ".{}-{}.mdrspart", + std::process::id(), + COUNTER.fetch_add(1, Ordering::Relaxed) + )); + dest.with_file_name(name) +} + #[derive(serde::Deserialize)] struct FileListResponse { pub next: Option, @@ -21,7 +43,7 @@ impl MDRSConnection { ]; let resp = self.get_with_query("v3/files/", ¶ms).await?; if !resp.status().is_success() { - anyhow::bail!("List files failed: {}", resp.status()); + return Err(response_error("List files failed", resp).await); } let list: FileListResponse = resp.json().await?; let has_next = list.next.is_some(); @@ -50,7 +72,7 @@ impl MDRSConnection { let _permit = limiter.acquire().await?; let resp = self.get_with_query("v3/files/", ¶ms).await?; if !resp.status().is_success() { - anyhow::bail!("List files failed: {}", resp.status()); + return Err(response_error("List files failed", resp).await); } let list: FileListResponse = resp.json().await?; let has_next = list.next.is_some(); @@ -79,9 +101,17 @@ impl MDRSConnection { .to_string_lossy() .nfc() .collect(); + // The file is handed to the request as a stream rather than a buffer: a + // repository holds recordings far larger than the machine's memory, and ten of + // them may be in flight at once. The length travels with it so the request keeps + // a Content-Length and does not have to be sent chunked. let build_form = || async { - let file_bytes = tokio::fs::read(file_path).await?; - let part = multipart::Part::bytes(file_bytes).file_name(file_name.clone()); + let file = tokio::fs::File::open(file_path).await?; + let length = file.metadata().await?.len(); + let stream = tokio_util::io::ReaderStream::with_capacity(file, TRANSFER_CHUNK_BYTES); + let part = + multipart::Part::stream_with_length(reqwest::Body::wrap_stream(stream), length) + .file_name(file_name.clone()); Ok::<_, anyhow::Error>( multipart::Form::new() .text("folder_id", folder_id.to_string()) @@ -95,7 +125,7 @@ impl MDRSConnection { .await?; if resp.status() != reqwest::StatusCode::UNAUTHORIZED { if !resp.status().is_success() { - bail!("Upload failed: {}", resp.status()); + return Err(response_error("Upload failed", resp).await); } return Ok(()); } @@ -105,7 +135,7 @@ impl MDRSConnection { // for the refusal would just cost a second full transfer to be told the same. let retry_conn = self.connection_with_fresh_token_for_retry().await?; if retry_conn.token == sent_with { - bail!("Upload failed: {}", resp.status()); + return Err(response_error("Upload failed", resp).await); } let resp = retry_conn .post_multipart("v3/files/", build_form().await?) @@ -115,7 +145,7 @@ impl MDRSConnection { bail!(crate::connection::SERVER_BUSY_MESSAGE); } if !resp.status().is_success() { - bail!("Upload failed: {}", resp.status()); + return Err(response_error("Upload failed", resp).await); } Ok(()) } @@ -127,14 +157,310 @@ impl MDRSConnection { dest: &str, limiter: &ApiRequestLimiter, ) -> Result<(), anyhow::Error> { + use futures::StreamExt; + use tokio::io::AsyncWriteExt; + let _permit = limiter.acquire().await?; + let dest_path = std::path::Path::new(dest); + // Checked before anything is fetched. The finished file is moved into place, and + // a rename would replace a destination the mode says is protected, which is not + // what this client or the Python one has ever done. + if dest_path.exists() { + tokio::fs::OpenOptions::new() + .write(true) + .open(dest_path) + .await + .map_err(|e| anyhow::anyhow!("Cannot write `{}`: {}", dest, e))?; + } let resp = self.get_url(url).await?; if !resp.status().is_success() { - bail!("Download failed: {}", resp.status()); + return Err(response_error("Download failed", resp).await); + } + // Written as it arrives, into a scratch file that is moved into place only once + // the whole body has landed. Holding the body in memory first would cost the size + // of the file, times however many transfers are running; writing straight to the + // destination would destroy whatever is already there the moment the transfer + // starts, and leave a truncated file behind if it does not finish. + let part_path = partial_path(dest_path); + let result = async { + let mut file = tokio::io::BufWriter::with_capacity( + TRANSFER_CHUNK_BYTES, + tokio::fs::File::create(&part_path).await?, + ); + let mut stream = resp.bytes_stream(); + while let Some(chunk) = stream.next().await { + file.write_all(&chunk?).await?; + } + file.flush().await?; + Ok::<(), anyhow::Error>(()) + } + .await; + match result { + Ok(()) => Ok(tokio::fs::rename(&part_path, dest_path).await?), + Err(err) => { + // Only the scratch file goes. Anything already at the destination was not + // written by this transfer and is not this transfer's to remove. + let _ = tokio::fs::remove_file(&part_path).await; + Err(err) + } } - let bytes = resp.bytes().await?; - drop(_permit); - tokio::fs::write(dest, &bytes).await?; - Ok(()) + } +} + +#[cfg(test)] +mod transfer_tests { + use super::*; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio::net::TcpListener; + + /// Read one HTTP request in full, using its Content-Length to know where it ends. + async fn read_request(stream: &mut tokio::net::TcpStream) -> (String, Vec) { + let mut buf = Vec::new(); + let mut chunk = [0u8; 8192]; + let header_end = loop { + let n = stream.read(&mut chunk).await.unwrap(); + buf.extend_from_slice(&chunk[..n]); + if let Some(pos) = buf.windows(4).position(|w| w == b"\r\n\r\n") { + break pos + 4; + } + assert!(n > 0, "connection closed before the headers ended"); + }; + let head = String::from_utf8_lossy(&buf[..header_end]).to_string(); + let length: usize = head + .lines() + .find_map(|l| { + l.to_ascii_lowercase() + .strip_prefix("content-length:") + .map(|v| v.trim().to_string()) + }) + .expect("request must carry a Content-Length") + .parse() + .unwrap(); + while buf.len() < header_end + length { + let n = stream.read(&mut chunk).await.unwrap(); + assert!(n > 0, "connection closed before the body ended"); + buf.extend_from_slice(&chunk[..n]); + } + (head, buf[header_end..].to_vec()) + } + + /// The file goes out as a stream, and the request still declares its length, so the + /// server is not asked to accept a chunked upload. + #[tokio::test] + async fn an_upload_streams_the_file_with_a_declared_length() { + let payload: Vec = (0..300_000u32).map(|i| (i % 251) as u8).collect(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("recording.dat"); + std::fs::write(&path, &payload).unwrap(); + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let expected = payload.clone(); + let server = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.unwrap(); + let (head, body) = read_request(&mut stream).await; + stream + .write_all(b"HTTP/1.1 200 OK\r\ncontent-length: 15\r\nconnection: close\r\n\r\n{\"id\":\"new\"}") + .await + .unwrap(); + (head, body, expected) + }); + + let conn = MDRSConnection::new(&format!("http://{addr}")); + let limiter = ApiRequestLimiter::new(1); + conn.upload_file_limited("folder-1", path.to_str().unwrap(), &limiter) + .await + .unwrap(); + + let (head, body, expected) = server.await.unwrap(); + assert!( + !head + .to_ascii_lowercase() + .contains("transfer-encoding: chunked"), + "the upload must not be chunked:\n{head}" + ); + assert!( + body.windows(expected.len()).any(|w| w == expected), + "the file bytes must reach the server" + ); + assert!( + body.windows(8).any(|w| w == b"folder-1"), + "the folder id must reach the server" + ); + assert!( + String::from_utf8_lossy(&body).contains("recording.dat"), + "the file name must reach the server" + ); + } + + /// The body is written to disk as it arrives rather than being held whole. + #[tokio::test] + async fn a_download_writes_what_the_server_sent() { + let payload: Vec = (0..300_000u32).map(|i| (i % 241) as u8).collect(); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let served = payload.clone(); + let server = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.unwrap(); + let mut chunk = [0u8; 4096]; + let _ = stream.read(&mut chunk).await.unwrap(); + let head = format!( + "HTTP/1.1 200 OK\r\ncontent-length: {}\r\nconnection: close\r\n\r\n", + served.len() + ); + stream.write_all(head.as_bytes()).await.unwrap(); + stream.write_all(&served).await.unwrap(); + }); + + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("out.dat"); + let conn = MDRSConnection::new(&format!("http://{addr}")); + let limiter = ApiRequestLimiter::new(1); + conn.download_file_limited( + &format!("http://{addr}/v3/files/1/download/"), + dest.to_str().unwrap(), + &limiter, + ) + .await + .unwrap(); + server.await.unwrap(); + + assert_eq!(std::fs::read(&dest).unwrap(), payload); + } + + /// A transfer that dies part way must not take the copy already on disk with it. + #[tokio::test] + async fn an_interrupted_download_leaves_the_existing_file_intact() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.unwrap(); + let mut chunk = [0u8; 4096]; + let _ = stream.read(&mut chunk).await.unwrap(); + // Promises 300000 bytes, sends 1000, then hangs up. + stream + .write_all( + b"HTTP/1.1 200 OK\r\ncontent-length: 300000\r\nconnection: close\r\n\r\n", + ) + .await + .unwrap(); + stream.write_all(&vec![b'x'; 1000]).await.unwrap(); + stream.shutdown().await.unwrap(); + }); + + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("existing.dat"); + std::fs::write(&dest, b"the copy already here").unwrap(); + + let conn = MDRSConnection::new(&format!("http://{addr}")); + let limiter = ApiRequestLimiter::new(1); + let outcome = conn + .download_file_limited( + &format!("http://{addr}/v3/files/1/download/"), + dest.to_str().unwrap(), + &limiter, + ) + .await; + server.await.unwrap(); + + assert!(outcome.is_err(), "a truncated body is a failed download"); + assert_eq!(std::fs::read(&dest).unwrap(), b"the copy already here"); + let leftovers: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .map(|e| e.unwrap().file_name().to_string_lossy().to_string()) + .filter(|name| name != "existing.dat") + .collect(); + assert!( + leftovers.is_empty(), + "no scratch file may be left: {leftovers:?}" + ); + } + + /// A destination this call could not even open is not a file it may delete. + #[cfg(unix)] + #[tokio::test] + async fn a_download_that_cannot_be_written_leaves_the_existing_file_intact() { + use std::os::unix::fs::PermissionsExt; + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.unwrap(); + let mut chunk = [0u8; 4096]; + let _ = stream.read(&mut chunk).await.unwrap(); + stream + .write_all(b"HTTP/1.1 200 OK\r\ncontent-length: 2\r\nconnection: close\r\n\r\nhi") + .await + .unwrap(); + }); + + // A writable directory holding a file this process may not open for writing. + // That is what separates "could not write it" from "may delete it": unlink needs + // the directory, not the file. + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("precious.dat"); + std::fs::write(&dest, b"do not touch").unwrap(); + let mut perms = std::fs::metadata(&dest).unwrap().permissions(); + perms.set_mode(0o444); + std::fs::set_permissions(&dest, perms).unwrap(); + if std::fs::OpenOptions::new().write(true).open(&dest).is_ok() { + // Running with rights that ignore the mode, so there is nothing to observe. + return; + } + + let conn = MDRSConnection::new(&format!("http://{addr}")); + let limiter = ApiRequestLimiter::new(1); + let outcome = conn + .download_file_limited( + &format!("http://{addr}/v3/files/1/download/"), + dest.to_str().unwrap(), + &limiter, + ) + .await; + // Nothing was fetched, so the stub is still waiting to be connected to. + server.abort(); + + let message = outcome.unwrap_err().to_string(); + assert!( + message.starts_with("Cannot write "), + "the refusal must name the problem, got: {message}" + ); + assert_eq!(std::fs::read(&dest).unwrap(), b"do not touch"); + } + + /// A refused download leaves nothing behind that a later run could mistake for the file. + #[tokio::test] + async fn a_refused_download_leaves_no_file() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.unwrap(); + let mut chunk = [0u8; 4096]; + let _ = stream.read(&mut chunk).await.unwrap(); + let body = r#"{"type":"client_error","errors":[{"code":"permission_denied","detail":"Access is denied.","attr":null}]}"#; + let head = format!( + "HTTP/1.1 403 Forbidden\r\ncontent-type: application/json\r\ncontent-length: {}\r\nconnection: close\r\n\r\n{}", + body.len(), + body + ); + stream.write_all(head.as_bytes()).await.unwrap(); + }); + + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("out.dat"); + let conn = MDRSConnection::new(&format!("http://{addr}")); + let limiter = ApiRequestLimiter::new(1); + let err = conn + .download_file_limited( + &format!("http://{addr}/v3/files/1/download/"), + dest.to_str().unwrap(), + &limiter, + ) + .await + .unwrap_err(); + server.await.unwrap(); + + assert_eq!(err.to_string(), "Download failed: Access is denied."); + assert!(!dest.exists()); } } diff --git a/src/api/folders.rs b/src/api/folders.rs index 49a4d26..e4647e4 100644 --- a/src/api/folders.rs +++ b/src/api/folders.rs @@ -1,4 +1,5 @@ use crate::connection::{ApiRequestLimiter, MDRSConnection}; +use crate::error::response_error; pub use crate::models::folder::{FolderDetail, FolderSimple}; use anyhow::{anyhow, bail}; @@ -15,7 +16,7 @@ impl MDRSConnection { ]; let resp = self.get_with_query("v3/folders/", ¶ms).await?; if !resp.status().is_success() { - bail!("List folders failed: {}", resp.status()); + return Err(response_error("List folders failed", resp).await); } Ok(resp.json::>().await?) } @@ -34,7 +35,7 @@ impl MDRSConnection { let _permit = limiter.acquire().await?; let resp = self.get_with_query("v3/folders/", ¶ms).await?; if !resp.status().is_success() { - bail!("List folders failed: {}", resp.status()); + return Err(response_error("List folders failed", resp).await); } Ok(resp.json::>().await?) } @@ -43,7 +44,7 @@ impl MDRSConnection { pub async fn retrieve_folder(&self, id: &str) -> Result { let resp = self.get(&format!("v3/folders/{}/", id)).await?; if !resp.status().is_success() { - bail!("Retrieve folder failed: {}", resp.status()); + return Err(response_error("Retrieve folder failed", resp).await); } Ok(resp.json::().await?) } @@ -57,7 +58,7 @@ impl MDRSConnection { let _permit = limiter.acquire().await?; let resp = self.get(&format!("v3/folders/{}/", id)).await?; if !resp.status().is_success() { - bail!("Retrieve folder failed: {}", resp.status()); + return Err(response_error("Retrieve folder failed", resp).await); } Ok(resp.json::().await?) } @@ -94,7 +95,11 @@ impl MDRSConnection { let _permit = limiter.acquire().await?; let resp = self.post_json("v3/folders/", &body).await?; if !resp.status().is_success() { - bail!("Failed to create remote folder: {}", folder_name); + return Err(response_error( + &format!("Failed to create remote folder `{}`", folder_name), + resp, + ) + .await); } let json: serde_json::Value = resp.json().await?; json["id"] @@ -116,7 +121,7 @@ impl MDRSConnection { bail!("Password is incorrect."); } if !resp.status().is_success() { - bail!("Folder auth failed: {}", resp.status()); + return Err(response_error("Folder auth failed", resp).await); } Ok(()) } @@ -140,7 +145,7 @@ impl MDRSConnection { bail!("Password is incorrect."); } if !resp.status().is_success() { - bail!("Folder auth failed: {}", resp.status()); + return Err(response_error("Folder auth failed", resp).await); } Ok(()) } diff --git a/src/api/laboratories.rs b/src/api/laboratories.rs index ed3dc47..dce24aa 100644 --- a/src/api/laboratories.rs +++ b/src/api/laboratories.rs @@ -1,4 +1,5 @@ use crate::connection::MDRSConnection; +use crate::error::response_error; use crate::models::laboratory::{Laboratories, Laboratory}; use serde::Deserialize; @@ -12,7 +13,7 @@ impl MDRSConnection { pub async fn list_laboratories(&self) -> Result { let resp = self.get("v3/laboratories/").await?; if !resp.status().is_success() { - anyhow::bail!("List laboratories failed: {}", resp.status()); + return Err(response_error("List laboratories failed", resp).await); } // The API may return a paginated object or a direct array let text = resp.text().await?; diff --git a/src/api/users.rs b/src/api/users.rs index 321d242..db77a6a 100644 --- a/src/api/users.rs +++ b/src/api/users.rs @@ -1,6 +1,6 @@ use crate::connection::MDRSConnection; +use crate::error::response_error; use crate::models::user::User as ModelUser; -use anyhow::bail; use serde::Deserialize; use std::time::Duration; @@ -44,7 +44,7 @@ impl MDRSConnection { pub async fn get_current_user(&self) -> Result { let resp = self.get("v3/users/current/").await?; if !resp.status().is_success() { - bail!("Get current user failed: {}", resp.status()); + return Err(response_error("Get current user failed", resp).await); } let obj = resp.json::().await?; let laboratory_ids = obj.laboratories.into_iter().map(|l| l.id).collect(); @@ -74,7 +74,7 @@ impl MDRSConnection { .send() .await?; if !resp.status().is_success() { - bail!("Token refresh failed: {}", resp.status()); + return Err(response_error("Token refresh failed", resp).await); } let r: TokenRefreshResponse = resp.json().await?; Ok(RefreshedToken { diff --git a/src/cache/mod.rs b/src/cache/mod.rs index 69a2ba1..07b65e8 100644 --- a/src/cache/mod.rs +++ b/src/cache/mod.rs @@ -139,14 +139,32 @@ fn write_cache_file(cache_path: &Path, cache: &Cache) -> Result<(), anyhow::Erro } fn parse_cache(remote: &str, data: &str) -> Result { - serde_json::from_str::(data).map_err(|e| { + let cache = serde_json::from_str::(data).map_err(|e| { anyhow!( "Cache for `{}` is invalid or outdated ({}). Run `mdrs login {}` to refresh it.", remote, e, remote ) - }) + })?; + // The digest is what says the file is still the one this client wrote. The Python + // client checks it on every read and the two share the file, so a cache one of them + // would refuse must not be honoured by the other. + let expected = compute_digest( + cache.user.as_ref(), + &cache.token.access, + &cache.token.refresh, + &cache.laboratories, + ); + if cache.digest != expected { + bail!( + "Cache for `{}` has been altered or was written by an incompatible version. \ + Run `mdrs login {}` to refresh it.", + remote, + remote + ); + } + Ok(cache) } fn load_cache_from_dir(remote: &str, config_dir: &Path) -> Result { @@ -213,7 +231,11 @@ fn load_cache_if_present_from_dir( }; let cache = match parse_cache(remote, &data) { Ok(cache) => cache, - Err(_) => { + Err(e) => { + // Said out loud before the credentials go. The alternative is a command that + // quietly carries on anonymously and fails later for a reason that looks + // unrelated to the cache it just threw away. + eprintln!("{e}"); remove_cache_in_dir(remote, config_dir)?; return Ok(None); } @@ -290,6 +312,9 @@ pub async fn load_cache_with_token_refresh(remote: &str) -> Result Cache { - Cache { + let mut cache = Cache { user: Some(CacheUser { id: 1, username: username.to_string(), @@ -462,8 +490,15 @@ mod tests { full_name: "Laboratory".to_string(), }], }, - digest: format!("digest-{username}"), - } + digest: String::new(), + }; + cache.digest = compute_digest( + cache.user.as_ref(), + &cache.token.access, + &cache.token.refresh, + &cache.laboratories, + ); + cache } fn remote_name(prefix: &str, config_dir: &Path) -> String { @@ -575,6 +610,25 @@ mod tests { assert!(loaded.is_none()); } + /// A cache whose contents no longer match its digest is not a session to act on. + #[test] + fn load_cache_refuses_a_tampered_cache() { + let dir = tempdir().unwrap(); + let remote = remote_name("tampered", dir.path()); + let mut cache = sample_cache("alice"); + persist_cache_in_dir(&remote, dir.path(), &cache).unwrap(); + + // Someone edits the stored token but leaves the digest as it was. + cache.token.access = "someone-elses-access".to_string(); + let cache_path = cache_file_path_in(dir.path(), &remote); + fs::write(&cache_path, serde_json::to_vec_pretty(&cache).unwrap()).unwrap(); + invalidate_cached_entry(dir.path(), &remote); + + let err = load_cache_from_dir(&remote, dir.path()).unwrap_err(); + + assert!(err.to_string().contains("has been altered")); + } + #[test] fn load_cache_if_present_clears_invalid_cache() { let dir = tempdir().unwrap(); @@ -597,6 +651,13 @@ mod tests { let mut cache = sample_cache("alice"); cache.token.access = make_jwt_with_exp(0); cache.token.refresh = make_jwt_with_exp(0); + // Re-stamped, so this exercises the expired session rather than the digest check. + cache.digest = compute_digest( + cache.user.as_ref(), + &cache.token.access, + &cache.token.refresh, + &cache.laboratories, + ); persist_cache_in_dir(&remote, dir.path(), &cache).unwrap(); let loaded = load_cache_with_token_refresh_optional_from_dir(&remote, dir.path()) diff --git a/src/cli.rs b/src/cli.rs index da228c5..8abc4e1 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -41,7 +41,7 @@ pub enum Commands { skip_if_exists: bool, #[arg(short = 'p', long)] password: Option, - #[arg(long)] + #[arg(short = 'e', long)] exclude: Vec, remote_path: String, local_path: String, diff --git a/src/commands/chacl.rs b/src/commands/chacl.rs index a335a78..cf64f4d 100644 --- a/src/commands/chacl.rs +++ b/src/commands/chacl.rs @@ -1,5 +1,6 @@ use crate::cache::{create_authenticated_conn, load_cache_with_token_refresh}; -use crate::commands::shared::{find_folder, find_lab_in_cache, parse_remote_path}; +use crate::commands::shared::{find_folder, find_laboratory, parse_remote_path}; +use crate::error::response_error; use anyhow::bail; pub async fn chacl( @@ -23,7 +24,7 @@ pub async fn chacl( let (remote, labname, folder_path) = parse_remote_path(remote_path)?; let cache = load_cache_with_token_refresh(&remote).await?; let conn = create_authenticated_conn(&remote, &cache)?; - let lab = find_lab_in_cache(&cache, &labname)?; + let lab = find_laboratory(&conn, Some(&cache), &labname).await?; let folder = find_folder(&conn, lab.id, &folder_path, None).await?; let mut data = serde_json::Map::new(); @@ -45,7 +46,7 @@ pub async fn chacl( .await?; if !resp.status().is_success() { - bail!("ACL change failed: {}", resp.status()); + return Err(response_error("ACL change failed", resp).await); } Ok(()) } diff --git a/src/commands/config.rs b/src/commands/config.rs index 482f161..bcaa4db 100644 --- a/src/commands/config.rs +++ b/src/commands/config.rs @@ -13,18 +13,18 @@ fn sanitize_config_file(path: &PathBuf) -> Result<(), anyhow::Error> { } let text = fs::read_to_string(path)?; let lines: Vec<&str> = text.lines().collect(); - if let Some(first) = lines.first() { - if first.trim() == path.to_string_lossy() { - // remove the first line and write atomically - let new_text = if lines.len() > 1 { - lines[1..].join("\n") - } else { - String::new() - }; - let tmp = path.with_extension("tmp"); - fs::write(&tmp, new_text.as_bytes())?; - fs::rename(&tmp, path)?; - } + if let Some(first) = lines.first() + && first.trim() == path.to_string_lossy() + { + // remove the first line and write atomically + let new_text = if lines.len() > 1 { + lines[1..].join("\n") + } else { + String::new() + }; + let tmp = path.with_extension("tmp"); + fs::write(&tmp, new_text.as_bytes())?; + fs::rename(&tmp, path)?; } Ok(()) } diff --git a/src/commands/cp.rs b/src/commands/cp.rs index 5bbe43e..8d97f38 100644 --- a/src/commands/cp.rs +++ b/src/commands/cp.rs @@ -1,8 +1,8 @@ use crate::cache::{create_authenticated_conn, load_cache_with_token_refresh}; use crate::commands::shared::{ - find_file_by_name, find_folder, find_lab_in_cache, find_subfolder_by_name, nfc, - parse_remote_path, + find_file_by_name, find_folder, find_laboratory, find_subfolder_by_name, nfc, parse_remote_path, }; +use crate::error::response_error; use anyhow::bail; pub async fn cp(src_path: &str, dest_path: &str, recursive: bool) -> Result<(), anyhow::Error> { @@ -19,7 +19,7 @@ pub async fn cp(src_path: &str, dest_path: &str, recursive: bool) -> Result<(), let cache = load_cache_with_token_refresh(&s_remote).await?; let conn = create_authenticated_conn(&s_remote, &cache)?; - let lab = find_lab_in_cache(&cache, &s_lab)?; + let lab = find_laboratory(&conn, Some(&cache), &s_lab).await?; let lab_id = lab.id; // Split source path into parent directory and target name @@ -62,7 +62,7 @@ pub async fn cp(src_path: &str, dest_path: &str, recursive: bool) -> Result<(), .post_json(&format!("v3/files/{}/copy/", src_file_id), &body) .await?; if !resp.status().is_success() { - bail!("Copy failed: {}", resp.status()); + return Err(response_error("Copy failed", resp).await); } return Ok(()); } @@ -102,7 +102,7 @@ pub async fn cp(src_path: &str, dest_path: &str, recursive: bool) -> Result<(), .post_json(&format!("v3/folders/{}/copy/", src_folder_id), &body) .await?; if !resp.status().is_success() { - bail!("Copy failed: {}", resp.status()); + return Err(response_error("Copy failed", resp).await); } Ok(()) } diff --git a/src/commands/download.rs b/src/commands/download.rs index 382c78d..34e9e68 100644 --- a/src/commands/download.rs +++ b/src/commands/download.rs @@ -27,7 +27,7 @@ pub async fn download( } // Detect DOI path: "remote:10.xxxx/prefix.ID[/optional/sub/path]" - if is_doi(remote_path.splitn(2, ':').nth(1).unwrap_or("")) { + if is_doi(remote_path.split_once(':').map(|x| x.1).unwrap_or("")) { let (remote, doi, subpath) = parse_doi_remote_path(remote_path)?; let (raw_conn, _cache) = create_readonly_conn(&remote).await?; let (doi_folder, lab) = find_folder_by_doi(&raw_conn, &doi, password).await?; @@ -84,15 +84,13 @@ pub async fn download( continue; } let dest = local_real.join(&file.name); - if skip_if_exists { - if dest.exists() { - if let Ok(meta) = std::fs::metadata(&dest) { - if meta.len() == file.size { - println!("{}", dest.display()); - continue; - } - } - } + if skip_if_exists + && dest.exists() + && let Ok(meta) = std::fs::metadata(&dest) + && meta.len() == file.size + { + println!("{}", dest.display()); + continue; } let url = make_absolute_url(&conn, &file.download_url); conn.download_file_limited(&url, &dest.to_string_lossy(), &limiter) @@ -102,30 +100,23 @@ pub async fn download( return Ok(()); } - spawn_download_folder_task( - &mut folder_tasks, - conn.clone(), - limiter.clone(), - lab_name.clone(), - excludes.clone(), - folder.id.clone(), - top_local, - password_owned.clone(), - skip_if_exists, - ); - - drive_download_tasks( - &mut folder_tasks, - &mut download_tasks, - &mut errors, - conn.clone(), + let session = DownloadSession { + conn: conn.clone(), limiter, lab_name, excludes, - password_owned, + password: password_owned, skip_if_exists, - ) - .await; + }; + spawn_download_folder_task( + &mut folder_tasks, + session.clone(), + folder.id.clone(), + top_local, + ); + + drive_download_tasks(&mut folder_tasks, &mut download_tasks, &mut errors, session) + .await; if !errors.is_empty() { bail!(errors.join("\n")); @@ -146,15 +137,13 @@ pub async fn download( return Ok(()); } let dest = local_real.join(&file.name); - if skip_if_exists { - if dest.exists() { - if let Ok(meta) = std::fs::metadata(&dest) { - if meta.len() == file.size { - println!("{}", dest.display()); - return Ok(()); - } - } - } + if skip_if_exists + && dest.exists() + && let Ok(meta) = std::fs::metadata(&dest) + && meta.len() == file.size + { + println!("{}", dest.display()); + return Ok(()); } let url = make_absolute_url(&conn, &file.download_url); conn.download_file_limited(&url, &dest.to_string_lossy(), &limiter) @@ -177,30 +166,23 @@ pub async fn download( let lab_name = Arc::new(lab.name.clone()); let password_owned = password.map(str::to_string); - spawn_download_folder_task( - &mut folder_tasks, - conn.clone(), - limiter.clone(), - lab_name.clone(), - excludes.clone(), - sub.id.clone(), - top_local, - password_owned.clone(), - skip_if_exists, - ); - - drive_download_tasks( - &mut folder_tasks, - &mut download_tasks, - &mut errors, - conn.clone(), + let session = DownloadSession { + conn: conn.clone(), limiter, lab_name, excludes, - password_owned, + password: password_owned, skip_if_exists, - ) - .await; + }; + spawn_download_folder_task( + &mut folder_tasks, + session.clone(), + sub.id.clone(), + top_local, + ); + + drive_download_tasks(&mut folder_tasks, &mut download_tasks, &mut errors, session) + .await; if !errors.is_empty() { bail!(errors.join("\n")); @@ -242,15 +224,13 @@ pub async fn download( } // Python always places the downloaded file inside the local directory. let dest = local_real.join(&file.name); - if skip_if_exists { - if dest.exists() { - if let Ok(meta) = std::fs::metadata(&dest) { - if meta.len() == file.size { - println!("{}", dest.display()); - return Ok(()); - } - } - } + if skip_if_exists + && dest.exists() + && let Ok(meta) = std::fs::metadata(&dest) + && meta.len() == file.size + { + println!("{}", dest.display()); + return Ok(()); } let url = make_absolute_url(&conn, &file.download_url); conn.download_file_limited(&url, &dest.to_string_lossy(), &limiter) @@ -277,30 +257,22 @@ pub async fn download( let lab_name = Arc::new(lab.name.clone()); let password = password.map(str::to_string); - spawn_download_folder_task( - &mut folder_tasks, - conn.clone(), - limiter.clone(), - lab_name.clone(), - excludes.clone(), - sub.id.clone(), - top_local, - password.clone(), - skip_if_exists, - ); - - drive_download_tasks( - &mut folder_tasks, - &mut download_tasks, - &mut errors, - conn.clone(), + let session = DownloadSession { + conn: conn.clone(), limiter, lab_name, excludes, password, skip_if_exists, - ) - .await; + }; + spawn_download_folder_task( + &mut folder_tasks, + session.clone(), + sub.id.clone(), + top_local, + ); + + drive_download_tasks(&mut folder_tasks, &mut download_tasks, &mut errors, session).await; if !errors.is_empty() { bail!(errors.join("\n")); @@ -353,30 +325,26 @@ struct DownloadJob { dest_path: PathBuf, } -fn spawn_download_folder_task( - folder_tasks: &mut JoinSet>, +/// What every folder of one recursive download shares: the connection and its request +/// budget, the laboratory the paths are named against, what to leave out, the password +/// for locked folders, and whether files already on disk may be left alone. +#[derive(Clone)] +struct DownloadSession { conn: Arc, limiter: ApiRequestLimiter, lab_name: Arc, excludes: Arc>, - folder_id: String, - local_dir: PathBuf, password: Option, skip_if_exists: bool, +} + +fn spawn_download_folder_task( + folder_tasks: &mut JoinSet>, + session: DownloadSession, + folder_id: String, + local_dir: PathBuf, ) { - folder_tasks.spawn(async move { - process_download_folder( - conn, - limiter, - lab_name, - excludes, - folder_id, - local_dir, - password, - skip_if_exists, - ) - .await - }); + folder_tasks.spawn(async move { process_download_folder(session, folder_id, local_dir).await }); } fn spawn_download_task( @@ -396,9 +364,8 @@ fn spawn_download_task( Ok(()) } Err(err) => { - if job.dest_path.is_file() { - let _ = std::fs::remove_file(&job.dest_path); - } + // Nothing to clear up: a failed transfer writes only to its own scratch + // file, and removes that itself. Err(anyhow!( "Failed to download {}: {}", job.dest_path.display(), @@ -410,15 +377,18 @@ fn spawn_download_task( } async fn process_download_folder( - conn: Arc, - limiter: ApiRequestLimiter, - lab_name: Arc, - excludes: Arc>, + session: DownloadSession, folder_id: String, local_dir: PathBuf, - password: Option, - skip_if_exists: bool, ) -> Result { + let DownloadSession { + conn, + limiter, + lab_name, + excludes, + password, + skip_if_exists, + } = session; let folder = conn.retrieve_folder_limited(&folder_id, &limiter).await?; if is_excluded(excludes.as_slice(), lab_name.as_str(), &folder.path, None) { @@ -443,13 +413,13 @@ async fn process_download_folder( continue; } let dest_path = local_dir.join(&file.name); - if skip_if_exists && dest_path.exists() { - if let Ok(meta) = std::fs::metadata(&dest_path) { - if meta.len() == file.size { - println!("{}", dest_path.display()); - continue; - } - } + if skip_if_exists + && dest_path.exists() + && let Ok(meta) = std::fs::metadata(&dest_path) + && meta.len() == file.size + { + println!("{}", dest_path.display()); + continue; } download_jobs.push(DownloadJob { url: make_absolute_url(&conn, &file.download_url), @@ -500,12 +470,7 @@ async fn drive_download_tasks( folder_tasks: &mut JoinSet>, download_tasks: &mut JoinSet>, errors: &mut Vec, - conn: Arc, - limiter: ApiRequestLimiter, - lab_name: Arc, - excludes: Arc>, - password: Option, - skip_if_exists: bool, + session: DownloadSession, ) { loop { match (folder_tasks.is_empty(), download_tasks.is_empty()) { @@ -517,20 +482,15 @@ async fn drive_download_tasks( folder_tasks, download_tasks, errors, - conn.clone(), - limiter.clone(), - lab_name.clone(), - excludes.clone(), - password.clone(), - skip_if_exists, + &session, ); } } (true, false) => { - if let Some(result) = download_tasks.join_next().await { - if let Err(err) = flatten_join_result(result) { - errors.push(err.to_string()); - } + if let Some(result) = download_tasks.join_next().await + && let Err(err) = flatten_join_result(result) + { + errors.push(err.to_string()); } } (false, false) => { @@ -542,21 +502,15 @@ async fn drive_download_tasks( folder_tasks, download_tasks, errors, - conn.clone(), - limiter.clone(), - lab_name.clone(), - excludes.clone(), - password.clone(), - skip_if_exists, + &session, ); } } result = download_tasks.join_next() => { - if let Some(result) = result { - if let Err(err) = flatten_join_result(result) { + if let Some(result) = result + && let Err(err) = flatten_join_result(result) { errors.push(err.to_string()); } - } } } } @@ -569,30 +523,20 @@ fn handle_download_folder_result( folder_tasks: &mut JoinSet>, download_tasks: &mut JoinSet>, errors: &mut Vec, - conn: Arc, - limiter: ApiRequestLimiter, - lab_name: Arc, - excludes: Arc>, - password: Option, - skip_if_exists: bool, + session: &DownloadSession, ) { match flatten_join_result(result) { Ok(task_result) => { for (folder_id, local_dir) in task_result.child_folders { - spawn_download_folder_task( - folder_tasks, - conn.clone(), - limiter.clone(), - lab_name.clone(), - excludes.clone(), - folder_id, - local_dir, - password.clone(), - skip_if_exists, - ); + spawn_download_folder_task(folder_tasks, session.clone(), folder_id, local_dir); } for job in task_result.download_jobs { - spawn_download_task(download_tasks, conn.clone(), limiter.clone(), job); + spawn_download_task( + download_tasks, + session.conn.clone(), + session.limiter.clone(), + job, + ); } } Err(err) => errors.push(err.to_string()), diff --git a/src/commands/file_metadata.rs b/src/commands/file_metadata.rs index fe9b1a9..19de2ef 100644 --- a/src/commands/file_metadata.rs +++ b/src/commands/file_metadata.rs @@ -1,10 +1,11 @@ use crate::cache::create_readonly_conn; use crate::commands::shared::{find_file_by_name, resolve_remote_file}; +use crate::error::response_error; use anyhow::anyhow; pub async fn file_metadata(remote_path: &str, password: Option<&str>) -> Result<(), anyhow::Error> { let remote = remote_path - .splitn(2, ':') + .split(':') .next() .ok_or_else(|| anyhow!("Invalid remote path"))?; let (conn, cache) = create_readonly_conn(remote).await?; @@ -17,6 +18,9 @@ pub async fn file_metadata(remote_path: &str, password: Option<&str>) -> Result< .ok_or_else(|| anyhow!("File `{}` not found.", basename))?; let resp = conn.get(&format!("v3/files/{}/metadata/", file.id)).await?; + if !resp.status().is_success() { + return Err(response_error("Failed to get file metadata", resp).await); + } let json: serde_json::Value = resp.json().await?; println!("{}", serde_json::to_string(&json)?); Ok(()) diff --git a/src/commands/ls.rs b/src/commands/ls.rs index 5331e89..7e411a5 100644 --- a/src/commands/ls.rs +++ b/src/commands/ls.rs @@ -1,12 +1,19 @@ -use crate::cache::create_readonly_conn; +use crate::cache::{Cache, create_readonly_conn}; use crate::commands::shared::{fmt_datetime, resolve_remote_folder}; use crate::connection::MDRSConnection; use crate::models::file::File; use crate::models::folder::{FolderDetail, FolderSimple}; use serde_json::{Value, json}; +use std::collections::HashMap; use std::future::Future; use std::pin::Pin; +/// Laboratory names by id, for the JSON output's per-folder `laboratory` field. +type LaboratoryNames = HashMap; + +/// Shown for a folder whose laboratory the client cannot name, matching the Python client. +const UNKNOWN_LABORATORY: &str = "(invalid)"; + pub async fn ls( remote_path: &str, password: Option<&str>, @@ -15,7 +22,7 @@ pub async fn ls( is_quiet: bool, ) -> Result<(), anyhow::Error> { let remote = remote_path - .splitn(2, ':') + .split(':') .next() .ok_or_else(|| anyhow::anyhow!("Invalid remote path"))?; let (conn, cache) = create_readonly_conn(remote).await?; @@ -24,10 +31,11 @@ pub async fn ls( let labname = lab.name; if is_json { + let names = laboratory_names(&conn, cache.as_ref()).await; let output = if is_recursive { - build_folder_json_recursive(&conn, folder, &labname).await? + build_folder_json_recursive(&conn, folder, &names, password).await? } else { - build_folder_json_flat(&conn, &folder, &labname).await? + build_folder_json_flat(&conn, &folder, &names).await? }; println!("{}", serde_json::to_string(&output)?); } else if is_recursive { @@ -235,7 +243,7 @@ fn file_to_json(f: &File, base_url: &str) -> Value { }) } -fn folder_simple_to_json(sf: &FolderSimple) -> Value { +fn folder_simple_to_json(sf: &FolderSimple, names: &LaboratoryNames) -> Value { json!({ "id": sf.id, "pid": sf.pid, @@ -243,26 +251,58 @@ fn folder_simple_to_json(sf: &FolderSimple) -> Value { "access_level": sf.access_level_name(), "lock": sf.lock, "size": sf.size, - "laboratory_id": sf.laboratory_id, + "laboratory": laboratory_name(names, sf.laboratory_id), "description": sf.description, "created_at": sf.created_at, "updated_at": sf.updated_at, }) } +/// Name the laboratory a folder belongs to, so a listing does not hand out an id the +/// reader has no way to resolve. +fn laboratory_name(names: &LaboratoryNames, id: u32) -> String { + names + .get(&id) + .cloned() + .unwrap_or_else(|| UNKNOWN_LABORATORY.to_string()) +} + +/// Collect laboratory names, preferring what the login cache already holds so a listing +/// does not pay for a request it does not need. +async fn laboratory_names(conn: &MDRSConnection, cache: Option<&Cache>) -> LaboratoryNames { + if let Some(cache) = cache.filter(|c| !c.laboratories.items.is_empty()) { + return cache + .laboratories + .items + .iter() + .map(|lab| (lab.id, lab.name.clone())) + .collect(); + } + match conn.list_laboratories().await { + Ok(labs) => labs + .items + .into_iter() + .map(|lab| (lab.id, lab.name)) + .collect(), + Err(_) => LaboratoryNames::new(), + } +} + /// Build JSON for the top-level folder without recursing into sub-folders. async fn build_folder_json_flat( conn: &MDRSConnection, folder: &FolderDetail, - labname: &str, + names: &LaboratoryNames, ) -> Result { let metadata = get_folder_metadata(conn, &folder.id).await?; - let files = conn.list_all_files(&folder.id).await?; + let mut files = conn.list_all_files(&folder.id).await?; + files.sort_by(|a, b| a.name.cmp(&b.name)); let files_json: Vec = files.iter().map(|f| file_to_json(f, &conn.url)).collect(); - let sub_folders_json: Vec = folder - .sub_folders + let mut sub_folders = folder.sub_folders.clone(); + sub_folders.sort_by(|a, b| a.name.cmp(&b.name)); + let sub_folders_json: Vec = sub_folders .iter() - .map(folder_simple_to_json) + .map(|sf| folder_simple_to_json(sf, names)) .collect(); Ok(json!({ @@ -272,7 +312,7 @@ async fn build_folder_json_flat( "size": folder.size, "access_level": folder.access_level_name(), "lock": folder.lock, - "laboratory": labname, + "laboratory": laboratory_name(names, folder.laboratory_id), "description": folder.description, "created_at": folder.created_at, "updated_at": folder.updated_at, @@ -286,17 +326,36 @@ async fn build_folder_json_flat( fn build_folder_json_recursive<'a>( conn: &'a MDRSConnection, folder: FolderDetail, - labname: &'a str, + names: &'a LaboratoryNames, + password: Option<&'a str>, ) -> Pin> + 'a>> { Box::pin(async move { let metadata = get_folder_metadata(conn, &folder.id).await?; - let files = conn.list_all_files(&folder.id).await?; + let mut files = conn.list_all_files(&folder.id).await?; + files.sort_by(|a, b| a.name.cmp(&b.name)); let files_json: Vec = files.iter().map(|f| file_to_json(f, &conn.url)).collect(); + let mut sub_folders = folder.sub_folders.clone(); + sub_folders.sort_by(|a, b| a.name.cmp(&b.name)); let mut sub_folders_json = Vec::new(); - for sf in &folder.sub_folders { + for sf in &sub_folders { + // A folder the caller cannot unlock is left out rather than ending the whole + // listing: a tree usually holds a locked folder or two, and the rest of it is + // still what the user asked to see. Anything else is reported, because a + // listing that quietly drops a branch cannot be told from one where the + // branch is empty, and this output is read by programs. + if sf.lock { + match password { + None => continue, + Some(pw) => { + if conn.folder_auth(&sf.id, pw).await.is_err() { + continue; + } + } + } + } let sf_detail = conn.retrieve_folder(&sf.id).await?; - let sf_json = build_folder_json_recursive(conn, sf_detail, labname).await?; + let sf_json = build_folder_json_recursive(conn, sf_detail, names, password).await?; sub_folders_json.push(sf_json); } @@ -307,7 +366,7 @@ fn build_folder_json_recursive<'a>( "size": folder.size, "access_level": folder.access_level_name(), "lock": folder.lock, - "laboratory": labname, + "laboratory": laboratory_name(names, folder.laboratory_id), "description": folder.description, "created_at": folder.created_at, "updated_at": folder.updated_at, diff --git a/src/commands/metadata.rs b/src/commands/metadata.rs index b4191e5..be16053 100644 --- a/src/commands/metadata.rs +++ b/src/commands/metadata.rs @@ -1,9 +1,10 @@ use crate::cache::create_readonly_conn; use crate::commands::shared::resolve_remote_folder; +use crate::error::response_error; pub async fn metadata(remote_path: &str, password: Option<&str>) -> Result<(), anyhow::Error> { let remote = remote_path - .splitn(2, ':') + .split(':') .next() .ok_or_else(|| anyhow::anyhow!("Invalid remote path"))?; let (conn, cache) = create_readonly_conn(remote).await?; @@ -13,6 +14,9 @@ pub async fn metadata(remote_path: &str, password: Option<&str>) -> Result<(), a let resp = conn .get(&format!("v3/folders/{}/metadata/", folder.id)) .await?; + if !resp.status().is_success() { + return Err(response_error("Failed to get folder metadata", resp).await); + } let json: serde_json::Value = resp.json().await?; println!("{}", serde_json::to_string(&json)?); Ok(()) diff --git a/src/commands/mkdir.rs b/src/commands/mkdir.rs index 83c0a8d..c7b4316 100644 --- a/src/commands/mkdir.rs +++ b/src/commands/mkdir.rs @@ -1,8 +1,8 @@ use crate::cache::{create_authenticated_conn, load_cache_with_token_refresh}; use crate::commands::shared::{ - find_file_by_name, find_folder, find_lab_in_cache, find_subfolder_by_name, nfc, - parse_remote_path, + find_file_by_name, find_folder, find_laboratory, find_subfolder_by_name, nfc, parse_remote_path, }; +use crate::error::response_error; use anyhow::{anyhow, bail}; pub async fn mkdir(remote_path: &str) -> Result<(), anyhow::Error> { @@ -25,7 +25,7 @@ pub async fn mkdir(remote_path: &str) -> Result<(), anyhow::Error> { let cache = load_cache_with_token_refresh(&remote).await?; let conn = create_authenticated_conn(&remote, &cache)?; - let lab = find_lab_in_cache(&cache, &labname)?; + let lab = find_laboratory(&conn, Some(&cache), &labname).await?; let parent_folder = find_folder(&conn, lab.id, parent_path, None).await?; // Check for name conflict in sub-folders or files @@ -40,7 +40,7 @@ pub async fn mkdir(remote_path: &str) -> Result<(), anyhow::Error> { .create_folder(&parent_folder.id, &nfc(new_folder_name)) .await?; if !resp.status().is_success() { - bail!("Failed to create folder: {}", resp.status()); + return Err(response_error("Failed to create folder", resp).await); } Ok(()) } diff --git a/src/commands/mv.rs b/src/commands/mv.rs index 215b3dd..7bd7ef2 100644 --- a/src/commands/mv.rs +++ b/src/commands/mv.rs @@ -1,8 +1,8 @@ use crate::cache::{create_authenticated_conn, load_cache_with_token_refresh}; use crate::commands::shared::{ - find_file_by_name, find_folder, find_lab_in_cache, find_subfolder_by_name, nfc, - parse_remote_path, + find_file_by_name, find_folder, find_laboratory, find_subfolder_by_name, nfc, parse_remote_path, }; +use crate::error::response_error; use anyhow::bail; pub async fn mv(src_path: &str, dest_path: &str) -> Result<(), anyhow::Error> { @@ -19,7 +19,7 @@ pub async fn mv(src_path: &str, dest_path: &str) -> Result<(), anyhow::Error> { let cache = load_cache_with_token_refresh(&s_remote).await?; let conn = create_authenticated_conn(&s_remote, &cache)?; - let lab = find_lab_in_cache(&cache, &s_lab)?; + let lab = find_laboratory(&conn, Some(&cache), &s_lab).await?; let lab_id = lab.id; // Split source path into parent directory and target name @@ -62,7 +62,7 @@ pub async fn mv(src_path: &str, dest_path: &str) -> Result<(), anyhow::Error> { .post_json(&format!("v3/files/{}/move/", src_file_id), &body) .await?; if !resp.status().is_success() { - bail!("Move failed: {}", resp.status()); + return Err(response_error("Move failed", resp).await); } return Ok(()); } @@ -99,7 +99,7 @@ pub async fn mv(src_path: &str, dest_path: &str) -> Result<(), anyhow::Error> { .post_json(&format!("v3/folders/{}/move/", src_folder_id), &body) .await?; if !resp.status().is_success() { - bail!("Move failed: {}", resp.status()); + return Err(response_error("Move failed", resp).await); } Ok(()) } diff --git a/src/commands/rm.rs b/src/commands/rm.rs index 8c09338..af67f4d 100644 --- a/src/commands/rm.rs +++ b/src/commands/rm.rs @@ -1,7 +1,8 @@ use crate::cache::{create_authenticated_conn, load_cache_with_token_refresh}; use crate::commands::shared::{ - find_file_by_name, find_folder, find_lab_in_cache, find_subfolder_by_name, parse_remote_path, + find_file_by_name, find_folder, find_laboratory, find_subfolder_by_name, parse_remote_path, }; +use crate::error::response_error; use anyhow::{anyhow, bail}; pub async fn rm(remote_path: &str, recursive: bool) -> Result<(), anyhow::Error> { @@ -22,7 +23,7 @@ pub async fn rm(remote_path: &str, recursive: bool) -> Result<(), anyhow::Error> let cache = load_cache_with_token_refresh(&remote).await?; let conn = create_authenticated_conn(&remote, &cache)?; - let lab = find_lab_in_cache(&cache, &labname)?; + let lab = find_laboratory(&conn, Some(&cache), &labname).await?; let parent_folder = find_folder(&conn, lab.id, parent_path, None).await?; // Check if target is a file @@ -30,7 +31,7 @@ pub async fn rm(remote_path: &str, recursive: bool) -> Result<(), anyhow::Error> if let Some(file) = find_file_by_name(&files, target_name) { let resp = conn.delete(&format!("v3/files/{}/", file.id)).await?; if !resp.status().is_success() { - bail!("Failed to delete file: {}", resp.status()); + return Err(response_error("Failed to delete file", resp).await); } return Ok(()); } @@ -47,7 +48,7 @@ pub async fn rm(remote_path: &str, recursive: bool) -> Result<(), anyhow::Error> ) .await?; if !resp.status().is_success() { - bail!("Failed to delete folder: {}", resp.status()); + return Err(response_error("Failed to delete folder", resp).await); } return Ok(()); } diff --git a/src/commands/shared.rs b/src/commands/shared.rs index 59717f0..63f6698 100644 --- a/src/commands/shared.rs +++ b/src/commands/shared.rs @@ -158,6 +158,10 @@ pub async fn find_folder_by_doi( // --------------------------------------------------------------------------- /// Parse "remote:/labname/path/" into (remote, labname, folder_path). +/// +/// The path is put in one form before anything is done with it: repeated and trailing +/// separators go, `.` segments go, and a `..` is refused rather than sent on. The Python +/// client does the same, and both talk to the same server. pub fn parse_remote_path(remote_path: &str) -> Result<(String, String, String), anyhow::Error> { let parts: Vec<&str> = remote_path.splitn(2, ':').collect(); if parts.len() != 2 { @@ -168,12 +172,22 @@ pub fn parse_remote_path(remote_path: &str) -> Result<(String, String, String), if !rest.starts_with('/') { bail!("Path must be absolute (start with '/')"); } - let folder_parts: Vec<&str> = rest.trim_start_matches('/').splitn(2, '/').collect(); - let labname = folder_parts[0].to_string(); - let path = if folder_parts.len() > 1 && !folder_parts[1].is_empty() { - format!("/{}", folder_parts[1].trim_end_matches('/')) - } else { + let mut segments = Vec::new(); + for segment in rest.split('/') { + match segment { + "" | "." => continue, + ".." => bail!("Path traversal found."), + other => segments.push(other), + } + } + if segments.is_empty() { + bail!("Laboratory name is missing from `{}`.", remote_path); + } + let labname = segments.remove(0).to_string(); + let path = if segments.is_empty() { "/".to_string() + } else { + format!("/{}", segments.join("/")) }; Ok((remote, labname, path)) } @@ -202,15 +216,15 @@ pub async fn find_laboratory( cache: Option<&Cache>, labname: &str, ) -> Result { - if let Some(cache) = cache { - if let Ok(lab) = find_lab_in_cache(cache, labname) { - return Ok(Laboratory { - id: lab.id, - name: lab.name.clone(), - pi_name: lab.pi_name.clone(), - full_name: lab.full_name.clone(), - }); - } + if let Some(cache) = cache + && let Ok(lab) = find_lab_in_cache(cache, labname) + { + return Ok(Laboratory { + id: lab.id, + name: lab.name.clone(), + pi_name: lab.pi_name.clone(), + full_name: lab.full_name.clone(), + }); } conn.list_laboratories() @@ -338,7 +352,7 @@ pub fn find_subfolder_by_name<'a>( /// Format an ISO 8601 timestamp as "YYYY/MM/DD HH:MM:SS". pub fn fmt_datetime(iso: &str) -> String { let s = iso.trim(); - let s = if let Some(pos) = s[10..].find(|c: char| c == '+' || c == '-') { + let s = if let Some(pos) = s[10..].find(['+', '-']) { &s[..10 + pos] } else { s.trim_end_matches('Z') @@ -361,7 +375,7 @@ pub async fn resolve_remote_folder( remote_path: &str, password: Option<&str>, ) -> Result<(FolderDetail, Laboratory), anyhow::Error> { - let path_component = remote_path.splitn(2, ':').nth(1).unwrap_or(""); + let path_component = remote_path.split_once(':').map(|x| x.1).unwrap_or(""); if is_doi(path_component) { let (_, doi, subpath) = parse_doi_remote_path(remote_path)?; let (doi_folder, lab) = find_folder_by_doi(conn, &doi, password).await?; @@ -395,7 +409,7 @@ pub async fn resolve_remote_file( remote_path: &str, password: Option<&str>, ) -> Result<(FolderDetail, String), anyhow::Error> { - let path_component = remote_path.splitn(2, ':').nth(1).unwrap_or(""); + let path_component = remote_path.split_once(':').map(|x| x.1).unwrap_or(""); if is_doi(path_component) { let (_, doi, subpath) = parse_doi_remote_path(remote_path)?; let (doi_folder, lab) = find_folder_by_doi(conn, &doi, password).await?; @@ -473,6 +487,53 @@ mod tests { server.await.unwrap(); } + // ------------------------------------------------------------------ + // Remote path parsing + // ------------------------------------------------------------------ + + #[test] + fn parse_remote_path_splits_remote_lab_and_path() { + let (remote, lab, path) = parse_remote_path("neurodata:/mylab/a/b/").unwrap(); + assert_eq!( + (remote.as_str(), lab.as_str(), path.as_str()), + ("neurodata", "mylab", "/a/b") + ); + } + + #[test] + fn parse_remote_path_gives_the_lab_root_its_own_form() { + let (_, lab, path) = parse_remote_path("neurodata:/mylab").unwrap(); + assert_eq!((lab.as_str(), path.as_str()), ("mylab", "/")); + } + + /// Both clients talk to the same server, so a doubled separator or a `.` segment has + /// to reach it as the same path from either one. + #[test] + fn parse_remote_path_collapses_redundant_segments() { + let (_, lab, path) = parse_remote_path("neurodata://mylab//a/./b//").unwrap(); + assert_eq!((lab.as_str(), path.as_str()), ("mylab", "/a/b")); + } + + #[test] + fn parse_remote_path_refuses_a_parent_segment() { + for path in [ + "neurodata:/mylab/../other", + "neurodata:/mylab/a/..", + "neurodata:/../mylab", + ] { + let err = parse_remote_path(path).unwrap_err(); + assert!( + err.to_string().contains("Path traversal found."), + "{path} should be refused" + ); + } + } + + #[test] + fn parse_remote_path_refuses_a_path_without_a_laboratory() { + assert!(parse_remote_path("neurodata:/").is_err()); + } + // ------------------------------------------------------------------ // DOI helper unit tests // ------------------------------------------------------------------ diff --git a/src/commands/upload.rs b/src/commands/upload.rs index f7cd486..8a4a31b 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -1,6 +1,6 @@ use crate::cache::{create_authenticated_conn, load_cache_with_token_refresh}; use crate::commands::shared::{ - find_file_by_name, find_folder_limited, find_lab_in_cache, nfc, parse_remote_path, + find_file_by_name, find_folder_limited, find_laboratory, nfc, parse_remote_path, }; use crate::connection::{ApiRequestLimiter, MDRSConnection}; use crate::models::folder::FolderSimple; @@ -20,7 +20,7 @@ pub async fn upload( let cache = load_cache_with_token_refresh(&remote).await?; let conn = Arc::new(create_authenticated_conn(&remote, &cache)?); let limiter = ApiRequestLimiter::new(crate::settings::SETTINGS.concurrent); - let lab = find_lab_in_cache(&cache, &labname)?; + let lab = find_laboratory(&conn, Some(&cache), &labname).await?; let dest_folder = find_folder_limited(&conn, &limiter, lab.id, &r_path, None).await?; // Normalize local_path: resolve to an absolute canonical path so that @@ -35,13 +35,12 @@ pub async fn upload( let remote_files = conn .list_all_files_limited(&dest_folder.id, &limiter) .await?; - if skip_if_exists { - if let Some(rf) = find_file_by_name(&remote_files, &filename) { - if rf.size == std::fs::metadata(local)?.len() { - println!("{}{}", dest_folder.path, filename); - return Ok(()); - } - } + if skip_if_exists + && let Some(rf) = find_file_by_name(&remote_files, &filename) + && rf.size == std::fs::metadata(local)?.len() + { + println!("{}{}", dest_folder.path, filename); + return Ok(()); } conn.upload_file_limited(&dest_folder.id, &local.to_string_lossy(), &limiter) .await?; @@ -219,15 +218,13 @@ async fn process_upload_folder( let mut upload_jobs = Vec::new(); for file_path in files { let filename = file_path.file_name().unwrap().to_string_lossy().to_string(); - if skip_if_exists { - if let Some(rf) = find_file_by_name(&remote_files, &filename) { - if let Ok(meta) = std::fs::metadata(&file_path) { - if rf.size == meta.len() { - println!("{}{}", folder_detail.path, filename); - continue; - } - } - } + if skip_if_exists + && let Some(rf) = find_file_by_name(&remote_files, &filename) + && let Ok(meta) = std::fs::metadata(&file_path) + && rf.size == meta.len() + { + println!("{}{}", folder_detail.path, filename); + continue; } upload_jobs.push(UploadJob { folder_id: remote_id.clone(), @@ -267,10 +264,10 @@ async fn drive_upload_tasks( } } (true, false) => { - if let Some(result) = upload_tasks.join_next().await { - if let Err(err) = flatten_join_result(result) { - errors.push(err.to_string()); - } + if let Some(result) = upload_tasks.join_next().await + && let Err(err) = flatten_join_result(result) + { + errors.push(err.to_string()); } } (false, false) => { @@ -289,11 +286,10 @@ async fn drive_upload_tasks( } } result = upload_tasks.join_next() => { - if let Some(result) = result { - if let Err(err) = flatten_join_result(result) { + if let Some(result) = result + && let Err(err) = flatten_join_result(result) { errors.push(err.to_string()); } - } } } } diff --git a/src/commands/whoami.rs b/src/commands/whoami.rs index 5d0be21..cfe4935 100644 --- a/src/commands/whoami.rs +++ b/src/commands/whoami.rs @@ -1,9 +1,14 @@ +use crate::cache::create_readonly_conn; + +/// Ask the server who it thinks is calling. +/// +/// The cached name is only what was true at login: an account disabled or removed since +/// then would still answer from the cache, which is the one thing this command exists to +/// tell the user. A session the server no longer honours reads as anonymous. pub async fn whoami(remote: &str) -> Result<(), anyhow::Error> { - match crate::cache::load_cache(remote) { - Ok(cache) => match cache.user { - Some(user) => println!("{}", user.username), - None => println!("(Anonymous)"), - }, + let (conn, _cache) = create_readonly_conn(remote).await?; + match conn.get_current_user().await { + Ok(user) => println!("{}", user.username), Err(_) => println!("(Anonymous)"), } Ok(()) diff --git a/src/error.rs b/src/error.rs index d9ed377..246d59e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,3 +1,33 @@ +use anyhow::anyhow; +use serde::Deserialize; + +/// The error envelope the API answers with (drf-standardized-errors). +#[derive(Deserialize)] +struct ApiErrors { + errors: Vec, +} + +#[derive(Deserialize)] +struct ApiError { + detail: String, +} + +/// Turn a refused response into an error carrying what the server actually said. +/// +/// A status code on its own leaves the user guessing. The reason - a permission, a +/// quota, a name already taken - is in the body the API sends along with it, and that +/// is the part worth showing. +pub async fn response_error(context: &str, resp: reqwest::Response) -> anyhow::Error { + let status = resp.status(); + let body = resp.text().await.unwrap_or_default(); + match serde_json::from_str::(&body) { + Ok(parsed) if !parsed.errors.is_empty() => { + anyhow!("{}: {}", context, parsed.errors[0].detail) + } + _ => anyhow!("{}: {}", context, status), + } +} + /// Print the error message and exit with code 2. /// JSON deserialization errors produce a friendlier message matching Python's /// JSONDecodeError handling. @@ -23,3 +53,53 @@ fn is_json_error(e: &(dyn std::error::Error + 'static)) -> bool { } false } + +#[cfg(test)] +mod response_error_tests { + use super::*; + + async fn refused_with(body: &'static str, content_type: &'static str) -> String { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio::net::TcpListener; + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.unwrap(); + let mut buf = [0u8; 4096]; + let _ = stream.read(&mut buf).await.unwrap(); + let response = format!( + "HTTP/1.1 403 Forbidden\r\ncontent-type: {}\r\ncontent-length: {}\r\nconnection: close\r\n\r\n{}", + content_type, + body.len(), + body + ); + stream.write_all(response.as_bytes()).await.unwrap(); + }); + + let resp = reqwest::get(format!("http://{addr}/")).await.unwrap(); + let message = response_error("Upload failed", resp).await.to_string(); + server.await.unwrap(); + message + } + + /// The reason the server gave is what the user needs, not the number that carried it. + #[tokio::test] + async fn the_servers_own_message_is_reported() { + let message = refused_with( + r#"{"type":"client_error","errors":[{"code":"permission_denied","detail":"You do not have permission.","attr":null}]}"#, + "application/json", + ) + .await; + + assert_eq!(message, "Upload failed: You do not have permission."); + } + + /// A gateway that answers in HTML has nothing to quote, so the status still stands in. + #[tokio::test] + async fn the_status_stands_in_when_the_body_says_nothing() { + let message = refused_with("nope", "text/html").await; + + assert_eq!(message, "Upload failed: 403 Forbidden"); + } +} diff --git a/src/main.rs b/src/main.rs index 9943ecb..30a82d3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -201,7 +201,7 @@ fn run(cli: Cli) { } Commands::SelfUpdate { yes } => { if let Err(e) = build_rt().block_on(commands::selfupdate::selfupdate(yes)) { - handle_error(e.into()); + handle_error(e); } } } diff --git a/src/settings.rs b/src/settings.rs index 652aeaa..d8c1024 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -13,14 +13,11 @@ impl Settings { fn load() -> Self { let config_dirname = std::env::var("MDRS_CLIENT_CONFIG_DIRNAME") .ok() - .map(|s| { - if s.starts_with("~/") { - dirs::home_dir() - .unwrap_or_else(|| std::path::PathBuf::from(".")) - .join(&s[2..]) - } else { - std::path::PathBuf::from(&s) - } + .map(|s| match s.strip_prefix("~/") { + Some(rest) => dirs::home_dir() + .unwrap_or_else(|| std::path::PathBuf::from(".")) + .join(rest), + None => std::path::PathBuf::from(&s), }) .unwrap_or_else(|| { dirs::home_dir()