Fix RBAC Phase 1 merge from master (#20226)

This updates RBAC Phase 1 to work against the latest master. Specifically: 1. Removes `xpack_main`'s `registerLicenseChangeCallback`, which we introduced in `security-app-privs`, in favor of `onLicenseInfoChange`, which was recently added to master 2. Updated `x-pack/plugins/security/server/lib/watch_status_and_license_to_initialize.js` to be compliant with rxjs v6
elastic · Jun 26, 2018 · f875cec · f875cec
1 parent e11d86b
commit f875cec
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 117 deletions.
diff --git a/x-pack/plugins/security/server/lib/watch_status_and_license_to_initialize.js b/x-pack/plugins/security/server/lib/watch_status_and_license_to_initialize.js
@@ -3,53 +3,55 @@
  * or more contributor license agreements. Licensed under the Elastic License;
  * you may not use this file except in compliance with the Elastic License.
  */
-import { Observable } from 'rxjs';
+import * as Rx from 'rxjs';
+import { map, switchMap, tap } from 'rxjs/operators';
 
 export function watchStatusAndLicenseToInitialize(xpackMainPlugin, downstreamPlugin, initialize) {
   const xpackInfo = xpackMainPlugin.info;
   const xpackInfoFeature = xpackInfo.feature(downstreamPlugin.id);
 
   const upstreamStatus = xpackMainPlugin.status;
-  const currentStatus$ = Observable
+  const currentStatus$ = Rx
     .of({
       state: upstreamStatus.state,
       message: upstreamStatus.message,
     });
-  const newStatus$ = Observable
+  const newStatus$ = Rx
     .fromEvent(upstreamStatus, 'change', null, (previousState, previousMsg, state, message) => {
       return {
         state,
         message,
       };
     });
-  const status$ = Observable.merge(currentStatus$, newStatus$);
+  const status$ = Rx.merge(currentStatus$, newStatus$);
 
-  const currentLicense$ = Observable
-    .of(xpackInfoFeature.getLicenseCheckResults());
-  const newLicense$ = Observable
-    .fromEventPattern(xpackInfoFeature.registerLicenseChangeCallback)
-    .map(() => xpackInfoFeature.getLicenseCheckResults());
-  const license$ = Observable.merge(currentLicense$, newLicense$);
+  const currentLicense$ = Rx.of(xpackInfoFeature.getLicenseCheckResults());
+  const newLicense$ = Rx
+    .fromEventPattern(xpackInfo.onLicenseInfoChange.bind(xpackInfo))
+    .pipe(map(() => xpackInfoFeature.getLicenseCheckResults()));
+  const license$ = Rx.merge(currentLicense$, newLicense$);
 
-  Observable.combineLatest(status$, license$)
-    .map(([status, license]) => ({ status, license }))
-    .switchMap(({ status, license }) => {
-      if (status.state !== 'green') {
-        return Observable.of({ state: status.state, message: status.message });
-      }
+  Rx.combineLatest(status$, license$)
+    .pipe(
+      map(([status, license]) => ({ status, license })),
+      switchMap(({ status, license }) => {
+        if (status.state !== 'green') {
+          return Rx.of({ state: status.state, message: status.message });
+        }
 
-      return initialize(license)
-        .then(() => ({
-          state: 'green',
-          message: 'Ready',
-        }))
-        .catch((err) => ({
-          state: 'red',
-          message: err.message
-        }));
-    })
-    .do(({ state, message }) => {
-      downstreamPlugin.status[state](message);
-    })
+        return initialize(license)
+          .then(() => ({
+            state: 'green',
+            message: 'Ready',
+          }))
+          .catch((err) => ({
+            state: 'red',
+            message: err.message
+          }));
+      }),
+      tap(({ state, message }) => {
+        downstreamPlugin.status[state](message);
+      })
+    )
     .subscribe();
 }
diff --git a/x-pack/plugins/security/server/lib/watch_status_and_license_to_initialize.test.js b/x-pack/plugins/security/server/lib/watch_status_and_license_to_initialize.test.js
@@ -12,9 +12,6 @@ const createMockXpackMainPluginAndFeature = (featureId) => {
 
   const mockFeature = {
     getLicenseCheckResults: jest.fn(),
-    registerLicenseChangeCallback: (callback) => {
-      licenseChangeCallbacks.push(callback);
-    },
     mock: {
       triggerLicenseChange: () => {
         for (const callback of licenseChangeCallbacks) {
@@ -29,6 +26,9 @@ const createMockXpackMainPluginAndFeature = (featureId) => {
 
   const mockXpackMainPlugin = {
     info: {
+      onLicenseInfoChange: (callback) => {
+        licenseChangeCallbacks.push(callback);
+      },
       feature: (id) => {
         if (id === featureId) {
           return mockFeature;

diff --git a/x-pack/plugins/xpack_main/server/lib/__tests__/xpack_info.js b/x-pack/plugins/xpack_main/server/lib/__tests__/xpack_info.js
@@ -462,72 +462,6 @@ describe('XPackInfo', () => {
       });
     });
 
-    it('registerLicenseChangeCallback() does not invoke callbacks if license has not changed', async () => {
-      const securityFeature = xPackInfo.feature('security');
-      const watcherFeature = xPackInfo.feature('watcher');
-
-      const securityChangeCallback = sinon.stub();
-      securityFeature.registerLicenseChangeCallback(securityChangeCallback);
-
-      const watcherChangeCallback = sinon.stub();
-      watcherFeature.registerLicenseChangeCallback(watcherChangeCallback);
-
-      mockElasticsearchCluster.callWithInternalUser.returns(
-        getMockXPackInfoAPIResponse({ mode: 'gold' })
-      );
-
-      await xPackInfo.refreshNow();
-
-      sinon.assert.notCalled(securityChangeCallback);
-      sinon.assert.notCalled(watcherChangeCallback);
-    });
-
-    it('registerLicenseChangeCallback() invokes callbacks on license change', async () => {
-      const securityFeature = xPackInfo.feature('security');
-      const watcherFeature = xPackInfo.feature('watcher');
-
-      const securityChangeCallback = sinon.stub();
-      securityFeature.registerLicenseChangeCallback(securityChangeCallback);
-
-      const watcherChangeCallback = sinon.stub();
-      watcherFeature.registerLicenseChangeCallback(watcherChangeCallback);
-
-      mockElasticsearchCluster.callWithInternalUser.returns(
-        getMockXPackInfoAPIResponse({ mode: 'platinum' })
-      );
-
-      await xPackInfo.refreshNow();
-
-      sinon.assert.calledOnce(securityChangeCallback);
-      sinon.assert.calledOnce(watcherChangeCallback);
-    });
-
-    it('registerLicenseChangeCallback() gracefully handles callbacks that throw errors', async () => {
-      const securityFeature = xPackInfo.feature('security');
-      const watcherFeature = xPackInfo.feature('watcher');
-
-      const securityChangeCallback = sinon.stub().throws(new Error(`Something happened`));
-      securityFeature.registerLicenseChangeCallback(securityChangeCallback);
-
-      const watcherChangeCallback = sinon.stub();
-      watcherFeature.registerLicenseChangeCallback(watcherChangeCallback);
-
-      mockElasticsearchCluster.callWithInternalUser.returns(
-        getMockXPackInfoAPIResponse({ mode: 'platinum' })
-      );
-
-      await xPackInfo.refreshNow();
-
-      sinon.assert.calledOnce(securityChangeCallback);
-      sinon.assert.calledOnce(watcherChangeCallback);
-
-      sinon.assert.calledWithExactly(
-        mockServer.log,
-        ['license', 'error', 'xpack'],
-        `Error during invocation of license change callback for security. Error: Something happened`
-      );
-    });
-
     it('getLicenseCheckResults() correctly returns feature specific info.', async () => {
       const securityFeature = xPackInfo.feature('security');
       const watcherFeature = xPackInfo.feature('watcher');

diff --git a/x-pack/plugins/xpack_main/server/lib/xpack_info.js b/x-pack/plugins/xpack_main/server/lib/xpack_info.js
@@ -36,12 +36,6 @@ export class XPackInfo {
    */
   _licenseInfoChangedListeners = new Set();
 
-  /**
-   * Feature name <-> license change callback mapping.
-   * @type {Map<string, Function>}
-   * @private
-   */
-  _featureLicenseChangeCallbacks = new Map();
 
   /**
    * Cache that may contain last xpack info API response or error, json representation
@@ -224,16 +218,6 @@ export class XPackInfo {
         this._cache.signature = undefined;
       },
 
-      /**
-       * Registers a callback function that will be called whenever the XPack license changes.
-       * Callback will be invoked after the license change have been applied to this XPack Info instance.
-       * Callbacks may be asynchronous, but will not be awaited.
-       * @param {Function} callback Function to call whenever the XPack license changes.
-       */
-      registerLicenseChangeCallback: (callback) => {
-        this._featureLicenseChangeCallbacks.set(name, callback);
-      },
-
       /**
        * Returns license check results that were previously produced by the `generator` function.
        * @returns {Object}

diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/delete.js b/x-pack/test/rbac_api_integration/apis/saved_objects/delete.js
@@ -21,7 +21,7 @@ export default function ({ getService }) {
       expect(resp.body).to.eql({
         statusCode: 404,
         error: 'Not Found',
-        message: 'Not Found'
+        message: 'Saved object [dashboard/not-a-real-id] not found'
       });
     };
 

diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/get.js b/x-pack/test/rbac_api_integration/apis/saved_objects/get.js
@@ -34,7 +34,7 @@ export default function ({ getService }) {
     const expectNotFound = (resp) => {
       expect(resp.body).to.eql({
         error: 'Not Found',
-        message: 'Not Found',
+        message: 'Saved object [visualization/foobar] not found',
         statusCode: 404,
       });
     };

diff --git a/x-pack/test/rbac_api_integration/apis/saved_objects/update.js b/x-pack/test/rbac_api_integration/apis/saved_objects/update.js
@@ -34,7 +34,7 @@ export default function ({ getService }) {
       expect(resp.body).eql({
         statusCode: 404,
         error: 'Not Found',
-        message: 'Not Found'
+        message: 'Saved object [visualization/not an id] not found'
       });
     };