Skip to content

Commit 108d1cf

Browse files
azasypkinazasypkin
committed
Review#1: dont unsubscribe from ES status and license changes after initial registration + add API integration tests for the license downgrade scenario.
1 parent 1634090 commit 108d1cf

8 files changed

Lines changed: 111 additions & 14 deletions

File tree

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -173,9 +173,9 @@ describe('#start', () => {
173173

174174
await nextTick();
175175

176-
// New changes don't trigger privileges registration.
176+
// New changes still trigger privileges re-registration.
177177
licenseSubject.next(({} as unknown) as SecurityLicenseFeatures);
178-
expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(1);
178+
expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(2);
179179
});
180180

181181
it('schedules retries if fails to register cluster privileges', async () => {
@@ -214,9 +214,9 @@ describe('#start', () => {
214214
jest.runAllTimers();
215215
expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(4);
216216

217-
// New changes don't trigger privileges registration.
217+
// New changes still trigger privileges re-registration.
218218
licenseSubject.next(({} as unknown) as SecurityLicenseFeatures);
219-
expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(4);
219+
expect(mockRegisterPrivilegesWithCluster).toHaveBeenCalledTimes(5);
220220
});
221221
});
222222

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

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import { combineLatest, BehaviorSubject, Subscription } from 'rxjs';
8-
import { filter, takeWhile } from 'rxjs/operators';
8+
import { distinctUntilChanged, filter } from 'rxjs/operators';
99
import { UICapabilities } from 'ui/capabilities';
1010
import {
1111
LoggerFactory,
@@ -183,10 +183,13 @@ export class AuthorizationService {
183183
this.statusSubscription = combineLatest([
184184
this.status.core$,
185185
this.license.features$,
186-
retries$.asObservable(),
186+
retries$.asObservable().pipe(
187+
// We shouldn't emit new value if retry counter is reset. This comparator isn't called for
188+
// the initial value.
189+
distinctUntilChanged((prev, curr) => prev === curr || curr === 0)
190+
),
187191
])
188192
.pipe(
189-
takeWhile(() => !retries$.isStopped),
190193
filter(
191194
([status]) =>
192195
this.license.isEnabled() && status.elasticsearch.level === ServiceStatusLevels.available
@@ -205,7 +208,7 @@ export class AuthorizationService {
205208
this.applicationName,
206209
clusterClient
207210
);
208-
retries$.complete();
211+
retries$.next(0);
209212
} catch (err) {
210213
const retriesElapsed = retries$.getValue() + 1;
211214
retryTimeout = setTimeout(

‎x-pack/scripts/functional_tests.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,9 @@ const alwaysImportedTests = [
1111
require.resolve('../test/functional/config_security_trial.ts'),
1212
];
1313
const onlyNotInCoverageTests = [
14-
require.resolve('../test/api_integration/config_security_basic.js'),
15-
require.resolve('../test/api_integration/config.js'),
14+
require.resolve('../test/api_integration/config_security_basic.ts'),
15+
require.resolve('../test/api_integration/config_security_trial.ts'),
16+
require.resolve('../test/api_integration/config.ts'),
1617
require.resolve('../test/alerting_api_integration/basic/config.ts'),
1718
require.resolve('../test/alerting_api_integration/spaces_only/config.ts'),
1819
require.resolve('../test/alerting_api_integration/security_and_spaces/config.ts'),

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
7+
import expect from '@kbn/expect/expect.js';
8+
import { FtrProviderContext } from '../../ftr_provider_context';
9+
10+
export default function ({ getService }: FtrProviderContext) {
11+
const supertest = getService('supertest');
12+
13+
describe('Privileges registration', function () {
14+
this.tags(['skipCloud']);
15+
16+
it('privileges are re-registered on license downgrade', async () => {
17+
// Verify currently registered privileges for TRIAL license.
18+
// If you're adding a privilege to the following, that's great!
19+
// If you're removing a privilege, this breaks backwards compatibility
20+
// Roles are associated with these privileges, and we shouldn't be removing them in a minor version.
21+
const expectedTrialLicenseDiscoverPrivileges = [
22+
'all',
23+
'read',
24+
'minimal_all',
25+
'minimal_read',
26+
'url_create',
27+
];
28+
const trialPrivileges = await supertest
29+
.get('/api/security/privileges')
30+
.set('kbn-xsrf', 'xxx')
31+
.send()
32+
.expect(200);
33+
34+
expect(trialPrivileges.body.features.discover).to.eql(expectedTrialLicenseDiscoverPrivileges);
35+
36+
// Revert license to basic.
37+
await supertest
38+
.post('/api/license/start_basic?acknowledge=true')
39+
.set('kbn-xsrf', 'xxx')
40+
.expect(200, {
41+
basic_was_started: true,
42+
acknowledged: true,
43+
});
44+
45+
// Verify that privileges were re-registered.
46+
const expectedBasicLicenseDiscoverPrivileges = ['all', 'read'];
47+
const basicPrivileges = await supertest
48+
.get('/api/security/privileges')
49+
.set('kbn-xsrf', 'xxx')
50+
.send()
51+
.expect(200);
52+
53+
expect(basicPrivileges.body.features.discover).to.eql(expectedBasicLicenseDiscoverPrivileges);
54+
});
55+
});
56+
}

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
7+
import { FtrProviderContext } from '../../ftr_provider_context';
8+
9+
export default function ({ loadTestFile }: FtrProviderContext) {
10+
describe('security (trial license)', function () {
11+
this.tags('ciGroup6');
12+
13+
// THIS TEST NEEDS TO BE LAST. IT IS DESTRUCTIVE! IT REMOVES TRIAL LICENSE!!!
14+
loadTestFile(require.resolve('./license_downgrade'));
15+
});
16+
}

x-pack/test/api_integration/config.js renamed to x-pack/test/api_integration/config.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,10 @@
44
* you may not use this file except in compliance with the Elastic License.
55
*/
66

7+
import { FtrConfigProviderContext } from '@kbn/test/types/ftr';
78
import { services } from './services';
89

9-
export async function getApiIntegrationConfig({ readConfigFile }) {
10+
export async function getApiIntegrationConfig({ readConfigFile }: FtrConfigProviderContext) {
1011
const xPackFunctionalTestsConfig = await readConfigFile(
1112
require.resolve('../functional/config.js')
1213
);

x-pack/test/api_integration/config_security_basic.js renamed to x-pack/test/api_integration/config_security_basic.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,14 @@
44
* you may not use this file except in compliance with the Elastic License.
55
*/
66

7+
/* eslint-disable import/no-default-export */
8+
9+
import { FtrConfigProviderContext } from '@kbn/test/types/ftr';
710
import { default as createTestConfig } from './config';
811

9-
export default async function ({ readConfigFile }) {
10-
//security APIs should function the same under a basic or trial license
11-
return createTestConfig({ readConfigFile }).then((config) => {
12+
export default async function (context: FtrConfigProviderContext) {
13+
// security APIs should function the same under a basic or trial license
14+
return createTestConfig(context).then((config) => {
1215
config.esTestCluster.license = 'basic';
1316
config.esTestCluster.serverArgs = [
1417
'xpack.license.self_generated.type=basic',

‎x-pack/test/api_integration/config_security_trial.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
7+
/* eslint-disable import/no-default-export */
8+
9+
import { FtrConfigProviderContext } from '@kbn/test/types/ftr';
10+
import { default as createTestConfig } from './config';
11+
12+
export default async function (context: FtrConfigProviderContext) {
13+
return createTestConfig(context).then((config) => {
14+
config.testFiles = [require.resolve('./apis/security/security_trial')];
15+
return config;
16+
});
17+
}

0 commit comments

Comments
 (0)