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] Populate alert status auditing fields #171589

Merged
merged 15 commits into from Dec 5, 2023

Conversation

marshallmain
Copy link
Contributor

@marshallmain marshallmain commented Nov 20, 2023

This PR populates the existing kibana.alert.workflow_user field in the alerts-as-data mappings with the profile_uid of the last user to modify the status of the alert. It also adds a new field, kibana.alert.workflow_status_updated_at, to track the last time the workflow status was updated and populates it with a timestamp.

Similar to the alert assignment PR, workflow_user renders in the table with a user avatar instead of the raw profile_uid value stored in the alert. The filter in/out buttons on the row cell automatically add a filter that uses the raw value so that filtering works correctly.

Due to limitations of Kibana's user profile implementation, workflow_user is only populated if a user changes the alert status using the alert status route (POST /api/detection_engine/signals/status) within an interactive session, i.e. logs in rather than passes credentials with each API request (related issue).

Alerts table

image

Alert details

image

Checklist

@marshallmain marshallmain marked this pull request as ready for review November 22, 2023 16:20
@marshallmain marshallmain requested review from a team as code owners November 22, 2023 16:20
@marshallmain marshallmain added release_note:enhancement Team:Detection Engine Security Solution Detection Engine Area v8.12.0 Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Nov 22, 2023
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@marshallmain marshallmain added the Feature:Detection Alerts Security Solution Detection Alerts Feature label Nov 22, 2023
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Obs UX Changes LGTM !!

@pmuellr
Copy link
Member

pmuellr commented Nov 22, 2023

The ResponseOps changes besides the alert table changes LGTM. I'd like to have someone from RAM review the alert table changes since I'm not sure what the ramifications of those changes are. @XavierM ?

@marshallmain marshallmain added ci:cloud-deploy Create or update a Cloud deployment ci:project-deploy-security Create a Security Project labels Nov 28, 2023
Copy link
Contributor

@e40pud e40pud left a comment

Choose a reason for hiding this comment

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

LGTM!

