Apartment persists search_path when switching between schemas

[aliekens]

I'm using Apartment on a Postgres setup. I have noted that when I'm trying to access non-existing schemas, that the reported search_path is dependent on previous tenant switches, persisting some of these tenants (in the search_path?) among user sessions.

In the example below, I try to access a non-existing tenant, giving me the error that this schema is invalid and could not be found in the search_path of the non-existing tenant nor in public.

Next, I access an existing tenant, where Apartment correctly switches to this tenant.

Next, I try to access the non-existing tenant again, but now Apartment reports that it could not find the non-existing tenant in the existing tenant's schema. Apartment shouldn't be trying to access this other tenant when I'm trying to access a faulty one.

$ rails console
Loading development environment (Rails 4.0.4)
irb(main):001:0> Apartment::Database.switch("non_existing_tenant")
Apartment::SchemaNotFound: One of the following schema(s) is invalid: non_existing_tenant, "public"
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/apartment-0.24.3/lib/apartment/adapters/postgresql_adapter.rb:95:in `rescue in connect_to_new'
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/apartment-0.24.3/lib/apartment/adapters/postgresql_adapter.rb:88:in `connect_to_new'
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/apartment-0.24.3/lib/apartment/adapters/abstract_adapter.rb:100:in `switch'
    from (irb):1
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/railties-4.0.4/lib/rails/commands/console.rb:90:in `start'
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/railties-4.0.4/lib/rails/commands/console.rb:9:in `start'
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/railties-4.0.4/lib/rails/commands.rb:62:in `<top (required)>'
    from bin/rails:4:in `require'
    from bin/rails:4:in `<main>'
irb(main):002:0> Apartment::Database.switch("existing_tenant")
=> "\"existing_tenant\""
irb(main):003:0> Apartment::Database.switch("non_existing_tenant")
Apartment::SchemaNotFound: One of the following schema(s) is invalid: non_existing_tenant, "existing_tenant"
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/apartment-0.24.3/lib/apartment/adapters/postgresql_adapter.rb:95:in `rescue in connect_to_new'
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/apartment-0.24.3/lib/apartment/adapters/postgresql_adapter.rb:88:in `connect_to_new'
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/apartment-0.24.3/lib/apartment/adapters/abstract_adapter.rb:100:in `switch'
    from (irb):3
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/railties-4.0.4/lib/rails/commands/console.rb:90:in `start'
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/railties-4.0.4/lib/rails/commands/console.rb:9:in `start'
    from /Users/aliekens/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/railties-4.0.4/lib/rails/commands.rb:62:in `<top (required)>'
    from bin/rails:4:in `require'
    from bin/rails:4:in `<main>'

The problem popped up when I noticed I could gain access to a tenant when testing for the nil tenant. I'm using the Subdomain elevator and my app should give an error when accessing the nil domain, not give me full access to a previously used tenant.

[bradrobertson]

we don't clear out the previous tenant when an exception is thrown due to an invalid one. So what you're seeing is just that the last successfully switched tenant hasn't changed. It's not really a security issue because the only way into your app is through the subdomain elevator, so it will just switch as appropriate.

It might be useful to reset the tenant back to the default though on exception. You're welcome to add some tests and submit a pull request if you wanted to see that.

[aliekens]

Thanks for your reply. Unfortunately, the story doesn't end there and I feel there's still a huge security risk here ...

In my current project, when switching from an action on a valid tenant to a public action that has no tenant (e.g., a global API call, an action to admin the tenant, i18n management, ...) I'm using no subdomain (i.e., these actions are defined on myapp.com instead of tenant.myapp.com).

At this point, the subdomain elevator is still maintaining the last used tenant as its current tenant, prioritizing the previous tenant's search path over the public path, thus accessing tables in a non-public schema instead of the public schema, reading and writing records in the wrong tables.

Is this faulty behavior a faulty interpretation and implementation of mine or is this indeed a buggy result of remembering the last tenant of the subdomain elevator?

[bradrobertson]

We've never encountered this because all of our requests are within the context of a particular tenant. It may be that we should reset the current_tenant if you switch to an invalid one, I'll have to think about that, but normally the way you'd do this is provide a pass through in your middleware.

If you inherit from the Apartment Elevator with your own, you can customize it by passing through any non tenanted requests.

[aliekens]

Note that issue #134 points to the same problem.

[vanboom]

I might suggest that the system revert back to "public" or the default tenant if an exception is thrown when a tenant is missing. In my application, tenants may come and go, and switching to a valid tenant is getting blocked when an exception is thrown because of a missing tenant.

[rabbitt]

You should be able to revert the connection back to public after every request (please correct me if I'm wrong) by adding this to your elevator:

module Apartment
  module Elevetors
    class MyElevator < Generic
      def call(*args)
        super
      ensure 
        Tenant.reset
      end
    end
  end
end

[bradrobertson]

Ya one change I'm going to make to the Generic elevator actually is to use switch with a block, which basically does what you've suggested (uses ensure to make sure you go back to the previous tenant)

[rabbitt]

@bradrobertson, so you're thinking something like so?

# lib/apartment/elevators/generic.rb
      def call(env)
        . . . 
        begin
          previous = Tenant.current
          Tenant.switch! database if database
          @app.call(env)
        ensure
          Tenant.switch!(previous) rescue Tenant.reset 
        end
      end

or more along the lines of this:

# lib/apartment/adapters/abstract_adapter.rb
      def switch!(tenant = nil)
        ...
      end

      def switch(tenant = nil, &block)
        raise ArgumentError, 'block expected, none given' unless block_given?

        previous = current_tenant
        begin
          switch!(tenant)
          block.call
        ensure 
          switch!(previous) rescue reset
        end
      end
# lib/apartment/elevators/generic.rb
      def call(env)
        . . . 
        if database
          Tenant.switch(database) { @app.call(env) } 
        else 
          @app.call(env)
        end  
      end

[bradrobertson]

the 2nd one... Since switch with a block does the ensure we don't need to duplicate it in the middleware.

But the adapter code itself def switch is already implemented, somewhat similar to what you have so it's really just the middleware that should change

[rabbitt]

Ah - I probably should have looked at the more recent code and I would have seen the newer #switch() that you speak of. But yeah, I grok ya.

[hiattp]

I'm not sure if this is related but I'm seeing a seemingly related issue in my RSpec test suite where roughly 20% of the time (and only on our CI server) a test will fail with:

One of the following schema(s) is invalid: "company_1" "company_1", "shared_extensions"

I have Apartment::Tenant.reset in both before and after hooks (shouldn't be necessary?) and I've tried manually setting the search path (Apartment.connection.schema_search_path = "public,shared_extensions") in efforts to weed out this failure but so far no luck. I'm at a bit of a loss for what to try next. We are creating/using/tearing down schemas during the test cases (both within integration tests, which is where we usually see this failure, and before/after blocks. But it seems like sometimes this process breaks down somewhere and leaves a schema in place. This is my (seemingly over-aggressive) after block:

config.after(:each) do
  Apartment::Tenant.reset
  Apartment.connection.schema_search_path = "public,shared_extensions"
  Company.all.each { |c| Apartment::Tenant.drop("company_#{c.id}") }
  Capybara.reset_sessions!
  DatabaseCleaner.clean
end

And on a potentially related note sometimes the failure is that a table doesn't exist, as if the switch into a tenant was not successful. Again, these only happen a minority of the time, and even sporadically within the same seed.

[hiattp]

Ok I think you can ignore my comment, I think those were leftover ajax requests that were executing after the after hook started. I'm guessing both issues were caused by that effect, my suite seems to be way more consistent after accounting for it.