From b20ed5844a13e988f83a1f605ffc5086801e2362 Mon Sep 17 00:00:00 2001 From: Pierre Gayvallet Date: Tue, 21 Jul 2020 11:34:04 +0200 Subject: [PATCH] preserve 401 errors from new es client (#71248) * intercept 401 error from new client in routing layer * improvements * lint * fix mocked client construction due to 7.9-rc1 bump * use default WWW-Authenticate value when not provided by ES 401 --- .../elasticsearch/client/errors.test.ts | 82 ++++++++++++ .../server/elasticsearch/client/errors.ts | 32 +++++ .../server/elasticsearch/client/mocks.test.ts | 6 + src/core/server/elasticsearch/client/mocks.ts | 15 ++- .../core_service.test.mocks.ts | 17 ++- .../integration_tests/core_services.test.ts | 117 +++++++++++++++++- src/core/server/http/router/router.ts | 34 ++++- 7 files changed, 288 insertions(+), 15 deletions(-) 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 00000000000000..35ad4ca71f48c6 --- /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 false when the input is a `ResponseError` and statusCode !== 401', () => { + expect( + isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 200 }))) + ).toBe(false); + expect( + isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 403 }))) + ).toBe(false); + expect( + isUnauthorizedError(new ResponseError(createApiResponseError({ statusCode: 500 }))) + ).toBe(false); + }); + + 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 00000000000000..31a27170e11559 --- /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 UnauthorizedError = ResponseError & { + statusCode: 401; +}; + +export function isResponseError(error: any): error is ResponseError { + return Boolean(error.body && error.statusCode && error.headers); +} + +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 b882f8d0c5d79c..a6ce95155331e2 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 34e83922d4d86c..ec2885dfdf922c 100644 --- a/src/core/server/elasticsearch/client/mocks.ts +++ b/src/core/server/elasticsearch/client/mocks.ts @@ -54,13 +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()); + 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; 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 c23724b7d332fe..515dad5383c010 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,10 +18,12 @@ */ import { elasticsearchServiceMock } from '../../elasticsearch/elasticsearch_service.mock'; -export const clusterClientMock = jest.fn(); -export const clusterClientInstanceMock = elasticsearchServiceMock.createLegacyScopedClusterClient(); +export const MockLegacyScopedClusterClient = jest.fn(); +export const legacyClusterClientInstanceMock = elasticsearchServiceMock.createLegacyScopedClusterClient(); jest.doMock('../../elasticsearch/legacy/scoped_cluster_client', () => ({ - LegacyScopedClusterClient: clusterClientMock.mockImplementation(() => clusterClientInstanceMock), + LegacyScopedClusterClient: MockLegacyScopedClusterClient.mockImplementation( + () => legacyClusterClientInstanceMock + ), })); jest.doMock('elasticsearch', () => { @@ -34,3 +36,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 3c5f22500e5e04..6338326626d546 100644 --- a/src/core/server/http/integration_tests/core_services.test.ts +++ b/src/core/server/http/integration_tests/core_services.test.ts @@ -17,14 +17,21 @@ * under the License. */ -import { clusterClientMock, clusterClientInstanceMock } from './core_service.test.mocks'; +import { + MockLegacyScopedClusterClient, + MockElasticsearchClient, + legacyClusterClientInstanceMock, +} from './core_service.test.mocks'; import Boom from 'boom'; import { Request } from 'hapi'; import { errors as esErrors } from 'elasticsearch'; import { LegacyElasticsearchErrorHelpers } from '../../elasticsearch/legacy'; +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; @@ -44,6 +51,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 () => { @@ -200,7 +218,7 @@ describe('http service', () => { }, 30000); afterEach(async () => { - clusterClientMock.mockClear(); + MockLegacyScopedClusterClient.mockClear(); await root.shutdown(); }); @@ -363,7 +381,7 @@ describe('http service', () => { }, 30000); afterEach(async () => { - clusterClientMock.mockClear(); + MockLegacyScopedClusterClient.mockClear(); await root.shutdown(); }); @@ -386,7 +404,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); }); @@ -410,7 +428,7 @@ 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 }); }); @@ -426,7 +444,7 @@ describe('http service', () => { }) ); - clusterClientInstanceMock.callAsCurrentUser.mockRejectedValue(authenticationError); + legacyClusterClientInstanceMock.callAsCurrentUser.mockRejectedValue(authenticationError); const router = createRouter('/new-platform'); router.get({ path: '/', validate: false }, async (context, req, res) => { @@ -441,4 +459,91 @@ describe('http service', () => { expect(response.header['www-authenticate']).toEqual('authenticate header'); }); }); + + 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'); + }); + + 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 35eec746163ceb..cc5279a3961636 100644 --- a/src/core/server/http/router/router.ts +++ b/src/core/server/http/router/router.ts @@ -23,8 +23,17 @@ import Boom from 'boom'; import { isConfigSchema } from '@kbn/config-schema'; import { Logger } from '../../logging'; import { LegacyElasticsearchErrorHelpers } from '../../elasticsearch/legacy/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,7 +273,13 @@ export class Router implements IRouter { return hapiResponseAdapter.handle(kibanaResponse); } catch (e) { this.log.error(e); - // forward 401 (boom) error from ES + // forward 401 errors from ES client + if (isElasticsearchUnauthorizedError(e)) { + return hapiResponseAdapter.handle( + kibanaResponseFactory.unauthorized(convertEsUnauthorized(e)) + ); + } + // forward 401 (boom) errors from legacy ES client if (LegacyElasticsearchErrorHelpers.isNotAuthorizedError(e)) { return e; } @@ -273,6 +288,21 @@ 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: { + 'www-authenticate': getAuthenticateHeaderValue(), + }, + }; +}; + type WithoutHeadArgument = T extends (first: any, ...rest: infer Params) => infer Return ? (...rest: Params) => Return : never;