Make alerting properly space aware #42081

mikecote · 2019-07-26T17:22:40Z

Extracted from #41389. This PR makes alerting properly space aware by capturing spaceId and passing it along anywhere that is needed.

elasticmachine · 2019-07-26T17:22:43Z

Pinging @elastic/kibana-stack-services

mikecote · 2019-07-29T13:29:34Z

Pending on #42133.

pmuellr

looking good to me so far, made a few comments

x-pack/legacy/plugins/actions/README.md

x-pack/legacy/plugins/actions/server/builtin_action_types/email.test.ts

x-pack/test/api_integration/apis/alerting/update.ts

elasticmachine · 2019-07-31T04:41:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…e-aware

elasticmachine · 2019-07-31T14:46:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

pmuellr

lgtm

bmcconaghy

Code LGTM, just some minor comments.

x-pack/legacy/plugins/actions/server/action_type_registry.ts

x-pack/legacy/plugins/actions/server/builtin_action_types/email.test.ts

x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts

x-pack/legacy/plugins/actions/server/init.ts

legrego · 2019-07-31T17:36:28Z

x-pack/legacy/plugins/actions/server/init.ts

@@ -75,6 +81,9 @@ export function init(server: Legacy.Server) {
  const fireFn = createFireFunction({


fireFn, which we expose via server.expose, allows consumers to specify their own space id. Do we know of any scenarios where consumers will need this capability? If not, we could potentially create a "scoped" fire function, where the space id is derived from the current request, or similar.

If we do need to allow consumers to specify their own space id, then you'll likely want to update this so that it can handle spaces being disabled, and handle cases where a space id is required, but missing. Currently, spaceIdToNamespace will throw an error if a space id is not provided.

Also, the underlying fireFn uses callWithInternalUser in an unguarded fashion, so there isn't any authorization being applied there. This would be less of a concern if it wasn't exposed to consumers, but since this is a consumer-facing function, we should have protections in place there. (perhaps out of scope for this PR?)

From what I can tell, consumers will need to specify their own space id. The example I can think of is task manager wants to fire action a in space b and uses this function to indicate that. We should be ok when spaces is disabled due to spaceIdToNamespace being passed as https://github.com/elastic/kibana/pull/42081/files#diff-c2af122155b3536ee79e8b5384fdaa6eR94.

In regards to callWithInternalUser being unguarded, we'll have this fixed when API keys are implemented. We will do the callWithInternalUser first, setup a connection with the keys and re-load the same object via the saved objects client.

x-pack/legacy/plugins/actions/server/init.ts

elasticmachine · 2019-07-31T20:48:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mikecote · 2019-08-02T12:26:20Z

retest

legrego

LGTM - thanks for the changes, Mike!

mikecote · 2019-08-02T12:34:02Z

As discussed with @kobelb, it would be great to have high-level tests to ensure denied access to spaces when users don't have access. But since this requires API keys to work, we will defer the tests for the PR implementing API keys. Right now alerting doesn't support security enabled in kibana and is pending API keys to support it.

elasticmachine · 2019-08-02T13:23:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Make alerting properly space aware * Fix broken jest tests * Fix broken integration test * Cleanup pt1 * Add spaces integration tests * Fix type check failure * Apply PR feedback pt1 * Use TS types from spaces plugin * Fix broken tests * Apply PR feedback pt1 * Fix getBasePath to return server.basePath when spaces is disabled * Apply new API changes to tests

…-or-edit-existing-rollup-job * 'master' of github.com:elastic/kibana: (67 commits) [TSVB] Shim new platform (elastic#39169) [Metric Vis] Shim new platform (elastic#42240) [Tag Cloud] Shim new platform (elastic#42348) Disable flaky request lib tests. Add es_ui_shared plugin to CODEOWNERS. Add disk space percentage to node listing (elastic#42145) [SIEM] Add chart interactions - update date picker after brush selection on charts (elastic#42440) Document HTTP service (elastic#42331) [Reporting] Sanitize 409 error log message (elastic#42495) [docs][skip ci] Maps read only access (elastic#35561) [x-pack/ftr] refactor types to be more accurate/consistent wit… (elastic#42407) [DOCS] Updates images and content in Dashboard docs (elastic#42500) Allow sorting on multiple columns in Discover (elastic#41918) [Infra UI][Logs UI] Fix autocomplete to use proper derived index pattern (elastic#42287) [ftr/cheerio] improve cheerio types to include test subject me… (elastic#42534) Upgraded EUI 13.0.0 -> 13.1.1 (elastic#42298) Increase max-old-space-size for builds (elastic#42218) [Infra UI] Add cloud metrics and cloud/host info to metadata endpoint (elastic#41836) [Logs UI][a11y] Announce name of column on remove column button (elastic#41695) Inspector 👉 New Platform (elastic#42164) Make alerting properly space aware (elastic#42081) ...

…s_autocomplete * 'master' of github.com:elastic/kibana: (189 commits) [TSVB] Shim new platform (elastic#39169) [Metric Vis] Shim new platform (elastic#42240) [Tag Cloud] Shim new platform (elastic#42348) Disable flaky request lib tests. Add es_ui_shared plugin to CODEOWNERS. Add disk space percentage to node listing (elastic#42145) [SIEM] Add chart interactions - update date picker after brush selection on charts (elastic#42440) Document HTTP service (elastic#42331) [Reporting] Sanitize 409 error log message (elastic#42495) [docs][skip ci] Maps read only access (elastic#35561) [x-pack/ftr] refactor types to be more accurate/consistent wit… (elastic#42407) [DOCS] Updates images and content in Dashboard docs (elastic#42500) Allow sorting on multiple columns in Discover (elastic#41918) [Infra UI][Logs UI] Fix autocomplete to use proper derived index pattern (elastic#42287) [ftr/cheerio] improve cheerio types to include test subject me… (elastic#42534) Upgraded EUI 13.0.0 -> 13.1.1 (elastic#42298) Increase max-old-space-size for builds (elastic#42218) [Infra UI] Add cloud metrics and cloud/host info to metadata endpoint (elastic#41836) [Logs UI][a11y] Announce name of column on remove column button (elastic#41695) Inspector 👉 New Platform (elastic#42164) Make alerting properly space aware (elastic#42081) ...

cachedout · 2019-08-14T09:47:14Z

Hi @mikecote I see that there were updates to the README for Actions in this PR and those make perfect sense but it's a little unclear to me after reading this whether there are also changes to the security model for the Alerting plugin.

Could you help me understand if that plugin is also Spaces-aware and if so, whether there are changes needed for a user of the Alerting plugin?

mikecote · 2019-08-14T12:29:18Z

Hi @cachedout,

tl;dr: no changes required.

Yes both plugins alerting and actions are space aware end to end with this PR. There isn't any changes required from your end. It's a bit hidden to the consumer just like regular saved objects in the system.

One thing you may notice between alerts and actions is we currently don't validate the actions array when creating / editing an alert. There's a task to implement that once security enabled is supported (#41389). That PR will also change the savedObjectsClient & callCluster provided to the executors to be scoped to the user who created the alert.

mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Stack Services v7.4.0 labels Jul 26, 2019

mikecote self-assigned this Jul 26, 2019

mikecote mentioned this pull request Jul 26, 2019

Add security support for alerts and actions #41389

Merged

This comment has been minimized.

Sign in to view

mikecote mentioned this pull request Jul 29, 2019

Expose spaces plugin function signatures in TypeScript #42133

Merged

mikecote added 4 commits July 30, 2019 09:44

Make alerting properly space aware

f10f3ac

Fix broken jest tests

a8af421

Fix broken integration test

5f0f48a

Cleanup pt1

d3040ba

mikecote force-pushed the alerting/space-aware branch from 09f5f51 to d3040ba Compare July 30, 2019 13:44

This comment has been minimized.

Sign in to view

Add spaces integration tests

e1bff5c

This comment has been minimized.

Sign in to view

Fix type check failure

4ddfea8

This comment has been minimized.

Sign in to view

pmuellr reviewed Jul 31, 2019

View reviewed changes

x-pack/legacy/plugins/actions/README.md Outdated Show resolved Hide resolved

x-pack/legacy/plugins/actions/server/builtin_action_types/email.test.ts Show resolved Hide resolved

x-pack/test/api_integration/apis/alerting/update.ts Outdated Show resolved Hide resolved

mikecote added 3 commits July 30, 2019 22:53

Merge with master

b7c9d2a

Apply PR feedback pt1

278f57e

Use TS types from spaces plugin

896b6a4

mikecote added 2 commits July 31, 2019 09:41

Merge branch 'master' of github.com:elastic/kibana into alerting/spac…

1718df8

…e-aware

Fix broken tests

bca3f0e

mikecote marked this pull request as ready for review July 31, 2019 13:50

mikecote requested a review from a team July 31, 2019 13:50

mikecote added the review label Jul 31, 2019

mikecote requested a review from legrego July 31, 2019 14:07

pmuellr approved these changes Jul 31, 2019

View reviewed changes

bmcconaghy approved these changes Jul 31, 2019

View reviewed changes

x-pack/legacy/plugins/actions/server/action_type_registry.ts Show resolved Hide resolved

x-pack/legacy/plugins/actions/server/action_type_registry.ts Show resolved Hide resolved

x-pack/legacy/plugins/actions/server/builtin_action_types/email.test.ts Show resolved Hide resolved

legrego reviewed Jul 31, 2019

View reviewed changes

Apply PR feedback pt1

76c0c99

legrego reviewed Jul 31, 2019

View reviewed changes

x-pack/legacy/plugins/actions/server/init.ts Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Fix getBasePath to return server.basePath when spaces is disabled

06c7334

mikecote added 2 commits August 1, 2019 14:41

Merge wtih master

37bdaf1

Apply new API changes to tests

9837c7c

legrego approved these changes Aug 2, 2019

View reviewed changes

mikecote merged commit b94e94e into elastic:master Aug 2, 2019

mikecote mentioned this pull request Aug 2, 2019

[7.x] Make alerting properly space aware (#42081) #42525

Merged

mikecote mentioned this pull request Sep 11, 2019

Create unit test for x-pack/legacy/plugins/actions/server/builtin_action_types/index.ts and move common code out of each action type. #45444

Closed

njd5475 mentioned this pull request Sep 13, 2019

Alerting and action services #24214

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make alerting properly space aware #42081

Make alerting properly space aware #42081

mikecote commented Jul 26, 2019

elasticmachine commented Jul 26, 2019

This comment has been minimized.

This comment has been minimized.

mikecote commented Jul 29, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pmuellr left a comment

elasticmachine commented Jul 31, 2019

elasticmachine commented Jul 31, 2019

pmuellr left a comment

bmcconaghy left a comment

legrego Jul 31, 2019

mikecote Jul 31, 2019

This comment has been minimized.

elasticmachine commented Jul 31, 2019

mikecote commented Aug 2, 2019

legrego left a comment

mikecote commented Aug 2, 2019

elasticmachine commented Aug 2, 2019

cachedout commented Aug 14, 2019

mikecote commented Aug 14, 2019

		@@ -75,6 +81,9 @@ export function init(server: Legacy.Server) {
		const fireFn = createFireFunction({

Make alerting properly space aware #42081

Make alerting properly space aware #42081

Conversation

mikecote commented Jul 26, 2019

elasticmachine commented Jul 26, 2019

This comment has been minimized.

This comment has been minimized.

mikecote commented Jul 29, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pmuellr left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 31, 2019

💔 Build Failed

elasticmachine commented Jul 31, 2019

💚 Build Succeeded

pmuellr left a comment

Choose a reason for hiding this comment

bmcconaghy left a comment

Choose a reason for hiding this comment

legrego Jul 31, 2019

Choose a reason for hiding this comment

mikecote Jul 31, 2019

Choose a reason for hiding this comment

This comment has been minimized.

elasticmachine commented Jul 31, 2019

💚 Build Succeeded

mikecote commented Aug 2, 2019

legrego left a comment

Choose a reason for hiding this comment

mikecote commented Aug 2, 2019

elasticmachine commented Aug 2, 2019

💚 Build Succeeded

cachedout commented Aug 14, 2019

mikecote commented Aug 14, 2019