feat: run-set access control and a browsable Sets hierarchy - #42
Merged
Conversation
Migration 14 widens the `users.role` CHECK to admit `viewer` and adds `run_set_restrictions` / `run_set_grants`, the two tables run-set visibility will be decided from. Both are inert here — nothing reads or writes them yet. Widening a CHECK means rebuilding `users`, which is the one table with `ON DELETE CASCADE` children. Doing that with foreign keys enforced destroys every session, API key and SSO identity on the instance, so the migrations now run with `foreign_keys=OFF` (SQLite's own rebuild recipe, step 1) — the pragma is a no-op inside a transaction, so it cannot live in the migration SQL itself. `Role::Viewer` maps to read scope, so a viewer reads runs, cannot write, cannot reach the admin API, and can only mint read-scoped API keys. The last-enabled-admin guard already counted admins rather than non-members, so demoting the last admin to viewer is refused like any other demotion; there is now a test that says so. `DOMARINN_SSO_DEFAULT_ROLE` lets an operator land SSO logins as viewers without touching the admin mapping.
The two `<option>` lists on the admin page were hand-written, so a role the server accepts but the UI never lists is simply unassignable — a silent failure that reads as "the role does not exist". Both now render `ALL_ROLES`, least privileged first, alongside `ALL_SCOPES`. The request mock coerced anything that was not `admin` to `member` on both the create and the patch path, which swallowed the new role and gave it write scope; it now keeps the role it was handed and mirrors the server's role-to-scope map.
Adds the access model behind run sets: ordered grant levels, the three identity classes a caller can fall into, and the SQL predicate every run query will append. The derivation from an Identity lives in exactly one place; a non-admin static token is deliberately Public, since a shared credential has no owning user and therefore no grants.
Threads a required RunVisibility through the run list, detail, export, config, cases, matrix, compare, history, search, project/suite catalogs, and the cache-entry run list, plus the MCP tools that share the same storage. An invisible run is indistinguishable from an absent one: the storage call returns None and the handler 404s, so no endpoint confirms that a restricted id is real. The cached=exclude count is computed after the visibility clause lands, so it cannot leak a count either.
POST /runs and the baseline PUT/DELETE now check the target (project, suite) against the caller's grants, on top of the write scope they already required. A 403 rather than a 404 here: the pair is caller-supplied, so refusing it discloses nothing, and a silent 404 on a legitimate upload would be a debugging trap.
The key endpoints move from a write gate to a read one. What protects them was never the route gate but the two per-request checks: require_user, which refuses any credential without an owning account, and the ceiling that caps a minted key at the caller's own scope. Both are untouched. In closed mode the write gate had locked a viewer out of minting the read-only key their role exists for.
Closed mode is where the key endpoints' read gate matters, so the ceiling test now runs there too — and asserts the minted key really is read-only. Folds in what a separate closed-mode test would have duplicated, keeping the file under the line cap.
Keeps the REST reference and the web mock handler in step with the server: the gate is read, and the account check plus the scope ceiling are what actually protect these routes.
…es on the run Two review findings. set_access waived every level on an unrestricted set, so any caller who could reach a write endpoint could have restricted a fresh project and granted themselves manage on it — the bootstrap step of the feature. The waiver now stops at upload; manage takes admin or a covering manage grant, which may sit dormant on an unrestricted set and still count. set_baseline read runs by id with no visibility and no check that the run belonged to the suite, then persisted the id where every reader of that suite would see it. It now requires the run to be visible AND to be of the target set, and the baseline reads filter by visibility too, so a row written by an earlier version cannot publish an invisible run's id. Splits the run-set tests along the seam they already had: policy storage in runset_policy.rs, route behaviour in runsets.rs.
The run-set browser needs per-project and per-suite totals, a recent pass-rate series, and the caller's own grant on each set. Add a storage module for those queries and the DTOs they fill. The aggregates live in their own module rather than in `storage/runs.rs` or `storage/projects.rs`: the first is at the per-file line ratchet, and the second serves `/api/v1/projects*`, whose wire shape older CLIs pin. Both list queries make a single window-function pass over the visible corpus, because the sets listing is unpaginated and a query per project would be N+1 over every project on the instance. `GrantLevel` gains a `TS` derive so the browser can name a level.
Adds `/api/v1/sets*`: the browsable project/suite tree, and the endpoints that read and edit a set's access list. All additive — `/api/v1/projects*` is untouched, byte for byte, because older CLIs pin its shape. Three gates, three status codes, and the difference is deliberate. The browse reads are visibility-filtered, so an invisible set 404s exactly like one that never existed. The access list needs a covering `manage` grant and *also* 404s on refusal: it names the users who can reach a restricted set, so "you may not see this" and "there is nothing here" have to be the same answer. Restriction toggling is admin-only and 403s a manage-grant holder, which is safe because holding that grant already proved they know the set exists. The grant mutations sit at `Scoped<Read>` rather than `Scoped<Write>`: the covering manage grant is the real gate and is strictly stronger — no auth mode hands it out, and the default-open waiver stops below it — so adding a write-scope check would only strip capability from a viewer-role account deliberately given `manage` over one set. Grants and restrictions never require the set to exist in `runs`, so access can be provisioned before the first upload. The browser itself stays runs-derived, so such a set has nothing to show until then. `require_set_access` now takes the level it demands instead of hardcoding `upload`, and says so in its refusal.
`SetAccessResponse.restricted` was the covering answer, so a suite inside a restricted project claimed a restriction it does not hold — and the Remove button beside it would have 404'd, because the row it deletes is the project's. The access panel is an exact-scope editor throughout: `list_run_set_grants` already answers "who is on *this* set's list" rather than "who can reach it". `restricted` now matches. The browse views keep the covering answer, which is what a reader needs.
The four grant-mutation routes sat at `Scoped<Read>`, on the reasoning that the covering manage grant was a strictly stronger gate. It is not, and the two gates are not comparable at all: a grant belongs to a *user*, and `RunVisibility::of` maps every user-backed credential to `User(user_id)` regardless of the scope it was minted at. So a `read` API key — the least privileged credential the product offers — inherited its owner's manage grant and could PUT a `manage` grant for any account. A leaked read-only key meant "install a persistent foothold on a restricted set", not "read what I can read". The mutations now require `write`. Delegation is unaffected: manage-grant holders are members, which is write scope. A viewer-role account cannot mutate policy even when holding a manage grant, which is what a viewer role is for — and it may still read the panel, since `GET .../access` stays at `read`. The module header claimed static-token callers are refused unconditionally. That was wrong in the other direction: a `Scope::Admin` static token resolves to `RunVisibility::Full` and manages every set, as this suite's own admin-token case already showed. It now names the real invariant and tabulates both gates per endpoint. Also stop `project_has_visible_runs` collapsing every storage error into "no such set" — a locked or corrupt database was answering 404 instead of 500. `.optional()?` like every sibling predicate.
The trail above a drill-down page had one ad-hoc implementation (RunDetail's `Runs / project / suite` line) and no semantics: three spans and two slashes, with nothing telling a screen reader it was navigation or which crumb was the page itself. The primitive treats the last item as the current page — text, never a link, with `aria-current="page"` — and hides the separators from assistive tech. Adopting it in the run header therefore renders the trailing suite in `fg` rather than `muted`; that is the whole visual delta.
Aggregates over the same generated runs the rest of the mock serves, plus mutable restriction and grant rows the access panel writes. The two properties worth pinning are the ones that are invisible once wrong: which sets a caller may see (an invisible set 404s through the same path as one that never existed, never a 403 — the access list names who can reach a restricted set), and that `restricted` is the COVERING answer while browsing but the EXACT one on the access payload, whose toggle owns that single row.
Everything nests under the `["sets"]` key, so one invalidate after a grant or a restriction change clears the listing, both detail levels and any open access panel — a restriction moves rows between all of them. A restriction change also invalidates `["runs"]`, because it changes which runs the caller may see. `isoFromEpoch` is the edge conversion for this surface: its timestamps are epoch-ms while every formatter in the module speaks RFC3339.
Three sibling routes behind the existing auth gate, plus a Sets nav item. No scope guard on any of them: the listing is readable by every authenticated view and the server filters it to what the caller may see, so a guard here would hide a page that has an answer for everyone. Two details are load-bearing. The suite rows draw their restricted chip from the suite's OWN lock, not the covering answer — inside a locked project the covering flag is true for every suite, and rendering it would repeat the heading's chip on every line. And the runs table is a deliberate copy of the runs list's rows: the runs list owns filtering and grouping this page does not have, and factoring them together would drag all of it in for no reader. `RateBadge` is the same pill for a payload carrying a rate rather than counts. The set's counts are lifetime totals while `latest_pass_rate` is one run's, so feeding the counts in would caption "latest" with a number over every run.
The access panel read "Restricted [restricted]" — the bold word and the chip beside it are one status, said twice. The listing's sparkline column was labelled "Pass rate", which is what the column beside a suite means but not what this one holds: it is one latest rate per suite, a spread across the project rather than a trend.
Covers the drill-down and the trail back up it, the covering-versus-exact restriction chips, the panel's add/re-level/remove cycle, and the two things a non-admin manager must and must not be offered. The locking test asserts on the LISTING after closing the panel: the panel's own query updating proves nothing about the pages behind it, and the whole point of nesting these keys is that one invalidate reaches all of them.
The panel read its status off the access payload, which is EXACT scope: the
restriction row the panel's own toggle writes. For a suite inside a locked
project — which owns no row of its own — that made the panel show an "open"
chip and assert "anyone who can read this server can see this set's runs"
over a set nobody outside the project's grants can see, directly under a page
heading correctly chipped "restricted".
The covering answer now comes down as a prop from the browse payload, which is
the only place that knows it, and drives the chip and the sentence. The exact
row keeps what it actually owns: the toggle's verb and the confirm copy. A
suite covered by its project says so, and names where the lock comes from.
The toggle verbs now name the scope they write ("Restrict suite" under a
locked project reads as adding a second lock, which is what it does), and the
row action is a bare "Remove" with the person in its aria-label, matching the
admin page.
The sets pages are a picture of policy, not of runs: with nothing restricted and nobody granted, every shot of them is an empty table. So the seed now provisions a small, realistic policy — one locked suite, one account at each grant level, and a pinned baseline on a second suite so the browser has one of each flag to show — and the capture specs stay read-only like every other shot they take. The policy block sits after the data-at-rest assertion on purpose. That check reads with a non-admin static token, which by design cannot see a restricted set, so restricting first would silently shrink the set of runs it examines. `SHOTS=<regex>` re-captures one page without churning the other twenty-six PNGs' run ids and dates. It is read from the environment rather than passed as `--grep` because the flag also filters dependency projects, which would drop the login the capture project's session comes from. No shot of the `/sets` root: every shipped example declares `project: examples`, so the root listing is a one-row table that documents the seed rather than the page.
Four surfaces changed behaviour and none of them said so: every run read is now filtered by what the caller may see, uploading into a restricted set needs an upload grant, `viewer` is a third role, and there is a whole `/api/v1/sets*` surface with no reference page. The REST reference gets the endpoints plus the model behind them — default-open, restriction coverage, the three grant levels, why `manage` sits outside the waiver, and why the browse reads 404 where the upload gate 403s. The web UI page gets the walkthrough in its own voice, including the three limitations worth knowing before planning around the access panel. The server page gets the operator's view: run sets are the axis scopes are not, and a static token never pierces one, so CI into a restricted set wants a bot account's API key. `DOMARINN_SSO_DEFAULT_ROLE` is called out loudly: it is re-evaluated on every SSO login, so flipping it to `viewer` demotes accounts that have existed for months, one at a time, as they sign in. The upload gate's refusal is described rather than quoted — its wording has a grammar bug that is fixed separately.
Four accuracy fixes from review. An `admin:` static token resolves to full visibility (runsets.rs's RunVisibility::of) and pierces every restriction, but four pages said flatly that static tokens never do — one of them five lines under a table that says otherwise. An operator could read that and hand CI an admin token believing run sets contained it. Each claim is now scoped to the `read` and `write` tokens it is true of, and names the admin token as the operator credential it is. `restricted` means two different things and neither was written down: a suite's browse views answer "is this locked for me", covering a project-level row, while its access endpoint answers "does this scope carry a row of its own" and returns false for a suite its project locks. A project's browse `restricted` is its own row too, so a project whose suites are individually locked reads false. Both look like bugs until you know which question is being asked. Admins are never filtered by a grant, so "disappears for everyone except the accounts granted it" was three sentences short of true. The seeded policy put three accounts on the users page, so the admin capture is re-shot to match; overview and run-detail were checked and are unaffected by the seed, so they keep their existing captures. Plus: the baseline PUT's 200 body, `my_level`'s two null cases and its project-only scope on /sets rows, `$USER_ID`'s provenance in the cookbook, that a manage grant on an open set is live delegation rather than dormant, and a SHOTS comment that contradicted the config.
…ing storage errors Three refusals on the run-set access-control path were weaker than they read. An API key carried the scope it was minted at, forever. A demotion was therefore cosmetic: a member demoted to viewer kept writing with yesterday's `write` key, defeating the one guarantee the viewer role makes, and a demoted admin kept an `admin` key that could lift any set's restriction and then read its runs. Cap the minted scope by the owner's current role at authentication time, so a role change binds on the next request instead of on a manual sweep of every key ever issued. Five access-control-bearing queries answered `.is_ok()`/`.ok()`, collapsing "the database could not answer" into the same 404 as "there is nothing here". A locked database or an unreadable schema was reported to the caller as a missing run and to the operator as a clean 404 log. They now use `.optional()?`, the pattern already documented on `project_has_visible_runs`, so a real fault surfaces as a 500. The no-rows case is unchanged. The refusal body for a restricted upload read "you need a upload grant on it". Agree the article with the level.
The bot-account guidance told operators to create an account and grant it `upload`, without saying the account has to be a `member`: a viewer can only mint `read` keys, so a viewer bot is refused by the scope gate before its grant is ever consulted. The scope gate was documented as answering 403 "by the time it can matter", which got both the status and the ordering wrong. It is an extractor, so it runs before the set gate, and it answers 401 with no credential and 403 with a weak one. The no-leak property still holds, for a different reason worth stating: its answer is path-independent. Also: name `ProjectSetDetailResponse` as the type actually is; say that `/sets` lists projects with at least one visible run rather than every project the caller may see; warn that `open` mode grants the restriction endpoints to anonymous callers, so restricting a set there is a self-DoS that no grant can undo; note that every migration, including future ones, runs with foreign keys disabled; and soften the claim that a 403 on a named set discloses nothing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds per-user access control over sets of runs, plus a click-through Sets browser for the existing
project → suitehierarchy.Access model
project → suitetree every run already declares — no new entity, no curation.run_set_restrictions; a project-level row covers all its suites). Restricted sets are omitted from listings, and fetching an invisible run/set by id returns 404 — no existence leak.run_set_grants) give users leveled access:view < upload < manage.uploadgatesPOST /runsand baselines into restricted sets;managelets a delegate edit the set's access list (restriction toggling stays admin-only).manageis never free — even on unrestricted sets it requires an explicit grant or admin.viewerrole (admin | member | viewer): read-only accounts that can browse what they're granted and mint read-only API keys, but never upload or mutate policy.read/writeenv tokens see only unrestricted sets (anadmin:token sees everything). CI that needs a restricted project should use amemberbot account's API key.Enforcement
One derivation point (
RunVisibility::of) and one SQL predicate, applied to every run-reading surface: runs list (including the hidden-cached count), run detail and all child endpoints, search, compare, history, matrix, project/suite catalogs, cache-entry run links, and the MCP tools. Tested via a seven-caller visibility matrix that every surface projects.API
New
/api/v1/sets*endpoints: browse tree (project → suite summaries with pass-rate sparklines, baselines, restriction state) and access management (restriction PUT/DELETE, grant PUT/DELETE, access listing). All additive; the legacy/api/v1/projects*surface is untouched.UI
/sets→/sets/:project→/sets/:project/:suitedrill-down with a new Breadcrumb primitive, severity-aware tables, sparklines, and restricted chips.rest-api.mdauthz semantics,web-ui.mdwalkthrough,server.mdroles/SSO default role).Migration
Runs-DB migration 14: rebuilds
usersto admit theviewerrole (FK-safe, cascade-verified by test) and adds the two policy tables. Forward-only, no backfill; absence of rows = unrestricted.Known follow-ups (deliberately deferred)
load_run_blobstill swallows storage errors as 404 (fails closed; zero-line sweep)./setsuntil their first run arrives; documented.storage/runs.rssits exactly at the 1000-line ratchet; next touch should split it.GET /apikeyslists minted (not role-capped) scope — display-only inconsistency.Test plan
cargo test --workspace(1380+ tests, includes the visibility matrix, migration rebuild, demoted-key caps, storage-error propagation)mise run gen-types-check,schema-check,docs,web-lint,web-buildpnpm -C web test(512) andpnpm -C web test:e2e(104)mise run screenshots(new sets captures; existing PNGs untouched exceptadmin, re-captured for the new seed)musl-build— not runnable locally (no musl toolchain); verify in CI