From 7dc734994830aa6c917a6af447721640c8c0d6b1 Mon Sep 17 00:00:00 2001 From: kobelb Date: Mon, 11 Jun 2018 15:33:57 -0400 Subject: [PATCH 01/18] Basic implementation, rather sloppy --- .../lib/authorization/has_privileges.js | 121 +++++++++++++++--- .../security/server/lib/privileges/index.js | 2 +- .../apis/saved_objects/bulk_get.js | 26 ++++ .../apis/saved_objects/create.js | 26 ++++ .../apis/saved_objects/delete.js | 34 +++++ .../apis/saved_objects/find.js | 68 ++++++++++ .../apis/saved_objects/get.js | 34 +++++ .../apis/saved_objects/index.js | 48 ++++++- .../apis/saved_objects/lib/authentication.js | 8 ++ .../apis/saved_objects/update.js | 34 +++++ 10 files changed, 378 insertions(+), 23 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.js index 5f119db027cb6f..7f33157fb802e3 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.js @@ -6,50 +6,131 @@ import { getClient } from '../../../../../server/lib/get_client_shield'; import { DEFAULT_RESOURCE } from '../../../common/constants'; -import { getVersionPrivilege, getLoginPrivilege } from '../privileges'; +import { buildPrivilegeMap, getVersionPrivilege, getLoginPrivilege } from '../privileges'; const getMissingPrivileges = (resource, application, privilegeCheck) => { const privileges = privilegeCheck.application[application][resource]; return Object.keys(privileges).filter(key => privileges[key] === false); }; +const hasApplicationPrivileges = async (callWithRequest, request, kibanaVersion, application, privileges) => { + const versionPrivilege = getVersionPrivilege(kibanaVersion); + const loginPrivilege = getLoginPrivilege(); + + const privilegeCheck = await callWithRequest(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application, + resources: [DEFAULT_RESOURCE], + privileges: [versionPrivilege, loginPrivilege, ...privileges] + }] + } + }); + + const privilegesResult = privilegeCheck.application[application][DEFAULT_RESOURCE]; + + // We include the login privilege on all privileges, so the existence of it and not the version privilege + // lets us know that we're running in an incorrect configuration. Without the login privilege check, we wouldn't + // know whether the user just wasn't authorized for this instance of Kibana in general + if (!privilegesResult[versionPrivilege] && privilegesResult[loginPrivilege]) { + throw new Error('Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.'); + } + + return privilegeCheck; +}; + +const hasLegacyPrivileges = async (callWithRequest, request, kibanaVersion, application, kibanaIndex, privileges) => { + const privilegeCheck = await callWithRequest(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ kibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + + if (privilegeCheck.index[kibanaIndex].index) { + return { + username: privilegeCheck.username, + has_all_requested: true, + application: { + [application]: { + [DEFAULT_RESOURCE]: { + ...privileges.reduce((acc, name) => { + acc[name] = true; + return acc; + }, {}) + } + } + } + }; + } + + if (privilegeCheck.index[kibanaIndex].read) { + const privilegeMap = buildPrivilegeMap(application, kibanaVersion); + const implicitPrivileges = privileges.reduce((acc, name) => { + acc[name] = privilegeMap[application].read.actions.includes(name); + return acc; + }, {}); + return { + username: privilegeCheck.username, + has_all_requested: Object.values(implicitPrivileges).every(x => x), + application: { + [application]: { + [DEFAULT_RESOURCE]: implicitPrivileges + } + } + }; + } + + return { + username: privilegeCheck.username, + has_all_requested: false, + application: { + [application]: { + [DEFAULT_RESOURCE]: { + ...privileges.reduce((acc, name) => { + acc[name] = false; + return acc; + }, {}) + } + } + } + }; + +}; + export function hasPrivilegesWithServer(server) { const callWithRequest = getClient(server).callWithRequest; const config = server.config(); const kibanaVersion = config.get('pkg.version'); const application = config.get('xpack.security.rbac.application'); + const kibanaIndex = config.get('kibana.index'); return function hasPrivilegesWithRequest(request) { return async function hasPrivileges(privileges) { - const versionPrivilege = getVersionPrivilege(kibanaVersion); - const loginPrivilege = getLoginPrivilege(); + let privilegeCheck = await hasApplicationPrivileges(callWithRequest, request, kibanaVersion, application, privileges); - const privilegeCheck = await callWithRequest(request, 'shield.hasPrivileges', { - body: { - applications: [{ - application, - resources: [DEFAULT_RESOURCE], - privileges: [versionPrivilege, loginPrivilege, ...privileges] - }] - } - }); + if (!privilegeCheck.application[application][DEFAULT_RESOURCE][getLoginPrivilege()]) { + privilegeCheck = await hasLegacyPrivileges( + callWithRequest, + request, + kibanaVersion, + application, + kibanaIndex, + [...privileges, getLoginPrivilege()] + ); + } const success = privilegeCheck.has_all_requested; const missingPrivileges = getMissingPrivileges(DEFAULT_RESOURCE, application, privilegeCheck); - // We include the login privilege on all privileges, so the existence of it and not the version privilege - // lets us know that we're running in an incorrect configuration. Without the login privilege check, we wouldn't - // know whether the user just wasn't authorized for this instance of Kibana in general - if (missingPrivileges.includes(versionPrivilege) && !missingPrivileges.includes(loginPrivilege)) { - throw new Error('Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.'); - } - return { success, // We don't want to expose the version privilege to consumers, as it's an implementation detail only to detect version mismatch - missing: missingPrivileges.filter(p => p !== versionPrivilege), + missing: missingPrivileges.filter(p => p !== getVersionPrivilege(kibanaVersion)), username: privilegeCheck.username, }; }; diff --git a/x-pack/plugins/security/server/lib/privileges/index.js b/x-pack/plugins/security/server/lib/privileges/index.js index a270b8a60331e9..a7a2455d5ec3b0 100644 --- a/x-pack/plugins/security/server/lib/privileges/index.js +++ b/x-pack/plugins/security/server/lib/privileges/index.js @@ -5,4 +5,4 @@ */ export { registerPrivilegesWithCluster } from './privilege_action_registry'; -export { getLoginPrivilege, getVersionPrivilege } from './privileges'; +export { buildPrivilegeMap, getLoginPrivilege, getVersionPrivilege } from './privileges'; diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/bulk_get.js b/x-pack/test/rbac_api_integration/apis/saved_objects/bulk_get.js index 18fe5a015dc785..12a25607a92acd 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/bulk_get.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/bulk_get.js @@ -120,6 +120,32 @@ export default function ({ getService }) { } }); + bulkGetTest(`kibana legacy user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_USER.PASSWORD, + }, + tests: { + default: { + statusCode: 200, + response: expectResults, + }, + } + }); + + bulkGetTest(`kibana legacy dashboard only user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.PASSWORD, + }, + tests: { + default: { + statusCode: 200, + response: expectResults, + }, + } + }); + bulkGetTest(`kibana rbac user`, { auth: { username: AUTHENTICATION.KIBANA_RBAC_USER.USERNAME, diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/create.js b/x-pack/test/rbac_api_integration/apis/saved_objects/create.js index 0db0fc41b5c4a6..c8ca5be09b6ad4 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/create.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/create.js @@ -82,6 +82,32 @@ export default function ({ getService }) { } }); + createTest(`kibana legacy user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_USER.PASSWORD, + }, + tests: { + default: { + statusCode: 200, + response: expectResults, + }, + } + }); + + createTest(`kibana legacy dashboard only user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.PASSWORD, + }, + tests: { + default: { + statusCode: 403, + response: createExpectForbidden(true), + }, + } + }); + createTest(`kibana rbac user`, { auth: { username: AUTHENTICATION.KIBANA_RBAC_USER.USERNAME, diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/delete.js b/x-pack/test/rbac_api_integration/apis/saved_objects/delete.js index ea73c927b869e5..f89073acca7fda 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/delete.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/delete.js @@ -90,6 +90,40 @@ export default function ({ getService }) { } }); + deleteTest(`kibana legacy user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_USER.PASSWORD, + }, + tests: { + actualId: { + statusCode: 200, + response: expectEmpty, + }, + invalidId: { + statusCode: 404, + response: expectNotFound, + } + } + }); + + deleteTest(`kibana legacy dashboard only user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.PASSWORD, + }, + tests: { + actualId: { + statusCode: 403, + response: createExpectForbidden(true), + }, + invalidId: { + statusCode: 403, + response: createExpectForbidden(true), + } + } + }); + deleteTest(`kibana rbac user`, { auth: { username: AUTHENTICATION.KIBANA_RBAC_USER.USERNAME, 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 e00dd1aa907155..59cc3dc12d5389 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 @@ -217,6 +217,74 @@ export default function ({ getService }) { }, }); + findTest(`kibana legacy user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_USER.PASSWORD, + }, + tests: { + normal: { + description: 'only the visualization', + statusCode: 200, + response: expectVisualizationResults, + }, + unknownType: { + description: 'empty result', + statusCode: 200, + response: createExpectEmpty(1, 20, 0), + }, + pageBeyondTotal: { + description: 'empty result', + statusCode: 200, + response: createExpectEmpty(100, 100, 1), + }, + unknownSearchField: { + description: 'empty result', + statusCode: 200, + response: createExpectEmpty(1, 20, 0), + }, + noType: { + description: 'all objects', + statusCode: 200, + response: expectAllResults, + }, + }, + }); + + findTest(`kibana legacy dashboard only user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.PASSWORD, + }, + tests: { + normal: { + description: 'only the visualization', + statusCode: 200, + response: expectVisualizationResults, + }, + unknownType: { + description: 'forbidden find wigwags message', + statusCode: 403, + response: createExpectActionForbidden(true, 'wigwags'), + }, + pageBeyondTotal: { + description: 'empty result', + statusCode: 200, + response: createExpectEmpty(100, 100, 1), + }, + unknownSearchField: { + description: 'forbidden find wigwags message', + statusCode: 403, + response: createExpectActionForbidden(true, 'wigwags'), + }, + noType: { + description: 'all objects', + statusCode: 200, + response: expectAllResults, + }, + } + }); + findTest(`kibana rbac user`, { auth: { username: AUTHENTICATION.KIBANA_RBAC_USER.USERNAME, diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/get.js b/x-pack/test/rbac_api_integration/apis/saved_objects/get.js index e5a462d30d30cb..029a44475b12e6 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/get.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/get.js @@ -106,6 +106,40 @@ export default function ({ getService }) { } }); + getTest(`kibana legacy user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_USER.PASSWORD, + }, + tests: { + exists: { + statusCode: 200, + response: expectResults, + }, + doesntExist: { + statusCode: 404, + response: expectNotFound, + }, + } + }); + + getTest(`kibana legacy dashboard only user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.PASSWORD, + }, + tests: { + exists: { + statusCode: 200, + response: expectResults, + }, + doesntExist: { + statusCode: 404, + response: expectNotFound, + }, + } + }); + getTest(`kibana rbac user`, { auth: { username: AUTHENTICATION.KIBANA_RBAC_USER.USERNAME, diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/index.js b/x-pack/test/rbac_api_integration/apis/saved_objects/index.js index 1199bd5986e66d..05f06088751ea3 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/index.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/index.js @@ -11,6 +11,30 @@ export default function ({ loadTestFile, getService }) { describe('saved_objects', () => { before(async () => { + await es.shield.putRole({ + name: 'kibana_legacy_user', + body: { + cluster: [], + index: [{ + names: ['.kibana'], + privileges: ['manage', 'read', 'index', 'delete'] + }], + applications: [] + } + }); + + await es.shield.putRole({ + name: 'kibana_legacy_dashboard_only_user', + body: { + cluster: [], + index: [{ + names: ['.kibana'], + privileges: ['read', 'view_index_metadata'] + }], + applications: [] + } + }); + await es.shield.putRole({ name: 'kibana_rbac_user', body: { @@ -51,13 +75,33 @@ export default function ({ loadTestFile, getService }) { } }); + await es.shield.putUser({ + username: AUTHENTICATION.KIBANA_LEGACY_USER.USERNAME, + body: { + password: AUTHENTICATION.KIBANA_LEGACY_USER.PASSWORD, + roles: ['kibana_legacy_user'], + full_name: 'a kibana legacy user', + email: 'a_kibana_legacy_user@elastic.co', + } + }); + + await es.shield.putUser({ + username: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.USERNAME, + body: { + password: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.PASSWORD, + roles: ["kibana_legacy_dashboard_only_user"], + full_name: 'a kibana legacy dashboard only user', + email: 'a_kibana_legacy_dashboard_only_user@elastic.co', + } + }); + await es.shield.putUser({ username: AUTHENTICATION.KIBANA_RBAC_USER.USERNAME, body: { password: AUTHENTICATION.KIBANA_RBAC_USER.PASSWORD, roles: ['kibana_rbac_user'], full_name: 'a kibana user', - email: 'a_kibana_user@elastic.co', + email: 'a_kibana_rbac_user@elastic.co', } }); @@ -67,7 +111,7 @@ export default function ({ loadTestFile, getService }) { password: AUTHENTICATION.KIBANA_RBAC_DASHBOARD_ONLY_USER.PASSWORD, roles: ["kibana_rbac_dashboard_only_user"], full_name: 'a kibana dashboard only user', - email: 'a_kibana_dashboard_only_user@elastic.co', + email: 'a_kibana_rbac_dashboard_only_user@elastic.co', } }); }); diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/lib/authentication.js b/x-pack/test/rbac_api_integration/apis/saved_objects/lib/authentication.js index e095a032934eaf..8b140fd3b2a30c 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/lib/authentication.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/lib/authentication.js @@ -13,6 +13,14 @@ export const AUTHENTICATION = { USERNAME: 'elastic', PASSWORD: 'changeme' }, + KIBANA_LEGACY_USER: { + USERNAME: 'a_kibana_legacy_user', + PASSWORD: 'password' + }, + KIBANA_LEGACY_DASHBOARD_ONLY_USER: { + USERNAME: 'a_kibana_legacy_dashboard_only_user', + PASSWORD: 'password' + }, KIBANA_RBAC_USER: { USERNAME: 'a_kibana_rbac_user', PASSWORD: 'password' diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/update.js b/x-pack/test/rbac_api_integration/apis/saved_objects/update.js index a9f5f0ab81aaba..ae299348847d69 100644 --- a/x-pack/test/rbac_api_integration/apis/saved_objects/update.js +++ b/x-pack/test/rbac_api_integration/apis/saved_objects/update.js @@ -114,6 +114,40 @@ export default function ({ getService }) { } }); + updateTest(`kibana legacy user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_USER.PASSWORD, + }, + tests: { + exists: { + statusCode: 200, + response: expectResults, + }, + doesntExist: { + statusCode: 404, + response: expectNotFound, + }, + } + }); + + updateTest(`kibana legacy dashboard only user`, { + auth: { + username: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.USERNAME, + password: AUTHENTICATION.KIBANA_LEGACY_DASHBOARD_ONLY_USER.PASSWORD, + }, + tests: { + exists: { + statusCode: 403, + response: createExpectForbidden(true), + }, + doesntExist: { + statusCode: 403, + response: createExpectForbidden(true), + }, + } + }); + updateTest(`kibana rbac user`, { auth: { username: AUTHENTICATION.KIBANA_RBAC_USER.USERNAME, From 7ce7de36f40334670499c84cb3918df99d975b52 Mon Sep 17 00:00:00 2001 From: kobelb Date: Mon, 11 Jun 2018 16:48:54 -0400 Subject: [PATCH 02/18] Cleaning stuff up a bit --- .../lib/authorization/has_privileges.js | 94 ++++++++----------- 1 file changed, 39 insertions(+), 55 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.js index 7f33157fb802e3..bc4d4918e9f201 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.js @@ -8,35 +8,22 @@ import { getClient } from '../../../../../server/lib/get_client_shield'; import { DEFAULT_RESOURCE } from '../../../common/constants'; import { buildPrivilegeMap, getVersionPrivilege, getLoginPrivilege } from '../privileges'; -const getMissingPrivileges = (resource, application, privilegeCheck) => { - const privileges = privilegeCheck.application[application][resource]; - return Object.keys(privileges).filter(key => privileges[key] === false); -}; - const hasApplicationPrivileges = async (callWithRequest, request, kibanaVersion, application, privileges) => { - const versionPrivilege = getVersionPrivilege(kibanaVersion); - const loginPrivilege = getLoginPrivilege(); - const privilegeCheck = await callWithRequest(request, 'shield.hasPrivileges', { body: { applications: [{ application, resources: [DEFAULT_RESOURCE], - privileges: [versionPrivilege, loginPrivilege, ...privileges] + privileges }] } }); - const privilegesResult = privilegeCheck.application[application][DEFAULT_RESOURCE]; - - // We include the login privilege on all privileges, so the existence of it and not the version privilege - // lets us know that we're running in an incorrect configuration. Without the login privilege check, we wouldn't - // know whether the user just wasn't authorized for this instance of Kibana in general - if (!privilegesResult[versionPrivilege] && privilegesResult[loginPrivilege]) { - throw new Error('Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.'); - } - - return privilegeCheck; + return { + username: privilegeCheck.username, + hasAllRequested: privilegeCheck.has_all_requested, + privileges: privilegeCheck.application[application][DEFAULT_RESOURCE] + }; }; const hasLegacyPrivileges = async (callWithRequest, request, kibanaVersion, application, kibanaIndex, privileges) => { @@ -52,16 +39,12 @@ const hasLegacyPrivileges = async (callWithRequest, request, kibanaVersion, appl if (privilegeCheck.index[kibanaIndex].index) { return { username: privilegeCheck.username, - has_all_requested: true, - application: { - [application]: { - [DEFAULT_RESOURCE]: { - ...privileges.reduce((acc, name) => { - acc[name] = true; - return acc; - }, {}) - } - } + hasAllRequested: true, + privileges: { + ...privileges.reduce((acc, name) => { + acc[name] = true; + return acc; + }, {}) } }; } @@ -72,32 +55,22 @@ const hasLegacyPrivileges = async (callWithRequest, request, kibanaVersion, appl acc[name] = privilegeMap[application].read.actions.includes(name); return acc; }, {}); + return { username: privilegeCheck.username, - has_all_requested: Object.values(implicitPrivileges).every(x => x), - application: { - [application]: { - [DEFAULT_RESOURCE]: implicitPrivileges - } - } + hasAllRequested: Object.values(implicitPrivileges).every(x => x), + privileges: implicitPrivileges, }; } return { username: privilegeCheck.username, - has_all_requested: false, - application: { - [application]: { - [DEFAULT_RESOURCE]: { - ...privileges.reduce((acc, name) => { - acc[name] = false; - return acc; - }, {}) - } - } - } + hasAllRequested: false, + privileges: privileges.reduce((acc, name) => { + acc[name] = false; + return acc; + }, {}) }; - }; export function hasPrivilegesWithServer(server) { @@ -110,28 +83,39 @@ export function hasPrivilegesWithServer(server) { return function hasPrivilegesWithRequest(request) { return async function hasPrivileges(privileges) { + const loginPrivilege = getLoginPrivilege(); + const versionPrivilege = getVersionPrivilege(kibanaVersion); - let privilegeCheck = await hasApplicationPrivileges(callWithRequest, request, kibanaVersion, application, privileges); + const allPrivileges = [...privileges, loginPrivilege, versionPrivilege]; + let privilegesCheck = await hasApplicationPrivileges(callWithRequest, request, kibanaVersion, application, allPrivileges); - if (!privilegeCheck.application[application][DEFAULT_RESOURCE][getLoginPrivilege()]) { - privilegeCheck = await hasLegacyPrivileges( + if (!privilegesCheck.privileges[loginPrivilege]) { + privilegesCheck = await hasLegacyPrivileges( callWithRequest, request, kibanaVersion, application, kibanaIndex, - [...privileges, getLoginPrivilege()] + allPrivileges ); } - const success = privilegeCheck.has_all_requested; - const missingPrivileges = getMissingPrivileges(DEFAULT_RESOURCE, application, privilegeCheck); + const success = privilegesCheck.hasAllRequested; + + // We include the login privilege on all privileges, so the existence of it and not the version privilege + // lets us know that we're running in an incorrect configuration. Without the login privilege check, we wouldn't + // know whether the user just wasn't authorized for this instance of Kibana in general + if (!privilegesCheck.privileges[versionPrivilege] && privilegesCheck.privileges[loginPrivilege]) { + throw new Error('Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.'); + } return { success, // We don't want to expose the version privilege to consumers, as it's an implementation detail only to detect version mismatch - missing: missingPrivileges.filter(p => p !== getVersionPrivilege(kibanaVersion)), - username: privilegeCheck.username, + missing: Object.keys(privilegesCheck.privileges) + .filter(key => privilegesCheck.privileges[key] === false) + .filter(p => p !== versionPrivilege), + username: privilegesCheck.username, }; }; }; From c2a67bb3cdff1800e19a0c0d595a0d6661a6447f Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 07:34:43 -0400 Subject: [PATCH 03/18] Beginning to write tests, going to refactor how we build the privileges --- .../lib/authorization/has_privileges.js | 30 +- .../lib/authorization/has_privileges.test.js | 273 ++++++++---------- 2 files changed, 134 insertions(+), 169 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.js index bc4d4918e9f201..b852690ad028eb 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.js @@ -19,10 +19,19 @@ const hasApplicationPrivileges = async (callWithRequest, request, kibanaVersion, } }); + const hasPrivileges = privilegeCheck.application[application][DEFAULT_RESOURCE]; + + // We include the login privilege on all privileges, so the existence of it and not the version privilege + // lets us know that we're running in an incorrect configuration. Without the login privilege check, we wouldn't + // know whether the user just wasn't authorized for this instance of Kibana in general + if (!hasPrivileges[getVersionPrivilege(kibanaVersion)] && hasPrivileges[getLoginPrivilege()]) { + throw new Error('Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.'); + } + return { username: privilegeCheck.username, hasAllRequested: privilegeCheck.has_all_requested, - privileges: privilegeCheck.application[application][DEFAULT_RESOURCE] + privileges: hasPrivileges }; }; @@ -40,12 +49,10 @@ const hasLegacyPrivileges = async (callWithRequest, request, kibanaVersion, appl return { username: privilegeCheck.username, hasAllRequested: true, - privileges: { - ...privileges.reduce((acc, name) => { - acc[name] = true; - return acc; - }, {}) - } + privileges: privileges.reduce((acc, name) => { + acc[name] = true; + return acc; + }, {}) }; } @@ -86,7 +93,7 @@ export function hasPrivilegesWithServer(server) { const loginPrivilege = getLoginPrivilege(); const versionPrivilege = getVersionPrivilege(kibanaVersion); - const allPrivileges = [...privileges, loginPrivilege, versionPrivilege]; + const allPrivileges = [versionPrivilege, loginPrivilege, ...privileges]; let privilegesCheck = await hasApplicationPrivileges(callWithRequest, request, kibanaVersion, application, allPrivileges); if (!privilegesCheck.privileges[loginPrivilege]) { @@ -102,13 +109,6 @@ export function hasPrivilegesWithServer(server) { const success = privilegesCheck.hasAllRequested; - // We include the login privilege on all privileges, so the existence of it and not the version privilege - // lets us know that we're running in an incorrect configuration. Without the login privilege check, we wouldn't - // know whether the user just wasn't authorized for this instance of Kibana in general - if (!privilegesCheck.privileges[versionPrivilege] && privilegesCheck.privileges[loginPrivilege]) { - throw new Error('Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.'); - } - return { success, // We don't want to expose the version privilege to consumers, as it's an implementation detail only to detect version mismatch diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js index b2e2759c3419b3..22ed541e3a2d4a 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js @@ -13,14 +13,7 @@ jest.mock('../../../../../server/lib/get_client_shield', () => ({ getClient: jest.fn() })); -let mockCallWithRequest; -beforeEach(() => { - mockCallWithRequest = jest.fn(); - getClient.mockReturnValue({ - callWithRequest: mockCallWithRequest - }); -}); - +const defaultKibanaIndex = 'default-kibana-index'; const defaultVersion = 'default-version'; const defaultApplication = 'default-application'; @@ -32,6 +25,7 @@ const createMockServer = ({ settings = {} } = {}) => { }; const defaultSettings = { + 'kibana.index': defaultKibanaIndex, 'pkg.version': defaultVersion, 'xpack.security.rbac.application': defaultApplication }; @@ -43,8 +37,8 @@ const createMockServer = ({ settings = {} } = {}) => { return mockServer; }; -const mockResponse = (hasAllRequested, privileges, application = defaultApplication, username = '') => { - mockCallWithRequest.mockImplementationOnce(async () => ({ +const mockApplicationPrivilegeResponse = ({ hasAllRequested, privileges, application = defaultApplication, username = '' }) =>{ + return { username: username, has_all_requested: hasAllRequested, application: { @@ -52,151 +46,147 @@ const mockResponse = (hasAllRequested, privileges, application = defaultApplicat [DEFAULT_RESOURCE]: privileges } } - })); + }; }; - -test(`calls shield.hasPrivileges with request`, async () => { - const mockServer = createMockServer(); - mockResponse(true, { - [getVersionPrivilege(defaultVersion)]: true, - [getLoginPrivilege()]: true, - foo: true, - }); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const request = {}; - const hasPrivileges = hasPrivilegesWithRequest(request); - await hasPrivileges(['foo']); - - expect(mockCallWithRequest).toHaveBeenCalledWith(request, expect.anything(), expect.anything()); -}); - -test(`calls shield.hasPrivileges with clientParams`, async () => { - const application = 'foo-application'; - const version = 'foo-version'; - const mockServer = createMockServer({ - settings: { - 'xpack.security.rbac.application': application, - 'pkg.version': version +const mockLegacyResponse = ({ hasAllRequested, privileges, index = defaultKibanaIndex, username = '' }) => { + return { + username: username, + has_all_requested: hasAllRequested, + index: { + [index]: privileges } - }); - - mockResponse(true, { - [getVersionPrivilege(version)]: true, - [getLoginPrivilege()]: true, - foo: true, - }, application); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const hasPrivileges = hasPrivilegesWithRequest({}); - - const privilege = 'foo'; - await hasPrivileges([privilege]); - - const clientParams = mockCallWithRequest.mock.calls[0][2]; - const applicationParam = clientParams.body.applications[0]; - expect(applicationParam).toHaveProperty('application', application); - expect(applicationParam).toHaveProperty('resources', [DEFAULT_RESOURCE]); - expect(applicationParam).toHaveProperty('privileges'); - expect(applicationParam.privileges).toContain(privilege); -}); + }; +}; -test(`includes version privilege when checking privileges`, async () => { - const mockServer = createMockServer(); - mockResponse(true, { - [getVersionPrivilege(defaultVersion)]: true, - [getLoginPrivilege()]: true, - foo: true, +const createMockCallWithRequest = (responses) => { + const mockCallWithRequest = jest.fn(); + getClient.mockReturnValue({ + callWithRequest: mockCallWithRequest }); - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const request = {}; - const hasPrivileges = hasPrivilegesWithRequest(request); - await hasPrivileges(['foo']); + for (const response of responses) { + mockCallWithRequest.mockImplementationOnce(async () => response); + } - const clientParams = mockCallWithRequest.mock.calls[0][2]; - const applicationParam = clientParams.body.applications[0]; - expect(applicationParam.privileges).toContain(getVersionPrivilege(defaultVersion)); -}); + return mockCallWithRequest; +}; -test(`includes login privilege when checking privileges`, async () => { +test(`uses application privileges if they have all privileges`, async () => { + const username = 'foo-username'; const mockServer = createMockServer(); - mockResponse(true, { - [getVersionPrivilege(defaultVersion)]: true, - [getLoginPrivilege()]: true, - foo: true, - }); + const mockCallWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: true, + privileges: { + [getVersionPrivilege(defaultVersion)]: true, + [getLoginPrivilege()]: true, + foo: true, + }, + application: defaultApplication, + username, + }) + ]); const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const request = {}; + const request = Symbol(); const hasPrivileges = hasPrivilegesWithRequest(request); - await hasPrivileges(['foo']); - - const clientParams = mockCallWithRequest.mock.calls[0][2]; - const applicationParam = clientParams.body.applications[0]; - expect(applicationParam.privileges).toContain(getLoginPrivilege()); -}); - -test(`returns success when has_all_requested`, async () => { - const mockServer = createMockServer(); - mockResponse(true, { - [getVersionPrivilege(defaultVersion)]: true, - [getLoginPrivilege()]: true, - foo: true, + const privileges = ['foo']; + const result = await hasPrivileges(privileges); + + expect(mockCallWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(result).toEqual({ + success: true, + missing: [], + username }); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const hasPrivileges = hasPrivilegesWithRequest({}); - const result = await hasPrivileges(['foo']); - expect(result.success).toBe(true); }); -test(`returns username from has_privileges response when has_all_requested`, async () => { +test(`throws error if missing version privilege and has login privilege`, async () => { const mockServer = createMockServer(); - const username = 'foo-username'; - mockResponse(true, { - [getVersionPrivilege(defaultVersion)]: true, - [getLoginPrivilege()]: true, - foo: true, - }, defaultApplication, username); + createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: true, + foo: true, + } + }) + ]); const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); const hasPrivileges = hasPrivilegesWithRequest({}); - const result = await hasPrivileges(['foo']); - expect(result.username).toBe(username); -}); -test(`returns false success when has_all_requested is false`, async () => { - const mockServer = createMockServer(); - mockResponse(false, { - [getVersionPrivilege(defaultVersion)]: true, - [getLoginPrivilege()]: true, - foo: false, - }); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const hasPrivileges = hasPrivilegesWithRequest({}); - const result = await hasPrivileges(['foo']); - expect(result.success).toBe(false); + await expect(hasPrivileges(['foo'])).rejects.toThrowErrorMatchingSnapshot(); }); -test(`returns username from has_privileges when has_all_requested is false`, async () => { +test(`returns success of false if the user doesn't have any application privileges and no legacy privileges`, async () => { const username = 'foo-username'; const mockServer = createMockServer(); - mockResponse(false, { - [getVersionPrivilege(defaultVersion)]: true, - [getLoginPrivilege()]: true, - foo: false, - }, defaultApplication, username); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + foo: false, + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: false, + index: false, + }, + username, + }) + ]); const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const hasPrivileges = hasPrivilegesWithRequest({ }); - const result = await hasPrivileges(['foo']); - expect(result.username).toBe(username); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = ['foo']; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: false, + missing: [getLoginPrivilege(), ...privileges], + username, + }); }); -test(`returns missing privileges`, async () => { +test.skip(`returns missing privileges`, async () => { const mockServer = createMockServer(); mockResponse(false, { [getVersionPrivilege(defaultVersion)]: true, @@ -210,7 +200,7 @@ test(`returns missing privileges`, async () => { expect(result.missing).toEqual(['foo']); }); -test(`excludes granted privileges from missing privileges`, async () => { +test.skip(`excludes granted privileges from missing privileges`, async () => { const mockServer = createMockServer(); mockResponse(false, { [getVersionPrivilege(defaultVersion)]: true, @@ -225,33 +215,8 @@ test(`excludes granted privileges from missing privileges`, async () => { expect(result.missing).toEqual(['foo']); }); -test(`throws error if missing version privilege and has login privilege`, async () => { - const mockServer = createMockServer(); - mockResponse(false, { - [getVersionPrivilege(defaultVersion)]: false, - [getLoginPrivilege()]: true, - foo: true, - }); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const hasPrivileges = hasPrivilegesWithRequest({}); - await expect(hasPrivileges(['foo'])).rejects.toThrowErrorMatchingSnapshot(); -}); - -test(`doesn't throw error if missing version privilege and missing login privilege`, async () => { - const mockServer = createMockServer(); - mockResponse(false, { - [getVersionPrivilege(defaultVersion)]: false, - [getLoginPrivilege()]: false, - foo: true, - }); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const hasPrivileges = hasPrivilegesWithRequest({}); - await hasPrivileges(['foo']); -}); -test(`excludes version privilege when missing version privilege and missing login privilege`, async () => { +test.skip(`excludes version privilege when missing version privilege and missing login privilege`, async () => { const mockServer = createMockServer(); mockResponse(false, { [getVersionPrivilege(defaultVersion)]: false, From 329bfa45ddbcecaf206ff5064322bc840add331c Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 07:46:03 -0400 Subject: [PATCH 04/18] Making the buildPrivilegesMap no longer return application name as the main key --- .../security/server/lib/authorization/has_privileges.js | 2 +- .../server/lib/privileges/privilege_action_registry.js | 4 +++- .../server/lib/privileges/privilege_action_registry.test.js | 6 ++++-- x-pack/plugins/security/server/lib/privileges/privileges.js | 4 +--- x-pack/plugins/security/server/routes/api/v1/privileges.js | 2 +- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.js index b852690ad028eb..637539629fe715 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.js @@ -59,7 +59,7 @@ const hasLegacyPrivileges = async (callWithRequest, request, kibanaVersion, appl if (privilegeCheck.index[kibanaIndex].read) { const privilegeMap = buildPrivilegeMap(application, kibanaVersion); const implicitPrivileges = privileges.reduce((acc, name) => { - acc[name] = privilegeMap[application].read.actions.includes(name); + acc[name] = privilegeMap.read.actions.includes(name); return acc; }, {}); diff --git a/x-pack/plugins/security/server/lib/privileges/privilege_action_registry.js b/x-pack/plugins/security/server/lib/privileges/privilege_action_registry.js index 7d31b287fe664c..c91a8e097f6916 100644 --- a/x-pack/plugins/security/server/lib/privileges/privilege_action_registry.js +++ b/x-pack/plugins/security/server/lib/privileges/privilege_action_registry.js @@ -16,7 +16,9 @@ export async function registerPrivilegesWithCluster(server) { const kibanaVersion = config.get('pkg.version'); const application = config.get('xpack.security.rbac.application'); - const expectedPrivileges = buildPrivilegeMap(application, kibanaVersion); + const expectedPrivileges = { + [application]: buildPrivilegeMap(application, kibanaVersion) + }; server.log(['security', 'debug'], `Registering Kibana Privileges with Elasticsearch for ${application}`); diff --git a/x-pack/plugins/security/server/lib/privileges/privilege_action_registry.test.js b/x-pack/plugins/security/server/lib/privileges/privilege_action_registry.test.js index 398a68a097ed94..74949536eb943c 100644 --- a/x-pack/plugins/security/server/lib/privileges/privilege_action_registry.test.js +++ b/x-pack/plugins/security/server/lib/privileges/privilege_action_registry.test.js @@ -55,7 +55,9 @@ const registerPrivilegesWithClusterTest = (description, { settings = {}, expecte expect(mockCallWithInternalUser).toHaveBeenCalledWith( 'shield.postPrivileges', { - body: privileges, + body: { + [defaultApplication]: privileges + }, } ); @@ -93,7 +95,7 @@ const registerPrivilegesWithClusterTest = (description, { settings = {}, expecte test(description, async () => { const mockServer = createMockServer(); const mockCallWithInternalUser = registerMockCallWithInternalUser(); - mockCallWithInternalUser.mockImplementationOnce(async () => (existingPrivileges)); + mockCallWithInternalUser.mockImplementationOnce(async () => ({ [defaultApplication]: existingPrivileges })); buildPrivilegeMap.mockReturnValue(expectedPrivileges); await registerPrivilegesWithCluster(mockServer); diff --git a/x-pack/plugins/security/server/lib/privileges/privileges.js b/x-pack/plugins/security/server/lib/privileges/privileges.js index 8c11014b4ae14f..e70d30f2e87fa6 100644 --- a/x-pack/plugins/security/server/lib/privileges/privileges.js +++ b/x-pack/plugins/security/server/lib/privileges/privileges.js @@ -35,9 +35,7 @@ export function buildPrivilegeMap(application, kibanaVersion) { metadata: {} }; - return { - [application]: privilegeActions - }; + return privilegeActions; } function buildSavedObjectsReadPrivileges() { diff --git a/x-pack/plugins/security/server/routes/api/v1/privileges.js b/x-pack/plugins/security/server/routes/api/v1/privileges.js index f7d6e7c6a00394..6c095da482dce4 100644 --- a/x-pack/plugins/security/server/routes/api/v1/privileges.js +++ b/x-pack/plugins/security/server/routes/api/v1/privileges.js @@ -23,7 +23,7 @@ export function initPrivilegesApi(server) { // with our view of them to allow users to more efficiently edit privileges for roles, and serialize it // into a different structure for enforcement within Elasticsearch const privileges = buildPrivilegeMap(application, kibanaVersion); - reply(Object.values(privileges[application])); + reply(Object.values(privileges)); } }); } From 30e91ea0cd5aa9ba182bfc27ba898bdf57c4c9a7 Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 08:28:20 -0400 Subject: [PATCH 05/18] Using real privileges since we need to use them for the legacy fallback --- .../lib/authorization/has_privileges.test.js | 111 +++++++++++++++++- 1 file changed, 105 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js index 22ed541e3a2d4a..b19a8a0a69b335 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js @@ -73,6 +73,7 @@ const createMockCallWithRequest = (responses) => { }; test(`uses application privileges if they have all privileges`, async () => { + const privilege = 'action:saved_objects/config/get'; const username = 'foo-username'; const mockServer = createMockServer(); const mockCallWithRequest = createMockCallWithRequest([ @@ -81,7 +82,7 @@ test(`uses application privileges if they have all privileges`, async () => { privileges: { [getVersionPrivilege(defaultVersion)]: true, [getLoginPrivilege()]: true, - foo: true, + [privilege]: true, }, application: defaultApplication, username, @@ -91,7 +92,7 @@ test(`uses application privileges if they have all privileges`, async () => { const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); const request = Symbol(); const hasPrivileges = hasPrivilegesWithRequest(request); - const privileges = ['foo']; + const privileges = [privilege]; const result = await hasPrivileges(privileges); expect(mockCallWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { @@ -113,6 +114,7 @@ test(`uses application privileges if they have all privileges`, async () => { }); test(`throws error if missing version privilege and has login privilege`, async () => { + const privilege = 'action:saved_objects/config/get'; const mockServer = createMockServer(); createMockCallWithRequest([ mockApplicationPrivilegeResponse({ @@ -120,7 +122,7 @@ test(`throws error if missing version privilege and has login privilege`, async privileges: { [getVersionPrivilege(defaultVersion)]: false, [getLoginPrivilege()]: true, - foo: true, + [privilege]: true, } }) ]); @@ -128,10 +130,51 @@ test(`throws error if missing version privilege and has login privilege`, async const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); const hasPrivileges = hasPrivilegesWithRequest({}); - await expect(hasPrivileges(['foo'])).rejects.toThrowErrorMatchingSnapshot(); + await expect(hasPrivileges([privilege])).rejects.toThrowErrorMatchingSnapshot(); +}); + +test(`uses application privileges if the user has the login privilege`, async () => { + const privilege = 'action:saved_objects/config/get'; + const username = 'foo-username'; + const mockServer = createMockServer(); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: true, + [getLoginPrivilege()]: true, + [privilege]: false, + }, + username, + }), + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege]; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(result).toEqual({ + success: false, + missing: [...privileges], + username, + }); }); test(`returns success of false if the user doesn't have any application privileges and no legacy privileges`, async () => { + const privilege = 'action:saved_objects/config/get'; const username = 'foo-username'; const mockServer = createMockServer(); const callWithRequest = createMockCallWithRequest([ @@ -140,7 +183,7 @@ test(`returns success of false if the user doesn't have any application privileg privileges: { [getVersionPrivilege(defaultVersion)]: false, [getLoginPrivilege()]: false, - foo: false, + [privilege]: false, }, username, }), @@ -157,7 +200,7 @@ test(`returns success of false if the user doesn't have any application privileg const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); const request = Symbol(); const hasPrivileges = hasPrivilegesWithRequest(request); - const privileges = ['foo']; + const privileges = [privilege]; const result = await hasPrivileges(privileges); expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { @@ -186,6 +229,62 @@ test(`returns success of false if the user doesn't have any application privileg }); }); +//eslint-disable-next-line max-len +test(`returns success of true if the user doesn't have any application privileges but they have index privilege on kibana index`, async () => { + const privilege = 'action:saved_objects/config/create'; + const username = 'foo-username'; + const mockServer = createMockServer(); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege]: false, + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: false, + index: true, + }, + username, + }) + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = ['foo']; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: true, + missing: [], + username, + }); +}); test.skip(`returns missing privileges`, async () => { const mockServer = createMockServer(); mockResponse(false, { From 7f93cfb217304a668b5eba55c35ca56d9e7d356c Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 09:04:37 -0400 Subject: [PATCH 06/18] Adding more tests --- .../lib/authorization/has_privileges.test.js | 275 ++++++++++++++++-- 1 file changed, 244 insertions(+), 31 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js index b19a8a0a69b335..f1da7127264726 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js @@ -72,7 +72,7 @@ const createMockCallWithRequest = (responses) => { return mockCallWithRequest; }; -test(`uses application privileges if they have all privileges`, async () => { +test(`returns success of true if they have all application privileges`, async () => { const privilege = 'action:saved_objects/config/get'; const username = 'foo-username'; const mockServer = createMockServer(); @@ -113,6 +113,49 @@ test(`uses application privileges if they have all privileges`, async () => { }); }); +test(`returns success of false if they have only one application privilege`, async () => { + const privilege1 = 'action:saved_objects/config/get'; + const privilege2 = 'action:saved_objects/config/create'; + const username = 'foo-username'; + const mockServer = createMockServer(); + const mockCallWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: true, + [getLoginPrivilege()]: true, + [privilege1]: true, + [privilege2]: false, + }, + application: defaultApplication, + username, + }) + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege1, privilege2]; + const result = await hasPrivileges(privileges); + + expect(mockCallWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(result).toEqual({ + success: false, + missing: [privilege2], + username + }); +}); + test(`throws error if missing version privilege and has login privilege`, async () => { const privilege = 'action:saved_objects/config/get'; const mockServer = createMockServer(); @@ -173,6 +216,46 @@ test(`uses application privileges if the user has the login privilege`, async () }); }); +test(`returns sucess of false application privileges if the user has the login privilege`, async () => { + const privilege = 'action:saved_objects/config/get'; + const username = 'foo-username'; + const mockServer = createMockServer(); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: true, + [getLoginPrivilege()]: true, + [privilege]: false, + }, + username, + }), + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege]; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(result).toEqual({ + success: false, + missing: [...privileges], + username, + }); +}); + test(`returns success of false if the user doesn't have any application privileges and no legacy privileges`, async () => { const privilege = 'action:saved_objects/config/get'; const username = 'foo-username'; @@ -231,7 +314,7 @@ test(`returns success of false if the user doesn't have any application privileg //eslint-disable-next-line max-len test(`returns success of true if the user doesn't have any application privileges but they have index privilege on kibana index`, async () => { - const privilege = 'action:saved_objects/config/create'; + const privilege = 'something-completely-arbitrary'; const username = 'foo-username'; const mockServer = createMockServer(); const callWithRequest = createMockCallWithRequest([ @@ -285,46 +368,176 @@ test(`returns success of true if the user doesn't have any application privilege username, }); }); -test.skip(`returns missing privileges`, async () => { + +//eslint-disable-next-line max-len +test(`returns success of false if the user doesn't have any application privileges and they have the read privilege on kibana but the privilege isn't a read action`, async () => { + const privilege = 'something-completely-arbitrary'; + const username = 'foo-username'; const mockServer = createMockServer(); - mockResponse(false, { - [getVersionPrivilege(defaultVersion)]: true, - [getLoginPrivilege()]: true, - foo: false, - }); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege]: false, + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: true, + index: false, + }, + username, + }) + ]); const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const hasPrivileges = hasPrivilegesWithRequest({}); - const result = await hasPrivileges(['foo']); - expect(result.missing).toEqual(['foo']); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege]; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: false, + missing: [ privilege ], + username, + }); }); -test.skip(`excludes granted privileges from missing privileges`, async () => { +//eslint-disable-next-line max-len +test(`returns success of false if the user doesn't have any application privileges and they have the read privilege on kibana but one privilege isn't a read action`, async () => { + const privilege1 = 'something-completely-arbitrary'; + const privilege2 = 'action:saved_objects/config/get'; + const username = 'foo-username'; const mockServer = createMockServer(); - mockResponse(false, { - [getVersionPrivilege(defaultVersion)]: true, - [getLoginPrivilege()]: true, - foo: false, - bar: true, - }); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege1]: false, + [privilege2]: true + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: true, + index: false, + }, + username, + }) + ]); const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const hasPrivileges = hasPrivilegesWithRequest({}); - const result = await hasPrivileges(['foo']); - expect(result.missing).toEqual(['foo']); -}); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege1, privilege2]; + const result = await hasPrivileges(privileges); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: false, + missing: [ privilege1 ], + username, + }); +}); -test.skip(`excludes version privilege when missing version privilege and missing login privilege`, async () => { +//eslint-disable-next-line max-len +test(`returns success of true if the user doesn't have any application privileges and they have the read privilege on kibana and the privilege is a read action`, async () => { + const privilege = 'action:saved_objects/config/get'; + const username = 'foo-username'; const mockServer = createMockServer(); - mockResponse(false, { - [getVersionPrivilege(defaultVersion)]: false, - [getLoginPrivilege()]: false, - foo: true, - }); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege]: false, + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: true, + index: false, + }, + username, + }) + ]); const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const hasPrivileges = hasPrivilegesWithRequest({}); - const result = await hasPrivileges(['foo']); - expect(result.missing).toEqual([getLoginPrivilege()]); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege]; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: true, + missing: [], + username, + }); }); From bfae62bc98f494410c8b85045f59b6e0202fac9d Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 09:05:40 -0400 Subject: [PATCH 07/18] Fixing spelling --- .../security/server/lib/authorization/has_privileges.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js index f1da7127264726..e0b7e999f55737 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js @@ -216,7 +216,7 @@ test(`uses application privileges if the user has the login privilege`, async () }); }); -test(`returns sucess of false application privileges if the user has the login privilege`, async () => { +test(`returns success of false application privileges if the user has the login privilege`, async () => { const privilege = 'action:saved_objects/config/get'; const username = 'foo-username'; const mockServer = createMockServer(); From 82f4c6b1b6f214d94f9ed8ac0c51de615b66235c Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 09:42:49 -0400 Subject: [PATCH 08/18] Fixing test description --- .../security/server/lib/authorization/has_privileges.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js index e0b7e999f55737..101f188d5acb25 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js @@ -216,7 +216,7 @@ test(`uses application privileges if the user has the login privilege`, async () }); }); -test(`returns success of false application privileges if the user has the login privilege`, async () => { +test(`returns success of false using application privileges if the user has the login privilege`, async () => { const privilege = 'action:saved_objects/config/get'; const username = 'foo-username'; const mockServer = createMockServer(); From f47804b8fd8ad8d24d726168bcb49163ffd66bb5 Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 09:47:46 -0400 Subject: [PATCH 09/18] Fixing comment description --- .../plugins/security/server/lib/authorization/has_privileges.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.js index 637539629fe715..cd1852d668c2f1 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.js @@ -21,7 +21,7 @@ const hasApplicationPrivileges = async (callWithRequest, request, kibanaVersion, const hasPrivileges = privilegeCheck.application[application][DEFAULT_RESOURCE]; - // We include the login privilege on all privileges, so the existence of it and not the version privilege + // We include the login action in all privileges, so the existence of it and not the version privilege // lets us know that we're running in an incorrect configuration. Without the login privilege check, we wouldn't // know whether the user just wasn't authorized for this instance of Kibana in general if (!hasPrivileges[getVersionPrivilege(kibanaVersion)] && hasPrivileges[getLoginPrivilege()]) { From 98223f2c939c105677f56b6e3ec2aad9f93616b7 Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 09:51:57 -0400 Subject: [PATCH 10/18] Adding similar line breaks in the has privilege calls --- .../security/server/lib/authorization/has_privileges.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.js index cd1852d668c2f1..5e918e84dbe164 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.js @@ -94,7 +94,13 @@ export function hasPrivilegesWithServer(server) { const versionPrivilege = getVersionPrivilege(kibanaVersion); const allPrivileges = [versionPrivilege, loginPrivilege, ...privileges]; - let privilegesCheck = await hasApplicationPrivileges(callWithRequest, request, kibanaVersion, application, allPrivileges); + let privilegesCheck = await hasApplicationPrivileges( + callWithRequest, + request, + kibanaVersion, + application, + allPrivileges + ); if (!privilegesCheck.privileges[loginPrivilege]) { privilegesCheck = await hasLegacyPrivileges( From 516c03755aa377964845f8937c0d25fd4be112bc Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 11:08:01 -0400 Subject: [PATCH 11/18] No more settings --- x-pack/plugins/security/index.js | 55 ++++++++++++++------------------ 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/x-pack/plugins/security/index.js b/x-pack/plugins/security/index.js index e0db6b206e3d91..1c12c0477bd594 100644 --- a/x-pack/plugins/security/index.js +++ b/x-pack/plugins/security/index.js @@ -44,7 +44,6 @@ export const security = (kibana) => new kibana.Plugin({ port: Joi.number().integer().min(0).max(65535) }).default(), rbac: Joi.object({ - enabled: Joi.boolean().default(false), application: Joi.string().default('kibana').regex( /[a-zA-Z0-9-_]+/, `may contain alphanumeric characters (a-z, A-Z, 0-9), underscores and hyphens` @@ -92,10 +91,6 @@ export const security = (kibana) => new kibana.Plugin({ const xpackMainPlugin = server.plugins.xpack_main; mirrorStatusAndInitialize(xpackMainPlugin.status, this.status, async () => { - if (!config.get('xpack.security.rbac.enabled')) { - return; - } - await registerPrivilegesWithCluster(server); }); @@ -113,36 +108,34 @@ export const security = (kibana) => new kibana.Plugin({ server.auth.strategy('session', 'login', 'required'); const auditLogger = new SecurityAuditLogger(server.config(), new AuditLogger(server, 'security')); - if (config.get('xpack.security.rbac.enabled')) { - const hasPrivilegesWithRequest = hasPrivilegesWithServer(server); - const { savedObjects } = server; + const hasPrivilegesWithRequest = hasPrivilegesWithServer(server); + const { savedObjects } = server; + + savedObjects.setScopedSavedObjectsClientFactory(({ + request, + index, + mappings, + onBeforeWrite + }) => { + const hasPrivileges = hasPrivilegesWithRequest(request); - savedObjects.setScopedSavedObjectsClientFactory(({ - request, + const adminCluster = server.plugins.elasticsearch.getCluster('admin'); + const { callWithInternalUser } = adminCluster; + + const repository = new savedObjects.SavedObjectsRepository({ index, mappings, - onBeforeWrite - }) => { - const hasPrivileges = hasPrivilegesWithRequest(request); - - const adminCluster = server.plugins.elasticsearch.getCluster('admin'); - const { callWithInternalUser } = adminCluster; - - const repository = new savedObjects.SavedObjectsRepository({ - index, - mappings, - onBeforeWrite, - callCluster: callWithInternalUser - }); - - return new SecureSavedObjectsClient({ - repository, - errors: savedObjects.SavedObjectsClient.errors, - hasPrivileges, - auditLogger, - }); + onBeforeWrite, + callCluster: callWithInternalUser }); - } + + return new SecureSavedObjectsClient({ + repository, + errors: savedObjects.SavedObjectsClient.errors, + hasPrivileges, + auditLogger, + }); + }); getUserProvider(server); From 3c6c061c6b2295202bf3b2dc4fcf3adb4938fff0 Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 11:30:12 -0400 Subject: [PATCH 12/18] No more rbac enabled setting, we just do RBAC --- x-pack/plugins/security/index.js | 1 - x-pack/plugins/security/public/views/management/edit_role.html | 2 +- x-pack/plugins/security/public/views/management/edit_role.js | 1 - x-pack/test/rbac_api_integration/config.js | 1 - 4 files changed, 1 insertion(+), 4 deletions(-) diff --git a/x-pack/plugins/security/index.js b/x-pack/plugins/security/index.js index 1c12c0477bd594..540741250c7b31 100644 --- a/x-pack/plugins/security/index.js +++ b/x-pack/plugins/security/index.js @@ -80,7 +80,6 @@ export const security = (kibana) => new kibana.Plugin({ return { secureCookies: config.get('xpack.security.secureCookies'), sessionTimeout: config.get('xpack.security.sessionTimeout'), - rbacEnabled: config.get('xpack.security.rbac.enabled'), rbacApplication: config.get('xpack.security.rbac.application'), }; } diff --git a/x-pack/plugins/security/public/views/management/edit_role.html b/x-pack/plugins/security/public/views/management/edit_role.html index 8c8c5d692cce21..da03371ae6cbd4 100644 --- a/x-pack/plugins/security/public/views/management/edit_role.html +++ b/x-pack/plugins/security/public/views/management/edit_role.html @@ -111,7 +111,7 @@

-
+
diff --git a/x-pack/plugins/security/public/views/management/edit_role.js b/x-pack/plugins/security/public/views/management/edit_role.js index ffc1962f449378..63f0b2f7159065 100644 --- a/x-pack/plugins/security/public/views/management/edit_role.js +++ b/x-pack/plugins/security/public/views/management/edit_role.js @@ -132,7 +132,6 @@ routes.when(`${EDIT_ROLES_PATH}/:name?`, { $scope.indexPatterns = $route.current.locals.indexPatterns; $scope.privileges = shieldPrivileges; - $scope.rbacEnabled = rbacEnabled; const kibanaApplicationPrivilege = $route.current.locals.kibanaApplicationPrivilege; const role = $route.current.locals.role; $scope.kibanaPrivileges = getKibanaPrivileges(kibanaApplicationPrivilege, role, rbacApplication); diff --git a/x-pack/test/rbac_api_integration/config.js b/x-pack/test/rbac_api_integration/config.js index b9f3f19b0576c9..c2ff96adcff559 100644 --- a/x-pack/test/rbac_api_integration/config.js +++ b/x-pack/test/rbac_api_integration/config.js @@ -54,7 +54,6 @@ export default async function ({ readConfigFile }) { serverArgs: [ ...config.xpack.api.get('kbnTestServer.serverArgs'), '--optimize.enabled=false', - '--xpack.security.rbac.enabled=true', '--server.xsrf.disableProtection=true', ], }, From 10add22149c96c69f84ef20fdb18ce5c84690b12 Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 13:15:23 -0400 Subject: [PATCH 13/18] Using describe to cleanup the test cases --- .../lib/authorization/has_privileges.test.js | 538 +++++++++--------- 1 file changed, 268 insertions(+), 270 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js index 101f188d5acb25..2dd2889450b946 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js @@ -256,288 +256,286 @@ test(`returns success of false using application privileges if the user has the }); }); -test(`returns success of false if the user doesn't have any application privileges and no legacy privileges`, async () => { - const privilege = 'action:saved_objects/config/get'; - const username = 'foo-username'; - const mockServer = createMockServer(); - const callWithRequest = createMockCallWithRequest([ - mockApplicationPrivilegeResponse({ - hasAllRequested: false, - privileges: { - [getVersionPrivilege(defaultVersion)]: false, - [getLoginPrivilege()]: false, - [privilege]: false, - }, - username, - }), - mockLegacyResponse({ - hasAllRequested: false, - privileges: { - read: false, - index: false, - }, +describe('legacy fallback with no application privileges', () => { + test(`returns success of false if the user has no legacy privileges`, async () => { + const privilege = 'action:saved_objects/config/get'; + const username = 'foo-username'; + const mockServer = createMockServer(); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege]: false, + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: false, + index: false, + }, + username, + }) + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege]; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: false, + missing: [getLoginPrivilege(), ...privileges], username, - }) - ]); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const request = Symbol(); - const hasPrivileges = hasPrivilegesWithRequest(request); - const privileges = [privilege]; - const result = await hasPrivileges(privileges); - - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - applications: [{ - application: defaultApplication, - resources: [DEFAULT_RESOURCE], - privileges: [ - getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges - ] - }] - } - }); - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - index: [{ - names: [ defaultKibanaIndex ], - privileges: ['read', 'index'] - }] - } - }); - expect(result).toEqual({ - success: false, - missing: [getLoginPrivilege(), ...privileges], - username, + }); }); -}); -//eslint-disable-next-line max-len -test(`returns success of true if the user doesn't have any application privileges but they have index privilege on kibana index`, async () => { - const privilege = 'something-completely-arbitrary'; - const username = 'foo-username'; - const mockServer = createMockServer(); - const callWithRequest = createMockCallWithRequest([ - mockApplicationPrivilegeResponse({ - hasAllRequested: false, - privileges: { - [getVersionPrivilege(defaultVersion)]: false, - [getLoginPrivilege()]: false, - [privilege]: false, - }, - username, - }), - mockLegacyResponse({ - hasAllRequested: false, - privileges: { - read: false, - index: true, - }, + test(`returns success of true if the user has index privilege on kibana index`, async () => { + const privilege = 'something-completely-arbitrary'; + const username = 'foo-username'; + const mockServer = createMockServer(); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege]: false, + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: false, + index: true, + }, + username, + }) + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = ['foo']; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: true, + missing: [], username, - }) - ]); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const request = Symbol(); - const hasPrivileges = hasPrivilegesWithRequest(request); - const privileges = ['foo']; - const result = await hasPrivileges(privileges); - - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - applications: [{ - application: defaultApplication, - resources: [DEFAULT_RESOURCE], - privileges: [ - getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges - ] - }] - } - }); - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - index: [{ - names: [ defaultKibanaIndex ], - privileges: ['read', 'index'] - }] - } + }); }); - expect(result).toEqual({ - success: true, - missing: [], - username, - }); -}); -//eslint-disable-next-line max-len -test(`returns success of false if the user doesn't have any application privileges and they have the read privilege on kibana but the privilege isn't a read action`, async () => { - const privilege = 'something-completely-arbitrary'; - const username = 'foo-username'; - const mockServer = createMockServer(); - const callWithRequest = createMockCallWithRequest([ - mockApplicationPrivilegeResponse({ - hasAllRequested: false, - privileges: { - [getVersionPrivilege(defaultVersion)]: false, - [getLoginPrivilege()]: false, - [privilege]: false, - }, - username, - }), - mockLegacyResponse({ - hasAllRequested: false, - privileges: { - read: true, - index: false, - }, + test(`returns success of false if the user has the read privilege on kibana index but the privilege isn't a read action`, async () => { + const privilege = 'something-completely-arbitrary'; + const username = 'foo-username'; + const mockServer = createMockServer(); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege]: false, + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: true, + index: false, + }, + username, + }) + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege]; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: false, + missing: [ privilege ], username, - }) - ]); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const request = Symbol(); - const hasPrivileges = hasPrivilegesWithRequest(request); - const privileges = [privilege]; - const result = await hasPrivileges(privileges); - - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - applications: [{ - application: defaultApplication, - resources: [DEFAULT_RESOURCE], - privileges: [ - getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges - ] - }] - } + }); }); - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - index: [{ - names: [ defaultKibanaIndex ], - privileges: ['read', 'index'] - }] - } - }); - expect(result).toEqual({ - success: false, - missing: [ privilege ], - username, - }); -}); -//eslint-disable-next-line max-len -test(`returns success of false if the user doesn't have any application privileges and they have the read privilege on kibana but one privilege isn't a read action`, async () => { - const privilege1 = 'something-completely-arbitrary'; - const privilege2 = 'action:saved_objects/config/get'; - const username = 'foo-username'; - const mockServer = createMockServer(); - const callWithRequest = createMockCallWithRequest([ - mockApplicationPrivilegeResponse({ - hasAllRequested: false, - privileges: { - [getVersionPrivilege(defaultVersion)]: false, - [getLoginPrivilege()]: false, - [privilege1]: false, - [privilege2]: true - }, - username, - }), - mockLegacyResponse({ - hasAllRequested: false, - privileges: { - read: true, - index: false, - }, + test(`returns success of false if the user has the read privilege on kibana index but one privilege isn't a read action`, async () => { + const privilege1 = 'something-completely-arbitrary'; + const privilege2 = 'action:saved_objects/config/get'; + const username = 'foo-username'; + const mockServer = createMockServer(); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege1]: false, + [privilege2]: true + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: true, + index: false, + }, + username, + }) + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege1, privilege2]; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: false, + missing: [ privilege1 ], username, - }) - ]); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const request = Symbol(); - const hasPrivileges = hasPrivilegesWithRequest(request); - const privileges = [privilege1, privilege2]; - const result = await hasPrivileges(privileges); - - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - applications: [{ - application: defaultApplication, - resources: [DEFAULT_RESOURCE], - privileges: [ - getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges - ] - }] - } + }); }); - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - index: [{ - names: [ defaultKibanaIndex ], - privileges: ['read', 'index'] - }] - } - }); - expect(result).toEqual({ - success: false, - missing: [ privilege1 ], - username, - }); -}); -//eslint-disable-next-line max-len -test(`returns success of true if the user doesn't have any application privileges and they have the read privilege on kibana and the privilege is a read action`, async () => { - const privilege = 'action:saved_objects/config/get'; - const username = 'foo-username'; - const mockServer = createMockServer(); - const callWithRequest = createMockCallWithRequest([ - mockApplicationPrivilegeResponse({ - hasAllRequested: false, - privileges: { - [getVersionPrivilege(defaultVersion)]: false, - [getLoginPrivilege()]: false, - [privilege]: false, - }, - username, - }), - mockLegacyResponse({ - hasAllRequested: false, - privileges: { - read: true, - index: false, - }, + test(`returns success of true if the user has the read privilege on kibana index and the privilege is a read action`, async () => { + const privilege = 'action:saved_objects/config/get'; + const username = 'foo-username'; + const mockServer = createMockServer(); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege]: false, + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: true, + index: false, + }, + username, + }) + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege]; + const result = await hasPrivileges(privileges); + + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: true, + missing: [], username, - }) - ]); - - const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); - const request = Symbol(); - const hasPrivileges = hasPrivilegesWithRequest(request); - const privileges = [privilege]; - const result = await hasPrivileges(privileges); - - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - applications: [{ - application: defaultApplication, - resources: [DEFAULT_RESOURCE], - privileges: [ - getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges - ] - }] - } - }); - expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { - body: { - index: [{ - names: [ defaultKibanaIndex ], - privileges: ['read', 'index'] - }] - } - }); - expect(result).toEqual({ - success: true, - missing: [], - username, + }); }); }); From b9ab949e4e08e650de5211c38cd0c0e9498298ab Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 13:29:26 -0400 Subject: [PATCH 14/18] Logging deprecations when using the legacy fallback --- .../lib/authorization/has_privileges.js | 10 ++++++++- .../lib/authorization/has_privileges.test.js | 21 ++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.js index 5e918e84dbe164..787aaf41a2abd9 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.js @@ -35,7 +35,7 @@ const hasApplicationPrivileges = async (callWithRequest, request, kibanaVersion, }; }; -const hasLegacyPrivileges = async (callWithRequest, request, kibanaVersion, application, kibanaIndex, privileges) => { +const hasLegacyPrivileges = async (deprecationLogger, callWithRequest, request, kibanaVersion, application, kibanaIndex, privileges) => { const privilegeCheck = await callWithRequest(request, 'shield.hasPrivileges', { body: { index: [{ @@ -46,6 +46,9 @@ const hasLegacyPrivileges = async (callWithRequest, request, kibanaVersion, appl }); if (privilegeCheck.index[kibanaIndex].index) { + deprecationLogger( + `Relying on implicit privileges determined from the index privileges is deprecated and will be removed in the next major version` + ); return { username: privilegeCheck.username, hasAllRequested: true, @@ -57,6 +60,9 @@ const hasLegacyPrivileges = async (callWithRequest, request, kibanaVersion, appl } if (privilegeCheck.index[kibanaIndex].read) { + deprecationLogger( + `Relying on implicit privileges determined from the index privileges is deprecated and will be removed in the next major version` + ); const privilegeMap = buildPrivilegeMap(application, kibanaVersion); const implicitPrivileges = privileges.reduce((acc, name) => { acc[name] = privilegeMap.read.actions.includes(name); @@ -87,6 +93,7 @@ export function hasPrivilegesWithServer(server) { const kibanaVersion = config.get('pkg.version'); const application = config.get('xpack.security.rbac.application'); const kibanaIndex = config.get('kibana.index'); + const deprecationLogger = (msg) => server.log(['warning', 'deprecated', 'security'], msg); return function hasPrivilegesWithRequest(request) { return async function hasPrivileges(privileges) { @@ -104,6 +111,7 @@ export function hasPrivilegesWithServer(server) { if (!privilegesCheck.privileges[loginPrivilege]) { privilegesCheck = await hasLegacyPrivileges( + deprecationLogger, callWithRequest, request, kibanaVersion, diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js index 2dd2889450b946..ba5e5bd62c2ea4 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js @@ -21,7 +21,8 @@ const createMockServer = ({ settings = {} } = {}) => { const mockServer = { config: jest.fn().mockReturnValue({ get: jest.fn() - }) + }), + log: jest.fn() }; const defaultSettings = { @@ -72,6 +73,14 @@ const createMockCallWithRequest = (responses) => { return mockCallWithRequest; }; +const expectNoDeprecationLogged = (mockServer) => { + expect(mockServer.log).not.toHaveBeenCalled(); +}; + +const expectDeprecationLogged = (mockServer) => { + expect(mockServer.log).toHaveBeenCalledWith(['warning', 'deprecated', 'security'], expect.stringContaining('deprecated')); +}; + test(`returns success of true if they have all application privileges`, async () => { const privilege = 'action:saved_objects/config/get'; const username = 'foo-username'; @@ -95,6 +104,7 @@ test(`returns success of true if they have all application privileges`, async () const privileges = [privilege]; const result = await hasPrivileges(privileges); + expectNoDeprecationLogged(mockServer); expect(mockCallWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ @@ -138,6 +148,7 @@ test(`returns success of false if they have only one application privilege`, asy const privileges = [privilege1, privilege2]; const result = await hasPrivileges(privileges); + expectNoDeprecationLogged(mockServer); expect(mockCallWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ @@ -174,6 +185,7 @@ test(`throws error if missing version privilege and has login privilege`, async const hasPrivileges = hasPrivilegesWithRequest({}); await expect(hasPrivileges([privilege])).rejects.toThrowErrorMatchingSnapshot(); + expectNoDeprecationLogged(mockServer); }); test(`uses application privileges if the user has the login privilege`, async () => { @@ -198,6 +210,7 @@ test(`uses application privileges if the user has the login privilege`, async () const privileges = [privilege]; const result = await hasPrivileges(privileges); + expectNoDeprecationLogged(mockServer); expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ @@ -238,6 +251,7 @@ test(`returns success of false using application privileges if the user has the const privileges = [privilege]; const result = await hasPrivileges(privileges); + expectNoDeprecationLogged(mockServer); expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ @@ -287,6 +301,7 @@ describe('legacy fallback with no application privileges', () => { const privileges = [privilege]; const result = await hasPrivileges(privileges); + expectNoDeprecationLogged(mockServer); expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ @@ -343,6 +358,7 @@ describe('legacy fallback with no application privileges', () => { const privileges = ['foo']; const result = await hasPrivileges(privileges); + expectDeprecationLogged(mockServer); expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ @@ -399,6 +415,7 @@ describe('legacy fallback with no application privileges', () => { const privileges = [privilege]; const result = await hasPrivileges(privileges); + expectDeprecationLogged(mockServer); expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ @@ -457,6 +474,7 @@ describe('legacy fallback with no application privileges', () => { const privileges = [privilege1, privilege2]; const result = await hasPrivileges(privileges); + expectDeprecationLogged(mockServer); expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ @@ -513,6 +531,7 @@ describe('legacy fallback with no application privileges', () => { const privileges = [privilege]; const result = await hasPrivileges(privileges); + expectDeprecationLogged(mockServer); expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { body: { applications: [{ From 0a541abfa8ef9719e164de59cca2ef435edfb497 Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 13:40:28 -0400 Subject: [PATCH 15/18] Cleaning up a bit... --- .../lib/authorization/has_privileges.js | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.js index 787aaf41a2abd9..2e84731f6429b4 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.js @@ -45,29 +45,34 @@ const hasLegacyPrivileges = async (deprecationLogger, callWithRequest, request, } }); + const createPrivileges = (cb) => { + return privileges.reduce((acc, name) => { + acc[name] = cb(name); + return acc; + }, {}); + }; + + // if they have the index privilege, then we grant them all actions if (privilegeCheck.index[kibanaIndex].index) { deprecationLogger( `Relying on implicit privileges determined from the index privileges is deprecated and will be removed in the next major version` ); + + const implicitPrivileges = createPrivileges(() => true); return { username: privilegeCheck.username, hasAllRequested: true, - privileges: privileges.reduce((acc, name) => { - acc[name] = true; - return acc; - }, {}) + privileges: implicitPrivileges }; } + // if they have the read privilege, then we only grant them the read actions if (privilegeCheck.index[kibanaIndex].read) { deprecationLogger( `Relying on implicit privileges determined from the index privileges is deprecated and will be removed in the next major version` ); const privilegeMap = buildPrivilegeMap(application, kibanaVersion); - const implicitPrivileges = privileges.reduce((acc, name) => { - acc[name] = privilegeMap.read.actions.includes(name); - return acc; - }, {}); + const implicitPrivileges = createPrivileges(name => privilegeMap.read.actions.includes(name)); return { username: privilegeCheck.username, @@ -79,10 +84,7 @@ const hasLegacyPrivileges = async (deprecationLogger, callWithRequest, request, return { username: privilegeCheck.username, hasAllRequested: false, - privileges: privileges.reduce((acc, name) => { - acc[name] = false; - return acc; - }, {}) + privileges: createPrivileges(() => false) }; }; From 5d73c1f14943bed695e316f72a27453c7a2dfaa3 Mon Sep 17 00:00:00 2001 From: kobelb Date: Tue, 12 Jun 2018 14:35:43 -0400 Subject: [PATCH 16/18] Using the privilegeMap for the legacy fallback tests --- .../lib/authorization/has_privileges.test.js | 75 +++++++++++++++++-- 1 file changed, 70 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js index ba5e5bd62c2ea4..2462b0a4434607 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.test.js @@ -4,10 +4,11 @@ * you may not use this file except in compliance with the Elastic License. */ +import { sample } from 'lodash'; import { hasPrivilegesWithServer } from './has_privileges'; import { getClient } from '../../../../../server/lib/get_client_shield'; import { DEFAULT_RESOURCE } from '../../../common/constants'; -import { getLoginPrivilege, getVersionPrivilege } from '../privileges'; +import { getLoginPrivilege, getVersionPrivilege, buildPrivilegeMap } from '../privileges'; jest.mock('../../../../../server/lib/get_client_shield', () => ({ getClient: jest.fn() @@ -443,8 +444,9 @@ describe('legacy fallback with no application privileges', () => { }); test(`returns success of false if the user has the read privilege on kibana index but one privilege isn't a read action`, async () => { + const privilegeMap = buildPrivilegeMap(defaultApplication, defaultVersion); const privilege1 = 'something-completely-arbitrary'; - const privilege2 = 'action:saved_objects/config/get'; + const privilege2 = sample(privilegeMap.read.actions); const username = 'foo-username'; const mockServer = createMockServer(); const callWithRequest = createMockCallWithRequest([ @@ -502,7 +504,68 @@ describe('legacy fallback with no application privileges', () => { }); test(`returns success of true if the user has the read privilege on kibana index and the privilege is a read action`, async () => { - const privilege = 'action:saved_objects/config/get'; + const privilegeMap = buildPrivilegeMap(defaultApplication, defaultVersion); + for (const action of privilegeMap.read.actions) { + const privilege = action; + const username = 'foo-username'; + const mockServer = createMockServer(); + const callWithRequest = createMockCallWithRequest([ + mockApplicationPrivilegeResponse({ + hasAllRequested: false, + privileges: { + [getVersionPrivilege(defaultVersion)]: false, + [getLoginPrivilege()]: false, + [privilege]: false, + }, + username, + }), + mockLegacyResponse({ + hasAllRequested: false, + privileges: { + read: true, + index: false, + }, + username, + }) + ]); + + const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); + const request = Symbol(); + const hasPrivileges = hasPrivilegesWithRequest(request); + const privileges = [privilege]; + const result = await hasPrivileges(privileges); + + expectDeprecationLogged(mockServer); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + applications: [{ + application: defaultApplication, + resources: [DEFAULT_RESOURCE], + privileges: [ + getVersionPrivilege(defaultVersion), getLoginPrivilege(), ...privileges + ] + }] + } + }); + expect(callWithRequest).toHaveBeenCalledWith(request, 'shield.hasPrivileges', { + body: { + index: [{ + names: [ defaultKibanaIndex ], + privileges: ['read', 'index'] + }] + } + }); + expect(result).toEqual({ + success: true, + missing: [], + username, + }); + } + }); + + test(`returns success of true if the user has the read privilege on kibana index and all privileges are read actions`, async () => { + const privilegeMap = buildPrivilegeMap(defaultApplication, defaultVersion); + const privileges = privilegeMap.read.actions; const username = 'foo-username'; const mockServer = createMockServer(); const callWithRequest = createMockCallWithRequest([ @@ -511,7 +574,10 @@ describe('legacy fallback with no application privileges', () => { privileges: { [getVersionPrivilege(defaultVersion)]: false, [getLoginPrivilege()]: false, - [privilege]: false, + ...privileges.reduce((acc, name) => { + acc[name] = false; + return acc; + }, {}) }, username, }), @@ -528,7 +594,6 @@ describe('legacy fallback with no application privileges', () => { const hasPrivilegesWithRequest = hasPrivilegesWithServer(mockServer); const request = Symbol(); const hasPrivileges = hasPrivilegesWithRequest(request); - const privileges = [privilege]; const result = await hasPrivileges(privileges); expectDeprecationLogged(mockServer); From d96d00780befed4abf8c562a6265bc5d9d9d436f Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 13 Jun 2018 15:51:58 -0400 Subject: [PATCH 17/18] Now with even less duplication --- .../server/lib/authorization/has_privileges.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/has_privileges.js b/x-pack/plugins/security/server/lib/authorization/has_privileges.js index 2e84731f6429b4..fdb00e63d29f25 100644 --- a/x-pack/plugins/security/server/lib/authorization/has_privileges.js +++ b/x-pack/plugins/security/server/lib/authorization/has_privileges.js @@ -52,12 +52,15 @@ const hasLegacyPrivileges = async (deprecationLogger, callWithRequest, request, }, {}); }; - // if they have the index privilege, then we grant them all actions - if (privilegeCheck.index[kibanaIndex].index) { + const logDeprecation = () => { deprecationLogger( `Relying on implicit privileges determined from the index privileges is deprecated and will be removed in the next major version` ); + }; + // if they have the index privilege, then we grant them all actions + if (privilegeCheck.index[kibanaIndex].index) { + logDeprecation(); const implicitPrivileges = createPrivileges(() => true); return { username: privilegeCheck.username, @@ -68,9 +71,7 @@ const hasLegacyPrivileges = async (deprecationLogger, callWithRequest, request, // if they have the read privilege, then we only grant them the read actions if (privilegeCheck.index[kibanaIndex].read) { - deprecationLogger( - `Relying on implicit privileges determined from the index privileges is deprecated and will be removed in the next major version` - ); + logDeprecation(); const privilegeMap = buildPrivilegeMap(application, kibanaVersion); const implicitPrivileges = createPrivileges(name => privilegeMap.read.actions.includes(name)); From 45efe42796e7f20f490bfec3d18b8af084d8aed7 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 13 Jun 2018 16:05:07 -0400 Subject: [PATCH 18/18] Removing stray `rbacEnabled` from angularjs --- x-pack/plugins/security/public/views/management/edit_role.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/security/public/views/management/edit_role.js b/x-pack/plugins/security/public/views/management/edit_role.js index 7dedd5d4864ac0..0aaebc83e6f72f 100644 --- a/x-pack/plugins/security/public/views/management/edit_role.js +++ b/x-pack/plugins/security/public/views/management/edit_role.js @@ -118,7 +118,7 @@ routes.when(`${EDIT_ROLES_PATH}/:name?`, { } }, controllerAs: 'editRole', - controller($injector, $scope, rbacEnabled, rbacApplication) { + controller($injector, $scope, rbacApplication) { const $route = $injector.get('$route'); const kbnUrl = $injector.get('kbnUrl'); const shieldPrivileges = $injector.get('shieldPrivileges');