address platform review feedback

x-pack/plugins/features/common/index.ts

@@ -5,5 +5,11 @@
  */
 
 export { FeatureKibanaPrivileges } from './feature_kibana_privileges';
-export * from './feature';
-export * from './sub_feature';
+export { Feature, FeatureConfig } from './feature';
+export {
+  SubFeature,
+  SubFeatureConfig,
+  SubFeaturePrivilegeConfig,
+  SubFeaturePrivilegeGroupConfig,
+  SubFeaturePrivilegeGroupType,
+} from './sub_feature';

x-pack/plugins/features/common/sub_feature.ts

@@ -17,6 +17,17 @@ export interface SubFeatureConfig {
   /** Collection of privilege groups */
   privilegeGroups: SubFeaturePrivilegeGroupConfig[];
 }
+
+/**
+ * The type of privilege group.
+ * - `mutually_exclusive`::
+ *     Users will be able to select at most one privilege within this group.
+ *     Privileges must be specified in descending order of permissiveness (e.g. `All`, `Read`, not `Read`, `All)
+ * - `independent`::
+ *     Users will be able to select any combination of privileges within this group.
+ */
+export type SubFeaturePrivilegeGroupType = 'mutually_exclusive' | 'independent';
+
 /**
  * Configuration for a sub-feature privilege group.
  */
