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

Make has_secure_password pluginable/extendable with different password hashing algorithms #48993

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
34 changes: 34 additions & 0 deletions activemodel/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,37 @@
* `has_secure_password` can support different password hashing algorithms (if defined) using the `:algorithm` option:

```ruby
module ActiveModel
module SecurePassword
class MyAlgo < Base
def self.algorithm_name
:my_algo
end

def hash_password(unencrypted_password, options = {})
SomeHashingLib.create_password_hash(unencrypted_password)
end

def verify_password(password, digest)
SomeHashingLib.verify_password_against_hash(password, digest)
end

def password_salt(digest)
SomeHashingLib.get_salt_from_hash(digest)
end
end
end
end
```

```ruby
class User < ActiveRecord::Base
has_secure_password algorithm: :my_algo
end
```

*Justin Bull*

* Error.full_message now strips ":base" from the message.

*zzak*
Expand Down
63 changes: 36 additions & 27 deletions activemodel/lib/active_model/secure_password.rb
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
# frozen_string_literal: true

require "active_model/secure_password/base"

module ActiveModel
module SecurePassword
extend ActiveSupport::Concern

# BCrypt hash function can handle maximum 72 bytes, and if we pass
# password of length more than 72 bytes it ignores extra characters.
# Hence need to put a restriction on password length.
MAX_PASSWORD_LENGTH_ALLOWED = 72

class << self
attr_accessor :min_cost # :nodoc:
attr_accessor :available_password_algorithms # :nodoc:
end
self.min_cost = false
# TODO: is this the best pattern to discover/register password hashing algo classes?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say the "Rails way" would be to make this configurable so one can write something like

config.active_model.secure_password_algorithms << :some_algo

or

config.active_model.secure_password_algorithms << 'SomeAlgo'

The default value of that option would then be just all Algorithms that are supported by default (only BCrypt for now?).

An example of how to add configurable options can be seen in: #41158 (or just search for something like "add config" in the commit history, there are plenty of examples)

self.available_password_algorithms = ActiveModel::SecurePassword::Base.subclasses.index_by(&:algorithm_name)

module ClassMethods
# Adds methods to set and authenticate against a BCrypt password.
Expand Down Expand Up @@ -54,6 +54,7 @@ module ClassMethods
# end
#
# user = User.new(name: "david", password: "", password_confirmation: "nomatch")
# user.password_algorithm # => :bcrypt
#
# user.save # => false, password required
# user.password = "vr00m"
Expand All @@ -78,6 +79,16 @@ module ClassMethods
# user.authenticate("vr00m") # => false, old password
# user.authenticate("nohack4u") # => user
#
# ===== Specifying a different password hashing algorithm (if implemented)
#
# class User < ActiveRecord::Base
# has_secure_password algorithm: :argon2
# end
#
# user = User.new(name: "david", password: "", password_confirmation: "nomatch")
# user.password_algorithm # => ":argon2"
# user.password_digest # => "$argon2id$v=19$m=65536,t=2,p=1$IFZDudguqDQCy2UYgMJ9AQ$efwmRCjPjPRdR4mVkidkGhFmODe0tY5bwgvHYjbQte8"
#
# ===== Conditionally requiring a password
#
# class Account
Expand All @@ -98,18 +109,12 @@ module ClassMethods
# account.is_guest = true
# account.valid? # => true
#
def has_secure_password(attribute = :password, validations: true)
# Load bcrypt gem only when has_secure_password is used.
# This is to avoid ActiveModel (and by extension the entire framework)
# being dependent on a binary library.
begin
require "bcrypt"
rescue LoadError
warn "You don't have bcrypt installed in your application. Please add it to your Gemfile and run bundle install."
raise
end
def has_secure_password(attribute = :password, validations: true, algorithm: :bcrypt)
password_hasher_klass = ActiveModel::SecurePassword.available_password_algorithms[algorithm]
raise NotImplementedError, "Unsupported password hashing algorithm '#{algorithm}'." if password_hasher_klass.nil?

