Merge pull request #610 from EdouardVanbelle/security/grants2

src/application/services/auth_application_service.rs

@@ -1924,6 +1924,59 @@ impl AuthApplicationService {
         ))
     }
 
+    /// Username-keyed sibling of [`Self::get_user_profile`], routing every
+    /// lookup through the same visibility check as the user-profile REST
+    /// endpoint. Preserves the anti-enum shape end-to-end: whether the
+    /// username doesn't exist OR the caller has no visibility path, the
+    /// response is `NotFound`.
+    ///
+    /// AuthZ audit #11 (2026-07-12): NextCloud OCS user-provisioning
+    /// (`nextcloud/ocs_handler.rs::user_provisioning_response`) used to
+    /// resolve `userid` via bare `get_user_by_username`, gated only by a
+    /// bespoke `caller.role == "admin"` shortcut. Admins bypassed the
+    /// `expose_system_users` gate; non-admins got a `403 Insufficient
+    /// privileges` for any cross-user probe (leaking existence via the
+    /// differential vs a genuine 404); zero audit lines. This wrapper
+    /// closes all three.
+    ///
+    /// The username→id resolution happens here so the target isn't
+    /// leaked through the audit line as a plaintext username on failure:
+    /// the `target_username_not_found` event carries the string
+    /// (unavoidable — we resolved it, we log it), but every other
+    /// downstream event keys off `target_id` after resolution, matching
+    /// the id-based endpoint.
+    pub async fn get_user_profile_by_username_with_perms(
+        &self,
+        caller_id: Uuid,
+        username: &str,
+        expose_system_users: bool,
+        pool: &sqlx::PgPool,
+    ) -> Result<UserDto, DomainError> {
+        let target = match self.user_storage.get_user_by_username(username).await {
+            Ok(u) => u,
+            Err(e) if e.kind == ErrorKind::NotFound => {
+                tracing::info!(
+                    target: "audit",
+                    event = "user_profile.rejected",
+                    reason = "target_username_not_found",
+                    caller_id = %caller_id,
+                    target_username = %username,
+                    "👮🏻‍♂️ user-profile rejected: username '{}' does not exist (caller {})",
+                    username,
+                    caller_id,
+                );
+                return Err(DomainError::new(
+                    ErrorKind::NotFound,
+                    "User",
+                    "User not found",
+                ));
+            }
+            Err(e) => return Err(e),
+        };
+        self.get_user_profile(caller_id, target.id(), expose_system_users, pool)
+            .await
+    }
+
     // New method to get user by username - needed for admin user handling
     pub async fn get_user_by_username(&self, username: &str) -> Result<UserDto, DomainError> {
         let user = self.user_storage.get_user_by_username(username).await?;

src/application/services/contact_service.rs

@@ -535,10 +535,16 @@ impl ContactUseCase for ContactService {
         let address_book_id = Uuid::parse_str(&dto.address_book_id)
             .map_err(|_| DomainError::validation_error("Invalid address book ID format"))?;
 
-        // Check if user has write access to the address book
+        // AuthZ audit #19 (2026-07-12): previously required
+        // `Permission::Update`, which is NOT in the Contributor bundle
+        // (Read + Create) — Contributor grantees on a shared address
+        // book couldn't add contacts via REST or CardDAV PUT despite
+        // holding the intended Create permission. `Delete` uses Delete
+        // (audit #13, above); creation must use Create. Same fix
+        // applied to `create_contact_from_vcard` + `create_group`.
         let caller_id = Uuid::parse_str(&dto.user_id)
             .map_err(|_| DomainError::validation_error("Invalid user ID format"))?;
-        self.require_address_book_perm(&address_book_id, &caller_id, Permission::Update)
+        self.require_address_book_perm(&address_book_id, &caller_id, Permission::Create)
             .await?;
 
         // Convert DTOs to domain entities
@@ -614,10 +620,13 @@ impl ContactUseCase for ContactService {
         let address_book_id = Uuid::parse_str(&dto.address_book_id)
             .map_err(|_| DomainError::validation_error("Invalid address book ID format"))?;
 
-        // Check if user has write access to the address book
+        // AuthZ audit #19 — see the sibling `create_contact` above.
+        // This is the CardDAV `PUT contact.vcf` entry point; the fix
+        // unblocks Contributor grantees creating contacts through the
+        // CardDAV protocol as well as the REST surface.
         let caller_id = Uuid::parse_str(&dto.user_id)
             .map_err(|_| DomainError::validation_error("Invalid user ID format"))?;
-        self.require_address_book_perm(&address_book_id, &caller_id, Permission::Update)
+        self.require_address_book_perm(&address_book_id, &caller_id, Permission::Create)
             .await?;
 
         // Parse vCard data
@@ -756,8 +765,14 @@ impl ContactUseCase for ContactService {
             .await?
             .ok_or_else(|| DomainError::not_found("Contact", "not found"))?;
 
-        // Check if user has write access to the address book
-        self.require_address_book_perm(contact.address_book_id(), &user_id, Permission::Update)
+        // AuthZ audit #13 (2026-07-12): previously required
+        // `Permission::Update`, which the Editor role bundle satisfies
+        // (Read + Comment + Create + Update). Every Editor grantee on a
+        // shared address book could delete individual contacts — a
+        // silent privilege escalation because the intent for CardDAV
+        // deletion is Delete, not Update. Sibling
+        // `CalendarService::delete_event` was the ground-truth pattern.
+        self.require_address_book_perm(contact.address_book_id(), &user_id, Permission::Delete)
             .await?;
 
         // Delete the contact
@@ -902,10 +917,10 @@ impl ContactUseCase for ContactService {
         let address_book_id = Uuid::parse_str(&dto.address_book_id)
             .map_err(|_| DomainError::validation_error("Invalid address book ID format"))?;
 
-        // Check if user has write access to the address book
+        // AuthZ audit #19 — see the sibling `create_contact` above.
         let caller_id = Uuid::parse_str(&dto.user_id)
             .map_err(|_| DomainError::validation_error("Invalid user ID format"))?;
-        self.require_address_book_perm(&address_book_id, &caller_id, Permission::Update)
+        self.require_address_book_perm(&address_book_id, &caller_id, Permission::Create)
             .await?;
 
         let group = ContactGroup::new(address_book_id, dto.name);
@@ -959,8 +974,11 @@ impl ContactUseCase for ContactService {
             .await?
             .ok_or_else(|| DomainError::not_found("Contact group", "not found"))?;
 
-        // Check if user has write access to the address book
-        self.require_address_book_perm(group.address_book_id(), &user_id, Permission::Update)
+        // AuthZ audit #13 (2026-07-12): see the sibling `delete_contact`
+        // above — required `Update` (in the Editor bundle) instead of
+        // `Delete`, letting any Editor on a shared address book delete
+        // groups they shouldn't.
+        self.require_address_book_perm(group.address_book_id(), &user_id, Permission::Delete)
             .await?;
 
         // Delete the group

src/application/services/drive_management_service.rs

@@ -480,6 +480,11 @@ impl DriveManagementService {
     /// supplied is overwritten. Returns the post-merge typed view.
     /// Audit emits `drive.policy_changed` with the post-merge bag for
     /// steady-state observability.
+    ///
+    /// Ed's call, 2026-07-17: intentional deviation from the AGENTS.md
+    /// "AuthZ in service layer" rule for this specific endpoint —
+    /// the handler-layer admin check stays, this method stays trusting.
+    /// See memory `feedback_drive_policies_admin_at_handler`.
     pub async fn update_policies(
         &self,
         caller_id: Uuid,

src/application/services/file_upload_service.rs

@@ -457,6 +457,44 @@ impl FileUploadUseCase for FileUploadService {
         Ok(dto)
     }
 
+    /// AuthZ audit #17 — `Create` on target folder is re-verified here
+    /// so mid-session grant revocations take effect at finalize. When
+    /// `folder_id` is `None` the write lands at drive-root; the drive
+    /// resolution for that case isn't plumbed through the chunked-
+    /// upload session (`UploadSession.folder_id` alone), so we fall
+    /// back to the pre-audit behaviour there. That drive-root path is
+    /// tracked separately as part of the D0 folder-id-walking work;
+    /// closing it here would require session-scoped drive_id.
+    async fn upload_file_streaming_with_perms(
+        &self,
+        name: String,
+        folder_id: Option<String>,
+        content_type: String,
+        blob: StoredBlob,
+        caller_id: Uuid,
+    ) -> Result<FileDto, DomainError> {
+        if let Some(fid) = folder_id.as_deref() {
+            let Some(authz) = &self.authorization else {
+                return Err(DomainError::internal_error(
+                    "FileUpload",
+                    "upload_file_streaming_with_perms called without authorization engine wired",
+                ));
+            };
+            let folder_uuid = Uuid::parse_str(fid)
+                .map_err(|_| DomainError::not_found("Folder", fid.to_string()))?;
+            authz
+                .require(
+                    Subject::User(caller_id),
+                    Permission::Create,
+                    Resource::Folder(folder_uuid),
+                )
+                .await?;
+        }
+
+        self.upload_file_streaming(name, folder_id, content_type, blob, caller_id)
+            .await
+    }
+
     /// Swap the content of the file at `path` to an already-ingested blob,
     /// creating the file when it doesn't exist (WebDAV/NextCloud/WOPI PUT).
     ///

src/application/services/folder_service.rs

@@ -581,7 +581,7 @@ impl FolderUseCase for FolderService {
             )
             .await?;
 
-        let folder = self
+        let renamed = self
             .folder_storage
             .rename_folder(id, dto.name, caller_id)
             .await
@@ -592,7 +592,26 @@ impl FolderUseCase for FolderService {
                 )
             })?;
 
-        Ok(FolderDto::from(folder))
+        // Root folders double as the drive's display name (see the
+        // `required_perm` branch above and `drive_pg_repository.rs`
+        // `readable_cache` + `default_drive_cache` docs).
+        // `drives.name` is sourced from `folders.name` of the root
+        // folder, so a rename affects BOTH caches — every user's
+        // readable-drive list AND the per-user default-drive lookup.
+        // Both are 30 s TTL; without the invalidation, `GET /api/drives`
+        // returns the stale name for up to that window after a root
+        // rename. Surfaced by `tests/api/drives_membership.hurl`
+        // Step 23. Regression from commit `12dc648c` ("perf: round 4 —
+        // drive-selector cache") which added the caches without
+        // wiring the root-rename invalidation.
+        if folder.parent_id().is_none()
+            && let Some(drive_repo) = &self.drive_repo
+        {
+            drive_repo.invalidate_readable_all();
+            drive_repo.invalidate_default_drive_all();
+        }
+
+        Ok(FolderDto::from(renamed))
     }
 
     /// Moves a folder to a new parent. Requires `Update` on the source and

src/application/services/storage_usage_service.rs

@@ -19,6 +19,13 @@ use uuid::Uuid;
 pub struct StorageUsageService {
     pool: Arc<PgPool>,
     user_repository: Arc<UserPgRepository>,
+    /// Optional so DI can wire it lazily and older test constructors
+    /// keep compiling. When `Some`, every write path that mutates
+    /// `drives.used_bytes` or `users.storage_used_bytes` invalidates
+    /// the drive lookup caches so `GET /api/drives` reflects the new
+    /// usage on the next call (see the invalidation calls in the
+    /// delta / sweep methods below).
+    drive_repo: Option<Arc<dyn crate::domain::repositories::drive_repository::DriveRepository>>,
 }
 
 impl StorageUsageService {
@@ -27,6 +34,44 @@ impl StorageUsageService {
         Self {
             pool,
             user_repository,
+            drive_repo: None,
+        }
+    }
+
+    /// Wires the drive repository used for cache-invalidation-on-write.
+    /// Production DI calls this in `common::di`; tests without a real
+    /// drive repo leave it `None` and the invalidation calls no-op.
+    pub fn with_drive_repo(
+        mut self,
+        drive_repo: Arc<dyn crate::domain::repositories::drive_repository::DriveRepository>,
+    ) -> Self {
+        self.drive_repo = Some(drive_repo);
+        self
+    }
+
+    /// Drop the per-caller readable-drive listing cache and the
+    /// per-user default-drive cache so `GET /api/drives` and the
+    /// WebDAV / NextCloud / WOPI drive-lookup paths re-read fresh
+    /// values.
+    ///
+    /// **Called only from the reconciliation sweep**, not from the
+    /// hot-path `add_drive_storage_usage_delta*` methods. The design
+    /// (Ed's call, 2026-07-17): keep the cache useful under active
+    /// upload load — per-mutation invalidation would nuke the cache
+    /// on every file upload, defeating the point. `used_bytes` on
+    /// `GET /api/drives` therefore lags by up to the cache TTL (30 s),
+    /// which matches the sibling caches' accepted UX phantom for
+    /// drive-name staleness. Tests / operators that need immediate
+    /// freshness call `POST /api/admin/internal/trigger-sweep`, which
+    /// runs `update_all_drives_storage_usage` → this method.
+    ///
+    /// Security posture unaffected: `check_drive_quota` reads
+    /// directly from SQL, bypassing the cache entirely, so quota
+    /// enforcement is honest regardless of listing staleness.
+    fn invalidate_drive_lookup_caches(&self) {
+        if let Some(repo) = &self.drive_repo {
+            repo.invalidate_readable_all();
+            repo.invalidate_default_drive_all();
         }
     }
 
@@ -209,6 +254,9 @@ impl StorageUsageService {
         .execute(self.pool.as_ref())
         .await
         .map_err(|e| DomainError::internal_error("StorageUsage", format!("drive delta: {e}")))?;
+        // Deliberate no-invalidate here — see the class doc on
+        // `invalidate_drive_lookup_caches`. Delta writes lag the
+        // cache by up to the TTL; the sweep is the escape hatch.
         Ok(())
     }
 
@@ -285,6 +333,7 @@ impl StorageUsageService {
         .map_err(|e| {
             DomainError::internal_error("StorageUsage", format!("drive delta by folder: {e}"))
         })?;
+        // See `add_drive_storage_usage_delta` — deliberate no-invalidate.
         Ok(())
     }
 
@@ -595,6 +644,20 @@ impl StorageUsagePort for StorageUsageService {
             "Drive storage-usage reconciliation corrected {} drive(s)",
             result.rows_affected()
         );
+        // Unconditional invalidation — do NOT gate on
+        // `rows_affected() > 0`. When a fire-and-forget delta has
+        // already made SQL correct BEFORE the sweep runs, the sweep
+        // touches zero rows but the cache may still hold the
+        // pre-delta value from an earlier `GET /api/drives`. Gating
+        // means the cache stays stale in exactly the case
+        // `trigger-sweep` is called to fix. The invalidation cost is
+        // small (moka `invalidate_all` on both caches); the
+        // correctness guarantee matters. Regression avoidance:
+        // drive_quota.hurl Step 6 exercises this race — 2nd upload's
+        // delta lands during the 200 ms delay, sweep sees SQL is
+        // already right → zero rows → without unconditional
+        // invalidation, cache stays at the previous step's value.
+        self.invalidate_drive_lookup_caches();
         Ok(())
     }
 
@@ -613,6 +676,7 @@ impl Clone for StorageUsageService {
         Self {
             pool: Arc::clone(&self.pool),
             user_repository: Arc::clone(&self.user_repository),
+            drive_repo: self.drive_repo.clone(),
         }
     }
 }

src/application/services/trash_service.rs

@@ -662,22 +662,25 @@ impl TrashUseCase for TrashService {
     async fn empty_trash_for_drive(&self, user_id: Uuid, drive_id: Uuid) -> Result<()> {
         // Per-drive trash empty — the Drive group-by on `/trash` exposes
         // this as a per-row affordance so multi-drive owners can clear
-        // one drive without touching the others. Refuses with
-        // `NotFound` (anti-enum) when the caller lacks Delete on the
-        // named drive — same shape as the user-facing drive listing
-        // would emit for an unknown id.
-        let allowed = self.drives_with_delete_for(user_id).await?;
-        if !allowed.contains(&drive_id) {
-            tracing::info!(
-                target: "audit",
-                event = "trash.empty_drive_rejected",
-                reason = "no_delete_on_drive",
-                user_id = %user_id,
-                drive_id = %drive_id,
-                "👮🏻‍♂️ refused per-drive empty — caller lacks Delete on this drive",
-            );
-            return Err(DomainError::not_found("Drive", drive_id.to_string()));
-        }
+        // one drive without touching the others.
+        //
+        // Route through `authz.require(Delete, Drive)` so the denial
+        // shape stays consistent with every other write verb: 403 when
+        // the caller has Read on the drive (viewer/editor holding no
+        // Delete), 404 when they don't (anti-enum). Before 2026-07-16
+        // this method rolled its own `drives_with_delete_for` check +
+        // hardcoded `NotFound` — that predated the graduated-denial
+        // engine change and returned 404 unconditionally even for a
+        // Viewer who could see the drive in `/api/drives`. The engine
+        // now emits `authz.denied` with `visibility="visible"|"hidden"`
+        // and the standard mapping renders it as 403 or 404.
+        self.authz
+            .require(
+                Subject::User(user_id),
+                Permission::Delete,
+                Resource::Drive(drive_id),
+            )
+            .await?;
         info!("Emptying trash for drive {} (user {})", drive_id, user_id);
         self.clear_trash_in(&[drive_id], user_id).await
     }