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

[Security Solution][Detections] fixes validation issues in rules actions form #141811

Merged
merged 5 commits into from
Sep 30, 2022

Conversation

vitaliidm
Copy link
Contributor

@vitaliidm vitaliidm commented Sep 26, 2022

Summary

Before

Single rule actions update

Screen.Recording.2022-09-26.at.12.36.53.mov

Bulk edit rule actions

rules_actions_form_issues.mp4

After

Single rule actions update and Bulk edit rule actions

Screen.Recording.2022-09-26.at.17.00.16.mov

@vitaliidm vitaliidm self-assigned this Sep 26, 2022
@vitaliidm vitaliidm added backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) v8.5.0 v8.6.0 Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Feature:Rule Actions Security Solution Detection Rule Actions area Feature:Rule Management Security Solution Detection Rule Management area labels Sep 26, 2022
@vitaliidm vitaliidm marked this pull request as ready for review September 27, 2022 09:57
@vitaliidm vitaliidm requested a review from a team as a code owner September 27, 2022 09:57
@vitaliidm vitaliidm requested a review from xcrzx September 27, 2022 09:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@banderror banderror added the bug Fixes for quality problems that affect the customer experience label Sep 27, 2022
};
return updatedActions;
});
// values in form controls and values used during form validation got out of sync leading to issue: https://github.com/elastic/kibana/issues/140593
Copy link
Contributor

@xcrzx xcrzx Sep 27, 2022

Choose a reason for hiding this comment

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

@vitaliidm Could you, please expand on why form and validation values get out of sync? If there's a race condition, using setTimeout to "win" it doesn't seem like a scalable solution. Maybe there're other options for streamlining those state updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xcrzx

so, setActionIdByIndex within this file fires update of actions(but w/o params properties)
setActionParamsProperty updates params in field actions of form with values displayed if form controls through calling field.setValue. In our case, it is a message form field in slack action form.

These updates trigger form validation:

  actions: {
    validations: [
      {
        validator: validateRuleActionsField(actionTypeRegistry),
      },
    ],
  },

validateRuleActionsField receives in curried function argument field value:

    const [{ value, path }] = data as [{ value: RuleAction[]; path: string }];

which doesn't have updates set in setActionParamsProperty. It happens consistently when you add some action, remove it, add again it.

here is logged values in functions calls

setActionIdByIndex [
  {
    "id": "50a49d70-34d4-11ed-87dc-f3f665e2eedb",
    "actionTypeId": ".slack",
    "group": "default",
    "params": {}
  }
]
validateRuleActionsField [
  {
    "id": "50a49d70-34d4-11ed-87dc-f3f665e2eedb",
    "actionTypeId": ".slack",
    "group": "default",
    "params": {}
  }
]
validateRuleActionsField [
  {
    "id": "50a49d70-34d4-11ed-87dc-f3f665e2eedb",
    "actionTypeId": ".slack",
    "group": "default",
    "params": {}
  }
]
setActionParamsProperty [
  {
    "id": "50a49d70-34d4-11ed-87dc-f3f665e2eedb",
    "actionTypeId": ".slack",
    "group": "default",
    "params": {
      "message": "Rule {{context.rule.name}} generated {{state.signals_count}} alerts"
    }
  }
]

As you can see validateRuleActionsField is not getting called after setActionParamsProperty called, instead validateRuleActionsField called twice on setActionIdByIndex. I would expect validation to be triggered once we set new params values.
But wrapping field.setValue in setTimeout helped to resolve the issue.
If you have any ideas how to resolve this, please share.

Copy link
Contributor

Choose a reason for hiding this comment

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

As you can see validateRuleActionsField is not getting called after setActionParamsProperty called, instead validateRuleActionsField called twice on setActionIdByIndex. I would expect validation to be triggered once we set new params values.

Thanks for the explanation! To me, that looks like a bug on the Actions Form side. Validation should definitely happen after params modification, but it's not the case.

@elastic/response-ops Could you please confirm if the behavior described above is a bug? In which case, I propose fixing it in the getActionForm instead of working it around on the solution side.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the validateRuleActionsField validation occurs inside the security form, not inside the ActionForm. What triggers this validation?

Looking through the triggers_actions_ui code, when a connector type is specified, the ActionForm calls setActionIdByIndex with empty params and then it loads the Params component for that connector type. When the Params component is loaded, it sets a defaultActionMessage which calls setActionParamsProperty. Based on the logs above, the order that this is happening is correct. What is triggering the validateRuleActionsField call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is triggering the validateRuleActionsField call?

