Merge pull request #16 from jstepien/bcrypt-v2

Hash passwords with BCrypt instead of SHA1

Gemfile

@@ -16,7 +16,7 @@ gem "actionwebservice", :git => "git://github.com/dejan/actionwebservice.git"
 gem "rubycas-client"
 gem "ruby-openid", :require => "openid"
 gem "sqlite3"
-
+gem 'bcrypt-ruby', '~> 2.1.4'
 
 gem "webrat", ">=0.7.0", :groups => [:cucumber, :test]
 gem "database_cleaner", ">=0.5.0", :groups => [:cucumber, :selenium]

Gemfile.lock

@@ -24,6 +24,7 @@ GEM
     activeresource (2.3.14)
       activesupport (= 2.3.14)
     activesupport (2.3.14)
+    bcrypt-ruby (2.1.4)
     builder (3.0.0)
     cgi_multipart_eof_fix (2.5.0)
     cucumber (1.0.2)
@@ -96,6 +97,7 @@ DEPENDENCIES
   ZenTest (>= 4.0.0)
   aasm (= 2.2.0)
   actionwebservice!
+  bcrypt-ruby (~> 2.1.4)
   cucumber-rails (~> 0.3.0)
   database_cleaner (>= 0.5.0)
   flexmock

app/controllers/application_controller.rb

@@ -16,6 +16,7 @@ class ApplicationController < ActionController::Base
   layout proc{ |controller| controller.mobile? ? "mobile" : "standard" }
   exempt_from_layout /\.js\.erb$/
  
+  before_filter :check_for_deprecated_password_hash
   before_filter :set_session_expiration
   before_filter :set_time_zone
   before_filter :set_zindex_counter
@@ -59,6 +60,15 @@ class ApplicationController < ActionController::Base
     end
   end
 
+  # Redirects to change_password_user_path if the current user uses a
+  # deprecated password hashing algorithm.
+  def check_for_deprecated_password_hash
+    if current_user and current_user.uses_deprecated_password?
+      notify :warning, t('users.you_have_to_reset_your_password')
+      redirect_to change_password_user_path current_user
+    end
+  end
+  
   def render_failure message, status = 404
     render :text => message, :status => status
   end

app/controllers/users_controller.rb

@@ -1,6 +1,8 @@
 class UsersController < ApplicationController
   before_filter :admin_login_required, :only => [ :index, :show, :destroy ]
   skip_before_filter :login_required, :only => [ :new, :create ]
+  skip_before_filter :check_for_deprecated_password_hash,
+    :only => [ :change_password, :update_password ]
   prepend_before_filter :login_optional, :only => [ :new, :create ]
   
   # GET /users GET /users.xml

app/models/user.rb

@@ -1,4 +1,5 @@
 require 'digest/sha1'
+require 'bcrypt'
 
 class User < ActiveRecord::Base
   # Virtual attribute for the unencrypted password
@@ -123,7 +124,8 @@ class User < ActiveRecord::Base
     return nil if candidate.nil?
 
     if Tracks::Config.auth_schemes.include?('database')
-      return candidate if candidate.auth_type == 'database' && candidate.crypted_password == sha1(pass)
+      return candidate if candidate.auth_type == 'database' and
+        candidate.password_matches? pass
     end
     
     if Tracks::Config.auth_schemes.include?('ldap')
@@ -190,7 +192,7 @@ class User < ActiveRecord::Base
   end
   
   def generate_token
-    self.token = Digest::SHA1.hexdigest "#{Time.now.to_i}#{rand}"
+    self.token = self.class.sha1 "#{Time.now.to_i}#{rand}"
   end
   
   def remember_token?
@@ -210,15 +212,36 @@ class User < ActiveRecord::Base
     save(false)
   end
 
+  # Returns true if the user has a password hashed using SHA-1.
+  def uses_deprecated_password?
+    crypted_password =~ /^[a-f0-9]{40}$/i
+  end
+
+  def password_matches?(pass)
+    if uses_deprecated_password?
+      crypted_password == User.sha1(pass)
+    else
+      BCrypt::Password.new(crypted_password) == pass
+    end
+  end
+
 protected
 
+  def self.salted(s)
+    "#{Tracks::Config.salt}--#{s}--"
+  end
+
   def self.sha1(s)
