Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misleading comment on ValidateOpts.Skew #101

Open
OscarVanL opened this issue Jan 24, 2025 · 0 comments
Open

Misleading comment on ValidateOpts.Skew #101

OscarVanL opened this issue Jan 24, 2025 · 0 comments

Comments

@OscarVanL
Copy link

The comment on the Skew field on ValidateOpts is a bit misleading.

It says:

otp/totp/totp.go

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:

otp/totp/totp.go

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant