fix(security, features): do not expose UI capabilities of the deprecated features (elastic#198656)

azasypkin · elasticmachine · web-flow · commit deeb9fe32af7 · 2024-11-06T15:06:39.000+01:00
## 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 <elasticmachine@users.noreply.github.com>
diff --git a/x-pack/plugins/features/server/mocks.ts b/x-pack/plugins/features/server/mocks.ts
@@ -25,8 +25,8 @@ const createSetup = (): jest.Mocked<FeaturesPluginSetup> => {
 
 const createStart = (): jest.Mocked<FeaturesPluginStart> => {
   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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -39,6 +39,7 @@ const enabledFeaturesPerSolution: Record<SolutionId, string[]> = {
  * 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
@@ -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
@@ -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
@@ -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
@@ -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<string, Set<string>>;
 
   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<v1.GetSpaceResult[]> {
@@ -247,6 +257,8 @@ export class SpacesClient implements ISpacesClient {
   };
 
   private transformSavedObjectToSpace = (savedObject: SavedObject<any>): 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
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
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
diff --git a/x-pack/test/ui_capabilities/common/config.ts b/x-pack/test/ui_capabilities/common/config.ts

Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,7 @@ function toggleDisabledFeatures(`
`72`	`72`	`(acc, feature) => {`
`73`	`73`	`if (disabledFeatureKeys.includes(feature.id)) {`
`74`	`74`	`acc.disabledFeatures.push(feature);`
`75`		`- } else {`
	`75`	`+ } else if (!feature.deprecated) {`
`76`	`76`	`acc.enabledFeatures.push(feature);`
`77`	`77`	`}`
`78`	`78`	`return acc;`
Original file line number	Diff line number	Diff line change
`@@ -39,6 +39,7 @@ const enabledFeaturesPerSolution: Record<SolutionId, string[]> = {`
`39`	`39`	`* This function takes the current space's disabled features and the space solution and returns`
`40`	`40`	`* the updated array of disabled features.`
`41`	`41`	`*`
	`42`	`+ * @param features The list of all Kibana registered features.`
`42`	`43`	`* @param spaceDisabledFeatures The current space's disabled features`
`43`	`44`	`* @param spaceSolution The current space's solution (es, oblt, security or classic)`
`44`	`45`	`* @returns The updated array of disabled features`