-    Digest::SHA1.hexdigest("#{Tracks::Config.salt}--#{s}--")
+    Digest::SHA1.hexdigest salted s
+  end
+
+  def self.hash(s)
+    BCrypt::Password.create s
   end
   
   def crypt_password
     return if password.blank?
-    write_attribute("crypted_password", self.class.sha1(password)) if password == password_confirmation
+    write_attribute("crypted_password", self.class.hash(password)) if password == password_confirmation
   end
   
   def password_required?
@@ -229,10 +252,6 @@ protected
     auth_type == 'open_id'
   end
   
-  def password_matches?(pass)
-    crypted_password == sha1(pass)
-  end
-  
   def normalize_open_id_url
     return if open_id_url.nil?
     

config/locales/en.yml

@@ -778,6 +778,7 @@ en:
     register_with_cas: With your CAS username
     label_auth_type: Authentication type
     new_password_label: New password
+    you_have_to_reset_your_password: "You have to reset your password"
     new_user_title: TRACKS::Sign up as the admin user
     destroy_user: Destroy user
     total_users_count: You have a total of %{count} users

db/migrate/20110727073510_change_crypted_password_length.rb

@@ -0,0 +1,20 @@
+class ChangeCryptedPasswordLength < ActiveRecord::Migration
+  def self.up
+    change_column 'users', 'crypted_password', :string, :limit => 60
+  end
+
+  def self.down
+    # Begin with setting all passwords hashed with BCrypt to SHA-1 ones as
+    # BCrypt's format won't fit into a narrower column.
+    User.transaction do
+      User.all.each do |user|
+        if user.auth_type == 'database' and not user.uses_deprecated_password?
+          user.password = user.password_confirmation = nil
+          user.crypted_password = User.sha1 'change_me'
+          user.save!
+        end
+      end
+    end
+    change_column 'users', 'crypted_password', :string, :limit => 40
+  end
+end

features/handling_users_with_deprecated_password_hashes.feature

@@ -0,0 +1,35 @@
+Feature: Handling users with deprecated passwords hashes
+  In order to have my password hashed with BCrypt
+  As a user with password hashed with SHA1
+  I have to be redirected to the password resetting form
+
+  Background:
+    Given the following user records
+      | login         | password_with_algorithm |
+      | new_hash_user | first_secret bcrypt     |
+      | old_hash_user | another_secret sha1     |
+
+  Scenario Outline: A user with SHA1 password
+    Given I have logged in as "old_hash_user" with password "another_secret"
+    When I go to the <name> page
+    Then I should be redirected to the change password page
+    And I should see "You have to reset your password"
+    When I change my password to "newer_better_password"
+    Then I should be redirected to the preference page
+
+    Examples:
+      | name        |
+      | home        |
+      | preferences |
+      | notes       |
+      | tickler     |
+
+  Scenario: A user with SHA1 password goes straight to the change password page
+    Given I have logged in as "old_hash_user" with password "another_secret"
+    When I go to the change password page
+    Then I should be on the change password page
+
+  Scenario: A user with BCrypt password
+    Given I have logged in as "new_hash_user" with password "first_secret"
+    When I go to the homepage
+    Then I should be on the homepage

features/step_definitions/user_steps.rb

@@ -32,3 +32,9 @@ Then "I should be an admin" do
   # just check on the presence of the menu item for managing users
   Then "I should see \"Manage users\""
 end
+
+When /^I change my password to "([^"]*)"$/ do |password|
+  Then 'I should be on the change password page'
+  %w{new confirm}.each { |name| fill_in name + ' password', :with => password }
+  click_button
+end

features/support/paths.rb

@@ -104,6 +104,8 @@ module NavigationHelpers
     when /the tag page for "([^"]*)"/i
       @source_view = "tag"
       tag_path($1, options)
+    when /the change password page/
+      change_password_user_path @current_user
 
       # Add more mappings here.
       # Here is an example that pulls values out of the Regexp:

features/support/user.rb

