From 09ba30e1cb81ad11ad33b777102a9b90933c726e Mon Sep 17 00:00:00 2001 From: Pierre Gayvallet Date: Mon, 23 Aug 2021 12:02:41 +0200 Subject: [PATCH] Add multiple namespaces support to PIT search and finder (#109062) * initial modifications * change approach for openPointInTime and add tests for spaces wrapper changes * fix and add security wrapper tests * fix export security FTR tests * update generated doc * add tests for PIT finder * NIT * improve doc * nits --- ...rver.savedobjectsopenpointintimeoptions.md | 3 +- ...bjectsopenpointintimeoptions.namespaces.md | 15 ++ .../service/lib/point_in_time_finder.test.ts | 222 +++++++++--------- .../service/lib/point_in_time_finder.ts | 4 +- .../service/saved_objects_client.ts | 11 +- src/core/server/server.api.md | 3 +- ...ecure_saved_objects_client_wrapper.test.ts | 116 +++++++-- .../secure_saved_objects_client_wrapper.ts | 31 +-- .../spaces_saved_objects_client.test.ts | 85 ++++++- .../spaces_saved_objects_client.ts | 63 ++--- .../common/suites/export.ts | 12 +- .../security_and_spaces/apis/export.ts | 2 +- .../security_only/apis/export.ts | 2 +- 13 files changed, 384 insertions(+), 185 deletions(-) create mode 100644 docs/development/core/server/kibana-plugin-core-server.savedobjectsopenpointintimeoptions.namespaces.md diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsopenpointintimeoptions.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsopenpointintimeoptions.md index 46516be2329e9..fc825e3bf2937 100644 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectsopenpointintimeoptions.md +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsopenpointintimeoptions.md @@ -8,7 +8,7 @@ Signature: ```typescript -export interface SavedObjectsOpenPointInTimeOptions extends SavedObjectsBaseOptions +export interface SavedObjectsOpenPointInTimeOptions ``` ## Properties @@ -16,5 +16,6 @@ export interface SavedObjectsOpenPointInTimeOptions extends SavedObjectsBaseOpti | Property | Type | Description | | --- | --- | --- | | [keepAlive](./kibana-plugin-core-server.savedobjectsopenpointintimeoptions.keepalive.md) | string | Optionally specify how long ES should keep the PIT alive until the next request. Defaults to 5m. | +| [namespaces](./kibana-plugin-core-server.savedobjectsopenpointintimeoptions.namespaces.md) | string[] | An optional list of namespaces to be used when opening the PIT.When the spaces plugin is enabled: - this will default to the user's current space (as determined by the URL) - if specified, the user's current space will be ignored - ['*'] will search across all available spaces | | [preference](./kibana-plugin-core-server.savedobjectsopenpointintimeoptions.preference.md) | string | An optional ES preference value to be used for the query. | diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsopenpointintimeoptions.namespaces.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsopenpointintimeoptions.namespaces.md new file mode 100644 index 0000000000000..06fb7519d52c2 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsopenpointintimeoptions.namespaces.md @@ -0,0 +1,15 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [SavedObjectsOpenPointInTimeOptions](./kibana-plugin-core-server.savedobjectsopenpointintimeoptions.md) > [namespaces](./kibana-plugin-core-server.savedobjectsopenpointintimeoptions.namespaces.md) + +## SavedObjectsOpenPointInTimeOptions.namespaces property + +An optional list of namespaces to be used when opening the PIT. + +When the spaces plugin is enabled: - this will default to the user's current space (as determined by the URL) - if specified, the user's current space will be ignored - `['*']` will search across all available spaces + +Signature: + +```typescript +namespaces?: string[]; +``` diff --git a/src/core/server/saved_objects/service/lib/point_in_time_finder.test.ts b/src/core/server/saved_objects/service/lib/point_in_time_finder.test.ts index 044bb45269538..160852f9160b7 100644 --- a/src/core/server/saved_objects/service/lib/point_in_time_finder.test.ts +++ b/src/core/server/saved_objects/service/lib/point_in_time_finder.test.ts @@ -7,7 +7,6 @@ */ import { loggerMock, MockedLogger } from '../../../logging/logger.mock'; -import type { SavedObjectsClientContract } from '../../types'; import type { SavedObjectsFindResult } from '../'; import { savedObjectsRepositoryMock } from './repository.mock'; @@ -43,37 +42,67 @@ const mockHits = [ describe('createPointInTimeFinder()', () => { let logger: MockedLogger; - let find: jest.Mocked['find']; - let openPointInTimeForType: jest.Mocked['openPointInTimeForType']; - let closePointInTime: jest.Mocked['closePointInTime']; + let repository: ReturnType; beforeEach(() => { logger = loggerMock.create(); - const mockRepository = savedObjectsRepositoryMock.create(); - find = mockRepository.find; - openPointInTimeForType = mockRepository.openPointInTimeForType; - closePointInTime = mockRepository.closePointInTime; + repository = savedObjectsRepositoryMock.create(); }); describe('#find', () => { - test('throws if a PIT is already open', async () => { - openPointInTimeForType.mockResolvedValueOnce({ + test('opens a PIT with the correct parameters', async () => { + repository.openPointInTimeForType.mockResolvedValueOnce({ id: 'abc123', }); - find.mockResolvedValueOnce({ + repository.find.mockResolvedValue({ total: 2, saved_objects: mockHits, pit_id: 'abc123', per_page: 1, page: 0, }); - find.mockResolvedValueOnce({ - total: 2, - saved_objects: mockHits, - pit_id: 'abc123', - per_page: 1, - page: 1, + + const findOptions: SavedObjectsCreatePointInTimeFinderOptions = { + type: ['visualization'], + search: 'foo*', + perPage: 1, + namespaces: ['ns1', 'ns2'], + }; + + const finder = new PointInTimeFinder(findOptions, { + logger, + client: repository, + }); + + expect(repository.openPointInTimeForType).not.toHaveBeenCalled(); + + await finder.find().next(); + + expect(repository.openPointInTimeForType).toHaveBeenCalledTimes(1); + expect(repository.openPointInTimeForType).toHaveBeenCalledWith(findOptions.type, { + namespaces: findOptions.namespaces, }); + }); + + test('throws if a PIT is already open', async () => { + repository.openPointInTimeForType.mockResolvedValueOnce({ + id: 'abc123', + }); + repository.find + .mockResolvedValueOnce({ + total: 2, + saved_objects: mockHits, + pit_id: 'abc123', + per_page: 1, + page: 0, + }) + .mockResolvedValueOnce({ + total: 2, + saved_objects: mockHits, + pit_id: 'abc123', + per_page: 1, + page: 1, + }); const findOptions: SavedObjectsCreatePointInTimeFinderOptions = { type: ['visualization'], @@ -83,30 +112,25 @@ describe('createPointInTimeFinder()', () => { const finder = new PointInTimeFinder(findOptions, { logger, - client: { - find, - openPointInTimeForType, - closePointInTime, - }, + client: repository, }); await finder.find().next(); - expect(find).toHaveBeenCalledTimes(1); - find.mockClear(); + expect(repository.find).toHaveBeenCalledTimes(1); expect(async () => { await finder.find().next(); }).rejects.toThrowErrorMatchingInlineSnapshot( `"Point In Time has already been opened for this finder instance. Please call \`close()\` before calling \`find()\` again."` ); - expect(find).toHaveBeenCalledTimes(0); + expect(repository.find).toHaveBeenCalledTimes(1); }); test('works with a single page of results', async () => { - openPointInTimeForType.mockResolvedValueOnce({ + repository.openPointInTimeForType.mockResolvedValueOnce({ id: 'abc123', }); - find.mockResolvedValueOnce({ + repository.find.mockResolvedValueOnce({ total: 2, saved_objects: mockHits, pit_id: 'abc123', @@ -121,11 +145,7 @@ describe('createPointInTimeFinder()', () => { const finder = new PointInTimeFinder(findOptions, { logger, - client: { - find, - openPointInTimeForType, - closePointInTime, - }, + client: repository, }); const hits: SavedObjectsFindResult[] = []; for await (const result of finder.find()) { @@ -133,10 +153,10 @@ describe('createPointInTimeFinder()', () => { } expect(hits.length).toBe(2); - expect(openPointInTimeForType).toHaveBeenCalledTimes(1); - expect(closePointInTime).toHaveBeenCalledTimes(1); - expect(find).toHaveBeenCalledTimes(1); - expect(find).toHaveBeenCalledWith( + expect(repository.openPointInTimeForType).toHaveBeenCalledTimes(1); + expect(repository.closePointInTime).toHaveBeenCalledTimes(1); + expect(repository.find).toHaveBeenCalledTimes(1); + expect(repository.find).toHaveBeenCalledWith( expect.objectContaining({ pit: expect.objectContaining({ id: 'abc123', keepAlive: '2m' }), sortField: 'updated_at', @@ -147,24 +167,25 @@ describe('createPointInTimeFinder()', () => { }); test('works with multiple pages of results', async () => { - openPointInTimeForType.mockResolvedValueOnce({ + repository.openPointInTimeForType.mockResolvedValueOnce({ id: 'abc123', }); - find.mockResolvedValueOnce({ - total: 2, - saved_objects: [mockHits[0]], - pit_id: 'abc123', - per_page: 1, - page: 0, - }); - find.mockResolvedValueOnce({ - total: 2, - saved_objects: [mockHits[1]], - pit_id: 'abc123', - per_page: 1, - page: 0, - }); - find.mockResolvedValueOnce({ + repository.find + .mockResolvedValueOnce({ + total: 2, + saved_objects: [mockHits[0]], + pit_id: 'abc123', + per_page: 1, + page: 0, + }) + .mockResolvedValueOnce({ + total: 2, + saved_objects: [mockHits[1]], + pit_id: 'abc123', + per_page: 1, + page: 0, + }); + repository.find.mockResolvedValueOnce({ total: 2, saved_objects: [], per_page: 1, @@ -180,11 +201,7 @@ describe('createPointInTimeFinder()', () => { const finder = new PointInTimeFinder(findOptions, { logger, - client: { - find, - openPointInTimeForType, - closePointInTime, - }, + client: repository, }); const hits: SavedObjectsFindResult[] = []; for await (const result of finder.find()) { @@ -192,12 +209,12 @@ describe('createPointInTimeFinder()', () => { } expect(hits.length).toBe(2); - expect(openPointInTimeForType).toHaveBeenCalledTimes(1); - expect(closePointInTime).toHaveBeenCalledTimes(1); + expect(repository.openPointInTimeForType).toHaveBeenCalledTimes(1); + expect(repository.closePointInTime).toHaveBeenCalledTimes(1); // called 3 times since we need a 3rd request to check if we // are done paginating through results. - expect(find).toHaveBeenCalledTimes(3); - expect(find).toHaveBeenCalledWith( + expect(repository.find).toHaveBeenCalledTimes(3); + expect(repository.find).toHaveBeenCalledWith( expect.objectContaining({ pit: expect.objectContaining({ id: 'abc123', keepAlive: '2m' }), sortField: 'updated_at', @@ -210,10 +227,10 @@ describe('createPointInTimeFinder()', () => { describe('#close', () => { test('calls closePointInTime with correct ID', async () => { - openPointInTimeForType.mockResolvedValueOnce({ + repository.openPointInTimeForType.mockResolvedValueOnce({ id: 'test', }); - find.mockResolvedValueOnce({ + repository.find.mockResolvedValueOnce({ total: 1, saved_objects: [mockHits[0]], pit_id: 'test', @@ -229,11 +246,7 @@ describe('createPointInTimeFinder()', () => { const finder = new PointInTimeFinder(findOptions, { logger, - client: { - find, - openPointInTimeForType, - closePointInTime, - }, + client: repository, }); const hits: SavedObjectsFindResult[] = []; for await (const result of finder.find()) { @@ -241,28 +254,28 @@ describe('createPointInTimeFinder()', () => { await finder.close(); } - expect(closePointInTime).toHaveBeenCalledWith('test'); + expect(repository.closePointInTime).toHaveBeenCalledWith('test'); }); test('causes generator to stop', async () => { - openPointInTimeForType.mockResolvedValueOnce({ + repository.openPointInTimeForType.mockResolvedValueOnce({ id: 'test', }); - find.mockResolvedValueOnce({ + repository.find.mockResolvedValueOnce({ total: 2, saved_objects: [mockHits[0]], pit_id: 'test', per_page: 1, page: 0, }); - find.mockResolvedValueOnce({ + repository.find.mockResolvedValueOnce({ total: 2, saved_objects: [mockHits[1]], pit_id: 'test', per_page: 1, page: 0, }); - find.mockResolvedValueOnce({ + repository.find.mockResolvedValueOnce({ total: 2, saved_objects: [], per_page: 1, @@ -278,11 +291,7 @@ describe('createPointInTimeFinder()', () => { const finder = new PointInTimeFinder(findOptions, { logger, - client: { - find, - openPointInTimeForType, - closePointInTime, - }, + client: repository, }); const hits: SavedObjectsFindResult[] = []; for await (const result of finder.find()) { @@ -290,15 +299,15 @@ describe('createPointInTimeFinder()', () => { await finder.close(); } - expect(closePointInTime).toHaveBeenCalledTimes(1); + expect(repository.closePointInTime).toHaveBeenCalledTimes(1); expect(hits.length).toBe(1); }); test('is called if `find` throws an error', async () => { - openPointInTimeForType.mockResolvedValueOnce({ + repository.openPointInTimeForType.mockResolvedValueOnce({ id: 'test', }); - find.mockRejectedValueOnce(new Error('oops')); + repository.find.mockRejectedValueOnce(new Error('oops')); const findOptions: SavedObjectsCreatePointInTimeFinderOptions = { type: ['visualization'], @@ -308,11 +317,7 @@ describe('createPointInTimeFinder()', () => { const finder = new PointInTimeFinder(findOptions, { logger, - client: { - find, - openPointInTimeForType, - closePointInTime, - }, + client: repository, }); const hits: SavedObjectsFindResult[] = []; try { @@ -323,27 +328,28 @@ describe('createPointInTimeFinder()', () => { // intentionally empty } - expect(closePointInTime).toHaveBeenCalledWith('test'); + expect(repository.closePointInTime).toHaveBeenCalledWith('test'); }); test('finder can be reused after closing', async () => { - openPointInTimeForType.mockResolvedValueOnce({ + repository.openPointInTimeForType.mockResolvedValueOnce({ id: 'abc123', }); - find.mockResolvedValueOnce({ - total: 2, - saved_objects: mockHits, - pit_id: 'abc123', - per_page: 1, - page: 0, - }); - find.mockResolvedValueOnce({ - total: 2, - saved_objects: mockHits, - pit_id: 'abc123', - per_page: 1, - page: 1, - }); + repository.find + .mockResolvedValueOnce({ + total: 2, + saved_objects: mockHits, + pit_id: 'abc123', + per_page: 1, + page: 0, + }) + .mockResolvedValueOnce({ + total: 2, + saved_objects: mockHits, + pit_id: 'abc123', + per_page: 1, + page: 1, + }); const findOptions: SavedObjectsCreatePointInTimeFinderOptions = { type: ['visualization'], @@ -353,11 +359,7 @@ describe('createPointInTimeFinder()', () => { const finder = new PointInTimeFinder(findOptions, { logger, - client: { - find, - openPointInTimeForType, - closePointInTime, - }, + client: repository, }); const findA = finder.find(); @@ -370,9 +372,9 @@ describe('createPointInTimeFinder()', () => { expect((await findA.next()).done).toBe(true); expect((await findB.next()).done).toBe(true); - expect(openPointInTimeForType).toHaveBeenCalledTimes(2); - expect(find).toHaveBeenCalledTimes(2); - expect(closePointInTime).toHaveBeenCalledTimes(2); + expect(repository.openPointInTimeForType).toHaveBeenCalledTimes(2); + expect(repository.find).toHaveBeenCalledTimes(2); + expect(repository.closePointInTime).toHaveBeenCalledTimes(2); }); }); }); diff --git a/src/core/server/saved_objects/service/lib/point_in_time_finder.ts b/src/core/server/saved_objects/service/lib/point_in_time_finder.ts index f0ed943c585e5..d11be250ad0a9 100644 --- a/src/core/server/saved_objects/service/lib/point_in_time_finder.ts +++ b/src/core/server/saved_objects/service/lib/point_in_time_finder.ts @@ -139,7 +139,9 @@ export class PointInTimeFinder private async open() { try { - const { id } = await this.#client.openPointInTimeForType(this.#findOptions.type); + const { id } = await this.#client.openPointInTimeForType(this.#findOptions.type, { + namespaces: this.#findOptions.namespaces, + }); this.#pitId = id; this.#open = true; } catch (e) { diff --git a/src/core/server/saved_objects/service/saved_objects_client.ts b/src/core/server/saved_objects/service/saved_objects_client.ts index 00d47d8d1fb03..1564df2969ecc 100644 --- a/src/core/server/saved_objects/service/saved_objects_client.ts +++ b/src/core/server/saved_objects/service/saved_objects_client.ts @@ -334,7 +334,7 @@ export interface SavedObjectsResolveResponse { /** * @public */ -export interface SavedObjectsOpenPointInTimeOptions extends SavedObjectsBaseOptions { +export interface SavedObjectsOpenPointInTimeOptions { /** * Optionally specify how long ES should keep the PIT alive until the next request. Defaults to `5m`. */ @@ -343,6 +343,15 @@ export interface SavedObjectsOpenPointInTimeOptions extends SavedObjectsBaseOpti * An optional ES preference value to be used for the query. */ preference?: string; + /** + * An optional list of namespaces to be used when opening the PIT. + * + * When the spaces plugin is enabled: + * - this will default to the user's current space (as determined by the URL) + * - if specified, the user's current space will be ignored + * - `['*']` will search across all available spaces + */ + namespaces?: string[]; } /** diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 47455e0c14316..1bd59dfc7fdb1 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -2460,8 +2460,9 @@ export interface SavedObjectsMigrationVersion { export type SavedObjectsNamespaceType = 'single' | 'multiple' | 'multiple-isolated' | 'agnostic'; // @public (undocumented) -export interface SavedObjectsOpenPointInTimeOptions extends SavedObjectsBaseOptions { +export interface SavedObjectsOpenPointInTimeOptions { keepAlive?: string; + namespaces?: string[]; preference?: string; } diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index e5a2340aba3f0..2f622d9e8a0e1 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -779,17 +779,6 @@ describe('#find', () => { ); }); - test(`throws BadRequestError when searching across namespaces when pit is provided`, async () => { - const options = { - type: [type1, type2], - pit: { id: 'abc123' }, - namespaces: ['some-ns', 'another-ns'], - }; - await expect(client.find(options)).rejects.toThrowErrorMatchingInlineSnapshot( - `"_find across namespaces is not permitted when using the \`pit\` option."` - ); - }); - test(`checks privileges for user, actions, and namespaces`, async () => { const options = { type: [type1, type2], namespaces }; await expectPrivilegeCheck(client.find, { options }, namespaces); @@ -884,7 +873,7 @@ describe('#openPointInTimeForType', () => { const apiCallReturnValue = Symbol(); clientOpts.baseClient.openPointInTimeForType.mockReturnValue(apiCallReturnValue as any); - const options = { namespace }; + const options = { namespaces: [namespace] }; const result = await expectSuccess(client.openPointInTimeForType, { type, options }); expect(result).toBe(apiCallReturnValue); }); @@ -892,18 +881,113 @@ describe('#openPointInTimeForType', () => { test(`adds audit event when successful`, async () => { const apiCallReturnValue = Symbol(); clientOpts.baseClient.openPointInTimeForType.mockReturnValue(apiCallReturnValue as any); - const options = { namespace }; + const options = { namespaces: [namespace] }; await expectSuccess(client.openPointInTimeForType, { type, options }); expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1); expectAuditEvent('saved_object_open_point_in_time', 'unknown'); }); - test(`adds audit event when not successful`, async () => { - clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error()); - await expect(() => client.openPointInTimeForType(type, { namespace })).rejects.toThrow(); + test(`throws an error when unauthorized`, async () => { + clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockImplementation( + getMockCheckPrivilegesFailure + ); + const options = { namespaces: [namespace] }; + await expect(() => client.openPointInTimeForType(type, options)).rejects.toThrowError( + 'unauthorized' + ); + }); + + test(`adds audit event when unauthorized`, async () => { + clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockImplementation( + getMockCheckPrivilegesFailure + ); + const options = { namespaces: [namespace] }; + await expect(() => client.openPointInTimeForType(type, options)).rejects.toThrow(); expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1); expectAuditEvent('saved_object_open_point_in_time', 'failure'); }); + + test(`filters types based on authorization`, async () => { + clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockResolvedValue({ + hasAllRequested: false, + username: USERNAME, + privileges: { + kibana: [ + { + resource: 'some-ns', + privilege: 'mock-saved_object:foo/open_point_in_time', + authorized: true, + }, + { + resource: 'some-ns', + privilege: 'mock-saved_object:bar/open_point_in_time', + authorized: true, + }, + { + resource: 'some-ns', + privilege: 'mock-saved_object:baz/open_point_in_time', + authorized: false, + }, + { + resource: 'some-ns', + privilege: 'mock-saved_object:qux/open_point_in_time', + authorized: false, + }, + { + resource: 'another-ns', + privilege: 'mock-saved_object:foo/open_point_in_time', + authorized: true, + }, + { + resource: 'another-ns', + privilege: 'mock-saved_object:bar/open_point_in_time', + authorized: false, + }, + { + resource: 'another-ns', + privilege: 'mock-saved_object:baz/open_point_in_time', + authorized: true, + }, + { + resource: 'another-ns', + privilege: 'mock-saved_object:qux/open_point_in_time', + authorized: false, + }, + { + resource: 'forbidden-ns', + privilege: 'mock-saved_object:foo/open_point_in_time', + authorized: false, + }, + { + resource: 'forbidden-ns', + privilege: 'mock-saved_object:bar/open_point_in_time', + authorized: false, + }, + { + resource: 'forbidden-ns', + privilege: 'mock-saved_object:baz/open_point_in_time', + authorized: false, + }, + { + resource: 'forbidden-ns', + privilege: 'mock-saved_object:qux/open_point_in_time', + authorized: false, + }, + ], + }, + }); + + await client.openPointInTimeForType(['foo', 'bar', 'baz', 'qux'], { + namespaces: ['some-ns', 'another-ns', 'forbidden-ns'], + }); + + expect(clientOpts.baseClient.openPointInTimeForType).toHaveBeenCalledWith( + ['foo', 'bar', 'baz'], + { + namespaces: ['some-ns', 'another-ns', 'forbidden-ns'], + } + ); + }); }); describe('#closePointInTime', () => { diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index a3bd215211983..6f2b8d28a5601 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -29,7 +29,7 @@ import type { SavedObjectsUpdateOptions, } from 'src/core/server'; -import { SavedObjectsUtils } from '../../../../../src/core/server'; +import { SavedObjectsErrorHelpers, SavedObjectsUtils } from '../../../../../src/core/server'; import { ALL_SPACES_ID, UNKNOWN_SPACE } from '../../common/constants'; import type { AuditLogger, SecurityAuditLogger } from '../audit'; import { SavedObjectAction, savedObjectEvent } from '../audit'; @@ -75,10 +75,12 @@ interface LegacyEnsureAuthorizedResult { status: 'fully_authorized' | 'partially_authorized' | 'unauthorized'; typeMap: Map; } + interface LegacyEnsureAuthorizedTypeResult { authorizedSpaces: string[]; isGloballyAuthorized?: boolean; } + export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContract { private readonly actions: Actions; private readonly legacyAuditLogger: PublicMethodsOf; @@ -236,11 +238,6 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra `_find across namespaces is not permitted when the Spaces plugin is disabled.` ); } - if (options.pit && Array.isArray(options.namespaces) && options.namespaces.length > 1) { - throw this.errors.createBadRequestError( - '_find across namespaces is not permitted when using the `pit` option.' - ); - } const args = { options }; const { status, typeMap } = await this.legacyEnsureAuthorized( @@ -508,22 +505,27 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra type: string | string[], options: SavedObjectsOpenPointInTimeOptions ) { - try { - const args = { type, options }; - await this.legacyEnsureAuthorized(type, 'open_point_in_time', options?.namespace, { + const args = { type, options }; + const { status, typeMap } = await this.legacyEnsureAuthorized( + type, + 'open_point_in_time', + options?.namespaces, + { args, // Partial authorization is acceptable in this case because this method is only designed // to be used with `find`, which already allows for partial authorization. requireFullAuthorization: false, - }); - } catch (error) { + } + ); + + if (status === 'unauthorized') { this.auditLogger.log( savedObjectEvent({ action: SavedObjectAction.OPEN_POINT_IN_TIME, - error, + error: new Error(status), }) ); - throw error; + throw SavedObjectsErrorHelpers.decorateForbiddenError(new Error(status)); } this.auditLogger.log( @@ -533,7 +535,8 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra }) ); - return await this.baseClient.openPointInTimeForType(type, options); + const allowedTypes = [...typeMap.keys()]; // only allow the user to open a PIT against indices for type(s) they are authorized to access + return await this.baseClient.openPointInTimeForType(allowedTypes, options); } public async closePointInTime(id: string, options?: SavedObjectsClosePointInTimeOptions) { diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts index 56bfe71b581ed..b94113436d7ad 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts @@ -533,27 +533,94 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); describe('#openPointInTimeForType', () => { - test(`throws error if options.namespace is specified`, async () => { - const { client } = createSpacesSavedObjectsClient(); + test(`throws error if if user is unauthorized in this space`, async () => { + const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); + const spacesClient = spacesClientMock.create(); + spacesClient.getAll.mockResolvedValue([]); + spacesService.createSpacesClient.mockReturnValue(spacesClient); - await expect(client.openPointInTimeForType('foo', { namespace: 'bar' })).rejects.toThrow( - ERROR_NAMESPACE_SPECIFIED + await expect( + client.openPointInTimeForType('foo', { namespaces: ['bar'] }) + ).rejects.toThrowError('Bad Request'); + + expect(baseClient.openPointInTimeForType).not.toHaveBeenCalled(); + }); + + test(`throws error if if user is unauthorized in any space`, async () => { + const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); + const spacesClient = spacesClientMock.create(); + spacesClient.getAll.mockRejectedValue(Boom.unauthorized()); + spacesService.createSpacesClient.mockReturnValue(spacesClient); + + await expect( + client.openPointInTimeForType('foo', { namespaces: ['bar'] }) + ).rejects.toThrowError('Bad Request'); + + expect(baseClient.openPointInTimeForType).not.toHaveBeenCalled(); + }); + + test(`filters options.namespaces based on authorization`, async () => { + const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); + const expectedReturnValue = { id: 'abc123' }; + baseClient.openPointInTimeForType.mockReturnValue(Promise.resolve(expectedReturnValue)); + + const spacesClient = spacesService.createSpacesClient( + null as any + ) as jest.Mocked; + spacesClient.getAll.mockImplementation(() => + Promise.resolve([ + { id: 'ns-1', name: '', disabledFeatures: [] }, + { id: 'ns-2', name: '', disabledFeatures: [] }, + ]) ); + + const options = Object.freeze({ namespaces: ['ns-1', 'ns-3'] }); + const actualReturnValue = await client.openPointInTimeForType(['foo', 'bar'], options); + + expect(actualReturnValue).toBe(expectedReturnValue); + expect(baseClient.openPointInTimeForType).toHaveBeenCalledWith(['foo', 'bar'], { + namespaces: ['ns-1'], + }); + expect(spacesClient.getAll).toHaveBeenCalledWith({ purpose: 'findSavedObjects' }); }); - test(`supplements options with the current namespace`, async () => { + test(`translates options.namespaces: ['*']`, async () => { + const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); + const expectedReturnValue = { id: 'abc123' }; + baseClient.openPointInTimeForType.mockReturnValue(Promise.resolve(expectedReturnValue)); + + const spacesClient = spacesService.createSpacesClient( + null as any + ) as jest.Mocked; + spacesClient.getAll.mockImplementation(() => + Promise.resolve([ + { id: 'ns-1', name: '', disabledFeatures: [] }, + { id: 'ns-2', name: '', disabledFeatures: [] }, + ]) + ); + + const options = Object.freeze({ namespaces: ['*'] }); + const actualReturnValue = await client.openPointInTimeForType(['foo', 'bar'], options); + + expect(actualReturnValue).toBe(expectedReturnValue); + expect(baseClient.openPointInTimeForType).toHaveBeenCalledWith(['foo', 'bar'], { + namespaces: ['ns-1', 'ns-2'], + }); + expect(spacesClient.getAll).toHaveBeenCalledWith({ purpose: 'findSavedObjects' }); + }); + + test(`supplements options with the current namespace if unspecified`, async () => { const { client, baseClient } = createSpacesSavedObjectsClient(); const expectedReturnValue = { id: 'abc123' }; baseClient.openPointInTimeForType.mockReturnValue(Promise.resolve(expectedReturnValue)); - const options = Object.freeze({ foo: 'bar' }); - // @ts-expect-error + const options = Object.freeze({ keepAlive: '2m' }); const actualReturnValue = await client.openPointInTimeForType('foo', options); expect(actualReturnValue).toBe(expectedReturnValue); expect(baseClient.openPointInTimeForType).toHaveBeenCalledWith('foo', { - foo: 'bar', - namespace: currentSpace.expectedNamespace, + keepAlive: '2m', + namespaces: [currentSpace.expectedNamespace ?? DEFAULT_SPACE_ID], }); }); }); diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts index e344aa8cecf07..9c51f22e280d8 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts @@ -30,7 +30,7 @@ import type { SavedObjectsUpdateOptions, } from 'src/core/server'; -import { SavedObjectsUtils } from '../../../../../src/core/server'; +import { SavedObjectsErrorHelpers, SavedObjectsUtils } from '../../../../../src/core/server'; import { ALL_SPACES_ID } from '../../common/constants'; import { spaceIdToNamespace } from '../lib/utils/namespace'; import type { ISpacesClient } from '../spaces_client'; @@ -175,32 +175,19 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { * @returns {promise} - { saved_objects: [{ id, type, version, attributes }], total, per_page, page } */ public async find(options: SavedObjectsFindOptions) { - throwErrorIfNamespaceSpecified(options); - - let namespaces = options.namespaces; - if (namespaces) { - try { - const availableSpaces = await this.spacesClient.getAll({ purpose: 'findSavedObjects' }); - if (namespaces.includes(ALL_SPACES_ID)) { - namespaces = availableSpaces.map((space) => space.id); - } else { - namespaces = namespaces.filter((namespace) => - availableSpaces.some((space) => space.id === namespace) - ); - } - if (namespaces.length === 0) { - // return empty response, since the user is unauthorized in this space (or these spaces), but we don't return forbidden errors for `find` operations - return SavedObjectsUtils.createEmptyFindResponse(options); - } - } catch (err) { - if (Boom.isBoom(err) && err.output.payload.statusCode === 403) { - // return empty response, since the user is unauthorized in any space, but we don't return forbidden errors for `find` operations - return SavedObjectsUtils.createEmptyFindResponse(options); - } - throw err; + let namespaces: string[]; + try { + namespaces = await this.getSearchableSpaces(options.namespaces); + } catch (err) { + if (Boom.isBoom(err) && err.output.payload.statusCode === 403) { + // return empty response, since the user is unauthorized in any space, but we don't return forbidden errors for `find` operations + return SavedObjectsUtils.createEmptyFindResponse(options); } - } else { - namespaces = [this.spaceId]; + throw err; + } + if (namespaces.length === 0) { + // return empty response, since the user is unauthorized in this space (or these spaces), but we don't return forbidden errors for `find` operations + return SavedObjectsUtils.createEmptyFindResponse(options); } return await this.client.find({ @@ -396,10 +383,15 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { type: string | string[], options: SavedObjectsOpenPointInTimeOptions = {} ) { - throwErrorIfNamespaceSpecified(options); + const namespaces = await this.getSearchableSpaces(options.namespaces); + if (namespaces.length === 0) { + // throw bad request if no valid spaces were found. + throw SavedObjectsErrorHelpers.createBadRequestError(); + } + return await this.client.openPointInTimeForType(type, { ...options, - namespace: spaceIdToNamespace(this.spaceId), + namespaces, }); } @@ -446,4 +438,19 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { ...dependencies, }); } + + private async getSearchableSpaces(namespaces?: string[]): Promise { + if (namespaces) { + const availableSpaces = await this.spacesClient.getAll({ purpose: 'findSavedObjects' }); + if (namespaces.includes(ALL_SPACES_ID)) { + return availableSpaces.map((space) => space.id); + } else { + return namespaces.filter((namespace) => + availableSpaces.some((space) => space.id === namespace) + ); + } + } else { + return [this.spaceId]; + } + } } diff --git a/x-pack/test/saved_object_api_integration/common/suites/export.ts b/x-pack/test/saved_object_api_integration/common/suites/export.ts index ea2f321458c22..77eb93a4107a5 100644 --- a/x-pack/test/saved_object_api_integration/common/suites/export.ts +++ b/x-pack/test/saved_object_api_integration/common/suites/export.ts @@ -155,8 +155,16 @@ export function exportTestSuiteFactory(esArchiver: any, supertest: SuperTest