Skip to content

Gate rendering on the identity a caller actually has - #2536

Merged
woksin merged 4 commits into
mainfrom
feat/imp-41-gate-rendering-on-identity
Aug 12, 2026
Merged

Gate rendering on the identity a caller actually has#2536
woksin merged 4 commits into
mainfrom
feat/imp-41-gate-rendering-on-identity

Conversation

@woksin

@woksin woksin commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Added

woksin and others added 4 commits August 12, 2026 23:54
An application gating routes on roles had to hand-roll the guard, and every
hand-rolled version inherits the same flaw: isSet reads false both before the
first identity fetch answers and after it answers anonymous. Nothing could tell
those apart, so a guard either renders nothing forever for a genuinely anonymous
caller, or flashes the signed-out view at a signed-in one.

Add isLoading to the React identity context to separate the two, and a
RequireRole component that renders one of three outcomes from it. Access is
expressed as roles, a predicate over details, or both - both must pass when both
are given. The forbidden and loading outcomes are slots rather than a redirect,
because Arc.React does not depend on a router; an application passes its own
navigation element.

isLoading lives on IIdentityContext rather than IIdentity so the
framework-agnostic package keeps no React lifecycle concern.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: 49eab3a7-0743-4895-8303-bf5632b6a992
Two independent reviews of the previous commit found the loading flag was
only ever seeded, never re-raised. Nothing re-entered loading after mount, so
during a refresh after sign-in the context read "not loading, not signed in"
for the whole round-trip and a gate rendered its forbidden slot - navigating
away, for an application that put a redirect there. The flash the flag exists
to prevent was moved from first paint to refresh rather than removed.

Raise it before every identity request, and lower it again on the paths that
end one, including a refresh that rejects.

RequireRole now denies instead of admitting when it is given no access rule at
all. A renamed configuration key yields undefined, which read as "no roles
required" and rendered guarded content to any authenticated caller; the props
now require roles or a predicate at the type level, and deny with a warning at
runtime, because undefined defeats the type. A roles that is not an array, an
identity carrying no details, and a predicate that throws all deny for the same
reason: a gate that cannot evaluate its rule has not decided yes.

isLoading is optional where the provider value reaches consumers and required
where the identity is read. The value shape travels out through the exported
context, so requiring it there would stop a hand-built test or Storybook
context from compiling, while a consumer reading the identity should never have
to handle its absence. Absent reads as resolved - a hand-built context makes a
gate deny rather than admit.

Specs for refresh, for clearing the identity, and for a fetch that fails: all
three were free variables that no assertion could observe. Three existing
refresh specs turned out to exercise nothing - one had its refresh call
commented out while keeping the name - and are repaired here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: 49eab3a7-0743-4895-8303-bf5632b6a992
The guard that stops a predicate from deciding on details it does not have
tested for undefined only. A server answering with an explicit null for details
produces null, not undefined, so the null reached the predicate - and a
predicate phrased as an absence reads null as innocence and admits. An identity
the application could say nothing about therefore passed a rule meant to
inspect it.

Treat both shapes as nothing to decide on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: 49eab3a7-0743-4895-8303-bf5632b6a992
The provider value carries the loading flag as optional so a context composed
by hand - a spec, a Storybook decorator - still compiles, and useIdentity fills
in false for one that omits it. Absent has to read as resolved: reading it as
still-fetching would hold a gate on its loading slot forever for an identity
that is in fact settled.

That fallback had no spec. Inverting it left the whole suite green, which is
exactly the shape of thing this argues for in prose and never checked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: 49eab3a7-0743-4895-8303-bf5632b6a992
@woksin woksin added the minor label Aug 12, 2026
@woksin

woksin commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Split out of #2531, which bundled this with command-form execution state. That half returned do-not-ship on independent review and needs redesign rather than patching; this half was reviewed twice, is independent of it, and should not wait behind it.

Access control. Every path that is not an unambiguous yes denies: an anonymous caller, a failed role or predicate check, a predicate that throws, roles that is empty or not an array, details that are undefined or null, and a gate configured with no access rule at all. That last one matters most — roles={featureFlags.adminRoles} with a renamed key yields undefined, which would otherwise read as "no roles required" and show the guarded content to every signed-in caller. The type now requires at least one of roles/allow, and the runtime denies anyway because undefined defeats the type. The docs state plainly that this hides UI and does not protect data.

Semver. Minor, deliberately. isLoading is optional on the provider's context value so a context composed by hand still compiles, and required on IIdentityContext so a consumer reading it gets a boolean rather than boolean | undefined. I tried making it optional on both and reverted: it immediately produced five compile errors in this repo's own specs, which is what every downstream reader would inherit to avoid a break with no measured victim — a cross-repo search across Studio, Components, Prologue, Stage, cli, Samples and Templates found zero implementers of IIdentityContext, all of them pure useIdentity() consumers.

Verification. 202 spec files / 499 tests, tsc -b clean, eslint clean.

Mutation evidence, each applied and restored with the restore verified:

Mutation Result
seed isLoading false instead of true 2 assertions, specific
delete the anonymous guard survived at first — every spec supplied roles, and an anonymous caller fails a role check anyway. Two specs added for the configurations that actually reach it. Then 4 assertions, specific
roles/allow conjunction flipped to disjunction 8 assertions, specific
details === undefined instead of covering null 1 assertion, specific
useIdentity's isLoading ?? false inverted to ?? true survived independent review — the fallback was argued in prose and never checked. A spec rendering against a hand-built context now kills it
beginLoading no-op, stopLoading no-op, clearIdentity/refresh flags flipped each specific

Repaired along the way. Three existing for_IdentityProvider/when_refresh_is_called/* specs exercised nothing — one had its refresh() call commented out while keeping the name — and one duplicated a sibling. Eleven assertions were strictly implied by another and were removed rather than left padding the count.

Known, not fixed here. A rejected refresh() clears the loading flag but keeps the pre-refresh identity, so the gate keeps admitting on a credential whose cookie the client has already cleared. Dropping the identity there would sign a user out on a transient network blip; it is recorded rather than changed. Overlapping refreshes are also not ordered — the beginSilentValidation token pattern already in CommandForm is the fix if it becomes a problem.

@woksin
woksin merged commit c71a55b into main Aug 12, 2026
13 of 14 checks passed
@woksin
woksin deleted the feat/imp-41-gate-rendering-on-identity branch August 12, 2026 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant