Review#1: dont unsubscribe from ES status and license changes after i…

…nitial registration + add API integration tests for the license downgrade scenario.

x-pack/plugins/security/server/authorization/authorization_service.test.ts

@@ -173,9 +173,9 @@ describe('#start', () => {
 
     await nextTick();
 
-    // New changes don't trigger privileges registration.
+    // New changes still trigger privileges re-registration.
     licenseSubject.next(({} as unknown) as SecurityLicenseFeatures);
-    expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(1);
+    expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(2);
   });
 
   it('schedules retries if fails to register cluster privileges', async () => {
@@ -214,9 +214,9 @@ describe('#start', () => {
     jest.runAllTimers();
     expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(4);
 
-    // New changes don't trigger privileges registration.
+    // New changes still trigger privileges re-registration.
     licenseSubject.next(({} as unknown) as SecurityLicenseFeatures);
-    expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(4);
+    expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(5);
   });
 });
 

x-pack/plugins/security/server/authorization/authorization_service.ts

@@ -5,7 +5,7 @@
  */
 
 import { combineLatest, BehaviorSubject, Subscription } from 'rxjs';
-import { filter, takeWhile } from 'rxjs/operators';
+import { distinctUntilChanged, filter } from 'rxjs/operators';
 import { UICapabilities } from 'ui/capabilities';
 import {
   LoggerFactory,
@@ -183,10 +183,13 @@ export class AuthorizationService {
     this.statusSubscription = combineLatest([
       this.status.core$,
       this.license.features$,
-      retries$.asObservable(),
+      retries$.asObservable().pipe(
+        // We shouldn't emit new value if retry counter is reset. This comparator isn't called for
+        // the initial value.
+        distinctUntilChanged((prev, curr) => prev === curr || curr === 0)
+      ),
     ])
       .pipe(
-        takeWhile(() => !retries$.isStopped),
         filter(
           ([status]) =>
             this.license.isEnabled() && status.elasticsearch.level === ServiceStatusLevels.available
@@ -205,7 +208,7 @@ export class AuthorizationService {
             this.applicationName,
             clusterClient
           );
-          retries$.complete();
+          retries$.next(0);
         } catch (err) {
           const retriesElapsed = retries$.getValue() + 1;
           retryTimeout = setTimeout(

x-pack/scripts/functional_tests.js

@@ -11,8 +11,9 @@ const alwaysImportedTests = [
   require.resolve('../test/functional/config_security_trial.ts'),
 ];
 const onlyNotInCoverageTests = [
-  require.resolve('../test/api_integration/config_security_basic.js'),
-  require.resolve('../test/api_integration/config.js'),
+  require.resolve('../test/api_integration/config_security_basic.ts'),
+  require.resolve('../test/api_integration/config_security_trial.ts'),
+  require.resolve('../test/api_integration/config.ts'),
   require.resolve('../test/alerting_api_integration/basic/config.ts'),
   require.resolve('../test/alerting_api_integration/spaces_only/config.ts'),
   require.resolve('../test/alerting_api_integration/security_and_spaces/config.ts'),

x-pack/test/api_integration/apis/security/license_downgrade.ts

@@ -0,0 +1,56 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import expect from '@kbn/expect/expect.js';
+import { FtrProviderContext } from '../../ftr_provider_context';
+
+export default function ({ getService }: FtrProviderContext) {
+  const supertest = getService('supertest');
+
+  describe('Privileges registration', function () {
+    this.tags(['skipCloud']);
+
+    it('privileges are re-registered on license downgrade', async () => {
+      // Verify currently registered privileges for TRIAL license.
+      // If you're adding a privilege to the following, that's great!
+      // If you're removing a privilege, this breaks backwards compatibility
+      // Roles are associated with these privileges, and we shouldn't be removing them in a minor version.
+      const expectedTrialLicenseDiscoverPrivileges = [
+        'all',
+        'read',
+        'minimal_all',
+        'minimal_read',
+        'url_create',
+      ];
+      const trialPrivileges = await supertest
+        .get('/api/security/privileges')
+        .set('kbn-xsrf', 'xxx')
+        .send()
+        .expect(200);
+
+      expect(trialPrivileges.body.features.discover).to.eql(expectedTrialLicenseDiscoverPrivileges);
+
+      // Revert license to basic.
+      await supertest
+        .post('/api/license/start_basic?acknowledge=true')
+        .set('kbn-xsrf', 'xxx')
+        .expect(200, {
+          basic_was_started: true,
+          acknowledged: true,
+        });
+
+      // Verify that privileges were re-registered.
+      const expectedBasicLicenseDiscoverPrivileges = ['all', 'read'];
+      const basicPrivileges = await supertest
+        .get('/api/security/privileges')
+        .set('kbn-xsrf', 'xxx')
+        .send()
+        .expect(200);
+
+      expect(basicPrivileges.body.features.discover).to.eql(expectedBasicLicenseDiscoverPrivileges);
+    });
+  });
+}

x-pack/test/api_integration/apis/security/security_trial.ts

@@ -0,0 +1,16 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { FtrProviderContext } from '../../ftr_provider_context';
+
+export default function ({ loadTestFile }: FtrProviderContext) {
+  describe('security (trial license)', function () {
+    this.tags('ciGroup6');
+
+    // THIS TEST NEEDS TO BE LAST. IT IS DESTRUCTIVE! IT REMOVES TRIAL LICENSE!!!
+    loadTestFile(require.resolve('./license_downgrade'));
+  });
+}

x-pack/test/api_integration/config.js → x-pack/test/api_integration/config.ts

@@ -4,9 +4,10 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
+import { FtrConfigProviderContext } from '@kbn/test/types/ftr';
 import { services } from './services';
 
-export async function getApiIntegrationConfig({ readConfigFile }) {
+export async function getApiIntegrationConfig({ readConfigFile }: FtrConfigProviderContext) {
   const xPackFunctionalTestsConfig = await readConfigFile(
     require.resolve('../functional/config.js')
   );

x-pack/test/api_integration/config_security_basic.js → x-pack/test/api_integration/config_security_basic.ts

@@ -4,11 +4,14 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
+/* eslint-disable import/no-default-export */
+
+import { FtrConfigProviderContext } from '@kbn/test/types/ftr';
 import { default as createTestConfig } from './config';
 
-export default async function ({ readConfigFile }) {
-  //security APIs should function the same under a basic or trial license
-  return createTestConfig({ readConfigFile }).then((config) => {
+export default async function (context: FtrConfigProviderContext) {
+  // security APIs should function the same under a basic or trial license
+  return createTestConfig(context).then((config) => {
     config.esTestCluster.license = 'basic';
     config.esTestCluster.serverArgs = [
       'xpack.license.self_generated.type=basic',

x-pack/test/api_integration/config_security_trial.ts

@@ -0,0 +1,17 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+/* eslint-disable import/no-default-export */
+
+import { FtrConfigProviderContext } from '@kbn/test/types/ftr';
+import { default as createTestConfig } from './config';
+
+export default async function (context: FtrConfigProviderContext) {
+  return createTestConfig(context).then((config) => {
+    config.testFiles = [require.resolve('./apis/security/security_trial')];
+    return config;
+  });
+}