fix(595): permit unlimited user quota
This commit is contained in:
Edouard Vanbelle
@@ -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$;
|
||||
@@ -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(
|
||||
|
||||
@@ -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
|
||||
@@ -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" \
|
||||
|
||||
Reference in New Issue
Block a user