-
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 Solution][RAC] - Add reason field #107532
[Security Solution][RAC] - Add reason field #107532
Conversation
d264522
to
2218a3b
Compare
1cf3196
to
8d40ac5
Compare
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
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.
Looks good! There's one small potential change regarding risk score and severity overrides. Great to see we'll be able to easily define different reason messages for each rule type!
.../security_solution/server/lib/detection_engine/rule_types/factories/utils/build_bulk_body.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/signals/build_bulk_body.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/signals/build_bulk_body.ts
Outdated
Show resolved
Hide resolved
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 looks good to me. Good work!
I left two minor nitpicking comments. Could you take a look?
export type BuildReasonMessage = (args: BuildReasonMessageArgs) => string; | ||
|
||
/** | ||
* Currently all security solution rule types share a commone reason message string. This function composes that string |
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.
commone
typo ?
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.
updated, thanks!
if (mergedDoc?.fields) { | ||
hostName = mergedDoc.fields['host.name'] != null ? mergedDoc.fields['host.name'] : hostName; | ||
userName = mergedDoc.fields['user.name'] != null ? mergedDoc.fields['user.name'] : userName; | ||
timestampForReason = |
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.
nit: This ternary operator chain is a bit hard to read. I think that a simple if/else statement might be more readable.
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, I changed it. Thanks!
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 just be the passed in timestamp. So removed all this logic
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
💔 Backport failed
To backport manually run: |
# Conflicts: # x-pack/plugins/security_solution/cypress/integration/detection_alerts/alerts_details.spec.ts
# Conflicts: # x-pack/plugins/security_solution/cypress/integration/detection_alerts/alerts_details.spec.ts
Summary
signal.reason
/kibana.alert.reason
field in security_solution as seen below.