[eventLog] search for actions/alerts as hidden saved objects #70395

pmuellr · 2020-07-01T04:01:04Z

resolves #70086

Configures the saved object client for the event log to access the recently
hidden action and alert saved objects.

We didn't have tests for action/alert event log activity, so added some now.

Also found a buglet that was preventing access to event log data from actions
and alerts in non-default spaces.

Also changed the event log query to enforce namespace matches for the saved object references. Technically, not a huge issue right now as the only queryable actions/alerts should be unique by their UUID ids, but when pre-configured actions become queryable, we'd absolutely need this.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-07-03T03:17:12Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

pmuellr · 2020-07-03T03:19:15Z

x-pack/plugins/event_log/server/es/cluster_client_adapter.ts

@@ -22,7 +22,7 @@ export interface QueryEventsBySavedObjectResult {
  page: number;
  per_page: number;
  total: number;
-  data: IEvent[];
+  data: IValidatedEvent[];


IValidatedEvent is a little nicer to use as a result of the query since it's not partial-ized or writable.

pmuellr · 2020-07-03T03:20:02Z

x-pack/plugins/event_log/server/event_log_start_service.ts

+  getClient(request: KibanaRequest) {
+    const savedObjectsClient: SavedObjectsClientContract = this.savedObjectsService.getScopedClient(
+      request,
+      { includedHiddenTypes }


Here's the fix for the hidden types.

pmuellr · 2020-07-03T03:22:30Z

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

@@ -125,8 +125,7 @@ export class Plugin implements CorePlugin<IEventLogService, IEventLogClientServi
  > => {
    return async (context, request) => {
      return {
-        getEventLogClient: () =>
-          this.eventLogClientService!.getClient(request, context.core.savedObjects.client),
+        getEventLogClient: () => this.eventLogClientService!.getClient(request),


Here's the fix for accessing event log for saved objects in non-default spaces. The getClient() call already built a properly scoped SO client, we were passing one in here that didn't work, so just changed to use the one it built instead.

pmuellr · 2020-07-03T03:24:40Z

x-pack/test/alerting_api_integration/common/config.ts

@@ -32,6 +32,7 @@ const enabledActionTypes = [
  'test.index-record',


Remainder of the files are basic tests for event log usage by actions and alerts. They weren't added when event log support was added to actions and alerts, because we didn't have the find function built yet. These tests would have caught the bugs fixed in this PR.

gmmorris

I still need to look through the whole PR, but I'm concerned this isn't going to work once the RBAC work is done- but I also don't want to block the RBAC branch anymore than we have to.
Could we have this PR use the ActionsClient and AlertsClient as a first step and then introduce some kind of pluggable approach in a future PR?

gmmorris · 2020-07-03T09:44:17Z

x-pack/plugins/event_log/server/event_log_start_service.test.ts

+      expect(savedObjectsService.getScopedClient).toHaveBeenCalledWith(request, {
+        includedHiddenTypes: ['action', 'alert'],
+      });


I think this approach means we won't have the alert/actions RBAC on the event log, but only the default SavedObjects RBAC. This means that if the user doesn't have privileges over the action and alert saved object types (as granted by some plugin) this code will throw.
You'll have to explicitly grant privileges via the EventLog feature for this to work.

I think we need an approach where plugins can plug handlers into the Event Log.
For example, the Actions plugin could "tell" the EventLog how to handle action saved object types, and it will use the ActionsClient internally to actually get the object.
That way you'll enjoy RBAC and you won't have to explicitly have to add actions to the EventLog feature's privileges config.

Yeah, that's basically the plan for how to fix issue: #63961

Dang, I guess it means this code is throw away anyway (despite it being some one-liner-ish simple fixes to the existing code). Probably makes sense to fix that issue in this PR as well.

Also forgot that I noticed the event log query should include the namespace, but doesn't currently. I think this isn't critical for 7.9, will take a quick peek at fixing that here, but afraid more tests will be required / existing ones fixed, perhaps better as a separate issue:

kibana/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts

Lines 130 to 135 in 7ec48fd

public async queryEventsBySavedObject(

index: string,

type: string,

id: string,

{ page, per_page: perPage, start, end, sort_field, sort_order }: FindOptionsType

): Promise<QueryEventsBySavedObjectResult> {

I haven't fixed this on the RBAC branch as it's broken on master and that PR is already way too big... but I could make a quick follow up PR that uses the Alerts/Actions clients after that to fix master and that would mean that this PR can be used for the long term solution and doesn't have to be done by 7.9.

What do you think?

I ended up spending 2 days on using the alerts/actionsClient to resolve the saved objects, waaay more complicated than I thought. Going to have to punt on adding that here, which seems fine since RBAC for alerts got pushed as well.

resolves elastic#70086 Configures the saved object client for the event log to access the recently hidden action and alert saved objects. We didn't have tests for action/alert event log activity, so added some now. Also found a buglet that was preventing access to event log data from actions and alerts in non-default spaces.

pmuellr · 2020-07-09T18:36:02Z

x-pack/plugins/event_log/server/es/cluster_client_adapter.ts

-                                  value: SAVED_OBJECT_REL_PRIMARY,
-                                },
-                              },
+    const defaultNamespaceQuery = {


These are the new query DSL clauses to ensure we only return SO's in the relevent space - likely not an issue now, but will be once we can query pre-configured actions - if you could query those now (you can't), the query would return events across all spaces, without these additional clauses.

The rule with namespaces is that they are string | undefined (undefined for the default space), and that's how we "store" them in ES - the SO reference in the event will not have a namespace field for the default space.

pmuellr · 2020-07-09T18:46:28Z

...est/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/email.ts

@@ -11,11 +11,8 @@ import { FtrProviderContext } from '../../../../common/ftr_provider_context';
 // eslint-disable-next-line import/no-default-export
 export default function emailTest({ getService }: FtrProviderContext) {
  const supertest = getService('supertest');
-  const esArchiver = getService('esArchiver');


File this under "how did this ever work?!". esArchiver.unload('empty_kibana')) will delete any existing spaces within Kibana when it runs. We have a bunch of test modules that used this on their own after() processing, but the spaces are created in an outer describe() section with it's own after() section that calls esArchiver.unload(). Which means that any tests run after one of these modules did the unload, would likely fail because the spaces no longer existed. But the tests didn't fail. No idea why. Maybe actions/alerts isn't "touching" the same spaces APIs that event log does (my guess). But the new tests in this PR did fail. So I ended removing all these calls in the "inner" module tests. Everything still passes!

Also note, esArchiver.unload() is an async method, and most/all of our usage didn't wait on it, so ... comedy of errors there :-)

Also note, esArchiver.unload() is an async method, and most/all of our usage didn't wait on it, so ... comedy of errors there :-)

I believe the after function also accepts promises being returned and will wait for them to resolve. I think that's how these have worked before.

pmuellr · 2020-07-09T18:49:56Z

x-pack/test/plugin_api_integration/test_suites/event_log/public_api_integration.ts


-      await logTestEvent(id, expectedEvents[0]);
-      await logTestEvent(id, expectedEvents[1]);
+    for (const namespace of [undefined, 'namespace-a']) {


The tests here previously just tested an event with a SO in the default space, so I refactored all those tests to test with the default space AND a named space. Hence the indentation changes, and parameterization of the namespace. The diff came out much worse than I expected :-)

pmuellr · 2020-07-09T18:53:19Z

Alrighty, this should be ready for review again.

Per Gidi's comment about fixing #63961 (use actions/alerts client to validate SOs instead of the generic SO client), I spent about 2 days trying to get that to work and ... didn't, and it was a bit of a mess. Should be cleaner to do it in a separate PR anyway, without all the other churn here.

While working on that code though, I realized we have issues with SO namespaces, and no tests for them, so that's a large amount of the new code in this PR.

pmuellr · 2020-07-13T15:27:43Z

@elasticmachine merge upstream

mikecote

Changes LGTM 👍 few nits, questions and comments.

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

mikecote · 2020-07-14T15:06:35Z

x-pack/plugins/event_log/server/event_log_start_service.ts

-    )
-  ) {
+  getClient(request: KibanaRequest) {
+    const savedObjectsClient: SavedObjectsClientContract = this.savedObjectsService.getScopedClient(


Will this break again once @gmmorris's RBAC work gets merged or will this skip privilege checks?

Yes it will break by skipping the privilege checks. I've updated the title and a little of the description of #63961, which is where we'll fix this.

I tried to squeeze it in here, but it was getting pretty hairy so I backed out it out. I'll make some notes in that issue on what I tried.

x-pack/plugins/event_log/server/event_log_client.ts

x-pack/plugins/event_log/server/es/cluster_client_adapter.ts

mikecote · 2020-07-14T15:33:13Z

...est/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/email.ts

@@ -11,11 +11,8 @@ import { FtrProviderContext } from '../../../../common/ftr_provider_context';
 // eslint-disable-next-line import/no-default-export
 export default function emailTest({ getService }: FtrProviderContext) {
  const supertest = getService('supertest');
-  const esArchiver = getService('esArchiver');


Also note, esArchiver.unload() is an async method, and most/all of our usage didn't wait on it, so ... comedy of errors there :-)

I believe the after function also accepts promises being returned and will wait for them to resolve. I think that's how these have worked before.

mikecote · 2020-07-14T15:39:00Z

x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/action_types.ts

+  const throwActionType: ActionType = {
+    id: 'test.throw',
+    name: 'Test: Throw',
+    minimumLicenseRequired: 'gold',
+    async executor() {
+      throw new Error('this action is intended to fail');
+    },
+  };


nit: this seems similar to test.failing action type below. I we can merge them, maybe test.failing could be modified to not always require an index param for ease of use.

these are simple enough, and will be reused with the getAlertStatus() PR, so seems better to just keep them separate.

mikecote · 2020-07-14T15:43:42Z

x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/alert_types.ts

+    producer: 'alerting',
+    defaultActionGroupId: 'default',
+    async executor({ services, params, state }: AlertExecutorOptions) {
+      throw new Error('this alert is intended to fail');


Similar comment here with test.failing.

mikecote · 2020-07-14T15:51:19Z

x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/alert_types.ts

+  const patternFiringAlertType: AlertType = {
+    id: 'test.patternFiring',
+    name: 'Test: Firing on a Pattern',
+    actionGroups: [{ id: 'default', name: 'Default' }],
+    producer: 'alerting',
+    defaultActionGroupId: 'default',
+    async executor(alertExecutorOptions: AlertExecutorOptions) {
+      const { services, state, params } = alertExecutorOptions;
+      const pattern = params.pattern;
+      if (!Array.isArray(pattern)) throw new Error('pattern is not an array');
+      if (pattern.length === 0) throw new Error('pattern is empty');
+
+      // get the pattern index, return if past it
+      const patternIndex = state.patternIndex ?? 0;
+      if (patternIndex > pattern.length) {
+        return { patternIndex };
+      }
+
+      // fire if pattern says to
+      if (pattern[patternIndex]) {
+        services.alertInstanceFactory('instance').scheduleActions('default');
+      }
+
+      return {
+        patternIndex: (patternIndex + 1) % pattern.length,
+      };
+    },
+  };


This seems similar to test.cumulative-firing?

Nah. This alert takes a "pattern" of booleans that indicate if it should fire on specific executions. Eg, [false, true, false, true] means on the 1s and 3rd executions it will schedule actions, but on the 2nd and 4th it will. I realized I was going to need something like this to be able to test the new-instance / resolved-instance events, in the getAlertStatus() PR, so I did a first pass here (getAlertStatus() will require per-instance patterns, or something :-) ).

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/event_log.ts

pmuellr · 2020-07-15T05:10:26Z

@elasticmachine merge upstream

gmmorris

I tried playing around with the API locally and it seems to be working as expected.
I'd feel more confident if we also had a test suite under the security_and_spaces package, so we know that security isn't affecting this somehow, but I guess that can wait until we address the pluggable behaviour and use the ActionsClient and AlertsClient.

gmmorris · 2020-07-15T09:43:04Z

x-pack/plugins/event_log/server/routes/find.ts

+      try {
+        result = await eventLogClient.findEventsBySavedObject(type, id, query);
+      } catch (err) {
+        return res.notFound();


I think we should log the err as we might be swallowing some failure state here

gmmorris · 2020-07-15T09:44:58Z

x-pack/plugins/event_log/server/routes/find.ts

+
+      let result: QueryEventsBySavedObjectResult;
+      try {
+        result = await eventLogClient.findEventsBySavedObject(type, id, query);


nit. but as we're returning from the catch I'd personally find it more readable if we did this:

Suggested change

result = await eventLogClient.findEventsBySavedObject(type, id, query);

return res.ok({

body: await eventLogClient.findEventsBySavedObject(type, id, query)

});

Super subjective and nitty :)

gmmorris · 2020-07-15T09:50:52Z

x-pack/test/alerting_api_integration/common/lib/get_event_log.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+// import { IEvent } from '../../../../../../plugins/event_log/server';


an abandoned piece of code :)

gmmorris · 2020-07-15T10:05:48Z

x-pack/test/plugin_api_integration/test_suites/event_log/public_api_integration.ts


-      await logTestEvent(id, expectedEvents[0]);
-      await logTestEvent(id, expectedEvents[1]);
+    for (const namespace of [undefined, 'namespace-a']) {


nit.

Could we use some kind of const here instead of just undefined?

something like:

const DEFAULT_NAMESPACE = undefined; for (const namespace of [DEFAULT_NAMESPACE, 'namespace-a']) { const namespaceName = namespace === DEFAULT_NAMESPACE ? 'default' : namespace; /// ...

There's a bunch of places in this test where we use undefined to refer to the default namespace and I think, for ease of maintenance for future developers, that using a const when undefined means the default NS will make life a little easier.

hmm, I just tried that, but felt like keeping the definition of namespace to string | undefined still makes sense, since that's what it looks like in the non-test code. So then changing the constant values besides that to a defined constant looked really weird:

function urlPrefixFromNamespace(namespace: string | undefined): string { if (namespace === DEFAULT_NAMESPACE) return ''; return `/s/${namespace}`; }

Feels better to just leave it as undefined to me. There were only 4 references to the constant value outside the type definition anyway.

pmuellr · 2020-07-15T14:05:09Z

from @gmmorris in #70395 (review)

I'd feel more confident if we also had a test suite under the security_and_spaces package, so we know that security isn't affecting this somehow, but I guess that can wait until we address the pluggable behaviour and use the ActionsClient and AlertsClient.

Same same - I'll see if I can squeeze something into security_and_spaces for now, but for sure the pluggable SO lookup/validation will need some significant s+s tests.

…er merge

pmuellr · 2020-07-16T05:56:50Z

@elasticmachine merge upstream

kibanamachine · 2020-07-16T07:37:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 13a4685

Build metrics

✅ unchanged

History

💔 Build #62087 failed 47301c9
💔 Build #62056 failed 8df8d44
💔 Build #61986 failed d1ab30e
💚 Build #61802 succeeded 4387f29
💛 Build #61733 was flaky b47704b

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

…#70395) resolves elastic#70086 Configures the saved object client for the event log to access the recently hidden action and alert saved objects. We didn't have tests for action/alert event log activity, so added some now. Also found a buglet that was preventing access to event log data from actions and alerts in non-default spaces.

…#72050) resolves #70086 Configures the saved object client for the event log to access the recently hidden action and alert saved objects. We didn't have tests for action/alert event log activity, so added some now. Also found a buglet that was preventing access to event log data from actions and alerts in non-default spaces.

…#72049) resolves #70086 Configures the saved object client for the event log to access the recently hidden action and alert saved objects. We didn't have tests for action/alert event log activity, so added some now. Also found a buglet that was preventing access to event log data from actions and alerts in non-default spaces.

pmuellr force-pushed the eventLog/hidden-saved-objects branch 3 times, most recently from 53fe564 to 6397143 Compare July 3, 2020 03:14

pmuellr added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.9.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jul 3, 2020

pmuellr marked this pull request as ready for review July 3, 2020 03:17

pmuellr requested a review from a team as a code owner July 3, 2020 03:17

pmuellr commented Jul 3, 2020

View reviewed changes

gmmorris reviewed Jul 3, 2020

View reviewed changes

pmuellr force-pushed the eventLog/hidden-saved-objects branch 8 times, most recently from e8fc10f to e4c91b3 Compare July 9, 2020 15:48

pmuellr force-pushed the eventLog/hidden-saved-objects branch from e4c91b3 to 8957be7 Compare July 9, 2020 18:15

pmuellr commented Jul 9, 2020

View reviewed changes

Merge branch 'master' into eventLog/hidden-saved-objects

3e53625

mikecote self-requested a review July 14, 2020 14:57

mikecote approved these changes Jul 14, 2020

View reviewed changes

pmuellr added 2 commits July 14, 2020 19:38

address PR review comments

b47704b

fix merge conflict

03f746b

Merge branch 'master' into eventLog/hidden-saved-objects

4387f29

gmmorris approved these changes Jul 15, 2020

View reviewed changes

mikecote mentioned this pull request Jul 15, 2020

[Alerting] event log should authenticate actions/alerts with their clients #63961

Closed

pmuellr added 3 commits July 15, 2020 12:33

apply fixes from Gidi's PR review

d1ab30e

add some FT for security_and_spaces per PR review

8df8d44

remove another esArchiver.unload('empty_kibana') that crept in a mast…

47301c9

…er merge

Merge branch 'master' into eventLog/hidden-saved-objects

13a4685

pmuellr merged commit b167d77 into elastic:master Jul 16, 2020

pmuellr mentioned this pull request Jul 16, 2020

[7.x] [eventLog] search for actions/alerts as hidden saved objects (#70395) #72049

Merged

pmuellr mentioned this pull request Jul 16, 2020

[7.9] [eventLog] search for actions/alerts as hidden saved objects (#70395) #72050

Merged

pmuellr added the backported label Jul 16, 2020

		@@ -32,6 +32,7 @@ const enabledActionTypes = [
		'test.index-record',

	public async queryEventsBySavedObject(
	index: string,
	type: string,
	id: string,
	{ page, per_page: perPage, start, end, sort_field, sort_order }: FindOptionsType
	): Promise<QueryEventsBySavedObjectResult> {

-        result = await eventLogClient.findEventsBySavedObject(type, id, query);
+        return res.ok({
+           body: await eventLogClient.findEventsBySavedObject(type, id, query)
+        });

[eventLog] search for actions/alerts as hidden saved objects #70395

[eventLog] search for actions/alerts as hidden saved objects #70395

Conversation

pmuellr commented Jul 1, 2020 • edited Loading

Checklist

elasticmachine commented Jul 3, 2020

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

gmmorris 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

pmuellr commented Jul 9, 2020

pmuellr commented Jul 13, 2020

mikecote 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

pmuellr commented Jul 15, 2020

gmmorris 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

pmuellr Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

pmuellr commented Jul 15, 2020

pmuellr commented Jul 16, 2020

kibanamachine commented Jul 16, 2020

💚 Build Succeeded

Build metrics

History

pmuellr commented Jul 1, 2020 •

edited

Loading

pmuellr Jul 15, 2020 •

edited

Loading