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

Exception when code type is integer (expected string) #38

Closed
Zaczero opened this issue Aug 8, 2019 · 1 comment

Comments

@Zaczero
Copy link

commented Aug 8, 2019

Actual behavior

While passing code parameter to verifyCode function and the code parameter is integer type the function crashes.

Expected behavior

verifyCode function verifies the code which is provided as integer.

Code to reproduce

$tfa = new RobThree\Auth\TwoFactorAuth("TEST");
$tfa->verifyCode("USJNHRTSCEAY2OME6IB7CLEE6HACYQGN", 123123);


Workaround

$tfa = new RobThree\Auth\TwoFactorAuth("TEST");
$tfa->verifyCode("USJNHRTSCEAY2OME6IB7CLEE6HACYQGN", (string) 123123);

Edit.

In case of the code starting by 0 the verifyCode function should prepend missing zeros based on the code generation length or drop the leading zeros from internal code generation. OR maybe just switch completely to integers as working with them is faster than strings = less cpu ticks = less co2 emission 🚭

@RobThree RobThree self-assigned this Aug 8, 2019

@RobThree RobThree added the wontfix label Aug 8, 2019

@RobThree

This comment has been minimized.

Copy link
Owner

commented Aug 8, 2019

Uh, no. The code is, and should be, a string. Just because it only contains digits doesn't make it an integer. You already discovered that leading zeroes will become a problem. Just like a phone number (which also, usually, only contains numbers) is a string, this verification code is supposed to be a string as well.

Also your argument about less cpu ticks is questionable at best. If we're measuring CPU cycles than the hash functions will use orders of magnitude more; switching to integers as an 'optimisation' is a micro-optimisation at best.

If anything, I should probably add typehinting someday and ensure all arguments are of the correct type.

@RobThree RobThree closed this Aug 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.