mozilla / django-browserid

Django application for adding BrowserID support.
Mozilla Public License 2.0
180 stars 80 forks source link

Signing in with email with different casing than the one store in database causes pages to refresh. #192

Closed glogiotatidis closed 10 years ago

glogiotatidis commented 10 years ago

If a user signs in with FOO@example.com, while on database foo@example.com exists, then django-browserid will sign the user in (*) but Persona will keep re-posting on assertion on every page because it believes that the case is not properly signed in.

To justify if a user is signed in, Persona checks browserid-info div which should list the email of the user signed in. The email is foo@example.com instead of FOO@example.com, thus Persona believe user is logged out.

While based on the specs FOO@example.com is different to foo@example.com, most email providers treat it the same and so does persona.org but not the include.js library of Persona.

One possible solution is to treat these emails as equal and update the case of the email on the database on each login if it's different. Another solution is to deny logging in the user if the case doesn't match.

(*) if the user will get signed in on the django level is dependant on the used database backend. MySQL for example does case insensitive searching on strings, thus it will sign user in.

Osmose commented 10 years ago

@callahad The inconsistency between Persona.org's handling of email case vs include.js is a little concerning. How do you recommend we handle two emails with different casing?

callahad commented 10 years ago

One sec...

/me attempts to reproduce...

callahad commented 10 years ago

Yep, I can repro. Filing a bug in the browserid repo. Sorry about that! Until we can get a fix out, is it possible to not munge the casing of whatever Persona returns? We should always be internally consistent.

Osmose commented 10 years ago

So we don't actually munge the casing when we store it in the database, I think. Instead, it's that MySQL does case-insensitive string comparisons unless you change the collation, which makes Django return bytestrings for text fields on models, which is a pain to deal with.

It's probably worth a note in the documentation, but I'm not sure what we can do, except maybe switching to using a hash of the email for lookup instead of the email itself (which actually may have other desirable properties, but also has some awkward downsides).

callahad commented 10 years ago

Yeah, it's curious that foo@example is able to get into the database initially, and FOO@example comes along later. That shouldn't happen if both addresses were gleaned from Persona assertions. Still, we should be case insensitive, so we'll get a fix out for you ASAP (it takes ~2-3 weeks to hit production after we land a patch).

glogiotatidis commented 10 years ago

foo@example.com got into the database before persona was enabled. Or it can get if you invite a user by email (== directly create an account for him) to use your persona enabled site.

Thanks for talking care of this!

callahad commented 10 years ago

Aah, that makes sense. Sorry about the trouble!

Osmose commented 10 years ago

So the fix is to make Persona return case-insensitive emails? Sounds like this should be closed, then. Reopen if I'm wrong. :D