From 2ee1d575052e6afbd913c2480593bd15f4013bc7 Mon Sep 17 00:00:00 2001 From: restrry Date: Thu, 1 Aug 2019 13:42:00 +0200 Subject: [PATCH 01/14] add response factory to the interceptors --- src/core/server/http/http_server.mocks.ts | 22 +- src/core/server/http/http_server.test.ts | 387 +------- src/core/server/http/http_server.ts | 40 +- src/core/server/http/http_service.mock.ts | 16 +- src/core/server/http/index.ts | 1 + ...st.mocks.ts => core_service.test.mocks.ts} | 0 .../integration_tests/core_services.test.ts | 308 ++++++ .../integration_tests/http_service.test.ts | 428 -------- .../http/integration_tests/lifecycle.test.ts | 921 ++++++++++++++++++ src/core/server/http/lifecycle/auth.test.ts | 110 --- src/core/server/http/lifecycle/auth.ts | 92 +- .../http/lifecycle/on_post_auth.test.ts | 95 -- .../server/http/lifecycle/on_post_auth.ts | 92 +- .../server/http/lifecycle/on_pre_auth.test.ts | 115 --- src/core/server/http/lifecycle/on_pre_auth.ts | 107 +- src/core/server/http/router/index.ts | 9 +- .../server/http/router/response_adapter.ts | 59 +- src/core/server/http/router/router.ts | 12 +- src/core/server/index.ts | 1 + 19 files changed, 1441 insertions(+), 1374 deletions(-) rename src/core/server/http/integration_tests/{http_service.test.mocks.ts => core_service.test.mocks.ts} (100%) create mode 100644 src/core/server/http/integration_tests/core_services.test.ts delete mode 100644 src/core/server/http/integration_tests/http_service.test.ts create mode 100644 src/core/server/http/integration_tests/lifecycle.test.ts delete mode 100644 src/core/server/http/lifecycle/auth.test.ts delete mode 100644 src/core/server/http/lifecycle/on_post_auth.test.ts delete mode 100644 src/core/server/http/lifecycle/on_pre_auth.test.ts diff --git a/src/core/server/http/http_server.mocks.ts b/src/core/server/http/http_server.mocks.ts index bb52fa1fa38df9..8500e6cebc6dc8 100644 --- a/src/core/server/http/http_server.mocks.ts +++ b/src/core/server/http/http_server.mocks.ts @@ -16,14 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -import { Request, ResponseToolkit } from 'hapi'; +import { Request } from 'hapi'; import { merge } from 'lodash'; import querystring from 'querystring'; import { schema } from '@kbn/config-schema'; -import { KibanaRequest, RouteMethod } from './router'; +import { KibanaRequest, RouteMethod, ResponseFactory } from './router'; interface RequestFixtureOptions { headers?: Record; @@ -97,12 +97,22 @@ function createRawRequestMock(customization: DeepPartial = {}) { ) as Request; } -function createRawResponseToolkitMock(customization: DeepPartial = {}) { - return merge({}, customization) as ResponseToolkit; -} +const createResponseFactoryMock = (): jest.Mocked => ({ + ok: jest.fn(), + accepted: jest.fn(), + noContent: jest.fn(), + custom: jest.fn(), + redirected: jest.fn(), + badRequest: jest.fn(), + unauthorized: jest.fn(), + forbidden: jest.fn(), + notFound: jest.fn(), + conflict: jest.fn(), + internal: jest.fn(), +}); export const httpServerMock = { createKibanaRequest: createKibanaRequestMock, createRawRequest: createRawRequestMock, - createRawResponseToolkit: createRawResponseToolkitMock, + createResponseFactory: createResponseFactoryMock, }; diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index 83e569e8752c5a..793201b98fb101 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -18,8 +18,6 @@ */ import { Server } from 'http'; -import request from 'request'; -import Boom from 'boom'; jest.mock('fs', () => ({ readFileSync: jest.fn(), @@ -40,16 +38,6 @@ const cookieOptions = { isSecure: false, }; -interface User { - id: string; - roles?: string[]; -} - -interface StorageData { - value: User; - expires: number; -} - const chance = new Chance(); let server: HttpServer; @@ -498,78 +486,12 @@ test('returns server and connection options on start', async () => { expect(innerServer).toBe((server as any).server); }); -test('registers registerOnPostAuth interceptor several times', async () => { - const { registerOnPostAuth } = await server.setup(config); - const doRegister = () => registerOnPostAuth(() => null as any); - - doRegister(); - expect(doRegister).not.toThrowError(); -}); - test('throws an error if starts without set up', async () => { await expect(server.start()).rejects.toThrowErrorMatchingInlineSnapshot( `"Http server is not setup up yet"` ); }); -test('enables auth for a route by default if registerAuth has been called', async () => { - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - - const router = new Router(''); - router.get({ path: '/', validate: false }, (req, res) => - res.ok({ authRequired: req.route.options.authRequired }) - ); - registerRouter(router); - - const authenticate = jest.fn().mockImplementation((req, t) => t.authenticated()); - registerAuth(authenticate); - - await server.start(); - await supertest(innerServer.listener) - .get('/') - .expect(200, { authRequired: true }); - - expect(authenticate).toHaveBeenCalledTimes(1); -}); - -test('supports disabling auth for a route explicitly', async () => { - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - - const router = new Router(''); - router.get({ path: '/', validate: false, options: { authRequired: false } }, (req, res) => - res.ok({ authRequired: req.route.options.authRequired }) - ); - registerRouter(router); - const authenticate = jest.fn(); - registerAuth(authenticate); - - await server.start(); - await supertest(innerServer.listener) - .get('/') - .expect(200, { authRequired: false }); - - expect(authenticate).toHaveBeenCalledTimes(0); -}); - -test('supports enabling auth for a route explicitly', async () => { - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - - const router = new Router(''); - router.get({ path: '/', validate: false, options: { authRequired: true } }, (req, res) => - res.ok({ authRequired: req.route.options.authRequired }) - ); - registerRouter(router); - const authenticate = jest.fn().mockImplementation((req, t) => t.authenticated({})); - await registerAuth(authenticate); - - await server.start(); - await supertest(innerServer.listener) - .get('/') - .expect(200, { authRequired: true }); - - expect(authenticate).toHaveBeenCalledTimes(1); -}); - test('allows attaching metadata to attach meta-data tag strings to a route', async () => { const tags = ['my:tag']; const { registerRouter, server: innerServer } = await server.setup(config); @@ -629,307 +551,6 @@ describe('setup contract', () => { expect(create()).rejects.toThrowError('A cookieSessionStorageFactory was already created'); }); }); - describe('#registerAuth', () => { - it('registers auth request interceptor only once', async () => { - const { registerAuth } = await server.setup(config); - const doRegister = () => registerAuth(() => null as any); - - doRegister(); - expect(doRegister).toThrowError('Auth interceptor was already registered'); - }); - - it('may grant access to a resource', async () => { - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - const router = new Router(''); - router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); - registerRouter(router); - - await registerAuth((req, t) => t.authenticated()); - await server.start(); - - await supertest(innerServer.listener) - .get('/') - .expect(200, { content: 'ok' }); - }); - - it('supports rejecting a request from an unauthenticated user', async () => { - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - const router = new Router(''); - router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); - registerRouter(router); - - await registerAuth((req, t) => t.rejected(Boom.unauthorized())); - await server.start(); - - await supertest(innerServer.listener) - .get('/') - .expect(401); - }); - - it('supports redirecting', async () => { - const redirectTo = '/redirect-url'; - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - const router = new Router(''); - router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); - registerRouter(router); - - await registerAuth((req, t) => { - return t.redirected(redirectTo); - }); - await server.start(); - - const response = await supertest(innerServer.listener) - .get('/') - .expect(302); - expect(response.header.location).toBe(redirectTo); - }); - - it(`doesn't expose internal error details`, async () => { - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - const router = new Router(''); - router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); - registerRouter(router); - - await registerAuth((req, t) => { - throw new Error('sensitive info'); - }); - await server.start(); - - await supertest(innerServer.listener) - .get('/') - .expect({ - statusCode: 500, - error: 'Internal Server Error', - message: 'An internal server error occurred', - }); - }); - - it('allows manipulating cookies via cookie session storage', async () => { - const router = new Router(''); - router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); - - const { - createCookieSessionStorageFactory, - registerAuth, - registerRouter, - server: innerServer, - } = await server.setup(config); - const sessionStorageFactory = await createCookieSessionStorageFactory( - cookieOptions - ); - registerAuth((req, t) => { - const user = { id: '42' }; - const sessionStorage = sessionStorageFactory.asScoped(req); - sessionStorage.set({ value: user, expires: Date.now() + 1000 }); - return t.authenticated({ state: user }); - }); - registerRouter(router); - await server.start(); - - const response = await supertest(innerServer.listener) - .get('/') - .expect(200, { content: 'ok' }); - - expect(response.header['set-cookie']).toBeDefined(); - const cookies = response.header['set-cookie']; - expect(cookies).toHaveLength(1); - - const sessionCookie = request.cookie(cookies[0]); - if (!sessionCookie) { - throw new Error('session cookie expected to be defined'); - } - expect(sessionCookie).toBeDefined(); - expect(sessionCookie.key).toBe('sid'); - expect(sessionCookie.value).toBeDefined(); - expect(sessionCookie.path).toBe('/'); - expect(sessionCookie.httpOnly).toBe(true); - }); - - it('allows manipulating cookies from route handler', async () => { - const { - createCookieSessionStorageFactory, - registerAuth, - registerRouter, - server: innerServer, - } = await server.setup(config); - const sessionStorageFactory = await createCookieSessionStorageFactory( - cookieOptions - ); - registerAuth((req, t) => { - const user = { id: '42' }; - const sessionStorage = sessionStorageFactory.asScoped(req); - sessionStorage.set({ value: user, expires: Date.now() + 1000 }); - return t.authenticated(); - }); - - const router = new Router(''); - router.get({ path: '/', validate: false }, (req, res) => res.ok({ content: 'ok' })); - router.get({ path: '/with-cookie', validate: false }, (req, res) => { - const sessionStorage = sessionStorageFactory.asScoped(req); - sessionStorage.clear(); - return res.ok({ content: 'ok' }); - }); - registerRouter(router); - - await server.start(); - - const responseToSetCookie = await supertest(innerServer.listener) - .get('/') - .expect(200); - - expect(responseToSetCookie.header['set-cookie']).toBeDefined(); - - const responseToResetCookie = await supertest(innerServer.listener) - .get('/with-cookie') - .expect(200); - - expect(responseToResetCookie.header['set-cookie']).toEqual([ - 'sid=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; Path=/', - ]); - }); - - it.skip('is the only place with access to the authorization header', async () => { - const token = 'Basic: user:password'; - const { - registerAuth, - registerOnPreAuth, - registerOnPostAuth, - registerRouter, - server: innerServer, - } = await server.setup(config); - - let fromRegisterOnPreAuth; - await registerOnPreAuth((req, t) => { - fromRegisterOnPreAuth = req.headers.authorization; - return t.next(); - }); - - let fromRegisterAuth; - await registerAuth((req, t) => { - fromRegisterAuth = req.headers.authorization; - return t.authenticated(); - }); - - let fromRegisterOnPostAuth; - await registerOnPostAuth((req, t) => { - fromRegisterOnPostAuth = req.headers.authorization; - return t.next(); - }); - - let fromRouteHandler; - const router = new Router(''); - router.get({ path: '/', validate: false }, (req, res) => { - fromRouteHandler = req.headers.authorization; - return res.ok({ content: 'ok' }); - }); - registerRouter(router); - - await server.start(); - - await supertest(innerServer.listener) - .get('/') - .set('Authorization', token) - .expect(200); - - expect(fromRegisterOnPreAuth).toEqual({}); - expect(fromRegisterAuth).toEqual({ authorization: token }); - expect(fromRegisterOnPostAuth).toEqual({}); - expect(fromRouteHandler).toEqual({}); - }); - - it('attach security header to a successful response', async () => { - const authResponseHeader = { - 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', - }; - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - - await registerAuth((req, t) => { - return t.authenticated({ responseHeaders: authResponseHeader }); - }); - - const router = new Router('/'); - router.get({ path: '/', validate: false }, (req, res) => res.ok({ header: 'ok' })); - registerRouter(router); - - await server.start(); - - const response = await supertest(innerServer.listener) - .get('/') - .expect(200); - - expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']); - }); - - it('attach security header to an error response', async () => { - const authResponseHeader = { - 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', - }; - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - - await registerAuth((req, t) => { - return t.authenticated({ responseHeaders: authResponseHeader }); - }); - - const router = new Router('/'); - router.get({ path: '/', validate: false }, (req, res) => res.badRequest(new Error('reason'))); - registerRouter(router); - - await server.start(); - - const response = await supertest(innerServer.listener) - .get('/') - .expect(400); - - expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']); - }); - - // TODO un-skip when NP ResponseFactory supports configuring custom headers - it.skip('logs warning if Auth Security Header rewrites response header for success response', async () => { - const authResponseHeader = { - 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', - }; - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - - await registerAuth((req, t) => { - return t.authenticated({ responseHeaders: authResponseHeader }); - }); - - const router = new Router('/'); - router.get({ path: '/', validate: false }, (req, res) => res.ok({})); - registerRouter(router); - - await server.start(); - - await supertest(innerServer.listener) - .get('/') - .expect(200); - - expect(loggingServiceMock.collect(logger).warn).toMatchInlineSnapshot(); - }); - - it.skip('logs warning if Auth Security Header rewrites response header for error response', async () => { - const authResponseHeader = { - 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', - }; - const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); - - await registerAuth((req, t) => { - return t.authenticated({ responseHeaders: authResponseHeader }); - }); - - const router = new Router('/'); - router.get({ path: '/', validate: false }, (req, res) => res.badRequest(new Error('reason'))); - registerRouter(router); - - await server.start(); - - await supertest(innerServer.listener) - .get('/') - .expect(400); - - expect(loggingServiceMock.collect(logger).warn).toMatchInlineSnapshot(); - }); - }); describe('#auth.isAuthenticated()', () => { it('returns true if has been authorized', async () => { @@ -943,7 +564,7 @@ describe('setup contract', () => { ); registerRouter(router); - await registerAuth((req, t) => t.authenticated()); + await registerAuth((req, res, toolkit) => toolkit.authenticated()); await server.start(); await supertest(innerServer.listener) @@ -962,7 +583,7 @@ describe('setup contract', () => { ); registerRouter(router); - await registerAuth((req, t) => t.authenticated()); + await registerAuth((req, res, toolkit) => toolkit.authenticated()); await server.start(); await supertest(innerServer.listener) @@ -997,9 +618,9 @@ describe('setup contract', () => { auth, } = await server.setup(config); const sessionStorageFactory = await createCookieSessionStorageFactory(cookieOptions); - registerAuth((req, t) => { + registerAuth((req, res, toolkit) => { sessionStorageFactory.asScoped(req).set({ value: user, expires: Date.now() + 1000 }); - return t.authenticated({ state: user }); + return toolkit.authenticated({ state: user }); }); const router = new Router(''); diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index c8b4c5c17f05e1..76a1740f611ea9 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -186,14 +186,14 @@ export class HttpServer { return; } - this.registerOnPreAuth((request, toolkit) => { + this.registerOnPreAuth((request, response, toolkit) => { const oldUrl = request.url.href!; const newURL = basePathService.remove(oldUrl); const shouldRedirect = newURL !== oldUrl; if (shouldRedirect) { - return toolkit.redirected(newURL, { forward: true }); + return toolkit.rewriteUrl(newURL); } - return toolkit.rejected(new Error('not found'), { statusCode: 404 }); + return response.notFound(new Error('not found')); }); } @@ -209,7 +209,7 @@ export class HttpServer { throw new Error('Server is not created yet'); } - this.server.ext('onPostAuth', adoptToHapiOnPostAuthFormat(fn)); + this.server.ext('onPostAuth', adoptToHapiOnPostAuthFormat(fn, this.log)); } private registerOnPreAuth(fn: OnPreAuthHandler) { @@ -217,7 +217,7 @@ export class HttpServer { throw new Error('Server is not created yet'); } - this.server.ext('onRequest', adoptToHapiOnPreAuthFormat(fn)); + this.server.ext('onRequest', adoptToHapiOnPreAuthFormat(fn, this.log)); } private async createCookieSessionStorageFactory( @@ -250,20 +250,24 @@ export class HttpServer { this.authRegistered = true; this.server.auth.scheme('login', () => ({ - authenticate: adoptToHapiAuthFormat(fn, (req, { state, requestHeaders, responseHeaders }) => { - this.authState.set(req, state); - - if (responseHeaders) { - this.authResponseHeaders.set(req, responseHeaders); - } - - if (requestHeaders) { - this.authRequestHeaders.set(req, requestHeaders); - // we mutate headers only for the backward compatibility with the legacy platform. - // where some plugin read directly from headers to identify whether a user is authenticated. - Object.assign(req.headers, requestHeaders); + authenticate: adoptToHapiAuthFormat( + fn, + this.log, + (req, { state, requestHeaders, responseHeaders }) => { + this.authState.set(req, state); + + if (responseHeaders) { + this.authResponseHeaders.set(req, responseHeaders); + } + + if (requestHeaders) { + this.authRequestHeaders.set(req, requestHeaders); + // we mutate headers only for the backward compatibility with the legacy platform. + // where some plugin read directly from headers to identify whether a user is authenticated. + Object.assign(req.headers, requestHeaders); + } } - }), + ), })); this.server.auth.strategy('session', 'login'); diff --git a/src/core/server/http/http_service.mock.ts b/src/core/server/http/http_service.mock.ts index 02103fc4acc848..6c7dc56f1c5c26 100644 --- a/src/core/server/http/http_service.mock.ts +++ b/src/core/server/http/http_service.mock.ts @@ -18,12 +18,10 @@ */ import { Server } from 'hapi'; -import { HttpService } from './http_service'; import { HttpServerSetup } from './http_server'; -import { HttpServiceSetup } from './http_service'; +import { HttpService, HttpServiceSetup } from './http_service'; import { OnPreAuthToolkit } from './lifecycle/on_pre_auth'; import { AuthToolkit } from './lifecycle/auth'; -import { OnPostAuthToolkit } from './lifecycle/on_post_auth'; import { sessionStorageMock } from './cookie_session_storage.mocks'; type ServiceSetupMockType = jest.Mocked & { @@ -75,20 +73,11 @@ const createHttpServiceMock = () => { const createOnPreAuthToolkitMock = (): jest.Mocked => ({ next: jest.fn(), - redirected: jest.fn(), - rejected: jest.fn(), + rewriteUrl: jest.fn(), }); const createAuthToolkitMock = (): jest.Mocked => ({ authenticated: jest.fn(), - redirected: jest.fn(), - rejected: jest.fn(), -}); - -const createOnPostAuthToolkitMock = (): jest.Mocked => ({ - next: jest.fn(), - redirected: jest.fn(), - rejected: jest.fn(), }); export const httpServiceMock = { @@ -97,5 +86,4 @@ export const httpServiceMock = { createSetupContract: createSetupContractMock, createOnPreAuthToolkit: createOnPreAuthToolkitMock, createAuthToolkit: createAuthToolkitMock, - createOnPostAuthToolkit: createOnPostAuthToolkitMock, }; diff --git a/src/core/server/http/index.ts b/src/core/server/http/index.ts index eff7de0979c119..479779d11101e1 100644 --- a/src/core/server/http/index.ts +++ b/src/core/server/http/index.ts @@ -26,6 +26,7 @@ export { KibanaRequestRoute, ResponseError, ResponseErrorMeta, + ResponseFactory, Router, RouteMethod, RouteConfigOptions, diff --git a/src/core/server/http/integration_tests/http_service.test.mocks.ts b/src/core/server/http/integration_tests/core_service.test.mocks.ts similarity index 100% rename from src/core/server/http/integration_tests/http_service.test.mocks.ts rename to src/core/server/http/integration_tests/core_service.test.mocks.ts diff --git a/src/core/server/http/integration_tests/core_services.test.ts b/src/core/server/http/integration_tests/core_services.test.ts new file mode 100644 index 00000000000000..691b1d01687148 --- /dev/null +++ b/src/core/server/http/integration_tests/core_services.test.ts @@ -0,0 +1,308 @@ +/* + * 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 Boom from 'boom'; +import { Request } from 'hapi'; +import { first } from 'rxjs/operators'; +import { clusterClientMock } from './core_service.test.mocks'; + +import { Router } from '../router'; +import * as kbnTestServer from '../../../../test_utils/kbn_server'; + +interface User { + id: string; + roles?: string[]; +} + +interface StorageData { + value: User; + expires: number; +} + +describe('http service', () => { + describe('legacy server', () => { + describe('#registerAuth()', () => { + const sessionDurationMs = 1000; + const cookieOptions = { + name: 'sid', + encryptionKey: 'something_at_least_32_characters', + validate: (session: StorageData) => true, + isSecure: false, + path: '/', + }; + + let root: ReturnType; + beforeEach(async () => { + root = kbnTestServer.createRoot(); + }, 30000); + + afterEach(async () => { + clusterClientMock.mockClear(); + await root.shutdown(); + }); + + it('runs auth for legacy routes and proxy request to legacy server route handlers', async () => { + const { http } = await root.setup(); + const sessionStorageFactory = await http.createCookieSessionStorageFactory( + cookieOptions + ); + http.registerAuth((req, res, toolkit) => { + if (req.headers.authorization) { + const user = { id: '42' }; + const sessionStorage = sessionStorageFactory.asScoped(req); + sessionStorage.set({ value: user, expires: Date.now() + sessionDurationMs }); + return toolkit.authenticated({ state: user }); + } else { + return res.unauthorized(); + } + }); + await root.start(); + + const legacyUrl = '/legacy'; + const kbnServer = kbnTestServer.getKbnServer(root); + kbnServer.server.route({ + method: 'GET', + path: legacyUrl, + handler: () => 'ok from legacy server', + }); + + const response = await kbnTestServer.request + .get(root, legacyUrl) + .expect(200, 'ok from legacy server'); + + expect(response.header['set-cookie']).toHaveLength(1); + }); + + it('passes authHeaders as request headers to the legacy platform', async () => { + const token = 'Basic: name:password'; + const { http } = await root.setup(); + const sessionStorageFactory = await http.createCookieSessionStorageFactory( + cookieOptions + ); + http.registerAuth((req, res, toolkit) => { + if (req.headers.authorization) { + const user = { id: '42' }; + const sessionStorage = sessionStorageFactory.asScoped(req); + sessionStorage.set({ value: user, expires: Date.now() + sessionDurationMs }); + return toolkit.authenticated({ + state: user, + requestHeaders: { + authorization: token, + }, + }); + } else { + return res.unauthorized(); + } + }); + await root.start(); + + const legacyUrl = '/legacy'; + const kbnServer = kbnTestServer.getKbnServer(root); + kbnServer.server.route({ + method: 'GET', + path: legacyUrl, + handler: (req: Request) => ({ + authorization: req.headers.authorization, + custom: req.headers.custom, + }), + }); + + await kbnTestServer.request + .get(root, legacyUrl) + .set({ custom: 'custom-header' }) + .expect(200, { authorization: token, custom: 'custom-header' }); + }); + + it('passes associated auth state to Legacy platform', async () => { + const user = { id: '42' }; + + const { http } = await root.setup(); + const sessionStorageFactory = await http.createCookieSessionStorageFactory( + cookieOptions + ); + http.registerAuth((req, res, toolkit) => { + if (req.headers.authorization) { + const sessionStorage = sessionStorageFactory.asScoped(req); + sessionStorage.set({ value: user, expires: Date.now() + sessionDurationMs }); + return toolkit.authenticated({ state: user }); + } else { + return res.unauthorized(); + } + }); + await root.start(); + + const legacyUrl = '/legacy'; + const kbnServer = kbnTestServer.getKbnServer(root); + kbnServer.server.route({ + method: 'GET', + path: legacyUrl, + handler: kbnServer.newPlatform.setup.core.http.auth.get, + }); + + const response = await kbnTestServer.request.get(root, legacyUrl).expect(200); + expect(response.body.state).toEqual(user); + expect(response.body.status).toEqual('authenticated'); + + expect(response.header['set-cookie']).toHaveLength(1); + }); + + it('attach security header to a successful response handled by Legacy platform', async () => { + const authResponseHeader = { + 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', + }; + const { http } = await root.setup(); + const { registerAuth } = http; + + await registerAuth((req, res, toolkit) => { + return toolkit.authenticated({ responseHeaders: authResponseHeader }); + }); + + await root.start(); + + const kbnServer = kbnTestServer.getKbnServer(root); + kbnServer.server.route({ + method: 'GET', + path: '/legacy', + handler: () => 'ok', + }); + + const response = await kbnTestServer.request.get(root, '/legacy').expect(200); + expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']); + }); + + it('attach security header to an error response handled by Legacy platform', async () => { + const authResponseHeader = { + 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', + }; + const { http } = await root.setup(); + const { registerAuth } = http; + + await registerAuth((req, res, toolkit) => { + return toolkit.authenticated({ responseHeaders: authResponseHeader }); + }); + + await root.start(); + + const kbnServer = kbnTestServer.getKbnServer(root); + kbnServer.server.route({ + method: 'GET', + path: '/legacy', + handler: () => { + throw Boom.badRequest(); + }, + }); + + const response = await kbnTestServer.request.get(root, '/legacy').expect(400); + expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']); + }); + }); + + describe('#basePath()', () => { + let root: ReturnType; + beforeEach(async () => { + root = kbnTestServer.createRoot(); + }, 30000); + + afterEach(async () => await root.shutdown()); + it('basePath information for an incoming request is available in legacy server', async () => { + const reqBasePath = '/requests-specific-base-path'; + const { http } = await root.setup(); + http.registerOnPreAuth((req, res, toolkit) => { + http.basePath.set(req, reqBasePath); + return toolkit.next(); + }); + + await root.start(); + + const legacyUrl = '/legacy'; + const kbnServer = kbnTestServer.getKbnServer(root); + kbnServer.server.route({ + method: 'GET', + path: legacyUrl, + handler: kbnServer.newPlatform.setup.core.http.basePath.get, + }); + + await kbnTestServer.request.get(root, legacyUrl).expect(200, reqBasePath); + }); + }); + }); + describe('elasticsearch', () => { + let root: ReturnType; + beforeEach(async () => { + root = kbnTestServer.createRoot(); + }, 30000); + + afterEach(async () => { + clusterClientMock.mockClear(); + await root.shutdown(); + }); + it('rewrites authorization header via authHeaders to make a request to Elasticsearch', async () => { + const authHeaders = { authorization: 'Basic: user:password' }; + const { http, elasticsearch } = await root.setup(); + const { registerAuth, registerRouter } = http; + + await registerAuth((req, res, toolkit) => + toolkit.authenticated({ requestHeaders: authHeaders }) + ); + + const router = new Router('/new-platform'); + router.get({ path: '/', validate: false }, async (req, res) => { + const client = await elasticsearch.dataClient$.pipe(first()).toPromise(); + client.asScoped(req); + return res.ok({ header: 'ok' }); + }); + registerRouter(router); + + await root.start(); + + await kbnTestServer.request.get(root, '/new-platform/').expect(200); + expect(clusterClientMock).toBeCalledTimes(1); + const [firstCall] = clusterClientMock.mock.calls; + const [, , headers] = firstCall; + expect(headers).toEqual(authHeaders); + }); + + it('passes request authorization header to Elasticsearch if registerAuth was not set', async () => { + const authorizationHeader = 'Basic: username:password'; + const { http, elasticsearch } = await root.setup(); + const { registerRouter } = http; + + const router = new Router('/new-platform'); + router.get({ path: '/', validate: false }, async (req, res) => { + const client = await elasticsearch.dataClient$.pipe(first()).toPromise(); + client.asScoped(req); + return res.ok({ header: 'ok' }); + }); + registerRouter(router); + + await root.start(); + + await kbnTestServer.request + .get(root, '/new-platform/') + .set('Authorization', authorizationHeader) + .expect(200); + + expect(clusterClientMock).toBeCalledTimes(1); + const [firstCall] = clusterClientMock.mock.calls; + const [, , headers] = firstCall; + expect(headers).toEqual({ + authorization: authorizationHeader, + }); + }); + }); +}); diff --git a/src/core/server/http/integration_tests/http_service.test.ts b/src/core/server/http/integration_tests/http_service.test.ts deleted file mode 100644 index 303b81a73da3cf..00000000000000 --- a/src/core/server/http/integration_tests/http_service.test.ts +++ /dev/null @@ -1,428 +0,0 @@ -/* - * 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 Boom from 'boom'; -import { Request } from 'hapi'; -import { first } from 'rxjs/operators'; -import { clusterClientMock } from './http_service.test.mocks'; - -import { Router } from '../router'; -import * as kbnTestServer from '../../../../test_utils/kbn_server'; - -interface User { - id: string; - roles?: string[]; -} - -interface StorageData { - value: User; - expires: number; -} - -describe('http service', () => { - describe('setup contract', () => { - describe('#registerAuth()', () => { - const sessionDurationMs = 1000; - const cookieOptions = { - name: 'sid', - encryptionKey: 'something_at_least_32_characters', - validate: (session: StorageData) => true, - isSecure: false, - path: '/', - }; - - let root: ReturnType; - beforeEach(async () => { - root = kbnTestServer.createRoot(); - }, 30000); - - afterEach(async () => { - clusterClientMock.mockClear(); - await root.shutdown(); - }); - - it('runs auth for legacy routes and proxy request to legacy server route handlers', async () => { - const { http } = await root.setup(); - const sessionStorageFactory = await http.createCookieSessionStorageFactory( - cookieOptions - ); - http.registerAuth((req, t) => { - if (req.headers.authorization) { - const user = { id: '42' }; - const sessionStorage = sessionStorageFactory.asScoped(req); - sessionStorage.set({ value: user, expires: Date.now() + sessionDurationMs }); - return t.authenticated({ state: user }); - } else { - return t.rejected(Boom.unauthorized()); - } - }); - await root.start(); - - const legacyUrl = '/legacy'; - const kbnServer = kbnTestServer.getKbnServer(root); - kbnServer.server.route({ - method: 'GET', - path: legacyUrl, - handler: () => 'ok from legacy server', - }); - - const response = await kbnTestServer.request - .get(root, legacyUrl) - .expect(200, 'ok from legacy server'); - - expect(response.header['set-cookie']).toHaveLength(1); - }); - - it('passes authHeaders as request headers to the legacy platform', async () => { - const token = 'Basic: name:password'; - const { http } = await root.setup(); - const sessionStorageFactory = await http.createCookieSessionStorageFactory( - cookieOptions - ); - http.registerAuth((req, t) => { - if (req.headers.authorization) { - const user = { id: '42' }; - const sessionStorage = sessionStorageFactory.asScoped(req); - sessionStorage.set({ value: user, expires: Date.now() + sessionDurationMs }); - return t.authenticated({ - state: user, - requestHeaders: { - authorization: token, - }, - }); - } else { - return t.rejected(Boom.unauthorized()); - } - }); - await root.start(); - - const legacyUrl = '/legacy'; - const kbnServer = kbnTestServer.getKbnServer(root); - kbnServer.server.route({ - method: 'GET', - path: legacyUrl, - handler: (req: Request) => ({ - authorization: req.headers.authorization, - custom: req.headers.custom, - }), - }); - - await kbnTestServer.request - .get(root, legacyUrl) - .set({ custom: 'custom-header' }) - .expect(200, { authorization: token, custom: 'custom-header' }); - }); - - it('passes associated auth state to Legacy platform', async () => { - const user = { id: '42' }; - - const { http } = await root.setup(); - const sessionStorageFactory = await http.createCookieSessionStorageFactory( - cookieOptions - ); - http.registerAuth((req, t) => { - if (req.headers.authorization) { - const sessionStorage = sessionStorageFactory.asScoped(req); - sessionStorage.set({ value: user, expires: Date.now() + sessionDurationMs }); - return t.authenticated({ state: user }); - } else { - return t.rejected(Boom.unauthorized()); - } - }); - await root.start(); - - const legacyUrl = '/legacy'; - const kbnServer = kbnTestServer.getKbnServer(root); - kbnServer.server.route({ - method: 'GET', - path: legacyUrl, - handler: kbnServer.newPlatform.setup.core.http.auth.get, - }); - - const response = await kbnTestServer.request.get(root, legacyUrl).expect(200); - expect(response.body.state).toEqual(user); - expect(response.body.status).toEqual('authenticated'); - - expect(response.header['set-cookie']).toHaveLength(1); - }); - - it('rewrites authorization header via authHeaders to make a request to Elasticsearch', async () => { - const authHeaders = { authorization: 'Basic: user:password' }; - const { http, elasticsearch } = await root.setup(); - const { registerAuth, registerRouter } = http; - - await registerAuth((req, t) => { - return t.authenticated({ requestHeaders: authHeaders }); - }); - - const router = new Router('/new-platform'); - router.get({ path: '/', validate: false }, async (req, res) => { - const client = await elasticsearch.dataClient$.pipe(first()).toPromise(); - client.asScoped(req); - return res.ok({ header: 'ok' }); - }); - registerRouter(router); - - await root.start(); - - await kbnTestServer.request.get(root, '/new-platform/').expect(200); - expect(clusterClientMock).toBeCalledTimes(1); - const [firstCall] = clusterClientMock.mock.calls; - const [, , headers] = firstCall; - expect(headers).toEqual(authHeaders); - }); - - it('passes request authorization header to Elasticsearch if registerAuth was not set', async () => { - const authorizationHeader = 'Basic: username:password'; - const { http, elasticsearch } = await root.setup(); - const { registerRouter } = http; - - const router = new Router('/new-platform'); - router.get({ path: '/', validate: false }, async (req, res) => { - const client = await elasticsearch.dataClient$.pipe(first()).toPromise(); - client.asScoped(req); - return res.ok({ header: 'ok' }); - }); - registerRouter(router); - - await root.start(); - - await kbnTestServer.request - .get(root, '/new-platform/') - .set('Authorization', authorizationHeader) - .expect(200); - - expect(clusterClientMock).toBeCalledTimes(1); - const [firstCall] = clusterClientMock.mock.calls; - const [, , headers] = firstCall; - expect(headers).toEqual({ - authorization: authorizationHeader, - }); - }); - - it('attach security header to a successful response handled by Legacy platform', async () => { - const authResponseHeader = { - 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', - }; - const { http } = await root.setup(); - const { registerAuth } = http; - - await registerAuth((req, t) => { - return t.authenticated({ responseHeaders: authResponseHeader }); - }); - - await root.start(); - - const kbnServer = kbnTestServer.getKbnServer(root); - kbnServer.server.route({ - method: 'GET', - path: '/legacy', - handler: () => 'ok', - }); - - const response = await kbnTestServer.request.get(root, '/legacy').expect(200); - expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']); - }); - - it('attach security header to an error response handled by Legacy platform', async () => { - const authResponseHeader = { - 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', - }; - const { http } = await root.setup(); - const { registerAuth } = http; - - await registerAuth((req, t) => { - return t.authenticated({ responseHeaders: authResponseHeader }); - }); - - await root.start(); - - const kbnServer = kbnTestServer.getKbnServer(root); - kbnServer.server.route({ - method: 'GET', - path: '/legacy', - handler: () => { - throw Boom.badRequest(); - }, - }); - - const response = await kbnTestServer.request.get(root, '/legacy').expect(400); - expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']); - }); - }); - - describe('#registerOnPostAuth()', () => { - let root: ReturnType; - beforeEach(async () => { - root = kbnTestServer.createRoot(); - }, 30000); - afterEach(async () => await root.shutdown()); - - it('supports passing request through to the route handler', async () => { - const router = new Router('/new-platform'); - router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); - - const { http } = await root.setup(); - http.registerOnPostAuth((req, t) => t.next()); - http.registerOnPostAuth(async (req, t) => { - await Promise.resolve(); - return t.next(); - }); - http.registerRouter(router); - await root.start(); - - await kbnTestServer.request.get(root, '/new-platform/').expect(200, { content: 'ok' }); - }); - - it('supports redirecting to configured url', async () => { - const redirectTo = '/redirect-url'; - const { http } = await root.setup(); - http.registerOnPostAuth(async (req, t) => t.redirected(redirectTo)); - await root.start(); - - const response = await kbnTestServer.request.get(root, '/new-platform/').expect(302); - expect(response.header.location).toBe(redirectTo); - }); - - it('fails a request with configured error and status code', async () => { - const { http } = await root.setup(); - http.registerOnPostAuth(async (req, t) => - t.rejected(new Error('unexpected error'), { statusCode: 400 }) - ); - await root.start(); - - await kbnTestServer.request - .get(root, '/new-platform/') - .expect(400, { statusCode: 400, error: 'Bad Request', message: 'unexpected error' }); - }); - - it(`doesn't expose internal error details`, async () => { - const { http } = await root.setup(); - http.registerOnPostAuth(async (req, t) => { - throw new Error('sensitive info'); - }); - await root.start(); - - await kbnTestServer.request.get(root, '/new-platform/').expect({ - statusCode: 500, - error: 'Internal Server Error', - message: 'An internal server error occurred', - }); - }); - - it(`doesn't share request object between interceptors`, async () => { - const { http } = await root.setup(); - http.registerOnPostAuth(async (req, t) => { - // @ts-ignore. don't complain customField is not defined on Request type - req.customField = { value: 42 }; - return t.next(); - }); - http.registerOnPostAuth((req, t) => { - // @ts-ignore don't complain customField is not defined on Request type - if (typeof req.customField !== 'undefined') { - throw new Error('Request object was mutated'); - } - return t.next(); - }); - const router = new Router('/new-platform'); - router.get({ path: '/', validate: false }, async (req, res) => - // @ts-ignore. don't complain customField is not defined on Request type - res.ok({ customField: String(req.customField) }) - ); - http.registerRouter(router); - await root.start(); - - await kbnTestServer.request - .get(root, '/new-platform/') - .expect(200, { customField: 'undefined' }); - }); - }); - - describe('#registerOnPostAuth() toolkit', () => { - let root: ReturnType; - beforeEach(async () => { - root = kbnTestServer.createRoot(); - }, 30000); - - afterEach(async () => await root.shutdown()); - it('supports Url change on the flight', async () => { - const { http } = await root.setup(); - http.registerOnPreAuth((req, t) => { - return t.redirected('/new-platform/new-url', { forward: true }); - }); - - const router = new Router('/new-platform'); - router.get({ path: '/new-url', validate: false }, async (req, res) => - res.ok({ key: 'new-url-reached' }) - ); - http.registerRouter(router); - - await root.start(); - - await kbnTestServer.request.get(root, '/').expect(200, { key: 'new-url-reached' }); - }); - - it('url re-write works for legacy server as well', async () => { - const { http } = await root.setup(); - const newUrl = '/new-url'; - http.registerOnPreAuth((req, t) => { - return t.redirected(newUrl, { forward: true }); - }); - - await root.start(); - const kbnServer = kbnTestServer.getKbnServer(root); - kbnServer.server.route({ - method: 'GET', - path: newUrl, - handler: () => 'ok-from-legacy', - }); - - await kbnTestServer.request.get(root, '/').expect(200, 'ok-from-legacy'); - }); - }); - - describe('#basePath()', () => { - let root: ReturnType; - beforeEach(async () => { - root = kbnTestServer.createRoot(); - }, 30000); - - afterEach(async () => await root.shutdown()); - it('basePath information for an incoming request is available in legacy server', async () => { - const reqBasePath = '/requests-specific-base-path'; - const { http } = await root.setup(); - http.registerOnPreAuth((req, t) => { - http.basePath.set(req, reqBasePath); - return t.next(); - }); - - await root.start(); - - const legacyUrl = '/legacy'; - const kbnServer = kbnTestServer.getKbnServer(root); - kbnServer.server.route({ - method: 'GET', - path: legacyUrl, - handler: kbnServer.newPlatform.setup.core.http.basePath.get, - }); - - await kbnTestServer.request.get(root, legacyUrl).expect(200, reqBasePath); - }); - }); - }); -}); diff --git a/src/core/server/http/integration_tests/lifecycle.test.ts b/src/core/server/http/integration_tests/lifecycle.test.ts new file mode 100644 index 00000000000000..0571b18d206e56 --- /dev/null +++ b/src/core/server/http/integration_tests/lifecycle.test.ts @@ -0,0 +1,921 @@ +/* + * 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 supertest from 'supertest'; +import { ByteSizeValue } from '@kbn/config-schema'; +import request from 'request'; + +import { HttpConfig, Router } from '..'; +import { ensureRawRequest } from '../router'; +import { HttpServer } from '../http_server'; + +import { LoggerFactory } from '../../logging'; +import { loggingServiceMock } from '../../logging/logging_service.mock'; + +let server: HttpServer; +let logger: LoggerFactory; + +const config = { + host: '127.0.0.1', + maxPayload: new ByteSizeValue(1024), + port: 10001, + ssl: { enabled: false }, +} as HttpConfig; + +interface User { + id: string; + roles?: string[]; +} + +interface StorageData { + value: User; + expires: number; +} + +beforeEach(() => { + logger = loggingServiceMock.create(); + server = new HttpServer(logger, 'tests'); +}); + +afterEach(async () => { + await server.stop(); +}); + +describe('OnPreAuth', () => { + it('supports registering request inceptors', async () => { + const router = new Router('/'); + + router.get({ path: '/', validate: false }, (req, res) => res.ok('ok')); + + const { registerRouter, registerOnPreAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + const callingOrder: string[] = []; + registerOnPreAuth((req, res, t) => { + callingOrder.push('first'); + return t.next(); + }); + + registerOnPreAuth((req, res, t) => { + callingOrder.push('second'); + return t.next(); + }); + await server.start(); + + await supertest(innerServer.listener) + .get('/') + .expect(200, 'ok'); + + expect(callingOrder).toEqual(['first', 'second']); + }); + + it('supports request forwarding to specified url', async () => { + const router = new Router('/'); + + router.get({ path: '/initial', validate: false }, (req, res) => res.ok('initial')); + router.get({ path: '/redirectUrl', validate: false }, (req, res) => res.ok('redirected')); + + const { registerRouter, registerOnPreAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + let urlBeforeForwarding; + registerOnPreAuth((req, res, t) => { + urlBeforeForwarding = ensureRawRequest(req).raw.req.url; + return t.rewriteUrl('/redirectUrl'); + }); + + let urlAfterForwarding; + registerOnPreAuth((req, res, t) => { + // used by legacy platform + urlAfterForwarding = ensureRawRequest(req).raw.req.url; + return t.next(); + }); + + await server.start(); + + await supertest(innerServer.listener) + .get('/initial') + .expect(200, 'redirected'); + + expect(urlBeforeForwarding).toBe('/initial'); + expect(urlAfterForwarding).toBe('/redirectUrl'); + }); + + it('supports redirection from the interceptor', async () => { + const router = new Router('/'); + const redirectUrl = '/redirectUrl'; + router.get({ path: '/initial', validate: false }, (req, res) => res.ok('initial')); + + const { registerRouter, registerOnPreAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPreAuth((req, res, t) => + res.redirected(undefined, { + headers: { + location: redirectUrl, + }, + }) + ); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/initial') + .expect(302); + + expect(result.header.location).toBe(redirectUrl); + }); + + it('supports rejecting request and adjusting response headers', async () => { + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok(undefined)); + + const { registerRouter, registerOnPreAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPreAuth((req, res, t) => + res.unauthorized('not found error', { + headers: { + 'www-authenticate': 'challenge', + }, + }) + ); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(401); + + expect(result.header['www-authenticate']).toBe('challenge'); + }); + + it("doesn't expose error details if interceptor throws", async () => { + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok(undefined)); + + const { registerRouter, registerOnPreAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPreAuth((req, res, t) => { + throw new Error('reason'); + }); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body.error).toBe('An internal server error occurred.'); + expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` + Array [ + Array [ + [Error: reason], + ], + ] + `); + }); + + it('returns internal error if interceptor returns unexpected result', async () => { + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok('ok')); + + const { registerRouter, registerOnPreAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPreAuth((req, res, t) => ({} as any)); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body.error).toBe('An internal server error occurred.'); + expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` + Array [ + Array [ + [Error: Unexpected result from OnPreAuth. Expected OnPreAuthResult or KibanaResponse, but given: [object Object].], + ], + ] + `); + }); + it(`doesn't share request object between interceptors`, async () => { + const { registerRouter, registerOnPreAuth, server: innerServer } = await server.setup(config); + registerOnPreAuth((req, res, t) => { + // @ts-ignore. don't complain customField is not defined on Request type + req.customField = { value: 42 }; + return t.next(); + }); + registerOnPreAuth((req, res, t) => { + // @ts-ignore don't complain customField is not defined on Request type + if (typeof req.customField !== 'undefined') { + throw new Error('Request object was mutated'); + } + return t.next(); + }); + const router = new Router('/'); + router.get({ path: '/', validate: false }, async (req, res) => + // @ts-ignore. don't complain customField is not defined on Request type + res.ok({ customField: String(req.customField) }) + ); + registerRouter(router); + await server.start(); + + await supertest(innerServer.listener) + .get('/') + .expect(200, { customField: 'undefined' }); + }); +}); + +describe('OnPostAuth', () => { + it('supports registering request inceptors', async () => { + const router = new Router('/'); + + router.get({ path: '/', validate: false }, (req, res) => res.ok('ok')); + + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + const callingOrder: string[] = []; + registerOnPostAuth((req, res, t) => { + callingOrder.push('first'); + return t.next(); + }); + + registerOnPostAuth((req, res, t) => { + callingOrder.push('second'); + return t.next(); + }); + await server.start(); + + await supertest(innerServer.listener) + .get('/') + .expect(200, 'ok'); + + expect(callingOrder).toEqual(['first', 'second']); + }); + + it('supports redirection from the interceptor', async () => { + const router = new Router('/'); + const redirectUrl = '/redirectUrl'; + router.get({ path: '/initial', validate: false }, (req, res) => res.ok('initial')); + + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPostAuth((req, res, t) => + res.redirected(undefined, { + headers: { + location: redirectUrl, + }, + }) + ); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/initial') + .expect(302); + + expect(result.header.location).toBe(redirectUrl); + }); + + it('supports rejecting request and adjusting response headers', async () => { + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok(undefined)); + + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPostAuth((req, res, t) => + res.unauthorized('not found error', { + headers: { + 'www-authenticate': 'challenge', + }, + }) + ); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(401); + + expect(result.header['www-authenticate']).toBe('challenge'); + }); + + it("doesn't expose error details if interceptor throws", async () => { + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok(undefined)); + + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPostAuth((req, res, t) => { + throw new Error('reason'); + }); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body.error).toBe('An internal server error occurred.'); + expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` + Array [ + Array [ + [Error: reason], + ], + ] + `); + }); + + it('returns internal error if interceptor returns unexpected result', async () => { + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok('ok')); + + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPostAuth((req, res, t) => ({} as any)); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body.error).toBe('An internal server error occurred.'); + expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` + Array [ + Array [ + [Error: Unexpected result from OnPostAuth. Expected OnPostAuthResult or KibanaResponse, but given: [object Object].], + ], + ] + `); + }); + it(`doesn't share request object between interceptors`, async () => { + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerOnPostAuth((req, res, t) => { + // @ts-ignore. don't complain customField is not defined on Request type + req.customField = { value: 42 }; + return t.next(); + }); + registerOnPostAuth((req, res, t) => { + // @ts-ignore don't complain customField is not defined on Request type + if (typeof req.customField !== 'undefined') { + throw new Error('Request object was mutated'); + } + return t.next(); + }); + const router = new Router('/'); + router.get({ path: '/', validate: false }, async (req, res) => + // @ts-ignore. don't complain customField is not defined on Request type + res.ok({ customField: String(req.customField) }) + ); + registerRouter(router); + await server.start(); + + await supertest(innerServer.listener) + .get('/') + .expect(200, { customField: 'undefined' }); + }); +}); + +describe('Auth', () => { + const cookieOptions = { + name: 'sid', + encryptionKey: 'something_at_least_32_characters', + validate: () => true, + isSecure: false, + }; + + it('registers auth request interceptor only once', async () => { + const { registerAuth } = await server.setup(config); + const doRegister = () => registerAuth(() => null as any); + + doRegister(); + expect(doRegister).toThrowError('Auth interceptor was already registered'); + }); + + it('may grant access to a resource', async () => { + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + const router = new Router(''); + router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); + registerRouter(router); + + registerAuth((req, res, t) => t.authenticated()); + await server.start(); + + await supertest(innerServer.listener) + .get('/') + .expect(200, { content: 'ok' }); + }); + + it('enables auth for a route by default if registerAuth has been called', async () => { + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router(''); + router.get({ path: '/', validate: false }, (req, res) => + res.ok({ authRequired: req.route.options.authRequired }) + ); + registerRouter(router); + + const authenticate = jest.fn().mockImplementation((req, res, t) => t.authenticated()); + registerAuth(authenticate); + + await server.start(); + await supertest(innerServer.listener) + .get('/') + .expect(200, { authRequired: true }); + + expect(authenticate).toHaveBeenCalledTimes(1); + }); + + test('supports disabling auth for a route explicitly', async () => { + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router(''); + router.get({ path: '/', validate: false, options: { authRequired: false } }, (req, res) => + res.ok({ authRequired: req.route.options.authRequired }) + ); + registerRouter(router); + const authenticate = jest.fn(); + registerAuth(authenticate); + + await server.start(); + await supertest(innerServer.listener) + .get('/') + .expect(200, { authRequired: false }); + + expect(authenticate).toHaveBeenCalledTimes(0); + }); + + test('supports enabling auth for a route explicitly', async () => { + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router(''); + router.get({ path: '/', validate: false, options: { authRequired: true } }, (req, res) => + res.ok({ authRequired: req.route.options.authRequired }) + ); + registerRouter(router); + const authenticate = jest.fn().mockImplementation((req, res, t) => t.authenticated({})); + await registerAuth(authenticate); + + await server.start(); + await supertest(innerServer.listener) + .get('/') + .expect(200, { authRequired: true }); + + expect(authenticate).toHaveBeenCalledTimes(1); + }); + + it('supports rejecting a request from an unauthenticated user', async () => { + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + const router = new Router(''); + router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); + registerRouter(router); + + registerAuth((req, res) => res.unauthorized()); + await server.start(); + + await supertest(innerServer.listener) + .get('/') + .expect(401); + }); + + it('supports redirecting', async () => { + const redirectTo = '/redirect-url'; + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + const router = new Router(''); + router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); + registerRouter(router); + + registerAuth((req, res) => + res.redirected(undefined, { + headers: { + location: redirectTo, + }, + }) + ); + await server.start(); + + const response = await supertest(innerServer.listener) + .get('/') + .expect(302); + expect(response.header.location).toBe(redirectTo); + }); + + it(`doesn't expose internal error details`, async () => { + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + const router = new Router(''); + router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); + registerRouter(router); + + registerAuth((req, t) => { + throw new Error('reason'); + }); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body.error).toBe('An internal server error occurred.'); + expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` + Array [ + Array [ + [Error: reason], + ], + ] + `); + }); + + it('allows manipulating cookies via cookie session storage', async () => { + const router = new Router(''); + router.get({ path: '/', validate: false }, async (req, res) => res.ok({ content: 'ok' })); + + const { + createCookieSessionStorageFactory, + registerAuth, + registerRouter, + server: innerServer, + } = await server.setup(config); + const sessionStorageFactory = await createCookieSessionStorageFactory( + cookieOptions + ); + registerAuth((req, res, toolkit) => { + const user = { id: '42' }; + const sessionStorage = sessionStorageFactory.asScoped(req); + sessionStorage.set({ value: user, expires: Date.now() + 1000 }); + return toolkit.authenticated({ state: user }); + }); + registerRouter(router); + await server.start(); + + const response = await supertest(innerServer.listener) + .get('/') + .expect(200, { content: 'ok' }); + + expect(response.header['set-cookie']).toBeDefined(); + const cookies = response.header['set-cookie']; + expect(cookies).toHaveLength(1); + + const sessionCookie = request.cookie(cookies[0]); + if (!sessionCookie) { + throw new Error('session cookie expected to be defined'); + } + expect(sessionCookie).toBeDefined(); + expect(sessionCookie.key).toBe('sid'); + expect(sessionCookie.value).toBeDefined(); + expect(sessionCookie.path).toBe('/'); + expect(sessionCookie.httpOnly).toBe(true); + }); + + it('allows manipulating cookies from route handler', async () => { + const { + createCookieSessionStorageFactory, + registerAuth, + registerRouter, + server: innerServer, + } = await server.setup(config); + const sessionStorageFactory = await createCookieSessionStorageFactory( + cookieOptions + ); + registerAuth((req, res, toolkit) => { + const user = { id: '42' }; + const sessionStorage = sessionStorageFactory.asScoped(req); + sessionStorage.set({ value: user, expires: Date.now() + 1000 }); + return toolkit.authenticated(); + }); + + const router = new Router(''); + router.get({ path: '/', validate: false }, (req, res) => res.ok({ content: 'ok' })); + router.get({ path: '/with-cookie', validate: false }, (req, res) => { + const sessionStorage = sessionStorageFactory.asScoped(req); + sessionStorage.clear(); + return res.ok({ content: 'ok' }); + }); + registerRouter(router); + + await server.start(); + + const responseToSetCookie = await supertest(innerServer.listener) + .get('/') + .expect(200); + + expect(responseToSetCookie.header['set-cookie']).toBeDefined(); + + const responseToResetCookie = await supertest(innerServer.listener) + .get('/with-cookie') + .expect(200); + + expect(responseToResetCookie.header['set-cookie']).toEqual([ + 'sid=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; Path=/', + ]); + }); + + it.skip('is the only place with access to the authorization header', async () => { + const token = 'Basic: user:password'; + const { + registerAuth, + registerOnPreAuth, + registerOnPostAuth, + registerRouter, + server: innerServer, + } = await server.setup(config); + + let fromRegisterOnPreAuth; + await registerOnPreAuth((req, res, toolkit) => { + fromRegisterOnPreAuth = req.headers.authorization; + return toolkit.next(); + }); + + let fromRegisterAuth; + registerAuth((req, res, toolkit) => { + fromRegisterAuth = req.headers.authorization; + return toolkit.authenticated(); + }); + + let fromRegisterOnPostAuth; + await registerOnPostAuth((req, res, toolkit) => { + fromRegisterOnPostAuth = req.headers.authorization; + return toolkit.next(); + }); + + let fromRouteHandler; + const router = new Router(''); + router.get({ path: '/', validate: false }, (req, res) => { + fromRouteHandler = req.headers.authorization; + return res.ok({ content: 'ok' }); + }); + registerRouter(router); + + await server.start(); + + await supertest(innerServer.listener) + .get('/') + .set('Authorization', token) + .expect(200); + + expect(fromRegisterOnPreAuth).toEqual({}); + expect(fromRegisterAuth).toEqual({ authorization: token }); + expect(fromRegisterOnPostAuth).toEqual({}); + expect(fromRouteHandler).toEqual({}); + }); + + it('attach security header to a successful response', async () => { + const authResponseHeader = { + 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', + }; + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + + registerAuth((req, res, toolkit) => { + return toolkit.authenticated({ responseHeaders: authResponseHeader }); + }); + + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok({ header: 'ok' })); + registerRouter(router); + + await server.start(); + + const response = await supertest(innerServer.listener) + .get('/') + .expect(200); + + expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']); + }); + + it('attach security header to an error response', async () => { + const authResponseHeader = { + 'www-authenticate': 'Negotiate ade0234568a4209af8bc0280289eca', + }; + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + + registerAuth((req, res, toolkit) => { + return toolkit.authenticated({ responseHeaders: authResponseHeader }); + }); + + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.badRequest(new Error('reason'))); + registerRouter(router); + + await server.start(); + + const response = await supertest(innerServer.listener) + .get('/') + .expect(400); + + expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']); + }); + + it('logs warning if Auth Security Header rewrites response header for success response', async () => { + const authResponseHeader = { + 'www-authenticate': 'from auth interceptor', + }; + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + + registerAuth((req, res, toolkit) => { + return toolkit.authenticated({ responseHeaders: authResponseHeader }); + }); + + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => + res.ok( + {}, + { + headers: { + 'www-authenticate': 'from handler', + }, + } + ) + ); + registerRouter(router); + + await server.start(); + + const response = await supertest(innerServer.listener) + .get('/') + .expect(200); + + expect(response.header['www-authenticate']).toBe('from auth interceptor'); + expect(loggingServiceMock.collect(logger).warn).toMatchInlineSnapshot(` + Array [ + Array [ + "Server rewrites a response header [www-authenticate].", + ], + ] + `); + }); + + it('logs warning if Auth Security Header rewrites response header for error response', async () => { + const authResponseHeader = { + 'www-authenticate': 'from auth interceptor', + }; + const { registerAuth, registerRouter, server: innerServer } = await server.setup(config); + + registerAuth((req, res, toolkit) => { + return toolkit.authenticated({ responseHeaders: authResponseHeader }); + }); + + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => + res.badRequest('reason', { + headers: { + 'www-authenticate': 'from handler', + }, + }) + ); + registerRouter(router); + + await server.start(); + + const response = await supertest(innerServer.listener) + .get('/') + .expect(400); + + expect(response.header['www-authenticate']).toBe('from auth interceptor'); + expect(loggingServiceMock.collect(logger).warn).toMatchInlineSnapshot(` + Array [ + Array [ + "Server rewrites a response header [www-authenticate].", + ], + ] + `); + }); + + it('supports redirection from the interceptor', async () => { + const router = new Router('/'); + const redirectUrl = '/redirectUrl'; + router.get({ path: '/initial', validate: false }, (req, res) => res.ok('initial')); + + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPostAuth((req, res, t) => + res.redirected(undefined, { + headers: { + location: redirectUrl, + }, + }) + ); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/initial') + .expect(302); + + expect(result.header.location).toBe(redirectUrl); + }); + + it('supports rejecting request and adjusting response headers', async () => { + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok(undefined)); + + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPostAuth((req, res, t) => + res.unauthorized('not found error', { + headers: { + 'www-authenticate': 'challenge', + }, + }) + ); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(401); + + expect(result.header['www-authenticate']).toBe('challenge'); + }); + + it("doesn't expose error details if interceptor throws", async () => { + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok(undefined)); + + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPostAuth((req, res, t) => { + throw new Error('reason'); + }); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body.error).toBe('An internal server error occurred.'); + expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` + Array [ + Array [ + [Error: reason], + ], + ] + `); + }); + + it('returns internal error if interceptor returns unexpected result', async () => { + const router = new Router('/'); + router.get({ path: '/', validate: false }, (req, res) => res.ok('ok')); + + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerRouter(router); + + registerOnPostAuth((req, res, t) => ({} as any)); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body.error).toBe('An internal server error occurred.'); + expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` + Array [ + Array [ + [Error: Unexpected result from OnPostAuth. Expected OnPostAuthResult or KibanaResponse, but given: [object Object].], + ], + ] + `); + }); + it(`doesn't share request object between interceptors`, async () => { + const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); + registerOnPostAuth((req, res, t) => { + // @ts-ignore. don't complain customField is not defined on Request type + req.customField = { value: 42 }; + return t.next(); + }); + registerOnPostAuth((req, res, t) => { + // @ts-ignore don't complain customField is not defined on Request type + if (typeof req.customField !== 'undefined') { + throw new Error('Request object was mutated'); + } + return t.next(); + }); + const router = new Router('/'); + router.get({ path: '/', validate: false }, async (req, res) => + // @ts-ignore. don't complain customField is not defined on Request type + res.ok({ customField: String(req.customField) }) + ); + registerRouter(router); + await server.start(); + + await supertest(innerServer.listener) + .get('/') + .expect(200, { customField: 'undefined' }); + }); +}); diff --git a/src/core/server/http/lifecycle/auth.test.ts b/src/core/server/http/lifecycle/auth.test.ts deleted file mode 100644 index 36aef7fc73bac2..00000000000000 --- a/src/core/server/http/lifecycle/auth.test.ts +++ /dev/null @@ -1,110 +0,0 @@ -/* - * 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 Boom from 'boom'; -import { adoptToHapiAuthFormat } from './auth'; -import { httpServerMock } from '../http_server.mocks'; - -describe('adoptToHapiAuthFormat', () => { - it('allows to associate arbitrary data with an incoming request', async () => { - const authData = { - state: { foo: 'bar' }, - requestHeaders: { authorization: 'baz' }, - }; - const authenticatedMock = jest.fn(); - const onSuccessMock = jest.fn(); - const onAuth = adoptToHapiAuthFormat((req, t) => t.authenticated(authData), onSuccessMock); - await onAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit({ - authenticated: authenticatedMock, - }) - ); - - expect(authenticatedMock).toBeCalledTimes(1); - expect(authenticatedMock).toBeCalledWith({ credentials: authData.state }); - - expect(onSuccessMock).toBeCalledTimes(1); - const [[, onSuccessData]] = onSuccessMock.mock.calls; - expect(onSuccessData).toEqual(authData); - }); - - it('Should allow redirecting to specified url', async () => { - const redirectUrl = '/docs'; - const onSuccessMock = jest.fn(); - const onAuth = adoptToHapiAuthFormat((req, t) => t.redirected(redirectUrl), onSuccessMock); - const takeoverSymbol = {}; - const redirectMock = jest.fn(() => ({ takeover: () => takeoverSymbol })); - const result = await onAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit({ - redirect: redirectMock, - }) - ); - - expect(redirectMock).toBeCalledWith(redirectUrl); - expect(result).toBe(takeoverSymbol); - expect(onSuccessMock).not.toHaveBeenCalled(); - }); - - it('Should allow to specify statusCode and message for Boom error', async () => { - const onSuccessMock = jest.fn(); - const onAuth = adoptToHapiAuthFormat( - (req, t) => t.rejected(new Error('not found'), { statusCode: 404 }), - onSuccessMock - ); - const result = (await onAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit() - )) as Boom; - - expect(result).toBeInstanceOf(Boom); - expect(result.message).toBe('not found'); - expect(result.output.statusCode).toBe(404); - expect(onSuccessMock).not.toHaveBeenCalled(); - }); - - it('Should return Boom.internal error error if interceptor throws', async () => { - const onAuth = adoptToHapiAuthFormat((req, t) => { - throw new Error('unknown error'); - }); - const result = (await onAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit() - )) as Boom; - - expect(result).toBeInstanceOf(Boom); - expect(result.message).toBe('unknown error'); - expect(result.output.statusCode).toBe(500); - }); - - it('Should return Boom.internal error if interceptor returns unexpected result', async () => { - const onAuth = adoptToHapiAuthFormat(async (req, t) => undefined as any); - const result = (await onAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit() - )) as Boom; - - expect(result).toBeInstanceOf(Boom); - expect(result.message).toBe( - 'Unexpected result from Authenticate. Expected AuthResult, but given: undefined.' - ); - expect(result.output.statusCode).toBe(500); - }); -}); diff --git a/src/core/server/http/lifecycle/auth.ts b/src/core/server/http/lifecycle/auth.ts index e95b7af6e44f6e..6631efb0439de1 100644 --- a/src/core/server/http/lifecycle/auth.ts +++ b/src/core/server/http/lifecycle/auth.ts @@ -16,33 +16,25 @@ * specific language governing permissions and limitations * under the License. */ -import Boom from 'boom'; -import { noop } from 'lodash'; import { Lifecycle, Request, ResponseToolkit } from 'hapi'; -import { KibanaRequest } from '../router'; +import { Logger } from '../../logging'; +import { + HapiResponseAdapter, + KibanaRequest, + KibanaResponse, + responseFactory, + ResponseFactory, +} from '../router'; enum ResultType { authenticated = 'authenticated', - redirected = 'redirected', - rejected = 'rejected', } interface Authenticated extends AuthResultParams { type: ResultType.authenticated; } -interface Redirected { - type: ResultType.redirected; - url: string; -} - -interface Rejected { - type: ResultType.rejected; - error: Error; - statusCode?: number; -} - -type AuthResult = Authenticated | Rejected | Redirected; +type AuthResult = Authenticated; const authResult = { authenticated(data: Partial = {}): AuthResult { @@ -53,28 +45,8 @@ const authResult = { responseHeaders: data.responseHeaders, }; }, - redirected(url: string): AuthResult { - return { type: ResultType.redirected, url }; - }, - rejected(error: Error, options: { statusCode?: number } = {}): AuthResult { - return { type: ResultType.rejected, error, statusCode: options.statusCode }; - }, - isValid(candidate: any): candidate is AuthResult { - return ( - candidate && - (candidate.type === ResultType.authenticated || - candidate.type === ResultType.rejected || - candidate.type === ResultType.redirected) - ); - }, isAuthenticated(result: AuthResult): result is Authenticated { - return result.type === ResultType.authenticated; - }, - isRedirected(result: AuthResult): result is Redirected { - return result.type === ResultType.redirected; - }, - isRejected(result: AuthResult): result is Rejected { - return result.type === ResultType.rejected; + return result && result.type === ResultType.authenticated; }, }; @@ -113,55 +85,53 @@ export interface AuthResultParams { export interface AuthToolkit { /** Authentication is successful with given credentials, allow request to pass through */ authenticated: (data?: AuthResultParams) => AuthResult; - /** Authentication requires to interrupt request handling and redirect to a configured url */ - redirected: (url: string) => AuthResult; - /** Authentication is unsuccessful, fail the request with specified error. */ - rejected: (error: Error, options?: { statusCode?: number }) => AuthResult; } const toolkit: AuthToolkit = { authenticated: authResult.authenticated, - redirected: authResult.redirected, - rejected: authResult.rejected, }; /** @public */ export type AuthenticationHandler = ( request: KibanaRequest, + response: ResponseFactory, t: AuthToolkit -) => AuthResult | Promise; +) => AuthResult | KibanaResponse | Promise>; /** @public */ export function adoptToHapiAuthFormat( fn: AuthenticationHandler, - onSuccess: (req: Request, data: AuthResultParams) => void = noop + log: Logger, + onSuccess: (req: Request, data: AuthResultParams) => void = () => void 0 ) { return async function interceptAuth( - req: Request, - h: ResponseToolkit + request: Request, + responseToolkit: ResponseToolkit ): Promise { + const hapiResponseAdapter = new HapiResponseAdapter(responseToolkit); try { - const result = await fn(KibanaRequest.from(req, undefined, false), toolkit); - if (!authResult.isValid(result)) { - throw new Error( - `Unexpected result from Authenticate. Expected AuthResult, but given: ${result}.` - ); + const result = await fn( + KibanaRequest.from(request, undefined, false), + responseFactory, + toolkit + ); + if (result instanceof KibanaResponse) { + return hapiResponseAdapter.handle(result); } if (authResult.isAuthenticated(result)) { - onSuccess(req, { + onSuccess(request, { state: result.state, requestHeaders: result.requestHeaders, responseHeaders: result.responseHeaders, }); - return h.authenticated({ credentials: result.state || {} }); - } - if (authResult.isRedirected(result)) { - return h.redirect(result.url).takeover(); + return responseToolkit.authenticated({ credentials: result.state || {} }); } - const { error, statusCode } = result; - return Boom.boomify(error, { statusCode }); + throw new Error( + `Unexpected result from Authenticate. Expected AuthResult or KibanaResponse, but given: ${result}.` + ); } catch (error) { - return Boom.internal(error.message, { statusCode: 500 }); + log.error(error); + return hapiResponseAdapter.toInternalError(); } }; } diff --git a/src/core/server/http/lifecycle/on_post_auth.test.ts b/src/core/server/http/lifecycle/on_post_auth.test.ts deleted file mode 100644 index 88e8fc91149b8d..00000000000000 --- a/src/core/server/http/lifecycle/on_post_auth.test.ts +++ /dev/null @@ -1,95 +0,0 @@ -/* - * 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 Boom from 'boom'; -import { adoptToHapiOnPostAuthFormat } from './on_post_auth'; -import { httpServerMock } from '../http_server.mocks'; - -describe('adoptToHapiOnPostAuthFormat', () => { - it('Should allow passing request to the next handler', async () => { - const continueSymbol = Symbol(); - const onPostAuth = adoptToHapiOnPostAuthFormat((req, t) => t.next()); - const result = await onPostAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit({ - ['continue']: continueSymbol, - }) - ); - - expect(result).toBe(continueSymbol); - }); - - it('Should support redirecting to specified url', async () => { - const redirectUrl = '/docs'; - const onPostAuth = adoptToHapiOnPostAuthFormat((req, t) => t.redirected(redirectUrl)); - const takeoverSymbol = {}; - const redirectMock = jest.fn(() => ({ takeover: () => takeoverSymbol })); - const result = await onPostAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit({ - redirect: redirectMock, - }) - ); - - expect(redirectMock).toBeCalledWith(redirectUrl); - expect(result).toBe(takeoverSymbol); - }); - - it('Should support specifying statusCode and message for Boom error', async () => { - const onPostAuth = adoptToHapiOnPostAuthFormat((req, t) => { - return t.rejected(new Error('unexpected result'), { statusCode: 501 }); - }); - const result = (await onPostAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit() - )) as Boom; - - expect(result).toBeInstanceOf(Boom); - expect(result.message).toBe('unexpected result'); - expect(result.output.statusCode).toBe(501); - }); - - it('Should return Boom.internal error if interceptor throws', async () => { - const onPostAuth = adoptToHapiOnPostAuthFormat((req, t) => { - throw new Error('unknown error'); - }); - const result = (await onPostAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit() - )) as Boom; - - expect(result).toBeInstanceOf(Boom); - expect(result.message).toBe('unknown error'); - expect(result.output.statusCode).toBe(500); - }); - - it('Should return Boom.internal error if interceptor returns unexpected result', async () => { - const onPostAuth = adoptToHapiOnPostAuthFormat((req, toolkit) => undefined as any); - const result = (await onPostAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit() - )) as Boom; - - expect(result).toBeInstanceOf(Boom); - expect(result.message).toMatchInlineSnapshot( - `"Unexpected result from OnPostAuth. Expected OnPostAuthResult, but given: undefined."` - ); - expect(result.output.statusCode).toBe(500); - }); -}); diff --git a/src/core/server/http/lifecycle/on_post_auth.ts b/src/core/server/http/lifecycle/on_post_auth.ts index 64d27bbe7c5c53..906be3515bd5b8 100644 --- a/src/core/server/http/lifecycle/on_post_auth.ts +++ b/src/core/server/http/lifecycle/on_post_auth.ts @@ -17,59 +17,32 @@ * under the License. */ -import Boom from 'boom'; -import { Lifecycle, Request, ResponseToolkit } from 'hapi'; -import { KibanaRequest } from '../router'; +import { Lifecycle, Request, ResponseToolkit as HapiResponseToolkit } from 'hapi'; +import { Logger } from '../../logging'; +import { + HapiResponseAdapter, + KibanaRequest, + KibanaResponse, + responseFactory, + ResponseFactory, +} from '../router'; enum ResultType { next = 'next', - redirected = 'redirected', - rejected = 'rejected', } interface Next { type: ResultType.next; } -interface Redirected { - type: ResultType.redirected; - url: string; -} - -interface Rejected { - type: ResultType.rejected; - error: Error; - statusCode?: number; -} - -type OnPostAuthResult = Next | Rejected | Redirected; +type OnPostAuthResult = Next; const postAuthResult = { next(): OnPostAuthResult { return { type: ResultType.next }; }, - redirected(url: string): OnPostAuthResult { - return { type: ResultType.redirected, url }; - }, - rejected(error: Error, options: { statusCode?: number } = {}): OnPostAuthResult { - return { type: ResultType.rejected, error, statusCode: options.statusCode }; - }, - isValid(candidate: any): candidate is OnPostAuthResult { - return ( - candidate && - (candidate.type === ResultType.next || - candidate.type === ResultType.rejected || - candidate.type === ResultType.redirected) - ); - }, isNext(result: OnPostAuthResult): result is Next { - return result.type === ResultType.next; - }, - isRedirected(result: OnPostAuthResult): result is Redirected { - return result.type === ResultType.redirected; - }, - isRejected(result: OnPostAuthResult): result is Rejected { - return result.type === ResultType.rejected; + return result && result.type === ResultType.next; }, }; @@ -80,51 +53,46 @@ const postAuthResult = { export interface OnPostAuthToolkit { /** To pass request to the next handler */ next: () => OnPostAuthResult; - /** To interrupt request handling and redirect to a configured url */ - redirected: (url: string) => OnPostAuthResult; - /** Fail the request with specified error. */ - rejected: (error: Error, options?: { statusCode?: number }) => OnPostAuthResult; } /** @public */ -export type OnPostAuthHandler = ( - request: KibanaRequest, - t: OnPostAuthToolkit -) => OnPostAuthResult | Promise; +export type OnPostAuthHandler = ( + request: KibanaRequest, + response: ResponseFactory, + toolkit: OnPostAuthToolkit +) => OnPostAuthResult | KibanaResponse | Promise>; const toolkit: OnPostAuthToolkit = { next: postAuthResult.next, - redirected: postAuthResult.redirected, - rejected: postAuthResult.rejected, }; + /** * @public * Adopt custom request interceptor to Hapi lifecycle system. * @param fn - an extension point allowing to perform custom logic for * incoming HTTP requests. */ -export function adoptToHapiOnPostAuthFormat(fn: OnPostAuthHandler) { +export function adoptToHapiOnPostAuthFormat(fn: OnPostAuthHandler, log: Logger) { return async function interceptRequest( request: Request, - h: ResponseToolkit + responseToolkit: HapiResponseToolkit ): Promise { + const hapiResponseAdapter = new HapiResponseAdapter(responseToolkit); try { - const result = await fn(KibanaRequest.from(request), toolkit); - if (!postAuthResult.isValid(result)) { - throw new Error( - `Unexpected result from OnPostAuth. Expected OnPostAuthResult, but given: ${result}.` - ); + const result = await fn(KibanaRequest.from(request), responseFactory, toolkit); + if (result instanceof KibanaResponse) { + return hapiResponseAdapter.handle(result); } if (postAuthResult.isNext(result)) { - return h.continue; - } - if (postAuthResult.isRedirected(result)) { - return h.redirect(result.url).takeover(); + return responseToolkit.continue; } - const { error, statusCode } = result; - return Boom.boomify(error, { statusCode }); + + throw new Error( + `Unexpected result from OnPostAuth. Expected OnPostAuthResult or KibanaResponse, but given: ${result}.` + ); } catch (error) { - return Boom.internal(error.message, { statusCode: 500 }); + log.error(error); + return hapiResponseAdapter.toInternalError(); } }; } diff --git a/src/core/server/http/lifecycle/on_pre_auth.test.ts b/src/core/server/http/lifecycle/on_pre_auth.test.ts deleted file mode 100644 index bae7c9f16eb139..00000000000000 --- a/src/core/server/http/lifecycle/on_pre_auth.test.ts +++ /dev/null @@ -1,115 +0,0 @@ -/* - * 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 Boom from 'boom'; -import { adoptToHapiOnPreAuthFormat } from './on_pre_auth'; -import { httpServerMock } from '../http_server.mocks'; - -describe('adoptToHapiOnPreAuthFormat', () => { - it('Should allow passing request to the next handler', async () => { - const continueSymbol = Symbol(); - const onPreAuth = adoptToHapiOnPreAuthFormat((req, t) => t.next()); - const result = await onPreAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit({ - ['continue']: continueSymbol, - }) - ); - - expect(result).toBe(continueSymbol); - }); - - it('Should support redirecting to specified url', async () => { - const redirectUrl = '/docs'; - const onPreAuth = adoptToHapiOnPreAuthFormat((req, t) => t.redirected(redirectUrl)); - const takeoverSymbol = {}; - const redirectMock = jest.fn(() => ({ takeover: () => takeoverSymbol })); - const result = await onPreAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit({ - redirect: redirectMock, - }) - ); - - expect(redirectMock).toBeCalledWith(redirectUrl); - expect(result).toBe(takeoverSymbol); - }); - - it('Should support request forwarding to specified url', async () => { - const redirectUrl = '/docs'; - const onPreAuth = adoptToHapiOnPreAuthFormat((req, t) => - t.redirected(redirectUrl, { forward: true }) - ); - const continueSymbol = Symbol(); - const setUrl = jest.fn(); - const mockedRequest = httpServerMock.createRawRequest({ setUrl }); - const result = await onPreAuth( - mockedRequest, - httpServerMock.createRawResponseToolkit({ - ['continue']: continueSymbol, - }) - ); - - expect(setUrl).toBeCalledWith(redirectUrl); - expect(mockedRequest.raw.req.url).toBe(redirectUrl); - expect(result).toBe(continueSymbol); - }); - - it('Should support specifying statusCode and message for Boom error', async () => { - const onPreAuth = adoptToHapiOnPreAuthFormat((req, t) => { - return t.rejected(new Error('unexpected result'), { statusCode: 501 }); - }); - const result = (await onPreAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit() - )) as Boom; - - expect(result).toBeInstanceOf(Boom); - expect(result.message).toBe('unexpected result'); - expect(result.output.statusCode).toBe(501); - }); - - it('Should return Boom.internal error if interceptor throws', async () => { - const onPreAuth = adoptToHapiOnPreAuthFormat((req, t) => { - throw new Error('unknown error'); - }); - const result = (await onPreAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit() - )) as Boom; - - expect(result).toBeInstanceOf(Boom); - expect(result.message).toBe('unknown error'); - expect(result.output.statusCode).toBe(500); - }); - - it('Should return Boom.internal error if interceptor returns unexpected result', async () => { - const onPreAuth = adoptToHapiOnPreAuthFormat((req, toolkit) => undefined as any); - const result = (await onPreAuth( - httpServerMock.createRawRequest(), - httpServerMock.createRawResponseToolkit() - )) as Boom; - - expect(result).toBeInstanceOf(Boom); - expect(result.message).toMatchInlineSnapshot( - `"Unexpected result from OnPreAuth. Expected OnPreAuthResult, but given: undefined."` - ); - expect(result.output.statusCode).toBe(500); - }); -}); diff --git a/src/core/server/http/lifecycle/on_pre_auth.ts b/src/core/server/http/lifecycle/on_pre_auth.ts index 13b267b1486669..55e83a3fbdee1a 100644 --- a/src/core/server/http/lifecycle/on_pre_auth.ts +++ b/src/core/server/http/lifecycle/on_pre_auth.ts @@ -17,60 +17,49 @@ * under the License. */ -import Boom from 'boom'; -import { Lifecycle, Request, ResponseToolkit } from 'hapi'; -import { KibanaRequest } from '../router'; +import { Lifecycle, Request, ResponseToolkit as HapiResponseToolkit } from 'hapi'; +import { Logger } from '../../logging'; +import { + HapiResponseAdapter, + KibanaRequest, + KibanaResponse, + responseFactory, + ResponseFactory, +} from '../router'; enum ResultType { next = 'next', - redirected = 'redirected', - rejected = 'rejected', + rewriteUrl = 'rewriteUrl', } interface Next { type: ResultType.next; } -interface Redirected { - type: ResultType.redirected; +interface RewriteUrl { + type: ResultType.rewriteUrl; url: string; - forward?: boolean; } -interface Rejected { - type: ResultType.rejected; - error: Error; - statusCode?: number; -} - -type OnPreAuthResult = Next | Rejected | Redirected; +type OnPreAuthResult = Next | RewriteUrl; const preAuthResult = { next(): OnPreAuthResult { return { type: ResultType.next }; }, - redirected(url: string, options: { forward?: boolean } = {}): OnPreAuthResult { - return { type: ResultType.redirected, url, forward: options.forward }; - }, - rejected(error: Error, options: { statusCode?: number } = {}): OnPreAuthResult { - return { type: ResultType.rejected, error, statusCode: options.statusCode }; + rewriteUrl(url: string): OnPreAuthResult { + return { type: ResultType.rewriteUrl, url }; }, isValid(candidate: any): candidate is OnPreAuthResult { return ( - candidate && - (candidate.type === ResultType.next || - candidate.type === ResultType.rejected || - candidate.type === ResultType.redirected) + candidate && (candidate.type === ResultType.next || candidate.type === ResultType.rewriteUrl) ); }, isNext(result: OnPreAuthResult): result is Next { return result.type === ResultType.next; }, - isRedirected(result: OnPreAuthResult): result is Redirected { - return result.type === ResultType.redirected; - }, - isRejected(result: OnPreAuthResult): result is Rejected { - return result.type === ResultType.rejected; + isRewriteUrl(result: OnPreAuthResult): result is RewriteUrl { + return result.type === ResultType.rewriteUrl; }, }; @@ -81,26 +70,20 @@ const preAuthResult = { export interface OnPreAuthToolkit { /** To pass request to the next handler */ next: () => OnPreAuthResult; - /** - * To interrupt request handling and redirect to a configured url. - * If "options.forwarded" = true, request will be forwarded to another url right on the server. - * */ - redirected: (url: string, options?: { forward: boolean }) => OnPreAuthResult; - /** Fail the request with specified error. */ - rejected: (error: Error, options?: { statusCode?: number }) => OnPreAuthResult; + rewriteUrl: (url: string) => OnPreAuthResult; } const toolkit: OnPreAuthToolkit = { next: preAuthResult.next, - redirected: preAuthResult.redirected, - rejected: preAuthResult.rejected, + rewriteUrl: preAuthResult.rewriteUrl, }; /** @public */ -export type OnPreAuthHandler = ( - request: KibanaRequest, - t: OnPreAuthToolkit -) => OnPreAuthResult | Promise; +export type OnPreAuthHandler = ( + request: KibanaRequest, + response: ResponseFactory, + toolkit: OnPreAuthToolkit +) => OnPreAuthResult | KibanaResponse | Promise>; /** * @public @@ -108,38 +91,38 @@ export type OnPreAuthHandler = ( * @param fn - an extension point allowing to perform custom logic for * incoming HTTP requests. */ -export function adoptToHapiOnPreAuthFormat(fn: OnPreAuthHandler) { +export function adoptToHapiOnPreAuthFormat(fn: OnPreAuthHandler, log: Logger) { return async function interceptPreAuthRequest( request: Request, - h: ResponseToolkit + responseToolkit: HapiResponseToolkit ): Promise { - try { - const result = await fn(KibanaRequest.from(request), toolkit); + const hapiResponseAdapter = new HapiResponseAdapter(responseToolkit); - if (!preAuthResult.isValid(result)) { - throw new Error( - `Unexpected result from OnPreAuth. Expected OnPreAuthResult, but given: ${result}.` - ); - } - if (preAuthResult.isNext(result)) { - return h.continue; + try { + const result = await fn(KibanaRequest.from(request), responseFactory, toolkit); + if (result instanceof KibanaResponse) { + return hapiResponseAdapter.handle(result); } - if (preAuthResult.isRedirected(result)) { - const { url, forward } = result; - if (forward) { + if (preAuthResult.isValid(result)) { + if (preAuthResult.isNext(result)) { + return responseToolkit.continue; + } + + if (preAuthResult.isRewriteUrl(result)) { + const { url } = result; request.setUrl(url); // We should update raw request as well since it can be proxied to the old platform request.raw.req.url = url; - return h.continue; + return responseToolkit.continue; } - return h.redirect(url).takeover(); } - - const { error, statusCode } = result; - return Boom.boomify(error, { statusCode }); + throw new Error( + `Unexpected result from OnPreAuth. Expected OnPreAuthResult or KibanaResponse, but given: ${result}.` + ); } catch (error) { - return Boom.internal(error.message, { statusCode: 500 }); + log.error(error); + return hapiResponseAdapter.toInternalError(); } }; } diff --git a/src/core/server/http/router/index.ts b/src/core/server/http/router/index.ts index eefa74cee0802e..fe1c66b66dcb81 100644 --- a/src/core/server/http/router/index.ts +++ b/src/core/server/http/router/index.ts @@ -21,4 +21,11 @@ export { Headers, filterHeaders, ResponseHeaders } from './headers'; export { Router } from './router'; export { KibanaRequest, KibanaRequestRoute, ensureRawRequest, isRealRequest } from './request'; export { RouteMethod, RouteConfigOptions } from './route'; -export { ResponseError, ResponseErrorMeta } from './response'; +export { + KibanaResponse, + ResponseError, + ResponseErrorMeta, + responseFactory, + ResponseFactory, +} from './response'; +export { HapiResponseAdapter } from './response_adapter'; diff --git a/src/core/server/http/router/response_adapter.ts b/src/core/server/http/router/response_adapter.ts index d8208b4a34058d..09e904d8a60628 100644 --- a/src/core/server/http/router/response_adapter.ts +++ b/src/core/server/http/router/response_adapter.ts @@ -18,13 +18,14 @@ */ import { ResponseObject as HapiResponseObject, ResponseToolkit as HapiResponseToolkit } from 'hapi'; import typeDetect from 'type-detect'; +import Boom from 'boom'; import { HttpResponsePayload, KibanaResponse, ResponseError } from './response'; function setHeaders(response: HapiResponseObject, headers: Record = {}) { Object.entries(headers).forEach(([header, value]) => { if (value !== undefined) { - // Hapi typings for header accept only string, although string[] is a valid value + // Hapi typings for header accept only strings, although string[] is a valid value response.header(header, value as any); } }); @@ -44,7 +45,15 @@ export class HapiResponseAdapter { } public toInternalError() { - return this.responseToolkit.response({ error: 'An internal server error occurred.' }).code(500); + const error = new Boom('', { + statusCode: 500, + }); + + error.output.payload = { + error: 'An internal server error occurred.', + } as any; // our error format is not compatible with boom + + return error; } public handle(kibanaResponse: KibanaResponse) { @@ -56,28 +65,30 @@ export class HapiResponseAdapter { ); } - const response = this.toHapiResponse(kibanaResponse); - setHeaders(response, kibanaResponse.options.headers); - return response; + return this.toHapiResponse(kibanaResponse); } private toHapiResponse(kibanaResponse: KibanaResponse) { + if (statusHelpers.isError(kibanaResponse.status)) { + return this.toError(kibanaResponse); + } if (statusHelpers.isSuccess(kibanaResponse.status)) { return this.toSuccess(kibanaResponse); } if (statusHelpers.isRedirect(kibanaResponse.status)) { return this.toRedirect(kibanaResponse); } - if (statusHelpers.isError(kibanaResponse.status)) { - return this.toError(kibanaResponse); - } throw new Error( `Unexpected Http status code. Expected from 100 to 599, but given: ${kibanaResponse.status}.` ); } private toSuccess(kibanaResponse: KibanaResponse) { - return this.responseToolkit.response(kibanaResponse.payload).code(kibanaResponse.status); + const response = this.responseToolkit + .response(kibanaResponse.payload) + .code(kibanaResponse.status); + setHeaders(response, kibanaResponse.options.headers); + return response; } private toRedirect(kibanaResponse: KibanaResponse) { @@ -86,20 +97,34 @@ export class HapiResponseAdapter { throw new Error("expected 'location' header to be set"); } - return this.responseToolkit + const response = this.responseToolkit .response(kibanaResponse.payload) .redirect(headers.location) - .code(kibanaResponse.status); + .code(kibanaResponse.status) + .takeover(); + + setHeaders(response, kibanaResponse.options.headers); + return response; } private toError(kibanaResponse: KibanaResponse) { const { payload } = kibanaResponse; - return this.responseToolkit - .response({ - error: getErrorMessage(payload), - meta: getErrorMeta(payload), - }) - .code(kibanaResponse.status); + const error = new Boom('', { + statusCode: kibanaResponse.status, + }); + + error.output.payload = { + error: getErrorMessage(payload), + meta: getErrorMeta(payload), + } as any; // our error format is not compatible with boom + + const headers = kibanaResponse.options.headers; + if (headers) { + // Hapi typings for header accept only strings, although string[] is a valid value + error.output.headers = headers as any; + } + + return error; } } diff --git a/src/core/server/http/router/router.ts b/src/core/server/http/router/router.ts index b801aa41754faa..46211eb43bc2e3 100644 --- a/src/core/server/http/router/router.ts +++ b/src/core/server/http/router/router.ts @@ -19,6 +19,7 @@ import { ObjectType, TypeOf, Type } from '@kbn/config-schema'; import { Request, ResponseObject, ResponseToolkit } from 'hapi'; +import Boom from 'boom'; import { Logger } from '../../logging'; import { KibanaRequest } from './request'; @@ -26,16 +27,23 @@ import { KibanaResponse, ResponseFactory, responseFactory } from './response'; import { RouteConfig, RouteConfigOptions, RouteMethod, RouteSchemas } from './route'; import { HapiResponseAdapter } from './response_adapter'; +/** + * @internal + */ export interface RouterRoute { method: RouteMethod; path: string; options: RouteConfigOptions; - handler: (req: Request, responseToolkit: ResponseToolkit, log: Logger) => Promise; + handler: ( + request: Request, + responseToolkit: ResponseToolkit, + log: Logger + ) => Promise>; } /** @public */ export class Router { - public routes: Array> = []; + private routes: Array> = []; constructor(readonly path: string) {} diff --git a/src/core/server/index.ts b/src/core/server/index.ts index 2bbcaa58f6af52..c7e933cf1e4923 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -72,6 +72,7 @@ export { OnPostAuthToolkit, ResponseError, ResponseErrorMeta, + ResponseFactory, Router, RouteMethod, RouteConfigOptions, From 6f6779ac01459f20a0064a810eb1b48624f98a15 Mon Sep 17 00:00:00 2001 From: restrry Date: Thu, 1 Aug 2019 13:43:15 +0200 Subject: [PATCH 02/14] adopt x-pack code to the changes --- .../on_post_auth_interceptor.test.ts | 13 ++-- .../on_request_interceptor.ts | 10 ++- .../server/authentication/index.test.ts | 77 +++++++++++-------- .../security/server/authentication/index.ts | 23 +++--- 4 files changed, 69 insertions(+), 54 deletions(-) diff --git a/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts b/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts index 532c47b7d12ee6..c23c30892126e8 100644 --- a/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts +++ b/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts @@ -17,6 +17,7 @@ import { SpacesServiceSetup } from '../../new_platform/spaces_service/spaces_ser import { elasticsearchServiceMock, httpServiceMock, + httpServerMock, } from '../../../../../../../src/core/server/mocks'; import * as kbnTestServer from '../../../../../../../src/test_utils/kbn_server'; import { HttpServiceSetup } from 'src/core/server'; @@ -184,17 +185,17 @@ describe('onPostAuthRequestInterceptor', () => { httpMock.basePath.set = jest.fn().mockImplementation((req: any, newPath: string) => { basePath = newPath; }); + const preAuthToolkit = httpServiceMock.createOnPreAuthToolkit(); + preAuthToolkit.rewriteUrl.mockImplementation(url => { + path = url; + return null as any; + }); httpMock.registerOnPreAuth = jest.fn().mockImplementation(async handler => { const preAuthRequest = { path, url: parse(path), }; - await handler(preAuthRequest, { - redirected: jest.fn().mockImplementation(url => { - path = url; - }), - next: jest.fn(), - }); + await handler(preAuthRequest, httpServerMock.createResponseFactory(), preAuthToolkit); }); const service = new SpacesService(log, configFn().get('server.basePath')); diff --git a/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts b/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts index b14201bdff2f6e..faac9f27cd15dc 100644 --- a/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts +++ b/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts @@ -3,7 +3,12 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import { KibanaRequest, OnPreAuthToolkit, HttpServiceSetup } from 'src/core/server'; +import { + KibanaRequest, + OnPreAuthToolkit, + ResponseFactory, + HttpServiceSetup, +} from 'src/core/server'; import { KibanaConfig } from 'src/legacy/server/kbn_server'; import { format } from 'url'; import { DEFAULT_SPACE_ID } from '../../../common/constants'; @@ -19,6 +24,7 @@ export function initSpacesOnRequestInterceptor({ config, http }: OnRequestInterc http.registerOnPreAuth(async function spacesOnPreAuthHandler( request: KibanaRequest, + response: ResponseFactory, toolkit: OnPreAuthToolkit ) { const path = request.url.pathname; @@ -41,7 +47,7 @@ export function initSpacesOnRequestInterceptor({ config, http }: OnRequestInterc }; }); - return toolkit.redirected(newUrl, { forward: true }); + return toolkit.rewriteUrl(newUrl); } return toolkit.next(); diff --git a/x-pack/plugins/security/server/authentication/index.test.ts b/x-pack/plugins/security/server/authentication/index.test.ts index 335beca4c4b6be..4f04ea06ce0c22 100644 --- a/x-pack/plugins/security/server/authentication/index.test.ts +++ b/x-pack/plugins/security/server/authentication/index.test.ts @@ -132,21 +132,23 @@ describe('setupAuthentication()', () => { it('replies with no credentials when security is disabled in elasticsearch', async () => { const mockRequest = httpServerMock.createKibanaRequest(); + const mockResponse = httpServerMock.createResponseFactory(); mockXpackInfo.feature.mockReturnValue(mockXPackFeature({ isEnabled: false })); - await authHandler(mockRequest, mockAuthToolkit); + await authHandler(mockRequest, mockResponse, mockAuthToolkit); expect(mockAuthToolkit.authenticated).toHaveBeenCalledTimes(1); expect(mockAuthToolkit.authenticated).toHaveBeenCalledWith(); - expect(mockAuthToolkit.redirected).not.toHaveBeenCalled(); - expect(mockAuthToolkit.rejected).not.toHaveBeenCalled(); + expect(mockResponse.redirected).not.toHaveBeenCalled(); + expect(mockResponse.unauthorized).not.toHaveBeenCalled(); expect(authenticate).not.toHaveBeenCalled(); }); it('continues request with credentials on success', async () => { const mockRequest = httpServerMock.createKibanaRequest(); + const mockResponse = httpServerMock.createResponseFactory(); const mockUser = mockAuthenticatedUser(); const mockAuthHeaders = { authorization: 'Basic xxx' }; @@ -154,15 +156,15 @@ describe('setupAuthentication()', () => { AuthenticationResult.succeeded(mockUser, { authHeaders: mockAuthHeaders }) ); - await authHandler(mockRequest, mockAuthToolkit); + await authHandler(mockRequest, mockResponse, mockAuthToolkit); expect(mockAuthToolkit.authenticated).toHaveBeenCalledTimes(1); expect(mockAuthToolkit.authenticated).toHaveBeenCalledWith({ state: mockUser, requestHeaders: mockAuthHeaders, }); - expect(mockAuthToolkit.redirected).not.toHaveBeenCalled(); - expect(mockAuthToolkit.rejected).not.toHaveBeenCalled(); + expect(mockResponse.redirected).not.toHaveBeenCalled(); + expect(mockResponse.unauthorized).not.toHaveBeenCalled(); expect(authenticate).toHaveBeenCalledTimes(1); expect(authenticate).toHaveBeenCalledWith(mockRequest); @@ -170,6 +172,7 @@ describe('setupAuthentication()', () => { it('returns authentication response headers on success if any', async () => { const mockRequest = httpServerMock.createKibanaRequest(); + const mockResponse = httpServerMock.createResponseFactory(); const mockUser = mockAuthenticatedUser(); const mockAuthHeaders = { authorization: 'Basic xxx' }; const mockAuthResponseHeaders = { 'WWW-Authenticate': 'Negotiate' }; @@ -181,7 +184,7 @@ describe('setupAuthentication()', () => { }) ); - await authHandler(mockRequest, mockAuthToolkit); + await authHandler(mockRequest, mockResponse, mockAuthToolkit); expect(mockAuthToolkit.authenticated).toHaveBeenCalledTimes(1); expect(mockAuthToolkit.authenticated).toHaveBeenCalledWith({ @@ -189,53 +192,59 @@ describe('setupAuthentication()', () => { requestHeaders: mockAuthHeaders, responseHeaders: mockAuthResponseHeaders, }); - expect(mockAuthToolkit.redirected).not.toHaveBeenCalled(); - expect(mockAuthToolkit.rejected).not.toHaveBeenCalled(); + expect(mockResponse.redirected).not.toHaveBeenCalled(); + expect(mockResponse.unauthorized).not.toHaveBeenCalled(); expect(authenticate).toHaveBeenCalledTimes(1); expect(authenticate).toHaveBeenCalledWith(mockRequest); }); it('redirects user if redirection is requested by the authenticator', async () => { + const mockResponse = httpServerMock.createResponseFactory(); authenticate.mockResolvedValue(AuthenticationResult.redirectTo('/some/url')); - await authHandler(httpServerMock.createKibanaRequest(), mockAuthToolkit); + await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); - expect(mockAuthToolkit.redirected).toHaveBeenCalledTimes(1); - expect(mockAuthToolkit.redirected).toHaveBeenCalledWith('/some/url'); + expect(mockResponse.redirected).toHaveBeenCalledTimes(1); + expect(mockResponse.redirected).toHaveBeenCalledWith(undefined, { + headers: { location: '/some/url' }, + }); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); - expect(mockAuthToolkit.rejected).not.toHaveBeenCalled(); + expect(mockResponse.unauthorized).not.toHaveBeenCalled(); }); it('rejects with `Internal Server Error` when `authenticate` throws unhandled exception', async () => { + const mockResponse = httpServerMock.createResponseFactory(); authenticate.mockRejectedValue(new Error('something went wrong')); - await authHandler(httpServerMock.createKibanaRequest(), mockAuthToolkit); + await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); - expect(mockAuthToolkit.rejected).toHaveBeenCalledTimes(1); - const [[error]] = mockAuthToolkit.rejected.mock.calls; - expect(error.message).toBe('something went wrong'); + expect(mockResponse.unauthorized).toHaveBeenCalledTimes(1); + const [[error]] = mockResponse.unauthorized.mock.calls; + expect(String(error)).toBe('Error: something went wrong'); expect(getErrorStatusCode(error)).toBe(500); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); - expect(mockAuthToolkit.redirected).not.toHaveBeenCalled(); + expect(mockResponse.redirected).not.toHaveBeenCalled(); }); it('rejects with wrapped original error when `authenticate` fails to authenticate user', async () => { + const mockResponse = httpServerMock.createResponseFactory(); const esError = Boom.badRequest('some message'); authenticate.mockResolvedValue(AuthenticationResult.failed(esError)); - await authHandler(httpServerMock.createKibanaRequest(), mockAuthToolkit); + await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); - expect(mockAuthToolkit.rejected).toHaveBeenCalledTimes(1); - const [[error]] = mockAuthToolkit.rejected.mock.calls; + expect(mockResponse.unauthorized).toHaveBeenCalledTimes(1); + const [[error]] = mockResponse.unauthorized.mock.calls; expect(error).toBe(esError); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); - expect(mockAuthToolkit.redirected).not.toHaveBeenCalled(); + expect(mockResponse.redirected).not.toHaveBeenCalled(); }); it('includes `WWW-Authenticate` header if `authenticate` fails to authenticate user and provides challenges', async () => { + const mockResponse = httpServerMock.createResponseFactory(); const originalError = Boom.unauthorized('some message'); originalError.output.headers['WWW-Authenticate'] = [ 'Basic realm="Access to prod", charset="UTF-8"', @@ -248,29 +257,31 @@ describe('setupAuthentication()', () => { }) ); - await authHandler(httpServerMock.createKibanaRequest(), mockAuthToolkit); + await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); - expect(mockAuthToolkit.rejected).toHaveBeenCalledTimes(1); - const [[error]] = mockAuthToolkit.rejected.mock.calls; - expect(error.message).toBe(originalError.message); - expect((error as Boom).output.headers).toEqual({ 'WWW-Authenticate': 'Negotiate' }); + expect(mockResponse.unauthorized).toHaveBeenCalledTimes(1); + const [[error, options]] = mockResponse.unauthorized.mock.calls; + expect(String(error)).toBe(`Error: ${originalError.message}`); + expect(options!.headers).toEqual({ 'WWW-Authenticate': 'Negotiate' }); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); - expect(mockAuthToolkit.redirected).not.toHaveBeenCalled(); + expect(mockResponse.redirected).not.toHaveBeenCalled(); }); it('returns `unauthorized` when authentication can not be handled', async () => { + const mockResponse = httpServerMock.createResponseFactory(); authenticate.mockResolvedValue(AuthenticationResult.notHandled()); - await authHandler(httpServerMock.createKibanaRequest(), mockAuthToolkit); + await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); + + expect(mockResponse.unauthorized).toHaveBeenCalledTimes(1); + const [[error]] = mockResponse.unauthorized.mock.calls; - expect(mockAuthToolkit.rejected).toHaveBeenCalledTimes(1); - const [[error]] = mockAuthToolkit.rejected.mock.calls; - expect(error.message).toBe('Unauthorized'); + expect(String(error)).toBe('Error: Unauthorized'); expect(getErrorStatusCode(error)).toBe(401); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); - expect(mockAuthToolkit.redirected).not.toHaveBeenCalled(); + expect(mockResponse.redirected).not.toHaveBeenCalled(); }); }); diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index 14acfd006e6ef2..04c13d061c4741 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -77,7 +77,7 @@ export async function setupAuthentication({ authLogger.debug('Successfully initialized authenticator.'); - core.http.registerAuth(async (request, t) => { + core.http.registerAuth(async (request, response, t) => { // If security is disabled continue with no user credentials and delete the client cookie as well. if (isSecurityFeatureDisabled()) { return t.authenticated(); @@ -88,7 +88,7 @@ export async function setupAuthentication({ authenticationResult = await authenticator.authenticate(request); } catch (err) { authLogger.error(err); - return t.rejected(wrapError(err)); + return response.unauthorized(wrapError(err)); } if (authenticationResult.succeeded()) { @@ -105,28 +105,25 @@ export async function setupAuthentication({ // authentication (username and password) or arbitrary external page managed by 3rd party // Identity Provider for SSO authentication mechanisms. Authentication provider is the one who // decides what location user should be redirected to. - return t.redirected(authenticationResult.redirectURL!); + return response.redirected(undefined, { + headers: { + location: authenticationResult.redirectURL!, + }, + }); } let error; if (authenticationResult.failed()) { authLogger.info(`Authentication attempt failed: ${authenticationResult.error!.message}`); error = wrapError(authenticationResult.error); - - const authResponseHeaders = authenticationResult.authResponseHeaders; - for (const [headerName, headerValue] of Object.entries(authResponseHeaders || {})) { - if (error.output.headers[headerName] !== undefined) { - authLogger.warn(`Server rewrites a error response header [${headerName}].`); - } - // Hapi typings don't support headers that are `string[]`. - error.output.headers[headerName] = headerValue as any; - } } else { authLogger.info('Could not handle authentication attempt'); error = Boom.unauthorized(); } - return t.rejected(error); + return response.unauthorized(error, { + headers: authenticationResult.authResponseHeaders, + }); }); authLogger.debug('Successfully registered core authentication handler.'); From 4c434d65789cc793b5126ca931c717f7b307c6fc Mon Sep 17 00:00:00 2001 From: restrry Date: Thu, 1 Aug 2019 14:12:16 +0200 Subject: [PATCH 03/14] Add a separate response factory for lifecycles. Only route handler can respond with 2xx response. Interceptors may redirect or reject an incoming request. --- src/core/server/http/http_server.mocks.ts | 13 ++++- src/core/server/http/index.ts | 1 + src/core/server/http/lifecycle/auth.ts | 8 +-- .../server/http/lifecycle/on_post_auth.ts | 8 +-- src/core/server/http/lifecycle/on_pre_auth.ts | 8 +-- src/core/server/http/router/index.ts | 2 + src/core/server/http/router/response.ts | 51 ++++++++++++------- src/core/server/index.ts | 1 + .../on_post_auth_interceptor.test.ts | 6 ++- .../on_request_interceptor.ts | 4 +- .../server/authentication/index.test.ts | 16 +++--- 11 files changed, 77 insertions(+), 41 deletions(-) diff --git a/src/core/server/http/http_server.mocks.ts b/src/core/server/http/http_server.mocks.ts index 8500e6cebc6dc8..8cb565753f2bc1 100644 --- a/src/core/server/http/http_server.mocks.ts +++ b/src/core/server/http/http_server.mocks.ts @@ -23,7 +23,7 @@ import querystring from 'querystring'; import { schema } from '@kbn/config-schema'; -import { KibanaRequest, RouteMethod, ResponseFactory } from './router'; +import { KibanaRequest, LifecycleResponseFactory, RouteMethod, ResponseFactory } from './router'; interface RequestFixtureOptions { headers?: Record; @@ -111,8 +111,19 @@ const createResponseFactoryMock = (): jest.Mocked => ({ internal: jest.fn(), }); +const createLifecycleResponseFactoryMock = (): jest.Mocked => ({ + redirected: jest.fn(), + badRequest: jest.fn(), + unauthorized: jest.fn(), + forbidden: jest.fn(), + notFound: jest.fn(), + conflict: jest.fn(), + internal: jest.fn(), +}); + export const httpServerMock = { createKibanaRequest: createKibanaRequestMock, createRawRequest: createRawRequestMock, createResponseFactory: createResponseFactoryMock, + createLifecycleResponseFactory: createLifecycleResponseFactoryMock, }; diff --git a/src/core/server/http/index.ts b/src/core/server/http/index.ts index 479779d11101e1..c1053f276d9a02 100644 --- a/src/core/server/http/index.ts +++ b/src/core/server/http/index.ts @@ -24,6 +24,7 @@ export { isRealRequest, KibanaRequest, KibanaRequestRoute, + LifecycleResponseFactory, ResponseError, ResponseErrorMeta, ResponseFactory, diff --git a/src/core/server/http/lifecycle/auth.ts b/src/core/server/http/lifecycle/auth.ts index 6631efb0439de1..98fa8291cb0e1a 100644 --- a/src/core/server/http/lifecycle/auth.ts +++ b/src/core/server/http/lifecycle/auth.ts @@ -22,8 +22,8 @@ import { HapiResponseAdapter, KibanaRequest, KibanaResponse, - responseFactory, - ResponseFactory, + lifecycleResponseFactory, + LifecycleResponseFactory, } from '../router'; enum ResultType { @@ -94,7 +94,7 @@ const toolkit: AuthToolkit = { /** @public */ export type AuthenticationHandler = ( request: KibanaRequest, - response: ResponseFactory, + response: LifecycleResponseFactory, t: AuthToolkit ) => AuthResult | KibanaResponse | Promise>; @@ -112,7 +112,7 @@ export function adoptToHapiAuthFormat( try { const result = await fn( KibanaRequest.from(request, undefined, false), - responseFactory, + lifecycleResponseFactory, toolkit ); if (result instanceof KibanaResponse) { diff --git a/src/core/server/http/lifecycle/on_post_auth.ts b/src/core/server/http/lifecycle/on_post_auth.ts index 906be3515bd5b8..e17440e1eb91e3 100644 --- a/src/core/server/http/lifecycle/on_post_auth.ts +++ b/src/core/server/http/lifecycle/on_post_auth.ts @@ -23,8 +23,8 @@ import { HapiResponseAdapter, KibanaRequest, KibanaResponse, - responseFactory, - ResponseFactory, + lifecycleResponseFactory, + LifecycleResponseFactory, } from '../router'; enum ResultType { @@ -58,7 +58,7 @@ export interface OnPostAuthToolkit { /** @public */ export type OnPostAuthHandler = ( request: KibanaRequest, - response: ResponseFactory, + response: LifecycleResponseFactory, toolkit: OnPostAuthToolkit ) => OnPostAuthResult | KibanaResponse | Promise>; @@ -79,7 +79,7 @@ export function adoptToHapiOnPostAuthFormat(fn: OnPostAuthHandler, log: Logger) ): Promise { const hapiResponseAdapter = new HapiResponseAdapter(responseToolkit); try { - const result = await fn(KibanaRequest.from(request), responseFactory, toolkit); + const result = await fn(KibanaRequest.from(request), lifecycleResponseFactory, toolkit); if (result instanceof KibanaResponse) { return hapiResponseAdapter.handle(result); } diff --git a/src/core/server/http/lifecycle/on_pre_auth.ts b/src/core/server/http/lifecycle/on_pre_auth.ts index 55e83a3fbdee1a..2fa1f27dad12be 100644 --- a/src/core/server/http/lifecycle/on_pre_auth.ts +++ b/src/core/server/http/lifecycle/on_pre_auth.ts @@ -23,8 +23,8 @@ import { HapiResponseAdapter, KibanaRequest, KibanaResponse, - responseFactory, - ResponseFactory, + lifecycleResponseFactory, + LifecycleResponseFactory, } from '../router'; enum ResultType { @@ -81,7 +81,7 @@ const toolkit: OnPreAuthToolkit = { /** @public */ export type OnPreAuthHandler = ( request: KibanaRequest, - response: ResponseFactory, + response: LifecycleResponseFactory, toolkit: OnPreAuthToolkit ) => OnPreAuthResult | KibanaResponse | Promise>; @@ -99,7 +99,7 @@ export function adoptToHapiOnPreAuthFormat(fn: OnPreAuthHandler, log: Logger) { const hapiResponseAdapter = new HapiResponseAdapter(responseToolkit); try { - const result = await fn(KibanaRequest.from(request), responseFactory, toolkit); + const result = await fn(KibanaRequest.from(request), lifecycleResponseFactory, toolkit); if (result instanceof KibanaResponse) { return hapiResponseAdapter.handle(result); } diff --git a/src/core/server/http/router/index.ts b/src/core/server/http/router/index.ts index fe1c66b66dcb81..59dff66aa1445d 100644 --- a/src/core/server/http/router/index.ts +++ b/src/core/server/http/router/index.ts @@ -23,6 +23,8 @@ export { KibanaRequest, KibanaRequestRoute, ensureRawRequest, isRealRequest } fr export { RouteMethod, RouteConfigOptions } from './route'; export { KibanaResponse, + lifecycleResponseFactory, + LifecycleResponseFactory, ResponseError, ResponseErrorMeta, responseFactory, diff --git a/src/core/server/http/router/response.ts b/src/core/server/http/router/response.ts index 65db87f8ae8f8e..fa1761d83ff40b 100644 --- a/src/core/server/http/router/response.ts +++ b/src/core/server/http/router/response.ts @@ -101,8 +101,7 @@ export type RedirectResponseOptions = HttpResponseOptions & { }; }; -export const responseFactory = { - // Success +const successResponseFactory = { /** * The request has succeeded. * Status code: `200`. @@ -127,21 +126,9 @@ export const responseFactory = { * @param options - {@link HttpResponseOptions} configures HTTP response parameters. */ noContent: (options: HttpResponseOptions = {}) => new KibanaResponse(204, undefined, options), +}; - /** - * Creates a response with defined status code and payload. - * @param payload - {@link HttpResponsePayload} payload to send to the client - * @param options - {@link CustomResponseOptions} configures HTTP response parameters. - */ - custom: (payload: HttpResponsePayload | ResponseError, options: CustomResponseOptions) => { - if (!options || !options.statusCode) { - throw new Error(`options.statusCode is expected to be set. given options: ${options}`); - } - const { statusCode: code, ...rest } = options; - return new KibanaResponse(code, payload, rest); - }, - - // Redirection +const redirectionResponseFactory = { /** * Redirect to a different URI. * Status code: `302`. @@ -151,8 +138,9 @@ export const responseFactory = { */ redirected: (payload: HttpResponsePayload, options: RedirectResponseOptions) => new KibanaResponse(302, payload, options), +}; - // Client error +const errorResponseFactory = { /** * The server cannot process the request due to something that is perceived to be a client error. * Status code: `400`. @@ -209,8 +197,37 @@ export const responseFactory = { new KibanaResponse(500, error, options), }; +export const responseFactory = { + ...successResponseFactory, + ...redirectionResponseFactory, + ...errorResponseFactory, + /** + * Creates a response with defined status code and payload. + * @param payload - {@link HttpResponsePayload} payload to send to the client + * @param options - {@link CustomResponseOptions} configures HTTP response parameters. + */ + custom: (payload: HttpResponsePayload | ResponseError, options: CustomResponseOptions) => { + if (!options || !options.statusCode) { + throw new Error(`options.statusCode is expected to be set. given options: ${options}`); + } + const { statusCode: code, ...rest } = options; + return new KibanaResponse(code, payload, rest); + }, +}; + +export const lifecycleResponseFactory = { + ...redirectionResponseFactory, + ...errorResponseFactory, +}; + /** * Creates an object containing request response payload, HTTP headers, error details, and other data transmitted to the client. * @public */ export type ResponseFactory = typeof responseFactory; + +/** + * Creates an object containing redirection or error response with error details, HTTP headers, and other data transmitted to the client. + * @public + */ +export type LifecycleResponseFactory = typeof lifecycleResponseFactory; diff --git a/src/core/server/index.ts b/src/core/server/index.ts index c7e933cf1e4923..336c54d00a12c9 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -66,6 +66,7 @@ export { GetAuthHeaders, KibanaRequest, KibanaRequestRoute, + LifecycleResponseFactory, OnPreAuthHandler, OnPreAuthToolkit, OnPostAuthHandler, diff --git a/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts b/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts index c23c30892126e8..ecd196a5a5a7b3 100644 --- a/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts +++ b/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts @@ -195,7 +195,11 @@ describe('onPostAuthRequestInterceptor', () => { path, url: parse(path), }; - await handler(preAuthRequest, httpServerMock.createResponseFactory(), preAuthToolkit); + await handler( + preAuthRequest, + httpServerMock.createLifecycleResponseFactory(), + preAuthToolkit + ); }); const service = new SpacesService(log, configFn().get('server.basePath')); diff --git a/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts b/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts index faac9f27cd15dc..88b6b5cd899ca3 100644 --- a/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts +++ b/x-pack/legacy/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts @@ -6,7 +6,7 @@ import { KibanaRequest, OnPreAuthToolkit, - ResponseFactory, + LifecycleResponseFactory, HttpServiceSetup, } from 'src/core/server'; import { KibanaConfig } from 'src/legacy/server/kbn_server'; @@ -24,7 +24,7 @@ export function initSpacesOnRequestInterceptor({ config, http }: OnRequestInterc http.registerOnPreAuth(async function spacesOnPreAuthHandler( request: KibanaRequest, - response: ResponseFactory, + response: LifecycleResponseFactory, toolkit: OnPreAuthToolkit ) { const path = request.url.pathname; diff --git a/x-pack/plugins/security/server/authentication/index.test.ts b/x-pack/plugins/security/server/authentication/index.test.ts index 4f04ea06ce0c22..cbe6c6976ed1f5 100644 --- a/x-pack/plugins/security/server/authentication/index.test.ts +++ b/x-pack/plugins/security/server/authentication/index.test.ts @@ -132,7 +132,7 @@ describe('setupAuthentication()', () => { it('replies with no credentials when security is disabled in elasticsearch', async () => { const mockRequest = httpServerMock.createKibanaRequest(); - const mockResponse = httpServerMock.createResponseFactory(); + const mockResponse = httpServerMock.createLifecycleResponseFactory(); mockXpackInfo.feature.mockReturnValue(mockXPackFeature({ isEnabled: false })); @@ -148,7 +148,7 @@ describe('setupAuthentication()', () => { it('continues request with credentials on success', async () => { const mockRequest = httpServerMock.createKibanaRequest(); - const mockResponse = httpServerMock.createResponseFactory(); + const mockResponse = httpServerMock.createLifecycleResponseFactory(); const mockUser = mockAuthenticatedUser(); const mockAuthHeaders = { authorization: 'Basic xxx' }; @@ -172,7 +172,7 @@ describe('setupAuthentication()', () => { it('returns authentication response headers on success if any', async () => { const mockRequest = httpServerMock.createKibanaRequest(); - const mockResponse = httpServerMock.createResponseFactory(); + const mockResponse = httpServerMock.createLifecycleResponseFactory(); const mockUser = mockAuthenticatedUser(); const mockAuthHeaders = { authorization: 'Basic xxx' }; const mockAuthResponseHeaders = { 'WWW-Authenticate': 'Negotiate' }; @@ -200,7 +200,7 @@ describe('setupAuthentication()', () => { }); it('redirects user if redirection is requested by the authenticator', async () => { - const mockResponse = httpServerMock.createResponseFactory(); + const mockResponse = httpServerMock.createLifecycleResponseFactory(); authenticate.mockResolvedValue(AuthenticationResult.redirectTo('/some/url')); await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); @@ -214,7 +214,7 @@ describe('setupAuthentication()', () => { }); it('rejects with `Internal Server Error` when `authenticate` throws unhandled exception', async () => { - const mockResponse = httpServerMock.createResponseFactory(); + const mockResponse = httpServerMock.createLifecycleResponseFactory(); authenticate.mockRejectedValue(new Error('something went wrong')); await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); @@ -229,7 +229,7 @@ describe('setupAuthentication()', () => { }); it('rejects with wrapped original error when `authenticate` fails to authenticate user', async () => { - const mockResponse = httpServerMock.createResponseFactory(); + const mockResponse = httpServerMock.createLifecycleResponseFactory(); const esError = Boom.badRequest('some message'); authenticate.mockResolvedValue(AuthenticationResult.failed(esError)); @@ -244,7 +244,7 @@ describe('setupAuthentication()', () => { }); it('includes `WWW-Authenticate` header if `authenticate` fails to authenticate user and provides challenges', async () => { - const mockResponse = httpServerMock.createResponseFactory(); + const mockResponse = httpServerMock.createLifecycleResponseFactory(); const originalError = Boom.unauthorized('some message'); originalError.output.headers['WWW-Authenticate'] = [ 'Basic realm="Access to prod", charset="UTF-8"', @@ -269,7 +269,7 @@ describe('setupAuthentication()', () => { }); it('returns `unauthorized` when authentication can not be handled', async () => { - const mockResponse = httpServerMock.createResponseFactory(); + const mockResponse = httpServerMock.createLifecycleResponseFactory(); authenticate.mockResolvedValue(AuthenticationResult.notHandled()); await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); From 4af298627a48c8a628a2990960c1d4560037dae7 Mon Sep 17 00:00:00 2001 From: restrry Date: Thu, 1 Aug 2019 14:33:43 +0200 Subject: [PATCH 04/14] re-generate docs --- ...ana-plugin-server.authenticationhandler.md | 2 +- .../kibana-plugin-server.authtoolkit.md | 2 - ...na-plugin-server.authtoolkit.redirected.md | 13 ------- ...bana-plugin-server.authtoolkit.rejected.md | 15 -------- ...-plugin-server.lifecycleresponsefactory.md | 13 +++++++ .../core/server/kibana-plugin-server.md | 2 + .../kibana-plugin-server.onpostauthhandler.md | 2 +- .../kibana-plugin-server.onpostauthtoolkit.md | 2 - ...gin-server.onpostauthtoolkit.redirected.md | 13 ------- ...lugin-server.onpostauthtoolkit.rejected.md | 15 -------- .../kibana-plugin-server.onpreauthhandler.md | 2 +- .../kibana-plugin-server.onpreauthtoolkit.md | 3 +- ...ugin-server.onpreauthtoolkit.redirected.md | 15 -------- ...plugin-server.onpreauthtoolkit.rejected.md | 15 -------- ...ugin-server.onpreauthtoolkit.rewriteurl.md | 13 +++++++ .../kibana-plugin-server.responsefactory.md | 13 +++++++ .../server/kibana-plugin-server.router.md | 1 - .../kibana-plugin-server.router.routes.md | 11 ------ src/core/server/http/lifecycle/auth.ts | 2 +- .../server/http/lifecycle/on_post_auth.ts | 2 +- src/core/server/http/lifecycle/on_pre_auth.ts | 3 +- src/core/server/http/router/response.ts | 2 +- .../server/http/router/response_adapter.ts | 4 +- src/core/server/http/router/router.ts | 2 +- src/core/server/server.api.md | 37 ++++++++----------- 25 files changed, 69 insertions(+), 135 deletions(-) delete mode 100644 docs/development/core/server/kibana-plugin-server.authtoolkit.redirected.md delete mode 100644 docs/development/core/server/kibana-plugin-server.authtoolkit.rejected.md create mode 100644 docs/development/core/server/kibana-plugin-server.lifecycleresponsefactory.md delete mode 100644 docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.redirected.md delete mode 100644 docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.rejected.md delete mode 100644 docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.redirected.md delete mode 100644 docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.rejected.md create mode 100644 docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.rewriteurl.md create mode 100644 docs/development/core/server/kibana-plugin-server.responsefactory.md delete mode 100644 docs/development/core/server/kibana-plugin-server.router.routes.md diff --git a/docs/development/core/server/kibana-plugin-server.authenticationhandler.md b/docs/development/core/server/kibana-plugin-server.authenticationhandler.md index 1437d5083df2d2..424aa3665e19c2 100644 --- a/docs/development/core/server/kibana-plugin-server.authenticationhandler.md +++ b/docs/development/core/server/kibana-plugin-server.authenticationhandler.md @@ -8,5 +8,5 @@ Signature: ```typescript -export declare type AuthenticationHandler = (request: KibanaRequest, t: AuthToolkit) => AuthResult | Promise; +export declare type AuthenticationHandler = (request: KibanaRequest, response: LifecycleResponseFactory, t: AuthToolkit) => AuthResult | KibanaResponse | Promise; ``` diff --git a/docs/development/core/server/kibana-plugin-server.authtoolkit.md b/docs/development/core/server/kibana-plugin-server.authtoolkit.md index 85bc0b42042412..0c030ddce4ec33 100644 --- a/docs/development/core/server/kibana-plugin-server.authtoolkit.md +++ b/docs/development/core/server/kibana-plugin-server.authtoolkit.md @@ -17,6 +17,4 @@ export interface AuthToolkit | Property | Type | Description | | --- | --- | --- | | [authenticated](./kibana-plugin-server.authtoolkit.authenticated.md) | (data?: AuthResultParams) => AuthResult | Authentication is successful with given credentials, allow request to pass through | -| [redirected](./kibana-plugin-server.authtoolkit.redirected.md) | (url: string) => AuthResult | Authentication requires to interrupt request handling and redirect to a configured url | -| [rejected](./kibana-plugin-server.authtoolkit.rejected.md) | (error: Error, options?: {
statusCode?: number;
}) => AuthResult | Authentication is unsuccessful, fail the request with specified error. | diff --git a/docs/development/core/server/kibana-plugin-server.authtoolkit.redirected.md b/docs/development/core/server/kibana-plugin-server.authtoolkit.redirected.md deleted file mode 100644 index eb07b1c4b0f647..00000000000000 --- a/docs/development/core/server/kibana-plugin-server.authtoolkit.redirected.md +++ /dev/null @@ -1,13 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [AuthToolkit](./kibana-plugin-server.authtoolkit.md) > [redirected](./kibana-plugin-server.authtoolkit.redirected.md) - -## AuthToolkit.redirected property - -Authentication requires to interrupt request handling and redirect to a configured url - -Signature: - -```typescript -redirected: (url: string) => AuthResult; -``` diff --git a/docs/development/core/server/kibana-plugin-server.authtoolkit.rejected.md b/docs/development/core/server/kibana-plugin-server.authtoolkit.rejected.md deleted file mode 100644 index bc353c7df9fba0..00000000000000 --- a/docs/development/core/server/kibana-plugin-server.authtoolkit.rejected.md +++ /dev/null @@ -1,15 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [AuthToolkit](./kibana-plugin-server.authtoolkit.md) > [rejected](./kibana-plugin-server.authtoolkit.rejected.md) - -## AuthToolkit.rejected property - -Authentication is unsuccessful, fail the request with specified error. - -Signature: - -```typescript -rejected: (error: Error, options?: { - statusCode?: number; - }) => AuthResult; -``` diff --git a/docs/development/core/server/kibana-plugin-server.lifecycleresponsefactory.md b/docs/development/core/server/kibana-plugin-server.lifecycleresponsefactory.md new file mode 100644 index 00000000000000..f1c427203dd24e --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.lifecycleresponsefactory.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [LifecycleResponseFactory](./kibana-plugin-server.lifecycleresponsefactory.md) + +## LifecycleResponseFactory type + +Creates an object containing redirection or error response with error details, HTTP headers, and other data transmitted to the client. + +Signature: + +```typescript +export declare type LifecycleResponseFactory = typeof lifecycleResponseFactory; +``` diff --git a/docs/development/core/server/kibana-plugin-server.md b/docs/development/core/server/kibana-plugin-server.md index bd4ef847230575..15a462c8364e2a 100644 --- a/docs/development/core/server/kibana-plugin-server.md +++ b/docs/development/core/server/kibana-plugin-server.md @@ -81,12 +81,14 @@ The plugin integrates with the core system via lifecycle events: `setup` | [GetAuthHeaders](./kibana-plugin-server.getauthheaders.md) | Get headers to authenticate a user against Elasticsearch. | | [Headers](./kibana-plugin-server.headers.md) | | | [LegacyRequest](./kibana-plugin-server.legacyrequest.md) | Support Legacy platform request for the period of migration. | +| [LifecycleResponseFactory](./kibana-plugin-server.lifecycleresponsefactory.md) | Creates an object containing redirection or error response with error details, HTTP headers, and other data transmitted to the client. | | [OnPostAuthHandler](./kibana-plugin-server.onpostauthhandler.md) | | | [OnPreAuthHandler](./kibana-plugin-server.onpreauthhandler.md) | | | [PluginInitializer](./kibana-plugin-server.plugininitializer.md) | The plugin export at the root of a plugin's server directory should conform to this interface. | | [PluginName](./kibana-plugin-server.pluginname.md) | Dedicated type for plugin name/id that is supposed to make Map/Set/Arrays that use it as a key or value more obvious. | | [RecursiveReadonly](./kibana-plugin-server.recursivereadonly.md) | | | [ResponseError](./kibana-plugin-server.responseerror.md) | Error message and optional data send to the client in case of error. | +| [ResponseFactory](./kibana-plugin-server.responsefactory.md) | Creates an object containing request response payload, HTTP headers, error details, and other data transmitted to the client. | | [RouteMethod](./kibana-plugin-server.routemethod.md) | The set of common HTTP methods supported by Kibana routing. | | [SavedObjectsClientContract](./kibana-plugin-server.savedobjectsclientcontract.md) | \#\# SavedObjectsClient errorsSince the SavedObjectsClient has its hands in everything we are a little paranoid about the way we present errors back to to application code. Ideally, all errors will be either:1. Caused by bad implementation (ie. undefined is not a function) and as such unpredictable 2. An error that has been classified and decorated appropriately by the decorators in [SavedObjectsErrorHelpers](./kibana-plugin-server.savedobjectserrorhelpers.md)Type 1 errors are inevitable, but since all expected/handle-able errors should be Type 2 the isXYZError() helpers exposed at SavedObjectsErrorHelpers should be used to understand and manage error responses from the SavedObjectsClient.Type 2 errors are decorated versions of the source error, so if the elasticsearch client threw an error it will be decorated based on its type. That means that rather than looking for error.body.error.type or doing substring checks on error.body.error.reason, just use the helpers to understand the meaning of the error:\`\`\`js if (SavedObjectsErrorHelpers.isNotFoundError(error)) { // handle 404 }if (SavedObjectsErrorHelpers.isNotAuthorizedError(error)) { // 401 handling should be automatic, but in case you wanted to know }// always rethrow the error unless you handle it throw error; \`\`\`\#\#\# 404s from missing indexFrom the perspective of application code and APIs the SavedObjectsClient is a black box that persists objects. One of the internal details that users have no control over is that we use an elasticsearch index for persistance and that index might be missing.At the time of writing we are in the process of transitioning away from the operating assumption that the SavedObjects index is always available. Part of this transition is handling errors resulting from an index missing. These used to trigger a 500 error in most cases, and in others cause 404s with different error messages.From my (Spencer) perspective, a 404 from the SavedObjectsApi is a 404; The object the request/call was targeting could not be found. This is why \#14141 takes special care to ensure that 404 errors are generic and don't distinguish between index missing or document missing.\#\#\# 503s from missing indexUnlike all other methods, create requests are supposed to succeed even when the Kibana index does not exist because it will be automatically created by elasticsearch. When that is not the case it is because Elasticsearch's action.auto_create_index setting prevents it from being created automatically so we throw a special 503 with the intention of informing the user that their Elasticsearch settings need to be updated.See [SavedObjectsErrorHelpers](./kibana-plugin-server.savedobjectserrorhelpers.md) | | [SavedObjectsClientWrapperFactory](./kibana-plugin-server.savedobjectsclientwrapperfactory.md) | | diff --git a/docs/development/core/server/kibana-plugin-server.onpostauthhandler.md b/docs/development/core/server/kibana-plugin-server.onpostauthhandler.md index b28f55233d5482..884eb3e6346bdd 100644 --- a/docs/development/core/server/kibana-plugin-server.onpostauthhandler.md +++ b/docs/development/core/server/kibana-plugin-server.onpostauthhandler.md @@ -8,5 +8,5 @@ Signature: ```typescript -export declare type OnPostAuthHandler = (request: KibanaRequest, t: OnPostAuthToolkit) => OnPostAuthResult | Promise; +export declare type OnPostAuthHandler = (request: KibanaRequest, response: LifecycleResponseFactory, toolkit: OnPostAuthToolkit) => OnPostAuthResult | KibanaResponse | Promise; ``` diff --git a/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.md b/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.md index b9d7a1463a2006..001c14c53fecb3 100644 --- a/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.md +++ b/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.md @@ -17,6 +17,4 @@ export interface OnPostAuthToolkit | Property | Type | Description | | --- | --- | --- | | [next](./kibana-plugin-server.onpostauthtoolkit.next.md) | () => OnPostAuthResult | To pass request to the next handler | -| [redirected](./kibana-plugin-server.onpostauthtoolkit.redirected.md) | (url: string) => OnPostAuthResult | To interrupt request handling and redirect to a configured url | -| [rejected](./kibana-plugin-server.onpostauthtoolkit.rejected.md) | (error: Error, options?: {
statusCode?: number;
}) => OnPostAuthResult | Fail the request with specified error. | diff --git a/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.redirected.md b/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.redirected.md deleted file mode 100644 index 94eab27724c8cd..00000000000000 --- a/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.redirected.md +++ /dev/null @@ -1,13 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [OnPostAuthToolkit](./kibana-plugin-server.onpostauthtoolkit.md) > [redirected](./kibana-plugin-server.onpostauthtoolkit.redirected.md) - -## OnPostAuthToolkit.redirected property - -To interrupt request handling and redirect to a configured url - -Signature: - -```typescript -redirected: (url: string) => OnPostAuthResult; -``` diff --git a/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.rejected.md b/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.rejected.md deleted file mode 100644 index 00efb4fde305ee..00000000000000 --- a/docs/development/core/server/kibana-plugin-server.onpostauthtoolkit.rejected.md +++ /dev/null @@ -1,15 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [OnPostAuthToolkit](./kibana-plugin-server.onpostauthtoolkit.md) > [rejected](./kibana-plugin-server.onpostauthtoolkit.rejected.md) - -## OnPostAuthToolkit.rejected property - -Fail the request with specified error. - -Signature: - -```typescript -rejected: (error: Error, options?: { - statusCode?: number; - }) => OnPostAuthResult; -``` diff --git a/docs/development/core/server/kibana-plugin-server.onpreauthhandler.md b/docs/development/core/server/kibana-plugin-server.onpreauthhandler.md index 8374f83fc810ce..5eca904227bb9e 100644 --- a/docs/development/core/server/kibana-plugin-server.onpreauthhandler.md +++ b/docs/development/core/server/kibana-plugin-server.onpreauthhandler.md @@ -8,5 +8,5 @@ Signature: ```typescript -export declare type OnPreAuthHandler = (request: KibanaRequest, t: OnPreAuthToolkit) => OnPreAuthResult | Promise; +export declare type OnPreAuthHandler = (request: KibanaRequest, response: LifecycleResponseFactory, toolkit: OnPreAuthToolkit) => OnPreAuthResult | KibanaResponse | Promise; ``` diff --git a/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.md b/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.md index 787c9010372e03..174f377eec292f 100644 --- a/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.md +++ b/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.md @@ -17,6 +17,5 @@ export interface OnPreAuthToolkit | Property | Type | Description | | --- | --- | --- | | [next](./kibana-plugin-server.onpreauthtoolkit.next.md) | () => OnPreAuthResult | To pass request to the next handler | -| [redirected](./kibana-plugin-server.onpreauthtoolkit.redirected.md) | (url: string, options?: {
forward: boolean;
}) => OnPreAuthResult | To interrupt request handling and redirect to a configured url. If "options.forwarded" = true, request will be forwarded to another url right on the server. | -| [rejected](./kibana-plugin-server.onpreauthtoolkit.rejected.md) | (error: Error, options?: {
statusCode?: number;
}) => OnPreAuthResult | Fail the request with specified error. | +| [rewriteUrl](./kibana-plugin-server.onpreauthtoolkit.rewriteurl.md) | (url: string) => OnPreAuthResult | Rewrite requested resources url before is was authenticated and routed to a handler | diff --git a/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.redirected.md b/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.redirected.md deleted file mode 100644 index 77deb5b61c4e25..00000000000000 --- a/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.redirected.md +++ /dev/null @@ -1,15 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [OnPreAuthToolkit](./kibana-plugin-server.onpreauthtoolkit.md) > [redirected](./kibana-plugin-server.onpreauthtoolkit.redirected.md) - -## OnPreAuthToolkit.redirected property - -To interrupt request handling and redirect to a configured url. If "options.forwarded" = true, request will be forwarded to another url right on the server. - -Signature: - -```typescript -redirected: (url: string, options?: { - forward: boolean; - }) => OnPreAuthResult; -``` diff --git a/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.rejected.md b/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.rejected.md deleted file mode 100644 index 1fd79d0b5766ba..00000000000000 --- a/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.rejected.md +++ /dev/null @@ -1,15 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [OnPreAuthToolkit](./kibana-plugin-server.onpreauthtoolkit.md) > [rejected](./kibana-plugin-server.onpreauthtoolkit.rejected.md) - -## OnPreAuthToolkit.rejected property - -Fail the request with specified error. - -Signature: - -```typescript -rejected: (error: Error, options?: { - statusCode?: number; - }) => OnPreAuthResult; -``` diff --git a/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.rewriteurl.md b/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.rewriteurl.md new file mode 100644 index 00000000000000..0f401379c20fdf --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.onpreauthtoolkit.rewriteurl.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [OnPreAuthToolkit](./kibana-plugin-server.onpreauthtoolkit.md) > [rewriteUrl](./kibana-plugin-server.onpreauthtoolkit.rewriteurl.md) + +## OnPreAuthToolkit.rewriteUrl property + +Rewrite requested resources url before is was authenticated and routed to a handler + +Signature: + +```typescript +rewriteUrl: (url: string) => OnPreAuthResult; +``` diff --git a/docs/development/core/server/kibana-plugin-server.responsefactory.md b/docs/development/core/server/kibana-plugin-server.responsefactory.md new file mode 100644 index 00000000000000..3b0e5b746c05a5 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.responsefactory.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [ResponseFactory](./kibana-plugin-server.responsefactory.md) + +## ResponseFactory type + +Creates an object containing request response payload, HTTP headers, error details, and other data transmitted to the client. + +Signature: + +```typescript +export declare type ResponseFactory = typeof responseFactory; +``` diff --git a/docs/development/core/server/kibana-plugin-server.router.md b/docs/development/core/server/kibana-plugin-server.router.md index 52193bbc553c71..b6046ec2381c85 100644 --- a/docs/development/core/server/kibana-plugin-server.router.md +++ b/docs/development/core/server/kibana-plugin-server.router.md @@ -22,7 +22,6 @@ export declare class Router | Property | Modifiers | Type | Description | | --- | --- | --- | --- | | [path](./kibana-plugin-server.router.path.md) | | string | | -| [routes](./kibana-plugin-server.router.routes.md) | | Array<Readonly<RouterRoute>> | | ## Methods diff --git a/docs/development/core/server/kibana-plugin-server.router.routes.md b/docs/development/core/server/kibana-plugin-server.router.routes.md deleted file mode 100644 index c825bfe72d236d..00000000000000 --- a/docs/development/core/server/kibana-plugin-server.router.routes.md +++ /dev/null @@ -1,11 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [Router](./kibana-plugin-server.router.md) > [routes](./kibana-plugin-server.router.routes.md) - -## Router.routes property - -Signature: - -```typescript -routes: Array>; -``` diff --git a/src/core/server/http/lifecycle/auth.ts b/src/core/server/http/lifecycle/auth.ts index 98fa8291cb0e1a..c05f5a14bda9fe 100644 --- a/src/core/server/http/lifecycle/auth.ts +++ b/src/core/server/http/lifecycle/auth.ts @@ -96,7 +96,7 @@ export type AuthenticationHandler = ( request: KibanaRequest, response: LifecycleResponseFactory, t: AuthToolkit -) => AuthResult | KibanaResponse | Promise>; +) => AuthResult | KibanaResponse | Promise; /** @public */ export function adoptToHapiAuthFormat( diff --git a/src/core/server/http/lifecycle/on_post_auth.ts b/src/core/server/http/lifecycle/on_post_auth.ts index e17440e1eb91e3..a7f4eb57c26947 100644 --- a/src/core/server/http/lifecycle/on_post_auth.ts +++ b/src/core/server/http/lifecycle/on_post_auth.ts @@ -60,7 +60,7 @@ export type OnPostAuthHandler = ( request: KibanaRequest, response: LifecycleResponseFactory, toolkit: OnPostAuthToolkit -) => OnPostAuthResult | KibanaResponse | Promise>; +) => OnPostAuthResult | KibanaResponse | Promise; const toolkit: OnPostAuthToolkit = { next: postAuthResult.next, diff --git a/src/core/server/http/lifecycle/on_pre_auth.ts b/src/core/server/http/lifecycle/on_pre_auth.ts index 2fa1f27dad12be..b02138360815d2 100644 --- a/src/core/server/http/lifecycle/on_pre_auth.ts +++ b/src/core/server/http/lifecycle/on_pre_auth.ts @@ -70,6 +70,7 @@ const preAuthResult = { export interface OnPreAuthToolkit { /** To pass request to the next handler */ next: () => OnPreAuthResult; + /** Rewrite requested resources url before is was authenticated and routed to a handler */ rewriteUrl: (url: string) => OnPreAuthResult; } @@ -83,7 +84,7 @@ export type OnPreAuthHandler = ( request: KibanaRequest, response: LifecycleResponseFactory, toolkit: OnPreAuthToolkit -) => OnPreAuthResult | KibanaResponse | Promise>; +) => OnPreAuthResult | KibanaResponse | Promise; /** * @public diff --git a/src/core/server/http/router/response.ts b/src/core/server/http/router/response.ts index fa1761d83ff40b..c958503e3d1870 100644 --- a/src/core/server/http/router/response.ts +++ b/src/core/server/http/router/response.ts @@ -40,7 +40,7 @@ export type ResponseError = meta?: ResponseErrorMeta; }; -export class KibanaResponse { +export class KibanaResponse { constructor( readonly status: number, readonly payload?: T, diff --git a/src/core/server/http/router/response_adapter.ts b/src/core/server/http/router/response_adapter.ts index 09e904d8a60628..d13612fda94bc1 100644 --- a/src/core/server/http/router/response_adapter.ts +++ b/src/core/server/http/router/response_adapter.ts @@ -56,7 +56,7 @@ export class HapiResponseAdapter { return error; } - public handle(kibanaResponse: KibanaResponse) { + public handle(kibanaResponse: KibanaResponse) { if (!(kibanaResponse instanceof KibanaResponse)) { throw new Error( `Unexpected result from Route Handler. Expected KibanaResponse, but given: ${typeDetect( @@ -68,7 +68,7 @@ export class HapiResponseAdapter { return this.toHapiResponse(kibanaResponse); } - private toHapiResponse(kibanaResponse: KibanaResponse) { + private toHapiResponse(kibanaResponse: KibanaResponse) { if (statusHelpers.isError(kibanaResponse.status)) { return this.toError(kibanaResponse); } diff --git a/src/core/server/http/router/router.ts b/src/core/server/http/router/router.ts index 46211eb43bc2e3..15c7d225aecfa2 100644 --- a/src/core/server/http/router/router.ts +++ b/src/core/server/http/router/router.ts @@ -187,4 +187,4 @@ export class Router { export type RequestHandler

= ( request: KibanaRequest, TypeOf, TypeOf>, createResponse: ResponseFactory -) => KibanaResponse | Promise>; +) => KibanaResponse | Promise; diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 6ab535a66cb360..45b7405b65dd07 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -25,9 +25,10 @@ import { Url } from 'url'; export type APICaller = (endpoint: string, clientParams: Record, options?: CallAPIOptions) => Promise; // Warning: (ae-forgotten-export) The symbol "AuthResult" needs to be exported by the entry point index.d.ts +// Warning: (ae-forgotten-export) The symbol "KibanaResponse" needs to be exported by the entry point index.d.ts // // @public (undocumented) -export type AuthenticationHandler = (request: KibanaRequest, t: AuthToolkit) => AuthResult | Promise; +export type AuthenticationHandler = (request: KibanaRequest, response: LifecycleResponseFactory, t: AuthToolkit) => AuthResult | KibanaResponse | Promise; // @public export type AuthHeaders = Record; @@ -42,10 +43,6 @@ export interface AuthResultParams { // @public export interface AuthToolkit { authenticated: (data?: AuthResultParams) => AuthResult; - redirected: (url: string) => AuthResult; - rejected: (error: Error, options?: { - statusCode?: number; - }) => AuthResult; } // Warning: (ae-forgotten-export) The symbol "BootstrapArgs" needs to be exported by the entry point index.d.ts @@ -233,6 +230,11 @@ export interface KibanaRequestRoute { // @public export type LegacyRequest = Request; +// Warning: (ae-forgotten-export) The symbol "lifecycleResponseFactory" needs to be exported by the entry point index.d.ts +// +// @public +export type LifecycleResponseFactory = typeof lifecycleResponseFactory; + // @public export interface Logger { debug(message: string, meta?: LogMeta): void; @@ -305,31 +307,22 @@ export interface LogRecord { // Warning: (ae-forgotten-export) The symbol "OnPostAuthResult" needs to be exported by the entry point index.d.ts // // @public (undocumented) -export type OnPostAuthHandler = (request: KibanaRequest, t: OnPostAuthToolkit) => OnPostAuthResult | Promise; +export type OnPostAuthHandler = (request: KibanaRequest, response: LifecycleResponseFactory, toolkit: OnPostAuthToolkit) => OnPostAuthResult | KibanaResponse | Promise; // @public export interface OnPostAuthToolkit { next: () => OnPostAuthResult; - redirected: (url: string) => OnPostAuthResult; - rejected: (error: Error, options?: { - statusCode?: number; - }) => OnPostAuthResult; } // Warning: (ae-forgotten-export) The symbol "OnPreAuthResult" needs to be exported by the entry point index.d.ts // // @public (undocumented) -export type OnPreAuthHandler = (request: KibanaRequest, t: OnPreAuthToolkit) => OnPreAuthResult | Promise; +export type OnPreAuthHandler = (request: KibanaRequest, response: LifecycleResponseFactory, toolkit: OnPreAuthToolkit) => OnPreAuthResult | KibanaResponse | Promise; // @public export interface OnPreAuthToolkit { next: () => OnPreAuthResult; - redirected: (url: string, options?: { - forward: boolean; - }) => OnPreAuthResult; - rejected: (error: Error, options?: { - statusCode?: number; - }) => OnPreAuthResult; + rewriteUrl: (url: string) => OnPreAuthResult; } // @public @@ -403,6 +396,11 @@ export interface ResponseErrorMeta { errorCode?: string; } +// Warning: (ae-forgotten-export) The symbol "responseFactory" needs to be exported by the entry point index.d.ts +// +// @public +export type ResponseFactory = typeof responseFactory; + // @public export interface RouteConfigOptions { authRequired?: boolean; @@ -419,15 +417,12 @@ export class Router { // Warning: (ae-forgotten-export) The symbol "RouteConfig" needs to be exported by the entry point index.d.ts // Warning: (ae-forgotten-export) The symbol "RequestHandler" needs to be exported by the entry point index.d.ts get

(route: RouteConfig, handler: RequestHandler): void; + // Warning: (ae-forgotten-export) The symbol "RouterRoute" needs to be exported by the entry point index.d.ts getRoutes(): Readonly[]; // (undocumented) readonly path: string; post

(route: RouteConfig, handler: RequestHandler): void; put

(route: RouteConfig, handler: RequestHandler): void; - // Warning: (ae-forgotten-export) The symbol "RouterRoute" needs to be exported by the entry point index.d.ts - // - // (undocumented) - routes: Array>; } // @public (undocumented) From 534ad9114be92d2d4676b0d6c9d5d66477d74dc2 Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Fri, 2 Aug 2019 12:30:27 +0200 Subject: [PATCH 05/14] response.internal --> response.internalError --- src/core/server/http/http_server.mocks.ts | 4 ++-- src/core/server/http/router/response.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/server/http/http_server.mocks.ts b/src/core/server/http/http_server.mocks.ts index 8cb565753f2bc1..c8b08cc6b30515 100644 --- a/src/core/server/http/http_server.mocks.ts +++ b/src/core/server/http/http_server.mocks.ts @@ -108,7 +108,7 @@ const createResponseFactoryMock = (): jest.Mocked => ({ forbidden: jest.fn(), notFound: jest.fn(), conflict: jest.fn(), - internal: jest.fn(), + internalError: jest.fn(), }); const createLifecycleResponseFactoryMock = (): jest.Mocked => ({ @@ -118,7 +118,7 @@ const createLifecycleResponseFactoryMock = (): jest.Mocked + internalError: (error: ResponseError = 'Internal Error', options: HttpResponseOptions = {}) => new KibanaResponse(500, error, options), }; From 6fb0fecda3a9d1db6d5e08e2c495f50a594e13a4 Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Fri, 2 Aug 2019 12:33:00 +0200 Subject: [PATCH 06/14] use internalError for exceptions in authenticator --- .../security/server/authentication/index.test.ts | 12 ++++++------ .../plugins/security/server/authentication/index.ts | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/security/server/authentication/index.test.ts b/x-pack/plugins/security/server/authentication/index.test.ts index cbe6c6976ed1f5..e3595cc0491761 100644 --- a/x-pack/plugins/security/server/authentication/index.test.ts +++ b/x-pack/plugins/security/server/authentication/index.test.ts @@ -141,7 +141,7 @@ describe('setupAuthentication()', () => { expect(mockAuthToolkit.authenticated).toHaveBeenCalledTimes(1); expect(mockAuthToolkit.authenticated).toHaveBeenCalledWith(); expect(mockResponse.redirected).not.toHaveBeenCalled(); - expect(mockResponse.unauthorized).not.toHaveBeenCalled(); + expect(mockResponse.internalError).not.toHaveBeenCalled(); expect(authenticate).not.toHaveBeenCalled(); }); @@ -164,7 +164,7 @@ describe('setupAuthentication()', () => { requestHeaders: mockAuthHeaders, }); expect(mockResponse.redirected).not.toHaveBeenCalled(); - expect(mockResponse.unauthorized).not.toHaveBeenCalled(); + expect(mockResponse.internalError).not.toHaveBeenCalled(); expect(authenticate).toHaveBeenCalledTimes(1); expect(authenticate).toHaveBeenCalledWith(mockRequest); @@ -193,7 +193,7 @@ describe('setupAuthentication()', () => { responseHeaders: mockAuthResponseHeaders, }); expect(mockResponse.redirected).not.toHaveBeenCalled(); - expect(mockResponse.unauthorized).not.toHaveBeenCalled(); + expect(mockResponse.internalError).not.toHaveBeenCalled(); expect(authenticate).toHaveBeenCalledTimes(1); expect(authenticate).toHaveBeenCalledWith(mockRequest); @@ -210,7 +210,7 @@ describe('setupAuthentication()', () => { headers: { location: '/some/url' }, }); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); - expect(mockResponse.unauthorized).not.toHaveBeenCalled(); + expect(mockResponse.internalError).not.toHaveBeenCalled(); }); it('rejects with `Internal Server Error` when `authenticate` throws unhandled exception', async () => { @@ -219,8 +219,8 @@ describe('setupAuthentication()', () => { await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); - expect(mockResponse.unauthorized).toHaveBeenCalledTimes(1); - const [[error]] = mockResponse.unauthorized.mock.calls; + expect(mockResponse.internalError).toHaveBeenCalledTimes(1); + const [[error]] = mockResponse.internalError.mock.calls; expect(String(error)).toBe('Error: something went wrong'); expect(getErrorStatusCode(error)).toBe(500); diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index 04c13d061c4741..ffca1d78e64c36 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -88,7 +88,7 @@ export async function setupAuthentication({ authenticationResult = await authenticator.authenticate(request); } catch (err) { authLogger.error(err); - return response.unauthorized(wrapError(err)); + return response.internalError(wrapError(err)); } if (authenticationResult.succeeded()) { From efe4daeb3331da6289cb134a6b30afa23e8ede90 Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Fri, 2 Aug 2019 12:35:37 +0200 Subject: [PATCH 07/14] before Security plugin proxied ES error status code. now sets explicitly. --- .../server/authentication/index.test.ts | 9 +++----- .../security/server/authentication/index.ts | 23 +++++++++++-------- .../apis/security/kerberos_login.ts | 2 +- .../apis/security/oidc_initiate_auth.js | 6 +---- .../apis/security/saml_login.js | 18 +++------------ 5 files changed, 22 insertions(+), 36 deletions(-) diff --git a/x-pack/plugins/security/server/authentication/index.test.ts b/x-pack/plugins/security/server/authentication/index.test.ts index e3595cc0491761..fa1721a1d57f8e 100644 --- a/x-pack/plugins/security/server/authentication/index.test.ts +++ b/x-pack/plugins/security/server/authentication/index.test.ts @@ -32,7 +32,6 @@ import { } from '../../../../../src/core/server'; import { AuthenticatedUser } from '../../common/model'; import { ConfigType, createConfig$ } from '../config'; -import { getErrorStatusCode } from '../errors'; import { LegacyAPI } from '../plugin'; import { AuthenticationResult } from './authentication_result'; import { setupAuthentication } from '.'; @@ -222,21 +221,20 @@ describe('setupAuthentication()', () => { expect(mockResponse.internalError).toHaveBeenCalledTimes(1); const [[error]] = mockResponse.internalError.mock.calls; expect(String(error)).toBe('Error: something went wrong'); - expect(getErrorStatusCode(error)).toBe(500); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); expect(mockResponse.redirected).not.toHaveBeenCalled(); }); - it('rejects with wrapped original error when `authenticate` fails to authenticate user', async () => { + it('rejects with original `badRequest` error when `authenticate` fails to authenticate user', async () => { const mockResponse = httpServerMock.createLifecycleResponseFactory(); const esError = Boom.badRequest('some message'); authenticate.mockResolvedValue(AuthenticationResult.failed(esError)); await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); - expect(mockResponse.unauthorized).toHaveBeenCalledTimes(1); - const [[error]] = mockResponse.unauthorized.mock.calls; + expect(mockResponse.badRequest).toHaveBeenCalledTimes(1); + const [[error]] = mockResponse.badRequest.mock.calls; expect(error).toBe(esError); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); @@ -278,7 +276,6 @@ describe('setupAuthentication()', () => { const [[error]] = mockResponse.unauthorized.mock.calls; expect(String(error)).toBe('Error: Unauthorized'); - expect(getErrorStatusCode(error)).toBe(401); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); expect(mockResponse.redirected).not.toHaveBeenCalled(); diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index ffca1d78e64c36..548a09ac72be68 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -13,7 +13,7 @@ import { } from '../../../../../src/core/server'; import { AuthenticatedUser } from '../../common/model'; import { ConfigType } from '../config'; -import { getErrorStatusCode, wrapError } from '../errors'; +import { getErrorStatusCode } from '../errors'; import { Authenticator, ProviderSession } from './authenticator'; import { LegacyAPI } from '../plugin'; import { createAPIKey, CreateAPIKeyOptions } from './api_keys'; @@ -88,7 +88,7 @@ export async function setupAuthentication({ authenticationResult = await authenticator.authenticate(request); } catch (err) { authLogger.error(err); - return response.internalError(wrapError(err)); + return response.internalError(err); } if (authenticationResult.succeeded()) { @@ -112,18 +112,23 @@ export async function setupAuthentication({ }); } - let error; if (authenticationResult.failed()) { authLogger.info(`Authentication attempt failed: ${authenticationResult.error!.message}`); - error = wrapError(authenticationResult.error); + const error = authenticationResult.error; + return getErrorStatusCode(error) === 400 + ? response.badRequest(error, { + headers: authenticationResult.authResponseHeaders, + }) + : response.unauthorized(error, { + headers: authenticationResult.authResponseHeaders, + }); } else { authLogger.info('Could not handle authentication attempt'); - error = Boom.unauthorized(); + const error = Boom.unauthorized(); + return response.unauthorized(error, { + headers: authenticationResult.authResponseHeaders, + }); } - - return response.unauthorized(error, { - headers: authenticationResult.authResponseHeaders, - }); }); authLogger.debug('Successfully registered core authentication handler.'); diff --git a/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts b/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts index a05c51b85e4f16..68dc1cf58e927f 100644 --- a/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts +++ b/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts @@ -146,7 +146,7 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) { const spnegoResponse = await supertest .get('/api/security/v1/me') .set('Authorization', 'Negotiate (:I am malformed:)') - .expect(500); + .expect(401); expect(spnegoResponse.headers['set-cookie']).to.be(undefined); expect(spnegoResponse.headers['www-authenticate']).to.be(undefined); }); diff --git a/x-pack/test/oidc_api_integration/apis/security/oidc_initiate_auth.js b/x-pack/test/oidc_api_integration/apis/security/oidc_initiate_auth.js index 7460b1f9e238c2..69b1277926834a 100644 --- a/x-pack/test/oidc_api_integration/apis/security/oidc_initiate_auth.js +++ b/x-pack/test/oidc_api_integration/apis/security/oidc_initiate_auth.js @@ -341,11 +341,7 @@ export default function ({ getService }) { .set('Cookie', sessionCookie.cookieString()) .expect(400); - expect(apiResponse.body).to.eql({ - error: 'Bad Request', - message: 'Both access and refresh tokens are expired.', - statusCode: 400 - }); + expect(apiResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); }); it('should reject AJAX requests', async () => { diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.js b/x-pack/test/saml_api_integration/apis/security/saml_login.js index 401ffdea0418d4..be4fef0f57ab30 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.js +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.js @@ -327,11 +327,7 @@ export default function ({ getService }) { .set('Cookie', sessionCookie.cookieString()) .expect(400); - expect(apiResponse.body).to.eql({ - error: 'Bad Request', - message: 'Both access and refresh tokens are expired.', - statusCode: 400 - }); + expect(apiResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); }); it('should redirect to home page if session cookie is not provided', async () => { @@ -384,11 +380,7 @@ export default function ({ getService }) { .set('Cookie', sessionCookie.cookieString()) .expect(400); - expect(apiResponse.body).to.eql({ - error: 'Bad Request', - message: 'Both access and refresh tokens are expired.', - statusCode: 400 - }); + expect(apiResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); }); it('should invalidate access token on IdP initiated logout even if there is no Kibana session', async () => { @@ -411,11 +403,7 @@ export default function ({ getService }) { .set('Cookie', sessionCookie.cookieString()) .expect(400); - expect(apiResponse.body).to.eql({ - error: 'Bad Request', - message: 'Both access and refresh tokens are expired.', - statusCode: 400 - }); + expect(apiResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); }); }); From fba3b7ab15dbcce310ec8f6ba1f2a8c991998af9 Mon Sep 17 00:00:00 2001 From: restrry Date: Fri, 2 Aug 2019 14:04:07 +0200 Subject: [PATCH 08/14] provide error via message field of error response for BWC --- src/core/server/http/http_server.test.ts | 6 +- .../http/integration_tests/lifecycle.test.ts | 62 +++++++++---------- .../http/integration_tests/router.test.ts | 50 +++++++-------- .../server/http/router/response_adapter.ts | 6 +- 4 files changed, 62 insertions(+), 62 deletions(-) diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index 793201b98fb101..70022fab25a96f 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -147,7 +147,7 @@ test('invalid params', async () => { .expect(400) .then(res => { expect(res.body).toEqual({ - error: '[request params.test]: expected value of type [number] but got [string]', + message: '[request params.test]: expected value of type [number] but got [string]', }); }); }); @@ -210,7 +210,7 @@ test('invalid query', async () => { .expect(400) .then(res => { expect(res.body).toEqual({ - error: '[request query.bar]: expected value of type [number] but got [string]', + message: '[request query.bar]: expected value of type [number] but got [string]', }); }); }); @@ -278,7 +278,7 @@ test('invalid body', async () => { .expect(400) .then(res => { expect(res.body).toEqual({ - error: '[request body.bar]: expected value of type [number] but got [string]', + message: '[request body.bar]: expected value of type [number] but got [string]', }); }); }); diff --git a/src/core/server/http/integration_tests/lifecycle.test.ts b/src/core/server/http/integration_tests/lifecycle.test.ts index 0571b18d206e56..185204011f761a 100644 --- a/src/core/server/http/integration_tests/lifecycle.test.ts +++ b/src/core/server/http/integration_tests/lifecycle.test.ts @@ -180,14 +180,14 @@ describe('OnPreAuth', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` - Array [ - Array [ - [Error: reason], - ], - ] - `); + Array [ + Array [ + [Error: reason], + ], + ] + `); }); it('returns internal error if interceptor returns unexpected result', async () => { @@ -204,14 +204,14 @@ describe('OnPreAuth', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` - Array [ - Array [ - [Error: Unexpected result from OnPreAuth. Expected OnPreAuthResult or KibanaResponse, but given: [object Object].], - ], - ] - `); + Array [ + Array [ + [Error: Unexpected result from OnPreAuth. Expected OnPreAuthResult or KibanaResponse, but given: [object Object].], + ], + ] + `); }); it(`doesn't share request object between interceptors`, async () => { const { registerRouter, registerOnPreAuth, server: innerServer } = await server.setup(config); @@ -332,14 +332,14 @@ describe('OnPostAuth', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` - Array [ - Array [ - [Error: reason], - ], - ] - `); + Array [ + Array [ + [Error: reason], + ], + ] + `); }); it('returns internal error if interceptor returns unexpected result', async () => { @@ -356,14 +356,14 @@ describe('OnPostAuth', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` - Array [ - Array [ - [Error: Unexpected result from OnPostAuth. Expected OnPostAuthResult or KibanaResponse, but given: [object Object].], - ], - ] - `); + Array [ + Array [ + [Error: Unexpected result from OnPostAuth. Expected OnPostAuthResult or KibanaResponse, but given: [object Object].], + ], + ] + `); }); it(`doesn't share request object between interceptors`, async () => { const { registerRouter, registerOnPostAuth, server: innerServer } = await server.setup(config); @@ -532,7 +532,7 @@ describe('Auth', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -859,7 +859,7 @@ describe('Auth', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -883,7 +883,7 @@ describe('Auth', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index 1d3ee4643830d9..40f6631182aa80 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -63,7 +63,7 @@ describe('Handler', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -88,7 +88,7 @@ describe('Handler', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -111,7 +111,7 @@ describe('Handler', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -146,7 +146,7 @@ describe('Handler', () => { .expect(400); expect(result.body).toEqual({ - error: '[request query.page]: expected value of type [number] but got [string]', + message: '[request query.page]: expected value of type [number] but got [string]', }); }); }); @@ -531,7 +531,7 @@ describe('Response factory', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -559,7 +559,7 @@ describe('Response factory', () => { .get('/') .expect(400); - expect(result.body).toEqual({ error: 'some message' }); + expect(result.body).toEqual({ message: 'some message' }); }); it('400 Bad request with default message', async () => { @@ -577,7 +577,7 @@ describe('Response factory', () => { .get('/') .expect(400); - expect(result.body).toEqual({ error: 'Bad Request' }); + expect(result.body).toEqual({ message: 'Bad Request' }); }); it('400 Bad request with additional data', async () => { @@ -596,7 +596,7 @@ describe('Response factory', () => { .expect(400); expect(result.body).toEqual({ - error: 'some message', + message: 'some message', meta: { data: ['good', 'bad'], }, @@ -623,7 +623,7 @@ describe('Response factory', () => { .get('/') .expect(401); - expect(result.body.error).toBe('no access'); + expect(result.body.message).toBe('no access'); expect(result.header['www-authenticate']).toBe('challenge'); }); @@ -642,7 +642,7 @@ describe('Response factory', () => { .get('/') .expect(401); - expect(result.body.error).toBe('Unauthorized'); + expect(result.body.message).toBe('Unauthorized'); }); it('403 Forbidden', async () => { @@ -661,7 +661,7 @@ describe('Response factory', () => { .get('/') .expect(403); - expect(result.body.error).toBe('reason'); + expect(result.body.message).toBe('reason'); }); it('403 Forbidden with default message', async () => { @@ -679,7 +679,7 @@ describe('Response factory', () => { .get('/') .expect(403); - expect(result.body.error).toBe('Forbidden'); + expect(result.body.message).toBe('Forbidden'); }); it('404 Not Found', async () => { @@ -698,7 +698,7 @@ describe('Response factory', () => { .get('/') .expect(404); - expect(result.body.error).toBe('file is not found'); + expect(result.body.message).toBe('file is not found'); }); it('404 Not Found with default message', async () => { @@ -716,7 +716,7 @@ describe('Response factory', () => { .get('/') .expect(404); - expect(result.body.error).toBe('Not Found'); + expect(result.body.message).toBe('Not Found'); }); it('409 Conflict', async () => { @@ -735,7 +735,7 @@ describe('Response factory', () => { .get('/') .expect(409); - expect(result.body.error).toBe('stale version'); + expect(result.body.message).toBe('stale version'); }); it('409 Conflict with default message', async () => { @@ -753,7 +753,7 @@ describe('Response factory', () => { .get('/') .expect(409); - expect(result.body.error).toBe('Conflict'); + expect(result.body.message).toBe('Conflict'); }); }); @@ -849,7 +849,7 @@ describe('Response factory', () => { .get('/') .expect(401); - expect(result.body.error).toBe('unauthorized'); + expect(result.body.message).toBe('unauthorized'); }); it('creates error response with additional data', async () => { @@ -876,7 +876,7 @@ describe('Response factory', () => { .expect(401); expect(result.body).toEqual({ - error: 'unauthorized', + message: 'unauthorized', meta: { errorCode: 'K401' }, }); }); @@ -905,7 +905,7 @@ describe('Response factory', () => { .expect(401); expect(result.body).toEqual({ - error: 'unauthorized', + message: 'unauthorized', meta: { errorCode: 'K401' }, }); }); @@ -928,7 +928,7 @@ describe('Response factory', () => { .get('/') .expect(401); - expect(result.body.error).toBe('Unauthorized'); + expect(result.body.message).toBe('Unauthorized'); }); it("Doesn't log details of created 500 Server error response", async () => { @@ -948,7 +948,7 @@ describe('Response factory', () => { .get('/') .expect(500); - expect(result.body.error).toBe('reason'); + expect(result.body.message).toBe('reason'); expect(loggingServiceMock.collect(logger).error).toHaveLength(0); }); @@ -972,7 +972,7 @@ describe('Response factory', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -999,7 +999,7 @@ describe('Response factory', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -1025,7 +1025,7 @@ describe('Response factory', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -1051,7 +1051,7 @@ describe('Response factory', () => { .get('/') .expect(500); - expect(result.body.error).toBe('An internal server error occurred.'); + expect(result.body.message).toBe('An internal server error occurred.'); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ diff --git a/src/core/server/http/router/response_adapter.ts b/src/core/server/http/router/response_adapter.ts index d13612fda94bc1..a4c290598ea49a 100644 --- a/src/core/server/http/router/response_adapter.ts +++ b/src/core/server/http/router/response_adapter.ts @@ -41,7 +41,7 @@ const statusHelpers = { export class HapiResponseAdapter { constructor(private readonly responseToolkit: HapiResponseToolkit) {} public toBadRequest(message: string) { - return this.responseToolkit.response({ error: message }).code(400); + return this.responseToolkit.response({ message }).code(400); } public toInternalError() { @@ -50,7 +50,7 @@ export class HapiResponseAdapter { }); error.output.payload = { - error: 'An internal server error occurred.', + message: 'An internal server error occurred.', } as any; // our error format is not compatible with boom return error; @@ -114,7 +114,7 @@ export class HapiResponseAdapter { }); error.output.payload = { - error: getErrorMessage(payload), + message: getErrorMessage(payload), meta: getErrorMeta(payload), } as any; // our error format is not compatible with boom From 5eadb6e547b4ea98cf511d3557023fba79a3202c Mon Sep 17 00:00:00 2001 From: restrry Date: Tue, 6 Aug 2019 17:32:07 +0200 Subject: [PATCH 09/14] update docs --- src/core/server/http/router/response.ts | 2 +- src/core/server/server.api.md | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/server/http/router/response.ts b/src/core/server/http/router/response.ts index e6034828c7532c..9272a98dcda093 100644 --- a/src/core/server/http/router/response.ts +++ b/src/core/server/http/router/response.ts @@ -279,7 +279,7 @@ export const kibanaResponseFactory = { /** * Creates a response with defined status code and payload. * @param payload - {@link HttpResponsePayload} payload to send to the client - * @param options - {@link CustomResponseOptions} configures HTTP response parameters. + * @param options - {@link CustomHttpResponseOptions} configures HTTP response parameters. */ custom: (payload: HttpResponsePayload | ResponseError, options: CustomHttpResponseOptions) => { if (!options || !options.statusCode) { diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 84de92285df987..69c3215ffa7376 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -983,7 +983,6 @@ export interface SessionStorageFactory { // Warnings were encountered during analysis: // -// src/core/server/http/router/response.ts:284:3 - (ae-unresolved-link) The @link reference could not be resolved: The package "kibana" does not have an export "CustomResponseOptions" // src/core/server/plugins/plugin_context.ts:34:10 - (ae-forgotten-export) The symbol "EnvironmentMode" needs to be exported by the entry point index.d.ts // src/core/server/plugins/plugins_service.ts:37:5 - (ae-forgotten-export) The symbol "DiscoveredPluginInternal" needs to be exported by the entry point index.d.ts From 93421851abcd0de14ab745146e6e58f7e530a34f Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Wed, 7 Aug 2019 10:42:05 +0200 Subject: [PATCH 10/14] add customError response --- src/core/server/http/http_server.mocks.ts | 2 + .../http/integration_tests/router.test.ts | 49 +++++++++++++++++++ src/core/server/http/router/response.ts | 12 +++++ 3 files changed, 63 insertions(+) diff --git a/src/core/server/http/http_server.mocks.ts b/src/core/server/http/http_server.mocks.ts index 15792ab1b0afaa..d676a67b734d8d 100644 --- a/src/core/server/http/http_server.mocks.ts +++ b/src/core/server/http/http_server.mocks.ts @@ -114,6 +114,7 @@ const createResponseFactoryMock = (): jest.Mocked => ({ notFound: jest.fn(), conflict: jest.fn(), internalError: jest.fn(), + customError: jest.fn(), }); const createLifecycleResponseFactoryMock = (): jest.Mocked => ({ @@ -124,6 +125,7 @@ const createLifecycleResponseFactoryMock = (): jest.Mocked { expect(result.body.message).toBe('Conflict'); }); + + it('Custom error response', async () => { + const router = new Router('/'); + + router.get({ path: '/', validate: false }, (req, res) => { + const error = new Error('some message'); + return res.customError(error, { + statusCode: 418, + }); + }); + + const { registerRouter, server: innerServer } = await server.setup(config); + registerRouter(router); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(418); + + expect(result.body).toEqual({ message: 'some message' }); + }); + + it('Custom error response requires error status code', async () => { + const router = new Router('/'); + + router.get({ path: '/', validate: false }, (req, res) => { + const error = new Error('some message'); + return res.customError(error, { + statusCode: 200, + }); + }); + + const { registerRouter, server: innerServer } = await server.setup(config); + registerRouter(router); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body).toEqual({ message: 'An internal server error occurred.' }); + expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` + Array [ + Array [ + [Error: Unexpected Http status code. Expected from 400 to 599, but given: 200], + ], + ] + `); + }); }); describe('Custom', () => { diff --git a/src/core/server/http/router/response.ts b/src/core/server/http/router/response.ts index 9272a98dcda093..715215c5edb0b8 100644 --- a/src/core/server/http/router/response.ts +++ b/src/core/server/http/router/response.ts @@ -179,6 +179,18 @@ const errorResponseFactory = { */ internalError: (error: ResponseError = 'Internal Error', options: HttpResponseOptions = {}) => new KibanaResponse(500, error, options), + + customError: (error: ResponseError, options: CustomHttpResponseOptions) => { + if (!options || !options.statusCode) { + throw new Error(`options.statusCode is expected to be set. given options: ${options}`); + } + if (options.statusCode < 400 || options.statusCode >= 600) { + throw new Error( + `Unexpected Http status code. Expected from 400 to 599, but given: ${options.statusCode}` + ); + } + return new KibanaResponse(options.statusCode, error, options); + }, }; /** * Set of helpers used to create `KibanaResponse` to form HTTP response on an incoming request. From 8634bea66c952e5b9061fc5853907c424b8da914 Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Wed, 7 Aug 2019 10:42:51 +0200 Subject: [PATCH 11/14] restore integration test and update unit tests --- .../server/authentication/index.test.ts | 24 +++++++++----- .../security/server/authentication/index.ts | 32 ++++++++++--------- .../apis/security/kerberos_login.ts | 2 +- 3 files changed, 34 insertions(+), 24 deletions(-) diff --git a/x-pack/plugins/security/server/authentication/index.test.ts b/x-pack/plugins/security/server/authentication/index.test.ts index fa1721a1d57f8e..5f45ca0f0de753 100644 --- a/x-pack/plugins/security/server/authentication/index.test.ts +++ b/x-pack/plugins/security/server/authentication/index.test.ts @@ -212,7 +212,7 @@ describe('setupAuthentication()', () => { expect(mockResponse.internalError).not.toHaveBeenCalled(); }); - it('rejects with `Internal Server Error` when `authenticate` throws unhandled exception', async () => { + it('rejects with `Internal Server Error` and log error when `authenticate` throws unhandled exception', async () => { const mockResponse = httpServerMock.createLifecycleResponseFactory(); authenticate.mockRejectedValue(new Error('something went wrong')); @@ -220,10 +220,18 @@ describe('setupAuthentication()', () => { expect(mockResponse.internalError).toHaveBeenCalledTimes(1); const [[error]] = mockResponse.internalError.mock.calls; - expect(String(error)).toBe('Error: something went wrong'); + expect(error).toBeUndefined(); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); expect(mockResponse.redirected).not.toHaveBeenCalled(); + expect(loggingServiceMock.collect(mockSetupAuthenticationParams.loggers).error) + .toMatchInlineSnapshot(` + Array [ + Array [ + [Error: something went wrong], + ], + ] + `); }); it('rejects with original `badRequest` error when `authenticate` fails to authenticate user', async () => { @@ -233,8 +241,8 @@ describe('setupAuthentication()', () => { await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); - expect(mockResponse.badRequest).toHaveBeenCalledTimes(1); - const [[error]] = mockResponse.badRequest.mock.calls; + expect(mockResponse.customError).toHaveBeenCalledTimes(1); + const [[error]] = mockResponse.customError.mock.calls; expect(error).toBe(esError); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); @@ -257,9 +265,9 @@ describe('setupAuthentication()', () => { await authHandler(httpServerMock.createKibanaRequest(), mockResponse, mockAuthToolkit); - expect(mockResponse.unauthorized).toHaveBeenCalledTimes(1); - const [[error, options]] = mockResponse.unauthorized.mock.calls; - expect(String(error)).toBe(`Error: ${originalError.message}`); + expect(mockResponse.customError).toHaveBeenCalledTimes(1); + const [[error, options]] = mockResponse.customError.mock.calls; + expect(error).toBe(originalError); expect(options!.headers).toEqual({ 'WWW-Authenticate': 'Negotiate' }); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); @@ -275,7 +283,7 @@ describe('setupAuthentication()', () => { expect(mockResponse.unauthorized).toHaveBeenCalledTimes(1); const [[error]] = mockResponse.unauthorized.mock.calls; - expect(String(error)).toBe('Error: Unauthorized'); + expect(error).toBeUndefined(); expect(mockAuthToolkit.authenticated).not.toHaveBeenCalled(); expect(mockResponse.redirected).not.toHaveBeenCalled(); diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index 548a09ac72be68..5652a3ccd9c8de 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -3,8 +3,6 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ - -import Boom from 'boom'; import { ClusterClient, CoreSetup, @@ -88,7 +86,7 @@ export async function setupAuthentication({ authenticationResult = await authenticator.authenticate(request); } catch (err) { authLogger.error(err); - return response.internalError(err); + return response.internalError(); } if (authenticationResult.succeeded()) { @@ -114,21 +112,25 @@ export async function setupAuthentication({ if (authenticationResult.failed()) { authLogger.info(`Authentication attempt failed: ${authenticationResult.error!.message}`); - const error = authenticationResult.error; - return getErrorStatusCode(error) === 400 - ? response.badRequest(error, { - headers: authenticationResult.authResponseHeaders, - }) - : response.unauthorized(error, { - headers: authenticationResult.authResponseHeaders, - }); - } else { - authLogger.info('Could not handle authentication attempt'); - const error = Boom.unauthorized(); - return response.unauthorized(error, { + const error = authenticationResult.error!; + // proxy Elasticsearch "native" errors + const statusCode = getErrorStatusCode(error); + if (typeof statusCode === 'number') { + return response.customError(error, { + statusCode, + headers: authenticationResult.authResponseHeaders, + }); + } + + return response.unauthorized(undefined, { headers: authenticationResult.authResponseHeaders, }); } + + authLogger.info('Could not handle authentication attempt'); + return response.unauthorized(undefined, { + headers: authenticationResult.authResponseHeaders, + }); }); authLogger.debug('Successfully registered core authentication handler.'); diff --git a/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts b/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts index 5bc422bca47f2b..0a3b7f595c66d3 100644 --- a/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts +++ b/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts @@ -145,7 +145,7 @@ export default function({ getService }: FtrProviderContext) { const spnegoResponse = await supertest .get('/api/security/v1/me') .set('Authorization', 'Negotiate (:I am malformed:)') - .expect(401); + .expect(500); expect(spnegoResponse.headers['set-cookie']).to.be(undefined); expect(spnegoResponse.headers['www-authenticate']).to.be(undefined); }); From 717d242baf79d1996c7039b3bca0d8c811ef9b77 Mon Sep 17 00:00:00 2001 From: restrry Date: Wed, 7 Aug 2019 10:55:10 +0200 Subject: [PATCH 12/14] update docs --- src/core/server/http/router/response.ts | 5 +++++ src/core/server/server.api.md | 1 + 2 files changed, 6 insertions(+) diff --git a/src/core/server/http/router/response.ts b/src/core/server/http/router/response.ts index 715215c5edb0b8..cd977e6b754ab5 100644 --- a/src/core/server/http/router/response.ts +++ b/src/core/server/http/router/response.ts @@ -180,6 +180,11 @@ const errorResponseFactory = { internalError: (error: ResponseError = 'Internal Error', options: HttpResponseOptions = {}) => new KibanaResponse(500, error, options), + /** + * Creates an error response with defined status code and payload. + * @param error - {@link ResponseError} Error object containing message and other error details to pass to the client + * @param options - {@link CustomHttpResponseOptions} configures HTTP response parameters. + */ customError: (error: ResponseError, options: CustomHttpResponseOptions) => { if (!options || !options.statusCode) { throw new Error(`options.statusCode is expected to be set. given options: ${options}`); diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 69c3215ffa7376..51d9ad49e485cc 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -295,6 +295,7 @@ export const kibanaResponseFactory: { notFound: (error?: ResponseError, options?: HttpResponseOptions) => KibanaResponse; conflict: (error?: ResponseError, options?: HttpResponseOptions) => KibanaResponse; internalError: (error?: ResponseError, options?: HttpResponseOptions) => KibanaResponse; + customError: (error: ResponseError, options: CustomHttpResponseOptions) => KibanaResponse; redirected: (payload: HttpResponsePayload, options: RedirectResponseOptions) => KibanaResponse | Buffer | Stream>; ok: (payload: HttpResponsePayload, options?: HttpResponseOptions) => KibanaResponse | Buffer | Stream>; accepted: (payload?: HttpResponsePayload, options?: HttpResponseOptions) => KibanaResponse | Buffer | Stream>; From 33e46e9248f61434b3cc8d575ede3c9e3a7ec9c3 Mon Sep 17 00:00:00 2001 From: restrry Date: Wed, 7 Aug 2019 13:44:06 +0200 Subject: [PATCH 13/14] support Hapi error format for BWC --- src/core/server/http/http_server.test.ts | 6 ++++ .../http/integration_tests/router.test.ts | 32 ++++++++++++++++--- .../server/http/router/response_adapter.ts | 23 +++++++------ .../apis/security/oidc_initiate_auth.js | 6 +++- .../apis/security/saml_login.js | 18 +++++++++-- 5 files changed, 68 insertions(+), 17 deletions(-) diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index 70022fab25a96f..f82c42317edb21 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -147,6 +147,8 @@ test('invalid params', async () => { .expect(400) .then(res => { expect(res.body).toEqual({ + error: 'Bad Request', + statusCode: 400, message: '[request params.test]: expected value of type [number] but got [string]', }); }); @@ -210,6 +212,8 @@ test('invalid query', async () => { .expect(400) .then(res => { expect(res.body).toEqual({ + error: 'Bad Request', + statusCode: 400, message: '[request query.bar]: expected value of type [number] but got [string]', }); }); @@ -278,6 +282,8 @@ test('invalid body', async () => { .expect(400) .then(res => { expect(res.body).toEqual({ + error: 'Bad Request', + statusCode: 400, message: '[request body.bar]: expected value of type [number] but got [string]', }); }); diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index abbcc39d000323..d0f697b5354574 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -146,7 +146,9 @@ describe('Handler', () => { .expect(400); expect(result.body).toEqual({ + error: 'Bad Request', message: '[request query.page]: expected value of type [number] but got [string]', + statusCode: 400, }); }); }); @@ -559,7 +561,11 @@ describe('Response factory', () => { .get('/') .expect(400); - expect(result.body).toEqual({ message: 'some message' }); + expect(result.body).toEqual({ + error: 'Bad Request', + message: 'some message', + statusCode: 400, + }); }); it('400 Bad request with default message', async () => { @@ -577,7 +583,11 @@ describe('Response factory', () => { .get('/') .expect(400); - expect(result.body).toEqual({ message: 'Bad Request' }); + expect(result.body).toEqual({ + error: 'Bad Request', + message: 'Bad Request', + statusCode: 400, + }); }); it('400 Bad request with additional data', async () => { @@ -596,10 +606,12 @@ describe('Response factory', () => { .expect(400); expect(result.body).toEqual({ + error: 'Bad Request', message: 'some message', meta: { data: ['good', 'bad'], }, + statusCode: 400, }); }); @@ -774,7 +786,11 @@ describe('Response factory', () => { .get('/') .expect(418); - expect(result.body).toEqual({ message: 'some message' }); + expect(result.body).toEqual({ + error: "I'm a teapot", + message: 'some message', + statusCode: 418, + }); }); it('Custom error response requires error status code', async () => { @@ -795,7 +811,11 @@ describe('Response factory', () => { .get('/') .expect(500); - expect(result.body).toEqual({ message: 'An internal server error occurred.' }); + expect(result.body).toEqual({ + error: 'Internal Server Error', + message: 'An internal server error occurred.', + statusCode: 500, + }); expect(loggingServiceMock.collect(logger).error).toMatchInlineSnapshot(` Array [ Array [ @@ -925,8 +945,10 @@ describe('Response factory', () => { .expect(401); expect(result.body).toEqual({ + error: 'Unauthorized', message: 'unauthorized', meta: { errorCode: 'K401' }, + statusCode: 401, }); }); @@ -954,8 +976,10 @@ describe('Response factory', () => { .expect(401); expect(result.body).toEqual({ + error: 'Unauthorized', message: 'unauthorized', meta: { errorCode: 'K401' }, + statusCode: 401, }); }); diff --git a/src/core/server/http/router/response_adapter.ts b/src/core/server/http/router/response_adapter.ts index a4c290598ea49a..89fdea1d67f67c 100644 --- a/src/core/server/http/router/response_adapter.ts +++ b/src/core/server/http/router/response_adapter.ts @@ -20,7 +20,13 @@ import { ResponseObject as HapiResponseObject, ResponseToolkit as HapiResponseTo import typeDetect from 'type-detect'; import Boom from 'boom'; -import { HttpResponsePayload, KibanaResponse, ResponseError } from './response'; +import { HttpResponsePayload, KibanaResponse, ResponseError, ResponseErrorMeta } from './response'; + +declare module 'boom' { + interface Payload { + meta?: ResponseErrorMeta; + } +} function setHeaders(response: HapiResponseObject, headers: Record = {}) { Object.entries(headers).forEach(([header, value]) => { @@ -41,7 +47,9 @@ const statusHelpers = { export class HapiResponseAdapter { constructor(private readonly responseToolkit: HapiResponseToolkit) {} public toBadRequest(message: string) { - return this.responseToolkit.response({ message }).code(400); + const error = Boom.badRequest(); + error.output.payload.message = message; + return error; } public toInternalError() { @@ -49,9 +57,7 @@ export class HapiResponseAdapter { statusCode: 500, }); - error.output.payload = { - message: 'An internal server error occurred.', - } as any; // our error format is not compatible with boom + error.output.payload.message = 'An internal server error occurred.'; return error; } @@ -109,14 +115,13 @@ export class HapiResponseAdapter { private toError(kibanaResponse: KibanaResponse) { const { payload } = kibanaResponse; + // we use for BWC with Boom payload for error responses - {error: string, message: string, statusCode: string} const error = new Boom('', { statusCode: kibanaResponse.status, }); - error.output.payload = { - message: getErrorMessage(payload), - meta: getErrorMeta(payload), - } as any; // our error format is not compatible with boom + error.output.payload.message = getErrorMessage(payload); + error.output.payload.meta = getErrorMeta(payload); const headers = kibanaResponse.options.headers; if (headers) { diff --git a/x-pack/test/oidc_api_integration/apis/security/oidc_initiate_auth.js b/x-pack/test/oidc_api_integration/apis/security/oidc_initiate_auth.js index 69b1277926834a..7460b1f9e238c2 100644 --- a/x-pack/test/oidc_api_integration/apis/security/oidc_initiate_auth.js +++ b/x-pack/test/oidc_api_integration/apis/security/oidc_initiate_auth.js @@ -341,7 +341,11 @@ export default function ({ getService }) { .set('Cookie', sessionCookie.cookieString()) .expect(400); - expect(apiResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); + expect(apiResponse.body).to.eql({ + error: 'Bad Request', + message: 'Both access and refresh tokens are expired.', + statusCode: 400 + }); }); it('should reject AJAX requests', async () => { diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.js b/x-pack/test/saml_api_integration/apis/security/saml_login.js index be4fef0f57ab30..401ffdea0418d4 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.js +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.js @@ -327,7 +327,11 @@ export default function ({ getService }) { .set('Cookie', sessionCookie.cookieString()) .expect(400); - expect(apiResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); + expect(apiResponse.body).to.eql({ + error: 'Bad Request', + message: 'Both access and refresh tokens are expired.', + statusCode: 400 + }); }); it('should redirect to home page if session cookie is not provided', async () => { @@ -380,7 +384,11 @@ export default function ({ getService }) { .set('Cookie', sessionCookie.cookieString()) .expect(400); - expect(apiResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); + expect(apiResponse.body).to.eql({ + error: 'Bad Request', + message: 'Both access and refresh tokens are expired.', + statusCode: 400 + }); }); it('should invalidate access token on IdP initiated logout even if there is no Kibana session', async () => { @@ -403,7 +411,11 @@ export default function ({ getService }) { .set('Cookie', sessionCookie.cookieString()) .expect(400); - expect(apiResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); + expect(apiResponse.body).to.eql({ + error: 'Bad Request', + message: 'Both access and refresh tokens are expired.', + statusCode: 400 + }); }); }); From 89c80c2487c8370a196eb6a19d51505d4fe8f8e3 Mon Sep 17 00:00:00 2001 From: restrry Date: Thu, 8 Aug 2019 10:52:08 +0200 Subject: [PATCH 14/14] add a couple of tests --- .../http/integration_tests/router.test.ts | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index d0f697b5354574..5f3976c7c1941c 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -793,6 +793,58 @@ describe('Response factory', () => { }); }); + it('Custom error response for server error', async () => { + const router = new Router('/'); + + router.get({ path: '/', validate: false }, (req, res) => { + const error = new Error('some message'); + + return res.customError(error, { + statusCode: 500, + }); + }); + + const { registerRouter, server: innerServer } = await server.setup(config); + registerRouter(router); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body).toEqual({ + error: 'Internal Server Error', + message: 'some message', + statusCode: 500, + }); + }); + + it('Custom error response for Boom server error', async () => { + const router = new Router('/'); + + router.get({ path: '/', validate: false }, (req, res) => { + const error = new Error('some message'); + + return res.customError(Boom.boomify(error), { + statusCode: 500, + }); + }); + + const { registerRouter, server: innerServer } = await server.setup(config); + registerRouter(router); + await server.start(); + + const result = await supertest(innerServer.listener) + .get('/') + .expect(500); + + expect(result.body).toEqual({ + error: 'Internal Server Error', + message: 'some message', + statusCode: 500, + }); + }); + it('Custom error response requires error status code', async () => { const router = new Router('/');