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

Pass name down alertName, spaceId, tags, and anything else of interest in the executor #50522

Closed
FrankHassanabad opened this issue Nov 13, 2019 · 5 comments · Fixed by #54035
Closed
Assignees
Labels
Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.6.0

Comments

@FrankHassanabad
Copy link
Contributor

FrankHassanabad commented Nov 13, 2019

Describe the feature:

For alerts right now you can pass in alertId, services, and params:

async executor({ alertId, services, params }) {

It would be great if other data such as alertName is passed:

async executor({ alertId, alertName, services, params }) {

Workaround at the moment is an additional roundtrip to the server like so:

async executor({ alertId, services, params }) {
  const savedObject = await services.savedObjectsClient.get('alert', alertId);
  const name = savedObject.attributes.name;
  const tags = savedObject.attributes.tags;
}

Describe a specific use case for the feature:

Use case is right now on signals/detection engine we take the parameters and the name of the alert and push it into our signals index so that the UI which shows signals does not have to do a lookup of the alertName when the signal for the index was generated.

Other additional items to pass down that we will need for sure is:

  • Existing Space ID that the alert is executing in
  • Use ID and User permissions the alert is executing in

Use case is right now on signals/detection engine we need to detect if the user has exported and re-imported rules from one space to another one without a signals index created for that particular space.

Another one that might be useful but we don't need it right now on SIEM is the userID and current level of permissions of the user as a nice to have for if we encounter permissions issues of the user that created the alert we need to log that information out or surface the error back to the UI to trouble shoot why they're creating signal rules without correct permissions. Right now we are going to try to stop these issues at the UI level before the user can create the rules but there will always be aways we can't anticipate where they could change permissions later on which will cause problems for trouble shooting without logging more of the messages.

In the meantime we will just catch any permission errors and log those so this last item is not a big issue for us at the moment.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services (Team:Stack Services)

@mikecote mikecote added this to Backlog in Make it Action Nov 13, 2019
@mikecote mikecote moved this from Backlog to Mid Term in Make it Action Dec 6, 2019
@FrankHassanabad FrankHassanabad changed the title Pass name down alertName in the executor Pass name down alertName, spaceId, and anything else of interest in the executor Dec 6, 2019
@pmuellr
Copy link
Member

pmuellr commented Dec 6, 2019

alertName, user<name|id|???>, and (name?)space(id?) all seem useful and safe, we'll just need to settle on the names and make sure we can actually provide these.

User permissions I'm less sure of. I'm not sure what shape that would even be, but I worry more about the safety. Since alerts are run with API keys, and those are snapshots of permissions when the API key was created, we would be exposing a user's permissions, which could then leak, and be visible to other users. Doesn't feel right to me.

trouble shoot why they're creating signal rules without correct permissions

I guess the problem here is that the API key, at the time of the alert function execution, may have different permissions than the user currently has, and so it would be a confusing situation that they'd look at their permissions "I have the permission I need" but then have the alert fail with permissions based on an older value of the permissions.

That seems like a general API key issue, and we do have some APIs to help out there (refresh an API key in an alert), and I guess we should doc this potentially confusing behavior in some fashion.

@FrankHassanabad
Copy link
Contributor Author

We're not needing the user permissions for any reason, it was just a "while you're in there" suggestion, but I agree let's not push it down if not needed.

These on your suggested list will be enough for us at the moment:

alertName, space-id

The spaceId we are using at the moment is the one you get back from plugins:

const spaceId = server.plugins.spaces.getSpaceId(request);

where it ends up being default if nothing is specified otherwise it is the urlId if that helps.

@FrankHassanabad FrankHassanabad changed the title Pass name down alertName, spaceId, and anything else of interest in the executor Pass name down alertName, spaceId, tags, and anything else of interest in the executor Dec 9, 2019
@bmcconaghy bmcconaghy added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) and removed Team:Stack Services labels Dec 12, 2019
@mikecote mikecote moved this from Mid Term to To-Do 7.6 in Make it Action Dec 16, 2019
@pmuellr
Copy link
Member

pmuellr commented Jan 3, 2020

Right now, the invocation of the alert executor is handled here:

const executionHandler = createExecutionHandler({
alertId,
logger,
executeAction,
apiKey,
actions: actionsWithIds,
spaceId,
alertType,
});

I'm not sure if we should try to organize this a bit more before sending it on, or just add the new properties. I'll try a few things, but I think first pass we'll just keep the current structure, and add more ...

@pmuellr
Copy link
Member

pmuellr commented Jan 3, 2020

Right now, it's pretty easy to add the following fields to the execution params object:

  namespace: string | undefined;
  name: string;
  tags: string[];
  createdBy: string | null;
  updatedBy: string | null;

The alt-typings come from the source (eg, namespace, createdBy, and updatedBy), but I guess these all make some sense given the options of running Kibana.

The entire Alert object is below. I was wondering if perhaps it would just make sense to pass down the entire alert object as an alert property, but that doesn't feel quite right.

export interface Alert {
enabled: boolean;
name: string;
tags: string[];
alertTypeId: string;
consumer: string;
schedule: IntervalSchedule;
actions: AlertAction[];
params: Record<string, any>;
scheduledTaskId?: string;
createdBy: string | null;
updatedBy: string | null;
createdAt: Date;
updatedAt: Date;
apiKey: string | null;
apiKeyOwner: string | null;
throttle: string | null;
muteAll: boolean;
mutedInstanceIds: string[];
}

pmuellr added a commit to pmuellr/kibana that referenced this issue Jan 3, 2020
resolves elastic#50522

There is a soon-to-land PR that changes the files quite a bit
here, but figured worth a commit to know what to do when those PRs
land. PR soon-to-land: elastic#53688
@peterschretlen peterschretlen moved this from To-Do 7.6 to In Progress in Make it Action Jan 6, 2020
pmuellr added a commit to pmuellr/kibana that referenced this issue Jan 6, 2020
resolves elastic#50522

The alert executor function is now passed these additional alert-specific
properties as parameters:

- spaceId
- namespace
- name
- tags
- createdBy
- updatedBy
pmuellr added a commit that referenced this issue Jan 9, 2020
resolves #50522

The alert executor function is now passed these additional alert-specific
properties as parameters:

- spaceId
- namespace
- name
- tags
- createdBy
- updatedBy
pmuellr added a commit to pmuellr/kibana that referenced this issue Jan 9, 2020
resolves elastic#50522

The alert executor function is now passed these additional alert-specific
properties as parameters:

- spaceId
- namespace
- name
- tags
- createdBy
- updatedBy
pmuellr added a commit that referenced this issue Jan 10, 2020
resolves #50522

The alert executor function is now passed these additional alert-specific
properties as parameters:

- spaceId
- namespace
- name
- tags
- createdBy
- updatedBy
@pmuellr pmuellr moved this from In Progress to Done (Ordered by most recent) in Make it Action Jan 10, 2020
thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this issue Jan 12, 2020
resolves elastic#50522

The alert executor function is now passed these additional alert-specific
properties as parameters:

- spaceId
- namespace
- name
- tags
- createdBy
- updatedBy
@kobelb kobelb added the needs-team Issues missing a team label label Jan 31, 2022
@botelastic botelastic bot removed the needs-team Issues missing a team label label Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.6.0
Projects
No open projects
Make it Action
  
Done (Ordered by most recent)
Development

Successfully merging a pull request may close this issue.

6 participants