Skip to content
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

Email validation #70

Closed
egorsmkv opened this issue Oct 20, 2018 · 4 comments
Closed

Email validation #70

egorsmkv opened this issue Oct 20, 2018 · 4 comments

Comments

@egorsmkv
Copy link

egorsmkv commented Oct 20, 2018

The function that validates emails, in my opinion, is poor:

https://github.com/keratin/authn-server/blob/master/services/util.go#L8-L14

There are some packages which validate emails: https://github.com/search?l=Go&q=email+validator&type=Repositories

We need to find a solution that does it better. I think, the authn-server must has a few options like validate by regexp, smtp checking, dns checking, or at all without email validation step.

Email validation is not simple because of the RFC.

@cainlevy
Copy link
Member

What problem do you hope to solve? Is there an email address that is not allowed by the validation, or are you concerned that it will allow too many false addresses?

If you have a real-world email address that is rejected by AuthN, then that's a bug and needs to be fixed.

If you have false addresses that are being allowed by AuthN, then I think it's out of scope for a validation routine. There's no substitute for asking a user to verify their email by clicking a link.

@egorsmkv
Copy link
Author

egorsmkv commented Oct 23, 2018

There are two sections of the RFC document which are representing the format of email addresses:

For example, these two addresses were rejected by AuthN:

  • "John..Doe"@example.com
  • admin@mailserver1

But they are valid.

@cainlevy
Copy link
Member

Yeah, I'm aware that the regular expression is not a complete implementation of the RFC. The problem is that "valid email address" is defined by:

  • RFCs 5322 & 1035 (email addresses), 5321 & 821 (SMTP)
  • real-world implementation details
  • ICANN's management of the DNS root zone

What I think matters most is:

  1. Accepting the vast majority of real-world email addresses (99%+)
  2. Providing a minimum amount of feedback for clients that rely on server validations
  3. Maintaining performance (the problem is not worth a denial of service vector)
  4. Relying on traditional email verification to ensure a legitimate address (out of scope)

Given that, I'd be happy to merge a PR that allows the validation to accept more real-world email addresses without adding significant complexity. 👍

@cainlevy
Copy link
Member

Happy to consider specific improvements to the email regex, but I can't see synchronous validation ever replacing traditional email verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants