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

[V6] Register OctaneReloadPermissions listener for Laravel Octane #2403

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

erikn69
Copy link
Contributor

@erikn69 erikn69 commented Apr 13, 2023

i'm not 100% sure on this
i can't test octane

this was a test for octane users to try but no one gave me feedback that it works

On #2322 (comment) was said that the problem was the spatie/once package, and that spatie/laravel-permission works correctly on octane

I recommend not merging it until you do more testing that it's the correct approach.

@drbyte
Copy link
Collaborator

drbyte commented Apr 13, 2023

I recommend not merging it until you do more testing that it's the correct approach

Agreed. I more just wanted it here as an easy place to reference the specific proposed fix. And if it's deemed "right" and not problematic, it's a simple thing then to merge it.
Thanks!

@drbyte
Copy link
Collaborator

drbyte commented Apr 13, 2023

@sweebee Are you able to test this? In reference to your issue #2401

@sweebee
Copy link

sweebee commented Apr 14, 2023

@drbyte Yes, its working!

@erikn69 erikn69 changed the title Register OctaneReloadPermissions listener for Laravel Octane [V6] Register OctaneReloadPermissions listener for Laravel Octane Apr 14, 2023
@davidjr82
Copy link

This solution will reload the permissions in every single tick of the octane server. Wouldn't this result in a drop of performance?

Also, we should make (I don't know how...) a test case where we can see it failing without this change, otherwise it wouldn't add anything.

@drbyte
Copy link
Collaborator

drbyte commented Apr 14, 2023

This solution will reload the permissions in every single tick of the octane server. Wouldn't this result in a drop of performance?

@sweebee Do you mind testing what benefits or drawbacks occur if not registering it with the Tick events?

Also, we should make (I don't know how...) a test case where we can see it failing without this change, otherwise it wouldn't add anything.

@sweebee Any thoughts on how best to automate such tests?

@drbyte
Copy link
Collaborator

drbyte commented Apr 18, 2023

@sweebee @davidjr82 does the change to listening on OperationTerminated work satisfactorily in your app?

@davidjr82
Copy link

For me the issue was in a different package (spatie/once) so this package worked without any change.

@parallels999
Copy link
Contributor

parallels999 commented Apr 19, 2023

this package worked without any change.
this result in a drop of performance

it would be better to make it optional, so, can be activated from permission.php for whoever needs it
Like

/*
 * When set to true, the Spatie\Permission\Listeners\OctaneReloadPermissions listener will be registered
 * on the Laravel\Octane\Events\OperationTerminated event, this will refresh permissions on every
 * TickTerminated, TaskTerminated and RequestTerminated
 */
'register_octane_permissions_refresh' => true,

@davidjr82
Copy link

this package worked without any change.
this result in a drop of performance

it would be better to make it optional, so, can be activated from permission.php for whoever needs it
Like

/*
 * When set to true, the Spatie\Permission\Listeners\OctaneReloadPermissions listener will be registered
 * on the Laravel\Octane\Events\OperationTerminated event, this will refresh permissions on every
 * TickTerminated, TaskTerminated and RequestTerminated
 */
'register_octane_permissions_refresh' => true,

I agree on this, although I wouldn't push a feature without a working test.

But if in this case we don't know how to do it, at least make it optimal sounds reasonable to me.

@erikn69
Copy link
Contributor Author

erikn69 commented Apr 26, 2023

it would be better to make it optional

that could also apply

@drbyte
Copy link
Collaborator

drbyte commented Apr 26, 2023

I'm in agreement with adding a config switch. Just undecided about the best name for it.
The variant I'm contemplating is register_octane_reset_listener

@drbyte
Copy link
Collaborator

drbyte commented Apr 26, 2023

adding tests

I agree: it would be preferable to get some tests set up for Octane. #HelpWanted

@drbyte
Copy link
Collaborator

drbyte commented Jun 17, 2023

@freekmurze do you have a preference on incorporating tests for this?

@drbyte drbyte requested a review from freekmurze June 17, 2023 17:51
@drbyte drbyte merged commit feb3014 into spatie:main Aug 17, 2023
20 checks passed
@erikn69 erikn69 mentioned this pull request Nov 9, 2023
@jihadismail8
Copy link

for some reason Spatie Permissions , gives me an error of not enough permissions after ~14 requests in a row under Octane RoadRunner !! ""message": "User does not have the right permissions. Necessary permissions are Manage Permissions"," , even thought the 13 requests where fine , then sudden not enough permission , tried almost everything , and i have the last version of laravel 11 , fresh spatie Permission .... here is the trace " "trace": [
{
"file": "/var/www/html/vendor/spatie/laravel-permission/src/Middleware/PermissionMiddleware.php",
"line": 36,
"function": "forPermissions",
"class": "Spatie\Permission\Exceptions\UnauthorizedException",
"type": "::"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Spatie\Permission\Middleware\PermissionMiddleware",
"type": "->"
},
{
"file": "/var/www/html/app/Http/Middleware/Cors.php",
"line": 23,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "App\Http\Middleware\Cors",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Routing/Middleware/SubstituteBindings.php",
"line": 50,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Illuminate\Routing\Middleware\SubstituteBindings",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Auth/Middleware/Authenticate.php",
"line": 64,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Illuminate\Auth\Middleware\Authenticate",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 119,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Routing/Router.php",
"line": 805,
"function": "then",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Routing/Router.php",
"line": 784,
"function": "runRouteWithinStack",
"class": "Illuminate\Routing\Router",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Routing/Router.php",
"line": 748,
"function": "runRoute",
"class": "Illuminate\Routing\Router",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Routing/Router.php",
"line": 737,
"function": "dispatchToRoute",
"class": "Illuminate\Routing\Router",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php",
"line": 200,
"function": "dispatch",
"class": "Illuminate\Routing\Router",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 144,
"function": "Illuminate\Foundation\Http\{closure}",
"class": "Illuminate\Foundation\Http\Kernel",
"type": "->"
},
{
"file": "/var/www/html/vendor/livewire/livewire/src/Features/SupportDisablingBackButtonCache/DisableBackButtonCacheMiddleware.php",
"line": 19,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Livewire\Features\SupportDisablingBackButtonCache\DisableBackButtonCacheMiddleware",
"type": "->"
},
{
"file": "/var/www/html/vendor/barryvdh/laravel-debugbar/src/Middleware/InjectDebugbar.php",
"line": 66,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Barryvdh\Debugbar\Middleware\InjectDebugbar",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/TransformsRequest.php",
"line": 21,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/ConvertEmptyStringsToNull.php",
"line": 31,
"function": "handle",
"class": "Illuminate\Foundation\Http\Middleware\TransformsRequest",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Illuminate\Foundation\Http\Middleware\ConvertEmptyStringsToNull",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/TransformsRequest.php",
"line": 21,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/TrimStrings.php",
"line": 51,
"function": "handle",
"class": "Illuminate\Foundation\Http\Middleware\TransformsRequest",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Illuminate\Foundation\Http\Middleware\TrimStrings",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Http/Middleware/ValidatePostSize.php",
"line": 27,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Illuminate\Http\Middleware\ValidatePostSize",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/PreventRequestsDuringMaintenance.php",
"line": 110,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Illuminate\Foundation\Http\Middleware\PreventRequestsDuringMaintenance",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Http/Middleware/HandleCors.php",
"line": 62,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Illuminate\Http\Middleware\HandleCors",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Http/Middleware/TrustProxies.php",
"line": 57,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 183,
"function": "handle",
"class": "Illuminate\Http\Middleware\TrustProxies",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php",
"line": 119,
"function": "Illuminate\Pipeline\{closure}",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php",
"line": 175,
"function": "then",
"class": "Illuminate\Pipeline\Pipeline",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php",
"line": 144,
"function": "sendRequestThroughRouter",
"class": "Illuminate\Foundation\Http\Kernel",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/octane/src/ApplicationGateway.php",
"line": 36,
"function": "handle",
"class": "Illuminate\Foundation\Http\Kernel",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/octane/src/Worker.php",
"line": 84,
"function": "handle",
"class": "Laravel\Octane\ApplicationGateway",
"type": "->"
},
{
"file": "/var/www/html/vendor/laravel/octane/bin/roadrunner-worker",
"line": 54,
"function": "handle",
"class": "Laravel\Octane\Worker",
"type": "->"
},
{
"file": "/var/www/html/vendor/bin/roadrunner-worker",
"line": 119,
"function": "include"
} "

@jihadismail8
Copy link

forgot to add that in php artisan serve , it works fine

@jihadismail8
Copy link

ok found the problem , it was because of teams , since teams is being flushed the Users loses permissions
that creates a dilemma now

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 this pull request may close these issues.

Wrong permissions on refresh (Vapor/Octane)
6 participants