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

Add alerting route to update API key #45925

Merged
merged 10 commits into from
Sep 20, 2019

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Sep 17, 2019

Fixes #45926.

In this PR, I'm adding an API to update an alert's API key. This API will generate a new API key in Elasticsearch that is tied to the user in context and assign it to the alert. This will make the alert use the privileges from that user afterwards.

@mikecote mikecote requested review from pmuellr, bmcconaghy and a team September 17, 2019 17:48
@mikecote mikecote self-assigned this Sep 17, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just had a couple of questions.

x-pack/legacy/plugins/alerting/server/alerts_client.ts Outdated Show resolved Hide resolved

const apiKey = await this.createAPIKey();
const username = await this.getUserName();
await this.savedObjectsClient.update(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So wondering here if we also want to invalidate the API key that was there previously. Seems like we're just going to leave behind a trail of unused but still valid API keys.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YES, we should definitely do that. Although, is there some chance an old one could be available - like cached on a different Kibana instance? Seems possible. Which would imply so check to see when using an api key if we get back a "key not found" response, and use that as a cache invaldation signal (re-read the key).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a card for api key cleanup (https://github.com/elastic/kibana/projects/26#card-25354998). The problem that arises is we don't know if any scheduled actions rely on the old api key. To solve this we would need to change a few things and we were planning to do so under that card.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it's good to go, once the questions about updating a disabled alert and deleting old keys are resolved. Adding key deletion perhaps would need another review, but a new issue to resolve it works for me. If we get rid of the constraint of not being able to update a disabled alert, I think that's just deleting some of this code.

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mikecote mikecote merged commit eb8b5ed into elastic:master Sep 20, 2019
mikecote added a commit to mikecote/kibana that referenced this pull request Sep 20, 2019
* Add route to update alert api key

* Allow updating an API key when the alert is disabled

* Fix typecheck failures

* Fix broken file references
mikecote added a commit that referenced this pull request Sep 23, 2019
* Add route to update alert api key

* Allow updating an API key when the alert is disabled

* Fix typecheck failures

* Fix broken file references
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes review v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to re-generate Elasticsearch API keys for an alert (using the user in context / doing the request).
4 participants