devise-two-factor: attr_encrypted is incompatible with the upcoming Rails 7 ActiveRecord::Encryption API

Just checking if there’s any movement on this one, it looks like https://github.com/cybersecuricy/devise-two-factor/commit/e409e2080bc503b3557b634bebf0585c25616550 might be suitable to drop in place?

What about making changes to attr_encrypted gem to make it compatible with activerecord 7?

On the other hand, if rails/AC supports encrypted attrs natively now, perhaps it’s simply worth using that functionality and removing the dependency on attr-encrypted altogether?

@jason-hobbs okay, so for existing projects, there is some legwork you’ll have to do. First off, you’d need to migrate the existing encrypted_otp_secret values to the new internal Rails encrypted formats, which basically involves decrypting the encrypted_otp_* fields and putting that into an otp_secret field. You’ll need to create a new column in your users table for that, because the built-in Rails 7 encryption infrastructure just decrypts the values in place – there is no separate encrypted_ATTRIBUTE_NAME column and then a plaintext ATTRIBUTE_NAME attribute. You would then drop the old encrypted_otp_secret column once it’s no longer needed. You also don’t need multiple fields for the IV and salt and that sort of thing, because the new Rails 7 encryption scheme puts it all into a single JSON structure within the encrypted field.

Something like the following would work:

class MigrateOtpCodes < ActiveRecord::Migration[7.0]
  def up
    add_column :users, :otp_secret, :text

    User.reset_column_information

    User.where.not(encrypted_otp_secret: nil).find_each do |user|
      user.update!(
        otp_secret: Encryptor.decrypt(
          Base64.decode64(user.encrypted_otp_secret),
          key: <the encryption key you used for devise-two-factor>,
          iv: user.encrypted_otp_secret_iv.unpack1('m'),
          salt: user.encrypted_otp_secret_salt.slice(1..-1).unpack1('m')
        )
      )
    end
  end

  def down
    remove_column :users, :otp_secret
  end
end

That will migrate the existing data into the new Rails 7 encryption format. At that point you can drop the old encrypted columns and you’re done. Note that you need to include the encryptor gem for Encryptor, which is what the encrypted attributes use in devise-two-factor.

This worked for us, and has been working in both Rails 6.1 using the aforementioned extraction gem I posted above and appears to be working in Rails 7, which we are in the process of evaluating for eventual use.

I could write up a PR for this, but I think some documentation would have to be added to include the details on migrating your existing data from Rails pre-7.0, as per the migration code above. It would perhaps help to have a generator for a migration that would help guide a migration. There would also likely have to be some kind of major version upgrade – as it exists now, my branch does not care about backwards compatibility at all, and works only with Rails 7+. Some good documentation would have to be in place to make sure people realize this, so if I were to go a head with a PR we should discuss some of these issues so we can work out how to move forward with Rails 7+.

Anybody using the https://github.com/cybersecuricy/devise-two-factor/tree/securicy-fixes-rails-7 fork who want to switch to mainline devise-two-factor version 5.0 again (which merged a similar/improved PR for the same functionality)

In your model you probably call devise :two_factor_authenticatable, otp_secret_encryption_key: 'some_secret_value' Change that to devise :two_factor_authenticatable and instead add this to your config/initializers/devise.rb: config.otp_encrypted_attribute_options = { key: 'some_secret_value' }

@Jonakemon i haven’t been in touch with anyone besides what’s here in this thread. Any and all help and discussion would be appreciated, as there would need to be decisions made about proceed, backwards compatibility, migrations, etc. These sorts of things are perhaps outside of what I’m capable of deciding without some discussion and direction. What I can say is that the patches provided have been running in production for us for over 8 months or so now without issue, but ours is but a single use-case and some care needs to be given to roll it all out to a wider audience.

It looks like this bit will work since the methods will be defined, but the gemspec is defensive with < 6.2. I’ll try removing the restriction tomorrow in my fork and try to comment back if that’s enough.