Skip to content

Add whitespace trimming to login#399

Merged
bennothommo merged 1 commit intorainlab:masterfrom
joseph-d:patch-1
Sep 4, 2019
Merged

Add whitespace trimming to login#399
bennothommo merged 1 commit intorainlab:masterfrom
joseph-d:patch-1

Conversation

@joseph-d
Copy link
Copy Markdown
Contributor

@joseph-d joseph-d commented Sep 3, 2019

I have had several instances of users trying to log in and authenticating failing because they have whitespace in their email address which is obviously difficult to see on the screen. While this is technically a case of user error, I think that adding whitespace trimming would make the login system more user friendly to less technical users.

I have had several instances of users trying to log in and authenticating failing because they have whitespace in their email address which is obviously difficult to see on the screen. While this is technically a case of user error, I think that adding whitespace trimming would make the login system more user friendly to less technical users.
@bennothommo
Copy link
Copy Markdown
Contributor

@joseph-d As simple as this fix is, it technically would introduce a breaking change as there's nothing currently stopping people from having spaces in their username (even trailing spaces).

@joseph-d
Copy link
Copy Markdown
Contributor Author

joseph-d commented Sep 3, 2019

That's a good point, yes. Would it be considered too messy to only do the trim and run authentication a second time only if authentication failed the first time?

I've tried putting in an extra space on a few different websites and all the ones I tried seem to be trimming. I just think it'd be a shame for October websites to behave differently to the norm. I'm running October on a reasonanly-busy site (3,000 - 5,000 visitors per day) and rarely a day goes by when a user doesn't complain about not being able to log in for this reason.

@LukeTowers
Copy link
Copy Markdown
Contributor

@bennothommo if you can verify that it is currently possible to have a user created with extra whitespace in either their username or email then what we can do is create a migration to trim those values in the DB. If it's not actually possible (i.e. it gets trimmed at some layer before being inserted) then it's fine to move forward with this PR. I agree with @joseph-d that October should be trimming those values as that is the standard elsewhere.

@w20k
Copy link
Copy Markdown
Contributor

w20k commented Sep 3, 2019

@LukeTowers you can not create a user with incorrect email (think there is a validation rule for that), but login -without any issues.

Screenshot:
Screenshot 2019-09-03 at 21 40 42

@joseph-d
Copy link
Copy Markdown
Contributor Author

joseph-d commented Sep 4, 2019

I have tested this and I can confirm that spaces within email addresses are not permitted by October but spaces within usernames are permitted.

However, if I try to create users with leading or trailing whitespace such as, " joseph" , "joseph " or " joseph " then the whitespace seems to be trimmed from the beginning and end before the data gets into the database. The situation is the same for email addresses.

Therefore, given the PHP trim() function only trims whitespace before and after a string, and not within, I don't think that this patch would create a breaking change after all.

In fact, this patch is wholly necessary because at present a user creating a username or email with a leading or trailing space would have their input accepted by the system and silently trimmed. At present, the user would then be unable to log in using the same whitespace that they just created their username with! This creates a usability nightmare because a lot of people use autocomplete which remembers data from the registration form for the login form.

For example, try registering a username "joseph " with the email address "josephtest@test.com " and the user will be created as "joseph" and "josephtest@test.com" so it would now be impossible to log in as "joseph " or "josephtest@test.com " even though that's what you just registered with.

@LukeTowers
Copy link
Copy Markdown
Contributor

@joseph-d perfect, thanks for checking.

@bennothommo since

nothing currently stopping people from having spaces in their username (even trailing spaces).

seems to be addressed are you fine with merging this?

@bennothommo
Copy link
Copy Markdown
Contributor

@LukeTowers I'll confirm that the trimming occurs sometime tonight - if that is the case, then yep it's good to merge.

@bennothommo
Copy link
Copy Markdown
Contributor

@LukeTowers confirmed - it is trimming the username. Interestingly, all strings are trimmed by the Model class - TIL.

Thanks for the fix, @joseph-d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants