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

User is always null? #32

Closed
repat opened this issue Sep 3, 2019 · 13 comments
Closed

User is always null? #32

repat opened this issue Sep 3, 2019 · 13 comments
Assignees

Comments

@repat
Copy link

repat commented Sep 3, 2019

image

If I remove = null I get a 403. Am I doing something wrong here or is this part broken?

>>> env('WEB_TINKER_ENABLED', false)
=> true
@edgrosvenor
Copy link
Sponsor

I'm seeing the same thing.

@joaolisboa
Copy link

Same issue. The gate doesn't seem to work at all.

@rubenvanassche
Copy link
Member

I guess you're not logged in, since you're trying to get the email address of a non existing user. Just tested it, and it seems that nothing is wrong with the package, since this works:

Auth::login(factory(User::class)->create([
    'email' => 'ruben@spatie.be'
]));

Gate::define('viewWebTinker', function ($user = null) {
    return $user->email === 'ruben@spatie.be';
});

Will close this for now.

@edgrosvenor
Copy link
Sponsor

I could also write a test that would pass. It doesn’t mean that it actually works in real life. I assure you I am logged in.

@repat
Copy link
Author

repat commented Oct 4, 2019

I'm also sure I'm logged in as I clicked on it in /admin ;-) Did you also move yours @edgrosvenor ? Maybe that's part of the issue for whatever reason 🤔

@joaolisboa
Copy link

I'm also sure I'm logged in and have the same exact behavior. With $user = null it comes as null, with $user only I get a 403.

@edgrosvenor
Copy link
Sponsor

@repat I didn't change the path. Something is definitely broken.

I'll fork it and figure it out this weekend. As good as testbench is, the "it works fine in my tests but doesn't work when pulled into an actual Laravel app" thing isn't a rare problem.

@edgrosvenor
Copy link
Sponsor

I think this might be a Laravel bug actually. It seems that Laravel isn't passing the user to the Gate closure as documented. I wonder if it has to do with the fact that this package explicitly defines its own middleware and does not also use the web middleware. I vaguely remember having a problem like this with another package when I upgraded a site to Laravel 6 and I worked around it by adding the web middleware to the route along with the package's own middleware. I could be wrong and don't have time to dig into it today but I'll experiment over the weekend.

@edgrosvenor
Copy link
Sponsor

Yup. That's definitely it. If I use the same gate check in a route that also has the web middleware the user model exists as expected. So I guess maybe we need to ask @rubenvanassche if he and the other Spatie developers think this should be handled via Laravel or here.

@joaolisboa
Copy link

If it defines it's own middleware shouldn't that be documented, since it's not using the 'default' web middleware?

@edgrosvenor
Copy link
Sponsor

Not necessarily. The Laravel documentation doesn't say that the user will only be provided to the Gate closure if the web middleware is used, though that does seem to be the behavior. I'm not sure who's actually at fault here.

@edgrosvenor
Copy link
Sponsor

edgrosvenor commented Oct 4, 2019

In the mean time, this will get anyone who's stuck past the problem. Do this in your web.php route file:

use Spatie\WebTinker\Http\Controllers\WebTinkerController;
use Spatie\WebTinker\Http\Middleware\Authorize;


Route::prefix('web_tinker')->middleware(['web', Authorize::class])->group(function () {
    Route::get('/', [WebTinkerController::class, 'index']);
    Route::post('/', [WebTinkerController::class, 'execute']);
});

Obviously use anything you like in the prefix as long as it does not match the path set in the web tinker config file.

@rubenvanassche
Copy link
Member

Hi @edgrosvenor,

thanks for your extensive research into this problem! Indeed adding the web middleware group will fix this problem but actually only the StartSession and EncryptsCookies middlewares from the web group are required to solve this problem.

So I've added them(c693f3f) and tagged a new release!

Thanks guys!

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

Successfully merging a pull request may close this issue.

4 participants