[Alerting] refactor action whitelisting functions

[pmuellr]

resolves #64659

In this PR, the action whitelisting utilities have been refactored to allow
them to (eventually) be used in plugins other than the actions plugin. Prior
to this, actions required deeper integration with the internal of the
actions plugin.

This also adds some generic typing to the actionType config, secrets, and
params properties, since they are now referred to in multiple places within
the actionType.

• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[pmuellr]

I have a first pass of the refactoring of the action whitelist validation in commit caeca99c82eed90beb53126c476d93f7fcd96c36 - not finished yet, but the email action has been converted to use new validationService to do the whitelisting, instead of the old way we used to do it. The other actions will also need to be changed, but figured I'd just do that one first, make sure this is headed in the right direction.

I ended up making the config, secrets and params types as generic params on action type, which should help to prevent folks from creating invalid validators/schemas due to the previous untyped access for those. Especially since I've added new runtimeValidate: { config, secrets, params } functions to ActionType, which are purpose-built for whitelisting, or any other validation needs we can provide to action authors in the future.

Why validate.config AND runtimeValidate.config (and secrets, and params)? The validate properties still expect an object which has a validate() function, which config-schema provides. So you can continue to just pass a config-schema Schema object to those. Very simple. The new runtimeValidate properties are a little different in that they expect the typed config/secrets/params object, and are also passed a validationService object, which today just contains some whitelisting utilities. One thing that is pretty nice is that instead of adding a validate option to your top-level schema (if you need one), you can move that to runtimeValidate instead, and get the signature typed. The config-schema validate() function also expects a validated object as a param, but it's impossible to get the typing on that AFAIK because of circular references. That doesn't happen with runtimeValidate.

The generic typing of the config, secrets, and params is opt-in at present, as it defaults to <any, any, any>. If we want to be stricter by changing that to either void or maybe never, I think we'll need to change all the internal code to type internal uses as <any, any, any>. There's little value in doing that, as the types don't have any value for the internal code - it's really just useful for action authors to make sure they've defined their actionTypes correctly. We can look into that later ...

[elasticmachine]

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

[gmmorris]

I like this approach, I think it's much cleaner and more extensible.
It's a nit but I don't like the name runtimeValidate as "runtime" means lots of different things to different people. Perhaps call it executionValidate as it validates before calling the actionExecutor?

Another thought I have is that we now have params and config under both validation and runtimeValidation- do we expect them to ever be different between those two types of validation? Perhaps we can just have params and config under runtimeValidation as it seems we're calling the runtimeValidation during setup as any? 🤔

[pmuellr]

I'm not in love with the name runtimeValidate either. I considered using config in it somewhere, as the extra parameter we send is (whitelisting utiltities) is all config based, but ... who knows, we may have some other "dynamic" thing later we want in here, that isn't config based.

executionValidate isn't quite right either, as we do call this when creating/updating actions as well as executing them.

The way these properties are paralleled looks a bit off to me, but I think it makes for a pretty nice story in the end, for action implementors. Basically, the validate properties can be config-schema objects, and the runtimeValidateproperties are functions that take the TS type of the config-schema as a parameter. So, there's no real need to add a top-level validate() function on your schema (as we used to), since the body of that can go in runtimeValidate(). The odd thing about the schema-config validate() is that even though you are getting a "validated" object which should match the TS type, it can't be declared that way because it creates a circular reference, eg

kibana/x-pack/plugins/actions/server/builtin_action_types/email.ts

Lines 98 to 100 in 7707cbd

	function validateParams(paramsObject: unknown): string | void {
	// avoids circular reference ...
	const params = paramsObject as ActionParamsType;

We can't remove params and config from validate and only have them in runtimeValidate, because we need validate to do the basic structure checking. runtimeValidate assumes it's parameter is an already validated object.

I tried a few things to see if we could keep one structure (instead of having two), and somehow provide an augmentation of a schema-config object as a parameter instead of schema-config itself, and then know we can do special processing on it (the runtimeValidate processing). I may take another go at that, but I kept having to resort to instanceof sniffing, which never makes me very happy.

I was also considering adding new schema for the whitelisting support - you'd do something like schema.whitelistedURL() instead of schema.string() and a whitelisting validation somewhere else. Not sure how that would work tho - somehow the validation for that property would need access to the whitelisting utilities, based off the current config. And it would be mucking with the innards of schema-object, which sounds like it could be fragile.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 8136c91

History

• 💚 Build #47376 succeeded caeca99c82eed90beb53126c476d93f7fcd96c36

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

[pmuellr]

Since we decided to leave the case-related actions in the actions plugin, we don't have a burning need for this function right now. Also, the shape of the action type with two validation functions is still a little smelly to me.

So, closing this one for now. We'll be back!