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

Define minimum license required for each action type #58668

Conversation

mikecote
Copy link
Contributor

⚠️ This PR merges into a feature branch

In this PR, I'm implementing step 1, 2 and 3 of my license check implementation proposal: #54946 (comment).

I'm adding a new minimumLicenseRequired attribute to each action type definition. Built-in action types can set a Basic+ license while non-built-in action types can only set a gold+ license.

@mikecote mikecote added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Feb 26, 2020
@mikecote mikecote self-assigned this Feb 26, 2020
@elasticmachine
Copy link
Contributor

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

if (!(actionType.minimumLicenseRequired in LICENSE_TYPE)) {
throw new Error(`"${actionType.minimumLicenseRequired}" is not a valid license type`);
}
if (LICENSE_TYPE[actionType.minimumLicenseRequired] < LICENSE_TYPE.gold) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elastic/kibana-alerting-services @peterschretlen do we want to allow other plugins to register basic license action types? With the code I have now, only built-in action types can specify basic as their minimum license required.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mikecote 👍 the way you have it is correct. Custom actions would be gold or higher.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM, made some comments

@@ -31,6 +31,7 @@ describe('register()', () => {
actionTypeRegistry.register({
id: 'my-action-type',
name: 'My action type',
minimumLicenseRequired: 'basic',
Copy link
Member

Choose a reason for hiding this comment

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

maybe there's an hook later, but this seems to violate "non-built-in action types can only set a gold+ license"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can set anything here but it will be validated when you register. The registration validation exists here: https://github.com/elastic/kibana/pull/58668/files#diff-adc6606fd00ebc8b46eecefae314f1b5R187 and the built ins don't go through there, they register directly to the actionTypeRegistry.

x-pack/plugins/actions/server/lib/action_executor.test.ts Outdated Show resolved Hide resolved
@mikecote mikecote marked this pull request as ready for review March 2, 2020 14:59
@mikecote mikecote requested a review from a team as a code owner March 2, 2020 14:59
@mikecote
Copy link
Contributor Author

mikecote commented Mar 2, 2020

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

mikecote commented Mar 2, 2020

@elasticmachine merge upstream

@@ -89,6 +89,7 @@ export function getActionType({
}): ActionType {
return {
id: '.servicenow',
minimumLicenseRequired: 'gold',

Choose a reason for hiding this comment

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

@mikecote @peterschretlen I just spoke with @MikePaquette, can we make this platinum. There is a good chance that's where it will land

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I went ahead and made it platinum here: 9669ebd.

Copy link
Contributor

@peterschretlen peterschretlen left a comment

Choose a reason for hiding this comment

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

LGTM, though see Alex's comment on ServiceNow.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@mikecote mikecote merged commit a28e6db into elastic:actions/license-checks Mar 2, 2020
mikecote added a commit that referenced this pull request Mar 20, 2020
* Define minimum license required for each action type (#58668)

* Add minimum required license

* Require at least gold license as a minimum license required on third party action types

* Use strings for license references

* Ensure license type is valid

* Fix some tests

* Add servicenow to gold

* Add tests

* Set license requirements on other built in action types

* Use jest.Mocked<ActionType> instead

* Change servicenow to platinum

Co-authored-by: Elastic Machine <[email protected]>

* Make actions config mock and license state mock use factory pattern and jest mocks (#59370)

* Add license checks to action HTTP APIs (#59153)

* Initial work

* Handle errors in update action API

* Add unit tests for APIs

* Make action executor throw when action type isn't enabled

* Add test suite for basic license

* Fix ESLint errors

* Fix failing tests

* Attempt 1 to fix CI

* ESLint fixes

* Create sendResponse function on ActionTypeDisabledError

* Make disabled action types by config return 403

* Remove switch case

* Fix ESLint

* Add license checks within alerting / actions framework (#59699)

* Initial work

* Handle errors in update action API

* Add unit tests for APIs

* Verify action type before scheduling action task

* Make actions plugin.execute throw error if action type is disabled

* Bug fixes

* Make action executor throw when action type isn't enabled

* Add test suite for basic license

* Fix ESLint errors

* Stop action task from re-running when license check fails

* Fix failing tests

* Attempt 1 to fix CI

* ESLint fixes

* Create sendResponse function on ActionTypeDisabledError

* Make disabled action types by config return 403

* Remove switch case

* Fix ESLint

* Fix confusing assertion

* Add comment explaining double mock

* Log warning when alert action isn't scheduled

* Disable action types in UI when license doesn't support it (#59819)

* Initial work

* Handle errors in update action API

* Add unit tests for APIs

* Verify action type before scheduling action task

* Make actions plugin.execute throw error if action type is disabled

* Bug fixes

* Make action executor throw when action type isn't enabled

* Add test suite for basic license

* Fix ESLint errors

* Stop action task from re-running when license check fails

* Fix failing tests

* Attempt 1 to fix CI

* ESLint fixes

* Return enabledInConfig and enabledInLicense from actions get types API

* Disable cards that have invalid license in create connector flyout

* Create sendResponse function on ActionTypeDisabledError

* Make disabled action types by config return 403

* Remove switch case

* Fix ESLint

* Disable when creating alert action

* Return minimumLicenseRequired in /types API

* Disable row in connectors when action type is disabled

* Fix failing jest test

* Some refactoring

* Card in edit alert flyout

* Sort action types by name

* Add tooltips to create connector action type selector

* Add tooltips to alert flyout action type selector

* Add get more actions link in alert flyout

* Add callout when creating a connector

* Typos

* remove float right and use flexgroup

* replace pixels with eui variables

* turn on sass lint for triggers_actions_ui dir

* trying to add padding around cards

* Add callout in edit alert screen when some actions are disabled

* improve card selection for Add Connector flyout

* Fix cards for create connector

* Add tests

* ESLint issue

* Cleanup

* Cleanup pt2

* Fix type check errors

* moving to 3-columns cards for connector selection

* Change re-enable to enable terminology

* Revert "Change re-enable to enable terminology"

This reverts commit b497dfd.

* Add re-enable comment

* Remove unecessary fragment

* Add type to actionTypeNodes

* Fix EuiLink to not have opacity of 0.7 when not hovered

* design cleanup in progress

* updating classNames

* using EuiIconTip

* Remove label on icon tip

* Fix failing jest test

Co-authored-by: Andrea Del Rio <[email protected]>

* Add index to .index action type test

* PR feedback

* Add isErrorThatHandlesItsOwnResponse

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Andrea Del Rio <[email protected]>
mikecote added a commit to mikecote/kibana that referenced this pull request Mar 20, 2020
* Define minimum license required for each action type (elastic#58668)

* Add minimum required license

* Require at least gold license as a minimum license required on third party action types

* Use strings for license references

* Ensure license type is valid

* Fix some tests

* Add servicenow to gold

* Add tests

* Set license requirements on other built in action types

* Use jest.Mocked<ActionType> instead

* Change servicenow to platinum

Co-authored-by: Elastic Machine <[email protected]>

* Make actions config mock and license state mock use factory pattern and jest mocks (elastic#59370)

* Add license checks to action HTTP APIs (elastic#59153)

* Initial work

* Handle errors in update action API

* Add unit tests for APIs

* Make action executor throw when action type isn't enabled

* Add test suite for basic license

* Fix ESLint errors

* Fix failing tests

* Attempt 1 to fix CI

* ESLint fixes

* Create sendResponse function on ActionTypeDisabledError

* Make disabled action types by config return 403

* Remove switch case

* Fix ESLint

* Add license checks within alerting / actions framework (elastic#59699)

* Initial work

* Handle errors in update action API

* Add unit tests for APIs

* Verify action type before scheduling action task

* Make actions plugin.execute throw error if action type is disabled

* Bug fixes

* Make action executor throw when action type isn't enabled

* Add test suite for basic license

* Fix ESLint errors

* Stop action task from re-running when license check fails

* Fix failing tests

* Attempt 1 to fix CI

* ESLint fixes

* Create sendResponse function on ActionTypeDisabledError

* Make disabled action types by config return 403

* Remove switch case

* Fix ESLint

* Fix confusing assertion

* Add comment explaining double mock

* Log warning when alert action isn't scheduled

* Disable action types in UI when license doesn't support it (elastic#59819)

* Initial work

* Handle errors in update action API

* Add unit tests for APIs

* Verify action type before scheduling action task

* Make actions plugin.execute throw error if action type is disabled

* Bug fixes

* Make action executor throw when action type isn't enabled

* Add test suite for basic license

* Fix ESLint errors

* Stop action task from re-running when license check fails

* Fix failing tests

* Attempt 1 to fix CI

* ESLint fixes

* Return enabledInConfig and enabledInLicense from actions get types API

* Disable cards that have invalid license in create connector flyout

* Create sendResponse function on ActionTypeDisabledError

* Make disabled action types by config return 403

* Remove switch case

* Fix ESLint

* Disable when creating alert action

* Return minimumLicenseRequired in /types API

* Disable row in connectors when action type is disabled

* Fix failing jest test

* Some refactoring

* Card in edit alert flyout

* Sort action types by name

* Add tooltips to create connector action type selector

* Add tooltips to alert flyout action type selector

* Add get more actions link in alert flyout

* Add callout when creating a connector

* Typos

* remove float right and use flexgroup

* replace pixels with eui variables

* turn on sass lint for triggers_actions_ui dir

* trying to add padding around cards

* Add callout in edit alert screen when some actions are disabled

* improve card selection for Add Connector flyout

* Fix cards for create connector

* Add tests

* ESLint issue

* Cleanup

* Cleanup pt2

* Fix type check errors

* moving to 3-columns cards for connector selection

* Change re-enable to enable terminology

* Revert "Change re-enable to enable terminology"

This reverts commit b497dfd.

* Add re-enable comment

* Remove unecessary fragment

* Add type to actionTypeNodes

* Fix EuiLink to not have opacity of 0.7 when not hovered

* design cleanup in progress

* updating classNames

* using EuiIconTip

* Remove label on icon tip

* Fix failing jest test

Co-authored-by: Andrea Del Rio <[email protected]>

* Add index to .index action type test

* PR feedback

* Add isErrorThatHandlesItsOwnResponse

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Andrea Del Rio <[email protected]>
mikecote added a commit that referenced this pull request Mar 20, 2020
* Define minimum license required for each action type (#58668)

* Add minimum required license

* Require at least gold license as a minimum license required on third party action types

* Use strings for license references

* Ensure license type is valid

* Fix some tests

* Add servicenow to gold

* Add tests

* Set license requirements on other built in action types

* Use jest.Mocked<ActionType> instead

* Change servicenow to platinum

Co-authored-by: Elastic Machine <[email protected]>

* Make actions config mock and license state mock use factory pattern and jest mocks (#59370)

* Add license checks to action HTTP APIs (#59153)

* Initial work

* Handle errors in update action API

* Add unit tests for APIs

* Make action executor throw when action type isn't enabled

* Add test suite for basic license

* Fix ESLint errors

* Fix failing tests

* Attempt 1 to fix CI

* ESLint fixes

* Create sendResponse function on ActionTypeDisabledError

* Make disabled action types by config return 403

* Remove switch case

* Fix ESLint

* Add license checks within alerting / actions framework (#59699)

* Initial work

* Handle errors in update action API

* Add unit tests for APIs

* Verify action type before scheduling action task

* Make actions plugin.execute throw error if action type is disabled

* Bug fixes

* Make action executor throw when action type isn't enabled

* Add test suite for basic license

* Fix ESLint errors

* Stop action task from re-running when license check fails

* Fix failing tests

* Attempt 1 to fix CI

* ESLint fixes

* Create sendResponse function on ActionTypeDisabledError

* Make disabled action types by config return 403

* Remove switch case

* Fix ESLint

* Fix confusing assertion

* Add comment explaining double mock

* Log warning when alert action isn't scheduled

* Disable action types in UI when license doesn't support it (#59819)

* Initial work

* Handle errors in update action API

* Add unit tests for APIs

* Verify action type before scheduling action task

* Make actions plugin.execute throw error if action type is disabled

* Bug fixes

* Make action executor throw when action type isn't enabled

* Add test suite for basic license

* Fix ESLint errors

* Stop action task from re-running when license check fails

* Fix failing tests

* Attempt 1 to fix CI

* ESLint fixes

* Return enabledInConfig and enabledInLicense from actions get types API

* Disable cards that have invalid license in create connector flyout

* Create sendResponse function on ActionTypeDisabledError

* Make disabled action types by config return 403

* Remove switch case

* Fix ESLint

* Disable when creating alert action

* Return minimumLicenseRequired in /types API

* Disable row in connectors when action type is disabled

* Fix failing jest test

* Some refactoring

* Card in edit alert flyout

* Sort action types by name

* Add tooltips to create connector action type selector

* Add tooltips to alert flyout action type selector

* Add get more actions link in alert flyout

* Add callout when creating a connector

* Typos

* remove float right and use flexgroup

* replace pixels with eui variables

* turn on sass lint for triggers_actions_ui dir

* trying to add padding around cards

* Add callout in edit alert screen when some actions are disabled

* improve card selection for Add Connector flyout

* Fix cards for create connector

* Add tests

* ESLint issue

* Cleanup

* Cleanup pt2

* Fix type check errors

* moving to 3-columns cards for connector selection

* Change re-enable to enable terminology

* Revert "Change re-enable to enable terminology"

This reverts commit b497dfd.

* Add re-enable comment

* Remove unecessary fragment

* Add type to actionTypeNodes

* Fix EuiLink to not have opacity of 0.7 when not hovered

* design cleanup in progress

* updating classNames

* using EuiIconTip

* Remove label on icon tip

* Fix failing jest test

Co-authored-by: Andrea Del Rio <[email protected]>

* Add index to .index action type test

* PR feedback

* Add isErrorThatHandlesItsOwnResponse

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Andrea Del Rio <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Andrea Del Rio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants