cjw/Oxicloud
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

security(/api/dedup): normalize dedup admin routes into /api/admin

Browse Source 
/dedup/stats       -> /api/admin/dedup/stats
    /dedup/recalculate -> /api/admin/dedup/recalculate
This commit is contained in:
Edouard Vanbelle
2026-07-17 21:33:47 +02:00
parent e0156a43f5
commit c2b5d9fe2e
7 changed files with 192 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/plan/drive.md
+2 -2
View File
@@ -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
src/interfaces/api/handlers/admin_handler.rs
+11
View File
@@ -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<Arc<AppState>> {
// `/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))
src/interfaces/api/handlers/dedup_handler.rs
+36 -27
View File
@@ -218,18 +218,16 @@ impl DedupHandler {
/// - Deduplication ratio
pub(super) async fn get_stats_impl(
State(state): State<GlobalState>,
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<GlobalState>,
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<GlobalState>, 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(
src/interfaces/api/routes.rs
+9 -8
View File
@@ -391,18 +391,19 @@ pub fn create_api_routes(app_state: &Arc<AppState>) -> Router<Arc<AppState>> {
// 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()
tests/api/dedup_admin_gate.hurl
+132
View File
@@ -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
tests/api/dedup_blob_cleanup.hurl
+1 -1
View File
@@ -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
tests/api/run.sh
+1
View File
@@ -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" \