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][Detections] Related Integrations & Required Fields Feedback & Fixes #133050

Merged
merged 19 commits into from
Jun 9, 2022

Conversation

spong
Copy link
Member

@spong spong commented May 26, 2022

Summary

Addressing the following feedback from #132847:

  • Adds tests
    • useInstalledIntegrations hook
    • relatedIntegrations utils
    • 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

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
Not installed

Installed

Installed: enabled

Checklist

Delete any items that are not applicable to this PR.

@spong spong added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Feature:Detection Rules Anything related to Security Solution's Detection Rules Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. auto-backport Deprecated - use backport:version if exact versions are needed Feature:Rule Management Security Solution Detection Rule Management Team:Detection Rule Management Security Detection Rule Management Team Theme: simp_prot_mgmt Security Solution Simplified Protection Management Theme v8.3.0 Feature:Rule Details Security Solution Detection Rule Details v8.4.0 labels May 26, 2022
@spong spong self-assigned this May 26, 2022
@spong spong requested review from a team as code owners May 26, 2022 17:53
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@spong spong requested a review from a team as a code owner June 2, 2022 22:15
@spong spong changed the title [Security Solution][Detections] Related Integrations Feedback & Fixes [Security Solution][Detections] Related Integrations & Required Fields Feedback & Fixes Jun 2, 2022
@yiyangliu9286
Copy link

I agree with the badge colour usage here to be consistent with what we use in ML Job enabled/disabled UI. On the different statuses itself though I wonder if it makes sense to combine the Installed and Enabled as Installed: enabled rather than two badges since the Tooltip content is similar. So we have 3 different status in total.

  1. Uninstalled Show tooltip:

171966180-8a2be278-d135-4770-8326-729d23e0bd7f

  1. Installed Show tooltip:

171966237-d47aee81-a9d9-4e83-b7ad-904989444900

  1. Installed: enabled Show tooltip
    Integration is installed. An integration policy with the required configuration exists. Ensure Elastic Agents are assigned this policy to ingest compatible events.

@jethr0null
Copy link

