diff --git a/src/core/server/http/cookie_session_storage.ts b/src/core/server/http/cookie_session_storage.ts index 559ab9137164f9..5983221ea140ac 100644 --- a/src/core/server/http/cookie_session_storage.ts +++ b/src/core/server/http/cookie_session_storage.ts @@ -20,6 +20,7 @@ import { Request, Server } from 'hapi'; import hapiAuthCookie from 'hapi-auth-cookie'; +import { Logger } from '..'; import { KibanaRequest, ensureRawRequest } from './router'; import { SessionStorageFactory, SessionStorage } from './session_storage'; @@ -31,11 +32,32 @@ export interface SessionStorageCookieOptions { } class ScopedCookieSessionStorage> implements SessionStorage { - constructor(private readonly server: Server, private readonly request: Request) {} + constructor( + private readonly log: Logger, + private readonly server: Server, + private readonly request: Request + ) {} public async get(): Promise { try { - return await this.server.auth.test('security-cookie', this.request as Request); + const session = await this.server.auth.test('security-cookie', this.request); + // A browser can send several cookies, if it's not an array, just return the session value + if (!Array.isArray(session)) { + return session as T; + } + + // If we have an array with one value, we're good also + if (session.length === 1) { + return session[0] as T; + } + + // Otherwise, we have more than one and won't be authing the user because we don't + // know which session identifies the actual user. There's potential to change this behavior + // to ensure all valid sessions identify the same user, or choose one valid one, but this + // is the safest option. + this.log.warn(`Found ${session.length} auth sessions when we were only expecting 1.`); + return null; } catch (error) { + this.log.debug(String(error)); return null; } } @@ -55,6 +77,7 @@ class ScopedCookieSessionStorage> implements Sessi * @param cookieOptions - cookies configuration */ export async function createCookieSessionStorageFactory( + log: Logger, server: Server, cookieOptions: SessionStorageCookieOptions, basePath?: string @@ -74,7 +97,7 @@ export async function createCookieSessionStorageFactory( return { asScoped(request: KibanaRequest) { - return new ScopedCookieSessionStorage(server, ensureRawRequest(request)); + return new ScopedCookieSessionStorage(log, server, ensureRawRequest(request)); }, }; } diff --git a/src/core/server/http/cookie_sesson_storage.test.ts b/src/core/server/http/cookie_sesson_storage.test.ts index 02ce240659a00f..e2cc4e5abec85e 100644 --- a/src/core/server/http/cookie_sesson_storage.test.ts +++ b/src/core/server/http/cookie_sesson_storage.test.ts @@ -22,14 +22,15 @@ import { ByteSizeValue } from '@kbn/config-schema'; import { HttpServer } from './http_server'; import { HttpConfig } from './http_config'; -import { Router } from './router'; +import { Router, KibanaRequest } from './router'; import { loggingServiceMock } from '../logging/logging_service.mock'; +import { httpServerMock } from './http_server.mocks'; import { createCookieSessionStorageFactory } from './cookie_session_storage'; let server: HttpServer; -const logger = loggingServiceMock.create(); +let logger: ReturnType; const config = { host: '127.0.0.1', maxPayload: new ByteSizeValue(1024), @@ -37,7 +38,8 @@ const config = { } as HttpConfig; beforeEach(() => { - server = new HttpServer(logger.get()); + logger = loggingServiceMock.create(); + server = new HttpServer(logger, 'tests'); }); afterEach(async () => { @@ -87,7 +89,11 @@ describe('Cookie based SessionStorage', () => { const { registerRouter, server: innerServer } = await server.setup(config); registerRouter(router); - const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions); + const factory = await createCookieSessionStorageFactory( + logger.get(), + innerServer, + cookieOptions + ); await server.start(); const response = await supertest(innerServer.listener) @@ -107,7 +113,7 @@ describe('Cookie based SessionStorage', () => { }); }); describe('#get()', () => { - it('Should read from session storage', async () => { + it('reads from session storage', async () => { const router = new Router(''); router.get({ path: '/', validate: false }, async (req, res) => { @@ -123,7 +129,11 @@ describe('Cookie based SessionStorage', () => { const { registerRouter, server: innerServer } = await server.setup(config); registerRouter(router); - const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions); + const factory = await createCookieSessionStorageFactory( + logger.get(), + innerServer, + cookieOptions + ); await server.start(); const response = await supertest(innerServer.listener) @@ -141,7 +151,7 @@ describe('Cookie based SessionStorage', () => { .set('Cookie', `${sessionCookie.key}=${sessionCookie.value}`) .expect(200, { value: userData }); }); - it('Should return null for empty session', async () => { + it('returns null for empty session', async () => { const router = new Router(''); router.get({ path: '/', validate: false }, async (req, res) => { @@ -153,7 +163,11 @@ describe('Cookie based SessionStorage', () => { const { registerRouter, server: innerServer } = await server.setup(config); registerRouter(router); - const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions); + const factory = await createCookieSessionStorageFactory( + logger.get(), + innerServer, + cookieOptions + ); await server.start(); const response = await supertest(innerServer.listener) @@ -163,7 +177,8 @@ describe('Cookie based SessionStorage', () => { const cookies = response.get('set-cookie'); expect(cookies).not.toBeDefined(); }); - it('Should return null for invalid session & clean cookies', async () => { + + it('returns null for invalid session & clean cookies', async () => { const router = new Router(''); let setOnce = false; @@ -181,7 +196,11 @@ describe('Cookie based SessionStorage', () => { const { registerRouter, server: innerServer } = await server.setup(config); registerRouter(router); - const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions); + const factory = await createCookieSessionStorageFactory( + logger.get(), + innerServer, + cookieOptions + ); await server.start(); const response = await supertest(innerServer.listener) @@ -204,9 +223,96 @@ describe('Cookie based SessionStorage', () => { 'sid=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; Path=/', ]); }); + // use mocks to simplify test setup + it('returns null if multiple session cookies are detected.', async () => { + const mockServer = { + register: jest.fn(), + auth: { + strategy: jest.fn(), + test: jest.fn(() => ['foo', 'bar']), + }, + }; + + const mockRequest = httpServerMock.createRawRequest(); + + const factory = await createCookieSessionStorageFactory( + logger.get(), + mockServer as any, + cookieOptions + ); + + expect(mockServer.register).toBeCalledTimes(1); + expect(mockServer.auth.strategy).toBeCalledTimes(1); + + const session = await factory.asScoped(KibanaRequest.from(mockRequest)).get(); + expect(session).toBe(null); + + expect(mockServer.auth.test).toBeCalledTimes(1); + expect(mockServer.auth.test).toHaveBeenCalledWith('security-cookie', mockRequest); + + expect(loggingServiceMock.collect(logger).warn).toEqual([ + ['Found 2 auth sessions when we were only expecting 1.'], + ]); + }); + + it('returns session if single session cookie is in an array.', async () => { + const mockServer = { + register: jest.fn(), + auth: { + strategy: jest.fn(), + test: jest.fn(() => ['foo']), + }, + }; + + const mockRequest = httpServerMock.createRawRequest(); + + const factory = await createCookieSessionStorageFactory( + logger.get(), + mockServer as any, + cookieOptions + ); + + expect(mockServer.register).toBeCalledTimes(1); + expect(mockServer.auth.strategy).toBeCalledTimes(1); + + const session = await factory.asScoped(KibanaRequest.from(mockRequest)).get(); + expect(session).toBe('foo'); + + expect(mockServer.auth.test).toBeCalledTimes(1); + expect(mockServer.auth.test).toHaveBeenCalledWith('security-cookie', mockRequest); + }); + + it('logs the reason of validation function failure.', async () => { + const mockServer = { + register: jest.fn(), + auth: { + strategy: jest.fn(), + test: () => { + throw new Error('Invalid cookie.'); + }, + }, + }; + + const mockRequest = httpServerMock.createRawRequest(); + + const factory = await createCookieSessionStorageFactory( + logger.get(), + mockServer as any, + cookieOptions + ); + + expect(mockServer.register).toBeCalledTimes(1); + expect(mockServer.auth.strategy).toBeCalledTimes(1); + + const session = await factory.asScoped(KibanaRequest.from(mockRequest)).get(); + expect(session).toBe(null); + + expect(loggingServiceMock.collect(logger).debug).toEqual([['Error: Invalid cookie.']]); + }); }); + describe('#clear()', () => { - it('Should clear session storage & remove cookies', async () => { + it('clears session storage & remove cookies', async () => { const router = new Router(''); router.get({ path: '/', validate: false }, async (req, res) => { @@ -222,7 +328,11 @@ describe('Cookie based SessionStorage', () => { const { registerRouter, server: innerServer } = await server.setup(config); registerRouter(router); - const factory = await createCookieSessionStorageFactory(innerServer, cookieOptions); + const factory = await createCookieSessionStorageFactory( + logger.get(), + innerServer, + cookieOptions + ); await server.start(); const response = await supertest(innerServer.listener) diff --git a/src/core/server/http/http_server.mocks.ts b/src/core/server/http/http_server.mocks.ts index 5a916c6890d284..df1620b0218362 100644 --- a/src/core/server/http/http_server.mocks.ts +++ b/src/core/server/http/http_server.mocks.ts @@ -19,6 +19,8 @@ import { Request, ResponseToolkit } from 'hapi'; import { merge } from 'lodash'; +import { KibanaRequest } from './router'; + type DeepPartial = T extends any[] ? DeepPartialArray : T extends object @@ -52,6 +54,7 @@ function createRawResponseToolkitMock(customization: DeepPartial KibanaRequest.from(createRawRequestMock()), createRawRequest: createRawRequestMock, createRawResponseToolkit: createRawResponseToolkitMock, }; diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index 3b5ffeaa95701a..69d2eb53e1674b 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -47,7 +47,7 @@ beforeEach(() => { ssl: {}, } as HttpConfig; - server = new HttpServer(logger.get()); + server = new HttpServer(logger, 'tests'); }); afterEach(async () => { diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index eb571bdb47ddd1..3a495fb78e2754 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -19,7 +19,7 @@ import { Request, Server } from 'hapi'; -import { Logger } from '../logging'; +import { Logger, LoggerFactory } from '../logging'; import { HttpConfig } from './http_config'; import { createServer, getServerOptions } from './http_tools'; import { adoptToHapiAuthFormat, AuthenticationHandler } from './lifecycle/auth'; @@ -83,12 +83,14 @@ export class HttpServer { private registeredRouters = new Set(); private authRegistered = false; + private readonly log: Logger; private readonly authState: AuthStateStorage; private readonly authHeaders: AuthHeadersStorage; - constructor(private readonly log: Logger) { + constructor(private readonly logger: LoggerFactory, private readonly name: string) { this.authState = new AuthStateStorage(() => this.authRegistered); this.authHeaders = new AuthHeadersStorage(); + this.log = logger.get('http', 'server', name); } public isListening() { @@ -221,6 +223,7 @@ export class HttpServer { this.authRegistered = true; const sessionStorageFactory = await createCookieSessionStorageFactory( + this.logger.get('http', 'server', this.name, 'cookie-session-storage'), this.server, cookieOptions, basePath diff --git a/src/core/server/http/http_service.ts b/src/core/server/http/http_service.ts index a056300f6ed7ee..b06c690cf26216 100644 --- a/src/core/server/http/http_service.ts +++ b/src/core/server/http/http_service.ts @@ -58,7 +58,7 @@ export class HttpService implements CoreService('server') .pipe(map(rawConfig => new HttpConfig(rawConfig, coreContext.env))); - this.httpServer = new HttpServer(coreContext.logger.get('http', 'server')); + this.httpServer = new HttpServer(coreContext.logger, 'Kibana'); this.httpsRedirectServer = new HttpsRedirectServer( coreContext.logger.get('http', 'redirect', 'server') ); @@ -148,9 +148,8 @@ export class HttpService implements CoreService