Add support for custom alert ids #89814

mikecote · 2021-01-30T01:40:49Z

Resolves #50210

In this PR, I'm adding support for custom uuid v1/v4 ids to the AlertsClient as well as the create HTTP API.

Example of new route option:
POST /api/alerts/alert/5031f8f0-629a-11eb-b500-d1931a8e5df7 will now use 5031f8f0-629a-11eb-b500-d1931a8e5df7 as the alert id and return 409 if a document with this id already exists.

Notes for doc

Document the limitation of ids that can be accepted including the new optional /{id?} addition to the API.

elasticmachine · 2021-01-30T01:44:58Z

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

…om-ids

mikecote · 2021-02-01T13:53:23Z

...ugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts

-      throw new Error(
-        'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
+      throw Boom.badRequest(
+        'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.'


@elastic/kibana-security, I thought of making the error message more generic when it is returned by an HTTP call (ex: when creating an alert). Let me know your thoughts?

I'm fine with the text change. Is this something you expect end users to organically encounter in the UI?

@legrego

Is this something you expect end users to organically encounter in the UI?

I don't think UI but I can see 3rd party developers/users encountering this when calling our HTTP API. I wasn't sure if mentioning SavedObjectsUtils.generateId in the error message would make sense to them or if it was better to say any UUID? I'm good either way.

I think your text change makes sense, and I have no problems with API consumers seeing this updated message. I just wanted to make sure the "normal" UI-based experience wouldn't encounter this 🙂

Awesome, good to know 🙂 I can't guarantee what solutions will build on top, but I agree this shouldn't be encountered in the UIs.

legrego · 2021-02-01T14:34:03Z

...ugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts

@@ -323,8 +324,8 @@ function getValidId(
    // and not being used to get around the specified ID limitation
    const canSpecifyID = (overwrite && version) || SavedObjectsUtils.isRandomId(id);
    if (!canSpecifyID) {
-      throw new Error(
-        'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
+      throw Boom.badRequest(


Rather than directly coupling to Boom, can we instead throw a Saved Objects Error? If you pull this function up into the class, then you'll have access to errors, so you can do something like:

throw this.errors.createBadRequestError('Predefined IDs are not allowed...')

Good idea, I went ahead and fixed that within c79ff9b.

legrego · 2021-02-01T14:34:59Z

...ugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts

-      throw new Error(
-        'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
+      throw Boom.badRequest(
+        'Predefined IDs are not allowed for saved objects with encrypted attributes unless the ID is a UUID.'


I'm fine with the text change. Is this something you expect end users to organically encounter in the UI?

pmuellr · 2021-02-01T14:52:29Z

I didn't see any explicit uuid validation in our code, so I assume the SO client enforces that the id is a suitable UUID? For some reason, I didn't think those were restricted to be UUIDs - I can see that if I create an index pattern, it allows you to provide an id, and then uses whatever you give it. Eg, I just created an index pattern with id custom-index-pattern-id-here and I can see the doc's _id is index-pattern:custom-index-pattern-id-here. So I'm curious how this works, and if we actually need to conform to the uuid format.

mikecote · 2021-02-01T15:05:51Z

@pmuellr

I didn't see any explicit uuid validation in our code, so I assume the SO client enforces that the id is a suitable UUID?

The enforcement comes from the ESO plugin to ensure the ids are long enough for AAD and not as easy to guess.

See (+ fn comment above): https://github.com/mikecote/kibana/blob/alerting/custom-ids/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts#L325

pmuellr · 2021-02-01T15:10:04Z

The enforcement comes from the ESO plugin to ensure the ids are long enough for AAD and not as easy to guess.

Ah, makes sense. I guess we should doc that, which makes me realize we need a doc update anyway for the new id param.

pmuellr

LGTM, but noted in a non-review comment that we need to update the docs, and should mention the UUID-conformance validation on that field.

mikecote · 2021-02-01T15:14:50Z

@pmuellr agreed, I left a needs_docs label and updated the description 👍 Thank you for the review!

YulNaumenko

LGTM

kibanamachine · 2021-02-01T16:53:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c79ff9b

Metrics [docs]

✅ unchanged

History

💚 Build #102567 succeeded 7588e57
💚 Build #102414 succeeded 2339b1f

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

* Add support for custom alert ids * UUID v4 also supported * Change ESO custom id error message * Update api integration test * Use errors.createBadRequestError

Add support for custom alert ids

2339b1f

mikecote added release_note:enhancement Feature:Alerting v8.0.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.12.0 labels Jan 30, 2021

mikecote self-assigned this Jan 30, 2021

mikecote mentioned this pull request Jan 30, 2021

Ability to specify custom id for alerts and actions #50210

Closed

mikecote marked this pull request as ready for review January 30, 2021 01:44

mikecote requested review from a team as code owners January 30, 2021 01:44

mikecote added the needs_docs label Jan 30, 2021

mikecote added 4 commits February 1, 2021 08:36

UUID v4 also supported

ba89911

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

fffb0a6

…om-ids

Change ESO custom id error message

8270267

Update api integration test

7588e57

mikecote commented Feb 1, 2021

View reviewed changes

legrego reviewed Feb 1, 2021

View reviewed changes

Use errors.createBadRequestError

c79ff9b

pmuellr approved these changes Feb 1, 2021

View reviewed changes

mikecote requested a review from legrego February 1, 2021 15:13

YulNaumenko approved these changes Feb 1, 2021

View reviewed changes

legrego approved these changes Feb 1, 2021

View reviewed changes

mikecote merged commit 51cfa90 into elastic:master Feb 1, 2021

mikecote mentioned this pull request Feb 1, 2021

[7.x] Add support for custom alert ids (#89814) #89911

Merged

mikecote added a commit that referenced this pull request Feb 2, 2021

Add support for custom alert ids (#89814) (#89911)

2273d9e

* Add support for custom alert ids * UUID v4 also supported * Change ESO custom id error message * Update api integration test * Use errors.createBadRequestError

mikecote mentioned this pull request Feb 23, 2021

Add docs for ability to specify custom alert ids #92410

Merged

mikecote removed the needs_docs label Feb 24, 2021

spong mentioned this pull request May 5, 2021

Import / export of rules and connectors as Kibana saved object Export/Import #50266

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom alert ids #89814

Add support for custom alert ids #89814

mikecote commented Jan 30, 2021 •

edited

Loading

elasticmachine commented Jan 30, 2021

mikecote Feb 1, 2021

legrego Feb 1, 2021

mikecote Feb 1, 2021

legrego Feb 1, 2021

mikecote Feb 1, 2021

legrego Feb 1, 2021

mikecote Feb 1, 2021

legrego Feb 1, 2021

pmuellr commented Feb 1, 2021

mikecote commented Feb 1, 2021

pmuellr commented Feb 1, 2021

pmuellr left a comment

mikecote commented Feb 1, 2021 •

edited

Loading

YulNaumenko left a comment

kibanamachine commented Feb 1, 2021

Add support for custom alert ids #89814

Add support for custom alert ids #89814

Conversation

mikecote commented Jan 30, 2021 • edited Loading

Notes for doc

elasticmachine commented Jan 30, 2021

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 Feb 1, 2021

mikecote commented Feb 1, 2021

pmuellr commented Feb 1, 2021

pmuellr left a comment

Choose a reason for hiding this comment

mikecote commented Feb 1, 2021 • edited Loading

YulNaumenko left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 1, 2021

💚 Build Succeeded

Metrics [docs]

History

mikecote commented Jan 30, 2021 •

edited

Loading

mikecote commented Feb 1, 2021 •

edited

Loading