ErwinM / acts_as_tenant

Easy multi-tenancy for Rails in a shared database setup.
MIT License
1.56k stars 264 forks source link

Issues with default_url_options #334

Closed jimmypoulsen closed 8 months ago

jimmypoulsen commented 8 months ago

Hi! We're using ActsAsTenant to enable multi-tenancy in our application. Each tenant represents a market (eg Denmark, Finland ...). Each tenant has a host (eg "myapplication.dk" for Denmark, "myapplication.fi" for Finland ...). On each request, I override Rails' default URL options to ensure all links have the correct domain. However, sometimes our customers in Denmark experience being redirected to the Finnish site.

Here's my RequireTenant module. It's included in my base controller:

module RequireTenant
  extend ActiveSupport::Concern

  included do
    set_current_tenant_through_filter
    prepend_before_action :ensure_tenant!
  end

  def ensure_tenant!
    organization_tenant = Organization::Tenant.find_by(host: request.host)
    organization = organization_tenant.organization

    set_current_tenant(organization)
    Rails.application.routes.default_url_options[:host] = request.host
  rescue ActiveRecord::RecordNotFound => e
    set_test_tenant if Rails.env.test?
    set_development_tenant if Rails.env.development?
    render plain: "No tenant found for host: #{request.host}" if ActsAsTenant.current_tenant.nil?
  end

  def set_test_tenant
    set_current_tenant(Organization.find_by(country_code: 'DK'))
  end

  def set_development_tenant
    organization_tenant = Organization::Tenant.find_by!(host: 'app.lexly.dk.local')
    set_current_tenant(organization_tenant.organization)
    Rails.application.routes.default_url_options[:host] = request.host
  end
end

Can you guys see anything wrong with this code?

Ruby: 2.7.8 Rails: 6.0.3.3 ActsAsTenant: 0.5.1

anelcanto commented 8 months ago

There is a typo in find.by. It should be find_by in ...Organization::Tenant.find.by(host: request.host)

jimmypoulsen commented 8 months ago

@anelcanto Sorry for that. That's not the error. I removed some code and rewrote it a bit to make it more clear for people in here what I'm trying to do.

jimmypoulsen commented 8 months ago

Also, in the meantime, I searched a bit back and forth and noticed that I might have introduced a thread safety issue by modifying the global Rails.application.routes.default_url_options hash. To fix this I implemented the default_url_options in the module. Like this:

module RequireTenant
  ..

  def default_url_options
    { host: request.host }
  end

  ..

I think that ended up solving the issue. Waiting a couple more days to see.

wwvuillemot commented 7 months ago

Hi, I've got a similar situation. For now I've been using _path but I'd prefer to not have possible bugs if anyone uses _url, too.

Can you post your complete solution as reference?

FWIW, I found that just adding a protected method to ApplicationController appears to work

class ApplicationController < ActionController::Base
...
  protected

  def default_url_options
    { host: request.host }
  end

I worry about security, but I can't tell if my gut is right (and it is) or I'm just being paranoid, though. Obviously, I need to think through ActionMailer.default_url_options, too.

Rails and no single source of truth for default_url_options ... 😛

jimmypoulsen commented 6 months ago

@wwvuillemot Sorry for the late reply here!

My solution is almost identical to yours. The only difference is that I put the default_url_options method inside a module (RequireTenant) which is included in my ApplicationController.