Comments (3)
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
from googleauthenticator.
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
from googleauthenticator.
I guess all comments/ideas are either fixed/implemented by you or me ;-)
from googleauthenticator.
Related Issues (20)
- hsusjs
- Verify myself verification information identification HOT 1
- Php authentication with authentication
- Change default secretLength length to 26
- can not log in
- Hello
- Cannot install, missing stable version HOT 2
- Still maintained? HOT 2
- PHP 8.2 issue with `${var}` declarations HOT 1
- [SECURITY] How to Report a Vulnerability? HOT 2
- chart.googleapis.com API deprecated
- recaptcha v2 not work HOT 1
- support config interval?
- Key not recognized
- verifyCode() method could be slightly improved HOT 3
- Still maintained? Or forked with more pull requests/features?
- https://accounts.google.com/signin/v2/identifier?flowName=GlifWebSignIn&flowEntry=ServiceLogin HOT 2
- authenticator key
- Gog
- Can not verify the code generated by Google Authenticator app after deploying the code on the server
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from googleauthenticator.