Skip to content

Commit

Permalink
Remove user conflicts and update SO filter
Browse files Browse the repository at this point in the history
  • Loading branch information
@lukasolson
lukasolson committed Feb 9, 2021
1 parent 879db30 commit 502b8e7
Show file tree
Hide file tree
Showing 2 changed files with 138 additions and 75 deletions.
161 changes: 119 additions & 42 deletions x-pack/plugins/data_enhanced/server/search/session/session_service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<any> = {
id: 'd7170a35-7e2c-48d6-8dec-9a056721b489',
type: SEARCH_SESSION_TYPE,
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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', () => {
Expand Down
52 changes: 19 additions & 33 deletions x-pack/plugins/data_enhanced/server/search/session/session_service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* 2.0.
*/

import { notFound } from '@hapi/boom';
import {
CoreSetup,
CoreStart,
Expand All @@ -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 {
Expand Down Expand Up @@ -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<SearchSessionSavedObjectAttributes>(
return savedObjectsClient.get<SearchSessionSavedObjectAttributes>(
SEARCH_SESSION_TYPE,
sessionId
);
this.throwOnUserConflict(user, session);
return session;
};

public find = (
Expand All @@ -216,28 +217,31 @@ 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<SearchSessionSavedObjectAttributes>({
...options,
filter,
type: SEARCH_SESSION_TYPE,
});
};

// 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<SearchSessionSavedObjectAttributes>
) => {
this.logger.debug(`update | ${sessionId}`);
const session = await this.get(deps, user, sessionId);
this.throwOnUserConflict(user, session);
return deps.savedObjectsClient.update<SearchSessionSavedObjectAttributes>(
SEARCH_SESSION_TYPE,
sessionId,
Expand All @@ -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,
Expand All @@ -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);
};

Expand Down Expand Up @@ -375,18 +375,4 @@ export class SearchSessionService
};
};
};

private throwOnUserConflict = (
user: AuthenticatedUser | null,
session?: SavedObject<SearchSessionSavedObjectAttributes>
) => {
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();
}
};
}

0 comments on commit 502b8e7

Please sign in to comment.