Fix email validation, add tests

[mauritsvanrees]

This fixes most of the open issues:

• Package has no tests #5
• Email regex does not allow apostrophes #6
• The EmailField does not allow umlaut-domains (or domains with other characters) #9
• Email regex does not allow ampersand (&) in the username #15
• More problems with email validation #30

It goes away from using a regular expression, and instead does basic string tests. The bad thing is that this accepts more invalid email addresses than before. The good thing is that it accepts more valid email addresses than before.

Alternatively we could reuse some code from Products.CMFPlone.PloneTool.py:validateSingleEmailAddress (we would need to copy it), but I fear this may have the same kinds of problems.

An open question could be whether we would want to allow ways of specifying multiple email addresses on one line, and possibly values like Arthur Dent <arthur@example.org>, which for example plone/Products.CMFPlone#4104 is inadvertently heading to for the email field in a group.

Also, we could decide that this code in plone.schema is the canonical way for other Plone code to check an email address: it is pulled in by plone.base, so it seems a fine spot. We should rename the function to is_email or similar then, without an underscore.

But for now I think this PR solves problems. What do you think?

[mister-roboto]

@mauritsvanrees thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[mauritsvanrees]

@jenkins-plone-org please run jobs

[ale-rt]

I super approve the approach.

I would even go a step further.

Currently we have email validation done differently in several places, e.g.:

• http://github.com/plone/Products.CMFPlone/blob/1a36ac90d85543feed261a41732d744dbc001bba/Products/CMFPlone/RegistrationTool.py#L175-L184

but if you look there are more.

IMO we should pick an already maintained library that does that, see e.g.:

• https://pypi.org/search/?q=email+validator

I would love to see the same library adopted by the zopefoundation packages as well.

Anyway please see the issue I spotted in the review.

[ale-rt]

LGTM, and it indeed fixes all the errors reported.
No doubt this should be merged and used ASAP.

As a reference, here we have the method from z3c.schema:

• https://github.com/zopefoundation/z3c.schema/blob/4f618ed6957f1853c3f237d161e06c1b9a67bdd5/src/z3c/schema/email/field.py#L28

It would be great to have just one of them around.

[petschki]

+1

[thet]

I have suggestions regarding domains without a TLD part...

I did not test them though!

[thet]

@mauritsvanrees Referring to my previous review:

While I think it would be a nice nerdy feature to address local domains / domains without a TLD part (especially in intranets - however we're not using this kind of adresses anywhere) and while this is are valid email addresses, ICANN discourages that from being used.

See: https://en.wikipedia.org/wiki/Email_address#Valid_email_addresses
And the referenced comment: https://www.icann.org/en/announcements/details/new-gtld-dotless-domain-names-prohibited-30-8-2013-en

I'm not sure about this.
Please dismiss my review, if you think it's better to disallow domains without a TLD.

I think we should disallow domains without a TLD. Most likely if the TLD is missing, this is user error. After your review I briefly thought of specifically allowing anything@localhost, but that may just open up a small DOS attack, sending mail to the local machine over and over again.