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][Exceptions] - Improve UX for missing exception list associated with rule #75898

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Aug 25, 2020

Summary

Current behavior:

  • Scenario 1: User is in the exceptions viewer flow, they select to edit an exception item, but the list the item is associated with has since been deleted (let's say by another user) - a user is able to open modal to edit exception item and on save, an error toaster shows but no information is given to the user to indicate the issue.
  • Scenario 2: User exports rules from space 'X' and imports into space 'Y'. The exception lists associated with their newly imported rules do not exist in space 'Y' - a user goes to add an exception item and gets a modal with an error, unable to add any exceptions.
  • Workaround: current workaround exists only via API - user would need to remove the exception list from their rule via API

New behavior:

  • Scenario 1: User is still able to oped edit modal, but on save they see an error explaining that the associated exception list does not exist and prompts them to remove the exception list --> now they're able to add exceptions to their rule
    edit_exception

  • Scenario 2: User navigates to exceptions after importing their rule, tries to add exception, modal pops up with error informing them that they need to remove association to missing exception list, button prompts them to do so --> now can continue adding exceptions to rule
    add_exception

Checklist

@@ -107,13 +109,14 @@ export const AddExceptionModal = memo(function AddExceptionModal({
}: AddExceptionModalProps) {
const { http } = useKibana().services;
const [comment, setComment] = useState('');
const { rule: maybeRule } = useRuleAsync(ruleId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using this here to pull out the exception list details of the issue exception list. The error itself gives an id but no other info. Thought of reworking useFetchOrCreateRuleExceptionList to pull out the call it makes to the rule, but wanted to minimize changes for backport to 7.9.1.

}: EditExceptionModalProps) {
const { http } = useKibana().services;
const [comment, setComment] = useState('');
const { rule: maybeRule } = useRuleAsync(ruleId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may be able to refactor these modals to have a common parent, since they both repeat a good amount of logic, right now just doubling logic in each file.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
securitySolution 1920 +2 1918

async chunks size

id value diff baseline
securitySolution 7.2MB +14.2KB 7.2MB

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

Copy link
Contributor

@FrankHassanabad FrankHassanabad 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 the fixes for this, nice tests too!


useEffect((): void => {
// Yes, it's redundant, unfortunately typescript wasn't picking up
// that `listToDelete` is checked in canDisplay404Actions
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha!

@yctercero yctercero merged commit b9c8201 into elastic:master Aug 26, 2020
yctercero added a commit to yctercero/kibana that referenced this pull request Aug 26, 2020
…st associated with rule (elastic#75898)

## Summary

**Current behavior:**
  - **Scenario 1:** User is in the exceptions viewer flow, they select to edit an exception item, but the list the item is associated with has since been deleted (let's say by another user) - a user is able to open modal to edit exception item and on save, an error toaster shows but no information is given to the user to indicate the issue.
  - **Scenario 2:** User exports rules from space 'X' and imports into space 'Y'. The exception lists associated with their newly imported rules do not exist in space 'Y' - a user goes to add an exception item and gets a modal with an error, unable to add any exceptions. 
  - **Workaround:** current workaround exists only via API - user would need to remove the exception list from their rule via API

**New behavior:**
  - **Scenario 1:** User is still able to oped edit modal, but on save they see an error explaining that the associated exception list does not exist and prompts them to remove the exception list --> now they're able to add exceptions to their rule
  - **Scenario 2:** User navigates to exceptions after importing their rule, tries to add  exception, modal pops up with error informing them that they need to remove association to missing exception list, button prompts them to do so --> now can continue adding exceptions to rule
@tylersmalley
Copy link
Contributor

Hi all, I am reverting this PR as it is causing failures on master due to the incompatability with #74920.

tylersmalley added a commit that referenced this pull request Aug 26, 2020
…ption list associated with rule (#75898)"

This reverts commit b9c8201.
tylersmalley pushed a commit that referenced this pull request Aug 26, 2020
…ption list associated with rule (#75898)"

This reverts commit b9c8201.
@tylersmalley
Copy link
Contributor

tylersmalley commented Aug 26, 2020

Revert: master/8.0: e773f22
The 7.x backport is correctly failing since the latest CI job included the previously mentioned PR

Note there is a functional test runner failure in master/7.x we are working through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants