From 9da30a15dbe2ce73e7e4fee6d2565b3bbc32b884 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 6 Jun 2018 07:35:28 -0400 Subject: [PATCH 01/12] Adding ability to specify filters when calling the repository --- .../saved_objects/service/lib/repository.js | 5 +- .../service/lib/repository.test.js | 7 +- .../service/lib/search_dsl/query_params.js | 52 ++-- .../lib/search_dsl/query_params.test.js | 274 +++++++++++++++++- .../service/lib/search_dsl/search_dsl.js | 5 +- .../service/lib/search_dsl/search_dsl.test.js | 6 +- 6 files changed, 321 insertions(+), 28 deletions(-) diff --git a/src/server/saved_objects/service/lib/repository.js b/src/server/saved_objects/service/lib/repository.js index 6f3b4fffb2158..405521c75dc57 100644 --- a/src/server/saved_objects/service/lib/repository.js +++ b/src/server/saved_objects/service/lib/repository.js @@ -212,6 +212,7 @@ export class SavedObjectsRepository { * @property {string} [options.sortField] * @property {string} [options.sortOrder] * @property {Array} [options.fields] + * @property {Array} [options.filters] * @returns {promise} - { saved_objects: [{ id, type, version, attributes }], total, per_page, page } */ async find(options = {}) { @@ -224,6 +225,7 @@ export class SavedObjectsRepository { sortField, sortOrder, fields, + filters, } = options; if (searchFields && !Array.isArray(searchFields)) { @@ -247,7 +249,8 @@ export class SavedObjectsRepository { searchFields, type, sortField, - sortOrder + sortOrder, + filters, }) } }; diff --git a/src/server/saved_objects/service/lib/repository.test.js b/src/server/saved_objects/service/lib/repository.test.js index a02bfb9abb619..b8c6a310cda5c 100644 --- a/src/server/saved_objects/service/lib/repository.test.js +++ b/src/server/saved_objects/service/lib/repository.test.js @@ -388,13 +388,18 @@ describe('SavedObjectsRepository', () => { } }); - it('passes mappings, search, searchFields, type, sortField, and sortOrder to getSearchDsl', async () => { + it('passes mappings, search, searchFields, type, sortField, sortOrder, and filters to getSearchDsl', async () => { const relevantOpts = { search: 'foo*', searchFields: ['foo'], type: 'bar', sortField: 'name', sortOrder: 'desc', + filters: [{ + terms: { + type: ['foo', 'bar'] + } + }], }; await savedObjectsRepository.find(relevantOpts); diff --git a/src/server/saved_objects/service/lib/search_dsl/query_params.js b/src/server/saved_objects/service/lib/search_dsl/query_params.js index bcf62f21ef415..e2f85973ca08e 100644 --- a/src/server/saved_objects/service/lib/search_dsl/query_params.js +++ b/src/server/saved_objects/service/lib/search_dsl/query_params.js @@ -67,33 +67,43 @@ function getFieldsForTypes(searchFields, types) { * @param {Array} searchFields * @return {Object} */ -export function getQueryParams(mappings, type, search, searchFields) { - if (!type && !search) { - return {}; - } - - const bool = {}; +export function getQueryParams(mappings, type, search, searchFields, filters = []) { + const filter = [...filters]; + const must = []; if (type) { - bool.filter = [ - { [Array.isArray(type) ? 'terms' : 'term']: { type } } - ]; + filter.push({ [Array.isArray(type) ? 'terms' : 'term']: { type } }); } if (search) { - bool.must = [ - ...bool.must || [], - { - simple_query_string: { - query: search, - ...getFieldsForTypes( - searchFields, - getTypes(mappings, type) - ) - } + must.push({ + simple_query_string: { + query: search, + ...getFieldsForTypes( + searchFields, + getTypes(mappings, type) + ) } - ]; + }); + } + + if (filter.length === 0 && must.length === 0) { + return {}; + } + + const result = { + query: { + bool: {} + } + }; + + if (filter.length > 0) { + result.query.bool.filter = filter; + } + + if (must.length > 0) { + result.query.bool.must = must; } - return { query: { bool } }; + return result; } diff --git a/src/server/saved_objects/service/lib/search_dsl/query_params.test.js b/src/server/saved_objects/service/lib/search_dsl/query_params.test.js index 53b943ee6793b..7b1a6f5dc7eb0 100644 --- a/src/server/saved_objects/service/lib/search_dsl/query_params.test.js +++ b/src/server/saved_objects/service/lib/search_dsl/query_params.test.js @@ -95,6 +95,36 @@ describe('searchDsl/queryParams', () => { }); }); + describe('{type,filters}', () => { + it('includes filters and a term filter for type when type is a string', () => { + expect(getQueryParams(MAPPINGS, 'saved', null, null, [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + { term: { type: 'saved' } } + ] + } + } + }); + }); + + it('includes filters and a terms filter for type when type is an array', () => { + expect(getQueryParams(MAPPINGS, ['saved', 'vis'], null, null, [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + { terms: { type: ['saved', 'vis'] } } + ] + } + } + }); + }); + }); + describe('{search}', () => { it('includes just a sqs query', () => { expect(getQueryParams(MAPPINGS, null, 'us*')) @@ -115,8 +145,31 @@ describe('searchDsl/queryParams', () => { }); }); + describe('{search,filters}', () => { + it('includes filters and a sqs query', () => { + expect(getQueryParams(MAPPINGS, null, 'us*', null, [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } } + ], + must: [ + { + simple_query_string: { + query: 'us*', + all_fields: true + } + } + ] + } + } + }); + }); + }); + describe('{type,search}', () => { - it('includes bool with sqs query and term filter for type', () => { + it('includes bool with sqs query and term filter for type when type is a string', () => { expect(getQueryParams(MAPPINGS, 'saved', 'y*')) .toEqual({ query: { @@ -136,7 +189,7 @@ describe('searchDsl/queryParams', () => { } }); }); - it('includes bool with sqs query and terms filter for type', () => { + it('includes bool with sqs query and terms filter for type when type is an array', () => { expect(getQueryParams(MAPPINGS, ['saved', 'vis'], 'y*')) .toEqual({ query: { @@ -158,6 +211,52 @@ describe('searchDsl/queryParams', () => { }); }); + describe('{type,search,filters}', () => { + it('includes bool with sqs query, filters and term filter for type when type is a string', () => { + expect(getQueryParams(MAPPINGS, 'saved', 'y*', null, [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + { term: { type: 'saved' } } + ], + must: [ + { + simple_query_string: { + query: 'y*', + all_fields: true + } + } + ] + } + } + }); + }); + + it('includes bool with sqs query, filters and terms filter for type when type is an array', () => { + expect(getQueryParams(MAPPINGS, ['saved', 'vis'], 'y*', null, [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + { terms: { type: ['saved', 'vis'] } } + ], + must: [ + { + simple_query_string: { + query: 'y*', + all_fields: true + } + } + ] + } + } + }); + }); + }); + describe('{search,searchFields}', () => { it('includes all types for field', () => { expect(getQueryParams(MAPPINGS, null, 'y*', ['title'])) @@ -223,6 +322,80 @@ describe('searchDsl/queryParams', () => { }); }); + describe('{search,searchFields,filters}', () => { + it('includes all types for field and includes filter', () => { + expect(getQueryParams(MAPPINGS, null, 'y*', ['title'], [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + ], + must: [ + { + simple_query_string: { + query: 'y*', + fields: [ + 'pending.title', + 'saved.title' + ] + } + } + ] + } + } + }); + }); + it('supports field boosting and includes filter', () => { + expect(getQueryParams(MAPPINGS, null, 'y*', ['title^3'], [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + ], + must: [ + { + simple_query_string: { + query: 'y*', + fields: [ + 'pending.title^3', + 'saved.title^3' + ] + } + } + ] + } + } + }); + }); + it('supports field and multi-field and includes filter', () => { + expect(getQueryParams(MAPPINGS, null, 'y*', ['title', 'title.raw'], [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + ], + must: [ + { + simple_query_string: { + query: 'y*', + fields: [ + 'pending.title', + 'saved.title', + 'pending.title.raw', + 'saved.title.raw', + ] + } + } + ] + } + } + }); + }); + }); + describe('{type,search,searchFields}', () => { it('includes bool, with term filter and sqs with field list', () => { expect(getQueryParams(MAPPINGS, 'saved', 'y*', ['title'])) @@ -315,4 +488,101 @@ describe('searchDsl/queryParams', () => { }); }); }); + + describe('{type,search,searchFields,filters}', () => { + it('includes bool, with term filter and filter and sqs with field list', () => { + expect(getQueryParams(MAPPINGS, 'saved', 'y*', ['title'], [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + { term: { type: 'saved' } } + ], + must: [ + { + simple_query_string: { + query: 'y*', + fields: [ + 'saved.title' + ] + } + } + ] + } + } + }); + }); + it('includes bool, with terms filter and filter and sqs with field list', () => { + expect(getQueryParams(MAPPINGS, ['saved', 'vis'], 'y*', ['title'], [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + { terms: { type: ['saved', 'vis'] } } + ], + must: [ + { + simple_query_string: { + query: 'y*', + fields: [ + 'saved.title', + 'vis.title' + ] + } + } + ] + } + } + }); + }); + it('supports fields pointing to multi-fields and filter', () => { + expect(getQueryParams(MAPPINGS, 'saved', 'y*', ['title.raw'], [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + { term: { type: 'saved' } } + ], + must: [ + { + simple_query_string: { + query: 'y*', + fields: [ + 'saved.title.raw' + ] + } + } + ] + } + } + }); + }); + it('supports multiple search fields and filter', () => { + expect(getQueryParams(MAPPINGS, 'saved', 'y*', ['title', 'title.raw'], [{ terms: { foo: ['bar', 'baz' ] } }])) + .toEqual({ + query: { + bool: { + filter: [ + { terms: { foo: ['bar', 'baz' ] } }, + { term: { type: 'saved' } } + ], + must: [ + { + simple_query_string: { + query: 'y*', + fields: [ + 'saved.title', + 'saved.title.raw' + ] + } + } + ] + } + } + }); + }); + }); }); diff --git a/src/server/saved_objects/service/lib/search_dsl/search_dsl.js b/src/server/saved_objects/service/lib/search_dsl/search_dsl.js index ea34c127e9854..6843ebc306e1b 100644 --- a/src/server/saved_objects/service/lib/search_dsl/search_dsl.js +++ b/src/server/saved_objects/service/lib/search_dsl/search_dsl.js @@ -28,7 +28,8 @@ export function getSearchDsl(mappings, options = {}) { search, searchFields, sortField, - sortOrder + sortOrder, + filters, } = options; if (!type && sortField) { @@ -40,7 +41,7 @@ export function getSearchDsl(mappings, options = {}) { } return { - ...getQueryParams(mappings, type, search, searchFields), + ...getQueryParams(mappings, type, search, searchFields, filters), ...getSortingParams(mappings, type, sortField, sortOrder), }; } diff --git a/src/server/saved_objects/service/lib/search_dsl/search_dsl.test.js b/src/server/saved_objects/service/lib/search_dsl/search_dsl.test.js index 85302b5e25722..b95f9f9967104 100644 --- a/src/server/saved_objects/service/lib/search_dsl/search_dsl.test.js +++ b/src/server/saved_objects/service/lib/search_dsl/search_dsl.test.js @@ -46,13 +46,16 @@ describe('getSearchDsl', () => { }); describe('passes control', () => { - it('passes (mappings, type, search, searchFields) to getQueryParams', () => { + it('passes (mappings, type, search, searchFields, filters) to getQueryParams', () => { const spy = sandbox.spy(queryParamsNS, 'getQueryParams'); const mappings = { type: { properties: {} } }; const opts = { type: 'foo', search: 'bar', searchFields: ['baz'], + filters: [ + { terms: { foo: ['bar', 'baz'] } } + ] }; getSearchDsl(mappings, opts); @@ -63,6 +66,7 @@ describe('getSearchDsl', () => { opts.type, opts.search, opts.searchFields, + opts.filters, ); }); From 128008f7741e60fb568ff954b256e257f9f722e9 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 6 Jun 2018 09:59:44 -0400 Subject: [PATCH 02/12] Implementing find filtering --- .../saved_objects/service/lib/repository.js | 6 +- .../secure_saved_objects_client.js | 53 +- .../secure_saved_objects_client.test.js | 683 ++++++++++++++++++ 3 files changed, 728 insertions(+), 14 deletions(-) create mode 100644 x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js diff --git a/src/server/saved_objects/service/lib/repository.js b/src/server/saved_objects/service/lib/repository.js index 405521c75dc57..9d5f1ba00226a 100644 --- a/src/server/saved_objects/service/lib/repository.js +++ b/src/server/saved_objects/service/lib/repository.js @@ -19,7 +19,7 @@ import uuid from 'uuid'; -import { getRootType } from '../../../mappings'; +import { getRootType, getRootPropertiesObjects } from '../../../mappings'; import { getSearchDsl } from './search_dsl'; import { trimIdPrefix } from './trim_id_prefix'; import { includedFields } from './included_fields'; @@ -409,6 +409,10 @@ export class SavedObjectsRepository { }; } + getTypes() { + return Object.keys(getRootPropertiesObjects(this._mappings)); + } + async _writeToCluster(method, params) { try { await this._onBeforeWrite(); diff --git a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js index 65e2d5890fea7..9b4892d94385a 100644 --- a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js +++ b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js @@ -6,6 +6,10 @@ import { get, uniq } from 'lodash'; +const getPrivilege = (type, action) => { + return `action:saved_objects/${type}/${action}`; +}; + export class SecureSavedObjectsClient { constructor(options) { const { @@ -51,11 +55,32 @@ export class SecureSavedObjectsClient { } async find(options = {}) { - await this._performAuthorizationCheck(options.type, 'find', { - options, - }); + const action = 'find'; - return await this._repository.find(options); + // when we have the type or types, it makes our life easy + if (options.type) { + await this._performAuthorizationCheck(options.type, action, { options }); + return await this._repository.find(options); + } + + // otherwise, we have to filter for only their authorized types + const types = this._repository.getTypes(); + const typesToPrivilegesMap = new Map(types.map(type => [type, getPrivilege(type, action)])); + const result = await this._hasSavedObjectPrivileges(Array.from(typesToPrivilegesMap.values())); + const authorizedTypes = Array.from(typesToPrivilegesMap.entries()) + .filter(([ , privilege]) => !result.missing.includes(privilege)) + .map(([type]) => type); + + if (authorizedTypes.length === 0) { + this._auditLogger.savedObjectsAuthorizationFailure(result.username, action, types, result.missing, { options }); + throw this.errors.decorateForbiddenError(new Error(`Not authorized to search any types`)); + } + this._auditLogger.savedObjectsAuthorizationSuccess(result.username, action, authorizedTypes, { options }); + + return await this._repository.find({ + ...options, + type: authorizedTypes + }); } async bulkGet(objects = []) { @@ -89,15 +114,8 @@ export class SecureSavedObjectsClient { async _performAuthorizationCheck(typeOrTypes, action, args) { const types = Array.isArray(typeOrTypes) ? typeOrTypes : [typeOrTypes]; - const actions = types.map(type => `action:saved_objects/${type}/${action}`); - - let result; - try { - result = await this._hasPrivileges(actions); - } catch(error) { - const { reason } = get(error, 'body.error', {}); - throw this.errors.decorateGeneralError(error, reason); - } + const privileges = types.map(type => getPrivilege(type, action)); + const result = await this._hasSavedObjectPrivileges(privileges); if (result.success) { this._auditLogger.savedObjectsAuthorizationSuccess(result.username, action, types, args); @@ -107,4 +125,13 @@ export class SecureSavedObjectsClient { throw this.errors.decorateForbiddenError(new Error(msg)); } } + + async _hasSavedObjectPrivileges(privileges) { + try { + return await this._hasPrivileges(privileges); + } catch(error) { + const { reason } = get(error, 'body.error', {}); + throw this.errors.decorateGeneralError(error, reason); + } + } } diff --git a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js new file mode 100644 index 0000000000000..e743620afe4f9 --- /dev/null +++ b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js @@ -0,0 +1,683 @@ +/* + * 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 { SecureSavedObjectsClient } from './secure_saved_objects_client'; + +const createMockErrors = () => { + const forbiddenError = new Error('Mock ForbiddenError'); + const generalError = new Error('Mock GeneralError'); + + return { + forbiddenError, + decorateForbiddenError: jest.fn().mockReturnValue(forbiddenError), + generalError, + decorateGeneralError: jest.fn().mockReturnValue(generalError) + }; +}; + +const createMockAuditLogger = () => { + return { + savedObjectsAuthorizationFailure: jest.fn(), + savedObjectsAuthorizationSuccess: jest.fn(), + }; +}; + +describe('#errors', () => { + test(`assigns errors from constructor to .errors`, () => { + const errors = Symbol(); + + const client = new SecureSavedObjectsClient({ errors }); + + expect(client.errors).toBe(errors); + }); +}); + +describe('#create', () => { + test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type}/create` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.create(type)).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/create`]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => { + throw new Error(); + }); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.create(type)).rejects.toThrowError(mockErrors.generalError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/create`]); + expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + }); + + test(`calls and returns result of repository.create`, async () => { + const type = 'foo'; + const returnValue = Symbol(); + const mockRepository = { + create: jest.fn().mockReturnValue(returnValue) + }; + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: true + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + const attributes = Symbol(); + const options = Symbol(); + + const result = await client.create(type, attributes, options); + + expect(result).toBe(returnValue); + expect(mockRepository.create).toHaveBeenCalledWith(type, attributes, options); + }); +}); + +describe('#bulkCreate', () => { + test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { + const type1 = 'foo'; + const type2 = 'bar'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type1}/bulk_create` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + const objects = [ + { type: type1 }, + { type: type1 }, + { type: type2 }, + ]; + + await expect(client.bulkCreate(objects)).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([ + `action:saved_objects/${type1}/bulk_create`, + `action:saved_objects/${type2}/bulk_create` + ]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => { + throw new Error(); + }); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.bulkCreate([{ type }])).rejects.toThrowError(mockErrors.generalError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/bulk_create`]); + expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + }); + + test(`calls and returns result of repository.bulkCreate`, async () => { + const returnValue = Symbol(); + const mockRepository = { + bulkCreate: jest.fn().mockReturnValue(returnValue) + }; + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: true + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + const objects = [ + { type: 'foo', otherThing: 'sup' }, + { type: 'bar', otherThing: 'everyone' }, + ]; + const options = Symbol(); + + const result = await client.bulkCreate(objects, options); + + expect(result).toBe(returnValue); + expect(mockRepository.bulkCreate).toHaveBeenCalledWith(objects, options); + }); +}); + +describe('#delete', () => { + test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type}/delete` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.delete(type, 'foo-id')).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/delete`]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => { + throw new Error(); + }); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.delete(type)).rejects.toThrowError(mockErrors.generalError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/delete`]); + expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + }); + + test(`calls and returns result of repository.delete`, async () => { + const type = 'foo'; + const returnValue = Symbol(); + const mockRepository = { + delete: jest.fn().mockReturnValue(returnValue) + }; + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: true + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + const id = Symbol(); + + const result = await client.delete(type, id); + + expect(result).toBe(returnValue); + expect(mockRepository.delete).toHaveBeenCalledWith(type, id); + }); +}); + +describe('#find', () => { + describe('type', () => { + test(`throws decorated ForbiddenError when type is sinuglar and user isn't authorized`, async () => { + const type = 'foo'; + const mockRepository = {}; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type}/find` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.find({ type })).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/find`]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated ForbiddenError when type is an array and user isn't authorized for one type`, async () => { + const type1 = 'foo'; + const type2 = 'bar'; + const mockRepository = {}; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type1}/find` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.find({ type: [ type1, type2 ] })).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type1}/find`, `action:saved_objects/${type2}/find`]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated ForbiddenError when type is an array and user isn't authorized for either type`, async () => { + const type1 = 'foo'; + const type2 = 'bar'; + const mockRepository = {}; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type1}/find`, + `action:saved_objects/${type2}/find` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.find({ type: [ type1, type2 ] })).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type1}/find`, `action:saved_objects/${type2}/find`]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => { + throw new Error(); + }); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.find({ type })).rejects.toThrowError(mockErrors.generalError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/find`]); + expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + }); + + test(`calls and returns result of repository.find`, async () => { + const type = 'foo'; + const returnValue = Symbol(); + const mockRepository = { + find: jest.fn().mockReturnValue(returnValue) + }; + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: true + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + const result = await client.find({ type }); + + expect(result).toBe(returnValue); + expect(mockRepository.find).toHaveBeenCalledWith({ type }); + }); + }); + + describe('no type', () => { + test(`throws decorated ForbiddenError when user has no authorized types`, async () => { + const type = 'foo'; + const mockRepository = { + getTypes: jest.fn().mockReturnValue([type]) + }; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type}/find` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.find()).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/find`]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { + const type1 = 'foo'; + const type2 = 'bar'; + const mockRepository = { + getTypes: jest.fn().mockReturnValue([type1, type2]) + }; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => { + throw new Error(); + }); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.find()).rejects.toThrowError(mockErrors.generalError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type1}/find`, `action:saved_objects/${type2}/find`]); + expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + }); + + test(`specifies authorized types when calling repository.find()`, async () => { + const type1 = 'foo'; + const type2 = 'bar'; + const mockRepository = { + getTypes: jest.fn().mockReturnValue([type1, type2]), + find: jest.fn(), + }; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type1}/find` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await client.find(); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type1}/find`, `action:saved_objects/${type2}/find`]); + expect(mockRepository.find).toHaveBeenCalledWith(expect.objectContaining({ + type: [type2] + })); + }); + + test(`calls and returns result of repository.find`, async () => { + const type = 'foo'; + const returnValue = Symbol(); + const mockRepository = { + getTypes: jest.fn().mockReturnValue([type]), + find: jest.fn().mockReturnValue(returnValue) + }; + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: true, + missing: [] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + const result = await client.find(); + + expect(result).toBe(returnValue); + expect(mockRepository.find).toHaveBeenCalledWith({ type: [type] }); + }); + }); +}); + +describe('#bulkGet', () => { + test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { + const type1 = 'foo'; + const type2 = 'bar'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type1}/bulk_get` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + const objects = [ + { type: type1 }, + { type: type1 }, + { type: type2 }, + ]; + + await expect(client.bulkGet(objects)).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type1}/bulk_get`, `action:saved_objects/${type2}/bulk_get`]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => { + throw new Error(); + }); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.bulkGet([{ type }])).rejects.toThrowError(mockErrors.generalError); + + expect(mockHasPrivileges).toHaveBeenCalledWith(['action:saved_objects/foo/bulk_get']); + expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + }); + + test(`calls and returns result of repository.bulkGet`, async () => { + const type1 = 'foo'; + const type2 = 'bar'; + const returnValue = Symbol(); + const mockRepository = { + bulkGet: jest.fn().mockReturnValue(returnValue) + }; + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: true + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + const objects = [ + { type: type1, id: 'foo-id' }, + { type: type2, id: 'bar-id' }, + ]; + + const result = await client.bulkGet(objects); + + expect(result).toBe(returnValue); + expect(mockRepository.bulkGet).toHaveBeenCalledWith(objects); + }); +}); + +describe('#get', () => { + test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + `action:saved_objects/${type}/get` + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.get(type, 'foo-id')).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/get`]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => { + throw new Error(); + }); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.get(type)).rejects.toThrowError(mockErrors.generalError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/get`]); + expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + }); + + test(`calls and returns result of repository.get`, async () => { + const type = 'foo'; + const returnValue = Symbol(); + const mockRepository = { + get: jest.fn().mockReturnValue(returnValue) + }; + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: true + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + const id = Symbol(); + + const result = await client.get(type, id); + + expect(result).toBe(returnValue); + expect(mockRepository.get).toHaveBeenCalledWith(type, id); + }); +}); + +describe('#update', () => { + test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: false, + missing: [ + 'action:saved_objects/foo/update' + ] + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.update(type)).rejects.toThrowError(mockErrors.forbiddenError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/update`]); + expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + }); + + test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { + const type = 'foo'; + const mockErrors = createMockErrors(); + const mockHasPrivileges = jest.fn().mockImplementation(async () => { + throw new Error(); + }); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + errors: mockErrors, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + + await expect(client.update(type)).rejects.toThrowError(mockErrors.generalError); + + expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/update`]); + expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + }); + + test(`calls and returns result of repository.update`, async () => { + const type = 'foo'; + const returnValue = Symbol(); + const mockRepository = { + update: jest.fn().mockReturnValue(returnValue) + }; + const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ + success: true + })); + const mockAuditLogger = createMockAuditLogger(); + const client = new SecureSavedObjectsClient({ + repository: mockRepository, + hasPrivileges: mockHasPrivileges, + auditLogger: mockAuditLogger, + }); + const id = Symbol(); + const attributes = Symbol(); + const options = Symbol(); + + const result = await client.update(type, id, attributes, options); + + expect(result).toBe(returnValue); + expect(mockRepository.update).toHaveBeenCalledWith(type, id, attributes, options); + }); +}); From 428286499a5c8fdb40dfdcfea1e90eb31cfb981f Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 6 Jun 2018 10:07:50 -0400 Subject: [PATCH 03/12] Revert "Adding ability to specify filters when calling the repository" This reverts commit 9da30a15dbe2ce73e7e4fee6d2565b3bbc32b884. --- .../saved_objects/service/lib/repository.js | 5 +- .../service/lib/repository.test.js | 7 +- .../service/lib/search_dsl/query_params.js | 52 ++-- .../lib/search_dsl/query_params.test.js | 274 +----------------- .../service/lib/search_dsl/search_dsl.js | 5 +- .../service/lib/search_dsl/search_dsl.test.js | 6 +- 6 files changed, 28 insertions(+), 321 deletions(-) diff --git a/src/server/saved_objects/service/lib/repository.js b/src/server/saved_objects/service/lib/repository.js index 9d5f1ba00226a..b7675adb5239a 100644 --- a/src/server/saved_objects/service/lib/repository.js +++ b/src/server/saved_objects/service/lib/repository.js @@ -212,7 +212,6 @@ export class SavedObjectsRepository { * @property {string} [options.sortField] * @property {string} [options.sortOrder] * @property {Array} [options.fields] - * @property {Array} [options.filters] * @returns {promise} - { saved_objects: [{ id, type, version, attributes }], total, per_page, page } */ async find(options = {}) { @@ -225,7 +224,6 @@ export class SavedObjectsRepository { sortField, sortOrder, fields, - filters, } = options; if (searchFields && !Array.isArray(searchFields)) { @@ -249,8 +247,7 @@ export class SavedObjectsRepository { searchFields, type, sortField, - sortOrder, - filters, + sortOrder }) } }; diff --git a/src/server/saved_objects/service/lib/repository.test.js b/src/server/saved_objects/service/lib/repository.test.js index b8c6a310cda5c..a02bfb9abb619 100644 --- a/src/server/saved_objects/service/lib/repository.test.js +++ b/src/server/saved_objects/service/lib/repository.test.js @@ -388,18 +388,13 @@ describe('SavedObjectsRepository', () => { } }); - it('passes mappings, search, searchFields, type, sortField, sortOrder, and filters to getSearchDsl', async () => { + it('passes mappings, search, searchFields, type, sortField, and sortOrder to getSearchDsl', async () => { const relevantOpts = { search: 'foo*', searchFields: ['foo'], type: 'bar', sortField: 'name', sortOrder: 'desc', - filters: [{ - terms: { - type: ['foo', 'bar'] - } - }], }; await savedObjectsRepository.find(relevantOpts); diff --git a/src/server/saved_objects/service/lib/search_dsl/query_params.js b/src/server/saved_objects/service/lib/search_dsl/query_params.js index e2f85973ca08e..bcf62f21ef415 100644 --- a/src/server/saved_objects/service/lib/search_dsl/query_params.js +++ b/src/server/saved_objects/service/lib/search_dsl/query_params.js @@ -67,43 +67,33 @@ function getFieldsForTypes(searchFields, types) { * @param {Array} searchFields * @return {Object} */ -export function getQueryParams(mappings, type, search, searchFields, filters = []) { - const filter = [...filters]; - const must = []; - - if (type) { - filter.push({ [Array.isArray(type) ? 'terms' : 'term']: { type } }); - } - - if (search) { - must.push({ - simple_query_string: { - query: search, - ...getFieldsForTypes( - searchFields, - getTypes(mappings, type) - ) - } - }); - } - - if (filter.length === 0 && must.length === 0) { +export function getQueryParams(mappings, type, search, searchFields) { + if (!type && !search) { return {}; } - const result = { - query: { - bool: {} - } - }; + const bool = {}; - if (filter.length > 0) { - result.query.bool.filter = filter; + if (type) { + bool.filter = [ + { [Array.isArray(type) ? 'terms' : 'term']: { type } } + ]; } - if (must.length > 0) { - result.query.bool.must = must; + if (search) { + bool.must = [ + ...bool.must || [], + { + simple_query_string: { + query: search, + ...getFieldsForTypes( + searchFields, + getTypes(mappings, type) + ) + } + } + ]; } - return result; + return { query: { bool } }; } diff --git a/src/server/saved_objects/service/lib/search_dsl/query_params.test.js b/src/server/saved_objects/service/lib/search_dsl/query_params.test.js index 7b1a6f5dc7eb0..53b943ee6793b 100644 --- a/src/server/saved_objects/service/lib/search_dsl/query_params.test.js +++ b/src/server/saved_objects/service/lib/search_dsl/query_params.test.js @@ -95,36 +95,6 @@ describe('searchDsl/queryParams', () => { }); }); - describe('{type,filters}', () => { - it('includes filters and a term filter for type when type is a string', () => { - expect(getQueryParams(MAPPINGS, 'saved', null, null, [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - { term: { type: 'saved' } } - ] - } - } - }); - }); - - it('includes filters and a terms filter for type when type is an array', () => { - expect(getQueryParams(MAPPINGS, ['saved', 'vis'], null, null, [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - { terms: { type: ['saved', 'vis'] } } - ] - } - } - }); - }); - }); - describe('{search}', () => { it('includes just a sqs query', () => { expect(getQueryParams(MAPPINGS, null, 'us*')) @@ -145,31 +115,8 @@ describe('searchDsl/queryParams', () => { }); }); - describe('{search,filters}', () => { - it('includes filters and a sqs query', () => { - expect(getQueryParams(MAPPINGS, null, 'us*', null, [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } } - ], - must: [ - { - simple_query_string: { - query: 'us*', - all_fields: true - } - } - ] - } - } - }); - }); - }); - describe('{type,search}', () => { - it('includes bool with sqs query and term filter for type when type is a string', () => { + it('includes bool with sqs query and term filter for type', () => { expect(getQueryParams(MAPPINGS, 'saved', 'y*')) .toEqual({ query: { @@ -189,7 +136,7 @@ describe('searchDsl/queryParams', () => { } }); }); - it('includes bool with sqs query and terms filter for type when type is an array', () => { + it('includes bool with sqs query and terms filter for type', () => { expect(getQueryParams(MAPPINGS, ['saved', 'vis'], 'y*')) .toEqual({ query: { @@ -211,52 +158,6 @@ describe('searchDsl/queryParams', () => { }); }); - describe('{type,search,filters}', () => { - it('includes bool with sqs query, filters and term filter for type when type is a string', () => { - expect(getQueryParams(MAPPINGS, 'saved', 'y*', null, [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - { term: { type: 'saved' } } - ], - must: [ - { - simple_query_string: { - query: 'y*', - all_fields: true - } - } - ] - } - } - }); - }); - - it('includes bool with sqs query, filters and terms filter for type when type is an array', () => { - expect(getQueryParams(MAPPINGS, ['saved', 'vis'], 'y*', null, [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - { terms: { type: ['saved', 'vis'] } } - ], - must: [ - { - simple_query_string: { - query: 'y*', - all_fields: true - } - } - ] - } - } - }); - }); - }); - describe('{search,searchFields}', () => { it('includes all types for field', () => { expect(getQueryParams(MAPPINGS, null, 'y*', ['title'])) @@ -322,80 +223,6 @@ describe('searchDsl/queryParams', () => { }); }); - describe('{search,searchFields,filters}', () => { - it('includes all types for field and includes filter', () => { - expect(getQueryParams(MAPPINGS, null, 'y*', ['title'], [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - ], - must: [ - { - simple_query_string: { - query: 'y*', - fields: [ - 'pending.title', - 'saved.title' - ] - } - } - ] - } - } - }); - }); - it('supports field boosting and includes filter', () => { - expect(getQueryParams(MAPPINGS, null, 'y*', ['title^3'], [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - ], - must: [ - { - simple_query_string: { - query: 'y*', - fields: [ - 'pending.title^3', - 'saved.title^3' - ] - } - } - ] - } - } - }); - }); - it('supports field and multi-field and includes filter', () => { - expect(getQueryParams(MAPPINGS, null, 'y*', ['title', 'title.raw'], [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - ], - must: [ - { - simple_query_string: { - query: 'y*', - fields: [ - 'pending.title', - 'saved.title', - 'pending.title.raw', - 'saved.title.raw', - ] - } - } - ] - } - } - }); - }); - }); - describe('{type,search,searchFields}', () => { it('includes bool, with term filter and sqs with field list', () => { expect(getQueryParams(MAPPINGS, 'saved', 'y*', ['title'])) @@ -488,101 +315,4 @@ describe('searchDsl/queryParams', () => { }); }); }); - - describe('{type,search,searchFields,filters}', () => { - it('includes bool, with term filter and filter and sqs with field list', () => { - expect(getQueryParams(MAPPINGS, 'saved', 'y*', ['title'], [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - { term: { type: 'saved' } } - ], - must: [ - { - simple_query_string: { - query: 'y*', - fields: [ - 'saved.title' - ] - } - } - ] - } - } - }); - }); - it('includes bool, with terms filter and filter and sqs with field list', () => { - expect(getQueryParams(MAPPINGS, ['saved', 'vis'], 'y*', ['title'], [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - { terms: { type: ['saved', 'vis'] } } - ], - must: [ - { - simple_query_string: { - query: 'y*', - fields: [ - 'saved.title', - 'vis.title' - ] - } - } - ] - } - } - }); - }); - it('supports fields pointing to multi-fields and filter', () => { - expect(getQueryParams(MAPPINGS, 'saved', 'y*', ['title.raw'], [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - { term: { type: 'saved' } } - ], - must: [ - { - simple_query_string: { - query: 'y*', - fields: [ - 'saved.title.raw' - ] - } - } - ] - } - } - }); - }); - it('supports multiple search fields and filter', () => { - expect(getQueryParams(MAPPINGS, 'saved', 'y*', ['title', 'title.raw'], [{ terms: { foo: ['bar', 'baz' ] } }])) - .toEqual({ - query: { - bool: { - filter: [ - { terms: { foo: ['bar', 'baz' ] } }, - { term: { type: 'saved' } } - ], - must: [ - { - simple_query_string: { - query: 'y*', - fields: [ - 'saved.title', - 'saved.title.raw' - ] - } - } - ] - } - } - }); - }); - }); }); diff --git a/src/server/saved_objects/service/lib/search_dsl/search_dsl.js b/src/server/saved_objects/service/lib/search_dsl/search_dsl.js index 6843ebc306e1b..ea34c127e9854 100644 --- a/src/server/saved_objects/service/lib/search_dsl/search_dsl.js +++ b/src/server/saved_objects/service/lib/search_dsl/search_dsl.js @@ -28,8 +28,7 @@ export function getSearchDsl(mappings, options = {}) { search, searchFields, sortField, - sortOrder, - filters, + sortOrder } = options; if (!type && sortField) { @@ -41,7 +40,7 @@ export function getSearchDsl(mappings, options = {}) { } return { - ...getQueryParams(mappings, type, search, searchFields, filters), + ...getQueryParams(mappings, type, search, searchFields), ...getSortingParams(mappings, type, sortField, sortOrder), }; } diff --git a/src/server/saved_objects/service/lib/search_dsl/search_dsl.test.js b/src/server/saved_objects/service/lib/search_dsl/search_dsl.test.js index b95f9f9967104..85302b5e25722 100644 --- a/src/server/saved_objects/service/lib/search_dsl/search_dsl.test.js +++ b/src/server/saved_objects/service/lib/search_dsl/search_dsl.test.js @@ -46,16 +46,13 @@ describe('getSearchDsl', () => { }); describe('passes control', () => { - it('passes (mappings, type, search, searchFields, filters) to getQueryParams', () => { + it('passes (mappings, type, search, searchFields) to getQueryParams', () => { const spy = sandbox.spy(queryParamsNS, 'getQueryParams'); const mappings = { type: { properties: {} } }; const opts = { type: 'foo', search: 'bar', searchFields: ['baz'], - filters: [ - { terms: { foo: ['bar', 'baz'] } } - ] }; getSearchDsl(mappings, opts); @@ -66,7 +63,6 @@ describe('getSearchDsl', () => { opts.type, opts.search, opts.searchFields, - opts.filters, ); }); From 1550797203f076782967534dc0ea244653ffa015 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 6 Jun 2018 12:08:44 -0400 Subject: [PATCH 04/12] Adding integration tests for find filtering --- .../secure_saved_objects_client.js | 2 +- .../apis/saved_objects/find.js | 104 ++++++- x-pack/test/rbac_api_integration/config.js | 7 +- .../saved_objects/basic/data.json.gz | Bin 0 -> 1837 bytes .../saved_objects/basic/mappings.json | 283 ++++++++++++++++++ 5 files changed, 380 insertions(+), 16 deletions(-) create mode 100644 x-pack/test/rbac_api_integration/fixtures/es_archiver/saved_objects/basic/data.json.gz create mode 100644 x-pack/test/rbac_api_integration/fixtures/es_archiver/saved_objects/basic/mappings.json diff --git a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js index 9b4892d94385a..6a30b1e8e063a 100644 --- a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js +++ b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js @@ -73,7 +73,7 @@ export class SecureSavedObjectsClient { if (authorizedTypes.length === 0) { this._auditLogger.savedObjectsAuthorizationFailure(result.username, action, types, result.missing, { options }); - throw this.errors.decorateForbiddenError(new Error(`Not authorized to search any types`)); + throw this.errors.decorateForbiddenError(new Error(`Not authorized to find any types`)); } this._auditLogger.savedObjectsAuthorizationSuccess(result.username, action, authorizedTypes, { options }); diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/find.js b/x-pack/test/rbac_api_integration/apis/saved_objects/find.js index 72a3fa2ec2bfc..4c38f42b5416e 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/find.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/find.js @@ -13,7 +13,7 @@ export default function ({ getService }) { describe('find', () => { - const expectResults = (resp) => { + const expectVisualizationResults = (resp) => { expect(resp.body).to.eql({ page: 1, per_page: 20, @@ -31,6 +31,44 @@ export default function ({ getService }) { }); }; + const expectAllResults = (resp) => { + expect(resp.body).to.eql({ + page: 1, + per_page: 20, + total: 4, + saved_objects: [ + { + id: '91200a00-9efd-11e7-acb3-3dab96693fab', + type: 'index-pattern', + updated_at: '2017-09-21T18:49:16.270Z', + version: 1, + attributes: resp.body.saved_objects[0].attributes + }, + { + id: '7.0.0-alpha1', + type: 'config', + updated_at: '2017-09-21T18:49:16.302Z', + version: 1, + attributes: resp.body.saved_objects[1].attributes + }, + { + id: 'dd7caf20-9efd-11e7-acb3-3dab96693fab', + type: 'visualization', + updated_at: '2017-09-21T18:51:23.794Z', + version: 1, + attributes: resp.body.saved_objects[2].attributes + }, + { + id: 'be3733a0-9efe-11e7-acb3-3dab96693fab', + type: 'dashboard', + updated_at: '2017-09-21T18:57:40.826Z', + version: 1, + attributes: resp.body.saved_objects[3].attributes + }, + ] + }); + }; + const createExpectEmpty = (page, perPage, total) => (resp) => { expect(resp.body).to.eql({ page: page, @@ -40,7 +78,7 @@ export default function ({ getService }) { }); }; - const createExpectForbidden = (canLogin, type) => resp => { + const createExpectActionForbidden = (canLogin, type) => resp => { expect(resp.body).to.eql({ statusCode: 403, error: 'Forbidden', @@ -48,6 +86,14 @@ export default function ({ getService }) { }); }; + const expectForbiddenCantFindAnyTypes = resp => { + expect(resp.body).to.eql({ + statusCode: 403, + error: 'Forbidden', + message: `Not authorized to find any types` + }); + }; + const findTest = (description, { auth, tests }) => { describe(description, () => { before(() => esArchiver.load('saved_objects/basic')); @@ -90,6 +136,16 @@ export default function ({ getService }) { .then(tests.unknownSearchField.response) )); }); + + describe('no type', () => { + it(`should return ${tests.noType.statusCode} with ${tests.noType.description}`, async () => ( + await supertest + .get('/api/saved_objects/_find') + .auth(auth.username, auth.password) + .expect(tests.noType.statusCode) + .then(tests.noType.response) + )); + }); }); }; @@ -102,23 +158,28 @@ export default function ({ getService }) { normal: { description: 'forbidden login and find visualization message', statusCode: 403, - response: createExpectForbidden(false, 'visualization'), + response: createExpectActionForbidden(false, 'visualization'), }, unknownType: { description: 'forbidden login and find wigwags message', statusCode: 403, - response: createExpectForbidden(false, 'wigwags'), + response: createExpectActionForbidden(false, 'wigwags'), }, pageBeyondTotal: { description: 'forbidden login and find visualization message', statusCode: 403, - response: createExpectForbidden(false, 'visualization'), + response: createExpectActionForbidden(false, 'visualization'), }, unknownSearchField: { description: 'forbidden login and find wigwags message', statusCode: 403, - response: createExpectForbidden(false, 'wigwags'), + response: createExpectActionForbidden(false, 'wigwags'), }, + noType: { + description: `forbidded can't find any types`, + statusCode: 403, + response: expectForbiddenCantFindAnyTypes, + } } }); @@ -129,9 +190,9 @@ export default function ({ getService }) { }, tests: { normal: { - description: 'individual responses', + description: 'only the visualization', statusCode: 200, - response: expectResults, + response: expectVisualizationResults, }, unknownType: { description: 'empty result', @@ -148,6 +209,11 @@ export default function ({ getService }) { statusCode: 200, response: createExpectEmpty(1, 20, 0), }, + noType: { + description: 'all objects', + statusCode: 200, + response: expectAllResults, + }, }, }); @@ -158,9 +224,9 @@ export default function ({ getService }) { }, tests: { normal: { - description: 'individual responses', + description: 'only the visualization', statusCode: 200, - response: expectResults, + response: expectVisualizationResults, }, unknownType: { description: 'empty result', @@ -177,6 +243,11 @@ export default function ({ getService }) { statusCode: 200, response: createExpectEmpty(1, 20, 0), }, + noType: { + description: 'all objects', + statusCode: 200, + response: expectAllResults, + }, }, }); @@ -187,14 +258,14 @@ export default function ({ getService }) { }, tests: { normal: { - description: 'individual responses', + description: 'only the visualization', statusCode: 200, - response: expectResults, + response: expectVisualizationResults, }, unknownType: { description: 'forbidden find wigwags message', statusCode: 403, - response: createExpectForbidden(true, 'wigwags'), + response: createExpectActionForbidden(true, 'wigwags'), }, pageBeyondTotal: { description: 'empty result', @@ -204,7 +275,12 @@ export default function ({ getService }) { unknownSearchField: { description: 'forbidden find wigwags message', statusCode: 403, - response: createExpectForbidden(true, 'wigwags'), + response: createExpectActionForbidden(true, 'wigwags'), + }, + noType: { + description: 'all objects', + statusCode: 200, + response: expectAllResults, }, } }); diff --git a/x-pack/test/rbac_api_integration/config.js b/x-pack/test/rbac_api_integration/config.js index 481b14913da91..d454ac014b42c 100644 --- a/x-pack/test/rbac_api_integration/config.js +++ b/x-pack/test/rbac_api_integration/config.js @@ -33,8 +33,13 @@ export default async function ({ readConfigFile }) { reportName: 'X-Pack RBAC API Integration Tests', }, + // The saved_objects/basic archives are almost an exact replica of the ones in OSS + // with the exception of a bogus "not-a-visualization" type that I added to make sure + // the find filtering without a type specified worked correctly. Once we have the ability + // to specify more granular access to the objects via the Kibana privileges, this should + // no longer be necessarly, and it's only required as long as we do read/all privileges. esArchiver: { - directory: resolveKibanaPath(path.join('test', 'api_integration', 'fixtures', 'es_archiver')) + directory: path.join(__dirname, 'fixtures', 'es_archiver') }, esTestCluster: { diff --git a/x-pack/test/rbac_api_integration/fixtures/es_archiver/saved_objects/basic/data.json.gz b/x-pack/test/rbac_api_integration/fixtures/es_archiver/saved_objects/basic/data.json.gz new file mode 100644 index 0000000000000000000000000000000000000000..910382479979df82f1c8d640cf8aba428bbb393f GIT binary patch literal 1837 zcmV+|2h#W-iwFP!000026YX1DZ`(E$e$THkytg4XBs+~0J$38Qp+j$If(}>{7_@jK zaiv6!q;hHC|2~qs$IfkO>SHV~ZFEQ;e*AcDmdLA}!CBwS2KQ%V+x z`}>E}h%D;yN)$3|r|wMB(^+*l%|%X$20AC&cA9wpY~&q|CjPO15bPZW{{DC}^Zsi4 z_tmuX*qNB-ZYnNfrHM*LKR4rCa|*8+aQdF4uG>p1F&)#q+byzPlx_cVbu!FM-;-f* zGJI*eDiWKA-4nMaCskUqEOxR`6qz0v#Fd++W2WnYZ8Hr;F zG0~N@?ka)M*HWaviSV=CNL9Bjch?~rOLG2%s4D3?P`2qBCQV|6h$0II7eBSh^$}Sg z*aV(Aqnn{-&1TJ=YvX~VLLslYdsd_ikPn7a3m<39^D?&f5p{(dfK=i@vSTI|+WD{q z9|s3h@L61HP~XL%zXO<%GeZx%76+^6AB+VqG-Qg243F_NkT4lg3}PG#At}qqiYb}K zc`jJxfg3iv|-r*NfN9WwWO)T_3nn z4i2iFLXqb=%f2ECI1Ub`T}nxqunfim5lJm3j7&+AVhKShq(WZeL8HmoD3oY+%mR|Y ziJJzEbz4-00y&PDRJxSo1INiaY@4&LJjrpHyMzcNpfUE!&R>a4+jkH|Y8G+`QbAe7 zfX?k(NE|O9Z$O$C3(i!s4Nw(?8r7?V+i_ybqBkJbBu3As!HAei5eMeCzMB@aY4%~_ z=98tUKK|VRa0@sL5@2zn(NkIB+y+L^8Py4XLLE%pd@814j;jj41te}R-Ej=eJUW2L zIegmhtB2qe!Sp^}H6fKKgkXyjzHWcP-ux>;DOWdVU4<%wYF{g5LdHkOO6sjND4iGMmR4y`w$}dYqm{?Ld2?5df;1Ule zhBkKwb`=Y%5Q!`%7CJ=qHR(bjaw-F6!#j04tZv{ecj|waJIkG&t0%eN6pAcjaWETu zV{b%wo)G`;3ryvphumD6dXvu0rD}Zd%F?1>oW3toBR$yLpLMPwQHV&v^_zNW_qv4j zBRM={svw+wBlg!}Kzt;i?%H^lf*(^zGYeKo7SWB zrJK=q5nM<&{wDwHV2~n6IPwpjzy&5327UfYU zm5B4!VH+Sz=lY~Z)#&5~xjp4$+c{YPuU6exWB$uR7ik0E^K9Rm)2rq3)I2(CTwTrQ zQrs-1iCAwT((9J!^sKIro9>TUooP-5txlN;nG$~2ilxxydTq>%7}xf6NVC7U@L~Rh z3*B0q-SJZwWF-Uju{NwP?Tk(x++c|fY^6!~wL1G`+=-NPxdn75+ike_%Oz5i7@T?j z3i=!%`)JSjs}6?-#49F44jrNC0x}o8Tw;&MvwBNcH`uL{P4&XQCsG|%sd{MzopvP@Ce%&jyg$ZXQ;%+)$j!;uOTNJ)K6O0w{p z+b>;$t<+BLP-~;xJYhx)g@xZ*CVuDey}$Uh_Hwvs}NGKUvNfBL4| zHYq?OIn^TyHT=WE=c}y^x=IKC{=!0VympKtueBK z^U@J`vjxkZw$8gotF0G~L8?;hccf*WIwEI4KP>)+P`?9BdePqI8dsVexs1~RTAkN6 z9NZ_bI+hwAc1C5QzpU0;|9LWW{W!_u!o7&Jc3*^$xu894P%VFCmZn4{3qd3e7BHPn zr^GE3{&R+I^xZQ%&w@Q~d@$MXWan1J`po=kO+rqXwpX&f``JrQG82{S?RJrpeS^8( z@crHD!S2}9d6enQcX%0xbvbmjfr0;Kd~HXKM#ah$N__4nfyll`@rca%t6mh@#Fwx1)u!D=714 zx21Lvul29*6eAw$;wv#CG5Ltwob-zedR5e~Hmd&Nv!S@~;Od>+9UC&CM`ZL|!#~&X b&o%sW4gXxjKiBa8uNwYeR=}DNj7b0hF` Date: Wed, 6 Jun 2018 13:19:26 -0400 Subject: [PATCH 05/12] Adding forbidden auth logging --- .../secure_saved_objects_client.test.js | 168 ++++++++++++++++-- 1 file changed, 149 insertions(+), 19 deletions(-) diff --git a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js index e743620afe4f9..2d9e5f19c6ff0 100644 --- a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js +++ b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js @@ -38,12 +38,14 @@ describe('#errors', () => { describe('#create', () => { test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { const type = 'foo'; + const username = Symbol(); const mockErrors = createMockErrors(); const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ success: false, missing: [ `action:saved_objects/${type}/create` - ] + ], + username })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -51,11 +53,24 @@ describe('#create', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const attributes = Symbol(); + const options = Symbol(); - await expect(client.create(type)).rejects.toThrowError(mockErrors.forbiddenError); + await expect(client.create(type, attributes, options)).rejects.toThrowError(mockErrors.forbiddenError); expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/create`]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'create', + [type], + [`action:saved_objects/${type}/create`], + { + type, + attributes, + options, + } + ); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -106,12 +121,14 @@ describe('#bulkCreate', () => { test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { const type1 = 'foo'; const type2 = 'bar'; + const username = Symbol(); const mockErrors = createMockErrors(); const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ success: false, missing: [ `action:saved_objects/${type1}/bulk_create` - ] + ], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -124,14 +141,25 @@ describe('#bulkCreate', () => { { type: type1 }, { type: type2 }, ]; + const options = Symbol(); - await expect(client.bulkCreate(objects)).rejects.toThrowError(mockErrors.forbiddenError); + await expect(client.bulkCreate(objects, options)).rejects.toThrowError(mockErrors.forbiddenError); expect(mockHasPrivileges).toHaveBeenCalledWith([ `action:saved_objects/${type1}/bulk_create`, `action:saved_objects/${type2}/bulk_create` ]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'bulk_create', + [type2, type1], + [`action:saved_objects/${type1}/bulk_create`], + { + objects, + options, + } + ); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -183,12 +211,14 @@ describe('#bulkCreate', () => { describe('#delete', () => { test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { const type = 'foo'; + const username = Symbol(); const mockErrors = createMockErrors(); const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ success: false, missing: [ `action:saved_objects/${type}/delete` - ] + ], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -196,11 +226,22 @@ describe('#delete', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const id = Symbol(); - await expect(client.delete(type, 'foo-id')).rejects.toThrowError(mockErrors.forbiddenError); + await expect(client.delete(type, id)).rejects.toThrowError(mockErrors.forbiddenError); expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/delete`]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'delete', + [type], + [`action:saved_objects/${type}/delete`], + { + type, + id, + } + ); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -250,13 +291,15 @@ describe('#find', () => { describe('type', () => { test(`throws decorated ForbiddenError when type is sinuglar and user isn't authorized`, async () => { const type = 'foo'; + const username = Symbol(); const mockRepository = {}; const mockErrors = createMockErrors(); const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ success: false, missing: [ `action:saved_objects/${type}/find` - ] + ], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -265,23 +308,35 @@ describe('#find', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const options = { type }; - await expect(client.find({ type })).rejects.toThrowError(mockErrors.forbiddenError); + await expect(client.find(options)).rejects.toThrowError(mockErrors.forbiddenError); expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/find`]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'find', + [type], + [`action:saved_objects/${type}/find`], + { + options + } + ); }); test(`throws decorated ForbiddenError when type is an array and user isn't authorized for one type`, async () => { const type1 = 'foo'; const type2 = 'bar'; + const username = Symbol(); const mockRepository = {}; const mockErrors = createMockErrors(); const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ success: false, missing: [ `action:saved_objects/${type1}/find` - ] + ], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -290,16 +345,27 @@ describe('#find', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const options = { type: [ type1, type2 ] }; - await expect(client.find({ type: [ type1, type2 ] })).rejects.toThrowError(mockErrors.forbiddenError); + await expect(client.find(options)).rejects.toThrowError(mockErrors.forbiddenError); expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type1}/find`, `action:saved_objects/${type2}/find`]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'find', + [type2, type1], + [`action:saved_objects/${type1}/find`], + { + options + } + ); }); test(`throws decorated ForbiddenError when type is an array and user isn't authorized for either type`, async () => { const type1 = 'foo'; const type2 = 'bar'; + const username = Symbol(); const mockRepository = {}; const mockErrors = createMockErrors(); const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ @@ -307,7 +373,8 @@ describe('#find', () => { missing: [ `action:saved_objects/${type1}/find`, `action:saved_objects/${type2}/find` - ] + ], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -316,11 +383,21 @@ describe('#find', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const options = { type: [ type1, type2 ] }; - await expect(client.find({ type: [ type1, type2 ] })).rejects.toThrowError(mockErrors.forbiddenError); + await expect(client.find(options)).rejects.toThrowError(mockErrors.forbiddenError); expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type1}/find`, `action:saved_objects/${type2}/find`]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'find', + [type2, type1], + [`action:saved_objects/${type2}/find`, `action:saved_objects/${type1}/find`], + { + options + } + ); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -368,6 +445,7 @@ describe('#find', () => { describe('no type', () => { test(`throws decorated ForbiddenError when user has no authorized types`, async () => { const type = 'foo'; + const username = Symbol(); const mockRepository = { getTypes: jest.fn().mockReturnValue([type]) }; @@ -376,7 +454,8 @@ describe('#find', () => { success: false, missing: [ `action:saved_objects/${type}/find` - ] + ], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -385,11 +464,21 @@ describe('#find', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const options = Symbol(); - await expect(client.find()).rejects.toThrowError(mockErrors.forbiddenError); + await expect(client.find(options)).rejects.toThrowError(mockErrors.forbiddenError); expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/find`]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'find', + [type], + [`action:saved_objects/${type}/find`], + { + options + } + ); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -476,12 +565,14 @@ describe('#bulkGet', () => { test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { const type1 = 'foo'; const type2 = 'bar'; + const username = Symbol(); const mockErrors = createMockErrors(); const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ success: false, missing: [ `action:saved_objects/${type1}/bulk_get` - ] + ], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -499,6 +590,15 @@ describe('#bulkGet', () => { expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type1}/bulk_get`, `action:saved_objects/${type2}/bulk_get`]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'bulk_get', + [type2, type1], + [`action:saved_objects/${type1}/bulk_get`], + { + objects + } + ); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -551,12 +651,14 @@ describe('#bulkGet', () => { describe('#get', () => { test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { const type = 'foo'; + const username = Symbol(); const mockErrors = createMockErrors(); const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ success: false, missing: [ `action:saved_objects/${type}/get` - ] + ], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -564,11 +666,22 @@ describe('#get', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const id = Symbol(); - await expect(client.get(type, 'foo-id')).rejects.toThrowError(mockErrors.forbiddenError); + await expect(client.get(type, id)).rejects.toThrowError(mockErrors.forbiddenError); expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/get`]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'get', + [type], + [`action:saved_objects/${type}/get`], + { + type, + id, + } + ); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -617,12 +730,14 @@ describe('#get', () => { describe('#update', () => { test(`throws decorated ForbiddenError when user doesn't have privileges`, async () => { const type = 'foo'; + const username = Symbol(); const mockErrors = createMockErrors(); const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ success: false, missing: [ 'action:saved_objects/foo/update' - ] + ], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -630,11 +745,26 @@ describe('#update', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const id = Symbol(); + const attributes = Symbol(); + const options = Symbol(); - await expect(client.update(type)).rejects.toThrowError(mockErrors.forbiddenError); + await expect(client.update(type, id, attributes, options)).rejects.toThrowError(mockErrors.forbiddenError); expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/update`]); expect(mockErrors.decorateForbiddenError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).toHaveBeenCalledWith( + username, + 'update', + [type], + [`action:saved_objects/${type}/update`], + { + type, + id, + attributes, + options, + } + ); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { From fa8e9a3202d490b7bda653dea29a17e98200f54a Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 6 Jun 2018 13:23:05 -0400 Subject: [PATCH 06/12] Adding asserts to make sure some audit log isn't used --- .../secure_saved_objects_client.test.js | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js index 2d9e5f19c6ff0..25d18dffa73fc 100644 --- a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js +++ b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js @@ -71,6 +71,7 @@ describe('#create', () => { options, } ); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -90,6 +91,8 @@ describe('#create', () => { expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/create`]); expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`calls and returns result of repository.create`, async () => { @@ -179,6 +182,8 @@ describe('#bulkCreate', () => { expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/bulk_create`]); expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`calls and returns result of repository.bulkCreate`, async () => { @@ -242,6 +247,7 @@ describe('#delete', () => { id, } ); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -261,6 +267,8 @@ describe('#delete', () => { expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/delete`]); expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`calls and returns result of repository.delete`, async () => { @@ -323,6 +331,7 @@ describe('#find', () => { options } ); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`throws decorated ForbiddenError when type is an array and user isn't authorized for one type`, async () => { @@ -360,6 +369,7 @@ describe('#find', () => { options } ); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`throws decorated ForbiddenError when type is an array and user isn't authorized for either type`, async () => { @@ -398,6 +408,7 @@ describe('#find', () => { options } ); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -417,6 +428,8 @@ describe('#find', () => { expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/find`]); expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`calls and returns result of repository.find`, async () => { @@ -479,6 +492,7 @@ describe('#find', () => { options } ); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -503,6 +517,8 @@ describe('#find', () => { expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type1}/find`, `action:saved_objects/${type2}/find`]); expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`specifies authorized types when calling repository.find()`, async () => { @@ -599,6 +615,7 @@ describe('#bulkGet', () => { objects } ); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -618,6 +635,8 @@ describe('#bulkGet', () => { expect(mockHasPrivileges).toHaveBeenCalledWith(['action:saved_objects/foo/bulk_get']); expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`calls and returns result of repository.bulkGet`, async () => { @@ -682,6 +701,7 @@ describe('#get', () => { id, } ); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -701,6 +721,8 @@ describe('#get', () => { expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/get`]); expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`calls and returns result of repository.get`, async () => { @@ -765,6 +787,7 @@ describe('#update', () => { options, } ); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { @@ -784,6 +807,8 @@ describe('#update', () => { expect(mockHasPrivileges).toHaveBeenCalledWith([`action:saved_objects/${type}/update`]); expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).not.toHaveBeenCalled(); }); test(`calls and returns result of repository.update`, async () => { From d2f2a59ca12938780a792ff977960987c21be74f Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 6 Jun 2018 14:05:46 -0400 Subject: [PATCH 07/12] Adding more audit log specific tests --- .../secure_saved_objects_client.test.js | 81 ++++++++++++++++--- 1 file changed, 70 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js index 25d18dffa73fc..3526d2684e5a7 100644 --- a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js +++ b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js @@ -97,12 +97,14 @@ describe('#create', () => { test(`calls and returns result of repository.create`, async () => { const type = 'foo'; + const username = Symbol(); const returnValue = Symbol(); const mockRepository = { create: jest.fn().mockReturnValue(returnValue) }; const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ - success: true + success: true, + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -117,6 +119,12 @@ describe('#create', () => { expect(result).toBe(returnValue); expect(mockRepository.create).toHaveBeenCalledWith(type, attributes, options); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).toHaveBeenCalledWith(username, 'create', [type], { + type, + attributes, + options, + }); }); }); @@ -187,12 +195,16 @@ describe('#bulkCreate', () => { }); test(`calls and returns result of repository.bulkCreate`, async () => { + const username = Symbol(); + const type1 = 'foo'; + const type2 = 'bar'; const returnValue = Symbol(); const mockRepository = { bulkCreate: jest.fn().mockReturnValue(returnValue) }; const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ - success: true + success: true, + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -201,8 +213,8 @@ describe('#bulkCreate', () => { auditLogger: mockAuditLogger, }); const objects = [ - { type: 'foo', otherThing: 'sup' }, - { type: 'bar', otherThing: 'everyone' }, + { type: type1, otherThing: 'sup' }, + { type: type2, otherThing: 'everyone' }, ]; const options = Symbol(); @@ -210,6 +222,11 @@ describe('#bulkCreate', () => { expect(result).toBe(returnValue); expect(mockRepository.bulkCreate).toHaveBeenCalledWith(objects, options); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).toHaveBeenCalledWith(username, 'bulk_create', [type1, type2], { + objects, + options, + }); }); }); @@ -273,12 +290,14 @@ describe('#delete', () => { test(`calls and returns result of repository.delete`, async () => { const type = 'foo'; + const username = Symbol(); const returnValue = Symbol(); const mockRepository = { delete: jest.fn().mockReturnValue(returnValue) }; const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ - success: true + success: true, + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -292,6 +311,11 @@ describe('#delete', () => { expect(result).toBe(returnValue); expect(mockRepository.delete).toHaveBeenCalledWith(type, id); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).toHaveBeenCalledWith(username, 'delete', [type], { + type, + id, + }); }); }); @@ -434,12 +458,14 @@ describe('#find', () => { test(`calls and returns result of repository.find`, async () => { const type = 'foo'; + const username = Symbol(); const returnValue = Symbol(); const mockRepository = { find: jest.fn().mockReturnValue(returnValue) }; const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ - success: true + success: true, + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -447,11 +473,16 @@ describe('#find', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const options = { type }; - const result = await client.find({ type }); + const result = await client.find(options); expect(result).toBe(returnValue); expect(mockRepository.find).toHaveBeenCalledWith({ type }); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).toHaveBeenCalledWith(username, 'find', [type], { + options, + }); }); }); @@ -553,6 +584,7 @@ describe('#find', () => { test(`calls and returns result of repository.find`, async () => { const type = 'foo'; + const username = Symbol(); const returnValue = Symbol(); const mockRepository = { getTypes: jest.fn().mockReturnValue([type]), @@ -560,7 +592,8 @@ describe('#find', () => { }; const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ success: true, - missing: [] + missing: [], + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -568,11 +601,16 @@ describe('#find', () => { hasPrivileges: mockHasPrivileges, auditLogger: mockAuditLogger, }); + const options = Symbol(); - const result = await client.find(); + const result = await client.find(options); expect(result).toBe(returnValue); expect(mockRepository.find).toHaveBeenCalledWith({ type: [type] }); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).toHaveBeenCalledWith(username, 'find', [type], { + options, + }); }); }); }); @@ -642,12 +680,14 @@ describe('#bulkGet', () => { test(`calls and returns result of repository.bulkGet`, async () => { const type1 = 'foo'; const type2 = 'bar'; + const username = Symbol(); const returnValue = Symbol(); const mockRepository = { bulkGet: jest.fn().mockReturnValue(returnValue) }; const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ - success: true + success: true, + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -664,6 +704,10 @@ describe('#bulkGet', () => { expect(result).toBe(returnValue); expect(mockRepository.bulkGet).toHaveBeenCalledWith(objects); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).toHaveBeenCalledWith(username, 'bulk_get', [type1, type2], { + objects, + }); }); }); @@ -727,12 +771,14 @@ describe('#get', () => { test(`calls and returns result of repository.get`, async () => { const type = 'foo'; + const username = Symbol(); const returnValue = Symbol(); const mockRepository = { get: jest.fn().mockReturnValue(returnValue) }; const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ - success: true + success: true, + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ @@ -746,6 +792,11 @@ describe('#get', () => { expect(result).toBe(returnValue); expect(mockRepository.get).toHaveBeenCalledWith(type, id); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).toHaveBeenCalledWith(username, 'get', [type], { + type, + id, + }); }); }); @@ -813,6 +864,7 @@ describe('#update', () => { test(`calls and returns result of repository.update`, async () => { const type = 'foo'; + const username = Symbol(); const returnValue = Symbol(); const mockRepository = { update: jest.fn().mockReturnValue(returnValue) @@ -834,5 +886,12 @@ describe('#update', () => { expect(result).toBe(returnValue); expect(mockRepository.update).toHaveBeenCalledWith(type, id, attributes, options); + expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled(); + expect(mockAuditLogger.savedObjectsAuthorizationSuccess).toHaveBeenCalledWith(username, 'update', [type], { + type, + id, + attributes, + options, + }); }); }); From 8bad6a3d4d3c447a3ab63543890c82689c867735 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 6 Jun 2018 14:09:53 -0400 Subject: [PATCH 08/12] Necessarly is not a work, unfortunately --- x-pack/test/rbac_api_integration/config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/test/rbac_api_integration/config.js b/x-pack/test/rbac_api_integration/config.js index d454ac014b42c..b9f3f19b0576c 100644 --- a/x-pack/test/rbac_api_integration/config.js +++ b/x-pack/test/rbac_api_integration/config.js @@ -37,7 +37,7 @@ export default async function ({ readConfigFile }) { // with the exception of a bogus "not-a-visualization" type that I added to make sure // the find filtering without a type specified worked correctly. Once we have the ability // to specify more granular access to the objects via the Kibana privileges, this should - // no longer be necessarly, and it's only required as long as we do read/all privileges. + // no longer be necessary, and it's only required as long as we do read/all privileges. esArchiver: { directory: path.join(__dirname, 'fixtures', 'es_archiver') }, From 34ef4504fb9e143d00506e5feee3f65f26472003 Mon Sep 17 00:00:00 2001 From: kobelb Date: Thu, 7 Jun 2018 07:11:33 -0400 Subject: [PATCH 09/12] Fixing test --- .../saved_objects_client/secure_saved_objects_client.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js index 3526d2684e5a7..036ed7a5e0c3a 100644 --- a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js +++ b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.test.js @@ -870,7 +870,8 @@ describe('#update', () => { update: jest.fn().mockReturnValue(returnValue) }; const mockHasPrivileges = jest.fn().mockImplementation(async () => ({ - success: true + success: true, + username, })); const mockAuditLogger = createMockAuditLogger(); const client = new SecureSavedObjectsClient({ From 790cd5f98dd6a2f162856dfdbbb0c10e586202aa Mon Sep 17 00:00:00 2001 From: kobelb Date: Thu, 7 Jun 2018 07:13:42 -0400 Subject: [PATCH 10/12] More descriptive name than "result" --- .../secure_saved_objects_client.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js index 6a30b1e8e063a..58343728f8795 100644 --- a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js +++ b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js @@ -66,16 +66,22 @@ export class SecureSavedObjectsClient { // otherwise, we have to filter for only their authorized types const types = this._repository.getTypes(); const typesToPrivilegesMap = new Map(types.map(type => [type, getPrivilege(type, action)])); - const result = await this._hasSavedObjectPrivileges(Array.from(typesToPrivilegesMap.values())); + const hasPrivilegesResult = await this._hasSavedObjectPrivileges(Array.from(typesToPrivilegesMap.values())); const authorizedTypes = Array.from(typesToPrivilegesMap.entries()) - .filter(([ , privilege]) => !result.missing.includes(privilege)) + .filter(([ , privilege]) => !hasPrivilegesResult.missing.includes(privilege)) .map(([type]) => type); if (authorizedTypes.length === 0) { - this._auditLogger.savedObjectsAuthorizationFailure(result.username, action, types, result.missing, { options }); + this._auditLogger.savedObjectsAuthorizationFailure( + hasPrivilegesResult.username, + action, + types, + hasPrivilegesResult.missing, + { options } + ); throw this.errors.decorateForbiddenError(new Error(`Not authorized to find any types`)); } - this._auditLogger.savedObjectsAuthorizationSuccess(result.username, action, authorizedTypes, { options }); + this._auditLogger.savedObjectsAuthorizationSuccess(hasPrivilegesResult.username, action, authorizedTypes, { options }); return await this._repository.find({ ...options, From 35c95200d72da4502b39492581ac3b606f4bd8f4 Mon Sep 17 00:00:00 2001 From: kobelb Date: Thu, 7 Jun 2018 07:19:11 -0400 Subject: [PATCH 11/12] Better unauthorized find message? --- .../lib/saved_objects_client/secure_saved_objects_client.js | 2 +- x-pack/test/rbac_api_integration/apis/saved_objects/find.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js index 58343728f8795..53d8dadeb180b 100644 --- a/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js +++ b/x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client.js @@ -79,7 +79,7 @@ export class SecureSavedObjectsClient { hasPrivilegesResult.missing, { options } ); - throw this.errors.decorateForbiddenError(new Error(`Not authorized to find any types`)); + throw this.errors.decorateForbiddenError(new Error(`Not authorized to find saved_object`)); } this._auditLogger.savedObjectsAuthorizationSuccess(hasPrivilegesResult.username, action, authorizedTypes, { options }); diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/find.js b/x-pack/test/rbac_api_integration/apis/saved_objects/find.js index 4c38f42b5416e..e00dd1aa90715 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/find.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/find.js @@ -90,7 +90,7 @@ export default function ({ getService }) { expect(resp.body).to.eql({ statusCode: 403, error: 'Forbidden', - message: `Not authorized to find any types` + message: `Not authorized to find saved_object` }); }; From ee98045e31046f92e0829eddc91d81774e9f96ea Mon Sep 17 00:00:00 2001 From: kobelb Date: Thu, 7 Jun 2018 07:34:50 -0400 Subject: [PATCH 12/12] Adding getTypes tests --- .../service/lib/repository.test.js | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/src/server/saved_objects/service/lib/repository.test.js b/src/server/saved_objects/service/lib/repository.test.js index a02bfb9abb619..d3e66c0554e43 100644 --- a/src/server/saved_objects/service/lib/repository.test.js +++ b/src/server/saved_objects/service/lib/repository.test.js @@ -632,6 +632,83 @@ describe('SavedObjectsRepository', () => { }); }); + describe('#getTypes', () => { + it(`returns no types if mappings have no types`, () => { + const mappings = { + doc: { + properties: { + 'updated_at': { + type: 'date' + }, + } + } + }; + + savedObjectsRepository = new SavedObjectsRepository({ + index: '.kibana-test', + mappings, + callCluster: callAdminCluster, + onBeforeWrite + }); + + const types = savedObjectsRepository.getTypes(); + expect(types).toEqual([]); + }); + + it(`returns single type defined in mappings`, () => { + const mappings = { + doc: { + properties: { + 'updated_at': { + type: 'date' + }, + 'index-pattern': { + properties: {} + } + } + } + }; + + savedObjectsRepository = new SavedObjectsRepository({ + index: '.kibana-test', + mappings, + callCluster: callAdminCluster, + onBeforeWrite + }); + + const types = savedObjectsRepository.getTypes(); + expect(types).toEqual(['index-pattern']); + }); + + it(`returns multiple types defined in mappings`, () => { + const mappings = { + doc: { + properties: { + 'updated_at': { + type: 'date' + }, + 'index-pattern': { + properties: {} + }, + 'visualization': { + properties: {} + }, + } + } + }; + + savedObjectsRepository = new SavedObjectsRepository({ + index: '.kibana-test', + mappings, + callCluster: callAdminCluster, + onBeforeWrite + }); + + const types = savedObjectsRepository.getTypes(); + expect(types).toEqual(['index-pattern', 'visualization']); + }); + }); + describe('onBeforeWrite', () => { it('blocks calls to callCluster of requests', async () => { onBeforeWrite.returns(delay(500));