Skip to content
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] Incorrect icon is displaying for numeric type under Required fields #133291

Closed
Tracked by #133050
ghost opened this issue Jun 1, 2022 · 7 comments · Fixed by #133050
Closed
Tracked by #133050
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Rule Details Security Solution Detection Rule Details page fixed impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. QA:Validated Issue has been validated by QA Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.3.0

Comments

@ghost
Copy link

ghost commented Jun 1, 2022

Describe the bug
Incorrect icon is displaying for numeric type under Required fields

Build info

Version: 8.3.0 Branch
Commit: fe0166d7991c7220bd4321defcbaa5c1e614dc90  

image

preconditions

  1. Kibana should be up and running

Steps to Reproduce

  1. Navigate to Rules page under security

  2. Import the below rule that contains the required fields with numeric type
    rules_export (2).zip

  3. After import the rule navigate to rule details page

  4. Observe that incorrect icon is displaying for numeric type under "Required fields"

Actual Result
Incorrect icon is displaying for numeric type under "Required fields"

Expected Result
Correct icon say # should be displayed for numeric type under "Required fields"

What's Working
image

Screenshot
image

image

@ghost ghost added bug Fixes for quality problems that affect the customer experience triage_needed Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Jun 1, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@ghost ghost added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. v8.3.0 labels Jun 1, 2022
@ghost ghost assigned ghost and MadameSheema and unassigned ghost Jun 1, 2022
@MadameSheema MadameSheema removed their assignment Jun 1, 2022
@MadameSheema MadameSheema added Team:Detections and Resp Security Detection Response Team Team:Detection Rule Management Security Detection Rule Management Team labels Jun 1, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@MadameSheema MadameSheema added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. and removed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Jun 1, 2022
@spong
Copy link
Member

spong commented Jun 1, 2022

Currently we're using the re-usable FieldIcon component for displaying these field types. This is the same component used by Discover so we should expect parity with that UI, but let me check and see what the FieldsBrowser component referenced in the description is using and maybe we can leverage that.

Either way, we should be able to add float (and any other missing field types) to the FieldIcon component here:

export const typeToEuiIconMap: Partial<Record<string, EuiTokenProps>> = {
boolean: { iconType: 'tokenBoolean' },
// icon for an index pattern mapping conflict in discover
conflict: { iconType: 'alert', color: 'euiColorVis9', shape: 'square' },
date: { iconType: 'tokenDate' },
date_range: { iconType: 'tokenDate' },
geo_point: { iconType: 'tokenGeo' },
geo_shape: { iconType: 'tokenGeo' },
ip: { iconType: 'tokenIP' },
ip_range: { iconType: 'tokenIP' },
match_only_text: { iconType: 'tokenString' },
// is a plugin's data type https://www.elastic.co/guide/en/elasticsearch/plugins/current/mapper-murmur3-usage.html
murmur3: { iconType: 'tokenSearchType' },
number: { iconType: 'tokenNumber' },
number_range: { iconType: 'tokenNumber' },
histogram: { iconType: 'tokenHistogram' },
_source: { iconType: 'editorCodeBlock', color: 'gray' },
string: { iconType: 'tokenString' },
text: { iconType: 'tokenString' },
keyword: { iconType: 'tokenKeyword' },
nested: { iconType: 'tokenNested' },
version: { iconType: 'tokenTag' },
};

@spong spong self-assigned this Jun 1, 2022
@banderror banderror added Feature:Rule Details Security Solution Detection Rule Details page and removed triage_needed labels Jun 2, 2022
@banderror banderror removed their assignment Jun 2, 2022
@spong
Copy link
Member

spong commented Jun 2, 2022

So there's a few different behaviors here depending on if you look at Discover, the Security Solution Fields Browser, or the Security Solution Event Details Flyout.

Discover, the Event Details Flyout, and the current Required Fields implementation are all using the FieldIcon component as mentioned above, with the key difference being that the Required Fields implementation is using ES_FIELD_TYPES vs KBN_FIELD_TYPES, as this is what is provided to us from the Rule SO. It's this mis-match between the two that causes this missing icon issue.

After some searching, it seems there's a conversion utility castEsToKbnFieldTypeName over in the kbn-field-types package that we can use to get a reliable kbnFieldType to pass on to the FieldIcon. And since the FieldIcon component is going to use that kbn type as the hover text, we'll need to override the label with the actual es type they need to have in their mapping so e.g. risk.score shows as float instead of number.

Without label override:

imageimage

With label override:

imageimage

spong added a commit that referenced this issue Jun 7, 2022
## Summary

A new Security Solution feature (#131475) was added in `8.3` that displays a field name and icon token using the reusable `FieldIcon` component. In testing an issue was reported (#133291) that the wrong icon token was being displayed. 

I had [previously updated](https://github.com/elastic/kibana/pull/131475/files#diff-d79a8297783f3177da25dd13fe807425d9136a0e235fe170f7c0a61f2448dacaR23) `FieldIcon` to support `match_only_text` however this new issue was with the `float` type not displaying correctly. After some searching I found the `castEsToKbnFieldTypeName` utility which solved the issue with `float` fields not displaying, but then `match_only_text` field types would not show since it is missing from `ES_FIELD_TYPES` and so would resolve as `unknown`. 

This PR adds the `match_only_text` [ES type](https://www.elastic.co/guide/en/elasticsearch/reference/current/text.html#match-only-text-field-type) to `ES_FIELD_TYPES` to resolve this missing icon token issue so that `castEsToKbnFieldTypeName` can be used in conjunction with the resuable `FieldIcon` component. 

I imagine this is fine as it's a sibling type to `text`, but am curious here since `sortable: true,` is set for the KbnFieldType even though it includes `ES_FIELD_TYPES.TEXT` (which is not sortable) as well?
@spong
Copy link
Member

spong commented Jun 7, 2022

Platform fix has been merged as of #133690, however still waiting on actual fix in #133050 to merge before this can be re-tested/closed.

kibanamachine pushed a commit that referenced this issue Jun 7, 2022
## Summary

A new Security Solution feature (#131475) was added in `8.3` that displays a field name and icon token using the reusable `FieldIcon` component. In testing an issue was reported (#133291) that the wrong icon token was being displayed.

I had [previously updated](https://github.com/elastic/kibana/pull/131475/files#diff-d79a8297783f3177da25dd13fe807425d9136a0e235fe170f7c0a61f2448dacaR23) `FieldIcon` to support `match_only_text` however this new issue was with the `float` type not displaying correctly. After some searching I found the `castEsToKbnFieldTypeName` utility which solved the issue with `float` fields not displaying, but then `match_only_text` field types would not show since it is missing from `ES_FIELD_TYPES` and so would resolve as `unknown`.

This PR adds the `match_only_text` [ES type](https://www.elastic.co/guide/en/elasticsearch/reference/current/text.html#match-only-text-field-type) to `ES_FIELD_TYPES` to resolve this missing icon token issue so that `castEsToKbnFieldTypeName` can be used in conjunction with the resuable `FieldIcon` component.

I imagine this is fine as it's a sibling type to `text`, but am curious here since `sortable: true,` is set for the KbnFieldType even though it includes `ES_FIELD_TYPES.TEXT` (which is not sortable) as well?

(cherry picked from commit de3410e)
kibanamachine added a commit that referenced this issue Jun 7, 2022
## Summary

A new Security Solution feature (#131475) was added in `8.3` that displays a field name and icon token using the reusable `FieldIcon` component. In testing an issue was reported (#133291) that the wrong icon token was being displayed.

I had [previously updated](https://github.com/elastic/kibana/pull/131475/files#diff-d79a8297783f3177da25dd13fe807425d9136a0e235fe170f7c0a61f2448dacaR23) `FieldIcon` to support `match_only_text` however this new issue was with the `float` type not displaying correctly. After some searching I found the `castEsToKbnFieldTypeName` utility which solved the issue with `float` fields not displaying, but then `match_only_text` field types would not show since it is missing from `ES_FIELD_TYPES` and so would resolve as `unknown`.

This PR adds the `match_only_text` [ES type](https://www.elastic.co/guide/en/elasticsearch/reference/current/text.html#match-only-text-field-type) to `ES_FIELD_TYPES` to resolve this missing icon token issue so that `castEsToKbnFieldTypeName` can be used in conjunction with the resuable `FieldIcon` component.

I imagine this is fine as it's a sibling type to `text`, but am curious here since `sortable: true,` is set for the KbnFieldType even though it includes `ES_FIELD_TYPES.TEXT` (which is not sortable) as well?

(cherry picked from commit de3410e)

Co-authored-by: Garrett Spong <[email protected]>
spong added a commit that referenced this issue Jun 9, 2022
…s Feedback & Fixes (#133050)

## Summary

Addressing the following feedback from #132847:

- [X] On the Rule Management page package name is used instead of package title when the package has only 1 integration:
- [X] move integrations_popover to `related_integrations` directory
- [X] update useInstalledIntegrations to always return array of installedIntegration
- [X] move useInstalledIntegrations to `related_integrations` directory
- [X] Slight update to copy in Rules Table popover
- [X] Ok to use Rule Details UI within Rules Table popover content
- [X] Sort integrations alphabetically
- [X] Update left padding on version mis-match tooltip
- [X] #133291
- [X] #133269
- [X]  Add Kibana Advanced Setting for disabling integrations badge on Rules Table
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/171750790-ffa2e3ef-dd7a-499c-9b08-89bafc06dd50.png" />
</p>

- [ ]  Adds tests
  - [x] `useInstalledIntegrations` hook 
  - [X] relatedIntegrations utils
  - [x] IntegrationDescription
- [ ]  Add loaders where necessary since there can now be API delay
  - May hold off on loaders as transition from no installed integrations -> installed integrations isn't too bad as-is

##### Updated integrations popover content on Rules Table to match Rule Details design
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263941-3e948b41-7ef7-4281-8354-57e77ddeb433.png" />
</p>


In discussions with @banderror reviewing the different integration states (uninstalled, installed, enabled, and agents enrolled), we are now capturing the distinction between `Installed` and `Enabled` so that we don't confuse users when a package is installed but the integration isn't enabled/configured. I also added tooltips for clarifying each state and what action the user should perform to ensure compatibility. In collab with @yiyangliu9286 @jethr0null (comments below) -- we've consolidated to a single `Installed: enabled` badge, and updated `Uninstalled` to `Not installed` as well.


##### Tooltips

<details><summary>Not installed</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172264210-00064485-2df9-408e-953b-9294f16dedf2.png" />
</p>
</details>





<details><summary>Installed</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263672-67b641cd-5895-464a-8897-f26bd0a61073.png" />
</p>
</details>




<details><summary>Installed: enabled</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263783-563ea48d-c96c-4519-87b4-7076582f5da2.png" />
</p>
</details>



### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [X] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials
  - Collaborating with docs teams on this dedicated docs issue: elastic/security-docs#2015
- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
- [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
spong added a commit to spong/kibana that referenced this issue Jun 9, 2022
…s Feedback & Fixes (elastic#133050)

## Summary

Addressing the following feedback from elastic#132847:

- [X] On the Rule Management page package name is used instead of package title when the package has only 1 integration:
- [X] move integrations_popover to `related_integrations` directory
- [X] update useInstalledIntegrations to always return array of installedIntegration
- [X] move useInstalledIntegrations to `related_integrations` directory
- [X] Slight update to copy in Rules Table popover
- [X] Ok to use Rule Details UI within Rules Table popover content
- [X] Sort integrations alphabetically
- [X] Update left padding on version mis-match tooltip
- [X] elastic#133291
- [X] elastic#133269
- [X]  Add Kibana Advanced Setting for disabling integrations badge on Rules Table
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/171750790-ffa2e3ef-dd7a-499c-9b08-89bafc06dd50.png" />
</p>

- [ ]  Adds tests
  - [x] `useInstalledIntegrations` hook
  - [X] relatedIntegrations utils
  - [x] IntegrationDescription
- [ ]  Add loaders where necessary since there can now be API delay
  - May hold off on loaders as transition from no installed integrations -> installed integrations isn't too bad as-is

##### Updated integrations popover content on Rules Table to match Rule Details design
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263941-3e948b41-7ef7-4281-8354-57e77ddeb433.png" />
</p>

In discussions with @banderror reviewing the different integration states (uninstalled, installed, enabled, and agents enrolled), we are now capturing the distinction between `Installed` and `Enabled` so that we don't confuse users when a package is installed but the integration isn't enabled/configured. I also added tooltips for clarifying each state and what action the user should perform to ensure compatibility. In collab with @yiyangliu9286 @jethr0null (comments below) -- we've consolidated to a single `Installed: enabled` badge, and updated `Uninstalled` to `Not installed` as well.

##### Tooltips

<details><summary>Not installed</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172264210-00064485-2df9-408e-953b-9294f16dedf2.png" />
</p>
</details>

<details><summary>Installed</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263672-67b641cd-5895-464a-8897-f26bd0a61073.png" />
</p>
</details>

<details><summary>Installed: enabled</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263783-563ea48d-c96c-4519-87b4-7076582f5da2.png" />
</p>
</details>

### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [X] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials
  - Collaborating with docs teams on this dedicated docs issue: elastic/security-docs#2015
- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
- [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

(cherry picked from commit 7bfcb52)

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/integrations_popover/helpers.tsx
#	x-pack/plugins/security_solution/public/common/components/integrations_popover/index.tsx
#	x-pack/plugins/security_solution/public/detections/components/rules/description_step/required_integrations_description.tsx
#	x-pack/plugins/security_solution/public/detections/components/rules/related_integrations/use_installed_integrations.tsx
spong added a commit that referenced this issue Jun 10, 2022
… Fields Feedback & Fixes (#133050) (#134148)

* [Security Solution][Detections] Related Integrations & Required Fields Feedback & Fixes (#133050)

## Summary

Addressing the following feedback from #132847:

- [X] On the Rule Management page package name is used instead of package title when the package has only 1 integration:
- [X] move integrations_popover to `related_integrations` directory
- [X] update useInstalledIntegrations to always return array of installedIntegration
- [X] move useInstalledIntegrations to `related_integrations` directory
- [X] Slight update to copy in Rules Table popover
- [X] Ok to use Rule Details UI within Rules Table popover content
- [X] Sort integrations alphabetically
- [X] Update left padding on version mis-match tooltip
- [X] #133291
- [X] #133269
- [X]  Add Kibana Advanced Setting for disabling integrations badge on Rules Table
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/171750790-ffa2e3ef-dd7a-499c-9b08-89bafc06dd50.png" />
</p>

- [ ]  Adds tests
  - [x] `useInstalledIntegrations` hook
  - [X] relatedIntegrations utils
  - [x] IntegrationDescription
- [ ]  Add loaders where necessary since there can now be API delay
  - May hold off on loaders as transition from no installed integrations -> installed integrations isn't too bad as-is

##### Updated integrations popover content on Rules Table to match Rule Details design
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263941-3e948b41-7ef7-4281-8354-57e77ddeb433.png" />
</p>

In discussions with @banderror reviewing the different integration states (uninstalled, installed, enabled, and agents enrolled), we are now capturing the distinction between `Installed` and `Enabled` so that we don't confuse users when a package is installed but the integration isn't enabled/configured. I also added tooltips for clarifying each state and what action the user should perform to ensure compatibility. In collab with @yiyangliu9286 @jethr0null (comments below) -- we've consolidated to a single `Installed: enabled` badge, and updated `Uninstalled` to `Not installed` as well.

##### Tooltips

<details><summary>Not installed</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172264210-00064485-2df9-408e-953b-9294f16dedf2.png" />
</p>
</details>

<details><summary>Installed</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263672-67b641cd-5895-464a-8897-f26bd0a61073.png" />
</p>
</details>

<details><summary>Installed: enabled</summary>
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/172263783-563ea48d-c96c-4519-87b4-7076582f5da2.png" />
</p>
</details>

### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [X] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials
  - Collaborating with docs teams on this dedicated docs issue: elastic/security-docs#2015
- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
- [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

(cherry picked from commit 7bfcb52)

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/integrations_popover/helpers.tsx
#	x-pack/plugins/security_solution/public/common/components/integrations_popover/index.tsx
#	x-pack/plugins/security_solution/public/detections/components/rules/description_step/required_integrations_description.tsx
#	x-pack/plugins/security_solution/public/detections/components/rules/related_integrations/use_installed_integrations.tsx

* Fixes type from missing backport
@banderror
Copy link
Contributor

Fixed in #133050. Will be available in the next 8.3.0 BC.

@ghost
Copy link
Author

ghost commented Jun 15, 2022

Hi @banderror ,

We have validated above issue on 8.3.0 BC4 and observed that issue is Fixed 🟢

Please find the below testing details

Build info:

Version : 8.3.0 BC4
Build : 53413
Commit : 875ea184462f73a04410981ac9eaf799db28b4f0

Screenshots/Screencast:

image

Hence, We are closing this issue and marking this as QA Validated.

Thanks!

@ghost ghost added the QA:Validated Issue has been validated by QA label Jun 15, 2022
@ghost ghost closed this as completed Jun 15, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Rule Details Security Solution Detection Rule Details page fixed impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. QA:Validated Issue has been validated by QA Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.3.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants