Skip to content
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

Honor strip_whitespace_keys from devise config #1028

Merged
merged 2 commits into from
Dec 6, 2017

Conversation

nerfologist
Copy link
Contributor

Hello,
in our application some (clumsy?) users are including whitespace before or after their email address when trying to log in, and failing (credentials were being rejected). We are also not stripping the whitespace in the front-end.

Based on our understanding, it seems that devise_token_auth is not honoring the strip_whitespace_keys as set in config/initializers/devise.rb.

This small patch will strip whitespace according to the strip_whitespace_keys configuration.

(tested and passing on Ruby 2.4.1)

Hope to help, thanks for this great gem!
Marco

@lynndylanhurley
Copy link
Owner

This change looks good to me. Any idea why the tests are failing?

I need to figure out how to disable all of those warnings in travis - it's impossible to see what the actual error was through all the noise.

@nerfologist
Copy link
Contributor Author

Hi, I tried to run the test suite on ruby 2.4.2 and all three databases (sqlite, mysql and postgresql, albeit version 10) and it failed the first time, but mysteriously worked on the second and following times. I have no idea why at the moment, so I was looking for a way to restart the build and see if it would fix itself... 😄

@lynndylanhurley
Copy link
Owner

The error may be unrelated to this PR. I'll pull down and test ASAP.

Copy link
Collaborator

@MaicolBen MaicolBen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I see that travis is failing in the test that you added ("request should succeed if configured")

@zachfeldman
Copy link
Contributor

closing and reopening to try running the tests again

@zachfeldman zachfeldman reopened this Nov 26, 2017
@zachfeldman
Copy link
Contributor

Unfortunately looks like tests are still failing here :(

@lynndylanhurley
Copy link
Owner

lynndylanhurley commented Nov 26, 2017

Looks like this is the failing test:

test_request_should_succeed_if_configured#DeviseTokenAuth::SessionsController::Confirmed user::stripping whitespace

@nerfologist are you sure you're setting the class variable correctly?

@resource_class.strip_whitespace_keys = [:email]

Could it be that you have to do something like this?

@resource_class.class_variable_set(:@@strip_whitespace_keys = [:email])

@nerfologist
Copy link
Contributor Author

Thanks all for your feedback, I'll look into it ASAP.

@nerfologist
Copy link
Contributor Author

nerfologist commented Dec 5, 2017

There was a leftover .upcase from the nearby case_insensitive_keys config option test case (which I copy&pasted). Should be good now.

I can't figure out why it kind of worked for most ruby versions/databases though 🤔 ... possibly some databases were not performing a case-sensitive string match.

Let me know if you want me to squash all commits! 👍

@MaicolBen
Copy link
Collaborator

It's okay, thanks!

If someone else approves this PR, we'll merge it

@zachfeldman zachfeldman merged commit 49911e7 into lynndylanhurley:master Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants