[SIEM] Added graph visualization to entity's flyout#260174
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
albertoblaz
left a comment
There was a problem hiding this comment.
A few non-blocking comments
| (isSingleEntity || isGroupedEntities) && onOpenEventPreview !== undefined, | ||
| }, | ||
| isEntityRelationshipsExpanded: isEntityRelationshipExpandedForScope(scopeId, node.id), | ||
| isInitialEntity, |
There was a problem hiding this comment.
I think we should also propagate this field in entity_actions_button.tsx to have a similar behavior when updating filters from the flyout section
There was a problem hiding this comment.
agreed, right now I couldn't have a use case where we have grouped entities
I'll continue to explore
| | EVAL actorDocData = CONCAT(${JSON_OBJECT_START}, | ||
| ${concatJsonObjectPropertyEsqlExprSafe('id', 'entity.id')}, | ||
| ${JSON_OBJECT_SEPARATOR}, ${concatJsonObjectPropertyString('type', 'entity')}, | ||
| ${JSON_OBJECT_SEPARATOR}, "\\"entity\\":", ${JSON_OBJECT_START}, | ||
| ${concatJsonObjectPropertyBool('availableInEntityStore', true)}, | ||
| ${JSON_OBJECT_SEPARATOR}, ${concatJsonObjectPropertyString( | ||
| 'ecsParentField', | ||
| ecsParentFieldValue | ||
| )}, | ||
| ${JSON_OBJECT_SEPARATOR}, ${concatJsonObjectPropertyEsqlExprSafe('name', 'entity.name')}, | ||
| ${JSON_OBJECT_SEPARATOR}, ${concatJsonObjectPropertyEsqlExprSafe('type', 'entity.type')}, | ||
| ${JSON_OBJECT_SEPARATOR}, ${concatJsonObjectPropertyEsqlExprSafe( | ||
| 'sub_type', | ||
| 'entity.sub_type' | ||
| )}, | ||
| CASE( | ||
| host.ip IS NOT NULL, | ||
| CONCAT(${JSON_OBJECT_SEPARATOR}, "\\"host\\":", ${JSON_OBJECT_START}, | ||
| "\\"ip\\":\\"", TO_STRING(host.ip), "\\"", | ||
| ${JSON_OBJECT_END}), | ||
| "" | ||
| ), | ||
| ${JSON_OBJECT_END}, | ||
| ${JSON_OBJECT_END}) |
There was a problem hiding this comment.
I love these helpers, I also suffered when changing this block with all the escaping etc.
I'd just make the delimiters and function names less verbose. I miss a helper for adding nested field such as "\\"entity\\" or "\\"host\\":" and another one for conditionally adding fields like the "host" inner object. But could be refactored in a separate PR
| | EVAL type = entity.type | ||
| | EVAL sub_type = entity.sub_type | ||
| | EVAL ecsParentField = CASE(entity.EngineMetadata.Type == "generic", "entity", entity.EngineMetadata.Type) | ||
| | EVAL docData = CONCAT(${JSON_OBJECT_START}, |
There was a problem hiding this comment.
For a separate PR: I would extract lines 304-333 into an auxiliary function to build the docData and keep the main function lean
This comment was marked as resolved.
This comment was marked as resolved.
afca318 to
886869c
Compare
|
Pinging @elastic/siem (Team:SIEM) |
| scopeId, | ||
| isPreviewMode: true, | ||
| banner: GENERIC_ENTITY_PREVIEW_BANNER, | ||
| isEngineMetadataExist: !!entity, |
There was a problem hiding this comment.
🟡 Medium hooks/use_open_entity_preview_panel.ts:57
isEngineMetadataExist is set to !!entity, which always evaluates to true because entity is a required parameter already dereferenced on line 23. The caller in EntityActionsButton previously passed !!item.entity.availableInEntityStore to indicate whether entity metadata exists in the store. Consider using !!entity.availableInEntityStore to preserve the intended semantics.
| isEngineMetadataExist: !!entity, | |
| isEngineMetadataExist: !!entity.availableInEntityStore, |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file x-pack/solutions/security/packages/kbn-cloud-security-posture/graph/src/components/graph_grouped_node_preview_panel/hooks/use_open_entity_preview_panel.ts around line 57:
`isEngineMetadataExist` is set to `!!entity`, which always evaluates to `true` because `entity` is a required parameter already dereferenced on line 23. The caller in `EntityActionsButton` previously passed `!!item.entity.availableInEntityStore` to indicate whether entity metadata exists in the store. Consider using `!!entity.availableInEntityStore` to preserve the intended semantics.
Evidence trail:
x-pack/solutions/security/packages/kbn-cloud-security-posture/graph/src/components/graph_grouped_node_preview_panel/hooks/use_open_entity_preview_panel.ts:22 (function signature shows entity as required), :23 (entity dereferenced), :57 (isEngineMetadataExist: !!entity). Commit c5cc04a2010f324a9fb3abc5223d2ad977c68935 shows the previous behavior in entity_actions_button.tsx used `isEngineMetadataExist: !!item.availableInEntityStore`.
…ty is not found (#261253) <!-- Macroscope (Fix It For Me) template starts here --> ### Macroscope: _Fix It For Me_ - This PR originated from [this comment](https://github.com/elastic/kibana/pull/260174/files#r3035574157) in #260174. - Since auto-merge is on, Macroscope will merge this PR after waiting for checks to pass. - If you'd rather not wait, you can always merge this yourself but **no further action from you is currently needed**. - You can also @mention Macroscope in this PR to request further changes. #### Activity Currently: <!-- Macroscope (Fix It For Me) current status starts here -->Not merged: unstable<!-- Macroscope (Fix It For Me) current status ends here --> <details> <summary>Previously</summary> <!-- Macroscope (Fix It For Me) previous status starts here --> - Waiting on checks - Pushed 906f8e6 <!-- Macroscope (Fix It For Me) previous status ends here --> </details> ---- <!-- Macroscope (Fix It For Me) template ends here --> <!-- Macroscope's pull request summary starts here --> <!-- Macroscope will only edit the content between these invisible markers, and the markers themselves will not be visible in the GitHub rendered markdown. --> <!-- If you delete either of the start / end markers from your PR's description, Macroscope will append its summary at the bottom of the description. --> <!-- Macroscope's pull request summary ends here --> Co-authored-by: macroscopeapp[bot] <170038800+macroscopeapp[bot]@users.noreply.github.com>
| if (firstItem && firstItem.itemType === DOCUMENT_TYPE_ENTITY) { | ||
| icon = firstItem.icon ?? icon; | ||
| groupedItemsType = capitalize(`${firstItem.type}s`) || 'Entities'; | ||
| groupedItemsType = capitalize(`${firstItem.entity?.type}s`) || 'Entities'; |
There was a problem hiding this comment.
🟢 Low graph_grouped_node_preview_panel/graph_grouped_node_preview_panel.stories.tsx:100
When firstItem.entity?.type is undefined, the template literal coerces it to the string "undefined", producing "undefineds" which capitalizes to "Undefineds". Since this is truthy, the || 'Entities' fallback never executes and users see "Undefineds" instead of "Entities".
- groupedItemsType = capitalize(`${firstItem.entity?.type}s`) || 'Entities';
+ groupedItemsType = capitalize(`${firstItem.entity?.type || 'entity'}s`) || 'Entities';🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file x-pack/solutions/security/packages/kbn-cloud-security-posture/graph/src/components/graph_grouped_node_preview_panel/graph_grouped_node_preview_panel.stories.tsx around line 100:
When `firstItem.entity?.type` is `undefined`, the template literal coerces it to the string `"undefined"`, producing `"undefineds"` which capitalizes to `"Undefineds"`. Since this is truthy, the `|| 'Entities'` fallback never executes and users see "Undefineds" instead of "Entities".
Evidence trail:
x-pack/solutions/security/packages/kbn-cloud-security-posture/graph/src/components/graph_grouped_node_preview_panel/graph_grouped_node_preview_panel.stories.tsx lines 95-101 (at REVIEWED_COMMIT). Line 100 shows `groupedItemsType = capitalize(`${firstItem.entity?.type}s`) || 'Entities';`. Lines 95-96 show the capitalize function: `const capitalize = (str: string) => !str ? '' : str[0].toUpperCase() + str.slice(1).toLowerCase();`. JavaScript template literal coercion of undefined to string "undefined" is standard ECMAScript behavior.
Flaky Test Runner Stats🎉 All tests passed! - kibana-flaky-test-suite-runner#11380[✅] x-pack/solutions/security/test/cloud_security_posture_api/config.ts: 25/25 tests passed. |
There was a problem hiding this comment.
Isn't this hook a duplication of x-pack/solutions/security/plugins/security_solution/public/flyout/document_details/shared/hooks/use_graph_preview.ts ?
CAWilson94
left a comment
There was a problem hiding this comment.
LGTM 🚀 @elastic/security-entity-analytics
Reviewed files owned by Entity Analytics only - one smol nit 🙏
| REPLACE( | ||
| REPLACE( | ||
| REPLACE(CONCAT("\\"sourceFields\\":", ${JSON_OBJECT_START}, ${properties}, ${JSON_OBJECT_END}), "[,]+", ","), | ||
| "\\\\{,", ${JSON_OBJECT_START}), | ||
| ",}", ${JSON_OBJECT_END})`; |
There was a problem hiding this comment.
🟢 Low graph/fetch_events_graph.ts:378
The REPLACE(..., "[,]+", ",") pattern on line 380 corrupts field values that contain consecutive commas. For example, if a user.name value is "Test,, User", the resulting JSON "user.name":"Test,, User" becomes "user.name":"Test, User" after the replace operation, losing data. The old code used leading comma prefixes per property and did not have this data-loss risk. Consider removing the [+,]+ pattern or using a different approach that only cleans up the structural commas between properties, not commas inside quoted string values.
REPLACE(
REPLACE(CONCAT("\\"sourceFields\\":", ${JSON_OBJECT_START}, ${properties}, ${JSON_OBJECT_END}), "[,]+", ","),
"\\\\{,", ${JSON_OBJECT_START}),
",}", ${JSON_OBJECT_END})`;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file x-pack/solutions/security/plugins/cloud_security_posture/server/routes/graph/fetch_events_graph.ts around lines 378-382:
The `REPLACE(..., "[,]+", ",")` pattern on line 380 corrupts field values that contain consecutive commas. For example, if a `user.name` value is `"Test,, User"`, the resulting JSON `"user.name":"Test,, User"` becomes `"user.name":"Test, User"` after the replace operation, losing data. The old code used leading comma prefixes per property and did not have this data-loss risk. Consider removing the `[+,]+` pattern or using a different approach that only cleans up the structural commas between properties, not commas inside quoted string values.
Evidence trail:
1. x-pack/solutions/security/plugins/cloud_security_posture/server/routes/graph/fetch_events_graph.ts lines 378-382 (REVIEWED_COMMIT) - shows the REPLACE with `"[,]+", ","` pattern
2. x-pack/solutions/security/plugins/cloud_security_posture/server/routes/graph/utils/esql_utils.ts lines 77-80 (REVIEWED_COMMIT) - `concatJsonObjectPropertyEsqlExprSafe` function showing values are inserted directly via CONCAT without comma escaping
3. git_diff between MERGE_BASE and REVIEWED_COMMIT showing the change from `REPLACE(..., "\\{,", "{")` (only fixing `{,` pattern) to the new three-REPLACE approach including `"[,]+"`
4. Elasticsearch ES|QL REPLACE documentation: https://www.elastic.co/docs/reference/query-languages/esql/functions-operators/string-functions/replace - confirms REPLACE operates on entire string with regex matching
There was a problem hiding this comment.
currently not an issue, we can ignore for now
⏳ Build in-progress
Failed CI StepsTest Failures
History
|
Summary
Adds visualization section to host, user and service entity flyout.
How to test this PR:
org-datausingsecurity-documents-generatorStorybooks:
Out of scope:
Checklist