@spong I’m on board with @yiyang’s suggestions as well. My only other thought/question is whether it makes sense to use Available instead of Uninstalled to avoid any ambiguity (e.g. uninstalled being interpreted as “was once installed and has since been removed”. I don't feel strongly about changing this if everyone else agrees that uninstalled makes more sense.

@spong spong removed the request for review from a team June 6, 2022 22:54
@spong
Copy link
Member Author

spong commented Jun 6, 2022

Thanks for the feedback @yiyangliu9286 and @jethr0null! 🙂

I've combined the Installed and Enabled badges, updated Uninstalled to Not installed (from our other discussion), and updated the badge colors such that Installed is the green success, and Not installed is the muted gray. I've updated the images in the description, so please check those out to confirm these changes 👍 .

@banderror
Copy link
Contributor

banderror commented Jun 8, 2022

While testing locally, found a few bugs. Will post these right away and continue testing and reviewing the code.

Version mismatch icon's color is not readable

When the dark mode is off:

Screenshot 2022-06-08 at 21 34 24

Links include a semver requirement instead of a target version

Looks like it happens for installed integrations, and works properly for those that are not installed when there's a version mismatch. Both pages are affected.

Screenshot 2022-06-08 at 21 37 28

Screenshot 2022-06-08 at 21 37 59

ndjson for repro:

{"id":"6cc39c80-da3a-11ec-9fce-65c1a0bee900","updated_at":"2022-05-23T01:48:23.422Z","updated_by":"elastic","created_at":"2022-05-23T01:48:20.940Z","created_by":"elastic","name":"Rule with Related Integrations, Required Fields, and Setup Guide","tags":["Test"],"interval":"5m","enabled":false,"description":"See Related Integrations, Required Fields, and Setup Guide on the Rule Details and Rule Management pages.","risk_score":47,"severity":"medium","license":"Elastic License v2","output_index":".siem-signals-default","meta":{"from":"5m"},"rule_name_override":"message","timestamp_override":"event.ingested","author":["Elastic"],"false_positives":[],"from":"now-600s","rule_id":"2c66bf23-6ae9-4eb2-859e-446bea181ae0","max_signals":10000,"risk_score_mapping":[],"severity_mapping":[],"threat":[],"to":"now","references":[],"version":7,"exceptions_list":[],"immutable":false,"related_integrations":[{"package":"system","version":"~1.6.4"},{"package":"aws","integration":"cloudfront","version":"~1.11.0"},{"package":"aws","integration":"cloudtrail","version":"~1.11.0"},{"package":"aws","integration":"route53","version":"~1.11.0"},{"package":"unknown","integration":"unknown","version":"^1.2.3"},{"package":"unknown2","version":"4.5.6"}],"required_fields":[{"ecs":true,"name":"event.code","type":"keyword"},{"ecs":true,"name":"message","type":"match_only_text"},{"ecs":false,"name":"winlog.event_data.AttributeLDAPDisplayName","type":"keyword"},{"ecs":false,"name":"winlog.event_data.AttributeValue","type":"keyword"},{"ecs":false,"name":"winlog.event_data.ShareName","type":"keyword"},{"ecs":false,"name":"winlog.event_data.RelativeTargetName","type":"keyword"},{"ecs":false,"name":"winlog.event_data.AccessList","type":"keyword"}],"setup":"## Config\n\nThe 'Audit Detailed File Share' audit policy must be configured (Success Failure).\nSteps to implement the logging policy with with Advanced Audit Configuration:\n\n```\nComputer Configuration > \nPolicies > \nWindows Settings > \nSecurity Settings > \nAdvanced Audit Policies Configuration > \nAudit Policies > \nObject Access > \nAudit Detailed File Share (Success,Failure)\n```\n\nThe 'Audit Directory Service Changes' audit policy must be configured (Success Failure).\nSteps to implement the logging policy with with Advanced Audit Configuration:\n\n```\nComputer Configuration > \nPolicies > \nWindows Settings > \nSecurity Settings > \nAdvanced Audit Policies Configuration > \nAudit Policies > \nDS Access > \nAudit Directory Service Changes (Success,Failure)\n```\n","type":"query","language":"kuery","index":["logs-*"],"query":"host.name:*","filters":[],"throttle":"no_actions","actions":[]}
{"id":"6cc39c80-da3a-11ec-9fce-65c1a0bee901","updated_at":"2022-05-23T01:48:23.422Z","updated_by":"elastic","created_at":"2022-05-23T01:48:20.940Z","created_by":"elastic","name":"Rule with Related Integrations","tags":["Test"],"interval":"5m","enabled":false,"description":"See Related Integrations on the Rule Details and Rule Management pages.","risk_score":47,"severity":"medium","license":"Elastic License v2","output_index":".siem-signals-default","meta":{"from":"5m"},"rule_name_override":"message","timestamp_override":"event.ingested","author":["Elastic"],"false_positives":[],"from":"now-600s","rule_id":"2c66bf23-6ae9-4eb2-859e-446bea181ae1","max_signals":10000,"risk_score_mapping":[],"severity_mapping":[],"threat":[],"to":"now","references":[],"version":7,"exceptions_list":[],"immutable":false,"related_integrations":[{"package":"unknown","version":"4.5.6"},{"package":"unknown","integration":"unknown","version":"^1.2.3"},{"package":"system","version":"~1.6.4"},{"package":"aws","integration":"cloudfront","version":"~1.11.0"},{"package":"aws","integration":"cloudtrail","version":"~1.11.0"},{"package":"aws","integration":"route53","version":"~123.456.789"},{"package":"azure","integration":"activitylogs","version":"~99.99.99"},{"package":"azure","integration":"platformlogs","version":"100.x.x"}],"required_fields":[],"setup":"","type":"query","language":"kuery","index":["logs-*"],"query":"host.name:*","filters":[],"throttle":"no_actions","actions":[]}
{"id":"6cc39c80-da3a-11ec-9fce-65c1a0bee902","updated_at":"2022-05-23T01:48:23.422Z","updated_by":"elastic","created_at":"2022-05-23T01:48:20.940Z","created_by":"elastic","name":"Rule with Required Fields","tags":["Test"],"interval":"5m","enabled":false,"description":"See Required Fields on the Rule Details page.","risk_score":47,"severity":"medium","license":"Elastic License v2","output_index":".siem-signals-default","meta":{"from":"5m"},"rule_name_override":"message","timestamp_override":"event.ingested","author":["Elastic"],"false_positives":[],"from":"now-600s","rule_id":"2c66bf23-6ae9-4eb2-859e-446bea181ae2","max_signals":10000,"risk_score_mapping":[],"severity_mapping":[],"threat":[],"to":"now","references":[],"version":7,"exceptions_list":[],"immutable":false,"related_integrations":[],"required_fields":[{"ecs":true,"name":"event.code","type":"keyword"},{"ecs":true,"name":"message","type":"match_only_text"},{"ecs":false,"name":"winlog.event_data.AttributeLDAPDisplayName","type":"keyword"},{"ecs":false,"name":"winlog.event_data.AttributeValue","type":"keyword"},{"ecs":false,"name":"winlog.event_data.ShareName","type":"keyword"},{"ecs":false,"name":"winlog.event_data.RelativeTargetName","type":"keyword"},{"ecs":false,"name":"winlog.event_data.AccessList","type":"keyword"}],"setup":"","type":"query","language":"kuery","index":["logs-*"],"query":"host.name:*","filters":[],"throttle":"no_actions","actions":[]}
{"id":"6cc39c80-da3a-11ec-9fce-65c1a0bee903","updated_at":"2022-05-23T01:48:23.422Z","updated_by":"elastic","created_at":"2022-05-23T01:48:20.940Z","created_by":"elastic","name":"Rule with Setup Guide","tags":["Test"],"interval":"5m","enabled":false,"description":"See Setup Guide on the Rule Details page.","risk_score":47,"severity":"medium","license":"Elastic License v2","output_index":".siem-signals-default","meta":{"from":"5m"},"rule_name_override":"message","timestamp_override":"event.ingested","author":["Elastic"],"false_positives":[],"from":"now-600s","rule_id":"2c66bf23-6ae9-4eb2-859e-446bea181ae3","max_signals":10000,"risk_score_mapping":[],"severity_mapping":[],"threat":[],"to":"now","references":[],"version":7,"exceptions_list":[],"immutable":false,"related_integrations":[],"required_fields":[],"setup":"## Config\n\nThe 'Audit Detailed File Share' audit policy must be configured (Success Failure).\nSteps to implement the logging policy with with Advanced Audit Configuration:\n\n```\nComputer Configuration > \nPolicies > \nWindows Settings > \nSecurity Settings > \nAdvanced Audit Policies Configuration > \nAudit Policies > \nObject Access > \nAudit Detailed File Share (Success,Failure)\n```\n\nThe 'Audit Directory Service Changes' audit policy must be configured (Success Failure).\nSteps to implement the logging policy with with Advanced Audit Configuration:\n\n```\nComputer Configuration > \nPolicies > \nWindows Settings > \nSecurity Settings > \nAdvanced Audit Policies Configuration > \nAudit Policies > \nDS Access > \nAudit Directory Service Changes (Success,Failure)\n```\n","type":"query","language":"kuery","index":["logs-*"],"query":"host.name:*","filters":[],"throttle":"no_actions","actions":[]}
{"id":"6cc39c80-da3a-11ec-9fce-65c1a0bee904","updated_at":"2022-05-23T01:48:23.422Z","updated_by":"elastic","created_at":"2022-05-23T01:48:20.940Z","created_by":"elastic","name":"Rule without the new fields","tags":["Test"],"interval":"5m","enabled":false,"description":"See no Related Integrations, Required Fields, and Setup Guide on the Rule Details and Rule Management pages.","risk_score":47,"severity":"medium","license":"Elastic License v2","output_index":".siem-signals-default","meta":{"from":"5m"},"rule_name_override":"message","timestamp_override":"event.ingested","author":["Elastic"],"false_positives":[],"from":"now-600s","rule_id":"2c66bf23-6ae9-4eb2-859e-446bea181ae4","max_signals":10000,"risk_score_mapping":[],"severity_mapping":[],"threat":[],"to":"now","references":[],"version":7,"exceptions_list":[],"immutable":false,"type":"query","language":"kuery","index":["logs-*"],"query":"host.name:*","filters":[],"throttle":"no_actions","actions":[]}
{"exported_count":5,"exported_rules_count":5,"missing_rules":[],"missing_rules_count":0,"exported_exception_list_count":0,"exported_exception_list_item_count":0,"missing_exception_list_item_count":0,"missing_exception_list_items":[],"missing_exception_lists":[],"missing_exception_lists_count":0}

@spong
Copy link
Member Author

spong commented Jun 8, 2022

While testing locally, found a few bugs. Will post these right away and continue testing and reviewing the code.

Version mismatch icon's color is not readable

When the dark mode is off:

Links include a semver requirement instead of a target version

Looks like it happens for installed integrations, and works properly for those that are not installed when there's a version mismatch. Both pages are affected.

Thanks @banderror! Should be resolved as of b0ba541. I've updated the icon color to be warning as it is in the fleet UI, ensure the left-margin was being applied (it was set, but was being overriden by the EuiIconTip), and lastly fixed the version calculation and started adding our tests from the test plan.

There might still be another bug in here with the version calculation, so will finish adding the remaining tests and push those next 👍

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Reviewed most of the changes and have some suggestions, so please check the comments I left @spong.

Still have to carefully scan the getInstalledRelatedIntegrations logic and review the test coverage. Will do it tomorrow morning.

docs/management/advanced-options.asciidoc Show resolved Hide resolved
Comment on lines +66 to +100
export const integrationDetailsUninstalled: IntegrationDetails = {
package_name: 'test',
package_title: 'Test',
package_version: '1.2.3',
integration_name: 'integration',
integration_title: 'Integration',
is_enabled: false,
is_installed: false,
target_version: '1.2.3',
version_satisfied: false,
};

export const integrationDetailsInstalled: IntegrationDetails = {
package_name: 'test',
package_title: 'Test',
package_version: '1.2.3',
integration_name: 'integration',
integration_title: 'Integration',
is_enabled: false,
is_installed: true,
target_version: '1.2.3',
version_satisfied: true,
};

export const integrationDetailsEnabled: IntegrationDetails = {
package_name: 'test',
package_title: 'Test',
package_version: '1.1.3',
integration_name: 'integration',
integration_title: 'Integration',
is_enabled: true,
is_installed: true,
target_version: '1.3.3',
version_satisfied: false,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have an integration_details.ts file and then move this mock data to integration_details.mock.ts?

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #35 / saved objects tagging - functional tests table listing "after all" hook in "table listing"
  • [job] [logs] FTR Configs #35 / saved objects tagging - functional tests table listing "before all" hook in "table listing"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3081 3087 +6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 5.1MB 5.1MB +3.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 250.5KB 250.6KB +82.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @spong

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last comments! Finished reviewing the code changes and tested it once again.

Couldn't find any new bugs, and the two bugs I reported above are fixed! 🙌

),
type: 'boolean',
category: [APP_ID],
requiresPageReload: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I disabled this setting and navigated to the Rules page without doing a page reload, it still worked (the column got hidden). Can we set requiresPageReload: false here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can change to false -- I had tested in multiple tabs and noticed the opposite behavior so had set as true, but this appears to be a cache issue.

: integration.target_version;

const integrationURL =
version !== ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case version can be an empty string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the error case when semver.valid(semver.coerce(i.version)) returns null, so we will navigate to just the latest package version (redirected by fleet) in those instances. That said, as we saw, we still need to include the integrationName if there is once so it doesn't just drop the user off on the base integration page.

Comment on lines +72 to +76
// Version check e.g. fleet match `1.2.3` satisfies rule dependency `~1.2.1`
const versionSatisfied = semver.satisfies(match.package_version, i.version);
const packageVersion = versionSatisfied
? i.version
: semver.valid(semver.coerce(i.version)) ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking - if the version requirement is satisfied, shouldn't we use the installed version? Here in the code, it would be match.package_version:

      const targetVersion = versionSatisfied
        ? match.package_version
        : semver.valid(semver.coerce(i.version)) ?? '';

i.version here is the related integration's version which is ~1.2.3 etc.

So to me, it looks like a bug, but it works as expected locally and there are some unit tests that seem to cover this code path, so I'm not sure about my sanity 😂

Comment on lines +102 to +112
return integrationDetails.sort((a, b) => {
if (a.integration_title != null && b.integration_title != null) {
return a.integration_title.localeCompare(b.integration_title);
} else if (a.integration_title != null) {
return a.integration_title.localeCompare(b.package_title);
} else if (b.integration_title != null) {
return a.package_title.localeCompare(b.integration_title);
} else {
return a.package_title.localeCompare(b.package_title);
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could this do the same?

  return integrationDetails.sort((a, b) => {
    const aTitle = a.integration_title ?? a.package_title;
    const bTitle = b.integration_title ?? b.package_title;
    return aTitle.localeCompare(bTitle);
  });

Also, if we always set integration_title in IntegrationDetails, could be even simpler:

  return integrationDetails.sort((a, b) => {
    return a.integration_title.localeCompare(b.integration_title);
  });

Then, in getIntegrationLink we wouldn't need to do ?? in const integrationTitle = integration.integration_title ?? integration.package_title;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we can always set integrationTitle to clean this up and then keep integrationName undefined if it's a single integration package (i.e. system package).

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synced on the comments with @spong and decided to address them in a follow-up PR.
🚀 🚀 🚀

@spong spong merged commit 7bfcb52 into elastic:main Jun 9, 2022
@spong spong deleted the related-integrations-fixes branch June 9, 2022 19:00
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.3 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 133050

Questions ?

Please refer to the Backport tool documentation

spong added a commit to spong/kibana that referenced this pull request 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 pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:Detection Rules Anything related to Security Solution's Detection Rules Feature:Rule Details Security Solution Detection Rule Details Feature:Rule Management Security Solution Detection Rule Management release_note:skip Skip the PR/issue when compiling release notes 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. Theme: simp_prot_mgmt Security Solution Simplified Protection Management Theme v8.3.0 v8.4.0
Projects
None yet
7 participants