diff --git a/web/src/components/app-sidebar.tsx b/web/src/components/app-sidebar.tsx index a23b407..647d40a 100644 --- a/web/src/components/app-sidebar.tsx +++ b/web/src/components/app-sidebar.tsx @@ -83,6 +83,12 @@ const accessItems: NavItem[] = [ icon: IconUsers, requiredPermissions: ["users:read"], }, + { + title: "Linked Repos", + url: "/dashboard/settings/linked-repos", + icon: IconLink, + requiredPermissions: ["linked-repos:read"], + }, { title: "API Keys", url: "/dashboard/settings/api-keys", @@ -143,12 +149,6 @@ const systemItems: NavItem[] = [ icon: IconArrowsShuffle, requiredPermissions: ["settings:manage"], }, - { - title: "Linked Repos", - url: "/dashboard/settings/linked-repos", - icon: IconLink, - requiredPermissions: ["linked-repos:read"], - }, { title: "ENV Variables", url: "/dashboard/settings/env-variables", @@ -205,7 +205,10 @@ export function AppSidebar({ ...props }: React.ComponentProps) { .then((info) => { if (!cancelled) { if (info.restart_recommended) { - addReason("backfill-pool", "Backfill pool is undersized for current concurrency settings."); + addReason( + "backfill-pool", + "Backfill pool is undersized for current concurrency settings.", + ); } else { removeReason("backfill-pool"); } diff --git a/web/src/components/linked-repos/link-repo-dialog.tsx b/web/src/components/linked-repos/link-repo-dialog.tsx index bd54146..71dbe4d 100644 --- a/web/src/components/linked-repos/link-repo-dialog.tsx +++ b/web/src/components/linked-repos/link-repo-dialog.tsx @@ -59,8 +59,16 @@ export function LinkRepoDialog({ const [lastAction, setLastAction] = useState(null); const [copied, setCopied] = useState(false); + // Builder-level problems the scope string can't express on its own: a `repo` + // row with no actions checked serialises to `repo:`, which is valid and + // grants *everything*. The builder reports it here rather than the string + // being re-read, since only the builder knows nothing was picked. + const [scopeSelectionError, setScopeSelectionError] = useState( + null, + ); + const handleTrimmed = handle.trim(); - const scopeError = validateScopeString(scopes); + const scopeError = validateScopeString(scopes) ?? scopeSelectionError; // The builder always emits `atproto`, so a non-empty scope string no longer // means the operator picked anything — keep a grant that can do nothing from // being created by merely opening this dialog. Once the grant exists its @@ -77,6 +85,7 @@ export function LinkRepoDialog({ setHandle(""); setReason(""); setScopes(""); + setScopeSelectionError(null); setRepoId(null); setInviteResult(null); setSubmitting(null); @@ -222,6 +231,7 @@ export function LinkRepoDialog({ key={formKey} value={scopes} onChange={setScopes} + onValidityChange={setScopeSelectionError} /> )} diff --git a/web/src/components/linked-repos/scope-builder.tsx b/web/src/components/linked-repos/scope-builder.tsx index 3a26d18..d612d41 100644 --- a/web/src/components/linked-repos/scope-builder.tsx +++ b/web/src/components/linked-repos/scope-builder.tsx @@ -1,9 +1,11 @@ "use client"; -import { useMemo, useState } from "react"; +import { useEffect, useMemo, useState } from "react"; import { AlertTriangle, X } from "lucide-react"; import { Button } from "@/components/ui/button"; +import { Checkbox } from "@/components/ui/checkbox"; +import { Label } from "@/components/ui/label"; import { Combobox, ComboboxContent, @@ -13,6 +15,22 @@ import { ComboboxList, } from "@/components/ui/combobox"; import { Input } from "@/components/ui/input"; +import { + InputGroup, + InputGroupAddon, + InputGroupInput, +} from "@/components/ui/input-group"; + +/** + * One shared column geometry for every row: prefix, value, remove. + * + * The third column is declared even on the trailing row that has no remove + * button, so the value inputs all end at the same x. Secondary content — + * checkboxes, errors, notes — sits at `col-start-2` and therefore lines up + * under the value it describes instead of hanging off the left edge. + */ +const ROW_GRID = + "grid grid-cols-[11rem_minmax(0,1fr)_2.25rem] items-start gap-x-2 gap-y-1"; // Mirrors src/linked_repos/scope.rs byte-for-byte. Keep the two in sync — // this is the client-side half of the same grammar the server enforces. @@ -24,6 +42,17 @@ function isRepoAction(value: string): value is RepoAction { return (REPO_ACTIONS as readonly string[]).includes(value); } +/** + * Normalise a set of actions to declaration order, dropping duplicates. + * + * The emitted string is then a function of *what* was picked and never of the + * order the boxes happened to be clicked in, so two operators granting the same + * access produce byte-identical scopes. + */ +function orderActions(actions: readonly string[]): RepoAction[] { + return REPO_ACTIONS.filter((a) => actions.includes(a)); +} + // Same regex atrium_api::types::string::Nsid::new() compiles server-side. const NSID_RE = /^[a-zA-Z]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)+(\.[a-zA-Z][a-zA-Z0-9]{0,62})$/; @@ -68,7 +97,10 @@ function validateBlobRest(rest: string): string | null { * it validates strictly; the rest are shape-checked only (non-empty). */ export const SCOPE_PREFIXES = [ - { value: "repo", hint: "com.example.note?action=create,update" }, + // No `?action=` in the repo hint: the checkboxes own the action list, and a + // placeholder advertising the query invites typing a second copy of it. + // Pasting one still works — `hoistRepoActions` lifts it into the boxes. + { value: "repo", hint: "com.example.note" }, { value: "blob", hint: "image/* or */*" }, { value: "rpc", hint: "* or com.example.doThing" }, { value: "identity", hint: "*" }, @@ -137,7 +169,31 @@ export function validateScopeString(value: string): string | null { interface ScopeRow { prefix: string; + /** For `repo` rows this is the target alone; the query lives in `actions`. */ rest: string; + /** Only meaningful for `repo` rows. */ + actions: RepoAction[]; +} + +const EMPTY_ROW: ScopeRow = { prefix: "", rest: "", actions: [] }; + +/** + * Pull an `?action=` query out of a `repo` value into the checkbox state. + * + * The value field is the target alone, but operators paste whole scope strings + * into it, so a pasted query is hoisted into the boxes rather than left to + * produce a second, invisible source of truth. Returns `null` when there is + * nothing to hoist or the query is malformed — malformed text stays in the + * field so the row error can explain what is wrong with it. + */ +function hoistRepoActions( + value: string, +): { rest: string; actions: RepoAction[] } | null { + const [target, query] = splitQuery(value); + if (query === null || !query.startsWith("action=")) return null; + const actions = query.slice("action=".length).split(",").filter(Boolean); + if (actions.length === 0 || !actions.every(isRepoAction)) return null; + return { rest: target, actions: orderActions(actions) }; } /** Split a stored scope string back into editable rows, dropping `atproto`. */ @@ -147,12 +203,19 @@ function parseInitialRows(value: string): ScopeRow[] { if (token === BASE_SCOPE) continue; const sep = token.indexOf(":"); if (sep === -1) { - rows.push({ prefix: token, rest: "" }); - } else { - rows.push({ prefix: token.slice(0, sep), rest: token.slice(sep + 1) }); + rows.push({ ...EMPTY_ROW, prefix: token }); + continue; } + const prefix = token.slice(0, sep); + const rest = token.slice(sep + 1); + const hoisted = prefix === "repo" ? hoistRepoActions(rest) : null; + rows.push( + hoisted + ? { prefix, rest: hoisted.rest, actions: hoisted.actions } + : { ...EMPTY_ROW, prefix, rest }, + ); } - rows.push({ prefix: "", rest: "" }); + rows.push({ ...EMPTY_ROW }); return rows; } @@ -161,9 +224,46 @@ function rowToToken(row: ScopeRow): string { const rest = row.rest.trim(); if (!prefix && !rest) return ""; if (!prefix) return rest; + if (prefix === "repo") { + const actions = orderActions(row.actions); + // With no actions this is the bare form, which the server reads as *every* + // action. The builder never means that — `repoActionError` refuses the row + // — but it still has to render something for the preview and the error to + // describe. + return actions.length + ? `repo:${rest}?action=${actions.join(",")}` + : `repo:${rest}`; + } return `${prefix}:${rest}`; } +/** + * Builder-only rule: a `repo` row has to say which actions it grants. + * + * `repo:` with no query is valid to the server and grants create, update + * *and* delete. Inheriting that default here would make "checked nothing" mean + * "granted everything" — the opposite of what someone assembling a + * least-privilege grant intends, and unfixable afterwards because scopes are + * immutable. The grammar keeps accepting the bare form for scopes written by + * hand or through the API; the builder simply won't produce one. + */ +function repoActionError(row: ScopeRow): string | null { + if (row.prefix.trim() !== "repo") return null; + // Stay quiet until there is a target, matching how row errors ignore blanks. + if (!row.rest.trim()) return null; + if (orderActions(row.actions).length > 0) return null; + return "select at least one action"; +} + +/** `update` without `create` can't upsert — see the note this renders. */ +function cannotUpsert(row: ScopeRow): boolean { + return ( + row.prefix.trim() === "repo" && + row.actions.includes("update") && + !row.actions.includes("create") + ); +} + function buildScopeString(rows: ScopeRow[]): string { // `atproto` is not a choice, so it is not a row: emit it first and show it in // the preview, so what the operator reads is exactly what the grant will @@ -180,13 +280,24 @@ function buildScopeString(rows: ScopeRow[]): string { interface ScopeBuilderProps { value: string; onChange: (scopes: string) => void; + /** + * Reports builder-level problems the scope string can't express — currently + * a `repo` row with no actions picked, which reads as a valid (and maximally + * permissive) scope once serialised. Pass a stable function; a `useState` + * setter is ideal. + */ + onValidityChange?: (error: string | null) => void; } // Note: `value` only seeds the builder's initial state on mount — after that, // this component is the source of truth and pushes every change back through // onChange. Callers that need a hard reset (e.g. reopening a dialog) should // remount via `key`, rather than relying on later `value` prop changes. -export function ScopeBuilder({ value, onChange }: ScopeBuilderProps) { +export function ScopeBuilder({ + value, + onChange, + onValidityChange, +}: ScopeBuilderProps) { // Lazy initializer: `value` seeds the rows once on mount and is not read // again — see the note above about remounting via `key` for a hard reset. const [rows, setRows] = useState(() => parseInitialRows(value)); @@ -198,14 +309,46 @@ export function ScopeBuilder({ value, onChange }: ScopeBuilderProps) { const trimmed = [...next]; const last = trimmed[trimmed.length - 1]; if (!last || last.prefix.trim() !== "" || last.rest.trim() !== "") { - trimmed.push({ prefix: "", rest: "" }); + trimmed.push({ ...EMPTY_ROW }); } setRows(trimmed); onChange(buildScopeString(trimmed)); } function setRow(index: number, patch: Partial) { - update(rows.map((r, i) => (i === index ? { ...r, ...patch } : r))); + update( + rows.map((r, i) => { + if (i !== index) return r; + const next = { ...r, ...patch }; + // Actions belong to `repo` alone, so leaving it drops them rather than + // letting a stale set reappear if the operator switches back. + if (patch.prefix !== undefined && patch.prefix.trim() !== "repo") { + next.actions = []; + } + return next; + }), + ); + } + + /** Value-field edits, hoisting a pasted `?action=` into the checkboxes. */ + function setRest(index: number, raw: string) { + if (rows[index]?.prefix.trim() === "repo") { + const hoisted = hoistRepoActions(raw); + if (hoisted) { + setRow(index, { rest: hoisted.rest, actions: hoisted.actions }); + return; + } + } + setRow(index, { rest: raw }); + } + + function toggleAction(index: number, action: RepoAction, checked: boolean) { + const current = rows[index]?.actions ?? []; + setRow(index, { + actions: orderActions( + checked ? [...current, action] : current.filter((a) => a !== action), + ), + }); } function removeRow(index: number) { @@ -215,9 +358,14 @@ export function ScopeBuilder({ value, onChange }: ScopeBuilderProps) { const rowErrors = rows.map((row) => { const token = rowToToken(row); if (!token) return null; - return validateScopeToken(token); + return validateScopeToken(token) ?? repoActionError(row); }); + const firstError = rowErrors.find((e) => e != null) ?? null; + useEffect(() => { + onValidityChange?.(firstError); + }, [firstError, onValidityChange]); + return (
@@ -225,18 +373,21 @@ export function ScopeBuilder({ value, onChange }: ScopeBuilderProps) {

What this grant may do. Pick a prefix and fill in the rest — for example repo +{" "} - com.example.note?action=create. + com.example.note, then the actions + it may perform on that collection.

-
+
{/* `atproto` is mandatory in every authorization request, so it is - shown as a fixed row rather than something to add or remove. */} -
+ shown as a fixed row rather than something to add or remove. It + has no prefix, so it spans the prefix and value columns and ends + flush with every other input. */} +
@@ -244,60 +395,130 @@ export function ScopeBuilder({ value, onChange }: ScopeBuilderProps) { const known = SCOPE_PREFIXES.find((p) => p.value === row.prefix); const isLast = index === rows.length - 1; return ( -
-
- { - if (details.reason === "input-change") { - setRow(index, { prefix: v }); - } - }} - onValueChange={(v) => { - if (typeof v === "string") setRow(index, { prefix: v }); - }} - > - + { + if (details.reason === "input-change") { + setRow(index, { prefix: v }); + } + }} + onValueChange={(v) => { + if (typeof v === "string") setRow(index, { prefix: v }); + }} + > + + + No matching prefix + + {(item: string) => ( + + {item} + + )} + + + + + {/* `repo` is the one prefix whose value has structure, so its + collection and its actions share a single bordered field. + Sitting inside the same box (and the same focus ring) is + what ties the checkboxes to the collection they apply to — + below-and-indented alone reads as a sibling row. */} + {row.prefix.trim() === "repo" ? ( + + setRest(index, e.target.value)} + placeholder={known?.hint ?? "value"} + aria-label={`Scope ${index + 1} value`} + className="font-mono text-sm" /> - - No matching prefix - - {(item: string) => ( - - {item} - - )} - - - - + e.stopPropagation()} + > +
+ + Scope {index + 1} actions + + {REPO_ACTIONS.map((action) => { + const id = `scope-${index}-${action}`; + return ( +
+ + toggleAction(index, action, checked === true) + } + /> + {/* The addon renders its contents muted, which + suits hint text but not a control label — + these are interactive and carry the same + weight as the value above them. */} + +
+ ); + })} +
+ + + ) : ( setRow(index, { rest: e.target.value })} + onChange={(e) => setRest(index, e.target.value)} placeholder={known?.hint ?? "value"} aria-label={`Scope ${index + 1} value`} className="font-mono text-sm" /> + )} + + {/* The cell is declared by the grid either way, so the trailing + row's input still ends where every other one does. */} + {!isLast && ( + + )} - {!isLast && ( - - )} -
{rowErrors[index] && ( -

{rowErrors[index]}

+

+ {rowErrors[index]} +

+ )} + + {cannotUpsert(row) && ( +

+ update without{" "} + create can't{" "} + put_record, which upserts + — unless the script always passes a{" "} + swap_cid. +

)}
); @@ -317,15 +538,6 @@ export function ScopeBuilder({ value, onChange }: ScopeBuilderProps) {
)} -

- put_record upserts, so it needs both{" "} - create and{" "} - update on a collection unless the - script always passes a swap_cid{" "} - (which needs only update). Granting - only update creates a grant that - can't upsert on its own. -

diff --git a/web/tests/e2e/linked-repos.spec.ts b/web/tests/e2e/linked-repos.spec.ts index 9d599f4..331c468 100644 --- a/web/tests/e2e/linked-repos.spec.ts +++ b/web/tests/e2e/linked-repos.spec.ts @@ -205,4 +205,82 @@ test.describe("Linked Repos", () => { await expect(dialog.getByText(/grants broad write access/i)).toBeVisible() await expect(createLinkButton).toBeEnabled() }) + + test("builds repo actions from checkboxes", async ({ page }) => { + await page.goto("/dashboard/settings/linked-repos") + await page.getByRole("button", { name: "Link a repo" }).click() + + const dialog = page.getByRole("dialog") + await expect(dialog).toBeVisible({ timeout: 3000 }) + + const createLinkButton = dialog.getByRole("button", { name: "Create link" }) + await dialog.getByLabel("Scope 1 prefix").fill("repo") + await dialog.getByLabel("Scope 1 value").fill(TEST_NSID) + + // A collection with no actions serialises to `repo:`, which the + // server reads as create *and* update *and* delete. The builder refuses to + // emit that by accident — scopes are immutable, so an over-broad grant made + // by not choosing can only be fixed by re-linking the repo. + await expect(dialog.getByText("select at least one action")).toBeVisible() + await expect(createLinkButton).toBeDisabled() + + await dialog.getByLabel("create", { exact: true }).check() + await expect( + dialog.getByText(`atproto repo:${TEST_NSID}?action=create`), + ).toBeVisible() + await expect(createLinkButton).toBeEnabled() + + // Actions serialise in declaration order, not the order they were clicked, + // so the same access always produces the same string. + await dialog.getByLabel("delete", { exact: true }).check() + await dialog.getByLabel("update", { exact: true }).check() + await expect( + dialog.getByText(`atproto repo:${TEST_NSID}?action=create,update,delete`), + ).toBeVisible() + + // `update` without `create` can't upsert — flagged on the row that has the + // problem rather than as a general note nobody reads. + await dialog.getByLabel("create", { exact: true }).uncheck() + await dialog.getByLabel("delete", { exact: true }).uncheck() + await expect(dialog.getByText(/can't/).first()).toBeVisible() + await expect( + dialog.getByText(`atproto repo:${TEST_NSID}?action=update`), + ).toBeVisible() + // Still a legitimate grant, just a narrow one — warned about, not blocked. + await expect(createLinkButton).toBeEnabled() + }) + + test("hoists a pasted action query into the checkboxes", async ({ page }) => { + await page.goto("/dashboard/settings/linked-repos") + await page.getByRole("button", { name: "Link a repo" }).click() + + const dialog = page.getByRole("dialog") + await expect(dialog).toBeVisible({ timeout: 3000 }) + + const value = dialog.getByLabel("Scope 1 value") + await dialog.getByLabel("Scope 1 prefix").fill("repo") + + // Pasting a whole scope keeps working: the query is lifted into the boxes + // and the field is left holding the collection alone, so there's never a + // second, invisible copy of the action list. + await value.fill(`${TEST_NSID}?action=create,update`) + await expect(value).toHaveValue(TEST_NSID) + await expect(dialog.getByLabel("create", { exact: true })).toBeChecked() + await expect(dialog.getByLabel("update", { exact: true })).toBeChecked() + await expect(dialog.getByLabel("delete", { exact: true })).not.toBeChecked() + await expect( + dialog.getByText(`atproto repo:${TEST_NSID}?action=create,update`), + ).toBeVisible() + + // A malformed action list is left in the field so the error can name it, + // rather than being silently swallowed by the checkboxes. + await value.fill(`${TEST_NSID}?action=frobnicate`) + await expect(value).toHaveValue(`${TEST_NSID}?action=frobnicate`) + await expect( + dialog.getByText("unknown repo action: frobnicate"), + ).toBeVisible() + await expect( + dialog.getByRole("button", { name: "Create link" }), + ).toBeDisabled() + }) })