From 64f266d8590b63a93cd7531bfe3c716739ff0b9c Mon Sep 17 00:00:00 2001 From: Serge Klochkov <3175289+slvrtrn@users.noreply.github.com> Date: Tue, 17 Dec 2024 11:33:06 +0100 Subject: [PATCH] JWT auth fixes (#367) --- CHANGELOG.md | 21 ++ .../__tests__/integration/auth.test.ts | 6 +- .../__tests__/unit/config.test.ts | 206 ++++++++++-------- .../client-common/__tests__/utils/client.ts | 2 +- packages/client-common/src/config.ts | 64 ++---- packages/client-common/src/version.ts | 2 +- .../integration/node_jwt_auth.test.ts | 21 +- packages/client-node/src/version.ts | 2 +- .../__tests__/jwt/web_jwt_auth.test.ts | 20 +- packages/client-web/src/version.ts | 2 +- 10 files changed, 176 insertions(+), 170 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 703c281..040cb5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,26 @@ +# 1.10.0 (Common, Node.js, Web) + +## New features + +- Added support for JWT authentication (ClickHouse Cloud feature) in both Node.js and Web API packages. JWT token can be set via `access_token` client configuration option. + + ```ts + const client = createClient({ + // ... + access_token: '', + }) + ``` + + Access token can also be configured via the URL params, e.g., `https://host:port?access_token=...`. + + It is also possible to override the access token for a particular request (see `BaseQueryParams.auth` for more details). + + NB: do not mix access token and username/password credentials in the configuration; the client will throw an error if both are set. + # 1.9.1 (Node.js only) +## Bug fixes + - Fixed an uncaught exception that could happen in case of malformed ClickHouse response when response compression is enabled ([#363](https://github.com/ClickHouse/clickhouse-js/issues/363)) # 1.9.0 (Common, Node.js, Web) diff --git a/packages/client-common/__tests__/integration/auth.test.ts b/packages/client-common/__tests__/integration/auth.test.ts index 5103260..e2c3221 100644 --- a/packages/client-common/__tests__/integration/auth.test.ts +++ b/packages/client-common/__tests__/integration/auth.test.ts @@ -7,10 +7,8 @@ describe('authentication', () => { let invalidAuthClient: ClickHouseClient beforeEach(() => { invalidAuthClient = createTestClient({ - auth: { - username: 'gibberish', - password: 'gibberish', - }, + username: 'gibberish', + password: 'gibberish', }) }) afterEach(async () => { diff --git a/packages/client-common/__tests__/unit/config.test.ts b/packages/client-common/__tests__/unit/config.test.ts index 7153ea2..375790e 100644 --- a/packages/client-common/__tests__/unit/config.test.ts +++ b/packages/client-common/__tests__/unit/config.test.ts @@ -96,7 +96,8 @@ describe('config', () => { pathname: '/my_proxy', request_timeout: 42_000, max_open_connections: 144, - auth: { username: 'bob', password: 'secret' }, + username: 'bob', + password: 'secret', database: 'analytics', http_headers: { 'X-CLICKHOUSE-AUTH': 'secret_header', @@ -123,10 +124,8 @@ describe('config', () => { pathname: '/my_proxy', request_timeout: 42_000, max_open_connections: 144, - auth: { - username: 'bob', - password: 'secret', - }, + username: 'bob', + password: 'secret', database: 'analytics', http_headers: { 'X-CLICKHOUSE-AUTH': 'secret_header', @@ -179,60 +178,6 @@ describe('config', () => { }) // should not be modified }) - it('should be able to use the deprecated username parameter', async () => { - const deprecated: BaseClickHouseClientConfigOptions = { - username: 'bob', - } - const res = prepareConfigWithURL(deprecated, logger, null) - expect(res).toEqual({ - ...defaultConfig, - auth: { - username: 'bob', - // to be "normalized" later by `getConnectionParams` - password: undefined, - }, - }) - expect(deprecated).toEqual({ - username: 'bob', - }) // should not be modified - }) - - it('should be able to use the deprecated password parameter', async () => { - const deprecated: BaseClickHouseClientConfigOptions = { - password: 'secret', - } - const res = prepareConfigWithURL(deprecated, logger, null) - expect(res).toEqual({ - ...defaultConfig, - auth: { - username: undefined, - password: 'secret', - }, - }) - expect(deprecated).toEqual({ - password: 'secret', - }) // should not be modified - }) - - it('should be able to use both deprecated username and password parameters', async () => { - const deprecated: BaseClickHouseClientConfigOptions = { - username: 'bob', - password: 'secret', - } - const res = prepareConfigWithURL(deprecated, logger, null) - expect(res).toEqual({ - ...defaultConfig, - auth: { - username: 'bob', - password: 'secret', - }, - }) - expect(deprecated).toEqual({ - username: 'bob', - password: 'secret', - }) // should not be modified - }) - // tested more thoroughly in the loadConfigOptionsFromURL section; // this is just a validation that everything works together it('should use settings from the URL', async () => { @@ -257,10 +202,8 @@ describe('config', () => { expect(res).toEqual({ ...defaultConfig, url: new URL('https://my.host:8443/'), - auth: { - username: 'bob', - password: 'secret', - }, + username: 'bob', + password: 'secret', database: 'analytics', application: 'my_app', impl_specific_setting: 42, @@ -367,12 +310,11 @@ describe('config', () => { url: new URL('https://my.host:8443'), database: 'analytics', application: 'my_app', - auth: { - access_token: 'jwt_secret', - }, + access_token: 'jwt_secret', } as unknown as BaseClickHouseClientConfigOptionsWithURL) }) + // this will throw later during the config validation anyway it('should correctly override credentials auth with JWT if both are present', async () => { const url = new URL( 'https://bob:secret@my.host:8443/analytics?' + @@ -384,9 +326,9 @@ describe('config', () => { url: new URL('https://my.host:8443'), database: 'analytics', application: 'my_app', - auth: { - access_token: 'jwt_secret', - }, + username: 'bob', + password: 'secret', + access_token: 'jwt_secret', } as unknown as BaseClickHouseClientConfigOptionsWithURL) }) }) @@ -402,6 +344,12 @@ describe('config', () => { }) describe('getConnectionParams', () => { + const authErrorMatcher = jasmine.objectContaining({ + message: jasmine.stringContaining( + 'Please use only one authentication method', + ), + }) + it('should return the default connection params', async () => { const res = getConnectionParams( { @@ -441,10 +389,8 @@ describe('config', () => { request: true, response: false, }, - auth: { - username: 'bob', - password: 'secret', - }, + username: 'bob', + password: 'secret', database: 'analytics', clickhouse_settings: { async_insert: 1, @@ -482,6 +428,76 @@ describe('config', () => { application_id: 'my_app', }) }) + + it('should throw if both JWT and username are set', async () => { + expect(() => + getConnectionParams( + { + url: new URL('https://my.host:8443/'), + username: 'bob', + access_token: 'jwt', + }, + logger, + ), + ).toThrow(authErrorMatcher) + }) + + it('should throw if both JWT and password are set', async () => { + expect(() => + getConnectionParams( + { + url: new URL('https://my.host:8443/'), + password: 'secret', + access_token: 'jwt', + }, + logger, + ), + ).toThrow(authErrorMatcher) + }) + + it('should throw if JWT, username, and password are all set', async () => { + expect(() => + getConnectionParams( + { + url: new URL('https://my.host:8443/'), + username: 'bob', + password: 'secret', + access_token: 'jwt', + }, + logger, + ), + ).toThrow(authErrorMatcher) + }) + + it('should not throw if only JWT auth is set', async () => { + expect( + getConnectionParams( + { + url: new URL('https://my.host:8443/'), + access_token: 'secret-token', + }, + logger, + ), + ).toEqual({ + url: new URL('https://my.host:8443/'), + request_timeout: 30_000, + max_open_connections: 10, + compression: { + decompress_response: false, + compress_request: false, + }, + auth: { + access_token: 'secret-token', + type: 'JWT', + }, + database: 'default', + clickhouse_settings: {}, + log_writer: jasmine.any(LogWriter), + keep_alive: { enabled: true }, + application_id: undefined, + http_headers: {}, + }) + }) }) describe('mergeConfigs', () => { @@ -492,10 +508,8 @@ describe('config', () => { it('should leave the base config as-is when there is nothing from the URL', async () => { const base: BaseClickHouseClientConfigOptions = { url: 'http://localhost:8123', - auth: { - username: 'bob', - password: 'secret', - }, + username: 'bob', + password: 'secret', } expect(mergeConfigs(base, {}, logger)).toEqual(base) }) @@ -503,21 +517,17 @@ describe('config', () => { it('should take URL values first, then base config for the rest', async () => { const base: BaseClickHouseClientConfigOptions = { url: 'https://my.host:8124', - auth: { - username: 'bob', - password: 'secret', - }, + username: 'bob', + password: 'secret', } const fromURL: BaseClickHouseClientConfigOptions = { - auth: { password: 'secret_from_url!' }, + password: 'secret_from_url!', } const res = mergeConfigs(base, fromURL, logger) expect(res).toEqual({ url: 'https://my.host:8124', - auth: { - username: 'bob', - password: 'secret_from_url!', - }, + username: 'bob', + password: 'secret_from_url!', }) }) @@ -526,12 +536,14 @@ describe('config', () => { url: 'https://my.host:8124', } const fromURL: BaseClickHouseClientConfigOptions = { - auth: { username: 'bob', password: 'secret' }, + username: 'bob', + password: 'secret', } const res = mergeConfigs(base, fromURL, logger) expect(res).toEqual({ url: 'https://my.host:8124', - auth: { username: 'bob', password: 'secret' }, + username: 'bob', + password: 'secret', }) }) @@ -539,12 +551,14 @@ describe('config', () => { it('should only take the URL values when there is nothing in the base config', async () => { const fromURL: BaseClickHouseClientConfigOptions = { url: 'https://my.host:8443', - auth: { username: 'bob', password: 'secret' }, + username: 'bob', + password: 'secret', } const res = mergeConfigs({}, fromURL, logger) expect(res).toEqual({ url: 'https://my.host:8443', - auth: { username: 'bob', password: 'secret' }, + username: 'bob', + password: 'secret', }) }) @@ -748,7 +762,8 @@ describe('config', () => { const res = loadConfigOptionsFromURL(url, null) expect(res[0].toString()).toEqual('https://my.host:8124/') // pathname will be attached later. expect(res[1]).toEqual({ - auth: { username: 'bob', password: 'secret' }, + username: 'bob', + password: 'secret', database: 'analytics', application: 'my_app', pathname: '/my_proxy', @@ -778,7 +793,8 @@ describe('config', () => { const res = loadConfigOptionsFromURL(url, null) expect(res[0].toString()).toEqual('http://localhost:8124/') expect(res[1]).toEqual({ - auth: { username: 'bob', password: 'secret' }, + username: 'bob', + password: 'secret', database: 'analytics', }) }) @@ -920,7 +936,8 @@ describe('config', () => { const res = loadConfigOptionsFromURL(url, handler) expect(res[0].toString()).toEqual('https://my.host:8124/') expect(res[1]).toEqual({ - auth: { username: 'bob', password: 'secret' }, + username: 'bob', + password: 'secret', database: 'analytics', application: 'my_app', session_id: 'sticky', @@ -970,7 +987,8 @@ describe('config', () => { const res = loadConfigOptionsFromURL(url, handler) expect(res[0].toString()).toEqual('https://my.host:8124/') expect(res[1]).toEqual({ - auth: { username: 'bob', password: 'secret' }, + username: 'bob', + password: 'secret', database: 'analytics', application: 'my_app', session_id: 'sticky', diff --git a/packages/client-common/__tests__/utils/client.ts b/packages/client-common/__tests__/utils/client.ts index 1544138..59f8795 100644 --- a/packages/client-common/__tests__/utils/client.ts +++ b/packages/client-common/__tests__/utils/client.ts @@ -56,7 +56,7 @@ export function createTestClient( if (isCloudTestEnv()) { const cloudConfig: BaseClickHouseClientConfigOptions = { url: `https://${getFromEnv(EnvKeys.host)}:8443`, - auth: { password: getFromEnv(EnvKeys.password) }, + password: getFromEnv(EnvKeys.password), database: databaseName, request_timeout: 60_000, ...logging, diff --git a/packages/client-common/src/config.ts b/packages/client-common/src/config.ts index d4b6c24..b7a7179 100644 --- a/packages/client-common/src/config.ts +++ b/packages/client-common/src/config.ts @@ -1,9 +1,4 @@ -import type { - ClickHouseAuth, - ClickHouseCredentialsAuth, - InsertValues, - ResponseHeaders, -} from './clickhouse_types' +import type { InsertValues, ResponseHeaders } from './clickhouse_types' import type { Connection, ConnectionParams } from './connection' import type { DataFormat } from './data_formatter' import type { Logger } from './logger' @@ -43,18 +38,19 @@ export interface BaseClickHouseClientConfigOptions { * @default false */ request?: boolean } - /** @deprecated Use {@link auth} object instead. - * The name of the user on whose behalf requests are made. + /** The name of the user on whose behalf requests are made. + * Should not be set if {@link access_token} is provided. * @default default */ username?: string - /** @deprecated Use {@link auth} object instead. - * The user password. + /** The user password. + * Should not be set if {@link access_token} is provided. * @default empty string */ password?: string - /** Username + password or a JWT token to authenticate with ClickHouse. + /** A JWT access token to authenticate with ClickHouse. * JWT token authentication is supported in ClickHouse Cloud only. - * @default username: `default`, password: empty string */ - auth?: ClickHouseAuth + * Should not be set if {@link username} or {@link password} are provided. + * @default empty */ + access_token?: string /** The name of the application using the JS client. * @default empty string */ application?: string @@ -188,19 +184,6 @@ export function prepareConfigWithURL( baseConfig.http_headers = baseConfig.additional_headers delete baseConfig.additional_headers } - if (baseConfig.username !== undefined || baseConfig.password !== undefined) { - logger.warn({ - module: 'Config', - message: - '"username" and "password" are deprecated. Use "auth" object instead.', - }) - baseConfig.auth = { - username: baseConfig.username, - password: baseConfig.password, - } - delete baseConfig.username - delete baseConfig.password - } let configURL if (baseConfig.host !== undefined) { logger.warn({ @@ -229,20 +212,17 @@ export function getConnectionParams( logger: Logger, ): ConnectionParams { let auth: ConnectionParams['auth'] - if (config.auth !== undefined) { - if ('access_token' in config.auth) { - auth = { access_token: config.auth.access_token, type: 'JWT' } - } else { - auth = { - username: config.auth.username ?? 'default', - password: config.auth.password ?? '', - type: 'Credentials', - } + if (config.access_token !== undefined) { + if (config.username !== undefined || config.password !== undefined) { + throw new Error( + 'Both access token and username/password are provided in the configuration. Please use only one authentication method.', + ) } + auth = { access_token: config.access_token, type: 'JWT' } } else { auth = { - username: 'default', - password: '', + username: config.username ?? 'default', + password: config.password ?? '', type: 'Credentials', } } @@ -340,16 +320,12 @@ export function loadConfigOptionsFromURL( handleExtraURLParams: HandleImplSpecificURLParams | null, ): [URL, BaseClickHouseClientConfigOptions] { let config: BaseClickHouseClientConfigOptions = {} - const auth: ClickHouseCredentialsAuth = {} if (url.username.trim() !== '') { - auth.username = url.username + config.username = url.username } // no trim for password if (url.password !== '') { - auth.password = url.password - } - if (Object.keys(auth).length > 0) { - config.auth = auth + config.password = url.password } if (url.pathname.trim().length > 1) { config.database = url.pathname.slice(1) @@ -442,7 +418,7 @@ export function loadConfigOptionsFromURL( config.keep_alive.enabled = booleanConfigURLValue({ key, value }) break case 'access_token': - config.auth = { access_token: value } + config.access_token = value break default: paramWasProcessed = false diff --git a/packages/client-common/src/version.ts b/packages/client-common/src/version.ts index 1ef321b..f9a4c7b 100644 --- a/packages/client-common/src/version.ts +++ b/packages/client-common/src/version.ts @@ -1 +1 @@ -export default '1.9.1' +export default '1.10.0' diff --git a/packages/client-node/__tests__/integration/node_jwt_auth.test.ts b/packages/client-node/__tests__/integration/node_jwt_auth.test.ts index e0bd6ef..728ed4a 100644 --- a/packages/client-node/__tests__/integration/node_jwt_auth.test.ts +++ b/packages/client-node/__tests__/integration/node_jwt_auth.test.ts @@ -1,10 +1,11 @@ -import type { ClickHouseClient } from '@clickhouse/client-common' -import { createTestClient, TestEnv, whenOnEnv } from '@test/utils' +import { TestEnv, whenOnEnv } from '@test/utils' import { EnvKeys, getFromEnv } from '@test/utils/env' +import { createClient } from '../../src' +import type { NodeClickHouseClient } from '../../src/client' import { makeJWT } from '../utils/jwt' whenOnEnv(TestEnv.CloudSMT).describe('[Node.js] JWT auth', () => { - let jwtClient: ClickHouseClient + let jwtClient: NodeClickHouseClient let url: string let jwt: string @@ -17,11 +18,9 @@ whenOnEnv(TestEnv.CloudSMT).describe('[Node.js] JWT auth', () => { }) it('should work with client configuration', async () => { - jwtClient = createTestClient({ + jwtClient = createClient({ url, - auth: { - access_token: jwt, - }, + access_token: jwt, }) const rs = await jwtClient.query({ query: 'SELECT 42 AS result', @@ -31,12 +30,10 @@ whenOnEnv(TestEnv.CloudSMT).describe('[Node.js] JWT auth', () => { }) it('should override the client instance auth', async () => { - jwtClient = createTestClient({ + jwtClient = createClient({ url, - auth: { - username: 'gibberish', - password: 'gibberish', - }, + username: 'gibberish', + password: 'gibberish', }) const rs = await jwtClient.query({ query: 'SELECT 42 AS result', diff --git a/packages/client-node/src/version.ts b/packages/client-node/src/version.ts index 1ef321b..f9a4c7b 100644 --- a/packages/client-node/src/version.ts +++ b/packages/client-node/src/version.ts @@ -1 +1 @@ -export default '1.9.1' +export default '1.10.0' diff --git a/packages/client-web/__tests__/jwt/web_jwt_auth.test.ts b/packages/client-web/__tests__/jwt/web_jwt_auth.test.ts index d2fb40a..4da2d98 100644 --- a/packages/client-web/__tests__/jwt/web_jwt_auth.test.ts +++ b/packages/client-web/__tests__/jwt/web_jwt_auth.test.ts @@ -1,12 +1,12 @@ -import type { ClickHouseClient } from '@clickhouse/client-common' -import { createTestClient } from '@test/utils' import { EnvKeys, getFromEnv } from '@test/utils/env' +import { createClient } from '../../src' +import type { WebClickHouseClient } from '../../src/client' /** Cannot use the jsonwebtoken library to generate the token: it is Node.js only. * The access token should be generated externally before running the test, * and set as the CLICKHOUSE_JWT_ACCESS_TOKEN environment variable */ describe('[Web] JWT auth', () => { - let client: ClickHouseClient + let client: WebClickHouseClient let url: string let jwt: string @@ -19,11 +19,9 @@ describe('[Web] JWT auth', () => { }) it('should work with client configuration', async () => { - client = createTestClient({ + client = createClient({ url, - auth: { - access_token: jwt, - }, + access_token: jwt, }) const rs = await client.query({ query: 'SELECT 42 AS result', @@ -33,12 +31,10 @@ describe('[Web] JWT auth', () => { }) it('should override the client instance auth', async () => { - client = createTestClient({ + client = createClient({ url, - auth: { - username: 'gibberish', - password: 'gibberish', - }, + username: 'gibberish', + password: 'gibberish', }) const rs = await client.query({ query: 'SELECT 42 AS result', diff --git a/packages/client-web/src/version.ts b/packages/client-web/src/version.ts index 1ef321b..f9a4c7b 100644 --- a/packages/client-web/src/version.ts +++ b/packages/client-web/src/version.ts @@ -1 +1 @@ -export default '1.9.1' +export default '1.10.0'