Keeping setTenantId persistent across the session.

[kyranb]

Hi there,

As in the previous version, it was possible for all queries to be scoped automatically, just by implementing the trait. Now that it is necessary to set the TenantScope, I am wondering what would be the best way to keep the TenantScope persistant across the session whilst the user is logged in.

To demonstrate what I am talking about, when requesting the index of a model like so:

 public function index()
    {
         TenantScope::setTenantId($tenant_id);
         $models = Model::all();

         return $models;
    }

The returned query will scope to the tenant id.

However if I have my login controller

if (Auth::attempt($input)
        {
            $tenant_id = Auth::user()->tenant_id;
            TenantScope::setTenantId($tenant_id);

                return Redirect::intended('admin');
        }

then call the index method on a different controller:

public function index()
    {
         $models = Model::all();

         return $models;
    }

the query will not be scoped and it will simply return every row in the table.

Is it possible/what would the best implementation be to set the tenant_id once on login and then not need to set it again?

Once again, sorry if this is a more Laravel specific question, than a package specific issue :)

[hipsterjazzbo]

Gah, good point. Might have to do something with the session, but I'm not sure if I like that.

[tonydew]

For what it's worth, this is the kind of thing that should throw a gigantic exception, and not 'fail' silently. If the queries are supposed to be scoped by default, they should return nothing if no scope is provided.

The only way one should get non-scoped results is if you purposefully unscope: Model::allTenants()->get()

Otherwise, that could be a world of hurt in a SaaS app when some other user's results showed up silently, by mistake.

[hipsterjazzbo]

I totally agree. There's obviously a basic architechtural issue here. I'm just not sure how to solve it in the best way.

On 16 July 2014 10:53, Tony Dew notifications@github.com wrote:

For what it's worth, this is the kind of thing that should throw a gigantic exception, and not 'fail' silently. If the queries are supposed to be scoped by default, they should return nothing if no scope is provided.

The only way one should get non-scoped results is if you purposefully unscope: Model::allTenants()->get()

Otherwise, that could be a world of hurt in a SaaS app when some other user's results showed up silently, by mistake.

— Reply to this email directly or view it on GitHub https://github.com/AuraEQ/laravel-multi-tenant/issues/5#issuecomment-49103651 .

[tonydew]

The best way of going about setting the tenant_id dynamically is still a bit of mystery to me as well.

In the meantime, you could default the tenant ID to something that is guaranteed not to match (0 maybe?) and then check to see if the dynamically assigned tenant_id was null or missing before setting it in setTenantId()

[kyranb]

I agree that using sessions may complicate things.

As for @tonydew 's idea of setting the tenant ID to 0, this seems like a pretty solid way of ensuring no data leakage at all.

As for a way to ensure that the tenant id is set on all requests At the moment I am setting the tenant_id within the 'before route filter' in (app/filters.php) So far everything seems to work alright. Can anyone else see any other downsides to this implementation?

App::before(function($request)
{
    // Tenant Scoping
    if (Auth::check())
    {
        $tenant_id = Auth::user()->tenant_id;
        TenantScope::setTenantId($tenant_id);
    }
    else
    {
        $tenant_id = 0;
        TenantScope::setTenantId($tenant_id);
    }
});

Once again, I can't thank you enough for putting this package together. It has been an incredible help!

[uxweb]

For a Multi Tenant - Multi DB solution, i think that 2 session variables should be used: 'tenant_connection' and 'tenant_id'.

Of course, you must create a filter to avoid a user to access those parts of the app that requires having a tenant_connection and tenant_id selected, then the model will set the right connection and tenant scope.

The question is: How to tell an Eloquent model to set its connection to the one stored on session?, well, after thinking a lot about this and, sure, i'm not an expert on architecture, my approach was to create a base Model that inherits from Eloquent and make any new model inherit from it to be able to get the connection stored in session.

class TenantModel extends \Eloquent {

    function __construct(array $attributes = array())
    {
        parent::__construct($attributes);

        // sets the model's connection from the one stored in session when it is created
        $this->setConnection(\Session::get('tenant_connection'));
    }
} 

[hipsterjazzbo]

Hey all.

I've made a few changes that may improve this issue.

