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.

[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:

https://github.com/3scale/porta/blob/46b2f9be02c9123d066814126d1f0194e9eb47ae/lib/three_scale/middleware/multitenant.rb#L56-L72

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

[akostadinov]

1. On startup, this module is initialized from https://github.com/3scale/porta/blob/bde8321f83ba79c499b9a4a88776e201f4089f63/config/application.rb#L273
2. Then, the EnforceTenant module is included in all models (I think it would be more correct to use ApplicationRecord than ActiveRecord::Base) https://github.com/3scale/porta/blob/46b2f9be02c9123d066814126d1f0194e9eb47ae/lib/three_scale/middleware/multitenant.rb#L101-L105
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: https://github.com/3scale/porta/blob/df9873b32671234aaa7e6d13461ced70c91ee763/lib/three_scale/middleware/multitenant.rb#L104

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, https://github.com/3scale/porta/blob/46b2f9be02c9123d066814126d1f0194e9eb47ae/lib/three_scale/middleware/multitenant.rb#L44-L50 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.

[jlledom]

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.

OK, I understand now.

5. In this snippet, https://github.com/3scale/porta/blob/46b2f9be02c9123d066814126d1f0194e9eb47ae/lib/three_scale/middleware/multitenant.rb#L44-L50 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?

But there are models that doesn't have a tenant_id attribute, e.g. UserSession. What happens when one of those is loaded? fresh.send(attribute) will fail, doesn't it raise an exception?

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.

Nevermind, I misunderstood the code.

[akostadinov]

But there are models that doesn't have a tenant_id attribute, e.g. UserSession. What happens when one of those is loaded? fresh.send(attribute) will fail, doesn't it raise an exception?

https://github.com/3scale/porta/pull/3674/files#diff-7d2c571af1e87846864f0616961800c30d2604f657e32f793e4a2bddb19d8220R39-R42

[jlledom]

But there are models that doesn't have a tenant_id attribute, e.g. UserSession. What happens when one of those is loaded? fresh.send(attribute) will fail, doesn't it raise an exception?

https://github.com/3scale/porta/pull/3674/files#diff-7d2c571af1e87846864f0616961800c30d2604f657e32f793e4a2bddb19d8220R39-R42

Right. Thanks.

[akostadinov]

I haven't tried the code locally, but it looks good to me. Can you share how do you test this locally?

It should run by every request. So no special way to test except the added integration test file.

So if two requests shared the Thread.current this would fail.

I don't think this is supposed to happen. A thread should finish a request before taking on another one be it single- or multi-threaded rack server.

Update, looking at https://workingwithruby.com/wwrt/puma/

        while true
          work = nil
          continue = true

          mutex.synchronize do
            <...>
            work = todo.pop if continue
          end

          break unless continue
          block.call(work, *extra)
        end

The work seems to be done with the block.call. I see no mechanism for a thread to leave aside a request, start a new one and then get back to the previous one.

But this is a very quick glance over it. If you see any indication that would be possible, let me know.

P.S. very strange that the break comes before the block.call... like a request would be lost in case we want to stop. But this may not be the latest puma code.

[jlledom]

But this is a very quick glance over it. If you see any indication that would be possible, let me know.

I assumed one could tweak puma to use fibers rather than threads, but apparently it's not possible yet: https://github.com/puma/puma/issues/2517

Apparently Unicorn doesn't support fibers neither, so we should be safe.

[akostadinov]

Apparently Unicorn doesn't support fibers neither, so we should be safe.

Switching between fibers should switch also the variables. But as you say, we don't use a fibered server.

[akostadinov]

if this fails and we have to revert, what's your plan to revert the triggers?

@jlledom , the trigger doesn't have to be reverted regardless of the rest of the code