Verify emails when creating an email connector, even if allowedDomain…

[ersin-erdal]

fixes: #133342

We were skipping email address validation on email connector creation UI.
With this PR, email addresses are always validated agains a basic email address template and against allowedDomains list if it is provided.

[elasticmachine]

Pinging @elastic/response-ops (Team:ResponseOps)

[ymao1]

LGTM! Verified I see validation error when email is invalid but allowedDomains is not set, on all From/To/CC/BCC fields.

[sebelga]

It would be awesome if that validator could be added to the form lib field validators. Do you only need it to validate fields?

We'd expose a new email validator from:
https://github.com/elastic/kibana/blob/main/src/plugins/es_ui_shared/static/forms/helpers/field_validators/index.ts

And your current function would live in:
https://github.com/elastic/kibana/tree/main/src/plugins/es_ui_shared/static/validators/string/index.ts

WDYT?

[ersin-erdal]

Hi @sebelga
AFAIK it's not just for field validation, same function is used in backed too.
But still could be added to the shared lib.

[pmuellr]

It would be awesome if that validator could be added to the form lib field validators. Do you only need it to validate fields?

As Ersin notes, we have some shared logic between client and server, so it could get a little messy, but generally, +1. Note that I'm not sure we use email addresses in any other forms, though it's easy enough to imagine this could be the case - for instance, some auth scheme of a connector that specifically requires an email address as the userid. So, probably not in a big hurry for this.

Can we get an issue opened up for this?

[pmuellr]

This PR does seem to do the correct validation, server side, but the originating issue mentioned client-side checks (red boxes). For example, if I have xpack.actions.email.domain_allowlist: ['elastic.co'], I'm able to enter in bad@x as the from when creating a connector, and I get a toast on saving that says error validating action type config: [from]: not allowed emails: bad@x - but really I should get the red box error highlighting.

Or is that not possible?

I noticed this was true for some of the other cases, like adding an invalid domain on a to in an alert action - save failed, but should have had the red error outline, but it didn't.

[ersin-erdal]

This PR does seem to do the correct validation, server side, but the originating issue mentioned client-side checks (red boxes). For example, if I have xpack.actions.email.domain_allowlist: ['elastic.co'], I'm able to enter in bad@x as the from when creating a connector, and I get a toast on saving that says error validating action type config: [from]: not allowed emails: bad@x - but really I should get the red box error highlighting.

Or is that not possible?

I noticed this was true for some of the other cases, like adding an invalid domain on a to in an alert action - save failed, but should have had the red error outline, but it didn't.

i just checked that and it didn't allow me to enter bad@x when there is allowed_list in kibana.yml. i got Email address bad@x is not allowed.
But when there is no allowed_list, i can enter bad@x but this check done by email-addresses plugin.
Don't know if we can contribute to it. Or we can use our own email validation regex WDYT?

[ersin-erdal]

Or i can simply add the following check but wonder if there is a reason behind email-addresses plugin allows that...

for (const domain of domains) {
  if (!domain.includes('.')) {
    return { address, valid: false, reason: InvalidEmailReason.invalid };
  }
  if (allowedDomains !== null && !allowedDomainsSet.has(domain)) {
    return { address, valid: false, reason: InvalidEmailReason.notAllowed };
  }
}

[sebelga]

As Ersin notes, we have some shared logic between client and server, so it could get a little messy, but generally, +1. Note that I'm not sure we use email addresses in any other forms, though it's easy enough to imagine this could be the case - for instance, some auth scheme of a connector that specifically requires an email address as the userid. So, probably not in a big hurry for this.

Yes no hurry for this, if we have an issue to track it it's enough. I just thought that more folks in the future might need to validate email fields in their form and get the validator for free :) Cheers

[pmuellr]

I think I may have been testing with some bad env (changed the config, server restarted, but didn't reload the browser). I can now see the red error when I used an allow list and enter a valid but not allowed email in the sender.

I tried the other case I mentioned, and can see the read error in the UX, so guessing this was all my fault for not testing this correctly :-(

And I'm thinking bad@x might be considered a valid email address, by 'email-addresses', and I'm fine with that. In theory you could have a domain like that in your hosts file, and it would work - localhost is an example of a dot-free domain, what in theory could work (but of course would not be practical for alerting purposes).

yup, bad@x is a valid email :-)

$ node
Welcome to Node.js v16.14.2.
Type ".help" for more information.
> em = require('email-addresses')
[Function: parse5322] {
  parseOneAddress: [Function: parseOneAddressSimple],
  parseAddressList: [Function: parseAddressListSimple],
  parseFrom: [Function: parseFromSimple],
  parseSender: [Function: parseSenderSimple],
  parseReplyTo: [Function: parseReplyToSimple]
}
> em.parseOneAddress('bad@x')
{
  parts: {
    name: null,
    address: {
      name: 'addr-spec',
      tokens: 'bad@x',
      semantic: 'bad@x',
      children: [Array]
    },
    local: {
      name: 'local-part',
      tokens: 'bad',
      semantic: 'bad',
      children: [Array]
    },
    domain: { name: 'domain', tokens: 'x', semantic: 'x', children: [Array] },
    comments: []
  },
  type: 'mailbox',
  name: null,
  address: 'bad@x',
  local: 'bad',
  domain: 'x',
  comments: '',
  groupName: null
}

[pmuellr]

I think I was testing it wrong before - appears to work correctly now ...

LGTM!

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: c61d002
• Storybooks Preview
• Webpack Bundle Analyzer

Failed CI Steps

• FTR Configs #32

Test Failures

• [job] [logs] FTR Configs #32 / apis Osquery Endpoints Packs create route should return 200 and multi line query, but single line query in packs config

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
actions	15.0KB	15.0KB	+1.0B

History

• 💔 Build #53586 failed 897e483
• 💚 Build #52341 succeeded ec00164
• 💚 Build #51700 succeeded b1baf09
• 💚 Build #49799 succeeded fef13e1

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