Generate QR codes internally #388
Conversation
4a480cb to
ef988bf
Compare
|
Thanks for the pull request @mallorydxw! This is a much needed feature! Looks like the https://github.com/endroid/qr-code library is licensed under MIT so it should be compatible with GPL2+ as far as I understand. The only issue is with the potential namespace conflicts. Bundling this library with the two-factor plugin means that it would trigger a PHP error if the same library is used either by another plugin or theme. This gets worse with the additional dependencies required by the library itself. Is there a way we could fork just the core functionality required for generating the most basic QR code and make it use the core WP image manipulation APIs? |
376bcb8 to
b3991a1
Compare
The latest versions of this library do not support PHP 5.6 so we're using version 1.0.3, the last release of the 1.x branch. Command used to clone this code: ``` git clone https://github.com/Bacon/BaconQrCode.git git -C BaconQrCode switch --detach 1.0.3 cp -r BaconQrCode/src/BaconQrCode includes ``` Fortunately the .php files in the library contain documentation on the files' origin and licence so we do not need to add that information. Link: https://github.com/Bacon/BaconQrCode/tree/1.0.3
This prevents namespace collisions with any other plugins or themes
using the BaconQrCode library.
The following command was used to replace all instances of `BaconQrCode`
with `TwoFactor\BaconQrCode` except when the line started with ` *`
(these lines are comments):
```
perl -pi -e 'if (!/^ \\*/) {s/BaconQrCode/TwoFactor\\BaconQrCode/g}' includes/BaconQrCode/**/*.php
```
This is for the BaconQrCode library which would otherwise require loading several different files.
- Creates an admin-ajax endpoint for displaying arbitrary values as QR codes - Uses the admin-ajax endpoint instead of the Google Chart API endpoint for displaying the QR code for setting up TOTP - This could be improved upon by storing the otpauth:// URI in the database instead of allowing any string to be passed to the QR code library Resolves: WordPress#42
b3991a1 to
b4e116d
Compare
|
Thanks for the direction, @kasparsd!
I noticed the I've copied the BaconQrCode library to the
I couldn't see how to use those APIs to create images, rather than merely resize or crop images. But I did notice that we could drop the requirement on the GD extension by generating SVG images instead of PNGs.
The BaconQrCode library is BSD 2 clause so I believe that's compatible too. The code is thankfully already commented with licence declarations in each and every |
Namespaces should be scoped to prevent such well-known issues. |
|
Thanks @DanielRuf that's useful to know. Anyway, I think this PR may be ready to be merged. The namespace has been scoped. It doesn't rely on Please let me know if there's any further improvements I need to make before you're ready to merge it. |
|
@kasparsd any thoughts on a review of this PR and whether it's ready for consideration in v0.8.0? |
|
The generated QR code was not accepted in the OTP clients I tested. I also have a suggestion. The QR code is a SVG image. This could be directly embedded into the HTML. This would remove the need for the AJAX request. |
|
Fixed the issues flagged by @walterebert here: https://github.com/WordPress/two-factor/tree/dxw-feature/generate-qr-internally. @kasparsd Do you think we should commit the library directly here or add to composer? |
|
Per yesterday's bug scrub, @georgestephanis is in favor of being able to generate QR codes internally but we're going to punt this to a future release (ahead of a core merge proposal) so the 0.8.0 focus can narrow on the U2F deprecation. |
|
IMO, it'd be best to use a JavaScript library, so that we're not running a bunch of 3rd party code on the server unnecessarily. The library mentioned in #42 seems like a good one, but there may be a few others in npm that are lean, popular, and well-tested. If an SVG is used, it'd be best to put it in a CSS Installing via npm instead of forking would give us npm/GitHub notifications if there's a security vuln discovered in it in the future. Those changes would help with the eventual merge to Core, since the standards there for 3rd party libraries are very high. |
These commits generate QR codes via PHP instead of via the Google Chart API. This stops sending sensitive data to a third party.
Resolves: #42
Note that this uses the endroid/qr-code composer library, so to test this please run
composer installafter checking this branch out.I suspect there's a standard WordPress way of including dependencies but I'm not sure what it is. I can update the PR if you let me know.
I would suggest storing the
otpauth://URI in the database instead of allowing passing arbitrary strings to the QR code generator. Just in case. But I haven't done that in this PR. Let me know if that's something you'd want to see before approving this, or if it ought to be raised as a separate PR.Thanks.