From 502b8e7d8f9f47189ca2a47b68cc41a26797041c Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Tue, 9 Feb 2021 15:05:32 -0700 Subject: [PATCH] Remove user conflicts and update SO filter --- .../search/session/session_service.test.ts | 161 +++++++++++++----- .../server/search/session/session_service.ts | 52 +++--- 2 files changed, 138 insertions(+), 75 deletions(-) diff --git a/x-pack/plugins/data_enhanced/server/search/session/session_service.test.ts b/x-pack/plugins/data_enhanced/server/search/session/session_service.test.ts index 37cc7a8622a57..fe4924360f0d9 100644 --- a/x-pack/plugins/data_enhanced/server/search/session/session_service.test.ts +++ b/x-pack/plugins/data_enhanced/server/search/session/session_service.test.ts @@ -39,13 +39,6 @@ describe('SearchSessionService', () => { name: 'my_realm_name', }, } as AuthenticatedUser; - const mockUser2 = { - username: 'bar', - authentication_realm: { - type: 'bar', - name: 'bar', - }, - } as AuthenticatedUser; const mockSavedObject: SavedObject = { id: 'd7170a35-7e2c-48d6-8dec-9a056721b489', type: SEARCH_SESSION_TYPE, @@ -145,9 +138,6 @@ describe('SearchSessionService', () => { expect(callAttributes).toHaveProperty('urlGeneratorId', 'panama'); expect(callAttributes).toHaveProperty('initialState', {}); expect(callAttributes).toHaveProperty('restoreState', {}); - expect(callAttributes).toHaveProperty('realmType', mockUser1.authentication_realm.type); - expect(callAttributes).toHaveProperty('realmName', mockUser1.authentication_realm.name); - expect(callAttributes).toHaveProperty('username', mockUser1.username); }); it('saving creates a new persisted saved object, if it did not exist', async () => { @@ -183,6 +173,9 @@ describe('SearchSessionService', () => { expect(callAttributes).toHaveProperty('urlGeneratorId', 'panama'); expect(callAttributes).toHaveProperty('initialState', {}); expect(callAttributes).toHaveProperty('restoreState', {}); + expect(callAttributes).toHaveProperty('realmType', mockUser1.authentication_realm.type); + expect(callAttributes).toHaveProperty('realmName', mockUser1.authentication_realm.name); + expect(callAttributes).toHaveProperty('username', mockUser1.username); }); it('works without security', async () => { @@ -220,14 +213,6 @@ describe('SearchSessionService', () => { expect(savedObjectsClient.get).toHaveBeenCalledWith(SEARCH_SESSION_TYPE, sessionId); }); - it('throws error if user conflicts', () => { - savedObjectsClient.get.mockResolvedValue(mockSavedObject); - - expect( - service.get({ savedObjectsClient }, mockUser2, sessionId) - ).rejects.toMatchInlineSnapshot(`[Error: Not Found]`); - }); - it('works without security', async () => { savedObjectsClient.get.mockResolvedValue(mockSavedObject); @@ -259,7 +244,66 @@ describe('SearchSessionService', () => { const [[findOptions]] = savedObjectsClient.find.mock.calls; expect(findOptions).toMatchInlineSnapshot(` Object { - "filter": "search-session.attributes.realmType: my_realm_type and search-session.attributes.realmName: my_realm_name and search-session.attributes.username: my_username", + "filter": Object { + "arguments": Array [ + Object { + "arguments": Array [ + Object { + "type": "literal", + "value": "search-session.attributes.realmType", + }, + Object { + "type": "literal", + "value": "my_realm_type", + }, + Object { + "type": "literal", + "value": false, + }, + ], + "function": "is", + "type": "function", + }, + Object { + "arguments": Array [ + Object { + "type": "literal", + "value": "search-session.attributes.realmName", + }, + Object { + "type": "literal", + "value": "my_realm_name", + }, + Object { + "type": "literal", + "value": false, + }, + ], + "function": "is", + "type": "function", + }, + Object { + "arguments": Array [ + Object { + "type": "literal", + "value": "search-session.attributes.username", + }, + Object { + "type": "literal", + "value": "my_username", + }, + Object { + "type": "literal", + "value": false, + }, + ], + "function": "is", + "type": "function", + }, + ], + "function": "and", + "type": "function", + }, "page": 0, "perPage": 5, "type": "search-session", @@ -287,7 +331,7 @@ describe('SearchSessionService', () => { const [[findOptions]] = savedObjectsClient.find.mock.calls; expect(findOptions).toMatchInlineSnapshot(` Object { - "filter": "", + "filter": undefined, "page": 0, "perPage": 5, "type": "search-session", @@ -323,20 +367,6 @@ describe('SearchSessionService', () => { expect(callAttributes).toHaveProperty('touched'); }); - it('throws if user conflicts', () => { - const mockUpdateSavedObject = { - ...mockSavedObject, - attributes: {}, - }; - savedObjectsClient.get.mockResolvedValue(mockSavedObject); - savedObjectsClient.update.mockResolvedValue(mockUpdateSavedObject); - - const attributes = { name: 'new_name' }; - expect( - service.update({ savedObjectsClient }, mockUser2, sessionId, attributes) - ).rejects.toMatchInlineSnapshot(`[Error: Not Found]`); - }); - it('works without security', async () => { const mockUpdateSavedObject = { ...mockSavedObject, @@ -370,14 +400,6 @@ describe('SearchSessionService', () => { expect(callAttributes).toHaveProperty('touched'); }); - it('throws if user conflicts', () => { - savedObjectsClient.get.mockResolvedValue(mockSavedObject); - - expect( - service.cancel({ savedObjectsClient }, mockUser2, sessionId) - ).rejects.toMatchInlineSnapshot(`[Error: Not Found]`); - }); - it('works without security', async () => { savedObjectsClient.get.mockResolvedValue(mockSavedObject); @@ -515,6 +537,61 @@ describe('SearchSessionService', () => { expect(callAttributes).toHaveProperty('sessionId', sessionId); expect(callAttributes).toHaveProperty('persisted', false); }); + + it('retries updating if update returned 404 and then update returned conflict 409 (first create race condition)', async () => { + const searchRequest = { params: {} }; + const searchId = 'FnpFYlBpeXdCUTMyZXhCLTc1TWFKX0EbdDFDTzJzTE1Sck9PVTBIcW1iU05CZzo4MDA0'; + + const mockUpdateSavedObject = { + ...mockSavedObject, + attributes: {}, + }; + + let counter = 0; + + savedObjectsClient.update.mockImplementation(() => { + return new Promise((resolve, reject) => { + if (counter === 0) { + counter++; + reject(SavedObjectsErrorHelpers.createGenericNotFoundError(sessionId)); + } else { + resolve(mockUpdateSavedObject); + } + }); + }); + + savedObjectsClient.create.mockRejectedValue( + SavedObjectsErrorHelpers.createConflictError(SEARCH_SESSION_TYPE, searchId) + ); + + await service.trackId({ savedObjectsClient }, mockUser1, searchRequest, searchId, { + sessionId, + strategy: MOCK_STRATEGY, + }); + + expect(savedObjectsClient.update).toHaveBeenCalledTimes(2); + expect(savedObjectsClient.create).toHaveBeenCalledTimes(1); + }); + + it('retries everything at most MAX_RETRIES times', async () => { + const searchRequest = { params: {} }; + const searchId = 'FnpFYlBpeXdCUTMyZXhCLTc1TWFKX0EbdDFDTzJzTE1Sck9PVTBIcW1iU05CZzo4MDA0'; + + savedObjectsClient.update.mockRejectedValue( + SavedObjectsErrorHelpers.createGenericNotFoundError(sessionId) + ); + savedObjectsClient.create.mockRejectedValue( + SavedObjectsErrorHelpers.createConflictError(SEARCH_SESSION_TYPE, searchId) + ); + + await service.trackId({ savedObjectsClient }, mockUser1, searchRequest, searchId, { + sessionId, + strategy: MOCK_STRATEGY, + }); + + expect(savedObjectsClient.update).toHaveBeenCalledTimes(MAX_UPDATE_RETRIES); + expect(savedObjectsClient.create).toHaveBeenCalledTimes(MAX_UPDATE_RETRIES); + }); }); describe('getId', () => { diff --git a/x-pack/plugins/data_enhanced/server/search/session/session_service.ts b/x-pack/plugins/data_enhanced/server/search/session/session_service.ts index 336a953648e81..48b0ac1ea092f 100644 --- a/x-pack/plugins/data_enhanced/server/search/session/session_service.ts +++ b/x-pack/plugins/data_enhanced/server/search/session/session_service.ts @@ -5,7 +5,6 @@ * 2.0. */ -import { notFound } from '@hapi/boom'; import { CoreSetup, CoreStart, @@ -16,7 +15,11 @@ import { SavedObjectsFindOptions, SavedObjectsErrorHelpers, } from '../../../../../../src/core/server'; -import { IKibanaSearchRequest, ISearchOptions } from '../../../../../../src/plugins/data/common'; +import { + IKibanaSearchRequest, + ISearchOptions, + nodeBuilder, +} from '../../../../../../src/plugins/data/common'; import { ISearchSessionService } from '../../../../../../src/plugins/data/server'; import { AuthenticatedUser, SecurityPluginSetup } from '../../../../security/server'; import { @@ -193,18 +196,16 @@ export class SearchSessionService ); }; - public get = async ( + public get = ( { savedObjectsClient }: SearchSessionDependencies, user: AuthenticatedUser | null, sessionId: string ) => { this.logger.debug(`get | ${sessionId}`); - const session = await savedObjectsClient.get( + return savedObjectsClient.get( SEARCH_SESSION_TYPE, sessionId ); - this.throwOnUserConflict(user, session); - return session; }; public find = ( @@ -216,11 +217,17 @@ export class SearchSessionService user === null ? [] : [ - `${SEARCH_SESSION_TYPE}.attributes.realmType: ${user.authentication_realm.type}`, - `${SEARCH_SESSION_TYPE}.attributes.realmName: ${user.authentication_realm.name}`, - `${SEARCH_SESSION_TYPE}.attributes.username: ${user.username}`, + nodeBuilder.is( + `${SEARCH_SESSION_TYPE}.attributes.realmType`, + `${user.authentication_realm.type}` + ), + nodeBuilder.is( + `${SEARCH_SESSION_TYPE}.attributes.realmName`, + `${user.authentication_realm.name}` + ), + nodeBuilder.is(`${SEARCH_SESSION_TYPE}.attributes.username`, `${user.username}`), ]; - const filter = userFilters.concat(options.filter ?? []).join(' and '); + const filter = nodeBuilder.and(userFilters.concat(options.filter ?? [])); return savedObjectsClient.find({ ...options, filter, @@ -228,16 +235,13 @@ export class SearchSessionService }); }; - // TODO: Throw an error if this session doesn't belong to this user - public update = async ( + public update = ( deps: SearchSessionDependencies, user: AuthenticatedUser | null, sessionId: string, attributes: Partial ) => { this.logger.debug(`update | ${sessionId}`); - const session = await this.get(deps, user, sessionId); - this.throwOnUserConflict(user, session); return deps.savedObjectsClient.update( SEARCH_SESSION_TYPE, sessionId, @@ -259,7 +263,6 @@ export class SearchSessionService return this.update(deps, user, sessionId, { expires: expires.toISOString() }); } - // TODO: Throw an error if this session doesn't belong to this user public cancel = ( deps: SearchSessionDependencies, user: AuthenticatedUser | null, @@ -271,14 +274,11 @@ export class SearchSessionService }); }; - // TODO: Throw an error if this session doesn't belong to this user - public delete = async ( + public delete = ( deps: SearchSessionDependencies, user: AuthenticatedUser | null, sessionId: string ) => { - const session = await this.get(deps, user, sessionId); - this.throwOnUserConflict(user, session); return deps.savedObjectsClient.delete(SEARCH_SESSION_TYPE, sessionId); }; @@ -375,18 +375,4 @@ export class SearchSessionService }; }; }; - - private throwOnUserConflict = ( - user: AuthenticatedUser | null, - session?: SavedObject - ) => { - if (user === null || !session) return; - if ( - user.authentication_realm.type !== session.attributes.realmType || - user.authentication_realm.name !== session.attributes.realmName || - user.username !== session.attributes.username - ) { - throw notFound(); - } - }; }