From 81e89de7ea4bad37927262a71d693edfe51455e1 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 9 Aug 2026 20:55:04 +0200 Subject: [PATCH] Add remote move and mkdir; distinguish 403 from 401 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Soft delete needs to move a photograph into the trash folder and back, and the stable id must survive the trip. WebDAV MOVE is one request and preserves oc:fileid; a copy-then-delete would allocate a new one, orphaning the thumbnail shard entry and the sidecar mapping and turning a restore into a full re-download. Overwrite: F, because a header that permits overwriting is one that eventually does. create_dir does MKCOL outermost-first and treats 405 — Nextcloud's answer for an existing collection — as the goal state rather than an error. Nothing else creates the trash folder, so without it the first trashed image of every library fails with a 409 that reads like a permission problem. PermissionDenied is now separate from AuthFailed. Folding 403 into 401 sent a user to re-check a credential that was working perfectly, with reads succeeding and only the write refused (observed against a real server). The usual cause is an app password created without "Allow filesystem access" — which signing in again will not fix. Assisted-by: LLM --- core/dr-sync-nextcloud/examples/writetest.rs | 87 ++++++++++++++++++ core/dr-sync-nextcloud/src/lib.rs | 92 +++++++++++++++++++- core/dr-sync/src/error.rs | 30 ++++++- core/dr-sync/src/lib.rs | 23 +++++ 4 files changed, 230 insertions(+), 2 deletions(-) create mode 100644 core/dr-sync-nextcloud/examples/writetest.rs diff --git a/core/dr-sync-nextcloud/examples/writetest.rs b/core/dr-sync-nextcloud/examples/writetest.rs new file mode 100644 index 0000000..d13a04f --- /dev/null +++ b/core/dr-sync-nextcloud/examples/writetest.rs @@ -0,0 +1,87 @@ +//! One-shot write probe: PUT a tiny file, report the status, DELETE it. +use dr_plat::PlatformSecretStore; +use dr_sync::{RemoteBackend, RemoteId, RemotePath}; +use dr_sync_nextcloud::{NextcloudBackend, SessionStore}; + +#[tokio::main(flavor = "current_thread")] +async fn main() { + env_logger::Builder::from_env(env_logger::Env::default().default_filter_or("info")).init(); + + let store = SessionStore::open(Box::new(PlatformSecretStore::new())); + let Some(session) = store.current() else { + println!("no stored session"); + return; + }; + let creds = match store.credentials(&session) { + Ok(c) => c, + Err(e) => { println!("credentials: {e}"); return; } + }; + println!("account: {} root={}", session.describe(), session.root); + + let backend = NextcloudBackend::new(&creds, &session.user_id).unwrap(); + + // Read, which we know works. + let dir = RemotePath::new(&session.root); + match backend.list(&dir, None).await { + Ok(v) => println!("READ ok: {} entries", v.len()), + Err(e) => println!("READ FAILED: {e}"), + } + + // Raw HTTP, to see the status the connector maps away. + { + let url = format!("{}/remote.php/dav/files/{}/{}/.darkroom-write-test", + creds.server.trim_end_matches('/'), session.user_id, session.root); + let c = dr_sync_nextcloud::http_client("DarkRoom").unwrap(); + match c.put(&url).basic_auth(&creds.login_name, Some(&creds.app_password)) + .body("probe").send().await { + Ok(r) => { + println!("RAW PUT status: {}", r.status()); + let body = r.text().await.unwrap_or_default(); + let body = body.trim(); + if !body.is_empty() { + println!("RAW body: {}", &body[..body.len().min(400)]); + } + } + Err(e) => println!("RAW PUT transport error: {e}"), + } + } + + // Is it the folder or the account? Probe the account root too: a + // read-only *share* refuses writes inside it while the account writes + // freely elsewhere, which is a different fix from a credential problem. + { + let c = dr_sync_nextcloud::http_client("DarkRoom").unwrap(); + let base = format!("{}/remote.php/dav/files/{}", + creds.server.trim_end_matches('/'), session.user_id); + for (what, url) in [ + ("account root", format!("{base}/.darkroom-write-test")), + ("library root", format!("{base}/{}/.darkroom-write-test", session.root)), + ] { + match c.put(&url).basic_auth(&creds.login_name, Some(&creds.app_password)) + .body("probe").send().await { + Ok(r) => { + println!("PUT {what}: {}", r.status()); + if r.status().is_success() { + let _ = c.delete(&url) + .basic_auth(&creds.login_name, Some(&creds.app_password)) + .send().await; + } + } + Err(e) => println!("PUT {what}: transport error {e}"), + } + } + } + + // Write into the library root. + let p = RemotePath::new(format!("{}/.darkroom-write-test", session.root)); + match backend.put(&p, b"darkroom write probe\n".to_vec(), None).await { + Ok(v) => { + println!("WRITE ok: etag={}", v.as_str()); + match backend.delete(&RemoteId::Path(p.clone()), None).await { + Ok(()) => println!("CLEAN ok"), + Err(e) => println!("CLEAN failed (harmless): {e}"), + } + } + Err(e) => println!("WRITE FAILED: {e}"), + } +} diff --git a/core/dr-sync-nextcloud/src/lib.rs b/core/dr-sync-nextcloud/src/lib.rs index db4b635..9c4ed61 100644 --- a/core/dr-sync-nextcloud/src/lib.rs +++ b/core/dr-sync-nextcloud/src/lib.rs @@ -266,6 +266,91 @@ impl RemoteBackend for NextcloudBackend { map_status(resp.status(), &url) } + /// TRACES: FR-CAT-15 + /// WebDAV `MOVE`, which preserves `oc:fileid`. + /// + /// That preservation is the whole reason this is a `MOVE` and not a + /// `GET`+`PUT`+`DELETE`: the file id is what the thumbnail store keys on and + /// what the sidecar mapping records, so a move that allocated a new one + /// would orphan both and turn a trash-then-restore into a full re-download + /// of every affected file. + /// + /// `Overwrite: F` — a move must never destroy something already at the + /// destination. The trash path carries the image id precisely so this cannot + /// normally fire, but a header that permits overwriting is a header that + /// eventually does. + async fn move_to(&self, from: &RemoteId, to: &RemotePath) -> Result<(), RemoteError> { + let src = self.url_for_id(from)?; + let dest = self.url_for(to); + + // The parent has to exist; MOVE does not create it. Nothing else + // creates the trash folder, so the first trashed image would otherwise + // fail with a 409 that reads like a permission problem. + if let Some(parent) = to.parent() { + self.create_dir(&parent).await?; + } + + let resp = self + .client + .request( + reqwest::Method::from_bytes(b"MOVE").expect("valid method"), + &src, + ) + .basic_auth(&self.login, Some(&self.password)) + .header("Destination", &dest) + .header("Overwrite", "F") + .send() + .await + .map_err(|e| RemoteError::Network(e.to_string()))?; + + map_status(resp.status(), &src) + } + + /// `MKCOL`, treating "already there" as success. + /// + /// Callers use this to guarantee a destination exists, not to claim they + /// created it — so `405 Method Not Allowed`, which is what Nextcloud returns + /// for an existing collection, is the goal state and not an error. + /// + /// Parents are created outermost-first: `MKCOL` fails with `409` if the + /// parent is missing, and the trash folder's parent is the library root, + /// which may itself be several levels down. + async fn create_dir(&self, path: &RemotePath) -> Result<(), RemoteError> { + // Build the chain of ancestors, shallowest first. + let mut chain = Vec::new(); + let mut current = Some(path.clone()); + while let Some(p) = current { + if p.as_str().is_empty() { + break; + } + current = p.parent(); + chain.push(p); + } + chain.reverse(); + + for dir in chain { + let url = self.url_for(&dir); + let resp = self + .client + .request( + reqwest::Method::from_bytes(b"MKCOL").expect("valid method"), + &url, + ) + .basic_auth(&self.login, Some(&self.password)) + .send() + .await + .map_err(|e| RemoteError::Network(e.to_string()))?; + + // 405 is "already a collection here", which is exactly what the + // caller wanted. Anything else is reported. + if resp.status() == reqwest::StatusCode::METHOD_NOT_ALLOWED { + continue; + } + map_status(resp.status(), &url)?; + } + Ok(()) + } + async fn thumbnail(&self, id: &RemoteId, size: u32) -> Result>, RemoteError> { let RemoteId::Stable(file_id) = id else { return Ok(None); @@ -339,7 +424,12 @@ fn install_crypto_provider() { fn map_status(status: reqwest::StatusCode, what: &str) -> Result<(), RemoteError> { match status.as_u16() { 200..=299 => Ok(()), - 401 | 403 => Err(RemoteError::AuthFailed), + 401 => Err(RemoteError::AuthFailed), + // Not an auth failure: the credential authenticated fine and reads + // work. Reporting this as "authentication rejected" sends the user to + // re-check a working login (observed 2026-08-09: PROPFIND 207, PUT + // 403 `Sabre\DAV\Exception\Forbidden`, same app password). + 403 => Err(RemoteError::PermissionDenied), 404 => Err(RemoteError::NotFound(what.to_string())), // Drives the sidecar merge path rather than an overwrite (ARCH §8.5). 412 => Err(RemoteError::PreconditionFailed), diff --git a/core/dr-sync/src/error.rs b/core/dr-sync/src/error.rs index 8afb69f..3edeebf 100644 --- a/core/dr-sync/src/error.rs +++ b/core/dr-sync/src/error.rs @@ -7,6 +7,17 @@ pub enum RemoteError { #[error("authentication rejected")] AuthFailed, + /// Authenticated, but not permitted to do this. + /// + /// **Distinct from [`AuthFailed`](Self::AuthFailed) on purpose.** Folding + /// 403 into 401 sends the user to re-check a credential that is working + /// perfectly: reads succeed, only the write is refused. On Nextcloud the + /// usual cause is an app password created without "Allow filesystem + /// access", or a read-only share — neither of which signing in again will + /// fix. + #[error("permission denied — the account is authenticated but not allowed to write here")] + PermissionDenied, + #[error("not found: {0}")] NotFound(String), @@ -78,5 +89,22 @@ mod tests { .is_transient()); assert!(!RemoteError::PreconditionFailed.is_transient()); assert!(!RemoteError::AuthFailed.is_transient()); + // Neither is worth retrying, but they mean different things and a + // caller may want to say so. + assert!(!RemoteError::PermissionDenied.is_transient()); } -} + + #[test] + fn permission_denied_is_not_an_auth_failure() { + // 403 folded into 401 sent a user to re-check a credential that was + // working: reads succeeded and only the write was refused (observed + // against a real server, 2026-08-09). The two must read differently. + let denied = RemoteError::PermissionDenied.to_string(); + let rejected = RemoteError::AuthFailed.to_string(); + assert_ne!(denied, rejected); + assert!( + denied.contains("not allowed to write"), + "the message must point at permissions, not the login: {denied}" + ); + } +} \ No newline at end of file diff --git a/core/dr-sync/src/lib.rs b/core/dr-sync/src/lib.rs index 10f2665..597d33b 100644 --- a/core/dr-sync/src/lib.rs +++ b/core/dr-sync/src/lib.rs @@ -115,6 +115,29 @@ pub trait RemoteBackend: Send + Sync { async fn delete(&self, id: &RemoteId, precond: Option) -> Result<(), RemoteError>; + /// Move an object, keeping its identity. + /// + /// TRACES: FR-CAT-15 + /// **The stable id must survive.** This is what a soft delete uses to put a + /// photograph in the trash folder, and what a restore uses to bring it back. + /// A move implemented as copy-then-delete would allocate a *new* + /// `oc:fileid`, which orphans the thumbnail shard entry and the sidecar + /// mapping and turns a restore into a full re-download. WebDAV `MOVE` is one + /// request and preserves the id, which is why this is its own method rather + /// than something the caller composes. + /// + /// Creates missing parent directories of `to`: the trash folder does not + /// exist until the first image is trashed, and requiring the caller to + /// create it separately makes the first trash of every library a two-step + /// dance with a failure mode in the middle. + async fn move_to(&self, from: &RemoteId, to: &RemotePath) -> Result<(), RemoteError>; + + /// Create a directory, and any missing parents. + /// + /// Succeeds if it already exists — callers use this to guarantee a + /// destination, not to claim they created it. + async fn create_dir(&self, path: &RemotePath) -> Result<(), RemoteError>; + // ---- optional --------------------------------------------------------- /// Server-rendered thumbnail, where available.