perf: round 24 — download_zip per-item authz+metadata N+1 → batch (validated authorization pass)
The ROUND23-deferred download_zip N+1, given its own validated pass. The individually-selected files were authorized + fetched one at a time via get_file_with_perms (require + get = 2 serial round-trips/file) before any streaming — a 200-file selection was 400 serial round-trips. AFTER routes the whole multi-select through the new FileRetrievalService::get_files_by_ids_with_perms: one check_files_read_batch (the PgAclEngine resolves every file's drive in ONE query and primes the resource->drive cache) + one get_files_by_ids. 2N round-trips -> 2. Authorization is unchanged and still enforced BEFORE any ZIP entry is written: - add_file_entry_streamed writes the entry header (the filename) before it opens the authorized stream, so the pre-filter is load-bearing — a denied file must never reach it or its name leaks into the archive. AFTER a denied/missing id is absent from the authorized map and is skipped in the same input order, exactly as the old loop skipped a denied get_file_with_perms; it never reaches the entry write. The authz moved from a per-file require to one batch check EARLIER in the same function, not into or after the stream. - The stream open keeps its own per-file Read check (now a primed-cache hit) + Recents recording; check_files_read_batch is documented + gated as identical to looping require. Because the change is authorization-sensitive, the gate is the security property itself. bench_round24_zip_authz drives the real PgAclEngine over a seeded, interleaved mix of owned (granted drive) + denied (other drive) + missing ids and asserts: the batch inclusion set AND input order are identical to the per-file require loop; the included set is exactly the caller's owned files; no denied or missing id is ever included (the authz-regression tripwire); and the batch fetch returns exactly the owned files. Latency (cold, 600-item 1/3-owned selection): 559 -> 267 ms (2.10x; the realistic all-owned selection is O(1) -> a larger win). See benches/ROUND24.md. The folder selections are left as-is (root counts are small and there is no check_folders_read_batch primitive to batch through). Verified: cargo clippy --features bench --all-targets -D warnings clean, cargo fmt --all --check clean, cargo test --lib --features bench = 529 passed / 0 failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DKyQ4AnYtgp1JtjzweyMeo
This commit is contained in:
@@ -726,31 +726,46 @@ impl BatchOperationService {
|
||||
let mut items_added: usize = 0;
|
||||
|
||||
// ── Add individual files at the root of the ZIP ──────────────────
|
||||
// Authorize + fetch metadata for the whole multi-select in 2 round-trips
|
||||
// (one batch Read check + one batch get) instead of the per-file
|
||||
// `get_file_with_perms` N+1 (2 round-trips/file). The batch check also
|
||||
// primes the resource→drive cache, so `add_file_entry_streamed`'s
|
||||
// per-file stream-open re-check lands on the cache. A denied / missing /
|
||||
// unparseable id is absent from the map → skipped in the same input
|
||||
// order, exactly as the old per-file loop skipped it. Authorization is
|
||||
// UNCHANGED — still enforced (pre-check here + the stream open's own
|
||||
// Read check + Recents recording) before any ZIP entry is written, so a
|
||||
// denied file never leaks its name into the archive (benches/ROUND24.md).
|
||||
let authorized = self
|
||||
.file_retrieval
|
||||
.get_files_by_ids_with_perms(&file_ids, user_id)
|
||||
.await
|
||||
.map_err(BatchOperationError::Domain)?;
|
||||
let by_id: HashMap<Uuid, FileDto> = authorized
|
||||
.into_iter()
|
||||
.filter_map(|f| Uuid::parse_str(&f.id).ok().map(|u| (u, f)))
|
||||
.collect();
|
||||
for file_id in &file_ids {
|
||||
let file_dto = match Uuid::parse_str(file_id).ok().and_then(|u| by_id.get(&u)) {
|
||||
Some(f) => f,
|
||||
None => {
|
||||
info!("Skipping file {} (not accessible or missing)", file_id);
|
||||
continue;
|
||||
}
|
||||
};
|
||||
match self
|
||||
.file_retrieval
|
||||
.get_file_with_perms(file_id, user_id)
|
||||
.add_file_entry_streamed(
|
||||
&mut zip,
|
||||
file_id,
|
||||
&file_dto.name,
|
||||
&file_dto.mime_type,
|
||||
Some(user_id),
|
||||
)
|
||||
.await
|
||||
{
|
||||
Ok(file_dto) => {
|
||||
match self
|
||||
.add_file_entry_streamed(
|
||||
&mut zip,
|
||||
file_id,
|
||||
&file_dto.name,
|
||||
&file_dto.mime_type,
|
||||
Some(user_id),
|
||||
)
|
||||
.await
|
||||
{
|
||||
Ok(_) => items_added += 1,
|
||||
Err(e) => {
|
||||
info!("Could not add file {} to ZIP: {}", file_dto.name, e);
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(_) => items_added += 1,
|
||||
Err(e) => {
|
||||
info!("Could not get file metadata {}: {}", file_id, e);
|
||||
info!("Could not add file {} to ZIP: {}", file_dto.name, e);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -287,6 +287,52 @@ impl FileRetrievalService {
|
||||
Ok(files.into_iter().map(FileDto::from).collect())
|
||||
}
|
||||
|
||||
/// Batched, authorized multi-get for the ZIP-download multi-select — the
|
||||
/// batch form of [`FileRetrievalUseCase::get_file_with_perms`] over an
|
||||
/// explicit id list.
|
||||
///
|
||||
/// Authorizes `Read` on every id in ONE `check_files_read_batch`
|
||||
/// round-trip (which resolves all drives in a single query AND primes the
|
||||
/// resource→drive cache, so the per-file re-check the subsequent stream
|
||||
/// open performs becomes a cache hit), then fetches only the authorized ids
|
||||
/// in ONE `get_files_by_ids` query. Replaces `download_zip`'s per-file
|
||||
/// `require_file` + `get_file` loop — 2 round-trips/file → 2 total.
|
||||
///
|
||||
/// Returns the authorized, existing files; a denied / missing / unparseable
|
||||
/// id is simply **absent** from the result (the caller re-associates by id
|
||||
/// and skips the rest, exactly as the per-file loop skipped a denied /
|
||||
/// missing `get_file_with_perms`). Read-authorization is identical to the
|
||||
/// per-file path (`check_files_read_batch` is documented and gated as
|
||||
/// semantically identical to looping `require`). Recents recording is left
|
||||
/// to the subsequent per-file stream open (`get_file_stream_with_perms`),
|
||||
/// which records it (throttle-coalesced) — same net effect as the old
|
||||
/// loop's `notify_file_accessed` + stream double-notify. Fail-closed if no
|
||||
/// engine was injected, mirroring [`Self::require_file`].
|
||||
pub async fn get_files_by_ids_with_perms(
|
||||
&self,
|
||||
ids: &[String],
|
||||
caller_id: Uuid,
|
||||
) -> Result<Vec<FileDto>, DomainError> {
|
||||
let authz = self.authz.as_ref().ok_or_else(|| {
|
||||
DomainError::internal_error("FileRetrieval", "Authorization engine unavailable")
|
||||
})?;
|
||||
// Unparseable ids can't be authorized (the per-file path 404s on them),
|
||||
// so drop them here — they stay absent from the authorized set.
|
||||
let uuids: Vec<Uuid> = ids.iter().filter_map(|s| Uuid::parse_str(s).ok()).collect();
|
||||
if uuids.is_empty() {
|
||||
return Ok(Vec::new());
|
||||
}
|
||||
let allowed = authz
|
||||
.check_files_read_batch(Subject::User(caller_id), &uuids)
|
||||
.await?;
|
||||
if allowed.is_empty() {
|
||||
return Ok(Vec::new());
|
||||
}
|
||||
let allowed_ids: Vec<String> = allowed.iter().map(Uuid::to_string).collect();
|
||||
let files = self.file_read.get_files_by_ids(&allowed_ids).await?;
|
||||
Ok(files.into_iter().map(FileDto::from).collect())
|
||||
}
|
||||
|
||||
/// Range read for HTTP Range Requests, cache-aware.
|
||||
///
|
||||
/// Media players and PDF viewers fetch these files *exclusively* through
|
||||
|
||||
Reference in New Issue
Block a user