diff --git a/Gemfile b/Gemfile index 7fd10bbe..0ba84054 100644 --- a/Gemfile +++ b/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] diff --git a/Gemfile.lock b/Gemfile.lock index 5dfd12ba..7ca0f70e 100644 --- a/Gemfile.lock +++ b/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 diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 403f5f8d..adaa08b6 100644 --- a/app/controllers/application_controller.rb +++ b/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 @@ -58,6 +59,15 @@ class ApplicationController < ActionController::Base end 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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 470af029..30931609 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index c249f3c4..51cb36aa 100644 --- a/app/models/user.rb +++ b/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? diff --git a/config/locales/en.yml b/config/locales/en.yml index 2d75abcb..475d3710 100644 --- a/config/locales/en.yml +++ b/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 diff --git a/db/migrate/20110727073510_change_crypted_password_length.rb b/db/migrate/20110727073510_change_crypted_password_length.rb new file mode 100644 index 00000000..9bd2c639 --- /dev/null +++ b/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 diff --git a/features/handling_users_with_deprecated_password_hashes.feature b/features/handling_users_with_deprecated_password_hashes.feature new file mode 100644 index 00000000..353afec5 --- /dev/null +++ b/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 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 diff --git a/features/step_definitions/user_steps.rb b/features/step_definitions/user_steps.rb index 164bf1fb..117b2b66 100644 --- a/features/step_definitions/user_steps.rb +++ b/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 diff --git a/features/support/paths.rb b/features/support/paths.rb index 52b32975..65579a71 100644 --- a/features/support/paths.rb +++ b/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: diff --git a/features/support/user.rb b/features/support/user.rb new file mode 100644 index 00000000..e40d3e89 --- /dev/null +++ b/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 diff --git a/spec/factories/factories.rb b/spec/factories/factories.rb index 749e0b33..cbf6c0a5 100644 --- a/spec/factories/factories.rb +++ b/spec/factories/factories.rb @@ -1,21 +1,25 @@ -Factory.define :user do |u| - u.sequence(:login) { |n| "testuser#{n}" } - u.password "secret" - u.password_confirmation { |user| user.password } - u.is_admin false -end +begin + Factory.define :user do |u| + u.sequence(:login) { |n| "testuser#{n}" } + u.password "secret" + u.password_confirmation { |user| user.password } + u.is_admin false + end -Factory.define :context do |c| - c.sequence(:name) { |n| "testcontext#{n}" } - c.hide false - c.created_at Time.now.utc -end + Factory.define :context do |c| + c.sequence(:name) { |n| "testcontext#{n}" } + c.hide false + c.created_at Time.now.utc + end -Factory.define :project do |p| - p.sequence(:name) { |n| "testproject#{n}" } -end + Factory.define :project do |p| + p.sequence(:name) { |n| "testproject#{n}" } + end -Factory.define :todo do |t| - t.sequence(:description) { |n| "testtodo#{n}" } - t.association :context + 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 diff --git a/spec/fixtures/users.yml b/spec/fixtures/users.yml index 5d027653..cdd9a914 100644 --- a/spec/fixtures/users.yml +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3fd36dfa..035960ad 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 010cfcc4..9056df2d 100644 --- a/test/fixtures/users.yml +++ b/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 diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 99999ab7..17c4f31b 100644 --- a/test/functional/users_controller_test.rb +++ b/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 diff --git a/test/integration/users_xml_api_test.rb b/test/integration/users_xml_api_test.rb index 6ae04a62..25cb5b2e 100644 --- a/test/integration/users_xml_api_test.rb +++ b/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 diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 968bd2ab..247ce2c0 100644 --- a/test/unit/user_test.rb +++ b/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 @@ -330,6 +330,32 @@ class UserTest < ActiveSupport::TestCase assert_equal u.id, User.find_by_open_id_url(raw_open_id_url).id 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