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

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

Closed
davidjr82 opened this issue Feb 7, 2023 · 21 comments

Comments

@davidjr82
Copy link

davidjr82 commented Feb 7, 2023

➡️ 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
@davidjr82
Copy link
Author

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

@davidjr82
Copy link
Author

Maybe cache is failing, try using array as cache

'store' => 'default',

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

@davidjr82
Copy link
Author

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

@davidjr82
Copy link
Author

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?

@olivernybroe
Copy link
Contributor

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

drbyte commented Feb 8, 2023

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;
}

@davidjr82
Copy link
Author

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.

@olivernybroe
Copy link
Contributor

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

drbyte commented Feb 10, 2023

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

@olivernybroe
Copy link
Contributor

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

@drbyte
Copy link
Collaborator

drbyte commented Feb 10, 2023

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

@davidjr82
Copy link
Author

@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).

@davidjr82
Copy link
Author

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.

@Cluster2a
Copy link

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?

@davidjr82
Copy link
Author

davidjr82 commented Mar 14, 2023

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)

@erikn69
Copy link
Contributor

erikn69 commented Mar 14, 2023

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

@Cluster2a
Copy link

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

@davidjr82
Copy link
Author

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

@davidjr82 davidjr82 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 14, 2023
@davidjr82
Copy link
Author

davidjr82 commented Apr 2, 2023

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

drbyte commented Apr 2, 2023

@davidjr82 Thanks for updating this!

@parallels999
Copy link
Contributor

There is a PR now #2403

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

6 participants