lookbook-hq / lookbook

A UI development environment for Ruby on Rails apps ✨
https://lookbook.build
MIT License
889 stars 91 forks source link

Configuring ActionCable `allowed_request_origins` #472

Open nevans opened 1 year ago

nevans commented 1 year ago

Describe the bug

Configuring Lookbook's ActionCable (e.g. to add allowed_request_origins) is counter-intuitive and difficult to debug.

To Reproduce

Steps to reproduce the behavior:

  1. Add something like the following to an initializer:
    Rails.application.configure do
     config.action_cable.allowed_request_origins ||= []
     config.action_cable.allowed_request_origins << ExampleApp::Config.default_origin
     config.action_cable.allowed_request_origins << %r{\Ahttps?://localhost(:\d+)?\z}
    end
  2. Using one of the origins configured in step one, load Lookbook and view the logs. They will confusingly claim that the origin is not allowed. E.g:
    Request origin not allowed: https://username-name-asldkfjasl-3000.preview.app.github.dev
  3. Notice that the websocket path is /lookbook/cable, which helps me realize that Lookbook::Cable has its own configuration: https://github.com/ViewComponent/lookbook/blob/764cf8f2b44ee559a52fcd69cde1c48967d4803b/lib/lookbook/cable/cable.rb#L44-L51
  4. Replace the above configuration with something like the following:
    Lookbook.after_initialize do
     if (config = Lookbook.engine.websocket&.server&.config)
       config.allowed_request_origins ||= []
       config.allowed_request_origins << ExampleApp::Config.default_origin
       config.allowed_request_origins << %r{\Ahttps?://localhost(:\d+)?\z}
     end
    end
  5. Reload and see the same error message!?!
  6. After a lot of debugging, realize that engine.websocket.full_mount_path is now /cable, and instead of Lookbook::Cable and Lookbook::Connection it is now ActionCable::Server::Base and ActionCable::Connection::Base.
  7. After some more debugging, realize that accessing Lookbook.engine.websocket from an initializer calls the following method: https://github.com/ViewComponent/lookbook/blob/764cf8f2b44ee559a52fcd69cde1c48967d4803b/lib/lookbook/engine.rb#L119-L121 This is problematic:
    • mount_path comes from config/routes.rb but that loads after the initializers
    • @_websocket memoizes Lookbook::Cable with an empty engine_mount_path

Workaround

Place the following line at the bottom of config/routes.rb:

ActiveSupport::Notifications.instrument "routes_loaded.application"

And place the following into an initializer:

ActiveSupport::Notifications.subscribe "routes_loaded.application" do           
  Lookbook.engine.instance_variable_set(:@_websocket, nil)        
  if (config = Lookbook.engine.websocket&.server&.config)                                                  
    config.allowed_request_origins ||= []                                       
    config.allowed_request_origins << ExampleApp::Config.default_origin
    config.allowed_request_origins << %r{\Ahttps?://localhost(:\d+)?\z}
  end
end

Is there a better approach?

Expected behavior

A simple method for configuring Lookbook's ActionCable::Server::Configuration, documented in the Lookbook documentation. Or, if one exists and is documented elsewhere already, a link to that documentation.

Version numbers

Please complete the following information:

Additional context

I've never configured ActionCable before, so it's entirely possible I simply overlooked a much simpler approach that was documented and I couldn't find it.

To debug the issue, I used rdbg and something like the following:

module LookbookLoggedViewAssigns
  def view_assigns
    if defined?(@engine) && @engine
      logger.info {
        "Lookbook engine: %p" % [{
          mount_path:       @engine.mount_path,
          cable_mount_path: @engine.websocket.full_mount_path,
          config:           @engine.websocket.server.config.allowed_request_origins,
        }]
      }
    end
    super
  end
end

module LookbookLoggedActionCable
  def allow_request_origin?
    logger.info({
      class: self.class,
      server_class: server.class,
      config: server.config,
      mount_path: Lookbook::Engine.mount_path,
      cable_mount_path: Lookbook::Engine.websocket.full_mount_path,
      engine_cable_mount_path: Lookbook.engine.websocket.full_mount_path,
    }.inspect)
    super
  end
end

ActiveSupport.on_load(:action_controller_base) do
  prepend LookbookLoggedViewAssigns
end

ActiveSupport.on_load(:action_cable_connection) do
  prepend LoookbookLoggedActionCable
end
allmarkedup commented 1 year ago

Hey @nevans - many thanks for the super-detailed bug report and sorry you are running into frustrations here!

I'm sure that this can be cleaned up a bunch to make it more intuitive. I must admit when I first put it together I didn't at all consider people needing to customise the cable configuration - clearly an oversight. And there is some stuff in there that is a bit of a hangover from older ways of doing things in previous versions so I'm sure this can be improved a lot.

I'll try and take a look at it when I next have a chance - I'm moving house in a couple of days so it may not happen too quickly but if this is causing people headaches it's definitely something I'd like to improve. When I've got something worth looking at I'll open a PR and let you know.

Thanks again for taking the time to explain the issue so thoroughly, much appreciated.

nevans commented 1 year ago

Thanks! This workaround is working for me---so take your time! If anyone else has a similar issue, hopefully this issue and its workaround will be easy to find.

allmarkedup commented 1 year ago

@nevans okay great, glad it's not a blocker for you as I'm pretty time-poor at the moment but I'll let you know as soon as I have something to test out :-)