Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BelongsToTenant not working on user model #45

Closed
obrunsmann opened this issue Nov 2, 2016 · 22 comments
Closed

BelongsToTenant not working on user model #45

obrunsmann opened this issue Nov 2, 2016 · 22 comments

Comments

@obrunsmann
Copy link
Contributor

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
Copy link

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
Copy link
Owner

Does your User model use BelongsToTenants?

@flofloflo
Copy link

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
Copy link
Contributor Author

obrunsmann commented Nov 30, 2016 via email

@hipsterjazzbo
Copy link
Owner

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
Copy link

flofloflo commented Dec 1, 2016

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
Copy link

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
Copy link

Did you try to use Landlord v1.0.2?

@flofloflo
Copy link

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

@MrRio
Copy link

MrRio commented Dec 27, 2016

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

@r3donnx
Copy link

r3donnx commented Jan 9, 2017

Code provided by @mariomka #49 (comment) somehow fixed this issue for me. Now it works on User model. :)

@bissolli
Copy link
Contributor

Anyone has found other solution?

@hipsterjazzbo
Copy link
Owner

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

@flofloflo
Copy link

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
Copy link

@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 👍

@flofloflo
Copy link

@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
Copy link

lasseeee commented Feb 12, 2017

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
Copy link

lasseeee commented Mar 3, 2017

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

@patroniton
Copy link

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
Copy link

lasseeee commented Mar 6, 2017

@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
Copy link
Owner

hipsterjazzbo commented May 25, 2017

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

@hipsterjazzbo
Copy link
Owner

hipsterjazzbo commented May 25, 2017

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:

  • Can't scope the user model
  • Landlord not working from global middleware
  • Anything that is solved by that one hack

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants