Spaces - NP updates for usage collection and capabilities #57693

legrego · 2020-02-14T15:06:46Z

Summary

Removes `legacyConfig.kibanaIndex`

Replaces this entry with the kibana index sourced from the NP initializerContext, consistent with the way lens accomplishes this.

This was being used by our usage collector. Since this was getting updated anyway, I moved the collector files to a more logical location (and out of the lib dumping ground).

Moves `uiCapabilities` provider

The NP has support for capability providers. This removes the spaces capabilities definition from the LP to the NP. Since this was getting updated anyway, I moved the capability files to a more logical location (and out of the lib dumping ground)

Exposes `features` plugin start contract

The features plugin did not have a start contract. This adds one to retrieve the available features.

This is a more logical location than setup, because features can no longer be registered once getFeatures() is called.

Mocks

Additional tests were added, which rely on plugins which did not have mocks in place. This PR added mocks as appropriate.

elasticmachine · 2020-02-14T15:07:20Z

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

legrego · 2020-02-14T17:02:07Z

@elasticmachine merge upstream

…s/moar-np

legrego · 2020-02-14T18:19:26Z

x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts

+    const isAnonymousRequest = !request.route.options.authRequired;
+
+    if (isAnonymousRequest) {
+      return capabilities;


The existing implementation behaved this way, but it wasn't explicit. We used to silently return the capabilities unmodified if an error was thrown here. If an unauthenticated route attempts to retrieve the active space, the request fails because auth headers aren't attached to the request.

There's a subtle distinction though. The existing implementation would still toggle capabilities for anonymous requests when security was disabled. Now, we will always skip this step. I think this is actually preferred, because we'll have consistent behavior between the two setups (spaces, spaces + security)

Yeah, I agree, thanks for explaining!

azasypkin · 2020-02-20T14:57:32Z

ACK: sorry for the delay, will review tomorrow

azasypkin

Code changes look good and thanks for adding more tests! PR is marked as Draft so I haven't tested changes locally yet, but let me know if you just forgot to toggle PR state and it's ready to be tested.

src/core/server/http/http_server.mocks.ts

src/plugins/usage_collection/server/mocks.ts

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

x-pack/plugins/features/server/mocks.ts

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

azasypkin · 2020-02-21T12:42:56Z

x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts

-        deps.features,
-        available
-      );
+      const kibanaIndex = (await deps.kibanaIndexConfig.pipe(take(1)).toPromise()).kibana.index;


optional nit: we can combine license$ and deps.kibanaIndexConfig$ to have just one pipe(take(1)).toPromise(), but it's not a big deal at all. Both approaches has benefits and drawbacks.

x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts

azasypkin · 2020-02-21T14:02:24Z

x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts

+    const isAnonymousRequest = !request.route.options.authRequired;
+
+    if (isAnonymousRequest) {
+      return capabilities;


Yeah, I agree, thanks for explaining!

x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts

legrego · 2020-02-26T15:26:43Z

@azasypkin thanks for your initial feedback - ready for final review!

azasypkin · 2020-02-26T17:35:04Z

ACK: will review tomorrow

pgayvallet

LGTM for platform changes

azasypkin

Looks good!

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

afharo

Pulse changes LGTM!

Thank you for adding the mock!

afharo · 2020-02-27T18:51:42Z

src/plugins/usage_collection/server/mocks.ts

+      logger: loggingServiceMock.createLogger(),
+      maximumWaitTimeForAllCollectorsInS: 1,
+    }),
+    registerLegacySavedObjects: jest.fn() as jest.Mocked<


Thank you for creating the mock for us!

FYI: This method is about to be removed in #58401 😉
Which is about to be merged (waiting for a second review from another Pulse team member to merge it. Sometime today or tomorrow) :)

Thanks for the heads-up. It looks like this will merge before #58401, so would you be ok removing the mock in your PR? I could drop it now, but then I'd have to suppress a TS warning about an incompatible interface, so you'd have to alter this file no matter what :/

Absolutely! It's a simple removal :)

legrego · 2020-02-27T21:59:53Z

@elasticmachine merge upstream

legrego · 2020-02-28T12:08:39Z

@elasticmachine merge upstream

kibanamachine · 2020-02-28T14:06:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3f97fad

History

💔 Build #29571 failed 3ab306f
💛 Build #28864 was flaky 4c792bc
💔 Build #28822 failed 2526b22
💚 Build #26967 succeeded 09cec11
💔 Build #26941 failed 5529529

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

) * remove kibanaIndex from LegacyAPI * moving capabilities, adding tests * moving usage collection * cleanup * don't toggle capabilities on unauthenticated routes * reintroduce exception handling * pipe dat config * start addressing PR feedback * fix CoreSetup's generic type * fix usage collector tests * PR review updates Co-authored-by: Elastic Machine <[email protected]>

* [Telemetry] Report the Application Usage (time of usage + number of clicks) * Add Unit tests to the server side * Do not use optional chaining in JS * Add tests on the public end * Fix jslint errors * jest.useFakeTimers() + jest.clearAllTimers() * Remove Jest timer handlers from my tests (only affecting to a minimum coverage bit) * Catch ES actions in the setup/start steps because it broke core_services tests * Fix boolean check * Use core's ES.adminCLient over .createClient * Fix tests after ES.adminClient * [Telemetry] Application Usage implemented in kbn-analytics * Use bulkCreate in store_report * ApplicationUsagePluginStart does not exist anymore * Fix usage_collection mock interface * Check there is something to store before calling the bulkCreate method * Add unit tests * Fix types in tests * Unit tests for rollTotals and actual fix for the bug found * Fix usage_collection mock after #57693 got merged Co-authored-by: Elastic Machine <[email protected]>

…58401) * [Telemetry] Report the Application Usage (time of usage + number of clicks) * Add Unit tests to the server side * Do not use optional chaining in JS * Add tests on the public end * Fix jslint errors * jest.useFakeTimers() + jest.clearAllTimers() * Remove Jest timer handlers from my tests (only affecting to a minimum coverage bit) * Catch ES actions in the setup/start steps because it broke core_services tests * Fix boolean check * Use core's ES.adminCLient over .createClient * Fix tests after ES.adminClient * [Telemetry] Application Usage implemented in kbn-analytics * Use bulkCreate in store_report * ApplicationUsagePluginStart does not exist anymore * Fix usage_collection mock interface * Check there is something to store before calling the bulkCreate method * Add unit tests * Fix types in tests * Unit tests for rollTotals and actual fix for the bug found * Fix usage_collection mock after elastic#57693 got merged Co-authored-by: Elastic Machine <[email protected]>

…58904) * [Telemetry] Report the Application Usage (time of usage + number of clicks) * Add Unit tests to the server side * Do not use optional chaining in JS * Add tests on the public end * Fix jslint errors * jest.useFakeTimers() + jest.clearAllTimers() * Remove Jest timer handlers from my tests (only affecting to a minimum coverage bit) * Catch ES actions in the setup/start steps because it broke core_services tests * Fix boolean check * Use core's ES.adminCLient over .createClient * Fix tests after ES.adminClient * [Telemetry] Application Usage implemented in kbn-analytics * Use bulkCreate in store_report * ApplicationUsagePluginStart does not exist anymore * Fix usage_collection mock interface * Check there is something to store before calling the bulkCreate method * Add unit tests * Fix types in tests * Unit tests for rollTotals and actual fix for the bug found * Fix usage_collection mock after #57693 got merged Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

