wmlele/devise-otp

Feedbacks with Rails 7

n-rodriguez opened this issue · 5 comments

n-rodriguez commented

Hi there!

First of all thanks for this great gem!

It's the first time I setup OTP on an application and globally everything went smooth except for 2 or 3 small things :)

First I had to patch engine.rb because config.assets raise NoMethodError :

nicolas@laptop:~/PROJECTS/CONCERTO/concerto$ bin/rails s
=> Booting Rails
Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead.
/home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/railties-7.0.2.2/lib/rails/railtie/configuration.rb:96:in `method_missing': undefined method `assets' for #<Rails::Engine::Configuration:0x00007f2e2d51ed40 @root=#<Pathname:/home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/bundler/gems/devise-otp-908c41ef85fe>, @generators=#<Rails::Configuration::Generators:0x00007f2e2d51ed18 @aliases={}, @options={:rails=>{:orm=>:active_record, :test_framework=>:test_unit, :integration_tool=>:test_unit, :system_tests=>:test_unit}, :active_record=>{:migration=>true, :timestamps=>true}, :test_unit=>{:fixture=>true, :fixture_replacement=>nil}}, @fallbacks={}, @templates=[], @colorize_logging=true, @api_only=false, @hidden_namespaces=[], @after_generate_callbacks=[]>, @middleware=#<Rails::Configuration::MiddlewareStackProxy:0x00007f2e2d51eb60 @operations=[], @delete_operations=[]>, @javascript_path="javascript"> (NoMethodError)
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/bundler/gems/devise-otp-908c41ef85fe/lib/devise_otp_authenticatable/engine.rb:18:in `<class:Engine>'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/bundler/gems/devise-otp-908c41ef85fe/lib/devise_otp_authenticatable/engine.rb:2:in `<module:DeviseOtpAuthenticatable>'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/bundler/gems/devise-otp-908c41ef85fe/lib/devise_otp_authenticatable/engine.rb:1:in `<main>'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/bootsnap-1.10.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/bootsnap-1.10.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/zeitwerk-2.5.4/lib/zeitwerk/kernel.rb:35:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/bundler/gems/devise-otp-908c41ef85fe/lib/devise-otp.rb:78:in `<main>'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/bootsnap-1.10.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/bootsnap-1.10.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/zeitwerk-2.5.4/lib/zeitwerk/kernel.rb:35:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/site_ruby/3.1.0/bundler/runtime.rb:60:in `block (2 levels) in require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/site_ruby/3.1.0/bundler/runtime.rb:55:in `each'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/site_ruby/3.1.0/bundler/runtime.rb:55:in `block in require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/site_ruby/3.1.0/bundler/runtime.rb:44:in `each'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/site_ruby/3.1.0/bundler/runtime.rb:44:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/site_ruby/3.1.0/bundler.rb:176:in `require'
	from /home/nicolas/PROJECTS/CONCERTO/concerto/config/application.rb:10:in `<main>'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/bootsnap-1.10.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/bootsnap-1.10.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/railties-7.0.2.2/lib/rails/commands/server/server_command.rb:137:in `block in perform'
	from <internal:kernel>:90:in `tap'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/railties-7.0.2.2/lib/rails/commands/server/server_command.rb:134:in `perform'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/thor-1.0.1/lib/thor/command.rb:27:in `run'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/thor-1.0.1/lib/thor/invocation.rb:127:in `invoke_command'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/thor-1.0.1/lib/thor.rb:392:in `dispatch'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/railties-7.0.2.2/lib/rails/command/base.rb:87:in `perform'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/railties-7.0.2.2/lib/rails/command.rb:48:in `invoke'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/railties-7.0.2.2/lib/rails/commands.rb:18:in `<main>'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/bootsnap-1.10.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
	from /home/nicolas/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/bootsnap-1.10.3/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in `require'
	from bin/rails:4:in `<main>'
-    config.assets.precompile += %w(devise-otp.js)
+    initializer "devise-otp", group: :all do |app|
+      # check if Rails api mode
+      if app.config.respond_to?(:assets)
+        puts "hello"
+        if defined?(Sprockets) && Sprockets::VERSION >= "4"
+          puts "world"
+          app.config.assets.precompile << "devise-otp.js"
+        else
+          # use a proc instead of a string
+          app.config.assets.precompile << proc { |path| path == "devise-otp.js" }
+        end
+      end
+    end

Inspired by https://github.com/ankane/pghero/blob/master/lib/pghero/engine.rb which I also use in my project.

With this patch everything works well :

nicolas@tchoum-laptop:~/PROJECTS/CONCERTO/concerto$ bin/rails s
=> Booting Rails
Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead.
=> Checking config
=> Starting app (development)
=> Booting Puma
=> Rails 7.0.2.2 application starting in development 
=> Run `bin/rails server --help` for more startup options
hello
world
[343741] Puma starting in cluster mode...
[343741] * Puma version: 5.6.2 (ruby 3.1.0-p0) ("Birdie's Version")
[343741] *  Min threads: 5
[343741] *  Max threads: 5
[343741] *  Environment: development
[343741] *   Master PID: 343741
[343741] *      Workers: 2
[343741] *     Restarts: (✔) hot (✖) phased
[343741] * Preloading application
[343741] * Listening on http://0.0.0.0:5000
[343741] Use Ctrl-C to stop
[343741] - Worker 0 (PID: 343888) booted in 0.0s, phase: 0
[343741] - Worker 1 (PID: 343895) booted in 0.0s, phase: 0
^C[343741] - Gracefully shutting down workers...
[343741] === puma shutdown: 2022-02-19 12:20:46 +0100 ===
[343741] - Goodbye!
Exiting

The second issue is with otp_authenticator_token_image method called here:

devise-otp/app/views/devise_otp/devise/tokens/_token_secret.html.erb

Line 4 in 908c41e

<%= otp_authenticator_token_image(resource) %>
(actually in my own template) and defined here:

devise-otp/lib/devise_otp_authenticatable/controllers/helpers.rb

Line 135 in 908c41e

def otp_authenticator_token_image(resource)
which calls otp_authenticator_token_image_js

The issue with otp_authenticator_token_image_js is that it adds some inlined JS without a nonce which is forbidden by my CSP policy.

To workaround this I override otp_authenticator_token_image in my ApplicationHelper to call otp_authenticator_token_image_google :

  def otp_authenticator_token_image(resource)
    otp_authenticator_token_image_google(resource.otp_provisioning_uri)
  end

which adds an image tag (instead of inlined JS) and configure secure_headers to allow chart.googleapis.com :

config.csp[:img_src] << 'chart.googleapis.com'

The question is : is it something expected? if so, maybe it should be added to the doc.

strzibny commented

Hi @n-rodriguez, I took over the maintenance a while ago so slowly preparing a new release with Rails 7 support. Thanks for your feedback!

I didn't see the issues you described but we should fix them!

strzibny commented

@n-rodriguez it's good to link issue <> PRs since I missed you submitted changes.

Related PR: #48

strzibny commented

So the sprockets issue is merged.

Would you update README about the CSP issue?

n-rodriguez commented

Would you update README about the CSP issue?

Done! Thank you!

  • 👍1
strzibny commented

Merged, so closing this issue.