Email validation - Case Sensitive and not
geekdenz opened this issue · 23 comments
We have a problem with this module in that it can happen that a user registers with say
MyName@MyOrganisation.com
once, then fail to login and try to register again.
This time they may register as
myName@myOrganisation.com
or
myname@myorganisation
In any case, in the database, at least in our version, this email gets recorded:
myname@myorganisation
However, the second time they register, it gets accepted and there are 2 records with the same email address and the user cannot log in at all, because the second email validation fails and the DB is in an illegal state.
Since email is not case sensitive, I believe the validation should fail the second time and login as well as registration should be case insensitive.
We need this fixed urgently, so I'm happy to do it. However, if one of you is available today to fix it then I can concentrate on something else.
Please answer ASAP if you have time to fix it now, otherwise I will go ahead and fix it.
Thanks,
Tim
Hi @geekdenz, thanks for bringing this issue forward. It's unlikely that we're going to have time today, but if you could please put a pull request through, I'll make sure it's actioned as soon as possible.
-
I'll do it on the stable branch 1.1.
-
Alternatively, I can check out the stable tag 1.1.1 and make a branch off that with the fix so it is a hotfix and then you can merge it where applicable.
I think 2. is what I'll do because it makes more sense.
Does that make sense?
Hmm, the 1.1 branch looks to be a long way behind, and doesn't even include the 1.1.1 tag (which doesn't make a lot of sense). I'm not sure why this may be the case, as I believe the module did previously change ownership. IMO you're best to branch from master.
Yes, but as I just found out our Email field is custom. However, the EmailValidator that EmailField uses in the SilverStripe framework supplies only validation for whether it is a valid email address in terms of a regular expression.
So, I think this issue might be deeper, so I'll do a custom work-around since we already have made a DataExtension on Member (CustomMember) where we can validate the Email field.
Maybe something to think about, but nothing I can fix within the memberprofiles module let alone have the time for.
Actually, this has to be fixed in this module!
Tested this fix and it works in our situation. I believe the password check is OK but may need to be checked as well by core.
Please merge asap and close to let us know. We are having problems with composer to deploy our custom version.
Having a quick play with a local copy of the member profile module, I don't seem to be having any issues. The member emails are being stored as case sensitive (so myName@myOrganisation.com for example), however, when I attempt to register a second member with the same email but different capitalisation (so myname@myorganisation.com for example), I get the following form validation error back from https://github.com/silverstripe-australia/silverstripe-memberprofiles/blob/master/code/forms/MemberProfileValidator.php#L59.
There is already a member with the same Email.
In which case, it looks like the case sensitivity is already taken into account. May I ask what version of the module and SS you are using? Also, have you tried with a fresh SS installation to confirm whether there may be project specific code getting in the way?
We are using version 1.1.1 and SilverStripe 3.1, I believe.
However, even so, we have site specific code as you suggest for members. I will try a fresh install like you suggest as well. I will close this issue if that solves it.
I'm also wondering if this may be an OS thing, as I know there are some DB case sensitivity differences between Windows and Unix. Which OS would you happen to be using? Anyway, let me know how you go :)
I'm using Ubuntu 16.04. We're using the postgresql module for the DB.
Do you know if I need Apache for SilverStripe to work?
I tried a manual RewriteRule before with something else and it worked with just
php -S localhost:8000
It's just a hassle to setup Apache on every dev machine.
Interesting, I wonder if PostgreSQL, or more specifically the connector is the issue here. I'm also on Ubuntu.
It should work, so long as you're not relying on a .htaccess file for the rewrite, as you mentioned. Also, there are some specific modules such as secure assets that rely on .htaccess. Something to keep an eye out for.
OK, so I tested it with PostgreSQL, a fresh installation of SilverStripe 3.1.15 and the memberprofiles 1.1.1 module from this repository.
When I register with
MyUser@MyCompany.com
I can only log in with that case. However, since email is case-insensitive, I believe it should be stored in all lower-case (or all upper-case, just consistent) and then checked against that case.
With MySQL, can you login with another case? I haven't tested with MySQL and don't have time for that right now.
I had created a work-around for this earlier. However, that didn't work completely. I can do the lower case storage if you don't get to it. But hopefully, you can accept the pull-request and fix it with these modifications.
Using MySQL I can indeed authenticate when using different capitalisation. In which case, it sounds like you're having general SS issues as opposed to member profile specific issues, quite possibly related to PostgreSQL.
I could accept this PR, but I'm not convinced that this is the correct approach, especially when members can still be added through your CMS manually (outside this module). If you need this fixed urgently, I think the best approach would be to patch something in for now, especially if you're using 1.1.1 when the current master branch is way ahead. If you can indeed confirm that PostgreSQL is the problem, I think this at the very least needs some public visibility before others hit the same problem.
Perhaps there is a configuration setting somewhere that allows case insensitive matching?
Did some more digging on the web trying to find out what the SQL standard is etc. as people I know have been claiming that PostgreSQL is much more advanced and standards compliant.
However, according to this and it seems according to the standard, it depends on the collation whether 2 different case strings are equal:
https://stackoverflow.com/questions/3969059/sql-case-sensitive-string-compare
See also:
http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt
Search for "8.2 ".
I think the module should solve it for all collations, no matter how strict the SQL implementation is. So, I would vote for fixing this and always inserting in lower case and then also comparing in lower case when re-registering with the same email but different case.
On the other hand, PostgreSQL also does the comparison when logging in and succeeds, even if the case is different. Now, I haven't logged the query in PostgreSQL but maybe that would be the next thing to do. So I cannot be sure of this, but it seems like an inconsistency more in PostgreSQL than MySQL. Because in PostgreSQL I can register a second time using another case when clearly it should find it in the DB.
no matter how strict the SQL implementation is
I completely agree on this.
However, the member profile module doesn't control how SS writes the emails to the DB in general. In which case, even with your PR, there are still likely to be duplicate emails entering the system. Would it be better for https://github.com/silverstripe-australia/silverstripe-memberprofiles/pull/125/files#diff-3ec937d1574f7cb0dd38aa2c845210b0R51 to make use of the SQL LOWER function to ensure that an email isn't repeated, regardless of the case?
make use of the SQL LOWER function
This is better, yes! I will adjust the fix and do another PR.
Easier would be however, to use strtolower just on the Email field in the code. I think I'll do that instead. Performance penalty is going to be small to non-existent.
@nglasl Please accept my pull request and close. I am sure it will work without testing. Thanks!
Have merged that through, thanks for your patience in getting this one over the line :)