I think it's triggered somewhere within eui form hook useForm, where validateRuleActionsField passed inside formSchema

  actions: {
    validations: [
      {
        validator: validateRuleActionsField(actionTypeRegistry),
      },
    ],
  },
  const { form } = useForm({
    schema: formSchema,
    defaultValue: defaultFormData,
  });

@xcrzx
I think it would make sense to merge this fix and investigate further issue with not triggered validation. The root cause seems to be not in Security Solution, so we will depend on other teams in this, but current bug affects customer experience and we already on BC2 of 8.5 release.

cc: @banderror

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make sense to merge this fix and investigate further issue with not triggered validation. The root cause seems to be not in Security Solution, so we will depend on other teams in this, but current bug affects customer experience and we already on BC2 of 8.5 release.

I'm fine with that. @vitaliidm, Could you please create a ticker for further investigation and mention it in the code comment for additional context?

Copy link
Contributor Author

@vitaliidm vitaliidm Sep 29, 2022

Choose a reason for hiding this comment

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

@xcrzx

I'm fine with that. @vitaliidm, Could you please create a ticker for further investigation and mention it in the code comment for additional context?

Here is created ticket #142217, also added it to comment

@vitaliidm vitaliidm requested a review from xcrzx September 27, 2022 15:51
Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for creating a tech debt issue to investigate the problem further

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 6.6MB 6.6MB +4.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 412 410 -2

Total ESLint disabled count

id before after diff
securitySolution 485 483 -2

History

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

cc @vitaliidm

