ErwinM / acts_as_tenant

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

Fix test_tenant leaking into current_tenant #278

Open artemave opened 2 years ago

artemave commented 2 years ago

Using test_tenant in conjunction with with_tenant {} and without_tenant {} in some cases leads to test_tenant being assign to current_tenant which leads to subtle bugs, that are difficult to pin down.

This happens because the value of test_tenant is restored as current_tenant in the ensure part of those methods. The test cases in this pr illustrate the issue.

excid3 commented 2 years ago

I'm pretty sure that's the expected behavior: https://github.com/ErwinM/acts_as_tenant#testing

refractalize commented 2 years ago

Hi @excid3

Just to give a bit more explanation to what's going on here. We start with test_tenant set to test_account, this behaves like this:

# start (set test_tenant = test_account)
RequestStore.store[:current_tenant] == nil
ActsAsTenant.test_tenant == test_account
ActsAsTenant.current_tenant == test_account

Then we might do a request, using ActsAsTenant::TestTenantMiddleware, which sets test_tenant to nil during the request. So far, everything is still fine.

# start request with ActsAsTenant::TestTenantMiddleware (set test_tenant = nil)
RequestStore.store[:current_tenant] == nil
ActsAsTenant.test_tenant == nil
ActsAsTenant.current_tenant == nil
# finish request with ActsAsTenant::TestTenantMiddleware (reset test_tenant = test_account)

Now we do a with_tenant block. This is where things start to go wrong.

ActsAsTenant.with_tenant(account1) do
  RequestStore.store[:current_tenant] == account1
  ActsAsTenant.test_tenant == test_account
  ActsAsTenant.current_tenant == account1
end

At the end of with_tenant, the RequestStore.store[:current_tenant] is set to the test_tenant... but, it still behaves correctly, for now.

RequestStore.store[:current_tenant] == test_account # <--- not correct!
ActsAsTenant.test_tenant == test_account
ActsAsTenant.current_tenant == test_account # <--- but still ok...

We do another request, again test_tenant is set to nil, BUT this time RequestStore.store[:current_tenant] is set, so current_tenant looks like it's set!

# start request with ActsAsTenant::TestTenantMiddleware (set test_tenant = nil)
RequestStore.store[:current_tenant] == test_account
ActsAsTenant.test_tenant == nil
ActsAsTenant.current_tenant == test_account # wrong!
# finish request with ActsAsTenant::TestTenantMiddleware (reset test_tenant = test_account)

In short, what we're seeing is that with_tenant sets the RequestStore.store[:current_tenant] and doesn't reset it afterwards, which affects any further requests that rely on ActsAsTenant::TestTenantMiddleware. In particular, requesting routes that don't expect a tenant (i.e. routes that show multiple tenants) now have a tenant set.