[Actions] Extended ActionTypeRegistry with connector validation to validate config with secrets #116079

YulNaumenko · 2021-10-25T04:53:52Z

Currently there was no ability to validate connector config with secrets values together, which causes to the issues in the API validation for .email action type:

validate if hasAuth (config) is true the username/password (secrets) should be populated;
validate if service (config) is exchange_server then clientSecret (secrets) should be populated;
Added new validation method for connector, which allows to implement by the action type the ability to validate connector properties secrets with config.

…lidate config with secrets

elasticmachine · 2021-10-25T17:41:54Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

pmuellr · 2021-10-25T21:07:44Z

Even though this is how I was thinking of implementing it, having to duplicate the schema references for config and secrets in the connector validation seems like it may be fragile in the future. What happens if someone has different schema for the config and the config section in the connector schema?

Wondering if to make this a little clearer, we'd want to have the connector validator not be config-schema based, but a function that returns the same results as schema.validate() but takes the typed config and secrets objects.

The existing validation function in the PR would then be unchanged! And the definition of the connector validator in types.ts would be:

    connector?: (config: Config, secrets: Secrets) => string | null;

I could go either way though, not sure it's worth having a "different" shaped validation makes sense, since the way this is currently shaped seems more consistent that what I proposed ^^^. And:

I think in practice we won't see too much of a need for this anyway
referencing the existing schema for config and secrets is likely how it would be used when defining this new validator, exactly how it's done in this PR for email, so that eliminates / greatly reduces the possibility of them getting out of sync
if they were wildly out of sync, you'd expect to find out with jest / functional tests
since we have control over all the connectors right now, changing this to a slightly different shape (like I suggested) would be a simple thing we could do in the future - that is UNTIL we allow other plugins to define connectors!

…ulNaumenko/kibana into actions-config-with-secrets-validation

YulNaumenko · 2021-10-26T00:24:47Z

Even though this is how I was thinking of implementing it, having to duplicate the schema references for config and secrets in the connector validation seems like it may be fragile in the future. What happens if someone has different schema for the config and the config section in the connector schema?

Wondering if to make this a little clearer, we'd want to have the connector validator not be config-schema based, but a function that returns the same results as schema.validate() but takes the typed config and secrets objects.

The existing validation function in the PR would then be unchanged! And the definition of the connector validator in types.ts would be:
    connector?: (config: Config, secrets: Secrets) => string | null;
I could go either way though, not sure it's worth having a "different" shaped validation makes sense, since the way this is currently shaped seems more consistent that what I proposed ^^^. And:

I think in practice we won't see too much of a need for this anyway

referencing the existing schema for config and secrets is likely how it would be used when defining this new validator, exactly how it's done in this PR for email, so that eliminates / greatly reduces the possibility of them getting out of sync

if they were wildly out of sync, you'd expect to find out with jest / functional tests

since we have control over all the connectors right now, changing this to a slightly different shape (like I suggested) would be a simple thing we could do in the future - that is UNTIL we allow other plugins to define connectors!

Thank you for a great thoughts @pmuellr! Yeah, I think you are right about the changes to the different shape of the connector validator.
My initial idea was to implement something which could replace/deprecate the existing separate validation in the future, but probably better to keep it as it is.

chrisronline

LGTM! Great work!

pmuellr

LGTM; made one note about some typing that we should probably fix up, if possible - and could do it later; also there's a test which doesn't seem like it's testing the connector validator, so I'm guessing it's some copy pasta.

x-pack/plugins/actions/server/lib/action_executor.test.ts

x-pack/plugins/actions/server/lib/action_executor.ts

kibanamachine · 2021-10-26T21:58:41Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #1889 failed 8cf9415
💔 Build #1662 failed 6684e70
💔 Build #1610 failed 186caf8
💔 Build #1571 failed 06f609c
💔 Build #1534 failed 7bd430c
💔 Build #1516 failed c009800

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

cc @YulNaumenko

…-migrate-away-from-injected-css-js * 'master' of github.com:elastic/kibana: (61 commits) [ML] Nodes overview for the Model Management page (elastic#116361) [Uptime] Uptime index config using kibana.yml (elastic#115775) [Controls] Dashboard Integration (elastic#115991) skip flaky suite (elastic#104260) Include Files in GitHub UI (elastic#115956) skip flaky suite (elastic#116060) [Canvas] By-Value Embeddables (elastic#113827) Skip failing test (elastic#115366) [Osquery] Fix live query search doesn't return relevant results for agents (elastic#116332) [Integrations] Added link in old Add Data description and fixed alignment in cards (elastic#116213) [Actions] Extended ActionTypeRegistry with connector validation to validate config with secrets (elastic#116079) skip flaky suite (elastic#109329) Grant access to machine learning features when base privileges are used (elastic#115444) Skipping failing test (elastic#84957) [RAC][Security Solution] Adds migration to new SecuritySolution rule types (elastic#112113) skip flaky suite (elastic#115366) [Fleet] Marking API spec as experimental (elastic#116331) [Docs] Cleaning up the versions in the upgrade paths. Closes elastic#116223 (elastic#116228) [Reporting] Suppress debug logs in the mock logger (elastic#116012) [Metrics UI] Clear threshold alert groups state when filterQuery changes (elastic#116205) ... # Conflicts: # src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx # src/plugins/dashboard/public/types.ts

kibanamachine · 2021-10-28T23:13:22Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 116079 or prevent reminders by adding the backport:skip label.

kibanamachine · 2021-10-29T23:23:21Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 116079 or prevent reminders by adding the backport:skip label.

[Actions] Extended ActionTypeRegistry with connector validation to va…

1f5d2d3

…lidate config with secrets

YulNaumenko self-assigned this Oct 25, 2021

YulNaumenko added 4 commits October 25, 2021 10:15

fixed typecheck

71896d4

added tests

b3c4044

more tests

0b78c4b

email validation fix

c009800

YulNaumenko added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/RuleActions Issues related to the Actions attached to Rules on the Alerting Framework v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 25, 2021

YulNaumenko marked this pull request as ready for review October 25, 2021 17:41

YulNaumenko requested a review from a team as a code owner October 25, 2021 17:41

YulNaumenko added 4 commits October 25, 2021 11:07

fixed typecheck

8ca5a83

fixed typecheck

7bd430c

added tests

06f609c

fixed typecheck

186caf8

YulNaumenko added 2 commits October 25, 2021 17:03

fixed test

cdbef89

Merge branch 'actions-config-with-secrets-validation' of github.com:Y…

bfa777c

…ulNaumenko/kibana into actions-config-with-secrets-validation

fixed due to comments

6684e70

chrisronline approved these changes Oct 26, 2021

View reviewed changes

YulNaumenko added 2 commits October 26, 2021 11:57

fixed test

8cf9415

fixed test

ecb85bd

pmuellr approved these changes Oct 26, 2021

View reviewed changes

x-pack/plugins/actions/server/lib/action_executor.test.ts Outdated Show resolved Hide resolved

x-pack/plugins/actions/server/lib/action_executor.ts Outdated Show resolved Hide resolved

fixed typecheck

672a0e5

YulNaumenko merged commit fac6286 into elastic:master Oct 26, 2021

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 28, 2021

spalger added the backport:skip This commit does not require backporting label Nov 1, 2021

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 1, 2021

ymao1 mentioned this pull request Jan 5, 2022

change test to match current message when decryption fails #122224

Merged

cnasikas mentioned this pull request Jan 19, 2022

[Connectors] Checking for undefined config and secrets during connector validation #122696

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Actions] Extended ActionTypeRegistry with connector validation to validate config with secrets #116079

[Actions] Extended ActionTypeRegistry with connector validation to validate config with secrets #116079

YulNaumenko commented Oct 25, 2021 •

edited

Loading

elasticmachine commented Oct 25, 2021

pmuellr commented Oct 25, 2021 •

edited

Loading

YulNaumenko commented Oct 26, 2021

chrisronline left a comment

pmuellr left a comment

kibanamachine commented Oct 26, 2021

kibanamachine commented Oct 28, 2021

kibanamachine commented Oct 29, 2021

[Actions] Extended ActionTypeRegistry with connector validation to validate config with secrets #116079

[Actions] Extended ActionTypeRegistry with connector validation to validate config with secrets #116079

Conversation

YulNaumenko commented Oct 25, 2021 • edited Loading

elasticmachine commented Oct 25, 2021

pmuellr commented Oct 25, 2021 • edited Loading

YulNaumenko commented Oct 26, 2021

chrisronline left a comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 26, 2021

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Oct 28, 2021

kibanamachine commented Oct 29, 2021

YulNaumenko commented Oct 25, 2021 •

edited

Loading

pmuellr commented Oct 25, 2021 •

edited

Loading