-
Notifications
You must be signed in to change notification settings - Fork 10
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
Support external clock mocking. #17
Conversation
@@ -15,7 +15,7 @@ import ( | |||
"github.com/benbjohnson/clock" | |||
) | |||
|
|||
var time clock.Clock | |||
var timeSource = clock.New() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that this would be muc better on the otp
struct rather than a global.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to jump in randomly, but without this patch, twofactor
is broken and as a consequence browserpass
is broken and OTP doesn't work for me on Firefox :D
@colemickens it can't be a field in the struct because the current twofactor
API does not require any instantiation. You can just call twofactor.FromURL(url)
, so the otp
struct's field would be uninitialised.
@kisom can we get this PR merged? If you'll accept my random PR approvals, LGTM :)
@@ -166,3 +166,7 @@ func totpFromURL(u *url.URL) (*TOTP, string, error) { | |||
func (otp *TOTP) QR(label string) ([]byte, error) { | |||
return otp.OATH.QR(otp.Type(), label) | |||
} | |||
|
|||
func SetClock(c clock.Clock) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then this could be func (otp *TOTP) SetClock(c clock.Clock) { ... }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@colemickens could you create a new PR with your suggestions? |
No description provided.