From ca1251600524b752aaf6632d2b3214c80f967496 Mon Sep 17 00:00:00 2001 From: Oier Bravo Urtasun Date: Tue, 21 Apr 2026 09:35:17 +0200 Subject: [PATCH] fix(auth): RP-initiated Keycloak logout + PKCE verifier race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking logout previously only called supabase.auth.signOut(): Keycloak's SSO cookie was untouched, so the (app) layout's $effect immediately re-fired login() and Keycloak silently re-authenticated — logout looked broken. That same chain also surfaced "PKCE code verifier not found in storage" on prod, since signOut()'s async storage clear could race signInWithOAuth()'s verifier write, or the $effect could double-fire and overwrite verifiers mid-flight. - logout(): signOut() then redirect to Keycloak's end_session endpoint (client_id + post_logout_redirect_uri=/logged-out). Keycloak shows a one-click "Do you want to log out?" confirmation because GoTrue's proxy flow does not expose the id_token, so we can't pass id_token_hint. - isLoggingOut flag + guard in (app) layout $effect: prevents auto-relogin during the logout navigation. - New public /logged-out route, outside the (app) group. - A-05 rewritten, new A-07 (fresh login must prompt for credentials after RP-initiated logout). - pgTAP 008 extended with 4 assertions for migration 014's UPDATE trigger. Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/web/messages/en.json | 3 + apps/web/messages/es.json | 3 + apps/web/src/lib/auth.ts | 42 +++++++++++++- apps/web/src/routes/(app)/+layout.svelte | 7 ++- apps/web/src/routes/logged-out/+page.svelte | 19 +++++++ apps/web/tests/e2e/auth.test.ts | 57 +++++++++++++++++-- supabase/tests/008_auth_user_role_default.sql | 51 ++++++++++++++--- 7 files changed, 165 insertions(+), 17 deletions(-) create mode 100644 apps/web/src/routes/logged-out/+page.svelte diff --git a/apps/web/messages/en.json b/apps/web/messages/en.json index 8b3f76c..31eb47f 100644 --- a/apps/web/messages/en.json +++ b/apps/web/messages/en.json @@ -13,6 +13,9 @@ "login_button": "Sign in", "logout_button": "Sign out", "auth_back_to_home": "Back to home", + "logged_out_title": "You've been signed out", + "logged_out_subtitle": "Sign in again to continue.", + "logged_out_sign_in": "Sign in", "onboarding_title": "Welcome to Colectivo", "onboarding_subtitle": "Get started by creating a new collective or joining an existing one.", "onboarding_create_tab": "Create", diff --git a/apps/web/messages/es.json b/apps/web/messages/es.json index 0e2fb2e..7840414 100644 --- a/apps/web/messages/es.json +++ b/apps/web/messages/es.json @@ -13,6 +13,9 @@ "login_button": "Iniciar sesión", "logout_button": "Cerrar sesión", "auth_back_to_home": "Volver al inicio", + "logged_out_title": "Sesión cerrada", + "logged_out_subtitle": "Inicia sesión de nuevo para continuar.", + "logged_out_sign_in": "Iniciar sesión", "onboarding_title": "Bienvenido a Colectivo", "onboarding_subtitle": "Crea un nuevo colectivo o únete a uno existente.", "onboarding_create_tab": "Crear", diff --git a/apps/web/src/lib/auth.ts b/apps/web/src/lib/auth.ts index ebd57a5..1183255 100644 --- a/apps/web/src/lib/auth.ts +++ b/apps/web/src/lib/auth.ts @@ -1,6 +1,18 @@ import { browser } from '$app/environment'; +import { + PUBLIC_KEYCLOAK_URL, + PUBLIC_KEYCLOAK_REALM, + PUBLIC_KEYCLOAK_CLIENT_ID +} from '$env/static/public'; import { getSupabase } from '$lib/supabase'; +/** + * Set for the duration of the logout flow so the (app) layout's auth-gate + * $effect doesn't race us by calling login() between signOut() and the + * Keycloak end-session redirect. + */ +export let isLoggingOut = false; + /** * Redirect to Keycloak login via GoTrue OAuth proxy. * GoTrue handles the PKCE exchange; the browser lands at /auth/callback. @@ -20,10 +32,34 @@ export async function login(redirectTo?: string): Promise { } /** - * Sign out from Supabase (clears local session). - * The Keycloak session remains active — the user won't be asked for credentials - * again on next login unless they also log out from the Keycloak Account Console. + * RP-initiated OIDC logout: clear the Supabase session AND end the Keycloak + * SSO session, then land on a public /logged-out page. + * + * Why both: `supabase.auth.signOut()` only clears GoTrue state. If we don't + * also hit Keycloak's end-session endpoint, the Keycloak SSO cookie survives + * and the next `signInWithOAuth()` silently re-authenticates — making logout + * look broken. It also eliminates a race where the (app) layout's auth + * effect re-fires `login()` before navigation and can clobber the PKCE + * verifier mid-flight ("PKCE code verifier not found in storage" on prod). + * + * Keycloak's `post.logout.redirect.uris` is set to "+" so any registered + * redirect URI (including `${origin}/*`) is valid as a post-logout target; + * passing `client_id` is sufficient — `id_token_hint` is not exposed by + * GoTrue's proxy path, so we rely on client_id + post_logout_redirect_uri. */ export async function logout(): Promise { + if (!browser) return; + + isLoggingOut = true; + await getSupabase().auth.signOut(); + + const postLogoutRedirect = `${window.location.origin}/logged-out`; + const endSession = new URL( + `${PUBLIC_KEYCLOAK_URL}/realms/${PUBLIC_KEYCLOAK_REALM}/protocol/openid-connect/logout` + ); + endSession.searchParams.set('client_id', PUBLIC_KEYCLOAK_CLIENT_ID); + endSession.searchParams.set('post_logout_redirect_uri', postLogoutRedirect); + + window.location.assign(endSession.toString()); } diff --git a/apps/web/src/routes/(app)/+layout.svelte b/apps/web/src/routes/(app)/+layout.svelte index 517a13d..ef33bed 100644 --- a/apps/web/src/routes/(app)/+layout.svelte +++ b/apps/web/src/routes/(app)/+layout.svelte @@ -2,7 +2,7 @@ import type { Snippet } from 'svelte'; import { page } from '$app/stores'; import { isAuthenticated, authLoading } from '$lib/stores/auth'; - import { login } from '$lib/auth'; + import { login, isLoggingOut } from '$lib/auth'; import DesktopSidebar from '$lib/components/layout/DesktopSidebar.svelte'; import MobileTopBar from '$lib/components/layout/MobileTopBar.svelte'; import BottomTabBar from '$lib/components/layout/BottomTabBar.svelte'; @@ -25,8 +25,11 @@ // When auth resolves as unauthenticated, redirect to Keycloak. // This runs after onAuthStateChange fires (authLoading = false), so there // is no race with Supabase's async localStorage initialisation. + // `isLoggingOut` suppresses re-login while logout() is mid-flight between + // signOut() and the Keycloak end-session redirect — otherwise we'd race + // the navigation and clobber the PKCE verifier. $effect(() => { - if (!$authLoading && !$isAuthenticated) { + if (!$authLoading && !$isAuthenticated && !isLoggingOut) { login(); } }); diff --git a/apps/web/src/routes/logged-out/+page.svelte b/apps/web/src/routes/logged-out/+page.svelte new file mode 100644 index 0000000..4bf00cc --- /dev/null +++ b/apps/web/src/routes/logged-out/+page.svelte @@ -0,0 +1,19 @@ + + +
+
+

