-
Notifications
You must be signed in to change notification settings - Fork 250
Misleading comment on ValidateOpts.Skew #101
Description
The comment on the Skew field on ValidateOpts is a bit misleading.
It says:
Lines 68 to 71 in 5971b1e
| // Periods before or after the current time to allow. Value of 1 allows up to Period | |
| // of either side of the specified time. Defaults to 0 allowed skews. Values greater | |
| // than 1 are likely sketchy. | |
| Skew uint |
If I decide "ok I'm happy with the defaults" and call Validate (rather than ValidateCustom), it actually uses a default Skew of 1:
Lines 34 to 50 in 5971b1e
| // Validate a TOTP using the current time. | |
| // A shortcut for ValidateCustom, Validate uses a configuration | |
| // that is compatible with Google-Authenticator and most clients. | |
| func Validate(passcode string, secret string) bool { | |
| rv, _ := ValidateCustom( | |
| passcode, | |
| secret, | |
| time.Now().UTC(), | |
| ValidateOpts{ | |
| Period: 30, | |
| Skew: 1, | |
| Digits: otp.DigitsSix, | |
| Algorithm: otp.AlgorithmSHA1, | |
| }, | |
| ) | |
| return rv | |
| } |
I suppose the comment is technically correct, because if I use ValidateCustom together with the zero-value of the ValidateOpts struct it will use a Skew value of 0, but I originally misinterpreted this to mean the package's default Skew was 0.
If you only read the docs, and ignore the package's internal implementation here you'll see what I mean. There's nothing to indicate what the default Skew is except the comment on ValidateOpts, so it would be fair to assume this is the default for Validate.
Since we shouldn't make a backwards-incompatible change to either ValidateOpts or Validate for API stability, I suppose this detail should be clarified via a more specific comment?