heapsource / active_model_otp

Adds methods to set and authenticate against one time passwords (Two-Factor Authentication). Inspired in AM::SecurePassword
MIT License
769 stars 81 forks source link

SystemStackError if counter column is named `otp_counter` #26

Closed garethrees closed 8 years ago

garethrees commented 8 years ago

Followed the Counter based OTP guide.

$ rails g migration AddOtpSecretKeyToUsers otp_secret_key:string
$ rails g migration AddCounterForOtpToUsers otp_counter:integer
$ rails g migration AddCounterForOtpToUsers my_otp_counter:integer
# Set the default value to `1` manually
$ rake db:migrate
class User < ActiveRecord::Base
  has_one_time_password :counter_based => true
end
u = User.new
# => #<User id: nil, email: nil, otp_counter: 1, otp_secret_key: nil, created_at: nil, updated_at: nil, my_otp_counter: 1>
u.otp_counter
SystemStackError: stack level too deep
        from /usr/local/rbenv/versions/2.0.0-p481/lib/ruby/2.0.0/irb/workspace.rb:86
Maybe IRB bug!
class User < ActiveRecord::Base
  has_one_time_password :counter_based => true, :counter_column_name => :my_otp_counter
end
u = User.new
# => #<User id: nil, email: nil, otp_counter: 1, otp_secret_key: nil, created_at: nil, updated_at: nil, my_otp_counter: 1>
u.otp_counter
# => 1

Not sure why the same problem isn't exhibited by the specs, but the following seems to fix the problem:

module ActiveModel
  module OneTimePassword
    module InstanceMethodsOnActivation
      def otp_counter
        if self.class.otp_counter_column_name != 'otp_counter'
          self.public_send(self.class.otp_counter_column_name)
        else
          super
        end
      end

      def otp_counter=(attr)
        if self.class.otp_counter_column_name != 'otp_counter'
          self.public_send("#{self.class.otp_counter_column_name}=", attr)
        else
          super
        end
      end
    end
  end
end
garethrees commented 8 years ago

I've added a test case to reproduce this https://github.com/garethrees/active_model_otp/commit/05baccf0b220f643eb84485fa764bdafe949d224

skunkworker commented 8 years ago

+1 for this, the fix makes it work as well.