Remove Legacy OTP Secret code

app/models/user.rb

@@ -44,14 +44,17 @@
 
 class User < ApplicationRecord
   self.ignored_columns += %w(
-    remember_created_at
-    remember_token
+    admin
     current_sign_in_ip
-    last_sign_in_ip
-    skip_sign_in_token
+    encrypted_otp_secret
+    encrypted_otp_secret_iv
+    encrypted_otp_secret_salt
     filtered_languages
-    admin
+    last_sign_in_ip
     moderator
+    remember_created_at
+    remember_token
+    skip_sign_in_token
   )
 
   include LanguagesHelper
@@ -73,8 +76,6 @@ 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
 

db/migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class RemoveLegacyDeviseTwoFactorSecretsFromUsers < ActiveRecord::Migration[7.1]
+  def change
+    safety_assured do
+      remove_column :users, :encrypted_otp_secret
+      remove_column :users, :encrypted_otp_secret_iv
+      remove_column :users, :encrypted_otp_secret_salt
+    end
+  end
+end

db/post_migrate/20240307180905_migrate_devise_two_factor_secrets.rb

@@ -3,13 +3,86 @@
 class MigrateDeviseTwoFactorSecrets < ActiveRecord::Migration[7.1]
   disable_ddl_transaction!
 
+  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
+
   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
+    include LegacyOtpSecret
   end
 
   def up

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do
+ActiveRecord::Schema[7.1].define(version: 2024_05_02_192024) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -1178,9 +1178,6 @@
     t.datetime "confirmation_sent_at", precision: nil
     t.string "unconfirmed_email"
     t.string "locale"
-    t.string "encrypted_otp_secret"
-    t.string "encrypted_otp_secret_iv"
-    t.string "encrypted_otp_secret_salt"
     t.integer "consumed_timestep"
     t.boolean "otp_required_for_login", default: false, null: false
     t.datetime "last_emailed_at", precision: nil

spec/models/user_spec.rb

@@ -9,17 +9,6 @@
 
   it_behaves_like 'two_factor_backupable'
 
-  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.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')