Skip to content

BCryptMigrationFromSHA1 doesn’t save the BCrypt-encrypted password to database #236

Closed
edouard opened this Issue Nov 13, 2012 · 2 comments

2 participants

@edouard
edouard commented Nov 13, 2012

I am using the BCryptMigrationFromSHA1 password strategy with Clearance 1.0.0.rc4.

When a user is authenticates with a SHA1 password, a new BCrypt password is generated and User.password is correctly set with the new Bcrypt-encrypted password.

But save is never called on the User model, and the new encrypted password generated is never saved to database, so the user’s password stays a SHA-1 password and doesn’t migrate to Bcrypt.

I would have created a pull request to fix this, but I wanted to discuss where’s the best place to fix this.

BCryptMigrationFromSHA1::SHA1User would be the easiest place:

module Clearance
  module PasswordStrategies
    module BCryptMigrationFromSHA1
      class SHA1User
        def authenticated_with_sha1?(password)
          if sha1_password?
            if SHA1User.new(self).authenticated? password
              self.password = password
              # self.save
              true
            end
          end
        end
      end
    end
  end
end

On the upside the BCryptMigrationFromSHA1 password strategy is the only strategy to need to save the User, so it doesn’t add unneeded code anywhere else. But doesn’t feel right to be to handle data persistence in password strategy logic.

The User model feels more like the right place to handle persistence, but adding this line here while this is only useful for the BCryptMigrationFromSHA1 strategy doesn’t feel so right.

module Clearance
  module User
    module ClassMethods
      def authenticate(email, password)
        if user = find_by_email(email.to_s.downcase)
          if user.authenticated? password
            # user.save if user.changed?
            return user
          end
        end
      end
    end
  end
end

What do you guys think?

@jferris
thoughtbot, inc. member
jferris commented Nov 14, 2012

I think the migration strategy is already a weird case. It modifies the password when you ask it if the user is authenticated (query method with side effects). I'd say take the pragmatic approach and go with Answer A: call save within authenticated_with_sha1?.

@edouard
edouard commented Nov 15, 2012

It seems like the easiest way to fix it. I opened a pull request #237 containing a fix and a test. I also tested it on my app and the fix works. Thanks!

@edouard edouard closed this Nov 15, 2012
@geoffharcourt geoffharcourt pushed a commit to geoffharcourt/clearance that referenced this issue Jul 8, 2014
@edouard edouard Persis BCrypt password during SHA1 migration
A full description of the issue this resolves is here:

thoughtbot#236
f529a1a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.