restore tenant_id enforcement in modern rails

[akostadinov]

needs tests before merging and also we should consider whether this is needed at all at this point

fixes THREESCALE-10686

[jlledom]

It's probably worth keeping it because we had related incidents in the past that could have been detected earlier if this check would have worked.

[jlledom]

I'm trying to understand this, but I need some help:

1. On startup, this module is initialized from

   porta/config/application.rb

   Line 273 in bde8321

   config.middleware.use ThreeScale::Middleware::Multitenant, :tenant_id

2. Then, the EnforceTenant module is included in all models (I think it would be more correct to use ApplicationRecord than ActiveRecord::Base)

   porta/lib/three_scale/middleware/multitenant.rb

   Lines 101 to 105 in 46b2f9b

   	def initialize(app, attribute)
   	@app = app
   	@attribute = attribute
   	ActiveRecord::Base.send(:include, EnforceTenant)
   	end

3. Every time a new model is created, :enforce_tenant! is called. I assume Thread.current[:multitenant] will always have the correct value here. Then it calls verify!
4. The module name EnforceTenant makes me think the purpose is to ensure :tenant_id will always have a value, but from the code at verify! it seems to me it only checks the value doesn't change. So this assumes :tenant_id is set somewhere else and this only makes sure it never changes. Am I right?
5. In this snippet,

   porta/lib/three_scale/middleware/multitenant.rb

   Lines 44 to 50 in 46b2f9b

   	begin
   	current = object.send(attribute)
   	rescue ActiveRecord::MissingAttributeError
   	# Multitenant.log("#{object} is missing #{attribute}. Reloading and trying again")
   	fresh = object.class.send(:with_exclusive_scope) { object.class.find(object.id, :select => attribute) }
   	current = fresh.send(attribute)
   	end

   why don't we rescue ActiveRecord::MissingAttributeError when calling current = fresh.send(attribute)? Is it a bug?

If the purpose is to make sure :tenant_id doesn't change, couldn't this be done in a much easier way? e.g. I found this:

https://discuss.rubyonrails.org/t/validates-immutable-true/81479/2

[jlledom]

Much better

[akostadinov]

How did you conclude that code checkd tenant_id never changed? My limited understanding is that it should have same tenant on all objects. But I'm not yet at there. Just trying to restore functionality. And I'm stuck currently getting access to the session store.

[jlledom]

How did you conclude that code checkd tenant_id never changed?

Maybe I'm wrong because I don't understand the code. I guessed that from this code:

porta/lib/three_scale/middleware/multitenant.rb

Lines 56 to 72 in 46b2f9b

	return if current == original
	# we still need to check if it's master before raising a tenant leak
	@cookie_store ||= find_cookie_store(@app)
	@session ||= @cookie_store.send(:load_session, @env)
	@master ||= Account.find_by_sql(["SELECT * FROM accounts WHERE master = ?", true]).first
	if (user_id = @session.last[:user_id].presence)
	@users ||= {}
	@users[user_id] ||= User.find_by_sql(["SELECT * FROM users WHERE id = ? AND account_id = ?", user_id, @master.id]).present?
	return if @users[user_id]
	end
	return if @env["action_controller.request.query_parameters"]["provider_key"] == @master.api_key
	raise TenantLeak.new(object, attribute, original)

If :tenant_id it's the same, return, otherwise raise

[akostadinov]

Interesting question here is why originally we kept user_id in a hash. As if we may observe multiple user_ids within the same request. Which doesn't make sense to me. Any ideas?

[akostadinov]

1. On startup, this module is initialized from

   porta/config/application.rb

   Line 273 in bde8321

   config.middleware.use ThreeScale::Middleware::Multitenant, :tenant_id

2. Then, the EnforceTenant module is included in all models (I think it would be more correct to use ApplicationRecord than ActiveRecord::Base)

   porta/lib/three_scale/middleware/multitenant.rb

   Lines 101 to 105 in 46b2f9b

   	def initialize(app, attribute)
   	@app = app
   	@attribute = attribute
   	ActiveRecord::Base.send(:include, EnforceTenant)
   	end

3. Every time a new model is created, :enforce_tenant! is called. I assume Thread.current[:multitenant] will always have the correct value here. Then it calls verify!

This is set once for each request here:

porta/lib/three_scale/middleware/multitenant.rb

Line 104 in df9873b

Thread.current[:multitenant] = TenantChecker.new(@attribute,@app,env)

4. The module name EnforceTenant makes me think the purpose is to ensure :tenant_id will always have a value, but from the code at verify! it seems to me it only checks the value doesn't change. So this assumes :tenant_id is set somewhere else and this only makes sure it never changes. Am I right?

The purpose is to make sure that a user only sees objects from its own tenant. The implementation seems to be that first initialized object with a tenant_id would set the value. Then if any other object has a different tenant_id, the request will fail. I think this should be good enough because we should always initialize a User object so if any object has a different tenant_id, it will be detected.

5. In this snippet,

   porta/lib/three_scale/middleware/multitenant.rb

   Lines 44 to 50 in 46b2f9b

   	begin
   	current = object.send(attribute)
   	rescue ActiveRecord::MissingAttributeError
   	# Multitenant.log("#{object} is missing #{attribute}. Reloading and trying again")
   	fresh = object.class.send(:with_exclusive_scope) { object.class.find(object.id, :select => attribute) }
   	current = fresh.send(attribute)
   	end

   why don't we rescue ActiveRecord::MissingAttributeError when calling current = fresh.send(attribute)? Is it a bug?

I don't think it is a bug. Because we retrieve the necessary attribute when we obtain the object. That's the point of the rescue clause to begin with. Makes sense?

If the purpose is to make sure :tenant_id doesn't change, couldn't this be done in a much easier way? e.g. I found this:

https://discuss.rubyonrails.org/t/validates-immutable-true/81479/2

I need more elaboration to understand how this applies.