include InstanceMethodsOnActivation.new(attribute)
password_hasher = password_hasher_klass.new
include InstanceMethodsOnActivation.new(attribute, password_hasher)

if validations
include ActiveModel::Validations
Expand All @@ -126,18 +131,15 @@ def has_secure_password(attribute = :password, validations: true)
if challenge = record.public_send(:"#{attribute}_challenge")
digest_was = record.public_send(:"#{attribute}_digest_was") if record.respond_to?(:"#{attribute}_digest_was")

unless digest_was.present? && BCrypt::Password.new(digest_was).is_password?(challenge)
unless digest_was.present? && password_hasher.verify_password(challenge, digest_was)
record.errors.add(:"#{attribute}_challenge")
end
end
end

# Validates that the password does not exceed the maximum allowed bytes for BCrypt (72 bytes).
# Performs password hashing algorthim-specific validations (such as a max input size)
validate do |record|
password_value = record.public_send(attribute)
if password_value.present? && password_value.bytesize > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED
record.errors.add(attribute, :password_too_long)
end
password_hasher.validate(record, attribute)
end

validates_confirmation_of attribute, allow_blank: true
Expand All @@ -146,7 +148,7 @@ def has_secure_password(attribute = :password, validations: true)
end

class InstanceMethodsOnActivation < Module
def initialize(attribute)
def initialize(attribute, password_hasher)
attr_reader attribute

define_method("#{attribute}=") do |unencrypted_password|
Expand All @@ -155,8 +157,8 @@ def initialize(attribute)
self.public_send("#{attribute}_digest=", nil)
elsif !unencrypted_password.empty?
instance_variable_set("@#{attribute}", unencrypted_password)
cost = ActiveModel::SecurePassword.min_cost ? BCrypt::Engine::MIN_COST : BCrypt::Engine.cost
self.public_send("#{attribute}_digest=", BCrypt::Password.create(unencrypted_password, cost: cost))
password_digest = password_hasher.hash_password(unencrypted_password, min_cost: ActiveModel::SecurePassword.min_cost)
self.public_send("#{attribute}_digest=", password_digest)
end
end

Expand All @@ -174,13 +176,20 @@ def initialize(attribute)
# user.authenticate_password('mUc3m00RsqyRe') # => user
define_method("authenticate_#{attribute}") do |unencrypted_password|
attribute_digest = public_send("#{attribute}_digest")
attribute_digest.present? && BCrypt::Password.new(attribute_digest).is_password?(unencrypted_password) && self
return false unless attribute_digest.present?
password_hasher.verify_password(unencrypted_password, attribute_digest) && self
end

# Returns the salt, a small chunk of random data added to the password before it's hashed.
define_method("#{attribute}_salt") do
attribute_digest = public_send("#{attribute}_digest")
attribute_digest.present? ? BCrypt::Password.new(attribute_digest).salt : nil
return unless attribute_digest.present?
password_hasher.password_salt(attribute_digest)
end

define_method("#{attribute}_algorithm") do
# TODO: Should this instead parse the digest to determine which hashing algo?
password_hasher.algorithm_name
end

alias_method :authenticate, :authenticate_password if attribute == :password
Expand Down
33 changes: 33 additions & 0 deletions activemodel/lib/active_model/secure_password/base.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# frozen_string_literal: true

module ActiveModel
module SecurePassword
class Base
def self.algorithm_name
raise NotImplementedError
end

def algorithm_name
self.class.algorithm_name
end

def validate(record)
nil
end

def hash_password(unencrypted_password, options = {})
raise NotImplementedError
end

def verify_password(password, digest)
raise NotImplementedError
end

def password_salt(digest)
raise NotImplementedError
end
end
end
end

require "active_model/secure_password/bcrypt"
46 changes: 46 additions & 0 deletions activemodel/lib/active_model/secure_password/bcrypt.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# frozen_string_literal: true

