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

"Session store not set on request." error after adding global EnsureValidTenantSession middleware #28

Closed
martincarvalho opened this issue May 29, 2020 · 8 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@martincarvalho
Copy link

If I remove that middleware the app runs fine.

I'm guessing that when I add the EnsureValidTenantSession middleware to the global middleware stack it's making sure that there's a valid tenant all the time, is that correct?
The problem is I don't want to have a tenant all the time.

Instead of loading tenants by domain, I want to find them by wildcard, which I'm already achieving by using a custom TenantFinder and that's working fine.
So, for example HTTP://myapp.test/myrestaurant will load the My Restaurant tenant.

If there's no wildcard on the request then I'm assuming it's a request for the landlord's application to be running.
So HTTP://myapp.test/clients would be where the landlord would see all the clients for the SaaS, not for the My Restaurant tenant.

Right now there are no middlewares for the routes.
My plan is to make all the landlord's routes checked by something like IsLandlordAdmin auth-like middleware group and then Route::get('/{tenant}', 'TenantController@someMethod'); would take care of the tenant's logic.

@freekmurze freekmurze added bug Something isn't working help wanted Extra attention is needed labels May 29, 2020
@freekmurze
Copy link
Member

I haven't come across this problem myself. I welcome a PR with tests that fixes this.

@martincarvalho
Copy link
Author

I'd love to do that but I'm still on my way to learn PHP testing and never really done a PR in my life (yet) lol
I'd love to help, though, if there's anything I can do please let me know!

@alibarthi
Copy link

Hi,

I haven't used the package myself yet, so I'm only talking from past experience. Such middleware should only be applied to tenant aware routes. Your landlord routes need some other protection anyway. So, a check to determine if the tenant id is not null (if it's null, then this is a landlord route - no valid tenant found) should solve the problem with this middleware applied globally:

if (app($this->currentTenantContainerKey())->id === null) {
    return $next($request);
}

$sessionKey = 'ensure_valid_tenant_session_tenant_id';
...

I can create a pull request with this small change if desired.

@jeffochoa
Copy link

I had the same issue, solved by moving the EnsureValidTenantSession::class middleware from the $middleware array to the web middleware group.

I'm using Laravel-Nova btw, I'm not sure if this has something to do.

@juanparati
Copy link

juanparati commented May 31, 2020

I also moved the EnsureValidTenantSession to the web group and now it works. I think that it's more logical move it to the web group because session are usually used in the Web group (At least in my solution).

@freekmurze
Copy link
Member

I've changed our docs: users now get instructed to place the middleware in the web group.

@masterix21
Copy link
Collaborator

Why not an admin guard to protect the landlord routes?

Next, if you like to be sure that there is no tenant selection, you can create a middleware with:

if (! app()->has(”currentTenant”)) {
        $next($request);
}

@martincarvalho
Copy link
Author

Thanks for all the help,
I just removed the global middleware and am now tackling routes specifically.
This is solved :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants