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

Account model login by email breaks if email is upper or mixed case compared to saved email #1261

Closed
wakatara opened this Issue Apr 26, 2013 · 16 comments

Comments

Projects
None yet
5 participants
@wakatara

wakatara commented Apr 26, 2013

Interestingly, I only had this happen on postgres on heroku, and worked without erroring on my local mysql dev box but a user reported it on a reasonably complex app I've built so thought I'd include it in the ActiveRecord template fix foe the framework.

Basically, if the backend email in the account is mixed case but the login comes in with slightly different case, it breaks the login. Admittedly, not a bug necessarily, but all users expect variations on their email address to work on login, so I put in downcasing in the Account generation template and also in the authenticate method there.

I've fixed it here on my forked repo and just made a pull request... =]
https://github.com/tundramonkey/padrino-framework/tree/email_downcase_login_fix

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 10, 2013

Member

It's design decision to accept mixed case user logins. Closing #1262 also.

Member

ujifgc commented May 10, 2013

It's design decision to accept mixed case user logins. Closing #1262 also.

@ujifgc ujifgc closed this May 10, 2013

@wakatara

This comment has been minimized.

Show comment
Hide comment
@wakatara

wakatara May 10, 2013

This is a very bad decision for email logins and contrary to both user and developer expectations in the Admin console, no offence.

name@example.com is different from NAme@Example.com?

Can we please have this decision reviewed or appealed? We're trying to attract people from the Rails community not drive them away.

wakatara commented May 10, 2013

This is a very bad decision for email logins and contrary to both user and developer expectations in the Admin console, no offence.

name@example.com is different from NAme@Example.com?

Can we please have this decision reviewed or appealed? We're trying to attract people from the Rails community not drive them away.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 10, 2013

Member

I mean, it's not framework's decision, it should be decided on design phase of the application.

Member

ujifgc commented May 10, 2013

I mean, it's not framework's decision, it should be decided on design phase of the application.

@wakatara

This comment has been minimized.

Show comment
Hide comment
@wakatara

wakatara May 10, 2013

Sorry, I misunderstood. The answer seemed a bit brusque.

However, can you think of an actual application where you'd want a mixed case login to be different from the lowercased variant? Just saying you could save actual developers tracking down a bug here. I was surprised when someone reported this in my padrino app. Also, interestingly, it caused the "bug" in postgres (on heroku) but mysql passed it fine, further confusing diagnosis and fix.

wakatara commented May 10, 2013

Sorry, I misunderstood. The answer seemed a bit brusque.

However, can you think of an actual application where you'd want a mixed case login to be different from the lowercased variant? Just saying you could save actual developers tracking down a bug here. I was surprised when someone reported this in my padrino app. Also, interestingly, it caused the "bug" in postgres (on heroku) but mysql passed it fine, further confusing diagnosis and fix.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 10, 2013

Member

I guess, the postgres vs. mysql discrepancy should be investigated.

As for the fix #1262, padrino tries to be tested and ORM-agnostic. The patch should provide tests for expected behavior.

Member

ujifgc commented May 10, 2013

I guess, the postgres vs. mysql discrepancy should be investigated.

As for the fix #1262, padrino tries to be tested and ORM-agnostic. The patch should provide tests for expected behavior.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 10, 2013

Member

@padrino/core-members do we want padrino-admin to auth with mixed-case emails?

Member

ujifgc commented May 10, 2013

@padrino/core-members do we want padrino-admin to auth with mixed-case emails?

@ujifgc ujifgc reopened this May 10, 2013

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 10, 2013

Member

Oh, and one more thing about #1262. Let's say, we pull it. Now imagine a guy, he registers with his awesome email Guy-like-a-BOSS@gmail.com . He's happy and proud with his email. Then he takes a glimpse at his profile on the site and sees mere guy-like-a-boss@gmail.com email. He immediately gets frustrated, then depressed in a little while. Then inevitably he hangs himself. Do we want this? I don't think so.

The patch should fix #authenticate and no more.

Member

ujifgc commented May 10, 2013

Oh, and one more thing about #1262. Let's say, we pull it. Now imagine a guy, he registers with his awesome email Guy-like-a-BOSS@gmail.com . He's happy and proud with his email. Then he takes a glimpse at his profile on the site and sees mere guy-like-a-boss@gmail.com email. He immediately gets frustrated, then depressed in a little while. Then inevitably he hangs himself. Do we want this? I don't think so.

The patch should fix #authenticate and no more.

@wakatara

This comment has been minimized.

Show comment
Hide comment
@wakatara

wakatara May 10, 2013

I might hang myself over this reasoning... =p

Emails are case insensitive. He'd have to deal with it anyway. It's all about the tough love, man. guy-like-a-boss needs to HTFU. =]

Anyhow, let me know what you'd like me to do... write tests for just AR? extend this to all the ORMs you use? or what have you. I disagree with your reasoning above, but do think having emails as case sensitive violates the principle of least surprise, which is something padrino should perhaps strive to do. YMMV.

For the beta of the app I'd deployed (a worldwide species monitoring database for AArk) it was the very first bug reported on the app since the email provided to me to create the account had been a KevinZ@... and he typed in lower case kevinz@ to login and cause the error.

But, obviously, I don't want anyone to hang themselves... =]

wakatara commented May 10, 2013

I might hang myself over this reasoning... =p

Emails are case insensitive. He'd have to deal with it anyway. It's all about the tough love, man. guy-like-a-boss needs to HTFU. =]

Anyhow, let me know what you'd like me to do... write tests for just AR? extend this to all the ORMs you use? or what have you. I disagree with your reasoning above, but do think having emails as case sensitive violates the principle of least surprise, which is something padrino should perhaps strive to do. YMMV.

