From cbeefd648774f0bf407b73d2ed938c18e7e1762a Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 20 Oct 2026 15:08:34 +0200 Subject: [PATCH 1/7] fix(auth): restore OIDC username while keeping it read-only (#371) input: false on user.additionalFields.username also made better-auth drop the field from the OIDC provider profile, so new users had no username and Trino ran their queries as 'anonymous'. Allow the write only from the OIDC callback via database hooks instead, which still rejects rewrites through /update-user (#330). --- e2e/auth/auth.spec.ts | 12 ++++++++++++ src/lib/server/auth.test.ts | 36 +++++++++++++++++++++++++++--------- src/lib/server/auth.ts | 22 ++++++++++++++++++++-- 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/e2e/auth/auth.spec.ts b/e2e/auth/auth.spec.ts index 0e81b6c2..2e179336 100644 --- a/e2e/auth/auth.spec.ts +++ b/e2e/auth/auth.spec.ts @@ -48,6 +48,18 @@ test.describe('Authentication', () => { await expect(page.getByRole('heading', { name: 'Dashboard' })).toBeVisible(); }); + test('username cannot be changed through the update-user endpoint', async ({ page, baseURL }) => { + const res = await page.request.post('/api/auth/update-user', { + headers: { origin: baseURL! }, + data: { username: 'someone-else' } + }); + expect(res.ok()).toBe(false); + expect(await res.json()).toMatchObject({ message: 'username is not allowed to be set' }); + + const session = await (await page.request.get('/api/auth/get-session')).json(); + expect(session.user.username).toMatch(/^testuser\d+$/); + }); + test('user menu shows sign out option', async ({ page }) => { await page.goto('/'); diff --git a/src/lib/server/auth.test.ts b/src/lib/server/auth.test.ts index ac7901f4..c3c3c83f 100644 --- a/src/lib/server/auth.test.ts +++ b/src/lib/server/auth.test.ts @@ -1,21 +1,39 @@ import { describe, expect, it } from 'vitest'; -import { parseUserInput } from 'better-auth/db'; +import { parseAdditionalUserInputFromProviderProfile } from 'better-auth/db'; import { auth } from './auth.js'; +const hooks = auth.options.databaseHooks!.user!; +const create = (data: Record, path: string) => + hooks.create!.before!(data as never, { path } as never); +const update = (data: Record, path: string) => + hooks.update!.before!(data as never, { path } as never); + describe('user.additionalFields.username', () => { - it('rejects a rewrite through the built-in update-user endpoint', () => { - expect(() => parseUserInput(auth.options, { username: 'someone-else' }, 'update')).toThrow( - /username is not allowed to be set/ - ); + it('is taken from the OIDC provider profile', () => { + expect( + parseAdditionalUserInputFromProviderProfile(auth.options, { username: 'alice' }, 'create') + ).toMatchObject({ username: 'alice' }); }); - it('rejects a rewrite smuggled in at sign-up', () => { - expect(() => parseUserInput(auth.options, { username: 'someone-else' }, 'create')).toThrow( + it('may be written by the OIDC callback', async () => { + await expect( + create({ email: 'alice@example.com', username: 'alice' }, '/oauth2/callback/:providerId') + ).resolves.toBeUndefined(); + }); + + it('rejects a rewrite through the built-in update-user endpoint', async () => { + await expect(update({ username: 'someone-else' }, '/update-user')).rejects.toThrow( /username is not allowed to be set/ ); }); - it('is still declared, so the OIDC claim can populate it', () => { - expect(auth.options.user?.additionalFields?.username).toMatchObject({ type: 'string' }); + it('rejects a rewrite smuggled in at sign-up', async () => { + await expect( + create({ email: 'eve@example.com', username: 'someone-else' }, '/sign-up/email') + ).rejects.toThrow(/username is not allowed to be set/); + }); + + it('leaves other user updates alone', async () => { + await expect(update({ emailVerified: true }, '/update-user')).resolves.toBeUndefined(); }); }); diff --git a/src/lib/server/auth.ts b/src/lib/server/auth.ts index f3b6e65c..855f3a38 100644 --- a/src/lib/server/auth.ts +++ b/src/lib/server/auth.ts @@ -1,4 +1,5 @@ import { betterAuth } from 'better-auth'; +import { APIError } from 'better-auth/api'; import { drizzleAdapter } from '@better-auth/drizzle-adapter'; import { genericOAuth, openAPI } from 'better-auth/plugins'; import { sveltekitCookies } from 'better-auth/svelte-kit'; @@ -15,6 +16,18 @@ const env = envModule?.env ?? (process.env as Record const getRequestEvent = appServer?.getRequestEvent ?? (() => undefined as unknown as RequestEvent); const log = loggingModule?.logger.child({ module: 'auth' }); +// The username is the Trino user we impersonate, so only the OIDC callback may +// write it. Marking the field `input: false` would also block that: better-auth +// drops such fields from the provider profile on sign-up (see #371). Database +// hooks see the endpoint's route template, not the request path. +const usernameWritePath = '/oauth2/callback/:providerId'; + +function guardUsernameWrite(data: Record, context: { path?: string } | null) { + if (!('username' in data) || context?.path === usernameWritePath) return; + log?.warn({ path: context?.path }, 'Rejected username write outside the OIDC callback'); + throw new APIError('FORBIDDEN', { message: 'username is not allowed to be set' }); +} + const trinoUserClaim = env.STACKABLE_COCKPIT_TRINO_USER_CLAIM ?? 'preferred_username'; const defaultScopes = ['openid', 'profile', 'email']; @@ -57,11 +70,16 @@ export const auth = betterAuth({ additionalFields: { username: { type: 'string', - required: false, - input: false + required: false } } }, + databaseHooks: { + user: { + create: { before: async (user, context) => guardUsernameWrite(user, context) }, + update: { before: async (user, context) => guardUsernameWrite(user, context) } + } + }, plugins: [ sveltekitCookies(getRequestEvent), openAPI(), From c75313c0e78ce7ae16cd369b533bffe05594873a Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 20 Oct 2026 15:08:40 +0200 Subject: [PATCH 2/7] fix(trino): fail instead of impersonating 'anonymous' without a username (#371) Fall back to 'anonymous' only when nobody is signed in (OIDC disabled). A signed-in user without a username now gets a logged error and a 500, so a lost username claim can no longer go unnoticed. --- src/lib/server/auth-utils.test.ts | 24 +++++++++++++++++-- src/lib/server/auth-utils.ts | 14 +++++++++++ src/routes/(app)/api/trino/catalog/+server.ts | 4 ++-- .../api/trino/completion/metadata/+server.ts | 4 ++-- src/routes/(app)/api/trino/query/+server.ts | 4 ++-- src/routes/(app)/trino/+page.server.ts | 4 ++-- 6 files changed, 44 insertions(+), 10 deletions(-) diff --git a/src/lib/server/auth-utils.test.ts b/src/lib/server/auth-utils.test.ts index 38f63988..27724b1a 100644 --- a/src/lib/server/auth-utils.test.ts +++ b/src/lib/server/auth-utils.test.ts @@ -1,5 +1,5 @@ -import { describe, it, expect } from 'vitest'; -import { getUserId } from './auth-utils.js'; +import { describe, it, expect, vi } from 'vitest'; +import { getTrinoUser, getUserId } from './auth-utils.js'; describe('getUserId', () => { it('returns the user ID when user is present', () => { @@ -17,3 +17,23 @@ describe('getUserId', () => { expect(getUserId(locals)).toBe('anonymous'); }); }); + +describe('getTrinoUser', () => { + const logger = { error: vi.fn() }; + + it('returns the username of the signed-in user', () => { + const locals = { user: { id: 'u1', username: 'alice' }, logger } as unknown as App.Locals; + expect(getTrinoUser(locals)).toBe('alice'); + }); + + it('returns "anonymous" without a signed-in user (OIDC disabled)', () => { + const locals = { user: null, logger } as unknown as App.Locals; + expect(getTrinoUser(locals)).toBe('anonymous'); + }); + + it('fails instead of falling back to "anonymous" when the username is missing', () => { + const locals = { user: { id: 'u1', username: null }, logger } as unknown as App.Locals; + expect(() => getTrinoUser(locals)).toThrow(expect.objectContaining({ status: 500 })); + expect(logger.error).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); + }); +}); diff --git a/src/lib/server/auth-utils.ts b/src/lib/server/auth-utils.ts index 0bb1e2c7..6769b2c7 100644 --- a/src/lib/server/auth-utils.ts +++ b/src/lib/server/auth-utils.ts @@ -1,3 +1,17 @@ +import { error } from '@sveltejs/kit'; + export function getUserId(locals: App.Locals): string { return locals.user?.id ?? 'anonymous'; } + +/** + * The user to impersonate in Trino. Without OIDC there is no signed-in user, + * so queries run as 'anonymous'. A signed-in user without a username means + * sign-up lost the OIDC claim; fail instead of silently running as 'anonymous'. + */ +export function getTrinoUser(locals: App.Locals): string { + if (!locals.user) return 'anonymous'; + if (locals.user.username) return locals.user.username; + locals.logger.error({ user_id: locals.user.id }, 'Signed-in user has no Trino username'); + error(500, 'User has no Trino username'); +} diff --git a/src/routes/(app)/api/trino/catalog/+server.ts b/src/routes/(app)/api/trino/catalog/+server.ts index ffca1c5c..e4ac03b9 100644 --- a/src/routes/(app)/api/trino/catalog/+server.ts +++ b/src/routes/(app)/api/trino/catalog/+server.ts @@ -1,6 +1,6 @@ import { json, error } from '@sveltejs/kit'; import { resolveTrinoClient, trinoMetadataQuery } from '$lib/server/trino/client.js'; -import { getUserId } from '$lib/server/auth-utils.js'; +import { getTrinoUser, getUserId } from '$lib/server/auth-utils.js'; import { quoteIdentifier, quoteStringLiteral } from '$lib/editor/identifiers.js'; import type { RequestHandler } from './$types'; @@ -14,7 +14,7 @@ export const GET: RequestHandler = async ({ url, locals }) => { error(400, 'No Trino connection configured'); } - const user = locals.user?.username ?? 'anonymous'; + const user = getTrinoUser(locals); const level = url.searchParams.get('level'); const catalog = url.searchParams.get('catalog'); const schema = url.searchParams.get('schema'); diff --git a/src/routes/(app)/api/trino/completion/metadata/+server.ts b/src/routes/(app)/api/trino/completion/metadata/+server.ts index a0529739..880907be 100644 --- a/src/routes/(app)/api/trino/completion/metadata/+server.ts +++ b/src/routes/(app)/api/trino/completion/metadata/+server.ts @@ -1,7 +1,7 @@ import { json, error } from '@sveltejs/kit'; import { z } from 'zod'; import { resolveTrinoClient, trinoMetadataQuery } from '$lib/server/trino/client.js'; -import { getUserId } from '$lib/server/auth-utils.js'; +import { getTrinoUser, getUserId } from '$lib/server/auth-utils.js'; import { completionEnabled } from '$lib/server/feature-flags.js'; import { quoteIdentifier, quoteStringLiteral } from '$lib/editor/identifiers.js'; import type { RequestHandler } from './$types'; @@ -30,7 +30,7 @@ export const GET: RequestHandler = async ({ url, locals }) => { error(400, 'No Trino connection configured'); } - const user = locals.user?.username ?? 'anonymous'; + const user = getTrinoUser(locals); const raw = Object.fromEntries(url.searchParams.entries()); const parsed = ParamsSchema.safeParse(raw); diff --git a/src/routes/(app)/api/trino/query/+server.ts b/src/routes/(app)/api/trino/query/+server.ts index e726f836..9d419839 100644 --- a/src/routes/(app)/api/trino/query/+server.ts +++ b/src/routes/(app)/api/trino/query/+server.ts @@ -1,6 +1,6 @@ import { json } from '@sveltejs/kit'; import { z } from 'zod'; -import { getUserId } from '$lib/server/auth-utils.js'; +import { getTrinoUser, getUserId } from '$lib/server/auth-utils.js'; import { startScript, getQuerySnapshots, @@ -33,7 +33,7 @@ export const POST: RequestHandler = async ({ request, locals }) => { return json({ error: z.prettifyError(parsed.error) }, { status: 400 }); } - const user = locals.user?.username ?? 'anonymous'; + const user = getTrinoUser(locals); const { statements, tabId, catalog, schema } = parsed.data; // Fire-and-forget — the script orchestrator runs in the background. diff --git a/src/routes/(app)/trino/+page.server.ts b/src/routes/(app)/trino/+page.server.ts index 3affaae5..995f9fc7 100644 --- a/src/routes/(app)/trino/+page.server.ts +++ b/src/routes/(app)/trino/+page.server.ts @@ -1,7 +1,7 @@ import { fail } from '@sveltejs/kit'; import { superValidate, message, type Infer, type InferIn } from 'sveltekit-superforms'; import { zod4 as zod } from 'sveltekit-superforms/adapters'; -import { getUserId } from '$lib/server/auth-utils.js'; +import { getTrinoUser, getUserId } from '$lib/server/auth-utils.js'; import { getAllQuerySummaries, resetTabQueries } from '$lib/server/trino/queries.js'; import { trinoConfigured, @@ -56,7 +56,7 @@ export const actions: Actions = { } const userId = getUserId(locals); - const user = locals.user?.username ?? 'anonymous'; + const user = getTrinoUser(locals); const { connectionUrl, authType, authUsername, authPassword } = form.data; const config = { url: connectionUrl, From 07b74cf9d02357406b33a19078873fabd2e6edad Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 20 Oct 2026 15:08:40 +0200 Subject: [PATCH 3/7] test(e2e): assert queries run as the OIDC user (#371) The mock Trino now records X-Trino-User, so the query API test can check the impersonated user matches the signed-in user. --- e2e/support/mock-trino.ts | 2 ++ e2e/support/start-mock-trino.ts | 11 ++++++++--- e2e/trino/query-api.spec.ts | 20 +++++++++++++++++++- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/e2e/support/mock-trino.ts b/e2e/support/mock-trino.ts index f1d19bac..2734028a 100644 --- a/e2e/support/mock-trino.ts +++ b/e2e/support/mock-trino.ts @@ -9,6 +9,8 @@ export interface MockTrinoLogEntry { event: 'submitted' | 'cancelled'; sql: string; queryId: string; + /** `X-Trino-User` of the submit; absent on cancellations. */ + user?: string; } /** diff --git a/e2e/support/start-mock-trino.ts b/e2e/support/start-mock-trino.ts index 2610438e..7b182c3a 100644 --- a/e2e/support/start-mock-trino.ts +++ b/e2e/support/start-mock-trino.ts @@ -1,5 +1,5 @@ import * as http from 'node:http'; -import { MOCK_TRINO_PORT } from './mock-trino'; +import { MOCK_TRINO_PORT, type MockTrinoLogEntry } from './mock-trino'; /** SQL substring → fixture response. First match wins; fallback is a 3-row success. */ const routes: [string, object][] = [ @@ -134,7 +134,7 @@ interface MockQuery { } const queries = new Map(); -const log: { event: 'submitted' | 'cancelled'; sql: string; queryId: string }[] = []; +const log: MockTrinoLogEntry[] = []; const released = new Set(); const waiters = new Map void)[]>(); let queryCounter = 0; @@ -300,7 +300,12 @@ http // Logged on arrival, so tests can act while a gated submit is still pending. const id = `q-mock-${++queryCounter}`; queries.set(id, { sql: body, pages: [] }); - log.push({ event: 'submitted', sql: body, queryId: id }); + log.push({ + event: 'submitted', + sql: body, + queryId: id, + user: req.headers['x-trino-user'] as string | undefined + }); const gate = /GATE:([\w-]+)/.exec(body)?.[1]; if (gate) await waitForRelease(gate); diff --git a/e2e/trino/query-api.spec.ts b/e2e/trino/query-api.spec.ts index b9e3bf00..bec22c3d 100644 --- a/e2e/trino/query-api.spec.ts +++ b/e2e/trino/query-api.spec.ts @@ -1,6 +1,11 @@ import { test, expect, type APIRequestContext } from '../support/fixtures.js'; import { randomUUID } from 'node:crypto'; -import { mockMarker, mockTrinoStatements, releaseMockTrino } from '../support/mock-trino'; +import { + mockMarker, + mockTrinoLog, + mockTrinoStatements, + releaseMockTrino +} from '../support/mock-trino'; /** * Trino API endpoints end to end: authentication through the server hooks and @@ -75,6 +80,19 @@ test.describe('Trino API', () => { expect(await snapshots(page.request, tabId)).toEqual([]); }); + test('runs statements as the OIDC user', async ({ page }) => { + const marker = mockMarker(); + await page.request.post('/api/trino/query', { + data: { statements: [`SELECT 1 /* ${marker} */`], tabId: randomUUID() } + }); + + const session = await (await page.request.get('/api/auth/get-session')).json(); + expect(session.user.username).toMatch(/^testuser\d+$/); + await expect + .poll(async () => (await mockTrinoLog(marker)).map((e) => e.user)) + .toEqual([session.user.username]); + }); + test('rejects an invalid request', async ({ page }) => { const res = await page.request.post('/api/trino/query', { data: { statements: ['SELECT 1'], tabId: 'not-a-uuid' } From 4238dbadfa7b93fdad856b486bdd6a62ed1b3d97 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Wed, 21 Oct 2026 10:45:17 +0200 Subject: [PATCH 4/7] fix(auth): refresh OIDC username on login, show form error without one (#371) - Set overrideUserInfo on the OIDC provider, so a changed Trino user claim reaches existing users on their next login, not only new ones. - Drop the `|| ''` fallback for the username: better-auth rejects a sign-in without email before creating the user, so an empty username is never stored. - Split findTrinoUser (returns null) from getTrinoUser (fails with 500). The connection-test action now returns a localised form error (trino_user_missing) instead of the error page. - Mock OIDC server: a test can sign in as a fixed user with chosen claims via the mock_oidc_user cookie. - E2E: a later login updates the username from the claim. --- e2e/auth/auth.spec.ts | 43 +++++++++++++++++++++++++- e2e/support/mock-oidc-server.ts | 23 ++++++++++++-- messages/de.json | 1 + messages/en.json | 1 + src/lib/server/auth-utils.test.ts | 12 ++++++- src/lib/server/auth-utils.ts | 11 +++++-- src/lib/server/auth.ts | 7 ++++- src/routes/(app)/trino/+page.server.ts | 11 +++++-- 8 files changed, 99 insertions(+), 10 deletions(-) diff --git a/e2e/auth/auth.spec.ts b/e2e/auth/auth.spec.ts index 2e179336..f7229eb6 100644 --- a/e2e/auth/auth.spec.ts +++ b/e2e/auth/auth.spec.ts @@ -53,13 +53,54 @@ test.describe('Authentication', () => { headers: { origin: baseURL! }, data: { username: 'someone-else' } }); - expect(res.ok()).toBe(false); + expect(res.status()).toBe(403); expect(await res.json()).toMatchObject({ message: 'username is not allowed to be set' }); const session = await (await page.request.get('/api/auth/get-session')).json(); expect(session.user.username).toMatch(/^testuser\d+$/); }); + test('a later login refreshes the username from the OIDC claim', async ({ browser }) => { + const context = await browser.newContext({ storageState: { cookies: [], origins: [] } }); + const page = await context.newPage(); + const sub = `relogin-${crypto.randomUUID()}`; + + const loginAs = async (preferred_username: string) => { + await context.clearCookies(); + await context.addCookies([ + { + name: 'mock_oidc_user', + value: encodeURIComponent( + JSON.stringify({ + sub, + name: 'Relogin User', + email: `${sub}@example.com`, + preferred_username + }) + ), + domain: 'localhost', + path: '/' + } + ]); + await page.goto('/'); + await waitForHydration(page); + await page.getByRole('button', { name: /sign in with sso/i }).click(); + await expect(page.getByRole('heading', { name: 'Dashboard' })).toBeVisible(); + return (await (await page.request.get('/api/auth/get-session')).json()).user; + }; + + try { + const first = await loginAs(`${sub}-old`); + expect(first.username).toBe(`${sub}-old`); + + const second = await loginAs(`${sub}-new`); + expect(second.id).toBe(first.id); + expect(second.username).toBe(`${sub}-new`); + } finally { + await context.close(); + } + }); + test('user menu shows sign out option', async ({ page }) => { await page.goto('/'); diff --git a/e2e/support/mock-oidc-server.ts b/e2e/support/mock-oidc-server.ts index c82c2a90..d6899758 100644 --- a/e2e/support/mock-oidc-server.ts +++ b/e2e/support/mock-oidc-server.ts @@ -24,10 +24,29 @@ export async function startMockOidc(): Promise { // Map sub → profile claims so the userinfo endpoint can look them up // from the access token instead of relying on shared mutable state. - const users = new Map(); + type Profile = { name: string; email: string; preferred_username: string }; + const users = new Map(); + + // A test can log in as a fixed user with chosen claims by setting the + // `mock_oidc_user` cookie (JSON `{ sub, ...profile }`) before signing in. + // The authorise request carries the cookie; the token request only the code. + const fixedUsers = new Map(); + server.service.on('beforeAuthorizeRedirect', ({ url }, req) => { + const cookie = (req.headers.cookie ?? '').match(/(?:^|;\s*)mock_oidc_user=([^;]+)/)?.[1]; + const code = url.searchParams.get('code'); + if (cookie && code) fixedUsers.set(code, JSON.parse(decodeURIComponent(cookie))); + }); // Add OIDC profile claims to every issued token - server.service.on('beforeTokenSigning', (token) => { + server.service.on('beforeTokenSigning', (token, req) => { + const fixed = fixedUsers.get((req.body as { code?: string }).code ?? ''); + if (fixed) { + const { sub, ...profile } = fixed; + users.set(sub, profile); + token.payload.sub = sub; + Object.assign(token.payload, profile); + return; + } userCounter++; const sub = `mock-user-${String(userCounter).padStart(3, '0')}`; const profile = { diff --git a/messages/de.json b/messages/de.json index b4d25409..c51a4b68 100644 --- a/messages/de.json +++ b/messages/de.json @@ -78,6 +78,7 @@ "trino_connection_test_failed": "Verbindung zu Trino fehlgeschlagen – bitte URL und Zugangsdaten prüfen.", "trino_connection_save_failed": "Die Verbindung konnte nicht gespeichert werden. Bitte erneut versuchen.", "trino_connection_env_managed": "Die Verbindung wird über Umgebungsvariablen verwaltet.", + "trino_user_missing": "Ihrem Konto ist kein Trino-Benutzername zugeordnet. Bitte wenden Sie sich an Ihre Administration.", "trino_cancel_query": "Abbrechen", "trino_state_submitting": "Wird eingereicht", "trino_state_queued": "Warteschlange", diff --git a/messages/en.json b/messages/en.json index 4cb3cb1c..8e1bd348 100644 --- a/messages/en.json +++ b/messages/en.json @@ -78,6 +78,7 @@ "trino_connection_test_failed": "Could not connect to Trino — check the URL and credentials.", "trino_connection_save_failed": "Could not save the connection. Please try again.", "trino_connection_env_managed": "The connection is managed via environment variables.", + "trino_user_missing": "Your account has no Trino username. Contact your administrator.", "trino_cancel_query": "Cancel", "trino_state_submitting": "Submitting", "trino_state_queued": "Queued", diff --git a/src/lib/server/auth-utils.test.ts b/src/lib/server/auth-utils.test.ts index 27724b1a..af7d2c45 100644 --- a/src/lib/server/auth-utils.test.ts +++ b/src/lib/server/auth-utils.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi } from 'vitest'; -import { getTrinoUser, getUserId } from './auth-utils.js'; +import { findTrinoUser, getTrinoUser, getUserId } from './auth-utils.js'; describe('getUserId', () => { it('returns the user ID when user is present', () => { @@ -37,3 +37,13 @@ describe('getTrinoUser', () => { expect(logger.error).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); }); }); + +describe('findTrinoUser', () => { + const logger = { error: vi.fn() }; + + it('returns null instead of "anonymous" when the username is missing', () => { + const locals = { user: { id: 'u1', username: null }, logger } as unknown as App.Locals; + expect(findTrinoUser(locals)).toBeNull(); + expect(logger.error).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); + }); +}); diff --git a/src/lib/server/auth-utils.ts b/src/lib/server/auth-utils.ts index 6769b2c7..c74a1595 100644 --- a/src/lib/server/auth-utils.ts +++ b/src/lib/server/auth-utils.ts @@ -7,11 +7,16 @@ export function getUserId(locals: App.Locals): string { /** * The user to impersonate in Trino. Without OIDC there is no signed-in user, * so queries run as 'anonymous'. A signed-in user without a username means - * sign-up lost the OIDC claim; fail instead of silently running as 'anonymous'. + * sign-up lost the OIDC claim; return null instead of falling back to 'anonymous'. */ -export function getTrinoUser(locals: App.Locals): string { +export function findTrinoUser(locals: App.Locals): string | null { if (!locals.user) return 'anonymous'; if (locals.user.username) return locals.user.username; locals.logger.error({ user_id: locals.user.id }, 'Signed-in user has no Trino username'); - error(500, 'User has no Trino username'); + return null; +} + +/** Like {@link findTrinoUser}, but fails the request when there is no username. */ +export function getTrinoUser(locals: App.Locals): string { + return findTrinoUser(locals) ?? error(500, 'User has no Trino username'); } diff --git a/src/lib/server/auth.ts b/src/lib/server/auth.ts index 855f3a38..3a8f9e8d 100644 --- a/src/lib/server/auth.ts +++ b/src/lib/server/auth.ts @@ -94,6 +94,9 @@ export const auth = betterAuth({ clientSecret: env.STACKABLE_COCKPIT_OIDC_CLIENT_SECRET!, scopes: oidcScopes, pkce: true, + // Refresh the user from the IdP on every login, so changes to the + // Trino user claim reach existing users instead of only new ones. + overrideUserInfo: true, mapProfileToUser: async (profile) => { const fullName = [profile.given_name, profile.family_name] .filter(Boolean) @@ -115,7 +118,9 @@ export const auth = betterAuth({ email: profile.email || '', emailVerified: profile.email_verified ?? false, image: profile.picture || null, - username: trinoUser || profile.preferred_username || profile.email || '' + // No `|| ''`: without an email better-auth rejects the sign-in before + // creating the user, so this never stores an empty username. + username: trinoUser || profile.preferred_username || profile.email }; } } diff --git a/src/routes/(app)/trino/+page.server.ts b/src/routes/(app)/trino/+page.server.ts index 995f9fc7..a9649095 100644 --- a/src/routes/(app)/trino/+page.server.ts +++ b/src/routes/(app)/trino/+page.server.ts @@ -1,7 +1,7 @@ import { fail } from '@sveltejs/kit'; import { superValidate, message, type Infer, type InferIn } from 'sveltekit-superforms'; import { zod4 as zod } from 'sveltekit-superforms/adapters'; -import { getTrinoUser, getUserId } from '$lib/server/auth-utils.js'; +import { findTrinoUser, getUserId } from '$lib/server/auth-utils.js'; import { getAllQuerySummaries, resetTabQueries } from '$lib/server/trino/queries.js'; import { trinoConfigured, @@ -56,7 +56,14 @@ export const actions: Actions = { } const userId = getUserId(locals); - const user = getTrinoUser(locals); + const user = findTrinoUser(locals); + if (!user) { + return message( + form, + { type: 'error', message: m.trino_user_missing() } satisfies ConnectionMessage, + { status: 500 } + ); + } const { connectionUrl, authType, authUsername, authPassword } = form.data; const config = { url: connectionUrl, From fec74facc1f53e12c25cbdacbe3eeba88470cdd2 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Wed, 21 Oct 2026 10:57:31 +0200 Subject: [PATCH 5/7] test(e2e): assert catalog and completion queries run as the OIDC user (#371) --- e2e/trino/query-api.spec.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/e2e/trino/query-api.spec.ts b/e2e/trino/query-api.spec.ts index bec22c3d..8e6116b0 100644 --- a/e2e/trino/query-api.spec.ts +++ b/e2e/trino/query-api.spec.ts @@ -93,6 +93,23 @@ test.describe('Trino API', () => { .toEqual([session.user.username]); }); + // The marker goes into a name the endpoint puts in its SQL, so the mock log + // can find the metadata statement. + for (const [endpoint, query] of [ + ['catalog', (marker: string) => `level=schemas&catalog=${marker}`], + ['completion/metadata', (marker: string) => `level=tables&catalog=tpch&schema=${marker}`] + ] as const) { + test(`${endpoint} runs metadata queries as the OIDC user`, async ({ page }) => { + const marker = mockMarker(); + const res = await page.request.get(`/api/trino/${endpoint}?${query(marker)}`); + expect(res.status()).toBe(200); + + const session = await (await page.request.get('/api/auth/get-session')).json(); + expect(session.user.username).toMatch(/^testuser\d+$/); + expect((await mockTrinoLog(marker)).map((e) => e.user)).toEqual([session.user.username]); + }); + } + test('rejects an invalid request', async ({ page }) => { const res = await page.request.post('/api/trino/query', { data: { statements: ['SELECT 1'], tabId: 'not-a-uuid' } From 2adba17d12e4a397e74fd0c01b69cece00759359 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Wed, 21 Oct 2026 16:42:34 +0200 Subject: [PATCH 6/7] refactor: rename user.username to trinoUser The field holds the Trino user we impersonate, not a login name. Rename the column to trino_user in the initial migration; there is no release yet, so no new migration is needed. --- e2e/auth/auth.spec.ts | 17 +++++++----- e2e/support/mock-trino.ts | 2 +- e2e/trino/query-api.spec.ts | 8 +++--- src/lib/server/auth-schema.ts | 2 +- src/lib/server/auth-utils.test.ts | 12 ++++----- src/lib/server/auth-utils.ts | 10 +++---- src/lib/server/auth.test.ts | 16 ++++++------ src/lib/server/auth.ts | 26 +++++++++---------- .../server/migrations/0000_initial_schema.sql | 2 +- .../server/migrations/meta/0000_snapshot.json | 4 +-- .../server/migrations/meta/0001_snapshot.json | 4 +-- .../server/migrations/meta/0002_snapshot.json | 4 +-- .../server/migrations/meta/0003_snapshot.json | 4 +-- .../(app)/api/trino/catalog/server.test.ts | 2 +- .../trino/completion/metadata/server.test.ts | 2 +- .../(app)/api/trino/query/server.test.ts | 2 +- 16 files changed, 60 insertions(+), 57 deletions(-) diff --git a/e2e/auth/auth.spec.ts b/e2e/auth/auth.spec.ts index f7229eb6..de54ddcd 100644 --- a/e2e/auth/auth.spec.ts +++ b/e2e/auth/auth.spec.ts @@ -48,19 +48,22 @@ test.describe('Authentication', () => { await expect(page.getByRole('heading', { name: 'Dashboard' })).toBeVisible(); }); - test('username cannot be changed through the update-user endpoint', async ({ page, baseURL }) => { + test('trinoUser cannot be changed through the update-user endpoint', async ({ + page, + baseURL + }) => { const res = await page.request.post('/api/auth/update-user', { headers: { origin: baseURL! }, - data: { username: 'someone-else' } + data: { trinoUser: 'someone-else' } }); expect(res.status()).toBe(403); - expect(await res.json()).toMatchObject({ message: 'username is not allowed to be set' }); + expect(await res.json()).toMatchObject({ message: 'trinoUser is not allowed to be set' }); const session = await (await page.request.get('/api/auth/get-session')).json(); - expect(session.user.username).toMatch(/^testuser\d+$/); + expect(session.user.trinoUser).toMatch(/^testuser\d+$/); }); - test('a later login refreshes the username from the OIDC claim', async ({ browser }) => { + test('a later login refreshes the Trino user from the OIDC claim', async ({ browser }) => { const context = await browser.newContext({ storageState: { cookies: [], origins: [] } }); const page = await context.newPage(); const sub = `relogin-${crypto.randomUUID()}`; @@ -91,11 +94,11 @@ test.describe('Authentication', () => { try { const first = await loginAs(`${sub}-old`); - expect(first.username).toBe(`${sub}-old`); + expect(first.trinoUser).toBe(`${sub}-old`); const second = await loginAs(`${sub}-new`); expect(second.id).toBe(first.id); - expect(second.username).toBe(`${sub}-new`); + expect(second.trinoUser).toBe(`${sub}-new`); } finally { await context.close(); } diff --git a/e2e/support/mock-trino.ts b/e2e/support/mock-trino.ts index d5f6447e..2734028a 100644 --- a/e2e/support/mock-trino.ts +++ b/e2e/support/mock-trino.ts @@ -22,7 +22,7 @@ export function mockMarker(): string { } /** Statements containing `marker` that reached the mock, and their cancellations. */ -async function mockTrinoLog(marker: string): Promise { +export async function mockTrinoLog(marker: string): Promise { const res = await fetch(`${MOCK_TRINO_URL}/__mock/log?marker=${encodeURIComponent(marker)}`); if (!res.ok) { throw new Error(`Mock Trino log request failed: ${res.status} ${res.statusText}`); diff --git a/e2e/trino/query-api.spec.ts b/e2e/trino/query-api.spec.ts index 8e6116b0..1f7adb67 100644 --- a/e2e/trino/query-api.spec.ts +++ b/e2e/trino/query-api.spec.ts @@ -87,10 +87,10 @@ test.describe('Trino API', () => { }); const session = await (await page.request.get('/api/auth/get-session')).json(); - expect(session.user.username).toMatch(/^testuser\d+$/); + expect(session.user.trinoUser).toMatch(/^testuser\d+$/); await expect .poll(async () => (await mockTrinoLog(marker)).map((e) => e.user)) - .toEqual([session.user.username]); + .toEqual([session.user.trinoUser]); }); // The marker goes into a name the endpoint puts in its SQL, so the mock log @@ -105,8 +105,8 @@ test.describe('Trino API', () => { expect(res.status()).toBe(200); const session = await (await page.request.get('/api/auth/get-session')).json(); - expect(session.user.username).toMatch(/^testuser\d+$/); - expect((await mockTrinoLog(marker)).map((e) => e.user)).toEqual([session.user.username]); + expect(session.user.trinoUser).toMatch(/^testuser\d+$/); + expect((await mockTrinoLog(marker)).map((e) => e.user)).toEqual([session.user.trinoUser]); }); } diff --git a/src/lib/server/auth-schema.ts b/src/lib/server/auth-schema.ts index 03855c16..4dbd1209 100644 --- a/src/lib/server/auth-schema.ts +++ b/src/lib/server/auth-schema.ts @@ -12,7 +12,7 @@ export const user = pgTable('user', { .defaultNow() .$onUpdate(() => /* @__PURE__ */ new Date()) .notNull(), - username: text('username') + trinoUser: text('trino_user') }); export const session = pgTable( diff --git a/src/lib/server/auth-utils.test.ts b/src/lib/server/auth-utils.test.ts index af7d2c45..5001c6be 100644 --- a/src/lib/server/auth-utils.test.ts +++ b/src/lib/server/auth-utils.test.ts @@ -21,8 +21,8 @@ describe('getUserId', () => { describe('getTrinoUser', () => { const logger = { error: vi.fn() }; - it('returns the username of the signed-in user', () => { - const locals = { user: { id: 'u1', username: 'alice' }, logger } as unknown as App.Locals; + it('returns the Trino user of the signed-in user', () => { + const locals = { user: { id: 'u1', trinoUser: 'alice' }, logger } as unknown as App.Locals; expect(getTrinoUser(locals)).toBe('alice'); }); @@ -31,8 +31,8 @@ describe('getTrinoUser', () => { expect(getTrinoUser(locals)).toBe('anonymous'); }); - it('fails instead of falling back to "anonymous" when the username is missing', () => { - const locals = { user: { id: 'u1', username: null }, logger } as unknown as App.Locals; + it('fails instead of falling back to "anonymous" when the Trino user is missing', () => { + const locals = { user: { id: 'u1', trinoUser: null }, logger } as unknown as App.Locals; expect(() => getTrinoUser(locals)).toThrow(expect.objectContaining({ status: 500 })); expect(logger.error).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); }); @@ -41,8 +41,8 @@ describe('getTrinoUser', () => { describe('findTrinoUser', () => { const logger = { error: vi.fn() }; - it('returns null instead of "anonymous" when the username is missing', () => { - const locals = { user: { id: 'u1', username: null }, logger } as unknown as App.Locals; + it('returns null instead of "anonymous" when the Trino user is missing', () => { + const locals = { user: { id: 'u1', trinoUser: null }, logger } as unknown as App.Locals; expect(findTrinoUser(locals)).toBeNull(); expect(logger.error).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); }); diff --git a/src/lib/server/auth-utils.ts b/src/lib/server/auth-utils.ts index c74a1595..99f65a8e 100644 --- a/src/lib/server/auth-utils.ts +++ b/src/lib/server/auth-utils.ts @@ -6,17 +6,17 @@ export function getUserId(locals: App.Locals): string { /** * The user to impersonate in Trino. Without OIDC there is no signed-in user, - * so queries run as 'anonymous'. A signed-in user without a username means + * so queries run as 'anonymous'. A signed-in user without a Trino user means * sign-up lost the OIDC claim; return null instead of falling back to 'anonymous'. */ export function findTrinoUser(locals: App.Locals): string | null { if (!locals.user) return 'anonymous'; - if (locals.user.username) return locals.user.username; - locals.logger.error({ user_id: locals.user.id }, 'Signed-in user has no Trino username'); + if (locals.user.trinoUser) return locals.user.trinoUser; + locals.logger.error({ user_id: locals.user.id }, 'Signed-in user has no Trino user'); return null; } -/** Like {@link findTrinoUser}, but fails the request when there is no username. */ +/** Like {@link findTrinoUser}, but fails the request when there is no Trino user. */ export function getTrinoUser(locals: App.Locals): string { - return findTrinoUser(locals) ?? error(500, 'User has no Trino username'); + return findTrinoUser(locals) ?? error(500, 'User has no Trino user'); } diff --git a/src/lib/server/auth.test.ts b/src/lib/server/auth.test.ts index c3c3c83f..b188d8cf 100644 --- a/src/lib/server/auth.test.ts +++ b/src/lib/server/auth.test.ts @@ -8,29 +8,29 @@ const create = (data: Record, path: string) => const update = (data: Record, path: string) => hooks.update!.before!(data as never, { path } as never); -describe('user.additionalFields.username', () => { +describe('user.additionalFields.trinoUser', () => { it('is taken from the OIDC provider profile', () => { expect( - parseAdditionalUserInputFromProviderProfile(auth.options, { username: 'alice' }, 'create') - ).toMatchObject({ username: 'alice' }); + parseAdditionalUserInputFromProviderProfile(auth.options, { trinoUser: 'alice' }, 'create') + ).toMatchObject({ trinoUser: 'alice' }); }); it('may be written by the OIDC callback', async () => { await expect( - create({ email: 'alice@example.com', username: 'alice' }, '/oauth2/callback/:providerId') + create({ email: 'alice@example.com', trinoUser: 'alice' }, '/oauth2/callback/:providerId') ).resolves.toBeUndefined(); }); it('rejects a rewrite through the built-in update-user endpoint', async () => { - await expect(update({ username: 'someone-else' }, '/update-user')).rejects.toThrow( - /username is not allowed to be set/ + await expect(update({ trinoUser: 'someone-else' }, '/update-user')).rejects.toThrow( + /trinoUser is not allowed to be set/ ); }); it('rejects a rewrite smuggled in at sign-up', async () => { await expect( - create({ email: 'eve@example.com', username: 'someone-else' }, '/sign-up/email') - ).rejects.toThrow(/username is not allowed to be set/); + create({ email: 'eve@example.com', trinoUser: 'someone-else' }, '/sign-up/email') + ).rejects.toThrow(/trinoUser is not allowed to be set/); }); it('leaves other user updates alone', async () => { diff --git a/src/lib/server/auth.ts b/src/lib/server/auth.ts index 3a8f9e8d..8564a0ed 100644 --- a/src/lib/server/auth.ts +++ b/src/lib/server/auth.ts @@ -16,16 +16,16 @@ const env = envModule?.env ?? (process.env as Record const getRequestEvent = appServer?.getRequestEvent ?? (() => undefined as unknown as RequestEvent); const log = loggingModule?.logger.child({ module: 'auth' }); -// The username is the Trino user we impersonate, so only the OIDC callback may +// trinoUser is the Trino user we impersonate, so only the OIDC callback may // write it. Marking the field `input: false` would also block that: better-auth // drops such fields from the provider profile on sign-up (see #371). Database // hooks see the endpoint's route template, not the request path. -const usernameWritePath = '/oauth2/callback/:providerId'; +const trinoUserWritePath = '/oauth2/callback/:providerId'; -function guardUsernameWrite(data: Record, context: { path?: string } | null) { - if (!('username' in data) || context?.path === usernameWritePath) return; - log?.warn({ path: context?.path }, 'Rejected username write outside the OIDC callback'); - throw new APIError('FORBIDDEN', { message: 'username is not allowed to be set' }); +function guardTrinoUserWrite(data: Record, context: { path?: string } | null) { + if (!('trinoUser' in data) || context?.path === trinoUserWritePath) return; + log?.warn({ path: context?.path }, 'Rejected trinoUser write outside the OIDC callback'); + throw new APIError('FORBIDDEN', { message: 'trinoUser is not allowed to be set' }); } const trinoUserClaim = env.STACKABLE_COCKPIT_TRINO_USER_CLAIM ?? 'preferred_username'; @@ -68,7 +68,7 @@ export const auth = betterAuth({ }, user: { additionalFields: { - username: { + trinoUser: { type: 'string', required: false } @@ -76,8 +76,8 @@ export const auth = betterAuth({ }, databaseHooks: { user: { - create: { before: async (user, context) => guardUsernameWrite(user, context) }, - update: { before: async (user, context) => guardUsernameWrite(user, context) } + create: { before: async (user, context) => guardTrinoUserWrite(user, context) }, + update: { before: async (user, context) => guardTrinoUserWrite(user, context) } } }, plugins: [ @@ -101,8 +101,8 @@ export const auth = betterAuth({ const fullName = [profile.given_name, profile.family_name] .filter(Boolean) .join(' '); - const trinoUser = profile[trinoUserClaim]; - if (trinoUser) { + const claimedTrinoUser = profile[trinoUserClaim]; + if (claimedTrinoUser) { log?.debug( { trino_user_claim: trinoUserClaim }, 'Resolved Trino user from OIDC claim' @@ -119,8 +119,8 @@ export const auth = betterAuth({ emailVerified: profile.email_verified ?? false, image: profile.picture || null, // No `|| ''`: without an email better-auth rejects the sign-in before - // creating the user, so this never stores an empty username. - username: trinoUser || profile.preferred_username || profile.email + // creating the user, so this never stores an empty Trino user. + trinoUser: claimedTrinoUser || profile.preferred_username || profile.email }; } } diff --git a/src/lib/server/migrations/0000_initial_schema.sql b/src/lib/server/migrations/0000_initial_schema.sql index c61520f9..7759cd54 100644 --- a/src/lib/server/migrations/0000_initial_schema.sql +++ b/src/lib/server/migrations/0000_initial_schema.sql @@ -47,7 +47,7 @@ CREATE TABLE "user" ( "image" text, "created_at" timestamp DEFAULT now() NOT NULL, "updated_at" timestamp DEFAULT now() NOT NULL, - "username" text, + "trino_user" text, CONSTRAINT "user_email_unique" UNIQUE("email") ); --> statement-breakpoint diff --git a/src/lib/server/migrations/meta/0000_snapshot.json b/src/lib/server/migrations/meta/0000_snapshot.json index a44a2f7a..c9fef698 100644 --- a/src/lib/server/migrations/meta/0000_snapshot.json +++ b/src/lib/server/migrations/meta/0000_snapshot.json @@ -358,8 +358,8 @@ "notNull": true, "default": "now()" }, - "username": { - "name": "username", + "trino_user": { + "name": "trino_user", "type": "text", "primaryKey": false, "notNull": false diff --git a/src/lib/server/migrations/meta/0001_snapshot.json b/src/lib/server/migrations/meta/0001_snapshot.json index b800842b..8974a663 100644 --- a/src/lib/server/migrations/meta/0001_snapshot.json +++ b/src/lib/server/migrations/meta/0001_snapshot.json @@ -581,8 +581,8 @@ "notNull": true, "default": "now()" }, - "username": { - "name": "username", + "trino_user": { + "name": "trino_user", "type": "text", "primaryKey": false, "notNull": false diff --git a/src/lib/server/migrations/meta/0002_snapshot.json b/src/lib/server/migrations/meta/0002_snapshot.json index cb745d21..20f01c74 100644 --- a/src/lib/server/migrations/meta/0002_snapshot.json +++ b/src/lib/server/migrations/meta/0002_snapshot.json @@ -606,8 +606,8 @@ "notNull": true, "default": "now()" }, - "username": { - "name": "username", + "trino_user": { + "name": "trino_user", "type": "text", "primaryKey": false, "notNull": false diff --git a/src/lib/server/migrations/meta/0003_snapshot.json b/src/lib/server/migrations/meta/0003_snapshot.json index 107ec55c..8a056a1b 100644 --- a/src/lib/server/migrations/meta/0003_snapshot.json +++ b/src/lib/server/migrations/meta/0003_snapshot.json @@ -603,8 +603,8 @@ "notNull": true, "default": "now()" }, - "username": { - "name": "username", + "trino_user": { + "name": "trino_user", "type": "text", "primaryKey": false, "notNull": false diff --git a/src/routes/(app)/api/trino/catalog/server.test.ts b/src/routes/(app)/api/trino/catalog/server.test.ts index 37956e3d..cb6537ac 100644 --- a/src/routes/(app)/api/trino/catalog/server.test.ts +++ b/src/routes/(app)/api/trino/catalog/server.test.ts @@ -13,7 +13,7 @@ const fakeClient = { fake: true } as unknown as TrinoClient; function mockEvent( params: Record, - user: object | null = { id: 'u1', username: 'alice' } + user: object | null = { id: 'u1', trinoUser: 'alice' } ) { const url = new URL('http://localhost/api/trino/catalog'); for (const [k, v] of Object.entries(params)) url.searchParams.set(k, v); diff --git a/src/routes/(app)/api/trino/completion/metadata/server.test.ts b/src/routes/(app)/api/trino/completion/metadata/server.test.ts index cce3e18b..7e2cca86 100644 --- a/src/routes/(app)/api/trino/completion/metadata/server.test.ts +++ b/src/routes/(app)/api/trino/completion/metadata/server.test.ts @@ -24,7 +24,7 @@ function mockEvent(params: Record) { for (const [k, v] of Object.entries(params)) url.searchParams.set(k, v); return { url, - locals: { logger: { debug: vi.fn(), info: vi.fn() }, user: { id: 'u1', username: 'alice' } } + locals: { logger: { debug: vi.fn(), info: vi.fn() }, user: { id: 'u1', trinoUser: 'alice' } } } as unknown as Parameters[0]; } diff --git a/src/routes/(app)/api/trino/query/server.test.ts b/src/routes/(app)/api/trino/query/server.test.ts index e60a3160..dba5f906 100644 --- a/src/routes/(app)/api/trino/query/server.test.ts +++ b/src/routes/(app)/api/trino/query/server.test.ts @@ -25,7 +25,7 @@ const TAB_ID = '6f1c2a4e-8b3d-4c5e-9f7a-1b2c3d4e5f60'; const fakeClient = { fake: true } as unknown as TrinoClient; function mockLocals( - user: { id: string; username?: string } | null = { id: 'u1', username: 'alice' } + user: { id: string; trinoUser?: string } | null = { id: 'u1', trinoUser: 'alice' } ) { return { logger: { trace: vi.fn(), debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, From ae5c4f61c3fe519a1d2f97ce784af4dba77e2e0b Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 22 Oct 2026 14:11:25 +0200 Subject: [PATCH 7/7] Show missing Trino user error in the query editor Return the query route error as JSON so the query runner shows it instead of "connection lost", log the missing Trino user at warn level, and tell affected users to sign in again. --- messages/de.json | 2 +- messages/en.json | 2 +- src/lib/server/auth-utils.test.ts | 8 ++++---- src/lib/server/auth-utils.ts | 2 +- src/routes/(app)/api/trino/query/+server.ts | 9 +++++++-- src/routes/(app)/api/trino/query/server.test.ts | 10 ++++++++++ 6 files changed, 24 insertions(+), 9 deletions(-) diff --git a/messages/de.json b/messages/de.json index cf63cbde..fab0bcdc 100644 --- a/messages/de.json +++ b/messages/de.json @@ -78,7 +78,7 @@ "trino_connection_test_failed": "Verbindung zu Trino fehlgeschlagen – bitte URL und Zugangsdaten prüfen.", "trino_connection_save_failed": "Die Verbindung konnte nicht gespeichert werden. Bitte erneut versuchen.", "trino_connection_env_managed": "Die Verbindung wird über Umgebungsvariablen verwaltet.", - "trino_user_missing": "Ihrem Konto ist kein Trino-Benutzername zugeordnet. Bitte wenden Sie sich an Ihre Administration.", + "trino_user_missing": "Ihrem Konto ist kein Trino-Benutzername zugeordnet. Melden Sie sich ab und erneut an; besteht das Problem weiterhin, wenden Sie sich an Ihre Administration.", "trino_cancel_query": "Abbrechen", "trino_state_submitting": "Wird eingereicht", "trino_state_queued": "Warteschlange", diff --git a/messages/en.json b/messages/en.json index a5c89734..dfd2bbbf 100644 --- a/messages/en.json +++ b/messages/en.json @@ -78,7 +78,7 @@ "trino_connection_test_failed": "Could not connect to Trino — check the URL and credentials.", "trino_connection_save_failed": "Could not save the connection. Please try again.", "trino_connection_env_managed": "The connection is managed via environment variables.", - "trino_user_missing": "Your account has no Trino username. Contact your administrator.", + "trino_user_missing": "Your account has no Trino username. Sign out and sign in again; if the problem persists, contact your administrator.", "trino_cancel_query": "Cancel", "trino_state_submitting": "Submitting", "trino_state_queued": "Queued", diff --git a/src/lib/server/auth-utils.test.ts b/src/lib/server/auth-utils.test.ts index 5001c6be..f794eabd 100644 --- a/src/lib/server/auth-utils.test.ts +++ b/src/lib/server/auth-utils.test.ts @@ -19,7 +19,7 @@ describe('getUserId', () => { }); describe('getTrinoUser', () => { - const logger = { error: vi.fn() }; + const logger = { warn: vi.fn() }; it('returns the Trino user of the signed-in user', () => { const locals = { user: { id: 'u1', trinoUser: 'alice' }, logger } as unknown as App.Locals; @@ -34,16 +34,16 @@ describe('getTrinoUser', () => { it('fails instead of falling back to "anonymous" when the Trino user is missing', () => { const locals = { user: { id: 'u1', trinoUser: null }, logger } as unknown as App.Locals; expect(() => getTrinoUser(locals)).toThrow(expect.objectContaining({ status: 500 })); - expect(logger.error).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); + expect(logger.warn).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); }); }); describe('findTrinoUser', () => { - const logger = { error: vi.fn() }; + const logger = { warn: vi.fn() }; it('returns null instead of "anonymous" when the Trino user is missing', () => { const locals = { user: { id: 'u1', trinoUser: null }, logger } as unknown as App.Locals; expect(findTrinoUser(locals)).toBeNull(); - expect(logger.error).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); + expect(logger.warn).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); }); }); diff --git a/src/lib/server/auth-utils.ts b/src/lib/server/auth-utils.ts index 99f65a8e..618e44ac 100644 --- a/src/lib/server/auth-utils.ts +++ b/src/lib/server/auth-utils.ts @@ -12,7 +12,7 @@ export function getUserId(locals: App.Locals): string { export function findTrinoUser(locals: App.Locals): string | null { if (!locals.user) return 'anonymous'; if (locals.user.trinoUser) return locals.user.trinoUser; - locals.logger.error({ user_id: locals.user.id }, 'Signed-in user has no Trino user'); + locals.logger.warn({ user_id: locals.user.id }, 'Signed-in user has no Trino user'); return null; } diff --git a/src/routes/(app)/api/trino/query/+server.ts b/src/routes/(app)/api/trino/query/+server.ts index 9d419839..8c3a1ba5 100644 --- a/src/routes/(app)/api/trino/query/+server.ts +++ b/src/routes/(app)/api/trino/query/+server.ts @@ -1,6 +1,7 @@ import { json } from '@sveltejs/kit'; import { z } from 'zod'; -import { getTrinoUser, getUserId } from '$lib/server/auth-utils.js'; +import { findTrinoUser, getUserId } from '$lib/server/auth-utils.js'; +import * as m from '$lib/paraglide/messages.js'; import { startScript, getQuerySnapshots, @@ -33,7 +34,11 @@ export const POST: RequestHandler = async ({ request, locals }) => { return json({ error: z.prettifyError(parsed.error) }, { status: 400 }); } - const user = getTrinoUser(locals); + // Return the error as JSON so the query runner shows it instead of "connection lost". + const user = findTrinoUser(locals); + if (!user) { + return json({ error: m.trino_user_missing() }, { status: 500 }); + } const { statements, tabId, catalog, schema } = parsed.data; // Fire-and-forget — the script orchestrator runs in the background. diff --git a/src/routes/(app)/api/trino/query/server.test.ts b/src/routes/(app)/api/trino/query/server.test.ts index dba5f906..9603c1eb 100644 --- a/src/routes/(app)/api/trino/query/server.test.ts +++ b/src/routes/(app)/api/trino/query/server.test.ts @@ -115,6 +115,16 @@ describe('POST /api/trino/query', () => { }); }); + it('returns a JSON error instead of running as anonymous when the Trino user is missing', async () => { + const res = await POST( + postEvent({ statements: ['SELECT 1'], tabId: TAB_ID }, mockLocals({ id: 'u1' })) + ); + + expect(res.status).toBe(500); + expect(await res.json()).toEqual({ error: expect.any(String) }); + expect(startScript).not.toHaveBeenCalled(); + }); + it('logs but does not fail the request when the script orchestrator rejects', async () => { const err = new Error('boom'); vi.mocked(startScript).mockRejectedValue(err);