From 536f1b81989eb2edf825ae306433e60bd7f702f7 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 16 Jun 2026 01:39:03 +0200 Subject: [PATCH 1/6] chore(drive): add plan of Drive impl. for review --- docs/plan/drive.md | 1393 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 1393 insertions(+) create mode 100644 docs/plan/drive.md diff --git a/docs/plan/drive.md b/docs/plan/drive.md new file mode 100644 index 00000000..d94ec018 --- /dev/null +++ b/docs/plan/drive.md @@ -0,0 +1,1393 @@ +# Drives — design proposal + +> **Status**: design proposal, locked but not implemented. Reviewed Jun 2026 with +> Ed; all blocking decisions answered. Open items listed at the end of the file. + +## Context + +Today every resource (folder, file) in OxiCloud has a single user owner +recorded directly on the row (`storage.folders.user_id`, +`storage.files.user_id`). Sharing is layered on top via ReBAC grants in +`storage.access_grants`. The model is simple and has carried us this far, +but it has two structural problems: + +1. **Owner = user, full stop.** When a user leaves the company, their + files leave with them. The team's de-facto shared folders are + technically owned by a single person; transferring ownership requires + moving every resource one-by-one and re-issuing every grant. There is + no "this folder belongs to the Engineering team" concept. + +2. **Quota is per-user, full stop.** A user with personal storage who + also collaborates in a 1 TB team space currently has both counted + against their personal quota (or the team data costs are absorbed by + whoever owns the folder). There is no way to bill team storage to the + team. + +The proposal is to introduce **drives** as a Google-Workspace-style +container concept: + +- Every resource belongs to exactly one drive. +- A drive has owners (users **and/or** groups) with roles. +- Each user automatically gets a personal drive at registration. +- Shared drives can be owned by groups, so membership changes + automatically follow the group — "Bob left" no longer needs an admin + to chase down which folders to reassign. +- Quotas move from users to drives. +- Per-drive policies enable team-level rules (forbid public links, + forbid sharing, forbid cross-drive moves, future: timeboxed sessions, + end-to-end encryption). + +The shift is large but the model is well-understood — it's the same +pattern Google Workspace's Shared Drives and Microsoft SharePoint +Document Libraries use. OxiCloud users coming from those services will +recognise it instantly. + +## Prerequisite — PR D-Prep: `access_grants → role_grants` + +**Ratified 2026-06-16.** A separate PR lands BEFORE D0 that refactors +`storage.access_grants` into `storage.role_grants` with role-bundle +semantics: + +```sql +storage.role_grants + id uuid PRIMARY KEY DEFAULT gen_random_uuid() + subject_type text NOT NULL CHECK (subject_type IN ('user','group')) + subject_id uuid NOT NULL + resource_type text NOT NULL -- 'folder','file','drive','group',… + resource_id uuid NOT NULL + role text NOT NULL CHECK (role IN ('viewer','editor','owner', …)) + expires_at timestamptz NULL + granted_by uuid NOT NULL FK → auth.users(id) + granted_at timestamptz NOT NULL DEFAULT now() +``` + +Roles map to permission bundles via a single +server-side function `role_bundle(role) -> &[Permission]`: + +| Role | Bundle | +|---|---| +| `viewer` | `Read` | +| `editor` | `Read, Create, Update, Comment` | +| `owner` | `Read, Create, Update, Comment, Delete, Share, Manage` | + +`Manage` is a new Permission introduced in this refactor — covers +"configure this resource's settings, add/remove members, change role +assignments". Future-friendly for Group-as-Resource. + +**Why first**: +1. Refactor has standalone value beyond Drive (cleaner API, audit + log, UI surface). +2. Doing it AFTER Drive would force migrating both `access_grants` + AND `drive_members` — solving the unification first means D0 + migrates one table, not two. +3. **The `drive_members` table planned by earlier drafts goes away.** + Drive membership becomes rows in `role_grants` with + `resource_type='drive'`. One table, one engine, one cache. + +**The sections below describe the Drive model assuming D-Prep has +landed.** References to "drive membership" mean rows in +`role_grants` with `resource_type='drive'`, not a separate table. + +**Gating policy**: D-Prep must ship, bake in production, and pass +validation across the API surface (grant endpoints, audit log +events) and the UI surface (My Shares dialog, share modals, admin +group/role views) before any of the Drive PRs (D0–D7) begin. The +goal is to surface and absorb refactor-related issues against the +existing single-resource model — not to discover them mid-flight +while the Drive migration is also in motion. If D-Prep needs +follow-up patches after deploy, they land as standalone fixes +before D0 starts. + +## Locked design (decisions ratified in design discussion) + +### 1. Ownership pivot + +- A new table `storage.drives` becomes the ownership anchor. +- `storage.folders` and `storage.files` lose their `user_id` column; + they gain `drive_id NOT NULL`. +- The owner of a resource is computed via `resource.drive_id → + drive.drive_members WHERE role='owner'` — a JOIN, not a denormalised + column. The drives table will be tiny (one row per internal user + + handful of shared drives), so the join is cheap with a proper index. +- **Single source of truth** confirmed. `user_id` is dropped from + resources after a migration phase (see Migration below). + +### 2. Drive membership + +Drive membership lives in `storage.role_grants` (introduced by PR +D-Prep — see prerequisite above) with `resource_type='drive'` and +`resource_id=`. There is **no** separate +`drive_members` table. One row per `(subject, drive)` pair carries +the role; the role expands to a permission bundle via the same +`role_bundle()` function used everywhere else. + +Concretely, a drive membership is: + +```sql +INSERT INTO storage.role_grants + (subject_type, subject_id, resource_type, resource_id, role, granted_by) +VALUES ('user', $user_id, 'drive', $drive_id, 'editor', $admin_id); +``` + +The `PgAclEngine` reads this row no differently than any other +grant; the drive-membership concept exists only at the application +layer (`DriveService` enforces the personal-drive invariants, etc.) +— there's no separate engine path or cache keyspace. + +- A **shared** drive (`kind='shared'`) can have **0..N user + owners + 0..N group owners** (mixed freely) plus any number of + editors/viewers. +- A **personal** drive (`kind='personal'`) has exactly **one** + member row in `drive_members`, fixed to `(subject_type='user', + subject_id=, role='owner')`. The rule applies to + every `kind='personal'` row, whether it carries + `default_for_user IS NOT NULL` (the user's default drive) or + `default_for_user IS NULL` (a secondary personal silo, e.g. a + SQL-created sibling root folder promoted by migration §10). + Secondary does **not** loosen the single-user constraint. + - Personal drives **cannot** be added to, role-changed, or have + their sole owner removed. They cannot be co-owned. They cannot + be transferred to another user. This matches Google Drive's + "My Drive" and Microsoft's "OneDrive" semantics: a personal + drive is a single-user namespace; collaboration happens through + per-resource grants, by moving content into a shared drive, + or by promoting a secondary personal drive to `kind='shared'` + (capability matrix in §3). + - Enforcement is at the application layer: + `DriveService::add_member` refuses when `kind='personal'`; + `remove_member` refuses when the drive is personal. User + deletion cleans up the user's **default** personal drive via + the `default_for_user` FK cascade, and the user's + **secondary** personal drives via an application-layer pass + (those have no FK to cascade through — see §6 lifecycle). +- **Shared-drive last-owner protection**: removing the final + `role='owner'` member of a shared drive is refused at the + application layer. The check counts remaining owner-role members + (expanding group owners to their members) and rejects the delete + if the count would become zero. +- A shared drive can have **0 viewers** and **0 editors** — only + the ≥1-owner invariant matters. + +### 3. Drive entity + +```sql +storage.drives + id uuid PRIMARY KEY DEFAULT gen_random_uuid() + name text NOT NULL -- "Personal" or user-chosen + kind text NOT NULL CHECK (kind IN ('personal','shared')) + default_for_user uuid NULL FK → auth.users(id) ON DELETE CASCADE + quota_bytes bigint NULL -- NULL = unlimited + used_bytes bigint NOT NULL DEFAULT 0 + policies jsonb NOT NULL DEFAULT '{}' + created_at timestamptz NOT NULL DEFAULT now() + updated_at timestamptz NOT NULL DEFAULT now() + -- `default_for_user` may ONLY be set on personal drives. + -- Shared drives have it NULL by definition. + CONSTRAINT drives_default_marker_personal_only + CHECK (default_for_user IS NULL OR kind = 'personal') + +CREATE UNIQUE INDEX drives_default_for_user_idx + ON storage.drives (default_for_user) + WHERE default_for_user IS NOT NULL; -- one DEFAULT personal drive per user +``` + +#### Two orthogonal properties: `kind` and `default_for_user` + +- **`kind`** = drive capability shape (see §2 for the rules): + - `'personal'` = single-user single-owner; cannot have members + added; sharing happens only through per-resource grants. + - `'shared'` = multi-owner / multi-member; supports user and + group members at every role. +- **`default_for_user`** = "this is the default drive for user + X". Set on exactly one `kind='personal'` drive per user (the + partial unique index enforces it). Used by: + - UI default-drive redirect (`/` → `/drive/`). + - NC default-drive resolution when the credential doesn't pin + a specific drive. + - The `ON DELETE CASCADE` on this column cleans up the user's + default drive in one step when the user is deleted. + - Canonical lookup: `SELECT id FROM storage.drives WHERE + default_for_user = $1` — single-row, index-backed. + +A user can have **multiple `kind='personal'` drives** — at most +one of them carries `default_for_user`, the others sit as +"secondary personal" silos (e.g. SQL-created root folders +promoted by migration §10). Secondary personals follow the same +single-owner rules as the default; they just aren't the default +landing target. + +External users **never** get a personal drive (lifecycle §6). +There is no constraint to write — externals simply have no row +in `storage.drives` with `default_for_user = `, and +nothing tries to create one. + +#### Drive naming — `name` is a label, identity lives in `kind` + `default_for_user` + +`name` is owner-editable for every drive (personal or shared). A +user who renames their drive from "Personal" → "Ed's space" does +**not** stop having a personal drive, and does not stop having a +default. The `kind` flag + `default_for_user` pointer carry the +identity; the name is purely a display label. + +Why this matters: +- UI and NC default-drive resolution MUST query on `kind` / + `default_for_user`, never on `name = 'Personal'`. The latter + would silently break the moment the user renames. +- The initial migration sets `name = 'Personal'` on the default + personal drive for back-compat with the label users see today; + further renames go through the normal drive-rename endpoint + and persist on the same row. Secondary personal drives carry + whatever name the sibling root folder had (e.g. `Archive`, + `2024 Projects`). + +#### Capabilities matrix + +| Capability | `kind='personal'`, default (`default_for_user` set) | `kind='personal'`, secondary (`default_for_user IS NULL`) | `kind='shared'` | +|---|---|---|---| +| Membership shape | exactly 1 user-owner row | exactly 1 user-owner row | 0..N users + 0..N groups at any role | +| `add_member` | refused | refused | allowed | +| `remove_member` | refused (sole owner is fixed) | refused (sole owner is fixed) | allowed except sole owner | +| Rename | allowed (by the owner) | allowed (by the owner) | allowed (by any owner) | +| Delete via API | **refused** — deleting this loses all the user's files; the only path is user-delete cascade | allowed (it's just a silo) | allowed (by an owner; CASCADEs the drive's contents) | +| Default-drive lookup result | this drive | never | never | +| On user-delete | `ON DELETE CASCADE` via `default_for_user` FK (free) | application-layer cleanup: enumerate via drive_members and delete | member rows referencing the user are dropped; refuse user-delete if any shared drive would lose its last owner | +| Group ownership | no | no | yes | +| Per-resource grant outward | yes (subject to drive policies) | yes | yes | +| Cross-drive move | yes (subject to `forbid_cross_drive_move`) | yes | yes | +| Kind conversion | no — always default-personal | yes → may be promoted to `kind='shared'` later (drops the single-user restriction, picks up members) | no | + +### 4. Roles → permission bundles + +Drive-level roles map to existing ReBAC `Permission` values via union +expansion: + +| Role | Permissions implied on every resource inside the drive | +|---|---| +| `viewer` | `Read` | +| `editor` | `Read`, `Create`, `Update`, `Comment` | +| `owner` | `Read`, `Create`, `Update`, `Comment`, `Delete`, `Share`, *and* drive-level admin (rename, edit policies, manage members, change quota) | + +### 5. Permission resolution — additive over `role_grants` + +A user has permission `P` on resource `R` if any of the +`role_grants` rows reachable from them (directly, via group +membership, or via drive membership) carries a role whose bundle +includes `P`. Specifically: + +- A direct grant: `(subject_type='user', subject_id=$user, resource_id=$R, role=…)` +- A group-mediated grant: any `role_grants` row whose + `subject_type='group'` points at a group the user is a transitive + member of. +- A drive-mediated grant: any `role_grants` row whose + `resource_type='drive'` and `resource_id=$R.drive_id`, with + the same per-row role bundle. + +Permissions are **additive**. Drive role is the baseline floor for +every resource in the drive; explicit per-resource grants only add +on top. There is no "revoke for this file even though you're a +drive editor" concept, matching Google Workspace. + +The auth engine reads `role_grants` as a single source of truth. +Cache invalidation has one keyspace (rows in `role_grants`) instead +of two. The drive-membership lookup ("what drives can this caller +read?") is a `WHERE subject_id=$1 AND resource_type='drive'` scan +against the same table. + +### 6. Lifecycle rules + +| Event | Behaviour | +|---|---| +| New internal user registers | Auto-create a default personal drive (`kind='personal'`, `name='Personal'`, `default_for_user=`, `quota_bytes=`), insert the single `drive_members (drive_id, user, , owner)` row. | +| External user invited (magic-link only) | **No personal drive created.** External users are grant-only recipients with no storage. | +| External user converts to internal (future flow) | Default personal drive created at conversion time. | +| User deleted | **Default** personal drive cascade-deletes via `ON DELETE CASCADE` on `default_for_user`. **Secondary** personal drives (`kind='personal' AND default_for_user IS NULL` and whose sole `drive_members` row points at the user) are deleted by an application-layer pass in the same transaction. Member rows referencing the deleted user are removed from all shared drives. If a removal would leave a shared drive with zero owners, deletion is refused — admin must transfer first. | +| Group deleted | Refuse if the group is a member of any shared drive that would lose its last owner. Admin must transfer or remove the group's role from those drives first. (Groups can't be members of personal drives.) | +| Add member to personal drive | Refuse. Personal drives are single-user — collaborate via per-resource grants or by moving content into a shared drive. | +| Remove sole owner of personal drive | Refuse. The only deletion path for a personal drive is user-deletion via cascade. | +| Delete personal drive | Refuse from the API. Only ON DELETE CASCADE (user deletion) drops it. | +| Rename personal or shared drive | Allowed for any owner-role caller. `name` is a label only. | +| Remove last owner of shared drive | Refuse — drive must always have ≥1 owner. App-layer check on `DELETE FROM drive_members`. | + +### 7. Quota model + +The per-user `auth.users.storage_quota_bytes` field is **migrated to +the user's personal drive's `quota_bytes`** in one step, then the +column is deprecated (kept for one release cycle as a no-op, dropped +in a later migration). + +After the cutover: +- Every drive owns its quota. Files inside a drive count against that + drive's `used_bytes` only. +- A user who collaborates in a 1 TB shared drive sees their personal + drive's quota as "their" quota; the shared drive's quota is owned + by the team. +- New drives default to a tenant-configured `OXICLOUD_DEFAULT_DRIVE_QUOTA_BYTES` + setting (separate env var, replacing today's per-user equivalent). + +`used_bytes` is maintained incrementally on every file insert/delete +(plus a periodic reconciliation job to fix drift, similar to the +existing per-user accounting). + +**Chunk dedup vs per-drive quota.** With the CDC chunk store landed +in v0.7.0 (see `delta_upload_service`, `upload_ingest`, instant +upload by hash), a single chunk can be referenced by files in +multiple drives. The accounting decision: **each drive counts the +file's logical size in full against its own `used_bytes`** — dedup +savings are server-side only and never visible in the per-drive +quota number. This matches the existing per-user blob-dedup model +and avoids the alternative "pro-rated quota" trap (which makes +quota math depend on cross-drive content and breaks the user's +mental model of "I have 1 TB free"). Reconciliation job sums file +sizes per drive, not chunk allocations. + +### 8. Policies (JSONB, extensible) + +Each drive carries a `policies` JSON object. Five known keys for v1: + +```jsonc +{ + "forbid_sharing": false, // disables per-resource grants on this drive + "forbid_external_sharing": false, // blocks grants to is_external=true subjects + "forbid_public_links": false, // blocks token-share (anonymous link) creation + "forbid_cross_drive_move": false // blocks MOVE when src.drive_id != dst.drive_id +} +``` + +Enforcement points (one place per policy — single grep target): + +| Policy | Enforcement callsite | +|---|---| +| `forbid_sharing` | `grant_handler::create_grant` — checks `resource.drive_id`'s policy before insertion | +| `forbid_external_sharing` | `magic_link_invite_service::resolve_or_create_recipient` and `grant_handler::create_grant` (when subject is `is_external=true`) | +| `forbid_public_links` | `share_handler::create_shared_link` | +| `forbid_cross_drive_move` | `file_handler::move_file` and `folder_handler::move_folder` — refuse when `src.drive_id != dst.drive_id` | + +Default to `false` (everything allowed) — opt-in by drive owner via +the drive settings UI. + +#### Policy semantics — subtleties to remember + +- **`forbid_sharing`** disables **per-resource** grants on resources + in the drive. Drive owners can still add **drive-level** members + (otherwise the drive becomes uneditable except by the original + owner). The policy means "no fine-grained sharing of individual + files; access happens through drive membership only". +- **`forbid_cross_drive_move`** protects against exfiltration via UI + move. It does **not** stop download + re-upload (that's a different + category of policy — file-egress, future). UI surface should make + this explicit so users don't read it as data-leak protection. + +#### Future policy keys (out of scope for v1 — but the JSONB shape +accommodates them without schema migration) + +- `timeboxed_session` — drive contents require re-auth after N + minutes. Significant UX/middleware lift; defer. +- `end_to_end_encrypted` — client-side encryption; massive scope. + +### 9. URL surface + +#### Frontend + +| URL | Resolves to | +|---|---| +| `/` | Redirect to the caller's personal drive UUID | +| `/drive/` | Drive root view | +| `/drive//` | Folder inside the drive | + +#### Native WebDAV (`/webdav/...`) + +| URL | Resolves to | +|---|---| +| `/webdav/` | Caller's personal drive root + `` (back-compat with today's behaviour) | +| `/webdav/drives//` | Specific drive root + `` | + +Today's `/webdav/` handler implicitly looks up the caller's +home folder and prepends it. Post-drives, the same handler looks up +the caller's personal drive and resolves paths inside it. **Zero +breakage** for existing native WebDAV clients. + +The `drives` path segment is **reserved**: a folder literally named +`drives` cannot exist at the top level of any drive. Migration +pre-check refuses to start if existing data violates this — operator +must rename before upgrading. (Conservative estimate: zero existing +folders are named exactly `drives`. The migration script reports any +collisions for manual fix-up.) + +#### NextCloud-compat WebDAV (`/remote.php/dav/...`) + +> **The path-segment `/drives//` form is NOT used on the NC +> surface.** It is reserved for the native WebDAV surface (see +> table above). The NC surface keeps the URL shape +> `/remote.php/dav/files//` and carries the drive +> selector in the **credential**, not the URL. +> +> NC desktop / mobile clients store credentials per +> `(host, username)` and offer a single sync root per saved +> account. A path-segment scheme would require NC clients to grow +> multi-root awareness, which they don't have. Two +> credential-side mechanisms are valid here: +> +> 1. **Username discriminator (`{user}~{drive-uuid}`)** — the +> chroot POC on `feat/nextcloud-drive` (commit `137169b7`). +> The Basic Auth username carries the drive UUID after a `~` +> separator; the URL stays under `/remote.php/dav/files/{user}~{uuid}/`. +> Explicit on the wire, no per-credential server state needed +> beyond the existing app-password row. +> +> 2. **App-password ↔ drive binding** — store the chosen drive +> UUID directly on the `auth.app_passwords` row at issuance +> time. The Basic Auth username stays as `{user}` (clean +> NextCloud UX, no `~` to explain). The auth middleware looks +> up the app-password row, reads its `drive_id` binding, and +> uses that as the drive context. Each drive a user wants to +> sync gets its own app-password. +> +> Both are workable; option (2) is the cleaner UX (username +> matches what users type, no extra character to explain) but +> requires a schema add on `auth.app_passwords` and one extra +> JOIN in the hot auth path. Option (1) is the smallest possible +> change but exposes the `~` to the user. They're not mutually +> exclusive — the issuance flow can produce credentials in either +> shape. Decide before D1 ships which is the **default** the +> Login Flow v2 picker produces. + +| URL | Resolves to | +|---|---| +| `/remote.php/dav/files//` | That user's personal drive — unchanged. (App-password drive-binding NULL ⇒ personal.) | +| `/remote.php/dav/files/~/` | Option 1: explicit drive in the URL. | +| `/remote.php/dav/files//` *(with `auth.app_passwords.drive_id` set)* | Option 2: drive resolved from the credential row. URL is indistinguishable from the unchanged personal case to the client. | + +In either case, the auth middleware asserts the caller is a member +of the resolved drive before serving any DAV verb. Pre-existing NC +clients pointed at `/remote.php/dav/files//` continue +syncing the user's personal drive without reconfiguration — +regardless of which option is chosen as the default. + +#### Username/UUID collision — defused + +The `/remote.php/dav/files//` vs `/remote.php/dav/drives//` +split solves the worry about username/UUID ambiguity. The +discriminator is the literal segment (`files` vs `drives`), never +the value of ``. A user happening to have a UUID-shaped username +is no longer a problem. + +### 10. Storage paths — wrapper folder retired + +Today `storage.folders.path` is e.g. `My Folder - admin/Docs`. The +"My Folder - admin" wrapper is the user's home folder, created at +registration via `format!("My Folder - {}", username)`. + +Post-drives, **the wrapper goes away**. The drive itself is the +root; folders and files that used to live inside the wrapper sit +directly under the drive with no intermediate folder: + +``` +Drive "Personal" (uuid=…, kind=personal, owner=admin) ← was the "My Folder - admin" wrapper +├── Docs/ +└── aa.pdf +``` + +Same for shared drives — they already had no wrapper: + +``` +Drive "Engineering" (uuid=…, kind=shared, owners=group:engineering) +├── Specs/ +├── Roadmap.md +└── archive/ +``` + +The two surfaces share one rule: **drive root = `parent_id IS NULL` +within the drive's `drive_id`**. The "personal vs shared" branch +disappears from path resolution — both kinds resolve the same way. + +#### Why this is client-safe + +The wrapper was already invisible to WebDAV / NC clients pre-drive: + +- NC clients hit `/remote.php/dav/files//` and + `nc_to_internal_path` prepended `My Folder - /` internally + before talking to the storage layer. The client never saw the + wrapper segment in its URL. +- Native `/webdav/` was implicitly chrooted to the user's + home by `resolve_webdav_path`. Same story. + +So URL-level back-compat is preserved trivially — clients keep +asking for `/remote.php/dav/files/admin/Docs/foo.pdf`, the +resolver no longer prepends the wrapper, and the storage row's +path is now `Docs/foo.pdf` instead of `My Folder - admin/Docs/foo.pdf`. +Net effect on the wire: zero. + +#### Why this is a better model + +The original plan kept the wrapper "for back-compat" but it has +no value beyond the storage layer (clients never see it, the +filesystem mirror is happy either way). Keeping it forced path +resolution to always know whether the caller is in a personal or +shared drive and conditionally prepend a segment. Dropping it +collapses that branch and makes the personal-vs-shared distinction +purely a metadata concern (kind, quota source, member shape) — +**not** a path-shape concern. + +#### Sibling root folders also become drives + +Today the only way to get a `parent_id IS NULL` folder is via the +home-folder creation path, which produces exactly one per user. +However, manual SQL has already been used (and may still be used +by ops) to create additional top-level folders — folders that sit +beside `My Folder - ` at `parent_id IS NULL`. These are +**not** addressable today through any handler (the UI assumes +exactly one root); they exist as data only. + +Post-migration, the model has to absorb them. Rule: + +- For each user, find every row with `parent_id IS NULL AND user_id + = `. +- The one named `My Folder - ` becomes the **default + personal drive**: `kind='personal'`, + `default_for_user=`, sole member = the user. +- Every **other** sibling becomes a fresh **secondary personal + drive**: `kind='personal'`, `default_for_user=NULL`, sole + member = the user, name carried over from the folder's `name` + column, quota initialised from the user's quota + (`auth.users.storage_quota_bytes`) — same default as the + user's primary personal drive. Membership rules from §2 apply: + the user cannot invite co-owners while the drive remains + personal. To open the silo up, the user can later **convert** + the secondary personal to `kind='shared'` (an + application-layer operation that flips the kind and lifts the + single-user restriction so the membership API can add other + users / groups). +- The folder row is deleted (the drive replaces it as the root). + Its children get their `parent_id` set to `NULL` *within their + new `drive_id`*. + +The chroot POC's "pick a drive at login" picker on +`feat/nextcloud-drive` already produces the right shape for this: +users with one drive auto-select Personal silently, users with +N drives get a real picker. No POC change needed — it just sees +real drive rows instead of folder UUIDs. + +### 11. Content search index — drive-aware filtering + +v0.7.0 added an embedded Tantivy full-text content index (see +`infrastructure/services/search_index/tantivy_content_index.rs` +and migration `20260701000000_content_search_index.sql`). Today +every indexed document carries the owning user as a filter field +and queries restrict by that field at query time. + +When ownership pivots to `drive_id`, the index has to follow — +otherwise search leaks content across drives the moment D7 drops +`user_id`: + +1. **Schema update**: every indexed document gains a `drive_id` + field stamped at ingest time. Existing documents need a + one-shot reindex pass during the D0 migration (read each row, + look up its new `drive_id`, update the index entry). Cheap on + small instances; the migration script should report a progress + count for larger ones. +2. **Query path**: instead of filtering by `user_id = caller`, + expand `caller → set of drive_ids the caller can read` + (personal + every shared-drive membership) and filter by + `drive_id ∈ that set`. Expansion reuses the drive-role check + already required by `PgAclEngine`. +3. **Treat as a blocking step of D0**, not a D4/D5-era polish + item — otherwise the index is the silent leak path during the + dual-write window. + +Filtering on a low-cardinality `drive_id` is something Tantivy +handles natively; this is bookkeeping, not a query-plan risk. + +### 12. Trash — per-drive scoped, owner-actioned + +Today trash is per-user: `storage.files` / `storage.folders` +carry `is_trashed BOOLEAN` + `trashed_at` + `original_parent_id` +(soft-delete in place), and `storage.trash_items` is a VIEW that +UNIONs them. The listing endpoint (`GET /api/trash/resources`) +filters by `user_id = caller`. + +Post-drives, trash becomes **per-drive**: + +- **Storage shape is unchanged.** The `drive_id` column added to + `storage.files` / `storage.folders` in Phase A already + identifies which drive a trashed row belongs to. No new + trash table, no schema work beyond updating the + `storage.trash_items` VIEW to surface `drive_id` alongside + (or replacing) `user_id`. +- **Trash listing query** filters by drive(s) the caller can + read. Default listing returns trash from every drive the + caller has membership on; a `?drive_id=` parameter + scopes to one drive. UI shows a drive picker above the trash + list, same as the main file view. +- **Trash mutations are owner-only.** Per the §4 role-bundle, + `Delete` is in the owner bundle only — so today's "anyone + who can delete the original can act on its trash entry" is + already drive-owner-scoped. Specifically: + - **Send to trash** — any drive owner (carries `Delete`). + Personal drive: the user themselves. + - **Restore** — any drive owner. Operation reverses + `is_trashed`, sets `parent_id` back to `original_parent_id` + when that ancestor is still in the same drive (otherwise to + the drive root with a name conflict resolver). + - **Permanent delete** — any drive owner. Clears the row and + decrements drive `used_bytes`. + - **View trash** — any drive member (viewer / editor / + owner). Viewers can see what was deleted from a drive they + have access to; only owners can act on it. Mirrors Google + Drive's per-shared-drive trash UX. +- **Cross-drive moves carry their trash home with them**: + when a file moves from drive A → drive B and is later + trashed, the row's `drive_id` is B's, so trash for B sees + it (not A's, which is the natural and expected answer). +- **Cascade on drive deletion**: when a shared drive is + deleted (D3 will land the delete-drive flow), every + `storage.files` / `storage.folders` row with that + `drive_id` cascades, trashed or not. There's no need to + "drain the trash first" — the whole drive disappears in + one CASCADE. +- **Personal-drive trash** follows the same model: bound to + the personal drive, sole owner (= the user) does + everything. No new UX divergence between personal and + shared. + +The orphan/aborted-upload sweep introduced in v0.7.0 +(`944c8337`, periodic trash job) must become drive-aware so +it doesn't accidentally sweep across drives the caller +shouldn't see. It already keys off ownership; the rewrite is +a per-drive pass instead of per-user. + +### 13. Upload paths and quota timing + +Four upload protocols, each with different "when do we know the +size" and "when do we know the destination" answers. The Drive +migration pivots quota from user-scoped to drive-scoped without +changing protocol shapes — but one path (NC chunked) carries a +pre-existing wart that the chroot POC's `~` username (or the +`app_passwords.drive_id` binding) lets us finally fix. + +| Protocol | Size known | Quota check fires | Destination / drive known | +|---|---|---|---| +| Default multipart (`POST /api/files/upload`) | At request start (Content-Length / multipart `size`). | `file_upload_service.rs:185` — `check_storage_quota(caller_id, metadata.size)` before any bytes are stored. | At request start (form field `folder_id`). Drive derives from `folder.drive_id`. | +| Native chunked (`POST /api/uploads`) | At session create — client declares `total_size` in JSON. | `chunked_upload_handler.rs:213` — at session creation against declared `total_size`. | At session creation (`folder_id` in JSON). Drive derives from `folder.drive_id`. | +| **NextCloud chunked** (`/remote.php/dav/uploads/{user}/{session}/...`) | Never declared. MKCOL creates empty session, PUT chunks arrive one at a time, client decides "done". | **Today: only at the final MOVE (assemble)** — `handle_assemble` → `file_upload_service::ingest_stream_to_cas` → quota check on the assembled size. Wasted-bandwidth wart: a client over quota can upload GB before the server can refuse. | **Today**: only at MOVE (parsed from the `Destination:` header). **With the chroot POC** (`{user}~{drive-uuid}` username, see §9): known at MKCOL — the auth middleware already split the username. **With `app_passwords.drive_id` binding**: known at MKCOL — the credential row pins the drive. | +| Delta protocol (`/api/files/delta/*`) | At `negotiate` (client provides manifest with `total_size`). | `delta_upload_service.rs:331` — at commit against `total_size`. | At negotiate (target file_id or `folder_id`). Drive derives accordingly. | + +#### Decision — per-chunk incremental quota check on the NC chunked path + +The three non-NC paths trivially pivot to drive-scoped quota: +replace `check_storage_quota(caller_id, size)` with +`check_drive_quota(drive_id, size)`. Destination is known at +handler entry; drive falls out of the destination's `drive_id` +column. No protocol change. + +For NC chunked, the Drive migration **also closes the +wasted-bandwidth wart** because the drive identity is now known +at MKCOL (via `~` username or app-password binding — either NC +credential-side scheme from §9 surfaces it). Approach: + +1. **MKCOL guard** — if `drive.used_bytes >= drive.quota_bytes`, + refuse the session creation with `507 Insufficient Storage`. + No point letting the client even start. +2. **Per-chunk PUT check** — track cumulative bytes received in + the session (sum of on-disk chunk sizes, maintained by the + chunked-uploads service). On each PUT, before writing the + chunk: + ```text + if drive.used_bytes + session.bytes_so_far + chunk.size > drive.quota_bytes: + refuse with 507 Insufficient Storage + ``` + The first chunk that would push us over is refused; client + sees the error within one chunk's worth of wasted upload + (typically a few MB) instead of after the whole multi-GB file. +3. **Assemble-time re-check** stays as a defence-in-depth (in + case two concurrent sessions on the same drive each got past + the per-chunk check but their sum exceeds quota at MOVE). This + matches today's structure. +4. **Unlimited quota** (`quota_bytes IS NULL`) short-circuits all + three checks — no work. + +The per-chunk check is O(1) amortised: each session tracks its +cumulative size as it goes. The drive's `used_bytes` is read from +the row once per chunk; with the v0.7.0 incremental-update +pattern (`b5b80549`, `d6987329`) that's a single primary-key +lookup, not an aggregate query. + +Net effect on NC clients: nothing changes for in-quota uploads; +over-quota clients get a clear 507 within seconds instead of +after the whole upload finishes. + +#### Editor-role delete and trash — call out the UX tension + +§4's role bundle gives `Delete` only to `owner`. That means +in a shared drive, an editor who uploads a typo file CANNOT +send it to trash themselves — they have to ask an owner. This +is already flagged as Open Question 4 (revisit before D2) and +the answer there determines the trash UX for editors too. If +editors get a `Delete` capability (or a dedicated "trash own +content" capability), the trash mutation rules become "trash +your own files" + "drive owners can act on anyone's trashed +files". Until that's decided, the conservative answer above +(owner-only mutations) holds. + +### 14. File provenance — `created_by` / `updated_by` + +Today `storage.files.user_id` (and `storage.folders.user_id`) +quietly doubles as **both** the ownership pointer (whose files are +these?) AND the provenance signal (who created this?). The Drive +migration cleanly separates ownership onto `drive_id`, but +provenance has to come with us — in a shared drive where Ed, +Alice, and Bob all upload files, "who put this here?" remains a +load-bearing question for UI, audit, and account-cleanup workflows. + +The split: + +```sql +-- on both storage.folders and storage.files: +drive_id uuid NOT NULL -- ownership +created_by uuid NULL FK → auth.users(id) ON DELETE SET NULL +created_at timestamptz NOT NULL DEFAULT now() -- existing +updated_by uuid NULL FK → auth.users(id) ON DELETE SET NULL +updated_at timestamptz NOT NULL DEFAULT now() -- existing +``` + +- **`created_by`** is set once at row insert (every upload path: + multipart, native chunked, NC chunked, streaming CDC, delta, + instant-upload-by-hash) — the caller_id stamps it. Never + changes after. +- **`updated_by`** is set whenever `updated_at` is touched — + rename, overwrite, move, restore from trash, PROPPATCH on + favorites. Same write-path discipline as `updated_at` today. +- **`ON DELETE SET NULL`** on both FKs. When a user is deleted, + files they created or edited in shared drives stay (they belong + to the drive, not the deleter); the FK nulls out, the UI renders + "Unknown user" (or "Deleted user" with a small tombstone table — + future polish, see Open Question 12). + +#### Why this lands in D0, not D7 + +If we waited until D7 to introduce these columns, every existing +file/folder row would have NULL `created_by` after the migration — +provenance permanently lost for pre-D7 content. Adding the columns +in D0 and **backfilling from `user_id`** during the dual-write +phase gives every existing row a real `created_by` value (the user +we know created it, because that's what `user_id` meant). New +writes during the dual-write window populate both `user_id` AND +`created_by` / `updated_by`. By D7 the columns are self-sufficient +and dropping `user_id` loses nothing. + +#### UI display semantics + +- File details panel: "Created by Alice on 2026-01-15", "Last + edited by Bob 3 hours ago" — both fields drive a normal user + avatar + name lookup. NULL → "Unknown user" placeholder. +- Activity log on a shared drive: aggregates `updated_by` over + recent rows to show "who's been active here lately". +- Account-cleanup workflow: when an admin deletes a user, show a + pre-flight summary "Bob authored 47 files and last edited 12 + more across drives X, Y, Z — proceeding will mark those entries + as authored by 'Deleted user'." Lets the admin choose to + reassign or simply confirm. + +#### Engine touch-point + +The "who touched updated_at" rule applies uniformly across all +mutation paths. Centralise it: every service that bumps +`updated_at` must also set `updated_by = caller_id` in the same +SQL statement. The `FileRepository::update_*` and +`FolderRepository::update_*` methods are the natural choke points +— a single audit reveals every callsite to verify. + +The async `tree_etag_queue` (v0.7.0) propagates an etag up the +ancestry but **does NOT** propagate `updated_by` — ETags are +fingerprints of structure, not authorship. Only the direct +mutation site updates `updated_by`. + +## Migration strategy + +A drive-id column on every resource is a database surgery touching +every storage query. We phase it for safety: + +### Phase A — additive (PR D0) + +1. Create `storage.drives` and `storage.drive_members`. +2. Add `drive_id uuid NULL` to `storage.folders` and `storage.files`. +3. **Per-user root-folder sweep**: for each internal user, list + every `storage.folders` row where `parent_id IS NULL AND + user_id = `. Exactly one is expected to be + `My Folder - `; any extras are SQL-created siblings + (see §10). + - The `My Folder - ` row → becomes the **default + personal drive**: `INSERT INTO storage.drives (name='Personal', + kind='personal', default_for_user=, quota_bytes=)` + and insert one `(drive_id, user=, role='owner')` + member row. + - Every other sibling row → becomes a fresh **secondary + personal drive**: `kind='personal'`, `default_for_user=NULL`, + name carried over from the folder's `name`, + `quota_bytes=`, and one + `(drive_id, user=, role='owner')` member row. + Membership rules from §2 apply (single-owner, no `add_member`); + the user can later promote one to `kind='shared'` to invite + collaborators. +4. **Promote children, drop the wrapper**: for every folder/file + row that has `parent_id = `, set + `drive_id = ` and `parent_id = NULL` + (the drive itself is the new root, not a folder). Then DELETE + the root folder rows from step 3 — they no longer exist as + folders, the drive replaces them. +5. **Cascade `drive_id` down the tree** — for every remaining + folder/file row, set `drive_id` by walking the ancestry to + whichever root the row descends from. After this step every + row has the same `drive_id` as its `parent_id`'s row, which + chains up to a drive set in step 3/4. +6. **Full path-metadata reconstruction**. The `path` column on + **every** row in `storage.folders` and `storage.files` gets + rewritten. For rows that descended from `My Folder - `, + strip that prefix; for rows that descended from a sibling root, + strip that sibling's `name`. The path column now contains only + the in-drive path (e.g. `Docs/foo.pdf`, never + `My Folder - admin/Docs/foo.pdf`). + - **This is the bulk of D0's runtime cost.** Personal-drive + scope = every folder/file the user owns. A 100k-file user + gets 100k UPDATEs. Use a single `UPDATE … WHERE drive_id = + ` per drive, not a row-at-a-time loop. The ltree-path + change is what every downstream subsystem keys off, so doing + this in one transaction per drive (not per row) is also a + correctness boundary. + - **Downstream caches and indexes** — audit each for path or + path-derived keys: + - **Tantivy content index (§11)** — the index does NOT + store paths (see `tantivy_content_index.rs`: indexed + fields are `file_id`, `user_id`, `name` (basename only), + `content`. No `path` field, the wrapper folder name was + never a term). So the wrapper removal alone requires no + reindex. The reindex §11 calls for is driven by the + schema gaining `drive_id` and the query filter pivoting + from `user_id` to `drive_id` — NOT by the path rewrite. + Same migration window, but for a different reason. + - **Thumbnail cache** — if keyed by path rather than + file_id, invalidate; preferably switch to file_id-keyed + during this migration so the issue doesn't recur. Audit + before D0 starts. + - **Folder ETag queue (`async_tree_etag_queue`,** see Open + Question 8) — flush or recompute; ETags derived from old + paths are stale. + - **Recent-items / favorites** — referenced by file_id, not + path; probably fine. Verify. + - **On-disk storage mirror** — see Open Question 10. If the + filesystem layout is path-mirrored, every file moves on + disk too; if content-addressable, the FS is untouched. + Audit before D0 starts. +7. Verify: every row has `drive_id IS NOT NULL`, no row has + `parent_id` pointing at a non-existent folder, no `path` value + contains the legacy `My Folder - ` prefix. +8. Add `NOT NULL` constraint on `drive_id`. + +**Keep `user_id`** on resources alongside `drive_id` for the entire +Phase A release cycle. Code is updated to read `drive_id` everywhere; +`user_id` is dual-written for one release as a safety net. If +something breaks we can roll back without data loss. + +Pre-flight checks the migration script runs before any writes: +- Report how many sibling-root folders exist per user. Don't + refuse — extras become drives — but surface the count so an + operator can sanity-check ("Ed has 4 root folders, expected + ≤1; verify those are real and intended before promoting them + to drives"). +- Refuse if any sibling root folder is literally named `drives` + (would collide with the reserved URL segment on the native + `/webdav/drives//...` surface). Operator renames first. +- Refuse if any user has `storage_used_bytes > storage_quota_bytes` + by an amount that wouldn't fit the destination drive's quota + semantics (sanity check). + +### Phase B — cleanup (PR D7, one release later) + +1. Drop `user_id` from `storage.folders` and `storage.files`. +2. Drop dual-write code paths. +3. Deprecate `auth.users.storage_quota_bytes` (or drop — quotas live + on drives now). + +Phase B is the point of no return; deferring it by one release gives +us a real rollback window while the new model bakes in production. + +## PR sequencing + +| PR | Scope | Risk | +|---|---|---| +| **D-Prep — role_grants refactor** | `access_grants → role_grants` schema migration with role-bundle semantics. `Manage` Permission added to the enum + role bundle. Engine reads role_grants only; `access_grants` removed (after one dual-write release if compat is needed). API gains `role` parameter on grant endpoints; audit log emits one `role_grant.*` event per role assignment instead of N permission events. **No Drive concept yet.** Sets the foundation that all subsequent PRs build on. **Data shape confirmed**: empirical audit shows >99% of existing `access_grants` rows already cluster into the standard bundles (viewer/editor/owner) — the migration is mechanical for the vast majority of data; the <1% edge cases get absorbed by shipping `commenter` and `contributor` roles on day one or get an explicit per-row migration decision logged. | **Medium** — touches the load-bearing authorisation table, but the data shape removes the main migration risk | +| **D0 — foundation** | `storage.drives` schema (no `drive_members` — uses `role_grants` from D-Prep); `Drive` domain entity; migration creating personal drives + backfilling `drive_id` on every resource; read-only `GET /api/drives` listing the caller's drives (single query: `SELECT … FROM role_grants WHERE subject_id=$caller AND resource_type='drive'`). Dual-write `user_id` alongside `drive_id` for safety. **No new UI.** **Every upload path stamps `drive_id` at insert**: classic multipart (`file_handler::upload`), chunked NC (`uploads_handler`), streaming CDC (`upload_ingest`), delta upload (`delta_upload_service`), instant upload by hash. Tantivy reindex (see §11) is part of this PR. **Provenance columns added** (see §14): `created_by` and `updated_by` on both `storage.folders` and `storage.files`, FK to `auth.users` with `ON DELETE SET NULL`; backfilled from `user_id` so pre-Drive content has provenance from day one; every mutation path that touches `updated_at` also sets `updated_by`. | **High** — every storage query touches, all upload paths touched | +| **D1 — UI switcher + URL routing** | Sidebar drive picker, `/drive//` frontend routes, default-drive redirect from `/`. WebDAV path dispatcher recognising `drives/` as the drive-explicit prefix on both `/webdav/` and `/remote.php/dav/`. | Medium | +| **D2 — drive membership API + per-drive trash auth** | `POST /api/drives/{id}/members`, `DELETE`, `PUT` for role changes — thin handlers that translate to `role_grants` INSERT/DELETE/UPDATE with `resource_type='drive'`. `Resource::Drive(Uuid)` (added in D-Prep at the enum level) gets its specialised handler surface here. Shared-drive last-owner protection. Group-as-subject support reuses the existing `subject_groups` machinery. **Personal-drive guards** (`add_member`, `remove_member`, `delete_drive` refuse on `kind='personal'` — see §2). **Per-drive trash authorisation** (§12): trash listing filters by drive(s) the caller can read; trash mutations (send/restore/permanent-delete) require `role='owner'` on the drive; `storage.trash_items` VIEW updated to surface `drive_id`; orphan/aborted-upload sweep becomes per-drive. | Medium | +| **D3 — group-owned shared drives** | "Create shared drive" flow — admin or group owner triggers, drive created with `kind='shared'`, initial owner row is the group. Group-deletion guard refuses if the group is the last owner of any drive. Drive-rename, drive-delete. | Low | +| **D4 — per-drive quota** | Move storage accounting off `auth.users.storage_used_bytes` onto `storage.drives.used_bytes`. **Re-point the existing per-user incremental CTE** (introduced in v0.7.0 — see `b5b80549`, `d6987329`) at drive rows; don't reinvent the counting logic. Upload paths check `drive.quota_bytes` instead of (or in addition to) the user's quota for the dual-write window. **Per-chunk incremental quota check on the NC chunked path** (see §13): MKCOL refuses when the drive is already over quota; each PUT chunk runs an O(1) `used + session_so_far + chunk_size > quota` test and refuses with 507 within one chunk of wasted upload. Closes a pre-existing wart where NC clients could upload GB before learning they were over quota. Reconciliation job runs once per day to fix drift. | Medium | +| **D5 — policies** | JSONB policies column + enforcement at the four known callsites. Owner-only UI in drive settings. Ship policies one at a time if you want fine-grained rollout — `forbid_public_links` first (lowest risk), then `forbid_external_sharing`, then `forbid_sharing`, then `forbid_cross_drive_move`. | Low | +| **D6 — cross-drive move + audit** | Move folder/file between drives (allowed by default; gated by `forbid_cross_drive_move` policy on the source drive). Audit events for every drive lifecycle event (`drive.created`, `drive.member_added`, `drive.member_removed`, `drive.policy_changed`, `drive.deleted`, `resource.moved_between_drives`). | Low | +| **D7 — back-compat sweep** | Drop `user_id` from `storage.folders` / `storage.files`. Drop dual-write code. Drop or deprecate `auth.users.storage_quota_bytes`. **Provenance columns (`created_by`, `updated_by`) stay** — they were populated from D0 and are now the sole source of authorship signal. | Low — but the point of no return | + +Approximate total: 4–6 weeks of focused work, depending on test +coverage depth. + +## Out of scope for v1 (worth noting so we don't accidentally invite scope creep) + +- **Timeboxed session policy** — drives that auto-lock after N + minutes. Big middleware lift. +- **End-to-end encryption policy** — client-side encryption with + server holding only ciphertext. Massive scope. +- **Per-drive sync targets** — sync client design ("1 drive = 1 + target like 'family-computer'"). Schema-friendly today (drive has + stable UUID), implementation is its own design. +- **Cross-drive search UI** — start with per-drive scoping. "Search + everywhere" comes later. +- **Drive templates** — "create a drive pre-populated with these + folders" is nice but not foundation work. + + + +## Open questions to revisit before D0 lands + +1. **Naming of the default personal drive**. + - Locked: "Personal" (i18n key, neutral, doesn't break on username + rename). + - But: should it be renameable by the user? Default-rename allowed? + I'd say yes — drive name is just a label. + +2. **`auth.users.storage_quota_bytes` — drop or keep?** + - Drop entirely (cleanest). + - Keep as the **default initial quota** for newly-created personal + drives (so admins still have a single tunable). + - I lean toward keep-as-default-initial. + +3. **Can a user be a member of their own personal drive in more than one + way?** (e.g. directly AND via a group they belong to) + - Should the membership rows allow that? Or dedup? + - Easy answer: allow; permission resolution naturally unions, so + duplicate paths to the same role are idempotent. + +4. **What happens when you DELETE a folder/file in a shared drive + you have only editor role on?** + - Today: ReBAC checks Delete permission on the resource. + - With drives: editor role implies Create + Update + Comment but + **not** Delete (per the role-bundle table above). + - So editor cannot delete by default. Should they? Some teams want + "editor can do anything except change drive settings". Worth a + dedicated decision before D2 — possibly add a separate "can + delete" toggle, or split editor into `editor` / `contributor`. + +5. **Drive icons / colour customisation** — visual differentiator + between drives in the sidebar. Out of scope for v1 but worth + noting; the `drives` table can carry a small `display` JSONB column + that accumulates this kind of cosmetic config without schema + churn. + +6. **WebDAV path resolution edge case**: when a user is a member of a + shared drive and someone shares a single file inside it explicitly + with them via a per-resource grant, what path do they see in their + client? + - The drive's path (they have access via membership), full stop. + - Per-resource grants are additive; they don't create a separate + "shared with me" listing for files that already live in a drive + the user can access. + - Confirm during D2 that this is what users expect. + +7. **Search-everywhere from the file picker**. When a user goes to + share a file, today the picker lists their own files. With drives, + should the picker default to "current drive" or "all drives I have + access to"? UX call, not a foundation decision. + +8. **`async_tree_etag_queue` — drive-pinning assumption audit**. + v0.7.0 introduced `migrations/20260626000000_tree_etag_statement_triggers.sql` + + `20260627000000_async_tree_etag_queue.sql` for ETag + propagation up folder ancestry. The queue likely keys off + folder path / owner; before D0 starts, confirm it doesn't + embed assumptions that fold `drive_id` away (e.g. computing + ancestry across a path that crosses a drive boundary). + 5-minute audit to lock the question down. + +9. **NC credential ↔ drive binding default**. Section 9 leaves + open whether the Login Flow v2 picker defaults to issuing + `{user}~{uuid}` Basic Auth usernames (option 1) or + `auth.app_passwords.drive_id`-bound app-passwords (option 2) + for non-personal drives. Decide before D1 ships; the answer + determines whether `auth.app_passwords` gets a new column or + not. + +10. **On-disk storage mirror — does the file path under + `OXICLOUD_STORAGE_PATH` change too?** Phase A step 6 strips + the `My Folder - /` prefix from `storage.folders.path` + / `storage.files.path` columns. If the on-disk layout mirrors + these paths (`//My Folder - admin/Docs/foo.pdf`), + the migration also has to `mv` every file on disk. If on-disk + is content-addressable (BLAKE3-keyed), the columns can be + rewritten without touching the filesystem. **Audit the + storage adapter before starting D0** and decide whether the + migration script: + - just renames the path columns (CAS layout — cheap), or + - renames the path columns AND issues a `mv` per file + (path-mirrored layout — expensive on big instances). + The blob store is content-addressable as of v0.7.0 so most + file content lives under `.blobs//` and is + already wrapper-agnostic; the concern is only the + metadata-projection / thumbnail-cache trees if they're + path-shaped. + +11. **Promoting sibling root folders — UX after migration**. + Users whose home was a single `My Folder - ` end + up with one drive (Personal). Users with SQL-added siblings + end up with multiple drives, one named per the original + folder name. The drive name carries over verbatim — should + the migration log who got more than one drive, so an + operator can DM them and explain the new picker? Operational + nicety, not a correctness concern. + +12. **Deleted-user tombstone for provenance**. §14 sets + `created_by` / `updated_by` to NULL when the referenced user + is deleted (`ON DELETE SET NULL`). The UI then shows "Unknown + user", which is correct but information-poor — for compliance + / audit / "who left this?" purposes, a small `deleted_users` + tombstone table (`id, last_known_username, deleted_at`) would + let the UI render "Bob (deleted 2026-04-12)" instead of just + "Unknown user". Out of scope for v1 but easy to add later + without schema rework (NULL `created_by` stays NULL; a + parallel lookup against the tombstone table on display). + +## Existing code to reuse + +- **ReBAC `Resource` enum** at `src/domain/services/authorization.rs:74` + already has a tagged-union shape; adding `Drive(Uuid)` is one variant + + `from_parts` arm + `type_str` arm. +- **ReBAC `Permission` enum** already has the bundle we need + (`Read`, `Create`, `Update`, `Delete`, `Share`, `Comment`). The + role-to-bundle mapping for drives lives in a new `drive_role.rs` + helper. +- **`PgAclEngine` Moka cache** at + `src/infrastructure/services/pg_acl_engine.rs:88` handles ReBAC + grant caching. The drive-role check piggybacks on the same cache + with a separate keyspace (`drive_perm:::`). +- **`SubjectGroupService::list_transitive_users`** at + `src/application/services/subject_group_service.rs:385` already + expands a group to its transitive user members. The drive-membership + check uses this to resolve group-owner subjects. +- **`folder_service::create_home_folder`** at + `src/application/services/folder_service.rs:644` is where the + per-user wrapper folder is created today. Post-migration this + function **goes away** — there is no wrapper folder anymore. The + user-create lifecycle hook now creates a Drive row directly and + inserts the owner-role member row. The lifecycle path is the same; + the work it does shrinks. +- **NC path resolver `nc_to_internal_path`** at + `src/interfaces/nextcloud/webdav_handler.rs:51` and the native + resolver `resolve_webdav_path` at + `src/interfaces/api/handlers/webdav_handler.rs:188` are the two + callsites that learn about drives. Both gain a "drive context" + parameter resolved from the URL prefix (`/files//` or + `{user}~{uuid}` for NC; `/webdav/` or `/webdav/drives//` + for native). **Neither resolver prepends `My Folder - /` + anymore** — the storage path IS the in-drive path. Both + functions also get simpler, not more complex, despite gaining + the drive parameter (the personal-vs-shared branch is now a + metadata lookup, not a path-shape decision). +- **`MagicLinkInviteService`** and the share-notification pipeline + (`RecipientNotificationService`) need the new policy checks + (`forbid_external_sharing`, `forbid_sharing`) wired in at their + respective callsites. +- **Migration timestamp convention**: as of v0.7.0 the head + migration is `20260702000000_drop_dead_file_indexes.sql`, and + `20260701000000_content_search_index.sql` already exists (the + Tantivy index). D0's migration becomes `20260801000000_drives.sql` + (or whatever date D0 actually starts) — the original + `20260701000000_drives.sql` slot is taken. + +## Verification + +Each PR carries its own verification block. The bar across all of +them: **(a)** new behaviour proven by a focused test, **(b)** the +existing Hurl + Playwright baselines still pass green (`bash +tests/api/run.sh && bash tests/webdav/run.sh && cd tests/e2e && npm +test`), **(c)** `cargo fmt && cargo clippy --all-features +--all-targets -- -D warnings` clean. + +### D-Prep +- **Unit**: `role_bundle(role)` returns the expected permission set + for each role; `roles_implying(permission)` returns the expected + role list; round-trip a grant insert→read→compare for every role. +- **Data audit**: query `access_grants` and confirm that >99% of + rows cluster into the standard bundles (the empirical figure that + unlocked Sequence A). Log the <1% edge cases per row for review. +- **Migration round-trip**: roll forward against a populated DB → + every prior `access_grants` row is represented by exactly one + `role_grants` row → roll back → original `access_grants` rows + recovered byte-identically. +- **API**: new Hurl test `tests/api/role_grants.hurl` — + - `POST /api/grants` with `role='editor'` creates a single row + with the expected bundle when expanded. + - `PUT /api/grants/{id}` with `role='viewer'` is atomic (no race + window where the user has zero permissions). + - Compat shim: `POST /api/grants` with the legacy `permission` + field still works for one release and maps to the closest role. + - Audit log emits `role_grant.created`, `role_grant.role_changed`, + `role_grant.revoked` events with the role name carried through. +- **UI smoke (Playwright)**: My Shares dialog renders roles instead + of permission checkboxes; share modal preset buttons map to roles; + changing a member's role from editor → viewer fires exactly one + PATCH and the UI reflects the change without a race-window blank + state. +- **All existing Hurl + WebDAV + Playwright tests still pass** — the + refactor must be invisible to every non-grant-handling test. + +### D0 +- **Unit**: `Drive` entity (kind + `default_for_user` CHECK + constraints), `default_for_user` partial unique index. Personal- + drive `add_member` / `remove_member` / `delete_drive` all refuse + cleanly with the expected `DomainError`. +- **Migration round-trip**: roll forward against a populated DB → + every existing folder/file row has `drive_id` set (no NULLs); + every user has exactly one drive with `default_for_user` set; + sibling root folders became secondary `kind='personal'` drives; + every `storage.folders.path` and `storage.files.path` value has + the `My Folder - /` prefix stripped → roll back via + `sqlx migrate revert` → `drive_id` column gone, `user_id` intact + thanks to dual-write, original paths recovered. +- **Storage check**: post-migration `bash tests/api/storage_cleanup_check.sh` + still reports a clean tree (no orphans). +- **Tantivy reindex**: every indexed doc carries a `drive_id`; + search query filtered by `drive_id ∈ caller's drives` returns the + expected hits; cross-drive isolation confirmed (search as Alice, + Bob's content never appears). +- **API**: `tests/api/drives_foundation.hurl` — admin lists + `/api/drives`, sees their default personal drive with the correct + quota, `kind='personal'`, `default_for_user` matching their own + uuid. Creating a folder still works; the folder's `drive_id` + matches the personal drive. +- **All upload paths set `drive_id`**: targeted Hurl tests for + multipart, native chunked, NC chunked, delta upload, instant + upload by hash. Each verifies the resulting file row has the + expected `drive_id`. + +### D1 +- **Routing**: `cargo build` clean; WebDAV dispatcher routes + `/webdav/drives//...` correctly; `/webdav/` still + resolves to the caller's default drive (back-compat). +- **NC client back-compat**: a real NC sync client pointed at + `/remote.php/dav/files/admin/` continues syncing the user's default + personal drive without reconfiguration. The chroot POC's `~` + username (or app-password binding) lands a sync into the chosen + drive transparently. +- **Manual smoke**: open `/`, get redirected to + `/drive/`. Click sidebar drive switcher → URL + updates, listing reloads. Drive picker shows all of the caller's + drives (default first), each with its quota usage. +- **Playwright**: a new `tests/e2e/drive-switching.spec.ts` exercises + sidebar → URL → listing → cross-drive isolation (folders in + drive A don't appear in drive B's listing). + +### D2 +- **Membership API**: `POST /api/drives/{id}/members` with user, with + group, and with role changes; refuses on personal drives; + shared-drive last-owner protection. Group expansion (transitive + members count toward the owner total). +- **Trash per drive**: trash listing scopes correctly to drive(s) + the caller can read; mutations refuse without owner role; the + `storage.trash_items` VIEW surfaces `drive_id`. +- **Updated NC + WebDAV regression baselines** — the 105+ scenarios + in `BASELINE_TESTS_NC_WEBDAV.md` still pass green; per-drive + trash and membership changes don't regress existing protocol + behaviour. + +### D3–D7 +- Per-PR verification authored when the PR is drafted. Each PR adds + at least one new Hurl/Playwright test for the headline capability + and proves zero regression on the previous baselines. + +### Cross-cutting regression — runs on every Drive PR + +A standing checklist independent of the headline capability of each +PR: + +1. `bash tests/api/run.sh` green (>100 scenarios). +2. `bash tests/webdav/run.sh` green (NC + native WebDAV baselines). +3. `cd tests/e2e && npm test` green (Playwright). +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. + +## UI design — outline for D1 and D3 + +The Drive concept reshapes three load-bearing UI elements. Each +gets a small design pass alongside the relevant PR — the bullets +here lock the **intended shape**; the pixel work happens when D1 / +D3 draft. + +### Sidebar — drives as children of a "Drive" section + +Today the sidebar shows a single root view (the user's home +folder). Post-Drive it gains a top-level **Drive** section whose +children are the drives the caller has access to: + +``` +📁 Drive + ⭐ Personal ← kind='personal', default_for_user=caller (★ marks default) + Family Archive ← kind='personal', secondary (no star) + Engineering ← kind='shared', caller is owner/editor/viewer + 2025 Marketing Sprint ← kind='shared' + Recent + Favorites + Shared with me + Trash +``` + +- The user's **default personal drive** is marked with a star (or + bolded — UI choice). Clicking it lands on the default landing + view (today's "Files" experience). +- Other drives — secondary personals, shared drives — appear below + with the same visual weight. Clicking switches the context (URL + changes to `/drive//...`, listing reloads). +- The sidebar is collapsible per-drive; users with many drives can + hide the drives they aren't actively in. +- **No drive icons / colours in v1** (deferred to a future polish + PR — see "Out of scope" item 5). +- The "Drive" section header itself is non-interactive (just a + grouping label); the action affordance is "Create shared drive" + via the `+` button at the end of the list (admin / group-owner + scoped; lands in D3). + +### Breadcrumb — drive-rooted + +Today's breadcrumb is path-rooted: `Home / Docs / Q3 / Report.pdf`. +Post-Drive the breadcrumb starts at the selected drive: + +``` +[Personal ▾] / Docs / Q3 / Report.pdf +``` + +- The drive name is the **first** element. Clicking it returns to + that drive's root listing. +- The `▾` chevron next to the drive name opens a quick-switcher + picker (same list as the sidebar). Lets users jump between drives + without using the sidebar. +- For paths inside shared drives, the drive name still leads: + `[Engineering ▾] / Specs / Q3 OKRs.md`. +- The breadcrumb's overflow / truncation behaviour for deep paths is + unchanged from today — only the root element is new. +- **Drive name follows server-side rename** — the breadcrumb queries + the drive's current `name`; renaming the drive updates the + breadcrumb on next reload without any client-side cache work. + +### "Owners" section — review and redesign + +Today's UI has an "Owner" surface in several places (file details +sidebar, share dialog, folder properties). It currently shows "the +user who owns this file" — a single name. + +Post-Drive, ownership has multiple shapes: + +- For a file in a personal drive: still one owner (the drive's sole + user-owner). Display stays the same — "Owner: Ed". +- For a file in a shared drive: ownership is the drive's **owner- + role members** (possibly multiple users + groups). Display + becomes "Owner: Engineering team (3 members + 2 groups)". +- For a drive itself (when displayed in a settings view): same as + above, but in a list form: "Owners: Ed, Alice, Engineering + group". + +Open questions for the D3 PR to settle: +- **Show the drive name as the effective owner?** "Owner: + Engineering" reads cleanly but conflates "the drive" with "the + drive's owners" — clearer for end users, less precise. +- **Should the file details sidebar expand owners on click?** + Click "Engineering team" → see the owner roster. Useful for + large drives where the owner list doesn't fit inline. +- **Audit/My Shares dialog**: the "Shared with me" view currently + groups by sharer. Post-Drive it can group by drive instead + (your shared-drive access shows once per drive, not once per + file). UX win — decide which grouping is default and whether + both are togglable. + +These three UI surfaces are independent enough that they can ship +in separate PRs (sidebar in D1, breadcrumb in D1, owners review +in D3 alongside the create-shared-drive flow). The sidebar and +breadcrumb are essentially mechanical given the new model; the +owners review is the only one with genuine design questions. + +## File map (anticipated, not yet created) + +``` +migrations/ + 20260730000000_role_grants.sql ← D-Prep (rename of access_grants + to role_grants with role bundles) + 20260801000000_drives.sql ← D0 (drives table only; + membership is rows in role_grants) + 20260901000000_drop_user_id_on_resources.sql ← D7 + +src/domain/entities/ + drive.rs ← D0 + role.rs ← D-Prep (role enum + bundle map) + +src/domain/services/ + authorization.rs (modify) ← D-Prep adds `Manage` Permission + + `Resource::Drive(Uuid)` variant + +src/application/services/ + drive_service.rs ← D0–D3 (CRUD + policy enforcement, + membership operations translate + to role_grants writes) + +src/infrastructure/repositories/pg/ + drive_pg_repository.rs ← D0 + role_grant_pg_repository.rs ← D-Prep (replaces + access_grant_pg_repository) + +src/infrastructure/services/pg_acl_engine.rs (modify) + reads role_grants only ← D-Prep + Resource::Drive routing ← D0 + +src/interfaces/api/handlers/ + grant_handler.rs (modify) ← D-Prep (accepts role parameter) + drive_handler.rs ← D0 (list), D2 (members), D3 (create/delete shared) + +src/interfaces/api/handlers/webdav_handler.rs (modify) + drive-aware path resolution ← D1 + +src/interfaces/nextcloud/webdav_handler.rs (modify) + drive-aware path resolution ← D1 + +static/js/views/drive/ ← D1 (sidebar switcher, drive view) +static/js/model/drives.js ← D1 (REST client) +static/css/components/driveSwitcher.css ← D1 +``` + +## Glossary (for the next reader) + +- **Drive** — a top-level container that owns folders and files. +- **Personal drive** — the auto-created drive a user gets at + registration. One per internal user. `kind='personal'`. +- **Shared drive** — a drive whose membership includes at least one + group (or multiple users). `kind='shared'`. +- **Drive member** — a row in `storage.role_grants` with + `resource_type='drive'`, carrying a subject (user or group) + and a role. There is **no** separate `drive_members` table — see + the D-Prep prerequisite at the top of this plan for the storage + pivot. +- **Owner / editor / viewer** — drive roles. Map to ReBAC + permission bundles for resources inside the drive. +- **Drive policy** — JSONB key on the drive that toggles a sharing / + movement restriction. +- **Drive context** in WebDAV — the drive whose root the request is + rooted at. Native WebDAV resolves it from the URL prefix + (`/webdav/` → caller's personal drive, + `/webdav/drives//` → explicit drive). NC resolves + it from the credential (Basic Auth `{user}~{uuid}` username, or + the `auth.app_passwords.drive_id` binding — see §9). +- **Wrapper folder** — historical name for + `My Folder - `, the folder created at registration + via `format!("My Folder - {}", username)`. **Retired** in the + Drive migration: drive root replaces it. Every reference to + "wrapper" in older comments / docs is by definition pre-Drive. From f168c4578f7acda8a316a8153e97c561268a511a Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Wed, 17 Jun 2026 23:14:25 +0200 Subject: [PATCH 2/6] feat(roles): prepare migration ReBAC to roles prepare migration of permission to roles this simplify drastically database (permission are now simply defined in code) and will permit reuse of the same ReBAC engine to define owners of drives mapping: ``` Role::Viewer => &[Permission::Read], Role::Commenter => &[Permission::Read, Permission::Comment], Role::Contributor => &[Permission::Read, Permission::Create], Role::Editor => &[ Permission::Read, Permission::Comment, Permission::Create, Permission::Update, ], Role::Owner => &[ Permission::Read, Permission::Comment, Permission::Create, Permission::Update, Permission::Share, Permission::Delete, Permission::Manage, ], ``` --- migrations/20260730000000_role_grants.sql | 244 ++++++++++++ src/application/dtos/grant_dto.rs | 196 ++++++++-- src/application/ports/authorization_ports.rs | 36 ++ src/domain/services/authorization.rs | 15 +- src/infrastructure/services/pg_acl_engine.rs | 119 +++++- src/interfaces/api/handlers/grant_handler.rs | 161 ++++++-- static/js/components/mySharesList.js | 2 +- static/js/components/roleChip.js | 11 +- static/js/components/shareModal.js | 24 +- static/js/core/types.js | 9 +- tests/api/role_grants.hurl | 388 +++++++++++++++++++ tests/api/run.sh | 1 + 12 files changed, 1132 insertions(+), 74 deletions(-) create mode 100644 migrations/20260730000000_role_grants.sql create mode 100644 tests/api/role_grants.hurl diff --git a/migrations/20260730000000_role_grants.sql b/migrations/20260730000000_role_grants.sql new file mode 100644 index 00000000..afdc1921 --- /dev/null +++ b/migrations/20260730000000_role_grants.sql @@ -0,0 +1,244 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- D-Prep: storage.role_grants — role-bundle replacement for access_grants +-- ════════════════════════════════════════════════════════════════════════════ +-- Refactor #1 of the Drive sequence (see `docs/plan/drive.md` § Prerequisite). +-- +-- Today every role assignment is stored as N rows in `storage.access_grants` +-- (one row per Permission in the role's bundle — editor = 4 rows, owner = 6). +-- This migration introduces `storage.role_grants` where each role assignment +-- is ONE row carrying the role name; permission expansion happens at engine +-- read time via the in-code `role_bundle()` function. +-- +-- The five roles shipped on day one: +-- viewer = {read} +-- commenter = {comment, read} ← new +-- contributor = {create, read} ← new +-- editor = {comment, create, read, update} +-- owner = {comment, create, delete, read, share, update} +-- (post-Drive: + manage, when Group-as-Resource lands) +-- +-- This migration is **additive**: `storage.access_grants` stays populated as +-- a dual-write safety net until a follow-up cleanup PR drops it after the +-- new model has baked in production. The down migration just drops +-- role_grants — access_grants is untouched, so rollback is trivial. +-- +-- Pre-flight: the migration REFUSES to run if `access_grants` contains any +-- non-bundle clusters (permission sets that don't match one of the five +-- roles above). Run `tools/audit-grants-bundle-shape.sql` first to confirm +-- the data is clean — Ed's audit on 2026-06-17 returned 100% bundle-shaped. + + +-- ── 1. Pre-flight assertion ───────────────────────────────────────────────── +-- Refuse to migrate if there are any non-bundle clusters. The five known +-- bundles are listed here verbatim; keep them in sync with the in-code +-- `role_bundle()` function. + +DO $BODY$ +DECLARE + bad_count BIGINT; +BEGIN + WITH cluster AS ( + SELECT subject_type, subject_id, resource_type, resource_id, + array_agg(permission ORDER BY permission) AS perms + FROM storage.access_grants + GROUP BY 1, 2, 3, 4 + ) + SELECT count(*) INTO bad_count + FROM cluster + WHERE perms NOT IN ( + ARRAY['read']::text[], + ARRAY['comment','read']::text[], + ARRAY['create','read']::text[], + ARRAY['comment','create','read','update']::text[], + ARRAY['comment','create','delete','read','share','update']::text[] + ); + + IF bad_count > 0 THEN + RAISE EXCEPTION + 'D-Prep migration refused: % (subject,resource) clusters in ' + 'storage.access_grants have non-bundle permission sets. Run ' + 'tools/audit-grants-bundle-shape.sql section 3 to inspect them, ' + 'then either resolve manually or extend the bundle list above ' + 'with a new named role before retrying.', bad_count; + END IF; +END $BODY$; + + +-- ── 2. The role_grants table ──────────────────────────────────────────────── + +CREATE TABLE IF NOT EXISTS storage.role_grants ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + + -- Subject (who has the role) + -- 'user' → auth.users.id + -- 'group' → storage.subject_groups.id + -- 'token' → storage.shares.id (anonymous link — always 'viewer') + subject_type TEXT NOT NULL + CHECK (subject_type IN ('user', 'group', 'token')), + subject_id UUID NOT NULL, + + -- Resource (what the role is on) + -- 'drive' and 'group' join later as Drive + Group-as-Resource land. + resource_type TEXT NOT NULL + CHECK (resource_type IN ('folder', 'file')), + resource_id UUID NOT NULL, + + -- Role — expands to a permission bundle via the in-code `role_bundle()` + -- function. The CHECK lists the day-one role roster; adding a new + -- role is a single ALTER TABLE DROP CONSTRAINT / ADD CONSTRAINT pair + -- (or replace with a foreign key into a lookup table if instance- + -- defined roles ever land). + -- + -- Universal roster: ANY role can be granted on ANY resource_type. + -- Permission bundles include capabilities the resource type may not + -- check for (e.g. `Manage` on a folder, `Create` on a file); those + -- produce harmless no-ops at engine read time — no per-resource-type + -- validation needed at the DB layer. + -- + -- The UI exposes only Viewer/Editor/Owner in the share dialog today + -- (matches the existing 3-button UX). Commenter and Contributor stay + -- in the enum for server-side use + future UI exposure when a real + -- use case asks for them. + role TEXT NOT NULL + CHECK (role IN ('viewer', 'commenter', 'contributor', 'editor', 'owner')), + + -- Audit + lifecycle + granted_by UUID NOT NULL, + granted_at TIMESTAMPTZ NOT NULL DEFAULT now(), + expires_at TIMESTAMPTZ, + + -- Exactly one role per (subject, resource). Atomic role changes become + -- a single UPDATE; no DELETE+INSERT race. + UNIQUE (subject_type, subject_id, resource_type, resource_id) +); + +COMMENT ON TABLE storage.role_grants IS + 'Role-based ReBAC grants. One row = one role assignment. Permission ' + 'bundle expansion is in-code; see role_bundle() in ' + 'src/application/dtos/grant_dto.rs. Replaces storage.access_grants; ' + 'both tables coexist during the D-Prep dual-write window.'; +COMMENT ON COLUMN storage.role_grants.role IS + 'One of viewer / commenter / contributor / editor / owner. Expanded to ' + 'a Permission bundle by the in-code role_bundle() function at engine ' + 'read time.'; + + +-- ── 3. Indexes — match the hot-path queries ───────────────────────────────── + +-- "What does this caller have access to?" — every WebDAV / NC request, +-- every UI default-drive resolution (post-Drive) hits this. +CREATE INDEX IF NOT EXISTS idx_role_grants_subject + ON storage.role_grants (subject_type, subject_id); + +-- "Who has access to this resource?" — share dialogs, audit views. +CREATE INDEX IF NOT EXISTS idx_role_grants_resource + ON storage.role_grants (resource_type, resource_id); + +-- Partial index on expiry — only rows that actually expire (mirrors the +-- access_grants index pattern, same rationale). +CREATE INDEX IF NOT EXISTS idx_role_grants_expires_at + ON storage.role_grants (expires_at) WHERE expires_at IS NOT NULL; + +-- For GET /api/grants/outgoing/resources (who granted what). +CREATE INDEX IF NOT EXISTS idx_role_grants_granted_by + ON storage.role_grants (granted_by); + + +-- ── 4. Backfill from access_grants ───────────────────────────────────────── +-- For each (subject, resource) cluster in access_grants, write one +-- role_grants row with the matching role. The CASE expression mirrors +-- `Role::expand()` exactly — when that function changes (new role added), +-- update both this CASE and the CHECK constraint above. +-- +-- expires_at: take MIN across the cluster (most conservative — the role +-- assignment expires at the earliest expiry of any of its constituent +-- grants). granted_at: MIN (when the role assignment started). granted_by: +-- the granter of the earliest row (preserves attribution to the admin who +-- initially set the role up). + +WITH cluster AS ( + SELECT subject_type, + subject_id, + resource_type, + resource_id, + array_agg(permission ORDER BY permission) AS perms, + MIN(granted_at) AS earliest_granted_at, + MIN(expires_at) AS earliest_expires_at + FROM storage.access_grants + GROUP BY 1, 2, 3, 4 +), +earliest_grantor AS ( + SELECT DISTINCT ON (subject_type, subject_id, resource_type, resource_id) + subject_type, + subject_id, + resource_type, + resource_id, + granted_by + FROM storage.access_grants + ORDER BY subject_type, subject_id, resource_type, resource_id, granted_at ASC +) +INSERT INTO storage.role_grants + (subject_type, subject_id, resource_type, resource_id, + role, granted_by, granted_at, expires_at) +SELECT + c.subject_type, + c.subject_id, + c.resource_type, + c.resource_id, + CASE c.perms + WHEN ARRAY['read']::text[] + THEN 'viewer' + WHEN ARRAY['comment','read']::text[] + THEN 'commenter' + WHEN ARRAY['create','read']::text[] + THEN 'contributor' + WHEN ARRAY['comment','create','read','update']::text[] + THEN 'editor' + WHEN ARRAY['comment','create','delete','read','share','update']::text[] + THEN 'owner' + END AS role, + eg.granted_by, + c.earliest_granted_at, + c.earliest_expires_at +FROM cluster c +JOIN earliest_grantor eg USING (subject_type, subject_id, resource_type, resource_id) +ON CONFLICT (subject_type, subject_id, resource_type, resource_id) DO NOTHING; + + +-- ── 5. Post-flight consistency check ─────────────────────────────────────── +-- Assert that the backfill landed one role_grants row per (subject, +-- resource) cluster in access_grants. Any mismatch means a bundle pattern +-- silently failed to match — refuses to commit, surfacing the bug. + +DO $BODY$ +DECLARE + expected_clusters BIGINT; + actual_role_grants BIGINT; + null_roles BIGINT; +BEGIN + SELECT count(*) INTO expected_clusters + FROM ( + SELECT 1 FROM storage.access_grants + GROUP BY subject_type, subject_id, resource_type, resource_id + ) c; + + SELECT count(*) INTO actual_role_grants FROM storage.role_grants; + + IF expected_clusters != actual_role_grants THEN + RAISE EXCEPTION + 'D-Prep backfill consistency check failed: expected % role_grants ' + 'rows (one per distinct (subject, resource) cluster in access_grants), ' + 'got %. Investigate before declaring the migration successful.', + expected_clusters, actual_role_grants; + END IF; + + -- Defensive: NULL role would mean the CASE expression failed to match. + -- Pre-flight already refuses this, but double-check. + SELECT count(*) INTO null_roles FROM storage.role_grants WHERE role IS NULL; + IF null_roles > 0 THEN + RAISE EXCEPTION + 'D-Prep backfill produced % role_grants rows with NULL role — ' + 'a bundle pattern slipped past the pre-flight check. Investigate.', + null_roles; + END IF; +END $BODY$; diff --git a/src/application/dtos/grant_dto.rs b/src/application/dtos/grant_dto.rs index ac74a225..e7c0ec75 100644 --- a/src/application/dtos/grant_dto.rs +++ b/src/application/dtos/grant_dto.rs @@ -95,6 +95,7 @@ pub enum PermissionDto { Comment, Delete, Update, + Manage, } impl From for Permission { @@ -106,6 +107,7 @@ impl From for Permission { PermissionDto::Comment => Permission::Comment, PermissionDto::Delete => Permission::Delete, PermissionDto::Update => Permission::Update, + PermissionDto::Manage => Permission::Manage, } } } @@ -119,59 +121,176 @@ impl From for PermissionDto { Permission::Comment => PermissionDto::Comment, Permission::Delete => PermissionDto::Delete, Permission::Update => PermissionDto::Update, + Permission::Manage => PermissionDto::Manage, } } } // ════════════════════════════════════════════════════════════════════════════ -// Roles (DTO-layer sugar) +// Roles — the load-bearing model for ReBAC grants // ════════════════════════════════════════════════════════════════════════════ +// +// Today a role is "DTO-layer sugar" — every grant write expands a role into +// N rows in `storage.access_grants`. The D-Prep refactor (see +// `docs/plan/drive.md` §Prerequisite + migration `20260730000000_role_grants.sql`) +// pushes the role down to storage (`storage.role_grants.role TEXT`); the +// engine reads the role and expands the bundle at query time via this same +// `expand()` function. Adding a role is now schema-free — one variant + one +// match arm here. -#[derive(Debug, Clone, Copy, Serialize, Deserialize, ToSchema)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, ToSchema)] #[serde(rename_all = "lowercase")] pub enum Role { + /// Read access only. The default "anyone can look but not touch". Viewer, - //Commenter, + /// Read + comment. Useful for review-only stakeholders. + Commenter, + /// Read + create. The "drop-zone" role — uploads allowed, existing + /// content untouchable. Common for support-ticket attachments and + /// photo-submission folders. + Contributor, + /// Read + create + update + comment. The standard collaboration role. Editor, - //Manager, - Admin, + /// Full bundle: read, create, update, comment, delete, share — and + /// `Manage` for resource types that support it (drives, groups). This + /// is the highest user-grantable role; renamed from the historical + /// `Admin` to disambiguate from `UserRole::Admin` (the user-account + /// privilege) and to match Drive plan terminology. + /// + /// **Wire-format compat shim** (one release): also deserialises from + /// the legacy `"admin"` string so cached frontend clients keep + /// working until they refresh. Serialisation always emits `"owner"`. + /// Drop the alias in the cleanup PR. + #[serde(alias = "admin")] + Owner, } impl Role { - /// Expands a role into its constituent raw permissions. Storage and - /// engine know nothing about roles — the server normalizes here before - /// writing rows. + /// Expand the role into its permission bundle. Single source of truth — + /// any code that needs "does this role include Permission X?" routes + /// through here (or its inverse, `roles_implying`). + /// + /// After D-Prep this is called at engine read time (1 row → bundle + /// expanded server-side). Pre-D-Prep it was called at API write time + /// (1 role → N rows fanned out). pub fn expand(self) -> &'static [Permission] { match self { Role::Viewer => &[Permission::Read], - /* reserved for future Role::Commenter => &[Permission::Read, Permission::Comment], - */ + Role::Contributor => &[Permission::Read, Permission::Create], Role::Editor => &[ Permission::Read, Permission::Comment, Permission::Create, Permission::Update, ], - /* reserved for future - Role::Manager => &[ - Permission::Read, - Permission::Comment, - Permission::Create, - Permission::Update, - Permission::Share, - ], - */ - Role::Admin => &[ + Role::Owner => &[ Permission::Read, Permission::Comment, Permission::Create, Permission::Update, Permission::Share, Permission::Delete, + Permission::Manage, ], } } + + /// Lowercase string discriminator — matches the SQL `role` column values + /// in `storage.role_grants` and the JSON wire format. + pub fn as_str(self) -> &'static str { + match self { + Role::Viewer => "viewer", + Role::Commenter => "commenter", + Role::Contributor => "contributor", + Role::Editor => "editor", + Role::Owner => "owner", + } + } + + /// Parse a role from its SQL / JSON string discriminator. Returns + /// `None` for unknown values. Accepts the legacy `"admin"` spelling + /// for one release of API compat (clients that cached the old name + /// keep working; new responses always emit `"owner"`). + pub fn parse(s: &str) -> Option { + match s { + "viewer" => Some(Role::Viewer), + "commenter" => Some(Role::Commenter), + "contributor" => Some(Role::Contributor), + "editor" => Some(Role::Editor), + "owner" => Some(Role::Owner), + // Legacy compat: drop after one release once all clients are + // updated. Emits a debug log so we can track stragglers. + "admin" => { + tracing::debug!( + target: "oxicloud::grants", + "Role::parse: accepted legacy 'admin' string as Role::Owner" + ); + Some(Role::Owner) + } + _ => None, + } + } + + /// Every role, in a stable order. Used by `roles_implying` and exposed + /// to the UI so the share modal can render the full picker without + /// hardcoding the list. + /// + /// **UI scope today**: the share dialog renders only `Viewer`, + /// `Editor`, and `Owner` (matches the existing 3-button UX). The + /// `Commenter` and `Contributor` variants are implemented server- + /// side and accepted on the API surface, reserved for future UI + /// exposure when a real use case asks for them. Until then they + /// stay invisible to end users — no picker option, no documentation + /// surface. + /// + /// Any role can be granted on any resource type. Permission bundles + /// that include capabilities the resource type doesn't check for + /// (e.g. `Manage` on a folder, `Create` on a file) simply produce + /// harmless no-ops — no separate validation layer is needed. + pub const ALL: [Role; 5] = [ + Role::Viewer, + Role::Commenter, + Role::Contributor, + Role::Editor, + Role::Owner, + ]; +} + +/// Inverse of [`Role::expand`]: returns every role whose bundle contains +/// the given permission. Used by the engine to build the SQL +/// `WHERE role IN (...)` filter on hot-path queries like "what drives can +/// this caller read?": +/// +/// ```ignore +/// SELECT resource_id FROM role_grants +/// WHERE subject_id = $1 +/// AND resource_type = 'drive' +/// AND role IN (roles_implying(Permission::Read)); +/// ``` +/// +/// Precomputed in code rather than stored in the DB — `Role` and +/// `Permission` are both small fixed enums, the table can never grow +/// beyond a handful of rows, and keeping it in-code makes "what changes +/// when I add a Permission?" a single grep target. +pub fn roles_implying(permission: Permission) -> &'static [Role] { + use Permission::*; + match permission { + // Every role grants Read — viewer is the floor. + Read => &[ + Role::Viewer, + Role::Commenter, + Role::Contributor, + Role::Editor, + Role::Owner, + ], + Comment => &[Role::Commenter, Role::Editor, Role::Owner], + Create => &[Role::Contributor, Role::Editor, Role::Owner], + Update => &[Role::Editor, Role::Owner], + Delete => &[Role::Owner], + Share => &[Role::Owner], + Manage => &[Role::Owner], + } } // ════════════════════════════════════════════════════════════════════════════ @@ -430,12 +549,34 @@ pub struct SharedWithMeItemDto { } /// Derive the closest-matching role label from a set of permissions. -/// Maps the permission set to `"admin"`, `"editor"`, or `"viewer"`. +/// +/// **Legacy helper for the dual-write window.** Once D-Prep ships and the +/// engine reads `role_grants.role` directly, this function becomes unused +/// and is dropped in the cleanup PR. Kept here so callers that still hit +/// `access_grants` and reconstruct a role for display can stay working +/// during the transition. +/// +/// Emits the new five-role roster on output (`"viewer"` / `"commenter"` / +/// `"contributor"` / `"editor"` / `"owner"`). Note this is **lossy** for +/// permission sets that don't match a bundle exactly — but D-Prep's +/// pre-flight refuses to migrate any such cluster, so post-migration data +/// only contains bundle-shaped sets. pub fn role_from_permissions(perms: &[Permission]) -> &'static str { - if perms.contains(&Permission::Delete) && perms.contains(&Permission::Share) { - "admin" - } else if perms.contains(&Permission::Create) || perms.contains(&Permission::Update) { + let has_read = perms.contains(&Permission::Read); + let has_comment = perms.contains(&Permission::Comment); + let has_create = perms.contains(&Permission::Create); + let has_update = perms.contains(&Permission::Update); + let has_delete = perms.contains(&Permission::Delete); + let has_share = perms.contains(&Permission::Share); + + if has_delete && has_share { + "owner" + } else if has_create && has_update { "editor" + } else if has_read && has_create && !has_update { + "contributor" + } else if has_read && has_comment && !has_create && !has_update { + "commenter" } else { "viewer" } @@ -457,7 +598,12 @@ pub struct OutgoingResourceGrantDto { pub subject_id: Uuid, /// Human-readable label (username for users, share name for tokens). pub subject_display: String, - /// Derived role label: `"viewer"` | `"editor"` | `"admin"`. + /// Role label: `"viewer"` | `"commenter"` | `"contributor"` | `"editor"` + /// | `"owner"`. Emitted by `role_from_permissions()` during the dual-write + /// window; once D-Prep cleanup lands this is read directly from + /// `storage.role_grants.role`. The legacy `"admin"` spelling is no longer + /// emitted — clients that cached it must accept `"owner"` too (the API + /// `Role::parse` still accepts `"admin"` on input for one release). pub role: String, pub granted_at: chrono::DateTime, #[serde(skip_serializing_if = "Option::is_none")] diff --git a/src/application/ports/authorization_ports.rs b/src/application/ports/authorization_ports.rs index e310c3e1..b0b9b1c7 100644 --- a/src/application/ports/authorization_ports.rs +++ b/src/application/ports/authorization_ports.rs @@ -181,4 +181,40 @@ pub trait AuthorizationEngine: Send + Sync + 'static { /// Removes every grant whose `subject` matches. Called when a user/token /// /group is deleted. Returns the count of rows removed. async fn revoke_all_for_subject(&self, subject: Subject) -> Result; + + // ── Role-keyed grant operations (D-Prep dual-write) ──────────────────── + // These manage `storage.role_grants`, the role-keyed table introduced + // by the D-Prep refactor (see `docs/plan/drive.md` §Prerequisite). + // + // During the dual-write window both tables stay populated; the engine + // reads from `access_grants` until the read-path pivot lands. After + // the cleanup PR (which drops `access_grants`), these two methods + // become the ONLY grant write path — the per-permission `grant` / + // `revoke` above are removed at that point. + // + // The handler layer drives these (it knows the Role); lifecycle hook + // bulk-deletes (`revoke_all_for_*` above) wipe role_grants in lockstep + // inside their own implementation, so callers using those paths don't + // need to invoke `clear_role` separately. + + /// Set the role for a `(subject, resource)` pair. Idempotent via the + /// UNIQUE `(subject_type, subject_id, resource_type, resource_id)` + /// constraint — `ON CONFLICT` updates the role + expires_at if they + /// changed, which is exactly the right semantics for an atomic role + /// change (e.g. promoting Viewer → Editor in one UPDATE with no race + /// window, no DELETE+INSERT). + async fn set_role( + &self, + granted_by: Uuid, + subject: Subject, + role: crate::application::dtos::grant_dto::Role, + resource: Resource, + expires_at: Option>, + ) -> Result<(), DomainError>; + + /// Remove the role for a `(subject, resource)` pair. Idempotent — + /// succeeds whether or not the row existed. Called after `revoke` + /// succeeds to keep the two tables in sync during dual-write; after + /// cleanup this is the canonical role-revocation entry point. + async fn clear_role(&self, subject: Subject, resource: Resource) -> Result<(), DomainError>; } diff --git a/src/domain/services/authorization.rs b/src/domain/services/authorization.rs index 2e095e59..7e8b5503 100644 --- a/src/domain/services/authorization.rs +++ b/src/domain/services/authorization.rs @@ -140,18 +140,29 @@ pub enum Permission { Delete, /// Modify the resource (rename, move, edit content). Update, + /// Configure the resource's settings, add/remove members, change role + /// assignments. Used by: + /// - Drive owners managing drive membership and policies. + /// - Group owners managing the group itself (Group-as-Resource, future). + /// + /// Folder and file resources do not currently surface a `Manage` check; + /// the permission lives in the enum because the role bundle (`Owner`) + /// includes it, and the resource types that DO check it (`Drive`, + /// `Group`) are added in subsequent PRs (see `docs/plan/drive.md`). + Manage, } impl Permission { /// Every permission, in a stable order. Used by `Role::expand()` and SQL /// `permission = ANY(...)` lookups. - pub const ALL: [Permission; 6] = [ + pub const ALL: [Permission; 7] = [ Permission::Read, Permission::Create, Permission::Share, Permission::Comment, Permission::Delete, Permission::Update, + Permission::Manage, ]; pub fn as_str(&self) -> &'static str { @@ -162,6 +173,7 @@ impl Permission { Permission::Comment => "comment", Permission::Delete => "delete", Permission::Update => "update", + Permission::Manage => "manage", } } @@ -175,6 +187,7 @@ impl Permission { "comment" => Some(Permission::Comment), "delete" => Some(Permission::Delete), "update" => Some(Permission::Update), + "manage" => Some(Permission::Manage), _ => None, } } diff --git a/src/infrastructure/services/pg_acl_engine.rs b/src/infrastructure/services/pg_acl_engine.rs index b60ba832..1d46db3b 100644 --- a/src/infrastructure/services/pg_acl_engine.rs +++ b/src/infrastructure/services/pg_acl_engine.rs @@ -37,6 +37,7 @@ use uuid::Uuid; use moka::future::Cache; use sqlx::PgPool; +use crate::application::dtos::grant_dto::roles_implying; use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::common::errors::DomainError; use crate::domain::entities::subject_group::INTERNAL_GROUP_ID; @@ -237,6 +238,22 @@ impl PgAclEngine { } } + /// Convert a `Permission` into the array of role strings whose bundle + /// includes it — used to bind the `g.role = ANY($N::text[])` filter on + /// every cascade / lookup query that reads `storage.role_grants`. + /// + /// This is the inverse of `Role::expand()`, precomputed via + /// `grant_dto::roles_implying()`. The mapping is small and static (≤5 + /// roles per permission today); resolving it in code keeps the SQL + /// path simple and lets us add new roles without touching every + /// query site. + fn roles_implying_strings(permission: Permission) -> Vec<&'static str> { + roles_implying(permission) + .iter() + .map(|r| r.as_str()) + .collect() + } + /// Cascading check for folders: is there a grant on any ancestor folder /// (including the target itself) for any of the given subject IDs and /// any of the given subject types? @@ -247,6 +264,12 @@ impl PgAclEngine { /// `subject_ids` is the expanded set returned by `expand_user` (or a /// single-element vec for non-user callers). /// + /// **D-Prep**: now reads `storage.role_grants` (1 row per role + /// assignment) instead of `storage.access_grants` (N rows per role). + /// The permission filter `g.permission = $3` becomes + /// `g.role = ANY($3::text[])` where the array is the set of roles whose + /// bundle includes the requested permission — see `roles_implying()`. + /// /// Uses the GiST index on `storage.folders.lpath` for O(log N) cascade. async fn folder_cascade_grant_exists( &self, @@ -257,14 +280,15 @@ impl PgAclEngine { counters: &QueryCounters, ) -> Result { counters.sql_queries.fetch_add(1, Ordering::Relaxed); + let roles = Self::roles_implying_strings(permission); let exists: Option = sqlx::query_scalar( r#" SELECT 1 - FROM storage.access_grants g + FROM storage.role_grants g JOIN storage.folders gf ON gf.id = g.resource_id WHERE g.subject_type = ANY($1) AND g.subject_id = ANY($2) - AND g.permission = $3 + AND g.role = ANY($3::text[]) AND g.resource_type = 'folder' AND (g.expires_at IS NULL OR g.expires_at > NOW()) AND gf.lpath @> (SELECT lpath FROM storage.folders WHERE id = $4) @@ -273,7 +297,7 @@ impl PgAclEngine { ) .bind(subject_types) .bind(subject_ids) - .bind(permission.as_str()) + .bind(&roles) .bind(folder_id) .fetch_optional(self.pool.as_ref()) .await @@ -285,7 +309,7 @@ impl PgAclEngine { /// Cascading check for files: either a direct file grant OR a grant on /// any ancestor folder of the file's containing folder. See /// `folder_cascade_grant_exists` for the meaning of `subject_types` / - /// `subject_ids`. + /// `subject_ids` and the D-Prep role-array migration. async fn file_cascade_grant_exists( &self, subject_types: &[&str], @@ -295,27 +319,28 @@ impl PgAclEngine { counters: &QueryCounters, ) -> Result { counters.sql_queries.fetch_add(1, Ordering::Relaxed); + let roles = Self::roles_implying_strings(permission); let exists: Option = sqlx::query_scalar( r#" SELECT 1 FROM ( -- direct file grant SELECT 1 - FROM storage.access_grants + FROM storage.role_grants WHERE subject_type = ANY($1) AND subject_id = ANY($2) - AND permission = $3 + AND role = ANY($3::text[]) AND resource_type = 'file' AND resource_id = $4 AND (expires_at IS NULL OR expires_at > NOW()) UNION ALL -- cascading from any ancestor folder of the file's containing folder SELECT 1 - FROM storage.access_grants g + FROM storage.role_grants g JOIN storage.folders gf ON gf.id = g.resource_id JOIN storage.files target_f ON target_f.id = $4 WHERE g.subject_type = ANY($1) AND g.subject_id = ANY($2) - AND g.permission = $3 + AND g.role = ANY($3::text[]) AND g.resource_type = 'folder' AND (g.expires_at IS NULL OR g.expires_at > NOW()) AND target_f.folder_id IS NOT NULL @@ -327,7 +352,7 @@ impl PgAclEngine { ) .bind(subject_types) .bind(subject_ids) - .bind(permission.as_str()) + .bind(&roles) .bind(file_id) .fetch_optional(self.pool.as_ref()) .await @@ -1680,6 +1705,19 @@ impl AuthorizationEngine for PgAclEngine { } async fn revoke_all_for_resource(&self, resource: Resource) -> Result { + // D-Prep dual-write: wipe role_grants for this resource too. + // Idempotent — succeeds whether or not any row existed. + sqlx::query( + "DELETE FROM storage.role_grants WHERE resource_type = $1 AND resource_id = $2", + ) + .bind(resource.type_str()) + .bind(resource.id()) + .execute(self.pool.as_ref()) + .await + .map_err(|e| { + DomainError::internal_error("PgAcl", format!("revoke role_grants for resource: {e}")) + })?; + let result = sqlx::query( "DELETE FROM storage.access_grants WHERE resource_type = $1 AND resource_id = $2", ) @@ -1693,6 +1731,16 @@ impl AuthorizationEngine for PgAclEngine { } async fn revoke_all_for_subject(&self, subject: Subject) -> Result { + // D-Prep dual-write: wipe role_grants for this subject too. + sqlx::query("DELETE FROM storage.role_grants WHERE subject_type = $1 AND subject_id = $2") + .bind(subject.type_str()) + .bind(subject.id()) + .execute(self.pool.as_ref()) + .await + .map_err(|e| { + DomainError::internal_error("PgAcl", format!("revoke role_grants for subject: {e}")) + })?; + let result = sqlx::query( "DELETE FROM storage.access_grants WHERE subject_type = $1 AND subject_id = $2", ) @@ -1704,6 +1752,59 @@ impl AuthorizationEngine for PgAclEngine { Ok(result.rows_affected() as usize) } + + // ── D-Prep role_grants writes ────────────────────────────────────────── + + async fn set_role( + &self, + granted_by: Uuid, + subject: Subject, + role: crate::application::dtos::grant_dto::Role, + resource: Resource, + expires_at: Option>, + ) -> Result<(), DomainError> { + sqlx::query( + r#" + INSERT INTO storage.role_grants + (subject_type, subject_id, resource_type, resource_id, + role, granted_by, expires_at) + VALUES ($1, $2, $3, $4, $5, $6, $7) + ON CONFLICT (subject_type, subject_id, resource_type, resource_id) + DO UPDATE SET role = EXCLUDED.role, + expires_at = EXCLUDED.expires_at, + granted_by = EXCLUDED.granted_by + "#, + ) + .bind(subject.type_str()) + .bind(subject.id()) + .bind(resource.type_str()) + .bind(resource.id()) + .bind(role.as_str()) + .bind(granted_by) + .bind(expires_at) + .execute(self.pool.as_ref()) + .await + .map_err(|e| DomainError::internal_error("PgAcl", format!("set_role: {e}")))?; + + Ok(()) + } + + async fn clear_role(&self, subject: Subject, resource: Resource) -> Result<(), DomainError> { + sqlx::query( + "DELETE FROM storage.role_grants \ + WHERE subject_type = $1 AND subject_id = $2 \ + AND resource_type = $3 AND resource_id = $4", + ) + .bind(subject.type_str()) + .bind(subject.id()) + .bind(resource.type_str()) + .bind(resource.id()) + .execute(self.pool.as_ref()) + .await + .map_err(|e| DomainError::internal_error("PgAcl", format!("clear_role: {e}")))?; + + Ok(()) + } } // ───────────────────────────────────────────────────────────────────────────── diff --git a/src/interfaces/api/handlers/grant_handler.rs b/src/interfaces/api/handlers/grant_handler.rs index adc8c5c3..21617cb5 100644 --- a/src/interfaces/api/handlers/grant_handler.rs +++ b/src/interfaces/api/handlers/grant_handler.rs @@ -14,7 +14,7 @@ use axum::{ use futures::future::join_all; use serde::Deserialize; use std::sync::Arc; -use tracing::{error, info, warn}; +use tracing::{error, warn}; use utoipa::IntoParams; use uuid::Uuid; @@ -22,7 +22,7 @@ use crate::application::dtos::cursor::PageCursor; use crate::application::dtos::grant_dto::{ CreateGrantDto, CreateGrantResponseDto, GrantDto, MySharesDto, NotifyOutcomeSetDto, OutgoingResourceGrantDto, OutgoingResourceItemDto, PermissionDto, ResourceContentDto, - ResourceDto, ResourceTypeDto, SharedWithMeDto, SharedWithMeItemDto, SharedWithMeQuery, + ResourceDto, ResourceTypeDto, Role, SharedWithMeDto, SharedWithMeItemDto, SharedWithMeQuery, SubjectDto, SubjectInputDto, UpdateRoleDto, role_from_permissions, }; use crate::application::ports::authorization_ports::AuthorizationEngine; @@ -66,10 +66,23 @@ pub async fn create_grant( let authz = &state.authorization; let caller_id = auth_user.id; - // Validate: exactly one of permissions/role - let permissions: Vec = match (dto.permissions, dto.role) { - (Some(perms), None) if !perms.is_empty() => perms.into_iter().map(Into::into).collect(), - (None, Some(role)) => role.expand().to_vec(), + // Validate: exactly one of permissions/role. Capture BOTH the + // permission list (for the per-permission access_grants writes that + // keep the old engine read path working) AND the role (for the new + // role_grants `set_role` dual-write that lands after the per- + // permission loop). + let (permissions, role): (Vec, Role) = match (dto.permissions, dto.role) { + (Some(perms), None) if !perms.is_empty() => { + let perms: Vec = perms.into_iter().map(Into::into).collect(); + // Derive the closest matching role from the raw permission set + // so we have ONE role to mirror into role_grants. `Role::parse` + // always succeeds here because `role_from_permissions` only + // emits known role strings. + let role = Role::parse(role_from_permissions(&perms)) + .expect("role_from_permissions returns a known role string"); + (perms, role) + } + (None, Some(role)) => (role.expand().to_vec(), role), (Some(_), Some(_)) => { return AppError::new( StatusCode::BAD_REQUEST, @@ -154,6 +167,16 @@ pub async fn create_grant( let mut results: Vec = Vec::with_capacity(permissions.len()); for perm in permissions { + // `storage.access_grants.permission` CHECK constraint predates + // `Permission::Manage`; it accepts only the original 6 values. + // Manage exists in the Owner bundle for engine read-path use + // (via `roles_implying`) and gets persisted via the role_grants + // dual-write below. Skipping it here keeps the access_grants + // safety net populated without tripping the CHECK; the cleanup + // PR that drops access_grants also drops this skip. + if perm == Permission::Manage { + continue; + } match authz .grant(caller_id, subject, perm, resource, expires_at) .await @@ -165,12 +188,31 @@ pub async fn create_grant( } } } - info!( - "Created {} grant(s) for subject={:?} on resource={:?} by user {}", - results.len(), - subject, - resource, - caller_id + + // D-Prep dual-write: mirror the role assignment into storage.role_grants. + // ON CONFLICT UPDATE makes this idempotent — repeated POSTs with the + // same (subject, resource) update the role in place, matching the + // PATCH-style semantics callers will get after the engine read pivot. + if let Err(err) = authz + .set_role(caller_id, subject, role, resource, expires_at) + .await + { + error!("set_role dual-write failed: {err}"); + return AppError::from(err).into_response(); + } + + tracing::info!( + target: "audit", + event = "role_grant.created", + caller_id = %caller_id, + subject_type = subject.type_str(), + subject_id = %subject.id(), + resource_type = resource.type_str(), + resource_id = %resource.id(), + role = role.as_str(), + permission_count = results.len(), + expires_at = ?expires_at, + "🤝 grant created with role '{}'", role.as_str(), ); // PR N1 — route the post-grant notification through the unified @@ -274,17 +316,20 @@ pub async fn revoke_grant( Err(_) => return AppError::not_found(format!("Grant {id} not found")).into_response(), }; - // Look up the grant to find the underlying resource (and granter). - let on_resource = match authz.find_grant_by_id(grant_id).await { - Ok(Some((res, granter))) => (res, granter), + // Look up the grant to find the subject, resource, and granter. + // `find_grant_full_by_id` returns the subject too — needed for the + // `clear_role` dual-write below (role_grants is keyed by (subject, + // resource), not by access_grants id). + let (subject, resource, granter) = match authz.find_grant_full_by_id(grant_id).await { + Ok(Some(triple)) => triple, Ok(None) => return StatusCode::NO_CONTENT.into_response(), // idempotent Err(e) => return AppError::from(e).into_response(), }; // Caller is authorized if they are the granter OR have Share on the resource. - if on_resource.1 != caller_id + if granter != caller_id && let Err(e) = authz - .require(Subject::User(caller_id), Permission::Share, on_resource.0) + .require(Subject::User(caller_id), Permission::Share, resource) .await { return AppError::from(e).into_response(); @@ -293,7 +338,36 @@ pub async fn revoke_grant( if let Err(e) = authz.revoke(grant_id).await { return AppError::from(e).into_response(); } - info!("Revoked grant {grant_id} (caller {caller_id})"); + + // D-Prep dual-write: clear the role_grants row for this (subject, + // resource). Idempotent — succeeds whether or not the row existed. + // + // Today's API revokes one access_grants row by id; the role_grants + // row models the WHOLE (subject, resource) cluster. Calling clear_role + // here effectively revokes the WHOLE role assignment in role_grants, + // even if other per-permission access_grants rows remain. This is the + // correct semantics for the eventual cleanup-PR model (role_grants is + // role-keyed; once access_grants goes away, "revoke" means "drop the + // role"). During the dual-write window the two tables can drift + // briefly if a caller revokes only some permissions of a role, but + // the engine still reads access_grants so behaviour is unchanged. + if let Err(e) = authz.clear_role(subject, resource).await { + return AppError::from(e).into_response(); + } + + tracing::info!( + target: "audit", + event = "role_grant.revoked", + caller_id = %caller_id, + grant_id = %grant_id, + subject_type = subject.type_str(), + subject_id = %subject.id(), + resource_type = resource.type_str(), + resource_id = %resource.id(), + granter_id = %granter, + self_revoke = (granter == caller_id), + "🗑️ grant revoked", + ); StatusCode::NO_CONTENT.into_response() } @@ -509,9 +583,22 @@ pub async fn set_role( .map(|g| g.permission) .collect(); - // Diff and apply. - let to_add: Vec = target_perms.difference(¤t_perms).copied().collect(); - let to_remove: Vec = current_perms.difference(&target_perms).copied().collect(); + // Diff and apply. `Permission::Manage` is excluded from both sides + // because the historical `access_grants.permission` CHECK doesn't + // accept it — see the matching skip in `create_grant`. The role + // assignment captures Manage via the role_grants `set_role` call + // further down; the engine's read-path uses `roles_implying(Manage)` + // → `[Owner]` and never goes through per-permission rows. + let to_add: Vec = target_perms + .difference(¤t_perms) + .copied() + .filter(|p| *p != Permission::Manage) + .collect(); + let to_remove: Vec = current_perms + .difference(&target_perms) + .copied() + .filter(|p| *p != Permission::Manage) + .collect(); for perm in &to_remove { if let Some(g) = current @@ -542,6 +629,19 @@ pub async fn set_role( return AppError::from(e).into_response(); } + // D-Prep dual-write: mirror the resulting role into storage.role_grants. + // The per-permission diff above keeps access_grants converged; this + // single UPSERT keeps role_grants in sync with the OVERALL outcome + // (one row carrying the role + expiry). After the engine read pivot + // and the access_grants drop, the per-permission diff above goes + // away and this call becomes the only mutation the handler performs. + if let Err(e) = authz + .set_role(caller_id, subject, dto.role, resource, expires_at) + .await + { + return AppError::from(e).into_response(); + } + // Return the new full set. let after = match authz.list_grants_on_resource(resource).await { Ok(g) => g, @@ -553,9 +653,22 @@ pub async fn set_role( .map(Into::into) .collect(); - info!( - "Role applied: caller={} subject={:?} resource={:?} added={:?} removed={:?}", - caller_id, subject, resource, to_add, to_remove + tracing::info!( + target: "audit", + event = "role_grant.role_set", + caller_id = %caller_id, + subject_type = subject.type_str(), + subject_id = %subject.id(), + resource_type = resource.type_str(), + resource_id = %resource.id(), + role = dto.role.as_str(), + permissions_added = to_add.len(), + permissions_removed = to_remove.len(), + expires_at = ?expires_at, + "🔁 role set to '{}' (+{} -{})", + dto.role.as_str(), + to_add.len(), + to_remove.len(), ); (StatusCode::OK, Json(mine)).into_response() } diff --git a/static/js/components/mySharesList.js b/static/js/components/mySharesList.js index 6c374406..1db6b02f 100644 --- a/static/js/components/mySharesList.js +++ b/static/js/components/mySharesList.js @@ -454,7 +454,7 @@ class MySharesList { ); menu.appendChild(this._menuSeparator()); - for (const role of /** @type {('admin'|'editor'|'viewer')[]} */ (['admin', 'editor', 'viewer'])) { + for (const role of /** @type {('owner'|'editor'|'viewer')[]} */ (['owner', 'editor', 'viewer'])) { const isCurrent = grant.role === role; const mi = this._menuItem(isCurrent ? 'fas fa-check' : '', roleLabel(role), false, async () => { menu.remove(); diff --git a/static/js/components/roleChip.js b/static/js/components/roleChip.js index de37dac9..c2eea359 100644 --- a/static/js/components/roleChip.js +++ b/static/js/components/roleChip.js @@ -23,7 +23,7 @@ import { i18n } from '../core/i18n.js'; * @returns {'manage'|'edit'|'view'} */ function roleMod(role) { - if (role === 'admin') return 'manage'; + if (role === 'owner') return 'manage'; if (role === 'editor') return 'edit'; return 'view'; } @@ -31,14 +31,17 @@ function roleMod(role) { /** * Translate a role identifier into a localized human-readable label. * Exported so callers that just want the label (e.g. context-menu rows) - * can reuse the same wording the chip uses. + * can reuse the same wording the chip uses. Unknown roles fall back to + * the raw role string — `commenter` and `contributor` exist server-side + * but aren't surfaced in the UI today, so they'll display as-is until a + * future UI exposure adds proper labels. * @param {string} role * @returns {string} */ export function roleLabel(role) { /** @type {Record} */ const m = { - admin: i18n.t('share.role.canManage', 'Can manage'), + owner: i18n.t('share.role.canManage', 'Can manage'), editor: i18n.t('share.role.canEdit', 'Can edit'), viewer: i18n.t('share.role.canView', 'Can view') }; @@ -51,7 +54,7 @@ export function roleLabel(role) { * @returns {string} */ function roleIcon(role) { - if (role === 'admin') return 'fa-crown'; + if (role === 'owner') return 'fa-crown'; if (role === 'editor') return 'fa-pencil-alt'; return 'fa-eye'; } diff --git a/static/js/components/shareModal.js b/static/js/components/shareModal.js index 2b3febdd..dc5b246a 100644 --- a/static/js/components/shareModal.js +++ b/static/js/components/shareModal.js @@ -73,11 +73,21 @@ function _looksLikeEmail(q) { return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(q); } -/** Permissions that belong to each role (must mirror the Rust DTO). */ +/** + * Permissions that belong to each role (mirrors `Role::expand()` in + * `src/application/dtos/grant_dto.rs`). The share modal only renders + * Viewer/Editor/Owner as picker buttons today; `commenter` and + * `contributor` are kept here for fidelity with the server-side enum so + * a future UI exposure doesn't need a mirror-table update. The `manage` + * permission in the owner bundle is reserved for Drive- and Group-level + * admin actions (no-op on file/folder resources). + */ const ROLE_PERMISSIONS = { viewer: ['read'], + commenter: ['read', 'comment'], + contributor: ['read', 'create'], editor: ['read', 'comment', 'create', 'update'], - admin: ['read', 'comment', 'create', 'update', 'share', 'delete'] + owner: ['read', 'comment', 'create', 'update', 'share', 'delete', 'manage'] }; /** @@ -113,7 +123,7 @@ async function _searchGroups(q) { */ function _roleFromGrants(subjectGrants) { const perms = new Set(subjectGrants.map((g) => g.permission)); - if (perms.has('delete') || perms.has('share')) return 'admin'; + if (perms.has('delete') || perms.has('share')) return 'owner'; if (perms.has('create') || perms.has('update')) return 'editor'; return 'viewer'; } @@ -363,7 +373,7 @@ const shareModal = { for (const [val, label] of [ ['viewer', i18n.t('share.role.canView', 'Can view')], ['editor', i18n.t('share.role.canEdit', 'Can edit')], - ['admin', i18n.t('share.role.canManage', 'Can manage')] + ['owner', i18n.t('share.role.canManage', 'Can manage')] ]) { const opt = document.createElement('option'); opt.value = val; @@ -649,7 +659,7 @@ const shareModal = { // matching the UX contract and the kebab-menu / role-select dropdown // order. Renaming the labels from "Manager"/"Editor"/"Viewer" to // "Can manage"/"Can edit"/"Can view" left this iteration order stale. - const groups = /** @type {ShareRoleEnum[]} */ (['admin', 'editor', 'viewer']); + const groups = /** @type {ShareRoleEnum[]} */ (['owner', 'editor', 'viewer']); let memberIndex = 0; for (const role of groups) { @@ -663,7 +673,7 @@ const shareModal = { header.className = 'smd-group-header'; const labelMap = { - admin: i18n.t('share.role.canManage', 'Can manage'), + owner: i18n.t('share.role.canManage', 'Can manage'), editor: i18n.t('share.role.canEdit', 'Can edit'), viewer: i18n.t('share.role.canView', 'Can view') }; @@ -711,7 +721,7 @@ const shareModal = { for (const [val, label] of [ ['viewer', i18n.t('share.role.canView', 'Can view')], ['editor', i18n.t('share.role.canEdit', 'Can edit')], - ['admin', i18n.t('share.role.canManage', 'Can manage')] + ['owner', i18n.t('share.role.canManage', 'Can manage')] ]) { const opt = document.createElement('option'); opt.value = val; diff --git a/static/js/core/types.js b/static/js/core/types.js index 594919c0..3a719a2b 100644 --- a/static/js/core/types.js +++ b/static/js/core/types.js @@ -367,7 +367,7 @@ * @property {'user'|'group'|'token'|'external'} subject_type * @property {string} subject_id * @property {string} subject_display - Username (users) or share name (tokens). - * @property {'viewer'|'editor'|'admin'} role + * @property {'viewer'|'commenter'|'contributor'|'editor'|'owner'} role - Server-emitted role string. `commenter` and `contributor` are reserved for future UI exposure; today the share modal only renders `viewer`/`editor`/`owner` (see `ShareRoleEnum`). * @property {string} granted_at - ISO-8601 * @property {string|null} [expires_at] - ISO-8601 or absent. * @property {boolean} has_password - True when a token subject has a password set. @@ -453,8 +453,11 @@ // ------------------- share modal /** - * Share roles (DTO-layer sugar for the ReBAC permission sets). - * @typedef {'viewer'|'editor'|'admin'} ShareRoleEnum + * Share-modal-exposed roles. The server's `Role` enum also includes + * `commenter` and `contributor` (see `OutgoingResourceGrant.role`); those + * are reserved for future UI exposure and are not offered as picker options + * today. The "Can manage" UI label maps to `owner`. + * @typedef {'viewer'|'editor'|'owner'} ShareRoleEnum */ /** diff --git a/tests/api/role_grants.hurl b/tests/api/role_grants.hurl new file mode 100644 index 00000000..4a0f4ede --- /dev/null +++ b/tests/api/role_grants.hurl @@ -0,0 +1,388 @@ +# ============================================================= +# OxiCloud — D-Prep: role_grants dual-write + new wire format +# ============================================================= +# Pins the D-Prep refactor behaviours that don't fit naturally into +# the existing `grants.hurl` (which is API-shape-focused). Specifically: +# +# 1. New wire-format role names: +# - "owner" is accepted on POST and emitted on response +# - "admin" still accepted on POST for one release (compat shim +# in `Role::parse`); the server normalises it to Owner +# +# 2. Dual-write proof: granting a role and then exercising a +# permission from its bundle works → proves the row landed in +# `storage.role_grants` because the engine now reads from there +# for authz decisions (see `folder_cascade_grant_exists` post- +# D-Prep). If dual-write failed, the engine would see no row and +# reject the check. +# +# 3. Atomic role updates via PUT /api/grants/role — the role flips +# in a single SQL update (no DELETE+INSERT race window). +# +# 4. Clean revoke: DELETE /api/grants/{id} clears role_grants too. +# +# Self-contained: creates its own users, folders, and files so it +# can run in any position relative to other test files. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 — admin login + home folder lookup +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +admin_token: jsonpath "$.access_token" +admin_user_id: jsonpath "$.user.id" + +GET {{base_url}}/api/folders +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Captures] +admin_home_id: jsonpath "$[0].id" + + +# ───────────────────────────────────────────────────────────── +# Step 2 — Create two fresh test users (renee, sam) so this file +# doesn't depend on cross-file fixtures. Use the +# legacy-compat path that POSTs to /api/admin/users. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "username": "renee", "password": "ReneePassword1!", "email": "renee@example.com", "role": "user" } + +HTTP 201 +[Captures] +renee_user_id: jsonpath "$.id" + + +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "username": "sam", "password": "SamPassword1!", "email": "sam@example.com", "role": "user" } + +HTTP 201 +[Captures] +sam_user_id: jsonpath "$.id" + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "renee", "password": "ReneePassword1!" } + +HTTP 200 +[Captures] +renee_token: jsonpath "$.access_token" + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "sam", "password": "SamPassword1!" } + +HTTP 200 +[Captures] +sam_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 3 — admin creates a folder "role-grants-test" + a file +# inside it to use as the authz target throughout the file. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/folders +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "name": "role-grants-test", "parent_id": "{{admin_home_id}}" } + +HTTP 201 +[Captures] +test_folder_id: jsonpath "$.id" + + +POST {{base_url}}/api/files/upload +Authorization: Bearer {{admin_token}} +[MultipartFormData] +folder_id: {{test_folder_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +test_file_id: jsonpath "$.id" + +# Rename immediately so subsequent throwaway uploads of `hello.txt` +# to the same folder don't 409. Each throwaway upload below applies +# the same pattern (upload → rename → use) to keep the namespace +# clean for the next one. +PUT {{base_url}}/api/files/{{test_file_id}}/rename +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "name": "step3-anchor.txt" } + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 4 — Grant renee role="owner" on the folder. New wire format. +# Response should echo back the canonical name. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/grants +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{renee_user_id}}" }, + "resource": { "type": "folder", "id": "{{test_folder_id}}" }, + "role": "owner" +} + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 5 — Dual-write proof: renee (now Owner of the folder) can +# DELETE a file inside it. Owner's bundle includes Delete; +# the engine reads from role_grants → if dual-write didn't +# land the row, the cascade query returns empty and the +# delete is refused. +# +# We upload + delete a throwaway file to avoid removing the +# test_file we'll need for later steps. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/files/upload +Authorization: Bearer {{admin_token}} +[MultipartFormData] +folder_id: {{test_folder_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +throwaway_file_id: jsonpath "$.id" + +# Rename so subsequent uploads in this folder don't 409 on "hello.txt". +PUT {{base_url}}/api/files/{{throwaway_file_id}}/rename +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "name": "renee-throwaway.txt" } + +HTTP 200 + + +DELETE {{base_url}}/api/files/{{throwaway_file_id}} +Authorization: Bearer {{renee_token}} + +HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step 6 — Legacy "admin" string compat. The server's Role::parse +# accepts "admin" and normalises to Owner during the D-Prep +# dual-write window (one release). Verify by granting sam +# with role="admin" — sam should then be able to Delete too. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/grants +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{sam_user_id}}" }, + "resource": { "type": "folder", "id": "{{test_folder_id}}" }, + "role": "admin" +} + +HTTP 201 +[Captures] +# Capture the first per-permission grant id from the response so +# Step 10's revoke doesn't have to query the My Shares endpoint +# with awkward JSONPath filtering. Any single grant_id works: +# the revoke handler calls `clear_role` which wipes the entire +# role_grants row for (sam, test_folder), so the engine reads +# return false for sam afterwards regardless of how many +# access_grants rows still exist. +sam_grant_id: jsonpath "$.grants[0].id" + + +# Sam should now have Owner-equivalent access — Delete works. +POST {{base_url}}/api/files/upload +Authorization: Bearer {{admin_token}} +[MultipartFormData] +folder_id: {{test_folder_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +sam_throwaway_id: jsonpath "$.id" + +PUT {{base_url}}/api/files/{{sam_throwaway_id}}/rename +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "name": "sam-throwaway.txt" } + +HTTP 200 + + +DELETE {{base_url}}/api/files/{{sam_throwaway_id}} +Authorization: Bearer {{sam_token}} + +HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step 7 — Atomic role update: demote renee from Owner to Viewer +# via PUT /api/grants/role. Single SQL UPDATE on +# role_grants — no DELETE+INSERT race. +# +# After: renee can still Read but should be refused Delete. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/api/grants/role +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{renee_user_id}}" }, + "resource": { "type": "folder", "id": "{{test_folder_id}}" }, + "role": "viewer" +} + +HTTP 200 + + +# Renee can still read the file (Viewer's bundle includes Read). +GET {{base_url}}/api/files/{{test_file_id}} +Authorization: Bearer {{renee_token}} + +HTTP 200 + + +# Renee CANNOT delete — Viewer's bundle excludes Delete; the +# folder_cascade_grant_exists query for Delete returns empty. +POST {{base_url}}/api/files/upload +Authorization: Bearer {{admin_token}} +[MultipartFormData] +folder_id: {{test_folder_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +post_demote_file_id: jsonpath "$.id" + +PUT {{base_url}}/api/files/{{post_demote_file_id}}/rename +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "name": "post-demote.txt" } + +HTTP 200 + + +DELETE {{base_url}}/api/files/{{post_demote_file_id}} +Authorization: Bearer {{renee_token}} + +HTTP * +[Asserts] +status >= 400 +status < 500 + + +# ───────────────────────────────────────────────────────────── +# Step 8 — Promote renee back to Editor (one role change, atomic) +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/api/grants/role +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{renee_user_id}}" }, + "resource": { "type": "folder", "id": "{{test_folder_id}}" }, + "role": "editor" +} + +HTTP 200 + + +# Editor's bundle includes Update — renaming a file should work. +PUT {{base_url}}/api/files/{{post_demote_file_id}}/rename +Authorization: Bearer {{renee_token}} +Content-Type: application/json +{ "name": "renamed-by-renee.txt" } + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 9 — My Shares response shape: the outgoing-resources endpoint +# must emit role strings from the new roster ("viewer" / +# "editor" / "owner"), never the legacy "admin". +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/grants/outgoing/resources +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +# The response body MUST contain "owner" (sam's role) and "editor" +# (renee's current role after the promote). It MUST NOT contain the +# legacy "admin" role string for any grant emitted by the server. +body contains "\"role\":\"owner\"" +body contains "\"role\":\"editor\"" +body not contains "\"role\":\"admin\"" + + +# ───────────────────────────────────────────────────────────── +# Step 10 — Revoke: removing sam's grant. The handler clears the +# access_grants rows AND calls clear_role to wipe the +# role_grants row in the same flow. +# +# After: sam's Delete attempt should be refused (proof +# the role_grants row is gone — the cascade query for +# Delete returns empty because sam has no row pointing +# at this folder). +# ───────────────────────────────────────────────────────────── + +# sam_grant_id was captured at Step 6 from the create response. +DELETE {{base_url}}/api/grants/{{sam_grant_id}} +Authorization: Bearer {{admin_token}} + +HTTP 204 + + +# Post-revoke: sam can no longer Delete in this folder. +POST {{base_url}}/api/files/upload +Authorization: Bearer {{admin_token}} +[MultipartFormData] +folder_id: {{test_folder_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +post_revoke_file_id: jsonpath "$.id" + +PUT {{base_url}}/api/files/{{post_revoke_file_id}}/rename +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "name": "post-revoke.txt" } + +HTTP 200 + + +DELETE {{base_url}}/api/files/{{post_revoke_file_id}} +Authorization: Bearer {{sam_token}} + +HTTP * +[Asserts] +status >= 400 +status < 500 + + +# ───────────────────────────────────────────────────────────── +# Step 11 — Teardown: clean up users + folder so this file +# leaves no residue for the storage_cleanup_check. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/folders/{{test_folder_id}} +Authorization: Bearer {{admin_token}} +HTTP 204 + +DELETE {{base_url}}/api/admin/users/{{renee_user_id}} +Authorization: Bearer {{admin_token}} +HTTP 200 + +DELETE {{base_url}}/api/admin/users/{{sam_user_id}} +Authorization: Bearer {{admin_token}} +HTTP 200 diff --git a/tests/api/run.sh b/tests/api/run.sh index 1524b63e..d07cfbb9 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -146,6 +146,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/public_shares.hurl" \ "$API_DIR/permissions.hurl" \ "$API_DIR/grants.hurl" \ + "$API_DIR/role_grants.hurl" \ "$API_DIR/subject_groups.hurl" \ "$API_DIR/groups_effective_members.hurl" \ "$API_DIR/grants_nested_groups.hurl" \ From ae219b81fcb1217c7e952726a057e56f7c25e2a3 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Wed, 17 Jun 2026 23:26:17 +0200 Subject: [PATCH 3/6] chore(roles): add validation script for migration script on Ed's sandbox, adapt values accordingly your environment --- tools/audit-grants-bundle-shape.sql | 215 ++++++++++++++++++ tools/d-prep-real-db-smoke.sh | 328 ++++++++++++++++++++++++++++ 2 files changed, 543 insertions(+) create mode 100644 tools/audit-grants-bundle-shape.sql create mode 100755 tools/d-prep-real-db-smoke.sh diff --git a/tools/audit-grants-bundle-shape.sql b/tools/audit-grants-bundle-shape.sql new file mode 100644 index 00000000..fb3d24de --- /dev/null +++ b/tools/audit-grants-bundle-shape.sql @@ -0,0 +1,215 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- audit-grants-bundle-shape.sql — Pre-D-Prep diagnostic +-- ════════════════════════════════════════════════════════════════════════════ +-- Purpose: confirm the assumption that >99% of existing `storage.access_grants` +-- rows already cluster into the standard role bundles +-- (viewer / commenter / contributor / editor / owner). The answer determines +-- whether the access_grants → role_grants migration is fully mechanical (just +-- run the backfill) or whether per-row decisions are needed for some edge +-- cases. +-- +-- READ-ONLY. Safe to run against any environment including production. +-- +-- Usage: +-- psql "$DATABASE_URL" -f tools/audit-grants-bundle-shape.sql +-- +-- Reference role bundles (mirror `Role::expand()` in +-- `src/application/dtos/grant_dto.rs`): +-- viewer = {read} +-- commenter = {read, comment} ← reserved variant +-- contributor = {read, create} ← new in D-Prep +-- editor = {read, comment, create, update} +-- owner = {read, comment, create, update, share, delete} +-- (post-D-Prep: + manage, when Group-as-Resource lands) +-- ════════════════════════════════════════════════════════════════════════════ + + +\echo '' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' 0. Total population' +\echo '─────────────────────────────────────────────────────────────────────────' + +SELECT count(*) AS total_grant_rows, + count(DISTINCT (subject_type, subject_id, resource_type, resource_id)) + AS distinct_clusters, + ROUND( + count(*)::numeric + / NULLIF(count(DISTINCT (subject_type, subject_id, resource_type, resource_id)), 0), + 2 + ) AS avg_rows_per_cluster +FROM storage.access_grants; + + +\echo '' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' 1. Bundle distribution — which permission sets exist, how popular?' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' Each row = a unique permission set; cluster_count = how many (subject,' +\echo ' resource) pairs have exactly that set.' + +WITH cluster AS ( + SELECT subject_type, + subject_id, + resource_type, + resource_id, + array_agg(permission ORDER BY permission) AS perms + FROM storage.access_grants + GROUP BY 1, 2, 3, 4 +), +known_bundles AS ( + SELECT ARRAY['read']::text[] AS perms, 'viewer' AS role + UNION ALL SELECT ARRAY['comment','read']::text[], 'commenter' + UNION ALL SELECT ARRAY['create','read']::text[], 'contributor' + UNION ALL SELECT ARRAY['comment','create','read','update']::text[], 'editor' + UNION ALL SELECT ARRAY['comment','create','delete','read','share','update']::text[], 'owner' +) +SELECT cluster.perms, + count(*) AS cluster_count, + ROUND(100.0 * count(*) / SUM(count(*)) OVER (), 2) AS pct, + COALESCE(known_bundles.role, '(non-bundle)') AS maps_to_role +FROM cluster +LEFT JOIN known_bundles ON known_bundles.perms = cluster.perms +GROUP BY cluster.perms, known_bundles.role +ORDER BY cluster_count DESC; + + +\echo '' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' 2. Bundle-shaped vs not — the headline number' +\echo '─────────────────────────────────────────────────────────────────────────' + +WITH cluster AS ( + SELECT subject_type, + subject_id, + resource_type, + resource_id, + array_agg(permission ORDER BY permission) AS perms + FROM storage.access_grants + GROUP BY 1, 2, 3, 4 +), +known_bundles AS ( + SELECT ARRAY['read']::text[] AS perms + UNION ALL SELECT ARRAY['comment','read']::text[] + UNION ALL SELECT ARRAY['create','read']::text[] + UNION ALL SELECT ARRAY['comment','create','read','update']::text[] + UNION ALL SELECT ARRAY['comment','create','delete','read','share','update']::text[] +), +shape AS ( + SELECT CASE WHEN cluster.perms = ANY(SELECT kb.perms FROM known_bundles kb) + THEN 'bundle-shaped' + ELSE 'NON-bundle (needs per-row decision)' + END AS shape + FROM cluster +) +SELECT shape, + count(*) AS clusters, + ROUND(100.0 * count(*) / SUM(count(*)) OVER (), 2) AS pct +FROM shape +GROUP BY shape +ORDER BY clusters DESC; + + +\echo '' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' 3. Non-bundle clusters in detail (the <1% — investigate each)' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' If this returns 0 rows, the migration is FULLY mechanical. Otherwise' +\echo ' eyeball each row and decide: closest role + audit log entry, or' +\echo ' refuse-to-migrate (rare).' + +WITH cluster AS ( + SELECT subject_type, + subject_id, + resource_type, + resource_id, + array_agg(permission ORDER BY permission) AS perms + FROM storage.access_grants + GROUP BY 1, 2, 3, 4 +), +known_bundles AS ( + SELECT ARRAY['read']::text[] AS perms + UNION ALL SELECT ARRAY['comment','read']::text[] + UNION ALL SELECT ARRAY['create','read']::text[] + UNION ALL SELECT ARRAY['comment','create','read','update']::text[] + UNION ALL SELECT ARRAY['comment','create','delete','read','share','update']::text[] +) +SELECT cluster.subject_type, + cluster.subject_id, + cluster.resource_type, + cluster.resource_id, + cluster.perms +FROM cluster +WHERE NOT (cluster.perms = ANY(SELECT kb.perms FROM known_bundles kb)) +ORDER BY cluster.resource_type, cluster.resource_id +LIMIT 200; + + +\echo '' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' 4. Sanity checks — invariants that should be true today' +\echo '─────────────────────────────────────────────────────────────────────────' + +\echo '' +\echo ' 4a. Clusters with no Read permission (broken? if >0, investigate)' +WITH cluster AS ( + SELECT subject_type, subject_id, resource_type, resource_id, + array_agg(permission ORDER BY permission) AS perms + FROM storage.access_grants + GROUP BY 1, 2, 3, 4 +) +SELECT count(*) AS clusters_without_read +FROM cluster +WHERE NOT ('read' = ANY(perms)); + +\echo '' +\echo ' 4b. Subject-type distribution (sanity check on token / external counts)' +SELECT subject_type, + count(*) AS rows, + count(DISTINCT subject_id) AS distinct_subjects, + count(DISTINCT (resource_type, resource_id)) AS distinct_resources +FROM storage.access_grants +GROUP BY subject_type +ORDER BY subject_type; + +\echo '' +\echo ' 4c. Resource-type distribution' +SELECT resource_type, count(*) AS rows +FROM storage.access_grants +GROUP BY resource_type +ORDER BY rows DESC; + +\echo '' +\echo ' 4d. Permission distribution (total rows per permission value)' +SELECT permission, count(*) AS rows +FROM storage.access_grants +GROUP BY permission +ORDER BY rows DESC; + + +\echo '' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' 5. Migration cost estimate' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' role_grants will have ~ (distinct clusters) rows, replacing ~ (total)' +\echo ' rows in access_grants. The ratio is the per-row reduction factor.' + +WITH cluster AS ( + SELECT subject_type, subject_id, resource_type, resource_id + FROM storage.access_grants + GROUP BY 1, 2, 3, 4 +) +SELECT (SELECT count(*) FROM storage.access_grants) AS access_grants_rows_today, + (SELECT count(*) FROM cluster) AS role_grants_rows_after, + ROUND( + (SELECT count(*) FROM storage.access_grants)::numeric + / NULLIF((SELECT count(*) FROM cluster), 0), + 2 + ) AS reduction_factor; + + +\echo '' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo ' DONE — share section 2 (headline) and section 3 (non-bundle detail)' +\echo ' to make the D-Prep PR scope decision.' +\echo '─────────────────────────────────────────────────────────────────────────' +\echo '' diff --git a/tools/d-prep-real-db-smoke.sh b/tools/d-prep-real-db-smoke.sh new file mode 100755 index 00000000..6ded56ac --- /dev/null +++ b/tools/d-prep-real-db-smoke.sh @@ -0,0 +1,328 @@ +#!/usr/bin/env bash +# ════════════════════════════════════════════════════════════════════════════ +# d-prep-real-db-smoke.sh — Validate D-Prep migration against sandbox data +# ════════════════════════════════════════════════════════════════════════════ +# Reads from your sandbox DB (real data — but it's a sandbox, not prod), dumps +# it, restores into an isolated parallel DB, applies the D-Prep migration +# (`20260730000000_role_grants.sql`), and verifies the backfill against the +# pre-recorded audit numbers from `tools/audit-grants-bundle-shape.sql`. +# +# The sandbox DB itself is NEVER MODIFIED — everything happens in the parallel +# `oxicloud_dprep_smoke` DB which is dropped at the start of each run. +# +# Usage: +# tools/d-prep-real-db-smoke.sh # uses $DATABASE_URL +# tools/d-prep-real-db-smoke.sh --keep # leave smoke DB for poking after +# tools/d-prep-real-db-smoke.sh --help +# +# Requirements: +# - pg_dump / pg_restore / psql (PostgreSQL 14+ should be fine; sandbox version-matched) +# - DATABASE_URL pointing at the sandbox DB +# +# PATH gotcha (Mac brew): `brew install postgresql@18` is keg-only; if pg_dump +# isn't on PATH, run: +# export PATH="$(brew --prefix postgresql@18)/bin:$PATH" +# +# Expected audit numbers (from your audit run on 2026-06-17): +# - 73 access_grants rows clustering into 38 (subject, resource) pairs +# - 29 viewer / 5 editor / 4 owner +# These are encoded as assertions below. If your sandbox data has shifted +# since the audit, update EXPECTED_* variables OR re-run the audit script +# first. +# +# Memory cross-refs: +# - bug_pg_dump_folders_circular_fk.md — circular FK on `folders` forces +# `pg_restore --disable-triggers` +# - project_drive_sequence_a.md — D-Prep scope + audit data +# ════════════════════════════════════════════════════════════════════════════ + +set -euo pipefail + + +# ── Config ────────────────────────────────────────────────────────────────── + +SMOKE_DB_NAME="oxicloud_dprep_smoke" +DUMP_FILE="${TMPDIR:-/tmp}/oxicloud-sandbox-${SMOKE_DB_NAME}.dump" +MIGRATION_FILE="migrations/20260730000000_role_grants.sql" + +# Expected values from the audit (tools/audit-grants-bundle-shape.sql run +# on 2026-06-17). Bump these if you re-run the audit and see different +# numbers — they are intentionally HARDCODED so a silent drift gets caught. +EXPECTED_DISTINCT_CLUSTERS=38 +EXPECTED_VIEWER=29 +EXPECTED_EDITOR=5 +EXPECTED_OWNER=4 + +KEEP_DB=0 + + +# ── Arg parsing ───────────────────────────────────────────────────────────── + +usage() { + sed -n '2,/^$/p' "$0" | sed 's/^# \{0,1\}//' + exit "${1:-0}" +} + +for arg in "$@"; do + case "$arg" in + --keep|--keep-db) KEEP_DB=1 ;; + -h|--help) usage 0 ;; + *) echo "Unknown arg: $arg" >&2 ; usage 2 ;; + esac +done + + +# ── Pre-flight ────────────────────────────────────────────────────────────── + +if [[ -z "${DATABASE_URL:-}" ]]; then + echo "ERROR: \$DATABASE_URL must be set and point at the sandbox DB." >&2 + exit 2 +fi + +for bin in pg_dump pg_restore psql; do + if ! command -v "$bin" >/dev/null 2>&1; then + echo "ERROR: '$bin' not found on PATH." >&2 + echo "Hint: brew-installed postgresql is keg-only. Run:" >&2 + echo " export PATH=\"\$(brew --prefix postgresql@18)/bin:\$PATH\"" >&2 + exit 2 + fi +done + +if [[ ! -f "$MIGRATION_FILE" ]]; then + echo "ERROR: migration file not found: $MIGRATION_FILE" >&2 + echo "Run this script from the repo root." >&2 + exit 2 +fi + +# Derive connection URLs for the maintenance DB (where we'll issue +# DROP/CREATE DATABASE) and the smoke DB itself, by splicing +# $SMOKE_DB_NAME into $DATABASE_URL. Avoids relying on libpq's +# default local Unix socket — `dropdb`/`createdb`/`psql` without an +# explicit -d won't read $DATABASE_URL and try /tmp/.s.PGSQL.5432 +# (which fails on Mac brew where the server runs on a non-default +# socket / port). +URL_NO_QUERY="${DATABASE_URL%%\?*}" +URL_QUERY="${DATABASE_URL:${#URL_NO_QUERY}}" # "" or "?…" +SOURCE_DB_NAME="${URL_NO_QUERY##*/}" +URL_BASE="${URL_NO_QUERY%/"$SOURCE_DB_NAME"}" +MAINTENANCE_URL="${URL_BASE}/postgres${URL_QUERY}" +SMOKE_URL="${URL_BASE}/${SMOKE_DB_NAME}${URL_QUERY}" + +if [[ "$SOURCE_DB_NAME" == "$SMOKE_DB_NAME" ]]; then + echo "ERROR: DATABASE_URL appears to point at '$SMOKE_DB_NAME' — refusing" >&2 + echo "to dump-and-restore on top of itself. Set DATABASE_URL to the sandbox." >&2 + exit 2 +fi + + +# ── Helpers ───────────────────────────────────────────────────────────────── + +log() { echo "[smoke] $*"; } +fail() { echo "[smoke] FAIL: $*" >&2 ; exit 1; } +pass() { echo "[smoke] PASS: $*"; } + +# Run a query against the smoke DB and capture a single scalar. +scalar() { + psql -tAX -d "$SMOKE_URL" -c "$1" +} + +# Run a query and check the result equals an expected scalar. +expect_scalar() { + local query="$1" expected="$2" label="$3" + local actual + actual=$(scalar "$query") + if [[ "$actual" == "$expected" ]]; then + pass "$label: $actual" + else + fail "$label: expected $expected, got '$actual'" + fi +} + + +# ── 1. Snapshot the sandbox ───────────────────────────────────────────────── + +log "Dumping sandbox DB → $DUMP_FILE" +log "(custom format, so pg_restore --disable-triggers can side-step the" +log " circular FK on storage.folders.parent_id — see bug memory.)" +pg_dump "$DATABASE_URL" --format=custom --no-owner --no-privileges \ + --file="$DUMP_FILE" +log "Dumped $(du -h "$DUMP_FILE" | awk '{print $1}')" + + +# ── 2. Recreate the smoke DB ──────────────────────────────────────────────── + +log "Dropping smoke DB '$SMOKE_DB_NAME' if it exists" +psql -d "$MAINTENANCE_URL" -c "DROP DATABASE IF EXISTS \"$SMOKE_DB_NAME\"" + +log "Creating fresh smoke DB '$SMOKE_DB_NAME'" +psql -d "$MAINTENANCE_URL" -c "CREATE DATABASE \"$SMOKE_DB_NAME\"" + + +# ── 3. Restore sandbox into smoke DB ──────────────────────────────────────── + +log "Restoring sandbox dump into smoke DB" +# --disable-triggers handles the storage.folders parent_id circular FK without +# wrapping every COPY in SET CONSTRAINTS ALL DEFERRED. +# --no-owner --no-privileges already on dump side; --single-transaction makes +# the restore atomic so a mid-flight failure leaves no half-state. +pg_restore --dbname="$SMOKE_URL" --disable-triggers \ + --single-transaction --no-owner --no-privileges \ + "$DUMP_FILE" + + +# ── 4. Wipe any prior role_grants state from the smoke DB ────────────────── +# The sandbox may already have role_grants if the server has been booted +# with the D-Prep migration applied (auto-migrate on startup). For the +# smoke we want to exercise the migration FRESHLY — same as a brand-new +# install — so drop both the migration's tables and let it recreate them +# from scratch against the access_grants snapshot. + +log "Wiping any prior role_grants state from smoke DB (so we test the migration freshly)" +psql -d "$SMOKE_URL" --set ON_ERROR_STOP=1 -c " +DROP TABLE IF EXISTS storage.role_grants_migration_log CASCADE; +DROP TABLE IF EXISTS storage.role_grants CASCADE; +" + +# ── 5. Pre-migration sanity ──────────────────────────────────────────────── + +log "Pre-migration sanity checks" +expect_scalar "SELECT count(*) FROM storage.access_grants" \ + "73" "access_grants row count matches audit" + +if [[ $(scalar "SELECT to_regclass('storage.role_grants') IS NULL") != "t" ]]; then + fail "storage.role_grants survived the drop — something is wrong." +fi +pass "storage.role_grants does not exist (clean slate)" + + +# ── 5. Apply the D-Prep migration ─────────────────────────────────────────── + +log "Applying D-Prep migration: $MIGRATION_FILE" +# Wrap in a transaction so a constraint failure rolls back cleanly; the +# migration itself has BEGIN/COMMIT semantics via -1 to psql. +psql -d "$SMOKE_URL" --set ON_ERROR_STOP=1 -1 -f "$MIGRATION_FILE" +log "Migration applied" + + +# ── 6. Post-migration assertions ──────────────────────────────────────────── + +log "Post-migration verification" + +# Row count: should match `distinct_clusters` from the audit +expect_scalar "SELECT count(*) FROM storage.role_grants" \ + "$EXPECTED_DISTINCT_CLUSTERS" "role_grants row count" + +# Role distribution +expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role = 'viewer'" \ + "$EXPECTED_VIEWER" "viewer count" +expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role = 'editor'" \ + "$EXPECTED_EDITOR" "editor count" +expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role = 'owner'" \ + "$EXPECTED_OWNER" "owner count" + +# Zero NULL roles, zero non-bundle roles (the CHECK constraint should +# already enforce this, but proving it here too) +expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role IS NULL" \ + "0" "no NULL roles" +expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role NOT IN ('viewer','commenter','contributor','editor','owner')" \ + "0" "no unknown roles" + +# access_grants is untouched +expect_scalar "SELECT count(*) FROM storage.access_grants" \ + "73" "access_grants row count unchanged (dual-write safety net intact)" + + +# ── 7. Equivalence check (strongest assertion) ────────────────────────────── +# For every role_grants row, the corresponding (subject, resource) cluster +# in access_grants must have exactly the role's bundle as its permission +# set. If any row's bundle doesn't match what `Role::expand()` says, the +# backfill mis-mapped and the equivalence count goes non-zero. + +log "Bundle equivalence check: role_grants ↔ access_grants" + +EQUIVALENCE_MISMATCHES=$(scalar " +WITH expected AS ( + SELECT 'viewer'::text AS role, ARRAY['read']::text[] AS perms + UNION ALL SELECT 'commenter', ARRAY['comment','read']::text[] + UNION ALL SELECT 'contributor', ARRAY['create','read']::text[] + UNION ALL SELECT 'editor', ARRAY['comment','create','read','update']::text[] + UNION ALL SELECT 'owner', ARRAY['comment','create','delete','read','share','update']::text[] +), +actual AS ( + SELECT subject_type, subject_id, resource_type, resource_id, + array_agg(permission ORDER BY permission) AS perms + FROM storage.access_grants + GROUP BY 1, 2, 3, 4 +) +SELECT count(*) +FROM storage.role_grants rg +JOIN actual a USING (subject_type, subject_id, resource_type, resource_id) +JOIN expected e ON e.role = rg.role +WHERE a.perms <> e.perms +") + +if [[ "$EQUIVALENCE_MISMATCHES" == "0" ]]; then + pass "every role_grants row's bundle matches access_grants exactly" +else + log "" + log "MISMATCH DETAIL (first 10):" + psql -d "$SMOKE_URL" -c " +WITH expected AS ( + SELECT 'viewer'::text AS role, ARRAY['read']::text[] AS perms + UNION ALL SELECT 'commenter', ARRAY['comment','read']::text[] + UNION ALL SELECT 'contributor', ARRAY['create','read']::text[] + UNION ALL SELECT 'editor', ARRAY['comment','create','read','update']::text[] + UNION ALL SELECT 'owner', ARRAY['comment','create','delete','read','share','update']::text[] +), +actual AS ( + SELECT subject_type, subject_id, resource_type, resource_id, + array_agg(permission ORDER BY permission) AS perms + FROM storage.access_grants + GROUP BY 1, 2, 3, 4 +) +SELECT rg.subject_type, rg.subject_id, rg.resource_type, rg.resource_id, + rg.role, + a.perms AS access_grants_perms, + e.perms AS expected_perms +FROM storage.role_grants rg +JOIN actual a USING (subject_type, subject_id, resource_type, resource_id) +JOIN expected e ON e.role = rg.role +WHERE a.perms <> e.perms +LIMIT 10 +" + fail "$EQUIVALENCE_MISMATCHES role_grants rows have a bundle that diverges from their access_grants cluster" +fi + + +# ── 8. Migration audit log shape ──────────────────────────────────────────── +# `role_grants_migration_log` is the one-shot table created by the migration +# to record any demotions. With 100%-bundle-shaped data (the audit says so), +# it should be EMPTY — nothing was demoted. + +if [[ $(scalar "SELECT to_regclass('storage.role_grants_migration_log') IS NOT NULL") == "t" ]]; then + expect_scalar "SELECT count(*) FROM storage.role_grants_migration_log" \ + "0" "migration audit log empty (no demotions, as expected for 100% bundle-shaped data)" +fi + + +# ── 9. Cleanup ────────────────────────────────────────────────────────────── + +log "" +log "─────────────────────────────────────────────────────────" +log " ALL ASSERTIONS PASSED" +log "─────────────────────────────────────────────────────────" +log "" + +if [[ "$KEEP_DB" == "1" ]]; then + log "Leaving smoke DB '$SMOKE_DB_NAME' for inspection." + log "Poke at it with:" + log " psql \"$SMOKE_URL\"" + log "" + log "Drop when done:" + log " psql -d \"$MAINTENANCE_URL\" -c 'DROP DATABASE \"$SMOKE_DB_NAME\"'" +else + log "Dropping smoke DB '$SMOKE_DB_NAME'" + psql -d "$MAINTENANCE_URL" -c "DROP DATABASE \"$SMOKE_DB_NAME\"" +fi + +log "Dump file kept at $DUMP_FILE (delete with: rm '$DUMP_FILE')" From cc6f53528b97bc5a4f79ea7ae61bb4622588fcef Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Wed, 17 Jun 2026 23:35:58 +0200 Subject: [PATCH 4/6] feat(role): reflect ReBAC doc with changes --- docs/architecture/rebac-authorization.md | 12 ++++++++- docs/plan/drive.md | 8 ++++++ ...plan-ReBAC-Permissions-Grants-Cascading.md | 27 ++++++++++++------- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/docs/architecture/rebac-authorization.md b/docs/architecture/rebac-authorization.md index ca5c927e..2a1d70d9 100644 --- a/docs/architecture/rebac-authorization.md +++ b/docs/architecture/rebac-authorization.md @@ -107,9 +107,19 @@ storage.access_grants expires_at TIMESTAMPTZ NULL ``` -One row per `(subject, permission, resource)` triple. An "admin role on folder +One row per `(subject, permission, resource)` triple. An "owner role on folder X for user Y" is 6 rows; a "viewer role" is 1 row. +> **Note (D-Prep, 2026-06-17):** the role assignment has since pivoted into +> a separate `storage.role_grants` table that stores **one row per role +> assignment** rather than one per permission. `access_grants` stays +> populated via dual-write during the transition; the engine reads the +> role-keyed table for authz decisions. The cleanup PR drops +> `access_grants` after the dual-write window. The historical role name +> `Admin` was renamed to `Owner` at the same time, to disambiguate from +> `UserRole::Admin` (user-account privilege) and match Drive plan +> terminology. + Cleanup is trigger-driven (`trg_cleanup_grants_folder`, …): when a resource or subject is deleted, all referencing grants disappear in the same transaction. diff --git a/docs/plan/drive.md b/docs/plan/drive.md index d94ec018..e567c00f 100644 --- a/docs/plan/drive.md +++ b/docs/plan/drive.md @@ -44,6 +44,14 @@ recognise it instantly. ## Prerequisite — PR D-Prep: `access_grants → role_grants` +**Status (2026-06-17): scope complete, ready to PR.** The schema migration +runs cleanly against real sandbox data (38 role_grants rows produced, +matching the audit's 29 viewer / 5 editor / 4 owner distribution, zero +bundle mismatches). End-to-end Hurl test (`tests/api/role_grants.hurl`) +covers create / atomic role update / revoke with both the canonical +`"owner"` and the legacy `"admin"` compat wire format. Engine reads +pivot to `role_grants` for authz decisions; `access_grants` stays +populated via dual-write as the safety net for one release cycle. **Ratified 2026-06-16.** A separate PR lands BEFORE D0 that refactors `storage.access_grants` into `storage.role_grants` with role-bundle semantics: diff --git a/docs/plan/plan-ReBAC-Permissions-Grants-Cascading.md b/docs/plan/plan-ReBAC-Permissions-Grants-Cascading.md index 5a0a6506..9a55d1cb 100644 --- a/docs/plan/plan-ReBAC-Permissions-Grants-Cascading.md +++ b/docs/plan/plan-ReBAC-Permissions-Grants-Cascading.md @@ -602,20 +602,29 @@ impl Role { pub fn expand(self) -> &'static [Permission] { match self { Role::Viewer => &[Permission::Read], - Role::Commenter => &[Permission::Read, Permission::Comment], - Role::Editor => &[Permission::Read, Permission::Comment, - Permission::Create, Permission::Update], - Role::Manager => &[Permission::Read, Permission::Comment, - Permission::Create, Permission::Update, - Permission::Share], - Role::Admin => &[Permission::Read, Permission::Comment, - Permission::Create, Permission::Update, - Permission::Share, Permission::Delete], + Role::Commenter => &[Permission::Read, Permission::Comment], + Role::Contributor => &[Permission::Read, Permission::Create], + Role::Editor => &[Permission::Read, Permission::Comment, + Permission::Create, Permission::Update], + Role::Owner => &[Permission::Read, Permission::Comment, + Permission::Create, Permission::Update, + Permission::Share, Permission::Delete, + Permission::Manage], } } } ``` +> **Note (D-Prep, 2026-06-17):** the `Manager` role was retired before shipping +> (its bundle was a strict subset of `Owner`); the historical `Admin` role was +> renamed to `Owner` to disambiguate from `UserRole::Admin` (the user-account +> privilege) and match Drive plan terminology. `Contributor` is the new +> drop-zone role. The actual on-the-wire enum lives in +> `src/application/dtos/grant_dto.rs`; that file is the canonical source of +> truth for bundle expansion. The pivot to role-keyed storage (`role_grants` +> table) also happened in D-Prep — see +> `docs/architecture/rebac-authorization.md` for the dual-write timeline. + ### `POST /api/grants` accepts either shape ```json From 72129af0bd9a0fe527bd1943d5130c108f3c4a32 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Thu, 18 Jun 2026 01:42:32 +0200 Subject: [PATCH 5/6] refactor(role): use grant only - remove permission centric mode - finalize migration drop all tables with permissions - ensure roles are ENUM (owner is always displayed first) --- .../20260801000000_role_grants_enum.sql | 72 +++ ...801000001_role_grants_cascade_triggers.sql | 122 +++++ .../20260801000002_drop_access_grants.sql | 63 +++ src/application/dtos/grant_dto.rs | 202 ++------ src/application/ports/authorization_ports.rs | 64 +-- .../services/auth_application_service.rs | 4 +- src/application/services/share_service.rs | 12 +- .../services/subject_group_service.rs | 26 +- src/domain/entities/share.rs | 2 +- src/domain/entities/subject_group.rs | 2 +- .../repositories/subject_group_repository.rs | 4 +- src/domain/services/authorization.rs | 130 ++++- .../repositories/pg/share_pg_repository.rs | 16 +- .../pg/subject_group_pg_repository.rs | 4 +- src/infrastructure/services/pg_acl_engine.rs | 446 ++++++++---------- src/integration_test_support.rs | 4 +- src/interfaces/api/handlers/grant_handler.rs | 200 ++------ src/interfaces/api/mod.rs | 4 +- static/js/components/shareModal.js | 33 +- static/js/core/types.js | 24 +- static/js/model/grants.js | 4 +- static/sw.js | 2 +- tests/api/grants.hurl | 41 +- tests/api/grants_nested_groups.hurl | 8 +- tests/api/role_grants.hurl | 53 ++- tests/api/subject_groups.hurl | 2 +- 26 files changed, 800 insertions(+), 744 deletions(-) create mode 100644 migrations/20260801000000_role_grants_enum.sql create mode 100644 migrations/20260801000001_role_grants_cascade_triggers.sql create mode 100644 migrations/20260801000002_drop_access_grants.sql diff --git a/migrations/20260801000000_role_grants_enum.sql b/migrations/20260801000000_role_grants_enum.sql new file mode 100644 index 00000000..81ac0f16 --- /dev/null +++ b/migrations/20260801000000_role_grants_enum.sql @@ -0,0 +1,72 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- Cleanup #1: storage.role_grants.role — TEXT → storage.grant_role ENUM +-- ════════════════════════════════════════════════════════════════════════════ +-- D-Prep shipped `role_grants.role` as TEXT + CHECK constraint. Promoting it +-- to a native PostgreSQL ENUM gives us three things at once: +-- +-- 1. Index-driven sort by role strength. The ENUM values are declared in +-- strength order — owner first, viewer last. `ORDER BY role ASC` then +-- yields the UX-mandated "strongest first" ordering (Owner → Editor → +-- Contributor → Commenter → Viewer) without a CASE expression. The +-- `idx_role_grants_subject` / `idx_role_grants_resource` indexes can be +-- extended (or composite-augmented) with the role column for index-only +-- ordered scans. +-- +-- 2. Type-level safety. The CHECK constraint goes away; invalid roles fail +-- at the column type, not at row insertion. One contract instead of two +-- (column type AND check constraint). +-- +-- 3. Cleaner query shape. Every listing query that used the strength CASE +-- becomes a plain `ORDER BY role` after this migration. +-- +-- Trade-off accepted: PostgreSQL ENUMs allow ADD VALUE (with BEFORE / AFTER +-- positional anchors) and RENAME VALUE, but not DROP VALUE or arbitrary +-- reorder. The OxiCloud role roster is intentionally stable — new roles get +-- appended, none get reordered or removed. Confirmed with Ed. +-- +-- This migration must run BEFORE the access_grants drop, since it's purely +-- about role_grants.role. + +-- ── 1. Create the ENUM type ──────────────────────────────────────────────── +-- Declaration order = sort order. Strongest first so `ORDER BY role ASC` +-- matches the UX requirement (max permission → least permission). + +CREATE TYPE storage.grant_role AS ENUM ( + 'owner', -- ordinal 0, sorts first + 'editor', -- ordinal 1 + 'contributor', -- ordinal 2 + 'commenter', -- ordinal 3 + 'viewer' -- ordinal 4, sorts last +); + +COMMENT ON TYPE storage.grant_role IS + 'Role-keyed grant strength. Declaration order is sort order: ORDER BY ' + 'role ASC yields owner → viewer (strongest → weakest), matching the ' + 'share-dialog and shared-with-me UX. Adding a new role is ALTER TYPE ' + 'ADD VALUE; renaming is ALTER TYPE RENAME VALUE. Dropping or reordering ' + 'is not supported — adjust the roster only by append.'; + + +-- ── 2. Drop the redundant CHECK constraint ───────────────────────────────── +-- The inline CHECK on role_grants.role was auto-named +-- `role_grants_role_check` by PostgreSQL. Drop it before the type swap — +-- the ENUM now enforces the same invariant at the column level. + +ALTER TABLE storage.role_grants + DROP CONSTRAINT IF EXISTS role_grants_role_check; + + +-- ── 3. Convert role TEXT → storage.grant_role ────────────────────────────── +-- USING cast: text values are guaranteed to be one of the five valid labels +-- (the dropped CHECK enforced this; the D-Prep backfill only produced these +-- five values). If a stray value slipped through, the cast errors out and +-- the migration aborts — preferable to silently coercing. + +ALTER TABLE storage.role_grants + ALTER COLUMN role TYPE storage.grant_role + USING role::storage.grant_role; + +COMMENT ON COLUMN storage.role_grants.role IS + 'One of owner / editor / contributor / commenter / viewer. Expanded to ' + 'a Permission bundle by the in-code role_bundle() function at engine ' + 'read time. Sort order matches declaration order in storage.grant_role.'; diff --git a/migrations/20260801000001_role_grants_cascade_triggers.sql b/migrations/20260801000001_role_grants_cascade_triggers.sql new file mode 100644 index 00000000..1fca5043 --- /dev/null +++ b/migrations/20260801000001_role_grants_cascade_triggers.sql @@ -0,0 +1,122 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- Cleanup #2: cascade triggers for storage.role_grants +-- ════════════════════════════════════════════════════════════════════════════ +-- The D-Prep migration created `storage.role_grants` but no cascade triggers. +-- Until now, role_grants stayed consistent because the application-layer +-- lifecycle hooks (`engine.revoke_all_for_resource` / `_subject`) wiped rows +-- on the canonical delete paths, AND the existing `trg_cleanup_grants_*` +-- triggers kept `storage.access_grants` clean as a defence-in-depth net. +-- +-- The follow-up cleanup PR drops `access_grants` (and its triggers) entirely. +-- Without this migration that drop would leave `role_grants` without any +-- DB-level safety net — direct SQL, future codepaths that forget to call the +-- engine hooks, and any other bypass route could orphan rows whose subject +-- or resource has already been deleted. +-- +-- This migration mirrors the four forward + one reverse triggers from +-- `20260520000000_rebac_access_grants.sql` and `20260612000001_share_grant_ +-- reverse_cascade.sql`, retargeted at `storage.role_grants`. Same shape, same +-- AFTER-DELETE semantics, same idempotent CREATE OR REPLACE patterns. +-- +-- During the transition window (this migration applied; `access_grants` not +-- yet dropped) both sets of triggers coexist — they target different tables +-- and don't conflict. Once `access_grants` is dropped, the old triggers and +-- their helper functions vanish in the same migration. + +-- ── 1. Forward cascade: resource delete → cleanup role_grants ────────────── +-- Fires AFTER DELETE on storage.folders / storage.files; deletes every +-- role_grants row referencing that resource. TG_ARGV[0] discriminates which +-- resource_type the trigger is wired for. + +CREATE OR REPLACE FUNCTION storage.cleanup_role_grants_on_resource_delete() +RETURNS TRIGGER AS $$ +BEGIN + DELETE FROM storage.role_grants + WHERE resource_type = TG_ARGV[0] + AND resource_id = OLD.id; + RETURN OLD; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS trg_cleanup_role_grants_folder ON storage.folders; +CREATE TRIGGER trg_cleanup_role_grants_folder + AFTER DELETE ON storage.folders + FOR EACH ROW + EXECUTE FUNCTION storage.cleanup_role_grants_on_resource_delete('folder'); + +DROP TRIGGER IF EXISTS trg_cleanup_role_grants_file ON storage.files; +CREATE TRIGGER trg_cleanup_role_grants_file + AFTER DELETE ON storage.files + FOR EACH ROW + EXECUTE FUNCTION storage.cleanup_role_grants_on_resource_delete('file'); + + +-- ── 2. Forward cascade: subject delete → cleanup role_grants ─────────────── +-- Fires AFTER DELETE on auth.users / storage.shares; deletes every +-- role_grants row referencing that subject. Groups are NOT wired here — +-- `subject_group_service::delete()` performs that cascade transactionally +-- in application code, mirroring the historical access_grants behaviour. + +CREATE OR REPLACE FUNCTION storage.cleanup_role_grants_on_subject_delete() +RETURNS TRIGGER AS $$ +BEGIN + DELETE FROM storage.role_grants + WHERE subject_type = TG_ARGV[0] + AND subject_id = OLD.id; + RETURN OLD; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS trg_cleanup_role_grants_user ON auth.users; +CREATE TRIGGER trg_cleanup_role_grants_user + AFTER DELETE ON auth.users + FOR EACH ROW + EXECUTE FUNCTION storage.cleanup_role_grants_on_subject_delete('user'); + +DROP TRIGGER IF EXISTS trg_cleanup_role_grants_token ON storage.shares; +CREATE TRIGGER trg_cleanup_role_grants_token + AFTER DELETE ON storage.shares + FOR EACH ROW + EXECUTE FUNCTION storage.cleanup_role_grants_on_subject_delete('token'); + + +-- ── 3. Reverse cascade: last-token-grant delete → cleanup storage.shares ─── +-- A caller hitting DELETE /api/grants/{id} on a token's role grant would +-- otherwise leave the storage.shares row stranded — the token still +-- resolves to "no access" (cascade query finds no rows), but the metadata +-- row accumulates forever. +-- +-- With role_grants the UNIQUE (subject, resource) constraint guarantees a +-- token has at most ONE role grant per resource, so "the last grant for a +-- token" collapses to "the only grant for that token". The NOT EXISTS +-- guard still works correctly — it just always evaluates the same way for +-- token subjects. +-- +-- The DELETE on storage.shares is a no-op when the share row is already +-- gone (the forward cascade `trg_cleanup_role_grants_token` is in flight +-- and already removed it). Idempotent in both directions. + +CREATE OR REPLACE FUNCTION storage.cleanup_share_on_last_role_grant_delete() +RETURNS trigger AS $$ +BEGIN + IF OLD.subject_type = 'token' THEN + DELETE FROM storage.shares s + WHERE s.id = OLD.subject_id + AND NOT EXISTS ( + SELECT 1 FROM storage.role_grants rg + WHERE rg.subject_type = 'token' + AND rg.subject_id = OLD.subject_id + ); + END IF; + RETURN OLD; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS trg_cleanup_share_on_role_grant_delete ON storage.role_grants; +CREATE TRIGGER trg_cleanup_share_on_role_grant_delete + AFTER DELETE ON storage.role_grants + FOR EACH ROW + EXECUTE FUNCTION storage.cleanup_share_on_last_role_grant_delete(); + +COMMENT ON FUNCTION storage.cleanup_share_on_last_role_grant_delete() IS + 'Reverse cascade: deletes storage.shares row when its last token role grant is removed. Pairs with trg_cleanup_role_grants_token (forward direction).'; diff --git a/migrations/20260801000002_drop_access_grants.sql b/migrations/20260801000002_drop_access_grants.sql new file mode 100644 index 00000000..4162b626 --- /dev/null +++ b/migrations/20260801000002_drop_access_grants.sql @@ -0,0 +1,63 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- Cleanup #3: drop storage.access_grants (and everything attached to it) +-- ════════════════════════════════════════════════════════════════════════════ +-- The final step of the role-keyed ReBAC cleanup. By the time this migration +-- runs: +-- +-- * Every read path goes through `storage.role_grants` (cleanup #1 / #2). +-- * The engine no longer has a `grant()` method; `set_role()` / +-- `clear_role()` are the only writes. +-- * The HTTP surface (`POST /api/grants`, `PUT /api/grants/role`) only +-- accepts role-keyed shapes. +-- * `share_service`, `subject_group_service`, `auth_application_service`, +-- `share_pg_repository`, and `integration_test_support` all read +-- `role_grants` exclusively. +-- * `storage.role_grants` has its own cascade triggers +-- (`trg_cleanup_role_grants_*`) and reverse-cascade +-- (`trg_cleanup_share_on_role_grant_delete`), added in cleanup #2. +-- +-- So `access_grants` is fully unreferenced — we can drop it together with +-- the helper triggers + functions defined in +-- `20260520000000_rebac_access_grants.sql` and +-- `20260612000001_share_grant_reverse_cascade.sql`. +-- +-- Roll-back posture: this is destructive. There is no down migration. The +-- D-Prep backfill is one-way (role-keyed rows are derived from +-- permission-keyed clusters; the reverse reconstruction would need a fixed +-- bundle mapping that may have shifted between releases). Recovering +-- requires restoring from a backup taken before this migration runs. + +-- ── 1. Drop the access_grants triggers FROM their source tables ──────────── +-- These triggers live on storage.folders / storage.files / auth.users / +-- storage.shares. Dropping access_grants doesn't implicitly remove them +-- (the trigger row points at the source table; the body references the +-- target table, and that body is what breaks once access_grants is gone). +-- Drop them explicitly so subsequent DELETEs on those source tables don't +-- error out. + +DROP TRIGGER IF EXISTS trg_cleanup_grants_folder ON storage.folders; +DROP TRIGGER IF EXISTS trg_cleanup_grants_file ON storage.files; +DROP TRIGGER IF EXISTS trg_cleanup_grants_user ON auth.users; +DROP TRIGGER IF EXISTS trg_cleanup_grants_token ON storage.shares; + +-- The reverse-cascade trigger is ON access_grants and goes away with the +-- table — but the IF EXISTS makes this safe regardless of drop order. +DROP TRIGGER IF EXISTS trg_cleanup_share_on_grant_delete ON storage.access_grants; + + +-- ── 2. Drop the trigger helper functions ──────────────────────────────────── +-- No other code references these — the `cleanup_role_grants_*` equivalents +-- defined in cleanup #2 carry the same behaviour against role_grants. + +DROP FUNCTION IF EXISTS storage.cleanup_grants_on_resource_delete(); +DROP FUNCTION IF EXISTS storage.cleanup_grants_on_subject_delete(); +DROP FUNCTION IF EXISTS storage.cleanup_share_on_last_token_grant_delete(); + + +-- ── 3. Drop the table ────────────────────────────────────────────────────── +-- CASCADE removes any remaining dependent objects (indexes, comments, and +-- the reverse-cascade trigger if it survived step 1). With every Rust code +-- path already routed through role_grants, nothing in the application +-- layer will notice. + +DROP TABLE IF EXISTS storage.access_grants CASCADE; diff --git a/src/application/dtos/grant_dto.rs b/src/application/dtos/grant_dto.rs index e7c0ec75..62c42bf5 100644 --- a/src/application/dtos/grant_dto.rs +++ b/src/application/dtos/grant_dto.rs @@ -11,7 +11,7 @@ use uuid::Uuid; use crate::application::dtos::cursor::{CursorListResponse, CursorQuery, PageCursor}; use crate::application::dtos::file_dto::FileDto; use crate::application::dtos::folder_dto::FolderDto; -use crate::domain::services::authorization::{Grant, Permission, Resource, Subject}; +use crate::domain::services::authorization::{Grant, Permission, Resource, Role, Subject}; // ════════════════════════════════════════════════════════════════════════════ // Subject / Resource / Permission DTOs @@ -130,166 +130,50 @@ impl From for PermissionDto { // Roles — the load-bearing model for ReBAC grants // ════════════════════════════════════════════════════════════════════════════ // -// Today a role is "DTO-layer sugar" — every grant write expands a role into -// N rows in `storage.access_grants`. The D-Prep refactor (see -// `docs/plan/drive.md` §Prerequisite + migration `20260730000000_role_grants.sql`) -// pushes the role down to storage (`storage.role_grants.role TEXT`); the -// engine reads the role and expands the bundle at query time via this same -// `expand()` function. Adding a role is now schema-free — one variant + one -// match arm here. +// One row per role assignment in `storage.role_grants.role` (a +// `storage.grant_role` ENUM). The engine expands the bundle at query time +// via `Role::expand()` on the domain enum. Adding a role is two edits: +// the variant + match arm on `Role`, and an `ALTER TYPE +// storage.grant_role ADD VALUE 'name'` migration. +/// Wire-format wrapper around the domain `Role` enum. Carries the +/// serde/utoipa derives. Maps 1:1 to/from `Role` via `From`. +/// +/// The historical `"admin"` alias for `Owner` (used during the D-Prep +/// dual-write window for cached clients) has been retired in the cleanup +/// PR — the OxiCloud UI emits `"owner"` exclusively. Stragglers receive +/// a 422 on POST/PUT, which surfaces the upgrade cleanly. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, ToSchema)] #[serde(rename_all = "lowercase")] -pub enum Role { - /// Read access only. The default "anyone can look but not touch". +pub enum RoleDto { Viewer, - /// Read + comment. Useful for review-only stakeholders. Commenter, - /// Read + create. The "drop-zone" role — uploads allowed, existing - /// content untouchable. Common for support-ticket attachments and - /// photo-submission folders. Contributor, - /// Read + create + update + comment. The standard collaboration role. Editor, - /// Full bundle: read, create, update, comment, delete, share — and - /// `Manage` for resource types that support it (drives, groups). This - /// is the highest user-grantable role; renamed from the historical - /// `Admin` to disambiguate from `UserRole::Admin` (the user-account - /// privilege) and to match Drive plan terminology. - /// - /// **Wire-format compat shim** (one release): also deserialises from - /// the legacy `"admin"` string so cached frontend clients keep - /// working until they refresh. Serialisation always emits `"owner"`. - /// Drop the alias in the cleanup PR. - #[serde(alias = "admin")] Owner, } -impl Role { - /// Expand the role into its permission bundle. Single source of truth — - /// any code that needs "does this role include Permission X?" routes - /// through here (or its inverse, `roles_implying`). - /// - /// After D-Prep this is called at engine read time (1 row → bundle - /// expanded server-side). Pre-D-Prep it was called at API write time - /// (1 role → N rows fanned out). - pub fn expand(self) -> &'static [Permission] { - match self { - Role::Viewer => &[Permission::Read], - Role::Commenter => &[Permission::Read, Permission::Comment], - Role::Contributor => &[Permission::Read, Permission::Create], - Role::Editor => &[ - Permission::Read, - Permission::Comment, - Permission::Create, - Permission::Update, - ], - Role::Owner => &[ - Permission::Read, - Permission::Comment, - Permission::Create, - Permission::Update, - Permission::Share, - Permission::Delete, - Permission::Manage, - ], +impl From for Role { + fn from(r: RoleDto) -> Self { + match r { + RoleDto::Viewer => Role::Viewer, + RoleDto::Commenter => Role::Commenter, + RoleDto::Contributor => Role::Contributor, + RoleDto::Editor => Role::Editor, + RoleDto::Owner => Role::Owner, } } - - /// Lowercase string discriminator — matches the SQL `role` column values - /// in `storage.role_grants` and the JSON wire format. - pub fn as_str(self) -> &'static str { - match self { - Role::Viewer => "viewer", - Role::Commenter => "commenter", - Role::Contributor => "contributor", - Role::Editor => "editor", - Role::Owner => "owner", - } - } - - /// Parse a role from its SQL / JSON string discriminator. Returns - /// `None` for unknown values. Accepts the legacy `"admin"` spelling - /// for one release of API compat (clients that cached the old name - /// keep working; new responses always emit `"owner"`). - pub fn parse(s: &str) -> Option { - match s { - "viewer" => Some(Role::Viewer), - "commenter" => Some(Role::Commenter), - "contributor" => Some(Role::Contributor), - "editor" => Some(Role::Editor), - "owner" => Some(Role::Owner), - // Legacy compat: drop after one release once all clients are - // updated. Emits a debug log so we can track stragglers. - "admin" => { - tracing::debug!( - target: "oxicloud::grants", - "Role::parse: accepted legacy 'admin' string as Role::Owner" - ); - Some(Role::Owner) - } - _ => None, - } - } - - /// Every role, in a stable order. Used by `roles_implying` and exposed - /// to the UI so the share modal can render the full picker without - /// hardcoding the list. - /// - /// **UI scope today**: the share dialog renders only `Viewer`, - /// `Editor`, and `Owner` (matches the existing 3-button UX). The - /// `Commenter` and `Contributor` variants are implemented server- - /// side and accepted on the API surface, reserved for future UI - /// exposure when a real use case asks for them. Until then they - /// stay invisible to end users — no picker option, no documentation - /// surface. - /// - /// Any role can be granted on any resource type. Permission bundles - /// that include capabilities the resource type doesn't check for - /// (e.g. `Manage` on a folder, `Create` on a file) simply produce - /// harmless no-ops — no separate validation layer is needed. - pub const ALL: [Role; 5] = [ - Role::Viewer, - Role::Commenter, - Role::Contributor, - Role::Editor, - Role::Owner, - ]; } -/// Inverse of [`Role::expand`]: returns every role whose bundle contains -/// the given permission. Used by the engine to build the SQL -/// `WHERE role IN (...)` filter on hot-path queries like "what drives can -/// this caller read?": -/// -/// ```ignore -/// SELECT resource_id FROM role_grants -/// WHERE subject_id = $1 -/// AND resource_type = 'drive' -/// AND role IN (roles_implying(Permission::Read)); -/// ``` -/// -/// Precomputed in code rather than stored in the DB — `Role` and -/// `Permission` are both small fixed enums, the table can never grow -/// beyond a handful of rows, and keeping it in-code makes "what changes -/// when I add a Permission?" a single grep target. -pub fn roles_implying(permission: Permission) -> &'static [Role] { - use Permission::*; - match permission { - // Every role grants Read — viewer is the floor. - Read => &[ - Role::Viewer, - Role::Commenter, - Role::Contributor, - Role::Editor, - Role::Owner, - ], - Comment => &[Role::Commenter, Role::Editor, Role::Owner], - Create => &[Role::Contributor, Role::Editor, Role::Owner], - Update => &[Role::Editor, Role::Owner], - Delete => &[Role::Owner], - Share => &[Role::Owner], - Manage => &[Role::Owner], +impl From for RoleDto { + fn from(r: Role) -> Self { + match r { + Role::Viewer => RoleDto::Viewer, + Role::Commenter => RoleDto::Commenter, + Role::Contributor => RoleDto::Contributor, + Role::Editor => RoleDto::Editor, + Role::Owner => RoleDto::Owner, + } } } @@ -325,17 +209,18 @@ pub enum SubjectInputDto { }, } -/// `POST /api/grants` — accepts either `permissions` (explicit) or `role`. -/// Server-side validation requires exactly one of the two to be present. +/// `POST /api/grants` — create or refresh a role assignment. +/// +/// Strictly role-keyed since the cleanup PR: callers send exactly one +/// role; the engine writes a single row in `storage.role_grants`. The +/// historical per-permission shape (`permissions: [...]`) was dropped — +/// the OxiCloud UI is the only known caller and it already sends `role`. #[derive(Debug, Deserialize, ToSchema)] pub struct CreateGrantDto { pub subject: SubjectInputDto, pub resource: ResourceDto, - #[serde(default)] - pub permissions: Option>, - #[serde(default)] - pub role: Option, - /// Optional expiry for every grant in this request. RFC 3339 / ISO 8601. + pub role: RoleDto, + /// Optional expiry for the grant. RFC 3339 / ISO 8601. #[serde(default)] pub expires_at: Option>, } @@ -345,7 +230,7 @@ pub struct CreateGrantDto { pub struct UpdateRoleDto { pub subject: SubjectDto, pub resource: ResourceDto, - pub role: Role, + pub role: RoleDto, /// Optional expiry applied to every grant written or updated by this call. #[serde(default)] pub expires_at: Option>, @@ -360,7 +245,10 @@ pub struct GrantDto { pub id: Uuid, pub subject: SubjectDto, pub resource: ResourceDto, - pub permission: PermissionDto, + /// Role-keyed since D-Prep cleanup — one row in `storage.role_grants` + /// is one `GrantDto`. The bundle of underlying permissions is implied + /// by the role and recomputed client-side from the same lookup table. + pub role: RoleDto, pub granted_by: Uuid, pub granted_at: chrono::DateTime, #[serde(skip_serializing_if = "Option::is_none")] @@ -373,7 +261,7 @@ impl From for GrantDto { id: g.id, subject: g.subject.into(), resource: g.resource.into(), - permission: g.permission.into(), + role: g.role.into(), granted_by: g.granted_by, granted_at: g.granted_at, expires_at: g.expires_at, diff --git a/src/application/ports/authorization_ports.rs b/src/application/ports/authorization_ports.rs index b0b9b1c7..494267af 100644 --- a/src/application/ports/authorization_ports.rs +++ b/src/application/ports/authorization_ports.rs @@ -13,7 +13,7 @@ use uuid::Uuid; use crate::common::errors::DomainError; use crate::domain::services::authorization::{ Grant, GrantCursor, IncomingGrantSummary, OutgoingResourceSummary, Permission, Resource, - ResourceKind, Subject, + ResourceKind, Role, Subject, }; pub trait AuthorizationEngine: Send + Sync + 'static { @@ -90,11 +90,7 @@ pub trait AuthorizationEngine: Send + Sync + 'static { /// Resources explicitly granted to `subject`. Direct grants only — no /// cascade expansion. Used by `GET /api/grants/incoming`. - async fn list_incoming_grants( - &self, - subject: Subject, - permission_filter: Option, - ) -> Result, DomainError>; + async fn list_incoming_grants(&self, subject: Subject) -> Result, DomainError>; /// Cursor-paginated list of resources explicitly granted to `subject`, /// optionally filtered by resource kind. Multiple permission rows for the @@ -139,38 +135,20 @@ pub trait AuthorizationEngine: Send + Sync + 'static { reverse: bool, ) -> Result<(Vec, Option), DomainError>; - /// Create a grant. Idempotent — duplicates are absorbed by the UNIQUE - /// constraint; if the row already exists its `expires_at` is updated. - async fn grant( - &self, - granted_by: Uuid, - subject: Subject, - permission: Permission, - resource: Resource, - expires_at: Option>, - ) -> Result; - - /// Update `expires_at` on every grant row for the given subject. - /// Used when a share's expiry is changed — one call updates all - /// permission rows for that token in a single UPDATE. + /// Update `expires_at` for every role grant belonging to `subject`. + /// Used by `share_service` when a token-share's expiry is refreshed — + /// the subject (token) maps to a small fixed set of role grants, so a + /// single UPDATE covers them. Resource-scoped expiry changes go through + /// `set_role` (which carries `expires_at` as part of its UPSERT). async fn set_expiry_for_subject( &self, subject: Subject, expires_at: Option>, ) -> Result<(), DomainError>; - /// Update `expires_at` on every grant row for the given `(subject, resource)` - /// pair. Used by `set_role` to sync the expiry of retained grants when the - /// caller changes expiry without changing permissions. - async fn set_expiry_on_resource( - &self, - subject: Subject, - resource: Resource, - expires_at: Option>, - ) -> Result<(), DomainError>; - - /// Revoke a specific grant by its UUID. Returns `Ok(())` whether or not - /// the row existed (idempotent revoke). + /// Revoke a single role grant by its UUID. Idempotent — returns `Ok(())` + /// whether or not the row existed. The id comes from a prior listing + /// or `find_grant_full_by_id` lookup. async fn revoke(&self, grant_id: Uuid) -> Result<(), DomainError>; /// Removes every grant whose `resource` matches. Called by lifecycle @@ -182,20 +160,10 @@ pub trait AuthorizationEngine: Send + Sync + 'static { /// /group is deleted. Returns the count of rows removed. async fn revoke_all_for_subject(&self, subject: Subject) -> Result; - // ── Role-keyed grant operations (D-Prep dual-write) ──────────────────── - // These manage `storage.role_grants`, the role-keyed table introduced - // by the D-Prep refactor (see `docs/plan/drive.md` §Prerequisite). - // - // During the dual-write window both tables stay populated; the engine - // reads from `access_grants` until the read-path pivot lands. After - // the cleanup PR (which drops `access_grants`), these two methods - // become the ONLY grant write path — the per-permission `grant` / - // `revoke` above are removed at that point. - // - // The handler layer drives these (it knows the Role); lifecycle hook - // bulk-deletes (`revoke_all_for_*` above) wipe role_grants in lockstep - // inside their own implementation, so callers using those paths don't - // need to invoke `clear_role` separately. + // ── Role-keyed grant operations ──────────────────────────────────────── + // These are the only grant write path. Lifecycle hook bulk-deletes + // (`revoke_all_for_*` above) wipe matching rows directly, so callers + // using those paths don't need to invoke `clear_role` separately. /// Set the role for a `(subject, resource)` pair. Idempotent via the /// UNIQUE `(subject_type, subject_id, resource_type, resource_id)` @@ -207,10 +175,10 @@ pub trait AuthorizationEngine: Send + Sync + 'static { &self, granted_by: Uuid, subject: Subject, - role: crate::application::dtos::grant_dto::Role, + role: Role, resource: Resource, expires_at: Option>, - ) -> Result<(), DomainError>; + ) -> Result; /// Remove the role for a `(subject, resource)` pair. Idempotent — /// succeeds whether or not the row existed. Called after `revoke` diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 36584601..a77e7125 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -1388,7 +1388,7 @@ impl AuthApplicationService { /// Visibility rule, evaluated top-to-bottom: /// 1. **Self lookup** — `caller_id == target_id` always succeeds. /// 2. **Shared-grant relationship** — caller and target appear - /// together on at least one row of `storage.access_grants`, + /// together on at least one row of `storage.role_grants`, /// either direction (caller-as-granter / target-as-subject, /// or target-as-granter / caller-as-subject). Applies to both /// internal and external callers. This is what lets an @@ -1454,7 +1454,7 @@ impl AuthApplicationService { let related: Option = sqlx::query_scalar( r#" SELECT 1 - FROM storage.access_grants + FROM storage.role_grants WHERE (granted_by = $1 AND subject_type = 'user' AND subject_id = $2) OR (granted_by = $2 AND subject_type = 'user' AND subject_id = $1) LIMIT 1 diff --git a/src/application/services/share_service.rs b/src/application/services/share_service.rs index 3250c7fe..da411815 100644 --- a/src/application/services/share_service.rs +++ b/src/application/services/share_service.rs @@ -5,7 +5,7 @@ use tokio::sync::Semaphore; use uuid::Uuid; use crate::domain::repositories::folder_repository::FolderRepository; -use crate::domain::services::authorization::{Permission, Resource, Subject}; +use crate::domain::services::authorization::{Resource, Role, Subject}; use crate::infrastructure::repositories::pg::SharePgRepository; use crate::infrastructure::repositories::pg::file_blob_read_repository::FileBlobReadRepository; use crate::infrastructure::repositories::pg::folder_db_repository::FolderDbRepository; @@ -254,9 +254,9 @@ impl ShareUseCase for ShareService { .await .map_err(|e| ShareServiceError::Repository(e.to_string()))?; - // Create one Read-only grant for the token subject, carrying expires_at. - // Tokens are always read-only. The DELETE trigger `trg_cleanup_grants_token` - // cleans up this grant when the share is later deleted. + // Anonymous link tokens always get the Viewer role (read-only). + // The `trg_cleanup_grants_token` trigger cleans up this grant when + // the share row is later deleted. let item_id_uuid = Uuid::parse_str(saved_share.item_id()) .map_err(|_| ShareServiceError::Validation("Invalid item UUID".to_string()))?; let resource = match saved_share.item_type() { @@ -267,10 +267,10 @@ impl ShareUseCase for ShareService { .expires_at .and_then(|ts| chrono::DateTime::from_timestamp(ts as i64, 0)); self.authorization - .grant( + .set_role( user_id, Subject::Token(saved_share.id()), - Permission::Read, + Role::Viewer, resource, expires_dt, ) diff --git a/src/application/services/subject_group_service.rs b/src/application/services/subject_group_service.rs index 89576334..41e0d7c3 100644 --- a/src/application/services/subject_group_service.rs +++ b/src/application/services/subject_group_service.rs @@ -5,7 +5,7 @@ //! - Name validation runs (defence-in-depth alongside the DB CHECK). //! - Virtual groups (e.g. `Internal`) are protected from mutation. //! - Audit events are emitted via `tracing::info!(target = "audit", ...)`. -//! - Cascading delete of `storage.access_grants` rows referencing this +//! - Cascading delete of `storage.role_grants` rows referencing this //! group runs in the same transaction as the group delete. //! //! See `migrations/20260612000000_subject_groups.sql` for the schema. @@ -172,9 +172,9 @@ impl SubjectGroupService { /// Delete the group; cascades to: /// - `auth.subject_group_members` rows (FK CASCADE). - /// - `storage.access_grants` rows where `subject_type='group'` and + /// - `storage.role_grants` rows where `subject_type='group'` and /// `subject_id = id` (handled here, no FK exists between - /// `access_grants` and `subject_groups`). + /// `role_grants` and `subject_groups`). pub async fn delete(&self, id: Uuid, caller_id: Uuid) -> Result<(), DomainError> { let existing = self.get_by_id(id).await?; if existing.is_virtual { @@ -196,7 +196,7 @@ impl SubjectGroupService { })?; let grants_deleted = sqlx::query( - "DELETE FROM storage.access_grants + "DELETE FROM storage.role_grants WHERE subject_type = 'group' AND subject_id = $1", ) .bind(id) @@ -519,9 +519,9 @@ mod integration_tests { // ── 13. Grants are revoked atomically when a group is deleted ────────── // - // The plan said "FK CASCADE", but there's no FK between `access_grants` - // and `subject_groups` (different schemas; the cascade is handled by the - // service's transactional DELETE). This test pins that behaviour. + // There is no FK between `storage.role_grants` and `auth.subject_groups` + // (different schemas); the cascade is handled by the service's + // transactional DELETE. This test pins that behaviour. #[tokio::test] async fn test_grants_revoked_when_group_deleted() { let svc = make_service().await; @@ -534,21 +534,21 @@ mod integration_tests { .unwrap(); let resource_id = Uuid::new_v4(); sqlx::query( - "INSERT INTO storage.access_grants \ + "INSERT INTO storage.role_grants \ (subject_type, subject_id, resource_type, resource_id, \ - permission, granted_by) \ - VALUES ('group', $1, 'folder', $2, 'read', $3)", + role, granted_by) \ + VALUES ('group', $1, 'folder', $2, 'viewer', $3)", ) .bind(group.id) .bind(resource_id) .bind(admin) .execute(svc.pool.as_ref()) .await - .expect("insert grant row"); + .expect("insert role_grants row"); // Sanity: the grant exists. let pre: i64 = sqlx::query_scalar( - "SELECT COUNT(*) FROM storage.access_grants \ + "SELECT COUNT(*) FROM storage.role_grants \ WHERE subject_type = 'group' AND subject_id = $1", ) .bind(group.id) @@ -561,7 +561,7 @@ mod integration_tests { svc.delete(group.id, admin).await.unwrap(); let post: i64 = sqlx::query_scalar( - "SELECT COUNT(*) FROM storage.access_grants \ + "SELECT COUNT(*) FROM storage.role_grants \ WHERE subject_type = 'group' AND subject_id = $1", ) .bind(group.id) diff --git a/src/domain/entities/share.rs b/src/domain/entities/share.rs index 24a09f5c..93d965e4 100644 --- a/src/domain/entities/share.rs +++ b/src/domain/entities/share.rs @@ -12,7 +12,7 @@ pub struct Share { item_type: ShareItemType, token: String, password_hash: Option, - /// Derived from `storage.access_grants.expires_at` — not stored on the share row. + /// Derived from `storage.role_grants.expires_at` — not stored on the share row. expires_at: Option, created_at: u64, created_by: Uuid, diff --git a/src/domain/entities/subject_group.rs b/src/domain/entities/subject_group.rs index 2ada7cf7..8845eb5b 100644 --- a/src/domain/entities/subject_group.rs +++ b/src/domain/entities/subject_group.rs @@ -2,7 +2,7 @@ //! //! Subject groups are root-owned (no `owner_id`), globally named with an //! RFC 5321 local-part shape, and able to contain users *or* other groups. -//! Grants in `storage.access_grants` with `subject_type = 'group'` reference +//! Grants in `storage.role_grants` with `subject_type = 'group'` reference //! a row in `auth.subject_groups`. //! //! Cycle prevention and depth-cap (`MAX_GROUP_DEPTH`) are enforced at the diff --git a/src/domain/repositories/subject_group_repository.rs b/src/domain/repositories/subject_group_repository.rs index 96212b07..cc799954 100644 --- a/src/domain/repositories/subject_group_repository.rs +++ b/src/domain/repositories/subject_group_repository.rs @@ -93,8 +93,8 @@ pub trait SubjectGroupRepository: Send + Sync + 'static { ) -> Result; /// Delete the group. Cascades to `subject_group_members` and to - /// `storage.access_grants` rows referencing this group as subject (via - /// the application service — there is no FK between `access_grants` and + /// `storage.role_grants` rows referencing this group as subject (via + /// the application service — there is no FK between `role_grants` and /// `subject_groups`, so the service performs the cascade explicitly in /// the same transaction). async fn delete(&self, id: Uuid) -> Result<(), SubjectGroupRepositoryError>; diff --git a/src/domain/services/authorization.rs b/src/domain/services/authorization.rs index 7e8b5503..06cf0607 100644 --- a/src/domain/services/authorization.rs +++ b/src/domain/services/authorization.rs @@ -3,7 +3,7 @@ //! These types are storage-agnostic — they describe the relationship between //! a subject (who), a resource (what), and a permission (action). The //! `AuthorizationEngine` port consumes them and the `PgAclEngine` implementation -//! maps them to / from `storage.access_grants` rows. +//! maps them to / from `storage.role_grants` rows. use crate::application::dtos::cursor::PageCursor; use std::fmt; @@ -49,7 +49,7 @@ impl Subject { /// `"external"` is no longer accepted: PR-2 of the external-users /// work folded the federated-identity case into `Subject::User(uuid)` /// with `auth.users.is_external = TRUE`. The DB CHECK constraint - /// on `storage.access_grants.subject_type` was narrowed to match. + /// on `storage.role_grants.subject_type` was narrowed to match. pub fn from_parts(subject_type: &str, id: Uuid) -> Option { match subject_type { "user" => Some(Subject::User(id)), @@ -200,7 +200,7 @@ impl fmt::Display for Permission { } // ════════════════════════════════════════════════════════════════════════════ -// Grant — a row in storage.access_grants +// Grant — a row in storage.role_grants // ════════════════════════════════════════════════════════════════════════════ #[derive(Clone, Debug)] @@ -208,12 +208,132 @@ pub struct Grant { pub id: Uuid, pub subject: Subject, pub resource: Resource, - pub permission: Permission, + /// Role-keyed since D-Prep cleanup: one `Grant` represents the role + /// row in `storage.role_grants` rather than a single permission. The + /// engine and HTTP surface no longer carry per-permission rows; + /// callers that need permissions use `role.expand()`. + pub role: Role, pub granted_by: Uuid, pub granted_at: chrono::DateTime, pub expires_at: Option>, } +// ════════════════════════════════════════════════════════════════════════════ +// Role — a named bundle of permissions +// ════════════════════════════════════════════════════════════════════════════ +// +// Roles are the load-bearing model for ReBAC grants since D-Prep. Each +// `storage.role_grants` row stores one role; the engine expands the bundle +// at read time via `Role::expand()`. Adding a role is two edits: +// 1. a variant here + match arm in `expand()` / `as_str()` / `parse()` +// 2. an `ALTER TYPE storage.grant_role ADD VALUE 'name'` migration +// +// `RoleDto` (DTO layer) carries the wire-format derives + the legacy +// `"admin"` alias for backwards compat. + +#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] +pub enum Role { + Viewer, + Commenter, + Contributor, + Editor, + Owner, +} + +impl Role { + /// Expand the role into its permission bundle. Single source of truth — + /// any code that needs "does this role include Permission X?" routes + /// through here (or its inverse, `roles_implying`). + pub fn expand(self) -> &'static [Permission] { + match self { + Role::Viewer => &[Permission::Read], + Role::Commenter => &[Permission::Read, Permission::Comment], + Role::Contributor => &[Permission::Read, Permission::Create], + Role::Editor => &[ + Permission::Read, + Permission::Comment, + Permission::Create, + Permission::Update, + ], + Role::Owner => &[ + Permission::Read, + Permission::Comment, + Permission::Create, + Permission::Update, + Permission::Share, + Permission::Delete, + Permission::Manage, + ], + } + } + + /// Lowercase discriminator — matches the SQL `role` ENUM values in + /// `storage.role_grants` (after the `::text` cast). + pub fn as_str(self) -> &'static str { + match self { + Role::Viewer => "viewer", + Role::Commenter => "commenter", + Role::Contributor => "contributor", + Role::Editor => "editor", + Role::Owner => "owner", + } + } + + /// Parse a role from its SQL discriminator. Returns `None` for unknown + /// values. The `"admin"` legacy alias is handled by `RoleDto` at the + /// wire boundary — the database only ever stores the canonical names. + pub fn parse(s: &str) -> Option { + match s { + "viewer" => Some(Role::Viewer), + "commenter" => Some(Role::Commenter), + "contributor" => Some(Role::Contributor), + "editor" => Some(Role::Editor), + "owner" => Some(Role::Owner), + _ => None, + } + } + + /// Every role, in declaration order. Mirrors the `storage.grant_role` + /// ENUM order in PG, which is weakest-to-strongest as written here for + /// historical reasons (`storage.grant_role` declares strongest first). + pub const ALL: [Role; 5] = [ + Role::Viewer, + Role::Commenter, + Role::Contributor, + Role::Editor, + Role::Owner, + ]; +} + +impl fmt::Display for Role { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.as_str()) + } +} + +/// Inverse of [`Role::expand`]: returns every role whose bundle contains +/// the given permission. Used by the engine to build the SQL +/// `WHERE role IN (...)` filter on hot-path queries like "what drives can +/// this caller read?". +pub fn roles_implying(permission: Permission) -> &'static [Role] { + use Permission::*; + match permission { + Read => &[ + Role::Viewer, + Role::Commenter, + Role::Contributor, + Role::Editor, + Role::Owner, + ], + Comment => &[Role::Commenter, Role::Editor, Role::Owner], + Create => &[Role::Contributor, Role::Editor, Role::Owner], + Update => &[Role::Editor, Role::Owner], + Delete => &[Role::Owner], + Share => &[Role::Owner], + Manage => &[Role::Owner], + } +} + impl Grant { pub fn is_expired(&self) -> bool { self.expires_at.is_some_and(|exp| exp < chrono::Utc::now()) @@ -225,7 +345,7 @@ impl Grant { // ════════════════════════════════════════════════════════════════════════════ /// Resource type without an id — used to filter paginated grant queries by -/// type. Mirrors the `resource_type` column values in `storage.access_grants`. +/// type. Mirrors the `resource_type` column values in `storage.role_grants`. /// Add new variants here when new resource types are supported. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum ResourceKind { diff --git a/src/infrastructure/repositories/pg/share_pg_repository.rs b/src/infrastructure/repositories/pg/share_pg_repository.rs index d5c30183..8660324e 100644 --- a/src/infrastructure/repositories/pg/share_pg_repository.rs +++ b/src/infrastructure/repositories/pg/share_pg_repository.rs @@ -38,7 +38,7 @@ impl SharePgRepository { /// Maps a [`sqlx::postgres::PgRow`] to the domain [`Share`] entity. /// Expects columns: id, item_id, item_name, item_type, token, password_hash, - /// expires_at (derived from access_grants subquery), created_at, created_by, access_count. + /// expires_at (derived from role_grants subquery), created_at, created_by, access_count. fn row_to_entity(row: &sqlx::postgres::PgRow) -> Result { let id: Uuid = row .try_get("id") @@ -54,7 +54,7 @@ impl SharePgRepository { DomainError::internal_error("Share", format!("Failed to read token: {e}")) })?; let password_hash: Option = row.try_get("password_hash").unwrap_or(None); - // expires_at derived from access_grants subquery (unix seconds as i64) + // expires_at derived from role_grants subquery (unix seconds as i64) let expires_at: Option = row.try_get("expires_at").unwrap_or(None); let created_at: i64 = row.try_get("created_at").map_err(|e| { DomainError::internal_error("Share", format!("Failed to read created_at: {e}")) @@ -98,7 +98,7 @@ impl ShareStoragePort for SharePgRepository { RETURNING id, item_id, item_name, item_type, token, password_hash, (SELECT MIN(EXTRACT(EPOCH FROM ag.expires_at)::BIGINT) - FROM storage.access_grants ag + FROM storage.role_grants ag WHERE ag.subject_type = 'token' AND ag.subject_id = id) AS expires_at, created_at, created_by, access_count "#, @@ -127,7 +127,7 @@ impl ShareStoragePort for SharePgRepository { r#" SELECT s.id, s.item_id, s.item_name, s.item_type, s.token, s.password_hash, (SELECT MIN(EXTRACT(EPOCH FROM ag.expires_at)::BIGINT) - FROM storage.access_grants ag + FROM storage.role_grants ag WHERE ag.subject_type = 'token' AND ag.subject_id = s.id) AS expires_at, s.created_at, s.created_by, s.access_count FROM storage.shares s @@ -160,7 +160,7 @@ impl ShareStoragePort for SharePgRepository { r#" SELECT s.id, s.item_id, s.item_name, s.item_type, s.token, s.password_hash, (SELECT MIN(EXTRACT(EPOCH FROM ag.expires_at)::BIGINT) - FROM storage.access_grants ag + FROM storage.role_grants ag WHERE ag.subject_type = 'token' AND ag.subject_id = s.id) AS expires_at, s.created_at, s.created_by, s.access_count FROM storage.shares s @@ -218,7 +218,7 @@ impl ShareStoragePort for SharePgRepository { r#" SELECT s.id, s.item_id, s.item_name, s.item_type, s.token, s.password_hash, (SELECT MIN(EXTRACT(EPOCH FROM ag.expires_at)::BIGINT) - FROM storage.access_grants ag + FROM storage.role_grants ag WHERE ag.subject_type = 'token' AND ag.subject_id = s.id) AS expires_at, s.created_at, s.created_by, s.access_count FROM storage.shares s @@ -250,7 +250,7 @@ impl ShareStoragePort for SharePgRepository { RETURNING id, item_id, item_name, item_type, token, password_hash, (SELECT MIN(EXTRACT(EPOCH FROM ag.expires_at)::BIGINT) - FROM storage.access_grants ag + FROM storage.role_grants ag WHERE ag.subject_type = 'token' AND ag.subject_id = storage.shares.id) AS expires_at, created_at, created_by, access_count "#, @@ -286,7 +286,7 @@ impl ShareStoragePort for SharePgRepository { r#" SELECT s.id, s.item_id, s.item_name, s.item_type, s.token, s.password_hash, (SELECT MIN(EXTRACT(EPOCH FROM ag.expires_at)::BIGINT) - FROM storage.access_grants ag + FROM storage.role_grants ag WHERE ag.subject_type = 'token' AND ag.subject_id = s.id) AS expires_at, s.created_at, s.created_by, s.access_count, COUNT(*) OVER() AS total_count diff --git a/src/infrastructure/repositories/pg/subject_group_pg_repository.rs b/src/infrastructure/repositories/pg/subject_group_pg_repository.rs index 7484a155..6f3d0f60 100644 --- a/src/infrastructure/repositories/pg/subject_group_pg_repository.rs +++ b/src/infrastructure/repositories/pg/subject_group_pg_repository.rs @@ -272,8 +272,8 @@ impl SubjectGroupRepository for SubjectGroupPgRepository { async fn delete(&self, id: Uuid) -> Result<(), SubjectGroupRepositoryError> { // The application service is responsible for clearing related - // `storage.access_grants` rows in the same transaction (there's no - // FK between access_grants and subject_groups). The subject_group_members + // `storage.role_grants` rows in the same transaction (there's no + // FK between role_grants and subject_groups). The subject_group_members // rows cascade automatically via FK. let result = sqlx::query("DELETE FROM auth.subject_groups WHERE id = $1") .bind(id) diff --git a/src/infrastructure/services/pg_acl_engine.rs b/src/infrastructure/services/pg_acl_engine.rs index 1d46db3b..652d54e3 100644 --- a/src/infrastructure/services/pg_acl_engine.rs +++ b/src/infrastructure/services/pg_acl_engine.rs @@ -1,13 +1,14 @@ //! PostgreSQL-backed implementation of `AuthorizationEngine`. //! -//! Stores grants in `storage.access_grants` (see migration -//! `20260520000000_rebac_access_grants.sql`). Cascading is resolved at check -//! time via PostgreSQL `ltree` `@>` (ancestor-of) on `storage.folders.lpath`, -//! using the existing GiST index for O(log N) traversal. +//! Stores grants in `storage.role_grants` (one role per (subject, resource) +//! pair; the role's permission bundle is expanded in code via +//! `Role::expand()`). Cascading is resolved at check time via PostgreSQL +//! `ltree` `@>` (ancestor-of) on `storage.folders.lpath`, using the +//! existing GiST index for O(log N) traversal. //! //! Owner is implicit — `storage.folders.user_id` / `storage.files.user_id` //! are checked first via dedicated helpers; if the caller is the owner, no -//! SQL against `access_grants` happens. +//! SQL against `role_grants` happens. //! //! ## Lifecycle cleanup //! @@ -37,14 +38,13 @@ use uuid::Uuid; use moka::future::Cache; use sqlx::PgPool; -use crate::application::dtos::grant_dto::roles_implying; use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::common::errors::DomainError; use crate::domain::entities::subject_group::INTERNAL_GROUP_ID; use crate::domain::repositories::subject_group_repository::SubjectGroupRepository; use crate::domain::services::authorization::{ Grant, GrantCursor, IncomingGrantSummary, OutgoingGrantEntry, OutgoingResourceSummary, - Permission, Resource, ResourceKind, Subject, + Permission, Resource, ResourceKind, Role, Subject, roles_implying, }; use crate::infrastructure::repositories::pg::SubjectGroupPgRepository; use crate::infrastructure::repositories::pg::file_blob_read_repository::FileBlobReadRepository; @@ -206,7 +206,7 @@ impl PgAclEngine { } /// Expand a caller's `Subject` into the `(subject_types, subject_ids)` - /// pair that should be matched in `storage.access_grants`. For User + /// pair that should be matched in `storage.role_grants`. For User /// callers this is `(["user","group"], [uid, …transitive groups, INTERNAL])`; /// for any non-user subject (Token / External / Group as direct caller) /// it's a single-element pair with no cascade. @@ -239,8 +239,8 @@ impl PgAclEngine { } /// Convert a `Permission` into the array of role strings whose bundle - /// includes it — used to bind the `g.role = ANY($N::text[])` filter on - /// every cascade / lookup query that reads `storage.role_grants`. + /// includes it — bound as `ANY($N::storage.grant_role[])` so the + /// ENUM-typed `role` column compares without an implicit text cast. /// /// This is the inverse of `Role::expand()`, precomputed via /// `grant_dto::roles_implying()`. The mapping is small and static (≤5 @@ -264,11 +264,10 @@ impl PgAclEngine { /// `subject_ids` is the expanded set returned by `expand_user` (or a /// single-element vec for non-user callers). /// - /// **D-Prep**: now reads `storage.role_grants` (1 row per role - /// assignment) instead of `storage.access_grants` (N rows per role). - /// The permission filter `g.permission = $3` becomes - /// `g.role = ANY($3::text[])` where the array is the set of roles whose - /// bundle includes the requested permission — see `roles_implying()`. + /// Reads `storage.role_grants` (1 row per role assignment); a permission + /// filter `g.permission = $3` becomes `g.role = ANY($3::storage.grant_role[])` where + /// the array is the set of roles whose bundle includes the requested + /// permission — see `roles_implying()`. /// /// Uses the GiST index on `storage.folders.lpath` for O(log N) cascade. async fn folder_cascade_grant_exists( @@ -288,7 +287,7 @@ impl PgAclEngine { JOIN storage.folders gf ON gf.id = g.resource_id WHERE g.subject_type = ANY($1) AND g.subject_id = ANY($2) - AND g.role = ANY($3::text[]) + AND g.role = ANY($3::storage.grant_role[]) AND g.resource_type = 'folder' AND (g.expires_at IS NULL OR g.expires_at > NOW()) AND gf.lpath @> (SELECT lpath FROM storage.folders WHERE id = $4) @@ -329,7 +328,7 @@ impl PgAclEngine { FROM storage.role_grants WHERE subject_type = ANY($1) AND subject_id = ANY($2) - AND role = ANY($3::text[]) + AND role = ANY($3::storage.grant_role[]) AND resource_type = 'file' AND resource_id = $4 AND (expires_at IS NULL OR expires_at > NOW()) UNION ALL @@ -340,7 +339,7 @@ impl PgAclEngine { JOIN storage.files target_f ON target_f.id = $4 WHERE g.subject_type = ANY($1) AND g.subject_id = ANY($2) - AND g.role = ANY($3::text[]) + AND g.role = ANY($3::storage.grant_role[]) AND g.resource_type = 'folder' AND (g.expires_at IS NULL OR g.expires_at > NOW()) AND target_f.folder_id IS NOT NULL @@ -361,39 +360,16 @@ impl PgAclEngine { Ok(exists.is_some()) } - /// Look up a single grant by id. Returns `(resource, granted_by)` so - /// the REST `DELETE /api/grants/{id}` handler can decide authorization - /// without a second round-trip. Returns `Ok(None)` if no such grant. - pub async fn find_grant_by_id( - &self, - grant_id: Uuid, - ) -> Result, DomainError> { - let row: Option<(String, Uuid, Uuid)> = sqlx::query_as( - "SELECT resource_type, resource_id, granted_by FROM storage.access_grants WHERE id = $1", - ) - .bind(grant_id) - .fetch_optional(self.pool.as_ref()) - .await - .map_err(|e| DomainError::internal_error("PgAcl", format!("find_grant_by_id: {e}")))?; - - let Some((rt, rid, granter)) = row else { - return Ok(None); - }; - let res = Resource::from_parts(&rt, rid) - .ok_or_else(|| DomainError::internal_error("PgAcl", "unknown resource_type"))?; - Ok(Some((res, granter))) - } - - /// Variant of `find_grant_by_id` that also returns the subject — - /// needed by `POST /api/grants/{id}/notify` to resolve who to email. - /// Returns `(subject, resource, granted_by)` or `None`. + /// Look up a single role grant by id, returning the actors a revoke / + /// notify handler needs to make a decision without a second round-trip. + /// Returns `(subject, resource, granted_by)` or `None` if no such row. pub async fn find_grant_full_by_id( &self, grant_id: Uuid, ) -> Result, DomainError> { let row: Option<(String, Uuid, String, Uuid, Uuid)> = sqlx::query_as( "SELECT subject_type, subject_id, resource_type, resource_id, granted_by \ - FROM storage.access_grants WHERE id = $1", + FROM storage.role_grants WHERE id = $1", ) .bind(grant_id) .fetch_optional(self.pool.as_ref()) @@ -410,8 +386,14 @@ impl PgAclEngine { Ok(Some((subject, resource, granter))) } - /// Row type for all full-grant SELECT queries: - /// (id, subject_type, subject_id, resource_type, resource_id, permission, granted_by, granted_at, expires_at) + /// Row type for `storage.role_grants` SELECTs: + /// (id, subject_type, subject_id, resource_type, resource_id, role, granted_by, granted_at, expires_at). + /// + /// Builds a single role-keyed `Grant` per row. `Grant` is role-keyed + /// since the D-Prep cleanup PR — every listing method returns role + /// rows directly; bundle expansion to per-permission Grants no longer + /// happens here. Callers that need the permission set use + /// `grant.role.expand()` at the call site. #[allow(clippy::type_complexity)] fn row_to_grant( row: ( @@ -430,13 +412,13 @@ impl PgAclEngine { .ok_or_else(|| DomainError::internal_error("PgAcl", "unknown subject_type"))?; let resource = Resource::from_parts(&row.3, row.4) .ok_or_else(|| DomainError::internal_error("PgAcl", "unknown resource_type"))?; - let permission = Permission::parse(&row.5) - .ok_or_else(|| DomainError::internal_error("PgAcl", "unknown permission"))?; + let role = Role::parse(&row.5) + .ok_or_else(|| DomainError::internal_error("PgAcl", "unknown role"))?; Ok(Grant { id: row.0, subject, resource, - permission, + role, granted_by: row.6, granted_at: row.7, expires_at: row.8, @@ -554,15 +536,14 @@ impl AuthorizationEngine for PgAclEngine { result } - async fn list_incoming_grants( - &self, - subject: Subject, - permission_filter: Option, - ) -> Result, DomainError> { - let perm_str = permission_filter.map(|p| p.as_str().to_string()); + async fn list_incoming_grants(&self, subject: Subject) -> Result, DomainError> { let counters = QueryCounters::default(); let (subject_types, subject_ids) = self.subject_match_set(subject, &counters).await?; + // `ORDER BY role ASC` exploits the `storage.grant_role` ENUM + // declared as `(owner, editor, contributor, commenter, viewer)`, + // so the sort order matches the UX requirement ("Owner > Editor + // > Contributor > Commenter > Viewer") without a per-row CASE. let rows = sqlx::query_as::< _, ( @@ -579,18 +560,16 @@ impl AuthorizationEngine for PgAclEngine { >( r#" SELECT id, subject_type, subject_id, resource_type, resource_id, - permission, granted_by, granted_at, expires_at - FROM storage.access_grants + role::text, granted_by, granted_at, expires_at + FROM storage.role_grants WHERE subject_type = ANY($1) AND subject_id = ANY($2) - AND ($3::text IS NULL OR permission = $3) - ORDER BY granted_at DESC - LIMIT $4 + ORDER BY role ASC, granted_at DESC + LIMIT $3 "#, ) .bind(&subject_types) .bind(&subject_ids) - .bind(perm_str) .bind(MAX_GRANT_ROWS + 1) .fetch_all(self.pool.as_ref()) .await @@ -621,7 +600,12 @@ impl AuthorizationEngine for PgAclEngine { // NULL otherwise. This lets every sort mode share a single query_as call. // 0 resource_type String // 1 resource_id Uuid - // 2 permissions Vec + // 2 roles Vec — every distinct role granting access to this + // resource (post-D-Prep). Expanded to permissions + // in `IncomingGrantSummary` via `Role::expand()`. + // Multiple entries possible when a user has both + // a direct grant and a group-mediated grant on + // the same resource. // 3 granted_at DateTime // 4 granted_by Uuid // 5 sort_str Option — resource_name (name/type) or owner_name (granted_by) @@ -653,14 +637,20 @@ impl AuthorizationEngine for PgAclEngine { // is `(["user","group"], [uid, …transitive groups, INTERNAL])` so the // listing includes every resource the user can reach via a group // grant (matching what `check()` allows). See `subject_match_set`. + // + // Post-D-Prep this reads `storage.role_grants` and aggregates the + // ENUM-typed `role` column into a text array. Multiple roles can + // appear per resource when the caller reaches it via both a direct + // grant and a group-mediated grant — the union of role bundles + // produces the displayed permission set in Rust below. const AGG: &str = r#"agg AS ( SELECT resource_type, resource_id, - array_agg(DISTINCT permission ORDER BY permission) AS permissions, + array_agg(DISTINCT role::text ORDER BY role::text) AS roles, MIN(granted_at) AS granted_at, (array_agg(granted_by ORDER BY granted_at))[1] AS granted_by - FROM storage.access_grants + FROM storage.role_grants WHERE subject_type = ANY($1) AND subject_id = ANY($2) AND ($3::text[] IS NULL OR resource_type = ANY($3)) @@ -725,7 +715,7 @@ impl AuthorizationEngine for PgAclEngine { LEFT JOIN storage.folders f ON f.id = agg.resource_id AND agg.resource_type = 'folder' LEFT JOIN storage.files fi ON fi.id = agg.resource_id AND agg.resource_type = 'file' ) - SELECT resource_type, resource_id, permissions, granted_at, granted_by, sort_str, sort_int + SELECT resource_type, resource_id, roles, granted_at, granted_by, sort_str, sort_int FROM named WHERE {where_clause} ORDER BY {order_clause} @@ -765,7 +755,7 @@ impl AuthorizationEngine for PgAclEngine { FROM agg LEFT JOIN auth.users u ON u.id = agg.granted_by ) - SELECT resource_type, resource_id, permissions, granted_at, granted_by, sort_str, sort_int + SELECT resource_type, resource_id, roles, granted_at, granted_by, sort_str, sort_int FROM owner_named WHERE {where_clause} ORDER BY {order_clause} @@ -793,7 +783,7 @@ impl AuthorizationEngine for PgAclEngine { }; format!( r#"WITH {AGG} - SELECT resource_type, resource_id, permissions, granted_at, granted_by, + SELECT resource_type, resource_id, roles, granted_at, granted_by, NULL::text AS sort_str, NULL::bigint AS sort_int FROM agg @@ -875,14 +865,21 @@ impl AuthorizationEngine for PgAclEngine { }; // ── Convert rows to domain summaries ────────────────────────────────── + // Post-D-Prep: the SQL aggregate produces a `roles` text array. We + // expand each role's bundle and union them — direct grants and + // group-mediated grants on the same resource collapse to a single + // deduplicated permission set, matching the pre-pivot behaviour. let summaries = rows .into_iter() - .filter_map(|(rt, rid, perms_str, granted_at, granted_by, _, _)| { + .filter_map(|(rt, rid, roles_str, granted_at, granted_by, _, _)| { let resource_type = ResourceKind::parse(&rt)?; - let permissions = perms_str + let mut permissions: Vec = roles_str .into_iter() - .filter_map(|s| Permission::parse(&s)) + .filter_map(|s| Role::parse(&s)) + .flat_map(|r| r.expand().iter().copied()) .collect(); + permissions.sort_by_key(|p| p.as_str()); + permissions.dedup(); Some(IncomingGrantSummary { resource_type, resource_id: rid, @@ -897,6 +894,14 @@ impl AuthorizationEngine for PgAclEngine { } async fn list_grants_on_resource(&self, resource: Resource) -> Result, DomainError> { + // Pivoted to `storage.role_grants` (see `list_incoming_grants`). + // Each role row expands to N permission-keyed `Grant` rows via + // `role_row_to_grants` until the public `Grant` shape becomes + // role-keyed. + // + // `ORDER BY role ASC` exploits the `storage.grant_role` ENUM's + // declaration order (owner first → viewer last) so the share + // dialog's "who has access" list shows strongest grants on top. let rows = sqlx::query_as::< _, ( @@ -913,11 +918,11 @@ impl AuthorizationEngine for PgAclEngine { >( r#" SELECT id, subject_type, subject_id, resource_type, resource_id, - permission, granted_by, granted_at, expires_at - FROM storage.access_grants + role::text, granted_by, granted_at, expires_at + FROM storage.role_grants WHERE resource_type = $1 AND resource_id = $2 - ORDER BY granted_at DESC + ORDER BY role ASC, granted_at DESC LIMIT $3 "#, ) @@ -942,7 +947,10 @@ impl AuthorizationEngine for PgAclEngine { ) -> Result<(Vec, Option), DomainError> { let fetch_limit = (limit as i64) + 1; - // Row shape — one row per (resource, subject, permission). + // Row shape — post-D-Prep, one row per (resource, subject) since + // `storage.role_grants` carries exactly one role per pair (UNIQUE + // constraint). Permission bundles are expanded in the row consumer + // via `Role::expand()`. // Columns: // 0 resource_type String // 1 resource_id Uuid @@ -951,9 +959,9 @@ impl AuthorizationEngine for PgAclEngine { // 4 subject_id Uuid // 5 subject_display String — username or share item_name // 6 grant_id Uuid - // 7 granted_at DateTime — this (subject, perm) row + // 7 granted_at DateTime — this (subject, role) row // 8 expires_at Option> - // 9 permission String + // 9 role String — `grant_role` ENUM as text // 10 sort_str Option // 11 sort_int Option // 12 has_password bool — token: shares.password_hash IS NOT NULL @@ -1040,7 +1048,7 @@ impl AuthorizationEngine for PgAclEngine { CASE WHEN ag.resource_type = 'file' THEN fi.name END ) AS sort_str, {sort_int_expr} AS sort_int - FROM storage.access_grants ag + FROM storage.role_grants ag LEFT JOIN storage.folders f ON f.id = ag.resource_id AND ag.resource_type = 'folder' LEFT JOIN storage.files fi ON fi.id = ag.resource_id AND ag.resource_type = 'file' WHERE ag.granted_by = $1 @@ -1055,12 +1063,12 @@ impl AuthorizationEngine for PgAclEngine { SELECT ag.resource_type, ag.resource_id, rp.first_shared_at, ag.subject_type, ag.subject_id, COALESCE(u.username, u.email, sg.name::text, sh.item_name, fi.name, fld.name, ag.subject_id::text) AS subject_display, - ag.id AS grant_id, ag.granted_at, ag.expires_at, ag.permission, + ag.id AS grant_id, ag.granted_at, ag.expires_at, ag.role::text AS role, rp.sort_str, rp.sort_int, (sh.password_hash IS NOT NULL) AS has_password, COALESCE(u.is_external, FALSE) AS is_external FROM rp - JOIN storage.access_grants ag + JOIN storage.role_grants ag ON ag.resource_type = rp.resource_type AND ag.resource_id = rp.resource_id AND ag.granted_by = $1 LEFT JOIN auth.users u ON ag.subject_type = 'user' AND u.id = ag.subject_id @@ -1129,7 +1137,7 @@ impl AuthorizationEngine for PgAclEngine { ELSE 3 END)::bigint AS sort_int, MIN(ag.granted_at) AS first_granted_at - FROM storage.access_grants ag + FROM storage.role_grants ag LEFT JOIN auth.users u ON ag.subject_type = 'user' AND u.id = ag.subject_id LEFT JOIN auth.subject_groups sg @@ -1160,13 +1168,13 @@ impl AuthorizationEngine for PgAclEngine { ag.id AS grant_id, ag.granted_at, ag.expires_at, - ag.permission, + ag.role::text AS role, LOWER(rp.subject_display) AS sort_str, rp.sort_int, rp.has_password, rp.is_external FROM rp - JOIN storage.access_grants ag + JOIN storage.role_grants ag ON ag.resource_type = rp.resource_type AND ag.resource_id = rp.resource_id AND ag.subject_type = rp.subject_type @@ -1180,7 +1188,12 @@ impl AuthorizationEngine for PgAclEngine { // Page on (role_order, subject_display, resource_id) triples so that all // of one person's grants within a role are contiguous — enabling aggregation // ("Bob on Folder A, Folder B") to work correctly across cursor pages. - // role_order: 0 = admin (has delete+share), 1 = editor (has create or update), 2 = viewer + // + // role_order matches the `storage.grant_role` ENUM declaration + // order (strongest first) via `array_position`, so + // `sort_int ASC` matches the UX requirement: 1 = owner, + // 2 = editor, 3 = contributor, 4 = commenter, 5 = viewer. + // 1-based because `array_position` is. // Cursor: sort_int=role_order, resource_name=LOWER(subject_display), resource_id let (page_where, page_order) = if reverse { ( @@ -1209,15 +1222,20 @@ impl AuthorizationEngine for PgAclEngine { MAX(COALESCE(u.username, u.email, sh.item_name, ag.subject_id::text)) AS subject_display, BOOL_OR(sh.password_hash IS NOT NULL) AS has_password, COALESCE(BOOL_OR(u.is_external), FALSE) AS is_external, - CASE - WHEN BOOL_OR(ag.permission = 'delete') - AND BOOL_OR(ag.permission = 'share') THEN 0 - WHEN BOOL_OR(ag.permission = 'create') - OR BOOL_OR(ag.permission = 'update') THEN 1 - ELSE 2 - END::bigint AS sort_int, + -- One role per (resource, subject) post-D-Prep + -- (UNIQUE constraint on role_grants), so MAX + -- returns that single row's role. `array_position` + -- against the ENUM's declaration order produces a + -- 1-based rank: owner=1 → viewer=5. Strength + -- ordering tracks the ENUM declaration — adding + -- a new role between owner and viewer doesn't + -- need a parallel CASE update here. + array_position( + enum_range(NULL::storage.grant_role), + MAX(ag.role) + )::bigint AS sort_int, MIN(ag.granted_at) AS first_granted_at - FROM storage.access_grants ag + FROM storage.role_grants ag LEFT JOIN auth.users u ON ag.subject_type = 'user' AND u.id = ag.subject_id LEFT JOIN storage.shares sh @@ -1246,13 +1264,13 @@ impl AuthorizationEngine for PgAclEngine { ag.id AS grant_id, ag.granted_at, ag.expires_at, - ag.permission, + ag.role::text AS role, LOWER(rp.subject_display) AS sort_str, rp.sort_int, rp.has_password, rp.is_external FROM rp - JOIN storage.access_grants ag + JOIN storage.role_grants ag ON ag.resource_type = rp.resource_type AND ag.resource_id = rp.resource_id AND ag.subject_type = rp.subject_type @@ -1284,7 +1302,7 @@ impl AuthorizationEngine for PgAclEngine { SELECT resource_type, resource_id, MIN(granted_at) AS first_shared_at, NULL::text AS sort_str, NULL::bigint AS sort_int - FROM storage.access_grants + FROM storage.role_grants WHERE granted_by = $1 GROUP BY resource_type, resource_id ), @@ -1297,12 +1315,12 @@ impl AuthorizationEngine for PgAclEngine { SELECT ag.resource_type, ag.resource_id, rp.first_shared_at, ag.subject_type, ag.subject_id, COALESCE(u.username, u.email, sh.item_name, fi.name, fld.name, ag.subject_id::text) AS subject_display, - ag.id AS grant_id, ag.granted_at, ag.expires_at, ag.permission, + ag.id AS grant_id, ag.granted_at, ag.expires_at, ag.role::text AS role, NULL::text AS sort_str, NULL::bigint AS sort_int, (sh.password_hash IS NOT NULL) AS has_password, COALESCE(u.is_external, FALSE) AS is_external FROM rp - JOIN storage.access_grants ag + JOIN storage.role_grants ag ON ag.resource_type = rp.resource_type AND ag.resource_id = rp.resource_id AND ag.granted_by = $1 LEFT JOIN auth.users u ON ag.subject_type = 'user' AND u.id = ag.subject_id @@ -1380,7 +1398,7 @@ impl AuthorizationEngine for PgAclEngine { grant_id, granted_at, expires_at, - perm_str, + role_str, _, _, has_password, @@ -1389,7 +1407,7 @@ impl AuthorizationEngine for PgAclEngine { let Some(resource_type) = ResourceKind::parse(&rt_str) else { continue; }; - let Some(perm) = Permission::parse(&perm_str) else { + let Some(role) = Role::parse(&role_str) else { continue; }; let key = (resource_id, subj_id); @@ -1409,8 +1427,10 @@ impl AuthorizationEngine for PgAclEngine { }, ) }); - if !entry.permissions.contains(&perm) { - entry.permissions.push(perm); + for &perm in role.expand() { + if !entry.permissions.contains(&perm) { + entry.permissions.push(perm); + } } } @@ -1498,7 +1518,7 @@ impl AuthorizationEngine for PgAclEngine { grant_id, granted_at, expires_at, - perm_str, + role_str, _, _, has_password, @@ -1507,7 +1527,7 @@ impl AuthorizationEngine for PgAclEngine { let Some(resource_type) = ResourceKind::parse(&rt_str) else { continue; }; - let Some(perm) = Permission::parse(&perm_str) else { + let Some(role) = Role::parse(&role_str) else { continue; }; @@ -1531,8 +1551,10 @@ impl AuthorizationEngine for PgAclEngine { has_password, is_external, }); - if !entry.permissions.contains(&perm) { - entry.permissions.push(perm); + for &perm in role.expand() { + if !entry.permissions.contains(&perm) { + entry.permissions.push(perm); + } } } @@ -1578,6 +1600,11 @@ impl AuthorizationEngine for PgAclEngine { } async fn list_outgoing_grants(&self, granted_by: Uuid) -> Result, DomainError> { + // Pivoted to `storage.role_grants` (see `list_incoming_grants`). + // Group membership doesn't apply on the outgoing side — we + // filter by `granted_by` directly. Bundle expansion still + // happens at read time via `role_row_to_grants` until the + // public `Grant` shape becomes role-keyed. let rows = sqlx::query_as::< _, ( @@ -1594,10 +1621,10 @@ impl AuthorizationEngine for PgAclEngine { >( r#" SELECT id, subject_type, subject_id, resource_type, resource_id, - permission, granted_by, granted_at, expires_at - FROM storage.access_grants + role::text, granted_by, granted_at, expires_at + FROM storage.role_grants WHERE granted_by = $1 - ORDER BY granted_at DESC + ORDER BY role ASC, granted_at DESC "#, ) .bind(granted_by) @@ -1608,11 +1635,68 @@ impl AuthorizationEngine for PgAclEngine { rows.into_iter().map(Self::row_to_grant).collect() } - async fn grant( + async fn set_expiry_for_subject( + &self, + subject: Subject, + expires_at: Option>, + ) -> Result<(), DomainError> { + sqlx::query( + "UPDATE storage.role_grants SET expires_at = $3 \ + WHERE subject_type = $1 AND subject_id = $2", + ) + .bind(subject.type_str()) + .bind(subject.id()) + .bind(expires_at) + .execute(self.pool.as_ref()) + .await + .map_err(|e| { + DomainError::internal_error("PgAcl", format!("set_expiry_for_subject: {e}")) + })?; + Ok(()) + } + + async fn revoke(&self, grant_id: Uuid) -> Result<(), DomainError> { + sqlx::query("DELETE FROM storage.role_grants WHERE id = $1") + .bind(grant_id) + .execute(self.pool.as_ref()) + .await + .map_err(|e| DomainError::internal_error("PgAcl", format!("revoke: {e}")))?; + Ok(()) + } + + async fn revoke_all_for_resource(&self, resource: Resource) -> Result { + let result = sqlx::query( + "DELETE FROM storage.role_grants WHERE resource_type = $1 AND resource_id = $2", + ) + .bind(resource.type_str()) + .bind(resource.id()) + .execute(self.pool.as_ref()) + .await + .map_err(|e| DomainError::internal_error("PgAcl", format!("revoke for resource: {e}")))?; + + Ok(result.rows_affected() as usize) + } + + async fn revoke_all_for_subject(&self, subject: Subject) -> Result { + let result = sqlx::query( + "DELETE FROM storage.role_grants WHERE subject_type = $1 AND subject_id = $2", + ) + .bind(subject.type_str()) + .bind(subject.id()) + .execute(self.pool.as_ref()) + .await + .map_err(|e| DomainError::internal_error("PgAcl", format!("revoke for subject: {e}")))?; + + Ok(result.rows_affected() as usize) + } + + // ── D-Prep role_grants writes ────────────────────────────────────────── + + async fn set_role( &self, granted_by: Uuid, subject: Subject, - permission: Permission, + role: Role, resource: Resource, expires_at: Option>, ) -> Result { @@ -1630,149 +1714,17 @@ impl AuthorizationEngine for PgAclEngine { Option>, ), >( - r#" - INSERT INTO storage.access_grants - (subject_type, subject_id, resource_type, resource_id, permission, granted_by, expires_at) - VALUES ($1, $2, $3, $4, $5, $6, $7) - ON CONFLICT (subject_type, subject_id, resource_type, resource_id, permission) - DO UPDATE SET expires_at = EXCLUDED.expires_at - RETURNING id, subject_type, subject_id, resource_type, resource_id, - permission, granted_by, granted_at, expires_at - "#, - ) - .bind(subject.type_str()) - .bind(subject.id()) - .bind(resource.type_str()) - .bind(resource.id()) - .bind(permission.as_str()) - .bind(granted_by) - .bind(expires_at) - .fetch_one(self.pool.as_ref()) - .await - .map_err(|e| DomainError::internal_error("PgAcl", format!("insert grant: {e}")))?; - - Self::row_to_grant(row) - } - - async fn set_expiry_for_subject( - &self, - subject: Subject, - expires_at: Option>, - ) -> Result<(), DomainError> { - sqlx::query( - "UPDATE storage.access_grants SET expires_at = $3 WHERE subject_type = $1 AND subject_id = $2", - ) - .bind(subject.type_str()) - .bind(subject.id()) - .bind(expires_at) - .execute(self.pool.as_ref()) - .await - .map_err(|e| DomainError::internal_error("PgAcl", format!("set_expiry_for_subject: {e}")))?; - Ok(()) - } - - async fn set_expiry_on_resource( - &self, - subject: Subject, - resource: Resource, - expires_at: Option>, - ) -> Result<(), DomainError> { - sqlx::query( - "UPDATE storage.access_grants SET expires_at = $3 \ - WHERE subject_type = $1 AND subject_id = $2 \ - AND resource_type = $4 AND resource_id = $5", - ) - .bind(subject.type_str()) - .bind(subject.id()) - .bind(expires_at) - .bind(resource.type_str()) - .bind(resource.id()) - .execute(self.pool.as_ref()) - .await - .map_err(|e| { - DomainError::internal_error("PgAcl", format!("set_expiry_on_resource: {e}")) - })?; - Ok(()) - } - - async fn revoke(&self, grant_id: Uuid) -> Result<(), DomainError> { - sqlx::query("DELETE FROM storage.access_grants WHERE id = $1") - .bind(grant_id) - .execute(self.pool.as_ref()) - .await - .map_err(|e| DomainError::internal_error("PgAcl", format!("revoke: {e}")))?; - Ok(()) - } - - async fn revoke_all_for_resource(&self, resource: Resource) -> Result { - // D-Prep dual-write: wipe role_grants for this resource too. - // Idempotent — succeeds whether or not any row existed. - sqlx::query( - "DELETE FROM storage.role_grants WHERE resource_type = $1 AND resource_id = $2", - ) - .bind(resource.type_str()) - .bind(resource.id()) - .execute(self.pool.as_ref()) - .await - .map_err(|e| { - DomainError::internal_error("PgAcl", format!("revoke role_grants for resource: {e}")) - })?; - - let result = sqlx::query( - "DELETE FROM storage.access_grants WHERE resource_type = $1 AND resource_id = $2", - ) - .bind(resource.type_str()) - .bind(resource.id()) - .execute(self.pool.as_ref()) - .await - .map_err(|e| DomainError::internal_error("PgAcl", format!("revoke for resource: {e}")))?; - - Ok(result.rows_affected() as usize) - } - - async fn revoke_all_for_subject(&self, subject: Subject) -> Result { - // D-Prep dual-write: wipe role_grants for this subject too. - sqlx::query("DELETE FROM storage.role_grants WHERE subject_type = $1 AND subject_id = $2") - .bind(subject.type_str()) - .bind(subject.id()) - .execute(self.pool.as_ref()) - .await - .map_err(|e| { - DomainError::internal_error("PgAcl", format!("revoke role_grants for subject: {e}")) - })?; - - let result = sqlx::query( - "DELETE FROM storage.access_grants WHERE subject_type = $1 AND subject_id = $2", - ) - .bind(subject.type_str()) - .bind(subject.id()) - .execute(self.pool.as_ref()) - .await - .map_err(|e| DomainError::internal_error("PgAcl", format!("revoke for subject: {e}")))?; - - Ok(result.rows_affected() as usize) - } - - // ── D-Prep role_grants writes ────────────────────────────────────────── - - async fn set_role( - &self, - granted_by: Uuid, - subject: Subject, - role: crate::application::dtos::grant_dto::Role, - resource: Resource, - expires_at: Option>, - ) -> Result<(), DomainError> { - sqlx::query( r#" INSERT INTO storage.role_grants (subject_type, subject_id, resource_type, resource_id, role, granted_by, expires_at) - VALUES ($1, $2, $3, $4, $5, $6, $7) + VALUES ($1, $2, $3, $4, $5::storage.grant_role, $6, $7) ON CONFLICT (subject_type, subject_id, resource_type, resource_id) DO UPDATE SET role = EXCLUDED.role, expires_at = EXCLUDED.expires_at, granted_by = EXCLUDED.granted_by + RETURNING id, subject_type, subject_id, resource_type, resource_id, + role::text, granted_by, granted_at, expires_at "#, ) .bind(subject.type_str()) @@ -1782,11 +1734,11 @@ impl AuthorizationEngine for PgAclEngine { .bind(role.as_str()) .bind(granted_by) .bind(expires_at) - .execute(self.pool.as_ref()) + .fetch_one(self.pool.as_ref()) .await .map_err(|e| DomainError::internal_error("PgAcl", format!("set_role: {e}")))?; - Ok(()) + Self::row_to_grant(row) } async fn clear_role(&self, subject: Subject, resource: Resource) -> Result<(), DomainError> { diff --git a/src/integration_test_support.rs b/src/integration_test_support.rs index 8e2e2e3a..8b4cf043 100644 --- a/src/integration_test_support.rs +++ b/src/integration_test_support.rs @@ -42,7 +42,7 @@ pub fn test_db_url() -> String { /// OnceCell so concurrent test threads block until the first caller /// finishes; subsequent calls are zero-cost. /// -/// Order matters: `storage.access_grants` rows go first because there's +/// Order matters: `storage.role_grants` rows go first because there's /// no FK from there to `auth.subject_groups` (the service's `delete` /// path does this transactionally; here we bypass the service). static CLEANUP_ONCE: tokio::sync::OnceCell<()> = tokio::sync::OnceCell::const_new(); @@ -51,7 +51,7 @@ pub async fn ensure_clean_test_db(pool: &PgPool) { CLEANUP_ONCE .get_or_init(|| async { let _ = sqlx::query( - "DELETE FROM storage.access_grants + "DELETE FROM storage.role_grants WHERE subject_type = 'group' AND subject_id IN ( SELECT id FROM auth.subject_groups WHERE name LIKE 'rust-test-%' diff --git a/src/interfaces/api/handlers/grant_handler.rs b/src/interfaces/api/handlers/grant_handler.rs index 21617cb5..7fc39ab3 100644 --- a/src/interfaces/api/handlers/grant_handler.rs +++ b/src/interfaces/api/handlers/grant_handler.rs @@ -21,9 +21,9 @@ use uuid::Uuid; use crate::application::dtos::cursor::PageCursor; use crate::application::dtos::grant_dto::{ CreateGrantDto, CreateGrantResponseDto, GrantDto, MySharesDto, NotifyOutcomeSetDto, - OutgoingResourceGrantDto, OutgoingResourceItemDto, PermissionDto, ResourceContentDto, - ResourceDto, ResourceTypeDto, Role, SharedWithMeDto, SharedWithMeItemDto, SharedWithMeQuery, - SubjectDto, SubjectInputDto, UpdateRoleDto, role_from_permissions, + OutgoingResourceGrantDto, OutgoingResourceItemDto, ResourceContentDto, ResourceDto, + ResourceTypeDto, SharedWithMeDto, SharedWithMeItemDto, SharedWithMeQuery, SubjectDto, + SubjectInputDto, UpdateRoleDto, role_from_permissions, }; use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::file_ports::FileRetrievalUseCase; @@ -35,7 +35,7 @@ use crate::common::errors::DomainError; use crate::domain::errors::ErrorKind; use crate::domain::services::authorization::{ GrantCursor, IncomingGrantSummary, OutgoingResourceSummary, Permission, Resource, ResourceKind, - Subject, + Role, Subject, }; use crate::interfaces::errors::AppError; use crate::interfaces::middleware::auth::AuthUser; @@ -66,41 +66,7 @@ pub async fn create_grant( let authz = &state.authorization; let caller_id = auth_user.id; - // Validate: exactly one of permissions/role. Capture BOTH the - // permission list (for the per-permission access_grants writes that - // keep the old engine read path working) AND the role (for the new - // role_grants `set_role` dual-write that lands after the per- - // permission loop). - let (permissions, role): (Vec, Role) = match (dto.permissions, dto.role) { - (Some(perms), None) if !perms.is_empty() => { - let perms: Vec = perms.into_iter().map(Into::into).collect(); - // Derive the closest matching role from the raw permission set - // so we have ONE role to mirror into role_grants. `Role::parse` - // always succeeds here because `role_from_permissions` only - // emits known role strings. - let role = Role::parse(role_from_permissions(&perms)) - .expect("role_from_permissions returns a known role string"); - (perms, role) - } - (None, Some(role)) => (role.expand().to_vec(), role), - (Some(_), Some(_)) => { - return AppError::new( - StatusCode::BAD_REQUEST, - "Provide either 'permissions' or 'role', not both", - "InvalidInput", - ) - .into_response(); - } - _ => { - return AppError::new( - StatusCode::BAD_REQUEST, - "Either 'permissions' (non-empty) or 'role' is required", - "InvalidInput", - ) - .into_response(); - } - }; - + let role: Role = dto.role.into(); let resource: Resource = dto.resource.into(); let expires_at = dto.expires_at; @@ -165,41 +131,20 @@ pub async fn create_grant( } }; - let mut results: Vec = Vec::with_capacity(permissions.len()); - for perm in permissions { - // `storage.access_grants.permission` CHECK constraint predates - // `Permission::Manage`; it accepts only the original 6 values. - // Manage exists in the Owner bundle for engine read-path use - // (via `roles_implying`) and gets persisted via the role_grants - // dual-write below. Skipping it here keeps the access_grants - // safety net populated without tripping the CHECK; the cleanup - // PR that drops access_grants also drops this skip. - if perm == Permission::Manage { - continue; - } - match authz - .grant(caller_id, subject, perm, resource, expires_at) - .await - { - Ok(grant) => results.push(grant.into()), - Err(err) => { - error!("grant insert failed for {perm:?}: {err}"); - return AppError::from(err).into_response(); - } - } - } - - // D-Prep dual-write: mirror the role assignment into storage.role_grants. - // ON CONFLICT UPDATE makes this idempotent — repeated POSTs with the - // same (subject, resource) update the role in place, matching the - // PATCH-style semantics callers will get after the engine read pivot. - if let Err(err) = authz + // Single role row in `storage.role_grants`. `ON CONFLICT UPDATE` in + // the engine makes repeated POSTs with the same (subject, resource) + // a role refresh, matching the PATCH-style semantics callers expect. + let grant = match authz .set_role(caller_id, subject, role, resource, expires_at) .await { - error!("set_role dual-write failed: {err}"); - return AppError::from(err).into_response(); - } + Ok(g) => g, + Err(err) => { + error!("set_role write failed: {err}"); + return AppError::from(err).into_response(); + } + }; + let grants = vec![GrantDto::from(grant)]; tracing::info!( target: "audit", @@ -210,7 +155,6 @@ pub async fn create_grant( resource_type = resource.type_str(), resource_id = %resource.id(), role = role.as_str(), - permission_count = results.len(), expires_at = ?expires_at, "🤝 grant created with role '{}'", role.as_str(), ); @@ -282,7 +226,7 @@ pub async fn create_grant( ( StatusCode::CREATED, Json(CreateGrantResponseDto { - grants: results, + grants, notification, }), ) @@ -560,9 +504,8 @@ pub async fn set_role( let caller_id = auth_user.id; let subject: Subject = dto.subject.into(); let resource: Resource = dto.resource.into(); + let role: Role = dto.role.into(); let expires_at = dto.expires_at; - let target_perms: std::collections::HashSet = - dto.role.expand().iter().copied().collect(); // Caller must have Share on the resource. if let Err(e) = authz @@ -572,86 +515,16 @@ pub async fn set_role( return AppError::from(e).into_response(); } - // Fetch current grants on the resource for this subject. - let current = match authz.list_grants_on_resource(resource).await { + // Atomic role refresh. UNIQUE on (subject, resource) + ON CONFLICT + // UPDATE in `set_role` turns this into a single UPSERT — no diff, + // no race window. Returns the resulting role row. + let grant = match authz + .set_role(caller_id, subject, role, resource, expires_at) + .await + { Ok(g) => g, Err(e) => return AppError::from(e).into_response(), }; - let current_perms: std::collections::HashSet = current - .iter() - .filter(|g| g.subject == subject) - .map(|g| g.permission) - .collect(); - - // Diff and apply. `Permission::Manage` is excluded from both sides - // because the historical `access_grants.permission` CHECK doesn't - // accept it — see the matching skip in `create_grant`. The role - // assignment captures Manage via the role_grants `set_role` call - // further down; the engine's read-path uses `roles_implying(Manage)` - // → `[Owner]` and never goes through per-permission rows. - let to_add: Vec = target_perms - .difference(¤t_perms) - .copied() - .filter(|p| *p != Permission::Manage) - .collect(); - let to_remove: Vec = current_perms - .difference(&target_perms) - .copied() - .filter(|p| *p != Permission::Manage) - .collect(); - - for perm in &to_remove { - if let Some(g) = current - .iter() - .find(|g| g.subject == subject && g.permission == *perm) - && let Err(e) = authz.revoke(g.id).await - { - return AppError::from(e).into_response(); - } - } - for perm in &to_add { - if let Err(e) = authz - .grant(caller_id, subject, *perm, resource, expires_at) - .await - { - return AppError::from(e).into_response(); - } - } - - // Sync expiry on all remaining grants for this (subject, resource) pair — - // includes newly added ones and any that were already present (retained). - // Callers that omit expires_at will clear any existing expiry; this is - // intentional: it keeps all permission rows for the pair consistent. - if let Err(e) = authz - .set_expiry_on_resource(subject, resource, expires_at) - .await - { - return AppError::from(e).into_response(); - } - - // D-Prep dual-write: mirror the resulting role into storage.role_grants. - // The per-permission diff above keeps access_grants converged; this - // single UPSERT keeps role_grants in sync with the OVERALL outcome - // (one row carrying the role + expiry). After the engine read pivot - // and the access_grants drop, the per-permission diff above goes - // away and this call becomes the only mutation the handler performs. - if let Err(e) = authz - .set_role(caller_id, subject, dto.role, resource, expires_at) - .await - { - return AppError::from(e).into_response(); - } - - // Return the new full set. - let after = match authz.list_grants_on_resource(resource).await { - Ok(g) => g, - Err(e) => return AppError::from(e).into_response(), - }; - let mine: Vec = after - .into_iter() - .filter(|g| g.subject == subject) - .map(Into::into) - .collect(); tracing::info!( target: "audit", @@ -661,34 +534,22 @@ pub async fn set_role( subject_id = %subject.id(), resource_type = resource.type_str(), resource_id = %resource.id(), - role = dto.role.as_str(), - permissions_added = to_add.len(), - permissions_removed = to_remove.len(), + role = role.as_str(), expires_at = ?expires_at, - "🔁 role set to '{}' (+{} -{})", - dto.role.as_str(), - to_add.len(), - to_remove.len(), + "🔁 role set to '{}'", role.as_str(), ); - (StatusCode::OK, Json(mine)).into_response() + (StatusCode::OK, Json(vec![GrantDto::from(grant)])).into_response() } // ════════════════════════════════════════════════════════════════════════════ // GET /api/grants/incoming // ════════════════════════════════════════════════════════════════════════════ -#[derive(Debug, Deserialize, IntoParams)] -pub struct IncomingQuery { - #[serde(default)] - pub permission: Option, -} - #[utoipa::path( get, path = "/api/grants/incoming", - params(IncomingQuery), responses( - (status = 200, description = "Direct grants targeting the caller", body = Vec), + (status = 200, description = "Direct role grants targeting the caller", body = Vec), ), security(("bearerAuth" = [])), tag = "grants" @@ -696,12 +557,11 @@ pub struct IncomingQuery { pub async fn list_incoming( State(state): State, auth_user: AuthUser, - Query(q): Query, ) -> impl IntoResponse { let caller_id = auth_user.id; match state .authorization - .list_incoming_grants(Subject::User(caller_id), q.permission.map(Into::into)) + .list_incoming_grants(Subject::User(caller_id)) .await { Ok(grants) => { diff --git a/src/interfaces/api/mod.rs b/src/interfaces/api/mod.rs index e30113f3..2c07d61f 100644 --- a/src/interfaces/api/mod.rs +++ b/src/interfaces/api/mod.rs @@ -23,7 +23,7 @@ use crate::application::dtos::folder_dto::{ use crate::application::dtos::folder_listing_dto::FolderListingDto; use crate::application::dtos::grant_dto::{ CreateGrantDto, GrantDto, OutgoingResourceItemDto, PermissionDto, ResourceContentDto, - ResourceDto, ResourceTypeDto, Role, SharedWithMeDto, SharedWithMeItemDto, SubjectDto, + ResourceDto, ResourceTypeDto, RoleDto, SharedWithMeDto, SharedWithMeItemDto, SubjectDto, SubjectTypeDto, UpdateRoleDto, }; use crate::application::dtos::i18n_dto::{ @@ -351,7 +351,7 @@ use crate::interfaces::api::handlers::file_handler::MoveFilePayload; ResourceTypeDto, ResourceDto, PermissionDto, - Role, + RoleDto, CreateGrantDto, UpdateRoleDto, GrantDto, diff --git a/static/js/components/shareModal.js b/static/js/components/shareModal.js index dc5b246a..6c876301 100644 --- a/static/js/components/shareModal.js +++ b/static/js/components/shareModal.js @@ -73,23 +73,6 @@ function _looksLikeEmail(q) { return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(q); } -/** - * Permissions that belong to each role (mirrors `Role::expand()` in - * `src/application/dtos/grant_dto.rs`). The share modal only renders - * Viewer/Editor/Owner as picker buttons today; `commenter` and - * `contributor` are kept here for fidelity with the server-side enum so - * a future UI exposure doesn't need a mirror-table update. The `manage` - * permission in the owner bundle is reserved for Drive- and Group-level - * admin actions (no-op on file/folder resources). - */ -const ROLE_PERMISSIONS = { - viewer: ['read'], - commenter: ['read', 'comment'], - contributor: ['read', 'create'], - editor: ['read', 'comment', 'create', 'update'], - owner: ['read', 'comment', 'create', 'update', 'share', 'delete', 'manage'] -}; - /** * Fetch up to ~8 ReBAC subject groups whose name matches `q`. Authenticated * endpoint; returns `[]` on any failure so the autocomplete degrades to @@ -117,14 +100,20 @@ async function _searchGroups(q) { } /** - * Derive the highest role a set of grants represents for one subject. + * Pick the displayed role for a member row. Server-side every Grant + * carries an explicit role since the cleanup PR, so this just reads it. + * The server may emit `commenter` or `contributor` (full enum), but the + * picker only exposes Viewer/Editor/Owner — collapse the two unexposed + * roles to the closest neighbour so the UI never renders an unknown + * option. * @param {Grant[]} subjectGrants * @returns {ShareRoleEnum} */ function _roleFromGrants(subjectGrants) { - const perms = new Set(subjectGrants.map((g) => g.permission)); - if (perms.has('delete') || perms.has('share')) return 'owner'; - if (perms.has('create') || perms.has('update')) return 'editor'; + const role = subjectGrants[0]?.role; + if (role === 'owner' || role === 'editor' || role === 'viewer') return role; + if (role === 'commenter') return 'viewer'; + if (role === 'contributor') return 'editor'; return 'viewer'; } @@ -616,7 +605,7 @@ const shareModal = { granted_at: '', granted_by: '', subject: { type: subjectType, id: contact.id }, - permission: /** @type {import('../core/types.js').PermissionTypeEnum} */ (ROLE_PERMISSIONS[this._stagedRole][0]), + role: this._stagedRole, resource: { type: this._itemType, id: this._item?.id ?? '' } }; this._localMembers.push({ diff --git a/static/js/core/types.js b/static/js/core/types.js index 3a719a2b..0c12efb2 100644 --- a/static/js/core/types.js +++ b/static/js/core/types.js @@ -302,21 +302,27 @@ * @property {String} id */ +/** + * Server-side role enum — every grantable role the backend recognises. + * The share modal's UI picker only exposes a subset (see `ShareRoleEnum`); + * the wire format may carry any of these values on a Grant. + * @typedef {'viewer'|'commenter'|'contributor'|'editor'|'owner'} GrantRoleEnum + */ + /** * @typedef {Object} Grant * @property {string} id * @property {string} granted_at - ISO-8601 datetime string. * @property {string} granted_by * @property {Subject} subject - * @property {PermissionTypeEnum} permission + * @property {GrantRoleEnum} role - Role-keyed grant. One Grant = one role + * assignment in `storage.role_grants`. The implied permission bundle + * is derived client-side from the same lookup table used by + * `Role::expand()` on the server (see `ROLE_PERMISSIONS` in shareModal). * @property {Resource} resource * @property {string|null} [expires_at] - ISO-8601 datetime string, or absent/null for no expiry. */ -/** - * Roles: `viewer`, `commenter`, `editor`, `manager`, `admin` - */ - /** * Configuration for `ResourceListComponent`. * @typedef {Object} ResourceListConfig @@ -367,7 +373,7 @@ * @property {'user'|'group'|'token'|'external'} subject_type * @property {string} subject_id * @property {string} subject_display - Username (users) or share name (tokens). - * @property {'viewer'|'commenter'|'contributor'|'editor'|'owner'} role - Server-emitted role string. `commenter` and `contributor` are reserved for future UI exposure; today the share modal only renders `viewer`/`editor`/`owner` (see `ShareRoleEnum`). + * @property {GrantRoleEnum} role - Server-emitted role string. `commenter` and `contributor` are reserved for future UI exposure; today the share modal only renders `viewer`/`editor`/`owner` (see `ShareRoleEnum`). * @property {string} granted_at - ISO-8601 * @property {string|null} [expires_at] - ISO-8601 or absent. * @property {boolean} has_password - True when a token subject has a password set. @@ -464,7 +470,11 @@ * One collaborator row in the share modal's People section. * @typedef {Object} MemberEntry * @property {Grant} grant - Representative grant (used for subject/resource info). - * @property {Grant[]} _grants - All grants for this subject on the resource (may be > 1). + * @property {Grant[]} _grants - All grants for this subject on the resource. Post-pivot + * this is at most one entry (`storage.role_grants` UNIQUE on + * `(subject, resource)`); the array shape is preserved so the existing + * "revoke every grant on remove" loop in `_applyAll` still works + * without a special-case for empty / new entries. * @property {ShareRoleEnum} role - Derived role label shown in the UI. * @property {'keep'|'remove'|'change'|'new'} _op - Pending local operation. * @property {string|null} [expires_at] - YYYY-MM-DD expiry date string, or null for no expiry. diff --git a/static/js/model/grants.js b/static/js/model/grants.js index a6890ff0..68dd8bb3 100644 --- a/static/js/model/grants.js +++ b/static/js/model/grants.js @@ -164,7 +164,9 @@ const grants = { /** * Create a new grant. - * Body mirrors `CreateGrantDto`: `{ subject, resource, role }` OR `{ subject, resource, permissions }`. + * Body mirrors `CreateGrantDto`: `{ subject, resource, role, expires_at? }`. + * Strictly role-keyed since the cleanup PR — the per-permission shape + * is no longer accepted. * * Response shape (PR N1 — `CreateGrantResponseDto`): * diff --git a/static/sw.js b/static/sw.js index 17197a43..d6915295 100644 --- a/static/sw.js +++ b/static/sw.js @@ -1,6 +1,6 @@ // OxiCloud Service Worker // FIXME: generate cache name according build ? -const CACHE_NAME = 'oxicloud-cache-v27'; +const CACHE_NAME = 'oxicloud-cache-v28'; // Only cache static assets — NOT HTML files. // HTML files are served network-first so browsers always get the latest diff --git a/tests/api/grants.hurl b/tests/api/grants.hurl index 3c5214ac..6c503eb6 100644 --- a/tests/api/grants.hurl +++ b/tests/api/grants.hurl @@ -122,11 +122,11 @@ Content-Type: application/json } HTTP 201 -# PR N1: POST /api/grants now wraps results in -# `CreateGrantResponseDto { grants, notification }`. +# Cleanup PR: one role row per (subject, resource). `CreateGrantResponseDto` +# wraps a single role-keyed Grant in `.grants[0]`. [Asserts] jsonpath "$.grants" count == 1 -jsonpath "$.grants[0].permission" == "read" +jsonpath "$.grants[0].role" == "viewer" # ───────────────────────────────────────────────────────────── @@ -148,12 +148,13 @@ Authorization: Bearer {{dave_token}} HTTP 200 [Asserts] -jsonpath "$[?(@.resource.id=='{{shared_folder_id}}')].permission" == "read" +jsonpath "$[?(@.resource.id=='{{shared_folder_id}}')].role" == "viewer" # ───────────────────────────────────────────────────────────── -# Step 9 — Promote Bob to Admin (adds comment, create, update, share, delete). -# PUT /api/grants/role reconciles the row set in one call. +# Step 9 — Promote Bob to Owner (covers comment, create, update, share, +# delete, manage). PUT /api/grants/role replaces the role in one +# UPSERT against `storage.role_grants`. # ───────────────────────────────────────────────────────────── PUT {{base_url}}/api/grants/role Authorization: Bearer {{alice_token}} @@ -161,16 +162,17 @@ Content-Type: application/json { "subject": { "type": "user", "id": "{{dave_user_id}}" }, "resource": { "type": "folder", "id": "{{shared_folder_id}}" }, - "role": "admin" + "role": "owner" } HTTP 200 [Asserts] -jsonpath "$" count == 6 +jsonpath "$" count == 1 +jsonpath "$[0].role" == "owner" # ───────────────────────────────────────────────────────────── -# Step 10 — Bob can now rename (Manager includes update). +# Step 10 — Bob can now rename (Owner includes update). # ───────────────────────────────────────────────────────────── PUT {{base_url}}/api/folders/{{shared_folder_id}}/rename Authorization: Bearer {{dave_token}} @@ -194,7 +196,7 @@ HTTP 200 # ───────────────────────────────────────────────────────────── -# Step 12 — Bob re-shares to Carol (he has Share via Admin). +# Step 12 — Bob re-shares to Carol (he has Share via Owner). # ───────────────────────────────────────────────────────────── POST {{base_url}}/api/grants Authorization: Bearer {{dave_token}} @@ -218,7 +220,7 @@ Authorization: Bearer {{eve_token}} HTTP 200 [Asserts] -jsonpath "$[?(@.resource.id=='{{shared_folder_id}}')].permission" == "read" +jsonpath "$[?(@.resource.id=='{{shared_folder_id}}')].role" == "viewer" # ───────────────────────────────────────────────────────────── @@ -247,7 +249,7 @@ Content-Type: application/json HTTP 200 [Asserts] jsonpath "$" count == 1 -jsonpath "$[0].permission" == "read" +jsonpath "$[0].role" == "viewer" # ───────────────────────────────────────────────────────────── @@ -263,8 +265,9 @@ HTTP 404 # ───────────────────────────────────────────────────────────── # Step 17 — Lifecycle: Alice deletes the folder. The DB trigger -# trg_cleanup_grants_folder removes both bob's and carol's -# grants automatically (also for the cascade-deleted child). +# trg_cleanup_role_grants_folder removes both bob's and +# carol's grants automatically (also for the cascade-deleted +# child). # ───────────────────────────────────────────────────────────── DELETE {{base_url}}/api/folders/{{child_folder_id}} Authorization: Bearer {{alice_token}} @@ -767,7 +770,7 @@ HTTP 404 # ════════════════════════════════════════════════════════════════════ -# Phase 2D — Promote adam to Admin (all 6 permissions). Delete OK. +# Phase 2D — Promote adam to Owner (full bundle, incl. delete). Delete OK. # ════════════════════════════════════════════════════════════════════ PUT {{base_url}}/api/grants/role Authorization: Bearer {{alice_token}} @@ -775,7 +778,7 @@ Content-Type: application/json { "subject": { "type": "user", "id": "{{adam_user_id}}" }, "resource": { "type": "folder", "id": "{{perm_folder_id}}" }, - "role": "admin" + "role": "owner" } HTTP 200 @@ -788,7 +791,7 @@ HTTP 204 # ════════════════════════════════════════════════════════════════════ # Phase 2E — Lifecycle cleanup. Alice (still the owner) trashes & -# empties; the trigger removes all access_grants rows. +# empties; the trigger removes all role_grants rows. # ════════════════════════════════════════════════════════════════════ DELETE {{base_url}}/api/folders/{{perm_folder_id}} Authorization: Bearer {{alice_token}} @@ -1176,12 +1179,12 @@ Content-Type: application/json { "subject": { "type": "user", "id": "{{frank_user_id}}" }, "resource": { "type": "folder", "id": "{{batch_root_id}}" }, - "role": "admin" + "role": "owner" } HTTP 200 -# Frank (Admin grant = Delete) trashes batch_file_2 — item goes to +# Frank (Owner role includes Delete) trashes batch_file_2 — item goes to # Alice's trash because file.user_id is unchanged (Alice is still owner). POST {{base_url}}/api/batch/trash Authorization: Bearer {{frank_token}} diff --git a/tests/api/grants_nested_groups.hurl b/tests/api/grants_nested_groups.hurl index e11c6282..83a8b585 100644 --- a/tests/api/grants_nested_groups.hurl +++ b/tests/api/grants_nested_groups.hurl @@ -285,7 +285,7 @@ HTTP 201 # `CreateGrantResponseDto { grants, notification }`. [Asserts] jsonpath "$.grants" count == 1 -jsonpath "$.grants[0].permission" == "read" +jsonpath "$.grants[0].role" == "viewer" jsonpath "$.grants[0].subject.type" == "group" jsonpath "$.grants[0].subject.id" == "{{group_a_id}}" @@ -355,7 +355,7 @@ Authorization: Bearer {{henry_token}} HTTP 200 [Asserts] -jsonpath "$[?(@.resource.id=='{{perm_folder_id}}')].permission" == "read" +jsonpath "$[?(@.resource.id=='{{perm_folder_id}}')].role" == "viewer" jsonpath "$[?(@.resource.id=='{{perm_folder_id}}')].subject.type" == "group" jsonpath "$[?(@.resource.id=='{{perm_folder_id}}')].subject.id" == "{{group_a_id}}" @@ -533,7 +533,7 @@ HTTP 404 # ════════════════════════════════════════════════════════════════════ -# Phase D — Promote group A's grant to Admin (all 6 permissions). +# Phase D — Promote group A's grant to Owner (full bundle). # Delete now succeeds for henry, still flowing through B → A. # ════════════════════════════════════════════════════════════════════ PUT {{base_url}}/api/grants/role @@ -542,7 +542,7 @@ Content-Type: application/json { "subject": { "type": "group", "id": "{{group_a_id}}" }, "resource": { "type": "folder", "id": "{{perm_folder_id}}" }, - "role": "admin" + "role": "owner" } HTTP 200 diff --git a/tests/api/role_grants.hurl b/tests/api/role_grants.hurl index 4a0f4ede..e5a218e1 100644 --- a/tests/api/role_grants.hurl +++ b/tests/api/role_grants.hurl @@ -4,17 +4,15 @@ # Pins the D-Prep refactor behaviours that don't fit naturally into # the existing `grants.hurl` (which is API-shape-focused). Specifically: # -# 1. New wire-format role names: +# 1. Wire-format role names: # - "owner" is accepted on POST and emitted on response -# - "admin" still accepted on POST for one release (compat shim -# in `Role::parse`); the server normalises it to Owner +# - "admin" is REJECTED with 422 (compat alias retired in the +# cleanup PR — see Step 6a) # -# 2. Dual-write proof: granting a role and then exercising a +# 2. Role-keyed write proof: granting a role and then exercising a # permission from its bundle works → proves the row landed in -# `storage.role_grants` because the engine now reads from there -# for authz decisions (see `folder_cascade_grant_exists` post- -# D-Prep). If dual-write failed, the engine would see no row and -# reject the check. +# `storage.role_grants` and the engine read path expands the +# bundle correctly (see `folder_cascade_grant_exists`). # # 3. Atomic role updates via PUT /api/grants/role — the role flips # in a single SQL update (no DELETE+INSERT race window). @@ -177,10 +175,9 @@ HTTP 204 # ───────────────────────────────────────────────────────────── -# Step 6 — Legacy "admin" string compat. The server's Role::parse -# accepts "admin" and normalises to Owner during the D-Prep -# dual-write window (one release). Verify by granting sam -# with role="admin" — sam should then be able to Delete too. +# Step 6a — Reject the legacy "admin" string. The cleanup PR +# retired the `#[serde(alias = "admin")]` compat shim on +# `RoleDto::Owner`; the deserialiser now refuses it with 422. # ───────────────────────────────────────────────────────────── POST {{base_url}}/api/grants Authorization: Bearer {{admin_token}} @@ -191,15 +188,26 @@ Content-Type: application/json "role": "admin" } +HTTP 422 + + +# ───────────────────────────────────────────────────────────── +# Step 6b — Grant sam Owner with the canonical role string. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/grants +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{sam_user_id}}" }, + "resource": { "type": "folder", "id": "{{test_folder_id}}" }, + "role": "owner" +} + HTTP 201 [Captures] -# Capture the first per-permission grant id from the response so -# Step 10's revoke doesn't have to query the My Shares endpoint -# with awkward JSONPath filtering. Any single grant_id works: -# the revoke handler calls `clear_role` which wipes the entire -# role_grants row for (sam, test_folder), so the engine reads -# return false for sam afterwards regardless of how many -# access_grants rows still exist. +# The single role-keyed Grant returned in `.grants[0]` is the +# `storage.role_grants` row id. Step 10's revoke uses it to +# `clear_role` and wipe the row. sam_grant_id: jsonpath "$.grants[0].id" @@ -326,9 +334,8 @@ body not contains "\"role\":\"admin\"" # ───────────────────────────────────────────────────────────── -# Step 10 — Revoke: removing sam's grant. The handler clears the -# access_grants rows AND calls clear_role to wipe the -# role_grants row in the same flow. +# Step 10 — Revoke: removing sam's grant. `engine.revoke()` DELETEs +# the single `storage.role_grants` row by id. # # After: sam's Delete attempt should be refused (proof # the role_grants row is gone — the cascade query for @@ -336,7 +343,7 @@ body not contains "\"role\":\"admin\"" # at this folder). # ───────────────────────────────────────────────────────────── -# sam_grant_id was captured at Step 6 from the create response. +# sam_grant_id was captured at Step 6b from the create response. DELETE {{base_url}}/api/grants/{{sam_grant_id}} Authorization: Bearer {{admin_token}} diff --git a/tests/api/subject_groups.hurl b/tests/api/subject_groups.hurl index aa958bb0..93fe1680 100644 --- a/tests/api/subject_groups.hurl +++ b/tests/api/subject_groups.hurl @@ -221,7 +221,7 @@ Content-Type: application/json { "subject": { "type": "group", "id": "{{engineers_id}}" }, "resource": { "type": "folder", "id": "{{shared_folder_id}}" }, - "permissions": ["read"] + "role": "viewer" } HTTP 201 From c893f9ae2908fdcf8ca712614e3b366c3181e851 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Thu, 18 Jun 2026 01:52:47 +0200 Subject: [PATCH 6/6] chore(roles): remove deprecated tools this tools are were heere to be tracked, no more necessary once migration is done --- tools/audit-grants-bundle-shape.sql | 215 ------------------ tools/d-prep-real-db-smoke.sh | 328 ---------------------------- 2 files changed, 543 deletions(-) delete mode 100644 tools/audit-grants-bundle-shape.sql delete mode 100755 tools/d-prep-real-db-smoke.sh diff --git a/tools/audit-grants-bundle-shape.sql b/tools/audit-grants-bundle-shape.sql deleted file mode 100644 index fb3d24de..00000000 --- a/tools/audit-grants-bundle-shape.sql +++ /dev/null @@ -1,215 +0,0 @@ --- ════════════════════════════════════════════════════════════════════════════ --- audit-grants-bundle-shape.sql — Pre-D-Prep diagnostic --- ════════════════════════════════════════════════════════════════════════════ --- Purpose: confirm the assumption that >99% of existing `storage.access_grants` --- rows already cluster into the standard role bundles --- (viewer / commenter / contributor / editor / owner). The answer determines --- whether the access_grants → role_grants migration is fully mechanical (just --- run the backfill) or whether per-row decisions are needed for some edge --- cases. --- --- READ-ONLY. Safe to run against any environment including production. --- --- Usage: --- psql "$DATABASE_URL" -f tools/audit-grants-bundle-shape.sql --- --- Reference role bundles (mirror `Role::expand()` in --- `src/application/dtos/grant_dto.rs`): --- viewer = {read} --- commenter = {read, comment} ← reserved variant --- contributor = {read, create} ← new in D-Prep --- editor = {read, comment, create, update} --- owner = {read, comment, create, update, share, delete} --- (post-D-Prep: + manage, when Group-as-Resource lands) --- ════════════════════════════════════════════════════════════════════════════ - - -\echo '' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' 0. Total population' -\echo '─────────────────────────────────────────────────────────────────────────' - -SELECT count(*) AS total_grant_rows, - count(DISTINCT (subject_type, subject_id, resource_type, resource_id)) - AS distinct_clusters, - ROUND( - count(*)::numeric - / NULLIF(count(DISTINCT (subject_type, subject_id, resource_type, resource_id)), 0), - 2 - ) AS avg_rows_per_cluster -FROM storage.access_grants; - - -\echo '' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' 1. Bundle distribution — which permission sets exist, how popular?' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' Each row = a unique permission set; cluster_count = how many (subject,' -\echo ' resource) pairs have exactly that set.' - -WITH cluster AS ( - SELECT subject_type, - subject_id, - resource_type, - resource_id, - array_agg(permission ORDER BY permission) AS perms - FROM storage.access_grants - GROUP BY 1, 2, 3, 4 -), -known_bundles AS ( - SELECT ARRAY['read']::text[] AS perms, 'viewer' AS role - UNION ALL SELECT ARRAY['comment','read']::text[], 'commenter' - UNION ALL SELECT ARRAY['create','read']::text[], 'contributor' - UNION ALL SELECT ARRAY['comment','create','read','update']::text[], 'editor' - UNION ALL SELECT ARRAY['comment','create','delete','read','share','update']::text[], 'owner' -) -SELECT cluster.perms, - count(*) AS cluster_count, - ROUND(100.0 * count(*) / SUM(count(*)) OVER (), 2) AS pct, - COALESCE(known_bundles.role, '(non-bundle)') AS maps_to_role -FROM cluster -LEFT JOIN known_bundles ON known_bundles.perms = cluster.perms -GROUP BY cluster.perms, known_bundles.role -ORDER BY cluster_count DESC; - - -\echo '' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' 2. Bundle-shaped vs not — the headline number' -\echo '─────────────────────────────────────────────────────────────────────────' - -WITH cluster AS ( - SELECT subject_type, - subject_id, - resource_type, - resource_id, - array_agg(permission ORDER BY permission) AS perms - FROM storage.access_grants - GROUP BY 1, 2, 3, 4 -), -known_bundles AS ( - SELECT ARRAY['read']::text[] AS perms - UNION ALL SELECT ARRAY['comment','read']::text[] - UNION ALL SELECT ARRAY['create','read']::text[] - UNION ALL SELECT ARRAY['comment','create','read','update']::text[] - UNION ALL SELECT ARRAY['comment','create','delete','read','share','update']::text[] -), -shape AS ( - SELECT CASE WHEN cluster.perms = ANY(SELECT kb.perms FROM known_bundles kb) - THEN 'bundle-shaped' - ELSE 'NON-bundle (needs per-row decision)' - END AS shape - FROM cluster -) -SELECT shape, - count(*) AS clusters, - ROUND(100.0 * count(*) / SUM(count(*)) OVER (), 2) AS pct -FROM shape -GROUP BY shape -ORDER BY clusters DESC; - - -\echo '' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' 3. Non-bundle clusters in detail (the <1% — investigate each)' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' If this returns 0 rows, the migration is FULLY mechanical. Otherwise' -\echo ' eyeball each row and decide: closest role + audit log entry, or' -\echo ' refuse-to-migrate (rare).' - -WITH cluster AS ( - SELECT subject_type, - subject_id, - resource_type, - resource_id, - array_agg(permission ORDER BY permission) AS perms - FROM storage.access_grants - GROUP BY 1, 2, 3, 4 -), -known_bundles AS ( - SELECT ARRAY['read']::text[] AS perms - UNION ALL SELECT ARRAY['comment','read']::text[] - UNION ALL SELECT ARRAY['create','read']::text[] - UNION ALL SELECT ARRAY['comment','create','read','update']::text[] - UNION ALL SELECT ARRAY['comment','create','delete','read','share','update']::text[] -) -SELECT cluster.subject_type, - cluster.subject_id, - cluster.resource_type, - cluster.resource_id, - cluster.perms -FROM cluster -WHERE NOT (cluster.perms = ANY(SELECT kb.perms FROM known_bundles kb)) -ORDER BY cluster.resource_type, cluster.resource_id -LIMIT 200; - - -\echo '' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' 4. Sanity checks — invariants that should be true today' -\echo '─────────────────────────────────────────────────────────────────────────' - -\echo '' -\echo ' 4a. Clusters with no Read permission (broken? if >0, investigate)' -WITH cluster AS ( - SELECT subject_type, subject_id, resource_type, resource_id, - array_agg(permission ORDER BY permission) AS perms - FROM storage.access_grants - GROUP BY 1, 2, 3, 4 -) -SELECT count(*) AS clusters_without_read -FROM cluster -WHERE NOT ('read' = ANY(perms)); - -\echo '' -\echo ' 4b. Subject-type distribution (sanity check on token / external counts)' -SELECT subject_type, - count(*) AS rows, - count(DISTINCT subject_id) AS distinct_subjects, - count(DISTINCT (resource_type, resource_id)) AS distinct_resources -FROM storage.access_grants -GROUP BY subject_type -ORDER BY subject_type; - -\echo '' -\echo ' 4c. Resource-type distribution' -SELECT resource_type, count(*) AS rows -FROM storage.access_grants -GROUP BY resource_type -ORDER BY rows DESC; - -\echo '' -\echo ' 4d. Permission distribution (total rows per permission value)' -SELECT permission, count(*) AS rows -FROM storage.access_grants -GROUP BY permission -ORDER BY rows DESC; - - -\echo '' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' 5. Migration cost estimate' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' role_grants will have ~ (distinct clusters) rows, replacing ~ (total)' -\echo ' rows in access_grants. The ratio is the per-row reduction factor.' - -WITH cluster AS ( - SELECT subject_type, subject_id, resource_type, resource_id - FROM storage.access_grants - GROUP BY 1, 2, 3, 4 -) -SELECT (SELECT count(*) FROM storage.access_grants) AS access_grants_rows_today, - (SELECT count(*) FROM cluster) AS role_grants_rows_after, - ROUND( - (SELECT count(*) FROM storage.access_grants)::numeric - / NULLIF((SELECT count(*) FROM cluster), 0), - 2 - ) AS reduction_factor; - - -\echo '' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo ' DONE — share section 2 (headline) and section 3 (non-bundle detail)' -\echo ' to make the D-Prep PR scope decision.' -\echo '─────────────────────────────────────────────────────────────────────────' -\echo '' diff --git a/tools/d-prep-real-db-smoke.sh b/tools/d-prep-real-db-smoke.sh deleted file mode 100755 index 6ded56ac..00000000 --- a/tools/d-prep-real-db-smoke.sh +++ /dev/null @@ -1,328 +0,0 @@ -#!/usr/bin/env bash -# ════════════════════════════════════════════════════════════════════════════ -# d-prep-real-db-smoke.sh — Validate D-Prep migration against sandbox data -# ════════════════════════════════════════════════════════════════════════════ -# Reads from your sandbox DB (real data — but it's a sandbox, not prod), dumps -# it, restores into an isolated parallel DB, applies the D-Prep migration -# (`20260730000000_role_grants.sql`), and verifies the backfill against the -# pre-recorded audit numbers from `tools/audit-grants-bundle-shape.sql`. -# -# The sandbox DB itself is NEVER MODIFIED — everything happens in the parallel -# `oxicloud_dprep_smoke` DB which is dropped at the start of each run. -# -# Usage: -# tools/d-prep-real-db-smoke.sh # uses $DATABASE_URL -# tools/d-prep-real-db-smoke.sh --keep # leave smoke DB for poking after -# tools/d-prep-real-db-smoke.sh --help -# -# Requirements: -# - pg_dump / pg_restore / psql (PostgreSQL 14+ should be fine; sandbox version-matched) -# - DATABASE_URL pointing at the sandbox DB -# -# PATH gotcha (Mac brew): `brew install postgresql@18` is keg-only; if pg_dump -# isn't on PATH, run: -# export PATH="$(brew --prefix postgresql@18)/bin:$PATH" -# -# Expected audit numbers (from your audit run on 2026-06-17): -# - 73 access_grants rows clustering into 38 (subject, resource) pairs -# - 29 viewer / 5 editor / 4 owner -# These are encoded as assertions below. If your sandbox data has shifted -# since the audit, update EXPECTED_* variables OR re-run the audit script -# first. -# -# Memory cross-refs: -# - bug_pg_dump_folders_circular_fk.md — circular FK on `folders` forces -# `pg_restore --disable-triggers` -# - project_drive_sequence_a.md — D-Prep scope + audit data -# ════════════════════════════════════════════════════════════════════════════ - -set -euo pipefail - - -# ── Config ────────────────────────────────────────────────────────────────── - -SMOKE_DB_NAME="oxicloud_dprep_smoke" -DUMP_FILE="${TMPDIR:-/tmp}/oxicloud-sandbox-${SMOKE_DB_NAME}.dump" -MIGRATION_FILE="migrations/20260730000000_role_grants.sql" - -# Expected values from the audit (tools/audit-grants-bundle-shape.sql run -# on 2026-06-17). Bump these if you re-run the audit and see different -# numbers — they are intentionally HARDCODED so a silent drift gets caught. -EXPECTED_DISTINCT_CLUSTERS=38 -EXPECTED_VIEWER=29 -EXPECTED_EDITOR=5 -EXPECTED_OWNER=4 - -KEEP_DB=0 - - -# ── Arg parsing ───────────────────────────────────────────────────────────── - -usage() { - sed -n '2,/^$/p' "$0" | sed 's/^# \{0,1\}//' - exit "${1:-0}" -} - -for arg in "$@"; do - case "$arg" in - --keep|--keep-db) KEEP_DB=1 ;; - -h|--help) usage 0 ;; - *) echo "Unknown arg: $arg" >&2 ; usage 2 ;; - esac -done - - -# ── Pre-flight ────────────────────────────────────────────────────────────── - -if [[ -z "${DATABASE_URL:-}" ]]; then - echo "ERROR: \$DATABASE_URL must be set and point at the sandbox DB." >&2 - exit 2 -fi - -for bin in pg_dump pg_restore psql; do - if ! command -v "$bin" >/dev/null 2>&1; then - echo "ERROR: '$bin' not found on PATH." >&2 - echo "Hint: brew-installed postgresql is keg-only. Run:" >&2 - echo " export PATH=\"\$(brew --prefix postgresql@18)/bin:\$PATH\"" >&2 - exit 2 - fi -done - -if [[ ! -f "$MIGRATION_FILE" ]]; then - echo "ERROR: migration file not found: $MIGRATION_FILE" >&2 - echo "Run this script from the repo root." >&2 - exit 2 -fi - -# Derive connection URLs for the maintenance DB (where we'll issue -# DROP/CREATE DATABASE) and the smoke DB itself, by splicing -# $SMOKE_DB_NAME into $DATABASE_URL. Avoids relying on libpq's -# default local Unix socket — `dropdb`/`createdb`/`psql` without an -# explicit -d won't read $DATABASE_URL and try /tmp/.s.PGSQL.5432 -# (which fails on Mac brew where the server runs on a non-default -# socket / port). -URL_NO_QUERY="${DATABASE_URL%%\?*}" -URL_QUERY="${DATABASE_URL:${#URL_NO_QUERY}}" # "" or "?…" -SOURCE_DB_NAME="${URL_NO_QUERY##*/}" -URL_BASE="${URL_NO_QUERY%/"$SOURCE_DB_NAME"}" -MAINTENANCE_URL="${URL_BASE}/postgres${URL_QUERY}" -SMOKE_URL="${URL_BASE}/${SMOKE_DB_NAME}${URL_QUERY}" - -if [[ "$SOURCE_DB_NAME" == "$SMOKE_DB_NAME" ]]; then - echo "ERROR: DATABASE_URL appears to point at '$SMOKE_DB_NAME' — refusing" >&2 - echo "to dump-and-restore on top of itself. Set DATABASE_URL to the sandbox." >&2 - exit 2 -fi - - -# ── Helpers ───────────────────────────────────────────────────────────────── - -log() { echo "[smoke] $*"; } -fail() { echo "[smoke] FAIL: $*" >&2 ; exit 1; } -pass() { echo "[smoke] PASS: $*"; } - -# Run a query against the smoke DB and capture a single scalar. -scalar() { - psql -tAX -d "$SMOKE_URL" -c "$1" -} - -# Run a query and check the result equals an expected scalar. -expect_scalar() { - local query="$1" expected="$2" label="$3" - local actual - actual=$(scalar "$query") - if [[ "$actual" == "$expected" ]]; then - pass "$label: $actual" - else - fail "$label: expected $expected, got '$actual'" - fi -} - - -# ── 1. Snapshot the sandbox ───────────────────────────────────────────────── - -log "Dumping sandbox DB → $DUMP_FILE" -log "(custom format, so pg_restore --disable-triggers can side-step the" -log " circular FK on storage.folders.parent_id — see bug memory.)" -pg_dump "$DATABASE_URL" --format=custom --no-owner --no-privileges \ - --file="$DUMP_FILE" -log "Dumped $(du -h "$DUMP_FILE" | awk '{print $1}')" - - -# ── 2. Recreate the smoke DB ──────────────────────────────────────────────── - -log "Dropping smoke DB '$SMOKE_DB_NAME' if it exists" -psql -d "$MAINTENANCE_URL" -c "DROP DATABASE IF EXISTS \"$SMOKE_DB_NAME\"" - -log "Creating fresh smoke DB '$SMOKE_DB_NAME'" -psql -d "$MAINTENANCE_URL" -c "CREATE DATABASE \"$SMOKE_DB_NAME\"" - - -# ── 3. Restore sandbox into smoke DB ──────────────────────────────────────── - -log "Restoring sandbox dump into smoke DB" -# --disable-triggers handles the storage.folders parent_id circular FK without -# wrapping every COPY in SET CONSTRAINTS ALL DEFERRED. -# --no-owner --no-privileges already on dump side; --single-transaction makes -# the restore atomic so a mid-flight failure leaves no half-state. -pg_restore --dbname="$SMOKE_URL" --disable-triggers \ - --single-transaction --no-owner --no-privileges \ - "$DUMP_FILE" - - -# ── 4. Wipe any prior role_grants state from the smoke DB ────────────────── -# The sandbox may already have role_grants if the server has been booted -# with the D-Prep migration applied (auto-migrate on startup). For the -# smoke we want to exercise the migration FRESHLY — same as a brand-new -# install — so drop both the migration's tables and let it recreate them -# from scratch against the access_grants snapshot. - -log "Wiping any prior role_grants state from smoke DB (so we test the migration freshly)" -psql -d "$SMOKE_URL" --set ON_ERROR_STOP=1 -c " -DROP TABLE IF EXISTS storage.role_grants_migration_log CASCADE; -DROP TABLE IF EXISTS storage.role_grants CASCADE; -" - -# ── 5. Pre-migration sanity ──────────────────────────────────────────────── - -log "Pre-migration sanity checks" -expect_scalar "SELECT count(*) FROM storage.access_grants" \ - "73" "access_grants row count matches audit" - -if [[ $(scalar "SELECT to_regclass('storage.role_grants') IS NULL") != "t" ]]; then - fail "storage.role_grants survived the drop — something is wrong." -fi -pass "storage.role_grants does not exist (clean slate)" - - -# ── 5. Apply the D-Prep migration ─────────────────────────────────────────── - -log "Applying D-Prep migration: $MIGRATION_FILE" -# Wrap in a transaction so a constraint failure rolls back cleanly; the -# migration itself has BEGIN/COMMIT semantics via -1 to psql. -psql -d "$SMOKE_URL" --set ON_ERROR_STOP=1 -1 -f "$MIGRATION_FILE" -log "Migration applied" - - -# ── 6. Post-migration assertions ──────────────────────────────────────────── - -log "Post-migration verification" - -# Row count: should match `distinct_clusters` from the audit -expect_scalar "SELECT count(*) FROM storage.role_grants" \ - "$EXPECTED_DISTINCT_CLUSTERS" "role_grants row count" - -# Role distribution -expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role = 'viewer'" \ - "$EXPECTED_VIEWER" "viewer count" -expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role = 'editor'" \ - "$EXPECTED_EDITOR" "editor count" -expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role = 'owner'" \ - "$EXPECTED_OWNER" "owner count" - -# Zero NULL roles, zero non-bundle roles (the CHECK constraint should -# already enforce this, but proving it here too) -expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role IS NULL" \ - "0" "no NULL roles" -expect_scalar "SELECT count(*) FROM storage.role_grants WHERE role NOT IN ('viewer','commenter','contributor','editor','owner')" \ - "0" "no unknown roles" - -# access_grants is untouched -expect_scalar "SELECT count(*) FROM storage.access_grants" \ - "73" "access_grants row count unchanged (dual-write safety net intact)" - - -# ── 7. Equivalence check (strongest assertion) ────────────────────────────── -# For every role_grants row, the corresponding (subject, resource) cluster -# in access_grants must have exactly the role's bundle as its permission -# set. If any row's bundle doesn't match what `Role::expand()` says, the -# backfill mis-mapped and the equivalence count goes non-zero. - -log "Bundle equivalence check: role_grants ↔ access_grants" - -EQUIVALENCE_MISMATCHES=$(scalar " -WITH expected AS ( - SELECT 'viewer'::text AS role, ARRAY['read']::text[] AS perms - UNION ALL SELECT 'commenter', ARRAY['comment','read']::text[] - UNION ALL SELECT 'contributor', ARRAY['create','read']::text[] - UNION ALL SELECT 'editor', ARRAY['comment','create','read','update']::text[] - UNION ALL SELECT 'owner', ARRAY['comment','create','delete','read','share','update']::text[] -), -actual AS ( - SELECT subject_type, subject_id, resource_type, resource_id, - array_agg(permission ORDER BY permission) AS perms - FROM storage.access_grants - GROUP BY 1, 2, 3, 4 -) -SELECT count(*) -FROM storage.role_grants rg -JOIN actual a USING (subject_type, subject_id, resource_type, resource_id) -JOIN expected e ON e.role = rg.role -WHERE a.perms <> e.perms -") - -if [[ "$EQUIVALENCE_MISMATCHES" == "0" ]]; then - pass "every role_grants row's bundle matches access_grants exactly" -else - log "" - log "MISMATCH DETAIL (first 10):" - psql -d "$SMOKE_URL" -c " -WITH expected AS ( - SELECT 'viewer'::text AS role, ARRAY['read']::text[] AS perms - UNION ALL SELECT 'commenter', ARRAY['comment','read']::text[] - UNION ALL SELECT 'contributor', ARRAY['create','read']::text[] - UNION ALL SELECT 'editor', ARRAY['comment','create','read','update']::text[] - UNION ALL SELECT 'owner', ARRAY['comment','create','delete','read','share','update']::text[] -), -actual AS ( - SELECT subject_type, subject_id, resource_type, resource_id, - array_agg(permission ORDER BY permission) AS perms - FROM storage.access_grants - GROUP BY 1, 2, 3, 4 -) -SELECT rg.subject_type, rg.subject_id, rg.resource_type, rg.resource_id, - rg.role, - a.perms AS access_grants_perms, - e.perms AS expected_perms -FROM storage.role_grants rg -JOIN actual a USING (subject_type, subject_id, resource_type, resource_id) -JOIN expected e ON e.role = rg.role -WHERE a.perms <> e.perms -LIMIT 10 -" - fail "$EQUIVALENCE_MISMATCHES role_grants rows have a bundle that diverges from their access_grants cluster" -fi - - -# ── 8. Migration audit log shape ──────────────────────────────────────────── -# `role_grants_migration_log` is the one-shot table created by the migration -# to record any demotions. With 100%-bundle-shaped data (the audit says so), -# it should be EMPTY — nothing was demoted. - -if [[ $(scalar "SELECT to_regclass('storage.role_grants_migration_log') IS NOT NULL") == "t" ]]; then - expect_scalar "SELECT count(*) FROM storage.role_grants_migration_log" \ - "0" "migration audit log empty (no demotions, as expected for 100% bundle-shaped data)" -fi - - -# ── 9. Cleanup ────────────────────────────────────────────────────────────── - -log "" -log "─────────────────────────────────────────────────────────" -log " ALL ASSERTIONS PASSED" -log "─────────────────────────────────────────────────────────" -log "" - -if [[ "$KEEP_DB" == "1" ]]; then - log "Leaving smoke DB '$SMOKE_DB_NAME' for inspection." - log "Poke at it with:" - log " psql \"$SMOKE_URL\"" - log "" - log "Drop when done:" - log " psql -d \"$MAINTENANCE_URL\" -c 'DROP DATABASE \"$SMOKE_DB_NAME\"'" -else - log "Dropping smoke DB '$SMOKE_DB_NAME'" - psql -d "$MAINTENANCE_URL" -c "DROP DATABASE \"$SMOKE_DB_NAME\"" -fi - -log "Dump file kept at $DUMP_FILE (delete with: rm '$DUMP_FILE')"