From 946c169d647bfa04aa61ef77cd0482bb188d9cbc Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 23 Sep 2020 12:19:58 -0400 Subject: [PATCH 1/4] Omit runtime fields from FLS suggestions --- .../server/routes/indices/get_fields.ts | 52 +++++++++++++--- x-pack/test/api_integration/apis/index.js | 42 ++++++------- .../api_integration/apis/security/index.js | 12 ++-- .../apis/security/index_fields.ts | 60 +++++++++++++++++++ .../security/flstest/data/mappings.json | 7 +++ 5 files changed, 137 insertions(+), 36 deletions(-) diff --git a/x-pack/plugins/security/server/routes/indices/get_fields.ts b/x-pack/plugins/security/server/routes/indices/get_fields.ts index 356b78aa33879..68c1030bc3d85 100644 --- a/x-pack/plugins/security/server/routes/indices/get_fields.ts +++ b/x-pack/plugins/security/server/routes/indices/get_fields.ts @@ -8,6 +8,20 @@ import { schema } from '@kbn/config-schema'; import { RouteDefinitionParams } from '../index'; import { wrapIntoCustomErrorResponse } from '../../errors'; +interface FieldMappingResponse { + [indexName: string]: { + mappings: { + [fieldName: string]: { + mapping: { + [fieldName: string]: { + type: string; + }; + }; + }; + }; + }; +} + export function defineGetFieldsRoutes({ router, clusterClient }: RouteDefinitionParams) { router.get( { @@ -23,21 +37,41 @@ export function defineGetFieldsRoutes({ router, clusterClient }: RouteDefinition fields: '*', allowNoIndices: false, includeDefaults: true, - })) as Record }>; + })) as FieldMappingResponse; // The flow is the following (see response format at https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-get-field-mapping.html): // 1. Iterate over all matched indices. // 2. Extract all the field names from the `mappings` field of the particular index. - // 3. Collect and flatten the list of the field names. + // 3. Collect and flatten the list of the field names, omitting any fields without mappings, and any runtime fields // 4. Use `Set` to get only unique field names. + const fields = Array.from( + new Set( + Object.values(indexMappings) + .map((indexMapping) => { + return Object.entries(indexMapping.mappings).map(([fieldName, properties]) => { + const mappingValues = Object.values(properties.mapping); + const hasMapping = mappingValues.length > 0; + + const isRuntimeField = hasMapping && mappingValues[0]?.type === 'runtime'; + + // fields without mappings are internal fields such as `_routing` and `_index`, + // and therefore don't make sense as autocomplete suggestions for FLS. + + // Runtime fields are not securable via FLS. + // Administrators should instead secure access to the fields which derive this information. + if (!hasMapping || isRuntimeField) { + return null; + } + + return fieldName; + }); + }) + .flat() + ) + ).filter((field) => field !== null) as string[]; + return response.ok({ - body: Array.from( - new Set( - Object.values(indexMappings) - .map((indexMapping) => Object.keys(indexMapping.mappings)) - .flat() - ) - ), + body: fields, }); } catch (error) { return response.customError(wrapIntoCustomErrorResponse(error)); diff --git a/x-pack/test/api_integration/apis/index.js b/x-pack/test/api_integration/apis/index.js index a1bcaa13cc52b..c9d15d228804f 100644 --- a/x-pack/test/api_integration/apis/index.js +++ b/x-pack/test/api_integration/apis/index.js @@ -8,27 +8,27 @@ export default function ({ loadTestFile }) { describe('apis', function () { this.tags('ciGroup6'); - loadTestFile(require.resolve('./es')); + // loadTestFile(require.resolve('./es')); loadTestFile(require.resolve('./security')); - loadTestFile(require.resolve('./spaces')); - loadTestFile(require.resolve('./monitoring')); - loadTestFile(require.resolve('./xpack_legacy')); - loadTestFile(require.resolve('./features')); - loadTestFile(require.resolve('./telemetry')); - loadTestFile(require.resolve('./logstash')); - loadTestFile(require.resolve('./kibana')); - loadTestFile(require.resolve('./metrics_ui')); - loadTestFile(require.resolve('./beats')); - loadTestFile(require.resolve('./console')); - loadTestFile(require.resolve('./management')); - loadTestFile(require.resolve('./uptime')); - loadTestFile(require.resolve('./maps')); - loadTestFile(require.resolve('./security_solution')); - loadTestFile(require.resolve('./short_urls')); - loadTestFile(require.resolve('./lens')); - loadTestFile(require.resolve('./ml')); - loadTestFile(require.resolve('./transform')); - loadTestFile(require.resolve('./lists')); - loadTestFile(require.resolve('./upgrade_assistant')); + // loadTestFile(require.resolve('./spaces')); + // loadTestFile(require.resolve('./monitoring')); + // loadTestFile(require.resolve('./xpack_legacy')); + // loadTestFile(require.resolve('./features')); + // loadTestFile(require.resolve('./telemetry')); + // loadTestFile(require.resolve('./logstash')); + // loadTestFile(require.resolve('./kibana')); + // loadTestFile(require.resolve('./metrics_ui')); + // loadTestFile(require.resolve('./beats')); + // loadTestFile(require.resolve('./console')); + // loadTestFile(require.resolve('./management')); + // loadTestFile(require.resolve('./uptime')); + // loadTestFile(require.resolve('./maps')); + // loadTestFile(require.resolve('./security_solution')); + // loadTestFile(require.resolve('./short_urls')); + // loadTestFile(require.resolve('./lens')); + // loadTestFile(require.resolve('./ml')); + // loadTestFile(require.resolve('./transform')); + // loadTestFile(require.resolve('./lists')); + // loadTestFile(require.resolve('./upgrade_assistant')); }); } diff --git a/x-pack/test/api_integration/apis/security/index.js b/x-pack/test/api_integration/apis/security/index.js index 19eddb311b451..6ba74cb506267 100644 --- a/x-pack/test/api_integration/apis/security/index.js +++ b/x-pack/test/api_integration/apis/security/index.js @@ -11,12 +11,12 @@ export default function ({ loadTestFile }) { // Updates here should be mirrored in `./security_basic.ts` if tests // should also run under a basic license. - loadTestFile(require.resolve('./api_keys')); - loadTestFile(require.resolve('./basic_login')); - loadTestFile(require.resolve('./builtin_es_privileges')); - loadTestFile(require.resolve('./change_password')); + // loadTestFile(require.resolve('./api_keys')); + // loadTestFile(require.resolve('./basic_login')); + // loadTestFile(require.resolve('./builtin_es_privileges')); + // loadTestFile(require.resolve('./change_password')); loadTestFile(require.resolve('./index_fields')); - loadTestFile(require.resolve('./roles')); - loadTestFile(require.resolve('./privileges')); + // loadTestFile(require.resolve('./roles')); + // loadTestFile(require.resolve('./privileges')); }); } diff --git a/x-pack/test/api_integration/apis/security/index_fields.ts b/x-pack/test/api_integration/apis/security/index_fields.ts index 795da7dbe8835..7f5b4808664b0 100644 --- a/x-pack/test/api_integration/apis/security/index_fields.ts +++ b/x-pack/test/api_integration/apis/security/index_fields.ts @@ -7,10 +7,33 @@ import expect from '@kbn/expect/expect.js'; import { FtrProviderContext } from '../../ftr_provider_context'; +interface FLSFieldMappingResponse { + flstest: { + mappings: { + [fieldName: string]: { + mapping: { + [fieldName: string]: { + type: string; + }; + }; + }; + }; + }; +} + export default function ({ getService }: FtrProviderContext) { const supertest = getService('supertest'); + const esArchiver = getService('esArchiver'); + const es = getService('legacyEs'); describe('Index Fields', () => { + before(async () => { + await esArchiver.load('security/flstest/data'); + }); + after(async () => { + await esArchiver.unload('security/flstest/data'); + }); + describe('GET /internal/security/fields/{query}', () => { it('should return a list of available index mapping fields', async () => { await supertest @@ -30,6 +53,43 @@ export default function ({ getService }: FtrProviderContext) { sampleOfExpectedFields.forEach((field) => expect(response.body).to.contain(field)); }); }); + + it('should not include runtime fields', async () => { + // First, make sure the mapping actually includes a runtime field + const fieldMapping = (await es.indices.getFieldMapping({ + index: 'flstest', + fields: '*', + includeDefaults: true, + })) as FLSFieldMappingResponse; + + expect(Object.keys(fieldMapping.flstest.mappings)).to.contain('runtime_customer_ssn'); + expect( + fieldMapping.flstest.mappings.runtime_customer_ssn.mapping.runtime_customer_ssn.type + ).to.eql('runtime'); + + // Now, make sure it's not returned here + await supertest + .get('/internal/security/fields/flstest') + .set('kbn-xsrf', 'xxx') + .send() + .expect(200) + .then((response: Record) => { + const actualFields = response.body as string[]; + const expectedFields = [ + 'customer_ssn', + 'customer_ssn.keyword', + 'customer_region', + 'customer_region.keyword', + 'customer_name', + 'customer_name.keyword', + ]; + + actualFields.sort(); + expectedFields.sort(); + + expect(actualFields).to.eql(expectedFields); + }); + }); }); }); } diff --git a/x-pack/test/functional/es_archives/security/flstest/data/mappings.json b/x-pack/test/functional/es_archives/security/flstest/data/mappings.json index c6f11ea26f647..3605533618a93 100644 --- a/x-pack/test/functional/es_archives/security/flstest/data/mappings.json +++ b/x-pack/test/functional/es_archives/security/flstest/data/mappings.json @@ -30,6 +30,13 @@ } }, "type": "text" + }, + "runtime_customer_ssn": { + "type": "runtime", + "runtime_type": "keyword", + "script": { + "source": "emit(doc['customer_ssn'].value + ' calculated at runtime')" + } } } }, From 026eea60412f3d72e98463ad8a8a05b706568173 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 23 Sep 2020 14:07:05 -0400 Subject: [PATCH 2/4] reenable test suites --- x-pack/test/api_integration/apis/index.js | 42 +++++++++---------- .../api_integration/apis/security/index.js | 12 +++--- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/x-pack/test/api_integration/apis/index.js b/x-pack/test/api_integration/apis/index.js index c9d15d228804f..a1bcaa13cc52b 100644 --- a/x-pack/test/api_integration/apis/index.js +++ b/x-pack/test/api_integration/apis/index.js @@ -8,27 +8,27 @@ export default function ({ loadTestFile }) { describe('apis', function () { this.tags('ciGroup6'); - // loadTestFile(require.resolve('./es')); + loadTestFile(require.resolve('./es')); loadTestFile(require.resolve('./security')); - // loadTestFile(require.resolve('./spaces')); - // loadTestFile(require.resolve('./monitoring')); - // loadTestFile(require.resolve('./xpack_legacy')); - // loadTestFile(require.resolve('./features')); - // loadTestFile(require.resolve('./telemetry')); - // loadTestFile(require.resolve('./logstash')); - // loadTestFile(require.resolve('./kibana')); - // loadTestFile(require.resolve('./metrics_ui')); - // loadTestFile(require.resolve('./beats')); - // loadTestFile(require.resolve('./console')); - // loadTestFile(require.resolve('./management')); - // loadTestFile(require.resolve('./uptime')); - // loadTestFile(require.resolve('./maps')); - // loadTestFile(require.resolve('./security_solution')); - // loadTestFile(require.resolve('./short_urls')); - // loadTestFile(require.resolve('./lens')); - // loadTestFile(require.resolve('./ml')); - // loadTestFile(require.resolve('./transform')); - // loadTestFile(require.resolve('./lists')); - // loadTestFile(require.resolve('./upgrade_assistant')); + loadTestFile(require.resolve('./spaces')); + loadTestFile(require.resolve('./monitoring')); + loadTestFile(require.resolve('./xpack_legacy')); + loadTestFile(require.resolve('./features')); + loadTestFile(require.resolve('./telemetry')); + loadTestFile(require.resolve('./logstash')); + loadTestFile(require.resolve('./kibana')); + loadTestFile(require.resolve('./metrics_ui')); + loadTestFile(require.resolve('./beats')); + loadTestFile(require.resolve('./console')); + loadTestFile(require.resolve('./management')); + loadTestFile(require.resolve('./uptime')); + loadTestFile(require.resolve('./maps')); + loadTestFile(require.resolve('./security_solution')); + loadTestFile(require.resolve('./short_urls')); + loadTestFile(require.resolve('./lens')); + loadTestFile(require.resolve('./ml')); + loadTestFile(require.resolve('./transform')); + loadTestFile(require.resolve('./lists')); + loadTestFile(require.resolve('./upgrade_assistant')); }); } diff --git a/x-pack/test/api_integration/apis/security/index.js b/x-pack/test/api_integration/apis/security/index.js index 6ba74cb506267..19eddb311b451 100644 --- a/x-pack/test/api_integration/apis/security/index.js +++ b/x-pack/test/api_integration/apis/security/index.js @@ -11,12 +11,12 @@ export default function ({ loadTestFile }) { // Updates here should be mirrored in `./security_basic.ts` if tests // should also run under a basic license. - // loadTestFile(require.resolve('./api_keys')); - // loadTestFile(require.resolve('./basic_login')); - // loadTestFile(require.resolve('./builtin_es_privileges')); - // loadTestFile(require.resolve('./change_password')); + loadTestFile(require.resolve('./api_keys')); + loadTestFile(require.resolve('./basic_login')); + loadTestFile(require.resolve('./builtin_es_privileges')); + loadTestFile(require.resolve('./change_password')); loadTestFile(require.resolve('./index_fields')); - // loadTestFile(require.resolve('./roles')); - // loadTestFile(require.resolve('./privileges')); + loadTestFile(require.resolve('./roles')); + loadTestFile(require.resolve('./privileges')); }); } From 2f7c6ddabc85f2672dc8a5de462fe186a1ec9127 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 30 Sep 2020 20:52:59 -0400 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Aleh Zasypkin --- .../server/routes/indices/get_fields.ts | 32 ++++++++----------- .../apis/security/index_fields.ts | 30 ++++++++--------- 2 files changed, 27 insertions(+), 35 deletions(-) diff --git a/x-pack/plugins/security/server/routes/indices/get_fields.ts b/x-pack/plugins/security/server/routes/indices/get_fields.ts index 68c1030bc3d85..44b8804ed8d6e 100644 --- a/x-pack/plugins/security/server/routes/indices/get_fields.ts +++ b/x-pack/plugins/security/server/routes/indices/get_fields.ts @@ -46,29 +46,23 @@ export function defineGetFieldsRoutes({ router, clusterClient }: RouteDefinition // 4. Use `Set` to get only unique field names. const fields = Array.from( new Set( - Object.values(indexMappings) - .map((indexMapping) => { - return Object.entries(indexMapping.mappings).map(([fieldName, properties]) => { - const mappingValues = Object.values(properties.mapping); - const hasMapping = mappingValues.length > 0; + Object.values(indexMappings).flatMap((indexMapping) => { + return Object.keys(indexMapping.mappings).filter((fieldName) => { + const mappingValues = Object.values(indexMapping.mappings[fieldName].mapping); + const hasMapping = mappingValues.length > 0; - const isRuntimeField = hasMapping && mappingValues[0]?.type === 'runtime'; + const isRuntimeField = hasMapping && mappingValues[0]?.type === 'runtime'; - // fields without mappings are internal fields such as `_routing` and `_index`, - // and therefore don't make sense as autocomplete suggestions for FLS. + // fields without mappings are internal fields such as `_routing` and `_index`, + // and therefore don't make sense as autocomplete suggestions for FLS. - // Runtime fields are not securable via FLS. - // Administrators should instead secure access to the fields which derive this information. - if (!hasMapping || isRuntimeField) { - return null; - } - - return fieldName; - }); - }) - .flat() + // Runtime fields are not securable via FLS. + // Administrators should instead secure access to the fields which derive this information. + return hasMapping && !isRuntimeField; + }); + }) ) - ).filter((field) => field !== null) as string[]; + ); return response.ok({ body: fields, diff --git a/x-pack/test/api_integration/apis/security/index_fields.ts b/x-pack/test/api_integration/apis/security/index_fields.ts index 7f5b4808664b0..193d0eea1590e 100644 --- a/x-pack/test/api_integration/apis/security/index_fields.ts +++ b/x-pack/test/api_integration/apis/security/index_fields.ts @@ -68,27 +68,25 @@ export default function ({ getService }: FtrProviderContext) { ).to.eql('runtime'); // Now, make sure it's not returned here - await supertest + const { body: actualFields } = (await supertest .get('/internal/security/fields/flstest') .set('kbn-xsrf', 'xxx') .send() - .expect(200) - .then((response: Record) => { - const actualFields = response.body as string[]; - const expectedFields = [ - 'customer_ssn', - 'customer_ssn.keyword', - 'customer_region', - 'customer_region.keyword', - 'customer_name', - 'customer_name.keyword', - ]; + .expect(200)) as { body: string[] }; - actualFields.sort(); - expectedFields.sort(); + const expectedFields = [ + 'customer_ssn', + 'customer_ssn.keyword', + 'customer_region', + 'customer_region.keyword', + 'customer_name', + 'customer_name.keyword', + ]; - expect(actualFields).to.eql(expectedFields); - }); + actualFields.sort(); + expectedFields.sort(); + + expect(actualFields).to.eql(expectedFields); }); }); }); From c99fd72f1c2bf55b9120cc2887ab51451d016b76 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Wed, 30 Sep 2020 21:23:05 -0400 Subject: [PATCH 4/4] adding unit test --- .../server/routes/indices/get_fields.test.ts | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 x-pack/plugins/security/server/routes/indices/get_fields.test.ts diff --git a/x-pack/plugins/security/server/routes/indices/get_fields.test.ts b/x-pack/plugins/security/server/routes/indices/get_fields.test.ts new file mode 100644 index 0000000000000..4c6182e99431d --- /dev/null +++ b/x-pack/plugins/security/server/routes/indices/get_fields.test.ts @@ -0,0 +1,58 @@ +/* + * 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 { httpServerMock, elasticsearchServiceMock } from '../../../../../../src/core/server/mocks'; +import { kibanaResponseFactory } from '../../../../../../src/core/server'; + +import { routeDefinitionParamsMock } from '../index.mock'; +import { defineGetFieldsRoutes } from './get_fields'; + +const createFieldMapping = (field: string, type: string) => ({ + [field]: { mapping: { [field]: { type } } }, +}); + +const createEmptyFieldMapping = (field: string) => ({ [field]: { mapping: {} } }); + +const mockFieldMappingResponse = { + foo: { + mappings: { + ...createFieldMapping('fooField', 'keyword'), + ...createFieldMapping('commonField', 'keyword'), + ...createEmptyFieldMapping('emptyField'), + }, + }, + bar: { + mappings: { + ...createFieldMapping('commonField', 'keyword'), + ...createFieldMapping('barField', 'keyword'), + ...createFieldMapping('runtimeField', 'runtime'), + }, + }, +}; + +describe('GET /internal/security/fields/{query}', () => { + it('returns a list of deduplicated fields, omitting empty and runtime fields', async () => { + const mockRouteDefinitionParams = routeDefinitionParamsMock.create(); + + const scopedClient = elasticsearchServiceMock.createLegacyScopedClusterClient(); + scopedClient.callAsCurrentUser.mockResolvedValue(mockFieldMappingResponse); + mockRouteDefinitionParams.clusterClient.asScoped.mockReturnValue(scopedClient); + + defineGetFieldsRoutes(mockRouteDefinitionParams); + + const [[, handler]] = mockRouteDefinitionParams.router.get.mock.calls; + + const headers = { authorization: 'foo' }; + const mockRequest = httpServerMock.createKibanaRequest({ + method: 'get', + path: `/internal/security/fields/foo`, + headers, + }); + const response = await handler({} as any, mockRequest, kibanaResponseFactory); + expect(response.status).toBe(200); + expect(response.payload).toEqual(['fooField', 'commonField', 'barField']); + }); +});