Add license checks to action HTTP APIs #59153

mikecote · 2020-03-03T15:09:10Z

⚠️ This PR merges into a feature branch

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."
}

elasticmachine · 2020-03-03T15:09:12Z

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

pmuellr · 2020-03-05T20:41:17Z

It was discussed further that response status of 403 would make more sense which is included in this PR.

I always worry about 403's, as they are so closely tied to security-related unauthorized scenarios, but of course the HTTP response itself isn't really tied to authn/authz. But, it seems like the best thing for now.

I did notice the execute API always returns status of 200 but will contain the error information within the response. ...

Ya, I think my thinking here was that we wanted to discriminate between errors caused by the action itself, vs errors generated by Kibana. So a "service error" would always return a 200, but indicate the error in the response. I don't think we neccessarily have to do that in this case. In fact, it could be confusing, since it could be implied the 403 came from the service. Seems like we should return a 403 status code from this one, rather than the 200.

You will notice there isn't any functional tests. ...

Ya, ETOOMANYSCENARIOS. For now, I think having a pass case and fail case as FT probably makes sense; one API, fail for basic, pass for gold. And lots of unit tests :-)

mikecote · 2020-03-05T21:08:50Z

Ya, I think my thinking here was that we wanted to discriminate between errors caused by the action itself, vs errors generated by Kibana. So a "service error" would always return a 200, but indicate the error in the response. I don't think we neccessarily have to do that in this case. In fact, it could be confusing, since it could be implied the 403 came from the service. Seems like we should return a 403 status code from this one, rather than the 200.

This makes sense, I went ahead and made the change here: a78c999.

Below is a sample response from the updated API when it returns a 403 status:

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

Ya, ETOOMANYSCENARIOS. For now, I think having a pass case and fail case as FT probably makes sense; one API, fail for basic, pass for gold. And lots of unit tests :-)

I will create a test suite with basic license, make sure the built ins for gold can't be created (copy tests from x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types but assert for 403s).

mikecote · 2020-03-05T21:31:19Z

Ya, ETOOMANYSCENARIOS. For now, I think having a pass case and fail case as FT probably makes sense; one API, fail for basic, pass for gold. And lots of unit tests :-)

I will create a test suite with basic license, make sure the built ins for gold can't be created (copy tests from x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types but assert for 403s).

Test suite has been created here: c5c7f1b.

pmuellr

LGTM

YulNaumenko

LGTM

mikecote · 2020-03-06T13:20:12Z

retest

…to actions/ensure-action-type-function

mikecote · 2020-03-06T19:39:04Z

@YulNaumenko @pmuellr I had to change a bit more code than I expected to make the tests (hopefully) pass. Feel free to take a look at the new commits since your last review.

mikecote · 2020-03-06T22:17:03Z

retest

…to actions/ensure-action-type-function

pmuellr

LGTM, left a few comments

x-pack/plugins/actions/server/routes/create.ts

x-pack/plugins/actions/server/routes/execute.ts

YulNaumenko · 2020-03-09T15:56:47Z

Changes LGTM

…to actions/ensure-action-type-function

mikecote · 2020-03-09T19:19:35Z

CI failure seems unrelated / flaky, going to merge into feature branch regardless to unblock follow up PRs. Will test stability in the feature branch and follow up accordingly if master doesn't have the issue.

kibanamachine · 2020-03-09T20:45:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e443fa0

History

💔 Build #31843 failed d5292e6
💔 Build #31798 failed da91d38
💚 Build #31714 succeeded 5c66bf2
💚 Build #31592 succeeded 4152be8
💛 Build #31552 was flaky 4152be8

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

* 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]>

* 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]>

* 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 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 Mar 3, 2020

mikecote self-assigned this Mar 3, 2020

mikecote mentioned this pull request Mar 5, 2020

License checks for actions plugin #59070

Merged

