Merge pull request #551 from EdouardVanbelle/security/authz

This commit is contained in:
Dionisio Pozo
2026-07-06 00:03:03 +02:00
committed by GitHub
34 changed files with 1651 additions and 326 deletions
+2 -2
View File
@@ -583,7 +583,7 @@
"item_deleted_permanently": "Elemento eliminato definitivamente",
"trash_emptied": "Cestino svuotato con successo",
"empty": "Nessuna notifica",
"title": "Notifiche"
"title": "Notifiche",
"link_created": "Link creato",
"share_success": "Link di condivisione creato con successo",
"upload_files_section_title": "Caricamento non disponibile qui",
@@ -948,7 +948,7 @@
"size": "Dimensione",
"favoriteDate": "Data preferito",
"byFiles": "Per file",
"sharedWith": "Condiviso con"
"sharedWith": "Condiviso con",
"justAdded": "Nuovo",
"folders": "Cartelle"
},
+6 -1
View File
@@ -75,7 +75,12 @@ pub trait FileUploadUseCase: Send + Sync + 'static {
/// `updated_by` column reflects the principal that performed the
/// PUT — not the file's existing owner (D2 shared drives let
/// non-owners overwrite content).
async fn update_file_streaming(
/// `_with_perms` suffix (AGENTS.md AuthZ convention): the
/// implementation calls `authz.require(caller, Update, File(id))`
/// on the overwrite branch and `authz.require(caller, Create,
/// Folder|Drive(id))` on the new-file branch. Handlers just plumb
/// `caller_id` through — no protocol-layer authz.
async fn update_file_streaming_with_perms(
&self,
path: &str,
drive_id: Uuid,
+35 -22
View File
@@ -9,10 +9,12 @@ use crate::application::dtos::favorites_dto::{
BatchFavoritesResult, BatchFavoritesStats, FavoriteItemDto, FavoriteResourceRow,
FavoritesCursor,
};
use crate::application::ports::authorization_ports::AuthorizationEngine;
use crate::application::ports::favorites_ports::{FavoritesRepositoryPort, FavoritesUseCase};
use crate::common::errors::{DomainError, ErrorKind, Result};
use crate::domain::services::authorization::ResourceKind;
use crate::common::errors::Result;
use crate::domain::services::authorization::{Permission, Resource, ResourceKind, Subject};
use crate::infrastructure::repositories::pg::FavoritesPgRepository;
use crate::infrastructure::services::pg_acl_engine::PgAclEngine;
/// Implementation of the FavoritesUseCase for managing user favorites.
///
@@ -20,12 +22,22 @@ use crate::infrastructure::repositories::pg::FavoritesPgRepository;
/// accessing the database directly, following hexagonal architecture.
pub struct FavoritesService {
repo: Arc<FavoritesPgRepository>,
/// ReBAC engine — enforces `Permission::Read` on the referenced
/// file/folder before enrolling it into a user's favorites.
/// Without this gate the write path is an information oracle:
/// listing endpoints JOIN back to `storage.files/folders` and
/// return name/mime/size/drive_id for any UUID the caller was
/// able to enroll. See `docs/plan/authz_audit/rest_storage.md`.
authorization: Arc<PgAclEngine>,
}
impl FavoritesService {
/// Create a new FavoritesService with the given repository port
pub fn new(repo: Arc<FavoritesPgRepository>) -> Self {
Self { repo }
pub fn new(repo: Arc<FavoritesPgRepository>, authorization: Arc<PgAclEngine>) -> Self {
Self {
repo,
authorization,
}
}
/// Subset of `(item_id, item_type)` pairs the user has favorited — used to
@@ -60,13 +72,15 @@ impl FavoritesUseCase for FavoritesService {
item_type, item_id, user_id
);
if item_type != "file" && item_type != "folder" {
return Err(DomainError::new(
ErrorKind::InvalidInput,
"Favorites",
"Item type must be 'file' or 'folder'",
));
}
// AuthZ pre-write: caller must have Read on the referenced
// resource. Denial routes through `require` → NotFound
// (anti-enum, matches the listing shape) + `authz.denied`
// audit line. Without this gate the write path was an
// information oracle over the whole tenant.
let resource = Resource::parse(item_type, item_id)?;
self.authorization
.require(Subject::User(user_id), Permission::Read, resource)
.await?;
self.repo.add_favorite(user_id, item_id, item_type).await?;
info!(
@@ -125,18 +139,17 @@ impl FavoritesUseCase for FavoritesService {
user_id
);
// Validate all item types
// AuthZ pre-write: caller must have Read on every referenced
// resource. Fail the whole batch on the first denial so the
// response shape doesn't tell an attacker which items were
// valid (partial success would leak the same oracle we
// closed on the single-item path). See
// `docs/plan/authz_audit/rest_storage.md`.
for (item_id, item_type) in items {
if item_type != "file" && item_type != "folder" {
return Err(DomainError::new(
ErrorKind::InvalidInput,
"Favorites",
format!(
"Item type must be 'file' or 'folder' for item '{}'",
item_id
),
));
}
let resource = Resource::parse(item_type, item_id)?;
self.authorization
.require(Subject::User(user_id), Permission::Read, resource)
.await?;
}
let requested = items.len();
@@ -42,6 +42,16 @@ pub struct FileUploadService {
/// `(file_id, blob_hash, content_type)`; the recording side needs the
/// `caller_id` the service already has in hand.
resource_access_hook: Option<Arc<dyn ResourceAccessHook>>,
/// ReBAC engine — enforces `Permission::Update` on
/// overwrite-existing and `Permission::Create` on new-file paths
/// inside `update_file_streaming_with_perms`. Optional at the
/// struct level for the minimal test constructors (`new`,
/// `new_with_read`) but the WebDAV/NC/WOPI put paths refuse
/// (fail-closed internal error) if this isn't wired. Set by
/// either `with_instant_upload` or `with_authorization` — both
/// stash the same Arc so DI callers wiring instant upload get
/// the streaming gate for free.
authorization: Option<Arc<PgAclEngine>>,
/// Dependencies of the instant-upload path
/// (`create_file_from_owned_blob_with_perms`); `None` in minimal test
/// wiring.
@@ -66,6 +76,7 @@ impl FileUploadService {
content_cache: None,
file_lifecycle_hook: None,
resource_access_hook: None,
authorization: None,
instant_upload: None,
}
}
@@ -82,18 +93,34 @@ impl FileUploadService {
content_cache: None,
file_lifecycle_hook: None,
resource_access_hook: None,
authorization: None,
instant_upload: None,
}
}
/// Wires the authorization engine used by
/// `update_file_streaming_with_perms` on the WebDAV / NC / WOPI
/// PUT path. Independent of `with_instant_upload` so callers can
/// enable the streaming gate without also opting into the
/// dedup-instant-upload check (test wiring, minimal deployments).
pub fn with_authorization(mut self, authz: Arc<PgAclEngine>) -> Self {
self.authorization = Some(authz);
self
}
/// Wires the authorization engine, dedup index and quota service that
/// power the instant-upload path.
///
/// Also stashes the `authz` handle in `self.authorization` so
/// DI callers wiring instant upload get the streaming-put gate
/// for free — a single `Arc` clone, no behavioural coupling.
pub fn with_instant_upload(
mut self,
authz: Arc<PgAclEngine>,
dedup: Arc<DedupService>,
quota: Arc<StorageUsageService>,
) -> Self {
self.authorization = Some(authz.clone());
self.instant_upload = Some(InstantUploadDeps {
authz,
dedup,
@@ -296,7 +323,6 @@ impl FileUploadService {
parts.folder_id,
parts.created_at,
updated_at as u64,
parts.owner_id,
new_hash,
)
.map_err(|e| DomainError::internal_error("FileUpload", format!("rebuild entity: {e}")))?;
@@ -425,7 +451,16 @@ impl FileUploadUseCase for FileUploadService {
/// 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).
async fn update_file_streaming(
///
/// AuthZ (post-Drive audit Round 2 fix): overwrite path requires
/// `Update` on the target file; new-file path requires `Create`
/// on the parent folder (or on the drive when writing at drive
/// root). Fail-closed if the engine wasn't wired — this method
/// is the last line of defence between a Viewer/Commenter drive
/// member and cross-tenant PUT. See
/// `docs/plan/authz_audit/nextcloud.md` and the sibling native
/// `/webdav/*` handler.
async fn update_file_streaming_with_perms(
&self,
path: &str,
drive_id: Uuid,
@@ -434,10 +469,33 @@ impl FileUploadUseCase for FileUploadService {
modified_at: Option<i64>,
caller_id: Uuid,
) -> Result<FileDto, DomainError> {
let Some(authz) = &self.authorization else {
return Err(DomainError::internal_error(
"FileUpload",
"update_file_streaming_with_perms called without authorization engine wired",
));
};
// Try to find the existing file first
if let Some(file_read) = &self.file_read
&& let Some(file) = file_read.find_file_by_path(path, drive_id).await?
{
// Overwrite branch — caller must have `Update` on the
// target file. Denial routes through `require` → 404
// (anti-enum, matches read-side shape). Before the D7
// audit this whole branch ran unchecked; Viewer members
// of shared drives could PUT freely.
let file_uuid = Uuid::parse_str(file.id()).map_err(|_| {
DomainError::internal_error("FileUpload", "invalid file id from repository")
})?;
authz
.require(
Subject::User(caller_id),
Permission::Update,
Resource::File(file_uuid),
)
.await?;
let file_id = file.id().to_string();
let (new_hash, updated_at) = self
.file_write
@@ -467,7 +525,6 @@ impl FileUploadUseCase for FileUploadService {
parts.folder_id,
parts.created_at,
updated_at as u64,
parts.owner_id,
new_hash,
)
.map_err(|e| {
@@ -507,6 +564,32 @@ impl FileUploadUseCase for FileUploadService {
None
};
// Create branch — caller must have `Create` on the parent
// scope. Two cases:
// * `parent_id.is_some()` → caller needs Create on the
// parent Folder resource.
// * `parent_id.is_none()` → the write lands at the drive
// root (either the path was single-segment, or the
// parent-folder lookup failed). We require Create on
// the Drive itself — bundled with owner/editor/contributor
// role_grants, refused for viewer/commenter.
let create_resource = match &parent_id {
Some(pid) => {
let uuid = Uuid::parse_str(pid).map_err(|_| {
DomainError::internal_error("FileUpload", "invalid parent folder id")
})?;
Resource::Folder(uuid)
}
None => Resource::Drive(drive_id),
};
authz
.require(
Subject::User(caller_id),
Permission::Create,
create_resource,
)
.await?;
let is_new_blob = blob.is_new_blob;
let created = self
.file_write
+31 -3
View File
@@ -5,17 +5,31 @@ use crate::application::dtos::playlist_dto::{
AddTracksDto, AudioMetadataDto, CreatePlaylistDto, PlaylistDto, PlaylistItemDto,
PlaylistQueryDto, PlaylistShareInfoDto, ReorderTracksDto, SharePlaylistDto, UpdatePlaylistDto,
};
use crate::application::ports::authorization_ports::AuthorizationEngine;
use crate::application::ports::music_ports::{MusicStoragePort, MusicUseCase};
use crate::common::errors::{DomainError, ErrorKind};
use crate::domain::services::authorization::{Permission, Resource, Subject};
use crate::infrastructure::adapters::music_storage_adapter::MusicStorageAdapter;
use crate::infrastructure::services::pg_acl_engine::PgAclEngine;
pub struct MusicService {
storage: Arc<MusicStorageAdapter>,
/// ReBAC engine — Round 1 fix from `docs/plan/authz_audit/`.
/// Currently used ONLY by `get_audio_metadata` to close the
/// cross-tenant IDOR (`_user_id: Uuid` was deliberately unused).
/// The full engine rewrite (Round 3 — `Resource::Playlist` +
/// authz.require on every playlist verb) is a separate PR;
/// don't extend the bespoke `user_has_access` / `user_can_write`
/// pattern to new methods, use `require` here instead.
authorization: Arc<PgAclEngine>,
}
impl MusicService {
pub fn new(storage: Arc<MusicStorageAdapter>) -> Self {
Self { storage }
pub fn new(storage: Arc<MusicStorageAdapter>, authorization: Arc<PgAclEngine>) -> Self {
Self {
storage,
authorization,
}
}
}
@@ -375,10 +389,24 @@ impl MusicUseCase for MusicService {
async fn get_audio_metadata(
&self,
file_id: &str,
_user_id: Uuid,
caller_id: Uuid,
) -> Result<Option<AudioMetadataDto>, DomainError> {
let file_uuid = Uuid::parse_str(file_id)
.map_err(|_| DomainError::new(ErrorKind::InvalidInput, "Music", "Invalid file ID"))?;
// AuthZ pre-read: caller must have `Read` on the underlying
// audio file. Before this check the endpoint returned
// metadata for any known file id (cross-tenant IDOR — the
// `_user_id` parameter was deliberately unused). `require`
// returns 404 on denial to match the anti-enum shape used
// everywhere else. Post-Drive AuthZ audit fix (Round 1
// BLOCKER — `docs/plan/authz_audit/rest_storage.md`).
self.authorization
.require(
Subject::User(caller_id),
Permission::Read,
Resource::File(file_uuid),
)
.await?;
self.storage.get_audio_metadata(&file_uuid).await
}
}
+69 -12
View File
@@ -1,10 +1,12 @@
use crate::application::dtos::cursor::PageCursor;
use crate::application::dtos::recent_dto::{RecentCursor, RecentItemDto, RecentResourceRow};
use crate::application::ports::authorization_ports::AuthorizationEngine;
use crate::application::ports::recent_ports::{RecentItemsRepositoryPort, RecentItemsUseCase};
use crate::application::ports::resource_access_hook::ResourceAccessHook;
use crate::common::errors::{DomainError, ErrorKind, Result};
use crate::domain::services::authorization::ResourceKind;
use crate::common::errors::{DomainError, Result};
use crate::domain::services::authorization::{Permission, Resource, ResourceKind, Subject};
use crate::infrastructure::repositories::pg::RecentItemsPgRepository;
use crate::infrastructure::services::pg_acl_engine::PgAclEngine;
use std::sync::{Arc, OnceLock};
use tracing::info;
use uuid::Uuid;
@@ -16,6 +18,13 @@ use uuid::Uuid;
pub struct RecentService {
repo: Arc<RecentItemsPgRepository>,
max_recent_items: i32,
/// ReBAC engine — enforces `Permission::Read` on the referenced
/// file/folder before enrolling it into a user's Recent list.
/// The listing side JOINs back to `storage.files/folders` and
/// returns name/mime/size/drive_id for any enrolled UUID, so
/// the write path is an information oracle without this gate.
/// See `docs/plan/authz_audit/rest_storage.md`.
authorization: Arc<PgAclEngine>,
/// Set after construction via [`Self::set_resource_access_hook`].
/// The hook is built FROM this service (it wraps an `Arc<Self>`), so
/// we can't take it as a constructor arg without circular ownership;
@@ -28,10 +37,15 @@ pub struct RecentService {
impl RecentService {
/// Create a new recent items service
pub fn new(repo: Arc<RecentItemsPgRepository>, max_recent_items: i32) -> Self {
pub fn new(
repo: Arc<RecentItemsPgRepository>,
authorization: Arc<PgAclEngine>,
max_recent_items: i32,
) -> Self {
Self {
repo,
max_recent_items: max_recent_items.clamp(1, 100),
authorization,
resource_access_hook: OnceLock::new(),
}
}
@@ -53,6 +67,41 @@ impl RecentService {
hook.on_recents_cleared(user_id);
}
}
/// Record access to an item WITHOUT the pre-write `authz.require`
/// gate. Callers must have gated the caller's Read upstream — this
/// method exists for the `RecentRecordingHook` fast path: writes
/// that reach the hook have already passed a `_with_perms` service
/// method (uploads, streams, GETs, etc.), so re-checking here
/// would be pure duplicate work AND widen the race window between
/// the POST response and the `tokio::spawn`ed upsert (
/// `tests/api/recent.hurl` step 7 hits this — the extra SQL
/// round-trip pushes the upsert past the client's immediate
/// `GET /api/recent/resources`).
///
/// **Do NOT call this from an externally-reachable handler.** The
/// REST endpoint goes through the trait method `record_item_access`
/// below, which enforces the Read gate per AGENTS.md convention.
pub async fn record_item_access_internal(
&self,
user_id: Uuid,
item_id: &str,
item_type: &str,
) -> Result<()> {
// Type validation only — no authz, no resource parse for the
// engine (the hook path is already resource-typed by construction).
if item_type != "file" && item_type != "folder" {
return Err(DomainError::new(
crate::common::errors::ErrorKind::InvalidInput,
"RecentItems",
"Item type must be 'file' or 'folder'",
));
}
self.repo.upsert_access(user_id, item_id, item_type).await?;
self.repo.prune(user_id, self.max_recent_items).await?;
Ok(())
}
}
impl RecentItemsUseCase for RecentService {
@@ -87,16 +136,24 @@ impl RecentItemsUseCase for RecentService {
item_type, item_id, user_id
);
if item_type != "file" && item_type != "folder" {
return Err(DomainError::new(
ErrorKind::InvalidInput,
"RecentItems",
"Item type must be 'file' or 'folder'",
));
}
// AuthZ pre-write: caller must have Read on the referenced
// resource. Denial routes through `require` → NotFound
// (anti-enum) + `authz.denied` audit line. Without this
// gate the write path was an information oracle over the
// whole tenant via the listing endpoint's JOIN back to
// storage.files/folders.
//
// Internal hook callers (RecentRecordingHook) bypass the
// trait entry point and call `record_item_access_internal`
// directly — Read has already been enforced upstream on
// whatever `_with_perms` service produced the access event.
let resource = Resource::parse(item_type, item_id)?;
self.authorization
.require(Subject::User(user_id), Permission::Read, resource)
.await?;
self.repo.upsert_access(user_id, item_id, item_type).await?;
self.repo.prune(user_id, self.max_recent_items).await?;
self.record_item_access_internal(user_id, item_id, item_type)
.await?;
info!(
"Successfully recorded access to {} '{}' for user {}",
+23 -1
View File
@@ -6,7 +6,7 @@ use uuid::Uuid;
use crate::domain::repositories::drive_repository::DriveRepository;
use crate::domain::repositories::folder_repository::FolderRepository;
use crate::domain::services::authorization::{Resource, Role, Subject};
use crate::domain::services::authorization::{Permission, Resource, Role, Subject};
use crate::infrastructure::repositories::pg::DrivePgRepository;
use crate::infrastructure::repositories::pg::SharePgRepository;
use crate::infrastructure::repositories::pg::file_blob_read_repository::FileBlobReadRepository;
@@ -243,6 +243,28 @@ impl ShareUseCase for ShareService {
self.verify_item_exists(&dto.item_id, &item_type).await?;
// AuthZ: only callers with `Share` on the resource may mint a
// public link. Without this gate, an ex-Viewer who kept a
// guessed UUID could launder a temporary read into a
// permanent anonymous URL that survives their own grant
// revocation. `Permission::Share` is bundled with the
// `owner` and `editor` role_grants only. `require` returns
// `not_found` on denial (anti-enum, matches the shape used
// by every other share route). See `docs/plan/authz_audit/`.
let item_uuid_for_authz = Uuid::parse_str(&dto.item_id)
.map_err(|_| ShareServiceError::Validation("Invalid item UUID".to_string()))?;
let resource_for_authz = match item_type {
ShareItemType::File => Resource::File(item_uuid_for_authz),
ShareItemType::Folder => Resource::Folder(item_uuid_for_authz),
};
self.authorization
.require(
Subject::User(user_id),
Permission::Share,
resource_for_authz,
)
.await?;
// D5: `forbid_public_links` policy gate. The drive owner can
// disable anonymous-link creation on every resource in their
// drive without per-resource intervention. Lookup is one JOIN
+49 -33
View File
@@ -889,23 +889,37 @@ impl AppServiceFactory {
Some(service)
}
/// Creates the favorites service (requires database)
pub fn create_favorites_service(&self, db_pool: &Arc<PgPool>) -> Arc<FavoritesService> {
/// Creates the favorites service (requires database + authz engine
/// for the Read gate on `add_to_favorites` — see the post-Drive
/// AuthZ audit).
pub fn create_favorites_service(
&self,
db_pool: &Arc<PgPool>,
authorization: &Arc<PgAclEngine>,
) -> Arc<FavoritesService> {
let repo = Arc::new(
crate::infrastructure::repositories::pg::FavoritesPgRepository::new(db_pool.clone()),
);
let service = Arc::new(FavoritesService::new(repo));
let service = Arc::new(FavoritesService::new(repo, authorization.clone()));
tracing::info!("Favorites service initialized");
service
}
/// Creates the recent items service (requires database)
pub fn create_recent_service(&self, db_pool: &Arc<PgPool>) -> Arc<RecentService> {
/// Creates the recent items service (requires database + authz
/// engine for the Read gate on `record_item_access` — see the
/// post-Drive AuthZ audit).
pub fn create_recent_service(
&self,
db_pool: &Arc<PgPool>,
authorization: &Arc<PgAclEngine>,
) -> Arc<RecentService> {
let repo = Arc::new(
crate::infrastructure::repositories::pg::RecentItemsPgRepository::new(db_pool.clone()),
);
let service = Arc::new(RecentService::new(
repo, 50, // Maximum recent items per user
repo,
authorization.clone(),
50, // Maximum recent items per user
));
tracing::info!("Recent items service initialized");
service
@@ -1161,31 +1175,6 @@ impl AppServiceFactory {
let pool = Arc::new(pools.primary);
let maintenance_pool = Arc::new(pools.maintenance);
// Recent service + recording hook are built up-front so the
// hook can be threaded into `create_application_services` below.
// The file services hold the hook directly so every authorised
// `_with_perms` read/write fires into `auth.user_recent_files`
// without per-handler wiring. Reordering vs the legacy in-block
// creation (further down) is safe: `create_recent_service` only
// needs `pool`, which is already in scope.
//
// The back-edge `recent_service_eager.set_resource_access_hook`
// closes the loop so the clear/remove handlers can drop the
// hook's in-memory throttle entries — without it a freshly
// cleared Recent list refuses to re-record the same file for a
// full TTL window, surfacing as "I cleared, opened the file,
// and Recent is still empty" (caught by tests/api/recent.hurl
// step 8).
let recent_service_eager = self.create_recent_service(&pool);
let resource_access_hook: Arc<
dyn crate::application::ports::resource_access_hook::ResourceAccessHook,
> = Arc::new(
crate::infrastructure::services::recent_recording_hook::RecentRecordingHook::new(
recent_service_eager.clone(),
),
);
recent_service_eager.set_resource_access_hook(resource_access_hook.clone());
// 1. Core services (PgPool needed for DedupService index)
let core = self.create_core_services(&pool, &maintenance_pool).await?;
@@ -1196,6 +1185,10 @@ impl AppServiceFactory {
// because services hold an Arc<PgAclEngine> for ReBAC checks.
// SubjectGroupPgRepository is constructed here too so the engine can
// expand a user's transitive group set on cache misses.
//
// Moved above the eager recent-service build so `create_recent_service`
// can receive an `Arc<PgAclEngine>` — the Read gate on
// `record_item_access` (post-Drive AuthZ audit fix) needs it.
let subject_group_repo = Arc::new(
crate::infrastructure::repositories::pg::SubjectGroupPgRepository::new(pool.clone()),
);
@@ -1206,6 +1199,29 @@ impl AppServiceFactory {
subject_group_repo.clone(),
);
// Recent service + recording hook are built up-front so the
// hook can be threaded into `create_application_services` below.
// The file services hold the hook directly so every authorised
// `_with_perms` read/write fires into `auth.user_recent_files`
// without per-handler wiring.
//
// The back-edge `recent_service_eager.set_resource_access_hook`
// closes the loop so the clear/remove handlers can drop the
// hook's in-memory throttle entries — without it a freshly
// cleared Recent list refuses to re-record the same file for a
// full TTL window, surfacing as "I cleared, opened the file,
// and Recent is still empty" (caught by tests/api/recent.hurl
// step 8).
let recent_service_eager = self.create_recent_service(&pool, &authorization);
let resource_access_hook: Arc<
dyn crate::application::ports::resource_access_hook::ResourceAccessHook,
> = Arc::new(
crate::infrastructure::services::recent_recording_hook::RecentRecordingHook::new(
recent_service_eager.clone(),
),
);
recent_service_eager.set_resource_access_hook(resource_access_hook.clone());
// Drive repository — needed both by the lifecycle hook (when auth
// is enabled) and by `GET /api/drives` on the final `AppState`,
// so declared at the outer scope.
@@ -1279,7 +1295,7 @@ impl AppServiceFactory {
> = None;
{
let favs = self.create_favorites_service(&pool);
let favs = self.create_favorites_service(&pool, &authorization);
favorites_service = Some(favs.clone());
apps.favorites_service = Some(favs);
@@ -1849,7 +1865,7 @@ impl AppServiceFactory {
audio_metadata_repo,
),
);
let music_svc = Arc::new(MusicService::new(music_storage));
let music_svc = Arc::new(MusicService::new(music_storage, authorization.clone()));
app_state.music_service = Some(music_svc);
tracing::info!("Music service initialized");
}
+1 -1
View File
@@ -500,7 +500,7 @@ impl FileUploadUseCase for StubFileUploadUseCase {
Ok(FileDto::default())
}
async fn update_file_streaming(
async fn update_file_streaming_with_perms(
&self,
_path: &str,
_drive_id: Uuid,
-22
View File
@@ -22,7 +22,6 @@ pub struct FileParts {
pub folder_id: Option<String>,
pub created_at: u64,
pub modified_at: u64,
pub owner_id: Option<Uuid>,
/// BLAKE3 content hash. See [`File::content_hash`] for semantics.
pub blob_hash: String,
/// §14 provenance: original creator. See [`File::created_by`].
@@ -70,9 +69,6 @@ pub struct File {
/// Last modification timestamp (seconds since UNIX epoch)
modified_at: u64,
/// Owner user ID (from storage.files.user_id)
owner_id: Option<Uuid>,
/// BLAKE3 content hash. Stable across renames/moves, changes only
/// when the file's content bytes change. Source of truth for both
/// content-addressable storage and the HTTP ETag (via
@@ -109,7 +105,6 @@ impl Default for File {
folder_id: None,
created_at: 0,
modified_at: 0,
owner_id: None,
blob_hash: String::new(),
created_by: None,
updated_by: None,
@@ -150,7 +145,6 @@ impl File {
folder_id,
created_at: now,
modified_at: now,
owner_id: None,
blob_hash: String::new(),
created_by: None,
updated_by: None,
@@ -184,7 +178,6 @@ impl File {
folder_id: parent_id,
created_at,
modified_at,
owner_id: None,
blob_hash: String::new(),
created_by: None,
updated_by: None,
@@ -201,7 +194,6 @@ impl File {
folder_id: Option<String>,
created_at: u64,
modified_at: u64,
owner_id: Option<Uuid>,
) -> FileResult<Self> {
Self::with_timestamps_and_blob_hash(
id,
@@ -212,7 +204,6 @@ impl File {
folder_id,
created_at,
modified_at,
owner_id,
String::new(),
)
}
@@ -227,7 +218,6 @@ impl File {
folder_id: Option<String>,
created_at: u64,
modified_at: u64,
owner_id: Option<Uuid>,
blob_hash: String,
) -> FileResult<Self> {
Self::with_timestamps_blob_hash_and_provenance(
@@ -239,7 +229,6 @@ impl File {
folder_id,
created_at,
modified_at,
owner_id,
blob_hash,
None,
None,
@@ -259,7 +248,6 @@ impl File {
folder_id: Option<String>,
created_at: u64,
modified_at: u64,
owner_id: Option<Uuid>,
blob_hash: String,
created_by: Option<Uuid>,
updated_by: Option<Uuid>,
@@ -282,7 +270,6 @@ impl File {
folder_id,
created_at,
modified_at,
owner_id,
blob_hash,
created_by,
updated_by,
@@ -304,7 +291,6 @@ impl File {
folder_id: self.folder_id,
created_at: self.created_at,
modified_at: self.modified_at,
owner_id: self.owner_id,
blob_hash: self.blob_hash,
created_by: self.created_by,
updated_by: self.updated_by,
@@ -407,10 +393,6 @@ impl File {
self.modified_at
}
pub fn owner_id(&self) -> Option<Uuid> {
self.owner_id
}
/// User that originally created this file (§14 provenance).
/// `None` when the referenced user has been deleted
/// (FK is `ON DELETE SET NULL`) or for stub/DTO entities.
@@ -455,7 +437,6 @@ impl File {
folder_id,
created_at,
modified_at,
owner_id: None,
blob_hash: String::new(),
// DTO round-trips don't carry provenance; callers needing
// it must reload from the repository.
@@ -607,7 +588,6 @@ mod tests {
None,
1_000,
2_000,
None,
"abcdef0123456789ZZZZZZZZ".to_string(),
)
.unwrap();
@@ -632,7 +612,6 @@ mod tests {
None,
1_000,
2_000,
None,
"shorthash".to_string(),
)
.unwrap();
@@ -655,7 +634,6 @@ mod tests {
None,
1_000,
2_000,
None,
"stable-content-hash".to_string(),
)
.unwrap();
+7 -70
View File
@@ -25,10 +25,6 @@ pub struct Folder {
/// Parent folder ID (None if it's a root folder)
parent_id: Option<String>,
/// Owner user ID — scopes folder visibility per user.
/// `None` only for legacy/stub folders; real folders always have an owner.
owner_id: Option<Uuid>,
/// Drive that owns this folder. Post-D0 every `storage.folders` row
/// has `drive_id NOT NULL` (M3 migration). Path-based lookups scope
/// by this axis (not by `user_id`, which is dropped in D7).
@@ -76,7 +72,6 @@ impl Default for Folder {
storage_path: StoragePath::from_string("/"),
path_string: "/".to_string(),
parent_id: None,
owner_id: None,
drive_id: Uuid::nil(),
created_at: 0,
modified_at: 0,
@@ -88,26 +83,20 @@ impl Default for Folder {
}
impl Folder {
/// Creates a new folder with validation
/// Creates a new folder with validation.
///
/// In-memory constructor: callers that don't supply a `drive_id`
/// are by definition stub/legacy paths (tests, pre-D0 fixtures,
/// DTO round-trips). Real DB-backed folders flow through
/// [`Folder::with_timestamps_and_tree`] which propagates the
/// drive scope and §14 provenance from the row.
pub fn new(
id: String,
name: String,
storage_path: StoragePath,
parent_id: Option<String>,
) -> FolderResult<Self> {
Self::new_with_owner(id, name, storage_path, parent_id, None)
}
/// Creates a new folder with validation and an explicit owner.
pub fn new_with_owner(
id: String,
name: String,
storage_path: StoragePath,
parent_id: Option<String>,
owner_id: Option<Uuid>,
) -> FolderResult<Self> {
let name = normalize_storage_name(&name);
// Validate folder name
if let Err(reason) = validate_storage_name(&name) {
return Err(FolderError::InvalidFolderName(format!("{name}: {reason}")));
}
@@ -117,7 +106,6 @@ impl Folder {
.unwrap_or_default()
.as_secs();
// Store the path string for serialization compatibility
let path_string = storage_path.to_string();
Ok(Self {
@@ -126,17 +114,10 @@ impl Folder {
storage_path,
path_string,
parent_id,
owner_id,
// In-memory constructor: callers that don't supply a
// drive_id are by definition stub/legacy paths (tests,
// pre-D0 fixtures, DTO round-trips). Real DB-backed
// folders flow through `with_timestamps_and_tree`.
drive_id: Uuid::nil(),
created_at: now,
modified_at: now,
tree_modified_at: now,
// Provenance is unknown for in-memory construction; the DB
// reconstruction path supplies real values.
created_by: None,
updated_by: None,
})
@@ -160,34 +141,6 @@ impl Folder {
name,
storage_path,
parent_id,
None,
Uuid::nil(),
created_at,
modified_at,
modified_at,
)
}
/// Creates a folder with specific timestamps and owner (legacy
/// constructor — `tree_modified_at` defaults to `modified_at`).
/// Prefer [`Folder::with_timestamps_and_tree`] for DB reconstruction
/// so the rollup ETag reflects descendant activity, not just this
/// row's own metadata.
pub fn with_timestamps_and_owner(
id: String,
name: String,
storage_path: StoragePath,
parent_id: Option<String>,
owner_id: Option<Uuid>,
created_at: u64,
modified_at: u64,
) -> FolderResult<Self> {
Self::with_timestamps_and_tree(
id,
name,
storage_path,
parent_id,
owner_id,
Uuid::nil(),
created_at,
modified_at,
@@ -209,7 +162,6 @@ impl Folder {
name: String,
storage_path: StoragePath,
parent_id: Option<String>,
owner_id: Option<Uuid>,
drive_id: Uuid,
created_at: u64,
modified_at: u64,
@@ -220,7 +172,6 @@ impl Folder {
name,
storage_path,
parent_id,
owner_id,
drive_id,
created_at,
modified_at,
@@ -239,7 +190,6 @@ impl Folder {
name: String,
storage_path: StoragePath,
parent_id: Option<String>,
owner_id: Option<Uuid>,
drive_id: Uuid,
created_at: u64,
modified_at: u64,
@@ -260,7 +210,6 @@ impl Folder {
storage_path,
path_string,
parent_id,
owner_id,
drive_id,
created_at,
modified_at,
@@ -299,10 +248,6 @@ impl Folder {
self.modified_at
}
pub fn owner_id(&self) -> Option<Uuid> {
self.owner_id
}
/// Drive that owns this folder. Path-based lookups scope by
/// this axis (post-D0 invariant: `storage.folders.drive_id`
/// is `NOT NULL`).
@@ -412,7 +357,6 @@ impl Folder {
storage_path,
path_string: path,
parent_id,
owner_id: None,
// DTO round-trips lose drive_id (FolderDto carries it,
// but the legacy `from_dto` signature predates this
// change). Callers that need real scoping must reload
@@ -460,7 +404,6 @@ impl Folder {
storage_path: new_storage_path,
path_string: new_path_string,
parent_id: self.parent_id.clone(),
owner_id: self.owner_id,
drive_id: self.drive_id,
created_at: self.created_at,
modified_at: now,
@@ -501,7 +444,6 @@ impl Folder {
storage_path: new_storage_path,
path_string: new_path_string,
parent_id,
owner_id: self.owner_id,
drive_id: self.drive_id,
created_at: self.created_at,
modified_at: now,
@@ -593,7 +535,6 @@ mod tests {
"folder".to_string(),
StoragePath::from_string("/folder"),
None,
None,
Uuid::nil(),
1_000,
2_000,
@@ -615,7 +556,6 @@ mod tests {
"a".to_string(),
StoragePath::from_string("/a"),
None,
None,
Uuid::nil(),
0,
0,
@@ -627,7 +567,6 @@ mod tests {
"b".to_string(),
StoragePath::from_string("/b"),
None,
None,
Uuid::nil(),
0,
0,
@@ -650,7 +589,6 @@ mod tests {
"folder".to_string(),
StoragePath::from_string("/folder"),
None,
None,
Uuid::nil(),
1_000,
2_000,
@@ -662,7 +600,6 @@ mod tests {
"folder".to_string(),
StoragePath::from_string("/folder"),
None,
None,
Uuid::nil(),
1_000,
2_000,
+28
View File
@@ -118,6 +118,34 @@ impl Resource {
_ => None,
}
}
/// Parse `(item_type, item_id)` from an API-facing pair of strings
/// (favorites, recent, batch endpoints all take this shape).
/// Combines UUID parse + type mapping so callers stay one-line and
/// error shapes are identical across surfaces. Returns
/// `DomainError::new(InvalidInput, …)` on malformed input; callers
/// that need the anti-enum 404 shape do that separately by feeding
/// the parsed `Resource` into `authz.require(...)`.
pub fn parse(
item_type: &str,
item_id: &str,
) -> Result<Self, crate::common::errors::DomainError> {
use crate::common::errors::{DomainError, ErrorKind};
let uuid = Uuid::parse_str(item_id).map_err(|_| {
DomainError::new(
ErrorKind::InvalidInput,
"Resource",
format!("Invalid item UUID '{item_id}'"),
)
})?;
Self::from_parts(item_type, uuid).ok_or_else(|| {
DomainError::new(
ErrorKind::InvalidInput,
"Resource",
format!("Unsupported item type '{item_type}'"),
)
})
}
}
impl fmt::Display for Resource {
@@ -397,8 +397,6 @@ impl FileBlobReadRepository {
}
}
/// Post-D7-step-6: `storage.files.user_id` dropped; the entity's
/// legacy `user_id` field is populated with `None` here.
#[allow(clippy::too_many_arguments)]
fn row_to_file(
id: String,
@@ -423,7 +421,6 @@ impl FileBlobReadRepository {
folder_id,
created_at as u64,
modified_at as u64,
None, // Post-D7: `files.user_id` column dropped.
blob_hash,
created_by,
updated_by,
@@ -104,7 +104,6 @@ impl FileBlobWriteRepository {
mime_type: String,
created_at: i64,
modified_at: i64,
owner_id: Option<Uuid>,
blob_hash: String,
created_by: Option<Uuid>,
updated_by: Option<Uuid>,
@@ -119,7 +118,6 @@ impl FileBlobWriteRepository {
folder_id,
created_at as u64,
modified_at as u64,
owner_id,
blob_hash,
created_by,
updated_by,
@@ -399,7 +397,6 @@ impl FileBlobWriteRepository {
content_type,
created_at,
updated_at,
None, // Post-D7: `files.user_id` no longer written on new rows.
blob_hash.to_string(),
created_by,
updated_by,
@@ -477,7 +474,6 @@ impl FileBlobWriteRepository {
mime_type,
created_at,
updated_at,
None,
blob_hash.to_string(),
created_by,
updated_by,
@@ -562,7 +558,6 @@ impl FileWritePort for FileBlobWriteRepository {
row.4,
row.5,
row.6,
None,
String::new(),
row.7,
row.8,
@@ -710,7 +705,6 @@ impl FileWritePort for FileBlobWriteRepository {
row.4,
row.5,
row.6,
None,
row.7,
row.8,
row.9,
@@ -773,7 +767,6 @@ impl FileWritePort for FileBlobWriteRepository {
row.4,
row.5,
row.6,
None,
String::new(),
row.7,
row.8,
@@ -874,7 +867,6 @@ impl FileWritePort for FileBlobWriteRepository {
content_type,
row.1,
row.2,
None, // Post-D7: `files.user_id` no longer written on new rows.
String::new(),
row.3,
row.4,
@@ -129,11 +129,6 @@ impl FolderDbRepository {
/// extra queries needed. `created_by` / `updated_by` carry the
/// §14 provenance signal through the entity layer; both are
/// `Option<Uuid>` because the FK is `ON DELETE SET NULL`.
///
/// Post-D7-step-6: the `storage.folders.user_id` column is gone;
/// the entity's legacy `user_id` field is populated with `None`
/// at construction time (removed in the follow-up entity
/// cleanup PR).
#[allow(clippy::too_many_arguments)]
fn row_to_folder(
id: String,
@@ -153,7 +148,6 @@ impl FolderDbRepository {
name,
storage_path,
parent_id,
None, // Post-D7: `folders.user_id` column dropped.
drive_id,
created_at as u64,
modified_at as u64,
@@ -29,7 +29,6 @@ use std::time::Duration;
use moka::sync::Cache;
use uuid::Uuid;
use crate::application::ports::recent_ports::RecentItemsUseCase;
use crate::application::ports::resource_access_hook::ResourceAccessHook;
use crate::application::services::recent_service::RecentService;
@@ -85,7 +84,16 @@ impl ResourceAccessHook for RecentRecordingHook {
let recent = Arc::clone(&self.recent);
let (caller_id, file_id) = key;
tokio::spawn(async move {
if let Err(e) = recent.record_item_access(caller_id, &file_id, "file").await {
// Fast path: skip the trait's `authz.require(Read, …)`
// (upstream `_with_perms` service already gated). The
// extra SQL round-trip pushes the upsert past the client's
// immediate `GET /api/recent/resources` in
// `tests/api/recent.hurl` step 7 — the whole reason for
// the internal variant.
if let Err(e) = recent
.record_item_access_internal(caller_id, &file_id, "file")
.await
{
tracing::warn!(
target: "oxicloud::recent",
caller_id = %caller_id,
@@ -6,7 +6,7 @@ use axum::{
};
use serde::Deserialize;
use std::sync::Arc;
use tracing::{error, info};
use tracing::info;
use utoipa::ToSchema;
use crate::application::dtos::display_helpers::{
@@ -66,7 +66,8 @@ pub async fn add_favorite(
Json(serde_json::json!({
"error": "Item type must be 'file' or 'folder'"
})),
);
)
.into_response();
}
match favorites_service
@@ -81,16 +82,14 @@ pub async fn add_favorite(
"message": "Item added to favorites"
})),
)
.into_response()
}
Err(err) => {
error!("Error adding to favorites: {}", err);
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({
"error": "Failed to add to favorites"
})),
)
}
// Route through AppError so the `DomainError::kind` maps to the
// right status code (NotFound → 404 anti-enum for the pre-write
// authz gate, InvalidInput → 400 for a malformed UUID, etc.).
// A hardcoded 500 here would mask the 404 the Round 1 AuthZ
// fix relies on.
Err(err) => AppError::from(err).into_response(),
}
}
@@ -129,6 +128,7 @@ pub async fn remove_favorite(
"message": "Item removed from favorites"
})),
)
.into_response()
} else {
info!("Item {} '{}' was not in favorites", item_type, item_id);
(
@@ -137,17 +137,12 @@ pub async fn remove_favorite(
"message": "Item was not in favorites"
})),
)
.into_response()
}
}
Err(err) => {
error!("Error removing from favorites: {}", err);
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({
"error": "Failed to remove from favorites"
})),
)
}
// Same rationale as `add_favorite` — preserve DomainError→HTTP
// status mapping instead of collapsing every error to 500.
Err(err) => AppError::from(err).into_response(),
}
}
@@ -347,15 +342,10 @@ pub async fn batch_add_favorites(
);
(StatusCode::OK, Json(serde_json::json!(result))).into_response()
}
Err(err) => {
error!("Error in batch add favorites: {}", err);
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({
"error": "Failed to batch add favorites"
})),
)
.into_response()
}
// Preserve DomainError→HTTP status mapping — the Round 1
// AuthZ fix relies on a per-item NotFound propagating out
// of the batch. A hardcoded 500 would mask the 404 that
// signals a cross-tenant probe.
Err(err) => AppError::from(err).into_response(),
}
}
+11 -31
View File
@@ -5,7 +5,7 @@ use axum::{
response::IntoResponse,
};
use std::sync::Arc;
use tracing::{error, info};
use tracing::info;
use crate::application::dtos::display_helpers::{
category_for, format_file_size, icon_class_for, icon_special_class_for,
@@ -70,16 +70,10 @@ pub async fn record_item_access(
)
.into_response()
}
Err(err) => {
error!("Error recording access in recents: {}", err);
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({
"error": "Failed to record access"
})),
)
.into_response()
}
// Preserve DomainError→HTTP status mapping — the Round 1
// AuthZ fix relies on the NotFound from `authz.require`
// propagating as 404 (anti-enum), not being masked as 500.
Err(err) => AppError::from(err).into_response(),
}
}
@@ -130,16 +124,9 @@ pub async fn remove_from_recent(
.into_response()
}
}
Err(err) => {
error!("Error removing from recents: {}", err);
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({
"error": "Failed to remove from recents"
})),
)
.into_response()
}
// Same rationale as `record_item_access` — preserve the
// DomainError→HTTP mapping instead of collapsing to 500.
Err(err) => AppError::from(err).into_response(),
}
}
@@ -170,16 +157,9 @@ pub async fn clear_recent_items(
)
.into_response()
}
Err(err) => {
error!("Error clearing recent items: {}", err);
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({
"error": "Failed to clear recent items"
})),
)
.into_response()
}
// Same rationale as `record_item_access` — preserve the
// DomainError→HTTP mapping instead of collapsing to 500.
Err(err) => AppError::from(err).into_response(),
}
}
@@ -1756,7 +1756,7 @@ async fn handle_put(
// internally via its `_with_perms` shape.
let content_type = ingested.content_type.clone();
let result = file_upload_service
.update_file_streaming(
.update_file_streaming_with_perms(
&path,
drive_id,
ingested.stored(),
+175 -15
View File
@@ -20,10 +20,13 @@ use axum::{
use serde::{Deserialize, Serialize};
use std::sync::Arc;
use crate::application::ports::authorization_ports::AuthorizationEngine;
use crate::application::ports::file_ports::{FileRetrievalUseCase, FileUploadUseCase};
use crate::application::services::wopi_lock_service::WopiLockService;
use crate::application::services::wopi_token_service::WopiTokenService;
use crate::domain::repositories::drive_repository::DriveRepository;
use crate::domain::services::authorization::{Permission, Resource, Subject};
use crate::infrastructure::services::pg_acl_engine::PgAclEngine;
use crate::infrastructure::services::wopi_discovery_service::WopiDiscoveryService;
/// Shared state for WOPI handlers.
@@ -64,6 +67,37 @@ pub struct CheckFileInfoResponse {
pub close_url: String,
}
/// Enforce that the WOPI caller (`claims.sub`) still has `perm` on the
/// file at redemption time — not just at token-mint time.
///
/// **Why every verb needs this.** WOPI tokens are validated locally
/// (HMAC over claims), so a token that was legitimately minted stays
/// verify-able until its TTL. If a grant is revoked after mint, or the
/// token was minted for view but is used to POST content, the token's
/// signature alone doesn't catch it. This helper re-checks against the
/// live authorization engine on every verb — the memory note
/// `wopi-authz-bypass` calls out the class of bugs this fences.
///
/// Returns 404 (anti-enumeration — same shape as "file doesn't exist")
/// on both bad UUID and authorization denial. The engine emits a
/// structured `audit` line on denial internally, so ops sees the real
/// reason without the attacker being able to distinguish "gone" from
/// "revoked".
async fn require_wopi_perm(
authz: &PgAclEngine,
caller_sub: &str,
file_id: &str,
perm: Permission,
) -> Result<(uuid::Uuid, uuid::Uuid), StatusCode> {
let caller_uuid = uuid::Uuid::parse_str(caller_sub).map_err(|_| StatusCode::UNAUTHORIZED)?;
let file_uuid = uuid::Uuid::parse_str(file_id).map_err(|_| StatusCode::NOT_FOUND)?;
authz
.require(Subject::User(caller_uuid), perm, Resource::File(file_uuid))
.await
.map_err(|_| StatusCode::NOT_FOUND)?;
Ok((caller_uuid, file_uuid))
}
/// GET /wopi/files/{file_id} — CheckFileInfo
async fn check_file_info(
Path(file_id): Path<String>,
@@ -82,6 +116,19 @@ async fn check_file_info(
return StatusCode::UNAUTHORIZED.into_response();
}
// Redemption-time authz: even with a valid token, the caller must
// still hold Read on this file. Catches revoked-grant-mid-session.
if let Err(status) = require_wopi_perm(
state.app_state.authorization.as_ref(),
&claims.sub,
&file_id,
Permission::Read,
)
.await
{
return status.into_response();
}
// Fetch file metadata
let file = match state
.app_state
@@ -99,6 +146,24 @@ async fn check_file_info(
.map(|dt| dt.to_rfc3339())
.unwrap_or_default();
// `user_can_write` = actual current Update permission ∧ token's
// can_write flag. If the caller's Update was revoked since the
// token was minted (e.g. their grant was downgraded from Editor
// to Viewer), the editor sees the file as read-only and won't
// even attempt PutFile. The stricter `require_wopi_perm(Update)`
// in put_file is the actual gate; this field is a UI hint.
let can_write_now = claims.can_write
&& state
.app_state
.authorization
.check(
Subject::User(uuid::Uuid::parse_str(&claims.sub).unwrap_or(uuid::Uuid::nil())),
Permission::Update,
Resource::File(uuid::Uuid::parse_str(&file_id).unwrap_or(uuid::Uuid::nil())),
)
.await
.unwrap_or(false);
let response = CheckFileInfoResponse {
base_file_name: file.name.clone(),
// WOPI's `OwnerId` field is required. Post-D7 the DTO no
@@ -112,9 +177,9 @@ async fn check_file_info(
user_id: claims.sub.clone(),
version: file.modified_at.to_string(),
supports_locks: true,
supports_update: claims.can_write,
supports_update: can_write_now,
supports_rename: false,
user_can_write: claims.can_write,
user_can_write: can_write_now,
user_friendly_name: claims.username.clone(),
post_message_origin: state.public_base_url.clone(),
last_modified_time: last_modified,
@@ -145,6 +210,18 @@ async fn get_file(
return StatusCode::UNAUTHORIZED.into_response();
}
// Redemption-time authz — see require_wopi_perm docstring.
if let Err(status) = require_wopi_perm(
state.app_state.authorization.as_ref(),
&claims.sub,
&file_id,
Permission::Read,
)
.await
{
return status.into_response();
}
match state
.app_state
.applications
@@ -184,6 +261,21 @@ async fn put_file(
return StatusCode::UNAUTHORIZED.into_response();
}
// Redemption-time authz: the token says the caller could write when
// it was minted, but Update permission may have been revoked since.
// Re-check now so a stale write-capable token can't survive a
// downgrade / share removal / drive-membership change until its TTL.
if let Err(status) = require_wopi_perm(
state.app_state.authorization.as_ref(),
&claims.sub,
&file_id,
Permission::Update,
)
.await
{
return status.into_response();
}
// Check lock
let request_lock = headers
.get("X-WOPI-Lock")
@@ -264,7 +356,7 @@ async fn put_file(
.app_state
.applications
.file_upload_service
.update_file_streaming(
.update_file_streaming_with_perms(
&file.path,
drive_id,
ingested.stored(),
@@ -302,6 +394,22 @@ async fn file_operations(
return StatusCode::UNAUTHORIZED.into_response();
}
// Every lock op mutates shared state (LOCK / UNLOCK / REFRESH_LOCK
// change the lock; GET_LOCK reads it but the read is only useful
// to a caller who could subsequently take a write action — so gate
// on Update uniformly rather than splitting per-op). A Viewer with
// a stale token must not be able to hold or contend for a lock.
if let Err(status) = require_wopi_perm(
state.app_state.authorization.as_ref(),
&claims.sub,
&file_id,
Permission::Update,
)
.await
{
return status.into_response();
}
let override_header = headers
.get("X-WOPI-Override")
.and_then(|v| v.to_str().ok())
@@ -374,25 +482,71 @@ pub struct EditorUrlResponse {
pub access_token_ttl: i64,
}
/// Determines if `caller_id` can access `file_id` and with what permissions.
/// Resolve the WOPI mint target: gate on real permissions and derive
/// the `can_write` flag from the caller's ACTUAL Update rights.
///
/// Uses the SQL-level ownership check (`get_file_owned`) so that files
/// belonging to other users — or non-existent files — both return `NOT_FOUND`,
/// avoiding existence-leak oracles.
/// Prior behaviour used a naive `requested_action != "view"` heuristic
/// so a Viewer clicking "Edit in Collabora" received a write-capable
/// token, promoting themselves to Editor for the token's TTL. The
/// memory note `wopi-authz-bypass` fix #12 calls this out explicitly.
///
/// Returns `(FileDto, can_write)` on success.
/// Contract:
///
/// 1. **Read** is the bar to open the file in any mode. If the caller
/// has no Read grant, return 404 (anti-enum — same shape as "no such
/// file").
/// 2. **Update** determines the returned `can_write` bit — INDEPENDENT
/// of what the client's `requested_action` said. A Viewer who
/// requested `action=edit` gets `can_write=false` and Collabora
/// opens in view mode; the token stays authorised for view-only
/// ops and put_file will 404 at redemption regardless.
/// 3. `requested_action == "view"` is respected as a downgrade — an
/// Editor can explicitly request view mode (co-browsing a doc
/// without accidentally editing) and get `can_write=false`.
///
/// The `PgAclEngine::require`/`check` calls emit structured audit
/// lines on denial (`authz.denied` event), so a Viewer's "edit"
/// attempt shows up in the audit stream as a rejected Update check.
async fn authorize_wopi_access<S: FileRetrievalUseCase>(
authz: &PgAclEngine,
file_retrieval: &S,
file_id: &str,
caller_id: uuid::Uuid,
requested_action: &str,
) -> Result<(crate::application::dtos::file_dto::FileDto, bool), StatusCode> {
let file = file_retrieval
.get_file_with_perms(file_id, caller_id)
let file_uuid = uuid::Uuid::parse_str(file_id).map_err(|_| StatusCode::NOT_FOUND)?;
// Step 1 — Read is required to even open the file.
authz
.require(
Subject::User(caller_id),
Permission::Read,
Resource::File(file_uuid),
)
.await
.map_err(|_| StatusCode::NOT_FOUND)?;
// Owner verified — grant write unless explicitly requesting view-only.
let can_write = requested_action != "view";
let file = file_retrieval
.get_file(file_id)
.await
.map_err(|_| StatusCode::NOT_FOUND)?;
// Step 2 — can_write reflects real Update, not the client's
// action-string. `check` returns bool without throwing; failure
// just means the caller lacks Update, so we degrade the token to
// read-only. Deliberately no `require` here — a Viewer opening
// the file is legitimate; only the write claim is suppressed.
let has_update = authz
.check(
Subject::User(caller_id),
Permission::Update,
Resource::File(file_uuid),
)
.await
.unwrap_or(false);
// Step 3 — allow explicit view-mode downgrade for Editors.
let can_write = has_update && requested_action != "view";
Ok((file, can_write))
}
@@ -409,6 +563,7 @@ pub async fn get_editor_url(
let username = &auth_user.username;
// Verify the caller owns the file (SQL-level check, no existence leak).
let (file, can_write) = match authorize_wopi_access(
state.app_state.authorization.as_ref(),
state.app_state.applications.file_retrieval_service.as_ref(),
&params.file_id,
user_id,
@@ -494,7 +649,8 @@ async fn host_page(
Ok(u) => u,
Err(_) => return StatusCode::UNAUTHORIZED.into_response(),
};
let file = match authorize_wopi_access(
let (file, can_write_now) = match authorize_wopi_access(
state.app_state.authorization.as_ref(),
state.app_state.applications.file_retrieval_service.as_ref(),
&file_id,
caller_uuid,
@@ -502,7 +658,7 @@ async fn host_page(
)
.await
{
Ok((f, _)) => f,
Ok((f, cw)) => (f, cw),
Err(status) => return status.into_response(),
};
@@ -519,11 +675,15 @@ async fn host_page(
_ => return StatusCode::INTERNAL_SERVER_ERROR.into_response(),
};
// Use the freshly-computed `can_write_now` (real Update permission
// ∧ requested_action) rather than the incoming token's `can_write`
// flag. Otherwise a Viewer who somehow reached this host page with
// a stale edit-capable token would get another one re-minted.
let (token, ttl) = match state.token_service.generate_token(
&file_id,
&claims.sub,
&claims.username,
claims.can_write,
can_write_now,
) {
Ok(t) => t,
Err(_) => return StatusCode::INTERNAL_SERVER_ERROR.into_response(),
+13 -11
View File
@@ -422,13 +422,17 @@ pub async fn handle_search(
let mut entries: Vec<serde_json::Value> = Vec::new();
// Map file results
// TODO(D1): drop the hardcoded "Personal/" prefix and read the
// caller's default-drive root folder name from `drives.root_folder_id`
// instead. Correct for D0-provisioned default drives; secondary
// drives keep their original root name.
// Map file results.
//
// `strip_drive_root_segment` handles both default and secondary
// drives — post-D0 the first path segment is the drive's root
// folder name (`"Personal"` for D0-provisioned defaults, the
// original sibling-root name for M2 backfilled secondaries).
// Read-scope is upstream in `state.applications.search_service`;
// this handler only formats display paths.
for file in &results.files {
let display_path = file.path.strip_prefix("Personal/").unwrap_or(&file.path);
let display_path =
crate::interfaces::nextcloud::webdav_handler::strip_drive_root_segment(&file.path);
let display_path = format!("/{}", display_path);
let numeric_id = file_id_map.get(&file.id).copied();
@@ -452,12 +456,10 @@ pub async fn handle_search(
}));
}
// Map folder results — same TODO(D1) as above.
// Map folder results — same drive-agnostic strip as above.
for folder in &results.folders {
let display_path = folder
.path
.strip_prefix("Personal/")
.unwrap_or(&folder.path);
let display_path =
crate::interfaces::nextcloud::webdav_handler::strip_drive_root_segment(&folder.path);
let display_path = format!("/{}", display_path);
entries.push(json!({
+77 -19
View File
@@ -62,6 +62,15 @@ async fn handle_filter_files(
) -> Result<Response<Body>, AppError> {
let user = &session.user;
let url_user = &session.raw_username;
// Chroot-scope the response: NC's `oc:filter-files` REPORT is a
// single-drive surface (the client PROPFINDs favorites under its
// "home" URL and has no cross-drive concept). Favorites that live
// in another drive the caller is a member of are dropped from
// this response; they're still reachable via REST
// `/api/favorites/resources`. `session.require_chroot()` is safe
// here — the REPORT verb only reaches this handler through a
// path-scoped route.
let chroot = session.require_chroot()?;
let fav_svc = match state.favorites_service.as_ref() {
Some(svc) => svc,
None => return Ok(empty_multistatus()),
@@ -84,11 +93,11 @@ async fn handle_filter_files(
// All items in this response are favorites.
let favorite_ids: HashSet<String> = favorites.iter().map(|f| f.item_id.clone()).collect();
// TODO(D1): replace the hardcoded "Personal/" prefix with the
// caller's default-drive root folder name read from
// `drives.root_folder_id`. Correct for D0-provisioned default
// drives; secondary drives keep their original root name.
let home_prefix = "Personal/";
// `home_prefix` is unused after the chroot-aware strip
// (see `strip_home_prefix`); kept as a positional argument in
// the emit calls below for signature stability with the
// report-handler tests and the parallel search-pass caller.
let home_prefix = "";
// Pass 1: resolve the favorited DTOs in two batch queries (was one
// get_* per favorite — up to N serial round-trips on a sync client's
@@ -155,7 +164,17 @@ async fn handle_filter_files(
// multi-drive `~{drive}` form is echoed back to the client;
// owner-id stays canonical via `&user.username`.
for file in &files {
let subpath = strip_home_prefix(&file.path, home_prefix);
// Skip favorites that live outside the caller's chroot
// (other-drive favorites); reachable via REST if needed.
let Some(subpath) = strip_home_prefix(chroot, &file.path, home_prefix) else {
tracing::debug!(
target: "oxicloud::nc",
"REPORT filter-files: dropping cross-chroot favorite '{}' at '{}'",
file.id,
file.path,
);
continue;
};
let href = nc_href(url_user, subpath);
let fid = file_id_map.get(&file.id).copied();
let oc_id = fid.map(|id| format_oc_id(id, file_id_svc));
@@ -172,7 +191,15 @@ async fn handle_filter_files(
}
for folder in &folders {
let subpath = strip_home_prefix(&folder.path, home_prefix);
let Some(subpath) = strip_home_prefix(chroot, &folder.path, home_prefix) else {
tracing::debug!(
target: "oxicloud::nc",
"REPORT filter-files: dropping cross-chroot favorite folder '{}' at '{}'",
folder.id,
folder.path,
);
continue;
};
let href = format!("{}/", nc_href(url_user, subpath));
let fid = folder_id_map.get(&folder.id).copied();
let oc_id = fid.map(|id| format_oc_id(id, file_id_svc));
@@ -207,9 +234,13 @@ async fn handle_search(
session: &crate::interfaces::nextcloud::session::NcSession,
) -> Result<Response<Body>, AppError> {
let user = &session.user;
// Validate chroot up-front (path-scoped handler); `resolve_scope_folder`
// below re-pulls it from the session for the path-mapping step.
session.require_chroot()?;
// Chroot-scope the response: NC's search REPORT is a single-drive
// surface. Results that live outside the chroot (other drives the
// caller is a member of) are dropped from the multistatus and
// recorded at debug — reachable via REST search if needed.
// `resolve_scope_folder` below re-pulls chroot from the session
// for the path-mapping step.
let chroot = session.require_chroot()?;
let url_user = &session.raw_username;
let search_svc = match state.applications.search_service.as_ref() {
Some(svc) => svc,
@@ -241,10 +272,9 @@ async fn handle_search(
let nc = state.nextcloud.as_ref();
let file_id_svc = nc.map(|n| &n.file_ids);
// TODO(D1): same as the favorites pass above — replace the
// hardcoded "Personal/" with the caller's actual default-drive
// root folder name from `drives.root_folder_id`.
let home_prefix = "Personal/";
// See the favorites pass above: `home_prefix` is unused after the
// chroot-aware strip, kept only for signature stability.
let home_prefix = "";
// No favorite checking for search results -- pass an empty set.
let favorite_ids: HashSet<String> = HashSet::new();
@@ -266,7 +296,15 @@ async fn handle_search(
// Files.
for file in &files {
let subpath = strip_home_prefix(&file.path, home_prefix);
let Some(subpath) = strip_home_prefix(chroot, &file.path, home_prefix) else {
tracing::debug!(
target: "oxicloud::nc",
"REPORT search: dropping cross-chroot file '{}' at '{}'",
file.id,
file.path,
);
continue;
};
let href = nc_href(url_user, subpath);
let fid = file_id_map.get(&file.id).copied();
let oc_id = fid.map(|id| format_oc_id(id, file_id_svc));
@@ -284,7 +322,15 @@ async fn handle_search(
// Folders.
for folder in &folders {
let subpath = strip_home_prefix(&folder.path, home_prefix);
let Some(subpath) = strip_home_prefix(chroot, &folder.path, home_prefix) else {
tracing::debug!(
target: "oxicloud::nc",
"REPORT search: dropping cross-chroot folder '{}' at '{}'",
folder.id,
folder.path,
);
continue;
};
let href = format!("{}/", nc_href(url_user, subpath));
let fid = folder_id_map.get(&folder.id).copied();
let oc_id = fid.map(|id| format_oc_id(id, file_id_svc));
@@ -545,7 +591,19 @@ fn extract_subpath_from_scope(href: &str, url_user: &str) -> Option<String> {
None
}
/// Strip the `My Folder - {username}/` prefix to get the DAV subpath.
fn strip_home_prefix<'a>(path: &'a str, prefix: &str) -> &'a str {
path.strip_prefix(prefix).unwrap_or(path)
/// Strip the caller's chroot prefix from an internal path so the
/// caller-facing DAV subpath is chroot-relative. Delegates to
/// `webdav_handler::strip_chroot_prefix` — chroot-aware, multi-segment
/// safe, and rejects items outside the chroot. Callers must decide
/// per-response whether an out-of-chroot item is dropped or falls
/// back to the naive strip.
///
/// See `strip_chroot_prefix` for the full contract. The `_prefix`
/// legacy arg stays for signature stability with the emit helpers.
fn strip_home_prefix<'a>(
chroot: &crate::application::dtos::folder_dto::FolderDto,
path: &'a str,
_prefix: &str,
) -> Option<&'a str> {
crate::interfaces::nextcloud::webdav_handler::strip_chroot_prefix(chroot, path)
}
+57 -15
View File
@@ -81,6 +81,14 @@ async fn handle_propfind(
session: &crate::interfaces::nextcloud::session::NcSession,
) -> Result<Response<Body>, AppError> {
let user = &session.user;
// Chroot-scope the trashbin view: `get_trash_items(user.id)`
// spans every drive the caller is a member of, but NC's
// trashbin surface is a single-drive concept from the client's
// POV. Items outside the chroot are dropped from the multistatus
// (see `write_trashbin_multistatus` → `strip_home_prefix` →
// `webdav_handler::strip_chroot_prefix`) and remain reachable
// via REST `/api/trash/resources`.
let chroot = session.require_chroot()?;
let trash_svc = state
.trash_service
.as_ref()
@@ -95,7 +103,7 @@ async fn handle_propfind(
let file_id_svc = nc.map(|n| &n.file_ids);
let mut buf = Vec::new();
write_trashbin_multistatus(&mut buf, &items, &user.username, file_id_svc)
write_trashbin_multistatus(&mut buf, &items, &user.username, chroot, file_id_svc)
.await
.map_err(|e| AppError::internal_error(format!("XML generation failed: {}", e)))?;
@@ -259,18 +267,23 @@ fn mime_from_name(name: &str) -> String {
.to_string()
}
/// Strip the home-folder prefix from an original path to produce the
/// Nextcloud-relative original location.
/// Strip the caller's chroot prefix from an original path to produce
/// the Nextcloud-relative original-location value.
///
/// TODO(D1): replace the hardcoded "Personal/" with the caller's actual
/// default-drive root folder name read from `drives.root_folder_id`.
/// Correct for D0-provisioned default drives; secondary drives keep
/// their original root name. The `_username` arg stays for now so the
/// upcoming dynamic lookup has a way to identify the caller.
fn strip_home_prefix<'a>(original_path: &'a str, _username: &str) -> &'a str {
original_path
.strip_prefix("Personal/")
.unwrap_or(original_path)
/// Delegates to `webdav_handler::strip_chroot_prefix` — chroot-aware,
/// multi-segment safe, and returns `None` when the item is outside
/// the chroot (e.g. a trashed item in another drive the caller is a
/// member of). The `_username` arg stays for signature stability
/// with call sites that thread it; the strip itself no longer uses it.
///
/// See the doc on `strip_chroot_prefix` for the AuthZ caveat — this
/// is a display helper, not an ownership check.
fn strip_home_prefix<'a>(
original_path: &'a str,
_username: &str,
chroot: &crate::application::dtos::folder_dto::FolderDto,
) -> Option<&'a str> {
crate::interfaces::nextcloud::webdav_handler::strip_chroot_prefix(chroot, original_path)
}
// ────────────── Trashbin PROPFIND XML Generation ──────────────
@@ -280,10 +293,16 @@ use crate::application::services::nextcloud_file_id_service::NextcloudFileIdServ
use std::collections::HashMap;
/// Generate a complete Nextcloud-compatible multistatus XML response for the trashbin.
///
/// `chroot` scopes the response — items whose original path is outside
/// the chroot (other drives the caller is a member of) are dropped
/// silently. NC's trashbin surface is single-drive from the client's
/// perspective; cross-drive items remain reachable via REST.
async fn write_trashbin_multistatus<W: std::io::Write>(
writer: W,
items: &[TrashedItemDto],
username: &str,
chroot: &crate::application::dtos::folder_dto::FolderDto,
file_id_svc: Option<&Arc<NextcloudFileIdService>>,
) -> Result<(), String> {
let mut xml = Writer::new(writer);
@@ -315,9 +334,24 @@ async fn write_trashbin_multistatus<W: std::io::Write>(
batch_resolve_ids(file_id_svc, &file_uuids, &folder_uuids).await;
id_map.extend(folder_id_map);
// Individual trashed items.
// Individual trashed items — skip those whose original path is
// outside the chroot (other-drive trash reachable via REST).
for item in items {
write_trash_item_response(&mut xml, item, username, file_id_svc, &id_map)?;
if crate::interfaces::nextcloud::webdav_handler::strip_chroot_prefix(
chroot,
&item.original_path,
)
.is_none()
{
tracing::debug!(
target: "oxicloud::nc",
"trashbin PROPFIND: dropping cross-chroot item '{}' at '{}'",
item.id,
item.original_path,
);
continue;
}
write_trash_item_response(&mut xml, item, username, chroot, file_id_svc, &id_map)?;
}
xml.write_event(Event::End(BytesEnd::new("d:multistatus")))
@@ -363,10 +397,18 @@ fn write_trash_root_response<W: std::io::Write>(
}
/// Write a single trashed item as a `<d:response>` element.
///
/// Caller is expected to have already verified the item is inside
/// `chroot` — see the guard in `write_trashbin_multistatus`. This
/// function trusts the invariant and expects `strip_home_prefix` to
/// return `Some(_)`; if it ever returns `None` (chroot drift between
/// the guard and the emit, defensive-only), the original-location
/// falls back to an empty string.
fn write_trash_item_response<W: std::io::Write>(
xml: &mut Writer<W>,
item: &TrashedItemDto,
username: &str,
chroot: &crate::application::dtos::folder_dto::FolderDto,
file_id_svc: Option<&Arc<NextcloudFileIdService>>,
id_map: &HashMap<String, i64>,
) -> Result<(), String> {
@@ -427,7 +469,7 @@ fn write_trash_item_response<W: std::io::Write>(
write_text_element(xml, "nc:trashbin-filename", &item.name)?;
// nc:trashbin-original-location
let original_location = strip_home_prefix(&item.original_path, username);
let original_location = strip_home_prefix(&item.original_path, username, chroot).unwrap_or("");
write_text_element(xml, "nc:trashbin-original-location", original_location)?;
// nc:trashbin-deletion-time
+11 -8
View File
@@ -367,12 +367,14 @@ async fn handle_assemble(
let chroot = session.require_chroot()?;
let drive_id = chroot.drive_id;
// TODO(D1): read the caller's default-drive root folder name from
// `drives.root_folder_id` instead of hardcoding "Personal". The
// constant is correct for every default personal drive provisioned
// by the D0 lifecycle hook, but secondary drives (M2 backfill from
// SQL-created sibling root folders) keep their original name.
let internal_path = format!("Personal/{}", dest_subpath.trim_matches('/'));
// Route through `nc_to_internal_path(chroot, …)` so the write
// lands under the caller's actual default-drive root (not the
// literal "Personal" folder). Post-D3 chroot resolution puts the
// correct FolderDto — including the drive's real root name — on
// the NcSession; secondary drives with SQL-provisioned sibling
// root names now work.
let internal_path =
crate::interfaces::nextcloud::webdav_handler::nc_to_internal_path(chroot, &dest_subpath)?;
let filename = filename_from_path(&dest_subpath).to_string();
let ingested = ingest_stream_to_cas(
@@ -393,7 +395,7 @@ async fn handle_assemble(
let etag: Option<String> = if existing.is_ok() {
let dto = upload_service
.update_file_streaming(
.update_file_streaming_with_perms(
&internal_path,
drive_id,
ingested.stored(),
@@ -412,7 +414,8 @@ async fn handle_assemble(
Some((p, n)) => (p, n),
None => ("", dest_subpath.as_str()),
};
let parent_internal = format!("Personal/{}", parent_sub.trim_matches('/'));
let parent_internal =
crate::interfaces::nextcloud::webdav_handler::nc_to_internal_path(chroot, parent_sub)?;
let parent_internal = parent_internal.trim_end_matches('/');
use crate::application::ports::folder_ports::FolderUseCase;
+159 -1
View File
@@ -80,6 +80,78 @@ pub fn nc_to_internal_path(chroot: &FolderDto, subpath: &str) -> Result<String,
Ok(format!("{}/{}", chroot.path, subpath))
}
/// Strip the caller's chroot prefix from an internal
/// `storage.folders.path` so the DAV subpath surfaced to the NC
/// client is chroot-relative. Handles multi-segment chroots
/// correctly (e.g. a future `"Personal/folderA/subfolder"` chroot
/// against an item at `"Personal/folderA/subfolder/file.txt"`
/// returns `"file.txt"`, not `"folderA/subfolder/file.txt"`).
///
/// Returns `None` when the path is NOT inside the chroot. Callers
/// should skip such items from the response (they belong to a
/// different drive or the caller's read scope has drifted) — do NOT
/// fall back to a naive segment strip, which would surface a
/// misleading display path.
///
/// **Defensive but not an AuthZ boundary.** Every current caller
/// reaches items through a `_with_perms` method upstream that
/// already gates Read; this helper is the display-string layer
/// that also serves as a "does this item belong under the chroot"
/// sanity check.
pub fn strip_chroot_prefix<'a>(chroot: &FolderDto, internal_path: &'a str) -> Option<&'a str> {
// Normalize both sides: `FolderDto.path` comes from
// `StoragePath::to_string()` which prepends a leading `/`
// (e.g. `"/Personal"`), but DB-side paths coming from
// `storage.folders.path` (composed by the `compute_folder_path`
// trigger) never have a leading slash. Trim both so `"/Personal"`
// vs `"Personal/g9-tree"` matches the intended prefix.
let root = chroot.path.trim_matches('/');
if root.is_empty() {
// Guard against a mis-set chroot with an empty root path —
// stripping "" from anything would return the whole path.
return None;
}
let path = internal_path.trim_start_matches('/');
let rest = path.strip_prefix(root)?;
// Reject a partial prefix match — a chroot of "Personal" must
// not match an item at "PersonalSecrets/…".
match rest.strip_prefix('/') {
Some(subpath) => Some(subpath),
// Item path equals the chroot exactly — the chroot itself
// (i.e. a folder) is not a legitimate response item, so
// treat as an empty subpath.
None if rest.is_empty() => Some(""),
None => None,
}
}
/// Naive fallback: strip the first path segment from an internal
/// `storage.folders.path`. Post-D0 every path starts with its drive's
/// root folder name (single segment), so for the current schema this
/// gives the drive-relative subpath.
///
/// Use this ONLY when the caller doesn't have a chroot in scope
/// (e.g. OCS unified search, whose results legitimately span every
/// drive the caller has Read on — no single chroot covers them all).
/// Every path-scoped NC handler that DOES have `session` in scope
/// should prefer [`strip_chroot_prefix`] — it validates the item
/// belongs under the chroot instead of trusting the schema
/// invariant, and it survives a future composed chroot like
/// `"Personal/folderA/subfolder"`.
///
/// **Not an AuthZ boundary.** Same caveat as `strip_chroot_prefix`
/// — AuthZ is enforced upstream via `_with_perms` methods; this
/// helper only formats display strings.
///
/// Returns `""` when the path is a single segment (i.e. the drive
/// root itself, which is never a legitimate item target).
pub fn strip_drive_root_segment(internal_path: &str) -> &str {
match internal_path.split_once('/') {
Some((_root, rest)) => rest,
None => "",
}
}
/// 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
@@ -871,7 +943,7 @@ async fn handle_put(
// Single streaming path — handles both update and create internally,
// swapping the file row onto the already-ingested blob.
let stored = upload_service
.update_file_streaming(
.update_file_streaming_with_perms(
&internal_path,
chroot.drive_id,
ingested.stored(),
@@ -1873,6 +1945,92 @@ mod tests {
);
}
// ── strip_chroot_prefix ──
//
// Regression guard for the "chroot.path has a leading slash from
// StoragePath::to_string() but DB-side original_path doesn't" trap
// that broke the NC trashbin PROPFIND after Round 2 rolled out.
// Also pins the composed-chroot behaviour Ed asked about.
#[test]
fn strip_chroot_prefix_default_drive_root() {
// FolderDto.path carries a leading slash (StoragePath Display);
// DB paths do not. Both must normalise to the same prefix.
let chroot = stub_folder("/Personal");
assert_eq!(
strip_chroot_prefix(&chroot, "Personal/g9-tree"),
Some("g9-tree")
);
}
#[test]
fn strip_chroot_prefix_deep_path() {
let chroot = stub_folder("/Personal");
assert_eq!(
strip_chroot_prefix(&chroot, "Personal/inner/deep.txt"),
Some("inner/deep.txt")
);
}
#[test]
fn strip_chroot_prefix_out_of_chroot_returns_none() {
// Items on a different drive (whose root isn't "Personal")
// must NOT be surfaced under the caller's chroot.
let chroot = stub_folder("/Personal");
assert_eq!(strip_chroot_prefix(&chroot, "team-drive/report.pdf"), None);
}
#[test]
fn strip_chroot_prefix_rejects_partial_prefix_match() {
// "Personal" is a prefix substring of "PersonalSecrets" but
// NOT a path-segment prefix — must reject.
let chroot = stub_folder("/Personal");
assert_eq!(
strip_chroot_prefix(&chroot, "PersonalSecrets/foo.txt"),
None
);
}
#[test]
fn strip_chroot_prefix_composed_chroot() {
// The future composed-chroot case Ed raised: chroot points at
// a subfolder inside a drive. The strip must remove the ENTIRE
// composed prefix, not just the first segment.
let chroot = stub_folder("/Personal/folderA/subfolder");
assert_eq!(
strip_chroot_prefix(&chroot, "Personal/folderA/subfolder/foo.txt"),
Some("foo.txt")
);
}
#[test]
fn strip_chroot_prefix_composed_chroot_sibling_leaks_blocked() {
// Same composed chroot, but the item lives in a sibling
// subfolder — must be rejected, not naively strip 1 segment.
let chroot = stub_folder("/Personal/folderA/subfolder");
assert_eq!(
strip_chroot_prefix(&chroot, "Personal/folderA/other/foo.txt"),
None
);
}
#[test]
fn strip_chroot_prefix_chroot_root_itself() {
// Item path equals chroot exactly — legitimate for a PROPFIND
// Depth:0 on the chroot itself. Subpath is empty.
let chroot = stub_folder("/Personal");
assert_eq!(strip_chroot_prefix(&chroot, "Personal"), Some(""));
}
#[test]
fn strip_chroot_prefix_empty_chroot_returns_none() {
// Defensive: a mis-set chroot with an empty path must not
// strip anything (stripping "" from any path would return
// the whole path — a silent leak).
let chroot = stub_folder("/");
assert_eq!(strip_chroot_prefix(&chroot, "Personal/foo.txt"), None);
}
// ── nc_href ──
#[test]
+74 -3
View File
@@ -511,6 +511,26 @@ HTTP 200
jsonpath "$[*].id" contains {{team_drive_id}}
# ─────────────────────────────────────────────────────────────
# Step 21b — Upload gate by role (post-Drive AuthZ audit Round 2).
# Bob is Editor on team_drive; `POST /api/files/upload`
# targeting team_root_folder_id should succeed. This is
# the REST-side counterpart of the WebDAV/NC PUT chain
# hardened by `update_file_streaming_with_perms`. If
# this fails, the whole role-bundle → Permission::Create
# wiring is broken.
# ─────────────────────────────────────────────────────────────
POST {{base_url}}/api/files/upload
Authorization: Bearer {{bob_token}}
[MultipartFormData]
folder_id: {{team_root_folder_id}}
file: file,fixtures/hello.txt; text/plain
HTTP 201
[Captures]
bob_editor_upload_id: jsonpath "$.id"
# ─────────────────────────────────────────────────────────────
# Step 22 — Higher role wins: Bob now ALSO gets a Viewer direct
# grant (would lower his bundle). The collapsed caller_role
@@ -537,6 +557,50 @@ HTTP 200
jsonpath "$[*].id" contains {{team_drive_id}}
# ─────────────────────────────────────────────────────────────
# Step 22b — Viewer CANNOT upload into a shared drive.
# Post-Drive AuthZ audit Round 2: the create branch of
# `update_file_streaming_with_perms` requires
# `Permission::Create` on the parent folder — bundled
# with `owner`/`editor`/`contributor` role_grants only,
# NOT with `viewer`. `POST /api/files/upload` shares the
# same `save_file_with_blob` gate, so a Viewer probe
# must land 404 (anti-enum: same shape as no-such-folder)
# + `authz.denied` audit line. Also verify the batch /
# overwrite paths refuse — the whole chain from
# drive-membership to file write is exercised here.
# ─────────────────────────────────────────────────────────────
# 22b.i — Fresh file: 404.
POST {{base_url}}/api/files/upload
Authorization: Bearer {{bob_token}}
[MultipartFormData]
folder_id: {{team_root_folder_id}}
file: file,fixtures/hello.txt; text/plain
HTTP 404
# 22b.ii — Overwrite attempt on the Editor-era upload: still 404.
# `save_file_with_blob` catches the duplicate name at the
# `Create`-permission check before the upsert races (which
# would otherwise 409). The audit shape stays 404.
POST {{base_url}}/api/files/upload
Authorization: Bearer {{bob_token}}
[MultipartFormData]
folder_id: {{team_root_folder_id}}
file: file,fixtures/hello.txt; text/plain
HTTP 404
# 22b.iii — Alice's Editor-era file is untouched.
GET {{base_url}}/api/files/{{bob_editor_upload_id}}
Authorization: Bearer {{alice_token}}
HTTP 200
# =============================================================
# Per-role mutation matrix — what every role can / can't do
# =============================================================
@@ -845,15 +909,22 @@ HTTP 409
# 30c — Clear the lingering content (the Editor-created folder from
# Step 27). Delete via the regular folder endpoint so the row
# lands in trash, not the live tree; `is_empty` excludes
# trashed rows so a populated trash bin is allowed.
# Step 27 and the Editor-era file from Step 21b). Delete via
# the regular endpoints so rows land in trash, not the live
# tree; `is_empty` excludes trashed rows so a populated trash
# bin is allowed.
DELETE {{base_url}}/api/folders/{{editor_created_folder_id}}
Authorization: Bearer {{alice_token}}
HTTP 204
DELETE {{base_url}}/api/files/{{bob_editor_upload_id}}
Authorization: Bearer {{alice_token}}
HTTP 204
# 30d — Owner on an empty drive → 204.
DELETE {{base_url}}/api/drives/{{team_drive_id}}
Authorization: Bearer {{alice_token}}
+74
View File
@@ -159,3 +159,77 @@ Authorization: Bearer {{token}}
HTTP 200
[Asserts]
jsonpath "$.items" count == 0
# ─────────────────────────────────────────────────────────────
# Step 12 — Cross-tenant regression (post-Drive AuthZ audit,
# Round 1 HIGH). Before this fix, `POST /api/favorites/…`
# accepted any UUID and enrolled it; the listing endpoint
# then JOINed back to storage.files/folders and returned
# name/mime/size/drive_id for anything the caller had
# managed to add — an information oracle over the whole
# tenant. Now the write path calls `authz.require(Read, …)`
# per item; a caller with no grant gets 404 (anti-enum)
# + `authz.denied` audit line. See
# `docs/plan/authz_audit/rest_storage.md`.
# ─────────────────────────────────────────────────────────────
# Create a second, unprivileged user. Idempotent: `HTTP *` accepts
# either 201 (first run) or 409 (subsequent runs). The login below
# is the actual precondition — if it succeeds we know the user
# exists with the expected password.
POST {{base_url}}/api/admin/users
Authorization: Bearer {{token}}
Content-Type: application/json
{ "username": "fav_mallory", "password": "FavMalloryPassword1!", "email": "fav_mallory@example.com", "role": "user" }
HTTP *
POST {{base_url}}/api/auth/login
Content-Type: application/json
{ "username": "fav_mallory", "password": "FavMalloryPassword1!" }
HTTP 200
[Captures]
mallory_token: jsonpath "$.access_token"
# Step 12a — Single-add on admin's file: 404 (anti-enum shape).
POST {{base_url}}/api/favorites/file/{{file_id}}
Authorization: Bearer {{mallory_token}}
HTTP 404
# Step 12b — Single-add on admin's folder: 404.
POST {{base_url}}/api/favorites/folder/{{test1_id}}
Authorization: Bearer {{mallory_token}}
HTTP 404
# Step 12c — Batch: must fail wholesale on the first denial. A partial
# success would still leak "which items are valid" — the same
# oracle we're closing.
POST {{base_url}}/api/favorites/batch
Authorization: Bearer {{mallory_token}}
Content-Type: application/json
{
"items": [
{ "item_id": "{{file_id}}", "item_type": "file" },
{ "item_id": "{{test1_id}}", "item_type": "folder" }
]
}
HTTP 404
# Step 12d — Mallory's favorites list is EMPTY — no partial success
# slipped through.
GET {{base_url}}/api/favorites/resources
Authorization: Bearer {{mallory_token}}
HTTP 200
[Asserts]
jsonpath "$.items" count == 0
+79
View File
@@ -274,6 +274,85 @@ status >= 400
status < 500
# ─────────────────────────────────────────────────────────────
# 14b — Viewer-laundering regression (post-Drive AuthZ audit,
# Round 1 HIGH). Before the fix, `POST /api/shares` checked
# only "does the item exist" — any authenticated user who
# could name the UUID could mint a public Viewer link,
# laundering read access into a permanent anonymous URL
# that survived their own grant revocation. Now the
# service calls `authz.require(Share, resource)` before
# minting the token; a caller without `Share`
# (Viewer/Commenter/Contributor/no-grant-at-all) gets 404
# (anti-enum) + `authz.denied` audit line. See
# `docs/plan/authz_audit/admin_membership.md`.
#
# We test the strongest form: an unrelated user with no
# grant at all. The intermediate case (Viewer with Read
# but not Share) is covered by the same code path — Share
# is bundled only with owner/editor role_grants.
# ─────────────────────────────────────────────────────────────
# Create/lookup the attacker. Idempotent: `HTTP *` accepts either
# 201 (first run) or 409 (subsequent runs). Login below is the real
# precondition.
POST {{base_url}}/api/admin/users
Authorization: Bearer {{admin_token}}
Content-Type: application/json
{ "username": "sh_mallory", "password": "ShMalloryPassword1!", "email": "sh_mallory@example.com", "role": "user" }
HTTP *
POST {{base_url}}/api/auth/login
Content-Type: application/json
{ "username": "sh_mallory", "password": "ShMalloryPassword1!" }
HTTP 200
[Captures]
mallory_token: jsonpath "$.access_token"
# Step 14b.i — Mallory tries to mint a public share on admin's
# folder: 404 (anti-enum). No token appears in the
# response body.
POST {{base_url}}/api/shares
Authorization: Bearer {{mallory_token}}
Content-Type: application/json
{
"item_id": "{{share_folder_id}}",
"item_name": "public-share-test",
"item_type": "folder"
}
HTTP 404
# Step 14b.ii — Same attempt on admin's file: 404.
POST {{base_url}}/api/shares
Authorization: Bearer {{mallory_token}}
Content-Type: application/json
{
"item_id": "{{shared_file_id}}",
"item_name": "hello.txt",
"item_type": "file"
}
HTTP 404
# Step 14b.iii — Mallory has no shares — no partial success slipped
# through. (`GET /api/shares` returns only shares the
# caller created; response is paginated.)
GET {{base_url}}/api/shares
Authorization: Bearer {{mallory_token}}
HTTP 200
[Asserts]
jsonpath "$.items" isCollection
jsonpath "$.items" count == 0
# ─────────────────────────────────────────────────────────────
# 15 — Teardown: revoke the password share + the direct
# file-share, then delete the folder.
+64
View File
@@ -187,3 +187,67 @@ DELETE {{base_url}}/api/recent/clear
Authorization: Bearer {{token}}
HTTP 200
# ─────────────────────────────────────────────────────────────
# Step 10 — Cross-tenant regression (post-Drive AuthZ audit,
# Round 1 HIGH). Before this fix, `POST /api/recent/…`
# accepted any UUID and the listing endpoint JOINed back
# to storage.files/folders (name/mime/size/drive_id) — a
# metadata oracle over the whole tenant. Now the write
# path calls `authz.require(Read, …)`; unauthorised
# callers get 404 (anti-enum) + `authz.denied` audit line.
# See `docs/plan/authz_audit/rest_storage.md`.
# ─────────────────────────────────────────────────────────────
# Re-discover a folder id so the attacker has TWO targets to probe
# (file + folder). Same test1 folder as favorites.hurl.
GET {{base_url}}/api/folders/{{home_folder_id}}/resources?resource_types=folder
Authorization: Bearer {{token}}
HTTP 200
[Captures]
test1_id: jsonpath "$.items[0].resource.id"
# Create/lookup the attacker. Idempotent: `HTTP *` accepts either
# 201 (first run) or 409 (subsequent runs). Login below is the real
# precondition.
POST {{base_url}}/api/admin/users
Authorization: Bearer {{token}}
Content-Type: application/json
{ "username": "rec_mallory", "password": "RecMalloryPassword1!", "email": "rec_mallory@example.com", "role": "user" }
HTTP *
POST {{base_url}}/api/auth/login
Content-Type: application/json
{ "username": "rec_mallory", "password": "RecMalloryPassword1!" }
HTTP 200
[Captures]
mallory_token: jsonpath "$.access_token"
# Step 10a — Record admin's file into mallory's recent: 404.
POST {{base_url}}/api/recent/file/{{file_id}}
Authorization: Bearer {{mallory_token}}
HTTP 404
# Step 10b — Same for admin's folder: 404.
POST {{base_url}}/api/recent/folder/{{test1_id}}
Authorization: Bearer {{mallory_token}}
HTTP 404
# Step 10c — Mallory's recent list stays empty.
GET {{base_url}}/api/recent/resources
Authorization: Bearer {{mallory_token}}
HTTP 200
[Asserts]
jsonpath "$.items" count == 0
+19 -1
View File
@@ -38,17 +38,34 @@ wait_for_http() {
SERVER_PID=""
WOPI_MOCK_PID=""
cleanup() {
if [[ -n "$SERVER_PID" ]]; then
log "Stopping OxiCloud server (pid $SERVER_PID)..."
kill "$SERVER_PID" 2>/dev/null || true
wait "$SERVER_PID" 2>/dev/null || true
fi
if [[ -n "$WOPI_MOCK_PID" ]]; then
log "Stopping WOPI mock discovery (pid $WOPI_MOCK_PID)..."
kill "$WOPI_MOCK_PID" 2>/dev/null || true
wait "$WOPI_MOCK_PID" 2>/dev/null || true
fi
bash "$COMMON/stop-db.sh"
}
trap cleanup EXIT
# ── 0. WOPI mock discovery ────────────────────────────────────────────────────
# Serves the static discovery.xml `OXICLOUD_WOPI_DISCOVERY_URL`
# points at (server.env pins port 9100). Started BEFORE OxiCloud so
# the server's cache-fill on first WOPI request finds it. The mock
# is stdlib-only Python (no deps) — see the file header for what it
# returns and why it's cheap.
log "Starting WOPI mock discovery on port 9100..."
node "$COMMON/wopi_mock_discovery.js" > /tmp/wopi-mock-discovery.log 2>&1 &
WOPI_MOCK_PID=$!
# ── 1. Start postgres ─────────────────────────────────────────────────────────
bash "$COMMON/spawn-db.sh"
@@ -168,7 +185,8 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test
"$API_DIR/cross_drive_move.hurl" \
"$API_DIR/cross_drive_copy.hurl" \
"$API_DIR/webdav_dead_properties.hurl" \
"$API_DIR/webdav_nested_move_cascade.hurl"
"$API_DIR/webdav_nested_move_cascade.hurl" \
"$API_DIR/wopi_authz.hurl"
#bash "$API_DIR/dedup_bulk_upload.sh"
+377
View File
@@ -0,0 +1,377 @@
# =============================================================
# OxiCloud — WOPI authorization at token redemption
# =============================================================
# Regression coverage for the WOPI verb-handler bypass documented in
# memory note `wopi-authz-bypass`. Two bugs closed:
#
# 1. Verb handlers (check_file_info, get_file, put_file,
# file_operations, host_page) previously did NOT call
# `AuthorizationEngine::require` at redemption. A grant
# revoked between mint-time and request-time silently kept
# working until the token TTL expired.
#
# 2. The mint helper decided `can_write` from the client's
# `requested_action` string (`!= "view"` → write). A Viewer
# clicking "Edit in Collabora" received a write-capable
# token because the string was "edit".
#
# The fix wires `authz.require` on every verb and derives
# `can_write` from the caller's actual Update permission. This
# suite hits both paths through the real HTTP surface.
#
# Note on infra:
# * `OXICLOUD_WOPI_ENABLED=true` in tests/common/server.env
# * `OXICLOUD_WOPI_SECRET` pinned so the tokens the server mints
# round-trip verify-able through the suite
# * WOPI discovery served by `tests/common/wopi_mock_discovery.py`
# started by run.sh — mock URL points at a black-hole editor
# so we only assert on OxiCloud's own responses
# =============================================================
# ─────────────────────────────────────────────────────────────
# Step 1 — Login as admin (owner) and capture home folder id
# ─────────────────────────────────────────────────────────────
POST {{base_url}}/api/auth/login
Content-Type: application/json
{ "username": "{{username}}", "password": "{{password}}" }
HTTP 200
[Captures]
alice_token: jsonpath "$.access_token"
alice_user_id: jsonpath "$.user.id"
GET {{base_url}}/api/folders
Authorization: Bearer {{alice_token}}
HTTP 200
[Captures]
alice_home_id: jsonpath "$[0].id"
# ─────────────────────────────────────────────────────────────
# Step 2 — Create a Bob user (Viewer under test) via admin API
# ─────────────────────────────────────────────────────────────
POST {{base_url}}/api/admin/users
Authorization: Bearer {{alice_token}}
Content-Type: application/json
{
"username": "wopi-bob",
"password": "WopiBobPassword1!",
"email": "wopi-bob@example.com",
"role": "user"
}
HTTP 201
[Captures]
bob_user_id: jsonpath "$.id"
POST {{base_url}}/api/auth/login
Content-Type: application/json
{ "username": "wopi-bob", "password": "WopiBobPassword1!" }
HTTP 200
[Captures]
bob_token: jsonpath "$.access_token"
# ─────────────────────────────────────────────────────────────
# Step 3 — Alice uploads a plain-text file the WOPI verbs will
# target. `text/plain` is in the mock discovery XML so
# `/api/wopi/editor-url` resolves to a real (black-hole)
# editor URL — the endpoint returns 200 with an
# access_token we can then poke at the verbs.
# ─────────────────────────────────────────────────────────────
POST {{base_url}}/api/files/upload
Authorization: Bearer {{alice_token}}
[MultipartFormData]
folder_id: {{alice_home_id}}
file: file,fixtures/hello.txt; text/plain
HTTP 201
[Captures]
file_id: jsonpath "$.id"
[Asserts]
jsonpath "$.mime_type" == "text/plain"
# ─────────────────────────────────────────────────────────────
# Step 4 — Alice mints an editor-URL for her own file with
# `action=edit`. Owner has Update → can_write=true.
# ─────────────────────────────────────────────────────────────
GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=edit
Authorization: Bearer {{alice_token}}
HTTP 200
[Captures]
alice_edit_token: jsonpath "$.access_token"
[Asserts]
jsonpath "$.access_token" isString
jsonpath "$.editor_url" contains "edit"
# ─────────────────────────────────────────────────────────────
# Step 5 — CheckFileInfo with the owner's edit token. Verb
# re-checks Read → allowed. `user_can_write=true`
# reflects real Update.
# ─────────────────────────────────────────────────────────────
GET {{base_url}}/wopi/files/{{file_id}}?access_token={{alice_edit_token}}
HTTP 200
[Asserts]
jsonpath "$.UserId" == "{{alice_user_id}}"
jsonpath "$.UserCanWrite" == true
jsonpath "$.SupportsUpdate" == true
# ─────────────────────────────────────────────────────────────
# Step 6 — GetFile with the owner's edit token. Verb re-checks
# Read → 200 with body.
# ─────────────────────────────────────────────────────────────
GET {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{alice_edit_token}}
HTTP 200
[Asserts]
body contains "Hello"
# ─────────────────────────────────────────────────────────────
# Step 7 — PutFile with the owner's edit token. Verb re-checks
# Update → 200. The owner overwrites her own file.
# ─────────────────────────────────────────────────────────────
POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{alice_edit_token}}
Content-Type: application/octet-stream
```
owner overwrite via WOPI PutFile
```
HTTP 200
# ─────────────────────────────────────────────────────────────
# Step 8 — Alice explicitly requests view mode. Even the owner
# gets `can_write=false` — the token respects the
# client's downgrade so Collabora can open a doc
# "read-only for co-browsing".
# ─────────────────────────────────────────────────────────────
GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=view
Authorization: Bearer {{alice_token}}
HTTP 200
[Captures]
alice_view_token: jsonpath "$.access_token"
GET {{base_url}}/wopi/files/{{file_id}}?access_token={{alice_view_token}}
HTTP 200
[Asserts]
# Owner explicitly requested view — supports_update flips off.
jsonpath "$.UserCanWrite" == false
jsonpath "$.SupportsUpdate" == false
# View token trying to write → 401 (token's can_write bit says no
# before the authz.require ever runs).
POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{alice_view_token}}
Content-Type: application/octet-stream
```
owner trying to write with view token
```
HTTP 401
# ─────────────────────────────────────────────────────────────
# Step 9 — SECURITY: Bob has NO grant on Alice's file. Requests
# an edit-URL. The mint helper's Read gate fires → 404
# (anti-enum). This is the pre-fix behaviour holding
# — mint-time Read was already enforced via
# get_file_with_perms.
# ─────────────────────────────────────────────────────────────
GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=edit
Authorization: Bearer {{bob_token}}
HTTP 404
# ─────────────────────────────────────────────────────────────
# Step 10 — Alice grants Bob the Viewer role on the file.
# Capture the grant id off the POST response so Step
# 13's revoke doesn't need to LIST + filter (the LIST
# endpoint returns a bare JSON array, not
# `.grants[?...]`, and Hurl's single-match filter
# capture behaviour is quirky — see memory note
# `feedback_hurl_jsonpath_filter_empty`).
# ─────────────────────────────────────────────────────────────
POST {{base_url}}/api/grants
Authorization: Bearer {{alice_token}}
Content-Type: application/json
{
"subject": { "type": "user", "id": "{{bob_user_id}}" },
"resource": { "type": "file", "id": "{{file_id}}" },
"role": "viewer"
}
HTTP 201
# ─────────────────────────────────────────────────────────────
# Step 11 — SECURITY: Bob (Viewer) requests an EDIT token. Fix
# #12: mint helper derives `can_write` from real
# Update permission, not from the requested_action
# string. Bob has Read but not Update → token is
# minted with `can_write=false` even though he asked
# for "edit".
# ─────────────────────────────────────────────────────────────
GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=edit
Authorization: Bearer {{bob_token}}
HTTP 200
[Captures]
bob_forged_edit_token: jsonpath "$.access_token"
# CheckFileInfo with Bob's "edit" token shows UserCanWrite=false
# because the token's can_write bit was scrubbed at mint. Prior
# to the fix this was `true` — a Viewer editing Alice's file.
GET {{base_url}}/wopi/files/{{file_id}}?access_token={{bob_forged_edit_token}}
HTTP 200
[Asserts]
jsonpath "$.UserId" == "{{bob_user_id}}"
jsonpath "$.UserCanWrite" == false
jsonpath "$.SupportsUpdate" == false
# Bob attempting PutFile with his "edit" token → 401. The
# token's own can_write=false is the outer gate; even if the
# token had somehow been forged with can_write=true, the
# redemption-time authz.require(Update) would return 404.
POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_forged_edit_token}}
Content-Type: application/octet-stream
```
Bob trying to write as Viewer
```
HTTP 401
# Bob CAN read (his Read grant is real).
GET {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_forged_edit_token}}
HTTP 200
# ─────────────────────────────────────────────────────────────
# Step 12 — SECURITY: promote Bob to Editor. Now he legitimately
# holds Update, so an edit token becomes truly write-
# capable.
# ─────────────────────────────────────────────────────────────
POST {{base_url}}/api/grants
Authorization: Bearer {{alice_token}}
Content-Type: application/json
{
"subject": { "type": "user", "id": "{{bob_user_id}}" },
"resource": { "type": "file", "id": "{{file_id}}" },
"role": "editor"
}
# The engine's `ON CONFLICT UPDATE` collapses one role row per
# (subject, resource), so this Editor grant REPLACES the Viewer
# grant from Step 10 rather than stacking. Bob now holds
# Editor alone; revoking it in Step 13 leaves him with no
# grants at all.
HTTP 201
[Captures]
bob_grant_id: jsonpath "$.grants[0].id"
GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=edit
Authorization: Bearer {{bob_token}}
HTTP 200
[Captures]
bob_real_edit_token: jsonpath "$.access_token"
GET {{base_url}}/wopi/files/{{file_id}}?access_token={{bob_real_edit_token}}
HTTP 200
[Asserts]
# Bob is a real Editor now → can_write flips to true.
jsonpath "$.UserCanWrite" == true
jsonpath "$.SupportsUpdate" == true
POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_real_edit_token}}
Content-Type: application/octet-stream
```
Bob as Editor legitimately writes
```
HTTP 200
# ─────────────────────────────────────────────────────────────
# Step 13 — SECURITY: revoke Bob's grant AFTER his edit token was
# minted. The token stays cryptographically valid until
# TTL, but every subsequent verb call must hit the
# authorization engine and reject.
#
# This is the CORE bug the memory note describes: prior
# to the fix Bob's PutFile still succeeded here because
# the verb handlers trusted the token in isolation.
#
# The Editor grant from Step 12 REPLACED the Viewer
# grant from Step 10 (engine's ON CONFLICT UPDATE —
# one role row per subject/resource). So revoking the
# Editor grant leaves Bob with no grants at all; every
# verb — Read AND Update — must refuse.
# ─────────────────────────────────────────────────────────────
DELETE {{base_url}}/api/grants/{{bob_grant_id}}
Authorization: Bearer {{alice_token}}
HTTP 204
# CheckFileInfo — no Read → 404. Prior to the fix the verb
# handler trusted the token and returned 200 with the file's
# metadata.
GET {{base_url}}/wopi/files/{{file_id}}?access_token={{bob_real_edit_token}}
HTTP 404
# GetFile — no Read → 404. Prior to the fix Bob could still
# download the file content until the token TTL expired.
GET {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_real_edit_token}}
HTTP 404
# PutFile — no Update → 404 (verb-side require_wopi_perm), OR
# 401 if the token's own `!claims.can_write` gate happened to
# fire first. The important assertion is "not 200" — a revoked
# grant must never let the caller through.
POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_real_edit_token}}
Content-Type: application/octet-stream
```
Bob post-revoke tries to write
```
HTTP 404
# ─────────────────────────────────────────────────────────────
# Cleanup — delete the test file so subsequent Hurl files don't
# see it. Bob user stays; other tests may reuse the `wopi-bob`
# username, but the grants that made this test meaningful are
# gone.
# ─────────────────────────────────────────────────────────────
DELETE {{base_url}}/api/files/{{file_id}}
Authorization: Bearer {{alice_token}}
HTTP 204
+11 -1
View File
@@ -13,7 +13,17 @@ OXICLOUD_ENABLE_SEARCH=true
OXICLOUD_ENABLE_FILE_SHARING=true
OXICLOUD_ENABLE_MUSIC=true
OXICLOUD_EXPOSE_SYSTEM_USERS=true
OXICLOUD_WOPI_ENABLED=false
OXICLOUD_WOPI_ENABLED=true
# Fixed secret so the Hurl WOPI test can hand-craft valid access
# tokens with a known signing key. Prod deployments MUST override
# this to a random per-deployment value.
OXICLOUD_WOPI_SECRET=test-wopi-secret-do-not-use-in-prod-do-not-use-in-prod
# Discovery URL points at a black hole — VERB endpoints don't need
# discovery, and the WOPI Hurl suite deliberately does NOT touch
# `/api/wopi/editor-url` (the only path that would fetch it), so
# an unreachable URL keeps startup fast and hermetic.
OXICLOUD_WOPI_DISCOVERY_URL=http://127.0.0.1:9100/discovery.xml
OXICLOUD_WOPI_TOKEN_TTL_SECS=3600
OXICLOUD_OIDC_ENABLED=false
OXICLOUD_NEXTCLOUD_ENABLED=true
+68
View File
@@ -0,0 +1,68 @@
#!/usr/bin/env node
// Minimal mock WOPI discovery server for the Hurl WOPI suite.
//
// Serves a valid RFC-shaped discovery XML on `GET /discovery.xml` so
// `OXICLOUD_WOPI_DISCOVERY_URL=http://127.0.0.1:<port>/discovery.xml`
// resolves to a real editor URL when `/api/wopi/editor-url` fetches it.
//
// The `urlsrc` we hand back points at a black-hole host so no real
// editor process needs to be running — the Hurl suite only asserts on
// OxiCloud's own responses (token contents, HTTP status codes,
// headers). The mock exists purely to let `get_editor_url` succeed
// end-to-end so we can exercise the mint-time authz path (Viewer-
// clicks-Edit gets a read-only token).
//
// Node stdlib only — matches the tooling used by tests/oidc/fake_idp
// (both are stdlib-free apart from `node-oidc-provider` on that side).
// No package.json, no npm install, no extra dependency for the api
// test suite. Started + reaped by `tests/api/run.sh`. Port comes from
// `WOPI_MOCK_PORT` env var (default 9100).
'use strict';
const http = require('http');
const DISCOVERY_XML = `<?xml version="1.0" encoding="utf-8"?>
<wopi-discovery>
<net-zone name="external-http">
<!-- text/plain lets the txt files the Hurl suite uploads round-trip. -->
<app name="text/plain" favIconUrl="http://mock-editor.invalid/favicon.ico">
<action name="edit" ext="txt" urlsrc="http://mock-editor.invalid/edit?"/>
<action name="view" ext="txt" urlsrc="http://mock-editor.invalid/view?"/>
</app>
<!-- One office extension so tests can also exercise the docx path
if they need to. -->
<app name="application/vnd.openxmlformats-officedocument.wordprocessingml.document">
<action name="edit" ext="docx" urlsrc="http://mock-editor.invalid/edit?"/>
<action name="view" ext="docx" urlsrc="http://mock-editor.invalid/view?"/>
</app>
</net-zone>
<proof-key oldvalue="" oldmodulus="" oldexponent=""
value="" modulus="" exponent=""/>
</wopi-discovery>
`;
const port = Number(process.env.WOPI_MOCK_PORT || 9100);
const server = http.createServer((req, res) => {
if (req.method === 'GET' && req.url === '/discovery.xml') {
res.writeHead(200, {
'Content-Type': 'application/xml; charset=utf-8',
'Content-Length': Buffer.byteLength(DISCOVERY_XML),
});
res.end(DISCOVERY_XML);
return;
}
res.writeHead(404);
res.end();
});
// SIGTERM from `kill` in run.sh cleanup — exit quietly so the test
// runner's tail-of-log stays clean.
for (const sig of ['SIGTERM', 'SIGINT']) {
process.on(sig, () => server.close(() => process.exit(0)));
}
server.listen(port, '127.0.0.1', () => {
console.log(`wopi-mock-discovery listening on 127.0.0.1:${port}`);
});
+14 -1
View File
@@ -323,7 +323,20 @@ grep -q 'g8-doomed' <<< "$BODY" \
|| fail "K1: g8-doomed.txt not in trashbin PROPFIND"
grep -q '<nc:trashbin-original-location>' <<< "$BODY" \
|| fail "K1: trashbin response missing <nc:trashbin-original-location>"
pass "K1: trashbin shows g8-doomed.txt with original-location"
# Post-D3 (secondary/shared drive support): the `original-location`
# value is drive-relative — the emitter strips the drive-root segment
# from the internal `storage.folders.path` (`"Personal/g8-doomed.txt"`
# for a file at the default drive root) so NC clients see
# `"g8-doomed.txt"` regardless of what the drive's root is named.
# Regression guard: the pre-D3 code hardcoded `strip_prefix("Personal/")`
# — a bug that would silently break secondary drives. Assert the
# stripped shape (no leading `Personal/`, no leading `/`, no drive
# segment).
grep -q '<nc:trashbin-original-location>g8-doomed\.txt</nc:trashbin-original-location>' <<< "$BODY" \
|| fail "K1: original-location not drive-relative (expected 'g8-doomed.txt', got: $(grep -o '<nc:trashbin-original-location>[^<]*</nc:trashbin-original-location>' <<< "$BODY"))"
pass "K1: trashbin shows g8-doomed.txt with drive-relative original-location"
# Extract the trashed item id (last segment of the href).
# Trashbin hrefs are `/remote.php/dav/trashbin/{user}/trash/{uuid}`