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] bubble up policy response errors #133349

Closed
wants to merge 2 commits into from

Conversation

joeypoon
Copy link
Member

@joeypoon joeypoon commented Jun 2, 2022

Summary

  • Add new error callouts for endpoint policy response. For each failure, there will be a nested callout in the accordion as well as a top-level bubbled up callout for easy viewing when the accordion is collapsed.
  • Refactor policy_response_friendly_names.ts so that new error descriptions and links can easily be added.
  • Add full_disk_access as failure action
  • Does not include restyling to match new designs for improved agent debugging

Screen Shot 2022-06-01 at 6 49 01 PM

Testing notes:

  • Link button only shows if failed action is mapped in linkTexts and linkUrls.
  • Callout description only shows if failed action is mapped in descriptions.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@joeypoon joeypoon added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution auto-backport Deprecated - use backport:version if exact versions are needed ci:deploy-cloud v8.4.0 labels Jun 2, 2022
i18n.translate(
'xpack.securitySolution.endpoint.details.policyResponse.link.text.full_disk_access',
{
defaultMessage: 'Documentation',
Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinlog thoughts on wording for full_disk_access link button?

Copy link
Contributor

Choose a reason for hiding this comment

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

@joeypoon

how about: Full disk access needs to be enabled on this host to enable all Endpoint capabilities. See the documentation[link] for more information.

cc @caitlinbetz

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree that using the link within the description is more elegant. though, I also feel like since the design had a CTA button it felt a bit more consistent if all links were just on the button. either way works for me though.

this.title =
policyResponseTitles.get(this.name) ??
this.name.replace(/_/g, ' ').replace(/\b(\w)/g, (m) => m.toUpperCase());
this.hasError = policyResponseAppliedAction.status === 'failure';
Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinlog should we also show callout on unsupported or warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

we should show a callout for warning, but no need for unsupported at this time.

@joeypoon joeypoon force-pushed the feature/policy-response branch from d680bb9 to 5828bfd Compare June 2, 2022 13:30
@joeypoon joeypoon marked this pull request as ready for review June 2, 2022 13:51
@joeypoon joeypoon requested a review from a team as a code owner June 2, 2022 13:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

@joeypoon joeypoon requested a review from dasansol92 June 2, 2022 13:53
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3011 3012 +1

Async chunks

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

id before after diff
securitySolution 5.1MB 5.1MB +5.4KB

History

  • 💔 Build #48764 failed d680bb93d7c2f516c0373f256cfb3e7dea441e3a

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

Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

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

Tested it out and it works as expected. Since this is to make the error handling UX better, it got me thinking, that currently, you can see where the error is with the notification bubble, but still have to click a couple of times to get to it depending on which level it is in.

It might be even more helpful if the bubbled-up callout title was clickable to expand the policy where the error is. What do you think?

Comment on lines +99 to +103
const key = action + index;
return (
<EuiAccordion
id={action + index}
key={action + index}
id={key}
key={key}
Copy link
Member

Choose a reason for hiding this comment

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

nice

color="danger"
iconType="alert"
>
<p style={{ marginBottom: '8px' }}>{policyResponseError.errorDescription}</p>
Copy link
Member

Choose a reason for hiding this comment

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

You should pull this from the euiTheme, there must be a token like euiMarginBottom or something.

@joeypoon
Copy link
Member Author

joeypoon commented Jun 2, 2022

Tested it out and it works as expected. Since this is to make the error handling UX better, it got me thinking, that currently, you can see where the error is with the notification bubble, but still have to click a couple of times to get to it depending on which level it is in.

It might be even more helpful if the bubbled-up callout title was clickable to expand the policy where the error is. What do you think?

I think this is a good idea. Though, I think the implementation might be a bit too involved with manual accordion state tracking. I feel like in a normal scenario, a user wouldn't need to drill down the accordion and the top level callout should be sufficient at providing enough actionable information.

@joeypoon
Copy link
Member Author

joeypoon commented Jun 2, 2022

Closing this PR for now as there's a decent amount of overlap with this PR: #133405. Will reopen after it is merged and reimplement the necessary changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed ci:cloud-deploy Create or update a Cloud deployment release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants