-
Notifications
You must be signed in to change notification settings - Fork 41
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
Default QRcode.js library issues #93
Comments
https://github.com/whomwah/rqrcode works as a gem for generating QR codes. |
https://github.com/kazuhikoarase/qrcode-generator is a more up-to-date js library for generating QR codes. |
Yes we should replace it but hard to say what's the good alternative here. Generating it server side could also be an interesting option. |
I would recommend removing the JS dependency entirely and going with the Currently https://github.com/devise-two-factor/devise-two-factor uses |
Yes I like the server-side option a lot. |
@pkarjala, that sounds awesome! Do you want to take a shot at this, or did you want someone else to look into it? |
I'm willing to but my time is a bit crunched at the moment. My hope is to come back to a fork I made and implement a number of suggestions to turn into pull requests. |
@pkarjala, if you are tied up, then I'll take a quick shot at it. I think it is an excellent option, and very worthwhile to get us up to speed with importmaps. Let me know if you want me to hold off for any reason. |
Please go ahead and work on it! My only input is that I would forego using JS entirely and just have it render the QR code server side using Here's my current implementation to jump off of: ### otp_rqrcode_image_generator
# Generates a QR code PNG image based on a URL using the rqrcode gem
#
# @param otp_url A string with the URL of the OTP code
# @return A string encoded in base64 of a PNG or a blank string if no URL was provided.
def otp_rqrcode_image_generator(otp_url)
return '' if otp_url.blank?
return RQRCode::QRCode.new(otp_url).as_png(resize_exactly_to: 300).to_data_url
end |
Done. This eliminates all JavaScript and Sprockets dependencies. FYI, I went with the SVG option, as that eliminates additional dependencies within rqrcode (vs. the png route). I realized some styling was missing, though, so I have added some default CSS to reapply a max-width, along with some centering), and updated the README. @strzibny, let me know your thoughts! |
Merged. I think we simply can suggest the styling directly also in README since it might be easier for people to copy&paste. |
When installing devise-otp into a fresh Rails 7.1 project using
sprockets
for asset compilation andimportmaps
, the default included QRCode.js library does not properly operate and generate QR codes.Relevant Gems:
application.js
file:When set up using the instructions in the default readme, the assets compile without issue. When logging in and viewing the
otp/token/edit
page, the QR code does not render.Inspecting the page where the QR code would be rendered displays the following JS console errors:
The first error points to the script tag generated in the devise-otp edit view where the QR code is supposed to be rendered, as follows (with sensitive data obfuscated):
The second set of errors refer to the qrcode.js file itself, referring to this specific line reference:
https://github.com/wmlele/devise-otp/blob/master/app/assets/javascripts/qrcode.js#L283
This is a known unfixed issue in the qrcode.js library: davidshimjs/qrcodejs#292
As a result of these errors, the qrcode.js library is unable to render the QR code for the 2FA authentication.
There are a few possible solutions I can see to this issue.
This may require fixes to the qrcode.js library itself, or changes to the way that the asset is referenced in using Importmaps or other Rails 7.1 tooling. I don't think it's an asset compilation issue at this time, but I'm also not 100% certain. There is also a https://www.npmjs.com/package/qrcodejs2-fixes package which repairs the
_android
issue listed above.https://github.com/davidshimjs/qrcodejs has not seen any commits for 9 years and appears unmaintained, so it may not be desirable to continue to use it. It may be better to use a drop-in replacement, such as the suggested
jquery-qrcode
library in the https://github.com/wmlele/devise-otp/blob/master/docs/QR_CODES.md document (with the caveat that the user now must use jquery).3. Remove the qrcode.js library and use the already required dependencyrotp
to generate QR codesThe suggestion at #90 (comment) notes that this is an option.This would resolve any js dependencies entirely by removing them, and could simplify the gem. The caveat is that it now would need to render the qrcode server-side and serve it to the browser, which has different implications, and may not be desirable by everyone.
This was mistaken; rotp cannot generate QR codes, just the URIs that you can feed into a QR code generator.
Pick a completely different gem to handle the client or browser side QR code rendering.
I'll continue to poke at this bug and report back with any additional findings. I suspect I'm going to override the current
otp_authenticator_token_image
call with something else.The text was updated successfully, but these errors were encountered: