Add remote move and mkdir; distinguish 403 from 401
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
This commit is contained in:
@@ -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}"),
|
||||
}
|
||||
}
|
||||
@@ -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<Option<Vec<u8>>, 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),
|
||||
|
||||
@@ -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}"
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -115,6 +115,29 @@ pub trait RemoteBackend: Send + Sync {
|
||||
async fn delete(&self, id: &RemoteId, precond: Option<Precondition>)
|
||||
-> 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.
|
||||
|
||||
Reference in New Issue
Block a user