[Alerts][License] Add license checks to alerts HTTP APIs and execution by YulNaumenko · Pull Request #85223 · elastic/kibana

YulNaumenko · 2020-12-08T06:51:04Z

Was added 403 Forbidder response for executing alert API (create, update, enable, disable, muteAll, unmuteAll, muteInstance, unmuteInstance) with the higher level alert type license.
Delete API remain available to remove higher license alerts.
When the task is running for a scheduled alert with the higher level license, the event log will provide the next execution status:

executionStatus: {lastExecutionDate: "2020-12-09T06:30:15.749Z", status: "error", error: {reason: "license",…}}
error: {reason: "license",…}
message: "Alert type .geo-threshold is disabled because your basic license does not support it. Please upgrade your license."
reason: "license"
lastExecutionDate: "2020-12-09T06:30:15.749Z"
status: "error"

elasticmachine · 2020-12-08T20:55:30Z

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

gmmorris · 2020-12-09T11:44:41Z

x-pack/plugins/alerts/server/alert_type_registry.ts

      },
    });
+    // No need to notify usage on basic alert types
+    if (alertType.minimumLicenseRequired !== 'basic') {


nit.
Is there an enum we can use to reference the basic string?
I'm always worried when we hard code values like this.

gmmorris · 2020-12-09T11:47:24Z

x-pack/plugins/alerts/server/alert_type_registry.ts

+            this.licenseState.isLicenseValidForAlertType(id, name, minimumLicenseRequired)
+              .isValid === true,


nit.
Since this is a boolean, the === seems redundant. :)

Suggested change

this.licenseState.isLicenseValidForAlertType(id, name, minimumLicenseRequired)

.isValid === true,

this.licenseState.isLicenseValidForAlertType(id, name, minimumLicenseRequired)

.isValid,

gmmorris · 2020-12-09T11:50:51Z

x-pack/plugins/alerts/server/lib/license_state.ts

+    this._notifyUsage = notifyUsage;
+  }
+
+  public isLicenseValidForAlertType(


nit.
Generally speaking I'd expecta method that starts with is... to return a boolean, while here we return a complex type.
It's no biggie, but I know this is a common pattern so I thought it's worth noting.

gmmorris · 2020-12-09T11:52:17Z

x-pack/plugins/alerts/server/lib/license_state.ts

+    if (notifyUsage) {
+      this.notifyUsage(alertTypeName, minimumLicenseRequired);
+    }


I definitely wouldn't expect a method that starts with is... to have a side effect like this.
Should the notification be extracted from here and have it's own entry point?

Renamed to getLicenseCheckForAlertType

gmmorris

I still need to do some manual testing, but wanted to post my notes so far so you can start looking into them.

Some concerns around missing tests, and code flow, but nothing major so far :)

gmmorris · 2020-12-09T11:55:17Z

x-pack/plugins/alerts/server/lib/license_state.ts

+    if (check.isValid) {
+      return;
+    }
+    switch (check.reason) {
+      case 'unavailable':
+        throw new AlertTypeDisabledError(
+          i18n.translate('xpack.alerts.serverSideErrors.unavailableLicenseErrorMessage', {
+            defaultMessage:
+              'Alert type {alertTypeId} is disabled because license information is not available at this time.',
+            values: {
+              alertTypeId: alertType.id,
+            },
+          }),
+          'license_unavailable'
+        );
+      case 'expired':
+        throw new AlertTypeDisabledError(
+          i18n.translate('xpack.alerts.serverSideErrors.expirerdLicenseErrorMessage', {
+            defaultMessage:
+              'Alert type {alertTypeId} is disabled because your {licenseType} license has expired.',
+            values: { alertTypeId: alertType.id, licenseType: this.license!.type },
+          }),
+          'license_expired'
+        );
+      case 'invalid':
+        throw new AlertTypeDisabledError(
+          i18n.translate('xpack.alerts.serverSideErrors.invalidLicenseErrorMessage', {
+            defaultMessage:
+              'Alert type {alertTypeId} is disabled because your {licenseType} license does not support it. Please upgrade your license.',
+            values: { alertTypeId: alertType.id, licenseType: this.license!.type },
+          }),
+          'license_invalid'
+        );
+      default:
+        assertNever(check.reason);
+    }
+  }


nit.
early returns can be error prone because developers often don't expect them.
It means there's an implicit assumption that if isValid is false, then the switch will not be called- this can easily be missed with an early return, so I've moved it into the if.

Suggested change

if (check.isValid) {

return;

}

