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

Use BCrypt to encrypt backup codes #111

Open
wants to merge 1 commit 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
1 change: 1 addition & 0 deletions active_model_otp.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Gem::Specification.new do |spec|
spec.required_ruby_version = ">= 2.3"

spec.add_dependency "activemodel"
spec.add_dependency "bcrypt", "~> 3.1"
spec.add_dependency "rotp", "~> 6.2.0"

spec.add_development_dependency "activerecord"
Expand Down
34 changes: 30 additions & 4 deletions lib/active_model/one_time_password.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,27 @@ module ActiveModel
module OneTimePassword
extend ActiveSupport::Concern

class << self
attr_accessor :min_bcrypt_cost # :nodoc:
end
self.min_bcrypt_cost = false

OTP_DEFAULT_COLUMN_NAME = 'otp_secret_key'.freeze
OTP_DEFAULT_COUNTER_COLUMN_NAME = 'otp_counter'.freeze
OTP_DEFAULT_BACKUP_CODES_COLUMN_NAME = 'otp_backup_codes'.freeze
OTP_DEFAULT_DIGITS = 6
OTP_DEFAULT_BACKUP_CODES_COUNT = 12
OTP_COUNTER_ENABLED_BY_DEFAULT = false
OTP_BACKUP_CODES_ENABLED_BY_DEFAULT = false
OTP_BACKUP_CODES_ENCRYPTED_BY_DEFAULT = true

module ClassMethods
def has_one_time_password(options = {})
cattr_accessor :otp_column_name, :otp_counter_column_name,
:otp_backup_codes_column_name, :otp_after_column_name
class_attribute :otp_digits, :otp_counter_based,
:otp_backup_codes_count, :otp_one_time_backup_codes,
:otp_interval
:otp_interval, :otp_backup_codes_encrypted

self.otp_column_name = (options[:column_name] || OTP_DEFAULT_COLUMN_NAME).to_s
self.otp_digits = options[:length] || OTP_DEFAULT_DIGITS
Expand All @@ -27,13 +33,14 @@ def has_one_time_password(options = {})
self.otp_backup_codes_column_name = (options[:backup_codes_column_name] || OTP_DEFAULT_BACKUP_CODES_COLUMN_NAME).to_s
self.otp_backup_codes_count = options[:backup_codes_count] || OTP_DEFAULT_BACKUP_CODES_COUNT
self.otp_one_time_backup_codes = options[:one_time_backup_codes] || OTP_BACKUP_CODES_ENABLED_BY_DEFAULT
self.otp_backup_codes_encrypted = options.fetch(:backup_codes_encrypted, OTP_BACKUP_CODES_ENCRYPTED_BY_DEFAULT)

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [119/80]


include InstanceMethodsOnActivation

before_create(**options.slice(:if, :unless)) do
self.otp_regenerate_secret if !otp_column
self.otp_regenerate_counter if otp_counter_based && !otp_counter
otp_regenerate_backup_codes if backup_codes_enabled?
self.otp_regenerate_backup_codes if backup_codes_enabled?

Choose a reason for hiding this comment

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

Style/RedundantSelf: Redundant self detected.

end

if respond_to?(:attributes_protected_by_default)
Expand All @@ -51,6 +58,8 @@ def otp_random_secret(length = 20)
end

module InstanceMethodsOnActivation
attr_accessor :plain_backup_codes

def otp_regenerate_secret
self.otp_column = self.class.otp_random_secret
end
Expand Down Expand Up @@ -127,6 +136,15 @@ def otp_regenerate_backup_codes
otp.generate_otp((SecureRandom.random_number(9e5) + 1e5).to_i)
end

if self.class.otp_backup_codes_encrypted
self.plain_backup_codes = backup_codes

backup_codes = backup_codes.map do |code|
cost = ActiveModel::OneTimePassword.min_bcrypt_cost ? BCrypt::Engine::MIN_COST : BCrypt::Engine.cost

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [112/80]

BCrypt::Password.create(code, cost: cost)
end
end

public_send("#{self.class.otp_backup_codes_column_name}=", backup_codes)
end

Expand Down Expand Up @@ -180,10 +198,18 @@ def totp_code(options = {})
def authenticate_backup_code(code)
backup_codes_column_name = self.class.otp_backup_codes_column_name
backup_codes = public_send(backup_codes_column_name)
return false unless backup_codes.present? && backup_codes.include?(code)

return false unless backup_codes.present?

valid_code = backup_codes.find do |backup_code|
backup_code = BCrypt::Password.new(backup_code) if self.class.otp_backup_codes_encrypted

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [98/80]

backup_code == code
end

return false unless valid_code

if self.class.otp_one_time_backup_codes
backup_codes.delete(code)
backup_codes.delete(valid_code)
public_send("#{backup_codes_column_name}=", backup_codes)
save if respond_to?(:changed?) && !new_record?
end
Expand Down
1 change: 1 addition & 0 deletions lib/active_model_otp.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require "active_model"
require "active_support/core_ext/module/attribute_accessors"
require "bcrypt"

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

require "cgi"
require "rotp"
require "active_model/one_time_password"
Expand Down
2 changes: 1 addition & 1 deletion test/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class User
define_model_callbacks :create
attr_accessor :otp_secret_key, :otp_backup_codes, :email

has_one_time_password one_time_backup_codes: true
has_one_time_password one_time_backup_codes: true, backup_codes_encrypted: false

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [82/80]


def attributes
{ "otp_secret_key" => otp_secret_key, "email" => email }
Expand Down
11 changes: 11 additions & 0 deletions test/models/user_with_encrypted_codes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class UserWithEncryptedCodes

Choose a reason for hiding this comment

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

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

extend ActiveModel::Callbacks
include ActiveModel::Serializers::JSON
include ActiveModel::Validations
include ActiveModel::OneTimePassword

define_model_callbacks :create
attr_accessor :otp_secret_key, :otp_backup_codes, :email

has_one_time_password backup_codes_encrypted: true
end
17 changes: 15 additions & 2 deletions test/one_time_password_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ def setup
@after_user = AfterUser.new
@after_user.email = '[email protected]'
@after_user.run_callbacks :create

@user_with_encrypted_code = UserWithEncryptedCodes.new
@user_with_encrypted_code.email = '[email protected]'
@user_with_encrypted_code.run_callbacks :create
end

def test_authenticate_with_otp
Expand Down Expand Up @@ -112,13 +116,22 @@ def test_authenticate_with_backup_code

backup_code = @user.public_send(@user.otp_backup_codes_column_name).last
@user.otp_regenerate_backup_codes
assert_equal true, [email protected]_otp(backup_code)
assert_equal false, @user.authenticate_otp(backup_code)
end

def test_authenticate_with_encrypted_backup_code
backup_code = @user_with_encrypted_code.plain_backup_codes.first
assert_equal true, @user_with_encrypted_code.authenticate_otp(backup_code)

backup_code = @user_with_encrypted_code.plain_backup_codes.last
@user_with_encrypted_code.otp_regenerate_backup_codes
assert_equal false, @user.authenticate_otp(backup_code)
end

def test_authenticate_with_one_time_backup_code
backup_code = @user.public_send(@user.otp_backup_codes_column_name).first
assert_equal true, @user.authenticate_otp(backup_code)
assert_equal true, !@user.authenticate_otp(backup_code)
assert_equal false, @user.authenticate_otp(backup_code)
end

def test_otp_code
Expand Down
2 changes: 2 additions & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,6 @@
ActiveRecord::Base.establish_connection adapter: "sqlite3", database: ":memory:"
load "#{ File.dirname(__FILE__) }/schema.rb"

ActiveModel::OneTimePassword.min_bcrypt_cost = true

Dir["models/*.rb"].each {|file| require file }