From 95f0f7144140ab473cd1045276f97aa557caa82d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Sat, 23 Jul 2011 10:52:38 +0200 Subject: [PATCH 01/13] Hash passwords with BCrypt instead of SHA1 BCrypt is regarded as a more secure alternative to hashing using message digest algorithms, such as MD5 and SHA families [0, 1, 2]. Apart from built-in salting it is adaptable to the increasing power of modern processing units, which makes it more secure against brute-force cracking. This commit makes all passwords hashed using BCrypt. The session tokens remain generated using SHA1. Tests were updated, `rake test:units` and `rake test:functionals` didn't report any regressions. [0] http://bcrypt.sourceforge.net/ [1] http://en.wikipedia.org/w/index.php?title=Bcrypt&oldid=439692871 [2] https://github.com/codahale/bcrypt-ruby/blob/eab1c72/README.md --- Gemfile | 2 +- Gemfile.lock | 2 ++ app/models/user.rb | 21 +++++++++++++-------- spec/fixtures/users.yml | 4 ++-- test/fixtures/users.yml | 6 +++--- test/functional/users_controller_test.rb | 2 +- test/unit/user_test.rb | 4 ++-- 7 files changed, 24 insertions(+), 17 deletions(-) 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/models/user.rb b/app/models/user.rb index c249f3c4..3a070dfe 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -123,7 +123,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' && + BCrypt::Password.new(candidate.crypted_password) == salted(pass) end if Tracks::Config.auth_schemes.include?('ldap') @@ -190,7 +191,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? @@ -212,13 +213,21 @@ class User < ActiveRecord::Base 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 salted 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 +238,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/spec/fixtures/users.yml b/spec/fixtures/users.yml index 5d027653..dbb1cf19 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("#{Tracks::Config.salt}--abracadabra--").to_s %> 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("#{Tracks::Config.salt}--sesame--").to_s %> token: <%= Digest::SHA1.hexdigest("janeSun Feb 19 14:42:45 GMT 20060.408173979260027") %> is_admin: false first_name: Jane diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 010cfcc4..e747c8dd 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("#{Tracks::Config.salt}--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("#{Tracks::Config.salt}--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("#{Tracks::Config.salt}--sesame--") %> token: <%= Digest::SHA1.hexdigest("sms_userSun Feb 19 14:42:45 GMT 20060.408173979260027") %> is_admin: false first_name: SMS diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 99999ab7..54af1f22 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -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/unit/user_test.rb b/test/unit/user_test.rb index 968bd2ab..a1946295 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 From 42437eadfa4b3efd253a859fb1fec94feccbe2ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Wed, 27 Jul 2011 09:38:56 +0200 Subject: [PATCH 02/13] Changed the length of users.crypted_password to 60 --- .../20110727073510_change_crypted_password_length.rb | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 db/migrate/20110727073510_change_crypted_password_length.rb 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..cdc1649c --- /dev/null +++ b/db/migrate/20110727073510_change_crypted_password_length.rb @@ -0,0 +1,9 @@ +class ChangeCryptedPasswordLength < ActiveRecord::Migration + def self.up + change_column 'users', 'crypted_password', :string, :limit => 60 + end + + def self.down + change_column 'users', 'crypted_password', :string, :limit => 40 + end +end From e7301608a64e7127aab92de013d8540f872ce195 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 00:49:42 +0200 Subject: [PATCH 03/13] Salting is unnecessary, BCrypt takes care of it Source: http://en.wikipedia.org/w/index.php?title=Bcrypt&oldid=439692871 --- app/models/user.rb | 4 ++-- test/fixtures/users.yml | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 3a070dfe..29c55439 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -124,7 +124,7 @@ class User < ActiveRecord::Base if Tracks::Config.auth_schemes.include?('database') return candidate if candidate.auth_type == 'database' && - BCrypt::Password.new(candidate.crypted_password) == salted(pass) + BCrypt::Password.new(candidate.crypted_password) == pass end if Tracks::Config.auth_schemes.include?('ldap') @@ -222,7 +222,7 @@ protected end def self.hash(s) - BCrypt::Password.create salted s + BCrypt::Password.create s end def crypt_password diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index e747c8dd..f43b9602 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -2,7 +2,7 @@ admin_user: id: 1 login: admin - crypted_password: <%= BCrypt::Password.create("#{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: <%= BCrypt::Password.create("#{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: <%= BCrypt::Password.create("#{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 From 34e0573fc40a34e88b0ecbbf522f90b5f05b09d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 09:48:50 +0200 Subject: [PATCH 04/13] Added to fixtures a user with a SHA-1 password --- test/fixtures/users.yml | 10 ++++++++++ test/functional/users_controller_test.rb | 2 +- test/integration/users_xml_api_test.rb | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index f43b9602..9056df2d 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -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 54af1f22..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 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 From 8e23d1105400b67ee5f5c34fa7f5da13ae56daa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 01:10:47 +0200 Subject: [PATCH 05/13] Added User.uses_deprecated_password? method --- app/models/user.rb | 5 +++++ test/unit/user_test.rb | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 29c55439..ab520d7b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -211,6 +211,11 @@ 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 + protected def self.salted(s) diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index a1946295..b820fbaf 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -330,6 +330,19 @@ 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 protected From e5708f5ce75797465237face857aae1bbbe5d685 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 01:29:48 +0200 Subject: [PATCH 06/13] Authenticate users with deprecated SHA-1 passwords --- app/models/user.rb | 12 ++++++++++-- test/unit/user_test.rb | 13 +++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index ab520d7b..05a50581 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -123,8 +123,8 @@ class User < ActiveRecord::Base return nil if candidate.nil? if Tracks::Config.auth_schemes.include?('database') - return candidate if candidate.auth_type == 'database' && - BCrypt::Password.new(candidate.crypted_password) == pass + return candidate if candidate.auth_type == 'database' and + candidate.password_matches? pass end if Tracks::Config.auth_schemes.include?('ldap') @@ -216,6 +216,14 @@ class User < ActiveRecord::Base 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) diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index b820fbaf..247ce2c0 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -343,6 +343,19 @@ class UserTest < ActiveSupport::TestCase 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 From e221264e74ae2826f876ae08c0cf54ecde07f073 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 16:06:07 +0200 Subject: [PATCH 07/13] Fixed User specs broken in commit b33044 Expressions '...should == @user' caused specs to fail because of ArgumentError in 'User authentication resets password' wrong number of arguments (0 for 1) Replacing expectations declared for User objects with expectations declared for their id fields solves the problem and doesn't change specs' logic. --- spec/fixtures/users.yml | 4 ++-- spec/models/user_spec.rb | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/spec/fixtures/users.yml b/spec/fixtures/users.yml index dbb1cf19..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: <%= BCrypt::Password.create("#{Tracks::Config.salt}--abracadabra--").to_s %> + 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: <%= BCrypt::Password.create("#{Tracks::Config.salt}--sesame--").to_s %> + 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 From a11937788e355660af430d4d75db78a7905d1b8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 20:00:08 +0200 Subject: [PATCH 08/13] Prevent redefinition of factories This commit catches Factory::DuplicateDefinitionErrors raised by factory_girl 2.1.0. See the following thread for some background. http://groups.google.com/group/factory_girl/browse_thread/thread/4df21d9240c20198 --- spec/factories/factories.rb | 38 ++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) 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 From 5d3829cfbfc0540b5713880adc640ec05d178a58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 22:06:37 +0200 Subject: [PATCH 09/13] Users with SHA-1 hashes are redirected to the password change page --- app/controllers/application_controller.rb | 10 +++++++++ app/controllers/users_controller.rb | 2 ++ config/locales/en.yml | 1 + ...rs_with_deprecated_password_hashes.feature | 21 +++++++++++++++++++ features/support/paths.rb | 2 ++ features/support/user.rb | 18 ++++++++++++++++ 6 files changed, 54 insertions(+) create mode 100644 features/handling_users_with_deprecated_password_hashes.feature create mode 100644 features/support/user.rb 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..4cf60081 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 ] prepend_before_filter :login_optional, :only => [ :new, :create ] # GET /users GET /users.xml 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/features/handling_users_with_deprecated_password_hashes.feature b/features/handling_users_with_deprecated_password_hashes.feature new file mode 100644 index 00000000..388a3729 --- /dev/null +++ b/features/handling_users_with_deprecated_password_hashes.feature @@ -0,0 +1,21 @@ +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: A user with SHA1 password + Given I have logged in as "old_hash_user" with password "another_secret" + When I go to the homepage + Then I should be redirected to the change password page + And I should see "You have to reset your password" + + 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/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 From ad1f3b58629610fb8d02b7ff70e9f42a3f976c34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 22:45:52 +0200 Subject: [PATCH 10/13] Users with SHA-1 hashes can reset their passwords --- app/controllers/users_controller.rb | 2 +- .../handling_users_with_deprecated_password_hashes.feature | 2 ++ features/step_definitions/user_steps.rb | 6 ++++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4cf60081..30931609 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -2,7 +2,7 @@ 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 ] + :only => [ :change_password, :update_password ] prepend_before_filter :login_optional, :only => [ :new, :create ] # GET /users GET /users.xml diff --git a/features/handling_users_with_deprecated_password_hashes.feature b/features/handling_users_with_deprecated_password_hashes.feature index 388a3729..a1443324 100644 --- a/features/handling_users_with_deprecated_password_hashes.feature +++ b/features/handling_users_with_deprecated_password_hashes.feature @@ -14,6 +14,8 @@ Feature: Handling users with deprecated passwords hashes When I go to the homepage 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 Scenario: A user with BCrypt password Given I have logged in as "new_hash_user" with password "first_secret" 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 From db77225ff76e043d94b757bef3eeaa7f6ed605c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 22:54:40 +0200 Subject: [PATCH 11/13] More SHA-1 hashes handling scenarios using outlines --- ...users_with_deprecated_password_hashes.feature | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/features/handling_users_with_deprecated_password_hashes.feature b/features/handling_users_with_deprecated_password_hashes.feature index a1443324..353afec5 100644 --- a/features/handling_users_with_deprecated_password_hashes.feature +++ b/features/handling_users_with_deprecated_password_hashes.feature @@ -9,14 +9,26 @@ Feature: Handling users with deprecated passwords hashes | new_hash_user | first_secret bcrypt | | old_hash_user | another_secret sha1 | - Scenario: A user with SHA1 password + 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 homepage + 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 From 97431324dba9bfdb41a6d982435f753fcb9d8465 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Mon, 5 Sep 2011 23:13:34 +0200 Subject: [PATCH 12/13] Password-rehashing "down" for migration 20110727073510 Reinier Balt wrote: > One problem I see is when people want to downgrade. You chop the > password field back to 40 chars, but it will cause all users incapable > of logging in. Perhaps we can put a default password in the password on > migration.down? like sha1('secret123') so we leave Tracks operable on > downgrade? https://github.com/bsag/tracks-old/pull/26#issuecomment-2001500 --- .../20110727073510_change_crypted_password_length.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/db/migrate/20110727073510_change_crypted_password_length.rb b/db/migrate/20110727073510_change_crypted_password_length.rb index cdc1649c..9bd2c639 100644 --- a/db/migrate/20110727073510_change_crypted_password_length.rb +++ b/db/migrate/20110727073510_change_crypted_password_length.rb @@ -4,6 +4,17 @@ class ChangeCryptedPasswordLength < ActiveRecord::Migration 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 From 342b8ab4ef76606f7bb53d57a2378e6fe938583f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20St=C4=99pie=C5=84?= Date: Wed, 7 Sep 2011 17:19:04 +0200 Subject: [PATCH 13/13] Require 'bcrypt' on top of app/models/user.rb --- app/models/user.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/user.rb b/app/models/user.rb index 05a50581..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