-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 team: AWP] [Session view] Add alert fly out callback #127991
Conversation
// react-query caching keys | ||
export const QUERY_KEY_PROCESS_EVENTS = 'sessionViewProcessEvents'; | ||
export const QUERY_KEY_ALERTS = 'sessionViewAlerts'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.keys(updatedAlertsStatus).forEach((alertUuid) => { | ||
const process = processMap[updatedAlertsStatus[alertUuid].processEntityId]; | ||
if (process) { | ||
updateAlertEventStatus(process.getAlerts(), updatedAlertsStatus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since getAlerts()
comes from ProcessImpl
, what about adding an update function to ProcessImpl
? i.e process.setAlerts()
, to make more explicit the intention of changing alerts in the process
then also, could change to a more functional approach using map transforms:
i.e:
process.setAlerts(updateAlertEventStatus(process.getAlerts(), updatedAlertsStatus))
// updateAlertEventStatus
export const updateAlertEventStatus = (
events: ProcessEvent[],
updatedAlertsStatus: UpdateAlertStatus
) => (
events.map((event) => ({
...event, kibana: { ...event.kibana, alert: {
...event.kibana.alert,
workflow_status : updatedAlertsStatus[event.kibana.alert.uuid].status
}}
)})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alerts currently are placed together with events in process.events
. It's probably a good idea to separate them to process.events
and process.alerts
. It seems to relate to some changes made in #127500 in efforts to separate fetching events and alerts. Should we make the optimization in a separate PR?
cc: @mitodrummer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, separate PR seems best. Maybe after i merge the alerts tab stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah another PR sounds nice to me too
) => void; | ||
} | ||
|
||
export interface UpdateAlertStatus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type seems too generic and doesn't seem to tell much about it, can it be improved somehow?
}; | ||
|
||
export const searchAlertByUuid = async (client: ElasticsearchClient, alertUuid: string) => { | ||
const search = await client.search({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to update this to use the AlertsClient once I've merged my alerts tab work. It handles authorized indexes and other rbac type stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I can do it in a separate PR since the flyout callback is also needed in your PR. I will get this one merged in first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -73,4 +75,46 @@ describe('process tree hook helpers tests', () => { | |||
// session leader should have autoExpand to be true | |||
expect(processMap[SESSION_ENTITY_ID].autoExpand).toBeTruthy(); | |||
}); | |||
|
|||
it('updateAlertEventStatus works', () => { | |||
const events: ProcessEvent[] = JSON.parse(JSON.stringify([...mockEvents, ...mockAlerts])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is okay I think since we know the data, but using lodash clone might be safer long term and potentially faster? Not sure though...
) => { | ||
events.forEach((event) => { | ||
if (event.kibana?.alert.uuid && updatedAlertsStatus[event.kibana.alert.uuid]) { | ||
event.kibana.alert.workflow_status = updatedAlertsStatus[event.kibana.alert.uuid].status; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Kibana, I would never assume you'll actually get the data so I would do updatedAlertStatus[event.kibana?.alert?.uuid]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for letting me know! There are other places in session_view that will require similar fixes. I will fix this one here and create a tech debt ticket to address the rest separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
@@ -36,6 +37,11 @@ interface ProcessTreeDeps { | |||
selectedProcess?: Process | null; | |||
onProcessSelected: (process: Process | null) => void; | |||
setSearchResults?: (results: Process[]) => void; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏾
...updatedAlertsStatus, | ||
[alertUuid]: { | ||
status: events[0].kibana?.alert.workflow_status ?? '', | ||
processEntityId: events[0].process.entity_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
processEntityId: events[0].process.entity_id, | |
processEntityId: events[0]?.process?.entity_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should always be there, but best to err on the side of caution
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
) => | ||
events.map((event) => { | ||
// do nothing if event is not an alert | ||
if (!event.kibana) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do in a follow up PR, but in the future event.kibana
may have other sub-properties, so may be best to use event?.kibana?.alert
to be explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Michael, will update in my follow up PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this work! LGTM 🚀
Issue: https://github.com/elastic/security-team/issues/3266
TODO: Add tests for
alert_status_route
and useAlertsClient
after #127500 is merged