wmlele / devise-otp

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

A few questions: otp drift, provisioning id, additional TOTP sources? #28

Open eimermusic opened 9 years ago

eimermusic commented 9 years ago

Hi, I just found this gem and started reading the code. It does looks cleaner and more understandable than most I have seen.

I hope you don't mins me asking a few questions about specifics in the code?

Drift

Is there a specific reason you do not let ROTP handle drift? The behaves is not identical to your code so I thought there might be some specific reason. I played around with the drift values a bit in a fork: https://github.com/wmlele/devise-otp/compare/master...eimermusic:feature/use-rotp-drift?expand=1

Provisioning ID

The provisioning identifier is visible to end-users in many authenticator apps. It is sometimes the only thing identifying which website or application the code is generated for. This gem defaults to the email address only. The downside is that it shows me which of my 2 logins this is for but not which website or app I am supposed to use it with.

Would you consider making the default something like ApplicationName:email? It would perhaps primarily work automatically for Rails. Alternatively maybe raising an error, requiring the application developer to implement this method and making it clear that the id is part of the end-user experience?

For Rails it is as simple as this if you think the application module name is relevant for most apps.

    def otp_provisioning_identifier
      "#{Rails.application.class.parent_name}:#{email}"
    end

Some sites I have TOTP logins for do the following for their ids:

The change would not be huge but I am of-course offering to supply a PR if you want one.

wmlele commented 9 years ago

I am on vacation until next week, when I am back I am going to revamp this project including the pr already pending.

----- Reply message ----- From: "Martin Westin" notifications@github.com To: "wmlele/devise-otp" devise-otp@noreply.github.com Subject: [devise-otp] A few questions: otp drift, provisioning id, additional TOTP sources? (#28) Date: Fri, Aug 28, 2015 14:38

Hi,

I just found this gem and started reading the code. It does looks cleaner and more understandable than most I have seen.

I hope you don't mins me asking a few questions about specifics in the code?

Drift

Is there a specific reason you do not let ROTP handle drift? The behaves is not identical to your code so I thought there might be some specific reason. I played around with the drift values a bit in a fork:

https://github.com/wmlele/devise-otp/compare/master...eimermusic:feature/use-rotp-drift?expand=1

Provisioning ID

The provisioning identifier is visible to end-users in many authenticator apps. It is sometimes the only thing identifying which website or application the code is generated for. This gem defaults to the email address only. The downside is that it shows me which of my 2 logins this is for but not which website or app I am supposed to use it with.

Would you consider making the default something like ApplicationName:email? It would perhaps primarily work automatically for Rails. Alternatively maybe raising an error, requiring the application developer to implement this method and making it clear that the id is part of the end-user experience?

For Rails it is as simple as this if you think the application module name is relevant for most apps.

def otp_provisioning_identifier "#{Rails.application.class.parent_name}:#{email}" end

Some sites I have TOTP logins for do the following for their ids:

github.com/username Dropbox:email Google:email username@account (this is AWS but that is up to me to remember that detail)

The change would not be huge but I am of-course offering to supply a PR if you want one.

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