From 65ac8d6ecc1e33df89a370d4c3716ce2c441726c Mon Sep 17 00:00:00 2001 From: Brandon Kobel Date: Fri, 19 Apr 2019 11:58:15 -0700 Subject: [PATCH] Feature Controls - either `base` or `feature` (#35321) * Only allowing either base or feature privileges * Get roles route return transform error if base and feature privileges * Treating [] and {} as undefined * Updating the role api integration tests --- .../server/routes/api/public/roles/get.js | 13 ++ .../routes/api/public/roles/get.test.js | 182 ++++++++++++++++++ .../server/routes/api/public/roles/put.js | 10 +- .../routes/api/public/roles/put.test.js | 137 ++++++++++--- .../api_integration/apis/security/roles.js | 30 +-- 5 files changed, 322 insertions(+), 50 deletions(-) diff --git a/x-pack/plugins/security/server/routes/api/public/roles/get.js b/x-pack/plugins/security/server/routes/api/public/roles/get.js index 38b50885a76f..d0594e32ba48 100644 --- a/x-pack/plugins/security/server/routes/api/public/roles/get.js +++ b/x-pack/plugins/security/server/routes/api/public/roles/get.js @@ -67,6 +67,19 @@ export function initGetRolesApi(server, callWithRequest, routePreCheckLicenseFn, }; } + // if base privilege assigned with feature privileges, we won't transform these + if (roleKibanaApplications.some(entry => + entry.privileges.some(privilege => PrivilegeSerializer.isSerializedFeaturePrivilege(privilege)) && + ( + entry.privileges.some(privilege => PrivilegeSerializer.isSerializedGlobalBasePrivilege(privilege)) || + entry.privileges.some(privilege => PrivilegeSerializer.isSerializedSpaceBasePrivilege(privilege)) + ) + )) { + return { + success: false + }; + } + // if any application entry contains the '*' resource in addition to another resource, we can't transform these if (roleKibanaApplications.some(entry => entry.resources.includes(GLOBAL_RESOURCE) && entry.resources.length > 1)) { return { diff --git a/x-pack/plugins/security/server/routes/api/public/roles/get.test.js b/x-pack/plugins/security/server/routes/api/public/roles/get.test.js index 1199544de869..24aa4bd6e02b 100644 --- a/x-pack/plugins/security/server/routes/api/public/roles/get.test.js +++ b/x-pack/plugins/security/server/routes/api/public/roles/get.test.js @@ -1032,6 +1032,98 @@ describe('GET roles', () => { }, }); + getRolesTest( + `global base privilege assigned with a feature privilege returns empty kibana section with _transform_error set to ['kibana']`, { + callWithRequestImpl: async () => ({ + first_role: { + cluster: [], + indices: [], + applications: [ + { + application, + privileges: ['all', 'feature_foo.foo-privilege-1'], + resources: ['*'], + } + ], + run_as: [], + metadata: { + _reserved: true, + }, + transient_metadata: { + enabled: true, + }, + }, + }), + asserts: { + statusCode: 200, + result: [ + { + name: 'first_role', + metadata: { + _reserved: true, + }, + transient_metadata: { + enabled: true, + }, + elasticsearch: { + cluster: [], + indices: [], + run_as: [], + }, + kibana: [], + _transform_error: ['kibana'], + _unrecognized_applications: [], + }, + ], + }, + }); + + getRolesTest( + `space base privilege assigned with a feature privilege returns empty kibana section with _transform_error set to ['kibana']`, { + callWithRequestImpl: async () => ({ + first_role: { + cluster: [], + indices: [], + applications: [ + { + application, + privileges: ['space_all', 'feature_foo.foo-privilege-1'], + resources: ['space:space_1'], + } + ], + run_as: [], + metadata: { + _reserved: true, + }, + transient_metadata: { + enabled: true, + }, + }, + }), + asserts: { + statusCode: 200, + result: [ + { + name: 'first_role', + metadata: { + _reserved: true, + }, + transient_metadata: { + enabled: true, + }, + elasticsearch: { + cluster: [], + indices: [], + run_as: [], + }, + kibana: [], + _transform_error: ['kibana'], + _unrecognized_applications: [], + }, + ], + }, + }); + getRolesTest(`transforms unrecognized applications`, { callWithRequestImpl: async () => ({ first_role: { @@ -2149,6 +2241,96 @@ describe('GET role', () => { }, }); + getRoleTest( + `global base privilege assigned with a feature privilege returns empty kibana section with _transform_error set to ['kibana']`, { + name: 'first_role', + callWithRequestImpl: async () => ({ + first_role: { + cluster: [], + indices: [], + applications: [ + { + application, + privileges: ['all', 'feature_foo.foo-privilege-1'], + resources: ['*'], + } + ], + run_as: [], + metadata: { + _reserved: true, + }, + transient_metadata: { + enabled: true, + }, + }, + }), + asserts: { + statusCode: 200, + result: { + name: 'first_role', + metadata: { + _reserved: true, + }, + transient_metadata: { + enabled: true, + }, + elasticsearch: { + cluster: [], + indices: [], + run_as: [], + }, + kibana: [], + _transform_error: ['kibana'], + _unrecognized_applications: [], + }, + }, + }); + + getRoleTest( + `space base privilege assigned with a feature privilege returns empty kibana section with _transform_error set to ['kibana']`, { + name: 'first_role', + callWithRequestImpl: async () => ({ + first_role: { + cluster: [], + indices: [], + applications: [ + { + application, + privileges: ['space_all', 'feature_foo.foo-privilege-1'], + resources: ['space:space_1'], + } + ], + run_as: [], + metadata: { + _reserved: true, + }, + transient_metadata: { + enabled: true, + }, + }, + }), + asserts: { + statusCode: 200, + result: { + name: 'first_role', + metadata: { + _reserved: true, + }, + transient_metadata: { + enabled: true, + }, + elasticsearch: { + cluster: [], + indices: [], + run_as: [], + }, + kibana: [], + _transform_error: ['kibana'], + _unrecognized_applications: [], + }, + }, + }); + getRoleTest(`transforms unrecognized applications`, { name: 'first_role', callWithRequestImpl: async () => ({ diff --git a/x-pack/plugins/security/server/routes/api/public/roles/put.js b/x-pack/plugins/security/server/routes/api/public/roles/put.js index 70149da43d33..c04a4f19420a 100644 --- a/x-pack/plugins/security/server/routes/api/public/roles/put.js +++ b/x-pack/plugins/security/server/routes/api/public/roles/put.js @@ -86,15 +86,19 @@ export function initPutRolesApi( Joi.object({ base: Joi.alternatives().when('spaces', { is: allSpacesSchema, - then: Joi.array().items(Joi.string().valid(Object.keys(privileges.global))), - otherwise: Joi.array().items(Joi.string().valid(Object.keys(privileges.space))), + then: Joi.array().items(Joi.string().valid(Object.keys(privileges.global))).empty(Joi.array().length(0)), + otherwise: Joi.array().items(Joi.string().valid(Object.keys(privileges.space))).empty(Joi.array().length(0)), }), - feature: Joi.object().pattern(/^[a-zA-Z0-9_-]+$/, Joi.array().items(Joi.string().regex(/^[a-zA-Z0-9_-]+$/))), + feature: Joi.object() + .pattern(/^[a-zA-Z0-9_-]+$/, Joi.array().items(Joi.string().regex(/^[a-zA-Z0-9_-]+$/))) + .empty(Joi.object().length(0)), spaces: Joi.alternatives( allSpacesSchema, Joi.array().items(Joi.string().regex(/^[a-z0-9_-]+$/)), ).default([GLOBAL_RESOURCE]), }) + // the following can be replaced with .oxor once we upgrade Joi + .without('base', ['feature']) ).unique((a, b) => { return intersection(a.spaces, b.spaces).length !== 0; }); diff --git a/x-pack/plugins/security/server/routes/api/public/roles/put.test.js b/x-pack/plugins/security/server/routes/api/public/roles/put.test.js index b9d20d5a28e9..99ba93d10d7a 100644 --- a/x-pack/plugins/security/server/routes/api/public/roles/put.test.js +++ b/x-pack/plugins/security/server/routes/api/public/roles/put.test.js @@ -198,6 +198,33 @@ describe('PUT role', () => { }, }); + putRoleTest(`doesn't allow both base and feature in the same entry`, { + name: 'foo-role', + payload: { + kibana: [ + { + base: ['all'], + feature: { + foo: ['foo'] + } + } + ] + }, + asserts: { + statusCode: 400, + result: { + error: 'Bad Request', + //eslint-disable-next-line max-len + message: `child \"kibana\" fails because [\"kibana\" at position 0 fails because [\"base\" conflict with forbidden peer \"feature\"]]`, + statusCode: 400, + validation: { + keys: ['kibana.0.base'], + source: 'payload', + }, + }, + }, + }); + describe('global', () => { putRoleTest(`only allows known Kibana global base privileges`, { name: 'foo-role', @@ -499,6 +526,90 @@ describe('PUT role', () => { }, }); + putRoleTest(`allows base with empty array and feature in the same entry`, { + name: 'foo-role', + payload: { + kibana: [ + { + base: [], + feature: { + foo: ['foo'] + } + } + ] + }, + preCheckLicenseImpl: defaultPreCheckLicenseImpl, + callWithRequestImpls: [async () => ({}), async () => { }], + asserts: { + callWithRequests: [ + ['shield.getRole', { name: 'foo-role', ignore: [404] }], + [ + 'shield.putRole', + { + name: 'foo-role', + body: { + cluster: [], + indices: [], + run_as: [], + applications: [ + { + application, + privileges: [ + 'feature_foo.foo', + ], + resources: [GLOBAL_RESOURCE], + }, + ], + }, + }, + ], + ], + statusCode: 204, + result: null, + }, + }); + + putRoleTest(`allows base and feature with empty object in the same entry`, { + name: 'foo-role', + payload: { + kibana: [ + { + base: ['all'], + feature: {} + } + ] + }, + preCheckLicenseImpl: defaultPreCheckLicenseImpl, + callWithRequestImpls: [async () => ({}), async () => { }], + asserts: { + callWithRequests: [ + ['shield.getRole', { name: 'foo-role', ignore: [404] }], + [ + 'shield.putRole', + { + name: 'foo-role', + body: { + cluster: [], + indices: [], + run_as: [], + applications: [ + { + application, + privileges: [ + 'all', + ], + resources: [GLOBAL_RESOURCE], + }, + ], + }, + }, + ], + ], + statusCode: 204, + result: null, + }, + }); + putRoleTest(`creates role with everything`, { name: 'foo-role', payload: { @@ -523,21 +634,13 @@ describe('PUT role', () => { kibana: [ { base: ['all', 'read'], - feature: { - foo: ['foo-privilege-1', 'foo-privilege-2'], - bar: ['bar-privilege-1', 'bar-privilege-2'] - }, spaces: ['*'], }, { base: ['all', 'read'], - feature: { - bar: ['bar-privilege-1', 'bar-privilege-2'] - }, spaces: ['test-space-1', 'test-space-2'] }, { - base: ['all', 'read'], feature: { foo: ['foo-privilege-1', 'foo-privilege-2'], }, @@ -561,10 +664,6 @@ describe('PUT role', () => { privileges: [ 'all', 'read', - 'feature_foo.foo-privilege-1', - 'feature_foo.foo-privilege-2', - 'feature_bar.bar-privilege-1', - 'feature_bar.bar-privilege-2', ], resources: [GLOBAL_RESOURCE], }, @@ -573,16 +672,12 @@ describe('PUT role', () => { privileges: [ 'space_all', 'space_read', - 'feature_bar.bar-privilege-1', - 'feature_bar.bar-privilege-2', ], resources: ['space:test-space-1', 'space:test-space-2'] }, { application, privileges: [ - 'space_all', - 'space_read', 'feature_foo.foo-privilege-1', 'feature_foo.foo-privilege-2', ], @@ -638,7 +733,6 @@ describe('PUT role', () => { }, kibana: [ { - base: ['all'], feature: { foo: ['foo-privilege-1'], bar: ['bar-privilege-1'] @@ -647,13 +741,9 @@ describe('PUT role', () => { }, { base: ['all'], - feature: { - foo: ['foo-privilege-2'] - }, spaces: ['test-space-1', 'test-space-2'] }, { - base: ['read'], feature: { bar: ['bar-privilege-2'] }, @@ -707,9 +797,8 @@ describe('PUT role', () => { { application, privileges: [ - 'all', 'feature_foo.foo-privilege-1', - 'feature_bar.bar-privilege-1' + 'feature_bar.bar-privilege-1', ], resources: [GLOBAL_RESOURCE], }, @@ -717,14 +806,12 @@ describe('PUT role', () => { application, privileges: [ 'space_all', - 'feature_foo.foo-privilege-2' ], resources: ['space:test-space-1', 'space:test-space-2'] }, { application, privileges: [ - 'space_read', 'feature_bar.bar-privilege-2', ], resources: ['space:test-space-3'] diff --git a/x-pack/test/api_integration/apis/security/roles.js b/x-pack/test/api_integration/apis/security/roles.js index 5e5c2f3d3b18..216d12becfab 100644 --- a/x-pack/test/api_integration/apis/security/roles.js +++ b/x-pack/test/api_integration/apis/security/roles.js @@ -44,13 +44,8 @@ export default function ({ getService }) { kibana: [ { base: ['read'], - feature: { - dashboard: ['read'], - dev_tools: ['all'], - } }, { - base: ['all'], feature: { dashboard: ['read'], discover: ['all'], @@ -81,12 +76,12 @@ export default function ({ getService }) { applications: [ { application: 'kibana-.kibana', - privileges: ['read', 'feature_dashboard.read', 'feature_dev_tools.all'], + privileges: ['read'], resources: ['*'], }, { application: 'kibana-.kibana', - privileges: ['space_all', 'feature_dashboard.read', 'feature_discover.all', 'feature_ml.all'], + privileges: ['feature_dashboard.read', 'feature_discover.all', 'feature_ml.all'], resources: ['space:marketing', 'space:sales'], } ], @@ -162,7 +157,6 @@ export default function ({ getService }) { }, kibana: [ { - base: ['read'], feature: { dashboard: ['read'], dev_tools: ['all'], @@ -171,11 +165,6 @@ export default function ({ getService }) { }, { base: ['all'], - feature: { - dashboard: ['read'], - discover: ['all'], - ml: ['all'] - }, spaces: ['marketing', 'sales'] } ], @@ -201,12 +190,12 @@ export default function ({ getService }) { applications: [ { application: 'kibana-.kibana', - privileges: ['read', 'feature_dashboard.read', 'feature_dev_tools.all'], + privileges: ['feature_dashboard.read', 'feature_dev_tools.all'], resources: ['*'], }, { application: 'kibana-.kibana', - privileges: ['space_all', 'feature_dashboard.read', 'feature_discover.all', 'feature_ml.all'], + privileges: ['space_all'], resources: ['space:marketing', 'space:sales'], }, { @@ -248,12 +237,12 @@ export default function ({ getService }) { applications: [ { application: 'kibana-.kibana', - privileges: ['read', 'feature_dashboard.read', 'feature_dev_tools.all'], + privileges: ['read'], resources: ['*'], }, { application: 'kibana-.kibana', - privileges: ['space_all', 'feature_dashboard.read', 'feature_discover.all', 'feature_ml.all'], + privileges: ['feature_dashboard.read', 'feature_discover.all', 'feature_ml.all'], resources: ['space:marketing', 'space:sales'], }, { @@ -299,14 +288,11 @@ export default function ({ getService }) { kibana: [ { base: ['read'], - feature: { - dashboard: ['read'], - dev_tools: ['all'], - }, + feature: {}, spaces: ['*'] }, { - base: ['all'], + base: [], feature: { dashboard: ['read'], discover: ['all'],