Remove Legacy OTP Secret code (#34743)

app/models/concerns/legacy_otp_secret.rb

@@ -1,77 +0,0 @@
-# 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

@@ -15,9 +15,6 @@
 #  current_sign_in_at        :datetime
 #  disabled                  :boolean          default(FALSE), not null
 #  email                     :string           default(""), not null
-#  encrypted_otp_secret      :string
-#  encrypted_otp_secret_iv   :string
-#  encrypted_otp_secret_salt :string
 #  encrypted_password        :string           default(""), not null
 #  last_emailed_at           :datetime
 #  last_sign_in_at           :datetime
@@ -46,14 +43,17 @@
 
 class User < ApplicationRecord
   self.ignored_columns += %w(
+    admin
+    current_sign_in_ip
+    encrypted_otp_secret
+    encrypted_otp_secret_iv
+    encrypted_otp_secret_salt
+    filtered_languages
+    last_sign_in_ip
+    moderator
     remember_created_at
     remember_token
-    current_sign_in_ip
-    last_sign_in_ip
     skip_sign_in_token
-    filtered_languages
-    admin
-    moderator
   )
 
   include LanguagesHelper
@@ -76,8 +76,6 @@ class User < ApplicationRecord
          otp_secret_encryption_key: Rails.configuration.x.otp_secret,
          otp_secret_length: 32
 
-  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/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/post_migrate/20250520192024_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, :string
+      remove_column :users, :encrypted_otp_secret_iv, :string
+      remove_column :users, :encrypted_otp_secret_salt, :string
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 2025_04_28_104538) do
+ActiveRecord::Schema[8.0].define(version: 2025_05_20_192024) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_catalog.plpgsql"
 
@@ -1224,9 +1224,6 @@ ActiveRecord::Schema[8.0].define(version: 2025_04_28_104538) do
     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

@@ -11,17 +11,6 @@ RSpec.describe User do
 
   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')