const getUpdateSignalStatusScript = (status: SetSignalsStatusSchemaDecoded['status']) => ({
source: `if (ctx._source['${ALERT_WORKFLOW_STATUS}'] != null) {
ctx._source['${ALERT_WORKFLOW_STATUS}'] = '${status}'
const getUpdateSignalStatusScript = (
Copy link
Contributor

Choose a reason for hiding this comment

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

This only returns user profiles that are enabled right? Is a profile enabled automatically on login? Would there be a case of someone being logged onto the UI with a disabled profile and so this status is not logged?

@@ -39,6 +42,7 @@ import { ALERTS_URL } from '../../../urls/navigation';

// FLAKY: https://github.com/elastic/kibana/issues/169091
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a PR trying to fix the flake, just haven't gotten back to it yet! #171676

@yctercero
Copy link
Contributor

Hey @marshallmain - sorry I just got to doing a very quick overview today. I plan to pull down and test tomorrow. Could we add the new PR checklist to the description? Thanks so much!

Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

Thanks for adding this in the table and the alert details flyout @marshallmain!

I left a few small comments.

Also I'm thinking that this could be a good candidate for some Cypress tests? We already have everything setup for you, and even have one test for each action performed on the alert (this one when closing an alert and that one when acknowledging an alert). You probably would have to update the page filters to show the changed alert, reopen the flyout and test that the new section correctly shows up under the About section. Wdyt?
(I tried manually once a test was finished and it worked, so we should be able to do this programmatically without any issues). I'm happy to help out on this if you need!

I also have a question regarding the UI. Was adding this new section validated by UIUX? It seems a bit weird to me, and I would have thought instead to add something in the header next to the already existing status component (by probably updating this status.tsx file).

Finally, when the Mitre Attack component is rendered, the spacing is a bit small between mitre attack and this new status component. The challenge is both these are conditionally rendered. What do you think about removing this <EuiSpacer size="m" /> here and add one <EuiSpacer size="m" /> at the top of both the mitre_attack component and your new alert_status? That way the spacing will always look nice, when the components are visible or not.

import { useBulkGetUserProfiles } from '../../../../common/components/user_profiles/use_bulk_get_user_profiles';
import { PreferenceFormattedDate } from '../../../../common/components/formatted_date';

export const AlertStatus: FC = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please add a short documentation here, just to be consistent with all the other components in the document_details folder?

Comment on lines +20 to +21
const statusUpdatedBy = searchHit.fields?.['kibana.alert.workflow_user'] as string;
const statusUpdatedAt = searchHit.fields?.['kibana.alert.workflow_status_updated_at'];
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for not using getFieldsData along side our x-pack/plugins/security_solution/public/flyout/document_details/shared/utils.tsx like we use in the rest of the document_details flyout code?

The getFieldsData is available from the useRightPanelContext you're already using.

It would be

const { getFieldsData } = useRightPanelContext();

const statusUpdatedBy = getField(getFieldsData('kibana.alert.workflow_user'));
const statusUpdatedAt = getField(getFieldsData('kibana.alert.workflow_status_updated_at'));

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 figured getting the raw data from the searchHit was simple enough, I can change it though

@@ -43,6 +43,10 @@ const MITRE_ATTACK_TEST_ID = `${PREFIX}MitreAttack` as const;
export const MITRE_ATTACK_TITLE_TEST_ID = `${MITRE_ATTACK_TEST_ID}Title` as const;
export const MITRE_ATTACK_DETAILS_TEST_ID = `${MITRE_ATTACK_TEST_ID}Details` as const;

export const WORKFLOW_STATUS_TEST_ID = `${PREFIX}WorkflowStatus` as const;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit but this one doesn't need to be exported

@marshallmain
Copy link
Contributor Author

I also have a question regarding the UI. Was adding this new section validated by UIUX? It seems a bit weird to me, and I would have thought instead to add something in the header next to the already existing status component (by probably updating this status.tsx file).

Is there a specific person that needs to approve changes to the flyout? I avoided adding it to the header because it looked cluttered to me already and adding status history information didn't seem to fit with the rest of the basic info that's displayed in the header.

I'm working on making the other suggested changes to the flyout, thanks for the review!

@kibana-ci
Copy link
Collaborator

kibana-ci commented Dec 5, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4773 4774 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/rule-data-utils 115 116 +1

Async chunks

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

id before after diff
apm 3.7MB 3.7MB +42.0B
infra 1.9MB 1.9MB +42.0B
observability 1.1MB 1.1MB +42.0B
securitySolution 12.8MB 12.8MB +3.9KB
triggersActionsUi 1.4MB 1.4MB +42.0B
total +4.0KB

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5661 +5661
total size - 5.9MB +5.9MB

Page load bundle

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

id before after diff
apm 36.7KB 36.8KB +73.0B
infra 103.0KB 103.1KB +73.0B
observability 101.8KB 101.9KB +73.0B
triggersActionsUi 104.1KB 104.2KB +73.0B
total +292.0B
Unknown metric groups

API count

id before after diff
@kbn/rule-data-utils 118 119 +1

References to deprecated APIs

id before after diff
securitySolution 560 559 -1

History

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

@PhilippeOberti
Copy link
Contributor

I also have a question regarding the UI. Was adding this new section validated by UIUX? It seems a bit weird to me, and I would have thought instead to add something in the header next to the already existing status component (by probably updating this status.tsx file).

Is there a specific person that needs to approve changes to the flyout? I avoided adding it to the header because it looked cluttered to me already and adding status history information didn't seem to fit with the rest of the basic info that's displayed in the header.

I'm working on making the other suggested changes to the flyout, thanks for the review!

@marshallmain: @ferenrigue owns the flyout design but she is on vacation right now. We could ask @codearos but if you want we can merge this as is and I'm happy to make small UI changes later once UIUX comes up with a solution they like?

Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

LGTM for the Threat Hunting Investigations team, thanks for making the changes @marshallmain

@marshallmain marshallmain merged commit 4a89208 into elastic:main Dec 5, 2023
38 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 5, 2023
@marshallmain marshallmain deleted the workflow-user-updated-at-3 branch December 5, 2023 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment ci:project-deploy-security Create a Security Project Feature:Detection Alerts Security Solution Detection Alerts Feature release_note:enhancement Team:Detection Engine Security Solution Detection Engine Area Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants