Validate emails on members

lekoala commented 1 year ago

Affected Version

4.*

Description

So this is follow to https://github.com/silverstripe/silverstripe-framework/pull/9887

When entering data in the cms, sometimes users put invalid emails (eg: "[no-email]" is one I got recently) in the Email field, resulting in issues (eg: when sending emails). While this is easy enough to fix on my end (adding a validate method on the member extension) I think it would make a lot of sense to have this by default.

Steps to Reproduce

Open a form in the admin with a member (eg in model admin)
Enter something stupid in the email field (eg: [no-email])
Save!

Suggestions

It would be possible to add a validation in the validate method using https://github.com/egulias/EmailValidator

if ($this->Email) {
    $email= trim($this->Email);
    $validator = new EmailValidator();
    $emailValidation = new RFCValidation();
    if (!$validator->isValid($email, $emailValidation)) {
        $validationResult->addFieldError('Email', 'Invalid email');
    }
}

And also use a proper DBEmail field instead of the default varchar, which would allow having an input field of type email (adding built in browser validation)

GuySartorelli commented 1 year ago

This would be a breaking change, so would need to be introduced in a major release. I'll bring it up in our next backlog refinement session for CMS 5 - though we're close to hitting the API change freeze for that with the beta release date coming up.

If you'd like to create a PR targetting CMS 5 including tests and a separate PR to add it to the CMS 5 changelog, there's a much higher chance of it making it in.

We do need to keep in mind that there will be existing projects where users do not have valid emails as their email address, so the changelog PR will need to include advice for how to resolve those scenarios (which probably boils down do "someone needs to contact those users and let them know to update their email address").

I'm on the fence as to whether we should also have the option of using something other than email address for authentication, but that's probably a larger discussion that could be handled in a separate later enhancement.

dhensby commented 1 year ago

The problem is that Email is really a misnomer, it's actually a username which can be an email address.

It's probably too much to rename Email to Username but we could add a config flag that enforces email address validation on the column. Though, as @GuySartorelli points out, this becomes problematic for existing projects that could have non-email addresses in the database as how do you gracefully catch them and get them updated?

Perhaps we take the approach we have with some other configs which is where email validation on the column is only enabled for fresh installs?

Another problem would be default admins that have usernames that aren't emails, but I have a feeling that's been fixed?

michalkleiner commented 1 year ago

Also platforms like Silverstripe Cloud Platform use its username when creating temporary admins for users with a set privilege, so things are probably reaching beyond a simple validation on the email address, also as Dan mentions it's more a username which can also be an email. There also used to be a config to determine which field was the username/used for authentication IIRC. And... would we need this to touch on SS_DEFAULT_ADMIN as well, somehow, if the validation was also added to the login form?

lekoala commented 1 year ago

:) well, maybe the issue is that an "Email" field may not contain an actual Email... for my part, that seems really counter intuitive and should be clearly documented (because changing it now seems difficult for compat reasons)

I think a really simple yml config flag going along with said documentation can safely enable this (disabled by default)

But maybe it's not worth it and can simply be implemented in userland code through extension if it adds complexity.

edit: but while saying that, then there are more issues if you think about it because for example the lostpasswordhandler assume Email is an actual Email

https://github.com/silverstripe/silverstripe-framework/blob/6c5448b70f91371f29499b1f8632a207137ac8a8/src/Security/MemberAuthenticator/LostPasswordHandler.php#L232-L248

dhensby commented 1 year ago

I think the ideal is to have a username column and an email address column. Then the email address will always be an email and the username can be optional. Login forms can then accept either the email or the username (or just one depending on config).

Perhaps it would be possible to drop the Email column, add Username and EmailAddress and then Email can be a (deprecated) magic prop that looks for EmailAddress and then Username with developers encouraged to actually pick one of the username/emailaddress fields for their logins/user ids

GuySartorelli commented 1 year ago

I really like that solution, @dhensby. It makes it very clear what's changing and how the two fields are intended to be used, and lets Silverstripe Cloud keep use its temporary username while giving developers the confidence that if there is a value in the email address field, they can (try to) send emails to it.

silverstripe / silverstripe-framework