-
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
[TIP] Add to timeline #138836
[TIP] Add to timeline #138836
Conversation
8601213
to
4fc2b5d
Compare
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 to me overall, the only issue is that column mapping thing I pointed out. Good job:)
x-pack/plugins/security_solution/public/threat_intelligence/routes.tsx
Outdated
Show resolved
Hide resolved
...reat_intelligence/public/modules/indicators/components/indicators_table/indicators_table.tsx
Outdated
Show resolved
Hide resolved
I have an overall question: are there any specific reasons why threat intelligence plugin made the decision to implement own data grid instead of reuse T-Grid which is used by security_solution and observability? This data grid already contains all the row and bulk actions, which is configurable. For example in kibana/x-pack/plugins/security_solution/public/common/components/sessions_viewer/index.tsx Lines 86 to 99 in 69ef96f
|
Hey Yuliia! Well. we did start working on threat intel around july, and I was not aware of TGrid up until the beginning of august, when I stumbled upon it working on adding fields browser to our thing. Shortly after, there was a presentation about it that you made - this is why our current implementation skips the existing API. One of our primary goals was to kick off the development early, with all of us discovering kibana codebase along the way. It's just my view, thoughts @maxcold @PhilippeOberti ? |
x-pack/plugins/threat_intelligence/cypress/integration/timeline.spec.ts
Outdated
Show resolved
Hide resolved
hi @YulNaumenko, thanks for the comment! Our team is pretty new and I'm the freshest of all of us, so my knowledge of the Kibana repo is still very limited 😄 @maxcold do you want me to create a tech debt ticket now or we'll discuss about it in our Tech Planning meeting in a few hours? |
@PhilippeOberti @lgmys let's discuss it in our tech planning, we might want to do an investigation first if @YulNaumenko thanks a lot for investing your time in reviewing the PR, it's great to have someone with a lot of experience with Kibana dev looking at our code as we are all new to Elastic!
I hope my long comment and newbie feedback on the current component state won't stop you from giving us advice in the future on what is available in security_solution and what can we reuse. As I said in the beginning it is super helpful for us! |
@maxcold yes, I agree that TGrid is overloaded with the features. Basically TGrid has two interfaces - the simpler one is for Observability and more feature loaded for Security. Another thing about the roadmap for TGrid - together with ResponseOps team we are planning migrate it to the new table currently exposed by triggers_actions_ui plugin. I recommend you take a look (cc @XavierM): kibana/x-pack/plugins/triggers_actions_ui/public/plugin.ts Lines 108 to 109 in 2b15e2a
Now you can find the example in Cases UI:
To be honest - No. And that's why we are working now on the architecture redesign of this component.
For now it probably could slow you down, but for confirming this assumption I need to know you upcoming requirements and feature on top of the data grid
We can discuss the details of this requirement.
Agree, but recommend you to review the new table designed by ResponseOps, which I've mentioned above. Again, we have an upcoming changes to simplify and separate TGrid by reusable parts.
Unfortunately this is a known problem. Security teams are working now to make a documentation better and add the visibility over the code base. Please, help us to move this initiative.
Please, invite me to the discussion if it makes sense for the team. I'm happy to help. Feel free to reach out with any technical questions you have. |
@YulNaumenko thanks a lot for more context on the TGrid! I created an issue in our backlog to play with TGrid for our Indicators view #138836 (comment), feel free to add any context you think is missing there
this is a hard part as we don't yet have a long-term vision for our feature, we want to put it out there and get feedback from our users before planning too far ahead. Filtering in/out, sorting and integration with Cases are on our roadmap for sure
You can find more details in this issue https://github.com/elastic/security-team/issues/4558
We had this discussion last week, where we agreed that we need to investigate TGrid capabilities closer but also decided that it's not a top priority for us right now. We will keep in mind that you are happy to help us with the context on TGrid when we get to building some POC with it |
``` | ||
yarn kbn reset && yarn kbn bootstrap | ||
yarn start --no-base-path | ||
``` | ||
|
||
> **Notes:** | ||
> | ||
> It is recommended to set `server.basePath: "/kbn"` to make you local instance persist the base Kibana path, otherwise the base path will be a random string every time you start Kibana. Any other value than `/kbn` will also work. |
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.
while this plugin is the only artifact that we own, should we just link to https://docs.elastic.dev/security-solution/protections/protections-experience-team/dev-setup to have one source of truth? (or wise-versa ofc if you think README is a better place )
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.
yup I'm ok with that, I'm modifying this README to point to the dev-setup one!
return result; | ||
}, [component, dataProvider, field]); | ||
|
||
return value === EMPTY_VALUE ? ( |
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.
does it make sense to return early from the component?
smth like
if (!value) { return <></> }
right after getting the value
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.
unless I'm mistaken you shouldn't use useMemo (or any other react hooks) conditionally. By that I mean I believe it's not good to have a early return on a component and have a react hook after that. If that is true I don't think you can return this <></>
empty component any earlier than it is currently...
Am I mistaken?
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.
yes you are right, couple of thoughts:
- one of the reasons I wasn't really sold on the idea of using memorization extensively without clearly measured performance gains was that it comes with a cost, in this case we need to adjust the way we code to the fact that we use hooks. not really specific to
useMemo
but to all hooks ofc - When looking at the component I'm a bit confused with the fact that we wrap the whole component in
memo
HOC and also useuseMemo
inside. If the component is wrapped inmemo
are there gains of usinguseMemo
inside it additionally? - initially the usage of
EMPTY_VALUE
caught my eye. Do we really need this logic of assigning EMPTY_VALUE and then checking against it? We used EMPTY_VALUE as an UI component for an empty cell, I wouldn't introduce it if it doesn't serve the purpose of displaying-
somewhere
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.
I took the opportunity to read more about useMemo
vs React.memo
when to use them when to now.
I feel like this addToTimeline
component for sure doesn't need to be wrapped by a memo
.
Also, I feel like that the use of useMemo inside it is also not providing any positive value (I was doing useMemo on the creation of a basic object, not any complex heavy logic..)
I have removed both of them (to be honest the code was lifted off of a Security Solution component). This allows me like you mentioned to early return if value
is undefined.
I will be more careful in the future about when to use vs not to use this. I also ned to learn about performance testing with React!
Thanks for challenging me on this!
@PhilippeOberti two comments after running the branch locally:
|
one comment on my comment :)
I'm also totally ok with having this covered in a separate 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.
On the current stage of the discussion the timelines changes LGTM
0062ae9
to
5a36c05
Compare
|
9306a89
to
c3006c0
Compare
- add button to all cells in the indicators table - add button to a new actions column for the indicator flyout table - add button to barchart legend - create new providers (kibana context and security context) for unit tests and storybook - change all Storybook console.log to window.alert to follow the EUI pattern - fix broken Storybook indicator table story elastic/security-team#4557
c3006c0
to
c1cbf89
Compare
if (typeof data === 'string') { | ||
value = data; | ||
} else if (field === ComputedIndicatorFieldId.DisplayValue) { | ||
field = indicatorTypeToFieldMap[unwrapValue(data, RawIndicatorFieldId.Type) || '']; |
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.
should we add some helper function in indicators/lib
to get the display_name original field by an IoC type? We will probably need it for other features, eg. for filter in/out
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.
I had thought about it but decided against it as it's only a one liner... I will extract it!
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.
agree, don't have a strong opinion, maybe it's a preemptive optimisation as well :)
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.
extracted like we discussed, colocated with the displayValue method
import { AddToTimeline } from './add_to_timeline'; | ||
import { TestProvidersComponent } from '../../../../common/mocks/test_providers'; | ||
|
||
describe('<AddToTimeline />', () => { |
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.
I think we need to add more test cases to cover the logic for display value
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.
good idea! When I first wrote this component it was a lot simpler, but now that it has more logic I need to add more test cases!
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.
added! Now supporting:
- good value/field input (both string and Indicator type)
- value is EMPTY_VALUE has input
- calculated value is null
- field doesn't exist in the Indicator
- field exist but has an unknown value (meaning NOT
url
,file
orip
for now)
export const indicatorTypeToFieldMap: { [id: string]: RawIndicatorFieldId } = { | ||
file: RawIndicatorFieldId.FileSha256, | ||
url: RawIndicatorFieldId.UrlFull, | ||
ip: RawIndicatorFieldId.Ip, |
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.
I believe this won't work for ip
indicator. The type
possible values for the IP indicator are ipv4-addr
and ipv6-addr
. In general, I'd think of how we could avoid having this mapping separate from the logic we have in display_value.ts
file. Every time we add support for a new type of indicator we would need to remember to add the mapping here, which is very error prone
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.
I know but couldn't find a better way to do this. I stole the logic from here where we return the actual value to display. We have a ruleArray with only 3 options: isIpIndicator
, isUrlIndicator
and isFileIndicator
with the extract method hardcoded. To your knowledge do we have a better solution to do this?
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.
my main point that the logic in add_to_timeline.tsx
field = indicatorTypeToFieldMap[unwrapValue(data, RawIndicatorFieldId.Type) || ''];
won't even work, because unwrapValue(data, RawIndicatorFieldId.Type)
will return ipv4-addr
and you don't have a mapping for that. Ofc the simplest would be to add all the possible keys from https://docs.google.com/spreadsheets/d/1oYx9CQodKVmr0OtlJcjm1mNoUBlDcbHai0UYxtP8YzY/edit#gid=1814426429
file
url
email-addr
email (doesn't exist in STIX 2.0)
email-message
domain-name
domain
ipv4-addr
ipv6-addr
x509-certificate
x509 Serial (doesn't exist in STIX 2.0)
windows-registry-key
autonomous-system
suppress (doesn't exist in STIX 2.0)
mac-addr
unknown
to the mapping, but then when a new type is added I'm 99% confident that we won't add it to the mapping until a user complain :)
Let me think on how we could awoid that
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.
ok thanks for the pointer, I will spend time thinking about a good way to handle this!
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.
for now still with a very basic key-value object, like discussed a cleaner work will be done in this ticket
if (typeof data === 'string') { | ||
value = data; | ||
} else if (field === ComputedIndicatorFieldId.DisplayValue) { | ||
field = indicatorTypeToFieldMap[unwrapValue(data, RawIndicatorFieldId.Type) || '']; |
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.
unwrapValue
can be null
, I think that's because you have || ''
there but I'd prefer more direct handling of null
case. Right now it's a bit unclear what will be rendered if unwrapValue
returns null
, to me it seems that the component will go in some undefined state
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.
good point!
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 now handled, along side an EMPTY_VALUE I had forgotten to support and that my newly added unit tests revealed :)
@@ -5,11 +5,14 @@ | |||
* 2.0. | |||
*/ | |||
|
|||
export function mockUiSetting(key: string): string | undefined { |
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.
very weird diff, is it just git
incorrectly interpreting it as a rename of the file?
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 for some reason git thinks I've renamed the file. Let me try to delete and recreate it real quick...
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.
didn't fix this as I would have had to amend the commit instead of adding a new one...
- still using basic key value pair for now - add more unit tests
*/ | ||
const indicatorTypeToField: { [id: string]: RawIndicatorFieldId } = { | ||
file: RawIndicatorFieldId.FileSha256, | ||
'ipv4-addr': RawIndicatorFieldId.UrlFull, |
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.
I believe ipv4-addr
and ipv6-addr
should have replaced ip
mapping, not the url
one
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.
🤦
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
* [TIP] Add to timeline - add button to all cells in the indicators table - add button to a new actions column for the indicator flyout table - add button to barchart legend - create new providers (kibana context and security context) for unit tests and storybook - change all Storybook console.log to window.alert to follow the EUI pattern - fix broken Storybook indicator table story elastic/security-team#4557
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
This PR adds buttons to multiple places in the indicators page, to add a field/value pair to an untitled timeline. This mimics the functionality of the Alerts page. Here are the 3 places the
Add to Timeline
icon button is added to:-
)Actions
column of the table, as this one will have theInvestigate in Timeline
button handled in a future PR (see this ticket)This PR also cleans up a bit our Storybook stories by extracting some logic out (KibanaContext and SecuritySoluetionContext).
To Do
https://github.com/elastic/security-team/issues/4557
Screen.Recording.2022-08-15.at.6.16.12.PM.mov
How/what to test
When clicking on one of the 3 add to timeline buttons, an entry will be added to the untitled timeline (accessible by clicking on the
Untitled timeline
button at the bottom left of the screen. The timeline should have a new entry, with the indicator field and its value.Checklist
Delete any items that are not applicable to this PR.