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

Fix NoMethodError in random_password #850

Merged
merged 3 commits into from Apr 14, 2021
Merged

Fix NoMethodError in random_password #850

merged 3 commits into from Apr 14, 2021

Conversation

rhymes
Copy link
Contributor

@rhymes rhymes commented Apr 14, 2021

The method random_password uses self to retrieve the password_length configuration, but self is the User class, which has no password_length by default.

This will result in:

     NoMethodError:
       undefined method `password_length' for #<Class:0x000000000ae64650>

when User.invite!() is called without a password. See also Forem's dependabot's integration build: forem/forem#13383

The length of the password should be fetch from Devise's own config I think

Refs #848

The method `random_password` uses `self` to retrieve the `password_length` configuration, but `self` is the `User` class, which has no `password_length` by default.

The length of the password should be fetch from Devise's own config
@scambra
Copy link
Owner

scambra commented Apr 14, 2021

Getting password_length from devise is not right, because it can be changed in the model, as it's done in the test you changed. If you change only lib, test fails, so it's not right way to fix it.

I'm not sure why it raises NoMethodError, but it doesn't happen in devise invitable tests, do you have validatable in your model?

@rhymes
Copy link
Contributor Author

rhymes commented Apr 14, 2021

Hi @scambra, thanks for the info! No, we're not using :validatable, we're only using :invitable.

In the default config/initializers/devise.rb file I see:

  # Ensure that invited record is valid.
  # The invitation won't be sent if this check fails.
  # Default: false
  # config.validate_on_invite = true

which leads me to believe that :validatable should be optional. The code though, expects password_length to exist on User which won't if https://www.rubydoc.info/github/heartcombo/devise/master/Devise/Models/Validatable is not used. The test doesn't test that scenario or at least the documentation doesn't explain that :validatable is now mandatory

Let me know if I misunderstood.

@scambra
Copy link
Owner

scambra commented Apr 14, 2021

No validatable shouldn't be mandatory, but when is added, model's password_length must be used. So right way to fix would be:

respond_to?(:password_length) ? password_length : Devise.password_length

Or other way to check if :validatable is used by model instead of using respond_to?

@rhymes
Copy link
Contributor Author

rhymes commented Apr 14, 2021

Cool, I'll change the code and test for both cases. Thank you!

@scambra
Copy link
Owner

scambra commented Apr 14, 2021

Thanks for working on this

@rhymes rhymes marked this pull request as ready for review April 14, 2021 12:58
@rhymes
Copy link
Contributor Author

rhymes commented Apr 14, 2021

@scambra those failures look unrelated, at least those related to nokogiri

@scambra scambra merged commit c18b083 into scambra:master Apr 14, 2021
@rhymes
Copy link
Contributor Author

rhymes commented Apr 14, 2021

Thank you! 🙌

@rhymes rhymes deleted the patch-1 branch April 14, 2021 13:08
@davidwessman
Copy link

Will you cut a new release with this fix? @scambra

@scambra
Copy link
Owner

scambra commented Apr 19, 2021

@davidwessman 2.0.5 released

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.

None yet

3 participants