Skip to content
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

Configuring ActionCable allowed_request_origins #472

Open
nevans opened this issue May 23, 2023 · 3 comments
Open

Configuring ActionCable allowed_request_origins #472

nevans opened this issue May 23, 2023 · 3 comments
Labels
bug Something isn't working v2.x Issues/PRs that affect the v2.x line of releases

Comments

@nevans
Copy link

nevans commented May 23, 2023

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:

  • Lookbook: 2.0.2
  • ViewComponent: 3.0.0
  • Rails: 6.1.7.3
  • Ruby: 3.0.6

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
@nevans nevans added the bug Something isn't working label May 23, 2023
@allmarkedup
Copy link
Collaborator

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.

@allmarkedup allmarkedup added the v2.x Issues/PRs that affect the v2.x line of releases label May 23, 2023
@nevans
Copy link
Author

nevans commented May 23, 2023

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 allmarkedup added feature request A suggestion for a feature or enhancement bug Something isn't working and removed bug Something isn't working feature request A suggestion for a feature or enhancement labels May 30, 2023
@allmarkedup
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v2.x Issues/PRs that affect the v2.x line of releases
Projects
None yet
Development

No branches or pull requests

2 participants