Follow up - ef546f54 - Fixes #4620 - Add recovery code hashing for saving in database.

Co-authored-by: Dominik Klein <dk@zammad.com>
Co-authored-by: Florian Liebe <fl@zammad.com>

lib/auth/two_factor/recovery_codes.rb

@@ -9,9 +9,10 @@ class Auth::TwoFactor::RecoveryCodes < Auth::TwoFactor::Method
 
     configuration = user_two_factor_preference_configuration
 
-    return verify_result(false) if configuration[:codes].exclude?(code)
+    hashed_code = configuration[:codes].detect { |saved_code| PasswordHash.verified?(saved_code, code) }
+    return verify_result(false) if hashed_code.blank?
 
-    configuration[:codes].delete(code)
+    configuration[:codes].delete(hashed_code)
 
     verify_result(true, new_configuration: configuration)
   end
@@ -24,10 +25,12 @@ class Auth::TwoFactor::RecoveryCodes < Auth::TwoFactor::Method
       codes << SecureRandom.hex(CODE_LENGTH / 2)
     end
 
+    hashed_codes = codes.map { |code| PasswordHash.crypt(code) }
+
     if exists?
-      update_user_config({ codes: codes })
+      update_user_config({ codes: hashed_codes })
     else
-      create_user_config({ codes: codes })
+      create_user_config({ codes: hashed_codes })
     end
 
     codes

spec/factories/user/two_factor_preference.rb

@@ -150,7 +150,7 @@ FactoryBot.define do
 
       configuration do
         {
-          codes: [recovery_code],
+          codes: [PasswordHash.crypt(recovery_code)],
         }
       end
     end

spec/lib/auth/two_factor/recovery_codes_spec.rb

@@ -24,10 +24,12 @@ RSpec.describe Auth::TwoFactor::RecoveryCodes, current_user_id: 1 do
 
   describe '#verify' do
     let(:current_codes) { instance.generate }
-    let(:code) { current_codes.first }
+    let(:current_hashed_codes) { user.reload.two_factor_preferences.recovery_codes.configuration[:codes] }
+    let(:code)                 { current_codes.first }
 
     before do
       current_codes
+      current_hashed_codes
     end
 
     context 'when code is correct' do
@@ -46,6 +48,7 @@ RSpec.describe Auth::TwoFactor::RecoveryCodes, current_user_id: 1 do
 
       before do
         current_codes
+        current_hashed_codes
       end
 
       it 'returns false' do
@@ -55,7 +58,7 @@ RSpec.describe Auth::TwoFactor::RecoveryCodes, current_user_id: 1 do
       it 'current code list is untouched' do
         instance.verify(code)
 
-        expect(user.reload.two_factor_preferences.find_by(method: 'recovery_codes').configuration[:codes]).to eq(current_codes)
+        expect(user.reload.two_factor_preferences.recovery_codes.configuration[:codes]).to eq(current_hashed_codes)
       end
     end
   end
@@ -72,20 +75,22 @@ RSpec.describe Auth::TwoFactor::RecoveryCodes, current_user_id: 1 do
     it 'codes are saved in two factor preferences' do
       instance.generate
 
-      expect(user.reload.two_factor_preferences.find_by(method: 'recovery_codes').configuration[:codes].length).to eq(described_class.const_get(:NUMBER_OF_CODES))
+      expect(user.reload.two_factor_preferences.recovery_codes.configuration[:codes].length).to eq(described_class.const_get(:NUMBER_OF_CODES))
     end
 
     context 'when codes already exist' do
-      let(:current_codes) { instance.generate }
+      let(:current_codes)        { instance.generate }
+      let(:current_hashed_codes) { user.reload.two_factor_preferences.recovery_codes.configuration[:codes] }
 
       before do
         current_codes
+        current_hashed_codes
       end
 
       it 'codes are saved in two factor preferences' do
         instance.generate
 
-        expect(user.reload.two_factor_preferences.find_by(method: 'recovery_codes').configuration[:codes]).not_to be(current_codes)
+        expect(user.reload.two_factor_preferences.recovery_codes.configuration[:codes]).not_to be(current_hashed_codes)
       end
     end
   end

spec/requests/user/two_factor_spec.rb

@@ -142,7 +142,7 @@ RSpec.describe 'User', current_user_id: 1, performs_jobs: true, type: :request d
       context 'with correct verification code', :aggregate_failures do
         it 'verified is true' do
           expect(json_response['verified']).to be(true)
-          expect(json_response['recovery_codes']).to eq(agent.reload.two_factor_preferences.recovery_codes.configuration[:codes])
+          expect(json_response['recovery_codes'].length).to eq(10)
         end
 
         context 'with disabled recovery codes' do
@@ -176,17 +176,16 @@ RSpec.describe 'User', current_user_id: 1, performs_jobs: true, type: :request d
     end
 
     context 'without existing recovery codes' do
-      it 'does not generate codes' do
-        expect(json_response).to eq(agent.reload.two_factor_preferences.recovery_codes.configuration[:codes])
+      it 'does generate codes' do
+        expect(json_response.length).to eq(10)
       end
     end
 
     context 'with existing recovery codes' do
       let(:current_codes) { Auth::TwoFactor::RecoveryCodes.new(agent).generate }
 
-      it 'does not generate codes', :aggregate_failures do
+      it 'does not generate codes' do
         expect(json_response).not_to eq(current_codes)
-        expect(json_response).to eq(agent.reload.two_factor_preferences.recovery_codes.configuration[:codes])
       end
     end
   end

spec/system/examples/authenticator_app_setup_examples.rb

@@ -40,10 +40,12 @@ def setup_authenticator_app_method(user:, password_check:, expect_recovery_codes
 
   if expect_recovery_codes
     in_modal do
-      any_code = user.two_factor_preferences.recovery_codes.configuration[:codes].sample
+      stored_codes_amount    = user.two_factor_preferences.recovery_codes.configuration[:codes].count
+      displayed_codes_amount = find('.two-factor-auth code').text.tr("\n", ' ').split.count
 
       expect(page).to have_text('Set up two-factor authentication: Recovery Codes')
-      expect(page).to have_text(any_code)
+      expect(stored_codes_amount).to eq(displayed_codes_amount)
+
       click_button "OK, I've saved my recovery codes"
     end
   end

spec/system/profile/password_spec.rb

@@ -175,8 +175,12 @@ RSpec.describe 'Profile > Password', authenticated_as: :user, type: :system do
         in_modal do
           expect(page).to have_text('Set up two-factor authentication: Recovery Codes')
 
-          any_code = user.two_factor_preferences.recovery_codes.configuration[:codes].sample
-          expect(page).to have_text(any_code)
+          stored_codes_amount    = user.two_factor_preferences.recovery_codes.configuration[:codes].count
+          displayed_codes_amount = find('.two-factor-auth code').text.tr("\n", ' ').split.count
+
+          expect(stored_codes_amount).to eq(displayed_codes_amount)
+
+          expect(page).to have_button("OK, I've saved my recovery codes")
         end
       end