Merge pull request #426 from EdouardVanbelle/refactor/etag-centralize

refactor & normalize etag for Nextcloud + fix NFC string (important fix)

src/interfaces/nextcloud/basic_auth_middleware.rs

@@ -114,6 +114,15 @@ pub async fn basic_auth_middleware(
                 );
                 return Err(NextcloudAuthError::Unauthorized);
             }
+            // Populate the deferred `user_id` field on the request
+            // tracing span (declared in `middleware/trace_span.rs::ClientIpMakeSpan`).
+            // Mirrors what `interfaces/middleware/auth.rs` does for the
+            // JWT path so the two auth surfaces produce log lines with
+            // the same structured shape — without this, every NC
+            // request would appear in the logs with `user_id=-`,
+            // making it harder to correlate WebDAV / OCS activity to
+            // a specific principal.
+            tracing::Span::current().record("user_id", user_id.to_string());
             request.extensions_mut().insert(Arc::new(CurrentUser {
                 id: user_id,
                 username: uname,

src/interfaces/nextcloud/report_handler.rs

@@ -21,6 +21,7 @@ use crate::application::ports::file_ports::FileRetrievalUseCase;
 use crate::application::ports::folder_ports::FolderUseCase;
 use crate::application::ports::inbound::SearchUseCase;
 use crate::common::di::AppState;
+use crate::domain::entities::file::File;
 use crate::interfaces::errors::AppError;
 use crate::interfaces::middleware::auth::CurrentUser;
 use crate::interfaces::nextcloud::webdav_handler::{
@@ -250,6 +251,16 @@ async fn handle_search(
 
 /// Build a `FileDto` from a search file result.
 fn file_dto_from_search(fr: &crate::application::dtos::search_dto::SearchFileResultDto) -> FileDto {
+    // Route ETag through `File::compute_etag` so REPORT/SEARCH hits
+    // emit the same opaque token NC's sync client cached from the
+    // earlier PROPFIND walk — without this, NC's conditional-request
+    // logic on search results disagrees with its own cached state
+    // and triggers a spurious re-fetch.
+    let etag = if fr.blob_hash.is_empty() {
+        String::new()
+    } else {
+        File::compute_etag(&fr.blob_hash, fr.modified_at)
+    };
     FileDto {
         id: fr.id.clone(),
         name: fr.name.clone(),
@@ -267,7 +278,8 @@ fn file_dto_from_search(fr: &crate::application::dtos::search_dto::SearchFileRes
         size_formatted: format_file_size(fr.size),
         owner_id: None,
         sort_date: None,
-        etag: String::new(),
+        content_hash: fr.blob_hash.clone(),
+        etag,
     }
 }
 
@@ -276,6 +288,7 @@ fn folder_dto_from_search(
     sr: &crate::application::dtos::search_dto::SearchFolderResultDto,
 ) -> FolderDto {
     FolderDto {
+        etag: sr.id.clone(),
         id: sr.id.clone(),
         name: sr.name.clone(),
         path: sr.path.clone(),

src/interfaces/nextcloud/webdav_handler.rs

@@ -62,10 +62,32 @@ pub fn nc_to_internal_path(username: &str, subpath: &str) -> Result<String, AppE
     Ok(format!("{}/{}", home, subpath))
 }
 
+/// Build the Nextcloud DAV href for a **collection** (folder). Always
+/// terminates with `/` — RFC 4918 §5.2 requires collection URLs to end
+/// in a slash, and the Nextcloud desktop client strictly enforces this
+/// for the "own entry" href in PROPFIND multi-status responses: a
+/// PROPFIND on `/remote.php/dav/files/admin/ext/` whose first response
+/// `<d:href>` doesn't end in `/` aborts the parse with
+/// `Invalid href "<…>" expected starting with "<requested-url>"` and
+/// surfaces as `Network request error "Erreur inconnue" HTTP status
+/// 207` in the client log. Files use [`nc_href`] (no trailing slash).
+pub fn nc_collection_href(username: &str, subpath: &str) -> String {
+    let h = nc_href(username, subpath);
+    if h.ends_with('/') {
+        h
+    } else {
+        format!("{}/", h)
+    }
+}
+
 /// Build the Nextcloud DAV href for a resource.
 ///
 /// Each path segment is URL-encoded individually so filenames with spaces,
 /// `#`, `%`, or non-ASCII characters produce valid PROPFIND hrefs.
+///
+/// Returns NO trailing slash for non-empty subpaths. Callers rendering
+/// a **collection** must use [`nc_collection_href`] (or append `/`
+/// manually) to satisfy RFC 4918 §5.2 and the NC client's parser.
 pub fn nc_href(username: &str, subpath: &str) -> String {
     let subpath = subpath.trim_matches('/');
     let encoded_user = urlencoding::encode(username);
@@ -120,9 +142,18 @@ pub async fn handle_nc_webdav(
 // ──────────────────── OPTIONS ────────────────────
 
 fn handle_options() -> Result<Response<Body>, AppError> {
+    // Advertise WebDAV compliance classes 1 + 3 only.
+    // Class 2 (LOCK/UNLOCK) is intentionally omitted because the NC
+    // surface has no LOCK/UNLOCK dispatch arm — claiming class 2
+    // would invite clients (notably the NC desktop sync engine) to
+    // start sending LOCK requests we then 405. Class 3 covers the
+    // weak-resource-validators behaviour PROPFIND already implements.
+    // If LOCK is ever wired in here, restore "1, 2, 3" in the same
+    // commit as the LOCK arm — never split the advertisement from
+    // the implementation.
     Ok(Response::builder()
         .status(StatusCode::OK)
-        .header(HEADER_DAV, "1, 2, 3")
+        .header(HEADER_DAV, "1, 3")
         .header(
             header::ALLOW,
             "OPTIONS, GET, HEAD, PUT, DELETE, MKCOL, MOVE, PROPFIND, PROPPATCH, REPORT, SEARCH",
@@ -318,11 +349,18 @@ async fn handle_get(
         chrono::DateTime::<Utc>::from_timestamp(timestamp_to_i64(file.modified_at), 0)
             .unwrap_or_else(Utc::now);
 
+    // ETag comes from `FileDto::etag` (populated from `File::etag()`
+    // in the `From<File>` impl) — single source of truth, so GET,
+    // HEAD, PUT-response, MOVE, and PROPFIND all emit byte-identical
+    // values for the same file. NC's sync engine compares cached
+    // PROPFIND ETags against GET/HEAD responses; using `file.id` here
+    // (a UUID) while PROPFIND emitted the blob hash made NC see
+    // every file as "remotely changed" on first descent.
     Ok(Response::builder()
         .status(StatusCode::OK)
         .header(header::CONTENT_TYPE, file.mime_type.as_ref())
         .header(header::CONTENT_LENGTH, file.size)
-        .header(header::ETAG, format!("\"{}\"", file.id))
+        .header(header::ETAG, format!("\"{}\"", file.etag))
         .header(header::LAST_MODIFIED, modified_at.to_rfc2822())
         .body(Body::from_stream(std::pin::Pin::from(stream)))
         .unwrap())
@@ -370,11 +408,14 @@ async fn handle_head(
         chrono::DateTime::<Utc>::from_timestamp(timestamp_to_i64(file.modified_at), 0)
             .unwrap_or_else(Utc::now);
 
+    // ETag comes from `FileDto::etag` — see the same comment block on
+    // the GET handler. HEAD and GET must agree byte-for-byte; pulling
+    // both from the same DTO field guarantees that.
     Ok(Response::builder()
         .status(StatusCode::OK)
         .header(header::CONTENT_TYPE, file.mime_type.as_ref())
         .header(header::CONTENT_LENGTH, file.size)
-        .header(header::ETAG, format!("\"{}\"", file.id))
+        .header(header::ETAG, format!("\"{}\"", file.etag))
         .header(header::LAST_MODIFIED, modified_at.to_rfc2822())
         .body(Body::empty())
         .unwrap())
@@ -394,23 +435,40 @@ async fn handle_proppatch(
 
     let body_str = String::from_utf8_lossy(&body_bytes);
 
+    // Resolve the target resource once — needed for two things:
+    //  1. Applying the oc:favorite mutation when the PROPPATCH body
+    //     carries one (`item_type` distinguishes file vs folder rows
+    //     in the favorites table).
+    //  2. Picking the right `<d:href>` shape in the multi-status
+    //     response: collection (folder) hrefs MUST end in `/` per
+    //     RFC 4918 §5.2 — see `nc_collection_href` for the full
+    //     reasoning. Without this distinction the NC desktop client
+    //     parser aborted on PROPFIND; PROPPATCH would hit the same
+    //     wall the moment the user favourited a folder.
+    //
+    // When the resource is missing we tolerate it for the no-op
+    // PROPPATCH path (no favorite directive in the body) — matches
+    // the prior behaviour. A PROPPATCH that *does* try to set
+    // favorite on a missing resource still returns NotFound.
+    let internal_path = nc_to_internal_path(&user.username, subpath)?;
+    let file_service = &state.applications.file_retrieval_service;
+    let folder_service = &state.applications.folder_service;
+    let resource = if let Ok(file) = file_service.get_file_by_path(&internal_path).await {
+        Some((file.id, "file"))
+    } else if let Ok(folder) = folder_service.get_folder_by_path(&internal_path).await {
+        Some((folder.id, "folder"))
+    } else {
+        None
+    };
+    let is_collection = matches!(resource, Some((_, "folder")));
+
     // Parse oc:favorite value from PROPPATCH XML.
     let favorite_value = parse_proppatch_favorite(&body_str);
 
     if let Some(value) = favorite_value {
-        let internal_path = nc_to_internal_path(&user.username, subpath)?;
-        let file_service = &state.applications.file_retrieval_service;
-        let folder_service = &state.applications.folder_service;
-
-        // Determine item_id and item_type.
-        let (item_id, item_type) =
-            if let Ok(file) = file_service.get_file_by_path(&internal_path).await {
-                (file.id, "file")
-            } else if let Ok(folder) = folder_service.get_folder_by_path(&internal_path).await {
-                (folder.id, "folder")
-            } else {
-                return Err(AppError::not_found("Resource not found"));
-            };
+        let Some((item_id, item_type)) = resource else {
+            return Err(AppError::not_found("Resource not found"));
+        };
 
         if let Some(fav_svc) = state.favorites_service.as_ref() {
             if value == 1 {
@@ -431,8 +489,15 @@ async fn handle_proppatch(
         }
     }
 
-    // Return 207 Multi-Status with success response using quick_xml for safe escaping.
-    let href = nc_href(&user.username, subpath);
+    // Return 207 Multi-Status with success response using quick_xml
+    // for safe escaping. Collection vs file href chosen by resource
+    // type to satisfy the RFC 4918 §5.2 trailing-slash invariant —
+    // see the comment block at the top of this function.
+    let href = if is_collection {
+        nc_collection_href(&user.username, subpath)
+    } else {
+        nc_href(&user.username, subpath)
+    };
     let mut buf = Vec::new();
     {
         let mut xml = Writer::new(&mut buf);
@@ -804,9 +869,13 @@ async fn handle_move(
         let dest_internal = nc_to_internal_path(&user.username, &dest_subpath)?;
         let mut builder = Response::builder().status(StatusCode::CREATED);
         if let Ok(moved) = file_service.get_file_by_path(&dest_internal).await {
+            // Route through `FileDto::etag` so the MOVE response
+            // matches what a subsequent PROPFIND on the destination
+            // will return — `moved.id` (UUID) would differ from the
+            // blob hash and trigger NC's "remote changed" detection.
             builder = builder
-                .header(header::ETAG, format!("\"{}\"", moved.id))
-                .header("oc-etag", format!("\"{}\"", moved.id));
+                .header(header::ETAG, format!("\"{}\"", moved.etag))
+                .header("oc-etag", format!("\"{}\"", moved.etag));
         }
 
         return Ok(builder.body(Body::empty()).unwrap());
@@ -935,9 +1004,10 @@ async fn write_nc_multistatus<W: std::io::Write>(
     ms.push_attribute(("xmlns:ocs", "http://open-collaboration-services.org/ns"));
     xml.write_event(Event::Start(ms)).xml_err()?;
 
-    // Current folder entry.
+    // Current folder entry. Collection hrefs MUST end in `/` (RFC 4918
+    // §5.2 + strict NC-client enforcement — see `nc_collection_href`).
     if let Some(f) = folder {
-        let href = nc_href(username, subpath);
+        let href = nc_collection_href(username, subpath);
         let file_id = resolve_folder_id(file_id_svc, &f.id).await;
         let oc_id = file_id.map(|id| format_oc_id(id, file_id_svc));
         write_folder_response(
@@ -981,14 +1051,14 @@ async fn write_nc_multistatus<W: std::io::Write>(
             )?;
         }
 
-        // Subfolders.
+        // Subfolders — also collections, same trailing-slash rule.
         for sf in subfolders {
             let child_sub = if subpath.is_empty() {
                 sf.name.clone()
             } else {
                 format!("{}/{}", subpath.trim_end_matches('/'), sf.name)
             };
-            let href = format!("{}/", nc_href(username, &child_sub));
+            let href = nc_collection_href(username, &child_sub);
             let file_id = resolve_folder_id(file_id_svc, &sf.id).await;
             let oc_id = file_id.map(|id| format_oc_id(id, file_id_svc));
             write_folder_response(
@@ -1047,7 +1117,10 @@ pub fn write_folder_response<W: std::io::Write>(
             .unwrap_or_else(Utc::now);
 
     write_text_element(xml, "d:getlastmodified", &modified_at.to_rfc2822())?;
-    write_text_element(xml, "d:getetag", &format!("\"{}\"", folder.id))?;
+    // Route through `FolderDto::etag` (= `Folder::etag()`, currently
+    // the folder UUID — see the entity for the documented v1 formula
+    // and the follow-up plan to make it descendant-aware).
+    write_text_element(xml, "d:getetag", &format!("\"{}\"", folder.etag))?;
     write_text_element(xml, "d:getcontenttype", "httpd/unix-directory")?;
     write_text_element(xml, "d:getcontentlength", "0")?;
     write_text_element(xml, "d:creationdate", &created_at.to_rfc3339())?;
@@ -1277,6 +1350,41 @@ mod tests {
         assert!(href.contains("file%231.txt"));
     }
 
+    // ── nc_collection_href ──
+    // RFC 4918 §5.2 requires a collection URL to end in '/'. The NC
+    // desktop client at `networkjobs.cpp:234` aborts the PROPFIND
+    // parse with `Invalid href "<…>" expected starting with
+    // "<requested-url>"` if the own-entry href is missing the slash.
+    // These tests pin the helper's behaviour so the regression can't
+    // come back silently.
+
+    #[test]
+    fn test_collection_href_appends_slash_when_missing() {
+        assert_eq!(
+            nc_collection_href("alice", "ext"),
+            "/remote.php/dav/files/alice/ext/"
+        );
+    }
+
+    #[test]
+    fn test_collection_href_idempotent_at_root() {
+        // Root subpath already ends in '/' — don't double-append.
+        assert_eq!(
+            nc_collection_href("alice", ""),
+            "/remote.php/dav/files/alice/"
+        );
+    }
+
+    #[test]
+    fn test_collection_href_preserves_encoding() {
+        // Wrapping must not re-encode or double-encode already-encoded
+        // segments.
+        assert_eq!(
+            nc_collection_href("alice", "My Photos/2024"),
+            "/remote.php/dav/files/alice/My%20Photos/2024/"
+        );
+    }
+
     // ── extract_nc_subpath_from_dest ──
 
     #[test]