Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mash up rolling option, PBKDF2 support and support for rolling from Devise's default #26

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

* Support Rails 4.1 - 7.0, dropped support for Rails <= 4.0. (same support as Devise as of v4.8)
* Support Ruby 2.1 - 3.1, dropped support for Ruby <= 2.0. (same support as Devise as of v4.8)
* Add support to roll legacy encryptors
* Add support for rolling from Devise BCrypt

### 0.2.0

Expand Down
8 changes: 7 additions & 1 deletion lib/devise/encryptable/encryptable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,22 @@ module Devise
mattr_accessor :encryptor
@@encryptor = nil

# Used to define a legacy algorithm that needs to be rolled to a new one.
mattr_accessor :transition_from_encryptor
@@transition_from_encryptor = nil

module Encryptable
module Encryptors
autoload :AuthlogicSha512, 'devise/encryptable/encryptors/authlogic_sha512'
autoload :Base, 'devise/encryptable/encryptors/base'
autoload :ClearanceSha1, 'devise/encryptable/encryptors/clearance_sha1'
autoload :DeviseBcrypt, 'devise/encryptable/encryptors/devise_bcrypt'
autoload :Pbkdf2, 'devise/encryptable/encryptors/pbkdf2'
autoload :RestfulAuthenticationSha1, 'devise/encryptable/encryptors/restful_authentication_sha1'
autoload :Sha1, 'devise/encryptable/encryptors/sha1'
autoload :Sha512, 'devise/encryptable/encryptors/sha512'
end
end
end

Devise.add_module(:encryptable, :model => 'devise/encryptable/model')
Devise.add_module(:encryptable, :model => 'devise/encryptable/model')
33 changes: 33 additions & 0 deletions lib/devise/encryptable/encryptors/devise_bcrypt.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
require 'bcrypt'

begin
module Devise
module Encryptable
module Encryptors
# Adapted from
# https://github.com/heartcombo/devise/blob/8593801130f2df94a50863b5db535c272b00efe1/lib/devise/encryptor.rb
class DeviseBcrypt < Base
def self.compare(hashed_password, password, stretches, _salt, pepper)
return false if hashed_password.blank?
bcrypt = ::BCrypt::Password.new(hashed_password)
if pepper.present?
password = "#{password}#{pepper}"
end
password = ::BCrypt::Engine.hash_secret(password, bcrypt.salt)
Devise.secure_compare(password, hashed_password)
rescue BCrypt::Errors::InvalidHash
# this probably means the password has already been migrated
false
end

def self.digest(password, stretches, _salt, pepper)
if pepper.present?
password = "#{password}#{pepper}"
end
::BCrypt::Password.create(password, cost: stretches).to_s
end
end
end
end
end
end
25 changes: 25 additions & 0 deletions lib/devise/encryptable/encryptors/pbkdf2.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
begin
module Devise
module Encryptable
module Encryptors
class Pbkdf2 < Base
def self.compare(encrypted_password, password, stretches, salt, pepper)
value_to_test = self.digest(password, stretches, salt, pepper)
Devise.secure_compare(encrypted_password, value_to_test)
end

def self.digest(password, stretches, salt, pepper)
hash = OpenSSL::Digest.new('SHA512').new
OpenSSL::KDF.pbkdf2_hmac(
password.to_s,
salt: "#{[salt].pack('H*')}#{pepper}",
iterations: stretches,
hash: hash,
length: hash.digest_length,
).unpack1('H*')
end
end
end
end
end
end
54 changes: 44 additions & 10 deletions lib/devise/encryptable/model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,13 @@ module Models
#
# * +encryptor+: the encryptor going to be used. By default is nil.
#
# * +transition_from_encryptor+: the legacy encryptor that needs to rolled to the +encryptor+.
#
# == Examples
#
# User.find(1).valid_password?('password123') # returns true/false
# User.find(1).valid_password?('password123')
# #=> returns true/false
# #=> if true and using a legacy encryptor, it will update the user to the new encryptor.
#
module Encryptable
extend ActiveSupport::Concern
Expand All @@ -38,7 +42,21 @@ def password=(new_password)
# Validates the password considering the salt.
def valid_password?(password)
return false if encrypted_password.blank?
encryptor_class.compare(encrypted_password, password, self.class.stretches, authenticatable_salt, self.class.pepper)

encryptor_arguments = [
encrypted_password,
password,
self.class.stretches,
authenticatable_salt,
self.class.pepper
]

if transition_from_encryptor_class.try(:compare, *encryptor_arguments)
update_attribute(:password, password)
return true
end

encryptor_class.compare(*encryptor_arguments)
end

# Overrides authenticatable salt to use the new password_salt
Expand All @@ -62,24 +80,40 @@ def encryptor_class
self.class.encryptor_class
end

def transition_from_encryptor_class
self.class.transition_from_encryptor_class
end

module ClassMethods
Devise::Models.config(self, :encryptor)
Devise::Models.config(self, :transition_from_encryptor)

# Returns the class for the configured encryptor.
def encryptor_class
@encryptor_class ||= case encryptor
when :bcrypt
raise "In order to use bcrypt as encryptor, simply remove :encryptable from your devise model"
when nil
raise "You need to give an :encryptor as option in order to use :encryptable"
else
Devise::Encryptable::Encryptors.const_get(encryptor.to_s.classify)
end
@encryptor_class ||= compute_encryptor_class(encryptor)
end

# Returns the class for the configured transition from encryptor.
def transition_from_encryptor_class
@transition_from_encryptor_class ||= compute_encryptor_class(transition_from_encryptor) if transition_from_encryptor
end

def password_salt
self.encryptor_class.salt(self.stretches)
end

private

def compute_encryptor_class(encryptor)
case encryptor
when :bcrypt
raise "In order to use bcrypt as encryptor, simply remove :encryptable from your devise model"
when nil
raise "You need to give an :encryptor as option in order to use :encryptable"
else
Devise::Encryptable::Encryptors.const_get(encryptor.to_s.classify)
end
end
end
end
end
Expand Down
15 changes: 15 additions & 0 deletions test/devise/encryptable/encryptable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,21 @@ def encrypt_password(admin, pepper=Admin.pepper, stretches=Admin.stretches, encr
assert_not_equal encrypted_password, admin.encrypted_password
end

test 'should roll from legacy encryptor to the new one' do
admin = nil

swap_with_encryptor Admin, :sha1 do
admin = create_admin
assert admin.valid_password?('123456')
assert_equal admin.encrypted_password, encrypt_password(admin, Admin.pepper, Admin.stretches, Devise::Encryptable::Encryptors::Sha1)
end

swap_with_encryptor Admin, :sha512, transition_from_encryptor: :sha1 do
assert admin.valid_password?('123456')
assert_equal admin.encrypted_password, encrypt_password(admin, Admin.pepper, Admin.stretches, Devise::Encryptable::Encryptors::Sha512)
end
end

test 'should respect encryptor configuration' do
swap_with_encryptor Admin, :sha512 do
admin = create_admin
Expand Down
39 changes: 39 additions & 0 deletions test/devise/encryptable/encryptors_test.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require "test_helper"
require "ostruct"

class Encryptors < ActiveSupport::TestCase
include Support::Swappers
Expand All @@ -21,6 +22,44 @@ class Encryptors < ActiveSupport::TestCase
assert_equal clearance, encryptor
end

test 'Devise can verify passwords generated by DeviseBcrypt' do
password = '123mudar'
klass = OpenStruct.new(pepper: '65c58472c207c829f28c68619d3e3aefed18ab3f', stretches: 10)

hashed_password = Devise::Encryptable::Encryptors::DeviseBcrypt.digest(password, klass.stretches, nil, klass.pepper)

assert Devise::Encryptor.compare(klass, hashed_password, password)
end

test 'DeviseBcrypt can verify bcrypt hashes created by Devise' do
password = '123mudar'
klass = OpenStruct.new(pepper: '65c58472c207c829f28c68619d3e3aefed18ab3f', stretches: 10)

devise_hash = Devise::Encryptor.digest(klass, password)

assert Devise::Encryptable::Encryptors::DeviseBcrypt.compare(devise_hash, password, klass.stretches, nil, klass.pepper)
end

test 'Devise fails to verify passwords generated by DeviseBcrypt when the password is wrong' do
password = '123mudar'
different_password = 'theskyisfalling123'
klass = OpenStruct.new(pepper: '65c58472c207c829f28c68619d3e3aefed18ab3f', stretches: 10)

hashed_password = Devise::Encryptable::Encryptors::DeviseBcrypt.digest(password, klass.stretches, nil, klass.pepper)

refute Devise::Encryptor.compare(klass, hashed_password, different_password)
end

test 'DeviseBcrypt fails to verify bcrypt hashes created by Devise when the password is wrong' do
password = '123mudar'
different_password = 'theskyisfalling123'
klass = OpenStruct.new(pepper: '65c58472c207c829f28c68619d3e3aefed18ab3f', stretches: 10)

devise_hash = Devise::Encryptor.digest(klass, password)

refute Devise::Encryptable::Encryptors::DeviseBcrypt.compare(devise_hash, different_password, klass.stretches, nil, klass.pepper)
end

test 'digest should raise NotImplementedError if not implemented in subclass' do
c = Class.new(Devise::Encryptable::Encryptors::Base)
assert_raise(NotImplementedError) do
Expand Down
1 change: 1 addition & 0 deletions test/support/swappers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module Support
module Swappers
def swap_with_encryptor(klass, encryptor, options={})
klass.instance_variable_set(:@encryptor_class, nil)
klass.instance_variable_set(:@transition_from_encryptor, nil)

swap klass, options.merge(:encryptor => encryptor) do
begin
Expand Down