Use BCrypt to encrypt backup codes

[guilleiguaran]

No description provided.

[houndci-bot]

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

[houndci-bot]

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

[houndci-bot]

Style/RedundantSelf: Redundant self detected.

[houndci-bot]

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

[houndci-bot]

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

[houndci-bot]

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

[houndci-bot]

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

[admsev]

@gooroodev can you review this PR please?

[gooroodev]

Hey @admsev , thanks for summoning me!

Summary of Changes

1. BCrypt Integration:

   • Added bcrypt as a dependency.
   • Introduced min_bcrypt_cost to control the BCrypt cost factor.
   • Added an option to encrypt backup codes with BCrypt.

2. Class and Instance Methods:

   • Added class-level attribute for BCrypt cost control.
   • Modified has_one_time_password to accept a backup_codes_encrypted option.
   • Modified otp_regenerate_backup_codes to encrypt backup codes if backup_codes_encrypted is true.
   • Modified authenticate_backup_code to handle encrypted backup codes.

3. Test Enhancements:

   • Added a new test model UserWithEncryptedCodes.
   • Added tests for encrypted backup codes.

Issues, Bugs, and Typos

1. Bug in Test:

   • In 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

2. Potential Issue with otp_regenerate_backup_codes:

   • The variable 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

General Review of Code Quality and Style

1. Code Quality:

   • The code is well-structured and follows good practices.
   • The use of class_attribute and module is appropriate for defining class-level and instance-level attributes.

2. Code Style:

   • The code adheres to Ruby conventions, making it readable and maintainable.
   • The use of self.class and public_send is appropriate for dynamic method calls and accessing class attributes.

3. Tests:

   • The tests are comprehensive and cover both encrypted and non-encrypted backup codes.
   • The addition of a new test model for encrypted codes is a good practice to isolate test cases.

Overall

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