feat(login): render sign-in methods from the backend, add email-code sign-in - #1
Merged
Conversation
The login page listed its providers from a module-level array with a
hardcoded `enabled` flag, so turning a sign-in method on in the backend
did nothing until the frontend shipped again. It now renders from
`GET /api/v1/auth/providers`, dispatching on `type` rather than `id`:
`id` is a machine key the backend may extend, `type` is the closed union
this build knows how to draw. An unrecognised type renders inert instead
of crashing the page or posing as a working way in.
Two decisions worth stating, both about not locking anyone out:
Google is rendered unconditionally and never from `methods`. It is not in
the backend's list at all — it lives entirely in NextAuth — so deriving
the whole button list from that list would delete the Google button
exactly when the auth service is unreachable, removing the one method
that still works.
The resolver never throws. `/login` is a dynamic server component, so an
uncaught throw is a 500, which would turn a degraded auth service into a
dead login page for a self-hosted instance — including for the
administrator who came to fix it. Every transport failure resolves to
`{ok:false}` and the page renders a break-glass password form. A
successfully parsed list is rendered as it stands, empty included:
treating empty as failure would, once the list becomes admin-toggleable,
hand an administrator who deliberately disabled password sign-in a
synthesized form that answers 401 forever.
Resolved server-side because `(public)` deliberately omits AppProviders
to keep the cold-start route thin, so there is no QueryClient to fetch
under.
The fixture is recorded from the live backend, and the contract test was
proved to bite by renaming `display_name` and watching it fail.
Refs RUK-288
The backend deliberately sets no cookie on either OTP step: it returns a
`session_nonce` in the 202 body and expects it back on verify, because
the browser never calls that API — the BFF does, server-to-server — so a
`Set-Cookie` from the backend would be stored by our HTTP client and
never reach the user, leaving a binding that looks implemented and binds
nothing. Binding the code to an actual browser is therefore ours to do,
on our own origin.
(The design doc describes a backend-set cookie. It is stale; the handler
is the source of truth. Tracked as FU-1.)
The value is base64url(JSON), not a delimiter join, because the email
travels with the nonce and the address is attacker-supplied — the backend
answers 202 for any well-formed one. `${nonce}:${email}` would let an
address containing the delimiter control the parsed nonce.
Reading is total: absent, truncated, non-base64, non-JSON and wrong-shaped
cookies all resolve to "no binding" rather than throwing. That matters
because the caller treats "no binding" as the request-a-new-code state, so
a corrupted cookie can never surface as "wrong code" to someone holding a
correct one.
maxAge equals the backend TTL rather than adding a margin, which would
otherwise create a window where a live cookie carries a dead code.
Refs RUK-288
Adds the `backend-login` credentials provider behind the two built-in methods, with the backend exchange in the `signIn` callback rather than in `authorize`. That split is the load-bearing decision. `authorize` cannot do the exchange: it would have to hand the resulting tokens to the callback through the browser, and the only alternative — verifying in a route and again in the provider — would check one 6-digit code twice, where the backend allows five attempts before burning the code for up to five minutes. Deferring to the callback mirrors how dev-bypass already reaches `runBackendExchange`, and keeps the tokens inside NextAuth, so "the browser never sees a token" holds by construction rather than by assertion. A lost browser binding is surfaced as its own error, never as a wrong code. Someone who reopened the tab is holding a perfectly valid code, and telling them it is wrong leaves them with nothing to do but retype it. The cookie is cleared on every terminal outcome except a wrong code, which must keep the remaining attempts alive. Password failures keep the backend's uniform 401: it refuses to say which of wrong-password, blocked, refused-signup or seats-exhausted occurred so that a caller cannot enumerate accounts, and we do not undo that by guessing a reason. The exchange and the profile load are caught separately, as in `runBackendExchange`, so a failure is attributed to the stage that failed. `refresh_token` is not required here — it carries `omitempty` on these endpoints, so demanding it would reject a valid pair. The AC-10 guard was proved to bite by deleting the callback branch and watching it fail: the catch-all `return false` makes an unregistered provider fail silently, after the user's code has already been burnt. Refs RUK-288
Wires the two built-in methods to server actions and gives each its own screen. Actions rather than a client fetch, for two reasons: NextAuth attaches its CSRF token only when `signIn` runs on the server — a bare POST to the callback fails with `MissingCSRF` — and the sanitized `redirectTo` is closed over on the page, so a client can never supply a destination of its own and route around `safeNext`. The two verify failures need different transports, because NextAuth delivers them identically. On the server-action path it rethrows the `CredentialsSignin` rather than building a `?code=` redirect, so the split is made in the action: a lost binding leaves step two, since the flow is genuinely over and re-rendering the input would invite the user to retype a code that can no longer be checked; every other failure renders in place, keeping the countdown and the remaining attempts that a redirect would discard. Details that are load-bearing rather than decorative: - Expiry wins over a wrong code. Telling someone to re-check a code that can no longer work is a dead end. - A second submit while one is in flight is ignored. The backend allows five attempts and floors every response to ~300ms, so a double-click would otherwise spend two of them. - The countdown runs from response receipt, leaving the client slightly optimistic against the server. That direction is deliberate: the backend decides validity, so clock drift can never reject a good code. - The code field is a numeric `one-time-code` input, not a masked password, so paste and platform autofill keep working; the password form carries `username`/`current-password` so password managers fill and save. - A failed request starts a fresh cooldown instead of leaving the button hot — immediate retries are what earned the 429. Refs RUK-288
`(public)` mounts ThemeProvider only, so a `useQuery` anywhere under the login page white-screens with "No QueryClient set". For `/accept-invite` that hurts invited users; for `/login` it is every user of the product, on the cold-start route. This work grew that page by two forms' worth of new modules, which is exactly the change that could drag React Query back in through a transitive import — so `/login` is now walked by the same import-graph check that already covers `/accept-invite`, rather than trusting a render-time assertion that only proves the hook wasn't hit on one path. Verified by adding a `@tanstack/react-query` import to the OTP flow component, two levels down from the page, and watching the walk name the offending file. Refs RUK-288
Review and a mutation-based coverage pass found that the ticket's own bug
could be reintroduced server-side with every test still green.
The trap, first. When the backend rejected a binding, the flow reported it
in place on step two — where "Sign in" stayed enabled and fired further
calls against a cookie the server had already cleared, while the residual
cooldown greyed out the "Request a new code" button the message told the
user to press. The flow now returns to step one, where asking again is the
primary action and nothing is throttled.
The coverage hole. `runBuiltInSignIn` decides whether a failure is a lost
binding or a wrong code; the client only renders what it is handed, so
testing the rendering left the actual decision unguarded. Mutating that
branch to report a lost binding as a wrong code — precisely the defect
this ticket exists to remove — left 386 tests passing. It is extracted to
its own module so it can be tested without NextAuth's module-load
environment, and the callback rewraps its error so `?code=` still reaches
the page. Three mutations that previously survived now fail: reporting a
lost binding as a wrong code, accepting a binding issued for a different
address, and leaving a verified code replayable.
Also from review:
- The action sanitized `next` with `startsWith("/")`, which accepts
protocol-relative `//evil.test`. It is an exported server action, so it
is invocable by action id and cannot assume its caller sanitized first;
it now uses `safeNext` like the page does.
- The password form labelled the *email* input "Password" and left the
password input with only a placeholder, so a screen reader announced the
wrong field and the real one not at all. The old test encoded that
inversion, so it would not have caught a regression.
- A dead network was reported as "too many requests", sending people into
a pointless retry loop; it now says something went wrong.
- The AC-2 dispatch test asserted shared label text, so a `password`
method rendering the OTP flow passed. It asserts the rendered component
now, and was mutation-checked.
- Dropped `hasUsableMethod` (no callers) and an assertion that could not
fail.
Refs RUK-288
This call blocks the first byte of the cold-start route, and it inherited the shared 10-second client timeout. The break-glass fallback below it exists precisely for an unreachable auth service — but holding a blank tab open for ten seconds before rendering that fallback is indistinguishable, from the user's side, from a dead site. Two seconds is generous for a static list read over the gateway, and it turns a ten-second blank page into a fast render of the form an administrator needs to sign in and fix things. Note this endpoint carries no anti-timing floor: that applies to the OTP and password paths, which have something to hide. A providers response is byte-identical for every caller. `cache: "no-store"` is deliberately left alone. Serving it from the data cache would likely be fine, but the audit that raised it said to measure first, and SPEC §6.1 committed to matching `resolveInvitationPreview` — changing it is a spec amendment, not a drive-by. Refs RUK-288
Two doc blocks on this branch ended up documenting the wrong thing, because later commits inserted code between a comment and what it described. `PROVIDERS_TIMEOUT_MS` landed under the resolver's never-throws rationale, and the three new exchange helpers landed under `acceptInvitation`'s security note — leaving `acceptInvitation` itself undocumented and its invitation-accept reasoning reading as a header for `requestOtpCode`. Both are pure relocations; no text changed. On a branch whose density is rationale rather than code, a comment pointing at the wrong function is worse than no comment. Also collapses the twice-duplicated "coming soon" disabled button into one component. The non-obvious part — a disabled button emits no pointer events, so the tooltip needs a wrapper span — was explained in only one of the two copies, and now sits on the single definition. Props spread before `disabled` so a caller cannot accidentally re-enable it. The action set threaded to `BuiltInMethod` is a `Pick` of the page's props rather than a second verbatim copy of four signatures. Rejected while here: inlining the `backend-login` provider registration would have broken the AC-10 guard, which locates the end of `authorize` by a literal delimiter. Retargeting a deliberately mutation-resistant security test to buy a cosmetic win is a bad trade. Refs RUK-288
The pre-release gate found that commit 5054d8a shipped a fix that never took effect. `backendRequest` spread `...init` and then assigned `signal`, so the caller's 2s deadline was silently replaced by the shared 10s default: `/login` still held a blank tab for ten seconds before painting its break-glass form. The client now composes the two signals instead of overwriting one. The test that was supposed to guard it asserted `toBeInstanceOf(AbortSignal)` against a mocked client — it held just as happily with the bound set to ten minutes, and did hold while the signal was being discarded entirely. It now asserts the deadline actually fires, and a new test drives the real client against a never-answering server. This is the repo's own fixture-derived-expectation failure mode, one layer over: a test that names a behaviour must fail when that behaviour breaks. Second defect, from the security audit. SPEC §6.6 claims step two "cannot be pointed at a different address" — but the code compared the cookie's address to the submitted one and then sent the *submitted* one to the backend. The comparison was byte-exact while the backend normalizes, so a password manager filling `user@x` after the user typed `User@x` failed the check, cleared the cookie, and destroyed a perfectly valid code: the binding locking out the person it protects. Addresses are now normalized on write and on compare, and the verify sends the bound address, making the spec's claim true by construction. Also: a 429 on verify was reported as "that code isn't valid", telling someone to re-check a correct code and driving more requests into the limiter already refusing them — the same misattributed-failure loop this ticket exists to end. The request step had this right; verify now does too. New coverage for gaps the gate named: the six-digit guard (extracted to `domain/` so the client form and `authorize` cannot drift, and so it is testable), the synchronous double-submit ref, the request action's uniform error mapping, and AC-7's session half. Each was verified by breaking the behaviour and watching the test fail. AC-11 and §8.3 said a 404 should render Google without a password form; §6.1 supersedes that, and the code follows §6.1. The spec is corrected rather than the code. Refs RUK-288
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.
The login page listed its providers from a hardcoded array, so enabling a
sign-in method on the backend did nothing until the frontend shipped again. It
now renders from
GET /api/v1/auth/providersand adds the two methods thatlist can advertise: a two-step email one-time-code flow and an email+password
form. Backend and contract are untouched; Google/NextAuth is unchanged.
type, notid— the backend can add or rename a methodwithout a release, and a type this build doesn't know renders inert rather
than posing as a working way in
signIncallback does the exchange, so tokens are born inside NextAuth and never
cross a route boundary; this adds no BFF route
__Host-cookie the BFF owns,since the backend deliberately sets none — a lost binding gets its own
"request a new code" state instead of being reported as a wrong code, which
is the defect this ticket exists to fix
unconditionally (it isn't in the backend list), the resolver never throws,
and a transport failure falls back to the break-glass password form
error code, or state transition reveals whether an address has an account
backendRequestspread...initand thenoverwrote
signal, so any caller-supplied timeout was silently discardedVerification:
npm run verifypasses; contract and unit tests go from 186/1215to 203/1324. Each new guard was checked by breaking the behaviour it names and
confirming the test fails. One acceptance criterion is manual — the full
emailed round trip — and it needs an HTTPS stand: the
__Host-prefix requiresSecure, so the cookie is silently dropped onhttp://localhost:3000.