Pad OTP codes with less than 6 digits

[robertomiranda]

I think would be better add this as a parameter of otp_code method.

def otp_code(time = Time.now, padded=false)
   ROTP::TOTP.new(self.otp_column).at(time, padded)
end

Also could you add a test case with an example in the Readme?

Thanks :+1:

[brissmyr]

Hi,

You don’t think it should be standard behavior to pad? I didn’t notice that something was wrong until someone used the Authenticator app to log in to our service. Anyways, if you want to make it a parameter then it’s totally fine of course.

I’ll add a test case tomorrow. Need to restructure the test setup a bit in order to set a fixed timestamp. The tests currently rely on Time.now.

Cheers, Johan

On 26 Apr 2014, at 12:26 am, Roberto Miranda notifications@github.com wrote:

I think would be better add this as a parameter of otp_code method.

def otp_code(time = Time.now, padded=false)

ROTP::TOTP.new(self.otp_column).at(time, padded) end Also could you add a test case with an example in the Readme?

Thanks

— Reply to this email directly or view it on GitHub.

[robertomiranda]

@brissmyr great!. lets add the pad behaviour as default :+1:

def otp_code(time = Time.now, padded=true)
   ROTP::TOTP.new(self.otp_column).at(time, padded)
end

[rderoldan1]

nice default behavior :+1:

[brissmyr]

Maybe we should use an options hash instead of fixed parameters? To avoid forcing people to send time = Time.now every time they want to change padding behaviour.

[robertomiranda]

:+1: sounds good, but anyway in the default options should be present the Time params. Also would be good to notify to the user that can send a integer(Epoch number) Insted if a time instance.

http://rubydoc.info/github/mdp/rotp/master/ROTP/TOTP:at

[brissmyr]

My initial suggestion was something like this:

def otp_code(options = {})
  time = options.fetch(:time, Time.now)
  padded = options.fetch(:padded, true)
  ROTP::TOTP.new(self.otp_column).at(time, padded)
end

However that would break current implementations unless I don't handle the single argument to also be a Time or Integer instance. I'll leave it as it is, and just add the extra parameter. I'll also add a test and update the README.

[robertomiranda]

great :grin:

[robertomiranda]

any progress on this?

[brissmyr]

Sorry for the delay. Just hit me with any feedback.

[guilleiguaran]

@brissmyr thanks <3