spatie/laravel-permission

Laravel octane random results when using Auth::user()->can('permisssion') after modifying role/permission belonging

davidjr82 opened this issue · 21 comments

➡️ UPDATE: THIS PROBLEM WAS RELATED TO ISSUES WITH ANOTHER PACKAGE ⬅️


Describe the bug
Using Laravel Octane with roadrunner, if you update a role permission, calling to Auth::user()->can('permisssion') leads to unexpected randomly results (randomly grants or denies).

This does not happens with the same code when not running Octane.

Happens both with Auth::user()->can('permisssion') and auth()->user()->can('permisssion')

Versions

  • spatie/laravel-permission package version: 5.8.0
  • laravel/framework package: 9.48.0
  • laravel/octane package: 1.4.0

PHP version: 8.1.15

Database version: mysql 8.0.31

To Reproduce
Steps to reproduce the behavior:

  • Install clean laravel app and laravel permissions package
  • create a role with a permission, and asign the role to your user
  • make a form to modify the belonging of the permission to the role:
        $grant_permission
            ? $role->givePermissionTo($permission)
            : $role->revokePermissionTo($permission);
  • Put in blade {{ auth()->user()->can('permission-name') ? 1 : 0 }} and refresh many times
  • You will see the number randomly changing to 0 or 1

Here is my example code and/or tests showing the problem in my app:

Expected behavior
Same code works fine without Octane. With Octane should work in the same way, it should show consistently the "can" of the user.

Environment (please complete the following information, because it helps us investigate better):

  • Using sail to develop in WSL2
  • laravel/sail: 1.18.1
  • supervisor line to start octane: command=/usr/bin/php -d variables_order=EGPCS /var/www/html/artisan octane:start --watch --server=roadrunner --host=0.0.0.0 --rpc-port=6001 --port=80

I would do, but I have no idea how to fix it 🤣

Maybe cache is failing, try using array as cache

'store' => 'default',

Already tested, not related to cache (tested also with array and redis).

This does not happens with the same code when not running Octane.

I'm not using octane, i can't replicate your problem

I understand, I will try to do a repo adding laravel sail with octane and roadrunner to make it easy to replicate in a sandbox

Octane boots your application once, keeps it in memory, and then feeds it requests at supersonic speeds.

Maybe it boots different threads, so when you update permissions on one on them, others stay unupdated, and on refresh many times the view changes from the updated one to others, so is there a way to reload octane boots after changing role/permission belonging?

It is clearly related to this. I think it has to do with how the gate or auth facade is resolved, like it does only once at the boot time and then never resolves again. Happens the same for example with the container, with Octane you should resolve it with the app() helper instead of using dependency injection.

What I don't know is how laravel/permission adds their permission system to the can utility. Can be it is not resolving in the "Octane way" and uses some kind of request cache?

A possible issue could be with the PermissionRegistrar class. It has multiple static properties being set on it instead of just utilizing the nature of a singleton.

This could potentially give some issues in the case of octane

drbyte commented

I think it has to do with how the gate or auth facade is resolved, like it does only once at the boot time and then never resolves again. Happens the same for example with the container, with Octane you should resolve it with the app() helper instead of using dependency injection.

What I don't know is how laravel/permission adds their permission system to the can utility. Can be it is not resolving in the "Octane way" and uses some kind of request cache?

It registers it on the gate using the app() helper:

/**
* Register the permission check method on the gate.
* We resolve the Gate fresh here, for benefit of long-running instances.
*
* @return bool
*/
public function registerPermissions(): bool
{
app(Gate::class)->before(function (Authorizable $user, string $ability) {
if (method_exists($user, 'checkPermissionTo')) {
return $user->checkPermissionTo($ability) ?: null;
}
});
return true;
}

app(Gate::class)->before(function (Authorizable $user, string $ability) {

@drbyte Couldn't it be the Authorizable $user dependency injection? If it is resolved only once during the boot time and then returns the same cached value, makes sense because all the workers boots once, and only the one who has made the update in the role permission update that change in their memory, so when the app asks for it, if is the worker who made the change gives the actual value, but any other worker will return the original unmodified boot-time value.

Also try cleaning loaded permissions on some middleware, forcing to read cache again on every call

public function clearClassPermissions()
{
$this->permissions = null;
}

@olivernybroe Already tried, both flushing whole cache and calling app()->make(\Spatie\Permission\PermissionRegistrar::class)->forgetCachedPermissions();. Same result.

It has multiple static properties being set on it instead of just utilizing the nature of a singleton.

hi @olivernybroe great idea, make a PR please

#2324

drbyte commented

dev-master (v6.x) now has the refactor by @olivernybroe

Feedback welcome!

Note the need to replace your migration files as part of the change.

/ping @davidjr82

@drbyte there might be more changes needed for full octane support. But that change should at least help a bit.

drbyte commented

@drbyte there might be more changes needed for full octane support. But that change should at least help a bit.

Thanks @olivernybroe. Appreciate your support.

Before Octane came along we made a few refinements for long-running instances, but since I'm not using that in my tech stack right now it's not one of my strengths these days.

Will keep this open as we get feedback from testing, and collaborate to ensure compatibility.

@drbyte tested dev-main branch publishing migration and config file, and clearing all caches, and same result.

@olivernybroe thanks for the PR. Even if doesn't resolves the issue, it is a needed step to have a good Octane integration.

I hope to have some time this weekend to create a repo with the issue, using sail and roadrunner. What I don't know at the moment is how to create a test specifically for Octane (this same issue in the PHP builtin server works).

I am not able to reproduce the issue in a simple repo, therefore there is something in my code that affects the behaviour that I am missing.

Although it is something related to Octane -because when running in the builtin php server the issue disappears-, I propose to close this issue and let someone that is able to reproduce an Octane issue to open a new one.

If you want to keep this issue open to address the Octane compatibility for 6.x, good for me. But we would need someone that can recreate the issue in a separate and simple repo to address the right problem.

In my case, I am going to stop using Octane for the moment and stick to a regular PHP server.
Thanks to you all for your help.

We also plan to use this package on an octane-driven Laravel 9 instance.
Are we good to go, or are there still potential issues with this package?

Should I use the dev-master for octane?

I personally have dropped Octane in my project to avoid conflicts with this package. I still have the issues with the dev-master branch.

EDIT: It was not this package. It was another: #2322 (comment)

Maybe a better solution here: mcamara/laravel-localization#780 (comment), mcamara/laravel-localization#780 (comment)

  1. Add a listener to hook into Octane's RequestReceived event
namespace App\Listeners;

use Laravel\Octane\Events\RequestReceived;
use Spatie\Permission\PermissionRegistrar;

class OctaneReloadPermissions
{
    public function handle(RequestReceived $event): void
    {
        $event->sandbox->make(PermissionRegistrar::class)->clearClassPermissions();
    }
}
  1. Reference the listener in Octane's config file:
'listeners' => [
        RequestReceived::class => [
            ...Octane::prepareApplicationForNextOperation(),
            ...Octane::prepareApplicationForNextRequest(),
            \App\Listeners\OctaneReloadPermissions::class
        ],

If this works I can make a PR to add the feature: erikn69@844161d

@davidjr82, I tested the following on my octane instance:

  • create a role R1
  • create permissions
  • assign permissions to role R1
  • assigned Role and permissions to user:
$user->syncRoles($role->name);
$user->syncPermissions($role->permissions);
  • checked permissions via API - good
    image

  • create a role R2

  • create permissions

  • assign permissions to role R2

  • assigned Role and permissions to user
    image

On querying the user via API I am getting the exact role and the currently assigned permissions. Switching the roles & permissions / do some requests, the roles & permissions are always the currently assigned ones.

Getting the roles and permissions is working well on my side.

I also added a can - attribute to my API endpoint to ensure also this works well:

'can' => $this->resource->can('projects.index'),

image
image

Also, this is correct on every request.

Is there anything else I can try to reproduce this issue? I want to be sure this is working on production, and I don't want to drop octane.

@Cluster2a maybe is my implementation. Have you tried to put one request waiting after the change (like sleep(20)) and check within other request?

@parallels999 maybe it is my implementation, but when I change sail to use the default php built-in server instead of roadrunner, the problem disappears.

But I am fine closing this issue because as I said I have dropped Octane to use this package.

I think it makes more sense to open a new issue when someone can replicate this kind of problems in a small environment, so we can solve it.

@Cluster2a I finally discovered that the issue was produced by spatie/once package.
Just to make you know so you don't drop this package to use Octane because of this.

Here's more information on the actual Octane issue: https://github.com/spatie/once/pull/87/files

drbyte commented

@davidjr82 Thanks for updating this!

There is a PR now #2403