Open guilleiguaran opened 1 year ago
Hey @admsev , thanks for summoning me!
BCrypt Integration:
bcrypt
as a dependency.min_bcrypt_cost
to control the BCrypt cost factor.Class and Instance Methods:
has_one_time_password
to accept a backup_codes_encrypted
option.otp_regenerate_backup_codes
to encrypt backup codes if backup_codes_encrypted
is true.authenticate_backup_code
to handle encrypted backup codes.Test Enhancements:
UserWithEncryptedCodes
.Bug in Test:
test_authenticate_with_encrypted_backup_code
, the last assertion uses @user
instead of @user_with_encrypted_code
.Improved Code:
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_with_encrypted_code.authenticate_otp(backup_code)
end
Potential Issue with otp_regenerate_backup_codes
:
backup_codes
is reassigned, which might cause confusion. It's better to use a different variable name for the encrypted codes.Improved Code:
def otp_regenerate_backup_codes
backup_codes = Array.new(self.class.otp_backup_codes_count) do
otp.generate_otp((SecureRandom.random_number(9e5) + 1e5).to_i)
end
if self.class.otp_backup_codes_encrypted
self.plain_backup_codes = backup_codes
encrypted_backup_codes = backup_codes.map do |code|
cost = ActiveModel::OneTimePassword.min_bcrypt_cost ? BCrypt::Engine::MIN_COST : BCrypt::Engine.cost
BCrypt::Password.create(code, cost: cost)
end
backup_codes = encrypted_backup_codes
end
public_send("#{self.class.otp_backup_codes_column_name}=", backup_codes)
end
Code Quality:
class_attribute
and module
is appropriate for defining class-level and instance-level attributes.Code Style:
self.class
and public_send
is appropriate for dynamic method calls and accessing class attributes.Tests:
The pull request introduces a valuable feature by adding encryption for backup codes using BCrypt. The implementation is clean, and the tests ensure that the feature works as expected. The minor issues identified can be easily fixed to improve the robustness of the code.
Yours, Gooroo.dev
@gooroodev can you review this PR please?