wmlele / devise-otp

Two Factors authentication for Devise using Time Based OTP/rfc6238 tokens.
MIT License
240 stars 43 forks source link

Default QRcode.js library issues #93

Closed pkarjala closed 1 month ago

pkarjala commented 3 months ago

When installing devise-otp into a fresh Rails 7.1 project using sprockets for asset compilation and importmaps, the default included QRCode.js library does not properly operate and generate QR codes.

Relevant Gems:

application.js file:

// Configure your import map in config/importmap.rb. Read more: https://github.com/rails/importmap-rails
// Bootstrap dependencies for Sprockets
//= require popper
//= require bootstrap-sprockets
// Devise-otp dependencies
//= require devise-otp

import "jquery";
import "select2";
import Rails from "@rails/ujs";

Rails.start()

window.addEventListener('DOMContentLoaded', () => {
  $(".select2").select2();
})

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:

Uncaught ReferenceError: QRCode is not defined
    at edit:79:15

Uncaught TypeError: Cannot read properties of undefined (reading '_android')
    at application-1c9eb959d4e521bbab99cec2bb53e7fb4bd94091d1906d7b80bc0dd645840c15.js:5189:12
    at application-1c9eb959d4e521bbab99cec2bb53e7fb4bd94091d1906d7b80bc0dd645840c15.js:5364:4
    at application-1c9eb959d4e521bbab99cec2bb53e7fb4bd94091d1906d7b80bc0dd645840c15.js:5515:3

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):

//<![CDATA[

              new QRCode("qrcode", {
                text: "otpauth://totp/MY-APPLICATION:user%40example.com?secret=SOMELONGSECRETTEXTSTRING&issuer=MY-APPLICATION",
                width: 256,
                height: 256,
                colorDark : "#000000",
                colorLight : "#ffffff",
                correctLevel : QRCode.CorrectLevel.H
              });

//]]>

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: https://github.com/davidshimjs/qrcodejs/issues/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.

  1. Fix the devise-otp gem's qrcode.js library so that it operates under Rails 7.1

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.

  1. Replace the qrcode.js library with a different browser-side rendering library for QR codes in JS.

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 dependency rotp to generate QR codes

~~The suggestion at https://github.com/wmlele/devise-otp/issues/90#issuecomment-2303435473 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.

  1. Replace the qrcode.js library with a server-side QR code rendering library in a different third party gem.

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.

pkarjala commented 3 months ago

https://github.com/whomwah/rqrcode works as a gem for generating QR codes.

pkarjala commented 3 months ago

https://github.com/kazuhikoarase/qrcode-generator is a more up-to-date js library for generating QR codes.

strzibny commented 3 months ago

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.

pkarjala commented 3 months ago

I would recommend removing the JS dependency entirely and going with the rqrcode solution of generating it server-side, while leaving some hooks or overrides available for someone to be able to drop in their own preferred replacement.

Currently https://github.com/devise-two-factor/devise-two-factor uses rqrcode for generating the QR codes.

strzibny commented 3 months ago

Yes I like the server-side option a lot.

strouptl commented 2 months ago

@pkarjala, that sounds awesome! Do you want to take a shot at this, or did you want someone else to look into it?

pkarjala commented 2 months ago

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.

strouptl commented 2 months ago

@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.

pkarjala commented 2 months ago

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 rqrcode. Maybe provide an override option for folks who would prefer to use JS, but just bake the default functionality into the gem using rqrcode.

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
strouptl commented 2 months ago

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!

strzibny commented 1 month ago

Merged. I think we simply can suggest the styling directly also in README since it might be easier for people to copy&paste.