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

[eanders]

Merging this PR

• use the squash-merge strategy for PRs targeting main

Description

Upgrade devise-two-factor 4.1.1 -> 6.4.0 (Rails 8 compatible)

6.x is the minimum devise-two-factor line that supports Rails 8. This retains existing OTP secrets via the gem's legacy_otp_secret bridge (no data migration): existing secrets stay in encrypted_otp_secret and are decrypted on read; new/re-enrolled secrets use a Rails-encrypted otp_secret column. AR encryption keys are derived from ENCRYPTION_KEY to prevent needing to setup secrets.yml.

Update the Keycloak importer to read otp_secret via the bridge so it covers both legacy and modern storage locations.

Type of change

Checklist before requesting review

• I performed a self-review of my code
• I ran the OP review skill
• I ran the code that is being changed under ideal conditions, and it doesn't fail
• If adding a new endpoint / exposing data in a new way, I have:
  • ensured the API can't leak data from other data sources
  • ensured this does not introduce N+1s
  • ensured permissions and visibility checks are performed in the right places
• Any major architectural changes are supported by an approved ADR (Architectural Decision Record)
• I updated the documentation (or not applicable)
• I added spec tests (or not applicable)
• I provided testing instructions in this PR or the related issue (or not applicable)

[eanders]

@ttoomey let me know if this should live somewhere else. I'm not seeing the IdP concerns so I think that hasn't been merged to main yet.

[ttoomey]

I'll take a look - let's huddle about this upgrade and the interaction with the IDP work

[ttoomey]

This will cause a small merge conflict with IDP but not a big deal. Needs to be relocated to app/models/concerns/devise_user.rb. We will figure it out when we bring the IDP integration branch in

[ttoomey]

Looks good, just need to re-run the migration to regen the schema

diff --git a/db/structure.sql b/db/structure.sql
index 122f0d4c42..010b66a251 100644
--- a/db/structure.sql
+++ b/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'),

[ttoomey]

This will cause a small merge conflict with IDP but not a big deal. Needs to be relocated to app/models/concerns/devise_user.rb. We will figure it out when we bring the IDP integration branch in

[ttoomey]

suggestion for AI review:

Changes to spec/models/user_otp_secret_legacy_bridge_spec.rb:

Legacy writer fixture

• Added algorithm: 'aes-256-gcm' explicitly to the attr_encrypted config so the fixture's cipher is pinned rather than inherited from attr_encrypted's default (which could change on a gem bump).
• Expanded the comment to explain why encode: true is required: the encrypted_otp_secret* columns are character varying, and attr_encrypted's default (encode: false) writes raw bytes that PostgreSQL rejects, so Base64 is the only storable format. Also added a note that the bytes are still generated by attr_encrypted rather than captured from production, and that a golden vector from real production data would be stronger.

New examples added

• stores the legacy columns in the Base64 format the concern decodes — reads the three raw columns and asserts they are valid UTF-8 and that the ciphertext body round-trips through Base64. Guards against the fixture drifting to a format that isn't what production stores.
• prefers the new Rails-encrypted secret over the legacy secret when both exist — writes a legacy secret, then sets a new otp_secret, and asserts the new secret wins and validates while the legacy column is still present.
• returns nil when the user has no OTP secret at all — asserts a user with no secret reads back nil instead of raising (covers the guard clause).
• raises when the legacy secret cannot be decrypted — corrupts the stored ciphertext and asserts otp_secret raises OpenSSL::Cipher::CipherError, documenting the current fail-loud behavior.

Assertion tightening

• Changed the two validate_and_consume_otp! assertions from be_truthy to be(true).

diff --git a/spec/models/user_otp_secret_legacy_bridge_spec.rb b/spec/models/user_otp_secret_legacy_bridge_spec.rb
index dfaa250e6d..c72596c75a 100644
--- a/spec/models/user_otp_secret_legacy_bridge_spec.rb
+++ b/spec/models/user_otp_secret_legacy_bridge_spec.rb
@@ -13,12 +13,31 @@ require 'rails_helper'
 # (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 exactly the way
-  # devise-two-factor <= 4.x did (attr_encrypted, per-attribute iv+salt, default
-  # aes-256-gcm) — i.e. how production secrets are currently stored.
+  # 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. This reproduces production's storage format exactly.
+  # devise-two-factor 6.x adds.
   let(:legacy_writer_class) do
     Class.new(ActiveRecord::Base) do
       self.table_name = 'users'
@@ -26,6 +45,7 @@ RSpec.describe 'User OTP secret legacy bridge', type: :model do
       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,
@@ -50,11 +70,77 @@ RSpec.describe 'User OTP secret legacy bridge', type: :model do
     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_truthy
+    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
@@ -72,6 +158,6 @@ RSpec.describe 'User OTP secret legacy bridge', type: :model do
     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_truthy
+    expect(user.validate_and_consume_otp!(code)).to be(true)
   end
 end