[7.x] [Feature Controls] - Secure Features API (#35841) (#36019)

* restrict access to Features API * introduce featureControls.manage capability to control calls to features api * add snapshots * rename manage_feature_controls api tag to features * Revert "introduce featureControls.manage capability to control calls to features api" This reverts commit addc149. * update spaces management to only call APIs if authorized * handle 404 response when requesting features on role management page * better variable naming * remove unnecessary mock * remove unused code * remove unnecessary snapshots
elastic · May 3, 2019 · f796084 · f796084
1 parent 61735cc
commit f796084
Show file tree

Hide file tree

Showing 15 changed files with 281 additions and 200 deletions.
diff --git a/x-pack/plugins/security/public/views/management/edit_role/index.js b/x-pack/plugins/security/public/views/management/edit_role/index.js
@@ -87,10 +87,17 @@ routes.when(`${EDIT_ROLES_PATH}/:name?`, {
       return [];
     },
     privileges() {
-      return  kfetch({ method: 'get', pathname: '/api/security/privileges', query: { includeActions: true } });
+      return kfetch({ method: 'get', pathname: '/api/security/privileges', query: { includeActions: true } });
     },
     features() {
-      return kfetch({ method: 'get', pathname: '/api/features/v1' });
+      return kfetch({ method: 'get', pathname: '/api/features/v1' }).catch(e => {
+        // TODO: This check can be removed once all of these `resolve` entries are moved out of Angular and into the React app.
+        const unauthorizedForFeatures = _.get(e, 'body.statusCode') === 404;
+        if (unauthorizedForFeatures) {
+          return [];
+        }
+        throw e;
+      });
     }
   },
   controllerAs: 'editRole',

diff --git a/x-pack/plugins/security/server/lib/authorization/privileges/privileges.test.ts b/x-pack/plugins/security/server/lib/authorization/privileges/privileges.test.ts
@@ -282,12 +282,14 @@ describe('features', () => {
   {
     group: 'global',
     expectManageSpaces: true,
+    expectGetFeatures: true,
   },
   {
     group: 'space',
     expectManageSpaces: false,
+    expectGetFeatures: false,
   },
-].forEach(({ group, expectManageSpaces }) => {
+].forEach(({ group, expectManageSpaces, expectGetFeatures }) => {
   describe(group, () => {
     test('actions defined only at the feature are included in `all` and `read`', () => {
       const features: Feature[] = [
@@ -331,6 +333,7 @@ describe('features', () => {
         all: [
           actions.login,
           actions.version,
+          ...(expectGetFeatures ? [actions.api.get('features')] : []),
           ...(expectManageSpaces ? [actions.space.manage, actions.ui.get('spaces', 'manage')] : []),
           actions.app.get('app-1'),
           actions.app.get('app-2'),
@@ -415,6 +418,7 @@ describe('features', () => {
       expect(actual).toHaveProperty(`${group}.all`, [
         actions.login,
         actions.version,
+        ...(expectGetFeatures ? [actions.api.get('features')] : []),
         ...(expectManageSpaces ? [actions.space.manage, actions.ui.get('spaces', 'manage')] : []),
         actions.ui.get('catalogue', 'bar-catalogue-1'),
         actions.ui.get('catalogue', 'bar-catalogue-2'),
@@ -745,6 +749,7 @@ describe('features', () => {
       expect(actual).toHaveProperty(`${group}.all`, [
         actions.login,
         actions.version,
+        ...(expectGetFeatures ? [actions.api.get('features')] : []),
         ...(expectManageSpaces ? [actions.space.manage, actions.ui.get('spaces', 'manage')] : []),
         actions.allHack,
       ]);

diff --git a/x-pack/plugins/security/server/lib/authorization/privileges/privileges.ts b/x-pack/plugins/security/server/lib/authorization/privileges/privileges.ts
@@ -62,6 +62,7 @@ export function privilegesFactory(actions: Actions, xpackMainPlugin: XPackMainPl
           all: [
             actions.login,
             actions.version,
+            actions.api.get('features'),
             actions.space.manage,
             actions.ui.get('spaces', 'manage'),
             ...allActions,

diff --git a/x-pack/plugins/spaces/public/lib/spaces_manager.ts b/x-pack/plugins/spaces/public/lib/spaces_manager.ts
@@ -29,7 +29,9 @@ export class SpacesManager extends EventEmitter {
   }
 
   public async getSpace(id: string): Promise<Space> {
-    return await this.httpAgent.get(`${this.baseUrl}/space/${id}`);
+    return await this.httpAgent
+      .get(`${this.baseUrl}/space/${id}`)
+      .then((response: IHttpResponse<Space[]>) => response.data);
   }
 
   public async createSpace(space: Space) {

diff --git a/...ugins/spaces/public/views/management/edit_space/customize_space/space_identifier.test.tsx b/...ugins/spaces/public/views/management/edit_space/customize_space/space_identifier.test.tsx
@@ -17,7 +17,7 @@ test('renders without crashing', () => {
     },
     editable: true,
     onChange: jest.fn(),
-    validator: new SpaceValidator({ features: [] }),
+    validator: new SpaceValidator(),
   };
   const wrapper = shallowWithIntl(
     <SpaceIdentifier.WrappedComponent {...props} intl={null as any} />

diff --git a/x-pack/plugins/spaces/public/views/management/edit_space/manage_space_page.test.tsx b/x-pack/plugins/spaces/public/views/management/edit_space/manage_space_page.test.tsx
@@ -3,7 +3,9 @@
  * or more contributor license agreements. Licensed under the Elastic License;
  * you may not use this file except in compliance with the Elastic License.
  */
-
+jest.mock('ui/kfetch', () => ({
+  kfetch: () => Promise.resolve([{ id: 'foo', name: 'foo', app: [], privileges: {} }]),
+}));
 import { EuiButton, EuiLink, EuiSwitch } from '@elastic/eui';
 import { ReactWrapper } from 'enzyme';
 import React from 'react';
@@ -19,11 +21,9 @@ const space = {
   name: 'My Space',
   disabledFeatures: [],
 };
-const buildMockChrome = () => {
-  return {
-    addBasePath: (path: string) => path,
-  };
-};
+const buildMockChrome = () => ({
+  addBasePath: (path: string) => path,
+});
 
 describe('ManageSpacePage', () => {
   it('allows a space to be created', async () => {
@@ -44,10 +44,12 @@ describe('ManageSpacePage', () => {
       <ManageSpacePage.WrappedComponent
         spacesManager={spacesManager}
         spacesNavState={spacesNavState}
-        features={[{ id: 'foo', name: 'foo', app: [], privileges: {} }]}
         intl={null as any}
       />
     );
+
+    await waitForDataLoad(wrapper);
+
     const nameInput = wrapper.find('input[name="name"]');
     const descriptionInput = wrapper.find('textarea[name="description"]');
 
@@ -70,8 +72,8 @@ describe('ManageSpacePage', () => {
 
   it('allows a space to be updated', async () => {
     const mockHttp = {
-      get: jest.fn(async () => {
-        return Promise.resolve({
+      get: jest.fn(async () =>
+        Promise.resolve({
           data: {
             id: 'existing-space',
             name: 'Existing Space',
@@ -80,8 +82,8 @@ describe('ManageSpacePage', () => {
             initials: 'AB',
             disabledFeatures: [],
           },
-        });
-      }),
+        })
+      ),
       delete: jest.fn(() => Promise.resolve()),
     };
     const mockChrome = buildMockChrome();
@@ -99,12 +101,11 @@ describe('ManageSpacePage', () => {
         spaceId={'existing-space'}
         spacesManager={spacesManager}
         spacesNavState={spacesNavState}
-        features={[{ id: 'foo', name: 'foo', app: [], privileges: {} }]}
         intl={null as any}
       />
     );
 
-    await Promise.resolve();
+    await waitForDataLoad(wrapper);
 
     expect(mockHttp.get).toHaveBeenCalledWith('/api/spaces/space/existing-space');
 
@@ -128,8 +129,8 @@ describe('ManageSpacePage', () => {
 
   it('warns when updating features in the active space', async () => {
     const mockHttp = {
-      get: jest.fn(async () => {
-        return Promise.resolve({
+      get: jest.fn(async () =>
+        Promise.resolve({
           data: {
             id: 'my-space',
             name: 'Existing Space',
@@ -138,8 +139,8 @@ describe('ManageSpacePage', () => {
             initials: 'AB',
             disabledFeatures: [],
           },
-        });
-      }),
+        })
+      ),
       delete: jest.fn(() => Promise.resolve()),
     };
     const mockChrome = buildMockChrome();
@@ -157,12 +158,11 @@ describe('ManageSpacePage', () => {
         spaceId={'my-space'}
         spacesManager={spacesManager}
         spacesNavState={spacesNavState}
-        features={[{ id: 'foo', name: 'foo', app: [], privileges: {} }]}
         intl={null as any}
       />
     );
 
-    await Promise.resolve();
+    await waitForDataLoad(wrapper);
 
     expect(mockHttp.get).toHaveBeenCalledWith('/api/spaces/space/my-space');
 
@@ -195,8 +195,8 @@ describe('ManageSpacePage', () => {
 
   it('does not warn when features are left alone in the active space', async () => {
     const mockHttp = {
-      get: jest.fn(async () => {
-        return Promise.resolve({
+      get: jest.fn(async () =>
+        Promise.resolve({
           data: {
             id: 'my-space',
             name: 'Existing Space',
@@ -205,8 +205,8 @@ describe('ManageSpacePage', () => {
             initials: 'AB',
             disabledFeatures: [],
           },
-        });
-      }),
+        })
+      ),
       delete: jest.fn(() => Promise.resolve()),
     };
     const mockChrome = buildMockChrome();
@@ -224,12 +224,11 @@ describe('ManageSpacePage', () => {
         spaceId={'my-space'}
         spacesManager={spacesManager}
         spacesNavState={spacesNavState}
-        features={[{ id: 'foo', name: 'foo', app: [], privileges: {} }]}
         intl={null as any}
       />
     );
 
-    await Promise.resolve();
+    await waitForDataLoad(wrapper);
 
     expect(mockHttp.get).toHaveBeenCalledWith('/api/spaces/space/my-space');
 
@@ -286,3 +285,9 @@ async function clickSaveButton(wrapper: ReactWrapper<any, any>) {
 
   wrapper.update();
 }
+
+async function waitForDataLoad(wrapper: ReactWrapper<any, any>) {
+  await Promise.resolve();
+  await Promise.resolve();
+  wrapper.update();
+}