Record security feature usage #67526

legrego · 2020-05-27T17:21:36Z

Summary

Updates the security plugin to notify the featureUsage service when its paid features are used:

Sub-feature privileges
Pre-access agreement

Resolves #64847
Resolves #67527

…rd-subfeature-usage

legrego · 2020-05-28T00:27:21Z

@elasticmachine merge upstream

legrego · 2020-05-28T00:23:45Z

x-pack/plugins/security/server/feature_usage/feature_usage_service.ts

+    featureUsage.register('security_pre_access_agreement');
+  }
+
+  public start({ featureUsage }: StartDeps): SecurityFeatureUsageServiceStart {


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:

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.

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.

Yeah, sounds reasonable to me 👍

legrego · 2020-05-28T10:00:20Z

@elasticmachine merge upstream

legrego · 2020-05-28T14:57:40Z

@elasticmachine merge upstream

legrego · 2020-05-28T16:41:17Z

Latest failure was due to OOM?

11:43:43 Cannot contact kibana-ci-immutable-ubuntu-16-1590677899491886197: java.lang.InterruptedException
11:48:19 OpenJDK 64-Bit Server VM warning: INFO: os::commit_memory(0x0000000789380000, 37224448, 0) failed; error='Cannot allocate memory' (errno=12)
11:48:19 #
11:48:19 # There is insufficient memory for the Java Runtime Environment to continue.
11:48:19 # Native memory allocation (mmap) failed to map 37224448 bytes for committing reserved memory.
11:48:19 # An error report file with more information is saved as:
11:48:19 # /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/hs_err_pid4148.log

legrego · 2020-05-28T16:41:30Z

@elasticmachine merge upstream

elasticmachine · 2020-05-28T18:49:58Z

Pinging @elastic/kibana-security (Team:Security)

x-pack/plugins/security/server/feature_usage/feature_usage_service.ts

…ubfeature-usage

legrego · 2020-06-01T10:22:40Z

@elasticmachine merge upstream

…/kibana into security/record-subfeature-usage

kobelb · 2020-06-01T17:10:47Z

#67712 just merged 😬

…rd-subfeature-usage

pgayvallet · 2020-06-02T14:49:27Z

Ack: will review tomorrow.

pgayvallet

LGTM once the FTR test is modified. Other comments are NITs

pgayvallet · 2020-06-02T19:58:13Z

x-pack/plugins/licensing/server/types.ts

@@ -8,6 +8,8 @@ import { IClusterClient } from 'src/core/server';
 import { ILicense, LicenseStatus, LicenseType } from '../common/types';
 import { FeatureUsageServiceSetup, FeatureUsageServiceStart } from './services';

+export { FeatureUsageServiceSetup, FeatureUsageServiceStart } from './services';


NIT: i feel like it would be slightly better to export them directly from the index, as there are no other re-export in types

pgayvallet · 2020-06-02T20:04:26Z

x-pack/plugins/security/server/feature_usage/feature_usage_service.ts

+      recordSubFeaturePrivilegeUsage(kibanaPrivileges: Array<Optional<RoleKibanaPrivilege>> = []) {
+        featureUsage.notifyUsage('Subfeature privileges');
+      },


This unused parameter is here to anticipate the changes on the featureUsage service?

Ah good catch, this was a holdover from an earlier implementation I was experimenting with. I'm surprised the linter didn't flag this as unused

pgayvallet · 2020-06-02T20:07:37Z

x-pack/plugins/security/server/plugin.test.ts

+    mockDependencies = ({
+      licensing: { license$: of({}), featureUsage: { register: jest.fn() } },
+    } as unknown) as PluginSetupDependencies;


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

pgayvallet · 2020-06-02T20:12:49Z

x-pack/plugins/security/server/plugin.ts

+    if (this.featureUsageService) {
+      this.featureUsageService = undefined;
+    }
+
+    if (this.featureUsageServiceStart) {
+      this.featureUsageServiceStart = undefined;
+    }


I know you are also doing it with your other services (but the other service also have a call to a destroy-ish method), but are these undefined attributions really useful in any way?

No I suppose they aren't. I can at least get rid of the logic for featureUsageService, but I'll probably keep the handling for featureUsageServiceStart for the time being

pgayvallet · 2020-06-02T20:15:26Z

x-pack/test/plugin_api_integration/test_suites/licensed_feature_usage/feature_usage.ts

+          {
+            last_used: null,
+            license_level: 'gold',
+            name: 'Subfeature privileges',
+          },


Sorry about this one, but to avoid all other futur consumers of the service to have to do the same thing, could you adapt the test to only assert existence / date of the 3 Test feature X instead?

I absolutely can! I wasn't sure about the intent of this test, but your suggestion makes sense to me based on the test description.

…rd-subfeature-usage

legrego · 2020-06-03T21:53:17Z

@elastic/kibana-security this is ready for review

azasypkin · 2020-06-04T09:52:35Z

ACK: will review today or early morning tomorrow.

azasypkin

LGTM, thanks for doing that! Tested locally and everything worked as expected.

azasypkin · 2020-06-04T10:31:29Z

x-pack/plugins/security/server/feature_usage/feature_usage_service.ts

+    featureUsage.register('security_pre_access_agreement');
+  }
+
+  public start({ featureUsage }: StartDeps): SecurityFeatureUsageServiceStart {


Yeah, sounds reasonable to me 👍

azasypkin · 2020-06-04T11:18:38Z

x-pack/plugins/security/server/plugin.ts

@@ -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({


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

azasypkin · 2020-06-04T11:56:37Z

x-pack/plugins/security/server/plugin.ts

+    if (!this.featureUsageServiceStart) {
+      throw new Error(`featureUsageServiceStart is not registered!`);
+    }
+    return this.featureUsageServiceStart!;


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

azasypkin · 2020-06-04T12:01:12Z

x-pack/plugins/security/server/plugin.ts

@@ -160,6 +183,11 @@ export class Plugin {
      authc,
      authz,
      license,
+      getFeatures: () =>


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).

azasypkin · 2020-06-04T12:19:47Z

x-pack/plugins/security/server/routes/authorization/roles/put.ts

@@ -14,7 +16,40 @@ import {
  transformPutPayloadToElasticsearchRole,
 } from './model';

-export function definePutRolesRoutes({ router, authz, clusterClient }: RouteDefinitionParams) {
+const roleGrantsSubFeaturePrivileges = (features: Feature[], role: Role) => {


question: I'm wondering if it'd be safer to use TypeOf<ReturnType<typeof getPutPayloadSchema>> to catch any discrepancies between Role and the type of request we send to create that role at the compile-time (and hence remove as Role cast below)?

optional nit: I think you could also simply this a bit (to get rid of reduce and hasOwnProperty) with a Map, but it's subjective and up to you:

const roleGrantsSubFeaturePrivileges = ( features: Feature[], role: TypeOf<ReturnType<typeof getPutPayloadSchema>> ) => { if (!role.kibana) { return false; } const subFeaturePrivileges = new Map( features.map( (feature) => [ feature.id, feature.subFeatures?.map((sf) => sf.privilegeGroups.map((pg) => pg.privileges)).flat(2), ] as [string, SubFeaturePrivilegeConfig[]] ) ); const hasAnySubFeaturePrivileges = role.kibana.some((kibanaPrivilege) => Object.entries(kibanaPrivilege.feature ?? {}).some(([featureId, privileges]) => { return !!subFeaturePrivileges.get(featureId)?.some(({ id }) => privileges.includes(id)); }) ); return hasAnySubFeaturePrivileges; };

Both great ideas, thank you!

azasypkin · 2020-06-04T12:57:16Z

x-pack/plugins/security/server/routes/authorization/roles/put.test.ts

+    if (asserts.recordSubFeaturePrivilegeUsage) {
+      expect(
+        mockRouteDefinitionParams.getFeatureUsageService().recordSubFeaturePrivilegeUsage
+      ).toHaveBeenCalledTimes(1);
+    } else {
+      expect(
+        mockRouteDefinitionParams.getFeatureUsageService().recordSubFeaturePrivilegeUsage
+      ).toHaveBeenCalledTimes(0);
+    }


optional nit:

Suggested change

if (asserts.recordSubFeaturePrivilegeUsage) {

expect(

mockRouteDefinitionParams.getFeatureUsageService().recordSubFeaturePrivilegeUsage

).toHaveBeenCalledTimes(1);

} else {

expect(

mockRouteDefinitionParams.getFeatureUsageService().recordSubFeaturePrivilegeUsage

).toHaveBeenCalledTimes(0);

}

if (asserts.recordSubFeaturePrivilegeUsage) {

expect(

mockRouteDefinitionParams.getFeatureUsageService().recordSubFeaturePrivilegeUsage

).toHaveBeenCalledTimes(1);

} else {

expect(

mockRouteDefinitionParams.getFeatureUsageService().recordSubFeaturePrivilegeUsage

).not.toHaveBeenCalled();

}

OR

Suggested change

if (asserts.recordSubFeaturePrivilegeUsage) {

expect(

mockRouteDefinitionParams.getFeatureUsageService().recordSubFeaturePrivilegeUsage

).toHaveBeenCalledTimes(1);

} else {

expect(

mockRouteDefinitionParams.getFeatureUsageService().recordSubFeaturePrivilegeUsage

).toHaveBeenCalledTimes(0);

}

expect(

mockRouteDefinitionParams.getFeatureUsageService().recordSubFeaturePrivilegeUsage

).toHaveBeenCalledTimes(asserts.recordSubFeaturePrivilegeUsage ? 1 : 0);

azasypkin · 2020-06-04T13:12:25Z

x-pack/plugins/security/server/routes/authorization/roles/put.ts

+    {}
+  );
+
+  const hasAnySubFeaturePrivileges = (role.kibana ?? []).some((kibanaPrivilege) =>


question: am I understanding this correctly that we're recording not the fact that subfeature privileges are used per se, but rather the fact that user chose to customize sub features (e.g. when user chooses All, all subfeature privileges are "granted" and relied upon implicitly if I'm not missing anything)? If so, wondering if we should make that clear in the feature usage name/id (e.g. put custom in there or something like this). Not important at all, just curious.

That's exactly right. We are only recording the fact that a role was created/updated with explicit sub-feature privileges granted, as opposed to the "primary" feature privileges, or the base privileges.

I think the usage name is ok for the time being. The intent is to figure out when sub-feature privileges are being used at all, and this customization check is a "close enough" approximation of this. There is potential that we'll change the way we check for this in the future.

Sure, works for me too 👍

…rd-subfeature-usage

kibanamachine · 2020-06-04T16:10:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 46e59e8

History

💚 Build #51514 succeeded 0fa0926
💔 Build #51350 failed a0bd707
💚 Build #50918 succeeded 4b092df
💔 Build #50881 failed f88753e
💚 Build #50771 succeeded ffdd91c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

legrego added 3 commits May 27, 2020 10:26

record subfeature privilege usage

5d50b00

prefix feature keys with 'security_'

1079df2

Merge branch 'master' of github.com:elastic/kibana into security/reco…

f8023a1

…rd-subfeature-usage

Merge branch 'master' into security/record-subfeature-usage

5bafd74

legrego commented May 28, 2020

View reviewed changes

Merge branch 'master' into security/record-subfeature-usage

0dd8f38

Merge branch 'master' into security/record-subfeature-usage

7cc7dc6

Merge branch 'master' into security/record-subfeature-usage

8300f8f

legrego added release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.9.0 v8.0.0 labels May 28, 2020

legrego commented May 28, 2020

View reviewed changes

x-pack/plugins/security/server/feature_usage/feature_usage_service.ts Outdated Show resolved Hide resolved

legrego marked this pull request as ready for review May 29, 2020 13:48

legrego requested review from a team as code owners May 29, 2020 13:48

legrego added 2 commits May 29, 2020 09:59

Merge remote-tracking branch 'upstream/master' into security/record-s…

40cdabb

…ubfeature-usage

remove duplicate variable identifier from merge

4d15c05

elasticmachine and others added 3 commits June 1, 2020 03:22

Merge branch 'master' into security/record-subfeature-usage

9a4e07d

remove legacy api

2974118

Merge branch 'security/record-subfeature-usage' of github.com:legrego…

ffdd91c

…/kibana into security/record-subfeature-usage

kobelb mentioned this pull request Jun 1, 2020

Updating the licensed feature usage API response format #67712

Merged

legrego added 3 commits June 1, 2020 14:09

Merge branch 'master' of github.com:elastic/kibana into security/reco…

8b2c17c

…rd-subfeature-usage

Use updated feature usage API changed by elastic#67712

f88753e

update functional test

4b092df

pgayvallet approved these changes Jun 2, 2020

View reviewed changes

legrego added 3 commits June 3, 2020 07:29

address PR feedback

dfb16e9

Merge branch 'master' of github.com:elastic/kibana into security/reco…

a0bd707

…rd-subfeature-usage

fix functional test

0fa0926

azasypkin self-requested a review June 4, 2020 09:52

azasypkin approved these changes Jun 4, 2020

View reviewed changes

legrego added 2 commits June 4, 2020 10:19

address PR feedback

de81ff1

Merge branch 'master' of github.com:elastic/kibana into security/reco…

46e59e8

…rd-subfeature-usage

legrego merged commit a9b2d50 into elastic:master Jun 4, 2020

legrego deleted the security/record-subfeature-usage branch June 4, 2020 16:29

legrego mentioned this pull request Jun 4, 2020

[7.x] Record security feature usage (#67526) #68283

Merged

legrego added a commit to legrego/kibana that referenced this pull request Jun 4, 2020

Record security feature usage (elastic#67526)

ee504cc

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

legrego added a commit that referenced this pull request Jun 4, 2020

[7.x] Record security feature usage (#67526) (#68283)

112b80b

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Record security feature usage #67526

Record security feature usage #67526

Conversation

legrego commented May 27, 2020 • edited Loading

Summary

legrego commented May 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego commented May 28, 2020

legrego commented May 28, 2020

legrego commented May 28, 2020

legrego commented May 28, 2020

elasticmachine commented May 28, 2020

legrego commented Jun 1, 2020

kobelb commented Jun 1, 2020

pgayvallet commented Jun 2, 2020

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego commented Jun 3, 2020

azasypkin commented Jun 4, 2020

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jun 4, 2020

💚 Build Succeeded

History

legrego commented May 27, 2020 •

edited

Loading