From e57009ce0374b0315d3c43cb4f2b33ba2a3af8bb Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Sep 2020 17:02:28 -0700 Subject: [PATCH] Update/refactor EnterpriseSearchRequestHandler to manage internal endpoint error behavior + pull out / refactor all errors into their own methods instead of relying on a single 'error connecting' message --- .../enterprise_search_request_handler.test.ts | 235 ++++++++++++------ .../lib/enterprise_search_request_handler.ts | 133 ++++++++-- 2 files changed, 280 insertions(+), 88 deletions(-) diff --git a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts index 34f83ef3a3fd2..68db0e4486230 100644 --- a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts +++ b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts @@ -30,109 +30,183 @@ describe('EnterpriseSearchRequestHandler', () => { fetchMock.mockReset(); }); - it('makes an API call and returns the response', async () => { - const responseBody = { - results: [{ name: 'engine1' }], - meta: { page: { total_results: 1 } }, - }; + describe('createRequest()', () => { + it('makes an API call and returns the response', async () => { + const responseBody = { + results: [{ name: 'engine1' }], + meta: { page: { total_results: 1 } }, + }; - EnterpriseSearchAPI.mockReturn(responseBody); + EnterpriseSearchAPI.mockReturn(responseBody); - const requestHandler = enterpriseSearchRequestHandler.createRequest({ - path: '/as/credentials/collection', - }); + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/as/credentials/collection', + }); - await makeAPICall(requestHandler, { - query: { - type: 'indexed', - pageIndex: 1, - }, - }); + await makeAPICall(requestHandler, { + query: { + type: 'indexed', + pageIndex: 1, + }, + }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith( - 'http://localhost:3002/as/credentials/collection?type=indexed&pageIndex=1', - { method: 'GET' } - ); + EnterpriseSearchAPI.shouldHaveBeenCalledWith( + 'http://localhost:3002/as/credentials/collection?type=indexed&pageIndex=1', + { method: 'GET' } + ); - expect(responseMock.custom).toHaveBeenCalledWith({ - body: responseBody, - statusCode: 200, + expect(responseMock.custom).toHaveBeenCalledWith({ + body: responseBody, + statusCode: 200, + }); }); - }); - describe('request passing', () => { - it('passes route method', async () => { - const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/example' }); + describe('request passing', () => { + it('passes route method', async () => { + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/api/example', + }); + + await makeAPICall(requestHandler, { route: { method: 'POST' } }); + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { + method: 'POST', + }); - await makeAPICall(requestHandler, { route: { method: 'POST' } }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { - method: 'POST', + await makeAPICall(requestHandler, { route: { method: 'DELETE' } }); + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { + method: 'DELETE', + }); }); - await makeAPICall(requestHandler, { route: { method: 'DELETE' } }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { - method: 'DELETE', + it('passes request body', async () => { + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/api/example', + }); + await makeAPICall(requestHandler, { body: { bodacious: true } }); + + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { + body: '{"bodacious":true}', + }); }); - }); - it('passes request body', async () => { - const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/example' }); - await makeAPICall(requestHandler, { body: { bodacious: true } }); + it('passes custom params set by the handler, which override request params', async () => { + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/api/example', + params: { someQuery: true }, + }); + await makeAPICall(requestHandler, { query: { someQuery: false } }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example', { - body: '{"bodacious":true}', + EnterpriseSearchAPI.shouldHaveBeenCalledWith( + 'http://localhost:3002/api/example?someQuery=true' + ); }); }); - it('passes custom params set by the handler, which override request params', async () => { - const requestHandler = enterpriseSearchRequestHandler.createRequest({ - path: '/api/example', - params: { someQuery: true }, + describe('response passing', () => { + it('returns the response status code from Enterprise Search', async () => { + EnterpriseSearchAPI.mockReturn({}, { status: 201 }); + + const requestHandler = enterpriseSearchRequestHandler.createRequest({ + path: '/api/example', + }); + await makeAPICall(requestHandler); + + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example'); + expect(responseMock.custom).toHaveBeenCalledWith({ body: {}, statusCode: 201 }); }); - await makeAPICall(requestHandler, { query: { someQuery: false } }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith( - 'http://localhost:3002/api/example?someQuery=true' - ); + // TODO: It's possible we may also pass back headers at some point + // from Enterprise Search, e.g. the x-read-only mode header }); }); - describe('response passing', () => { - it('returns the response status code from Enterprise Search', async () => { - EnterpriseSearchAPI.mockReturn({}, { status: 404 }); + describe('error responses', () => { + describe('handleClientError()', () => { + afterEach(() => { + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/4xx'); + expect(mockLogger.error).not.toHaveBeenCalled(); + }); - const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/example' }); - await makeAPICall(requestHandler); + it('passes back json.error', async () => { + const error = 'some error message'; + EnterpriseSearchAPI.mockReturn({ error }, { status: 404, headers: JSON_HEADER }); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/example'); - expect(responseMock.custom).toHaveBeenCalledWith({ body: {}, statusCode: 404 }); - }); + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/4xx' }); + await makeAPICall(requestHandler); - // TODO: It's possible we may also pass back headers at some point - // from Enterprise Search, e.g. the x-read-only mode header - }); + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 404, + body: { + message: 'some error message', + attributes: { errors: ['some error message'] }, + }, + }); + }); - describe('error handling', () => { - afterEach(() => { - expect(mockLogger.error).toHaveBeenCalledWith( - expect.stringContaining('Error connecting to Enterprise Search') - ); + it('passes back json.errors', async () => { + const errors = ['one', 'two', 'three']; + EnterpriseSearchAPI.mockReturn({ errors }, { status: 400, headers: JSON_HEADER }); + + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/4xx' }); + await makeAPICall(requestHandler); + + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 400, + body: { + message: 'one,two,three', + attributes: { errors: ['one', 'two', 'three'] }, + }, + }); + }); + + it('handles empty json', async () => { + EnterpriseSearchAPI.mockReturn({}, { status: 400, headers: JSON_HEADER }); + + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/4xx' }); + await makeAPICall(requestHandler); + + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 400, + body: { + message: 'Bad Request', + attributes: { errors: ['Bad Request'] }, + }, + }); + }); + + it('handles blank bodies', async () => { + EnterpriseSearchAPI.mockReturn(undefined as any, { status: 404 }); + + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/4xx' }); + await makeAPICall(requestHandler); + + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 404, + body: { + message: 'Not Found', + attributes: { errors: ['Not Found'] }, + }, + }); + }); }); - it('returns an error when an API request fails', async () => { - EnterpriseSearchAPI.mockReturnError(); - const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/failed' }); + it('handleServerError()', async () => { + EnterpriseSearchAPI.mockReturn('something crashed!' as any, { status: 500 }); + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/5xx' }); await makeAPICall(requestHandler); - EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/failed'); + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/5xx'); expect(responseMock.customError).toHaveBeenCalledWith({ - body: 'Error connecting to Enterprise Search: Failed', statusCode: 502, + body: expect.stringContaining('Enterprise Search encountered an internal server error'), }); + expect(mockLogger.error).toHaveBeenCalledWith( + 'Enterprise Search Server Error 500 at : "something crashed!"' + ); }); - it('returns an error when `hasValidData` fails', async () => { + it('handleInvalidDataError()', async () => { EnterpriseSearchAPI.mockReturn({ results: false }); const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/invalid', @@ -143,15 +217,29 @@ describe('EnterpriseSearchRequestHandler', () => { EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/invalid'); expect(responseMock.customError).toHaveBeenCalledWith({ - body: 'Error connecting to Enterprise Search: Invalid data received', statusCode: 502, + body: 'Invalid data received from Enterprise Search', }); - expect(mockLogger.debug).toHaveBeenCalledWith( + expect(mockLogger.error).toHaveBeenCalledWith( 'Invalid data received from : {"results":false}' ); }); - describe('user authentication errors', () => { + it('handleConnectionError()', async () => { + EnterpriseSearchAPI.mockReturnError(); + const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/failed' }); + + await makeAPICall(requestHandler); + EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/failed'); + + expect(responseMock.customError).toHaveBeenCalledWith({ + statusCode: 502, + body: 'Error connecting to Enterprise Search: Failed', + }); + expect(mockLogger.error).toHaveBeenCalled(); + }); + + describe('handleAuthenticationError()', () => { afterEach(async () => { const requestHandler = enterpriseSearchRequestHandler.createRequest({ path: '/api/unauthenticated', @@ -160,9 +248,10 @@ describe('EnterpriseSearchRequestHandler', () => { EnterpriseSearchAPI.shouldHaveBeenCalledWith('http://localhost:3002/api/unauthenticated'); expect(responseMock.customError).toHaveBeenCalledWith({ - body: 'Error connecting to Enterprise Search: Cannot authenticate Enterprise Search user', statusCode: 502, + body: 'Cannot authenticate Enterprise Search user', }); + expect(mockLogger.error).toHaveBeenCalled(); }); it('errors when redirected to /login', async () => { @@ -175,7 +264,7 @@ describe('EnterpriseSearchRequestHandler', () => { }); }); - it('has a helper for checking empty objects', async () => { + it('isEmptyObj', async () => { expect(enterpriseSearchRequestHandler.isEmptyObj({})).toEqual(true); expect(enterpriseSearchRequestHandler.isEmptyObj({ empty: false })).toEqual(false); }); diff --git a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts index 18f10c590847c..00d5eaf5d6a83 100644 --- a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts +++ b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import fetch from 'node-fetch'; +import fetch, { Response } from 'node-fetch'; import querystring from 'querystring'; import { RequestHandler, @@ -25,6 +25,12 @@ interface IRequestParams { params?: object; hasValidData?: (body?: ResponseBody) => boolean; } +interface IErrorResponse { + message: string; + attributes: { + errors: string[]; + }; +} export interface IEnterpriseSearchRequestHandler { createRequest(requestParams?: object): RequestHandler; } @@ -71,34 +77,131 @@ export class EnterpriseSearchRequestHandler { ? JSON.stringify(request.body) : undefined; - // Call the Enterprise Search API and pass back response to the front-end + // Call the Enterprise Search API const apiResponse = await fetch(url, { method, headers, body }); + // Handle authentication redirects if (apiResponse.url.endsWith('/login') || apiResponse.url.endsWith('/ent/select')) { - throw new Error('Cannot authenticate Enterprise Search user'); + return this.handleAuthenticationError(response); } + // Handle 400-500+ responses from the Enterprise Search server const { status } = apiResponse; - const json = await apiResponse.json(); + if (status >= 500) { + return this.handleServerError(response, apiResponse, url); + } else if (status >= 400) { + return this.handleClientError(response, apiResponse); + } - if (hasValidData(json)) { - return response.custom({ statusCode: status, body: json }); - } else { - this.log.debug(`Invalid data received from <${url}>: ${JSON.stringify(json)}`); - throw new Error('Invalid data received'); + // Check returned data + const json = await apiResponse.json(); + if (!hasValidData(json)) { + return this.handleInvalidDataError(response, url, json); } + + // Pass successful responses back to the front-end + return response.custom({ statusCode: status, body: json }); } catch (e) { - const errorMessage = `Error connecting to Enterprise Search: ${e?.message || e.toString()}`; + // Catch connection/auth errors + return this.handleConnectionError(response, e); + } + }; + } - this.log.error(errorMessage); - if (e instanceof Error) this.log.debug(e.stack as string); + /** + * Attempt to grab a usable error body from Enterprise Search - this isn't + * always possible because some of our internal endpoints send back blank + * bodies, and sometimes the server sends back Ruby on Rails error pages + */ + async getErrorResponseBody(apiResponse: Response) { + const { statusText } = apiResponse; + const contentType = apiResponse.headers.get('content-type') || ''; - return response.customError({ statusCode: 502, body: errorMessage }); - } + // Default response + let body: IErrorResponse = { + message: statusText, + attributes: { errors: [statusText] }, }; + + try { + if (contentType.includes('application/json')) { + // Try parsing body as JSON + const json = await apiResponse.json(); + + // Some of our internal endpoints return either an `error` or `errors` key, + // which can both return either a string or array of strings ¯\_(ツ)_/¯ + const errors = json.error || json.errors || [statusText]; + body = { + message: errors.toString(), + attributes: { errors: Array.isArray(errors) ? errors : [errors] }, + }; + } else { + // Try parsing body as text/html + const text = await apiResponse.text(); + if (text) { + body = { + message: text, + attributes: { errors: [text] }, + }; + } + } + } catch { + // Fail silently + } + + return body; + } + + /** + * Error response helpers + */ + + async handleClientError(response: KibanaResponseFactory, apiResponse: Response) { + const { status } = apiResponse; + const body = await this.getErrorResponseBody(apiResponse); + + return response.customError({ statusCode: status, body }); + } + + async handleServerError(response: KibanaResponseFactory, apiResponse: Response, url: string) { + const { status } = apiResponse; + const { message } = await this.getErrorResponseBody(apiResponse); + + // Don't expose server errors to the front-end, as they may contain sensitive stack traces + const errorMessage = + 'Enterprise Search encountered an internal server error. Please contact your system administrator if the problem persists.'; + + this.log.error(`Enterprise Search Server Error ${status} at <${url}>: ${message}`); + return response.customError({ statusCode: 502, body: errorMessage }); + } + + handleInvalidDataError(response: KibanaResponseFactory, url: string, json: object) { + const errorMessage = 'Invalid data received from Enterprise Search'; + + this.log.error(`Invalid data received from <${url}>: ${JSON.stringify(json)}`); + return response.customError({ statusCode: 502, body: errorMessage }); } - // Small helper + handleConnectionError(response: KibanaResponseFactory, e: Error) { + const errorMessage = `Error connecting to Enterprise Search: ${e?.message || e.toString()}`; + + this.log.error(errorMessage); + if (e instanceof Error) this.log.debug(e.stack as string); + + return response.customError({ statusCode: 502, body: errorMessage }); + } + + handleAuthenticationError(response: KibanaResponseFactory) { + const errorMessage = 'Cannot authenticate Enterprise Search user'; + + this.log.error(errorMessage); + return response.customError({ statusCode: 502, body: errorMessage }); + } + + /** + * Misc helpers + */ + isEmptyObj(obj: object) { return Object.keys(obj).length === 0; }