feat(security): extend Feature definition to support explicit featu…

…re replacements (elastic#206660)

## Summary

Today, when a developer deprecates a feature and replaces its privileges
with those of another feature, we reasonably assume that the new feature
fully replaces the old one in all possible contexts - whether in role
management UIs or in the Spaces feature toggles visibility UI. However,
when deprecated privileges are replaced by the privileges of multiple
features, such as in [this
case](elastic#202863 (comment))
where the Discover/Dashboard/Maps feature privileges are replaced by the
privileges of Discover_v2/Dashboard_v2/Maps_v2, respectively, **and**
the privileges of the Saved Query Management feature, the choice is
ambiguous.

Which of these features should be treated as the replacement for the
deprecated feature in contexts that deal with entire features (like the
Spaces feature toggles visibility UI) rather than individual privileges
(like in role management UIs)? Should all referenced features be
considered replacements? Or just a subset - or even a single feature? If
so, which one? Currently, we treat all referenced features as
replacements for the deprecated feature, which creates problems, as
described in detail in [this
discussion](elastic#202863 (comment)).

This PR allows developers to customize this behavior by specifying which
features Kibana should treat as direct successors to deprecated features
in contexts that deal with whole features rather than individual
privileges:

```ts
deps.features.registerKibanaFeature({
  deprecated: {
    notice: 'The feature is deprecated because … well, there’s a reason.',
    --> replacedBy: ['feature_id_v2'], <--
  },
  id: 'feature_id'
  name: `Case #4 feature ${suffix} (DEPRECATED)`,
  …
});
```

## How to test

1. Run test server
```bash
node scripts/functional_tests_server.js --config x-pack/test/security_api_integration/features.config.ts
```

2. Execute the following request from the Dev Tools (`case_4_feature_a`
is a deprecated feature that is replaced by multiple features and
**doesn't use** `deprecated.replacedBy`)
```http
PUT kbn:/api/spaces/space/default?overwrite=true
{
  "id":"default",
  "name":"Default",
  "description":"This is your default space!",
  "color":"#00bfb3",
  "disabledFeatures":["case_4_feature_a"],
  "_reserved":true,
  "imageUrl":"",
  "initials":"D"
}
```

3. Observe that in response deprecated `case_4_feature_a` is replaced by
two features (you can also check
http://localhost:5620/app/management/kibana/spaces/edit/default to see
how it's reflected in UI)
```http
{
  "id": "default",
  "name": "Default",
  "description": "This is your default space!",
  "color": "#00bfb3",
  "initials": "D",
  "imageUrl": "",
  "disabledFeatures": [
    "case_4_feature_a_v2",
    "case_4_feature_c"
  ],
  "_reserved": true
}
```

4. Execute the following request from the Dev Tools (`case_4_feature_b`
is a deprecated feature that is replaced by multiple features, but
**uses** `deprecated.replacedBy` to set the conceptual
feature-successor)
```http
PUT kbn:/api/spaces/space/default?overwrite=true
{
  "id":"default",
  "name":"Default",
  "description":"This is your default space!",
  "color":"#00bfb3",
  "disabledFeatures":["case_4_feature_b"],
  "_reserved":true,
  "imageUrl":"",
  "initials":"D"
}
```

5. Observe that in response deprecated `case_4_feature_b` is replaced by
a single feature (you can also check
http://localhost:5620/app/management/kibana/spaces/edit/default to see
how it's reflected in UI)
```http
{
  "id": "default",
  "name": "Default",
  "description": "This is your default space!",
  "color": "#00bfb3",
  "initials": "D",
  "imageUrl": "",
  "disabledFeatures": [
    "case_4_feature_b_v2"
  ],
  "_reserved": true
}
```

__Required by:__
elastic#202863 (comment)

//cc @davismcphee

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[];
   }>;
 }
 

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();
+      });
     });
   });
 

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<string, KibanaFeatureConfig> = {};
@@ -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');
     }

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({

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<TypeOf<typeof ConfigSchema>> = { schema: ConfigSchema };

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 }
+        ),
     });
   }
 

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,
     });
 

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<FeaturesPluginStart>;
   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,

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);

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 () => {

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) =>