ROTP 4.0 Proposal
mdp opened this issue · 7 comments
The API has gotten a bit verbose with non-breaking changes. For 4.0 I'd like to simplify it a bit more.
totp = ROTP::TOTP.new 'wrn3pqx5uqxqvnqr'
totp.verify(code, {drift: 30, prior: 0, at: Time.now }) #=> timestampThoughts and ideas?
Probably don't need the extra braces, right?
I have a slight preference for staying backward compatible by retaining the optional second positional arg for time. It makes upgrading a lot smoother for non-advanced users. We could support both ways, actually, although that might be confusing. Maybe deprecate the second positional arg but still allow it for now.
So we'd have
totp.verify(code, drift: 30, prior: 0, at: Time.now) #=> timestamp (preferred)or
totp.verify(code, Time.now, drift: 30, prior: 0) #=> timestamp (deprecated)And we might declare that as
def verify(otp, time = nil, drift: nil, prior: nil, at: nil)
at ||= time || Time.now
...I like the new style named parameters. I don't think maintain backwards compatible is worth it here. The cost of migration is very small, and anyone using this library should be paying close attention during updates anyway, since it is security critical.
Hey, just now getting around to taking a stab at this. Cleaned up the API along with the tests. Removed support for Ruby < 2.0.
New verify signature:
https://github.com/mdp/rotp/blob/v4.0/lib/rotp/totp.rb#L42
Thoughts?
This is all on the v4.0 branch - https://github.com/mdp/rotp/tree/v4.0
This looks excellent! Thanks!
Ok, final revision is up in the same branch. Primary thing I changed was 'drift'.
I think most people just want to allow for a recently expired code to be used (ex. when you enter the code just as it's about to change, then submit and it's expired). Now there is 'drift_behind' and 'drift_ahead' which allows users to be more explicit about what they wish to allow. Improved the testing and implementation on this as well.
I've also made the default secret key size 32, which matches what Google moved to (160 bits vs 80bits). Should be little impact on library users from this one.
Let me know what you think and I'll publish this as 4.0.0.rc1
Pushed this out finally