Oier Bravo Urtasun
6bc7092ce5
- CLAUDE.md status line + logout paragraph + migration 013 gotcha updated to cover the INSERT+UPDATE two-step GoTrue behaviour and the RP-initiated logout flow; new SvelteKit gotcha block spelling out the signOut→$effect race that produced "PKCE code verifier not found in storage" and how the isLoggingOut flag + /logged-out public route close it. - docs/history/fase-8-auth-logout.md captures both prod bugs with symptom, root cause, fix, the Keycloak id_token_hint tradeoff (why the one-click confirmation page is accepted UX), and the test deltas. - docs/development.md auth.ts one-liner updated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
83 lines
6.8 KiB
Markdown
83 lines
6.8 KiB
Markdown
# Fase 8 — prod auth hardening (2026-04-20..21)
|
||
|
||
Two prod-only bugs observed on ambrosio after fase-7 shipped, both in the Keycloak → GoTrue → Supabase auth integration. Neither is reachable from dev because dev uses pre-seeded `auth.users` rows and localhost round-trips.
|
||
|
||
## Bug 1 — empty `auth.users.role` on new self-registrations
|
||
|
||
### Symptom
|
||
After the first real self-registrations via Keycloak, every REST call from those users errored. PostgREST logged `role "" does not exist`. The users existed in `auth.users` but with `role=''` and `aud='authenticated'`.
|
||
|
||
### Root cause
|
||
GoTrue v2.158.1's OIDC-provisioning path first INSERTs the user with empty `role` and `aud`, then immediately UPDATEs the same row (the pop ORM resends the full struct — `last_sign_in_at`, etc. — with the zero-value `role=''` still in the field). The INSERT trigger from migration 013 filled both columns correctly, but the follow-up UPDATE clobbered `role` back to empty. The footprint in `auth.users` is `created_at` / `updated_at` ~150–250ms apart with `role=''` and `aud='authenticated'` (aud stays filled because GoTrue's UPDATE sends `'authenticated'`).
|
||
|
||
### Fix — migration 014
|
||
`supabase/migrations/014_auth_users_role_update_trigger.sql`:
|
||
|
||
- Re-backfills any rows already clobbered by the UPDATE (idempotent).
|
||
- Installs `ensure_role_default_update` — a BEFORE UPDATE trigger on `auth.users` reusing the existing `auth.ensure_user_role_default()` function from migration 013.
|
||
|
||
Applied directly to prod DB 2026-04-20 (rsync + `psql < migration.sql` + manual insert into `_applied_migrations`). Two users who had been broken were backfilled in the same statement. pgTAP `008_auth_user_role_default.sql` was extended from 7 to 11 assertions to cover the UPDATE path + unrelated-UPDATE no-op behaviour.
|
||
|
||
## Bug 2 — logout doesn't log out + "PKCE code verifier not found in storage"
|
||
|
||
### Symptom
|
||
User reported two things, connected:
|
||
1. Clicking Logout returned them to the app still logged in (Keycloak re-authenticated silently).
|
||
2. Occasionally the browser console showed `PKCE code verifier not found in storage. This can happen if the auth flow was initiated in a different browser or device, or if the storage was cleared.`
|
||
|
||
### Root cause
|
||
Old `logout()` was `supabase.auth.signOut()` only, which clears the Supabase/GoTrue session but never touches Keycloak's SSO cookie. `(app)/+layout.svelte`'s `$effect` watches `$isAuthenticated` and fires `login()` the moment auth becomes false. So clicking logout produced this chain:
|
||
|
||
1. `signOut()` clears Supabase session.
|
||
2. `onAuthStateChange` fires `SIGNED_OUT`.
|
||
3. `$effect` sees `!$isAuthenticated && !$authLoading` → calls `login()`.
|
||
4. `signInWithOAuth` writes a fresh PKCE verifier to localStorage, redirects to Keycloak.
|
||
5. Keycloak SSO cookie still valid → immediate redirect back to `/auth/callback?code=…`.
|
||
6. Callback runs `exchangeCodeForSession(code)`.
|
||
|
||
In principle step 6 should find the verifier from step 4, but the chain is race-adjacent and breaks intermittently on prod:
|
||
|
||
- supabase-js v2.46's `signOut()` clears storage through an async adapter; if that clear lands after the verifier write, it wipes it.
|
||
- The `$effect` can fire twice in quick succession (Svelte 5 reactive re-emits) — two concurrent `signInWithOAuth` calls overwrite each other's verifiers while only one `window.location.assign` wins.
|
||
- iOS Safari occasionally drops localStorage writes issued immediately before navigation.
|
||
|
||
Dev never reproduced it because the loopback round-trip is sub-ms.
|
||
|
||
### Fix — RP-initiated OIDC logout
|
||
- `apps/web/src/lib/auth.ts` — `logout()` now sets a module-level `isLoggingOut = true`, calls `signOut()`, then `window.location.assign()` to Keycloak's `/protocol/openid-connect/logout?client_id=colectivo-web&post_logout_redirect_uri=${origin}/logged-out`. Keycloak clears the SSO cookie and redirects back.
|
||
- `apps/web/src/routes/(app)/+layout.svelte` — `$effect` reads `isLoggingOut` and skips `login()` during the logout navigation, closing the PKCE race.
|
||
- `apps/web/src/routes/logged-out/+page.svelte` — new public route outside the `(app)` group, so the auth-gate effect never runs there. Shows a "Signed out" message + "Sign in" button that triggers a normal `login()`.
|
||
- `apps/web/messages/{en,es}.json` — added `logged_out_title`, `logged_out_subtitle`, `logged_out_sign_in`.
|
||
|
||
### Accepted UX caveat — Keycloak confirmation page
|
||
Keycloak only skips the "Do you want to log out?" confirmation page when `id_token_hint` is supplied. GoTrue's session object exposes `provider_token` and `provider_refresh_token` but NOT the Keycloak `id_token` (confirmed in `@supabase/auth-js` types — no `provider_id_token` field). We can't derive the id_token client-side because colectivo-web is a confidential client and the token-endpoint refresh flow requires `client_secret`. Options to eliminate the click were considered and rejected as overkill:
|
||
|
||
- **Server-side id_token fetch** — a SvelteKit `+server.ts` endpoint that uses service_role to SELECT `auth.identities.identity_data.id_token` for the current user, then returns the end-session URL with `id_token_hint`. Feasible (adapter-node is in use) but the one-click confirmation is standard OIDC and clear UX; the complexity wasn't justified.
|
||
|
||
The confirmation is documented in the A-05 / A-07 E2E tests — a helper `confirmKeycloakLogout(page)` clicks through it.
|
||
|
||
## Tests added or changed
|
||
|
||
- `supabase/tests/008_auth_user_role_default.sql` — plan bumped 7 → 11. New assertions: `ensure_role_default_update` trigger exists; UPDATE to `role=''`/`aud=''` gets reverted; unrelated UPDATE leaves explicit `service_role` untouched.
|
||
- `apps/web/tests/e2e/auth.test.ts` — A-05 rewritten to assert landing on `/logged-out` + no PKCE error in console + Supabase session cleared from localStorage. New A-07 asserts that the next login after logout prompts for Keycloak credentials (i.e. SSO cookie was actually cleared).
|
||
|
||
Totals after the fix: 259 tests green (45 pgTAP + 140 Vitest integration + 15 Vitest unit + 59 Playwright), same 3 skips as before.
|
||
|
||
## Deploy
|
||
|
||
- Migration 014 applied directly on prod 2026-04-20 (SQL-only change, no need for full rebuild).
|
||
- Web code (RP-initiated logout + `/logged-out`) deployed via `infra/scripts/deploy-erosi.sh` on 2026-04-21. Migrations were skipped (already applied); app image rebuilt + rolling restart.
|
||
|
||
## Files touched
|
||
|
||
- `supabase/migrations/014_auth_users_role_update_trigger.sql` (new)
|
||
- `supabase/tests/008_auth_user_role_default.sql` (+4 assertions)
|
||
- `apps/web/src/lib/auth.ts`
|
||
- `apps/web/src/routes/(app)/+layout.svelte`
|
||
- `apps/web/src/routes/logged-out/+page.svelte` (new)
|
||
- `apps/web/messages/en.json`, `apps/web/messages/es.json`
|
||
- `apps/web/tests/e2e/auth.test.ts`
|
||
- `CLAUDE.md`, `docs/development.md`, `docs/history/fase-8-auth-logout.md` (this file)
|
||
|
||
Commits: `9067ab8` (migration 014), `ca12516` (logout fix).
|