@vitaliidm vitaliidm merged commit 6b11352 into elastic:main Sep 30, 2022
@vitaliidm vitaliidm deleted the detections/rules-actions-form branch September 30, 2022 10:03
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 30, 2022
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 30, 2022
…ons form (#141811) (#142318)

## Summary

- addresses #140593
   - bulk edit of rules actions
   - [single rule actions update](#140593 (comment) )

### Before

Single rule actions update

https://user-images.githubusercontent.com/92328789/192327231-8fdc846c-55f2-4ab1-8786-e96d2376af48.mov

Bulk edit rule actions

https://user-images.githubusercontent.com/92328789/192327094-ea830769-9633-43dc-be37-7ec68de4bd6f.mp4

### After

Single rule actions update and Bulk edit rule actions

https://user-images.githubusercontent.com/92328789/192325274-010d11fc-17eb-47a1-b817-7a24eba8a365.mov

(cherry picked from commit 6b11352)

Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com>
spong added a commit that referenced this pull request Feb 16, 2023
…ng Rule Action Message (#150823)

## Summary

Resolves: #149885

For additional details, please see
#141811 (comment) and
#142217.

As mentioned in #142217, this
issue is the result of managing stale events and timings (renderings +
field updates) between the Detections `RuleActionsField` component,
validation within the hooks-form lib, and field updates coming from the
`trigger_actions_ui` components.

Note: this behavior is explicit to the Edit Rule flow (`ActionsStepRule`
form), and not the Bulk Actions flyout (`RuleActionsFormData` form) as
events flow as intended in the latter, presumably because of all the
nested components and use of `useFormData` within the Edit Rule flow
(see [form libs
docs](https://docs.elastic.dev/form-lib/examples/listening-to-changes#forward-the-form-state-to-a-parent-component)).

The fix here is a modification of the fix provided in
#141811 with `setTimeout`, but
instead of always pushing the params update to be within a `setTimeout`,
it now only does it when initially loading `Actions` with empty
`Params`. Since this fix also has the unintended side-effect of
flickering the validation error callout when first adding an action,
validation is now throttled to 100ms intervals, which also helps with
extraneous re-renders.

Before initial fix (with cursor issue) / Before throttle fix w/ flashing
error callout:
<p align="center">
<img width="350"
src="https://user-images.githubusercontent.com/2946766/215585424-8c056440-5906-4168-b997-aea3690c7ea5.gif"
/> <img width="242"
src="https://user-images.githubusercontent.com/2946766/217961284-704013b7-2f0a-4a24-95a4-4183348448f4.gif"
/>
</p>

After:
<p align="center">
<img width="242"
src="https://user-images.githubusercontent.com/2946766/218591986-e52456e0-e374-4d8e-bc84-9fb6da69f1f9.gif"
/>
</p>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 16, 2023
…ng Rule Action Message (elastic#150823)

## Summary

Resolves: elastic#149885

For additional details, please see
elastic#141811 (comment) and
elastic#142217.

As mentioned in elastic#142217, this
issue is the result of managing stale events and timings (renderings +
field updates) between the Detections `RuleActionsField` component,
validation within the hooks-form lib, and field updates coming from the
`trigger_actions_ui` components.

Note: this behavior is explicit to the Edit Rule flow (`ActionsStepRule`
form), and not the Bulk Actions flyout (`RuleActionsFormData` form) as
events flow as intended in the latter, presumably because of all the
nested components and use of `useFormData` within the Edit Rule flow
(see [form libs
docs](https://docs.elastic.dev/form-lib/examples/listening-to-changes#forward-the-form-state-to-a-parent-component)).

The fix here is a modification of the fix provided in
elastic#141811 with `setTimeout`, but
instead of always pushing the params update to be within a `setTimeout`,
it now only does it when initially loading `Actions` with empty
`Params`. Since this fix also has the unintended side-effect of
flickering the validation error callout when first adding an action,
validation is now throttled to 100ms intervals, which also helps with
extraneous re-renders.

Before initial fix (with cursor issue) / Before throttle fix w/ flashing
error callout:
<p align="center">
<img width="350"
src="https://user-images.githubusercontent.com/2946766/215585424-8c056440-5906-4168-b997-aea3690c7ea5.gif"
/> <img width="242"
src="https://user-images.githubusercontent.com/2946766/217961284-704013b7-2f0a-4a24-95a4-4183348448f4.gif"
/>
</p>

After:
<p align="center">
<img width="242"
src="https://user-images.githubusercontent.com/2946766/218591986-e52456e0-e374-4d8e-bc84-9fb6da69f1f9.gif"
/>
</p>

(cherry picked from commit 736759a)
kibanamachine added a commit that referenced this pull request Feb 16, 2023
… editing Rule Action Message (#150823) (#151518)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Security Solution] Fix cursor jumping to end of text area when
editing Rule Action Message
(#150823)](#150823)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Garrett
Spong","email":"spong@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-02-16T21:05:26Z","message":"[Security
Solution] Fix cursor jumping to end of text area when editing Rule
Action Message (#150823)\n\n## Summary\r\n\r\nResolves:
https://github.com/elastic/kibana/issues/149885\r\n\r\nFor additional
details, please
see\r\nhttps://github.com//pull/141811#discussion_r981346237
and\r\nhttps://github.com//issues/142217.\r\n\r\nAs
mentioned in #142217,
this\r\nissue is the result of managing stale events and timings
(renderings +\r\nfield updates) between the Detections
`RuleActionsField` component,\r\nvalidation within the hooks-form lib,
and field updates coming from the\r\n`trigger_actions_ui`
components.\r\n\r\nNote: this behavior is explicit to the Edit Rule flow
(`ActionsStepRule`\r\nform), and not the Bulk Actions flyout
(`RuleActionsFormData` form) as\r\nevents flow as intended in the
latter, presumably because of all the\r\nnested components and use of
`useFormData` within the Edit Rule flow\r\n(see [form
libs\r\ndocs](https://docs.elastic.dev/form-lib/examples/listening-to-changes#forward-the-form-state-to-a-parent-component)).\r\n\r\nThe
fix here is a modification of the fix provided
in\r\nhttps://github.com//pull/141811 with `setTimeout`,
but\r\ninstead of always pushing the params update to be within a
`setTimeout`,\r\nit now only does it when initially loading `Actions`
with empty\r\n`Params`. Since this fix also has the unintended
side-effect of\r\nflickering the validation error callout when first
adding an action,\r\nvalidation is now throttled to 100ms intervals,
which also helps with\r\nextraneous re-renders.\r\n\r\nBefore initial
fix (with cursor issue) / Before throttle fix w/ flashing\r\nerror
callout:\r\n<p align=\"center\">\r\n<img
width=\"350\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/215585424-8c056440-5906-4168-b997-aea3690c7ea5.gif\"\r\n/>
<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/217961284-704013b7-2f0a-4a24-95a4-4183348448f4.gif\"\r\n/>\r\n</p>\r\n\r\nAfter:\r\n<p
align=\"center\">\r\n<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/218591986-e52456e0-e374-4d8e-bc84-9fb6da69f1f9.gif\"\r\n/>\r\n</p>","sha":"736759af76623966684e6dbb06009f1517a8d8a8","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Team:Detections
and Resp","Team: SecuritySolution","Feature:Rule
Actions","Team:Detection
Rules","v8.7.0","v8.8.0"],"number":150823,"url":"https://github.com/elastic/kibana/pull/150823","mergeCommit":{"message":"[Security
Solution] Fix cursor jumping to end of text area when editing Rule
Action Message (#150823)\n\n## Summary\r\n\r\nResolves:
https://github.com/elastic/kibana/issues/149885\r\n\r\nFor additional
details, please
see\r\nhttps://github.com//pull/141811#discussion_r981346237
and\r\nhttps://github.com//issues/142217.\r\n\r\nAs
mentioned in #142217,
this\r\nissue is the result of managing stale events and timings
(renderings +\r\nfield updates) between the Detections
`RuleActionsField` component,\r\nvalidation within the hooks-form lib,
and field updates coming from the\r\n`trigger_actions_ui`
components.\r\n\r\nNote: this behavior is explicit to the Edit Rule flow
(`ActionsStepRule`\r\nform), and not the Bulk Actions flyout
(`RuleActionsFormData` form) as\r\nevents flow as intended in the
latter, presumably because of all the\r\nnested components and use of
`useFormData` within the Edit Rule flow\r\n(see [form
libs\r\ndocs](https://docs.elastic.dev/form-lib/examples/listening-to-changes#forward-the-form-state-to-a-parent-component)).\r\n\r\nThe
fix here is a modification of the fix provided
in\r\nhttps://github.com//pull/141811 with `setTimeout`,
but\r\ninstead of always pushing the params update to be within a
`setTimeout`,\r\nit now only does it when initially loading `Actions`
with empty\r\n`Params`. Since this fix also has the unintended
side-effect of\r\nflickering the validation error callout when first
adding an action,\r\nvalidation is now throttled to 100ms intervals,
which also helps with\r\nextraneous re-renders.\r\n\r\nBefore initial
fix (with cursor issue) / Before throttle fix w/ flashing\r\nerror
callout:\r\n<p align=\"center\">\r\n<img
width=\"350\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/215585424-8c056440-5906-4168-b997-aea3690c7ea5.gif\"\r\n/>
<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/217961284-704013b7-2f0a-4a24-95a4-4183348448f4.gif\"\r\n/>\r\n</p>\r\n\r\nAfter:\r\n<p
align=\"center\">\r\n<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/218591986-e52456e0-e374-4d8e-bc84-9fb6da69f1f9.gif\"\r\n/>\r\n</p>","sha":"736759af76623966684e6dbb06009f1517a8d8a8"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/150823","number":150823,"mergeCommit":{"message":"[Security
Solution] Fix cursor jumping to end of text area when editing Rule
Action Message (#150823)\n\n## Summary\r\n\r\nResolves:
https://github.com/elastic/kibana/issues/149885\r\n\r\nFor additional
details, please
see\r\nhttps://github.com//pull/141811#discussion_r981346237
and\r\nhttps://github.com//issues/142217.\r\n\r\nAs
mentioned in #142217,
this\r\nissue is the result of managing stale events and timings
(renderings +\r\nfield updates) between the Detections
`RuleActionsField` component,\r\nvalidation within the hooks-form lib,
and field updates coming from the\r\n`trigger_actions_ui`
components.\r\n\r\nNote: this behavior is explicit to the Edit Rule flow
(`ActionsStepRule`\r\nform), and not the Bulk Actions flyout
(`RuleActionsFormData` form) as\r\nevents flow as intended in the
latter, presumably because of all the\r\nnested components and use of
`useFormData` within the Edit Rule flow\r\n(see [form
libs\r\ndocs](https://docs.elastic.dev/form-lib/examples/listening-to-changes#forward-the-form-state-to-a-parent-component)).\r\n\r\nThe
fix here is a modification of the fix provided
in\r\nhttps://github.com//pull/141811 with `setTimeout`,
but\r\ninstead of always pushing the params update to be within a
`setTimeout`,\r\nit now only does it when initially loading `Actions`
with empty\r\n`Params`. Since this fix also has the unintended
side-effect of\r\nflickering the validation error callout when first
adding an action,\r\nvalidation is now throttled to 100ms intervals,
which also helps with\r\nextraneous re-renders.\r\n\r\nBefore initial
fix (with cursor issue) / Before throttle fix w/ flashing\r\nerror
callout:\r\n<p align=\"center\">\r\n<img
width=\"350\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/215585424-8c056440-5906-4168-b997-aea3690c7ea5.gif\"\r\n/>
<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/217961284-704013b7-2f0a-4a24-95a4-4183348448f4.gif\"\r\n/>\r\n</p>\r\n\r\nAfter:\r\n<p
align=\"center\">\r\n<img
width=\"242\"\r\nsrc=\"https://user-images.githubusercontent.com/2946766/218591986-e52456e0-e374-4d8e-bc84-9fb6da69f1f9.gif\"\r\n/>\r\n</p>","sha":"736759af76623966684e6dbb06009f1517a8d8a8"}}]}]
BACKPORT-->

Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) bug Fixes for quality problems that affect the customer experience Feature:Rule Actions Security Solution Detection Rule Actions area Feature:Rule Management Security Solution Detection Rule Management area release_note:fix Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.5.0 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants