Several bugs / comments about googleauthenticator

[PHPGangsta]

Hi Rob,

I wrote this a long time ago, let's see:

Your first suggestion is not correct I think. $currentTimeSlice is a "slice", not a timestamp. This slice is a "range" of 30 seconds, see line 91, the current time is devided by 30. That's why "just" $i is added and not $i*30. So I think the current code is correct.

Your second comment would be correct if the otpauth://-URL would be directly used somewhere. But here it is added as a GET-Parameter to the Google-URL, that's why it has to be completely urlencoded. In your version you end up having two question marks in the resulting URL-string.
You are right regarding the additional parameter "Issuer" that should be added. The Counter is just used if the type is hotp, but we are doing totp here.

Your version of createSecret() adds an addition external dependency on OpenSSL. Not all systems have it installed. There is a nice Pull Request from Steve, he is only using openssl_random_pseudo_bytes() if it exists, otherwise he falls back to array_rand(), which I would say is a better solution:
#10

Good catch regarding the "nipple" ;-)

You are right, _base32Encode() is not used in the current version of the code, I think it was used in one of the first (internal) versions before putting it on GitHub. It can be removed.

I'm not sure if I want to have an external dependency on PHPQRCode. It's of cause a good idea not to use Google and external HTTP requests, but it adds an external dependency to a library not being on GitHub as far as I can see (it's on sourceforge).

Thank you for your suggestions! I would like to hear from you!

Do you want to send a pull request regarding the "nipple" and "base32Encode()", or should I do the necessary changes?

Michael

[RobThree]

Your first suggestion is not correct I think. $currentTimeSlice is a "slice", not a timestamp. This slice is a "range" of 30 seconds, see line 91, the current time is devided by 30. That's why "just" $i is added and not $i*30. So I think the current code is correct.

Looking over this again I see you are correct. Must've misread it or had one too many beer 😜

Your second comment would be correct if the otpauth://-URL would be directly used somewhere. But here it is added as a GET-Parameter to the Google-URL, that's why it has to be completely urlencoded. In your version you end up having two question marks in the resulting URL-string.

urlencode should always be used on single values. The parameters are all, and each, a single value that requires encoding. However, you then send the entire url as as single value (parameter) to Google. Because the entire URL is a single value it requires URLEncoding (again). You should be able to test/reproduce this quite easily (use a (nonsensical) name like test & testing = ?foo? for example). Your method should display incorrectly in the authenticator app (if it doesn't it may compensate for this (common) mistake; try other apps too!).

You are right regarding the additional parameter "Issuer" that should be added. The Counter is just used if the type is hotp, but we are doing totp here.

I never implemented totp either so I left that one out too. It was just a suggestion (though I would also consider Algorithm and Digits) as I mentioned.

Your version of createSecret() adds an addition external dependency on OpenSSL. Not all systems have it installed.

That is a good, fair, point. I would, however, consider allowing a "pluggable" random-bytes-provider so people can easily provide the function with their own provider of choice (much like I support different QR code providers by providing a simple interface. I am by no means very skilled in PHP (I'm more of a .Net kinda-guy) so I wasn't aware that openssl_random_pseudo_bytes() isn't some "built-in BCL-sort-of functionality" but relies on Cryptography Extensions being installed. Update: Implemented it :)

There is a nice Pull Request from Steve, he is only using openssl_random_pseudo_bytes() if it exists, otherwise he falls back to array_rand(), which I would say is a better solution:

This could very well be. As said: I'm not much of a PHP guy so all I can say is that, to me, it at least looks better than the current implementation. (Edit: However, I have commented on that PR as well 😝 )

Good catch regarding the "nipple" ;-)

A dirty mind is a joy forever 😜

You are right, _base32Encode() is not used in the current version of the code, I think it was used in one of the first (internal) versions before putting it on GitHub. It can be removed.

Yay! 😜

I'm not sure if I want to have an external dependency on PHPQRCode. It's of cause a good idea not to use Google and external HTTP requests, but it adds an external dependency to a library not being on GitHub as far as I can see (it's on sourceforge).

As said: why not give people a choice. It's part of good SOLID design 😜

Thank you for your suggestions! I would like to hear from you!

You're welcome!

Do you want to send a pull request regarding the "nipple" and "base32Encode()", or should I do the necessary changes?

By all means go ahead; I'm currently not in a position to do (well tested) PR's.

Rob

[PHPGangsta]

I guess all comments/ideas are either fixed/implemented by you or me ;-)