For the beta of the app I'd deployed (a worldwide species monitoring database for AArk) it was the very first bug reported on the app since the email provided to me to create the account had been a KevinZ@... and he typed in lower case kevinz@ to login and cause the error.

But, obviously, I don't want anyone to hang themselves... =]

@NobbZ

This comment has been minimized.

Show comment
Hide comment
@NobbZ

NobbZ May 10, 2013

Am 10.05.2013 12:02 schrieb "Daryl Manning" notifications@github.com:

I might hang myself over this reasoning... =p

Emails are case insensitive. He'd have to deal with it anyway. It's all about the tough love, man. guy-like-a-boss needs to HTFU. =]

That's only half of the truth. The host part (behind @) is case insensitive
by definition. The local part (in front of @) might be case sensitive,
depending on the configuration of the receiving host! So generally down
casing the email might get you into trouble some when.

NobbZ commented May 10, 2013

Am 10.05.2013 12:02 schrieb "Daryl Manning" notifications@github.com:

I might hang myself over this reasoning... =p

Emails are case insensitive. He'd have to deal with it anyway. It's all about the tough love, man. guy-like-a-boss needs to HTFU. =]

That's only half of the truth. The host part (behind @) is case insensitive
by definition. The local part (in front of @) might be case sensitive,
depending on the configuration of the receiving host! So generally down
casing the email might get you into trouble some when.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 10, 2013

Member

@tundramonkey I would be happy to see a pull request containing a patch for #authenticate method which would construct case-insensitive filter (like LIKE or whatever) for the basic ORMs: ActiveRecord, DataMapper, Sequel. Tampering with the case of the saved email is just wrong. Also it would be extremely nice to have tests for these cases.

Member

ujifgc commented May 10, 2013

@tundramonkey I would be happy to see a pull request containing a patch for #authenticate method which would construct case-insensitive filter (like LIKE or whatever) for the basic ORMs: ActiveRecord, DataMapper, Sequel. Tampering with the case of the saved email is just wrong. Also it would be extremely nice to have tests for these cases.

@wakatara

This comment has been minimized.

Show comment
Hide comment
@wakatara

wakatara May 10, 2013

@NobbZ Ok, actually good point, under the rfc this is true (which I was actually surprised to find out... who knew?).

But... if you think that's a good idea, I double dog dare you to set your mail server to make the local part case sensitive. =]

wakatara commented May 10, 2013

@NobbZ Ok, actually good point, under the rfc this is true (which I was actually surprised to find out... who knew?).

But... if you think that's a good idea, I double dog dare you to set your mail server to make the local part case sensitive. =]

@wakatara

This comment has been minimized.

Show comment
Hide comment
@wakatara

wakatara May 10, 2013

@ujifgc Fair enough... in my head it seemed like a good idea at the time to not send out mixed case emails so downcasing the stored data it seemed a dead simple solution, but can see your point about altering data (at the time, I was thinking it would avoid other possible issues where not having a downcased email might cause issues... so that I didn't always have to add email.downcase to everything).

I'm familiar with AR, mongoid and mongomapper but not datamapper and sequel so will need to take a look at these over the weekend or early next week (you picked a bad weekend, I'm uber-busy =< ).

Stand by... =]

wakatara commented May 10, 2013

@ujifgc Fair enough... in my head it seemed like a good idea at the time to not send out mixed case emails so downcasing the stored data it seemed a dead simple solution, but can see your point about altering data (at the time, I was thinking it would avoid other possible issues where not having a downcased email might cause issues... so that I didn't always have to add email.downcase to everything).

I'm familiar with AR, mongoid and mongomapper but not datamapper and sequel so will need to take a look at these over the weekend or early next week (you picked a bad weekend, I'm uber-busy =< ).

Stand by... =]

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jul 1, 2013

Member

@ujifgc as a programmer I agree with you, as a user, I know (sadly so well) that this problem happen so often and is hard to track.

Member

DAddYE commented Jul 1, 2013

@ujifgc as a programmer I agree with you, as a user, I know (sadly so well) that this problem happen so often and is hard to track.

@ghost ghost assigned DAddYE Jul 1, 2013

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jul 1, 2013

Contributor

👍 on lowercasing the email by default on the admin.

On Monday, July 1, 2013, DAddYE wrote:

@ujifgc https://github.com/ujifgc as a programmer I agree with you, as
a user, I know (sadly so well) that this problem happen so often and is
hard to track.


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/issues/1261#issuecomment-20268754
.

Darío

Contributor

dariocravero commented Jul 1, 2013

👍 on lowercasing the email by default on the admin.

On Monday, July 1, 2013, DAddYE wrote:

@ujifgc https://github.com/ujifgc as a programmer I agree with you, as
a user, I know (sadly so well) that this problem happen so often and is
hard to track.


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/issues/1261#issuecomment-20268754
.

Darío

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jul 1, 2013

Member

NO!

Member

ujifgc commented Jul 1, 2013

NO!

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE
Member

DAddYE commented Jul 2, 2013

@ghost ghost assigned ujifgc Jul 9, 2013

@ujifgc ujifgc closed this in 8b2d2c0 Jul 10, 2013

ujifgc added a commit that referenced this issue Jul 10, 2013

Merge pull request #1344 from padrino/mixed-emails
introduce case insensitive authentication by email, closes #1261

deni64k added a commit to deni64k/padrino-framework that referenced this issue Jul 16, 2013

introduce case insensitive authentication by email, closes #1261
Supported ORMS: activerecord, datamapper, minirecord, mongoid, mongomapper, sequel
ORMs not supporting case insensitive search: ohm
Cryptic ORMS: couchrest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment