Skip to content

Treat ACL evaluate failure as error, not deny - #601

Merged
TheGreatAxios merged 2 commits into
mainfrom
cl-6829-settings-acl-evaluate-failure-removes-nav-as-if-denied
Sep 4, 2026
Merged

TheGreatAxios merged 2 commits into
mainfrom
cl-6829-settings-acl-evaluate-failure-removes-nav-as-if-denied

Conversation

@TheGreatAxios

Copy link
Copy Markdown
Contributor

Settings nav probes evaluate to decide whether People, Roles, Grants, and Credentials belong in the list. A network or 5xx failure was mapped to denied, so those sections vanished as if the principal were unauthorized.

This adds a distinct error access state. Authenticated deny (effect other than allow) still withholds gated sections. A thrown probe does not: last-known allow/deny is kept across a failed refetch, and a first-load failure marks a could-not-check state in the Shared Settings group instead of looking like a deny.

Fixes CL-6829

Evaluate network and 5xx failures currently collapse to denied, so
People/Roles/Grants/Credentials vanish as if the principal were
unauthorized. These tests pin catch → error, last-known coalesce, and
a registry flag that is not an authenticated deny.
A thrown evaluate probe (network, 5xx) is no longer mapped to denied. The registry still withholds gated sections while loading or denied, keeps last-known allow/deny across a failed refetch, and marks a could-not-check state so the nav does not vanish as if unauthorized.
@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Self-review

Verdict: the ACL evaluate/deny/error split is correctly implemented — deny stays a gate, a thrown probe becomes error, coalescing preserves last-known allow/deny across a failed refetch, and the nav surfaces a couldn't-check hint instead of hiding sections as unauthorized. No defects found; nothing changed.

  • Registry accessProbeFailed flag / gating logic in resolveSettingsSectionGroups: DISMISSED — correctly withholds gated sections on error (never flashes) and only flags the group, verified against section-registry.test.tsx's mixed-error case.
  • coalesceTenancyAccess in apps/web/src/settings-access.ts reads previous from the query cache before the new fetch resolves: DISMISSED — correct ordering, confirmed by reading the queryFn closure.
  • report-error-ignore comment on the moved catch in packages/settings-ui/src/access.ts, baseline entry removed for the old apps/web/src/settings-access.ts line: DISMISSED — matches the ledger's shrink-only rule, the violation moved to a covered ignore rather than being silently dropped.
  • No exhaustive switch elsewhere in the codebase assumes the old 3-state SectionAccess union (checked via grep across packages/settings-ui/src and apps/web/src): DISMISSED — no such site exists.

Targeted tests run:

  • packages/settings-ui: bun test test/access.test.ts test/section-registry.test.tsx — 21 pass, 0 fail
  • apps/web: bun test test/settings-access.test.tsx test/settings-page.test.tsx — 8 pass, 0 fail
  • bun run typecheck (repo root) — clean

@linear-code

linear-code Bot commented Sep 4, 2026

Copy link
Copy Markdown

CL-6829

@TheGreatAxios
TheGreatAxios merged commit 55ec971 into main Sep 4, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant