Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record security feature usage #67526

Merged
merged 20 commits into from
Jun 4, 2020
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5d50b00
record subfeature privilege usage
legrego May 20, 2020
1079df2
prefix feature keys with 'security_'
legrego May 27, 2020
f8023a1
Merge branch 'master' of github.com:elastic/kibana into security/reco…
legrego May 27, 2020
5bafd74
Merge branch 'master' into security/record-subfeature-usage
elasticmachine May 28, 2020
0dd8f38
Merge branch 'master' into security/record-subfeature-usage
elasticmachine May 28, 2020
7cc7dc6
Merge branch 'master' into security/record-subfeature-usage
elasticmachine May 28, 2020
8300f8f
Merge branch 'master' into security/record-subfeature-usage
elasticmachine May 28, 2020
40cdabb
Merge remote-tracking branch 'upstream/master' into security/record-s…
legrego May 29, 2020
4d15c05
remove duplicate variable identifier from merge
legrego May 29, 2020
9a4e07d
Merge branch 'master' into security/record-subfeature-usage
elasticmachine Jun 1, 2020
2974118
remove legacy api
legrego Jun 1, 2020
ffdd91c
Merge branch 'security/record-subfeature-usage' of github.com:legrego…
legrego Jun 1, 2020
8b2c17c
Merge branch 'master' of github.com:elastic/kibana into security/reco…
legrego Jun 1, 2020
f88753e
Use updated feature usage API changed by https://github.com/elastic/k…
legrego Jun 1, 2020
4b092df
update functional test
legrego Jun 1, 2020
dfb16e9
address PR feedback
legrego Jun 3, 2020
a0bd707
Merge branch 'master' of github.com:elastic/kibana into security/reco…
legrego Jun 3, 2020
0fa0926
fix functional test
legrego Jun 3, 2020
de81ff1
address PR feedback
legrego Jun 4, 2020
46e59e8
Merge branch 'master' of github.com:elastic/kibana into security/reco…
legrego Jun 4, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions x-pack/plugins/licensing/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { LicensingPlugin } from './plugin';
export const plugin = (context: PluginInitializerContext) => new LicensingPlugin(context);

export * from '../common/types';
export { FeatureUsageServiceSetup, FeatureUsageServiceStart } from './services';
export * from './types';
export { config } from './licensing_config';
export { CheckLicense, wrapRouteWithLicenseCheck } from './wrap_route_with_license_check';
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { AuthenticationResult } from './authentication_result';
import { Authenticator, AuthenticatorOptions, ProviderSession } from './authenticator';
import { DeauthenticationResult } from './deauthentication_result';
import { BasicAuthenticationProvider, SAMLAuthenticationProvider } from './providers';
import { securityFeatureUsageServiceMock } from '../feature_usage/index.mock';

function getMockOptions({
session,
Expand All @@ -54,6 +55,9 @@ function getMockOptions({
{ isTLSEnabled: false }
),
sessionStorageFactory: sessionStorageMock.createFactory<ProviderSession>(),
getFeatureUsageService: jest
.fn()
.mockReturnValue(securityFeatureUsageServiceMock.createStartContract()),
};
}

Expand Down Expand Up @@ -1451,6 +1455,9 @@ describe('Authenticator', () => {
);

expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(
mockOptions.getFeatureUsageService().recordPreAccessAgreementUsage
).not.toHaveBeenCalled();
});

it('fails if cannot retrieve user session', async () => {
Expand All @@ -1463,6 +1470,9 @@ describe('Authenticator', () => {
);

expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(
mockOptions.getFeatureUsageService().recordPreAccessAgreementUsage
).not.toHaveBeenCalled();
});

it('fails if license doesn allow access agreement acknowledgement', async () => {
Expand All @@ -1477,6 +1487,9 @@ describe('Authenticator', () => {
);

expect(mockSessionStorage.set).not.toHaveBeenCalled();
expect(
mockOptions.getFeatureUsageService().recordPreAccessAgreementUsage
).not.toHaveBeenCalled();
});

it('properly acknowledges access agreement for the authenticated user', async () => {
Expand All @@ -1493,6 +1506,10 @@ describe('Authenticator', () => {
type: 'basic',
name: 'basic1',
});

expect(
mockOptions.getFeatureUsageService().recordPreAccessAgreementUsage
).toHaveBeenCalledTimes(1);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { DeauthenticationResult } from './deauthentication_result';
import { Tokens } from './tokens';
import { canRedirectRequest } from './can_redirect_request';
import { HTTPAuthorizationHeader } from './http_authentication';
import { SecurityFeatureUsageServiceStart } from '../feature_usage';

/**
* The shape of the session that is actually stored in the cookie.
Expand Down Expand Up @@ -94,6 +95,7 @@ export interface ProviderLoginAttempt {

export interface AuthenticatorOptions {
auditLogger: SecurityAuditLogger;
getFeatureUsageService: () => SecurityFeatureUsageServiceStart;
getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null;
config: Pick<ConfigType, 'session' | 'authc'>;
basePath: HttpServiceSetup['basePath'];
Expand Down Expand Up @@ -502,6 +504,8 @@ export class Authenticator {
currentUser.username,
existingSession.provider
);

this.options.getFeatureUsageService().recordPreAccessAgreementUsage();
}

/**
Expand Down
6 changes: 6 additions & 0 deletions x-pack/plugins/security/server/authentication/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ import {
} from './api_keys';
import { SecurityLicense } from '../../common/licensing';
import { SecurityAuditLogger } from '../audit';
import { SecurityFeatureUsageServiceStart } from '../feature_usage';
import { securityFeatureUsageServiceMock } from '../feature_usage/index.mock';

describe('setupAuthentication()', () => {
let mockSetupAuthenticationParams: {
Expand All @@ -51,6 +53,7 @@ describe('setupAuthentication()', () => {
http: jest.Mocked<CoreSetup['http']>;
clusterClient: jest.Mocked<IClusterClient>;
license: jest.Mocked<SecurityLicense>;
getFeatureUsageService: () => jest.Mocked<SecurityFeatureUsageServiceStart>;
};
let mockScopedClusterClient: jest.Mocked<PublicMethodsOf<ScopedClusterClient>>;
beforeEach(() => {
Expand All @@ -69,6 +72,9 @@ describe('setupAuthentication()', () => {
clusterClient: elasticsearchServiceMock.createClusterClient(),
license: licenseMock.create(),
loggers: loggingServiceMock.create(),
getFeatureUsageService: jest
.fn()
.mockReturnValue(securityFeatureUsageServiceMock.createStartContract()),
};

mockScopedClusterClient = elasticsearchServiceMock.createScopedClusterClient();
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/security/server/authentication/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { ConfigType } from '../config';
import { getErrorStatusCode } from '../errors';
import { Authenticator, ProviderSession } from './authenticator';
import { APIKeys, CreateAPIKeyParams, InvalidateAPIKeyParams } from './api_keys';
import { SecurityFeatureUsageServiceStart } from '../feature_usage';

export { canRedirectRequest } from './can_redirect_request';
export { Authenticator, ProviderLoginAttempt } from './authenticator';
Expand All @@ -37,6 +38,7 @@ export {

interface SetupAuthenticationParams {
auditLogger: SecurityAuditLogger;
getFeatureUsageService: () => SecurityFeatureUsageServiceStart;
http: CoreSetup['http'];
clusterClient: IClusterClient;
config: ConfigType;
Expand All @@ -48,6 +50,7 @@ export type Authentication = UnwrapPromise<ReturnType<typeof setupAuthentication

export async function setupAuthentication({
auditLogger,
getFeatureUsageService,
http,
clusterClient,
config,
Expand Down Expand Up @@ -86,6 +89,7 @@ export async function setupAuthentication({

const authenticator = new Authenticator({
auditLogger,
getFeatureUsageService,
getCurrentUser,
clusterClient,
basePath: http.basePath,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* 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 { SecurityFeatureUsageService } from './feature_usage_service';

describe('#setup', () => {
it('registers all known security features', () => {
const featureUsage = { register: jest.fn() };
const securityFeatureUsage = new SecurityFeatureUsageService();
securityFeatureUsage.setup({ featureUsage });
expect(featureUsage.register).toHaveBeenCalledTimes(2);
expect(featureUsage.register.mock.calls.map((c) => c[0])).toMatchInlineSnapshot(`
Array [
"Subfeature privileges",
"Pre-access agreement",
]
`);
});
});

describe('start contract', () => {
it('notifies when sub-feature privileges are in use', () => {
const featureUsage = { notifyUsage: jest.fn(), getLastUsages: jest.fn() };
const securityFeatureUsage = new SecurityFeatureUsageService();
const startContract = securityFeatureUsage.start({ featureUsage });
startContract.recordSubFeaturePrivilegeUsage();
expect(featureUsage.notifyUsage).toHaveBeenCalledTimes(1);
expect(featureUsage.notifyUsage).toHaveBeenCalledWith('Subfeature privileges');
});

it('notifies when pre-access agreement is used', () => {
const featureUsage = { notifyUsage: jest.fn(), getLastUsages: jest.fn() };
const securityFeatureUsage = new SecurityFeatureUsageService();
const startContract = securityFeatureUsage.start({ featureUsage });
startContract.recordPreAccessAgreementUsage();
expect(featureUsage.notifyUsage).toHaveBeenCalledTimes(1);
expect(featureUsage.notifyUsage).toHaveBeenCalledWith('Pre-access agreement');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* 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 { FeatureUsageServiceSetup, FeatureUsageServiceStart } from '../../../licensing/server';

interface SetupDeps {
featureUsage: FeatureUsageServiceSetup;
}

interface StartDeps {
featureUsage: FeatureUsageServiceStart;
}

export interface SecurityFeatureUsageServiceStart {
recordPreAccessAgreementUsage: () => void;
recordSubFeaturePrivilegeUsage: () => void;
}

export class SecurityFeatureUsageService {
public setup({ featureUsage }: SetupDeps) {
featureUsage.register('Subfeature privileges', 'gold');
featureUsage.register('Pre-access agreement', 'gold');
}

public start({ featureUsage }: StartDeps): SecurityFeatureUsageServiceStart {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This SecurityFeaureUsageService feels rather unnecessary given the amount of work it's doing. It's really just a very thin facade over the featureUsage service exposed by the licensing plugin.

I opted to do this anyway because:

  1. We've previously tried to isolate the security plugin from the licensing plugin by introducing the SecurityLicense service, so it felt appropriate to continue this initiative.
  2. We expect the featureUsage service to change in the near future, but we don't yet know what that will look like. Having this encapsulated here will hopefully prevent sprawling changes to the security plugin when this happens.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sounds reasonable to me 👍

return {
recordPreAccessAgreementUsage() {
featureUsage.notifyUsage('Pre-access agreement');
},
recordSubFeaturePrivilegeUsage() {
featureUsage.notifyUsage('Subfeature privileges');
},
};
}
}
16 changes: 16 additions & 0 deletions x-pack/plugins/security/server/feature_usage/index.mock.ts
Original file line number Diff line number Diff line change
@@ -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 { SecurityFeatureUsageServiceStart } from './feature_usage_service';

export const securityFeatureUsageServiceMock = {
createStartContract() {
return {
recordPreAccessAgreementUsage: jest.fn(),
recordSubFeaturePrivilegeUsage: jest.fn(),
} as jest.Mocked<SecurityFeatureUsageServiceStart>;
},
};
10 changes: 10 additions & 0 deletions x-pack/plugins/security/server/feature_usage/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* 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.
*/

export {
SecurityFeatureUsageService,
SecurityFeatureUsageServiceStart,
} from './feature_usage_service';
4 changes: 3 additions & 1 deletion x-pack/plugins/security/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ describe('Security Plugin', () => {
mockClusterClient = elasticsearchServiceMock.createCustomClusterClient();
mockCoreSetup.elasticsearch.legacy.createClient.mockReturnValue(mockClusterClient);

mockDependencies = { licensing: { license$: of({}) } } as PluginSetupDependencies;
mockDependencies = ({
licensing: { license$: of({}), featureUsage: { register: jest.fn() } },
} as unknown) as PluginSetupDependencies;
Comment on lines +44 to +46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the licensing plugin's mocks in x-pack/plugins/licensing/server/mocks.ts instead. That would also avoid the unknown cast.

});

describe('setup()', () => {
Expand Down
40 changes: 37 additions & 3 deletions x-pack/plugins/security/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,15 @@ import {
CoreSetup,
Logger,
PluginInitializerContext,
CoreStart,
} from '../../../../src/core/server';
import { deepFreeze } from '../../../../src/core/server';
import { SpacesPluginSetup } from '../../spaces/server';
import { PluginSetupContract as FeaturesSetupContract } from '../../features/server';
import { LicensingPluginSetup } from '../../licensing/server';
import {
PluginSetupContract as FeaturesSetupContract,
PluginStartContract as FeaturesStartContract,
} from '../../features/server';
import { LicensingPluginSetup, LicensingPluginStart } from '../../licensing/server';

import { Authentication, setupAuthentication } from './authentication';
import { Authorization, setupAuthorization } from './authorization';
Expand All @@ -26,6 +30,7 @@ import { SecurityLicenseService, SecurityLicense } from '../common/licensing';
import { setupSavedObjects } from './saved_objects';
import { AuditService, SecurityAuditLogger, AuditServiceSetup } from './audit';
import { elasticsearchClientPlugin } from './elasticsearch_client_plugin';
import { SecurityFeatureUsageService, SecurityFeatureUsageServiceStart } from './feature_usage';

export type SpacesService = Pick<
SpacesPluginSetup['spacesService'],
Expand Down Expand Up @@ -72,6 +77,11 @@ export interface PluginSetupDependencies {
licensing: LicensingPluginSetup;
}

export interface PluginStartDependencies {
features: FeaturesStartContract;
licensing: LicensingPluginStart;
}

/**
* Represents Security Plugin instance that will be managed by the Kibana plugin system.
*/
Expand All @@ -80,6 +90,16 @@ export class Plugin {
private clusterClient?: ICustomClusterClient;
private spacesService?: SpacesService | symbol = Symbol('not accessed');
private securityLicenseService?: SecurityLicenseService;

private readonly featureUsageService = new SecurityFeatureUsageService();
private featureUsageServiceStart?: SecurityFeatureUsageServiceStart;
private readonly getFeatureUsageService = () => {
if (!this.featureUsageServiceStart) {
throw new Error(`featureUsageServiceStart is not registered!`);
}
return this.featureUsageServiceStart!;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we need ! here? Theoretically the check above should tell TS that it's defined.

};

private readonly auditService = new AuditService(this.initializerContext.logger.get('audit'));

private readonly getSpacesService = () => {
Expand Down Expand Up @@ -118,11 +138,14 @@ export class Plugin {
license$: licensing.license$,
});

this.featureUsageService.setup({ featureUsage: licensing.featureUsage });

const audit = this.auditService.setup({ license, config: config.audit });
const auditLogger = new SecurityAuditLogger(audit.getLogger());

const authc = await setupAuthentication({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: yeah, the time has come 🙂 We need a proper AuthenticationService with setup and start methods. Added to my To Do list, doing similar things with AuthorizationService in #65472

auditLogger,
getFeatureUsageService: this.getFeatureUsageService,
http: core.http,
clusterClient: this.clusterClient,
config,
Expand Down Expand Up @@ -160,6 +183,11 @@ export class Plugin {
authc,
authz,
license,
getFeatures: () =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I guess we can simply define core argument as core: CoreSetup<PluginStartDependencies> and then we can get rid of as Promise<[CoreStart, PluginStartDependencies, unknown]> completely

P.S. we'll need to migrate to getFeatures from start contract in other places too, and the worst thing is that we need features synchronously in one of our custom route validation code 🙈 (tried once and stuck at that point).

(core.getStartServices() as Promise<
[CoreStart, PluginStartDependencies, unknown]
>).then(([, { features: featuresStart }]) => featuresStart.getFeatures()),
getFeatureUsageService: this.getFeatureUsageService,
});

return deepFreeze<SecurityPluginSetup>({
Expand Down Expand Up @@ -199,8 +227,11 @@ export class Plugin {
});
}

public start() {
public start(core: CoreStart, { licensing }: PluginStartDependencies) {
this.logger.debug('Starting plugin');
this.featureUsageServiceStart = this.featureUsageService.start({
featureUsage: licensing.featureUsage,
});
}

public stop() {
Expand All @@ -216,6 +247,9 @@ export class Plugin {
this.securityLicenseService = undefined;
}

if (this.featureUsageServiceStart) {
this.featureUsageServiceStart = undefined;
}
this.auditService.stop();
}

Expand Down
Loading