Merge pull request #437 from EdouardVanbelle/fix/nextcloud-propfind-on-chunk-upload

src/infrastructure/services/nextcloud_chunked_upload_service.rs

@@ -139,6 +139,92 @@ impl NextcloudChunkedUploadService {
             .map(|p| p.exists())
             .unwrap_or(false)
     }
+
+    /// Enumerate the chunks already stored in a session, plus the
+    /// session directory's own mtime. Used by the PROPFIND handler
+    /// to drive NextCloud's resume-upload flow — the Android client
+    /// (and several mobile clients) issue PROPFIND on the session
+    /// URL to discover which chunks are already uploaded, then only
+    /// PUT the missing ones.
+    ///
+    /// Returns `None` when the session directory doesn't exist
+    /// (handler maps to 404). The `.file` and `.assembled` markers
+    /// are filtered out — they're internal bookkeeping, not real
+    /// chunks the client uploaded.
+    pub async fn list_chunks(&self, user: &str, upload_id: &str) -> Result<Option<SessionListing>> {
+        let session_dir = self.safe_session_dir(user, upload_id)?;
+        if !session_dir.exists() {
+            return Ok(None);
+        }
+
+        let session_meta = fs::metadata(&session_dir)
+            .await
+            .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))?;
+        let session_mtime = session_meta
+            .modified()
+            .ok()
+            .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok())
+            .map(|d| d.as_secs())
+            .unwrap_or(0);
+
+        let mut chunks: Vec<ChunkInfo> = Vec::new();
+        let mut dir = fs::read_dir(&session_dir)
+            .await
+            .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))?;
+        while let Some(entry) = dir
+            .next_entry()
+            .await
+            .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))?
+        {
+            let name = entry.file_name().to_string_lossy().to_string();
+            // Filter internal markers — `.file` is the NC-protocol
+            // assembly trigger target (it never reaches the disk
+            // because MOVE redirects it), `.assembled` is our own
+            // staging file from `assemble()`. Surfacing either to
+            // the client would confuse its chunk-count check.
+            if name == ".file" || name == ".assembled" {
+                continue;
+            }
+            let meta = entry
+                .metadata()
+                .await
+                .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))?;
+            let size = meta.len();
+            let mtime = meta
+                .modified()
+                .ok()
+                .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok())
+                .map(|d| d.as_secs())
+                .unwrap_or(0);
+            chunks.push(ChunkInfo { name, size, mtime });
+        }
+        // Sort by chunk name so PROPFIND output is deterministic
+        // (clients don't strictly require this, but reproducible
+        // listings make debugging from logs much easier).
+        chunks.sort_by(|a, b| a.name.cmp(&b.name));
+
+        Ok(Some(SessionListing {
+            session_mtime,
+            chunks,
+        }))
+    }
+}
+
+/// One chunk file inside an upload session.
+#[derive(Debug, Clone)]
+pub struct ChunkInfo {
+    pub name: String,
+    pub size: u64,
+    pub mtime: u64,
+}
+
+/// What `list_chunks` returns: the session's own mtime (for the
+/// collection's `<d:getlastmodified>`) plus the list of stored
+/// chunks.
+#[derive(Debug, Clone)]
+pub struct SessionListing {
+    pub session_mtime: u64,
+    pub chunks: Vec<ChunkInfo>,
 }
 
 #[cfg(test)]

src/interfaces/nextcloud/uploads_handler.rs

@@ -14,10 +14,11 @@ use crate::interfaces::middleware::auth::{AuthUser, CurrentUser};
 /// Dispatch Nextcloud chunked upload WebDAV requests.
 ///
 /// Routes:
-///   MKCOL  /remote.php/dav/uploads/{user}/{upload_id}             → create session
+///   MKCOL    /remote.php/dav/uploads/{user}/{upload_id}             → create session
-///   PUT    /remote.php/dav/uploads/{user}/{upload_id}/{chunk}      → store chunk
+///   PUT      /remote.php/dav/uploads/{user}/{upload_id}/{chunk}     → store chunk
-///   MOVE   /remote.php/dav/uploads/{user}/{upload_id}/.file        → assemble
+///   MOVE     /remote.php/dav/uploads/{user}/{upload_id}/.file       → assemble
-///   DELETE /remote.php/dav/uploads/{user}/{upload_id}              → abort
+///   DELETE   /remote.php/dav/uploads/{user}/{upload_id}             → abort
+///   PROPFIND /remote.php/dav/uploads/{user}/{upload_id}             → list chunks (for resume)
 pub async fn handle_nc_uploads(
     state: Arc<AppState>,
     req: Request<Body>,
@@ -31,6 +32,7 @@ pub async fn handle_nc_uploads(
         "PUT" => handle_put_chunk(state, req, &user, &upload_id, &rest).await,
         "MOVE" => handle_assemble(state, req, &user, &upload_id).await,
         "DELETE" => handle_abort(state, &user, &upload_id).await,
+        "PROPFIND" => handle_propfind_session(state, &user, &upload_id).await,
         _ => Ok(Response::builder()
             .status(StatusCode::METHOD_NOT_ALLOWED)
             .body(Body::empty())
@@ -38,6 +40,107 @@ pub async fn handle_nc_uploads(
     }
 }
 
+/// PROPFIND on an upload session — used by the NextCloud Android
+/// client (and several mobile clients) to enumerate which chunks
+/// are already uploaded before resuming an interrupted transfer.
+/// Without this handler the client gets `405 METHOD_NOT_ALLOWED`
+/// and falls back to either failing the upload or starting from
+/// scratch — neither is acceptable on cellular / flaky links where
+/// resume is the whole point of chunked upload.
+///
+/// Response shape: 207 Multi-Status with one `<d:response>` for the
+/// session collection itself and one per chunk file. Properties
+/// returned are the minimum the NC client reads: `resourcetype`,
+/// `getcontentlength` (chunks only), and `getlastmodified` (so
+/// clients can detect stale partial uploads). Depth is ignored —
+/// we always return one level (the session + its direct chunks),
+/// which matches NC server behaviour.
+async fn handle_propfind_session(
+    state: Arc<AppState>,
+    user: &CurrentUser,
+    upload_id: &str,
+) -> Result<Response<Body>, AppError> {
+    let nc = state
+        .nextcloud
+        .as_ref()
+        .ok_or_else(|| AppError::internal_error("Nextcloud services unavailable"))?;
+
+    let listing = nc
+        .chunked_uploads
+        .list_chunks(&user.username, upload_id)
+        .await
+        .map_err(|e| AppError::internal_error(format!("Failed to list chunks: {}", e)))?
+        .ok_or_else(|| AppError::not_found("Upload session not found"))?;
+
+    let session_href = format!("/remote.php/dav/uploads/{}/{}/", user.username, upload_id);
+    let session_last_modified =
+        chrono::DateTime::<chrono::Utc>::from_timestamp(listing.session_mtime as i64, 0)
+            .unwrap_or_else(chrono::Utc::now)
+            .to_rfc2822();
+
+    let mut body = String::new();
+    body.push_str(r#"<?xml version="1.0" encoding="utf-8"?>"#);
+    body.push_str(r#"<d:multistatus xmlns:d="DAV:">"#);
+
+    // Session collection itself.
+    body.push_str("<d:response>");
+    body.push_str(&format!("<d:href>{}</d:href>", xml_escape(&session_href)));
+    body.push_str("<d:propstat><d:prop>");
+    body.push_str("<d:resourcetype><d:collection/></d:resourcetype>");
+    body.push_str(&format!(
+        "<d:getlastmodified>{}</d:getlastmodified>",
+        xml_escape(&session_last_modified)
+    ));
+    body.push_str("</d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat>");
+    body.push_str("</d:response>");
+
+    // One entry per chunk file.
+    for chunk in &listing.chunks {
+        let chunk_href = format!(
+            "/remote.php/dav/uploads/{}/{}/{}",
+            user.username, upload_id, chunk.name
+        );
+        let chunk_modified = chrono::DateTime::<chrono::Utc>::from_timestamp(chunk.mtime as i64, 0)
+            .unwrap_or_else(chrono::Utc::now)
+            .to_rfc2822();
+
+        body.push_str("<d:response>");
+        body.push_str(&format!("<d:href>{}</d:href>", xml_escape(&chunk_href)));
+        body.push_str("<d:propstat><d:prop>");
+        body.push_str("<d:resourcetype/>");
+        body.push_str(&format!(
+            "<d:getcontentlength>{}</d:getcontentlength>",
+            chunk.size
+        ));
+        body.push_str(&format!(
+            "<d:getlastmodified>{}</d:getlastmodified>",
+            xml_escape(&chunk_modified)
+        ));
+        body.push_str("</d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat>");
+        body.push_str("</d:response>");
+    }
+
+    body.push_str("</d:multistatus>");
+
+    Ok(Response::builder()
+        .status(StatusCode::MULTI_STATUS)
+        .header(header::CONTENT_TYPE, "application/xml; charset=utf-8")
+        .body(Body::from(body))
+        .unwrap())
+}
+
+/// Minimal XML escape — every value we inject above is either a
+/// well-formed RFC 2822 date, a number, or a path segment we
+/// control, but defense-in-depth keeps the response well-formed
+/// even if a chunk name ever contained an unexpected character.
+fn xml_escape(s: &str) -> String {
+    s.replace('&', "&amp;")
+        .replace('<', "&lt;")
+        .replace('>', "&gt;")
+        .replace('"', "&quot;")
+        .replace('\'', "&apos;")
+}
+
 /// MKCOL — create upload session directory.
 async fn handle_mkcol(
     state: Arc<AppState>,

tests/common/server.env

@@ -15,6 +15,9 @@ OXICLOUD_ENABLE_MUSIC=true
 OXICLOUD_EXPOSE_SYSTEM_USERS=true
 OXICLOUD_WOPI_ENABLED=false
 OXICLOUD_OIDC_ENABLED=false
+
+OXICLOUD_NEXTCLOUD_ENABLED=true
+
 RUST_LOG="warn,audit=info"
 #RUST_LOG=debug
 #RUST_LOG=info

tests/webdav/test_nextcloud_chunked_upload_propfind.sh

@@ -0,0 +1,212 @@
+#!/usr/bin/env bash
+# =============================================================
+# OxiCloud – NextCloud chunked-upload PROPFIND (resume support)
+# =============================================================
+# Verifies the PROPFIND handler on /remote.php/dav/uploads/{user}/{upload_id}.
+#
+# Why this matters: the NextCloud Android client (and several other
+# mobile NC clients) issue PROPFIND on the upload-session URL before
+# resuming an interrupted chunked upload. Without it the client gets
+# 405 METHOD_NOT_ALLOWED and either restarts the whole upload from
+# scratch or fails outright. This was a real bug reported against the
+# OxiCloud NC gateway by an admin running NC Android 3.31.1.
+#
+# Sequence:
+#   1. Login → JWT, then mint an app password (NC surface requires
+#      Basic Auth with an app password, not the user's login password).
+#   2. MKCOL the session → 201.
+#   3. PROPFIND empty session → 207 Multi-Status with exactly ONE
+#      <d:response> entry (the session collection itself).
+#   4. PUT two chunks (00000001, 00000002).
+#   5. PROPFIND again → 207 with THREE entries (collection + 2 chunks).
+#      Asserts the chunk byte counts come back correctly so the
+#      Android client's resume logic can compare against its expected
+#      chunk sizes.
+#   6. Security checks:
+#        a. PROPFIND with mismatched URL user → 403.
+#        b. PROPFIND on a session that doesn't exist → 404.
+#        c. PROPFIND with no auth → 401.
+#   7. DELETE the session → 204.
+#
+# Prerequisites:
+#   - Server running at base_url
+#   - OXICLOUD_ENABLE_AUTH=true (NC surface is auth-only)
+#   - jq, curl, xmllint in PATH
+#
+# Run (from repo root):
+#   bash tests/webdav/test_nextcloud_chunked_upload_propfind.sh
+# =============================================================
+
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+cd "$SCRIPT_DIR"
+
+source test.env
+source common.sh
+
+# ── helpers ──────────────────────────────────────────────────────────────────
+
+PASS=0
+FAIL=0
+
+pass() { PASS=$(( PASS + 1 )); echo "  PASS: $*"; }
+fail() { FAIL=$(( FAIL + 1 )); echo "  FAIL: $*" >&2; exit 1; }
+
+# All NC-surface curls go through this helper so the Basic-Auth
+# header (app password, not the user's login password) is applied
+# uniformly. The NC handler rejects login-password Basic Auth — only
+# app passwords pass the verify_basic_auth check.
+nc_curl() {
+    curl -s -u "$username:$APP_PASS" "$@"
+}
+
+# Mint an app password using the JWT we just got. The NC handlers
+# accept only app-password Basic Auth (rfc 7617), never the login
+# password, so this step is mandatory.
+#
+# Request DTO is `CreateAppPasswordRequestDto`: `label` is required,
+# `scopes` and `expires_in_days` are optional. Sending the wrong
+# field name makes axum's Json extractor return a plain-text error
+# body that subsequent jq parsing chokes on with "Invalid numeric
+# literal" — a confusing error mode worth pinning the field name
+# against.
+mint_app_password() {
+    local response
+    response=$(curl -s -X POST \
+        -H "Authorization: Bearer $TOKEN" \
+        -H "Content-Type: application/json" \
+        -d '{"label":"nc-chunked-upload-propfind-test"}' \
+        "$base_url/api/auth/app-passwords")
+    APP_PASS=$(jq -r '.password // empty' <<< "$response" 2>/dev/null || echo "")
+    [[ -n "$APP_PASS" ]] || fail "Could not mint app password (response: $response)"
+}
+
+# Count <d:response> children in a multistatus body. We don't use a
+# full XML parser because the response shape is fixed by our handler
+# and a plain grep is robust enough for the assertions we need.
+count_responses() {
+    grep -o '<d:response>' <<< "$1" | wc -l | tr -d ' '
+}
+
+echo
+echo "=== NextCloud chunked-upload PROPFIND (resume support) ==="
+echo
+
+# ── authenticate + mint app password ─────────────────────────────────────────
+
+oxicloud_login
+mint_app_password
+
+UPLOAD_ID="test-propfind-$(date +%s)-$$"
+NC_BASE="$base_url/remote.php/dav/uploads/$username/$UPLOAD_ID"
+
+# Idempotent pre-test cleanup — drop any stale session from a previous run.
+nc_curl -o /dev/null -X DELETE "$NC_BASE" > /dev/null 2>&1 || true
+
+# ── Step 1: MKCOL — create session ───────────────────────────────────────────
+
+echo "  step 1: MKCOL $NC_BASE"
+STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MKCOL "$NC_BASE")
+[[ "$STATUS" == "201" ]] || fail "MKCOL expected 201, got $STATUS"
+pass "Session created (MKCOL → 201)"
+
+# ── Step 2: PROPFIND on empty session — only the collection ──────────────────
+
+echo "  step 2: PROPFIND on empty session"
+BODY=$(nc_curl -X PROPFIND -H "Depth: 1" "$NC_BASE")
+[[ "$BODY" == *"<d:multistatus"* ]] \
+    || fail "PROPFIND empty session: not a multistatus body (got: $BODY)"
+N=$(count_responses "$BODY")
+[[ "$N" == "1" ]] \
+    || fail "PROPFIND empty session: expected 1 <d:response>, got $N (body: $BODY)"
+[[ "$BODY" == *"<d:collection/>"* ]] \
+    || fail "PROPFIND empty session: collection marker missing"
+pass "Empty-session PROPFIND returns 1 response (collection only)"
+
+# ── Step 3: PUT two chunks ───────────────────────────────────────────────────
+
+echo "  step 3: PUT chunks 00000001 and 00000002"
+CHUNK1_SIZE=$(printf "first chunk bytes" | wc -c | tr -d ' ')
+CHUNK2_SIZE=$(printf "second chunk has different size" | wc -c | tr -d ' ')
+
+STATUS=$(printf "first chunk bytes" | nc_curl -o /dev/null -w "%{http_code}" \
+    -X PUT -H "Content-Type: application/octet-stream" --data-binary @- \
+    "$NC_BASE/00000001")
+[[ "$STATUS" == "201" ]] || fail "PUT chunk 00000001 expected 201, got $STATUS"
+
+STATUS=$(printf "second chunk has different size" | nc_curl -o /dev/null -w "%{http_code}" \
+    -X PUT -H "Content-Type: application/octet-stream" --data-binary @- \
+    "$NC_BASE/00000002")
+[[ "$STATUS" == "201" ]] || fail "PUT chunk 00000002 expected 201, got $STATUS"
+pass "Both chunks uploaded ($CHUNK1_SIZE B + $CHUNK2_SIZE B)"
+
+# ── Step 4: PROPFIND populated session ───────────────────────────────────────
+
+echo "  step 4: PROPFIND populated session"
+BODY=$(nc_curl -X PROPFIND -H "Depth: 1" "$NC_BASE")
+N=$(count_responses "$BODY")
+[[ "$N" == "3" ]] \
+    || fail "PROPFIND populated session: expected 3 <d:response>, got $N (body: $BODY)"
+pass "Populated PROPFIND returns 3 responses (collection + 2 chunks)"
+
+# Chunk hrefs appear in the body.
+[[ "$BODY" == *"/$UPLOAD_ID/00000001"* ]] \
+    || fail "PROPFIND populated session: chunk 00000001 href missing"
+[[ "$BODY" == *"/$UPLOAD_ID/00000002"* ]] \
+    || fail "PROPFIND populated session: chunk 00000002 href missing"
+pass "Both chunk hrefs present in multistatus body"
+
+# Chunk content-lengths match what we uploaded — this is the field
+# the Android client reads to decide whether a stored chunk is
+# "complete" or "partial".
+[[ "$BODY" == *"<d:getcontentlength>$CHUNK1_SIZE</d:getcontentlength>"* ]] \
+    || fail "PROPFIND: chunk 00000001 size $CHUNK1_SIZE missing in body"
+[[ "$BODY" == *"<d:getcontentlength>$CHUNK2_SIZE</d:getcontentlength>"* ]] \
+    || fail "PROPFIND: chunk 00000002 size $CHUNK2_SIZE missing in body"
+pass "Chunk getcontentlength values match upload byte counts"
+
+# ── Step 5a: Security — URL user doesn't match auth user → 403 ────────────────
+
+echo "  step 5a: PROPFIND with mismatched URL user → 403"
+STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X PROPFIND \
+    "$base_url/remote.php/dav/uploads/someoneelse/$UPLOAD_ID")
+[[ "$STATUS" == "403" ]] \
+    || fail "Mismatched-user PROPFIND expected 403, got $STATUS"
+pass "Mismatched URL user rejected with 403"
+
+# ── Step 5b: Security — nonexistent session → 404 ────────────────────────────
+
+echo "  step 5b: PROPFIND on nonexistent session → 404"
+STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X PROPFIND \
+    "$base_url/remote.php/dav/uploads/$username/does-not-exist-$$-$(date +%s)")
+[[ "$STATUS" == "404" ]] \
+    || fail "Nonexistent-session PROPFIND expected 404, got $STATUS"
+pass "Nonexistent session returns 404"
+
+# ── Step 5c: Security — no auth → 401 ────────────────────────────────────────
+
+echo "  step 5c: PROPFIND with no auth → 401"
+STATUS=$(curl -s -o /dev/null -w "%{http_code}" -X PROPFIND "$NC_BASE")
+[[ "$STATUS" == "401" ]] \
+    || fail "Unauthenticated PROPFIND expected 401, got $STATUS"
+pass "Unauthenticated request rejected with 401"
+
+# ── Cleanup ──────────────────────────────────────────────────────────────────
+
+echo "  cleanup..."
+STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X DELETE "$NC_BASE")
+[[ "$STATUS" == "204" ]] || fail "DELETE session expected 204, got $STATUS"
+pass "Session deleted"
+
+# Confirm cleanup: PROPFIND now returns 404.
+STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X PROPFIND "$NC_BASE")
+[[ "$STATUS" == "404" ]] \
+    || fail "Post-delete PROPFIND expected 404, got $STATUS"
+pass "Post-delete PROPFIND confirms session gone"
+
+# ── summary ───────────────────────────────────────────────────────────────────
+
+echo
+echo "Results: $PASS passed, $FAIL failed."
+[[ "$FAIL" -eq 0 ]] && echo "All tests passed." || exit 1