elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.81k stars 8.2k forks source link

[SIEM] Detection Engine Design Review #55753

Closed MichaelMarcialis closed 3 years ago

MichaelMarcialis commented 4 years ago

First, kudos and thanks to everyone involved in the new detections feature. I'm so impressed with what this team was able to accomplish in a short amount of time.

The purpose of this issue is to note any deviations from the designs, UX issues and bugs from a design perspective for the detections feature. Please note that the expectation is not that these will all be required for the 7.6 release. Instead, I will setup a follow-up meeting for the relevant team members to discuss and prioritize if and when these suggested changes should be made.

CCing @XavierM, @spong, @patrykkopycinski, @tsg, @mchopda.

General

All Page Headers

UX Priority Issue Notes Done
đź’›Medium The EuiBetaBadge appears in every page header across the entire detections section. I think it would be less confusing and sufficient to just include the badge on the first, main detections page only. For example, with the way we have it now, it may give the false impression that the rules themselves are in a beta state. Can address in 7.7. +1

All Utility Bars

UX Priority Issue Notes Done
đź’šLow The "Showing" and the "Selected" text present in the UtilityBar components throughout detections are missing their colon (:) suffixes. Can we change to "Showing:" and "Selected:" respectively? Can address in 7.7. +1
❤️High For all instances of UtilityBar, only the Showing: {x} {things} text should be visible to the user by default. Currently it appears as if everything that can be shown in a UtilityBar is being showing at all times. Examples of the intended flow can be seen in the original wireframe prototypes.
  • The selected count and any bulk actions should not show until one or more table rows have been checked/selected by the user.
  • The select everything button should not show unless the user has opted to select all rows on the current page.
Can address in 7.7. +1
đź’šLow ~The Select all {x} {things} text currently feels redundant, as we already tell the user how many things are showing at the beginning of each UtilityBar. Can we change it back to the text shown in the wireframes (Select all {things} on all pages)? Or was this changed because the concept of "pages" doesn't really exist for event viewer tables?~ Considering a non-issue until pagination enhancements can be made to the event viewer tables. N/A

All Event Viewer Tables

UX Priority Issue Notes Done
đź’šLow When there is no data to display in a table, the left-hand select all checkbox is still functional. Can we disable this when the table has no rows to select? Can address in 7.7. âś…
đź’šLow On small viewports, the table footer contents overflow out of their panel container. Will address during timeline redesign (i.e. history feature, etc.) project. Timeframe to be determined. +1
đź’šLow In the table footer, please change the casing of the units being counted to lowercase. Can address in 7.6.1. +1
đź’šLow ~In the table footer, please change the casing of "Load More" button to be "Load more".~ N/A with pagination N/A

Detections Page

Signals Histogram

UX Priority Issue Notes Done
đź’šLow Change the "Signal count" histogram header text to be "Signals detection count" to be consistent with the alerts histogram title. Applies to rule details page as well. Can address in 7.7. âś…
đź’›Medium It looks like the signals histogram currently displays each individual risk score in the legend. The original wireframes showed the risk scores grouped in ranges, attributed to a keyword. With the discussions we had regarding severity versus risk score, it probably no longer makes sense to attribute them to a keyword, but I think the grouping of risk score ranges still makes sense (and can potentially be easier to understand for situations with lots of signals). In short, could we use the following ranges? Applies to rule details page as well.
  • 0–24
  • 25–49
  • 50–74
  • 75–100
Can address in 7.7. Needs discussion
đź’›Medium For most cases, a random selection of EUI visualization colors is probably fine. However, in certain specific circumstances (such as signal.rule.risk_score and signal.rule.severity), it would make sense to use visualization colors that represent a positive-to-negative spectrum. For example, my original wireframes are using green ($euiColorVis0), yellow ($euiColorVis5), orange ($euiColorVis7) and red ($euiColorVis9). Would that be possible to do for these two stack dimensions? +1

Signals Table

UX Priority Issue Notes Done
đź’šLow For the initial loading state of this table, can we set the EuiLoadingContent component's lines prop to 10, to increase its height/line count? Applies to rule details page as well. Will address during timeline redesign (i.e. history feature, etc.) project. Timeframe to be determined. N/A
đź’šLow For the "Signals" table, the UtilityBar component should have a prop of border added to add a divider between itself and the table below (as the table doesn't have its own top border). Applies to rule details page as well. Can address in 7.7. N/A
đź’›Medium As we currently only have one method with which to create a detection rule, let's remove the "Method" column from this table by default. Applies to signal detection rules page as well. âś…
đź’›Medium Let's change "Rule", "Severity" and "Risk score" column headers to use their ECS names, to be consistent with the rest of the columns and with other aspects of the SIEM app. All table columns should use ECS names; no aliasing. Will address in 7.7. Consider reverse truncation for ECS names. Also consider ability for user to toggle between ECS names and aliases in configuration/settings. Needs discussion
đź’›Medium Can we move the "@timestamp" column in the table to appear before the "Rule" column? Since we're not using EuiDataGrid as initially thought during the design phase, there is now the concern of the timestamp falling out of view due to overflow. This should address that concern. Can address in 7.6. âś…
❤️High Can we add a toast for users when they open or close a signal from the table? I think that would help to make it clear to the user that something has happened after they they took these actions. Can address in 7.7. +1

Signal Detection Rules Page

Page Header

UX Priority Issue Notes Done
💛Medium This page's subtitle was meant to show a timestamp for the last rule run, as shown in the wireframes. Currently, it's just showing an m-dash (—). Can we fix to show last rule run timestamp? Can address in 7.7. ✅
đź’šLow Assuming @benskelker is amenable to the following, I'd like to suggest we change the "Load Elastic prebuilt rules" button text to "Install Elastic rules". @MichaelMarcialis to speak with @benskelker directly about verbiage changes. Timeframe to be determined. âś…
đź’šLow I know the "Activity monitor" feature was descoped for the MVP during development. However, are we not planning to include the "Failure history" tab for this page (as we do for individual rules)? If we are not, can we add a border prop back on the HeaderPage component to reinstate the divider between header and content? Can address border prop in 7.7. @MichaelMarcialis to talk with @MikePaquette on what the plan is for activity monitor or failure history on main rules page. N/A

Import Rule Modal

UX Priority Issue Notes Done
đź’šLow The field label text "Select a SIEM rule (as exported from the Detection Engine UI) to import" seems a bit too wordy. Can we change this to "Select a file to import"? Can address in 7.7, assuming @benskelker agrees with wording. +1
đź’šLow Likewise, can we change the file picker field's placeholder text to read "Select or drag and drop a file". Can address in 7.7, assuming @benskelker agrees with wording. +1
đź’šLow Can we add helpText beneath the file picker field to say File must be a valid signal detection rule saved object (rules_export.ndjson).. Can address in 7.7, assuming @benskelker agrees with wording. +1
đź’šLow Can we add an importAction icon to the "Import rule" button (on the left side), as shown in the wireframes? Can address in 7.7. +1

No Rules Empty Prompt

UX Priority Issue Notes Done
đź’šLow Assuming @benskelker is amenable, I'd like to suggest we change the header text. "Load Elastic prebuilt detection rules" explains what action we recommend, but not why we're recommending it. I suggest we change to something like "No detection rules. Let's add some!" @MichaelMarcialis to speak with @benskelker directly about verbiage changes. Timeframe to be determined. N/A, empty states currently being updated in EUI
đź’šLow Suggest we change this text:
Elastic SIEM comes with prebuilt detection rules that run in the background and create signals when their conditions are met.By default, all prebuilt rules are disabled and you select which rules you want to activate
To this text:
In order to begin using SIEM detections, you must first have one or more activated detection rules. To help you get started, SIEM comes with prepackaged with a number of recommended detection rules for you to optionally install and use. After installation, simply activate the rules from which you wish to begin generating signals (i.e. events of interest).
@MichaelMarcialis to speak with @benskelker directly about verbiage changes. Timeframe to be determined. Needs discussion
đź’šLow Change button text from "Load prebuilt detection rules" to "Install Elastic rules". @MichaelMarcialis to speak with @benskelker directly about verbiage changes. Timeframe to be determined. Needs discussion
đź’šLow Since we seem to be desiring to point the user toward the direction of installing the Elastic-authored rules initially, I'm thinking we should just have one call to action for this empty prompt. Doing so will help avoid user decision paralysis. Can we remove the second button from the empty prompt altogether? The ability to create your own rule still exists above, and this will give us a single strong CTA here. @MichaelMarcialis to speak with @benskelker directly about verbiage changes. Timeframe to be determined. Needs discussion

Rule Updates Callout

UX Priority Issue Notes Done
đź’šLow Assuming @benskelker is amenable, can we change the header text "Update available for Elastic prebuilt rules" to "Update available for installed Elastic rules"? @MichaelMarcialis to speak with @benskelker directly about verbiage changes. Timeframe to be determined. Needs discussion
đź’šLow Can we update the message text:
You can update {updateRules} Elastic prebuilt {updateRules, plural, =1 {rule} other {rules}}. Note that this will reload deleted Elastic prebuilt rules.
To this text:
An update is available for {updateRules} installed Elastic {updateRules, plural, =1 {rule} other {rules}}. Please note that updating will also reinstall any previously deleted Elastic rules.
@MichaelMarcialis to speak with @benskelker directly about verbiage changes. Timeframe to be determined. Needs discussion
đź’šLow Can we change the button text:
Update {updateRules} Elastic prebuilt {updateRules, plural, =1 {rule} other {rules}}
To this text:
Update {updateRules} Elastic {updateRules, plural, =1 {rule} other {rules}}
@MichaelMarcialis to speak with @benskelker directly about verbiage changes. Timeframe to be determined. Needs discussion

All Rules Table

UX Priority Issue Notes Done
đź’šLow On initial load, the header "All rules" does not appear until the loading is complete. This is inconsistent with how we do it elsewhere. Are we able to show the header text during the loading process? Can address in 7.7. âś…
đź’›Medium When performing a rule keyword search that returns no results, the entire table is hidden from view, making it look like an error. Can we instead show the table headers and an empty table message within (like we do elsewhere)? âś…
đź’›Medium I created a rule named "Michael's test". When performing a rule keyword search for the string Michael, I would expect this rule to appear, but it does not. I must input Michael's in order for it to appear as a search result. Is this a bug? UI person would need to work with someone on the back-end. Can address in 7.7. +1
đź’›Medium The UtilityBar for this table is missing the ability to select all rules on all pages (like the signals table). Again, this should appear when the user has selected all rules on the current page. Developer Effort: High +1
đź’šLow What is the purpose of the "Refresh" button here? If it's unnecessary, can we remove it? Or can we alternatively perform a data refresh at a set interval? If not and the refresh button must remain, can we move the icon to the left side of the text and put the entire button in a new/separate UtilityBarSection and UtilityBarGroup? Doing so will move it to the right side of the UtilityBar. Consider usage of a callout to display when change exists in the table and requires a refresh, but shouldn't be added until after activity monitor feature is in. Can easily move icon as described above in the interim. Developer Effort: High N/A
đź’šLow During a data refresh, rather than using the overlay loading spinner, can we use an EuiProgress absolutely positioned to the top? No need for us to obscure content during the refresh. Developer Effort: Low N/A
đź’šLow Can we use the compressed prop for this table to match with the event viewer tables font size? Developer Effort: Low âś…
đź’›Medium In addition to the "Activated" and "Last run" columns, the table should allow sorting for the "Rule", "Severity" and "Last response" as well. Fix by changing to in-memory table on the front-end. Developer Effort: Medium +1
đź’›Medium The "Last response" of "going to run" seems odd to me. That's not what I would envision as a response. I intended this value to only be "Success" or "Fail". Was this added to make up for the fact that the Activity monitor (with its status column) got descoped? Yes, keeping for now while activity monitor is not available. Revisit after activity monitor added. Developer Effort: TBD N/A
đź’›Medium The "Last response" column appears to be using the EuiHealth component for cell data. The design intentionally didn't use that to differentiate itself from the severity column (and also because it looks odd to have the fail tooltip icon inside the EuiHealth component). Assuming we still only have two response possibilities (success or fail), can we simply use EuiTextColor to mimic the wireframes? Developer Effort: Low âś…
đź’šLow The "Last response" column values appear to be lowercase. Can we change to sentence case (i.e. "succeeded" becomes "Succeeded")? Developer Effort: Low +1
❤️High Deactivating a rule appears to clear/delete that rule's "Last run" and "Last response" values. Is this desired behavior? In my designs, I envisioned both still being populated even if a rule is deactivated (assuming it has run at least once before deactivation). If not desired behavior, can we fix? Appears to get lost prior to data refresh. Developer Effort: Low Needs discussion
đź’›Medium Activating or deactivating individual rules quickly in a serial fashion causes the previous row's loading spinner to disappear and to be replaced with the toggle element before it has completed its activation/deactivation. Ideally it would be good to support multiple loading spinners happening at a time, in such situations. Consider assigning to @rylnd. Developer Effort: Low âś…
❤️High Per the wireframe prototype workflow, the original intent of the "Duplicate rule" action was to take the user directly to the "Create new rule" flow (although perhaps with an alternative page name, like "Duplicate rule") in order to let them make changes to their duplicate immediately. I imagined this would be desired behavior, as I don't believe there is a use case for duplicating a rule and making no changes to it. The current behavior keeps the user on the signal detection rules page and just adds a new rule to the table with a suffix of " [Duplicate]". Why was this flow changed? Was there a desire to support duplicating multiple/bulk rules at once (which could not be done with the original flow)? Could we not make the individual rule duplication behavior run through the "Create new rule" flow? Applies to rule details page as well. For single rule, let's take them to the edit page for that rule. Multiple rules will just get a toast. Developer Effort: Low +1
❤️High Per the wireframes, deleting rules is supposed to trigger a confirmation modal, to ensure the user is not making a mistake. Can this be added in? Applies to rule details page as well. Developer Effort: Low +1
đź’›Medium It appears that ability to bulk edit rule index patterns was not included. Did this get descoped from MVP? Requires redesign of modal to match current index pattern experience in new rule creation. Developer Effort: Medium/High Needs discussion
đź’›Medium Design and add confirmation modal (or some other design solution) when exporting multiple rules. Currently, exporting more than 2,000 rules will kill performance in the app. Warning the user of this fact and showing them that something is happening in the background would be helpful. +1

Create New Rule Page

All Steps

UX Priority Issue Notes Done
đź’›Medium Form validation appears to be happening on key down or up, rather than on blur. Can we change validation to happen on blur? Applies to edit rule page as well. âś…
💚Low All completed step summary items appear to be organized by appearance vertically in two columns. The wireframes indicated the items should be laid out horizontally in two columns, with some exceptions in the layout (such as the “Description” field taking up two columns of width). Would it be possible to change? ✅

Define Rule Step

UX Priority Issue Notes Done
❤️High When using "Import query from saved timeline", the import action always appears to populate the KQL bar by adding a "timeline-filter-drop-area" filter. This could potentially be confusing for a user, as it requires them to go in and edit the filter in order to see what query is actually contained within. Can we not add the query (from the timeline query builder and KQL bar) directly as KQL (or Lucene) in the input? And have any attached filters come in as filters? Doing so would save the user time in making sure they have the right query. Applies to edit rule page as well. Needs discussion
đź’šLow In EUI, form field labels turn blue when the related field is focused by the user. It appears that the "Custom query" field does not do this currently. Can it be changed to mimic other field labels? Applies to edit rule page as well. âś…
💛Medium The query filters in the “Define rule” completed step summary are listed apart from the “Custom query” field. The wireframes show them being combined under a unified header. Can we combine as shown in wireframes? If not, can we at least make "Filters" appear after "Custom query"? Applies to rule details page as well. N/A

About Rule Step

UX Priority Issue Notes Done
đź’šLow "Investigate detections using this timeline template" is a very wordy label. Can we instead go back to using "Timeline template" as the field label and also add helpText below the field that reads as follows?
Select an existing timeline to use as a template when investigating generated signals.
Applies to edit rule page as well.
âś…
đź’›Medium The "Timeline template" field's dropdown menu layout breaks on smaller viewport sizes. Applies to edit rule page as well. âś…
đź’›Medium The "Reference URLs", "False positive examples" and "MITRE ATT&CK threats" field layouts break on smaller viewport sizes. Applies to edit rule page as well. âś…
đź’›Medium The same "MITRE ATT&CK tactic" is able to be picked more than once. Is this desired behavior? Or would it be better to allow each tactic to only be selected once? Applies to edit rule page as well. N/A
đź’›Medium The completed step summary view for "Reference URLs" still shows empty link items when additional inputs are added but not populated by the user. âś…
💚Low The completed step summary for the “Description” field does not extend across two columns, as indicated in the wireframes. Can this be done? N/A

Schedule Rule Step

UX Priority Issue Notes Done
đź’›Medium The layout for the "Runs every" and "Additional look-back time" fields (namely the spacing between the labels and related fields) is broken at smaller viewport sizes. Applies to edit rule page as well. âś…
💚Low The “Optional” text for the “Additional look-back” field is not right aligned properly with the right edge of the form field below. Can this be addressed? Applies to edit rule page as well. ✅
đź’›Medium The "Create rule without activating it" and "Create & activate rule" buttons to not wrap at smaller viewport sizes, and instead overflow out of their container. âś…
❤️High In the wireframe prototypes, the intended flow was that users would be taken to the newly created rule's detail page after rule creation. Currently, the user is dropped back to the main rules page. Is this desired? If not, can we change the flow to go to the new rule's detail page as intended? Needs discussion

Rule Details Page

Page Header

UX Priority Issue Notes Done
đź’šLow The "Last signal" timestamp is missing from the subtitle area, as shown in wireframes. +1
đź’šLow What is the purpose of the "Refresh" button here? If it's unnecessary, can we remove it? Or can we alternatively perform a data refresh at a set interval? N/A
❤️High It appears we're currently using the "Status:" subtitle to display what is actually the response value; not the status value. However, per the wireframes, it was intended to show either "Idle" (after a rule either completed its run or stopped) or "Running" (with an accompanying progress bar showing the number of events that have been processed). If this is a mistake, can we change it back to as it was intended? If this is not a mistake and it is more desirable to show response instead of status, can you help me understand why (and possibly change the "Status:" text to be "Last response:")? Needs discussion
đź’›Medium If we are replacing "Status" with "Last response" (per comment above), then the value of of "going to run" seems out-of-place to me. That's not what I would envision as a response. I intended responses to only be "Success" or "Fail". Was this added to make up for the fact that the Activity monitor (with its status column) got descoped? Needs discussion
đź’›Medium In order to better inform the user, can we have a tooltip on the "Edit rule settings" button when the rule is immutable (i.e. Elastic-authored)? It can say something like "Elastic-authored rules may not be edited directly. Instead, please duplicate this rule and edit that copy." Needs discussion
đź’šLow Can we change the page's overflow menu button icon from boxesVertical to boxesHorizontal? Doing so will match how overflow menus are being treated elsewhere in the app (forgive me; I wasn't aware that this icon is hardcoded into table action menus when I created the wireframes). +1
đź’šLow Can we change the page's overflow menu button style to match that of a standard EuiButtonIcon? +1
đź’šLow Can the tooltip for the page's overflow menu button be changed from "All actions" to "Additional actions"? This is more accurate. +1

