fix(auth): RP-initiated Keycloak logout + PKCE verifier race
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) <noreply@anthropic.com>
This commit is contained in:
@@ -13,6 +13,9 @@
|
|||||||
"login_button": "Sign in",
|
"login_button": "Sign in",
|
||||||
"logout_button": "Sign out",
|
"logout_button": "Sign out",
|
||||||
"auth_back_to_home": "Back to home",
|
"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_title": "Welcome to Colectivo",
|
||||||
"onboarding_subtitle": "Get started by creating a new collective or joining an existing one.",
|
"onboarding_subtitle": "Get started by creating a new collective or joining an existing one.",
|
||||||
"onboarding_create_tab": "Create",
|
"onboarding_create_tab": "Create",
|
||||||
|
|||||||
@@ -13,6 +13,9 @@
|
|||||||
"login_button": "Iniciar sesión",
|
"login_button": "Iniciar sesión",
|
||||||
"logout_button": "Cerrar sesión",
|
"logout_button": "Cerrar sesión",
|
||||||
"auth_back_to_home": "Volver al inicio",
|
"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_title": "Bienvenido a Colectivo",
|
||||||
"onboarding_subtitle": "Crea un nuevo colectivo o únete a uno existente.",
|
"onboarding_subtitle": "Crea un nuevo colectivo o únete a uno existente.",
|
||||||
"onboarding_create_tab": "Crear",
|
"onboarding_create_tab": "Crear",
|
||||||
|
|||||||
@@ -1,6 +1,18 @@
|
|||||||
import { browser } from '$app/environment';
|
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';
|
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.
|
* Redirect to Keycloak login via GoTrue OAuth proxy.
|
||||||
* GoTrue handles the PKCE exchange; the browser lands at /auth/callback.
|
* GoTrue handles the PKCE exchange; the browser lands at /auth/callback.
|
||||||
@@ -20,10 +32,34 @@ export async function login(redirectTo?: string): Promise<void> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Sign out from Supabase (clears local session).
|
* RP-initiated OIDC logout: clear the Supabase session AND end the Keycloak
|
||||||
* The Keycloak session remains active — the user won't be asked for credentials
|
* SSO session, then land on a public /logged-out page.
|
||||||
* again on next login unless they also log out from the Keycloak Account Console.
|
*
|
||||||
|
* 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<void> {
|
export async function logout(): Promise<void> {
|
||||||
|
if (!browser) return;
|
||||||
|
|
||||||
|
isLoggingOut = true;
|
||||||
|
|
||||||
await getSupabase().auth.signOut();
|
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());
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2,7 +2,7 @@
|
|||||||
import type { Snippet } from 'svelte';
|
import type { Snippet } from 'svelte';
|
||||||
import { page } from '$app/stores';
|
import { page } from '$app/stores';
|
||||||
import { isAuthenticated, authLoading } from '$lib/stores/auth';
|
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 DesktopSidebar from '$lib/components/layout/DesktopSidebar.svelte';
|
||||||
import MobileTopBar from '$lib/components/layout/MobileTopBar.svelte';
|
import MobileTopBar from '$lib/components/layout/MobileTopBar.svelte';
|
||||||
import BottomTabBar from '$lib/components/layout/BottomTabBar.svelte';
|
import BottomTabBar from '$lib/components/layout/BottomTabBar.svelte';
|
||||||
@@ -25,8 +25,11 @@
|
|||||||
// When auth resolves as unauthenticated, redirect to Keycloak.
|
// When auth resolves as unauthenticated, redirect to Keycloak.
|
||||||
// This runs after onAuthStateChange fires (authLoading = false), so there
|
// This runs after onAuthStateChange fires (authLoading = false), so there
|
||||||
// is no race with Supabase's async localStorage initialisation.
|
// 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(() => {
|
$effect(() => {
|
||||||
if (!$authLoading && !$isAuthenticated) {
|
if (!$authLoading && !$isAuthenticated && !isLoggingOut) {
|
||||||
login();
|
login();
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|||||||
19
apps/web/src/routes/logged-out/+page.svelte
Normal file
19
apps/web/src/routes/logged-out/+page.svelte
Normal file
@@ -0,0 +1,19 @@
|
|||||||
|
<script lang="ts">
|
||||||
|
import { login } from '$lib/auth';
|
||||||
|
import * as m from '$lib/paraglide/messages';
|
||||||
|
</script>
|
||||||
|
|
||||||
|
<div class="flex h-screen items-center justify-center bg-background">
|
||||||
|
<div class="max-w-sm px-6 text-center">
|
||||||
|
<h1 class="mb-2 text-[20px] font-semibold tracking-[-0.01em] text-slate-900 dark:text-slate-50">
|
||||||
|
{m.logged_out_title()}
|
||||||
|
</h1>
|
||||||
|
<p class="mb-6 text-sm text-text-secondary">{m.logged_out_subtitle()}</p>
|
||||||
|
<button
|
||||||
|
onclick={() => login()}
|
||||||
|
class="rounded-md bg-slate-900 px-4 py-2 text-sm font-semibold text-white hover:opacity-90 dark:bg-slate-50 dark:text-slate-900"
|
||||||
|
>
|
||||||
|
{m.logged_out_sign_in()}
|
||||||
|
</button>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
@@ -32,7 +32,25 @@ test.describe('Auth — login and route protection', () => {
|
|||||||
).toBeVisible({ timeout: 15_000 });
|
).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 loginAs(page, USERS.ana);
|
||||||
await page.goto('/settings');
|
await page.goto('/settings');
|
||||||
|
|
||||||
@@ -40,10 +58,39 @@ test.describe('Auth — login and route protection', () => {
|
|||||||
await expect(signOutButton).toBeVisible({ timeout: 10_000 });
|
await expect(signOutButton).toBeVisible({ timeout: 10_000 });
|
||||||
await signOutButton.click();
|
await signOutButton.click();
|
||||||
|
|
||||||
// After sign out, the session is cleared. The (app) layout's auth effect
|
await confirmKeycloakLogout(page);
|
||||||
// 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.
|
// After the Keycloak confirmation, browser lands on /logged-out.
|
||||||
await expect(page).not.toHaveURL(/\/settings/, { timeout: 15_000 });
|
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 }) => {
|
test('A-06: David (guest) can log in and see the collective', async ({ page }) => {
|
||||||
|
|||||||
@@ -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
|
-- 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
|
-- Covers two bugs observed on ambrosio:
|
||||||
-- OIDC-created users into auth.users with empty-string `role` and `aud`,
|
-- * 2026-04-15 — GoTrue v2.158.1 inserts OIDC-created users into auth.users
|
||||||
-- which later makes PostgREST's `SET LOCAL role = ''` fail with
|
-- with empty-string `role` and `aud`, later making PostgREST's
|
||||||
-- `role "" does not exist`. Migration 013 adds a BEFORE INSERT trigger +
|
-- `SET LOCAL role = ''` fail with `role "" does not exist`.
|
||||||
-- backfills the existing rows.
|
-- 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;
|
CREATE EXTENSION IF NOT EXISTS pgtap;
|
||||||
|
|
||||||
BEGIN;
|
BEGIN;
|
||||||
SELECT plan(7);
|
SELECT plan(11);
|
||||||
|
|
||||||
-- ── Existence assertions ────────────────────────────────────────────────
|
-- ── Existence assertions ────────────────────────────────────────────────
|
||||||
SELECT has_function(
|
SELECT has_function(
|
||||||
@@ -23,6 +27,11 @@ SELECT has_trigger(
|
|||||||
'BEFORE INSERT trigger ensure_role_default exists on auth.users'
|
'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 ──────────────────────────────────────────────────
|
-- ── Backfill assertion ──────────────────────────────────────────────────
|
||||||
SELECT ok(
|
SELECT ok(
|
||||||
(SELECT count(*) FROM auth.users WHERE role IS NULL OR role = '' OR aud IS NULL OR aud = '') = 0,
|
(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'
|
'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();
|
SELECT * FROM finish();
|
||||||
ROLLBACK;
|
ROLLBACK;
|
||||||
|
|||||||
Reference in New Issue
Block a user