From 816a3f457bf3e2debae3b8981ee1393029308a4f Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 3 Jul 2019 11:28:02 -0400 Subject: [PATCH 1/4] allow SOC wrappers to be excluded --- .../service/lib/priority_collection.ts | 4 ++ .../lib/scoped_client_provider.test.js | 4 +- .../service/lib/scoped_client_provider.ts | 35 +++++++++++---- .../saved_objects/saved_objects_mixin.js | 4 +- .../encrypted_saved_objects/server/plugin.ts | 1 + x-pack/legacy/plugins/security/index.js | 2 +- .../spaces/server/new_platform/plugin.ts | 1 + .../spaces/server/routes/api/v1/example.ts | 43 +++++++++++++++++++ .../spaces/server/routes/api/v1/index.ts | 2 + 9 files changed, 83 insertions(+), 13 deletions(-) create mode 100644 x-pack/legacy/plugins/spaces/server/routes/api/v1/example.ts diff --git a/src/core/server/saved_objects/service/lib/priority_collection.ts b/src/core/server/saved_objects/service/lib/priority_collection.ts index 3c918f0c1e1fc..a2fe13b933347 100644 --- a/src/core/server/saved_objects/service/lib/priority_collection.ts +++ b/src/core/server/saved_objects/service/lib/priority_collection.ts @@ -38,6 +38,10 @@ export class PriorityCollection { this.array.splice(spliceIndex, 0, { priority, value }); } + public has(predicate: (value: T) => boolean): boolean { + return this.array.some(entry => predicate(entry.value)); + } + public toPrioritizedArray(): T[] { return this.array.map(entry => entry.value); } diff --git a/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js b/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js index d0cc2e6d97285..f109f31593c8b 100644 --- a/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js +++ b/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js @@ -76,8 +76,8 @@ test(`invokes and uses wrappers in specified order`, () => { const secondClientWrapperFactoryMock = jest.fn().mockReturnValue(secondWrapperClient); const request = Symbol(); - clientProvider.addClientWrapperFactory(1, secondClientWrapperFactoryMock); - clientProvider.addClientWrapperFactory(0, firstClientWrapperFactoryMock); + clientProvider.addClientWrapperFactory(1, 'foo', secondClientWrapperFactoryMock); + clientProvider.addClientWrapperFactory(0, 'bar', firstClientWrapperFactoryMock); const actualClient = clientProvider.getClient(request); expect(actualClient).toBe(firstWrappedClient); diff --git a/src/core/server/saved_objects/service/lib/scoped_client_provider.ts b/src/core/server/saved_objects/service/lib/scoped_client_provider.ts index 201b316005d7c..1d3ed34768f3d 100644 --- a/src/core/server/saved_objects/service/lib/scoped_client_provider.ts +++ b/src/core/server/saved_objects/service/lib/scoped_client_provider.ts @@ -32,13 +32,18 @@ export type SavedObjectsClientFactory = ( { request }: { request: Request } ) => SavedObjectsClientContract; +export interface SavedObjectsClientProviderOptions { + excludedWrappers?: string[]; +} + /** * Provider for the Scoped Saved Object Client. */ export class ScopedSavedObjectsClientProvider { - private readonly _wrapperFactories = new PriorityCollection< - SavedObjectsClientWrapperFactory - >(); + private readonly _wrapperFactories = new PriorityCollection<{ + id: string; + factory: SavedObjectsClientWrapperFactory; + }>(); private _clientFactory: SavedObjectsClientFactory; private readonly _originalClientFactory: SavedObjectsClientFactory; @@ -52,9 +57,14 @@ export class ScopedSavedObjectsClientProvider { addClientWrapperFactory( priority: number, - wrapperFactory: SavedObjectsClientWrapperFactory + id: string, + factory: SavedObjectsClientWrapperFactory ): void { - this._wrapperFactories.add(priority, wrapperFactory); + if (this._wrapperFactories.has(entry => entry.id === id)) { + throw new Error(`wrapper factory with id ${id} is already defined`); + } + + this._wrapperFactories.add(priority, { id, factory }); } setClientFactory(customClientFactory: SavedObjectsClientFactory) { @@ -65,15 +75,24 @@ export class ScopedSavedObjectsClientProvider { this._clientFactory = customClientFactory; } - getClient(request: Request): SavedObjectsClientContract { + getClient( + request: Request, + options: SavedObjectsClientProviderOptions = {} + ): SavedObjectsClientContract { const client = this._clientFactory({ request, }); + const excludedWrappers = options.excludedWrappers || []; + return this._wrapperFactories .toPrioritizedArray() - .reduceRight((clientToWrap, wrapperFactory) => { - return wrapperFactory({ + .reduceRight((clientToWrap, { id, factory }) => { + if (excludedWrappers.includes(id)) { + return clientToWrap; + } + + return factory({ request, client: clientToWrap, }); diff --git a/src/legacy/server/saved_objects/saved_objects_mixin.js b/src/legacy/server/saved_objects/saved_objects_mixin.js index 52845abbf08c2..29e2cb356da8e 100644 --- a/src/legacy/server/saved_objects/saved_objects_mixin.js +++ b/src/legacy/server/saved_objects/saved_objects_mixin.js @@ -148,14 +148,14 @@ export function savedObjectsMixin(kbnServer, server) { server.decorate('server', 'savedObjects', service); const savedObjectsClientCache = new WeakMap(); - server.decorate('request', 'getSavedObjectsClient', function() { + server.decorate('request', 'getSavedObjectsClient', function(options) { const request = this; if (savedObjectsClientCache.has(request)) { return savedObjectsClientCache.get(request); } - const savedObjectsClient = server.savedObjects.getScopedSavedObjectsClient(request); + const savedObjectsClient = server.savedObjects.getScopedSavedObjectsClient(request, options); savedObjectsClientCache.set(request, savedObjectsClient); return savedObjectsClient; diff --git a/x-pack/legacy/plugins/encrypted_saved_objects/server/plugin.ts b/x-pack/legacy/plugins/encrypted_saved_objects/server/plugin.ts index 02b20798afc59..0117e0a878848 100644 --- a/x-pack/legacy/plugins/encrypted_saved_objects/server/plugin.ts +++ b/x-pack/legacy/plugins/encrypted_saved_objects/server/plugin.ts @@ -60,6 +60,7 @@ export class Plugin { // `namespace` is included into AAD. core.savedObjects.addScopedSavedObjectsClientWrapperFactory( Number.MAX_SAFE_INTEGER, + 'encrypted_saved_objects', ({ client: baseClient }) => new EncryptedSavedObjectsClientWrapper({ baseClient, service }) ); diff --git a/x-pack/legacy/plugins/security/index.js b/x-pack/legacy/plugins/security/index.js index 250c8729b5466..61d60bf13391d 100644 --- a/x-pack/legacy/plugins/security/index.js +++ b/x-pack/legacy/plugins/security/index.js @@ -186,7 +186,7 @@ export const security = (kibana) => new kibana.Plugin({ return new savedObjects.SavedObjectsClient(callWithRequestRepository); }); - savedObjects.addScopedSavedObjectsClientWrapperFactory(Number.MIN_SAFE_INTEGER, ({ client, request }) => { + savedObjects.addScopedSavedObjectsClientWrapperFactory(Number.MIN_SAFE_INTEGER, 'security', ({ client, request }) => { if (authorization.mode.useRbacForRequest(request)) { return new SecureSavedObjectsClientWrapper({ actions: authorization.actions, diff --git a/x-pack/legacy/plugins/spaces/server/new_platform/plugin.ts b/x-pack/legacy/plugins/spaces/server/new_platform/plugin.ts index 5689804c125bd..6d6b8c83470be 100644 --- a/x-pack/legacy/plugins/spaces/server/new_platform/plugin.ts +++ b/x-pack/legacy/plugins/spaces/server/new_platform/plugin.ts @@ -120,6 +120,7 @@ export class Plugin { const { addScopedSavedObjectsClientWrapperFactory, types } = core.savedObjects; addScopedSavedObjectsClientWrapperFactory( Number.MAX_SAFE_INTEGER - 1, + 'spaces', spacesSavedObjectsClientWrapperFactory(spacesService, types) ); diff --git a/x-pack/legacy/plugins/spaces/server/routes/api/v1/example.ts b/x-pack/legacy/plugins/spaces/server/routes/api/v1/example.ts new file mode 100644 index 0000000000000..02adeeee79f6f --- /dev/null +++ b/x-pack/legacy/plugins/spaces/server/routes/api/v1/example.ts @@ -0,0 +1,43 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +import { InternalRouteDeps } from '.'; + +export function initExampleApi(deps: InternalRouteDeps) { + const { http, savedObjects, routePreCheckLicenseFn } = deps; + + http.route({ + method: 'GET', + path: '/api/spaces/v1/without_wrapper', + async handler(request: any) { + const { getScopedSavedObjectsClient } = savedObjects; + + const savedObjectsClient = getScopedSavedObjectsClient(request, { + excludedWrappers: ['spaces'], + }); + + // This wouldn't normally be possible if the spaces wrapper were still included + return await savedObjectsClient.get('space', 'default'); + }, + options: { + pre: [routePreCheckLicenseFn], + }, + }); + + http.route({ + method: 'GET', + path: '/api/spaces/v1/with_wrapper', + async handler(request: any) { + const { getScopedSavedObjectsClient } = savedObjects; + + const savedObjectsClient = getScopedSavedObjectsClient(request); + + return await savedObjectsClient.get('space', 'default'); + }, + options: { + pre: [routePreCheckLicenseFn], + }, + }); +} diff --git a/x-pack/legacy/plugins/spaces/server/routes/api/v1/index.ts b/x-pack/legacy/plugins/spaces/server/routes/api/v1/index.ts index 932c3c869af6b..aac7923cd45bf 100644 --- a/x-pack/legacy/plugins/spaces/server/routes/api/v1/index.ts +++ b/x-pack/legacy/plugins/spaces/server/routes/api/v1/index.ts @@ -11,6 +11,7 @@ import { routePreCheckLicense } from '../../../lib/route_pre_check_license'; import { initInternalSpacesApi } from './spaces'; import { SpacesServiceSetup } from '../../../new_platform/spaces_service/spaces_service'; import { SpacesHttpServiceSetup } from '../../../new_platform/plugin'; +import { initExampleApi } from './example'; type Omit = Pick>; @@ -35,4 +36,5 @@ export function initInternalApis({ xpackMain, ...rest }: RouteDeps) { }; initInternalSpacesApi(deps); + initExampleApi(deps); } From 09f805c7f767bb4532a8bc6760436ada5727e7cd Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 9 Jul 2019 11:04:13 -0400 Subject: [PATCH 2/4] remove example route --- .../spaces/server/routes/api/v1/example.ts | 43 ------------------- .../spaces/server/routes/api/v1/index.ts | 2 - 2 files changed, 45 deletions(-) delete mode 100644 x-pack/legacy/plugins/spaces/server/routes/api/v1/example.ts diff --git a/x-pack/legacy/plugins/spaces/server/routes/api/v1/example.ts b/x-pack/legacy/plugins/spaces/server/routes/api/v1/example.ts deleted file mode 100644 index 02adeeee79f6f..0000000000000 --- a/x-pack/legacy/plugins/spaces/server/routes/api/v1/example.ts +++ /dev/null @@ -1,43 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -import { InternalRouteDeps } from '.'; - -export function initExampleApi(deps: InternalRouteDeps) { - const { http, savedObjects, routePreCheckLicenseFn } = deps; - - http.route({ - method: 'GET', - path: '/api/spaces/v1/without_wrapper', - async handler(request: any) { - const { getScopedSavedObjectsClient } = savedObjects; - - const savedObjectsClient = getScopedSavedObjectsClient(request, { - excludedWrappers: ['spaces'], - }); - - // This wouldn't normally be possible if the spaces wrapper were still included - return await savedObjectsClient.get('space', 'default'); - }, - options: { - pre: [routePreCheckLicenseFn], - }, - }); - - http.route({ - method: 'GET', - path: '/api/spaces/v1/with_wrapper', - async handler(request: any) { - const { getScopedSavedObjectsClient } = savedObjects; - - const savedObjectsClient = getScopedSavedObjectsClient(request); - - return await savedObjectsClient.get('space', 'default'); - }, - options: { - pre: [routePreCheckLicenseFn], - }, - }); -} diff --git a/x-pack/legacy/plugins/spaces/server/routes/api/v1/index.ts b/x-pack/legacy/plugins/spaces/server/routes/api/v1/index.ts index aac7923cd45bf..932c3c869af6b 100644 --- a/x-pack/legacy/plugins/spaces/server/routes/api/v1/index.ts +++ b/x-pack/legacy/plugins/spaces/server/routes/api/v1/index.ts @@ -11,7 +11,6 @@ import { routePreCheckLicense } from '../../../lib/route_pre_check_license'; import { initInternalSpacesApi } from './spaces'; import { SpacesServiceSetup } from '../../../new_platform/spaces_service/spaces_service'; import { SpacesHttpServiceSetup } from '../../../new_platform/plugin'; -import { initExampleApi } from './example'; type Omit = Pick>; @@ -36,5 +35,4 @@ export function initInternalApis({ xpackMain, ...rest }: RouteDeps) { }; initInternalSpacesApi(deps); - initExampleApi(deps); } From de50cf406109c956a4b2d39471bda1f520fdf50a Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 9 Jul 2019 11:04:33 -0400 Subject: [PATCH 3/4] add tests --- .../service/lib/priority_collection.test.ts | 12 +++++ .../lib/scoped_client_provider.test.js | 51 +++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/src/core/server/saved_objects/service/lib/priority_collection.test.ts b/src/core/server/saved_objects/service/lib/priority_collection.test.ts index 9256b2e913931..13180b912e7d7 100644 --- a/src/core/server/saved_objects/service/lib/priority_collection.test.ts +++ b/src/core/server/saved_objects/service/lib/priority_collection.test.ts @@ -55,3 +55,15 @@ test(`1, 1 throws Error`, () => { priorityCollection.add(1, 1); expect(() => priorityCollection.add(1, 1)).toThrowErrorMatchingSnapshot(); }); + +test(`#has when empty returns false`, () => { + const priorityCollection = new PriorityCollection(); + expect(priorityCollection.has(() => true)).toEqual(false); +}); + +test(`#has returns result of predicate`, () => { + const priorityCollection = new PriorityCollection(); + priorityCollection.add(1, 'foo'); + expect(priorityCollection.has(val => val === 'foo')).toEqual(true); + expect(priorityCollection.has(val => val === 'bar')).toEqual(false); +}); diff --git a/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js b/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js index f109f31593c8b..8084273ca36ed 100644 --- a/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js +++ b/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js @@ -90,3 +90,54 @@ test(`invokes and uses wrappers in specified order`, () => { client: defaultClient, }); }); + +test(`does not invoke or use excluded wrappers`, () => { + const defaultClient = Symbol(); + const defaultClientFactoryMock = jest.fn().mockReturnValue(defaultClient); + const clientProvider = new ScopedSavedObjectsClientProvider({ + defaultClientFactory: defaultClientFactoryMock, + }); + const firstWrappedClient = Symbol('first client'); + const firstClientWrapperFactoryMock = jest.fn().mockReturnValue(firstWrappedClient); + const secondWrapperClient = Symbol('second client'); + const secondClientWrapperFactoryMock = jest.fn().mockReturnValue(secondWrapperClient); + const request = Symbol(); + + clientProvider.addClientWrapperFactory(1, 'foo', secondClientWrapperFactoryMock); + clientProvider.addClientWrapperFactory(0, 'bar', firstClientWrapperFactoryMock); + + const actualClient = clientProvider.getClient(request, { + excludedWrappers: ['foo'] + }); + + expect(actualClient).toBe(firstWrappedClient); + expect(firstClientWrapperFactoryMock).toHaveBeenCalledWith({ + request, + client: defaultClient, + }); + expect(secondClientWrapperFactoryMock).not.toHaveBeenCalled(); +}); + +test(`allows all wrappers to be excluded`, () => { + const defaultClient = Symbol(); + const defaultClientFactoryMock = jest.fn().mockReturnValue(defaultClient); + const clientProvider = new ScopedSavedObjectsClientProvider({ + defaultClientFactory: defaultClientFactoryMock, + }); + const firstWrappedClient = Symbol('first client'); + const firstClientWrapperFactoryMock = jest.fn().mockReturnValue(firstWrappedClient); + const secondWrapperClient = Symbol('second client'); + const secondClientWrapperFactoryMock = jest.fn().mockReturnValue(secondWrapperClient); + const request = Symbol(); + + clientProvider.addClientWrapperFactory(1, 'foo', secondClientWrapperFactoryMock); + clientProvider.addClientWrapperFactory(0, 'bar', firstClientWrapperFactoryMock); + + const actualClient = clientProvider.getClient(request, { + excludedWrappers: ['foo', 'bar'] + }); + + expect(actualClient).toBe(defaultClient); + expect(firstClientWrapperFactoryMock).not.toHaveBeenCalled(); + expect(secondClientWrapperFactoryMock).not.toHaveBeenCalled(); +}); From 42fd42a168515442bed45872394a046271a11565 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 9 Jul 2019 11:17:52 -0400 Subject: [PATCH 4/4] more testing --- .../service/lib/scoped_client_provider.test.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js b/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js index 8084273ca36ed..bce76bc7841e6 100644 --- a/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js +++ b/src/core/server/saved_objects/service/lib/scoped_client_provider.test.js @@ -64,6 +64,20 @@ test(`throws error when more than one scoped saved objects client factory is set }).toThrowErrorMatchingSnapshot(); }); +test(`throws error when registering a wrapper with a duplicate id`, () => { + const defaultClientFactoryMock = jest.fn(); + const clientProvider = new ScopedSavedObjectsClientProvider({ + defaultClientFactory: defaultClientFactoryMock, + }); + const firstClientWrapperFactoryMock = jest.fn(); + const secondClientWrapperFactoryMock = jest.fn(); + + clientProvider.addClientWrapperFactory(1, 'foo', secondClientWrapperFactoryMock); + expect(() => + clientProvider.addClientWrapperFactory(0, 'foo', firstClientWrapperFactoryMock) + ).toThrowErrorMatchingInlineSnapshot(`"wrapper factory with id foo is already defined"`); +}); + test(`invokes and uses wrappers in specified order`, () => { const defaultClient = Symbol(); const defaultClientFactoryMock = jest.fn().mockReturnValue(defaultClient); @@ -107,7 +121,7 @@ test(`does not invoke or use excluded wrappers`, () => { clientProvider.addClientWrapperFactory(0, 'bar', firstClientWrapperFactoryMock); const actualClient = clientProvider.getClient(request, { - excludedWrappers: ['foo'] + excludedWrappers: ['foo'], }); expect(actualClient).toBe(firstWrappedClient); @@ -134,7 +148,7 @@ test(`allows all wrappers to be excluded`, () => { clientProvider.addClientWrapperFactory(0, 'bar', firstClientWrapperFactoryMock); const actualClient = clientProvider.getClient(request, { - excludedWrappers: ['foo', 'bar'] + excludedWrappers: ['foo', 'bar'], }); expect(actualClient).toBe(defaultClient);