wmlele / devise-otp

Two Factors authentication for Devise using Time Based OTP/rfc6238 tokens.
MIT License
205 stars 39 forks source link

Migrations missing Active Record version numbering #89

Closed pkarjala closed 1 week ago

pkarjala commented 3 weeks ago

When initially setting up this gem on a Rails 7.1 project and generating the migration for the user model

rails g devise_otp user

it generates something like:

class DeviseOtpAddToUsers < ActiveRecord::Migration
  def self.up
    change_table :users do |t|
      t.string    :otp_auth_secret
      t.string    :otp_recovery_secret
      t.boolean   :otp_enabled,          :default => false, :null => false
      t.boolean   :otp_mandatory,        :default => false, :null => false
      t.datetime  :otp_enabled_on
      t.integer   :otp_failed_attempts,  :default => 0, :null => false
      t.integer   :otp_recovery_counter, :default => 0, :null => false
      t.string    :otp_persistence_seed

      t.string    :otp_session_challenge
      t.datetime  :otp_challenge_expires
    end
    add_index :users, :otp_session_challenge,  :unique => true
    add_index :users, :otp_challenge_expires
  end

  def self.down
    change_table :users do |t|
      t.remove :otp_auth_secret, :otp_recovery_secret, :otp_enabled, :otp_mandatory, :otp_enabled_on, :otp_session_challenge,
          :otp_challenge_expires, :otp_failed_attempts, :otp_recovery_counter, :otp_persistence_seed

    end
  end
end

Attempting to run this migration results in the following errors:

$ rails db:migrate
bin/rails aborted!
StandardError: Directly inheriting from ActiveRecord::Migration is not supported. Please specify the Active Record release the migration was written for: (StandardError)

  class DeviseOtpAddToUsers < ActiveRecord::Migration[7.1]
[...]/db/migrate/20240820213917_devise_otp_add_to_users.rb:1:in `<main>'

Caused by:
StandardError: Directly inheriting from ActiveRecord::Migration is not supported. Please specify the Active Record release the migration was written for: (StandardError)

  class DeviseOtpAddToUsers < ActiveRecord::Migration[7.1]
[...]/db/migrate/20240820213917_devise_otp_add_to_users.rb:1:in `<main>'
Tasks: TOP => db:migrate
(See full trace by running task with --trace)

The version number of Active Record must be included with the migration generated as of Rails 5.x (as far as I could find). It would be good to build this into the generators for the migrations that devise-otp creates, or add documentation to manually add the version number after running the generator.

strzibny commented 2 weeks ago

Yes, you are right. This need to be fixed.

pkarjala commented 1 week ago

Thank you!

Small suggested tweak to this, instead of hard-coding it as [7.0], you could use the following on generation:

"[" + ActiveRecord::Migration::current_version.to_s + "]"

Documentation: https://api.rubyonrails.org/classes/ActiveRecord/Migration.html#method-c-current_version

This will always return the current ActiveRecord version as a two digit float, and will be both forwards and backwards compatible across various Rails versions.

There are also more complex methods for invoking the generator directly instead of manually copying a file; instead you pass in arguments for the data you want generated. See https://api.rubyonrails.org/classes/Rails/Generators.html#method-c-invoke for documentation, and https://github.com/devise-two-factor/devise-two-factor/blob/bb9f435d1d1a17a7f52f62c547ae53bcdceabb96/lib/generators/devise_two_factor/devise_two_factor_generator.rb#L17 for an example.

This should always properly generate the version number since it creates the entire generation file from scratch each time. The downside is you have less control over exactly what is generated; if you want specific migrations, the method currently used is probably better.

Hope that helps!

strzibny commented 1 week ago

I was thinking about it but then it could be that a change in Rails will make the file wrong, no? Maybe it's not as likely tho and we could have this dynamically done. Contributions are welcome.

pkarjala commented 1 week ago

This is actually intentional; Rails wants to know what version of ActiveRecord a migration was generated with so that it will correctly apply certain considerations since ActiveRecord has changed and will continue to change over time. So an older migration that happened on Rails 5.1 can still run on Rails 7.1 without compatibility issues as long as it is properly labeled. Moving forward, migrations made on 7.x will run fine under 8.x and so forth since ActiveRecord knows what version the migration was created with.

See the comments on https://github.com/rails/rails/blob/53f703b3e84aebf7799f24092cf3c2976f94b4be/activerecord/lib/active_record/migration/compatibility.rb#L1 for more information.

strzibny commented 1 week ago

Yes, hence my comment and why I hardcoded it. The migration file already exists and is written against Rails 7 (well technically older version but I put 7). So Rails will know how to migrate it.

pkarjala commented 1 week ago

Apologies--for some reason I had in my head that devise-otp was dynamically generating this each time the migration was run, not copying an existing pre-generated migration.

Yes, the hard-coded version is fine! Sorry for the unnecessary back-and-forth.