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

Cached permissions on multiple instances running on Octane / Kubernetes #2575

Closed
Cluster2a opened this issue Dec 11, 2023 · 12 comments · Fixed by #2583
Closed

Cached permissions on multiple instances running on Octane / Kubernetes #2575

Cluster2a opened this issue Dec 11, 2023 · 12 comments · Fixed by #2583
Labels

Comments

@Cluster2a
Copy link

Cluster2a commented Dec 11, 2023

Describe the bug
Running on Octane and multiple Kubernetes pods, the permissions seem to be cached on each pod. So basically a simple can check for viewAny will result in a 403 or a result (depending on which pod the request hits).

We are using wildcard permissions.

Versions

  • spatie/laravel-permission package version: 6.2.0
  • laravel/framework: v10.35.0

PHP version: 8.2

Database version: mysql:8.2.0

To Reproduce

  • run multiple instances on octane
  • assign a role to the user with a wildcard permission (e.g.: suites.viewAny)
  • call an API endpoint that checks for this endpoint
  • remove the permission from the role
  • call the endpoint multiple times: some requests result in a 403 / some in showing the resource list

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

Controller:

$user = auth()->user();
Gate::forUser($user)->authorize('viewAny', Booking::class);

Policy:

public function viewAny(User $user): bool
{
    return $user->can(Resources::BOOKINGS . '.' . PermissionsActions::VIEW_ANY);
}

Even though register_octane_reset_listener set to true the permissions seem to be cached somehow. After checking the code, I saw that you call $event->sandbox->make(PermissionRegistrar::class)->clearPermissionsCollection(); if the flag is set to true.

I made my own Event Listener for the OperationTerminated-Event which looks like this:

class OctaneReloadPermissions
{
    /**
     * @param $event
     *
     * @return void
     */
    public function handle($event): void
    {
        $event->sandbox->make(PermissionRegistrar::class)->setPermissionsTeamId(null);
        $event->sandbox->make(PermissionRegistrar::class)->forgetCachedPermissions();
    }
}

I noticed that $event->sandbox->make(PermissionRegistrar::class)->forgetCachedPermissions(); also seems to clear the wildcard permissions, which we are using. Calling forgetCachedPermissions() seems to do the trick, as I am not able to reproduce the behaviour anymore.

If enable_wildcard_permission is set to true shouldn't the forgetWildcardPermissionIndex() also be called within the registerOctaneListener()?

I think the comment in the permissions.php NOTE: This should not be needed in most cases, but an Octane/Vapor combination benefited from it. might be missleading. I think every multi instance setup running on octane might be effected.

Expected behavior
The permission cache should be cleared after each request including wildcard permissions.

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

  • Image: php:8.2.13-cli-alpine3.18
@Cluster2a Cluster2a changed the title Cached permissions on multiple Instances running on Octane / Kubernetes Cached permissions on multiple instances running on Octane / Kubernetes Dec 11, 2023
@drbyte
Copy link
Collaborator

drbyte commented Dec 11, 2023

I've not yet worked with Octane for a production app, so my "testing" of things like this has been limited. But would like to dig deeper.

Do you mind providing a demo app, replicating the features and symptoms you describe, which we can use for exploring and debugging further?

@Cluster2a
Copy link
Author

@drbyte, as the current code base relies on many services (realtime events, after role switch), it would be easier to create a simple example with a single controller and a few endpoints to change a role.

I think the best way to test & debug would be an example which runs on a local micrk8s instance. But I would need some time for the example.

@parallels999
Copy link
Contributor

parallels999 commented Dec 11, 2023

$event->sandbox->make(PermissionRegistrar::class)->forgetCachedPermissions();

that removes the cache in each request and create a new one again, if you are going to do that it is better to use array in the cache store configuration, this way you will use less resources

'store' => 'default',

What do you use for the cache? redis?

@Cluster2a
Copy link
Author

What do you use for the cache? redis?

I am using redis. I tried a different approach, since I don't want to clear the entire cache:
image

This also works.

@drbyte, fyi.

@parallels999
Copy link
Contributor

Make a PR

$dispatcher->listen(function (\Laravel\Octane\Events\OperationTerminated $event) {
// @phpstan-ignore-next-line
$event->sandbox->make(PermissionRegistrar::class)->clearPermissionsCollection();
});

@drbyte
Copy link
Collaborator

drbyte commented Dec 13, 2023

I'm not sure it's safe to hard-code any resets to instance of User because we can't guarantee that.
Perhaps instance of Authenticatable contract.

But is the call to $user->resetPermissionDate() critical?

@drbyte
Copy link
Collaborator

drbyte commented Dec 13, 2023

I agree with the call to setPermissionsTeamId(null) and forgetWildcardPermissionIndex().

@Cluster2a
Copy link
Author

@drbyte, we are using some custom permission cache on the user - this part can be ignored. Most important seems to be, that the wildcard cache is beeing deleted.

@drbyte
Copy link
Collaborator

drbyte commented Dec 14, 2023

Okay. I think what we'll do is, in this package add the reset for team ID and wildcard permissions.

And then for the parts you need in your own app, you can register a listener on Octane's OperationTerminated for those things unique to your own requirements.

@drbyte
Copy link
Collaborator

drbyte commented Dec 14, 2023

Hmmm ... I forgot: we already have the reset for team ID since 6.1.0

@drbyte
Copy link
Collaborator

drbyte commented Dec 14, 2023

So ... @erikn69 @parallels999 I'm curious your opinion on whether we should add an Octane-specific reset for forgetWildcardPermissionIndex(), or simply combine forgetWildcardPermissionIndex() into clearPermissionsCollection()

/**
* Clear already-loaded permissions collection.
* This is only intended to be called by the PermissionServiceProvider on boot,
* so that long-running instances like Octane or Swoole don't keep old data in memory.
*/
public function clearPermissionsCollection(): void
{
$this->permissions = null;
}

Noting:

/**
* Flush the cache.
*/
public function forgetCachedPermissions()
{
$this->permissions = null;
$this->forgetWildcardPermissionIndex();
return $this->cache->forget($this->cacheKey);
}
public function forgetWildcardPermissionIndex(?Model $record = null): void
{
if ($record) {
unset($this->wildcardPermissionsIndex[get_class($record)][$record->getKey()]);
return;
}
$this->wildcardPermissionsIndex = [];
}

@parallels999
Copy link
Contributor

public function clearPermissionsCollection(): void 
{ 
   $this->permissions = null; 
   $this->wildcardPermissionsIndex = []; 
} 

drbyte added a commit to drbyte/laravel-permission that referenced this issue Dec 24, 2023
@drbyte drbyte added the bug label Dec 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants