Merge e758e9a49c into 65093c619f

Gemfile

@@ -31,7 +31,7 @@ gem 'browser'
 gem 'charlock_holmes', '~> 0.7.7'
 gem 'chewy', '~> 7.3'
 gem 'devise', '~> 4.9'
-gem 'devise-two-factor', '~> 4.1'
+gem 'devise-two-factor'
 
 group :pam_authentication, optional: true do
   gem 'devise_pam_authenticatable2', '~> 9.2'

Gemfile.lock

@@ -97,8 +97,6 @@ GEM
       activerecord (>= 3.2, < 8.0)
       rake (>= 10.4, < 14.0)
     ast (2.4.2)
-    attr_encrypted (4.0.0)
-      encryptor (~> 3.0.0)
     attr_required (1.0.2)
     awrence (1.2.1)
     aws-eventstream (1.3.0)
@@ -204,9 +202,8 @@ GEM
       railties (>= 4.1.0)
       responders
       warden (~> 1.2.3)
-    devise-two-factor (4.1.1)
+    devise-two-factor (5.0.0)
       activesupport (~> 7.0)
-      attr_encrypted (>= 1.3, < 5, != 2)
       devise (~> 4.0)
       railties (~> 7.0)
       rotp (~> 6.0)
@@ -236,7 +233,6 @@ GEM
       htmlentities (~> 4.3.3)
       launchy (~> 2.1)
       mail (~> 2.7)
-    encryptor (3.0.0)
     erubi (1.12.0)
     et-orbi (1.2.11)
       tzinfo
@@ -842,7 +838,7 @@ DEPENDENCIES
   database_cleaner-active_record
   debug (~> 1.8)
   devise (~> 4.9)
-  devise-two-factor (~> 4.1)
+  devise-two-factor
   devise_pam_authenticatable2 (~> 9.2)
   discard (~> 1.2)
   doorkeeper (~> 5.6)

app/models/concerns/legacy_otp_secret.rb

@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+# TODO: This file is here for legacy support during devise-two-factor upgrade.
+# It should be removed after all records have been migrated.
+
+module LegacyOtpSecret
+  extend ActiveSupport::Concern
+
+  private
+
+  # Decrypt and return the `encrypted_otp_secret` attribute which was used in
+  # prior versions of devise-two-factor
+  # @return [String] The decrypted OTP secret
+  def legacy_otp_secret
+    return nil unless self[:encrypted_otp_secret]
+    return nil unless self.class.otp_secret_encryption_key
+
+    hmac_iterations = 2000 # a default set by the Encryptor gem
+    key = self.class.otp_secret_encryption_key
+    salt = Base64.decode64(encrypted_otp_secret_salt)
+    iv = Base64.decode64(encrypted_otp_secret_iv)
+
+    raw_cipher_text = Base64.decode64(encrypted_otp_secret)
+    # The last 16 bytes of the ciphertext are the authentication tag - we use
+    # Galois Counter Mode which is an authenticated encryption mode
+    cipher_text = raw_cipher_text[0..-17]
+    auth_tag =  raw_cipher_text[-16..-1] # rubocop:disable Style/SlicingWithRange
+
+    # this alrorithm lifted from
+    # https://github.com/attr-encrypted/encryptor/blob/master/lib/encryptor.rb#L54
+
+    # create an OpenSSL object which will decrypt the AES cipher with 256 bit
+    # keys in Galois Counter Mode (GCM). See
+    # https://ruby.github.io/openssl/OpenSSL/Cipher.html
+    cipher = OpenSSL::Cipher.new('aes-256-gcm')
+
+    # tell the cipher we want to decrypt. Symmetric algorithms use a very
+    # similar process for encryption and decryption, hence the same object can
+    # do both.
+    cipher.decrypt
+
+    # Use a Password-Based Key Derivation Function to generate the key actually
+    # used for encryptoin from the key we got as input.
+    cipher.key = OpenSSL::PKCS5.pbkdf2_hmac_sha1(key, salt, hmac_iterations, cipher.key_len)
+
+    # set the Initialization Vector (IV)
+    cipher.iv = iv
+
+    # The tag must be set after calling Cipher#decrypt, Cipher#key= and
+    # Cipher#iv=, but before calling Cipher#final. After all decryption is
+    # performed, the tag is verified automatically in the call to Cipher#final.
+    #
+    # If the auth_tag does not verify, then #final will raise OpenSSL::Cipher::CipherError
+    cipher.auth_tag = auth_tag
+
+    # auth_data must be set after auth_tag has been set when decrypting See
+    # http://ruby-doc.org/stdlib-2.0.0/libdoc/openssl/rdoc/OpenSSL/Cipher.html#method-i-auth_data-3D
+    # we are not adding any authenticated data but OpenSSL docs say this should
+    # still be called.
+    cipher.auth_data = ''
+
+    # #update is (somewhat confusingly named) the method which actually
+    # performs the decryption on the given chunk of data. Our OTP secret is
+    # short so we only need to call it once.
+    #
+    # It is very important that we call #final because:
+    #
+    # 1. The authentication tag is checked during the call to #final
+    # 2. Block based cipher modes (e.g. CBC) work on fixed size chunks. We need
+    #    to call #final to get it to process the last chunk properly. The output
+    #    of #final should be appended to the decrypted value. This isn't
+    #    required for streaming cipher modes but including it is a best practice
+    #    so that your code will continue to function correctly even if you later
+    #    change to a block cipher mode.
+    cipher.update(cipher_text) + cipher.final
+  end
+end

app/models/user.rb

@@ -39,6 +39,7 @@
 #  role_id                   :bigint(8)
 #  settings                  :text
 #  time_zone                 :string
+#  otp_secret                :string
 #
 
 class User < ApplicationRecord
@@ -72,6 +73,8 @@ class User < ApplicationRecord
   devise :two_factor_authenticatable,
          otp_secret_encryption_key: Rails.configuration.x.otp_secret
 
+  include LegacyOtpSecret # Must be after the above `devise` line in order to override the legacy method
+
   devise :two_factor_backupable,
          otp_number_of_backup_codes: 10
 
@@ -131,11 +134,6 @@ class User < ApplicationRecord
   normalizes :time_zone, with: ->(time_zone) { ActiveSupport::TimeZone[time_zone].nil? ? nil : time_zone }
   normalizes :chosen_languages, with: ->(chosen_languages) { chosen_languages.compact_blank.presence }
 
-  # This avoids a deprecation warning from Rails 5.1
-  # It seems possible that a future release of devise-two-factor will
-  # handle this itself, and this can be removed from our User class.
-  attribute :otp_secret
-
   has_many :session_activations, dependent: :destroy
 
   delegate :can?, to: :role

config/environments/development.rb

@@ -87,8 +87,7 @@ Rails.application.configure do
   # Otherwise, use letter_opener, which launches a browser window to view sent mail.
   config.action_mailer.delivery_method = ENV['HEROKU'] || ENV['VAGRANT'] || ENV['REMOTE_DEV'] ? :letter_opener_web : :letter_opener
 
-  # We provide a default secret for the development environment here.
-  # This value should not be used in production environments!
+  # TODO: Remove once devise-two-factor data migration complete
   config.x.otp_secret = ENV.fetch('OTP_SECRET', '1fc2b87989afa6351912abeebe31ffc5c476ead9bf8b3d74cbc4a302c7b69a45b40b1bbef3506ddad73e942e15ed5ca4b402bf9a66423626051104f4b5f05109')
 
   # Raise error when a before_action's only/except options reference missing actions

config/environments/production.rb

@@ -157,6 +157,7 @@ Rails.application.configure do
     'Referrer-Policy' => 'same-origin',
   }
 
+  # TODO: Remove once devise-two-factor data migration complete
   config.x.otp_secret = ENV.fetch('OTP_SECRET')
 
   # Enable DNS rebinding protection and other `Host` header attacks.

config/environments/test.rb

@@ -44,6 +44,7 @@ Rails.application.configure do
   # Print deprecation notices to the stderr.
   config.active_support.deprecation = :stderr
 
+  # TODO: Remove once devise-two-factor data migration complete
   config.x.otp_secret = '100c7faeef00caa29242f6b04156742bf76065771fd4117990c4282b8748ff3d99f8fdae97c982ab5bd2e6756a159121377cce4421f4a8ecd2d67bd7749a3fb4'
 
   # Generate random VAPID keys

db/migrate/20231210154528_add_otp_secret_to_user.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddOtpSecretToUser < ActiveRecord::Migration[7.1]
+  def change
+    add_column :users, :otp_secret, :string
+  end
+end

