From deeb9fe32af717a883727aed7d83c6106d8d839f Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Wed, 6 Nov 2024 16:06:39 +0200 Subject: [PATCH] fix(security, features): do not expose UI capabilities of the deprecated features (#198656) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This PR ensures that we don’t expose UI capabilities for deprecated features since they’re unnecessary, and the code should rely on the UI capabilities of the replacement features instead. Additionally, this PR transforms the `disabledFeatures` property of Space objects returned from our programmatic and HTTP APIs to replace any deprecated feature IDs with the IDs of their replacement features, ensuring that feature visibility toggles work for deprecated features as well. ## How to test 1. Run Kibana FTR server with the following config (registers test deprecated features): ```shell node scripts/functional_tests_server.js --config x-pack/test/security_api_integration/features.config.ts ``` 2. Once server is up and running create Space with the `case_1_feature_a` **deprecated** feature disabled: ```shell curl 'http://localhost:5620/api/spaces/space' -u elastic:changeme \ -X POST -H 'Content-Type: application/json' -H 'kbn-version: 9.0.0' \ --data-raw '{"name":"space-alpha","id":"space-alpha","initials":"s","color":"#D6BF57","disabledFeatures":["case_1_feature_a"],"imageUrl":""}' ``` 3. Log in to Kibana and [navigate to a Space `space-alpha`](http://localhost:5620/app/management/kibana/spaces/edit/space-alpha) you've just created. Observe that deprecated `Case #1 feature A` (`case_1_feature_a`) isn't displayed, and instead you should see that replaces deprecated one - `Case #1 feature B` (`case_1_feature_b`): ![Screen Shot 2024-11-01 at 17 40 59](https://github.com/user-attachments/assets/5b91e71c-7d46-4ff1-bf73-d148622e8ec4) Co-authored-by: Elastic Machine --- x-pack/plugins/features/server/mocks.ts | 4 +- x-pack/plugins/features/server/plugin.ts | 3 +- .../authorization_service.test.ts | 10 +- x-pack/plugins/security/server/plugin.test.ts | 4 +- .../capabilities_switcher.test.ts | 28 ++++- .../capabilities/capabilities_switcher.ts | 2 +- .../utils/space_solution_disabled_features.ts | 1 + .../server/routes/api/external/post.test.ts | 6 +- .../server/routes/api/external/put.test.ts | 6 +- .../spaces_client/spaces_client.test.ts | 48 ++++++++ .../server/spaces_client/spaces_client.ts | 59 ++++++++- .../plugins/features_provider/server/index.ts | 30 ++++- .../features_provider/server/init_routes.ts | 25 ++++ .../tests/features/deprecated_features.ts | 113 ++++++++++++++++-- x-pack/test/ui_capabilities/common/config.ts | 4 + 15 files changed, 301 insertions(+), 42 deletions(-) create mode 100644 x-pack/test/security_api_integration/plugins/features_provider/server/init_routes.ts diff --git a/x-pack/plugins/features/server/mocks.ts b/x-pack/plugins/features/server/mocks.ts index bb2292a45377f..15339b068e7e8 100644 --- a/x-pack/plugins/features/server/mocks.ts +++ b/x-pack/plugins/features/server/mocks.ts @@ -25,8 +25,8 @@ const createSetup = (): jest.Mocked => { const createStart = (): jest.Mocked => { return { - getKibanaFeatures: jest.fn(), - getElasticsearchFeatures: jest.fn(), + getKibanaFeatures: jest.fn().mockReturnValue([]), + getElasticsearchFeatures: jest.fn().mockReturnValue([]), }; }; diff --git a/x-pack/plugins/features/server/plugin.ts b/x-pack/plugins/features/server/plugin.ts index 15888358bb773..9f6cae36f6aee 100644 --- a/x-pack/plugins/features/server/plugin.ts +++ b/x-pack/plugins/features/server/plugin.ts @@ -138,7 +138,8 @@ export class FeaturesPlugin this.featureRegistry.validateFeatures(); this.capabilities = uiCapabilitiesForFeatures( - this.featureRegistry.getAllKibanaFeatures(), + // Don't expose capabilities of the deprecated features. + this.featureRegistry.getAllKibanaFeatures({ omitDeprecated: true }), this.featureRegistry.getAllElasticsearchFeatures() ); diff --git a/x-pack/plugins/security/server/authorization/authorization_service.test.ts b/x-pack/plugins/security/server/authorization/authorization_service.test.ts index 275a6d2643f24..de3646166d8f9 100644 --- a/x-pack/plugins/security/server/authorization/authorization_service.test.ts +++ b/x-pack/plugins/security/server/authorization/authorization_service.test.ts @@ -145,12 +145,9 @@ describe('#start', () => { customBranding: mockCoreSetup.customBranding, }); - const featuresStart = featuresPluginMock.createStart(); - featuresStart.getKibanaFeatures.mockReturnValue([]); - authorizationService.start({ clusterClient: mockClusterClient, - features: featuresStart, + features: featuresPluginMock.createStart(), online$: statusSubject.asObservable(), }); @@ -217,12 +214,9 @@ it('#stop unsubscribes from license and ES updates.', async () => { customBranding: mockCoreSetup.customBranding, }); - const featuresStart = featuresPluginMock.createStart(); - featuresStart.getKibanaFeatures.mockReturnValue([]); - authorizationService.start({ clusterClient: mockClusterClient, - features: featuresStart, + features: featuresPluginMock.createStart(), online$: statusSubject.asObservable(), }); diff --git a/x-pack/plugins/security/server/plugin.test.ts b/x-pack/plugins/security/server/plugin.test.ts index 37c6e22a07fab..4b9479f51a0f3 100644 --- a/x-pack/plugins/security/server/plugin.test.ts +++ b/x-pack/plugins/security/server/plugin.test.ts @@ -64,10 +64,8 @@ describe('Security Plugin', () => { mockCoreStart = coreMock.createStart(); - const mockFeaturesStart = featuresPluginMock.createStart(); - mockFeaturesStart.getKibanaFeatures.mockReturnValue([]); mockStartDependencies = { - features: mockFeaturesStart, + features: featuresPluginMock.createStart(), licensing: licensingMock.createStart(), taskManager: taskManagerMock.createStart(), }; 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 d48095638babf..688f8297271a3 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts @@ -66,7 +66,7 @@ const features = [ category: { id: 'securitySolution' }, }, { - // feature 4 intentionally delcares the same items as feature 3 + // feature 4 intentionally declares the same items as feature 3 id: 'feature_4', name: 'Feature 4', app: ['feature3', 'feature3_app'], @@ -87,6 +87,32 @@ const features = [ }, category: { id: 'observability' }, }, + { + deprecated: { notice: 'It was a mistake.' }, + id: 'deprecated_feature', + name: 'Deprecated Feature', + // Expose the same `app` and `catalogue` entries as `feature_2` to make sure they are disabled + // when `feature_2` is disabled even if the deprecated feature isn't explicitly disabled. + app: ['feature2'], + catalogue: ['feature2Entry'], + category: { id: 'deprecated', label: 'deprecated' }, + privileges: { + all: { + savedObject: { all: [], read: [] }, + ui: ['ui_deprecated_all'], + app: ['feature2'], + catalogue: ['feature2Entry'], + replacedBy: [{ feature: 'feature_2', privileges: ['all'] }], + }, + read: { + savedObject: { all: [], read: [] }, + ui: ['ui_deprecated_read'], + app: ['feature2'], + catalogue: ['feature2Entry'], + replacedBy: [{ feature: 'feature_2', privileges: ['all'] }], + }, + }, + }, ] as unknown as KibanaFeature[]; const buildCapabilities = () => diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts index 90ee85fece486..41d5dcdf2cb14 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts @@ -72,7 +72,7 @@ function toggleDisabledFeatures( (acc, feature) => { if (disabledFeatureKeys.includes(feature.id)) { acc.disabledFeatures.push(feature); - } else { + } else if (!feature.deprecated) { acc.enabledFeatures.push(feature); } return acc; diff --git a/x-pack/plugins/spaces/server/lib/utils/space_solution_disabled_features.ts b/x-pack/plugins/spaces/server/lib/utils/space_solution_disabled_features.ts index 066166e7e87dd..6d30645325535 100644 --- a/x-pack/plugins/spaces/server/lib/utils/space_solution_disabled_features.ts +++ b/x-pack/plugins/spaces/server/lib/utils/space_solution_disabled_features.ts @@ -39,6 +39,7 @@ const enabledFeaturesPerSolution: Record = { * This function takes the current space's disabled features and the space solution and returns * the updated array of disabled features. * + * @param features The list of all Kibana registered features. * @param spaceDisabledFeatures The current space's disabled features * @param spaceSolution The current space's solution (es, oblt, security or classic) * @returns The updated array of disabled features diff --git a/x-pack/plugins/spaces/server/routes/api/external/post.test.ts b/x-pack/plugins/spaces/server/routes/api/external/post.test.ts index 984d684762159..88c846b77eb53 100644 --- a/x-pack/plugins/spaces/server/routes/api/external/post.test.ts +++ b/x-pack/plugins/spaces/server/routes/api/external/post.test.ts @@ -56,13 +56,9 @@ describe('Spaces Public API', () => { basePath: httpService.basePath, }); - const featuresPluginMockStart = featuresPluginMock.createStart(); - - featuresPluginMockStart.getKibanaFeatures.mockReturnValue([]); - const usageStatsServicePromise = Promise.resolve(usageStatsServiceMock.createSetupContract()); - const clientServiceStart = clientService.start(coreStart, featuresPluginMockStart); + const clientServiceStart = clientService.start(coreStart, featuresPluginMock.createStart()); const spacesServiceStart = service.start({ basePath: coreStart.http.basePath, diff --git a/x-pack/plugins/spaces/server/routes/api/external/put.test.ts b/x-pack/plugins/spaces/server/routes/api/external/put.test.ts index 8aa71d30fc4bb..cf2e9981fd024 100644 --- a/x-pack/plugins/spaces/server/routes/api/external/put.test.ts +++ b/x-pack/plugins/spaces/server/routes/api/external/put.test.ts @@ -56,13 +56,9 @@ describe('PUT /api/spaces/space', () => { basePath: httpService.basePath, }); - const featuresPluginMockStart = featuresPluginMock.createStart(); - - featuresPluginMockStart.getKibanaFeatures.mockReturnValue([]); - const usageStatsServicePromise = Promise.resolve(usageStatsServiceMock.createSetupContract()); - const clientServiceStart = clientService.start(coreStart, featuresPluginMockStart); + const clientServiceStart = clientService.start(coreStart, featuresPluginMock.createStart()); const spacesServiceStart = service.start({ basePath: coreStart.http.basePath, diff --git a/x-pack/plugins/spaces/server/spaces_client/spaces_client.test.ts b/x-pack/plugins/spaces/server/spaces_client/spaces_client.test.ts index 364afdcaba66a..4b7c1de0b3fcb 100644 --- a/x-pack/plugins/spaces/server/spaces_client/spaces_client.test.ts +++ b/x-pack/plugins/spaces/server/spaces_client/spaces_client.test.ts @@ -55,6 +55,37 @@ const features = [ catalogue: ['feature3Entry'], category: { id: 'securitySolution' }, }, + { + deprecated: { notice: 'It was a mistake.' }, + id: 'feature_4_deprecated', + name: 'Deprecated Feature', + app: ['feature2', 'feature3'], + catalogue: ['feature2Entry', 'feature3Entry'], + category: { id: 'deprecated', label: 'deprecated' }, + scope: ['spaces', 'security'], + privileges: { + all: { + savedObject: { all: [], read: [] }, + ui: [], + app: ['feature2', 'feature3'], + catalogue: ['feature2Entry', 'feature3Entry'], + replacedBy: [ + { feature: 'feature_2', privileges: ['all'] }, + { feature: 'feature_3', privileges: ['all'] }, + ], + }, + read: { + savedObject: { all: [], read: [] }, + ui: [], + app: ['feature2', 'feature3'], + catalogue: ['feature2Entry', 'feature3Entry'], + replacedBy: [ + { feature: 'feature_2', privileges: ['read'] }, + { feature: 'feature_3', privileges: ['read'] }, + ], + }, + }, + }, ] as unknown as KibanaFeature[]; const featuresStart = featuresPluginMock.createStart(); @@ -103,6 +134,17 @@ describe('#getAll', () => { bar: 'baz-bar', // an extra attribute that will be ignored during conversion }, }, + { + // alpha only has deprecated disabled features + id: 'alpha', + type: 'space', + references: [], + attributes: { + name: 'alpha-name', + description: 'alpha-description', + disabledFeatures: ['feature_1', 'feature_4_deprecated'], + }, + }, ]; const expectedSpaces: Space[] = [ @@ -130,6 +172,12 @@ describe('#getAll', () => { description: 'baz-description', disabledFeatures: [], }, + { + id: 'alpha', + name: 'alpha-name', + description: 'alpha-description', + disabledFeatures: ['feature_1', 'feature_2', 'feature_3'], + }, ]; test(`finds spaces using callWithRequestRepository`, async () => { diff --git a/x-pack/plugins/spaces/server/spaces_client/spaces_client.ts b/x-pack/plugins/spaces/server/spaces_client/spaces_client.ts index 5d7ae1159f5ea..66728636f9752 100644 --- a/x-pack/plugins/spaces/server/spaces_client/spaces_client.ts +++ b/x-pack/plugins/spaces/server/spaces_client/spaces_client.ts @@ -14,6 +14,7 @@ import type { SavedObject, } from '@kbn/core/server'; import type { LegacyUrlAliasTarget } from '@kbn/core-saved-objects-common'; +import type { KibanaFeature } from '@kbn/features-plugin/common'; import { KibanaFeatureScope } from '@kbn/features-plugin/common'; import type { FeaturesPluginStart } from '@kbn/features-plugin/server'; @@ -84,7 +85,13 @@ export interface ISpacesClient { * Client for interacting with spaces. */ export class SpacesClient implements ISpacesClient { - private isServerless = false; + private readonly isServerless: boolean; + + /** + * A map of deprecated feature IDs to the feature IDs that replace them used to transform the disabled features + * of a space to make sure they only reference non-deprecated features. + */ + private readonly deprecatedFeaturesReferences: Map>; constructor( private readonly debugLogger: (message: string) => void, @@ -95,6 +102,9 @@ export class SpacesClient implements ISpacesClient { private readonly features: FeaturesPluginStart ) { this.isServerless = this.buildFlavour === 'serverless'; + this.deprecatedFeaturesReferences = this.collectDeprecatedFeaturesReferences( + features.getKibanaFeatures() + ); } public async getAll(options: v1.GetAllSpacesOptions = {}): Promise { @@ -247,6 +257,8 @@ export class SpacesClient implements ISpacesClient { }; private transformSavedObjectToSpace = (savedObject: SavedObject): v1.Space => { + // Solution isn't supported in the serverless offering. + const solution = !this.isServerless ? savedObject.attributes.solution : undefined; return { id: savedObject.id, name: savedObject.attributes.name ?? '', @@ -256,11 +268,13 @@ export class SpacesClient implements ISpacesClient { imageUrl: savedObject.attributes.imageUrl, disabledFeatures: withSpaceSolutionDisabledFeatures( this.features.getKibanaFeatures(), - savedObject.attributes.disabledFeatures ?? [], - !this.isServerless ? savedObject.attributes.solution : undefined + savedObject.attributes.disabledFeatures?.flatMap((featureId: string) => + Array.from(this.deprecatedFeaturesReferences.get(featureId) ?? [featureId]) + ) ?? [], + solution ), _reserved: savedObject.attributes._reserved, - ...(!this.isServerless ? { solution: savedObject.attributes.solution } : {}), + ...(solution ? { solution } : {}), } as v1.Space; }; @@ -275,4 +289,41 @@ export class SpacesClient implements ISpacesClient { ...(!this.isServerless && space.solution ? { solution: space.solution } : {}), }; }; + + /** + * Collects a map of all deprecated feature IDs and the feature IDs that replace them. + * @param features A list of all available Kibana features including deprecated ones. + */ + private collectDeprecatedFeaturesReferences(features: KibanaFeature[]) { + const deprecatedFeatureReferences = new Map(); + for (const feature of features) { + if (!feature.deprecated || !feature.scope?.includes(KibanaFeatureScope.Spaces)) { + continue; + } + + // Collect all feature privileges including the ones provided by sub-features, if any. + const allPrivileges = Object.values(feature.privileges ?? {}).concat( + feature.subFeatures?.flatMap((subFeature) => + subFeature.privilegeGroups.flatMap(({ privileges }) => privileges) + ) ?? [] + ); + + // Collect all features IDs that are referenced by the deprecated feature privileges. + const referencedFeaturesIds = new Set(); + for (const privilege of allPrivileges) { + const replacedBy = privilege.replacedBy + ? 'default' in privilege.replacedBy + ? privilege.replacedBy.default.concat(privilege.replacedBy.minimal) + : privilege.replacedBy + : []; + for (const privilegeReference of replacedBy) { + referencedFeaturesIds.add(privilegeReference.feature); + } + } + + deprecatedFeatureReferences.set(feature.id, referencedFeaturesIds); + } + + return deprecatedFeatureReferences; + } } diff --git a/x-pack/test/security_api_integration/plugins/features_provider/server/index.ts b/x-pack/test/security_api_integration/plugins/features_provider/server/index.ts index 646fe327a0015..61100babefea7 100644 --- a/x-pack/test/security_api_integration/plugins/features_provider/server/index.ts +++ b/x-pack/test/security_api_integration/plugins/features_provider/server/index.ts @@ -9,8 +9,11 @@ import type { PluginSetupContract as AlertingPluginsSetup } from '@kbn/alerting- import { schema } from '@kbn/config-schema'; import type { CoreSetup, Plugin, PluginInitializer } from '@kbn/core/server'; import { DEFAULT_APP_CATEGORIES } from '@kbn/core/server'; +import { KibanaFeatureScope } from '@kbn/features-plugin/common'; import type { FeaturesPluginSetup, FeaturesPluginStart } from '@kbn/features-plugin/server'; +import { initRoutes } from './init_routes'; + export interface PluginSetupDependencies { features: FeaturesPluginSetup; alerting: AlertingPluginsSetup; @@ -23,7 +26,7 @@ export interface PluginStartDependencies { export const plugin: PluginInitializer = async (): Promise< Plugin > => ({ - setup: (_: CoreSetup, deps: PluginSetupDependencies) => { + setup: (core: CoreSetup, deps: PluginSetupDependencies) => { // Case #1: feature A needs to be renamed to feature B. It's unfortunate, but the existing feature A // should be deprecated and re-created as a new feature with the same privileges. case1FeatureRename(deps); @@ -46,6 +49,8 @@ export const plugin: PluginInitializer = async (): Promise< // * `case_4_feature_b_v2` (new, decoupled from `ab` SO, partially replaces `case_4_feature_b`) // * `case_4_feature_c` (new, only for `ab` SO access) case4FeatureExtract(deps); + + initRoutes(core); }, start: () => {}, stop: () => {}, @@ -61,6 +66,7 @@ function case1FeatureRename(deps: PluginSetupDependencies) { all: { savedObject: { all: ['one'], read: [] }, ui: ['ui_all'] }, read: { savedObject: { all: [], read: ['one'] }, ui: ['ui_read'] }, }, + scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], }; // Step 2: mark feature A as deprecated and provide proper replacements for all feature and @@ -96,6 +102,8 @@ function case2FeatureSplit(deps: PluginSetupDependencies) { deps.features.registerKibanaFeature({ deprecated: { notice: 'Case #2 is deprecated.' }, + scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], + app: ['app_one', 'app_two'], catalogue: ['cat_one', 'cat_two'], management: { kibana: ['management_one', 'management_two'] }, @@ -139,6 +147,8 @@ function case2FeatureSplit(deps: PluginSetupDependencies) { read: { savedObject: { all: [], read: ['one', 'two'] }, ui: ['ui_read_one', 'ui_read_two'], + catalogue: ['cat_one', 'cat_two'], + app: ['app_one', 'app_two'], replacedBy: [ { feature: 'case_2_feature_b', privileges: ['read'] }, { feature: 'case_2_feature_c', privileges: ['read'] }, @@ -149,6 +159,8 @@ function case2FeatureSplit(deps: PluginSetupDependencies) { // Step 2: define new features deps.features.registerKibanaFeature({ + scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], + category: DEFAULT_APP_CATEGORIES.kibana, id: 'case_2_feature_b', name: 'Case #2 feature B', @@ -182,10 +194,14 @@ function case2FeatureSplit(deps: PluginSetupDependencies) { read: { savedObject: { all: [], read: ['one'] }, ui: ['ui_read_one'], + catalogue: ['cat_one'], + app: ['app_one'], }, }, }); deps.features.registerKibanaFeature({ + scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], + category: DEFAULT_APP_CATEGORIES.kibana, id: 'case_2_feature_c', name: 'Case #2 feature C', @@ -219,6 +235,8 @@ function case2FeatureSplit(deps: PluginSetupDependencies) { read: { savedObject: { all: [], read: ['two'] }, ui: ['ui_read_two'], + app: ['app_two'], + catalogue: ['cat_two'], }, }, }); @@ -249,6 +267,8 @@ function case3FeatureSplitSubFeature(deps: PluginSetupDependencies) { deps.features.registerKibanaFeature({ deprecated: { notice: 'Case #3 is deprecated.' }, + scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], + category: DEFAULT_APP_CATEGORIES.kibana, id: 'case_3_feature_a', name: 'Case #3 feature A (DEPRECATED)', @@ -275,6 +295,8 @@ function case3FeatureSplitSubFeature(deps: PluginSetupDependencies) { // Step 2: Create a new feature with the desired privileges structure. deps.features.registerKibanaFeature({ + scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], + category: DEFAULT_APP_CATEGORIES.kibana, id: 'case_3_feature_a_v2', name: 'Case #3 feature A', @@ -324,6 +346,8 @@ function case4FeatureExtract(deps: PluginSetupDependencies) { deps.features.registerKibanaFeature({ deprecated: { notice: 'Case #4 is deprecated.' }, + scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], + category: DEFAULT_APP_CATEGORIES.kibana, id: `case_4_feature_${suffix.toLowerCase()}`, name: `Case #4 feature ${suffix} (DEPRECATED)`, @@ -350,6 +374,8 @@ function case4FeatureExtract(deps: PluginSetupDependencies) { // Step 2: introduce new features (v2) with privileges that don't grant access to `ab`. deps.features.registerKibanaFeature({ + scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], + category: DEFAULT_APP_CATEGORIES.kibana, id: `case_4_feature_${suffix.toLowerCase()}_v2`, name: `Case #4 feature ${suffix}`, @@ -363,6 +389,8 @@ function case4FeatureExtract(deps: PluginSetupDependencies) { // Step 3: introduce new feature C that only grants access to `ab`. deps.features.registerKibanaFeature({ + scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], + category: DEFAULT_APP_CATEGORIES.kibana, id: 'case_4_feature_c', name: 'Case #4 feature C', diff --git a/x-pack/test/security_api_integration/plugins/features_provider/server/init_routes.ts b/x-pack/test/security_api_integration/plugins/features_provider/server/init_routes.ts new file mode 100644 index 0000000000000..d58f2f3078a3a --- /dev/null +++ b/x-pack/test/security_api_integration/plugins/features_provider/server/init_routes.ts @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import type { CoreSetup } from '@kbn/core/server'; + +import type { PluginStartDependencies } from '.'; + +export function initRoutes(core: CoreSetup) { + const router = core.http.createRouter(); + + // This route mirrors existing `GET /api/features` route except that it also returns all deprecated features. + router.get( + { path: '/internal/features_provider/features', validate: false }, + async (context, request, response) => { + const [, pluginDeps] = await core.getStartServices(); + return response.ok({ + body: pluginDeps.features.getKibanaFeatures().map((feature) => feature.toRaw()), + }); + } + ); +} diff --git a/x-pack/test/security_api_integration/tests/features/deprecated_features.ts b/x-pack/test/security_api_integration/tests/features/deprecated_features.ts index 030f5ac704d8b..6e868fc5946ec 100644 --- a/x-pack/test/security_api_integration/tests/features/deprecated_features.ts +++ b/x-pack/test/security_api_integration/tests/features/deprecated_features.ts @@ -14,6 +14,7 @@ import type { FeatureKibanaPrivilegesReference, KibanaFeatureConfig, } from '@kbn/features-plugin/common'; +import { KibanaFeatureScope } from '@kbn/features-plugin/common'; import type { Role } from '@kbn/security-plugin-types-common'; import type { FtrProviderContext } from '../../ftr_provider_context'; @@ -163,11 +164,97 @@ export default function ({ getService }: FtrProviderContext) { } }); + it('all deprecated features are known', async () => { + const { body: features } = await supertest + .get('/internal/features_provider/features') + .expect(200); + + // **NOTE**: This test is to ensure the AppEx Security team has a chance to review all features marked as + // deprecated. If you’re adding a new deprecated feature, make sure to add it to the list below manually or by + // running the API integration test locally with the --updateSnapshot flag. + expectSnapshot( + (features as KibanaFeatureConfig[]).flatMap((f) => (f.deprecated ? [f.id] : [])).sort() + ).toMatchInline(` + Array [ + "case_1_feature_a", + "case_2_feature_a", + "case_3_feature_a", + "case_4_feature_a", + "case_4_feature_b", + ] + `); + }); + + it('all deprecated features are replaced by a single feature only', async () => { + const featuresResponse = await supertest + .get('/internal/features_provider/features') + .expect(200); + const features = featuresResponse.body as KibanaFeatureConfig[]; + + // **NOTE**: This test ensures that deprecated features displayed in the Space’s feature visibility toggles screen + // are only replaced by a single feature. This way, if a feature is toggled off for a particular Space, there + // won’t be any ambiguity about which replacement feature should also be toggled off. Currently, we don’t + // anticipate having a deprecated feature replaced by more than one feature, so this test is intended to catch + // such scenarios early. If there’s a need for a deprecated feature to be replaced by multiple features, please + // reach out to the AppEx Security team to discuss how this should affect Space’s feature visibility toggles. + const featureIdsThatSupportMultipleReplacements = new Set([ + 'case_2_feature_a', + 'case_4_feature_a', + 'case_4_feature_b', + ]); + for (const feature of features) { + if ( + !feature.deprecated || + !feature.scope?.includes(KibanaFeatureScope.Spaces) || + featureIdsThatSupportMultipleReplacements.has(feature.id) + ) { + continue; + } + + // Collect all feature privileges including the ones provided by sub-features, if any. + const allPrivileges = Object.values(feature.privileges ?? {}).concat( + feature.subFeatures?.flatMap((subFeature) => + subFeature.privilegeGroups.flatMap(({ privileges }) => privileges) + ) ?? [] + ); + + // Collect all features IDs that are referenced by the deprecated feature privileges. + const referencedFeaturesIds = new Set(); + for (const privilege of allPrivileges) { + const replacedBy = privilege.replacedBy + ? 'default' in privilege.replacedBy + ? privilege.replacedBy.default.concat(privilege.replacedBy.minimal) + : privilege.replacedBy + : []; + for (const privilegeReference of replacedBy) { + referencedFeaturesIds.add(privilegeReference.feature); + } + } + + if (referencedFeaturesIds.size > 1) { + throw new Error( + `Feature "${feature.id}" is deprecated and replaced by more than one feature: ${ + referencedFeaturesIds.size + } features: ${Array.from(referencedFeaturesIds).join( + ', ' + )}. If it's intentional, please contact the AppEx Security team.` + ); + } + } + }); + it('all privileges of the deprecated features should have a proper replacement', async () => { // Fetch all features first. - const featuresResponse = await supertest.get('/api/features').expect(200); + const featuresResponse = await supertest + .get('/internal/features_provider/features') + .expect(200); const features = featuresResponse.body as KibanaFeatureConfig[]; + // Check if the action provided by the deprecated feature is directly replaceable by other + // features. The `ui:`-prefixed actions are special since they are prefixed with a feature ID, + // and do not need to be replaced like any other privilege actions. + const isReplaceableAction = (action: string) => !action.startsWith('ui:'); + // Collect all deprecated features. const deprecatedFeatures = features.filter((f) => f.deprecated); log.info(`Found ${deprecatedFeatures.length} deprecated features.`); @@ -207,7 +294,10 @@ export default function ({ getService }: FtrProviderContext) { ); for (const deprecatedAction of deprecatedActions) { - if (!replacementActions.has(deprecatedAction)) { + if ( + isReplaceableAction(deprecatedAction) && + !replacementActions.has(deprecatedAction) + ) { throw new Error( `Action "${deprecatedAction}" granted by the privilege "${privilegeId}" of the deprecated feature "${feature.id}" is not properly replaced.` ); @@ -225,22 +315,23 @@ export default function ({ getService }: FtrProviderContext) { .send({ applications: [] }) .expect(200); - // Both deprecated and new UI capabilities should be toggled. + // Only new UI capabilities should be toggled, deprecated ones should not be present. expect(capabilities).toEqual( expect.objectContaining({ - // UI flags from the deprecated feature privilege. - case_2_feature_a: { - ui_all_one: true, - ui_all_two: true, - ui_read_one: false, - ui_read_two: false, - }, - // UI flags from the feature privileges that replace deprecated one. case_2_feature_b: { ui_all_one: true, ui_read_one: false }, case_2_feature_c: { ui_all_two: true, ui_read_two: false }, }) ); + for (const deprecatedFeatureId of [ + 'case_1_feature_a', + 'case_2_feature_a', + 'case_3_feature_a', + 'case_4_feature_a', + 'case_4_feature_b', + ]) { + expect(capabilities).not.toHaveProperty(deprecatedFeatureId); + } }); it('Cases privileges are properly handled for deprecated privileges', async () => { diff --git a/x-pack/test/ui_capabilities/common/config.ts b/x-pack/test/ui_capabilities/common/config.ts index ba40e613c0d69..18a96b8e26274 100644 --- a/x-pack/test/ui_capabilities/common/config.ts +++ b/x-pack/test/ui_capabilities/common/config.ts @@ -46,6 +46,10 @@ export function createTestConfig(name: string, options: CreateTestConfigOptions) .filter((k) => k !== 'security') .map((key) => `--xpack.${key}.enabled=false`), `--plugin-path=${path.resolve(__dirname, 'plugins/foo_plugin')}`, + `--plugin-path=${path.resolve( + __dirname, + '../../security_api_integration/plugins/features_provider' + )}`, ], }, };