@@ -0,0 +1,18 @@
+class User
+  # A method used in features' user records definitions. It accepts a string
+  # with a password and the name of a hashing algorithm ('sha1' or 'bcrypt')
+  # concatenated with a space. It encrypts user's password using the given
+  # mechanism and the given password value.
+  def password_with_algorithm=(x)
+    pass, algorithm = *x.split
+    case algorithm
+    when 'bcrypt'
+      change_password pass, pass
+    when 'sha1'
+      self.crypted_password = User.sha1 pass
+      self.password = self.password_confirmation = nil
+    else
+      raise "Unknown hashing algorithm: #{algorithm}"
+    end
+  end
+end

spec/factories/factories.rb

@@ -1,3 +1,4 @@
+begin
   Factory.define :user do |u|
     u.sequence(:login) { |n| "testuser#{n}" }
     u.password "secret"
@@ -19,3 +20,6 @@ Factory.define :todo do |t|
     t.sequence(:description) { |n| "testtodo#{n}" }
     t.association :context
   end
+rescue FactoryGirl::DuplicateDefinitionError
+  # No problem, apparently this file was included already.
+end

spec/fixtures/users.yml

@@ -1,7 +1,7 @@
 # Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html
 admin_user:
   login: admin
-  crypted_password: <%= Digest::SHA1.hexdigest("#{Tracks::Config.salt}--abracadabra--") %>
+  crypted_password: <%= BCrypt::Password.create("abracadabra") %>
   token: <%= Digest::SHA1.hexdigest("adminSat Feb 25 17:14:00 GMT 20060.236961325863376") %>
   is_admin: true
   first_name: Admin
@@ -10,7 +10,7 @@ admin_user:
 
 other_user:
   login: jane
-  crypted_password: <%= Digest::SHA1.hexdigest("#{Tracks::Config.salt}--sesame--") %>
+  crypted_password: <%= BCrypt::Password.create("sesame") %>
   token: <%= Digest::SHA1.hexdigest("janeSun Feb 19 14:42:45 GMT 20060.408173979260027") %>
   is_admin: false
   first_name: Jane

spec/models/user_spec.rb

@@ -109,7 +109,7 @@ describe User do
     end
 
     it 'authenticates user' do
-      User.authenticate('simon', 'foobarspam').should == @user
+      User.authenticate('simon', 'foobarspam').id.should be @user.id
     end
 
     it 'resets password' do
@@ -117,12 +117,13 @@ describe User do
         :password => 'new password',
         :password_confirmation => 'new password'
       )
-      User.authenticate('simon', 'new password').should == @user
+      User.authenticate('simon', 'foobarspam').should be_nil
+      User.authenticate('simon', 'new password').id.should be @user.id
     end
 
     it 'does not rehash password after update of login' do
       @user.update_attribute(:login, 'foobar')
-      User.authenticate('foobar', 'foobarspam').should == @user
+      User.authenticate('foobar', 'foobarspam').id.should be @user.id
     end
 
     it 'sets remember token' do

test/fixtures/users.yml

@@ -2,7 +2,7 @@
 admin_user:
   id: 1
   login: admin
-  crypted_password: <%= Digest::SHA1.hexdigest("#{Tracks::Config.salt}--abracadabra--") %>
+  crypted_password: <%= BCrypt::Password.create("abracadabra") %>
   token: <%= Digest::SHA1.hexdigest("adminSat Feb 25 17:14:00 GMT 20060.236961325863376") %>
   is_admin: true
   first_name: Admin
@@ -12,7 +12,7 @@ admin_user:
 other_user:
   id: 2
   login: jane
-  crypted_password: <%= Digest::SHA1.hexdigest("#{Tracks::Config.salt}--sesame--") %>
+  crypted_password: <%= BCrypt::Password.create("sesame") %>
   token: <%= Digest::SHA1.hexdigest("janeSun Feb 19 14:42:45 GMT 20060.408173979260027") %>
   is_admin: false
   first_name: Jane
@@ -32,7 +32,7 @@ ldap_user:
 sms_user:
   id: 4
   login: sms_user
-  crypted_password: <%= Digest::SHA1.hexdigest("#{Tracks::Config.salt}--sesame--") %>
+  crypted_password: <%= BCrypt::Password.create("sesame") %>
   token: <%= Digest::SHA1.hexdigest("sms_userSun Feb 19 14:42:45 GMT 20060.408173979260027") %>
   is_admin: false
   first_name: SMS
@@ -48,3 +48,13 @@ ldap_user:
   first_name: International
   last_name: Harvester
   auth_type: CAS