switch (check.reason) {

case 'unavailable':

throw new AlertTypeDisabledError(

i18n.translate('xpack.alerts.serverSideErrors.unavailableLicenseErrorMessage', {

defaultMessage:

'Alert type {alertTypeId} is disabled because license information is not available at this time.',

values: {

alertTypeId: alertType.id,

},

}),

'license_unavailable'

);

case 'expired':

throw new AlertTypeDisabledError(

i18n.translate('xpack.alerts.serverSideErrors.expirerdLicenseErrorMessage', {

defaultMessage:

'Alert type {alertTypeId} is disabled because your {licenseType} license has expired.',

values: { alertTypeId: alertType.id, licenseType: this.license!.type },

}),

'license_expired'

);

case 'invalid':

throw new AlertTypeDisabledError(

i18n.translate('xpack.alerts.serverSideErrors.invalidLicenseErrorMessage', {

defaultMessage:

'Alert type {alertTypeId} is disabled because your {licenseType} license does not support it. Please upgrade your license.',

values: { alertTypeId: alertType.id, licenseType: this.license!.type },

}),

'license_invalid'

);

default:

assertNever(check.reason);

}

}

if (!check.isValid) {

switch (check.reason) {

case 'unavailable':

throw new AlertTypeDisabledError(

i18n.translate('xpack.alerts.serverSideErrors.unavailableLicenseErrorMessage', {

defaultMessage:

'Alert type {alertTypeId} is disabled because license information is not available at this time.',

values: {

alertTypeId: alertType.id,

},

}),

'license_unavailable'

);

case 'expired':

throw new AlertTypeDisabledError(

i18n.translate('xpack.alerts.serverSideErrors.expirerdLicenseErrorMessage', {

defaultMessage:

'Alert type {alertTypeId} is disabled because your {licenseType} license has expired.',

values: { alertTypeId: alertType.id, licenseType: this.license!.type },

}),

'license_expired'

);

case 'invalid':

throw new AlertTypeDisabledError(

i18n.translate('xpack.alerts.serverSideErrors.invalidLicenseErrorMessage', {

defaultMessage:

'Alert type {alertTypeId} is disabled because your {licenseType} license does not support it. Please upgrade your license.',

values: { alertTypeId: alertType.id, licenseType: this.license!.type },

}),

'license_invalid'

);

default:

assertNever(check.reason);

}

}

}

The styleguide recommends returning early from functions.

According to the styleguide will keep it as it is.

gmmorris · 2020-12-09T11:55:35Z

x-pack/plugins/alerts/server/lib/license_state.ts

    }
  }
+
+  public ensureLicenseForAlertType(alertType: AlertType) {


This has no unit tests - can add some?

gmmorris · 2020-12-09T11:59:56Z

x-pack/test/alerting_api_integration/basic/tests/alerts/gold_noop_alert_type.ts

+          statusCode: 403,
+          error: 'Forbidden',
+          message:
+            'Action type .email is disabled because your basic license does not support it. Please upgrade your license.',


I'm confused by this test- it says it's testing a Gold AlertType, but the error is about a Gold Action type.
Feels wrong, unless I'm missing something 🤔

Ops, copy paste issue :-)

mikecote

Looking good 👍 there's a few changes I'd like to re-review once complete but this is looking great!

mikecote · 2020-12-09T13:22:30Z

x-pack/plugins/alerts/server/routes/list_alert_types.test.ts

        producer: 'test',
-      },
+        enabledInLicense: true,
+      } as RegistryAlertTypeWithAuth,


In this file, it's better to do things like const listTypes: RegistryAlertTypeWithAuth[] in the above instead. Otherwise it's not fully typed to RegistryAlertTypeWithAuth from what I can see. I was able to add extra properties to the object without typecheck failing.

mikecote · 2020-12-09T13:30:35Z

x-pack/plugins/alerts/server/alerts_client/alerts_client.ts

+    this.alertTypeRegistry.ensureAlertTypeEnabled(data.alertTypeId);
+
    // Throws an error if alert type isn't registered
    const alertType = this.alertTypeRegistry.get(data.alertTypeId);


Since ensureAlertTypeEnabled calls get internally, we can remove the .get call while keeping the comment.

Suggested change

this.alertTypeRegistry.ensureAlertTypeEnabled(data.alertTypeId);

// Throws an error if alert type isn't registered

const alertType = this.alertTypeRegistry.get(data.alertTypeId);

// Throws an error if alert type isn't registered

this.alertTypeRegistry.ensureAlertTypeEnabled(data.alertTypeId);

But alertType is used in the code below in the create method..

