Retain APIKey when disabling/enabling a rule by ersin-erdal · Pull Request #131581 · elastic/kibana

ersin-erdal · 2022-05-04T22:05:18Z

Currently we invalidate the API key when a rule is disabled and generate a new one when it is enabled again.
With this PR, we retain the API key when a rule is disabled and generate a new one just in case of the rule somehow doesn't have an API key (disabled before this feature, imported etc.)

This PR also adds an Update API Key button to rules list and rule details page to allow users to generate anew API key manually.

User interaction flow is:

…retain-api-key � Conflicts: � x-pack/plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.test.tsx � x-pack/plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx

…34-retain-api-key

mdefazio · 2022-05-06T14:47:11Z

Ran through the PR together and had the following updates:

Move 'Update API' row action link to be between Disable and Delete --> we want to keep Delete as the last option in that overflow menu
Confirmation modal should use primary button styling
Will need to get some writers to help with the copy
Add option to Update APIKey on detail page
- Make 'Edit' a base style button (not empty)
- Keep View in app exposed as empty button
- Add medium icon button (base style with background, horizontalBoxes icon) to the right of Edit button
- Add popover menu to icon button with Disable, Update APIKey, and Delete

Here's a quick mockup of the page header buttons on the detail page. I kept refresh exposed alongside View in app. We are updating the page to place the disable button within the page more clearly, but whatever the result of that, I think we'll want to duplicate that action here.

Showing a user with the option to update the api key and one without.

…retain-api-key

elasticmachine · 2022-05-10T12:48:59Z

Pinging @elastic/response-ops (Team:ResponseOps)

pmuellr · 2022-05-10T17:52:26Z

Just starting a review - thinking we should probably have some functional tests, but not quite clear to me what they should do! I don't believe we expose the API key in any HTTP outputs, do we? On purpose. Or via public types. Only immediate thought is to get the rule SO directly in the function test, unencrypted, and see if there is a key set.

pmuellr

I review the server side code - surprised it's so small! Wondering what we're missing :-)

I'll review the UX code next, but thought I'd send along the current comments, which are mostly questions.

x-pack/plugins/alerting/server/rules_client/rules_client.ts

x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts

x-pack/plugins/alerting/server/rules_client/tests/enable.test.ts

mikecote

Looking good! Did a pass of the code. I think it would be good to get a 👍 from R&AM team as well (cc @XavierM). Left some feedback and will approve after my questions are answered.

thinking we should probably have some functional tests, but not quite clear to me what they should do!

I do think we should add some functional tests. Something to ensure an API key isn't overwritten on enable when it exists. Ensure disable then enable preserves the API key, maybe?

x-pack/plugins/alerting/server/rules_client/rules_client.ts

...gins/triggers_actions_ui/public/application/components/update_api_key_modal_confirmation.tsx

Support updating multiple rules (Future proof)

mdefazio

Tiny nit to lowercase API key.

...gins/triggers_actions_ui/public/application/components/update_api_key_modal_confirmation.tsx

...gers_actions_ui/public/application/sections/rule_details/components/rule_actions_popover.tsx

...actions_ui/public/application/sections/rules_list/components/collapsed_item_actions.test.tsx

...gers_actions_ui/public/application/sections/rules_list/components/collapsed_item_actions.tsx

...gins/triggers_actions_ui/public/application/components/update_api_key_modal_confirmation.tsx

...rs_actions_ui/public/application/sections/rule_details/components/rule_actions_popopver.scss

remove checkAAD from test

mikecote

Tested locally and LGTM!

gmmorris · 2022-05-16T09:54:52Z

We should update the user facing docs as they are no longer correct after this PR is merged.

https://www.elastic.co/guide/en/kibana/master/alerting-setup.html#alerting-authorization

…retain-api-key � Conflicts: � x-pack/plugins/alerting/server/rules_client/rules_client.ts � x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts � x-pack/plugins/alerting/server/rules_client/tests/enable.test.ts

mdefazio

Thanks for making those changes!

mdefazio

Doh, hit approve then saw that it looks like we're still targeting the button instead of a classname (comment)

pmuellr

Left a question and request for some small changes

x-pack/test/alerting_api_integration/security_and_spaces/group1/tests/alerting/disable.ts

...ck/test/alerting_api_integration/security_and_spaces/group1/tests/alerting/retain_api_key.ts

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

test apiKei is not null or undefined

kibana-ci · 2022-05-17T19:57:48Z

💛 Build succeeded, but was flaky

Failed CI Steps

FTR Configs #42

Test Failures

[job] [logs] FTR Configs #42 / machine learning - data frame analytics results view content and total feature importance multi class classification job should display the total feature importance in the results view

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`triggersActionsUi`	380	388	+8

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`triggersActionsUi`	745.7KB	750.5KB	+4.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`triggersActionsUi`	101.1KB	101.3KB	+137.0B

History

💔 Build #45408 failed 521ef6e
💚 Build #45199 succeeded 0da7084
💚 Build #45050 succeeded d0c0dfc
💚 Build #44909 succeeded 72598c3
💚 Build #44168 succeeded 29168a7

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

pmuellr

LGTM!

Retain APIKey when disabling/enabling a rule

329f1de

ersin-erdal added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework v8.3.0 labels May 4, 2022

ersin-erdal and others added 5 commits May 5, 2022 00:22

Merge branch 'main' into 131234-retain-api-key

4c4b6c8

fix failing test

76b3afe

Merge remote-tracking branch 'origin/131234-retain-api-key' into 1312…

ceb4e26

…34-retain-api-key

Fix failing test

ad84a32

ersin-erdal added 8 commits May 6, 2022 18:14

Translations

487dfbf

Add delete and Update API Key buttons to rule details page

7c5cbd9

Remove Refresh button from the dropdown

267abf4

revert rules list

94b7a3a

merge

8df86ac

Merge branch 'main' of https://github.com/elastic/kibana into 131234-…

7e49f56

…retain-api-key

fix type check error

c38f350

Add enable/disable rule buttons

b485ff8

ersin-erdal marked this pull request as ready for review May 10, 2022 12:48

ersin-erdal requested review from a team as code owners May 10, 2022 12:48

pmuellr reviewed May 10, 2022

View reviewed changes

mikecote self-requested a review May 10, 2022 19:02

mikecote reviewed May 10, 2022

View reviewed changes

ersin-erdal added 3 commits May 10, 2022 22:34

Use correct mock data in enable rule test

446003e

Update meta when disable a rule,

fba81a9

Support updating multiple rules (Future proof)

Update error message

ef30e4e

mdefazio requested changes May 11, 2022

View reviewed changes

cchaos reviewed May 11, 2022

View reviewed changes

...rs_actions_ui/public/application/sections/rule_details/components/rule_actions_popopver.scss Outdated Show resolved Hide resolved

ersin-erdal added 2 commits May 11, 2022 17:05

Copy and styling fixes

92d4dce

Fix Delete modal text.

29168a7

remove checkAAD from test

mikecote approved these changes May 11, 2022

View reviewed changes

mdefazio approved these changes May 16, 2022

View reviewed changes

mdefazio reviewed May 16, 2022

View reviewed changes

ersin-erdal added 2 commits May 16, 2022 20:41

Add functional test and use classname for styling

d0c0dfc

Update Auth docs

0da7084

ersin-erdal requested a review from a team as a code owner May 17, 2022 07:47

pmuellr requested changes May 17, 2022

View reviewed changes

ersin-erdal added 2 commits May 17, 2022 17:21

use alerts_fixture in _get_api_key ep path

521ef6e

test apiKei is not null or undefined

fix object remover

5e87f3e

pmuellr approved these changes May 17, 2022

View reviewed changes

szabosteve mentioned this pull request May 18, 2022

[DOCS] Updates alerting authorization docs with info on retaining API keys #132402

Merged

1 task

ersin-erdal merged commit de29010 into elastic:main May 18, 2022

kibanamachine added the backport:skip This PR does not require backporting label May 18, 2022

ersin-erdal deleted the 131234-retain-api-key branch May 18, 2022 13:13

banderror mentioned this pull request Oct 24, 2022

[Security Solution] Bulk actions and Custom rules got disabled when custom user have "NONE" access of actions and connectors. #141050

Closed

Conversation

ersin-erdal commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdefazio commented May 6, 2022

Uh oh!

elasticmachine commented May 10, 2022

Uh oh!

pmuellr commented May 10, 2022

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikecote left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdefazio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

gmmorris commented May 16, 2022

Uh oh!

mdefazio left a comment

Choose a reason for hiding this comment

Uh oh!

mdefazio left a comment

Choose a reason for hiding this comment

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibana-ci commented May 17, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ersin-erdal commented May 4, 2022 •

edited

Loading

mikecote left a comment •

edited

Loading