diff --git a/docs/plan/drive.md b/docs/plan/drive.md index d4195693..ac5f8df1 100644 --- a/docs/plan/drive.md +++ b/docs/plan/drive.md @@ -2037,8 +2037,8 @@ PR: 4. `tests/api/storage_cleanup_check.sh` clean. 5. No new `cargo clippy` warnings. 6. Tantivy index returns no cross-drive results for any caller. -7. `/api/dedup/stats` shows blob ref-counts consistent with the - number of files referencing each blob across all drives. +7. `/api/admin/dedup/stats` shows blob ref-counts consistent with + the number of files referencing each blob across all drives. ## UI design — outline for D1 and D3 diff --git a/src/interfaces/api/handlers/admin_handler.rs b/src/interfaces/api/handlers/admin_handler.rs index 031cef5b..c6ac3ebb 100644 --- a/src/interfaces/api/handlers/admin_handler.rs +++ b/src/interfaces/api/handlers/admin_handler.rs @@ -27,6 +27,7 @@ use crate::application::ports::storage_ports::StorageUsagePort; use crate::common::di::AppState; use crate::domain::repositories::drive_repository::DriveRepository; use crate::domain::services::authorization::{Resource, Subject}; +use crate::interfaces::api::handlers::dedup_handler::{get_stats, recalculate_stats}; use crate::interfaces::api::handlers::search_handler::clear_search_cache; use crate::interfaces::errors::AppError; use crate::interfaces::middleware::auth::AuthUser; @@ -96,6 +97,16 @@ pub fn admin_routes() -> Router> { // `/api/search/cache` pre-2026-07-17; the URL now declares // its admin intent up front. .route("/search/cache", delete(clear_search_cache)) + // Dedup — global storage stats + integrity recalculation + // (AuthZ audit #24 + #25, 2026-07-17). Both are operator-only + // observability / maintenance surfaces (blob-count-level data + // + verify_integrity sweep). Moved here from `/api/dedup/*` + // so the URL declares admin intent and the middleware layer + // enforces it — same pattern as `search/cache` above. The + // any-authenticated sibling routes (`/check`, `/check-batch`, + // `/blob/{hash}`) stay at `/api/dedup/*`. + .route("/dedup/stats", get(get_stats)) + .route("/dedup/recalculate", post(recalculate_stats)) // SMTP diagnostics .route("/smtp/info", get(get_smtp_info)) .route("/smtp/test", post(send_smtp_test)) diff --git a/src/interfaces/api/handlers/dedup_handler.rs b/src/interfaces/api/handlers/dedup_handler.rs index 1d25780a..f7e8ef83 100644 --- a/src/interfaces/api/handlers/dedup_handler.rs +++ b/src/interfaces/api/handlers/dedup_handler.rs @@ -218,18 +218,16 @@ impl DedupHandler { /// - Deduplication ratio pub(super) async fn get_stats_impl( State(state): State, - auth_user: AuthUser, + _auth_user: AuthUser, ) -> impl IntoResponse { - // Admin-only — global dedup statistics are sensitive infrastructure data - if auth_user.role != "admin" { - return Response::builder() - .status(StatusCode::FORBIDDEN) - .header(header::CONTENT_TYPE, "application/json") - .body(Body::from(r#"{"error": "Admin role required"}"#)) - .unwrap() - .into_response(); - } - + // AuthZ audit #24 (2026-07-17): admin check moved to the + // `/api/admin/*` middleware layer. Reaching this handler means + // the caller is admin by construction — the bespoke role + // string comparison here (`auth_user.role != "admin"` → 403 + // with a hand-rolled JSON body, no audit line) is gone. The + // route is registered at `admin_handler::admin_routes()`; + // moving the URL to `/api/admin/dedup/stats` also declares + // the admin intent up front. let dedup = &state.core.dedup_service; let stats = dedup.get_stats().await; @@ -343,16 +341,10 @@ impl DedupHandler { State(state): State, auth_user: AuthUser, ) -> impl IntoResponse { - // Admin-only — integrity verification is a privileged operation - if auth_user.role != "admin" { - return Response::builder() - .status(StatusCode::FORBIDDEN) - .header(header::CONTENT_TYPE, "application/json") - .body(Body::from(r#"{"error": "Admin role required"}"#)) - .unwrap() - .into_response(); - } - + // AuthZ audit #25 (2026-07-17): admin check moved to the + // `/api/admin/*` middleware layer — see the sibling + // `get_stats_impl` comment. `auth_user` is kept so the + // success-side audit line carries the caller id. let dedup = &state.core.dedup_service; // Verify integrity first @@ -392,6 +384,21 @@ impl DedupHandler { savings_percentage: savings_pct, }; + // AuthZ audit #25 (2026-07-17): integrity recalculation is a + // low-frequency privileged operation — landing an audit event + // so security reviews can see who ran verify + integrity + // sweeps and when. The pre-fix path emitted no audit line at + // all (the accepted 200 was silent from the security POV). + tracing::info!( + target: "audit", + event = "dedup.integrity_recalculated", + caller_id = %auth_user.id, + unique_blobs = response.unique_blobs, + total_references = response.total_references, + bytes_saved = response.bytes_saved, + "🧮 dedup integrity verified and stats recomputed by admin", + ); + Response::builder() .status(StatusCode::OK) .header(header::CONTENT_TYPE, "application/json") @@ -453,12 +460,13 @@ pub async fn check_hashes_batch( #[utoipa::path( get, - path = "/api/dedup/stats", + path = "/api/admin/dedup/stats", responses( (status = 200, description = "Deduplication statistics", body = StatsResponse), - (status = 403, description = "Admin role required"), + (status = 401, description = "Missing or invalid token"), + (status = 403, description = "Caller is not an admin"), ), - tag = "dedup", + tag = "admin", security(("bearerAuth" = [])) )] pub async fn get_stats(state: State, auth_user: AuthUser) -> impl IntoResponse { @@ -489,13 +497,14 @@ pub async fn get_blob( #[utoipa::path( post, - path = "/api/dedup/recalculate", + path = "/api/admin/dedup/recalculate", responses( (status = 200, description = "Statistics after integrity verification", body = StatsResponse), - (status = 403, description = "Admin role required"), + (status = 401, description = "Missing or invalid token"), + (status = 403, description = "Caller is not an admin"), (status = 500, description = "Integrity verification failed"), ), - tag = "dedup", + tag = "admin", security(("bearerAuth" = [])) )] pub async fn recalculate_stats( diff --git a/src/interfaces/api/routes.rs b/src/interfaces/api/routes.rs index 60d23070..ccec5f2e 100644 --- a/src/interfaces/api/routes.rs +++ b/src/interfaces/api/routes.rs @@ -391,18 +391,19 @@ pub fn create_api_routes(app_state: &Arc) -> Router> { // Create routes for deduplication endpoints. // All handlers are free functions — see dedup_handler.rs for why // #[utoipa::path] cannot be applied to DedupHandler impl methods directly. - use super::handlers::dedup_handler::{ - check_hash, check_hashes_batch, get_blob, get_stats, recalculate_stats, - }; + use super::handlers::dedup_handler::{check_hash, check_hashes_batch, get_blob}; let dedup_router = Router::new() .route("/check/{hash}", get(check_hash)) .route("/check-batch", post(check_hashes_batch)) - .route("/stats", get(get_stats)) .route("/blob/{hash}", get(get_blob)) - // NOTE: remove_reference is intentionally NOT exposed as a public - // endpoint — ref_count management is an internal concern handled - // automatically when files are deleted via the file API. - .route("/recalculate", post(recalculate_stats)) + // NOTE: `remove_reference` is intentionally NOT exposed as a + // public endpoint — ref_count management is an internal concern + // handled automatically when files are deleted via the file API. + // + // `/stats` and `/recalculate` moved to `/api/admin/dedup/*` + // (AuthZ audit #24/#25, 2026-07-17) so the middleware admin + // gate covers them by construction. See + // `admin_handler::admin_routes()`. .with_state(app_state.clone()); let mut router = Router::new() diff --git a/tests/api/dedup_admin_gate.hurl b/tests/api/dedup_admin_gate.hurl new file mode 100644 index 00000000..690c6242 --- /dev/null +++ b/tests/api/dedup_admin_gate.hurl @@ -0,0 +1,132 @@ +# ============================================================= +# OxiCloud — Dedup admin gate + URL move +# ============================================================= +# Regression pin for AuthZ audit #24 + #25 (2026-07-12). +# +# `dedup_handler.rs` previously rolled its own admin check on +# `/api/dedup/stats` and `/api/dedup/recalculate` — a bespoke +# `if auth_user.role != "admin" { 403 with hand-rolled JSON }` +# with no audit line on rejection. That's the same drift class +# the admin middleware layer refactor closed elsewhere on +# 2026-07-17. +# +# Fix: +# 1. Both endpoints moved to `/api/admin/dedup/*` where the +# `/api/admin` middleware gate covers them by construction. +# URL declares admin intent up front. +# 2. Inline role check removed from the handlers — reaching +# them at all means the caller is admin. +# 3. `recalculate` emits `dedup.integrity_recalculated` on +# success (audit #25). Not asserted here (no log-scrape +# harness in Hurl); the shape is pinned in the handler +# code and covered by the `audit` tracing target contract. +# +# This test pins: +# * Admin can hit both endpoints at the new URL → 200. +# * Non-admin (bob) hits both → 403 (middleware layer). +# * The OLD URLs `/api/dedup/stats` and `/api/dedup/recalculate` +# are no longer registered → 404. Trips if someone +# re-introduces the routes to `dedup_router` without also +# removing them from `admin_handler::admin_routes()`. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Setup — admin login + bob (re-)provisioning. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +admin_token: jsonpath "$.access_token" + + +# Anti-enum registration. +POST {{base_url}}/api/auth/register +Content-Type: application/json +{ + "username": "dedup_bob", + "email": "dedup_bob@example.com", + "password": "DedupBobPassword1!" +} + +HTTP 200 + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "dedup_bob", "password": "DedupBobPassword1!" } + +HTTP 200 +[Captures] +bob_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Admin can hit the new URL. `stats` returns a +# `StatsResponse`-shaped body. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/admin/dedup/stats +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +jsonpath "$.unique_blobs" isNumber +jsonpath "$.total_references" isNumber +jsonpath "$.bytes_saved" isNumber +jsonpath "$.total_logical_bytes" isNumber +jsonpath "$.total_physical_bytes" isNumber + + +# ───────────────────────────────────────────────────────────── +# Step 2 — Admin can trigger the integrity recalculation. +# Response shape mirrors `stats`. Server-side, this +# also emits the `dedup.integrity_recalculated` audit +# event (not asserted from Hurl). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/dedup/recalculate +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +jsonpath "$.unique_blobs" isNumber +jsonpath "$.total_references" isNumber + + +# ───────────────────────────────────────────────────────────── +# Step 3 — Bob (non-admin) is denied. The `/api/admin/*` +# middleware layer emits `AuthError::AccessDenied` → +# 403. No hand-rolled 403 body from the handler; the +# handler doesn't even run. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/admin/dedup/stats +Authorization: Bearer {{bob_token}} + +HTTP 403 + + +POST {{base_url}}/api/admin/dedup/recalculate +Authorization: Bearer {{bob_token}} + +HTTP 403 + + +# ───────────────────────────────────────────────────────────── +# Step 4 — The old URLs are no longer registered. Trips if a +# future refactor re-adds them to `dedup_router` without +# removing them from `admin_handler::admin_routes()` (or +# vice versa). Anti-enum catch-all in the `/api/*` router +# returns 404 for unknown paths. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/dedup/stats +Authorization: Bearer {{admin_token}} + +HTTP 404 + + +POST {{base_url}}/api/dedup/recalculate +Authorization: Bearer {{admin_token}} + +HTTP 404 diff --git a/tests/api/dedup_blob_cleanup.hurl b/tests/api/dedup_blob_cleanup.hurl index 63849750..355e6d78 100644 --- a/tests/api/dedup_blob_cleanup.hurl +++ b/tests/api/dedup_blob_cleanup.hurl @@ -14,7 +14,7 @@ # (proves blob NOT prematurely deleted — bug 3 detection) # 4. Permanently delete file 2 → blob and thumbnail cleaned up # -# NOTE: The /api/dedup/stats endpoint counts CDC chunk rows in +# NOTE: The /api/admin/dedup/stats endpoint counts CDC chunk rows in # storage.blobs and derives bytes_saved from chunk_manifests. # Both tables may be 0 when the CDC path is disabled or the # server uses the legacy blob path — so we avoid stats-based diff --git a/tests/api/run.sh b/tests/api/run.sh index f2313c89..862758be 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -164,6 +164,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/recent.hurl" \ "$API_DIR/batch_folder_copy.hurl" \ "$API_DIR/dedup_blob_cleanup.hurl" \ + "$API_DIR/dedup_admin_gate.hurl" \ "$API_DIR/default_caldav_carddav.hurl" \ "$API_DIR/dav_error_mapping.hurl" \ "$API_DIR/carddav_vcard_properties.hurl" \