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

nil password should be allowed #3887

Closed
wants to merge 1 commit into from
Closed

Conversation

kuroda
Copy link
Contributor

@kuroda kuroda commented Dec 7, 2011

User registration process can be divided into several steps.
If the user registration form No. 1, for example, does not have
password field, validation never succeeds with current implementation.

@drogus
Copy link
Member

drogus commented Dec 10, 2011

I think this can be misleading and it can be a cause of subtle bugs, if you just pass the params without a password, nil will be used and validation will pass, while it should not. This could be achieved by providing some kind of options, like :allow_nil => true, although I'm not sure if this is the case for has_secure_password - it was supposed to be implementation for simple case.

@kuroda
Copy link
Contributor Author

kuroda commented Dec 10, 2011

@drogus My intention is following:

  • If we pass the params without a :password key, validation SHOULD be passed.
  • If we pass the params with a blank password, validation should not be passed (as the current implementation).

Sometimes, I want to have a user who has nil password and can not be authenticated as a kind of place holder.

Think of the case that the user registration process goes like this:

  • A user without password is saved and a confirmation mail is sent to his/her email address.
  • He/she clicks the confirmation URL.
  • Then, he/she enters the password to complete the user registration process.

has_secure_password might has been implemented for simpler cases, but I think we could utilize it in a broader context with some modifications.

That said, I admit my commit can introduce unintentional changes of behavior into existing Rails applications.

I will think more deeply on this theme. Thanks a lot for your comment.

With this option set to true, you can have a valid user whose
password_digest is nil. Such a user can't be authenticated,
but can be saved as a kind of place holder.

This option is useful when users are required to input their
password at the later stage of registration process.
@kuroda
Copy link
Contributor Author

kuroda commented Dec 10, 2011

@drogus I added :allow_nil option so that my commit does not affect the existing Rails applications.

@drogus
Copy link
Member

drogus commented Dec 10, 2011

I've discussed this with some of the core team members and the problem is the thing that I put in my last comment: "it was supposed to be implementation for simple case".

If we start merging things like that, we will end up with full featured authentication system and this is not the intention with has_secure_password. Thank you very much for your contribution, but please use some kind of authentication plugin or implement such module by yourself to make it fit your needs.

@drogus drogus closed this Dec 10, 2011
@kuroda
Copy link
Contributor Author

kuroda commented Dec 10, 2011

OK. I see. Thanks.

@fermion
Copy link

fermion commented Mar 31, 2012

I think it's a little ridiculous to not be open to this simple addition to has_secure_password. @drogus, @josevalim do you really think this would be the start of a slippery slope toward inclusion of an RBAC for has_secure_password? RLY?!

Be reasonable, there are simple use cases where this is desired for a site where simple password authentication is necessary (which has_secure_password is perfect for). Example: a site where invitations are sent by user X to user Y and user Y confirms by filling out a password/password confirmation. The invited user should persist without without a valid password until user Y clicks a link in an email and completes registration.

A not-so-terrible workaround is to manually validate the minimum required data, manually populate the errors and, if all goes well, save without validations. :allow_nil, please. kthx!

@wizardwerdna
Copy link

I see fermion's argument.

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

4 participants