Definition/About/Schedule Rule Panels

UX Priority Issue Notes Done
đź’šLow On initial page load, the loading state for these panels is the absolutely positioned top EuiProgress component. In these situations, I think it makes more sense to use EuiLoadingContent. Can we change it? N/A

About Rule Panel

UX Priority Issue Notes Done
💚Low All items appear to be organized by appearance vertically in two columns. The wireframes indicated the items should be laid out horizontally in two columns, with some exceptions in the layout (such as the “Description” field taking up two columns of width). Would it be possible to change? ✅
❤️High There is a bug where the desired risk score is not being appropriately set on rule creation or edit. To replicate:
  • Create a new rule.
  • Set the severity to "Critical".
  • Adjust the risk score down to "10".
  • Complete the rule creation process.
  • The rule detail page shows the risk score incorrectly as 99 (the default for "Critical" severity, before changing it).
Needs discussion
đź’›Medium The "MITRE ATT&CK" technique/child is not properly left aligned in small viewport sizes. âś…

Schedule Rule Panel

UX Priority Issue Notes Done
đź’šLow This panel is missing the "Next rule run" timestamp, as shown in wireframes. N/A

Tabbed Navigation

UX Priority Issue Notes Done
❤️High The tabbed navigation is currently located in wrong position on the page. It should be placed just above signal histogram, as shown in the wireframes. This way, the rule definition, about and schedule information is always present. ✅
đź’šLow Change "Failure History" tab text to "Failure history". +1

Signals Table

UX Priority Issue Notes Done
đź’›Medium Remove "Rule", "Method", "Severity", and "Risk score" columns, as they will always be the same value on the rule details page (and it is already given in the information panels above). +1
đź’›Medium Can we move the "@timestamp" column in the table to appear before the "event.action" column? Since we're not using EuiDataGrid as initially thought during the design phase, there is now the concern of the timestamp falling out of view due to overflow. This should address that concern. âś…

Last Five Errors Table

UX Priority Issue Notes Done
đź’šLow Can we change the header text from "Last five errors" to "Latest failures (limited to five)"? N/A
đź’šLow Can we add a UtilityBar to display the count of errors in the table, to be consistent with other tables? N/A
đź’šLow For the initial loading state of this table, can we set the EuiLoadingContent component's lines prop to 10, to increase its height/line count? N/A

Edit Rule Page

Page Header

UX Priority Issue Notes Done
đź’›Medium The header is missing the "Cancel" and "Save changes" buttons shown in the wireframes (duplicate of the ones that appear at the bottom of the page, in case the form gets too long). +1

About Tab

UX Priority Issue Notes Done
đź’›Medium If the user did not add values during the rule creation process, form fields for "Reference URLs", "False positive examples" and "MITRE ATT&CK threats" do not get rendered until the user first clicks their respective "Add" buttons. In cases where there are no values, I think it would be a better user experience to always show the first row of fields, even if they are empty. Subsequent field row additions beyond the first would require user interaction with the aforementioned "Add" buttons. +1
elasticmachine commented 4 years ago

Pinging @elastic/siem (Team:SIEM)