Oh, I see 🙈 all good then.

mikecote · 2020-12-09T13:31:33Z

x-pack/plugins/alerts/server/alerts_client/alerts_client.ts

      alertSavedObject = await this.unsecuredSavedObjectsClient.get<RawAlert>('alert', id);
    }

+    this.alertTypeRegistry.ensureAlertTypeEnabled(alertSavedObject.attributes.alertTypeId);


I think all checks in this file that happen before authorization checks / audit logging should be moved to happen after.

mikecote · 2020-12-09T13:32:54Z

x-pack/plugins/alerts/server/alerts_client/alerts_client.ts

    { id, data }: UpdateOptions,
    { attributes, version }: SavedObject<RawAlert>
  ): Promise<PartialAlert> {
+    this.alertTypeRegistry.ensureAlertTypeEnabled(attributes.alertTypeId);


nit: since updateAlert is only called from updateWithOCC and updateWithOCC already ensures the alert type is enabled, we should be good to remove this.

Suggested change

this.alertTypeRegistry.ensureAlertTypeEnabled(attributes.alertTypeId);

mikecote · 2020-12-09T13:52:26Z

x-pack/test/alerting_api_integration/basic/tests/alerts/basic_noop_alert_type.ts

+          getTestAlertData({
+            actions: [
+              {
+                id: createdAction.id,
+                group: 'default',
+                params: {},
+              },
+            ],
+          })


nit: I would maybe remove actions from the scenario

Suggested change

getTestAlertData({

actions: [

{

id: createdAction.id,

group: 'default',

params: {},

},

],

})

getTestAlertData()

mikecote · 2020-12-09T13:54:24Z

x-pack/test/alerting_api_integration/basic/tests/alerts/gold_noop_alert_type.ts

+        .post(`/api/alerts/alert`)
+        .set('kbn-xsrf', 'foo')
+        .send(
+          getTestAlertData({


This would need to overwrite the alertTypeId to the one you created (test.gold.noop). This will also give you the error message you're looking for below.

nit: I would also remove the actions from the scenario as the test.* actions are all gold+. Otherwise, it would need to be .server-log or something along those lines.

Suggested change

getTestAlertData({

getTestAlertData({

alertTypeId: 'test.gold.noop'

gmmorris

LGTM, behaves as expected.

We should evaluate the labeling though, as the error on the UI isn't very informative.

It says:

Alert type example.always-firing is disabled because your basic license does not support it. Please upgrade your license.

Which isn't enough as the user has no idea that this alert is still running and erroring for ever.
@YulNaumenko and I have discussed adding documentation that explains the situation, ramifications and perhaps remediation recommendations so users know what to do next.

This will hopefully save us from some SDHs.

YulNaumenko · 2020-12-09T17:48:54Z

@gchaps could you please help with the providing the informative error message, when the license is expired for the remaining alert.
The idea is that the user created alert which alert type requires a Gold license, for example on the stack Trial license.
But after some time the Trial license is expired and Alert Type become disabled due to the license restriction.
The alert keeps running in the background, causing noise and costing task manager cycles. In the UI and in the Event Log the user will get the proper error message: Alert type .geo-threshold is disabled because your basic license does not support it. Please upgrade your license.
The confusing part in this message is the word Disabled. Disabling Alert means it won’t automatically restart when license is upgraded, but the disabling alert type will do the opposite.

mikecote · 2020-12-09T19:38:16Z

x-pack/plugins/alerts/common/index.ts

+export enum LicenseTypeValues {
+  Basic = 'basic',
+  Gold = 'gold',
+  Platinum = 'platinum',
+  Enterprise = 'enterprise',
+  Standard = 'standard',
+  Trial = 'trial',
+}


It seems like a duplication of sources and similar to LICENSE_TYPE in the licensing plugin?

mikecote · 2020-12-09T19:38:52Z

x-pack/plugins/actions/server/action_type_registry.ts


 import Boom from '@hapi/boom';
 import { i18n } from '@kbn/i18n';
+import { LicenseTypeValues } from '../../alerts/common';


To keep the plugins decoupled, this shouldn't be requiring code from alerts.

mikecote · 2020-12-09T19:38:55Z

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

 import type { PublicMethodsOf } from '@kbn/utility-types';
 import { Observable, Subscription } from 'rxjs';
 import { assertNever } from '@kbn/std';
+import { LicenseTypeValues } from '../../../alerts/common';


To keep the plugins decoupled, this shouldn't be requiring code from alerts.

mikecote · 2020-12-09T19:41:31Z

x-pack/plugins/alerts/server/lib/license_state.test.ts

      const licenseState = new LicenseState(licensing.license$);
-      const alertingLicenseInfo = licenseState.checkLicense(getRawLicense());
-      expect(alertingLicenseInfo.enableAppLink).to.be(false);
+      const actionsLicenseInfo = licenseState.checkLicense(getRawLicense());


Looks like there's a few variables in this file referencing actions terminology when it should be alerts.

kibanamachine · 2020-12-09T23:08:30Z

💚 Build Succeeded

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

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [09d437f]

History

💔 Build #93271 failed 8710c2f
💔 Build #93232 failed 395ea84
💔 Build #93003 failed fbe0fbb
💔 Build #92989 failed 85f2c99
💔 Build #92943 failed f6e2157

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

* [Alerts][License] Define minimum license required for each alert type (#84997) * Define minimum license required for each alert type * fixed typechecks * fixed tests * fixed tests * fixed due to comments * fixed due to comments * removed file * removed casting to LicenseType * [Alerts][License] Add license checks to alerts HTTP APIs and execution (#85223) * [Alerts][License] Add license checks to alerts HTTP APIs and execution * fixed typechecks * resolved conflicts * resolved conflicts * added router tests * fixed typechecks * added license check support for alert task running * fixed typechecks * added integration tests * fixed due to comments * fixed due to comments * fixed tests * fixed typechecks * [Alerting UI][License] Disable alert types in UI when the license doesn't support it. (#85496) * [Alerting UI][License] Disable alert types in UI when the license doesn't support it. * fixed typechecks * added licensing for alert list and details page * fixed multy select menu * fixed due to comments * fixed due to comments * fixed due to comments * fixed typechecks * fixed license error message * fixed license error message * fixed typechecks * fixed license error message Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

[Alerts][License] Add license checks to alerts HTTP APIs and execution

fd23a21

YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.11.0 labels Dec 8, 2020

YulNaumenko self-assigned this Dec 8, 2020

YulNaumenko linked an issue Dec 8, 2020 that may be closed by this pull request

[Alerts][License] Add license checks to alerts HTTP APIs and execution #85102

Closed

kindsun mentioned this pull request Dec 8, 2020

Add geo containment and geo threshold to gold license #85222

Closed

YulNaumenko added 2 commits December 8, 2020 12:49

fixed typechecks

a110e39

Merge remote-tracking branch upstream/master

c18264b

YulNaumenko marked this pull request as ready for review December 8, 2020 20:55

YulNaumenko requested a review from a team as a code owner December 8, 2020 20:55

YulNaumenko added 7 commits December 8, 2020 13:03

resolved conflicts

126225e

resolved conflicts

a0731bb

added router tests

f6e2157

fixed typechecks

85f2c99

added license check support for alert task running

70949a8

fixed typechecks

a72560e

added integration tests

fbe0fbb

gmmorris reviewed Dec 9, 2020

View reviewed changes

gmmorris suggested changes Dec 9, 2020

View reviewed changes

mikecote self-requested a review December 9, 2020 12:56

mikecote reviewed Dec 9, 2020

View reviewed changes

gmmorris approved these changes Dec 9, 2020

View reviewed changes

fixed due to comments

395ea84

YulNaumenko requested a review from mikecote December 9, 2020 19:16

mikecote reviewed Dec 9, 2020

View reviewed changes

fixed due to comments

7cf7e61

YulNaumenko requested a review from mikecote December 9, 2020 20:38

YulNaumenko added 3 commits December 9, 2020 12:40

Merge remote-tracking branch upstream/master

8da12ff

fixed tests

8710c2f

fixed typechecks

e0f28d9

YulNaumenko merged commit 93614af into elastic:alerts/license-checks Dec 9, 2020

YulNaumenko mentioned this pull request Dec 10, 2020

License checks for alerts plugin #85649

Merged

mikecote mentioned this pull request Dec 14, 2020

[Alerts][License] Add license checks to alerts HTTP APIs and execution #85102

Closed

		this.licenseState.isLicenseValidForAlertType(id, name, minimumLicenseRequired)
		.isValid === true,

	getTestAlertData({
	getTestAlertData({
	alertTypeId: 'test.gold.noop'

Conversation

YulNaumenko commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmmorris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmmorris left a comment

Choose a reason for hiding this comment

Uh oh!

YulNaumenko commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Dec 9, 2020

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

YulNaumenko commented Dec 8, 2020 •

edited

Loading

YulNaumenko commented Dec 9, 2020 •

edited

Loading