From 1beef16448f921e4c035fedf4ddb38289c40fa7f Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Tue, 14 Jan 2025 20:23:43 +0200 Subject: [PATCH] feat(security): extend `Feature` definition to support explicit feature replacements --- .../shared/features/common/kibana_feature.ts | 8 +++ .../features/server/feature_registry.test.ts | 48 +++++++++++++++++- .../features/server/feature_registry.ts | 44 ++++++++++++++-- .../shared/features/server/feature_schema.ts | 7 ++- .../plugins/shared/features/server/index.ts | 1 + .../plugins/shared/features/server/plugin.ts | 13 +++-- .../on_post_auth_interceptor.test.ts | 4 +- .../on_post_auth_interceptor.ts | 15 +++--- .../plugins/shared/spaces/server/plugin.ts | 2 +- .../spaces_client/spaces_client.test.ts | 50 ++++++++++++++++++- .../server/spaces_client/spaces_client.ts | 8 +++ .../plugins/features_provider/server/index.ts | 5 +- .../tests/features/deprecated_features.ts | 26 +++++----- 13 files changed, 200 insertions(+), 31 deletions(-) diff --git a/x-pack/platform/plugins/shared/features/common/kibana_feature.ts b/x-pack/platform/plugins/shared/features/common/kibana_feature.ts index dba79a1663fca..d427c28aea575 100644 --- a/x-pack/platform/plugins/shared/features/common/kibana_feature.ts +++ b/x-pack/platform/plugins/shared/features/common/kibana_feature.ts @@ -177,6 +177,14 @@ export interface KibanaFeatureConfig { * documentation. */ notice: string; + /** + * An optional list of feature IDs representing the features that should _conceptually_ replace this deprecated + * feature. This is used, for example, in the Spaces feature visibility toggles UI to display the replacement + * feature(s) instead of the deprecated one. By default, the list of replacement features is derived from the + * `replacedBy` fields of the feature privileges. However, if the feature privileges are replaced by the privileges + * of multiple features, this behavior is not always desired and can be overridden here. + */ + replacedBy?: readonly string[]; }>; } diff --git a/x-pack/platform/plugins/shared/features/server/feature_registry.test.ts b/x-pack/platform/plugins/shared/features/server/feature_registry.test.ts index 07f4a242b3176..1a2cecca3c244 100644 --- a/x-pack/platform/plugins/shared/features/server/feature_registry.test.ts +++ b/x-pack/platform/plugins/shared/features/server/feature_registry.test.ts @@ -2702,10 +2702,12 @@ describe('FeatureRegistry', () => { } function createDeprecatedFeature({ + deprecated, all, read, subAlpha, }: { + deprecated?: { notice: string; replacedBy?: string[] }; all?: FeatureKibanaPrivilegesReference[]; read?: { minimal: FeatureKibanaPrivilegesReference[]; @@ -2714,7 +2716,7 @@ describe('FeatureRegistry', () => { subAlpha?: FeatureKibanaPrivilegesReference[]; } = {}): KibanaFeatureConfig { return { - deprecated: { notice: 'It was a mistake.' }, + deprecated: deprecated ?? { notice: 'It was a mistake.' }, id: 'feature-alpha', name: 'Feature Alpha', app: [], @@ -3240,6 +3242,50 @@ describe('FeatureRegistry', () => { `"Cannot replace privilege \\"sub-alpha-1-1\\" of deprecated feature \\"feature-alpha\\" with disabled privilege \\"read\\" of feature \\"feature-delta\\"."` ); }); + + it('requires correct list of feature IDs to be replaced by', () => { + // Case 1: empty list of feature IDs. + expect(() => + createRegistry( + createDeprecatedFeature({ deprecated: { notice: 'some notice', replacedBy: [] } }) + ).validateFeatures() + ).toThrowErrorMatchingInlineSnapshot( + `"Feature “feature-alpha” is deprecated and must have at least one feature ID added to the “replacedBy” property, or the property must be left out completely."` + ); + + // Case 2: invalid feature IDs. + expect(() => + createRegistry( + createDeprecatedFeature({ + deprecated: { + notice: 'some notice', + replacedBy: ['feature-beta', 'feature-gamma', 'feature-delta'], + }, + }) + ).validateFeatures() + ).toThrowErrorMatchingInlineSnapshot( + `"Cannot replace deprecated feature “feature-alpha” with the following features, as they aren’t used to replace feature privileges: feature-gamma, feature-delta."` + ); + + // Case 3: valid feature ID. + expect(() => + createRegistry( + createDeprecatedFeature({ + deprecated: { notice: 'some notice', replacedBy: ['feature-beta'] }, + }) + ).validateFeatures() + ).not.toThrow(); + + // Case 4: valid multiple feature IDs. + expect(() => + createRegistry( + createDeprecatedFeature({ + deprecated: { notice: 'some notice', replacedBy: ['feature-beta', 'feature-delta'] }, + all: [{ feature: 'feature-delta', privileges: ['all'] }], + }) + ).validateFeatures() + ).not.toThrow(); + }); }); }); diff --git a/x-pack/platform/plugins/shared/features/server/feature_registry.ts b/x-pack/platform/plugins/shared/features/server/feature_registry.ts index cb4090dd38208..d801af9b97304 100644 --- a/x-pack/platform/plugins/shared/features/server/feature_registry.ts +++ b/x-pack/platform/plugins/shared/features/server/feature_registry.ts @@ -21,9 +21,9 @@ import { validateKibanaFeature, validateElasticsearchFeature } from './feature_s import type { ConfigOverridesType } from './config'; /** - * Describes parameters used to retrieve all Kibana features. + * Describes parameters used to retrieve all Kibana features (for internal consumers). */ -export interface GetKibanaFeaturesParams { +export interface GetKibanaFeaturesParamsInternal { /** * If provided, the license will be used to filter out features that require a license higher than the specified one. * */ @@ -41,6 +41,17 @@ export interface GetKibanaFeaturesParams { omitDeprecated?: boolean; } +/** + * Describes parameters used to retrieve all Kibana features (for public consumers). + */ +export interface GetKibanaFeaturesParams { + /** + * If true, deprecated features will be omitted. For backward compatibility reasons, deprecated features are included + * in the result by default. + */ + omitDeprecated: boolean; +} + export class FeatureRegistry { private locked = false; private kibanaFeatures: Record = {}; @@ -207,6 +218,7 @@ export class FeatureRegistry { // Iterate over all top-level and sub-feature privileges. const isFeatureDeprecated = !!feature.deprecated; + const replacementFeatureIds = new Set(); for (const [privilegeId, privilege] of [ ...Object.entries(feature.privileges), ...collectSubFeaturesPrivileges(feature), @@ -263,6 +275,32 @@ export class FeatureRegistry { ); } } + + replacementFeatureIds.add(featureReference.feature); + } + } + + const featureReplacedBy = feature.deprecated?.replacedBy; + if (featureReplacedBy) { + if (featureReplacedBy.length === 0) { + throw new Error( + `Feature “${feature.id}” is deprecated and must have at least one feature ID added to the “replacedBy” property, or the property must be left out completely.` + ); + } + + // The feature can be marked as replaced by another feature only if that feature is actually used to replace any + // of the deprecated feature’s privileges. + const invalidFeatureIds = featureReplacedBy.filter( + (featureId) => !replacementFeatureIds.has(featureId) + ); + if (invalidFeatureIds.length > 0) { + throw new Error( + `Cannot replace deprecated feature “${ + feature.id + }” with the following features, as they aren’t used to replace feature privileges: ${invalidFeatureIds.join( + ', ' + )}.` + ); } } } @@ -272,7 +310,7 @@ export class FeatureRegistry { license, ignoreLicense = false, omitDeprecated = false, - }: GetKibanaFeaturesParams = {}): KibanaFeature[] { + }: GetKibanaFeaturesParamsInternal = {}): KibanaFeature[] { if (!this.locked) { throw new Error('Cannot retrieve Kibana features while registration is still open'); } diff --git a/x-pack/platform/plugins/shared/features/server/feature_schema.ts b/x-pack/platform/plugins/shared/features/server/feature_schema.ts index 3923eb31dbecb..4766e597f2211 100644 --- a/x-pack/platform/plugins/shared/features/server/feature_schema.ts +++ b/x-pack/platform/plugins/shared/features/server/feature_schema.ts @@ -285,7 +285,12 @@ const kibanaFeatureSchema = schema.object({ ), }) ), - deprecated: schema.maybe(schema.object({ notice: schema.string() })), + deprecated: schema.maybe( + schema.object({ + notice: schema.string(), + replacedBy: schema.maybe(schema.arrayOf(schema.string())), + }) + ), }); const elasticsearchPrivilegeSchema = schema.object({ diff --git a/x-pack/platform/plugins/shared/features/server/index.ts b/x-pack/platform/plugins/shared/features/server/index.ts index 734a1aa256f73..5e4cf929a52d2 100644 --- a/x-pack/platform/plugins/shared/features/server/index.ts +++ b/x-pack/platform/plugins/shared/features/server/index.ts @@ -23,6 +23,7 @@ export type { } from '../common'; export type { SubFeaturePrivilegeIterator } from './feature_privilege_iterator'; export { KibanaFeature, ElasticsearchFeature } from '../common'; +export type { GetKibanaFeaturesParams } from './feature_registry'; export type { FeaturesPluginSetup, FeaturesPluginStart } from './plugin'; export const config: PluginConfigDescriptor> = { schema: ConfigSchema }; diff --git a/x-pack/platform/plugins/shared/features/server/plugin.ts b/x-pack/platform/plugins/shared/features/server/plugin.ts index 9f6cae36f6aee..ebb7881c579d3 100644 --- a/x-pack/platform/plugins/shared/features/server/plugin.ts +++ b/x-pack/platform/plugins/shared/features/server/plugin.ts @@ -17,7 +17,7 @@ import { Capabilities as UICapabilities, } from '@kbn/core/server'; import { ConfigType } from './config'; -import { FeatureRegistry } from './feature_registry'; +import { FeatureRegistry, GetKibanaFeaturesParams } from './feature_registry'; import { uiCapabilitiesForFeatures } from './ui_capabilities_for_features'; import { buildOSSFeatures } from './oss_features'; import { defineRoutes } from './routes'; @@ -84,7 +84,11 @@ export interface FeaturesPluginSetup { export interface FeaturesPluginStart { getElasticsearchFeatures(): ElasticsearchFeature[]; - getKibanaFeatures(): KibanaFeature[]; + /** + * Returns all registered Kibana features. + * @param params Optional parameters to filter features. + */ + getKibanaFeatures(params?: GetKibanaFeaturesParams): KibanaFeature[]; } /** @@ -147,7 +151,10 @@ export class FeaturesPlugin getElasticsearchFeatures: this.featureRegistry.getAllElasticsearchFeatures.bind( this.featureRegistry ), - getKibanaFeatures: this.featureRegistry.getAllKibanaFeatures.bind(this.featureRegistry), + getKibanaFeatures: (params) => + this.featureRegistry.getAllKibanaFeatures( + params && { omitDeprecated: params.omitDeprecated } + ), }); } diff --git a/x-pack/platform/plugins/shared/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts b/x-pack/platform/plugins/shared/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts index 9da144facf4f4..e3ff05f7ad29a 100644 --- a/x-pack/platform/plugins/shared/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts +++ b/x-pack/platform/plugins/shared/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts @@ -80,7 +80,7 @@ describe.skip('onPostAuthInterceptor', () => { const loggingMock = loggingSystemMock.create().asLoggerFactory().get('xpack', 'spaces'); - const featuresPlugin = featuresPluginMock.createSetup(); + const featuresPlugin = featuresPluginMock.createStart(); featuresPlugin.getKibanaFeatures.mockReturnValue([ { id: 'feature-1', @@ -163,7 +163,7 @@ describe.skip('onPostAuthInterceptor', () => { initSpacesOnPostAuthRequestInterceptor({ http: http as unknown as CoreSetup['http'], log: loggingMock, - features: featuresPlugin, + getFeatures: async () => featuresPlugin, getSpacesService: () => spacesServiceStart, }); diff --git a/x-pack/platform/plugins/shared/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts b/x-pack/platform/plugins/shared/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts index 67617185ad0f2..7910ebe1d5172 100644 --- a/x-pack/platform/plugins/shared/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts +++ b/x-pack/platform/plugins/shared/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts @@ -6,25 +6,25 @@ */ import type { CoreSetup, Logger } from '@kbn/core/server'; +import type { FeaturesPluginStart } from '@kbn/features-plugin/server'; import type { Space } from '../../../common'; import { addSpaceIdToPath } from '../../../common'; import { DEFAULT_SPACE_ID, ENTER_SPACE_PATH } from '../../../common/constants'; -import type { PluginsSetup } from '../../plugin'; -import type { SpacesServiceStart } from '../../spaces_service/spaces_service'; +import type { SpacesServiceStart } from '../../spaces_service'; import { wrapError } from '../errors'; import { getSpaceSelectorUrl } from '../get_space_selector_url'; import { withSpaceSolutionDisabledFeatures } from '../utils/space_solution_disabled_features'; export interface OnPostAuthInterceptorDeps { http: CoreSetup['http']; - features: PluginsSetup['features']; + getFeatures: () => Promise; getSpacesService: () => SpacesServiceStart; log: Logger; } export function initSpacesOnPostAuthRequestInterceptor({ - features, + getFeatures, getSpacesService, log, http, @@ -38,7 +38,7 @@ export function initSpacesOnPostAuthRequestInterceptor({ const spaceId = spacesService.getSpaceId(request); - // The root of kibana is also the root of the defaut space, + // The root of kibana is also the root of the default space, // since the default space does not have a URL Identifier (i.e., `/s/foo`). const isRequestingKibanaRoot = path === '/' && spaceId === DEFAULT_SPACE_ID; const isRequestingSpaceRoot = path === '/' && spaceId !== DEFAULT_SPACE_ID; @@ -106,7 +106,10 @@ export function initSpacesOnPostAuthRequestInterceptor({ } } - const allFeatures = features.getKibanaFeatures(); + // The Spaces client returns migrated feature IDs in `disabledFeatures`, so we need to omit + // deprecated features. Otherwise, apps granted by deprecated features will be considered + // available when they shouldn't be, since their IDs won't be present in `disabledFeatures`. + const allFeatures = (await getFeatures()).getKibanaFeatures({ omitDeprecated: true }); const disabledFeatureKeys = withSpaceSolutionDisabledFeatures( allFeatures, space.disabledFeatures, diff --git a/x-pack/platform/plugins/shared/spaces/server/plugin.ts b/x-pack/platform/plugins/shared/spaces/server/plugin.ts index e1c3c78976ecd..6f3e49688d5c0 100644 --- a/x-pack/platform/plugins/shared/spaces/server/plugin.ts +++ b/x-pack/platform/plugins/shared/spaces/server/plugin.ts @@ -191,7 +191,7 @@ export class SpacesPlugin http: core.http, log: this.log, getSpacesService, - features: plugins.features, + getFeatures: async () => (await core.getStartServices())[1].features, }); setupCapabilities(core, getSpacesService, this.log); diff --git a/x-pack/platform/plugins/shared/spaces/server/spaces_client/spaces_client.test.ts b/x-pack/platform/plugins/shared/spaces/server/spaces_client/spaces_client.test.ts index 4b7c1de0b3fcb..4d059dd49ad39 100644 --- a/x-pack/platform/plugins/shared/spaces/server/spaces_client/spaces_client.test.ts +++ b/x-pack/platform/plugins/shared/spaces/server/spaces_client/spaces_client.test.ts @@ -86,6 +86,37 @@ const features = [ }, }, }, + { + deprecated: { notice: 'It was another mistake.', replacedBy: ['feature_2'] }, + id: 'feature_5_deprecated', + name: 'Another 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(); @@ -135,7 +166,7 @@ describe('#getAll', () => { }, }, { - // alpha only has deprecated disabled features + // alpha has deprecated disabled features id: 'alpha', type: 'space', references: [], @@ -145,6 +176,17 @@ describe('#getAll', () => { disabledFeatures: ['feature_1', 'feature_4_deprecated'], }, }, + { + // beta has deprecated disabled features with specified `replacedBy` on feature level + id: 'beta', + type: 'space', + references: [], + attributes: { + name: 'beta-name', + description: 'beta-description', + disabledFeatures: ['feature_1', 'feature_5_deprecated'], + }, + }, ]; const expectedSpaces: Space[] = [ @@ -178,6 +220,12 @@ describe('#getAll', () => { description: 'alpha-description', disabledFeatures: ['feature_1', 'feature_2', 'feature_3'], }, + { + id: 'beta', + name: 'beta-name', + description: 'beta-description', + disabledFeatures: ['feature_1', 'feature_2'], + }, ]; test(`finds spaces using callWithRequestRepository`, async () => { diff --git a/x-pack/platform/plugins/shared/spaces/server/spaces_client/spaces_client.ts b/x-pack/platform/plugins/shared/spaces/server/spaces_client/spaces_client.ts index 66728636f9752..befa43e06a6b7 100644 --- a/x-pack/platform/plugins/shared/spaces/server/spaces_client/spaces_client.ts +++ b/x-pack/platform/plugins/shared/spaces/server/spaces_client/spaces_client.ts @@ -301,6 +301,14 @@ export class SpacesClient implements ISpacesClient { continue; } + // If the feature is deprecated and replacement features are explicitly defined, use them. + // Otherwise, use the replacement features defined in the feature privileges. + const featureReplacedBy = feature.deprecated?.replacedBy; + if (featureReplacedBy) { + deprecatedFeatureReferences.set(feature.id, new Set(featureReplacedBy)); + 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) => 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 9b7158aca7b32..938adc4606474 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 @@ -415,7 +415,10 @@ function case4FeatureExtract(deps: PluginSetupDependencies) { for (const suffix of ['A', 'B']) { // Step 1: mark existing feature A and feature B as deprecated. deps.features.registerKibanaFeature({ - deprecated: { notice: 'Case #4 is deprecated.' }, + deprecated: { + notice: 'Case #4 is deprecated.', + ...(suffix === 'B' ? { replacedBy: [`case_4_feature_${suffix.toLowerCase()}_v2`] } : {}), + }, scope: [KibanaFeatureScope.Security, KibanaFeatureScope.Spaces], 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 7887cd6a23dc0..35e502a58a343 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,7 +14,6 @@ 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'; @@ -188,28 +187,29 @@ export default function ({ getService }: FtrProviderContext) { `); }); - it('all deprecated features are replaced by a single feature only', async () => { + it('all deprecated features are replaced by a single feature only or explicitly specify replacement features', 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([ + // are only replaced by a single feature unless explicitly configured otherwise by the developer. This approach + // validates that if a deprecated feature was toggled off for a particular Space, there is no ambiguity about + // which replacement feature or features should also be toggled off. Currently, we don’t anticipate having many + // deprecated features replaced by more than one feature, so this test is designed 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. You may need to explicitly + // specify feature replacements in the feature configuration (`feature.deprecated.replacedBy`). + const featureIdsImplicitlyReplacedWithMultipleFeatures = 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) + (feature.deprecated?.replacedBy?.length ?? 0) > 0 || + featureIdsImplicitlyReplacedWithMultipleFeatures.has(feature.id) ) { continue; } @@ -236,7 +236,9 @@ export default function ({ getService }: FtrProviderContext) { if (referencedFeaturesIds.size > 1) { throw new Error( - `Feature "${feature.id}" is deprecated and replaced by more than one feature: ${ + `Feature "${ + feature.id + }" is deprecated and implicitly replaced by more than one feature: ${ referencedFeaturesIds.size } features: ${Array.from(referencedFeaturesIds).join( ', '