From edbd1e23f916a11ad70f48db2d8bc8ef825714bf Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 9 Jul 2020 16:06:16 +0200 Subject: [PATCH 1/5] intercept 401 error from new client in routing layer --- .../elasticsearch/client/errors.test.ts | 82 +++++++++++++++++++ .../server/elasticsearch/client/errors.ts | 32 ++++++++ src/core/server/http/router/router.ts | 12 +++ 3 files changed, 126 insertions(+) create mode 100644 src/core/server/elasticsearch/client/errors.test.ts create mode 100644 src/core/server/elasticsearch/client/errors.ts diff --git a/src/core/server/elasticsearch/client/errors.test.ts b/src/core/server/elasticsearch/client/errors.test.ts new file mode 100644 index 0000000000000..454b94b7c6b97 --- /dev/null +++ b/src/core/server/elasticsearch/client/errors.test.ts @@ -0,0 +1,82 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + ResponseError, + ConnectionError, + ConfigurationError, +} from '@elastic/elasticsearch/lib/errors'; +import { ApiResponse } from '@elastic/elasticsearch'; +import { isResponseError, isUnauthorizedError } from './errors'; + +const createApiResponseError = ({ + statusCode = 200, + headers = {}, + body = {}, +}: { + statusCode?: number; + headers?: Record; + body?: Record; +} = {}): ApiResponse => { + return { + body, + statusCode, + headers, + warnings: [], + meta: {} as any, + }; +}; + +describe('isResponseError', () => { + it('returns `true` when the input is a `ResponseError`', () => { + expect(isResponseError(new ResponseError(createApiResponseError()))).toBe(true); + }); + + it('returns `false` when the input is not a `ResponseError`', () => { + expect(isResponseError(new Error('foo'))).toBe(false); + expect(isResponseError(new ConnectionError('error', createApiResponseError()))).toBe(false); + expect(isResponseError(new ConfigurationError('foo'))).toBe(false); + }); +}); + +describe('isUnauthorizedError', () => { + it('returns true when the input is a `ResponseError` and statusCode === 401', () => { + expect( + isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 401 }))) + ).toBe(true); + }); + + it('returns true when the input is a `ResponseError` and statusCode !== 401', () => { + expect( + isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 200 }))) + ).toBe(true); + expect( + isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 403 }))) + ).toBe(true); + expect( + isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 500 }))) + ).toBe(true); + }); + + it('returns `false` when the input is not a `ResponseError`', () => { + expect(isUnauthorizedError(new Error('foo'))).toBe(false); + expect(isUnauthorizedError(new ConnectionError('error', createApiResponseError()))).toBe(false); + expect(isUnauthorizedError(new ConfigurationError('foo'))).toBe(false); + }); +}); diff --git a/src/core/server/elasticsearch/client/errors.ts b/src/core/server/elasticsearch/client/errors.ts new file mode 100644 index 0000000000000..99b73d593df29 --- /dev/null +++ b/src/core/server/elasticsearch/client/errors.ts @@ -0,0 +1,32 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { ResponseError } from '@elastic/elasticsearch/lib/errors'; + +export type NotAuthorizedError = ResponseError & { + statusCode: 401; +}; + +export function isResponseError(error: any): error is ResponseError { + return !!(error.body && error.statusCode && error.headers); +} + +export function isUnauthorizedError(error: any): error is NotAuthorizedError { + return isResponseError(error) && error.statusCode === 401; +} diff --git a/src/core/server/http/router/router.ts b/src/core/server/http/router/router.ts index 69402a74eda5f..adc5de318d3ef 100644 --- a/src/core/server/http/router/router.ts +++ b/src/core/server/http/router/router.ts @@ -22,6 +22,7 @@ import Boom from 'boom'; import { isConfigSchema } from '@kbn/config-schema'; import { Logger } from '../../logging'; +import { isUnauthorizedError as isElasticsearchNotAuthorizedError } from '../../elasticsearch/client/errors'; import { KibanaRequest } from './request'; import { KibanaResponseFactory, kibanaResponseFactory, IKibanaResponse } from './response'; import { RouteConfig, RouteConfigOptions, RouteMethod, validBodyOutput } from './route'; @@ -263,6 +264,17 @@ export class Router implements IRouter { return hapiResponseAdapter.handle(kibanaResponse); } catch (e) { this.log.error(e); + if (isElasticsearchNotAuthorizedError(e)) { + return hapiResponseAdapter.handle( + kibanaResponseFactory.unauthorized({ + body: e.message, + headers: { + 'WWW-Authenticate': + e.headers['WWW-Authenticate'] ?? 'Basic realm="Authorization Required"', + }, + }) + ); + } return hapiResponseAdapter.toInternalError(); } } From 1ec9664d22645b832bc17102a8d7e7102dcabc71 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Fri, 10 Jul 2020 10:33:30 +0200 Subject: [PATCH 2/5] improvements --- .../elasticsearch/client/errors.test.ts | 8 +- .../server/elasticsearch/client/errors.ts | 4 +- .../server/elasticsearch/client/mocks.test.ts | 6 ++ src/core/server/elasticsearch/client/mocks.ts | 3 + .../core_service.test.mocks.ts | 13 ++- .../integration_tests/core_services.test.ts | 79 +++++++++++++++++-- src/core/server/http/router/router.ts | 37 ++++++--- 7 files changed, 126 insertions(+), 24 deletions(-) diff --git a/src/core/server/elasticsearch/client/errors.test.ts b/src/core/server/elasticsearch/client/errors.test.ts index 454b94b7c6b97..35ad4ca71f48c 100644 --- a/src/core/server/elasticsearch/client/errors.test.ts +++ b/src/core/server/elasticsearch/client/errors.test.ts @@ -62,16 +62,16 @@ describe('isUnauthorizedError', () => { ).toBe(true); }); - it('returns true when the input is a `ResponseError` and statusCode !== 401', () => { + it('returns false when the input is a `ResponseError` and statusCode !== 401', () => { expect( isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 200 }))) - ).toBe(true); + ).toBe(false); expect( isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 403 }))) - ).toBe(true); + ).toBe(false); expect( isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 500 }))) - ).toBe(true); + ).toBe(false); }); it('returns `false` when the input is not a `ResponseError`', () => { diff --git a/src/core/server/elasticsearch/client/errors.ts b/src/core/server/elasticsearch/client/errors.ts index 99b73d593df29..64c0f4a78655c 100644 --- a/src/core/server/elasticsearch/client/errors.ts +++ b/src/core/server/elasticsearch/client/errors.ts @@ -19,7 +19,7 @@ import { ResponseError } from '@elastic/elasticsearch/lib/errors'; -export type NotAuthorizedError = ResponseError & { +export type UnauthorizedError = ResponseError & { statusCode: 401; }; @@ -27,6 +27,6 @@ export function isResponseError(error: any): error is ResponseError { return !!(error.body && error.statusCode && error.headers); } -export function isUnauthorizedError(error: any): error is NotAuthorizedError { +export function isUnauthorizedError(error: any): error is UnauthorizedError { return isResponseError(error) && error.statusCode === 401; } diff --git a/src/core/server/elasticsearch/client/mocks.test.ts b/src/core/server/elasticsearch/client/mocks.test.ts index b882f8d0c5d79..a6ce95155331e 100644 --- a/src/core/server/elasticsearch/client/mocks.test.ts +++ b/src/core/server/elasticsearch/client/mocks.test.ts @@ -49,6 +49,12 @@ describe('Mocked client', () => { expectMocked(client.close); }); + it('used EventEmitter functions should be mocked', () => { + expectMocked(client.on); + expectMocked(client.off); + expectMocked(client.once); + }); + it('`child` should be mocked and return a mocked Client', () => { expectMocked(client.child); diff --git a/src/core/server/elasticsearch/client/mocks.ts b/src/core/server/elasticsearch/client/mocks.ts index 75644435a7f2a..67112b0db637e 100644 --- a/src/core/server/elasticsearch/client/mocks.ts +++ b/src/core/server/elasticsearch/client/mocks.ts @@ -59,6 +59,9 @@ const createInternalClientMock = (): DeeplyMockedKeys => { }; client.close = jest.fn().mockReturnValue(Promise.resolve()); client.child = jest.fn().mockImplementation(() => createInternalClientMock()); + client.on = jest.fn(); + client.off = jest.fn(); + client.once = jest.fn(); return (client as unknown) as DeeplyMockedKeys; }; diff --git a/src/core/server/http/integration_tests/core_service.test.mocks.ts b/src/core/server/http/integration_tests/core_service.test.mocks.ts index f7ebd18b9c488..fecae6378b1c6 100644 --- a/src/core/server/http/integration_tests/core_service.test.mocks.ts +++ b/src/core/server/http/integration_tests/core_service.test.mocks.ts @@ -18,9 +18,9 @@ */ import { elasticsearchServiceMock } from '../../elasticsearch/elasticsearch_service.mock'; -export const clusterClientMock = jest.fn(); +export const MockLegacyScopedClusterClient = jest.fn(); jest.doMock('../../elasticsearch/legacy/scoped_cluster_client', () => ({ - LegacyScopedClusterClient: clusterClientMock.mockImplementation(function () { + LegacyScopedClusterClient: MockLegacyScopedClusterClient.mockImplementation(function () { return elasticsearchServiceMock.createLegacyScopedClusterClient(); }), })); @@ -35,3 +35,12 @@ jest.doMock('elasticsearch', () => { }, }; }); + +export const MockElasticsearchClient = jest.fn(); +jest.doMock('@elastic/elasticsearch', () => { + const real = jest.requireActual('@elastic/elasticsearch'); + return { + ...real, + Client: MockElasticsearchClient, + }; +}); diff --git a/src/core/server/http/integration_tests/core_services.test.ts b/src/core/server/http/integration_tests/core_services.test.ts index ba39effa77016..bcf7e6fb1f0b2 100644 --- a/src/core/server/http/integration_tests/core_services.test.ts +++ b/src/core/server/http/integration_tests/core_services.test.ts @@ -16,11 +16,16 @@ * specific language governing permissions and limitations * under the License. */ + +import { MockLegacyScopedClusterClient, MockElasticsearchClient } from './core_service.test.mocks'; + import Boom from 'boom'; import { Request } from 'hapi'; -import { clusterClientMock } from './core_service.test.mocks'; +import { elasticsearchClientMock } from '../../elasticsearch/client/mocks'; +import { ResponseError } from '@elastic/elasticsearch/lib/errors'; import * as kbnTestServer from '../../../../test_utils/kbn_server'; +import { InternalElasticsearchServiceStart } from '../../elasticsearch'; interface User { id: string; @@ -40,6 +45,17 @@ const cookieOptions = { }; describe('http service', () => { + let esClient: ReturnType; + + beforeEach(async () => { + esClient = elasticsearchClientMock.createInternalClient(); + MockElasticsearchClient.mockImplementation(() => esClient); + }, 30000); + + afterEach(async () => { + MockElasticsearchClient.mockClear(); + }); + describe('auth', () => { let root: ReturnType; beforeEach(async () => { @@ -196,7 +212,7 @@ describe('http service', () => { }, 30000); afterEach(async () => { - clusterClientMock.mockClear(); + MockLegacyScopedClusterClient.mockClear(); await root.shutdown(); }); @@ -352,14 +368,14 @@ describe('http service', () => { }); }); }); - describe('elasticsearch', () => { + describe('legacy elasticsearch client', () => { let root: ReturnType; beforeEach(async () => { root = kbnTestServer.createRoot({ plugins: { initialize: false } }); }, 30000); afterEach(async () => { - clusterClientMock.mockClear(); + MockLegacyScopedClusterClient.mockClear(); await root.shutdown(); }); @@ -382,7 +398,7 @@ describe('http service', () => { await kbnTestServer.request.get(root, '/new-platform/').expect(200); // client contains authHeaders for BWC with legacy platform. - const [client] = clusterClientMock.mock.calls; + const [client] = MockLegacyScopedClusterClient.mock.calls; const [, , clientHeaders] = client; expect(clientHeaders).toEqual(authHeaders); }); @@ -406,9 +422,60 @@ describe('http service', () => { .set('Authorization', authorizationHeader) .expect(200); - const [client] = clusterClientMock.mock.calls; + const [client] = MockLegacyScopedClusterClient.mock.calls; const [, , clientHeaders] = client; expect(clientHeaders).toEqual({ authorization: authorizationHeader }); }); }); + + describe('elasticsearch client', () => { + let root: ReturnType; + + beforeEach(async () => { + root = kbnTestServer.createRoot({ plugins: { initialize: false } }); + }, 30000); + + afterEach(async () => { + MockElasticsearchClient.mockClear(); + await root.shutdown(); + }); + + it('forwards unauthorized errors from elasticsearch', async () => { + const { http } = await root.setup(); + const { createRouter } = http; + // eslint-disable-next-line prefer-const + let elasticsearch: InternalElasticsearchServiceStart; + + esClient.ping.mockImplementation(() => + elasticsearchClientMock.createClientError( + new ResponseError({ + statusCode: 401, + body: { + error: { + type: 'Unauthorized', + }, + }, + warnings: [], + headers: { + 'WWW-Authenticate': 'content', + }, + meta: {} as any, + }) + ) + ); + + const router = createRouter('/new-platform'); + router.get({ path: '/', validate: false }, async (context, req, res) => { + await elasticsearch.client.asScoped(req).asInternalUser.ping(); + return res.ok(); + }); + + const coreStart = await root.start(); + elasticsearch = coreStart.elasticsearch; + + const { header } = await kbnTestServer.request.get(root, '/new-platform/').expect(401); + + expect(header['www-authenticate']).toEqual('content'); + }); + }); }); diff --git a/src/core/server/http/router/router.ts b/src/core/server/http/router/router.ts index adc5de318d3ef..eef9ed4e15e7b 100644 --- a/src/core/server/http/router/router.ts +++ b/src/core/server/http/router/router.ts @@ -22,9 +22,17 @@ import Boom from 'boom'; import { isConfigSchema } from '@kbn/config-schema'; import { Logger } from '../../logging'; -import { isUnauthorizedError as isElasticsearchNotAuthorizedError } from '../../elasticsearch/client/errors'; +import { + isUnauthorizedError as isElasticsearchUnauthorizedError, + UnauthorizedError as EsNotAuthorizedError, +} from '../../elasticsearch/client/errors'; import { KibanaRequest } from './request'; -import { KibanaResponseFactory, kibanaResponseFactory, IKibanaResponse } from './response'; +import { + KibanaResponseFactory, + kibanaResponseFactory, + IKibanaResponse, + ErrorHttpResponseOptions, +} from './response'; import { RouteConfig, RouteConfigOptions, RouteMethod, validBodyOutput } from './route'; import { HapiResponseAdapter } from './response_adapter'; import { RequestHandlerContext } from '../../../server'; @@ -264,15 +272,9 @@ export class Router implements IRouter { return hapiResponseAdapter.handle(kibanaResponse); } catch (e) { this.log.error(e); - if (isElasticsearchNotAuthorizedError(e)) { + if (isElasticsearchUnauthorizedError(e)) { return hapiResponseAdapter.handle( - kibanaResponseFactory.unauthorized({ - body: e.message, - headers: { - 'WWW-Authenticate': - e.headers['WWW-Authenticate'] ?? 'Basic realm="Authorization Required"', - }, - }) + kibanaResponseFactory.unauthorized(convertEsUnauthorized(e)) ); } return hapiResponseAdapter.toInternalError(); @@ -280,6 +282,21 @@ export class Router implements IRouter { } } +const convertEsUnauthorized = (e: EsNotAuthorizedError): ErrorHttpResponseOptions => { + return { + body: e.message, + headers: Object.entries(e.headers).reduce((headers, [key, value]) => { + if (key.toLowerCase() === 'www-authenticate') { + return { + ...headers, + 'www-authenticate': value, + }; + } + return headers; + }, {} as Record), + }; +}; + type WithoutHeadArgument = T extends (first: any, ...rest: infer Params) => infer Return ? (...rest: Params) => Return : never; From 245bb4b914efa4ee781f890e164394ba905d49e3 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Tue, 14 Jul 2020 08:20:09 +0200 Subject: [PATCH 3/5] lint --- .../server/http/integration_tests/core_service.test.mocks.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/server/http/integration_tests/core_service.test.mocks.ts b/src/core/server/http/integration_tests/core_service.test.mocks.ts index cd1dd1963f958..515dad5383c01 100644 --- a/src/core/server/http/integration_tests/core_service.test.mocks.ts +++ b/src/core/server/http/integration_tests/core_service.test.mocks.ts @@ -21,7 +21,9 @@ import { elasticsearchServiceMock } from '../../elasticsearch/elasticsearch_serv export const MockLegacyScopedClusterClient = jest.fn(); export const legacyClusterClientInstanceMock = elasticsearchServiceMock.createLegacyScopedClusterClient(); jest.doMock('../../elasticsearch/legacy/scoped_cluster_client', () => ({ - LegacyScopedClusterClient: MockLegacyScopedClusterClient.mockImplementation(() => legacyClusterClientInstanceMock), + LegacyScopedClusterClient: MockLegacyScopedClusterClient.mockImplementation( + () => legacyClusterClientInstanceMock + ), })); jest.doMock('elasticsearch', () => { From c4be5d2d069d80de18efcb6038b179cea1586eaf Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Mon, 20 Jul 2020 11:14:08 +0200 Subject: [PATCH 4/5] fix mocked client construction due to 7.9-rc1 bump --- src/core/server/elasticsearch/client/mocks.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/core/server/elasticsearch/client/mocks.ts b/src/core/server/elasticsearch/client/mocks.ts index 445de62d4c5c0..ec2885dfdf922 100644 --- a/src/core/server/elasticsearch/client/mocks.ts +++ b/src/core/server/elasticsearch/client/mocks.ts @@ -54,16 +54,20 @@ const createInternalClientMock = (): DeeplyMockedKeys => { mockify(client, omittedProps); - client.transport = { + // client got some read-only (getter) properties + // so we need to extend it to override the getter-only props. + const mock: any = { ...client }; + + mock.transport = { request: jest.fn(), }; - client.close = jest.fn().mockReturnValue(Promise.resolve()); - client.child = jest.fn().mockImplementation(() => createInternalClientMock()); - client.on = jest.fn(); - client.off = jest.fn(); - client.once = jest.fn(); + mock.close = jest.fn().mockReturnValue(Promise.resolve()); + mock.child = jest.fn().mockImplementation(() => createInternalClientMock()); + mock.on = jest.fn(); + mock.off = jest.fn(); + mock.once = jest.fn(); - return (client as unknown) as DeeplyMockedKeys; + return (mock as unknown) as DeeplyMockedKeys; }; export type ElasticSearchClientMock = DeeplyMockedKeys; From 051bab476f243af24f99608853e8e609ce5cfdd2 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Tue, 21 Jul 2020 08:22:17 +0200 Subject: [PATCH 5/5] use default WWW-Authenticate value when not provided by ES 401 --- .../server/elasticsearch/client/errors.ts | 2 +- .../integration_tests/core_services.test.ts | 36 +++++++++++++++++++ src/core/server/http/router/router.ts | 18 +++++----- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/core/server/elasticsearch/client/errors.ts b/src/core/server/elasticsearch/client/errors.ts index 64c0f4a78655c..31a27170e1155 100644 --- a/src/core/server/elasticsearch/client/errors.ts +++ b/src/core/server/elasticsearch/client/errors.ts @@ -24,7 +24,7 @@ export type UnauthorizedError = ResponseError & { }; export function isResponseError(error: any): error is ResponseError { - return !!(error.body && error.statusCode && error.headers); + return Boolean(error.body && error.statusCode && error.headers); } export function isUnauthorizedError(error: any): error is UnauthorizedError { diff --git a/src/core/server/http/integration_tests/core_services.test.ts b/src/core/server/http/integration_tests/core_services.test.ts index 42c42f0a52222..6338326626d54 100644 --- a/src/core/server/http/integration_tests/core_services.test.ts +++ b/src/core/server/http/integration_tests/core_services.test.ts @@ -509,5 +509,41 @@ describe('http service', () => { expect(header['www-authenticate']).toEqual('content'); }); + + it('uses a default value for `www-authenticate` header when ES 401 does not specify it', async () => { + const { http } = await root.setup(); + const { createRouter } = http; + // eslint-disable-next-line prefer-const + let elasticsearch: InternalElasticsearchServiceStart; + + esClient.ping.mockImplementation(() => + elasticsearchClientMock.createClientError( + new ResponseError({ + statusCode: 401, + body: { + error: { + type: 'Unauthorized', + }, + }, + warnings: [], + headers: {}, + meta: {} as any, + }) + ) + ); + + const router = createRouter('/new-platform'); + router.get({ path: '/', validate: false }, async (context, req, res) => { + await elasticsearch.client.asScoped(req).asInternalUser.ping(); + return res.ok(); + }); + + const coreStart = await root.start(); + elasticsearch = coreStart.elasticsearch; + + const { header } = await kbnTestServer.request.get(root, '/new-platform/').expect(401); + + expect(header['www-authenticate']).toEqual('Basic realm="Authorization Required"'); + }); }); }); diff --git a/src/core/server/http/router/router.ts b/src/core/server/http/router/router.ts index 7b8c271d8afe0..cc5279a396163 100644 --- a/src/core/server/http/router/router.ts +++ b/src/core/server/http/router/router.ts @@ -289,17 +289,17 @@ export class Router implements IRouter { } const convertEsUnauthorized = (e: EsNotAuthorizedError): ErrorHttpResponseOptions => { + const getAuthenticateHeaderValue = () => { + const header = Object.entries(e.headers).find( + ([key]) => key.toLowerCase() === 'www-authenticate' + ); + return header ? header[1] : 'Basic realm="Authorization Required"'; + }; return { body: e.message, - headers: Object.entries(e.headers).reduce((headers, [key, value]) => { - if (key.toLowerCase() === 'www-authenticate') { - return { - ...headers, - 'www-authenticate': value, - }; - } - return headers; - }, {} as Record), + headers: { + 'www-authenticate': getAuthenticateHeaderValue(), + }, }; };