From 9905934f263bd6ddc53897ce98a51a73a055597c Mon Sep 17 00:00:00 2001 From: Lukas Bestle Date: Mon, 23 Dec 2024 11:04:59 +0100 Subject: [PATCH] DRY permission caching --- src/Cms/File.php | 14 ++------- src/Cms/FilePermissions.php | 8 +++++ src/Cms/ModelPermissions.php | 39 +++++++++++++++++++++++++ src/Cms/Page.php | 14 ++------- src/Cms/PagePermissions.php | 8 +++++ src/Cms/UserPermissions.php | 8 +++++ tests/Cms/Files/FilePermissionsTest.php | 30 +++++++++++++++++++ tests/Cms/Pages/PagePermissionsTest.php | 26 +++++++++++++++++ 8 files changed, 123 insertions(+), 24 deletions(-) diff --git a/src/Cms/File.php b/src/Cms/File.php index 0c5a346306..f3f7b363e8 100644 --- a/src/Cms/File.php +++ b/src/Cms/File.php @@ -318,12 +318,7 @@ public function isAccessible(): bool return false; } - static $accessible = []; - $role = $this->kirby()->role()?->id() ?? '__none__'; - $template = $this->template() ?? '__none__'; - $accessible[$role] ??= []; - - return $accessible[$role][$template] ??= $this->permissions()->access(); + return $this->permissions()->access(); } /** @@ -342,12 +337,7 @@ public function isListable(): bool return false; } - static $listable = []; - $role = $this->kirby()->role()?->id() ?? '__none__'; - $template = $this->template() ?? '__none__'; - $listable[$role] ??= []; - - return $listable[$role][$template] ??= $this->permissions()->list(); + return $this->permissions()->list(); } /** diff --git a/src/Cms/FilePermissions.php b/src/Cms/FilePermissions.php index 2f8b777237..d10194505c 100644 --- a/src/Cms/FilePermissions.php +++ b/src/Cms/FilePermissions.php @@ -15,6 +15,14 @@ class FilePermissions extends ModelPermissions { protected string $category = 'files'; + /** + * Used to cache once determined permissions in memory + */ + protected function cacheKey(): string + { + return $this->model->template() ?? '__none__'; + } + protected function canChangeTemplate(): bool { if (count($this->model->blueprints()) <= 1) { diff --git a/src/Cms/ModelPermissions.php b/src/Cms/ModelPermissions.php index e5baa290a9..5f93b7f968 100644 --- a/src/Cms/ModelPermissions.php +++ b/src/Cms/ModelPermissions.php @@ -18,6 +18,8 @@ abstract class ModelPermissions protected string $category; protected array $options; + protected static array $cache = []; + public function __construct(protected ModelWithContent|Language $model) { $this->options = match (true) { @@ -40,6 +42,16 @@ public function __debugInfo(): array return $this->toArray(); } + /** + * Can be overridden by specific child classes + * to return a model-specific value used to + * cache a once determined permission in memory + */ + protected function cacheKey(): string + { + return ''; + } + /** * Returns whether the current user is allowed to do * a certain action on the model @@ -75,6 +87,33 @@ public function can( return true; } + // cache the often-used read permissions; + // note that the custom `can` method check happens before, + // so dynamic code still has the chance to be run each time + if ($action === 'access' || $action === 'list') { + $category = $this->category(); + $cacheKey = $category . '.' . $action . '/' . $this->cacheKey() . '/' . $role; + + if (isset(static::$cache[$cacheKey]) === true) { + return static::$cache[$cacheKey]; + } + + return static::$cache[$cacheKey] = $this->canFromOptionsAndRole($action, $role, $default); + } + + // determine all other permissions dynamically + // TODO: caching these is generally possible, but currently makes many unit tests break + return $this->canFromOptionsAndRole($action, $role, $default); + } + + /** + * Main logic for `can()` that can be cached + */ + protected function canFromOptionsAndRole( + string $action, + string $role, + bool $default = false + ): bool { // evaluate the blueprint options block if (isset($this->options[$action]) === true) { $options = $this->options[$action]; diff --git a/src/Cms/Page.php b/src/Cms/Page.php index 5ad56c5df3..50eede5461 100644 --- a/src/Cms/Page.php +++ b/src/Cms/Page.php @@ -522,12 +522,7 @@ public function isAccessible(): bool return false; } - static $accessible = []; - $role = $this->kirby()->role()?->id() ?? '__none__'; - $template = $this->intendedTemplate()->name(); - $accessible[$role] ??= []; - - return $accessible[$role][$template] ??= $this->permissions()->access(); + return $this->permissions()->access(); } /** @@ -694,12 +689,7 @@ public function isListable(): bool return false; } - static $listable = []; - $role = $this->kirby()->role()?->id() ?? '__none__'; - $template = $this->intendedTemplate()->name(); - $listable[$role] ??= []; - - return $listable[$role][$template] ??= $this->permissions()->list(); + return $this->permissions()->list(); } /** diff --git a/src/Cms/PagePermissions.php b/src/Cms/PagePermissions.php index b4ca11840d..fc412f108a 100644 --- a/src/Cms/PagePermissions.php +++ b/src/Cms/PagePermissions.php @@ -15,6 +15,14 @@ class PagePermissions extends ModelPermissions { protected string $category = 'pages'; + /** + * Used to cache once determined permissions in memory + */ + protected function cacheKey(): string + { + return $this->model->intendedTemplate()->name(); + } + protected function canChangeSlug(): bool { return $this->model->isHomeOrErrorPage() !== true; diff --git a/src/Cms/UserPermissions.php b/src/Cms/UserPermissions.php index 6b9ca49cc9..e8cd5f8b86 100644 --- a/src/Cms/UserPermissions.php +++ b/src/Cms/UserPermissions.php @@ -13,6 +13,14 @@ */ class UserPermissions extends ModelPermissions { + /** + * Used to cache once determined permissions in memory + */ + protected function cacheKey(): string + { + return $this->model->role()->id(); + } + protected function canChangeRole(): bool { // protect admin from role changes by non-admin diff --git a/tests/Cms/Files/FilePermissionsTest.php b/tests/Cms/Files/FilePermissionsTest.php index ec1cfac989..66e3131e1c 100644 --- a/tests/Cms/Files/FilePermissionsTest.php +++ b/tests/Cms/Files/FilePermissionsTest.php @@ -14,6 +14,9 @@ public function setUp(): void $this->app = new App([ 'roots' => [ 'index' => '/dev/null' + ], + 'users' => [ + ['id' => 'bastian', 'role' => 'admin'] ] ]); } @@ -60,6 +63,33 @@ public function testWithNobody($action) $this->assertFalse($perms->can($action)); } + /** + * @covers \Kirby\Cms\ModelPermissions::can + */ + public function testCaching() + { + $this->app->impersonate('bastian'); + + $page = new Page(['slug' => 'test']); + $file = new File([ + 'filename' => 'test.jpg', + 'parent' => $page, + 'template' => 'some-template', + 'blueprint' => [ + 'name' => 'files/some-template', + 'options' => [ + 'access' => false, + 'list' => false + ] + ] + ]); + + $this->assertFalse($file->permissions()->can('access')); + $this->assertFalse($file->permissions()->can('access')); + $this->assertFalse($file->permissions()->can('list')); + $this->assertFalse($file->permissions()->can('list')); + } + /** * @covers ::canChangeTemplate */ diff --git a/tests/Cms/Pages/PagePermissionsTest.php b/tests/Cms/Pages/PagePermissionsTest.php index 2d8ff56de4..5d43ccef6e 100644 --- a/tests/Cms/Pages/PagePermissionsTest.php +++ b/tests/Cms/Pages/PagePermissionsTest.php @@ -270,6 +270,32 @@ public function testWithNobody($action) $this->assertFalse($perms->can($action)); } + /** + * @covers \Kirby\Cms\ModelPermissions::can + */ + public function testCaching() + { + $this->app->impersonate('bastian'); + + $page = new Page([ + 'slug' => 'test', + 'num' => 1, + 'template' => 'some-template', + 'blueprint' => [ + 'name' => 'some-template', + 'options' => [ + 'access' => false, + 'list' => false + ] + ] + ]); + + $this->assertFalse($page->permissions()->can('access')); + $this->assertFalse($page->permissions()->can('access')); + $this->assertFalse($page->permissions()->can('list')); + $this->assertFalse($page->permissions()->can('list')); + } + /** * @covers ::canChangeTemplate */