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][Case] Fix subcases bugs on detections and case view #91836

Merged
merged 39 commits into from
Feb 26, 2021

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Feb 18, 2021

Summary

Fixes a list of bugs on detections and case view:

  • UI don't allow manually attaching alert to closed case
  • UI don't allow manually attaching alert to a collection
    • Gray out collections but enable sub cases
  • Make sure to show sub cases
  • When manually attaching an alert to a case, the case is pushed before the alert is attached
  • Infinity state loop when using the connector
  • Remove status when viewing a collection
  • Posting a comment to a sub case fails [request query.subCaseId]: definition for this key is missing
  • Patching a comment to a sub case fails.
  • Attaching an alert to a sub case fails with an encoding error because the UI is expecting a CaseResponseRt and the backend is sending a CollectionWithSubCaseResponseRt
  • User actions are empty when you post/patch a comment to a subcase

Meta: #91843

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cnasikas cnasikas added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 Team:Detections and Resp Security Detection Response Team labels Feb 18, 2021
@cnasikas cnasikas self-assigned this Feb 18, 2021
@cnasikas cnasikas requested a review from a team as a code owner February 18, 2021 13:21
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@elasticmachine
Copy link
Contributor

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

@azasypkin azasypkin added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. and removed Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Feb 18, 2021
@elasticmachine
Copy link
Contributor

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

@cnasikas cnasikas changed the title [Security Solution][Case] Fix subcases bugs [Security Solution][Case] Fix subcases bugs on detections Feb 18, 2021
@cnasikas cnasikas added Team:Threat Hunting Security Solution Threat Hunting Team and removed Team:Detections and Resp Security Detection Response Team labels Feb 18, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@cnasikas cnasikas requested a review from a team February 18, 2021 15:11
@cnasikas cnasikas changed the title [Security Solution][Case] Fix subcases bugs on detections [Security Solution][Case] Fix subcases bugs on detections and case view Feb 18, 2021
@cnasikas cnasikas added the bug Fixes for quality problems that affect the customer experience label Feb 18, 2021
Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Backend changes look good, I left a couple comments on the front end changes.

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Nice work here!

I tested a couple things:

Manually adding alerts to case

  • Collections are disabled ✅
  • Sub cases are clickable ✅

One UX improvement here I think would be to use a different color for the disabled rows in the table. Currently it looks like the collections are already selected. I'm not sure how much work that'd be to change though.
image

Navigating to a collection

  • No longer shows the status ✅
    image

Adding/editing a comment on a sub case
image

User actions are shown for sub case

Manually attaching an alert for a sub case

image

I'm not sure if this was something we were trying to address in this PR but an empty collection has it's status displayed and is selectable for bulk actions. I think that might be an existing issue though. I added it to the meta ticket here: #91843

image

onStatusChanged={changeStatus}
disabled={!userCanCrud}
isLoading={isLoading && updateKey === 'status'}
{(caseData.type !== CaseType.collection || hasDataToPush) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to find a better way to do this instead of having the double checks but couldn't think of one haha 👍

Copy link
Member Author

@cnasikas cnasikas Feb 26, 2021

Choose a reason for hiding this comment

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

Me too. I fought a lot with this :)

@@ -29,7 +29,7 @@ export const useCreateCaseModal = ({
const closeModal = useCallback(() => setIsModalOpen(false), []);
const openModal = useCallback(() => setIsModalOpen(true), []);
const onSuccess = useCallback(
(theCase) => {
async (theCase) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the async here? Do we need an await on one of the calls?

Copy link
Member Author

Choose a reason for hiding this comment

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

form_context.tsx expects onSuccess: (theCase: Case) => Promise<void> and waits for the onSuccess to be fulfilled. Do you think it shouldn't?

@cnasikas
Copy link
Member Author

cnasikas commented Feb 26, 2021

@jonathan-buttner Thank you so much for testing!!

One UX improvement here I think would be to use a different color for the disabled rows in the table. Currently it looks like the collections are already selected. I'm not sure how much work that'd be to change though.
image

You are right. I think we should discuss it with @monina-n and address it on another PR.

I'm not sure if this was something we were trying to address in this PR but an empty collection has it's status displayed and is selectable for bulk actions. I think that might be an existing issue though. I added it to the meta ticket here: #91843

image

Yes, it wasn't supposed to be addressed in this PR. Maybe this #91863 gonna address it.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2204 2202 -2

Async chunks

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

id before after diff
securitySolution 7.8MB 7.8MB +1.2KB

Page load bundle

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

id before after diff
securitySolution 235.9KB 235.8KB -127.0B

History

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

cc @cnasikas

@cnasikas cnasikas merged commit c2877a6 into elastic:master Feb 26, 2021
@cnasikas cnasikas deleted the fix_subcases_bugs branch February 26, 2021 13:35
cnasikas added a commit that referenced this pull request Feb 26, 2021
…ew (#91836) (#92966)

Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co>

Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 26, 2021
…bana into task-manager/docs-monitoring

* 'task-manager/docs-monitoring' of github.com:gmmorris/kibana: (40 commits)
  [Security Solution][Case][Bug] Improve case logging (elastic#91924)
  [Alerts][Doc] Added README documentation for alerts plugin status and framework health checks configuration options. (elastic#92761)
  Add warning for EQL and Threshold rules if exception list contains value list items (elastic#92914)
  [Security Solution][Case] Fix subcases bugs on detections and case view (elastic#91836)
  [APM] Always allow access to Profiling via URL (elastic#92889)
  [Vega] Allow image loading without CORS policy by changing the default to crossOrigin=null (elastic#91991)
  skip flaky suite (elastic#92114)
  [APM] Fix for default fields in correlations view (elastic#91868) (elastic#92090)
  chore(NA): bump bazelisk to v1.7.5 (elastic#92905)
  [Maps] fix selecting EMS basemap does not populate input (elastic#92711)
  API docs (elastic#92827)
  [kbn/test] add import/export support to KbnClient (elastic#92526)
  Test fix management scripted field filter functional test and unskip it  (elastic#92756)
  [App Search] Create Curation view/functionality (elastic#92560)
  [Reporting/Discover] include the document's entire set of fields (elastic#92730)
  [Fleet] Add new index to fleet for artifacts being served out of fleet-server (elastic#92860)
  [Alerts][Doc] Added README documentation for API key invalidation configuration options. (elastic#92757)
  [Discover][docs] Add search for relevance (elastic#90611)
  [Alerts][Docs] Extended README.md and the user docs with the licensing information. (elastic#92564)
  [7.12][Telemetry] Security telemetry allowlist fix. (elastic#92850)
  ...
cnasikas added a commit to cnasikas/kibana that referenced this pull request Feb 27, 2021
…ew (elastic#91836)

Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co>
# Conflicts:
#	x-pack/plugins/security_solution/public/cases/components/status/config.ts
cnasikas added a commit that referenced this pull request Feb 27, 2021
…ew (#91836) (#92967)

Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co>
# Conflicts:
#	x-pack/plugins/security_solution/public/cases/components/status/config.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team v7.12.0 v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants