From 53c301e472a82c0b69bcbf468cd1c257c046fe0b Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Wed, 15 Jul 2026 21:47:07 +0200 Subject: [PATCH] fix(595): permit unlimited user quota --- ...260916000000_null_personal_drive_quota.sql | 81 +++++++++ src/application/services/folder_service.rs | 15 +- .../regression_595_unlimited_user_quota.hurl | 154 ++++++++++++++++++ tests/api/run.sh | 1 + 4 files changed, 250 insertions(+), 1 deletion(-) create mode 100644 migrations/20260916000000_null_personal_drive_quota.sql create mode 100644 tests/api/regression_595_unlimited_user_quota.hurl diff --git a/migrations/20260916000000_null_personal_drive_quota.sql b/migrations/20260916000000_null_personal_drive_quota.sql new file mode 100644 index 00000000..9f09fc59 --- /dev/null +++ b/migrations/20260916000000_null_personal_drive_quota.sql @@ -0,0 +1,81 @@ +-- ───────────────────────────────────────────────────────────────────────── +-- Heal + pin the "personal drives always have NULL quota_bytes" +-- invariant from docs/plan/drive.md §7. +-- +-- Bug (#595): `folder_service.rs::PersonalDriveLifecycleHook` was +-- calling `create_personal_drive_atomic(user_id, Some(user.storage_quota_bytes()))`, +-- baking the user's envelope quota into `storage.drives.quota_bytes` +-- for every personal drive. Two conventions then collided at upload +-- time: +-- +-- * User-envelope check (`check_storage_quota`) treats `0` as +-- unlimited (`quota <= 0 → Ok`). +-- * Drive-quota check (`check_drive_quota`) treats `NULL` as +-- unlimited but `Some(0)` as a literal zero-byte cap. +-- +-- Setting user quota to 0 in the Admin UI ("unlimited" per the UI +-- convention) therefore stamped `drives.quota_bytes = 0` on the +-- personal drive at creation, and every subsequent upload was +-- rejected with 507 Insufficient Storage. +-- +-- Rust-side fix: `folder_service.rs` now passes `None`. This +-- migration: +-- +-- 1. NULLs every existing personal drive's `quota_bytes` so already- +-- created users can upload immediately after deploy (Fix 2). +-- 2. Adds a CHECK constraint so any future code path that tries to +-- write a non-NULL quota on a personal drive fails at the DB +-- layer instead of silently corrupting state (Fix 3). +-- +-- Shared drives are untouched — their quota model is orthogonal and +-- the "NULL = unlimited, positive = numeric cap, 0 = literal zero" +-- semantics are the design (an admin can legitimately lock a shared +-- drive at 0 bytes, e.g. archive-only). + +-- ── 1. Heal existing personal-drive rows ──────────────────────────────── +-- +-- Every row today with `kind = 'personal'` should carry NULL. Set them +-- to NULL unconditionally (a personal drive already at NULL is a no-op +-- under IS DISTINCT FROM). Idempotent on re-run. +UPDATE storage.drives + SET quota_bytes = NULL + WHERE kind = 'personal' + AND quota_bytes IS DISTINCT FROM NULL; + +-- ── 2. Pin the invariant at the schema layer ──────────────────────────── +-- +-- Uses `NOT VALID` + `VALIDATE CONSTRAINT` so the ALTER TABLE grabs +-- only the fast metadata lock instead of scanning the whole table +-- under an ACCESS EXCLUSIVE lock. The row heal above already satisfies +-- every existing row, so the subsequent VALIDATE completes without +-- error. +ALTER TABLE storage.drives + ADD CONSTRAINT drives_personal_quota_null + CHECK (kind <> 'personal' OR quota_bytes IS NULL) + NOT VALID; + +ALTER TABLE storage.drives + VALIDATE CONSTRAINT drives_personal_quota_null; + +-- ── 3. Post-flight sanity ─────────────────────────────────────────────── +-- +-- Refuse to finish if any personal drive still carries a non-NULL +-- quota (defense against a race where a concurrent transaction +-- inserted a bad row between the UPDATE and the VALIDATE — the +-- VALIDATE would already have failed in that case, but the explicit +-- check makes the failure mode obvious in logs). +DO $BODY$ +DECLARE + bad BIGINT; +BEGIN + SELECT COUNT(*) INTO bad + FROM storage.drives + WHERE kind = 'personal' + AND quota_bytes IS NOT NULL; + IF bad > 0 THEN + RAISE EXCEPTION + 'Migration 20260916000000 left % personal drive(s) with a non-NULL quota_bytes', + bad; + END IF; +END; +$BODY$; diff --git a/src/application/services/folder_service.rs b/src/application/services/folder_service.rs index 0073a346..69dfaf4c 100644 --- a/src/application/services/folder_service.rs +++ b/src/application/services/folder_service.rs @@ -922,9 +922,22 @@ impl PersonalDriveLifecycleHook { // parent_id=NULL, drive_id pinned) + drives.root_folder_id // wire-up + Owner role_grant. Single SQL statement, atomic // against server crash mid-sequence (docs/plan/drive.md §3). + // + // `quota_bytes = None` (NULL in the DB) is the invariant for + // every personal drive per plan §7: the cap for a user's + // personal storage lives on `auth.users.storage_quota_bytes` + // (the user envelope), not on the drive row. Passing + // `Some(user.storage_quota_bytes())` here previously baked + // the user quota into `drives.quota_bytes` and — combined + // with the "0 = unlimited" convention on the user check but + // "0 = literal zero" convention on the drive check — turned + // "unlimited user" into "0-byte drive" (see #595). The + // migration `20260916000000_null_personal_drive_quota.sql` + // heals existing rows and adds a CHECK constraint pinning + // this invariant at the schema layer. let drive_with_name = self .drive_repo - .create_personal_drive_atomic(user.id(), Some(user.storage_quota_bytes())) + .create_personal_drive_atomic(user.id(), None) .await .map_err(|e| { DomainError::internal_error( diff --git a/tests/api/regression_595_unlimited_user_quota.hurl b/tests/api/regression_595_unlimited_user_quota.hurl new file mode 100644 index 00000000..928f75f1 --- /dev/null +++ b/tests/api/regression_595_unlimited_user_quota.hurl @@ -0,0 +1,154 @@ +# ============================================================= +# Regression #595 — Admin-created user with quota=0 ("unlimited" +# per UI convention) must be able to upload. +# ============================================================= +# Pre-fix behaviour (documented in the issue): +# +# 1. Admin creates user with `quota_bytes: 0` (meaning "unlimited" +# per the check-code convention: `check_storage_quota` treats +# `quota <= 0` as unlimited). +# 2. `PersonalDriveLifecycleHook::create_personal_drive_atomic` was +# called with `Some(user.storage_quota_bytes())` — so +# `storage.drives.quota_bytes` on the new personal drive was +# stamped `0`. +# 3. On upload, the drive-quota check (`check_drive_quota_by_folder`) +# reads `drives.quota_bytes = 0`, interprets Some(0) as a literal +# zero-byte cap (its NULL check only accepts `None` as unlimited), +# and rejects with 507 Insufficient Storage. +# +# The two conventions collided: user-quota "0 = unlimited" vs +# drive-quota "0 = literal zero, NULL = unlimited". Documented as +# a spec violation of docs/plan/drive.md §7: "For personal drives +# this column is NULL … the effective cap comes from the user +# envelope." +# +# Fix (three parts, this test guards all three): +# 1. `folder_service.rs:927` — pass `None`, never `Some(user quota)`. +# 2. Migration `20260916000000_null_personal_drive_quota.sql` — +# NULL every existing personal drive's `quota_bytes` (data heal). +# 3. Same migration — CHECK constraint pinning +# `kind <> 'personal' OR quota_bytes IS NULL` at the DB layer. +# +# This scenario reproduces the bug against a fresh user and asserts +# the upload succeeds (Fix 1 evidence) AND the personal drive's +# `quota_bytes` field is absent from the wire (`Option::is_none` +# serde-skip → `quota_bytes` key missing = Fix 1 + migration evidence). +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Admin login. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +admin_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 2 — Admin creates a new user with `quota_bytes: 0` +# (the "unlimited" UI convention that triggered #595). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "username": "unlimited_regression_595", + "password": "UnlimitedPwd1!", + "email": "unlimited_regression_595@example.com", + "role": "user", + "quota_bytes": 0 +} + +HTTP 201 +[Asserts] +# The user record itself carries the literal `0` (the convention: +# 0 at the user layer means unlimited, `check_storage_quota` passes). +jsonpath "$.storage_quota_bytes" == 0 + + +# ───────────────────────────────────────────────────────────── +# Step 3 — New user logs in. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "unlimited_regression_595", "password": "UnlimitedPwd1!" } + +HTTP 200 +[Captures] +user_token: jsonpath "$.access_token" +user_user_id: jsonpath "$.user.id" + + +# ───────────────────────────────────────────────────────────── +# Step 4 — Personal drive should be created with NULL quota_bytes +# (Fix 1). `DriveDto` uses +# `#[serde(skip_serializing_if = "Option::is_none")]` +# on `quota_bytes`, so NULL = the field is OMITTED from +# the JSON. `body not contains` on `quota_bytes` is the +# strongest anti-regression assertion available at this +# layer: if a future change re-introduces `Some(0)` (or +# any numeric value), the field will surface and this +# assertion fires. Fresh user has exactly one drive +# (their default personal) so a body-level contains +# check is unambiguous. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/drives +Authorization: Bearer {{user_token}} + +HTTP 200 +[Asserts] +jsonpath "$" count == 1 +jsonpath "$[0].kind" == "personal" +jsonpath "$[0].default_for_user" == "{{user_user_id}}" +body not contains "quota_bytes" + + +# ───────────────────────────────────────────────────────────── +# Step 5 — Grab the personal drive's root folder id for the +# upload target. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/folders +Authorization: Bearer {{user_token}} + +HTTP 200 +[Captures] +personal_root_id: jsonpath "$[0].id" + + +# ───────────────────────────────────────────────────────────── +# Step 6 — THE REGRESSION ASSERTION. Upload a file to the +# user's personal drive. Pre-fix this returned 507 +# Insufficient Storage; post-fix it returns 201 with +# the created file DTO. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/files/upload +Authorization: Bearer {{user_token}} +[MultipartFormData] +folder_id: {{personal_root_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +uploaded_file_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step 7 — Cleanup. Delete the file so the storage cleanup +# check at the end of run.sh doesn't complain, then +# leave the throwaway user + their empty personal +# drive in place (deleting the user via the admin API +# is the same shape as the sibling admin_user_ops.hurl; +# keeping it minimal here since the fixture user has +# a deterministic unique name). +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/files/{{uploaded_file_id}} +Authorization: Bearer {{user_token}} + +HTTP * +[Asserts] +status >= 200 +status < 300 diff --git a/tests/api/run.sh b/tests/api/run.sh index 9fcb00fd..6fb74ec5 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -193,6 +193,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/trash_per_drive.hurl" \ "$API_DIR/drive_quota.hurl" \ "$API_DIR/user_envelope_quota.hurl" \ + "$API_DIR/regression_595_unlimited_user_quota.hurl" \ "$API_DIR/drive_policies.hurl" \ "$API_DIR/cross_drive_move.hurl" \ "$API_DIR/cross_drive_copy.hurl" \