+ {m.logged_out_title()} +

+

{m.logged_out_subtitle()}

+ +
+
diff --git a/apps/web/tests/e2e/auth.test.ts b/apps/web/tests/e2e/auth.test.ts index 15b9f79..a543554 100644 --- a/apps/web/tests/e2e/auth.test.ts +++ b/apps/web/tests/e2e/auth.test.ts @@ -32,7 +32,25 @@ test.describe('Auth — login and route protection', () => { ).toBeVisible({ timeout: 15_000 }); }); - test('A-05: Ana can sign out', async ({ page }) => { + // Helper: click through Keycloak's "Do you want to log out?" confirmation. + // Keycloak shows this whenever the end-session request is made without + // `id_token_hint` — which is our case, because GoTrue's proxy flow does + // not expose the Keycloak id_token to the client. In prod users see it + // as a one-click confirmation, which is acceptable. + async function confirmKeycloakLogout(page: import('@playwright/test').Page) { + await page.waitForURL(/realms\/colectivo\/protocol\/openid-connect\/logout/, { + timeout: 15_000 + }); + await page.getByRole('button', { name: /^logout$/i }).click(); + } + + test('A-05: Ana can sign out (ends Keycloak SSO, lands on /logged-out)', async ({ page }) => { + const pkceErrors: string[] = []; + page.on('console', (msg) => { + const t = msg.text(); + if (/PKCE code verifier not found/i.test(t)) pkceErrors.push(t); + }); + await loginAs(page, USERS.ana); await page.goto('/settings'); @@ -40,10 +58,39 @@ test.describe('Auth — login and route protection', () => { await expect(signOutButton).toBeVisible({ timeout: 10_000 }); await signOutButton.click(); - // After sign out, the session is cleared. The (app) layout's auth effect - // will redirect back to Keycloak for re-auth (SSO still active there), so - // we end up somewhere other than /settings. We just verify we left. - await expect(page).not.toHaveURL(/\/settings/, { timeout: 15_000 }); + await confirmKeycloakLogout(page); + + // After the Keycloak confirmation, browser lands on /logged-out. + await page.waitForURL(/\/logged-out(\?|$|#)/, { timeout: 20_000 }); + + // No Supabase session should remain. + const hasSession = await page.evaluate(() => + Object.keys(localStorage).some( + (k) => k.startsWith('sb-') && k.endsWith('-auth-token') && !!localStorage.getItem(k) + ) + ); + expect(hasSession).toBe(false); + + // And no PKCE error was emitted during the flow. + expect(pkceErrors).toEqual([]); + }); + + test('A-07: after sign out, next login prompts for Keycloak credentials', async ({ page }) => { + await loginAs(page, USERS.ana); + await page.goto('/settings'); + await page.getByRole('button', { name: /sign out|cerrar sesión/i }).click(); + await confirmKeycloakLogout(page); + await page.waitForURL(/\/logged-out/, { timeout: 20_000 }); + + // Navigate anywhere in the (app) group — the auth effect should push us + // to Keycloak, and because the SSO cookie was cleared by RP-initiated + // logout, Keycloak must render the login form (not silently redirect). + await page.goto('/lists'); + await page.waitForURL( + new RegExp(`realms/${KEYCLOAK_REALM}/protocol/openid-connect/auth`), + { timeout: 15_000 } + ); + await expect(page.locator('#username')).toBeVisible({ timeout: 10_000 }); }); test('A-06: David (guest) can log in and see the collective', async ({ page }) => { diff --git a/supabase/tests/008_auth_user_role_default.sql b/supabase/tests/008_auth_user_role_default.sql index 91d70cd..283cc9f 100644 --- a/supabase/tests/008_auth_user_role_default.sql +++ b/supabase/tests/008_auth_user_role_default.sql @@ -1,16 +1,20 @@ --- pgTAP: auth.users role/aud defaulting (migration 013) +-- pgTAP: auth.users role/aud defaulting (migrations 013 + 014) -- Run with: psql -U postgres -d postgres -f supabase/tests/008_auth_user_role_default.sql -- --- Covers the bug found on ambrosio (2026-04-15): GoTrue v2.158.1 inserts --- OIDC-created users into auth.users with empty-string `role` and `aud`, --- which later makes PostgREST's `SET LOCAL role = ''` fail with --- `role "" does not exist`. Migration 013 adds a BEFORE INSERT trigger + --- backfills the existing rows. +-- Covers two bugs observed on ambrosio: +-- * 2026-04-15 — GoTrue v2.158.1 inserts OIDC-created users into auth.users +-- with empty-string `role` and `aud`, later making PostgREST's +-- `SET LOCAL role = ''` fail with `role "" does not exist`. +-- Migration 013 adds a BEFORE INSERT trigger + backfills. +-- * 2026-04-20 — GoTrue additionally emits an UPDATE right after the +-- insert (pop ORM resends the whole struct incl. role='') which clobbers +-- the role that 013's trigger just filled in. Migration 014 adds a +-- matching BEFORE UPDATE trigger. CREATE EXTENSION IF NOT EXISTS pgtap; BEGIN; -SELECT plan(7); +SELECT plan(11); -- ── Existence assertions ──────────────────────────────────────────────── SELECT has_function( @@ -23,6 +27,11 @@ SELECT has_trigger( 'BEFORE INSERT trigger ensure_role_default exists on auth.users' ); +SELECT has_trigger( + 'auth', 'users', 'ensure_role_default_update', + 'BEFORE UPDATE trigger ensure_role_default_update exists on auth.users' +); + -- ── Backfill assertion ────────────────────────────────────────────────── SELECT ok( (SELECT count(*) FROM auth.users WHERE role IS NULL OR role = '' OR aud IS NULL OR aud = '') = 0, @@ -85,5 +94,33 @@ SELECT is( 'Explicit role is preserved by the trigger' ); +-- ── UPDATE trigger behavior (migration 014) ──────────────────────────── +-- Simulate GoTrue's pop-ORM resending role='' on the follow-up UPDATE. +-- The BEFORE UPDATE trigger must rewrite it to 'authenticated'. +UPDATE auth.users +SET role = '', aud = '' +WHERE id = 'f0000000-0000-0000-0000-000000000001'; +SELECT is( + (SELECT role FROM auth.users WHERE id = 'f0000000-0000-0000-0000-000000000001'), + 'authenticated', + 'UPDATE clearing role to empty is reverted to authenticated' +); +SELECT is( + (SELECT aud FROM auth.users WHERE id = 'f0000000-0000-0000-0000-000000000001'), + 'authenticated', + 'UPDATE clearing aud to empty is reverted to authenticated' +); + +-- And a legitimate UPDATE to some other field (e.g. raw_user_meta_data) +-- does not change role/aud. +UPDATE auth.users +SET raw_user_meta_data = '{"locale":"es"}'::jsonb +WHERE id = 'f0000000-0000-0000-0000-000000000003'; +SELECT is( + (SELECT role FROM auth.users WHERE id = 'f0000000-0000-0000-0000-000000000003'), + 'service_role', + 'Unrelated UPDATE leaves an explicit role untouched' +); + SELECT * FROM finish(); ROLLBACK;