From fff21a1229768d4a30728af2a8e6933c16287aad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 29 Aug 2016 20:36:42 +0200 Subject: [PATCH 1/3] UserPasswords: Replace salted SHA-1 with bcrypt This adds a typed userpassword class for Bcrypt password hashes that replace the previous default of salted SHA1. It still provides a class for SHA1 passwords, however new passwords of that type may no longer be created directly, and passwords are checked whether to upgrade them on every login. --- Gemfile | 2 + Gemfile.lock | 2 + app/controllers/my_controller.rb | 2 +- app/models/user.rb | 12 +-- app/models/user_password.rb | 81 +++++++++++-------- app/models/user_password/bcrypt.rb | 44 ++++++++++ app/models/user_password/sha1.rb | 70 ++++++++++++++++ config/initializers/bcrypt.rb | 41 ++++++++++ ...160829225633_introduce_bcrypt_passwords.rb | 36 +++++++++ lib/open_project/configuration.rb | 1 + spec/factories/user_password_factory.rb | 16 +++- spec/models/user_password_spec.rb | 54 ++++++++++++- spec/models/user_passwords/sha1_spec.rb | 54 +++++++++++++ spec_legacy/fixtures/user_passwords.yml | 10 +++ 14 files changed, 385 insertions(+), 40 deletions(-) create mode 100644 app/models/user_password/bcrypt.rb create mode 100644 app/models/user_password/sha1.rb create mode 100644 config/initializers/bcrypt.rb create mode 100644 db/migrate/20160829225633_introduce_bcrypt_passwords.rb create mode 100644 spec/models/user_passwords/sha1_spec.rb diff --git a/Gemfile b/Gemfile index 0ed66eb88c..6e36ab47fa 100644 --- a/Gemfile +++ b/Gemfile @@ -78,6 +78,8 @@ gem 'ruby-duration', '~> 3.2.0' # provide compatible filesystem information for available storage gem 'sys-filesystem', '~> 1.1.4', require: false +gem 'bcrypt', '~> 3.1.6' + # We rely on this specific version, which is the latest as of now (end of 2013), # because we have to apply to it a bugfix which could break things in other versions. # This can be removed as soon as said bugfix is integrated into rabl itself. diff --git a/Gemfile.lock b/Gemfile.lock index 68639c8e8b..4eb099fd7d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -152,6 +152,7 @@ GEM descendants_tracker (~> 0.0.4) ice_nine (~> 0.11.0) thread_safe (~> 0.3, >= 0.3.1) + bcrypt (3.1.10) binding_of_caller (0.7.2) debug_inspector (>= 0.0.1) bourbon (4.2.6) @@ -589,6 +590,7 @@ DEPENDENCIES airbrake (~> 5.1.0) autoprefixer-rails awesome_nested_set! + bcrypt (~> 3.1.6) bourbon (~> 4.2.0) capybara (~> 2.6.2) capybara-ng (~> 0.2.2) diff --git a/app/controllers/my_controller.rb b/app/controllers/my_controller.rb index 0384ed1e2d..217a93b3fc 100644 --- a/app/controllers/my_controller.rb +++ b/app/controllers/my_controller.rb @@ -103,7 +103,7 @@ class MyController < ApplicationController @user = User.current # required by "my" layout @username = @user.login return if redirect_if_password_change_not_allowed_for(@user) - if @user.check_password?(params[:password]) + if @user.check_password?(params[:password], update_legacy: false) @user.password = params[:new_password] @user.password_confirmation = params[:new_password_confirmation] @user.force_password_change = false diff --git a/app/models/user.rb b/app/models/user.rb index 6f106adff5..e08ec03b57 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -165,7 +165,7 @@ class User < Principal # create new password if password was set def update_password if password && auth_source_id.blank? - new_password = passwords.build + new_password = passwords.build(type: UserPassword.active_type.to_s) new_password.plain_password = password new_password.save @@ -357,12 +357,14 @@ class User < Principal end # Returns true if +clear_password+ is the correct user's password, otherwise false - def check_password?(clear_password) + # If +update_legacy+ is set, will automatically save legacy passwords using the current + # format. + def check_password?(clear_password, update_legacy: true) if auth_source_id.present? auth_source.authenticate(login, clear_password) else return false if current_password.nil? - current_password.same_as_plain_password?(clear_password) + current_password.matches_plaintext?(clear_password, update_legacy: update_legacy) end end @@ -741,7 +743,7 @@ class User < Principal ban_count = Setting[:password_count_former_banned].to_i # make reducing the number of banned former passwords immediately effective # by only checking this number of former passwords - passwords[0, ban_count].any? { |f| f.same_as_plain_password?(password) } + passwords[0, ban_count].any? { |f| f.matches_plaintext?(password) } end def clean_up_former_passwords @@ -840,7 +842,7 @@ class User < Principal end def self.default_admin_account_changed? - !User.active.find_by_login('admin').try(:current_password).try(:same_as_plain_password?, 'admin') + !User.active.find_by_login('admin').try(:current_password).try(:matches_plaintext?, 'admin') end end diff --git a/app/models/user_password.rb b/app/models/user_password.rb index dd9c8fb92a..67446b87c8 100644 --- a/app/models/user_password.rb +++ b/app/models/user_password.rb @@ -35,11 +35,42 @@ class UserPassword < ActiveRecord::Base attr_accessor :plain_password - # Checks whether the stored password is the same as a given plaintext password - def same_as_plain_password?(plain_password) - UserPassword.secure_equals?(UserPassword.hash_with_salt(plain_password, - salt), - hashed_password) + ## + # Fixes the active UserPassword Type to use. + # This could allow for an entrypoint for plugins or customization + def self.active_type + UserPassword::Bcrypt + end + + ## + # Determines whether the hashed value of +plain+ matches the stored password hash. + def matches_plaintext?(plain, update_legacy: true) + if hash_matches?(plain) + + # Update hash if necessary + if update_legacy + rehash_as_active(plain) + end + + return true + end + + false + end + + ## + # Rehash the password using the currently active strategy. + # This replaces the password and keeps expiry date identical. + def rehash_as_active(plain) + active_class = UserPassword.active_type + + unless is_a?(active_class) + becomes!(active_class) + self.hashed_password = derive_password!(plain) + save! + end + rescue => e + Rails.logger.error("Unable to re-hash UserPassword for #{user.login}: #{e.message}") end def expired? @@ -48,39 +79,25 @@ class UserPassword < ActiveRecord::Base created_at < (Time.now - days_valid) end - # Returns a 128bits random salt as a hex string (32 chars long) - def self.generate_salt - SecureRandom.hex(16) - end + protected - # Return password digest - def self.hash_password(plain_password) - Digest::SHA1.hexdigest(plain_password) + # Save hashed_password from the initially passed plain password + # if it is is set. + def salt_and_hash_password! + return if plain_password.nil? + self.hashed_password = derive_password!(plain_password) end - # Hash a plaintext password with a given salt - # The hashed password has following form: SHA1(salt + SHA1(password)) - def self.hash_with_salt(plain_password, salt) - # We should really use a standard key-derivation function like bcrypt here - hash_password("#{salt}#{hash_password plain_password}") + # Require the implementation to provide a secure comparisation + def hash_matches?(_plain) + raise NotImplementedError, 'Must be overridden by subclass' end - # constant-time comparison algorithm to prevent timing attacks - def self.secure_equals?(a, b) - return false if a.blank? || b.blank? || a.bytesize != b.bytesize - l = a.unpack "C#{a.bytesize}" - - res = 0 - b.each_byte do |byte| res |= byte ^ l.shift end - res == 0 + def generate_salt + raise NotImplementedError, 'Must be overridden by subclass' end - private - - def salt_and_hash_password! - return if plain_password.nil? - self.salt = UserPassword.generate_salt - self.hashed_password = UserPassword.hash_with_salt(plain_password, - salt) + def derive_password!(_input) + raise NotImplementedError, 'Must be overridden by subclass' end end diff --git a/app/models/user_password/bcrypt.rb b/app/models/user_password/bcrypt.rb new file mode 100644 index 0000000000..7601b548ae --- /dev/null +++ b/app/models/user_password/bcrypt.rb @@ -0,0 +1,44 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +## +# Password hashing method using bcrypt +class UserPassword::Bcrypt < UserPassword + protected + + ## + # Determines whether the hashed value of +plain+ matches the stored password hash. + def hash_matches?(plain) + BCrypt::Password.new(hashed_password) == plain + end + + def derive_password!(input) + BCrypt::Password.create(input) + end +end diff --git a/app/models/user_password/sha1.rb b/app/models/user_password/sha1.rb new file mode 100644 index 0000000000..91d6515f14 --- /dev/null +++ b/app/models/user_password/sha1.rb @@ -0,0 +1,70 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +## +# LEGACY password hashing method using salted SHA-1 +# This is only included for testing hashed passwords and will raise when trying +# to save new passwords with that strategy. +class UserPassword::SHA1 < UserPassword + protected + + ## + # Determines whether the hashed value of +plain+ matches the stored password hash. + def hash_matches?(plain) + test_hash = derive_password!(plain) + secure_equals?(test_hash, hashed_password) + end + + # constant-time comparison algorithm to prevent timing attacks + def secure_equals?(a, b) + return false if a.blank? || b.blank? || a.bytesize != b.bytesize + l = a.unpack "C#{a.bytesize}" + + res = 0 + b.each_byte do |byte| res |= byte ^ l.shift end + res == 0 + end + + ## + # Override the base method to disallow new passwords being generated this way. + def salt_and_hash_password! + raise ArgumentError, 'Do not use UserPassword::SHA1 for new passwords!' + end + + ## + # Hash a plaintext password with a given salt + # The hashed password has following form: SHA1(salt + SHA1(password)) + def derive_password!(input) + hashfn("#{salt}#{hashfn(input)}") + end + + def hashfn(input) + Digest::SHA1.hexdigest(input) + end +end diff --git a/config/initializers/bcrypt.rb b/config/initializers/bcrypt.rb new file mode 100644 index 0000000000..c649939782 --- /dev/null +++ b/config/initializers/bcrypt.rb @@ -0,0 +1,41 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +if OpenProject::Configuration.override_bcrypt_cost_factor? + cost_factor = OpenProject::Configuration.override_bcrypt_cost_factor.to_i + current = BCrypt::Engine.cost + + if cost_factor < 8 + Rails.logger.warn { + "Ignoring BCrypt cost factor #{cost_factor}. Using default (#{current})." + } + else + BCrypt::Engine.cost = cost_factor + end +end diff --git a/db/migrate/20160829225633_introduce_bcrypt_passwords.rb b/db/migrate/20160829225633_introduce_bcrypt_passwords.rb new file mode 100644 index 0000000000..5987479102 --- /dev/null +++ b/db/migrate/20160829225633_introduce_bcrypt_passwords.rb @@ -0,0 +1,36 @@ +class IntroduceBcryptPasswords < ActiveRecord::Migration + def up + # Introduce type to UserPassword + add_column :user_passwords, :type, :string, null: true + + # Increase hash limit due to bcrypt embedded salt + change_column :user_passwords, :hashed_password, :string, limit: 128, null: false + + # All current passwords are assumed to be SHA-1 salted. + UserPassword.update_all(type: 'UserPassword::SHA1') + + # Make type non-optional + change_column :user_passwords, :type, :string, null: false + + # Make salt explicitly optional + change_column_null :user_passwords, :salt, true + end + + def down + unless ENV['OPENPROJECT_CONFIRM_ROLLBACK'] == '20160829225633' + raise ActiveRecord::IrreversibleMigration, <<-EXC.strip_heredoc + WARNING + + You cannot roll back this migration without losing passwords. + If you really want to do undo BCrypt passwords, set the following ENV variable: + + export OPENPROJECT_CONFIRM_ROLLBACK="20160829225633" + EXC + end + + UserPassword.where(type: 'UserPassword::Bcrypt').delete_all + remove_column :user_passwords, :type + change_column :user_passwords, :hashed_password, :string, limit: 40 + # Salt was (implictly) optional + end +end diff --git a/lib/open_project/configuration.rb b/lib/open_project/configuration.rb index bbceadefca..36d7700de9 100644 --- a/lib/open_project/configuration.rb +++ b/lib/open_project/configuration.rb @@ -84,6 +84,7 @@ module OpenProject 'internal_password_confirmation' => true, 'disable_password_choice' => false, + 'override_bcrypt_cost_factor' => nil, 'disabled_modules' => [], # allow to disable default modules 'hidden_menu_items' => {}, diff --git a/spec/factories/user_password_factory.rb b/spec/factories/user_password_factory.rb index 008c3e7275..d5fd732efa 100644 --- a/spec/factories/user_password_factory.rb +++ b/spec/factories/user_password_factory.rb @@ -27,7 +27,7 @@ #++ FactoryGirl.define do - factory :user_password do + factory :user_password, class: UserPassword.active_type do association :user plain_password 'adminADMIN!' @@ -36,4 +36,18 @@ FactoryGirl.define do updated_at 1.year.ago end end + + factory :legacy_sha1_password, class: UserPassword::SHA1 do + association :user + type 'UserPassword::SHA1' + plain_password 'mylegacypassword!' + + # Avoid going through the after_save hook + # As it's no longer possible for Sha1 passwords + after(:build) do |obj| + obj.salt = SecureRandom.hex(16) + obj.hashed_password = obj.send(:derive_password!, obj.plain_password) + obj.plain_password = nil + end + end end diff --git a/spec/models/user_password_spec.rb b/spec/models/user_password_spec.rb index 308070ad1d..48df691df2 100644 --- a/spec/models/user_password_spec.rb +++ b/spec/models/user_password_spec.rb @@ -30,7 +30,8 @@ require 'spec_helper' describe UserPassword, type: :model do let(:old_password) { FactoryGirl.create(:old_user_password) } - let(:password) { FactoryGirl.create(:user_password) } + let(:user) { FactoryGirl.create(:user) } + let(:password) { FactoryGirl.create(:user_password, user: user, plain_password: 'adminAdmin!') } describe '#expired?' do context 'with expiry value set', @@ -51,4 +52,55 @@ describe UserPassword, type: :model do end end end + + describe '#matches_plaintext?' do + it 'still matches the password' do + expect(password).to be_a(UserPassword.active_type) + expect(password.matches_plaintext?('adminAdmin!')).to be_truthy + end + end + + describe '#rehash_as_active' do + let(:password) { + pass = FactoryGirl.build(:legacy_sha1_password, user: user, plain_password: 'adminAdmin!') + expect(pass).to receive(:salt_and_hash_password!).and_return nil + + pass.save! + pass + } + + before do + password + user.reload + end + + it 'rehashed the password when correct' do + expect(user.current_password).to be_a(UserPassword::SHA1) + expect { + password.matches_plaintext?('adminAdmin!') + }.to_not change { user.passwords.count } + + expect(user.current_password).to be_a(UserPassword::Bcrypt) + end + + it 'does not alter the password when invalid' do + expect(password.matches_plaintext?('wat')).to be false + expect(password).to be_a(UserPassword::SHA1) + end + + it 'does not alter the password when disabled' do + expect(password.matches_plaintext?('adminAdmin!', update_legacy: false)).to be true + expect(user.current_password).to be_a(UserPassword::SHA1) + end + end + + describe '#save' do + let(:password) { FactoryGirl.build(:user_password) } + + it 'saves correctly' do + expect(password).to receive(:salt_and_hash_password!).and_call_original + expect { password.save! }.not_to raise_error + expect(password).not_to be_expired + end + end end diff --git a/spec/models/user_passwords/sha1_spec.rb b/spec/models/user_passwords/sha1_spec.rb new file mode 100644 index 0000000000..566b806284 --- /dev/null +++ b/spec/models/user_passwords/sha1_spec.rb @@ -0,0 +1,54 @@ +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +describe UserPassword::SHA1, type: :model do + let(:legacy_password) { + pass = FactoryGirl.build(:legacy_sha1_password, plain_password: 'adminAdmin!') + expect(pass).to receive(:salt_and_hash_password!).and_return nil + + pass.save! + pass + } + + describe '#matches_plaintext?' do + it 'still matches for existing passwords' do + expect(legacy_password).to be_a(UserPassword::SHA1) + expect(legacy_password.matches_plaintext?('adminAdmin!')).to be_truthy + end + end + + describe '#create' do + let(:legacy_password) { FactoryGirl.build(:legacy_sha1_password) } + + it 'raises an exception trying to save it' do + expect { legacy_password.save! }.to raise_error(ArgumentError) + end + end +end diff --git a/spec_legacy/fixtures/user_passwords.yml b/spec_legacy/fixtures/user_passwords.yml index 60d745fe1f..b7349fe1a6 100644 --- a/spec_legacy/fixtures/user_passwords.yml +++ b/spec_legacy/fixtures/user_passwords.yml @@ -30,6 +30,7 @@ user_passwords_004: created_at: 2006-07-19 19:34:07 +02:00 # password = foo + type: UserPassword::SHA1 salt: 3126f764c3c5ac61cbfc103f25f934cf hashed_password: 9e4dd7eeb172c12a0691a6d9d3a269f7e9fe671b updated_at: 2006-07-19 19:34:07 +02:00 @@ -38,6 +39,7 @@ user_passwords_004: user_passwords_001: created_at: 2006-07-19 19:12:21 +02:00 # password = adminADMIN! + type: UserPassword::SHA1 salt: a19f9743f4b7b043a2fd07b8eb13a1df hashed_password: b4596ce83c0e154e5ce9d3dd5636fde4d6f38b75 updated_at: 2006-07-19 22:57:52 +02:00 @@ -46,6 +48,7 @@ user_passwords_001: user_passwords_002: created_at: 2006-07-19 19:32:09 +02:00 # password = jsmith + type: UserPassword::SHA1 salt: 67eb4732624d5a7753dcea7ce0bb7d7d hashed_password: bfbe06043353a677d0215b26a5800d128d5413bc updated_at: 2006-07-19 22:42:15 +02:00 @@ -54,6 +57,7 @@ user_passwords_002: user_passwords_003: created_at: 2006-07-19 19:33:19 +02:00 # password = foo + type: UserPassword::SHA1 salt: 7599f9963ec07b5a3b55b354407120c0 hashed_password: 8f659c8d7c072f189374edacfa90d6abbc26d8ed updated_at: 2006-07-19 19:33:19 +02:00 @@ -63,28 +67,34 @@ user_passwords_005: id: 5 user_id: 5 created_at: 2006-07-19 19:33:19 +02:00 + type: UserPassword::SHA1 hashed_password: 1 updated_at: 2006-07-19 19:33:19 +02:00 user_passwords_006: id: 6 user_id: 6 created_at: 2006-07-19 19:33:19 +02:00 + type: UserPassword::SHA1 hashed_password: 1 updated_at: 2006-07-19 19:33:19 +02:00 user_passwords_007: id: 7 user_id: 7 + type: UserPassword::SHA1 + hashed_password: 1 created_at: 2006-07-19 19:33:19 +02:00 updated_at: 2006-07-19 19:33:19 +02:00 user_passwords_008: id: 8 user_id: 8 + type: UserPassword::SHA1 created_at: 2006-07-19 19:33:19 +02:00 hashed_password: 1 updated_at: 2006-07-19 19:33:19 +02:00 user_passwords_009: id: 9 user_id: 9 + type: UserPassword::SHA1 created_at: 2006-07-19 19:33:19 +02:00 hashed_password: 1 updated_at: 2006-07-19 19:33:19 +02:00 From 59da2c6175e2e330f6ca933ad8de8456a87efbdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 5 Sep 2016 14:01:06 +0200 Subject: [PATCH 2/3] Actually rehash on the correct object instance `becomes!` does not change the current object (how would it?), so we need to call `hash_password!` on the object returned from it. This also extends the false positive test. The rehash actually happened, but it was hashing the plain as a legacy SHA-1 again. --- app/models/user_password.rb | 16 +++++++++++++--- spec/models/user_password_spec.rb | 1 + 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/models/user_password.rb b/app/models/user_password.rb index 67446b87c8..84af764e0c 100644 --- a/app/models/user_password.rb +++ b/app/models/user_password.rb @@ -65,14 +65,24 @@ class UserPassword < ActiveRecord::Base active_class = UserPassword.active_type unless is_a?(active_class) - becomes!(active_class) - self.hashed_password = derive_password!(plain) - save! + active = becomes!(active_class) + active.rehash_from_legacy(plain) + + active end rescue => e Rails.logger.error("Unable to re-hash UserPassword for #{user.login}: #{e.message}") end + ## + # Actually derive and save the password using +active_type+ + # We require a public interface since +becomes!+ does change the STI instance, + # but returns, not changes the actual current object. + def rehash_from_legacy(plain) + self.hashed_password = derive_password!(plain) + save! + end + def expired? days_valid = Setting.password_days_valid.to_i.days return false if days_valid == 0 diff --git a/spec/models/user_password_spec.rb b/spec/models/user_password_spec.rb index 48df691df2..cff2ae4074 100644 --- a/spec/models/user_password_spec.rb +++ b/spec/models/user_password_spec.rb @@ -81,6 +81,7 @@ describe UserPassword, type: :model do }.to_not change { user.passwords.count } expect(user.current_password).to be_a(UserPassword::Bcrypt) + expect(user.current_password.hashed_password).to start_with '$2a$' end it 'does not alter the password when invalid' do From 833b79645bd5427d7cac55131135cf366674f504 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 5 Sep 2016 14:34:21 +0200 Subject: [PATCH 3/3] Remove unused method --- app/models/user_password.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/user_password.rb b/app/models/user_password.rb index 84af764e0c..eb192beea3 100644 --- a/app/models/user_password.rb +++ b/app/models/user_password.rb @@ -103,10 +103,6 @@ class UserPassword < ActiveRecord::Base raise NotImplementedError, 'Must be overridden by subclass' end - def generate_salt - raise NotImplementedError, 'Must be overridden by subclass' - end - def derive_password!(_input) raise NotImplementedError, 'Must be overridden by subclass' end