@@ -29,7 +40,7 @@ export interface SubFeaturePrivilegeGroupConfig {
    * - `independent`::
    *     Users will be able to select any combination of privileges within this group.
    */
-  groupType: 'mutually_exclusive' | 'independent';
+  groupType: SubFeaturePrivilegeGroupType;
 
   /**
    * The privileges which belong to this group.

x-pack/plugins/features/public/features_api_client.ts

@@ -4,11 +4,11 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { CoreSetup } from 'src/core/public';
+import { HttpSetup } from 'src/core/public';
 import { FeatureConfig, Feature } from '.';
 
 export class FeaturesAPIClient {
-  constructor(private readonly http: CoreSetup['http']) {}
+  constructor(private readonly http: HttpSetup) {}
 
   public async getFeatures() {
     const features = await this.http.get<FeatureConfig[]>('/api/features');

x-pack/plugins/features/public/mocks.ts

@@ -4,13 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { FeaturesPluginSetup, FeaturesPluginStart } from './plugin';
-
-const createSetup = (): jest.Mocked<FeaturesPluginSetup> => {
-  return {
-    getFeatures: jest.fn(),
-  };
-};
+import { FeaturesPluginStart } from './plugin';
 
 const createStart = (): jest.Mocked<FeaturesPluginStart> => {
   return {
@@ -19,6 +13,5 @@ const createStart = (): jest.Mocked<FeaturesPluginStart> => {
 };
 
 export const featuresPluginMock = {
-  createSetup,
   createStart,
 };

x-pack/plugins/features/public/plugin.test.ts

@@ -6,17 +6,24 @@
 
 import { FeaturesPlugin } from './plugin';
 
-import { coreMock } from 'src/core/public/mocks';
+import { coreMock, httpServiceMock } from 'src/core/public/mocks';
+
+jest.mock('./features_api_client', () => {
+  const instance = {
+    getFeatures: jest.fn(),
+  };
+  return {
+    FeaturesAPIClient: jest.fn().mockImplementation(() => instance),
+  };
+});
+
+import { FeaturesAPIClient } from './features_api_client';
 
 describe('Features Plugin', () => {
   describe('#setup', () => {
     it('returns expected public contract', () => {
       const plugin = new FeaturesPlugin();
-      expect(plugin.setup(coreMock.createSetup())).toMatchInlineSnapshot(`
-        Object {
-          "getFeatures": [Function],
-        }
-      `);
+      expect(plugin.setup(coreMock.createSetup())).toMatchInlineSnapshot(`undefined`);
     });
   });
 
@@ -31,5 +38,16 @@ describe('Features Plugin', () => {
         }
       `);
     });
+
+    it('#getFeatures calls the underlying FeaturesAPIClient', () => {
+      const plugin = new FeaturesPlugin();
+      const apiClient = new FeaturesAPIClient(httpServiceMock.createSetupContract());
+
+      plugin.setup(coreMock.createSetup());
+
+      const start = plugin.start();
+      start.getFeatures();
+      expect(apiClient.getFeatures).toHaveBeenCalledTimes(1);
+    });
   });
 });

x-pack/plugins/features/public/plugin.ts

@@ -8,18 +8,15 @@ import { Plugin, CoreSetup } from 'src/core/public';
 import { FeaturesAPIClient } from './features_api_client';
 
 export class FeaturesPlugin implements Plugin<FeaturesPluginSetup, FeaturesPluginStart> {
-  private apiClient!: FeaturesAPIClient;
+  private apiClient?: FeaturesAPIClient;
 
   public setup(core: CoreSetup) {
     this.apiClient = new FeaturesAPIClient(core.http);
-    return {
-      getFeatures: () => this.apiClient.getFeatures(),
-    };
   }
 
   public start() {
     return {
-      getFeatures: () => this.apiClient.getFeatures(),
+      getFeatures: () => this.apiClient!.getFeatures(),
     };
   }
 

x-pack/plugins/features/server/feature_registry.ts

@@ -25,9 +25,9 @@ export class FeatureRegistry {
       throw new Error(`Feature with id ${feature.id} is already registered.`);
     }
 
-    const featureCopy: FeatureConfig = cloneDeep(feature as FeatureConfig);
+    const featureCopy = cloneDeep(feature);
 
-    this.features[feature.id] = applyAutomaticPrivilegeGrants(featureCopy as FeatureConfig);
+    this.features[feature.id] = applyAutomaticPrivilegeGrants(featureCopy);
   }
 
   public getAll(): Feature[] {

x-pack/plugins/features/server/feature_schema.ts

@@ -71,12 +71,15 @@ const subFeaturePrivilegeSchema = Joi.object({
 });
 
 const subFeatureSchema = Joi.object({
-  name: Joi.string(),
+  name: Joi.string().required(),
   privilegeGroups: Joi.array().items(
     Joi.object({
-      name: Joi.string(),
-      groupType: Joi.string(),
-      privileges: Joi.array().items(subFeaturePrivilegeSchema),
+      groupType: Joi.string()
+        .valid('mutually_exclusive', 'independent')
+        .required(),
+      privileges: Joi.array()
+        .items(subFeaturePrivilegeSchema)
+        .min(1),
     })
   ),
 });

x-pack/plugins/features/server/ui_capabilities_for_features.test.ts

@@ -5,19 +5,17 @@
  */
 
 import { uiCapabilitiesForFeatures } from './ui_capabilities_for_features';
-import { Feature, FeatureConfig } from '.';
+import { Feature } from '.';
 import { SubFeaturePrivilegeGroupConfig } from '../common';
 
-function createFeaturePrivilege(key: string, capabilities: string[] = []) {
+function createFeaturePrivilege(capabilities: string[] = []) {
   return {
-    [key]: {
-      savedObject: {
-        all: [],
-        read: [],
-      },
-      app: [],
-      ui: [...capabilities],
+    savedObject: {
+      all: [],
+      read: [],
     },
+    app: [],
+    ui: [...capabilities],
   };
 }
 
@@ -47,9 +45,10 @@ describe('populateUICapabilities', () => {
           id: 'newFeature',
           name: 'my new feature',
           app: ['bar-app'],
-          privileges: ({
-            ...createFeaturePrivilege('all'),
-          } as unknown) as FeatureConfig['privileges'],
+          privileges: {
+            all: createFeaturePrivilege(),
+            read: createFeaturePrivilege(),
+          },
         }),
       ])
     ).toEqual({
@@ -66,9 +65,10 @@ describe('populateUICapabilities', () => {
           name: 'my new feature',
           navLinkId: 'newFeatureNavLink',
           app: ['bar-app'],
-          privileges: ({
-            ...createFeaturePrivilege('all', ['capability1', 'capability2']),
-          } as unknown) as FeatureConfig['privileges'],
+          privileges: {
+            all: createFeaturePrivilege(['capability1', 'capability2']),
+            read: createFeaturePrivilege(),
+          },
         }),
       ])
     ).toEqual({
@@ -89,11 +89,10 @@ describe('populateUICapabilities', () => {
           navLinkId: 'newFeatureNavLink',
           app: ['bar-app'],
           catalogue: ['anotherFooEntry', 'anotherBarEntry'],
-          privileges: ({
-            ...createFeaturePrivilege('foo', ['capability1', 'capability2']),
-            ...createFeaturePrivilege('bar', ['capability3', 'capability4']),
-            ...createFeaturePrivilege('baz'),
-          } as unknown) as FeatureConfig['privileges'],
+          privileges: {
+            all: createFeaturePrivilege(['capability1', 'capability2']),
+            read: createFeaturePrivilege(['capability3', 'capability4']),
+          },
         }),
       ])
     ).toEqual({
@@ -118,11 +117,10 @@ describe('populateUICapabilities', () => {
           name: 'my new feature',
           navLinkId: 'newFeatureNavLink',
           app: ['bar-app'],
-          privileges: ({
-            ...createFeaturePrivilege('foo', ['capability1', 'capability2']),
-            ...createFeaturePrivilege('bar', ['capability3', 'capability4']),
-            ...createFeaturePrivilege('baz', ['capability1', 'capability5']),
-          } as unknown) as FeatureConfig['privileges'],
+          privileges: {
+            all: createFeaturePrivilege(['capability1', 'capability2']),
+            read: createFeaturePrivilege(['capability3', 'capability4', 'capability5']),
+          },
         }),
       ])
     ).toEqual({
@@ -145,10 +143,10 @@ describe('populateUICapabilities', () => {
           name: 'my new feature',
           navLinkId: 'newFeatureNavLink',
           app: ['bar-app'],
-          privileges: ({
-            ...createFeaturePrivilege('foo', ['capability1', 'capability2']),
-            ...createFeaturePrivilege('bar', ['capability3', 'capability4']),
-          } as unknown) as FeatureConfig['privileges'],
+          privileges: {
+            all: createFeaturePrivilege(['capability1', 'capability2']),
+            read: createFeaturePrivilege(['capability3', 'capability4']),
+          },
           subFeatures: [
             {
               name: 'sub-feature-1',
@@ -209,35 +207,29 @@ describe('populateUICapabilities', () => {
           name: 'my new feature',
           navLinkId: 'newFeatureNavLink',
           app: ['bar-app'],
-          privileges: ({
-            ...createFeaturePrivilege('foo', ['capability1', 'capability2']),
-            ...createFeaturePrivilege('bar', ['capability3', 'capability4']),
-            ...createFeaturePrivilege('baz', ['capability1', 'capability5']),
-          } as unknown) as FeatureConfig['privileges'],
+          privileges: {
+            all: createFeaturePrivilege(['capability1', 'capability2']),
+            read: createFeaturePrivilege(['capability3', 'capability4']),
+          },
         }),
         new Feature({
           id: 'anotherNewFeature',
           name: 'another new feature',
           app: ['bar-app'],
-          privileges: ({
-            ...createFeaturePrivilege('foo', ['capability1', 'capability2']),
-            ...createFeaturePrivilege('bar', ['capability3', 'capability4']),
-          } as unknown) as FeatureConfig['privileges'],
+          privileges: {
+            all: createFeaturePrivilege(['capability1', 'capability2']),
+            read: createFeaturePrivilege(['capability3', 'capability4']),
+          },
         }),
         new Feature({
           id: 'yetAnotherNewFeature',
           name: 'yet another new feature',
           navLinkId: 'yetAnotherNavLink',
           app: ['bar-app'],
-          privileges: ({
-            ...createFeaturePrivilege('all', ['capability1', 'capability2']),
-            ...createFeaturePrivilege('read', []),
-            ...createFeaturePrivilege('somethingInBetween', [
-              'something1',
-              'something2',
-              'something3',
-            ]),
-          } as unknown) as FeatureConfig['privileges'],
+          privileges: {
+            all: createFeaturePrivilege(['capability1', 'capability2']),
+            read: createFeaturePrivilege(['something1', 'something2', 'something3']),
+          },
           subFeatures: [
             {
               name: 'sub-feature-1',
@@ -267,7 +259,6 @@ describe('populateUICapabilities', () => {
         capability2: true,
         capability3: true,
         capability4: true,
-        capability5: true,
       },
       yetAnotherNewFeature: {
         capability1: true,