-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add notifications plugin, offering basic email service #143303
Add notifications plugin, offering basic email service #143303
Conversation
…into kbn-140743-notifications-api-mvp
…into kbn-140743-notifications-api-mvp
…/unsecured-client
…into kbn-140743-notifications-api-mvp
actionsToExectute: ExecuteOptions[] | ||
) => Promise<T>; | ||
|
||
export function createBulkUnsecuredExecutionEnqueuerFunction({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious what does unsecured
vs secured
mean? That apiKey is null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is to highlight the fact that this API does not require any form of authentication whatsoever (apiKey or request object), so it should be used cautiously.
This is part of the changes that response-ops are performing to support the case assignment email notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dosant Normally action execution enqueuing is secured via Kibana RBAC where the actions client checks that the user has access to the Actions and Connectors
Kibana feature using the request object before allowing execution enqueuing. For this use case, the system will be scheduling the action so there is no user involved in the request.
x-pack/plugins/actions/server/create_unsecured_execute_function.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/actions/server/create_unsecured_execute_function.ts
Outdated
Show resolved
Hide resolved
} | ||
|
||
public setup(core: CoreSetup, plugins: NotificationsPluginSetupDeps) { | ||
// TODO this must be defaulted to 'elastic-cloud-email' for cloud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we configure this in cloud deployment templates or hardcode in Kibana?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the cloud deployment template I'm guessing (hoping)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the idea, gonna remove the TODO line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is addressed by https://github.com/elastic/cloud/pull/109221
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good, LGTM overall! My only "big" question (aside from the optional nits below): Is it possible to execute the action (send email) without creating any saved objects and enqueueing them in the Task Manager? (Using the execute
method.)
.../plugins/actions/server/unsecured_actions_client/unsecured_actions_client_access_registry.ts
Outdated
Show resolved
Hide resolved
|
||
1. Make sure `notifications` is in your `optionalPlugins` in the `kibana.json` file: | ||
|
||
```json5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, TIL: json5
👍
export type ResolvablePromise<T> = Promise<T> & { | ||
doResolve: (value: unknown) => void; | ||
doReject: (reason?: any) => void; | ||
}; | ||
|
||
export function getResolvablePromise<T>(): ResolvablePromise<T> { | ||
const resolvablePromise: Partial<ResolvablePromise<T>> = {}; | ||
|
||
Object.assign( | ||
resolvablePromise, | ||
new Promise((resolve, reject) => { | ||
resolvablePromise.doResolve = resolve; | ||
resolvablePromise.doReject = reject; | ||
}) | ||
); | ||
|
||
return resolvablePromise as ResolvablePromise<T>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: In kibana_utils
plugin there is Defer
class, which does the same. Maybe could be re-used instead of creating this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspected that something like this would probably exist already, but I couldn't find it. Thanks!
private email: ResolvablePromise<EmailService>; | ||
private emailConnector: string; | ||
|
||
constructor(initializerContext: PluginInitializerContext<NotificationsConfigType>) { | ||
this.logger = initializerContext.logger.get(); | ||
this.email = getResolvablePromise(); | ||
this.initialConfig = initializerContext.config.get(); | ||
} | ||
|
||
public setup(core: CoreSetup, plugins: NotificationsPluginSetupDeps) { | ||
// TODO this must be defaulted to 'elastic-cloud-email' for cloud | ||
if (!plugins.actions) { | ||
this.email.doReject(`Error starting notification services: 'actions' plugin not available.`); | ||
return { email: this.email }; | ||
} | ||
|
||
const emailConnector = this.initialConfig.connectors?.default?.email; | ||
if (!emailConnector) { | ||
this.email.doReject('Error starting notification services: Email connector not specified'); | ||
return { email: this.email }; | ||
} | ||
|
||
if (!plugins.actions.isPreconfiguredConnector(emailConnector)) { | ||
this.email.doReject( | ||
`Error starting notification services: Unexisting email connector '${emailConnector}' specified` | ||
); | ||
return { email: this.email }; | ||
} | ||
|
||
plugins.actions.registerUnsecuredActionsClientAccess(PLUGIN_ID); | ||
|
||
this.emailConnector = emailConnector; | ||
|
||
return { | ||
email: this.email, | ||
}; | ||
} | ||
|
||
public start(core: CoreStart, plugins: NotificationsPluginStartDeps) { | ||
if (this.emailConnector) { | ||
plugins.actions.getUnsecuredActionsClient().then( | ||
(actionsClient) => { | ||
const email = new EmailService(PLUGIN_ID, this.emailConnector, actionsClient); | ||
this.email.doResolve(email); | ||
}, | ||
(error) => { | ||
this.logger.warn(`Error starting notification services: ${error}`); | ||
this.email.doReject(error); | ||
} | ||
); | ||
} | ||
|
||
return { | ||
email: this.email, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things I would like to suggest here:
- From what I understand the email service is async only to handle the case of connectors not being available and to be able to store a specific error message in that promise. I'm not sure that is a good reason to make the service async, would be still nice to keep it sync.
- All the business logic here (about 50 lines of code) is specific to the
EmailService
, there will be other services here in the future, to make it self contained and future proof, would be nice to remove thisEmailService
construction code out of theplugin.ts
file.
Something like:
private email?: EmailService;
public setup(core: CoreSetup, plugins: NotificationsPluginSetupDeps) {
this.email = new EmailService(core, plugins.actions);
return { email: this.email }
}
public start() {
this.email._boot();
return { email: this.email! };
}
where:
class EmailService {
public error?: Error;
constructor (protected readonly core, protected readonly actions) {}
public _boot(): void {
this.core.getStartServices().then(() => {
// ...
});
}
private assertIsReady() {
// ...
}
public async sendPlainTextEmail(options) {
await this.assertIsReady();
// send email ...
}
}
Another option could be to create a "provider" for the email service, where in the future we could use it to setup a non-connector based email sender:
private emailProvider?: EmailServiceProvider;
public setup(core: CoreSetup, plugins: NotificationsPluginSetupDeps) {
this.emailProvider = new EmailServiceProvider();
if (core.email) {
this.emailProvider.setImplementation(new CoreEmailService(core));
} else if (plugins.cloud) {
this.emailProvider.setImplementation(new CloudEmailService(core, plugins.cloud));
} else if (plugins.actions) {
this.emailProvider.setImplementation(new AlertingEmailService(core, plugins.actions));
}
return { email: () => this.emailProvider.create() }
}
public start() {
this.emailProvider._boot();
return { email: () => this.emailProvider.create() }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions!
(1) The email service is actually async
because the Actions plugin's getUnsecuredActionsClient()
method is also async, so I cannot instantiate the EmailService
until I have an instance of the ActionsClient
. That being said, looking at the Actions plugin code, I don't see a strong reason why this method is async, so perhaps we can simply make it synchronous. @ymao1 WDYT?
Another reason is, if we want to expose it on the setup()
contract, we have to await for Actions plugin start()
contract in order to get the ActionsClient
, but again, I don't know if there's a good reason not to expose the getUnsecuredActionsClient()
in the setup()
...
(2) Good point, I'll create a factory and extract this logic there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsoldevila I can make getUnsecuredActionsClient()
synchronous, you are correct that there doesn't seem to be a reason for it to be asynchronous, however, it uses core.savedObjects.createInternalRepository
which is only available on SavedObjectsServiceStart
, not on SavedObjectsServiceSetup
, so I need to keep that function inside the start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ymao1. I don't see anything wrong with having the EmailService available only at start
time (and not on setup
).
@cnasikas if this works for you, I'll remove it from the setup
contract, and have it on the start contract as a property (rather than a Promise). We can always start like this, and then add it as a Promise on setup
if the need arises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsoldevila It works also for us. The cases client, which is used in our routes, is initialized on start
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadimkibana
(1) it will no longer be a Promise, although it will be available in the start
contract only.
(2) I created a factory to instantiate the EmailService and extracted the initialization logic + error handling there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsoldevila nice, LGTM, thx
// TODO this must be defaulted to 'elastic-cloud-email' for cloud | ||
if (!plugins.actions) { | ||
this.email.doReject(`Error starting notification services: 'actions' plugin not available.`); | ||
return { email: this.email }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Early returns is a clean and nice pattern:
if (something) {
return early;
}
return late;
But here it is a bit risky to use, especially as plugin classes are usually not tested. The reason being that once this plugin has more services, it will not be possible to use early returns in this manner and it will need to be refactored:
setup() {
if (!plugins.actions) {
this.email.doReject(`...`);
return { email: this.email }; // BUG: "sms" service is missing.
}
// ... more code ...
return {
email: this.email,
sms: this.sms,
};
}
to, | ||
}, | ||
})); | ||
return await this.actionsClient.bulkEnqueueExecution(this.requesterId, actions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we will execute the email action immediately, using the execute()
method, (I hope that does not create a saved object), instead of "enqueueing", which creates a saved object.
Do we gain something by enqueueing an action?
return await this.actionsClient.bulkEnqueueExecution(this.requesterId, actions); | |
return await this.actionsClient.bulkExecute(this.requesterId, actions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment about that here: #140743 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the discussion, it seems that there's agreement in queuing the task and executing asynchronously from task manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not look at the changes of the actions
plugin.
const emailService = await notifications?.email; | ||
emailService.sendPlainTextEmail({ | ||
to: 'foo@bar.com', | ||
subject: 'Some subject', | ||
message: 'Hello world!', | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: this example will actually NPE if notifications
is not present.
Either emailService?.sendPlainTextEmail
or set the dependency are required for the example.
|
||
### Requirements | ||
|
||
- This plugin currently depends on the `'actions'` plugin, as it uses `Connectors` under the hood. Please make sure the `'actions'` plugin is included in your deployment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary point ihmo ( at least the Please make sure the 'actions' plugin is included in your deployment.
part).
} | ||
|
||
public setup(core: CoreSetup, plugins: NotificationsPluginSetupDeps) { | ||
// TODO this must be defaulted to 'elastic-cloud-email' for cloud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the cloud deployment template I'm guessing (hoping)?
* 2.0. | ||
*/ | ||
|
||
export interface IEmailService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: misleading naming. Rename this file types.ts
and the email.ts
file (containing the service) email_service.ts
.
sendPlainTextEmail(payload: PlainTextEmail): Promise<void>; | ||
} | ||
|
||
export interface PlainTextEmail extends Record<string, unknown> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that extending Record<string, unknown>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make much sense, and I don't remember why I added it on the first place.
I will remove it, thanks for spotting it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseOps code LGTM. I created a PR on top of this PR to test sending an email when assigning a user to a case and is working as expected 🚀 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
export const config: PluginConfigDescriptor<ConnectorsEmailConfigType> = { | ||
schema: configSchema, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: this descriptor declaration is duplicated in x-pack/plugins/notifications/server/config/config.ts
.
(Also NIT/optional: ihmo these two files could be merged, not sure to see the value separating them?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to have a "connectors specific part" so that we can more easily get rid of it if we ever change the underlying implementation. But I think it makes things unnecessarily complex. I'll merge both.
...(params.context?.relatedObjects?.length && { | ||
relatedSavedObjects: params.context!.relatedObjects!.map( | ||
({ id, type, spaceId: namespace }) => ({ id, type, namespace }) | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: can be extracted outside of the map
call.
return { | ||
isEmailServiceAvailable: () => !!email, | ||
getEmailService: () => { | ||
if (email) return email; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT/optional: code style consistency
if (email) {
return email;
}
Even more NIT and optional: following the 'fail fast' logic, I would personally have inverted the statements
if(!email) {
throw ...;
}
return email;
); | ||
} | ||
|
||
this.setupSuccessful = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: no real impact, but I would have cleared this.setupError
when we get there (but that implies to make that property optional and force cast !
when throwing the error in start
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, we want the variables to have the right value at all times. I'll clear it to ''
email = new LicensedEmailService( | ||
new ConnectorsEmailService(PLUGIN_ID, emailConnector, unsecuredActionsClient), | ||
licensing.license$, | ||
'platinum', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I would extract that to a const
return { | ||
id, | ||
type, | ||
namespace: spaceId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While spaceId === namespace
in most cases, it does not directly map to namespace
in the case of the default space. For the default space, spaceId = default
while namespace is undefined
. The spaces service has a spaceIdToNamespace
helper for the conversion, or you could update the typing to accept namespace and pass it through
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I wasn't aware of that! I thought it was a simple naming discrepancy. I can switch back to 'namespace' or use the converter. @cnasikas if that's fine with you I'll update the type and use namespace
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Public APIs missing exports
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
## Summary The PR adds the ability to notify users by email when assigned to a case. A user is: - Not notified if he/she assigns themselves - Notified if added as an assignee to a case - Not notified if removed from a case I did not add integration tests due to the complexity of simulating an email server. I added unit test coverage. If integration test coverage is needed we can add the tests on another PR. Depends on: #143303 Fixes: #142307 ## Email screenshot <img width="361" alt="Screenshot 2022-11-07 at 1 27 13 PM" src="https://user-images.githubusercontent.com/7871006/200299356-52c08515-4d43-49d6-bd47-3797b52f97e5.png"> @shanisagiv1 @lcawl What do you think about the content of the email (see screenshot)? ## Testing 1. Put the following in your `kibana.yml`: ``` notifications.connectors.default.email: 'mail-dev' xpack.actions.preconfigured: mail-dev: name: preconfigured-email-notification-maildev actionTypeId: .email config: service: other from: mlr-test-sink@elastic.co host: localhost port: 1025 secure: false hasAuth: false ``` 2. Install [`maildev`](https://www.npmjs.com/package/maildev): `npm install -g maildev` 3. Run `maildev`: `maildev` 4. Open MailDev's web interface (http://0.0.0.0:1080/) 5. Create a case and assign users. You should see the emails in the MailDev inbox. 6. Update the assignees of a case. You should see the emails in the MailDev inbox. Note: If you assign yourself you should not see an email. If you delete an assignee you should not see an email. ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) ## Release notes Notify users by email when assigned to a case
The goal of this PR is to implement a new
'notifications'
plugin that will expose a service to send simple, plain text, email notifications, using the'actions'
plugin Email connector under the hood.Addresses #140743