[Security Solution][Platform] - Add connectors to import/export API #148703

WafaaNasr · 2023-01-11T08:47:24Z

Summary

Addresses [Security Solution][Platform] - Add connectors to import/export API #118774
Enable Security Rule to be imported even if one of its connectors has a missing secret
Shows Warning Callout in the Import Modal when missing secrets connector is imported.
Added Link connectors to the connectors page in the same tab, so that the user can fix imported connectors.
Added Overwrite existing connectors with conflicting action "id" option to the Import Modal

Cases:

Export:
- Export Rule(s) with connectors through Export All or Bulk Actions

Import:
- Import Rule with correct connectors data
- Import Rule with missing secrets' connectors by showing a warning callout
- Re-Import connectors even if they were stored when overwrite is true

Error:
- Showing an error message when the user has a Read Actions permission and stops the importing => You may not have actions privileges required to import rules with actions ...
- Showing an error message when the user has an old imported rule missing all the connectors data OR these connectors were not in the user's env => X connector is missing. Connector id missing is: X
- Showing an error if the new connectors defined in the exported file are not corresponding to the actions array under the rules param => X connector is missing. Connector id missing is: X
- Showing a conflict error in case of existing connectors and re-importing again with an overwrite false => this won't happen in case of implementing the Skipping action-connectors importing if all connectors have been imported/created before

Skip importing:
- Skipping action-connectors importing if the actions array is empty, even if the user has exported-connectors in the file
- Skipping action-connectors importing if all connectors have been imported/created before

Screenshots

1. Importing Connectors successfully

2. Importing Connectors with warnings

3.Connector Page

New text: @nastasha-solomon

1. Warning message

title => could be 1 connector imported or x connectors imported
message => 1 connector has sensitive information that requires updates. review in connectors or x connectors have sensitive information that requires updates. review in connectors

2. New Overwrite checkbox

3. Success Toast message

4. Error messages
a. Missing import action privileges

b. Missing connectors

References: Use getImporter and getExporter from Saved Object Connectors SO import/export implementation , Kibana-Core confirmation
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2023-01-11T08:47:28Z

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

…-fix'

…import-export-connectors-with-rules

…fix'

…github.com/WafaaNasr/kibana into 118774-import-export-connectors-with-rules

WafaaNasr · 2023-01-17T10:11:30Z

@elasticmachine merge upstream

kibanamachine · 2023-01-17T10:11:32Z

merge conflict between base and head

spong · 2023-02-01T23:35:45Z

...ty_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts

@@ -34,6 +34,8 @@ import { importRulesRoute } from './route';

 jest.mock('../../../../../machine_learning/authz');

+// TODO add tests for connectors


Are you planning to add them for this PR, or a follow up? 👍 if you'd like to follow-up with an additional test-coverage PR -- would be nice to add a few more FTR tests covering connectors as well.

I did already in this file x-pack/test/detection_engine_api_integration/security_and_spaces/group10/import_rules.ts which I found exactly the same as this file x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts and the interesting thing is this file is not referenced at all, that's why I didn't add tests there and I wanted to ask if we need this file actually?

So this file contains the jest unit tests against the route, whereas .../detection_engine_api_integration/security_and_spaces/group10/import_rules.ts are the FTR (functional test runner) API integration tests that actually stand up a kibana/es instance to test the import e2e.

This file would be good for testing the basic route inputs and such (or any other conditional logic specific to the route itself), but the latter is best for testing the complete flow from request to response, so good to have both. For these unit tests, the only connector specific test you might want to add is around the new overwrite_action_connectors query param that was added to the importQuerySchema, but as you mention, this could be covered by the FTR tests as well.

x-pack/test/detection_engine_api_integration/security_and_spaces/group10/perform_bulk_action.ts

...on/server/lib/detection_engine/rule_management/logic/import/action_connectors/utils/index.ts

..._engine/rule_management/logic/import/action_connectors/import_rule_action_connectors.test.ts

spong

Checked out, tested locally, and did a high-level code review and LGTM! 👍 🚀 🙂

I did add a few comments/questions for some future cleanup and additional tests, but no changes required in this PR.
As a note, I did find two tangential issues ([Security Solution] Rule import error toast is too large to view title when many errors happen on import #149994, [Security Solution] Unable to view multiple errors toasts when rule import fails #149995) in testing, but they were pre-existing.
Also, as mentioned in an above comment, rule import is currently broken (as of [Security Solution] Write and read Rule Execution Logs from rule instead of saved object #147035) since we're exporting additional fields that break validation on import, so I went ahead and patched that locally by returning execution_summary: undefined over in internalRuleToAPIResponse() so I could perform full e2e testing.
I went ahead and added the needs_docs label as a reminder that we'll need to update our API docs with this change

Overall looks great @WafaaNasr -- thank you for all your efforts here and getting our users closer to a one-click backup/recover workflow! 🙌 🙂

WafaaNasr · 2023-02-02T11:52:00Z

Checked out, tested locally, and did a high-level code review and LGTM! 👍 🚀 🙂

I did add a few comments/questions for some future cleanup and additional tests, but no changes required in this PR.

As a note, I did find two tangential issues ([Security Solution] Rule import error toast is too large to view title when many errors happen on import #149994, [Security Solution] Unable to view multiple errors toasts when rule import fails #149995) in testing, but they were pre-existing.

Also, as mentioned in an above comment, rule import is currently broken (as of [Security Solution] Write and read Rule Execution Logs from rule instead of saved object #147035) since we're exporting additional fields that break validation on import, so I went ahead and patched that locally by returning execution_summary: undefined over in internalRuleToAPIResponse() so I could perform full e2e testing.

I went ahead and added the needs_docs label as a reminder that we'll need to update our API docs with this change

Overall looks great @WafaaNasr -- thank you for all your efforts here and getting our users closer to a one-click backup/recover workflow! 🙌 🙂

Thank you so much @spong for your very thorough review!! really appreciated 🙏🏻😊
I will address all the comments thanks!

nastasha-solomon · 2023-02-02T16:50:23Z

@ARWNightingale do you have any objections to changing the last part of this warning message from review in connectors to Go to connectors.?

This change would align the message with the one that displays when users perform a similar act on the Saved Objects page, which is importing a connector that needs configuration details re-applied or fixed. I think the consistent messaging can encourage the same behavior (i.e., go to the Connectors page to fix your connectors) and create a seamless experience for users whether they're receiving the warning from the Import rules modal or the Saved Objects page.

cc: @WafaaNasr

WafaaNasr · 2023-02-02T22:37:40Z

Thanks for pulling the copy together for review, @WafaaNasr! I've left some suggestions and questions below for your consideration. Hopefully they're helpful!

1. Warning message

Title: Would it be possible to change the title so it's more descriptive of the action that we want users to take. Instead of x connector imported can it be x connectors need your attention or x connectors are missing information?
Message: The phrase "sensitive information" is accurate but sounds a little scary. What do you think about changing it to describe what needs to be re-entered or re-applied? How about something like x connectors must be re-authenticated or are missing configuration details. Fix them here. (here would be linked to the Connectors page) => Will leave the message as it is to be consistent with the Saved Object page under Stack Management

2. New Overwrite checkbox I recommend removing action before connectors since we generally refer to connectors without it. Here's what I'm suggesting: Overwrite existing connectors with conflicting action "id" => DONE please validate in the new cloud build

3. Success Toast message LGTM!

4. Error messages

Missing import action privileges: Will users see this error message in any other situations or does it only appear if they for sure don't have the necessary feature privileges? If this only appears because they're missing feature privs, I recommend changing the message to something like You need additional privileges to import rules with actions. Refer to [the documentation](https://www.elastic.co/guide/en/security/master/detections-permissions-section.html#enable-detections-ui) for more information.

=> DONE please validate in the new cloud build, as agreed the new message is You need additional privileges to import rules with actions. only
- Missing connectors: Can you provide a little background on the purpose of this error message and what it describes? Why would a connector be missing? What should users do when they encounter this error? => ** Covered on our meeting :) **

@nastasha-solomon thanks for the comments, please find the updated state per each item :)

nkhristinin · 2023-02-06T12:52:03Z

@elasticmachine merge upstream

WafaaNasr · 2023-02-06T12:53:55Z

@ARWNightingale do you have any objections to changing the last part of this warning message from review in connectors to Go to connectors.?

This change would align the message with the one that displays when users perform a similar act on the Saved Objects page, which is importing a connector that needs configuration details re-applied or fixed. I think the consistent messaging can encourage the same behavior (i.e., go to the Connectors page to fix your connectors) and create a seamless experience for users whether they're receiving the warning from the Import rules modal or the Saved Objects page.

cc: @WafaaNasr

@nastasha-solomon I confirmed with @ARWNightingale that we are going to implement the same UI as in the Saved Object page, I will add the Go to connectors button instead of review in connectors`

WafaaNasr · 2023-02-06T14:37:34Z

@elasticmachine merge upstream

XavierM

Response Ops side looks good

kibana-ci · 2023-02-06T15:08:27Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3584	3585	+1

Async chunks

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

id	before	after	diff
`infra`	1.3MB	1.3MB	+31.0B
`lists`	152.1KB	152.1KB	+30.0B
`securitySolution`	12.9MB	12.9MB	+13.5KB
total			+13.6KB

History

💔 Build #105686 failed eb6dfe5
💚 Build #105648 succeeded 2da1121
💚 Build #105314 succeeded 31bcd73
💛 Build #104860 was flaky 7b2c13e
💛 Build #104801 was flaky f192914

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

cc @WafaaNasr

nkhristinin

Tested locally, LGTM!
Great work!

nkhristinin · 2023-02-06T14:35:23Z

x-pack/test/detection_engine_api_integration/security_and_spaces/tests/import_rules.ts

@@ -589,6 +633,18 @@ export default ({ getService }: FtrProviderContext): void => {
          exceptions_errors: [],
          exceptions_success: true,
          exceptions_success_count: 0,
+          action_connectors_success: true,


Why do we have here action_connectors_success but also action_connectors_errors

That's a good question, actually, this test is not needed as per this discussion initially I added the default values of the exported connectors, just to mimic the exported file if no actions' connectors were exported.

use getImporter and getExporter from Saved Object

ea2319e

WafaaNasr self-assigned this Jan 11, 2023

WafaaNasr requested a review from a team as a code owner January 11, 2023 08:47

WafaaNasr requested a review from spong January 11, 2023 08:47

[CI] Auto-commit changed files from 'node scripts/ts_project_linter -…

dddba87

…-fix'

kibanamachine requested a review from a team as a code owner January 11, 2023 08:52

WafaaNasr added the ci:cloud-deploy Create or update a Cloud deployment label Jan 11, 2023

WafaaNasr added 4 commits January 11, 2023 10:55

Merge branch 'main' of https://github.com/elastic/kibana into 118774-…

1d5af44

…import-export-connectors-with-rules

resolve conflicts

2f5dcb4

fix tests

605ba89

fix get export by id

c9a2656

elastic deleted a comment from kibana-ci Jan 11, 2023

kibanamachine and others added 4 commits January 11, 2023 11:22

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

9b2cb0e

…fix'

comment warnings until decide if needed

f5a199b

Merge branch '118774-import-export-connectors-with-rules' of https://…

eb98ea3

…github.com/WafaaNasr/kibana into 118774-import-export-connectors-with-rules

fix tests

0a00326

elastic deleted a comment from kibana-ci Jan 11, 2023

enable Rule importing even if connectors are missing secrets

eca3cf3

WafaaNasr requested a review from a team as a code owner January 17, 2023 10:10

merge with latest

3f56c94

elastic deleted a comment from kibana-ci Jan 17, 2023

test: Import rules response schema

912b6c9

spong reviewed Feb 1, 2023

View reviewed changes

spong added the needs_docs label Feb 1, 2023

spong reviewed Feb 1, 2023

View reviewed changes

x-pack/test/detection_engine_api_integration/security_and_spaces/group10/perform_bulk_action.ts Show resolved Hide resolved

spong reviewed Feb 2, 2023

View reviewed changes

...on/server/lib/detection_engine/rule_management/logic/import/action_connectors/utils/index.ts Show resolved Hide resolved

spong reviewed Feb 2, 2023

View reviewed changes

..._engine/rule_management/logic/import/action_connectors/import_rule_action_connectors.test.ts Outdated Show resolved Hide resolved

spong approved these changes Feb 2, 2023

View reviewed changes

WafaaNasr added 2 commits February 2, 2023 18:15

apply docs team changes

680530f

apply comments

31bcd73

Merge branch 'main' into 118774-import-export-connectors-with-rules

879ea94

WafaaNasr added 3 commits February 6, 2023 13:15

apply same ux as in the connectors page

5098552

merge with latest

2da1121

remove todos already addressed

eb6dfe5

Merge branch 'main' into 118774-import-export-connectors-with-rules

6dac771

XavierM approved these changes Feb 6, 2023

View reviewed changes

nkhristinin approved these changes Feb 6, 2023

View reviewed changes

dhurley14 approved these changes Feb 6, 2023

View reviewed changes

WafaaNasr merged commit 8733774 into elastic:main Feb 6, 2023

WafaaNasr deleted the 118774-import-export-connectors-with-rules branch February 6, 2023 16:47

kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Feb 6, 2023

elasticmachine unassigned WafaaNasr Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Platform] - Add connectors to import/export API #148703

[Security Solution][Platform] - Add connectors to import/export API #148703

WafaaNasr commented Jan 11, 2023 •

edited

Loading

elasticmachine commented Jan 11, 2023

WafaaNasr commented Jan 17, 2023

kibanamachine commented Jan 17, 2023

spong Feb 1, 2023

WafaaNasr Feb 2, 2023

spong Feb 2, 2023

spong left a comment •

edited

Loading

WafaaNasr commented Feb 2, 2023 •

edited

Loading

nastasha-solomon commented Feb 2, 2023

WafaaNasr commented Feb 2, 2023 •

edited

Loading

nkhristinin commented Feb 6, 2023

WafaaNasr commented Feb 6, 2023

WafaaNasr commented Feb 6, 2023

XavierM left a comment

kibana-ci commented Feb 6, 2023 •

edited

Loading

nkhristinin left a comment •

edited

Loading

nkhristinin Feb 6, 2023

WafaaNasr Feb 6, 2023 •

edited

Loading

		@@ -34,6 +34,8 @@ import { importRulesRoute } from './route';

		jest.mock('../../../../../machine_learning/authz');

		// TODO add tests for connectors

[Security Solution][Platform] - Add connectors to import/export API #148703

[Security Solution][Platform] - Add connectors to import/export API #148703

Conversation

WafaaNasr commented Jan 11, 2023 • edited Loading

Summary

Cases:

Screenshots

New text: @nastasha-solomon

elasticmachine commented Jan 11, 2023

WafaaNasr commented Jan 17, 2023

kibanamachine commented Jan 17, 2023

spong Feb 1, 2023

Choose a reason for hiding this comment

WafaaNasr Feb 2, 2023

Choose a reason for hiding this comment

spong Feb 2, 2023

Choose a reason for hiding this comment

spong left a comment • edited Loading

Choose a reason for hiding this comment

WafaaNasr commented Feb 2, 2023 • edited Loading

nastasha-solomon commented Feb 2, 2023

WafaaNasr commented Feb 2, 2023 • edited Loading

nkhristinin commented Feb 6, 2023

WafaaNasr commented Feb 6, 2023

WafaaNasr commented Feb 6, 2023

XavierM left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 6, 2023 • edited Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

nkhristinin left a comment • edited Loading

Choose a reason for hiding this comment

nkhristinin Feb 6, 2023

Choose a reason for hiding this comment

WafaaNasr Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

WafaaNasr commented Jan 11, 2023 •

edited

Loading

spong left a comment •

edited

Loading

WafaaNasr commented Feb 2, 2023 •

edited

Loading

WafaaNasr commented Feb 2, 2023 •

edited

Loading

kibana-ci commented Feb 6, 2023 •

edited

Loading

nkhristinin left a comment •

edited

Loading

WafaaNasr Feb 6, 2023 •

edited

Loading