module ActiveModel
module SecurePassword
class BCrypt < Base
# BCrypt hash function can handle maximum 72 bytes, and if we pass
# password of length more than 72 bytes it ignores extra characters.
# Hence need to put a restriction on password length.
MAX_PASSWORD_LENGTH_ALLOWED = 72

# Load bcrypt gem only when has_secure_password is used.
# This is to avoid ActiveModel (and by extension the entire framework)
# being dependent on a binary library.
def initialize
require "bcrypt"
rescue LoadError
warn "You don't have bcrypt installed in your application. Please add it to your Gemfile and run bundle install."
raise
end

def self.algorithm_name
:bcrypt
end

def validate(record, attribute)
password_value = record.public_send(attribute)
if password_value.present?
record.errors.add(attribute, :password_too_long) if password_value.bytesize > MAX_PASSWORD_LENGTH_ALLOWED
end
end

def hash_password(unencrypted_password, options = {})
cost = options[:min_cost] ? ::BCrypt::Engine::MIN_COST : ::BCrypt::Engine.cost
::BCrypt::Password.create(unencrypted_password, cost: cost)
end

def verify_password(password, digest)
::BCrypt::Password.new(digest).is_password?(password)
end

def password_salt(digest)
::BCrypt::Password.new(digest).salt
end
end
end
end
51 changes: 51 additions & 0 deletions activemodel/test/cases/secure_password_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,33 @@
require "models/user"
require "models/visitor"

class SomeAlgo < ActiveModel::SecurePassword::Base
def self.algorithm_name
:my_algo
end

def hash_password(unencrypted_password, options = {})
"some_digest_for_#{unencrypted_password}"
end

def verify_password(password, digest)
"some_digest_for_#{password}" == digest
end

def password_salt(digest)
"some_salt_from_#{digest}"
end
end

class SecurePasswordTest < ActiveModel::TestCase
setup do
# Used only to speed up tests
@original_min_cost = ActiveModel::SecurePassword.min_cost
ActiveModel::SecurePassword.min_cost = true

@original_algos = ActiveModel::SecurePassword.available_password_algorithms
ActiveModel::SecurePassword.available_password_algorithms[:my_algo] = SomeAlgo

@user = User.new
@visitor = Visitor.new

Expand All @@ -21,6 +42,7 @@ class SecurePasswordTest < ActiveModel::TestCase

teardown do
ActiveModel::SecurePassword.min_cost = @original_min_cost
ActiveModel::SecurePassword.available_password_algorithms = @original_algos
end

test "automatically include ActiveModel::Validations when validations are enabled" do
Expand Down Expand Up @@ -290,6 +312,10 @@ class SecurePasswordTest < ActiveModel::TestCase
assert_nil @user.password_salt
end

test "password_algorithm" do
assert_equal @user.password_algorithm, :bcrypt
end

test "Password digest cost defaults to bcrypt default cost when min_cost is false" do
ActiveModel::SecurePassword.min_cost = false

Expand All @@ -314,4 +340,29 @@ class SecurePasswordTest < ActiveModel::TestCase
@user.password = "secret"
assert_equal BCrypt::Engine::MIN_COST, @user.password_digest.cost
end

test "Specifying a different algorithm utilizes corresponding class" do
user_with_diff_algo = Class.new(User) do
has_secure_password algorithm: :my_algo
end.new

user_with_diff_algo.password = "secret"
assert_equal "some_digest_for_secret", user_with_diff_algo.password_digest

assert_equal false, user_with_diff_algo.authenticate("wrong")
assert_equal user_with_diff_algo, user_with_diff_algo.authenticate("secret")

assert_equal false, user_with_diff_algo.authenticate_password("wrong")
assert_equal user_with_diff_algo, user_with_diff_algo.authenticate_password("secret")

assert_equal "some_salt_from_some_digest_for_secret", user_with_diff_algo.password_salt
end

test "Specifying unknown algorithm name raises NotImplementedError" do
assert_raise NotImplementedError do
Class.new(User) do
has_secure_password algorithm: :some_hashing_name
end.new
end
end
end