db/post_migrate/20240307180905_migrate_devise_two_factor_secrets.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+class MigrateDeviseTwoFactorSecrets < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  class MigrationUser < ApplicationRecord
+    self.table_name = :users
+
+    devise :two_factor_authenticatable,
+           otp_secret_encryption_key: Rails.configuration.x.otp_secret
+
+    include LegacyOtpSecret # Must be after the above `devise` line in order to override the legacy method
+  end
+
+  def up
+    MigrationUser.reset_column_information
+
+    users_with_otp_enabled.find_each do |user|
+      # Gets the new value on already-updated users
+      # Falls back to legacy value on not-yet-migrated users
+      otp_secret = user.otp_secret
+
+      Rails.logger.debug { "Processing #{user.email}" }
+
+      # This is a no-op for migrated users and updates format for not migrated
+      user.update!(otp_secret: otp_secret)
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+
+  private
+
+  def users_with_otp_enabled
+    MigrationUser.where(otp_required_for_login: true, otp_secret: nil)
+  end
+end

db/schema.rb

@@ -1199,6 +1199,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do
     t.bigint "role_id"
     t.text "settings"
     t.string "time_zone"
+    t.string "otp_secret"
     t.index ["account_id"], name: "index_users_on_account_id"
     t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true
     t.index ["created_by_application_id"], name: "index_users_on_created_by_application_id", where: "(created_by_application_id IS NOT NULL)"

lib/tasks/tests.rake

@@ -127,6 +127,14 @@ namespace :tests do
         exit(1)
       end
 
+      # This is checking the attribute rather than the method, to avoid the legacy fallback
+      # and ensure the data has been migrated
+      unless Account.find_local('qcuser').user[:otp_secret] == 'anotpsecretthatshouldbeencrypted'
+        puts "DEBUG: #{Account.find_local('qcuser').user.inspect}"
+        puts 'OTP secret for user not preserved as expected'
+        exit(1)
+      end
+
       puts 'No errors found. Database state is consistent with a successful migration process.'
     end
 
@@ -213,9 +221,15 @@ namespace :tests do
           (4, 10, 'kmruser@localhost', now(), now(), false, 'ku', '{en,kmr,ku,ckb}');
 
         INSERT INTO "users"
-          (id, account_id, email, created_at, updated_at, locale)
+          (id, account_id, email, created_at, updated_at, locale,
+           encrypted_otp_secret, encrypted_otp_secret_iv, encrypted_otp_secret_salt,
+           otp_required_for_login)
         VALUES
-          (5, 11, 'qcuser@localhost', now(), now(), 'fr-QC');
+          (5, 11, 'qcuser@localhost', now(), now(), 'fr-QC',
+           E'Fttsy7QAa0edaDfdfSz094rRLAxc8cJweDQ4BsWH/zozcdVA8o9GLqcKhn2b\nGi/V\n',
+           'rys3THICkr60BoWC',
+           '_LMkAGvdg7a+sDIKjI3mR2Q==',
+           true);
 
         INSERT INTO "settings"
           (id, thing_type, thing_id, var, value, created_at, updated_at)

spec/models/user_spec.rb

@@ -9,14 +9,25 @@ RSpec.describe User do
 
   it_behaves_like 'two_factor_backupable'
 
-  describe 'otp_secret' do
+  describe 'legacy_otp_secret' do
     it 'is encrypted with OTP_SECRET environment variable' do
       user = Fabricate(:user,
                        encrypted_otp_secret: "Fttsy7QAa0edaDfdfSz094rRLAxc8cJweDQ4BsWH/zozcdVA8o9GLqcKhn2b\nGi/V\n",
                        encrypted_otp_secret_iv: 'rys3THICkr60BoWC',
                        encrypted_otp_secret_salt: '_LMkAGvdg7a+sDIKjI3mR2Q==')
 
-      expect(user.otp_secret).to eq 'anotpsecretthatshouldbeencrypted'
+      expect(user.send(:legacy_otp_secret)).to eq 'anotpsecretthatshouldbeencrypted'
+    end
+  end
+
+  describe 'otp_secret' do
+    it 'encrypts the saved value' do
+      user = Fabricate(:user, otp_secret: '123123123')
+
+      user.reload
+
+      expect(user.otp_secret).to eq '123123123'
+      expect(user.attributes_before_type_cast[:otp_secret]).to_not eq '123123123'
     end
   end