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

Add new email field type #3240

Closed
wants to merge 7 commits into from
Closed

Add new email field type #3240

wants to merge 7 commits into from

Conversation

jordanoverbye
Copy link
Contributor

@jordanoverbye jordanoverbye commented Jul 8, 2020

This PR adds a new field type Email. This is essentially the same as the Text field type, but with some built-in email validation.

Screenshots

Show a warning when creating an item with an invalid email address. The tooltip is native to the browser and happens because we've added type="email to the input.

Screen Shot 2020-07-08 at 11 13 23 am

Show a warning when trying to update an item with an invalid email address.

Screen Shot 2020-07-08 at 11 09 10 am

GraphQL errors

Using this list

keystone.createList('User', {
  fields: {
    email: { type: Email, isRequired: true },
  },
});
mutation {
  createUser(data: { email: "__invalid__email" }) {
    id
    email
  }
}


// Returns

{
  "errors": [
    {
      "message": "You attempted to perform an invalid mutation",
      "name": "ValidationFailureError",
      "time_thrown": "2020-07-09T01:21:24.941Z",
      "data": {
        "messages": [
          "Invalid email"
        ],
        "errors": [
          {
            "value": "__invalid__email"
          }
        ],
        "listKey": "User",
        "operation": "create"
      },
      "path": [
        "createUser"
      ],
      "uid": "ckce3v2ll0001creff2e019iu"
    }
  ],
  "data": {
    "createUser": null
  }
}

@changeset-bot
Copy link

changeset-bot bot commented Jul 8, 2020

🦋 Changeset is good to go

Latest commit: 997542b

We got this.

This PR includes changesets to release 1 package
Name Type
@keystonejs/fields Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

const handleChange = event => {
onChange(event.target.value);
};
const isAccessDeniedError = error => error instanceof Error && error.name === 'AccessDeniedError';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here are mainly just moving things around / using useCallback and useMemo.

}

