I am curious what does unsecured vs secured mean? That apiKey is null?

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.

@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.

Will we configure this in cloud deployment templates or hardcode in Kibana?

In the cloud deployment template I'm guessing (hoping)?

Yes, that's the idea, gonna remove the TODO line.

This is addressed by https://github.com/elastic/cloud/pull/109221

Ah, TIL: json5 👍

nit: In kibana_utils plugin there is Defer class, which does the same. Maybe could be re-used instead of creating this.

I suspected that something like this would probably exist already, but I couldn't find it. Thanks!

Two things I would like to suggest here:

1. 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.
2. 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 this EmailService construction code out of the plugin.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() }
  }

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.

@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

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.

@gsoldevila It works also for us. The cases client, which is used in our routes, is initialized on start.

@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.

@gsoldevila nice, LGTM, thx

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,
  };
}

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?

Added a comment about that here: #140743 (comment)

Following the discussion, it seems that there's agreement in queuing the task and executing asynchronously from task manager.

NIT: this example will actually NPE if notifications is not present.

Either emailService?.sendPlainTextEmail or set the dependency are required for the example.

Unnecessary point ihmo ( at least the Please make sure the 'actions' plugin is included in your deployment. part).

In the cloud deployment template I'm guessing (hoping)?

NIT: misleading naming. Rename this file types.ts and the email.ts file (containing the service) email_service.ts.

Why is that extending Record<string, unknown>?

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!

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?)

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.

NIT: can be extracted outside of the map call.

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;

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)

Makes sense, we want the variables to have the right value at all times. I'll clear it to ''

NIT: I would extract that to a const

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

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.

Yes, it is fine!