hipsterjazzbo / Landlord

A simple, single database multi-tenancy solution for Laravel 5.2+
MIT License
614 stars 138 forks source link

BelongsToTenant not working on user model #45

Closed obrunsmann closed 7 years ago

obrunsmann commented 7 years ago

Hey Guys,

everything works fine: If I query with Model::find(123) on my model and the resource does not belong to the current tenant (set via middleware) the request fails.

But: If I query on the User model it does not work. While asking for User::all() I get really all results including other customers and while querying User::findOrFail() with an ID which does not belong to the current tenant I get the result .. so everytime working with my user model I have to check if $model->tenant_id == $request->user()->tenant_id

flofloflo commented 7 years ago

I'm having the same problem since upgrading Laravel from 5.2 to 5.3 and switching to Landlord v2 at the same time. Did someone already find out why this happens?

hipsterjazzbo commented 7 years ago

Does your User model use BelongsToTenants?

flofloflo commented 7 years ago

Yes, it does. When I use another model which uses the same table everything works as expected.

The difference is that the User model extends Illuminate\Foundation\Auth\User while my other model extends Illuminate\Database\Eloquent\Model. Maybe something from the Laravel Authentication collides with Landlord?

obrunsmann commented 7 years ago

Yes, same to me. Trait in my User model nevertheless not working.

hipsterjazzbo commented 7 years ago

Hmm that's really strange. All the trait does is register some global scopes. Is anyone able to provide a test case for me? I can't replicate.

flofloflo commented 7 years ago

I'll try to create a test setup tomorrow, thanks for the support!

Edit/small update: I've created a basic test setup and the problem disappeared. I'll now add authentication features and see if that brings up the problem again...

flofloflo commented 7 years ago

Maybe I have found out what is happening: In my app I use Landlord::addTenant('tenant_id', Auth::user()->tenant_id); inside of a middleware to add the scope.

When I call the middleware with $middleware as global middleware no scope is applied and (by creating some debug points in the middleware) you can find out that Auth::user() is empty. This happens because of a change in Laravel 5.3 (see: https://laravel.com/docs/5.3/upgrade#upgrade-5.3.0)

In previous versions of Laravel, you could access session variables or the authenticated user in your controller's constructor. This was never intended to be an explicit feature of the framework. In Laravel 5.3, you can't access the session or authenticated user in your controller's constructor because the middleware has not run yet.

If the scope is applied later by using a group middleware (via $middlewareGroups), Auth::user() contains the authenticated user. The interesting part here is that in this case the scope is only applied to the "non-user" model. I guess the reason is that the User model has already booted when the scope is being applied (as described in #46 ).

So I think the problem is a combination of the changed behavior in Laravel 5.3 and the issue described in #46.

Does someone know a way to access the authenticated user in the global middleware? This would avoid the "booting-problem" I think.

dschniepp commented 7 years ago

Did you try to use Landlord v1.0.2?

flofloflo commented 7 years ago

Unfortunately only with Laravel 5.2 before the upgrade. In this setup everything worked fine.

MrRio commented 7 years ago

I'm having the same issue, anyone got a resolution?

r3donnx commented 7 years ago

Code provided by @mariomka https://github.com/HipsterJazzbo/Landlord/issues/49#issuecomment-269249778 somehow fixed this issue for me. Now it works on User model. :)

bissolli commented 7 years ago

Anyone has found other solution?

hipsterjazzbo commented 7 years ago

If anyone can work out the actual reason that @mariomka's solution works, we can try to tackle this once and for all.

flofloflo commented 7 years ago

I can confirm that the solution from #49 also works for me.

@gustavobissolli Currently I use a workaround in my project, so that I can use the unmodified Landlord release. I have created a second User model (named Employee) which utilizes the same table but doesn't have the scoping problems, because it isn't used for authentication.

I have created a demo project which shows how the scope changes dependent on the login state. Maybe this could also help someone to find a way to solve the problem. You can find the project under: https://github.com/flofloflo/laravel-landlord-test/tree/landlord-issue-45

EmadAdly commented 7 years ago

@flofloflo You mean that the summary of your solution is that clones the user model to Employee model. Taking into consideration extends Model not extends Authenticatable If you mean that, I think it's a good solution :+1:

flofloflo commented 7 years ago

@EmadAdly Yes, that's the solution (or workaround) I'm currently using.

The repository I posted shows the difference between using Model and Authenticatable. It also shows the behaviour of the Authenticatable-Model when the user is logged out (everything works, because the User-Model isn't booted).

lasseeee commented 7 years ago

On Laravel v5.2.45 using Landlord v2.0.5 I can scope the User model by adding Landlord::addTenant('tenant_id') to RouteServiceProvider's boot() method, but can't get the tenant_id off the currently authenticated user, so another solution would be to get the tenant_id off the subdomain like tenant.example.com instead of the user.

lasseeee commented 7 years ago

Did not work out after all. Any solution to this which doesn't involve cloning the User model?

patroniton commented 7 years ago

This is also happening for me.

The code from @mariomka #49 is also working for me.

@lasseal I managed to use your example to get my User model scoping using the subdomain of the url. What didn't work out for you on your end?

lasseeee commented 7 years ago

@patroniton I was accidentally still checkin the DB for the tenant_id, instead of of the subdomain, which I didn't realise before now, doh.

hipsterjazzbo commented 7 years ago

Has anyone been able to work out why doing a single scope instead of multiple scopes makes a difference?

hipsterjazzbo commented 7 years ago

I'm certain there are multiple issues here. I do want to get to the bottom of it, but this thread has gotten too confused.

I'm going to close this issue, but if anyone is still having any of these issues:

Please open a new issue with details about exactly where you are calling addTenant() (e.g., the whole middleware, or whatever)

Thanks!