diff --git a/e2e/auth/auth.spec.ts b/e2e/auth/auth.spec.ts index 0e81b6c2..de54ddcd 100644 --- a/e2e/auth/auth.spec.ts +++ b/e2e/auth/auth.spec.ts @@ -48,6 +48,62 @@ test.describe('Authentication', () => { await expect(page.getByRole('heading', { name: 'Dashboard' })).toBeVisible(); }); + 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: { trinoUser: 'someone-else' } + }); + expect(res.status()).toBe(403); + 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.trinoUser).toMatch(/^testuser\d+$/); + }); + + 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()}`; + + 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.trinoUser).toBe(`${sub}-old`); + + const second = await loginAs(`${sub}-new`); + expect(second.id).toBe(first.id); + expect(second.trinoUser).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 30ba9c26..a3902c21 100644 --- a/e2e/support/mock-oidc-server.ts +++ b/e2e/support/mock-oidc-server.ts @@ -23,10 +23,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/e2e/support/mock-trino.ts b/e2e/support/mock-trino.ts index 5d79ff3c..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; } /** @@ -20,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/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..1f7adb67 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,36 @@ 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.trinoUser).toMatch(/^testuser\d+$/); + await expect + .poll(async () => (await mockTrinoLog(marker)).map((e) => e.user)) + .toEqual([session.user.trinoUser]); + }); + + // 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.trinoUser).toMatch(/^testuser\d+$/); + expect((await mockTrinoLog(marker)).map((e) => e.user)).toEqual([session.user.trinoUser]); + }); + } + test('rejects an invalid request', async ({ page }) => { const res = await page.request.post('/api/trino/query', { data: { statements: ['SELECT 1'], tabId: 'not-a-uuid' } diff --git a/messages/de.json b/messages/de.json index b7ce2634..fab0bcdc 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. 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 d64250a1..dfd2bbbf 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. 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-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 38f63988..f794eabd 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 { findTrinoUser, getTrinoUser, getUserId } from './auth-utils.js'; describe('getUserId', () => { it('returns the user ID when user is present', () => { @@ -17,3 +17,33 @@ describe('getUserId', () => { expect(getUserId(locals)).toBe('anonymous'); }); }); + +describe('getTrinoUser', () => { + 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; + 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 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.warn).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String)); + }); +}); + +describe('findTrinoUser', () => { + 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.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 0bb1e2c7..618e44ac 100644 --- a/src/lib/server/auth-utils.ts +++ b/src/lib/server/auth-utils.ts @@ -1,3 +1,22 @@ +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 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.trinoUser) return locals.user.trinoUser; + locals.logger.warn({ 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 Trino user. */ +export function getTrinoUser(locals: App.Locals): string { + 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 ac7901f4..b188d8cf 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'; -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/ - ); +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.trinoUser', () => { + it('is taken from the OIDC provider profile', () => { + expect( + 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', trinoUser: 'alice' }, '/oauth2/callback/:providerId') + ).resolves.toBeUndefined(); }); - it('rejects a rewrite smuggled in at sign-up', () => { - expect(() => parseUserInput(auth.options, { username: 'someone-else' }, 'create')).toThrow( - /username is not allowed to be set/ + it('rejects a rewrite through the built-in update-user endpoint', async () => { + await expect(update({ trinoUser: 'someone-else' }, '/update-user')).rejects.toThrow( + /trinoUser 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', trinoUser: 'someone-else' }, '/sign-up/email') + ).rejects.toThrow(/trinoUser 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..8564a0ed 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' }); +// 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 trinoUserWritePath = '/oauth2/callback/:providerId'; + +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'; const defaultScopes = ['openid', 'profile', 'email']; @@ -55,13 +68,18 @@ export const auth = betterAuth({ }, user: { additionalFields: { - username: { + trinoUser: { type: 'string', - required: false, - input: false + required: false } } }, + databaseHooks: { + user: { + create: { before: async (user, context) => guardTrinoUserWrite(user, context) }, + update: { before: async (user, context) => guardTrinoUserWrite(user, context) } + } + }, plugins: [ sveltekitCookies(getRequestEvent), openAPI(), @@ -76,12 +94,15 @@ 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) .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' @@ -97,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 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.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/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.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/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.ts b/src/routes/(app)/api/trino/query/+server.ts index e726f836..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 { 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 = locals.user?.username ?? 'anonymous'; + // 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 e60a3160..9603c1eb 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() }, @@ -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); diff --git a/src/routes/(app)/trino/+page.server.ts b/src/routes/(app)/trino/+page.server.ts index 3affaae5..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 { 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 = locals.user?.username ?? 'anonymous'; + 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,