Skip to content
This repository
Browse code

BCrypt does its own salting, lovely!

  • Loading branch information...
commit bd9dc4ff23ab1e185df6ccf35d6058c0a3d234ce 1 parent 39b5ea6
David Heinemeier Hansson dhh authored
23 activemodel/lib/active_model/secure_password.rb
@@ -4,17 +4,19 @@ module ActiveModel
4 4 module SecurePassword
5 5 extend ActiveSupport::Concern
6 6
  7 + WEAK_PASSWORDS = %w( password qwerty 123456 )
  8 +
7 9 module ClassMethods
8   - # Adds methods to set and authenticate against a SHA2-encrypted and salted password.
9   - # This mechanism requires you to have password_digest and password_salt attributes.
  10 + # Adds methods to set and authenticate against a BCrypt password.
  11 + # This mechanism requires you to have a password_digest attribute.
10 12 #
11 13 # Validations for presence of password, confirmation of password (using a "password_confirmation" attribute),
12   - # and strength of password (at least 6 chars, not "password") are automatically added.
  14 + # and strength of password (at least 6 chars, not "password", etc) are automatically added.
13 15 # You can add more validations by hand if need be.
14 16 #
15 17 # Example using Active Record (which automatically includes ActiveModel::SecurePassword):
16 18 #
17   - # # Schema: User(name:string, password_digest:string, password_salt:string)
  19 + # # Schema: User(name:string, password_digest:string)
18 20 # class User < ActiveRecord::Base
19 21 # has_secure_password
20 22 # end
@@ -33,7 +35,7 @@ def has_secure_password
33 35 attr_reader :password
34 36 attr_accessor :password_confirmation
35 37
36   - attr_protected(:password_digest, :password_salt) if respond_to?(:attr_protected)
  38 + attr_protected(:password_digest) if respond_to?(:attr_protected)
37 39
38 40 validates_confirmation_of :password
39 41 validates_presence_of :password_digest
@@ -44,7 +46,7 @@ def has_secure_password
44 46 module InstanceMethods
45 47 # Returns self if the password is correct, otherwise false.
46 48 def authenticate(unencrypted_password)
47   - if BCrypt::Password.new(password_digest) == (unencrypted_password + salt_for_password)
  49 + if BCrypt::Password.new(password_digest) == unencrypted_password
48 50 self
49 51 else
50 52 false
@@ -54,18 +56,15 @@ def authenticate(unencrypted_password)
54 56 # Encrypts the password into the password_digest attribute.
55 57 def password=(unencrypted_password)
56 58 @password = unencrypted_password
57   - self.password_digest = BCrypt::Password.create(unencrypted_password + salt_for_password)
  59 + self.password_digest = BCrypt::Password.create(unencrypted_password)
58 60 end
59 61
60   - private
61   - def salt_for_password
62   - self.password_salt ||= self.object_id.to_s + rand.to_s
63   - end
64 62
  63 + private
65 64 def password_must_be_strong
66 65 if @password.present?
67 66 errors.add(:password, "must be longer than 6 characters") unless @password.size > 6
68   - errors.add(:password, "can't be 'password'") if @password == "password"
  67 + errors.add(:password, "is a too weak and common") if WEAK_PASSWORDS.include?(@password)
69 68 end
70 69 end
71 70 end
11 activemodel/test/cases/secure_password_test.rb
@@ -33,6 +33,17 @@ class SecurePasswordTest < ActiveModel::TestCase
33 33 assert @user.valid?
34 34 end
35 35
  36 + test "too weak passwords" do
  37 + @user.password = "123456"
  38 + assert !@user.valid?
  39 +
  40 + @user.password = "password"
  41 + assert !@user.valid?
  42 +
  43 + @user.password = "d9034rfjlakj34RR$!!"
  44 + assert @user.valid?
  45 + end
  46 +
36 47 test "authenticate" do
37 48 @user.password = "secret"
38 49

1 comment on commit bd9dc4f

Aditya Sanghi

not "is a too weak and common" but "is too weak and common"

Preston Marshall

actually I think "is insecure" more aptly describes the error.

José Valim

@bbhoss, In a technical point of view yes, but "is insecure" would be a bad message to show to an user. It doesn't reveal much.

Thanks @asanghi, I have pushed a fix.

Michael van Rooijen

It's awesome that BCrypt handles salting.

Vinicius Spader

Maybe this is a too beginer question but, do this line free me from writting it on my app/models/user.rb file?

Please sign in to comment.
Something went wrong with that request. Please try again.