+
+user_with_sha1_password:
+  id: 6
+  login: mr_deprecated
+  crypted_password: <%= Digest::SHA1::hexdigest("#{Tracks::Config.salt}--foobar--") %>
+  token: <%= Digest::SHA1.hexdigest("mr_deprecatedSun Feb 19 14:42:45 GMT 20060.408173979260027") %>
+  is_admin: false
+  first_name: Mister
+  last_name: Deprecated
+  auth_type: database

test/functional/users_controller_test.rb

@@ -31,7 +31,7 @@ class UsersControllerTest < ActionController::TestCase
     get :index
     assert_response :success
     assert_equal "TRACKS::Manage Users", assigns['page_title']  
-    assert_equal 4, assigns['total_users']
+    assert_equal 5, assigns['total_users']
     assert_equal "/users", session['return-to']
   end
 
@@ -68,7 +68,7 @@ class UsersControllerTest < ActionController::TestCase
     post :update_password, :updateuser => {:password => 'newpassword', :password_confirmation => 'newpassword'}
     assert_redirected_to preferences_path
     @updated_user = User.find(users(:admin_user).id)
-    assert_equal @updated_user.crypted_password, Digest::SHA1.hexdigest("#{Tracks::Config.salt}--newpassword--")
+    assert_not_nil User.authenticate(@updated_user.login, 'newpassword')
     assert_equal "Password updated.", flash[:notice]
   end
   

test/integration/users_xml_api_test.rb

@@ -79,7 +79,7 @@ class UsersXmlApiTest < ActionController::IntegrationTest
     get '/users.xml', {}, basic_auth_headers()
     assert_response :success
     assert_tag :tag => "users",
-               :children => { :count => 4, :only => { :tag => "user" } }
+               :children => { :count => 5, :only => { :tag => "user" } }
     assert_no_tag :tag => "password"
   end
 

test/unit/user_test.rb

@@ -33,7 +33,7 @@ class UserTest < ActiveSupport::TestCase
     assert_kind_of User, @admin_user
     assert_equal 1, @admin_user.id
     assert_equal "admin", @admin_user.login
-    assert_equal "#{Digest::SHA1.hexdigest("#{Tracks::Config.salt}--abracadabra--")}", @admin_user.crypted_password
+    assert_not_nil @admin_user.crypted_password
     assert_not_nil @admin_user.token
     assert @admin_user.is_admin
   end
@@ -43,7 +43,7 @@ class UserTest < ActiveSupport::TestCase
     assert_kind_of User, @other_user
     assert_equal 2, @other_user.id
     assert_equal "jane", @other_user.login
-    assert_equal "#{Digest::SHA1.hexdigest("#{Tracks::Config.salt}--sesame--")}", @other_user.crypted_password
+    assert_not_nil @other_user.crypted_password
     assert_not_nil @other_user.token
     assert @other_user.is_admin == false || @other_user.is_admin == 0
   end
@@ -331,6 +331,32 @@ class UserTest < ActiveSupport::TestCase
      end
   end
 
+  def test_should_discover_using_depracted_password
+    assert_nil @admin_user.uses_deprecated_password?
+    assert_nil @other_user.uses_deprecated_password?
+    assert users(:user_with_sha1_password).uses_deprecated_password?
+  end
+
+  def test_should_not_have_deprecated_password_after_update
+    u = users(:user_with_sha1_password)
+    assert u.uses_deprecated_password?
+    u.change_password("foobar", "foobar")
+    assert_nil u.uses_deprecated_password?
+  end
+
+  def test_should_authenticate_with_deprecated_password
+    assert_nil User.authenticate('mr_deprecated', 'wrong password')
+    assert_equal users(:user_with_sha1_password),
+      User.authenticate('mr_deprecated', 'foobar')
+  end
+
+  def test_password_matches
+    assert_not_nil User.authenticate(@admin_user.login, "abracadabra")
+    assert_nil User.authenticate(@admin_user.login, "incorrect")
+    assert_not_nil User.authenticate(users(:user_with_sha1_password).login, "foobar")
+    assert_nil User.authenticate(users(:user_with_sha1_password).login, "wrong")
+  end
+  
   
   protected
     def create_user(options = {})