[#8844] Upgrade devise-two-factor prior to Rails 8 upgrade

Gemfile

@@ -97,7 +97,7 @@ gem 'devise', '~> 4.9'
 gem 'devise_invitable', '~> 2.0.9'
 gem 'devise-pwned_password'
 gem 'devise-security'
-gem 'devise-two-factor', '~> 4.1.1'
+gem 'devise-two-factor', '~> 6.4' # 6.x is rails-8-compatible; legacy otp secrets read via User#legacy_otp_secret
 gem 'rack-cors'
 gem 'doorkeeper'
 

Gemfile.lock

@@ -423,11 +423,10 @@ GEM
       pwned (~> 2.4)
     devise-security (0.18.0)
       devise (>= 4.3.0)
-    devise-two-factor (4.1.1)
-      activesupport (~> 7.0)
-      attr_encrypted (>= 1.3, < 5, != 2)
-      devise (~> 4.0)
-      railties (~> 7.0)
+    devise-two-factor (6.4.0)
+      activesupport (>= 7.2, < 8.2)
+      devise (>= 4.0, < 6.0)
+      railties (>= 7.2, < 8.2)
       rotp (~> 6.0)
     devise_invitable (2.0.11)
       actionmailer (>= 5.0)
@@ -1192,7 +1191,7 @@ DEPENDENCIES
   devise (~> 4.9)
   devise-pwned_password
   devise-security
-  devise-two-factor (~> 4.1.1)
+  devise-two-factor (~> 6.4)
   devise_invitable (~> 2.0.9)
   doorkeeper
   dotenv-rails
@@ -1481,7 +1480,7 @@ CHECKSUMS
   devise (4.9.4) sha256=920042fe5e704c548aa4eb65ebdd65980b83ffae67feb32c697206bfd975a7f8
   devise-pwned_password (0.1.12) sha256=876452466634560a79910a1f22ef467f656e95e746c77d8266d70345b2279672
   devise-security (0.18.0) sha256=fc06be1624b5151044ff9c5d8e61abdfa7d56eb16bfdaec16a11235d54708513
-  devise-two-factor (4.1.1) sha256=c95f5b07533e62217aaed3c386874d94e2d472fb5f2b6598afe8600fc17a8b95
+  devise-two-factor (6.4.0) sha256=09e3a23b5b9ae7da78881d238a89860454d9e2ba0912e175576e9c028757adb4
   devise_invitable (2.0.11) sha256=780014f74b8848218d93a297300590120f1742984396acf3d6799ab49b9a6a3f
   diff-lcs (1.6.2) sha256=9ae0d2cba7d4df3075fe8cd8602a8604993efc0dfa934cff568969efb1909962
   docile (1.4.1) sha256=96159be799bfa73cdb721b840e9802126e4e03dfc26863db73647204c727f21e

app/models/concerns/user_concern.rb

@@ -45,9 +45,42 @@ module UserConcern
            :two_factor_backupable,
            password_length: 10..128,
            otp_secret_encryption_key: ENV['ENCRYPTION_KEY'],
-           otp_secret_length: 26, # 128 bits keys, per RFC 4226. See GHSA-qjxf-mc72-wjr2
+           otp_secret_length: 26, # 26 random bytes (>= 128-bit key, per RFC 4226 / GHSA-qjxf-mc72-wjr2)
            otp_number_of_backup_codes: 10
 
+    # devise-two-factor 6.x reads the Rails-encrypted `otp_secret` column, falling back to
+    # this method when it is empty. Existing users' secrets live in the pre-6.x
+    # attr_encrypted columns (encrypted_otp_secret/_iv/_salt, aes-256-gcm, per-attribute
+    # iv+salt, PBKDF2-HMAC-SHA1); decrypt them here so 2FA keeps working with no data
+    # migration. Defined on the class so it overrides the gem's no-op default. See the
+    # gem's UPGRADING.md (4.x -> 5.x).
+    private def legacy_otp_secret
+      return nil unless self[:encrypted_otp_secret]
+      return nil unless self.class.otp_secret_encryption_key
+
+      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)
+      # aes-256-gcm appends the 16-byte auth tag to the ciphertext
+      cipher_text = raw_cipher_text[0..-17]
+      auth_tag = raw_cipher_text[-16..]
+
+      cipher = OpenSSL::Cipher.new('aes-256-gcm')
+      cipher.decrypt
+      cipher.key = OpenSSL::KDF.pbkdf2_hmac(
+        key,
+        salt: salt,
+        iterations: 2000, # the Encryptor gem's default
+        length: cipher.key_len,
+        hash: 'sha1',
+      )
+      cipher.iv = iv
+      cipher.auth_tag = auth_tag
+      cipher.auth_data = ''
+      cipher.update(cipher_text) + cipher.final
+    end
+
     include OmniauthSupport
 
     # Doorkeeper

app/services/idp/keycloak/user_importer.rb

@@ -173,16 +173,18 @@ def bcrypt_cost(encrypted_password)
       # recovery-code format has no clean partialImport mapping. Affected users
       # fall back to their authenticator app or an admin 2FA reset.
       def build_otp_credential(user)
-        return nil unless user.encrypted_otp_secret.present? && user.otp_required_for_login?
+        return nil unless user.otp_required_for_login?
 
         begin
+          # user.otp_secret bridges both storage locations: the Rails-encrypted otp_secret
+          # column (devise-two-factor 6.x) and the legacy encrypted_otp_secret* columns.
           otp_secret = user.otp_secret
         rescue StandardError => e
           Rails.logger.warn "Failed to decrypt OTP secret for #{user.email}: #{e.message}"
           return nil
         end
 
-        return nil unless otp_secret
+        return nil unless otp_secret.present?
 
         {
           type: 'otp',

config/application.rb

@@ -74,6 +74,25 @@ def secrets
     # config.active_record.yaml_column_permitted_classes = [Symbol, Date, Time]
     config.active_record.use_yaml_unsafe_load = true
 
+    # ActiveRecord encryption backs devise-two-factor 6.x's `otp_secret` column (new and
+    # re-enrolled 2FA secrets). Existing secrets remain in encrypted_otp_secret* and are
+    # read via User#legacy_otp_secret. Keys are derived from the existing ENCRYPTION_KEY so
+    # no new secrets need provisioning; set the AR_ENCRYPTION_* env vars to override.
+    if (encryption_root = ENV['ENCRYPTION_KEY']).present?
+      derive_ar_encryption_key = lambda do |label, override_env|
+        ENV[override_env].presence || OpenSSL::KDF.pbkdf2_hmac(
+          encryption_root,
+          salt: "ar-encryption:#{label}",
+          iterations: 2**16,
+          length: 32,
+          hash: 'sha256',
+        ).unpack1('H*')
+      end
+      config.active_record.encryption.primary_key = derive_ar_encryption_key.call('primary_key', 'AR_ENCRYPTION_PRIMARY_KEY')
+      config.active_record.encryption.deterministic_key = derive_ar_encryption_key.call('deterministic_key', 'AR_ENCRYPTION_DETERMINISTIC_KEY')
+      config.active_record.encryption.key_derivation_salt = derive_ar_encryption_key.call('key_derivation_salt', 'AR_ENCRYPTION_KEY_DERIVATION_SALT')
+    end
+
     # Use the responders controller from the responders gem
     config.app_generators.scaffold_controller :responders_controller
 

db/migrate/20260715120000_add_otp_secret_to_users.rb

@@ -0,0 +1,16 @@
+###
+# Copyright Green River Data Group, Inc.
+#
+# License detail: https://github.com/greenriver/hmis-warehouse/blob/production/LICENSE.md
+###
+
+# frozen_string_literal: true
+
+# devise-two-factor 6.x stores the TOTP secret in a Rails-encrypted `otp_secret`
+# column. Existing secrets remain in encrypted_otp_secret/_iv/_salt and are read via
+# User#legacy_otp_secret; this nullable column holds new and re-enrolled secrets.
+class AddOtpSecretToUsers < ActiveRecord::Migration[7.2]
+  def change
+    add_column :users, :otp_secret, :string
+  end
+end

db/structure.sql

@@ -2490,7 +2490,8 @@ CREATE TABLE public.users (
     training_courses jsonb,
     custom_session_invalidator character varying,
     theme character varying DEFAULT 'legacy'::character varying,
-    last_connector_id character varying
+    last_connector_id character varying,
+    otp_secret character varying
 );
 
 
@@ -4249,6 +4250,7 @@ ALTER TABLE ONLY public.oauth_access_tokens
 SET search_path TO "$user", public;
 
 INSERT INTO "schema_migrations" (version) VALUES
+('20260715120000'),
 ('20260620000000'),
 ('20260614130000'),
 ('20260611120000'),

spec/models/user_otp_secret_legacy_bridge_spec.rb

@@ -0,0 +1,163 @@
+###
+# Copyright Green River Data Group, Inc.
+#
+# License detail: https://github.com/greenriver/hmis-warehouse/blob/production/LICENSE.md
+###
+
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+# devise-two-factor 6.x stores new OTP secrets in a Rails-encrypted `otp_secret` column,
+# but existing users' secrets remain in the legacy attr_encrypted columns
+# (encrypted_otp_secret/_iv/_salt). User#legacy_otp_secret must decrypt those so existing
+# 2FA keeps working with no data migration.
+RSpec.describe 'User OTP secret legacy bridge', type: :model do
+  # Writes a secret into the legacy encrypted_otp_secret* columns the way
+  # devise-two-factor <= 4.x did (attr_encrypted, per-attribute iv+salt, aes-256-gcm),
+  # i.e. how production secrets are currently stored.
+  #
+  # Every crypto option is pinned explicitly rather than left to attr_encrypted's
+  # defaults, so the fixture cannot silently drift out of production's format when the
+  # gem's defaults change:
+  #   - algorithm: aes-256-gcm      — must match the concern's OpenSSL::Cipher('aes-256-gcm')
+  #   - encode*/base64 on all three — REQUIRED, not stylistic: encrypted_otp_secret* are
+  #     `character varying` columns (see db/structure.sql). attr_encrypted's default
+  #     `encode: false` emits raw AES-GCM bytes, which PostgreSQL rejects on write with
+  #     `PG::CharacterNotInRepertoire (invalid byte sequence for encoding "UTF8")`. So the
+  #     only format that can physically live in these columns — and therefore the only
+  #     format production can hold — is Base64. The `stored legacy columns are Base64` test
+  #     below asserts this so the fixture can't drift to an unstorable (non-production) shape.
+  #
+  # NOTE: this reproduces production's *format*, but the bytes are still generated by
+  # attr_encrypted here rather than captured from production. The strongest possible
+  # version of this test is a golden known-answer vector: a real (encrypted_otp_secret,
+  # _iv, _salt) triple captured from a production/staging user asserted to decrypt to that
+  # user's known secret. Add one when such a sample is available.
+  #
+  # Virtual attr `legacy_secret` is mapped to the encrypted_otp_secret* columns (via
+  # `attribute:`) so it does not collide with the real `otp_secret` column that
+  # devise-two-factor 6.x adds.
+  let(:legacy_writer_class) do
+    Class.new(ActiveRecord::Base) do
+      self.table_name = 'users'
+      extend AttrEncrypted
+      attr_encrypted :legacy_secret,
+                     key: ENV['ENCRYPTION_KEY'],
+                     mode: :per_attribute_iv_and_salt,
+                     algorithm: 'aes-256-gcm',
+                     encode: true,
+                     encode_iv: true,
+                     encode_salt: true,
+                     attribute: 'encrypted_otp_secret'
+    end
+  end
+
+  let(:plaintext_secret) { User.generate_otp_secret }
+
+  def user_with_legacy_secret(secret)
+    user = create(:user)
+    writer = legacy_writer_class.find(user.id)
+    writer.legacy_secret = secret
+    writer.save!(validate: false)
+    user.reload
+  end
+
+  it 'reads a legacy-encrypted secret through otp_secret' do
+    user = user_with_legacy_secret(plaintext_secret)
+
+    expect(user[:otp_secret]).to be_nil # nothing in the new Rails-encrypted column
+    expect(user.otp_secret).to eq(plaintext_secret)
+  end
+
+  # Guards fixture fidelity: the concern decrypts by Base64.decode64'ing all three
+  # columns, and these `character varying` columns can only physically hold a UTF-8
+  # (Base64) string — raw AES-GCM bytes are rejected by PostgreSQL on write. If a future
+  # change made the fixture store some other shape, otp_secret would no longer reflect how
+  # production data is actually stored; this catches that drift at the source.
+  it 'stores the legacy columns in the Base64 format the concern decodes' do
+    user = user_with_legacy_secret(plaintext_secret)
+    raw = User.connection.select_one(
+      'SELECT encrypted_otp_secret, encrypted_otp_secret_iv, encrypted_otp_secret_salt ' \
+      "FROM users WHERE id = #{user.id}",
+    )
+
+    # None of the columns are raw binary — PostgreSQL could not store that in these
+    # `character varying` columns, and the concern's Base64.decode64 would mangle it.
+    raw.each_value do |column|
+      expect(column.dup.force_encoding('UTF-8').valid_encoding?).to be(true)
+    end
+    # The ciphertext body is genuinely Base64: re-encoding the decoded bytes reproduces
+    # the stored value (modulo attr_encrypted's line wrapping). Arbitrary non-Base64 text
+    # would not survive this round-trip.
+    body = raw['encrypted_otp_secret']
+    expect(Base64.strict_encode64(Base64.decode64(body))).to eq(body.delete("\n"))
+  end
+
+  it 'validates an OTP generated from the legacy secret' do
+    user = user_with_legacy_secret(plaintext_secret)
+
+    code = ROTP::TOTP.new(plaintext_secret).now
+    expect(user.validate_and_consume_otp!(code)).to be(true)
+  end
+
+  # During the migration window a user can hold BOTH a legacy secret and a new
+  # Rails-encrypted secret — e.g. an existing user re-enrolls after the upgrade. The new
+  # secret must win. If precedence were inverted, a freshly re-enrolled authenticator
+  # would silently validate against the stale legacy secret.
+  it 'prefers the new Rails-encrypted secret over the legacy secret when both exist' do
+    user = user_with_legacy_secret(plaintext_secret)
+    new_secret = User.generate_otp_secret
+    user.update!(otp_secret: new_secret)
+    user.reload
+
+    expect(user.encrypted_otp_secret).to be_present # legacy secret is still stored...
+    expect(user.otp_secret).to eq(new_secret)       # ...but the new secret takes precedence
+    # ...and with BOTH columns populated, the functional login path resolves to the new
+    # secret (unique to this test — the fresh-secret test only ever has the new column).
+    expect(user.validate_and_consume_otp!(ROTP::TOTP.new(new_secret).now)).to be(true)
+  end
+
+  # A user with no 2FA secret at all (new user, or one who never enrolled) must read back
+  # as nil, not raise. This exercises the `return nil unless self[:encrypted_otp_secret]`
+  # guard directly; without it, Base64.decode64(nil) would blow up on every such read.
+  it 'returns nil when the user has no OTP secret at all' do
+    user = create(:user)
+
+    expect(user[:otp_secret]).to be_nil
+    expect(user.otp_secret).to be_nil
+  end
+
+  # When the legacy secret cannot be decrypted — the realistic case being a rotated
+  # ENCRYPTION_KEY, which fails GCM authentication — the bridge raises rather than
+  # returning a wrong or empty secret. This pins the current fail-loud behavior: if the
+  # team later wants graceful degradation (return nil so the user is prompted to re-enroll
+  # rather than 500'ing on login), that becomes a deliberate change that flips this test.
+  it 'raises when the legacy secret cannot be decrypted' do
+    user = user_with_legacy_secret(plaintext_secret)
+    # Keep valid Base64 but swap in ciphertext that fails the GCM auth tag check — the same
+    # failure mode a wrong/rotated key produces.
+    user.update_columns(encrypted_otp_secret: Base64.encode64('not the real ciphertext'))
+    user.reload
+
+    expect { user.otp_secret }.to raise_error(OpenSSL::Cipher::CipherError)
+  end
+
+  # New/re-enrolled secrets use the Rails-encrypted otp_secret column; this also proves
+  # the ActiveRecord encryption config (derived from ENCRYPTION_KEY) works end to end.
+  it 'stores a newly generated secret in the Rails-encrypted otp_secret column' do
+    user = create(:user)
+    user.set_initial_two_factor_secret!
+    user.reload
+
+    expect(user.encrypted_otp_secret).to be_nil # not in the legacy columns
+    expect(user.otp_secret).to be_present
+
+    raw = User.connection.select_value("SELECT otp_secret FROM users WHERE id = #{user.id}")
+    expect(raw).to be_present
+    expect(raw).not_to eq(user.otp_secret) # stored encrypted at rest
+
+    code = ROTP::TOTP.new(user.otp_secret).now
+    expect(user.validate_and_consume_otp!(code)).to be(true)
+  end
+end

spec/services/idp/keycloak/user_importer_spec.rb

@@ -275,6 +275,20 @@
           with(/Failed to decrypt OTP secret/)
       end
     end
+
+    context 'when 2FA is required but no secret is configured' do
+      before { user.update(otp_required_for_login: true) }
+
+      it 'excludes the OTP credential when neither the new nor legacy secret is set' do
+        # otp_secret bridges both storage locations; nil means 2FA was never set up
+        # despite the requirement flag, so we must not emit an (empty) OTP credential.
+        expect(user.otp_secret).to be_nil
+
+        result = importer.build_import_user_data(user)
+
+        expect(result[:credentials].find { |c| c[:type] == 'otp' }).to be_nil
+      end
+    end
   end
 
   describe '#bulk_import_users' do