Merge pull request #589 from EdouardVanbelle/fix/caldav-carddav-error-mapping

src/interfaces/api/handlers/caldav_handler.rs

@@ -248,7 +248,7 @@ async fn handle_propfind(
             calendar_service
                 .list_my_calendars(user.id)
                 .await
-                .map_err(|e| AppError::internal_error(format!("Failed to list calendars: {}", e)))?
+                .map_err(AppError::from)?
         };
 
         let base_href = "/caldav/";
@@ -364,9 +364,7 @@ async fn handle_propfind(
                 let calendars = calendar_service
                     .list_my_calendars(user.id)
                     .await
-                    .map_err(|e| {
+                    .map_err(AppError::from)?;
-                        AppError::internal_error(format!("Failed to list calendars: {}", e))
-                    })?;
 
                 let base_href = &format!("/caldav/{}/", first_segment);
                 let mut response_body = Vec::new();
@@ -448,7 +446,7 @@ async fn handle_propfind(
             let event = calendar_service
                 .get_event_by_ical_uid(calendar_id, ical_uid, user.id)
                 .await
-                .map_err(|e| AppError::internal_error(format!("Failed to look up event: {}", e)))?
+                .map_err(AppError::from)?
                 .ok_or_else(|| AppError::not_found(format!("Event not found: {}", ical_uid)))?;
 
             let base_href = &format!("/caldav/{}/", calendar_id);
@@ -505,16 +503,12 @@ async fn handle_report(
                 calendar_service
                     .get_events_in_range(calendar_id, *start, *end, user.id)
                     .await
-                    .map_err(|e| {
+                    .map_err(AppError::from)?
-                        AppError::internal_error(format!("Failed to query events: {}", e))
-                    })?
             } else {
                 calendar_service
                     .list_events(calendar_id, None, None, user.id)
                     .await
-                    .map_err(|e| {
+                    .map_err(AppError::from)?
-                        AppError::internal_error(format!("Failed to list events: {}", e))
-                    })?
             }
         }
         CalDavReportType::CalendarMultiget { hrefs, .. } => {
@@ -529,12 +523,12 @@ async fn handle_report(
             calendar_service
                 .get_events_by_ical_uids(calendar_id, &uids, user.id)
                 .await
-                .map_err(|e| AppError::internal_error(format!("Failed to fetch events: {}", e)))?
+                .map_err(AppError::from)?
         }
         CalDavReportType::SyncCollection { .. } => calendar_service
             .list_events(calendar_id, None, None, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to list events: {}", e)))?,
+            .map_err(AppError::from)?,
     };
 
     let base_href = &format!("/caldav/{}/", calendar_id);
@@ -693,12 +687,12 @@ async fn handle_get(
         let events = calendar_service
             .list_events(calendar_id, None, None, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to list events: {}", e)))?;
+            .map_err(AppError::from)?;
 
         let calendar = calendar_service
             .get_calendar(calendar_id, user.id)
             .await
-            .map_err(|e| AppError::not_found(format!("Calendar not found: {}", e)))?;
+            .map_err(AppError::from)?;
 
         let ical = generate_full_calendar_ical(&calendar.name, &events);
 
@@ -716,7 +710,7 @@ async fn handle_get(
         let event = calendar_service
             .get_event_by_ical_uid(calendar_id, ical_uid, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to look up event: {}", e)))?
+            .map_err(AppError::from)?
             .ok_or_else(|| AppError::not_found(format!("Event not found: {}", ical_uid)))?;
 
         let ical = generate_event_ical(&event);
@@ -809,7 +803,7 @@ async fn handle_delete(
         calendar_service
             .delete_calendar(calendar_id, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to delete calendar: {}", e)))?;
+            .map_err(AppError::from)?;
     } else {
         let event_file = parts[1];
         let ical_uid = event_file.trim_end_matches(".ics");
@@ -818,13 +812,13 @@ async fn handle_delete(
         let event = calendar_service
             .get_event_by_ical_uid(calendar_id, ical_uid, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to look up event: {}", e)))?
+            .map_err(AppError::from)?
             .ok_or_else(|| AppError::not_found(format!("Event not found: {}", ical_uid)))?;
 
         calendar_service
             .delete_event(&event.id, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to delete event: {}", e)))?;
+            .map_err(AppError::from)?;
     }
 
     Ok(Response::builder()
@@ -881,7 +875,7 @@ async fn handle_proppatch(
         calendar_service
             .update_calendar(calendar_id, update, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to update calendar: {}", e)))?;
+            .map_err(AppError::from)?;
     }
 
     let mut results = Vec::new();

src/interfaces/api/handlers/carddav_handler.rs

@@ -254,9 +254,7 @@ async fn handle_propfind(
             addressbook_service
                 .list_user_address_books(user.id)
                 .await
-                .map_err(|e| {
+                .map_err(AppError::from)?
-                    AppError::internal_error(format!("Failed to list address books: {}", e))
-                })?
         };
 
         let mut response_body = Vec::new();
@@ -307,9 +305,7 @@ async fn handle_propfind(
         let address_books = addressbook_service
             .list_user_address_books(user.id)
             .await
-            .map_err(|e| {
+            .map_err(AppError::from)?;
-                AppError::internal_error(format!("Failed to list address books: {}", e))
-            })?;
 
         let user_part = path.split('/').next().unwrap_or(path);
         let base_href = format!("/carddav/{}/", user_part);
@@ -373,7 +369,7 @@ async fn handle_propfind(
             let contact = contact_svc
                 .get_contact_by_uid(address_book_id, contact_uid, user.id)
                 .await
-                .map_err(|e| AppError::internal_error(format!("Failed to look up contact: {}", e)))?
+                .map_err(AppError::from)?
                 .ok_or_else(|| {
                     AppError::not_found(format!("Contact not found: {}", contact_uid))
                 })?;
@@ -432,7 +428,7 @@ async fn handle_report(
         CardDavReportType::AddressbookQuery { .. } => contact_svc
             .list_contacts(address_book_id, None, None, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?,
+            .map_err(AppError::from)?,
         CardDavReportType::AddressbookMultiget { hrefs, .. } => {
             // Indexed batch lookup (`uid = ANY(...)`) — a multiget for a
             // handful of contacts must not pay for listing the whole
@@ -445,12 +441,12 @@ async fn handle_report(
             contact_svc
                 .get_contacts_by_uids(address_book_id, &uids, user.id)
                 .await
-                .map_err(|e| AppError::internal_error(format!("Failed to fetch contacts: {}", e)))?
+                .map_err(AppError::from)?
         }
         CardDavReportType::SyncCollection { .. } => contact_svc
             .list_contacts(address_book_id, None, None, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?,
+            .map_err(AppError::from)?,
     };
 
     // Generate vCards
@@ -646,7 +642,7 @@ async fn handle_get(
         let contacts = contact_svc
             .list_contacts(address_book_id, None, None, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?;
+            .map_err(AppError::from)?;
 
         let mut vcf_data = String::new();
         for contact in &contacts {
@@ -666,7 +662,7 @@ async fn handle_get(
         let contact = contact_svc
             .get_contact_by_uid(address_book_id, contact_uid, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to look up contact: {}", e)))?
+            .map_err(AppError::from)?
             .ok_or_else(|| AppError::not_found(format!("Contact not found: {}", contact_uid)))?;
 
         let vcard = contact_to_vcard(&contact);
@@ -704,9 +700,7 @@ async fn handle_delete(
         addressbook_service
             .delete_address_book(address_book_id, user.id)
             .await
-            .map_err(|e| {
+            .map_err(AppError::from)?;
-                AppError::internal_error(format!("Failed to delete address book: {}", e))
-            })?;
     } else {
         // Delete contact — indexed lookup by vCard UID.
         let contact_file = parts[1];
@@ -715,13 +709,13 @@ async fn handle_delete(
         let contact = contact_svc
             .get_contact_by_uid(address_book_id, contact_uid, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to look up contact: {}", e)))?
+            .map_err(AppError::from)?
             .ok_or_else(|| AppError::not_found(format!("Contact not found: {}", contact_uid)))?;
 
         contact_svc
             .delete_contact(&contact.id, user.id)
             .await
-            .map_err(|e| AppError::internal_error(format!("Failed to delete contact: {}", e)))?;
+            .map_err(AppError::from)?;
     }
 
     Ok(Response::builder()
@@ -779,9 +773,7 @@ async fn handle_proppatch(
         addressbook_service
             .update_address_book(address_book_id, update)
             .await
-            .map_err(|e| {
+            .map_err(AppError::from)?;
-                AppError::internal_error(format!("Failed to update address book: {}", e))
-            })?;
     }
 
     let mut results = Vec::new();

tests/api/dav_error_mapping.hurl

@@ -205,3 +205,189 @@ HTTP *
 [Asserts]
 status >= 200
 status < 300
+
+
+# ─────────────────────────────────────────────────────────────
+# Cross-user AuthZ mapping (fix/caldav-carddav-error-mapping)
+# ─────────────────────────────────────────────────────────────
+# Regression pin for the second half of the CalDAV/CardDAV
+# error-mapping sweep: EVERY handler used to
+# `map_err(|e| AppError::internal_error(format!("Failed to ...: {}", e)))`,
+# turning a domain-layer `NotFound` (which is what AuthZ returns
+# for anti-enum on denied resources) into a 500 InternalError.
+#
+# Symptom: PROPPATCH / DELETE on a calendar the caller has no
+# permission on returned 500 with the calendar UUID leaked in
+# the body; on-call metrics tripped for benign perm denials.
+#
+# Fix: `.map_err(AppError::from)` — the kind-aware mapping via
+# `From<DomainError> for AppError` routes NotFound → 404.
+#
+# Provision a second user (Alice), have her hit admin's default
+# calendar + address book across the four verbs. Every response
+# MUST be a 4xx client error, NOT a 5xx server error. We don't
+# assert an exact 404 in every case because some paths naturally
+# return 403 or 401 depending on the auth stack; the invariant
+# the fix defends is "never 5xx for a perm denial".
+# ─────────────────────────────────────────────────────────────
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 8 — Provision + log in Alice (a distinct throwaway user).
+# HTTP * on the create because a re-run inside the same DB will
+# hit 409 Conflict; login is the actual precondition.
+# ─────────────────────────────────────────────────────────────
+POST {{base_url}}/api/admin/users
+Authorization: Bearer {{admin_token}}
+Content-Type: application/json
+{
+  "username": "dav-err-alice",
+  "password": "DavErrAlicePassword1!",
+  "email":    "dav-err-alice@example.com",
+  "role":     "user"
+}
+
+HTTP *
+[Captures]
+alice_id: jsonpath "$.id"
+
+
+POST {{base_url}}/api/auth/login
+Content-Type: application/json
+{
+  "username": "dav-err-alice",
+  "password": "DavErrAlicePassword1!"
+}
+
+HTTP 200
+[Captures]
+alice_token: jsonpath "$.access_token"
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 9 — Alice PROPPATCH on admin's default calendar.
+# Pre-fix: 500 InternalError with "Failed to update calendar:
+# Not Found: Calendar not found: <uuid>" in the body.
+# Post-fix: 4xx (typically 404 anti-enum from `authz.require`).
+# ─────────────────────────────────────────────────────────────
+PROPPATCH {{base_url}}/caldav/{{default_calendar_id}}/
+Authorization: Bearer {{alice_token}}
+Content-Type: application/xml
+```
+<?xml version="1.0" encoding="UTF-8"?>
+<D:propertyupdate xmlns:D="DAV:">
+  <D:set>
+    <D:prop>
+      <D:displayname>hijacked</D:displayname>
+    </D:prop>
+  </D:set>
+</D:propertyupdate>
+```
+
+HTTP *
+[Asserts]
+status >= 400
+status < 500
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 10 — Alice DELETE on admin's default calendar. Same
+# invariant — 4xx, never 5xx.
+# ─────────────────────────────────────────────────────────────
+DELETE {{base_url}}/caldav/{{default_calendar_id}}/
+Authorization: Bearer {{alice_token}}
+
+HTTP *
+[Asserts]
+status >= 400
+status < 500
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 11 — Alice DELETE on the well-formed event Step 4 created
+# in admin's calendar. Pre-fix: 500 on the lookup or delete step.
+# Post-fix: 4xx via NotFound anti-enum.
+# ─────────────────────────────────────────────────────────────
+DELETE {{base_url}}/caldav/{{default_calendar_id}}/dav-error-test-ok.ics
+Authorization: Bearer {{alice_token}}
+
+HTTP *
+[Asserts]
+status >= 400
+status < 500
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 12 — Alice PROPPATCH on admin's default address book.
+# Mirror of Step 9 on the CardDAV side. Pre-fix: 500. Post-fix: 4xx.
+# ─────────────────────────────────────────────────────────────
+PROPPATCH {{base_url}}/carddav/{{default_book_id}}/
+Authorization: Bearer {{alice_token}}
+Content-Type: application/xml
+```
+<?xml version="1.0" encoding="UTF-8"?>
+<D:propertyupdate xmlns:D="DAV:">
+  <D:set>
+    <D:prop>
+      <D:displayname>hijacked</D:displayname>
+    </D:prop>
+  </D:set>
+</D:propertyupdate>
+```
+
+HTTP *
+[Asserts]
+status >= 400
+status < 500
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 13 — Alice DELETE on admin's default address book.
+# ─────────────────────────────────────────────────────────────
+DELETE {{base_url}}/carddav/{{default_book_id}}/
+Authorization: Bearer {{alice_token}}
+
+HTTP *
+[Asserts]
+status >= 400
+status < 500
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 14 — Sanity: admin's own PROPPATCH still succeeds. Guards
+# against a fix that over-corrects and starts denying legitimate
+# writes. `HTTP *` because PROPPATCH multi-status can be 207 or
+# 200 depending on the property set; we assert the negative
+# invariant (no 4xx/5xx).
+# ─────────────────────────────────────────────────────────────
+PROPPATCH {{base_url}}/caldav/{{default_calendar_id}}/
+Authorization: Bearer {{admin_token}}
+Content-Type: application/xml
+```
+<?xml version="1.0" encoding="UTF-8"?>
+<D:propertyupdate xmlns:D="DAV:">
+  <D:set>
+    <D:prop>
+      <D:displayname>Personal (renamed by sanity step)</D:displayname>
+    </D:prop>
+  </D:set>
+</D:propertyupdate>
+```
+
+HTTP *
+[Asserts]
+status >= 200
+status < 400
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 15 — Cleanup: delete Alice so downstream test files don't
+# inherit an extra user (per feedback_hurl_teardown_shared_db —
+# state carries across the run.sh invocation).
+# ─────────────────────────────────────────────────────────────
+DELETE {{base_url}}/api/admin/users/{{alice_id}}
+Authorization: Bearer {{admin_token}}
+
+HTTP *
+[Asserts]
+status < 500