From 9b7060f70d0dec95cfaf7ff0fdaade6713e06e67 Mon Sep 17 00:00:00 2001 From: Isolator acm Date: Sun, 21 Jun 2026 23:15:45 +0200 Subject: [PATCH] fix(auth): show a meaningful error when ClickHouse refuses an authenticated user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A valid IdP login that ClickHouse won't authorize (no CH user, missing grants, or an unverifiable JWT) surfaced as "Session expired" on the login screen, and re-authenticating never helped. Root cause: authedFetch flagged any 401/403 as auth-expired. But getToken() already guarantees a non-expired bearer before the POST, so a 401/403 that survives the single refresh-retry means CH rejected a *valid* login — an authorization/identity problem, not session expiry. Now that case shows an accurate message that names the cause and appends ClickHouse's own reason (e.g. "Code: 516 ... Token could not be verified"). Genuine expiry (no token, or expired + refresh failed) keeps its own path, reworded to "Your session expired — please sign in again." - core/stream.js: new pure authDeniedMessage(status, reason) - net/ch-client.js: classify valid-token 401/403 as authorization denial, passing CH's reason to onSignedOut - ui/app.js: onSignedOut(detail) renders the detail, else the expiry default Verified live on antalya by injecting a non-expired but unverifiable token: the login screen shows the authorization message + AUTHENTICATION_FAILED reason. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01QennTvGKAtJZrv9EpQagef --- src/core/stream.js | 15 +++++++++++++++ src/net/ch-client.js | 9 +++++++-- src/ui/app.js | 7 ++++++- tests/unit/app.test.js | 10 ++++++++++ tests/unit/ch-client.test.js | 13 ++++++++++--- tests/unit/stream.test.js | 18 ++++++++++++++++++ 6 files changed, 66 insertions(+), 6 deletions(-) diff --git a/src/core/stream.js b/src/core/stream.js index 40c0720..c50a598 100644 --- a/src/core/stream.js +++ b/src/core/stream.js @@ -79,3 +79,18 @@ export function parseExceptionText(text) { export function isAuthExpiredBody(text) { return /token_verification_exception|token expired/i.test(text); } + +/** + * Build the login-screen message shown when ClickHouse rejects a *valid* login + * (HTTP 401/403 with a non-expired token) — an authorization/identity problem, + * not session expiry. `reason` is ClickHouse's own text (already run through + * parseExceptionText); it's trimmed/collapsed and appended only when present. + */ +export function authDeniedMessage(status, reason) { + const base = + 'ClickHouse denied your account (HTTP ' + status + "). You're signed in, " + + 'but this server is not authorizing you — your identity may have no ' + + 'ClickHouse user or the required grants.'; + const r = String(reason || '').replace(/\s+/g, ' ').trim(); + return r ? base + ' Server: ' + r : base; +} diff --git a/src/net/ch-client.js b/src/net/ch-client.js index 1e80d09..c4a3e7c 100644 --- a/src/net/ch-client.js +++ b/src/net/ch-client.js @@ -7,7 +7,7 @@ // onSignedOut() } // so the whole module is unit-testable with plain stubs. -import { parseExceptionText, isAuthExpiredBody } from '../core/stream.js'; +import { parseExceptionText, isAuthExpiredBody, authDeniedMessage } from '../core/stream.js'; /** Build a ClickHouse HTTP URL with query-string options. Pure. */ export function chUrl(origin, opts = {}) { @@ -56,7 +56,12 @@ export async function authedFetch(ctx, url, sql, signal) { attempt++; continue; } - ctx.onSignedOut(); + // getToken() already guaranteed a non-expired token above, so a 401/403 + // that survives the one refresh-retry means CH rejected a *valid* login — + // an authorization/identity problem, not session expiry. Surface CH's own + // reason so it's diagnosable. + const reason = parseExceptionText(await resp.clone().text()); + ctx.onSignedOut(authDeniedMessage(resp.status, reason)); throw new Error('signed out'); } return resp; diff --git a/src/ui/app.js b/src/ui/app.js index 40b3269..045858c 100644 --- a/src/ui/app.js +++ b/src/ui/app.js @@ -159,7 +159,12 @@ export function createApp(env = {}) { getToken, refresh, authHeader, - onSignedOut: () => { clearTokens(); renderLogin(app, 'Session expired'); }, + // detail is set when CH rejects a *valid* login (authorization denial); the + // no-arg calls (no token / expired + refresh failed) fall back to expiry. + onSignedOut: (detail) => { + clearTokens(); + renderLogin(app, detail || 'Your session expired — please sign in again.'); + }, }; app.chCtx = chCtx; diff --git a/tests/unit/app.test.js b/tests/unit/app.test.js index 1c89321..880fe98 100644 --- a/tests/unit/app.test.js +++ b/tests/unit/app.test.js @@ -385,6 +385,16 @@ describe('auth flows', () => { const app = createApp(e); expect(await app.chCtx.getToken()).toBeNull(); }); + it('onSignedOut shows the given message, else a session-expired default', async () => { + const app = createApp(env()); + app.renderApp(); + // authorization denial: CH-supplied message is shown verbatim on the login screen + app.chCtx.onSignedOut('ClickHouse denied your account (HTTP 403). Server: nope'); + expect(app.root.querySelector('.login-error').textContent).toContain('denied your account'); + // genuine expiry: no detail → the reworded default + app.chCtx.onSignedOut(); + expect(app.root.querySelector('.login-error').textContent).toContain('session expired'); + }); }); describe('share + star + columns', () => { diff --git a/tests/unit/ch-client.test.js b/tests/unit/ch-client.test.js index 253786b..4d1b37d 100644 --- a/tests/unit/ch-client.test.js +++ b/tests/unit/ch-client.test.js @@ -78,10 +78,17 @@ describe('authedFetch', () => { expect(r.ok).toBe(true); expect(ctx.refresh).toHaveBeenCalledTimes(1); }); - it('signs out when refresh fails on 403', async () => { - const ctx = ctxWith(async () => jsonResp({}, false, 403), { refresh: async () => false }); + it('signs out with an authorization message + server reason when CH rejects a valid token (403)', async () => { + const ctx = ctxWith( + async () => textResp('Code: 516. DB::Exception: Authentication failed', false, 403), + { refresh: async () => false }, + ); await expect(authedFetch(ctx, 'u', 'sql')).rejects.toThrow('signed out'); - expect(ctx.onSignedOut).toHaveBeenCalled(); + expect(ctx.onSignedOut).toHaveBeenCalledTimes(1); + const msg = ctx.onSignedOut.mock.calls[0][0]; + expect(msg).toContain('HTTP 403'); + expect(msg).toContain('not authorizing you'); + expect(msg).toContain('Server: Code: 516. DB::Exception: Authentication failed'); }); it('treats a token_verification body as auth-expired', async () => { let n = 0; diff --git a/tests/unit/stream.test.js b/tests/unit/stream.test.js index c77ac8a..ce3f349 100644 --- a/tests/unit/stream.test.js +++ b/tests/unit/stream.test.js @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest'; import { newResult, applyStreamLine, splitBuffer, parseExceptionText, isAuthExpiredBody, + authDeniedMessage, } from '../../src/core/stream.js'; describe('newResult', () => { @@ -87,3 +88,20 @@ describe('isAuthExpiredBody', () => { expect(isAuthExpiredBody('syntax error')).toBe(false); }); }); + +describe('authDeniedMessage', () => { + it('interpolates the status and appends a collapsed server reason', () => { + const m = authDeniedMessage(403, ' Code: 516.\n DB::Exception: Authentication failed '); + expect(m).toContain('HTTP 403'); + expect(m).toContain('not authorizing you'); + expect(m).toContain('Server: Code: 516. DB::Exception: Authentication failed'); + expect(m).not.toContain('\n'); + }); + it('omits the Server tail when there is no reason', () => { + const m = authDeniedMessage(401, ''); + expect(m).toContain('HTTP 401'); + expect(m).not.toContain('Server:'); + expect(authDeniedMessage(401, ' ')).toBe(m); // whitespace-only is treated as empty + expect(authDeniedMessage(401)).toBe(m); // undefined reason + }); +});