Skip to content

Commit

Permalink
Don’t keep user object in memory cache
Browse files Browse the repository at this point in the history
Made the permission checks return stale results if the user changed during the request
  • Loading branch information
lukasbestle committed Dec 22, 2024
1 parent b6e81f9 commit 13ea6fc
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 39 deletions.
32 changes: 23 additions & 9 deletions src/Cms/ModelPermissions.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,9 @@ abstract class ModelPermissions
{
protected string $category;
protected array $options;
protected Permissions $permissions;
protected User $user;

public function __construct(protected ModelWithContent|Language $model)
{
$this->user = $model->kirby()->user() ?? User::nobody();
$this->permissions = $this->user->role()->permissions();

$this->options = match (true) {
$model instanceof ModelWithContent => $model->blueprint()->options(),
default => []
Expand Down Expand Up @@ -55,8 +50,9 @@ public function can(
string $action,
bool $default = false
): bool {
$user = $this->user->id();
$role = $this->user->role()->id();
$user = $this->user();
$userId = $user->id();
$role = $user->role()->id();

// users with the `nobody` role can do nothing
// that needs a permission check
Expand All @@ -75,7 +71,7 @@ public function can(
}

// the almighty `kirby` user can do anything
if ($user === 'kirby' && $role === 'admin') {
if ($userId === 'kirby' && $role === 'admin') {
return true;
}

Expand Down Expand Up @@ -105,7 +101,8 @@ public function can(
}
}

return $this->permissions->for($this->category, $action, $default);
$permissions = $this->user()->role()->permissions();
return $permissions->for($this->category(), $action, $default);
}

/**
Expand All @@ -121,6 +118,15 @@ public function cannot(
return $this->can($action, !$default) === false;
}

/**
* Can be overridden by specific child classes
* if the permission category needs to be dynamic
*/
protected function category(): string
{
return $this->category;
}

public function toArray(): array
{
$array = [];
Expand All @@ -131,4 +137,12 @@ public function toArray(): array

return $array;
}

/**
* Returns the currently logged in user
*/
protected function user(): User
{
return $this->model->kirby()->user() ?? User::nobody();
}
}
22 changes: 9 additions & 13 deletions src/Cms/UserPermissions.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,12 @@
*/
class UserPermissions extends ModelPermissions
{
protected string $category = 'users';

public function __construct(User $model)
{
parent::__construct($model);

// change the scope of the permissions,
// when the current user is this user
$this->category = $this->user?->is($model) ? 'user' : 'users';
}

protected function canChangeRole(): bool
{
// protect admin from role changes by non-admin
if (
$this->model->isAdmin() === true &&
$this->user->isAdmin() !== true
$this->user()->isAdmin() !== true
) {
return false;
}
Expand All @@ -45,7 +34,7 @@ protected function canChangeRole(): bool
protected function canCreate(): bool
{
// the admin can always create new users
if ($this->user->isAdmin() === true) {
if ($this->user()->isAdmin() === true) {
return true;
}

Expand All @@ -61,4 +50,11 @@ protected function canDelete(): bool
{
return $this->model->isLastAdmin() !== true;
}

protected function category(): string
{
// change the scope of the permissions,
// when the current user is this user
return $this->user()->is($this->model) ? 'user' : 'users';
}
}
38 changes: 21 additions & 17 deletions tests/Cms/Users/UserPermissionsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ public function testWithNoAdmin($action)
'index' => '/dev/null'
],
'roles' => [
['name' => 'admin'],
[
'name' => 'editor',
'permissions' => [
Expand All @@ -88,36 +87,41 @@ public function testWithNoAdmin($action)
'update' => false
],
'users' => [
'changeEmail' => false,
'changeLanguage' => false,
'changeName' => false,
'changePassword' => false,
'changeRole' => false,
'create' => false,
'delete' => false,
'update' => false
'changeEmail' => true,
'changeLanguage' => true,
'changeName' => true,
'changePassword' => true,
'changeRole' => true,
'create' => true,
'delete' => true,
'update' => true
]
]
]
],
'user' => 'editor@getkirby.com',
'user' => 'editor1@getkirby.com',
'users' => [
[
'email' => 'admin@getkirby.com',
'role' => 'admin'
'email' => 'editor1@getkirby.com',
'role' => 'editor'
],
[
'email' => 'editor@getkirby.com',
'email' => 'editor2@getkirby.com',
'role' => 'editor'
]
],
]);

$user = $app->user();
$perms = $user->permissions();
// `user` permissions are disabled
$user1 = $app->user();
$perms1 = $user1->permissions();
$this->assertSame('editor', $user1->role()->name());
$this->assertFalse($perms1->can($action));

$this->assertSame('editor', $user->role()->name());
$this->assertFalse($perms->can($action));
// `users` permissions are enabled
$user2 = $app->user('[email protected]');
$perms2 = $user2->permissions();
$this->assertTrue($perms2->can($action));
}

public function testChangeSingleRole()
Expand Down

0 comments on commit 13ea6fc

Please sign in to comment.