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

valid_password? : a misnomer #3519

Closed
ianks opened this Issue Mar 17, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@ianks
Contributor

ianks commented Mar 17, 2015

Browsing through the authenticatable strategy code I noticed this piece of code that is rather misleading.

# Check if password is present.
def valid_password?
  password.present?
end

This code doesn't really check if a password is valid; it checks if as password is present. This is very confusing as you have to dig deeper into the code to find out what it truly does. The actual body of the function is much more easily comprehensible than the function name itself; I would much more easily comprehend password.present? than valid_password?.

In the spirit of 'complexity is the enemy of security', I propose we deprecate valid_password? and replace it with either password_present? or a simple password.present? unless someone has a strong reason not to.

@josevalim

This comment has been minimized.

Member

josevalim commented Mar 17, 2015

@ianks the current name is fine. Checking for presence is a validation.

Take a sign up form, for example. When you validate the "Name", all you require is the "Name" to be present, nothing less, nothing more. It is still a validation.

This has the same perspective: "is the password valid for authentication?". Also, the reason this method exists as such is because a subclass may change it.

@josevalim josevalim closed this Mar 17, 2015

@ianks

This comment has been minimized.

Contributor

ianks commented Mar 17, 2015

@josevalim Although I do agree that it is a form of validation, that is mostly a pedantic argument. I am arguing from a perspective of audit-ability and comprehensibility of the code.

Another reason why valid_password? is confusing is that in different contexts it has different meanings. Now while it is typically fine for methods sharing the same name to have differing semantics in different contexts, I think that for issues involving security we should achieve to make things as straight-forward as possible to the user.

I could be wrong here, but to me it is overly vague and goes against 'complexity is the enemy of security' sentiment. Also, why close the issue so soon? Maybe you think that I am nit-picking here, but I was genuinely confused by the nature of the code when auditing it; which is typically a bad sign for a lib which require high security (see OpenSSL for more on this 😆).

@josevalim

This comment has been minimized.

Member

josevalim commented Mar 17, 2015

@ianks well, this is a base class. :) The context will be clear on each child class: for HTTPAuthenticable, the context is HTTP authentication and you may have specific rules there for checking if a password is valid in a HTTP authentication context or not. password_present? definitely does not encapsulate what this method should do.

If you find the code confusing, then more documentation is definitely welcome! However I do believe the method is aptly named and we would need stronger evidence of "complexity" to justify pushing a backwards incompatible change.

@ianks

This comment has been minimized.

Contributor

ianks commented Mar 17, 2015

The context will be clear on each child class

My original concern was that is is not actually clear in the DatabaseAuthenticatable child class. Maybe a decent compromise would be to make it more explicit in this particular child class.

module Devise
  module Strategies
    # Default strategy for signing in a user, based on their email and password in the database.
    class DatabaseAuthenticatable < Authenticatable
      def authenticate!
        resource  = valid_password? && mapping.to.find_for_database_authentication(authentication_hash)
        # ...
      end
    end
  end
end
@josevalim

This comment has been minimized.

Member

josevalim commented Mar 17, 2015

Thanks @ianks. You are absolutely right, we should just call password.present? in there.

ianks added a commit to ianks/devise that referenced this issue Mar 17, 2015

Use `password.present?` in DatabaseAuthenticatable strategy
In order to be more clear about the expectations of for authenticating, we use
`password.present?` so there is no confusion about the role of the `valid_password?`
method.

More info: plataformatec#3519

@ianks ianks referenced this issue Mar 17, 2015

Merged

Valid password #3520

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