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

License checks for actions plugin #59070

Merged
merged 22 commits into from
Mar 20, 2020
Merged

License checks for actions plugin #59070

merged 22 commits into from
Mar 20, 2020

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Mar 2, 2020

Resolves #54946

In this PR, we're making action types license aware and adding the proper behaviours to the APIs.

This branch is a feature branch that sub PRs are merging into until the feature is complete.

The following PRs merged into this feature branch

Define minimum license required for each action type (#58668)

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.

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

In this PR, I'm changing the mocks for license state and actions config to allow my follow up PR to have unit tests that mock different responses and implementations from these classes.

Add license checks to action HTTP APIs (#59153)

In this PR, I'm implementing step 4, 9 and 14 of my license check implementation proposal: #54946 (comment). It was discussed further that response status of 403 would make more sense which is included in this PR.

I did notice the execute API always returns status of 200 but will contain the error information within the response. By design, the ensureActionTypeEnabled validation gets wrapped in a try catch and then returns the error in a JSON structure. I'm thinking of staying consistent instead of adding a special scenario. Would be curious of other's think?
EDIT: Execute API now returns 403 errors for license checks.

A few extra changes with this PR:

  • The update action API won't allow updates for disabled action types
  • I renamed the license_api_access.ts file to verify_api_access.ts in order to match what it exports
  • The license check on execution will also apply for alerts but only after a task is scheduled and attempting to run. This will be improved with step 5 and 7 of Action registration / framework level license check #54946 (comment) to keep this PR small.

You will notice there isn't any functional tests. It would require creating different configurations for each license (basic, gold, etc) and adding CI time of > 5-10 minutes per configuration. I have focused heavily on unit tests for these but would be curious to know what others think?
EDIT: Functional tests are now in place.

Below are the APIs and their new responses that will return in such scenario:

POST /api/action

Status Code: 403 Forbidden

{
   "statusCode":403,
   "error":"Forbidden",
   "message":"Action type .slack is disabled because your basic license does not support it. Please upgrade your license."
}

PUT /api/action/{id}

Status Code: 403 Forbidden

{
   "statusCode":403,
   "error":"Forbidden",
   "message":"Action type .slack is disabled because your basic license does not support it. Please upgrade your license."
}

POST /api/action/{id}/_execute

Status Code: 403 Forbidden

{
    "statusCode": 403,
    "error": "Forbidden",
    "message": "Action type .server-log is disabled because your basic license does not support it. Please upgrade your license."
}

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

In this PR, I'm adding license checks within the alerting framework for the areas touched by non-HTTP API calls.

  • Calling the action plugin execute function (exposed by start contract) will now throw an error if the action type is disabled via config OR license
  • The action task runner will stop retrying failures caused by action type disabled via config OR license
  • Alerts will skip scheduling actions for action types that are disabled via config OR license

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

In this PR, I'm adding UI indicators whenever a connector is disabled via config or via license (see below for screenshots). Users can only delete existing connectors.

* 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]>
@mikecote mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Mar 2, 2020
@mikecote mikecote self-assigned this Mar 2, 2020
@elasticmachine
Copy link
Contributor

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

* 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
* 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
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the whole thing yet, but just leaving this note in the context of our previous conversation

Comment on lines 54 to 56
if (e instanceof ActionTypeDisabledError) {
return e.sendResponse(res);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the context of our conversation about handling the Api Keys error differently, perhaps we could do this more generically using a duck-typing approach.

Have a Type like:

interface ErrorThatHandlesItsOwnResponse {
  sendResponse(res: KibanaResponse);
}

function isErrorThatHandlesItsOwnResponse(e: Error) : e is ErrorThatHandlesItsOwnResponse {
  return typeof (e as ErrorThatHandlesItsOwnResponse)['sendResponse'] === 'function';
}

and then in our Routes we could do:

if (isErrorThatHandlesItsOwnResponse(e)) {
  return e.sendResponse(res);
}

Copy link
Member

Choose a reason for hiding this comment

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

Ya, I immediately thought of some Boom-like approaches here. Suggest we use the "remove Boom" issue #49822 as the place to look into something like this. Wanna add a comment there if you agree, @gmmorris ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmuellr @gmmorris let me know if what I added looks good: b3f1282

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that looks good!

One little thing we could do is wrap all the handlers in a single error handling function that calls the handler in a try catch and that way the special casing would only appear in one place:

try {
 ... call handler
      } catch (e) {
        if (isErrorThatHandlesItsOwnResponse(e)) {
          return e.sendResponse(res);
        }
        throw e;
      }

Like the Boom errors hadling.

But it's no biggie :)

* 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]>
@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@mikecote mikecote marked this pull request as ready for review March 19, 2020 14:29
@mikecote mikecote requested review from a team as code owners March 19, 2020 14:29
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, left some nit comments

Kinda sad, will now have to run with a trial license to use slack/etc actions :-( hehe

expect(() =>
setup.registerType({
...sampleActionType,
minimumLicenseRequired: 'foo' as any,
Copy link
Member

Choose a reason for hiding this comment

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

nit: is the as any required here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is to ensure the license is a valid LicenseType. A scenario where TS might not catch this would be a call from a JS file that passes a string containing an invalid license. I had to use the any to simulate that and make TS happy.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@alexfrancoeur
Copy link

Checked out this PR earlier today and just wanted to say that it's looking amazing. Great work @mikecote!

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

@mikecote mikecote merged commit 851b8a8 into master Mar 20, 2020
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]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
* master: (52 commits)
  [SIEM] Fix types in rules tests (elastic#60736)
  [Alerting] prevent flickering when fields are updated in an alert (elastic#60666)
  License checks for actions plugin (elastic#59070)
  Implemented ability to clear and properly validate alert interval (elastic#60571)
  WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568)
  [Maps] Update layer dependencies to NP (elastic#59585)
  [Discover] Remove StateManagementConfigProvider (elastic#60221)
  [ML] Listing all categorization wizard checks (elastic#60502)
  [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887)
  [SIEM] Export timeline (elastic#58368)
  [SIEM] Add support for actions and throttle in Rules (elastic#59641)
  Fix ace a11y listener (elastic#60639)
  Add addInfo toast to core notifications service (elastic#60574)
  fix test description (elastic#60638)
  [SIEM] Cypress screenshots upload to google cloud (elastic#60556)
  [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653)
  [SIEM] Fixes Modification of ML Rules (elastic#60662)
  [SIEM] [Case] Bulk status update, add comment avatar, id => title in breadcrumbs (elastic#60410)
  [Alerting] add functional tests for index threshold alertType (elastic#60597)
  [Ingest]EMT-248: add post action request handler and resources (elastic#60581)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
* master: (55 commits)
  Update dependency @elastic/charts to v18.1.0 (elastic#60578)
  Only set timezone when user setting is a valid timezone (elastic#57850)
  [NP] Remove `ui/agg_types` dependencies and move paginated table to kibana_legacy (elastic#60276)
  [SIEM] Fix types in rules tests (elastic#60736)
  [Alerting] prevent flickering when fields are updated in an alert (elastic#60666)
  License checks for actions plugin (elastic#59070)
  Implemented ability to clear and properly validate alert interval (elastic#60571)
  WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568)
  [Maps] Update layer dependencies to NP (elastic#59585)
  [Discover] Remove StateManagementConfigProvider (elastic#60221)
  [ML] Listing all categorization wizard checks (elastic#60502)
  [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887)
  [SIEM] Export timeline (elastic#58368)
  [SIEM] Add support for actions and throttle in Rules (elastic#59641)
  Fix ace a11y listener (elastic#60639)
  Add addInfo toast to core notifications service (elastic#60574)
  fix test description (elastic#60638)
  [SIEM] Cypress screenshots upload to google cloud (elastic#60556)
  [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653)
  [SIEM] Fixes Modification of ML Rules (elastic#60662)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 20, 2020
* master:
  Only set timezone when user setting is a valid timezone (elastic#57850)
  [NP] Remove `ui/agg_types` dependencies and move paginated table to kibana_legacy (elastic#60276)
  [SIEM] Fix types in rules tests (elastic#60736)
  [Alerting] prevent flickering when fields are updated in an alert (elastic#60666)
  License checks for actions plugin (elastic#59070)
  Implemented ability to clear and properly validate alert interval (elastic#60571)
  WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568)
  [Maps] Update layer dependencies to NP (elastic#59585)
  [Discover] Remove StateManagementConfigProvider (elastic#60221)
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]>
@mikecote mikecote added release_note:breaking release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 15, 2020
@mikecote mikecote deleted the actions/license-checks branch May 10, 2021 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Action registration / framework level license check
8 participants