ErwinM / acts_as_tenant

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

`current_tenant` leakage? Double hit spec fails. #207

Closed nitsujri closed 5 years ago

nitsujri commented 5 years ago

Hi all, thanks Erwin for this great gem. I found when I do the same test twice in rspec I get hit with an error:

    let(:account) { create :account }
    let(:user) { create :user, account: account }

    subject { get :show, params: { id: account.id } }

    before { sign_in user }

    it 'shows account for logged in user' do
      subject

      expect(response).to have_http_status(:success)
    end

    it 'shows account for logged in user' do
      subject

      expect(response).to have_http_status(:success)
    end

The spec creates an account, attaches a user and visits a show page twice in a row. The first will pass and any subsequent will fail.

Under the hood what's happening is ActsAsTenant.current_tenant = <Account id:1> in the first run and in the 2nd pass, it remains 1 and does not get cleared. This is documented in the testing procedure (The wording is quite confusing in the first paragraph, but this at least shows the first sentence is true, we need to set clear current_tenant).

My question is why is the necessary? This makes me believe that ActsAsTenancy is not request safe?

Specifically my concern is, am I expected to wrap my entire request in an around_action and use the block version of set_tenant?

FWIW, the problem did not show up in SQLite, but only in Postgres which does not allow overriding of old primary keys.

ErwinM commented 5 years ago

When AaT is running on a server the tenant does not survive between requests.

The reason it needs to be cleared manually in some testing scenarios has to do with the request_store gem used for storing the tenant in a thread safe manner. I forget the details, but from what I remember that gem injects a clearing mechanism at the RACK level, which is not always used in testing.