…) (#58872) * Spaces - NP updates for usage collection and capabilities (#57693) * remove kibanaIndex from LegacyAPI * moving capabilities, adding tests * moving usage collection * cleanup * don't toggle capabilities on unauthenticated routes * reintroduce exception handling * pipe dat config * start addressing PR feedback * fix CoreSetup's generic type * fix usage collector tests * PR review updates Co-authored-by: Elastic Machine <[email protected]> * fix mock Co-authored-by: Elastic Machine <[email protected]>

legrego added 4 commits February 14, 2020 08:42

remove kibanaIndex from LegacyAPI

8d68b1c

moving capabilities, adding tests

fe13a9a

moving usage collection

33e48e7

cleanup

0dc7685

legrego added Feature:NP Migration Feature:Security/Spaces Platform Security - Spaces feature 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.7.0 v8.0.0 labels Feb 14, 2020

elasticmachine and others added 3 commits February 14, 2020 11:02

Merge branch 'master' into spaces/moar-np

81b18f6

don't toggle capabilities on unauthenticated routes

90c73cb

Merge branch 'spaces/moar-np' of github.com:legrego/kibana into space…

fedbfa1

…s/moar-np

legrego commented Feb 14, 2020

View reviewed changes

legrego added 3 commits February 14, 2020 14:26

reintroduce exception handling

6e5cc69

Merge branch 'master' of github.com:elastic/kibana into spaces/moar-np

5529529

pipe dat config

09cec11

legrego requested a review from azasypkin February 14, 2020 23:46

azasypkin reviewed Feb 21, 2020

View reviewed changes

legrego added 4 commits February 24, 2020 14:05

start addressing PR feedback

993c0bb

fix CoreSetup's generic type

64089ce

Merge branch 'master' of github.com:elastic/kibana into spaces/moar-np

2526b22

fix usage collector tests

4c792bc

legrego marked this pull request as ready for review February 25, 2020 19:08

legrego requested review from a team as code owners February 25, 2020 19:08

pgayvallet approved these changes Feb 27, 2020

View reviewed changes

azasypkin approved these changes Feb 27, 2020

View reviewed changes

x-pack/plugins/features/server/plugin.ts Outdated Show resolved Hide resolved

x-pack/plugins/features/server/plugin.ts Outdated Show resolved Hide resolved

legrego added 2 commits February 27, 2020 13:31

Merge branch 'master' of github.com:elastic/kibana into spaces/moar-np

b6fe106

PR review updates

0003e4d

afharo approved these changes Feb 27, 2020

View reviewed changes

Merge branch 'master' into spaces/moar-np

3ab306f

Merge branch 'master' into spaces/moar-np

3f97fad

legrego merged commit 91330d2 into elastic:master Feb 28, 2020

legrego deleted the spaces/moar-np branch February 28, 2020 14:06

legrego mentioned this pull request Feb 28, 2020

[7.x] Spaces - NP updates for usage collection and capabilities (#57693) #58872

Merged

afharo added a commit to afharo/kibana that referenced this pull request Feb 28, 2020

Fix usage_collection mock after elastic#57693 got merged

c53a740

legrego mentioned this pull request Mar 24, 2020

do not warn when switching capabilities for resources with optional auth #61043

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spaces - NP updates for usage collection and capabilities #57693

Spaces - NP updates for usage collection and capabilities #57693

legrego commented Feb 14, 2020 •

edited

Loading

elasticmachine commented Feb 14, 2020

legrego commented Feb 14, 2020

legrego Feb 14, 2020 •

edited

Loading

azasypkin Feb 21, 2020

azasypkin commented Feb 20, 2020

azasypkin left a comment

azasypkin Feb 21, 2020

azasypkin Feb 21, 2020

legrego commented Feb 26, 2020

azasypkin commented Feb 26, 2020

pgayvallet left a comment

azasypkin left a comment

afharo left a comment

afharo Feb 27, 2020

legrego Feb 27, 2020

afharo Feb 28, 2020

legrego commented Feb 27, 2020

legrego commented Feb 28, 2020

kibanamachine commented Feb 28, 2020

Spaces - NP updates for usage collection and capabilities #57693

Spaces - NP updates for usage collection and capabilities #57693

Conversation

legrego commented Feb 14, 2020 • edited Loading

Summary

Removes legacyConfig.kibanaIndex

Moves uiCapabilities provider

Exposes features plugin start contract

Mocks

elasticmachine commented Feb 14, 2020

legrego commented Feb 14, 2020

legrego Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

azasypkin Feb 21, 2020

Choose a reason for hiding this comment

azasypkin commented Feb 20, 2020

azasypkin left a comment

Choose a reason for hiding this comment

azasypkin Feb 21, 2020

Choose a reason for hiding this comment

azasypkin Feb 21, 2020

Choose a reason for hiding this comment

legrego commented Feb 26, 2020

azasypkin commented Feb 26, 2020

pgayvallet left a comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

afharo Feb 27, 2020

Choose a reason for hiding this comment

legrego Feb 27, 2020

Choose a reason for hiding this comment

afharo Feb 28, 2020

Choose a reason for hiding this comment

legrego commented Feb 27, 2020

legrego commented Feb 28, 2020

kibanamachine commented Feb 28, 2020

💚 Build Succeeded

History

legrego commented Feb 14, 2020 •

edited

Loading

Removes `legacyConfig.kibanaIndex`

Moves `uiCapabilities` provider

Exposes `features` plugin start contract

legrego Feb 14, 2020 •

edited

Loading