1. I've made it throw a TenantIdNotSetException when the scope is called with a null tenant_id
2. The tenant_id is now stored in the Session.

The only caveat I've found to throwing an exception when no tenant_id is set, is when trying to login with Laravel's Auth functions, it gets thrown (because no scope is set, obviously). Because of this, I've also added a TenantScope::disable() which can be called before using Auth::login() or the like.

I'm not how much I like that solution, but I'm not sure of any other way around it. I don't really like how complex this package is becoming, but at the same time, I can't see any way around it without being integrated into core.

Please have a test on dev-master and see if it works out for you.

:)

[hipsterjazzbo]

@uxweb Could you please open that up as a separate issue? That'll give us a place to discuss multi-db multitenancy, which I feel is a big issue.

[pspiteri]

Hi - we first discussed this on the old Laracasts forum. I see you are making great ground.

When you implement this type of multi tenancy, does your testing hold up?

I ask because I am using a very similar approach (though I don't use an observer at all) and acceptance tests pass fine but functional tests do not - I set my tenant context using the auth filter, which works with acceptance testing but not with functional.

I am using Codeception.

I am not using your package, but as you are still experimenting with it, I thought I'd pick your brain to see if you'd come across something like this before.

Cheers.

[hipsterjazzbo]

@pspiteri Hmm, unfortunately I'm still an old dinosaur rocking the PHPUnit tests. I've never actually tried codeception style tests with this package. I'd love to know though, if you ever try it!

[pspiteri]

I have used Codeception with my implementation of this - which is very similar, but not exactly the same, to your own because I am following the same global scope approach - the only trouble I had related to setting the global context and how that affected tests.

I don't think I have come across example on how you do this, but this is how I went about setting tenant context

• Very similar to the Culttt approach, I create a service (shared / static) with a couple of methods, set(Model) to set that context and id() to get the id of that context. Phil (of Culttt) uses more methods, but the current global scope approach was not around when he wrote the article.
• I originally called set(Model) as set(Auth:user()) from the auth filter - this is consistent with the Culttt approach. It seemed to work fine and acceptance tests passed (all tests in this post will be tests in Codeception) but functional tests did not. I can't remember the error message, but they indicated that tenant context was not being set
• So I moved the code to set context out of the auth filter and into each controller that requires it. That fixed the functional tests. I'll need to DRY that up a little as for now I am just copying the routine to set tenant context in the controller constructor.

While we are at it, where do you set tenant context?

Hey about PHPUnit tests ... I've almost been tempted to move to PHPUnit because this is the method built into Laravel. Can I ask a couple of questions about this approach (forgive me if the are a bit naff)

• My understanding is the PHPUnit is really flexible. So I know it will do unit tests (of course) and I think it will do integration tests. But will it do functional tests (I ask because Laravel comes bundled with Symphony Crawler)? What about acceptance test (I don't expect it will)?
• Importantly, how to you test for data separation. I figured I would use functional tests for this because I wanted to peer into the database to make sure that currently logged in user could not access any records in the database and not rely on screen output to test that (so, assert that I can't see any record with a tenant id not equal to my own user->tenant_id). But it is not working out that way and I don't seem to be able to make my functional tests take the tenant context into account when peering into the database. I am really concerned about data separation and want to test the begeezus out of it before rolling it out. I tried a multiple schema approach but testing and maintenance was a nightmare.

I look forward to hearing from you on this topic. If you'd rather I not use this particular medium, let me know and I'll move it to somewhere else in accordance with your preferences.

[hipsterjazzbo]

Hey all, @geomagilles brought up a good point in #7:

Storing this in session does not appear a good practice to me. I think TenantScope must be set where you're are checking access rights (ie. in your app/filters.php). Doing so, you do not have any session issue. (link)

If you just stick the call to setTenant in the part of your code where you check access and grant it, you should be fine across multiple tabs/etc, right?

Either that's right, and I feel dumb, or it's wrong, and I feel dumb.

[hipsterjazzbo]

Hey guys,

I'm gonna paste this note a few times, so apologies if you get multiple notifications.

Check out #13, it's a big refactor that I think address all outstanding issues. I'd love love love to have a discussion about it over there.

:heart: