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

Custom attribute name in has_secure_password #1440

Closed
wants to merge 3 commits into from
Closed

Custom attribute name in has_secure_password #1440

wants to merge 3 commits into from

Conversation

andhapp
Copy link
Contributor

@andhapp andhapp commented Jun 1, 2011

Added code to allow users to pass a custom attribute name to has_secure_password.

Example:

class User
   has_secure_password :encrypted_password
end

@sikachu
Copy link
Member

sikachu commented Jun 1, 2011

Assigned to @dhh for review.

I think this ticket looks good to me, just curious if you want to add anything to it or not.

@ghost ghost assigned dhh Jun 1, 2011
@dmathieu
Copy link
Contributor

dmathieu commented Jun 1, 2011

I think if we allow a customized password field, we need to allow to have more than one secured_password field.

# Returns self if the password is correct, otherwise false.
def authenticate(unencrypted_password)
if BCrypt::Password.new(password_digest) == unencrypted_password
if BCrypt::Password.new(send(:"#{custom_password_attribute}")) == unencrypted_password
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simply send(custom_password_attribute), no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I did change it but forgot to push it. Will do now.

@andhapp
Copy link
Contributor Author

andhapp commented Jun 1, 2011

More than one secure field does make sense. Should I do it in this pull request or may be do it as a separate pull request? Please give your opinion.

@josevalim
Copy link
Contributor

I don't think we need to support more than one field. As maintainer of Devise, nobody has ever asked for a feature like that.

@andhapp
Copy link
Contributor Author

andhapp commented Jun 1, 2011

I think the ability to encrypt any field and not just password field would be interesting. Something like:

has_secure_field :some_random_attribute

However, it would need more work as at the moment the whole idea of secure field is tightly bound to password but IMHO the code should be able to adapt to any field irrespective. Therefore, I would create a diff pull request for it if it's deemed useful.

Opinion?

@andhapp
Copy link
Contributor Author

andhapp commented Jun 3, 2011

Any more feedback on this pull request?

@dasch
Copy link
Contributor

dasch commented Jun 3, 2011

There is a rampant discussion on this topic in issue #1159. My personal opinion is that this is so easy to implement, any non-standard interaction (field name, encryption method, etc.) should just have its own implementation.

If absolutely needed, any options to has_secure_password should be passed in a options hash, e.g. has_secure_password :column => "pwd".

@josevalim
Copy link
Contributor

I am closing this after some discussion with other cores. If you want more customization, there are plenty auth tools out there.

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.

None yet

6 participants