mikecote force-pushed the actions/ensure-action-type-function branch 2 times, most recently from 92d80dd to a50ae68 Compare March 5, 2020 16:47

Initial work

bf35822

mikecote force-pushed the actions/ensure-action-type-function branch from a50ae68 to bf35822 Compare March 5, 2020 17:14

mikecote added 2 commits March 5, 2020 13:25

Handle errors in update action API

ea73855

Add unit tests for APIs

2088182

mikecote marked this pull request as ready for review March 5, 2020 19:05

mikecote requested a review from a team as a code owner March 5, 2020 19:05

Make action executor throw when action type isn't enabled

a78c999

Add test suite for basic license

c5c7f1b

pmuellr approved these changes Mar 5, 2020

View reviewed changes

YulNaumenko approved these changes Mar 5, 2020

View reviewed changes

Fix ESLint errors

b5e28f8

mikecote added 3 commits March 6, 2020 10:55

Merge branch 'actions/license-checks' of github.com:elastic/kibana in…

c10d719

…to actions/ensure-action-type-function

Fix failing tests

dfc787a

Attempt 1 to fix CI

6b54409

ESLint fixes

4152be8

Merge branch 'actions/license-checks' of github.com:elastic/kibana in…

5c66bf2

…to actions/ensure-action-type-function

pmuellr approved these changes Mar 9, 2020

View reviewed changes

x-pack/plugins/actions/server/routes/create.ts Outdated Show resolved Hide resolved

x-pack/plugins/actions/server/routes/create.ts Outdated Show resolved Hide resolved

x-pack/plugins/actions/server/routes/execute.ts Outdated Show resolved Hide resolved

mikecote added 4 commits March 9, 2020 11:05

Create sendResponse function on ActionTypeDisabledError

9af1336

Make disabled action types by config return 403

4c38702

Remove switch case

33235e1

Fix ESLint

da91d38

mikecote added 2 commits March 9, 2020 13:26

Merge branch 'actions/license-checks' of github.com:elastic/kibana in…

d5292e6

…to actions/ensure-action-type-function

Merge branch 'actions/license-checks' of github.com:elastic/kibana in…

e443fa0

…to actions/ensure-action-type-function

mikecote merged commit 6b7a134 into elastic:actions/license-checks Mar 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add license checks to action HTTP APIs #59153

Add license checks to action HTTP APIs #59153

mikecote commented Mar 3, 2020 •

edited

Loading

elasticmachine commented Mar 3, 2020

pmuellr commented Mar 5, 2020

mikecote commented Mar 5, 2020

mikecote commented Mar 5, 2020

pmuellr left a comment

YulNaumenko left a comment

mikecote commented Mar 6, 2020

mikecote commented Mar 6, 2020

mikecote commented Mar 6, 2020

pmuellr left a comment

YulNaumenko commented Mar 9, 2020

mikecote commented Mar 9, 2020

kibanamachine commented Mar 9, 2020

Add license checks to action HTTP APIs #59153

Add license checks to action HTTP APIs #59153

Conversation

mikecote commented Mar 3, 2020 • edited Loading

⚠️ This PR merges into a feature branch

POST /api/action

PUT /api/action/{id}

POST /api/action/{id}/_execute

elasticmachine commented Mar 3, 2020

pmuellr commented Mar 5, 2020

mikecote commented Mar 5, 2020

mikecote commented Mar 5, 2020

pmuellr left a comment

Choose a reason for hiding this comment

YulNaumenko left a comment

Choose a reason for hiding this comment

mikecote commented Mar 6, 2020

mikecote commented Mar 6, 2020

mikecote commented Mar 6, 2020

pmuellr left a comment

Choose a reason for hiding this comment

YulNaumenko commented Mar 9, 2020

mikecote commented Mar 9, 2020

kibanamachine commented Mar 9, 2020

💚 Build Succeeded

History

mikecote commented Mar 3, 2020 •

edited

Loading