isValidEmail(email) {
return /\S+@\S+\.\S+/.test(email);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly certain it's valid for an email to not have a period: hi@io if the TLD supports it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @jesstelford! I didn't know that was a thing.

I've just updated that regex to be the same as what browsers use for input type=”email”: https://www.w3.org/TR/2012/WD-html-markup-20120329/input.email.html#input.email.attrs.value.single

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also valid: very.”(),:;<>[]”.VERY.”very@\\ "very”[email protected]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Email addresses are super weird and very hard to validate. Pretty sure you can have almost anything in the user portion since it's up to the receiving server to interpret it. I believe you also don't need a dot in the host portion as you can refer to hosts on a local network, eg. me@localhost is valid (if I'm the kind of person to run a personal mail server on my own machine where I'm also hosting Keystone I guess and, for some reason, emailing myself.. 🤷🏻‍♂️).

Glad we're delegating all this to the W3C suggested regex!

@gautamsi
Copy link
Member

gautamsi commented Jul 8, 2020

My thought would be to add validation features to Text field with certain built in validations. There are several of the other requests are pending like Text length validation

@Vultraz
Copy link
Contributor

Vultraz commented Jul 8, 2020

I was also thinking that, but how would the admin UI views be handled?

@gautamsi
Copy link
Member

gautamsi commented Jul 8, 2020

@Vultraz this PR is mostly same as Text Field just adding a validation of email in controller and setting type on input field.
practically it can be a flag in Text field as validate=email (or any suitable thing) which triggers email validation inside the Text controller instead of brand new field type

@MadeByMike
Copy link
Contributor

I'm of 2 minds about whether we need this or just a better way to specify adminUI interfaces.

@JedWatson
Copy link
Member

@jordanoverbye this looks good to me, but a quick question about the graphql errors you included as an example above - it demonstrates that the missing name field is required where I think you meant to demo the invalid value in the email field not passing validation?

@jordanoverbye
Copy link
Contributor Author

@jordanoverbye this looks good to me, but a quick question about the graphql errors you included as an example above - it demonstrates that the missing name field is required where I think you meant to demo the invalid value in the email field not passing validation?

@JedWatson in the demo I setup for this, I created a list with name and email field. I probably should have excluded the name field which would have simplified those errors. I've just updated the PR description which does that 👍

Copy link
Member

@molomby molomby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is going to suck but we need to address case sensitivity here.

TL;DR: as written, this field type can't be used for user identifiers (usernames) without causing serious data and usability problems.

The user portion ("local-part") of an email address is, technically, case-sensitive...

The local-part of a mailbox MUST BE treated as case sensitive.

In practice this is almost never what you want; as far as I know no major email providers support this.. but we clearly can't assume that no email providers do.

Solution 1: If you're only storing an email address as somewhere to fling mail you can ignore this – just store and use what the user provides. Easy. Of course, it's very common to use email addresses as a form of user identifier for auth. Now you need to make difficult decisions...

If someone signs up as [email protected] and later someone tries to sign in as [email protected] with the correct password, do you let them in? According to the RFC you shouldn't but this is going to cause a torrent of support enquiries. Not the least of which because, if you strictly support case-sensitive "local-parts", your forgotten password function will also not recognise [email protected] as an existing user. Further, the same person could also sign up again using [email protected] since the isUnique constraint, implemented by the underlying Text field, is case-sensitive on most DBs). Ignoring the problem like this is ok for "contact me at" email fields but terrible for "username" email fields.

Solution 2: Seems like you should just ignore case completely then, right? The domain part of the email is already case-insensitive so we can just lowercase the entire string and store that. This solves the problems above but now you have new ones! Anyone who does use a "correct" (to the spec) mail server is now basically locked out of your system. If my email address really is upper-case-J [email protected] and you force that to lowercase, I won't receive your emails, be able to activate my account or "forget" my password. No one using my mail provider will (unless they happen to have an all lowercase email by chance).

Solution 3: Ok, what if we stored the cased version but made sure all our equality checks (ie. when someone's trying to sign in, etc.) are case-insensitive. Then we can appear case-insensitive but also send mail to the exact address the user entered. Cool. This also needs to hit the isUnique constraint too though, since you still need user identifiers to be unique. This works ok but your still blocking some users. If Joanna Bell ([email protected]) signs up, loves your site and tells her coworker, Jo Annabel ([email protected]), Jo will be blocked from registering because she's "already a user".

In this scenario you need to be careful around things like forgotten password functions too. If Jo sees the "already a user" error, thinks she may have registered and forgotten about it, and tries to recover her password, it's easy to have a bug where the system checks verifies the email entered exists in the DB (which is does when using case-insensitive equality) then sends the recovery link to the email entered which, in this case, would give Jo access to Joanna's account. Bad.

Solution 3 is also a bit of a pain to implement. Knex doesn't have options for things like case-insensitive unique constraints and we don't want Keystone to be aware of/have code for specific DB platforms. You end up duplicating the data; one value for what they entered (used on read) and a forced-lowercase value (used for filtering and indexing/enforcing uniqueness).

So none of these solutions solve the problem well and they all have sharp edges the developer should be aware of. Keystone likes to make these things easy and have sensible defaults but I have trouble picking the lesser evil here on behalf of all Keystone users. Solution 2 is probably the best/most practical but it explicitly violates the actual RFC, which feels gross. Still probably better than storing duplicate data (solution 2). Ignoring the problem (solution 1, also current implementation) is a footgun that will create hard to resolve data/security issues in most projects unless used carefully.

The ideal outcome would be to support all three; to have an option for caseFolding you could set to disabled (solution 1), forceUpper, forceLower (solution 2) or equalityOnly (solution 3) – with forceLower being the default – then some good docs explaining the problem/tradeoffs. I'm not suggesting we do all that work up front but supporting at least solutions 1 and 2 (disabled, forceUpper, forceLower) would be pretty easy.

See also the caseTo option for Uuid where we solve a similar problem. The difference there is case truly doesn't matter at a technical level, it's just about the convention you want to use.

@VinayaSathyanarayana
Copy link
Contributor

I support the need for Email Field.

Some suggestions:-
We should support (directly or through hooks) ability to validate email domain names and handle common typing errors

Please look at

@@ -26,7 +38,7 @@ const TextField = ({ onChange, autoFocus, field, errors, value = '', isDisabled
autoComplete="off"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a dumb call-out on my behalf; but should we not enable autoComplete for email addresses?

@MadeByMike
Copy link
Contributor

Sadly @jordanoverbye I think we should close this PR :( essentially this is a text field. We need to make custom interfaces easier but should not add to core field types every time there is a variation of text.

If @molomby suggestions were to be considered and the issues resolved then this would warrant a new field type.

@MadeByMike MadeByMike closed this Jul 24, 2020
@molomby
Copy link
Member

molomby commented Jul 29, 2020

I'm definitely going to come back to this; the requirement comes up so often, it's worth solving.

@timleslie timleslie deleted the fields/email branch November 9, 2020 03:52
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

Successfully merging this pull request may close these issues.

10 participants