From 09b11b61f0fefb736847f5000713bcf6ebfae0b0 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 28 Jul 2020 07:44:37 -0400 Subject: [PATCH] Introduce reserved ml privilege for the apm_user role (#72266) Co-authored-by: Elastic Machine --- x-pack/plugins/infra/server/features.ts | 12 ++++----- .../plugins/ml/common/types/capabilities.ts | 16 ++++++++++++ x-pack/plugins/ml/server/plugin.ts | 6 ++++- .../disable_ui_capabilities.test.ts | 8 +++--- .../authorization/disable_ui_capabilities.ts | 9 +++---- .../feature_privilege_builder/navlink.ts | 5 +--- .../privileges/privileges.test.ts | 25 +++---------------- .../capabilities_switcher.test.ts | 2 +- .../capabilities/capabilities_switcher.ts | 3 +-- .../apis/security/privileges.ts | 2 +- .../apis/security/privileges_basic.ts | 2 +- .../infrastructure_security.ts | 24 +++++++++--------- .../feature_controls/infrastructure_spaces.ts | 22 ++++++++-------- .../infra/feature_controls/logs_security.ts | 24 +++++++++--------- .../infra/feature_controls/logs_spaces.ts | 22 ++++++++-------- .../test/ui_capabilities/common/features.ts | 2 +- .../plugins/foo_plugin/server/index.ts | 6 ++--- .../common/nav_links_builder.ts | 13 ++++++---- .../common/services/features.ts | 2 +- .../common/services/ui_capabilities.ts | 2 +- .../security_only/tests/nav_links.ts | 2 +- 21 files changed, 102 insertions(+), 107 deletions(-) diff --git a/x-pack/plugins/infra/server/features.ts b/x-pack/plugins/infra/server/features.ts index 0de431186b15..fdbd1ec89402 100644 --- a/x-pack/plugins/infra/server/features.ts +++ b/x-pack/plugins/infra/server/features.ts @@ -17,12 +17,12 @@ export const METRICS_FEATURE = { order: 700, icon: 'metricsApp', navLinkId: 'metrics', - app: ['infra', 'kibana'], + app: ['infra', 'metrics', 'kibana'], catalogue: ['infraops'], alerting: [METRIC_THRESHOLD_ALERT_TYPE_ID, METRIC_INVENTORY_THRESHOLD_ALERT_TYPE_ID], privileges: { all: { - app: ['infra', 'kibana'], + app: ['infra', 'metrics', 'kibana'], catalogue: ['infraops'], api: ['infra'], savedObject: { @@ -35,7 +35,7 @@ export const METRICS_FEATURE = { ui: ['show', 'configureSource', 'save', 'alerting:show'], }, read: { - app: ['infra', 'kibana'], + app: ['infra', 'metrics', 'kibana'], catalogue: ['infraops'], api: ['infra'], savedObject: { @@ -58,12 +58,12 @@ export const LOGS_FEATURE = { order: 800, icon: 'logsApp', navLinkId: 'logs', - app: ['infra', 'kibana'], + app: ['infra', 'logs', 'kibana'], catalogue: ['infralogging'], alerting: [LOG_DOCUMENT_COUNT_ALERT_TYPE_ID], privileges: { all: { - app: ['infra', 'kibana'], + app: ['infra', 'logs', 'kibana'], catalogue: ['infralogging'], api: ['infra'], savedObject: { @@ -76,7 +76,7 @@ export const LOGS_FEATURE = { ui: ['show', 'configureSource', 'save'], }, read: { - app: ['infra', 'kibana'], + app: ['infra', 'logs', 'kibana'], catalogue: ['infralogging'], api: ['infra'], alerting: { diff --git a/x-pack/plugins/ml/common/types/capabilities.ts b/x-pack/plugins/ml/common/types/capabilities.ts index 504cd28b8fa1..58a2043502d2 100644 --- a/x-pack/plugins/ml/common/types/capabilities.ts +++ b/x-pack/plugins/ml/common/types/capabilities.ts @@ -7,6 +7,10 @@ import { KibanaRequest } from 'kibana/server'; import { PLUGIN_ID } from '../constants/app'; +export const apmUserMlCapabilities = { + canGetJobs: false, +}; + export const userMlCapabilities = { canAccessML: false, // Anomaly Detection @@ -68,6 +72,7 @@ export function getDefaultCapabilities(): MlCapabilities { } export function getPluginPrivileges() { + const apmUserMlCapabilitiesKeys = Object.keys(apmUserMlCapabilities); const userMlCapabilitiesKeys = Object.keys(userMlCapabilities); const adminMlCapabilitiesKeys = Object.keys(adminMlCapabilities); const allMlCapabilitiesKeys = [...adminMlCapabilitiesKeys, ...userMlCapabilitiesKeys]; @@ -101,6 +106,17 @@ export function getPluginPrivileges() { read: savedObjects, }, }, + apmUser: { + excludeFromBasePrivileges: true, + app: [], + catalogue: [], + savedObject: { + all: [], + read: [], + }, + api: apmUserMlCapabilitiesKeys.map((k) => `ml:${k}`), + ui: apmUserMlCapabilitiesKeys, + }, }; } diff --git a/x-pack/plugins/ml/server/plugin.ts b/x-pack/plugins/ml/server/plugin.ts index 812db744d1bd..3c3824a78503 100644 --- a/x-pack/plugins/ml/server/plugin.ts +++ b/x-pack/plugins/ml/server/plugin.ts @@ -75,7 +75,7 @@ export class MlServerPlugin implements Plugin { new Feature({ id: 'fooFeature', name: 'Foo Feature', - app: ['fooApp'], + app: ['fooApp', 'foo'], navLinkId: 'foo', privileges: null, }), @@ -129,7 +129,7 @@ describe('usingPrivileges', () => { new Feature({ id: 'fooFeature', name: 'Foo Feature', - app: [], + app: ['foo'], navLinkId: 'foo', privileges: null, }), @@ -262,7 +262,7 @@ describe('usingPrivileges', () => { id: 'barFeature', name: 'Bar Feature', navLinkId: 'bar', - app: [], + app: ['bar'], privileges: null, }), ], @@ -412,7 +412,7 @@ describe('all', () => { new Feature({ id: 'fooFeature', name: 'Foo Feature', - app: [], + app: ['foo'], navLinkId: 'foo', privileges: null, }), diff --git a/x-pack/plugins/security/server/authorization/disable_ui_capabilities.ts b/x-pack/plugins/security/server/authorization/disable_ui_capabilities.ts index a9b3fa54d361..c126be1b07f6 100644 --- a/x-pack/plugins/security/server/authorization/disable_ui_capabilities.ts +++ b/x-pack/plugins/security/server/authorization/disable_ui_capabilities.ts @@ -18,12 +18,11 @@ export function disableUICapabilitiesFactory( logger: Logger, authz: AuthorizationServiceSetup ) { - // nav links are sourced from two places: - // 1) The `navLinkId` property. This is deprecated and will be removed (https://github.com/elastic/kibana/issues/66217) - // 2) The apps property. The Kibana Platform associates nav links to the app which registers it, in a 1:1 relationship. - // This behavior is replacing the `navLinkId` property above. + // nav links are sourced from the apps property. + // The Kibana Platform associates nav links to the app which registers it, in a 1:1 relationship. + // This behavior is replacing the `navLinkId` property. const featureNavLinkIds = features - .flatMap((feature) => [feature.navLinkId, ...feature.app]) + .flatMap((feature) => feature.app) .filter((navLinkId) => navLinkId != null); const shouldDisableFeatureUICapability = ( diff --git a/x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/navlink.ts b/x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/navlink.ts index f25632407be8..a6e5a01c7dba 100644 --- a/x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/navlink.ts +++ b/x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/navlink.ts @@ -9,9 +9,6 @@ import { BaseFeaturePrivilegeBuilder } from './feature_privilege_builder'; export class FeaturePrivilegeNavlinkBuilder extends BaseFeaturePrivilegeBuilder { public getActions(privilegeDefinition: FeatureKibanaPrivileges, feature: Feature): string[] { - const appNavLinks = feature.app.map((app) => this.actions.ui.get('navLinks', app)); - return feature.navLinkId - ? [this.actions.ui.get('navLinks', feature.navLinkId), ...appNavLinks] - : appNavLinks; + return (privilegeDefinition.app ?? []).map((app) => this.actions.ui.get('navLinks', app)); } } diff --git a/x-pack/plugins/security/server/authorization/privileges/privileges.test.ts b/x-pack/plugins/security/server/authorization/privileges/privileges.test.ts index d8ece8f68d42..89ac73c22075 100644 --- a/x-pack/plugins/security/server/authorization/privileges/privileges.test.ts +++ b/x-pack/plugins/security/server/authorization/privileges/privileges.test.ts @@ -54,20 +54,8 @@ describe('features', () => { const actual = privileges.get(); expect(actual).toHaveProperty('features.foo-feature', { - all: [ - actions.login, - actions.version, - actions.ui.get('navLinks', 'kibana:foo'), - actions.ui.get('navLinks', 'app-1'), - actions.ui.get('navLinks', 'app-2'), - ], - read: [ - actions.login, - actions.version, - actions.ui.get('navLinks', 'kibana:foo'), - actions.ui.get('navLinks', 'app-1'), - actions.ui.get('navLinks', 'app-2'), - ], + all: [actions.login, actions.version], + read: [actions.login, actions.version], }); }); @@ -275,7 +263,6 @@ describe('features', () => { actions.ui.get('catalogue', 'all-catalogue-2'), actions.ui.get('management', 'all-management', 'all-management-1'), actions.ui.get('management', 'all-management', 'all-management-2'), - actions.ui.get('navLinks', 'kibana:foo'), actions.savedObject.get('all-savedObject-all-1', 'bulk_get'), actions.savedObject.get('all-savedObject-all-1', 'get'), actions.savedObject.get('all-savedObject-all-1', 'find'), @@ -386,7 +373,6 @@ describe('features', () => { actions.ui.get('catalogue', 'read-catalogue-2'), actions.ui.get('management', 'read-management', 'read-management-1'), actions.ui.get('management', 'read-management', 'read-management-2'), - actions.ui.get('navLinks', 'kibana:foo'), actions.savedObject.get('read-savedObject-all-1', 'bulk_get'), actions.savedObject.get('read-savedObject-all-1', 'get'), actions.savedObject.get('read-savedObject-all-1', 'find'), @@ -644,12 +630,7 @@ describe('reserved', () => { const privileges = privilegesFactory(actions, mockXPackMainPlugin as any, mockLicenseService); const actual = privileges.get(); - expect(actual).toHaveProperty('reserved.foo', [ - actions.version, - actions.ui.get('navLinks', 'kibana:foo'), - actions.ui.get('navLinks', 'app-1'), - actions.ui.get('navLinks', 'app-2'), - ]); + expect(actual).toHaveProperty('reserved.foo', [actions.version]); }); test(`actions only specified at the privilege are alright too`, () => { diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts index 797d7fd1bdcc..c9ea1b44e723 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts @@ -23,7 +23,7 @@ const features = ([ id: 'feature_2', name: 'Feature 2', navLinkId: 'feature2', - app: [], + app: ['feature2'], catalogue: ['feature2Entry'], management: { kibana: ['somethingElse'], diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts index 00e2419136f4..e8d964b22010 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts @@ -83,8 +83,7 @@ function toggleDisabledFeatures( for (const feature of disabledFeatures) { // Disable associated navLink, if one exists - const featureNavLinks = feature.navLinkId ? [feature.navLinkId, ...feature.app] : feature.app; - featureNavLinks.forEach((app) => { + feature.app.forEach((app) => { if (navLinks.hasOwnProperty(app) && !enabledAppEntries.has(app)) { navLinks[app] = false; } diff --git a/x-pack/test/api_integration/apis/security/privileges.ts b/x-pack/test/api_integration/apis/security/privileges.ts index 1ad25a11be87..07233f168538 100644 --- a/x-pack/test/api_integration/apis/security/privileges.ts +++ b/x-pack/test/api_integration/apis/security/privileges.ts @@ -43,7 +43,7 @@ export default function ({ getService }: FtrProviderContext) { }, global: ['all', 'read'], space: ['all', 'read'], - reserved: ['ml_user', 'ml_admin', 'monitoring'], + reserved: ['ml_user', 'ml_admin', 'ml_apm_user', 'monitoring'], }; await supertest diff --git a/x-pack/test/api_integration/apis/security/privileges_basic.ts b/x-pack/test/api_integration/apis/security/privileges_basic.ts index d5263aed26d0..74d95fa1e4a7 100644 --- a/x-pack/test/api_integration/apis/security/privileges_basic.ts +++ b/x-pack/test/api_integration/apis/security/privileges_basic.ts @@ -41,7 +41,7 @@ export default function ({ getService }: FtrProviderContext) { }, global: ['all', 'read'], space: ['all', 'read'], - reserved: ['ml_user', 'ml_admin', 'monitoring'], + reserved: ['ml_user', 'ml_admin', 'ml_apm_user', 'monitoring'], }; await supertest diff --git a/x-pack/test/functional/apps/infra/feature_controls/infrastructure_security.ts b/x-pack/test/functional/apps/infra/feature_controls/infrastructure_security.ts index 971826112a3e..3c471516e9c6 100644 --- a/x-pack/test/functional/apps/infra/feature_controls/infrastructure_security.ts +++ b/x-pack/test/functional/apps/infra/feature_controls/infrastructure_security.ts @@ -423,19 +423,19 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) { expect(navLinks).to.not.contain(['Metrics']); }); - it(`metrics app is inaccessible and Application Not Found message is rendered`, async () => { - await PageObjects.common.navigateToApp('infraOps'); - await testSubjects.existOrFail('~appNotFoundPageContent'); - await PageObjects.common.navigateToUrlWithBrowserHistory( - 'infraOps', - '/inventory', - undefined, - { - ensureCurrentUrl: false, - shouldLoginIfPrompted: false, - } + it(`metrics app is inaccessible and returns a 404`, async () => { + await PageObjects.common.navigateToActualUrl('infraOps', '', { + ensureCurrentUrl: false, + shouldLoginIfPrompted: false, + }); + const messageText = await PageObjects.common.getBodyText(); + expect(messageText).to.eql( + JSON.stringify({ + statusCode: 404, + error: 'Not Found', + message: 'Not Found', + }) ); - await testSubjects.existOrFail('~appNotFoundPageContent'); }); }); }); diff --git a/x-pack/test/functional/apps/infra/feature_controls/infrastructure_spaces.ts b/x-pack/test/functional/apps/infra/feature_controls/infrastructure_spaces.ts index 211a9ce718b5..1bf8ded69016 100644 --- a/x-pack/test/functional/apps/infra/feature_controls/infrastructure_spaces.ts +++ b/x-pack/test/functional/apps/infra/feature_controls/infrastructure_spaces.ts @@ -79,21 +79,19 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) { }); it(`metrics app is inaccessible and Application Not Found message is rendered`, async () => { - await PageObjects.common.navigateToApp('infraOps', { + await PageObjects.common.navigateToActualUrl('infraOps', '', { + ensureCurrentUrl: false, + shouldLoginIfPrompted: false, basePath: '/s/custom_space', }); - await testSubjects.existOrFail('~appNotFoundPageContent'); - await PageObjects.common.navigateToUrlWithBrowserHistory( - 'infraOps', - '/inventory', - undefined, - { - basePath: '/s/custom_space', - ensureCurrentUrl: false, - shouldLoginIfPrompted: false, - } + const messageText = await PageObjects.common.getBodyText(); + expect(messageText).to.eql( + JSON.stringify({ + statusCode: 404, + error: 'Not Found', + message: 'Not Found', + }) ); - await testSubjects.existOrFail('~appNotFoundPageContent'); }); }); diff --git a/x-pack/test/functional/apps/infra/feature_controls/logs_security.ts b/x-pack/test/functional/apps/infra/feature_controls/logs_security.ts index c7d94f86ea42..64154ff6cf3f 100644 --- a/x-pack/test/functional/apps/infra/feature_controls/logs_security.ts +++ b/x-pack/test/functional/apps/infra/feature_controls/logs_security.ts @@ -187,19 +187,19 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) { expect(navLinks).to.not.contain('Logs'); }); - it(`logs app is inaccessible and Application Not Found message is rendered`, async () => { - await PageObjects.common.navigateToApp('infraLogs'); - await testSubjects.existOrFail('~appNotFoundPageContent'); - await PageObjects.common.navigateToUrlWithBrowserHistory( - 'infraLogs', - '/stream', - undefined, - { - ensureCurrentUrl: false, - shouldLoginIfPrompted: false, - } + it(`logs app is inaccessible and returns a 404`, async () => { + await PageObjects.common.navigateToActualUrl('infraLogs', '', { + ensureCurrentUrl: false, + shouldLoginIfPrompted: false, + }); + const messageText = await PageObjects.common.getBodyText(); + expect(messageText).to.eql( + JSON.stringify({ + statusCode: 404, + error: 'Not Found', + message: 'Not Found', + }) ); - await testSubjects.existOrFail('~appNotFoundPageContent'); }); }); }); diff --git a/x-pack/test/functional/apps/infra/feature_controls/logs_spaces.ts b/x-pack/test/functional/apps/infra/feature_controls/logs_spaces.ts index 4d54539a4d09..ea08307ccedd 100644 --- a/x-pack/test/functional/apps/infra/feature_controls/logs_spaces.ts +++ b/x-pack/test/functional/apps/infra/feature_controls/logs_spaces.ts @@ -80,21 +80,19 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) { }); it(`logs app is inaccessible and Application Not Found message is rendered`, async () => { - await PageObjects.common.navigateToApp('infraLogs', { + await PageObjects.common.navigateToActualUrl('infraLogs', '', { + ensureCurrentUrl: false, + shouldLoginIfPrompted: false, basePath: '/s/custom_space', }); - await testSubjects.existOrFail('~appNotFoundPageContent'); - await PageObjects.common.navigateToUrlWithBrowserHistory( - 'infraLogs', - '/stream', - undefined, - { - basePath: '/s/custom_space', - ensureCurrentUrl: false, - shouldLoginIfPrompted: false, - } + const messageText = await PageObjects.common.getBodyText(); + expect(messageText).to.eql( + JSON.stringify({ + statusCode: 404, + error: 'Not Found', + message: 'Not Found', + }) ); - await testSubjects.existOrFail('~appNotFoundPageContent'); }); }); }); diff --git a/x-pack/test/ui_capabilities/common/features.ts b/x-pack/test/ui_capabilities/common/features.ts index 3c015bc21e93..e3febc945c29 100644 --- a/x-pack/test/ui_capabilities/common/features.ts +++ b/x-pack/test/ui_capabilities/common/features.ts @@ -5,7 +5,7 @@ */ interface Feature { - navLinkId: string; + app: string[]; } export interface Features { diff --git a/x-pack/test/ui_capabilities/common/fixtures/plugins/foo_plugin/server/index.ts b/x-pack/test/ui_capabilities/common/fixtures/plugins/foo_plugin/server/index.ts index bff794801119..5c80b4283a69 100644 --- a/x-pack/test/ui_capabilities/common/fixtures/plugins/foo_plugin/server/index.ts +++ b/x-pack/test/ui_capabilities/common/fixtures/plugins/foo_plugin/server/index.ts @@ -19,11 +19,11 @@ class FooPlugin implements Plugin { name: 'Foo', icon: 'upArrow', navLinkId: 'foo_plugin', - app: ['kibana'], + app: ['foo_plugin', 'kibana'], catalogue: ['foo'], privileges: { all: { - app: ['kibana'], + app: ['foo_plugin', 'kibana'], catalogue: ['foo'], savedObject: { all: ['foo'], @@ -32,7 +32,7 @@ class FooPlugin implements Plugin { ui: ['create', 'edit', 'delete', 'show'], }, read: { - app: ['kibana'], + app: ['foo_plugin', 'kibana'], catalogue: ['foo'], savedObject: { all: [], diff --git a/x-pack/test/ui_capabilities/common/nav_links_builder.ts b/x-pack/test/ui_capabilities/common/nav_links_builder.ts index b20a499ba7e2..04ab08e08a2b 100644 --- a/x-pack/test/ui_capabilities/common/nav_links_builder.ts +++ b/x-pack/test/ui_capabilities/common/nav_links_builder.ts @@ -13,11 +13,14 @@ export class NavLinksBuilder { ...features, // management isn't a first-class "feature", but it makes our life easier here to pretend like it is management: { - navLinkId: 'kibana:stack_management', + app: ['kibana:stack_management'], }, // TODO: Temp until navLinkIds fix is merged in appSearch: { - navLinkId: 'appSearch', + app: ['appSearch', 'workplaceSearch'], + }, + kibana: { + app: ['kibana'], }, }; } @@ -38,9 +41,9 @@ export class NavLinksBuilder { private build(callback: buildCallback): Record { const navLinks = {} as Record; for (const [featureId, feature] of Object.entries(this.features)) { - if (feature.navLinkId) { - navLinks[feature.navLinkId] = callback(featureId); - } + feature.app.forEach((app) => { + navLinks[app] = callback(featureId); + }); } return navLinks; diff --git a/x-pack/test/ui_capabilities/common/services/features.ts b/x-pack/test/ui_capabilities/common/services/features.ts index 0f796c1d0a0c..5f6ec0ad050c 100644 --- a/x-pack/test/ui_capabilities/common/services/features.ts +++ b/x-pack/test/ui_capabilities/common/services/features.ts @@ -40,7 +40,7 @@ export class FeaturesService { (acc: Features, feature: any) => ({ ...acc, [feature.id]: { - navLinkId: feature.navLinkId, + app: feature.app, }, }), {} diff --git a/x-pack/test/ui_capabilities/common/services/ui_capabilities.ts b/x-pack/test/ui_capabilities/common/services/ui_capabilities.ts index bb1f3b6eefe4..7f831973aea5 100644 --- a/x-pack/test/ui_capabilities/common/services/ui_capabilities.ts +++ b/x-pack/test/ui_capabilities/common/services/ui_capabilities.ts @@ -52,7 +52,7 @@ export class UICapabilitiesService { }): Promise { const features = await this.featureService.get(); const applications = Object.values(features) - .map((feature) => feature.navLinkId) + .flatMap((feature) => feature.app) .filter((link) => !!link); const spaceUrlPrefix = spaceId ? `/s/${spaceId}` : ''; diff --git a/x-pack/test/ui_capabilities/security_only/tests/nav_links.ts b/x-pack/test/ui_capabilities/security_only/tests/nav_links.ts index 18838e536cf9..d7a0dfa1cf80 100644 --- a/x-pack/test/ui_capabilities/security_only/tests/nav_links.ts +++ b/x-pack/test/ui_capabilities/security_only/tests/nav_links.ts @@ -57,7 +57,7 @@ export default function navLinksTests({ getService }: FtrProviderContext) { expect(uiCapabilities.success).to.be(true); expect(uiCapabilities.value).to.have.property('navLinks'); expect(uiCapabilities.value!.navLinks).to.eql( - navLinksBuilder.only('management', 'foo') + navLinksBuilder.only('management', 'foo', 'kibana') ); break; case 'legacy_all':