From 7aa69faadfd6ae536099afeabfe7ae254b13bfc3 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 28 Feb 2025 11:17:04 +0000 Subject: [PATCH 01/27] Only perform super user check in `Gate::before()` when ability is a Statamic permission --- src/Providers/AuthServiceProvider.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Providers/AuthServiceProvider.php b/src/Providers/AuthServiceProvider.php index c37bb1b03af..0e3b7db1fee 100755 --- a/src/Providers/AuthServiceProvider.php +++ b/src/Providers/AuthServiceProvider.php @@ -14,6 +14,7 @@ use Statamic\Contracts\Auth\RoleRepository; use Statamic\Contracts\Auth\UserGroupRepository; use Statamic\Contracts\Auth\UserRepository; +use Statamic\Facades\Permission; use Statamic\Facades\User; use Statamic\Policies; @@ -84,7 +85,11 @@ public function boot() }); Gate::before(function ($user, $ability) { - return optional(User::fromUser($user))->isSuper() ? true : null; + $isStatamicPermission = Permission::all()->first(fn ($permission) => $permission->value() === $ability); + + if ($isStatamicPermission) { + return optional(User::fromUser($user))->isSuper() ? true : null; + } }); Gate::after(function ($user, $ability) { From 39a696e6bf30bb0f9dce9659b09856730e1ddfe5 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 28 Feb 2025 11:18:41 +0000 Subject: [PATCH 02/27] Ensure permissions are booted before authorization is handled Otherwise, when the `Authorize` middleware checks if the user has access to the CP, `Permission:all()` in the `Gate::before()` closure won't return any permissions as they haven't been booted yet. --- src/Providers/CpServiceProvider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Providers/CpServiceProvider.php b/src/Providers/CpServiceProvider.php index 666ba482b33..2ef316489de 100644 --- a/src/Providers/CpServiceProvider.php +++ b/src/Providers/CpServiceProvider.php @@ -94,10 +94,10 @@ protected function registerMiddlewareGroups() $router->middlewareGroup('statamic.cp.authenticated', [ \Statamic\Http\Middleware\CP\AuthenticateSession::class, + \Statamic\Http\Middleware\CP\BootPermissions::class, \Statamic\Http\Middleware\CP\Authorize::class, \Statamic\Http\Middleware\CP\Localize::class, \Statamic\Http\Middleware\CP\SelectedSite::class, - \Statamic\Http\Middleware\CP\BootPermissions::class, \Statamic\Http\Middleware\CP\BootPreferences::class, \Statamic\Http\Middleware\CP\BootUtilities::class, \Statamic\Http\Middleware\CP\CountUsers::class, From f34eafae7081ca02402588347d41d554a1b3f886 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 28 Feb 2025 11:18:56 +0000 Subject: [PATCH 03/27] =?UTF-8?q?Remove=20commented=20out=20`dd()`=20from?= =?UTF-8?q?=20middleware=20=F0=9F=98=84?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/Http/Middleware/CP/Authorize.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Http/Middleware/CP/Authorize.php b/src/Http/Middleware/CP/Authorize.php index 336acc232f8..17d21efd299 100644 --- a/src/Http/Middleware/CP/Authorize.php +++ b/src/Http/Middleware/CP/Authorize.php @@ -18,7 +18,6 @@ public function handle($request, Closure $next) } if ($user->cant('access cp')) { - // dd('theres a user but they are unauthorized', $user); throw new AuthorizationException('Unauthorized.'); } From 639a0590b804ff2c45c9d1f987f82fef9c8864dc Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 28 Feb 2025 11:19:52 +0000 Subject: [PATCH 04/27] Add super user check to the `before` method in authorization policies. --- src/Policies/AssetContainerPolicy.php | 7 ++++++- src/Policies/AssetFolderPolicy.php | 4 ++++ src/Policies/AssetPolicy.php | 5 ++++- src/Policies/CollectionPolicy.php | 5 ++++- src/Policies/EntryPolicy.php | 5 ++++- src/Policies/FieldsetPolicy.php | 5 ++++- src/Policies/FormPolicy.php | 5 ++++- src/Policies/FormSubmissionPolicy.php | 5 ++++- src/Policies/GlobalSetPolicy.php | 5 ++++- src/Policies/NavPolicy.php | 5 ++++- src/Policies/NavTreePolicy.php | 7 +++++++ src/Policies/SitePolicy.php | 7 +++++++ src/Policies/TaxonomyPolicy.php | 5 ++++- src/Policies/TermPolicy.php | 5 ++++- src/Policies/UserPolicy.php | 7 +++++++ 15 files changed, 71 insertions(+), 11 deletions(-) diff --git a/src/Policies/AssetContainerPolicy.php b/src/Policies/AssetContainerPolicy.php index 2daa4455388..06cca1bd0a1 100644 --- a/src/Policies/AssetContainerPolicy.php +++ b/src/Policies/AssetContainerPolicy.php @@ -9,7 +9,12 @@ class AssetContainerPolicy { public function before($user, $ability) { - if (User::fromUser($user)->hasPermission('configure asset containers')) { + $user = User::fromUser($user); + + if ( + $user->isSuper() || + $user->hasPermission('configure asset containers') + ) { return true; } } diff --git a/src/Policies/AssetFolderPolicy.php b/src/Policies/AssetFolderPolicy.php index f977508f84d..5a7d304cfb7 100644 --- a/src/Policies/AssetFolderPolicy.php +++ b/src/Policies/AssetFolderPolicy.php @@ -12,6 +12,10 @@ public function create($user, $assetContainer) { $user = User::fromUser($user); + if ($user->isSuper()) { + return true; + } + if (! $user->hasPermission("upload {$assetContainer->handle()} assets")) { return false; } diff --git a/src/Policies/AssetPolicy.php b/src/Policies/AssetPolicy.php index 2892deed267..3e4de51d875 100644 --- a/src/Policies/AssetPolicy.php +++ b/src/Policies/AssetPolicy.php @@ -10,7 +10,10 @@ public function before($user) { $user = User::fromUser($user); - if ($user->hasPermission('configure asset containers')) { + if ( + $user->isSuper() || + $user->hasPermission('configure asset containers') + ) { return true; } } diff --git a/src/Policies/CollectionPolicy.php b/src/Policies/CollectionPolicy.php index d17a9682cec..dfd8aa1c2db 100644 --- a/src/Policies/CollectionPolicy.php +++ b/src/Policies/CollectionPolicy.php @@ -13,7 +13,10 @@ public function before($user) { $user = User::fromUser($user); - if ($user->hasPermission('configure collections')) { + if ( + $user->isSuper() || + $user->hasPermission('configure collections') + ) { return true; } } diff --git a/src/Policies/EntryPolicy.php b/src/Policies/EntryPolicy.php index ba6f7ab9e57..073b4afec19 100644 --- a/src/Policies/EntryPolicy.php +++ b/src/Policies/EntryPolicy.php @@ -12,7 +12,10 @@ public function before($user) { $user = User::fromUser($user); - if ($user->hasPermission('configure collections')) { + if ( + $user->isSuper() || + $user->hasPermission('configure collections') + ) { return true; } } diff --git a/src/Policies/FieldsetPolicy.php b/src/Policies/FieldsetPolicy.php index dcdc22d0502..4bf6ee807e8 100644 --- a/src/Policies/FieldsetPolicy.php +++ b/src/Policies/FieldsetPolicy.php @@ -10,7 +10,10 @@ public function before($user, $ability, $fieldset) { $user = User::fromUser($user); - if ($user->hasPermission('configure fields')) { + if ( + $user->isSuper() || + $user->hasPermission('configure fields') + ) { return true; } } diff --git a/src/Policies/FormPolicy.php b/src/Policies/FormPolicy.php index 13358aba186..e75a6e6d86b 100644 --- a/src/Policies/FormPolicy.php +++ b/src/Policies/FormPolicy.php @@ -11,7 +11,10 @@ public function before($user, $ability) { $user = User::fromUser($user); - if ($user->hasPermission('configure forms')) { + if ( + $user->isSuper() || + $user->hasPermission('configure forms') + ) { return true; } } diff --git a/src/Policies/FormSubmissionPolicy.php b/src/Policies/FormSubmissionPolicy.php index e5d4e7b318e..2624b56a9de 100644 --- a/src/Policies/FormSubmissionPolicy.php +++ b/src/Policies/FormSubmissionPolicy.php @@ -10,7 +10,10 @@ public function before($user, $ability) { $user = User::fromUser($user); - if ($user->hasPermission('configure forms')) { + if ( + $user->isSuper() || + $user->hasPermission('configure forms') + ) { return true; } } diff --git a/src/Policies/GlobalSetPolicy.php b/src/Policies/GlobalSetPolicy.php index 6f48eb36b49..2417dd553f4 100644 --- a/src/Policies/GlobalSetPolicy.php +++ b/src/Policies/GlobalSetPolicy.php @@ -13,7 +13,10 @@ public function before($user) { $user = User::fromUser($user); - if ($user->hasPermission('configure globals')) { + if ( + $user->isSuper() || + $user->hasPermission('configure globals') + ) { return true; } } diff --git a/src/Policies/NavPolicy.php b/src/Policies/NavPolicy.php index 9b63a6ad26e..8f939ebadc4 100644 --- a/src/Policies/NavPolicy.php +++ b/src/Policies/NavPolicy.php @@ -13,7 +13,10 @@ public function before($user) { $user = User::fromUser($user); - if ($user->hasPermission('configure navs')) { + if ( + $user->isSuper() || + $user->hasPermission('configure navs') + ) { return true; } } diff --git a/src/Policies/NavTreePolicy.php b/src/Policies/NavTreePolicy.php index 2caa568e0af..1fbdfc8dcef 100644 --- a/src/Policies/NavTreePolicy.php +++ b/src/Policies/NavTreePolicy.php @@ -8,6 +8,13 @@ class NavTreePolicy extends NavPolicy { use Concerns\HasMultisitePolicy; + public function before($user) + { + if (User::fromUser($user)->isSuper()) { + return true; + } + } + public function view($user, $nav) { $user = User::fromUser($user); diff --git a/src/Policies/SitePolicy.php b/src/Policies/SitePolicy.php index 5981a5c2e90..61cab2040b1 100644 --- a/src/Policies/SitePolicy.php +++ b/src/Policies/SitePolicy.php @@ -7,6 +7,13 @@ class SitePolicy { + public function before($user) + { + if (User::fromUser($user)->isSuper()) { + return true; + } + } + public function view($user, $site) { if (! Site::multiEnabled()) { diff --git a/src/Policies/TaxonomyPolicy.php b/src/Policies/TaxonomyPolicy.php index ed91bd4c8bc..db345ee857d 100644 --- a/src/Policies/TaxonomyPolicy.php +++ b/src/Policies/TaxonomyPolicy.php @@ -13,7 +13,10 @@ public function before($user) { $user = User::fromUser($user); - if ($user->hasPermission('configure taxonomies')) { + if ( + $user->isSuper() || + $user->hasPermission('configure taxonomies') + ) { return true; } } diff --git a/src/Policies/TermPolicy.php b/src/Policies/TermPolicy.php index b823b87743d..f3fedceff9b 100644 --- a/src/Policies/TermPolicy.php +++ b/src/Policies/TermPolicy.php @@ -12,7 +12,10 @@ public function before($user) { $user = User::fromUser($user); - if ($user->hasPermission('configure taxonomies')) { + if ( + $user->isSuper() || + $user->hasPermission('configure taxonomies') + ) { return true; } } diff --git a/src/Policies/UserPolicy.php b/src/Policies/UserPolicy.php index 3bf5380ba11..c691d2256ba 100644 --- a/src/Policies/UserPolicy.php +++ b/src/Policies/UserPolicy.php @@ -6,6 +6,13 @@ class UserPolicy { + public function before($user) + { + if (User::fromUser($user)->isSuper()) { + return true; + } + } + public function index($authed) { $authed = User::fromUser($authed); From 8fa1c6b3e64155a7aa64c5b266469519a8846086 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 28 Feb 2025 12:22:25 +0000 Subject: [PATCH 05/27] Ensure permissions exist, otherwise the super user check will fail. --- tests/CP/Navigation/ActiveNavItemTest.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/CP/Navigation/ActiveNavItemTest.php b/tests/CP/Navigation/ActiveNavItemTest.php index 5f344c902c8..779da8e24bc 100644 --- a/tests/CP/Navigation/ActiveNavItemTest.php +++ b/tests/CP/Navigation/ActiveNavItemTest.php @@ -218,6 +218,9 @@ public function it_resolves_core_children_closure_and_can_check_when_parent_and_ #[Test] public function it_can_check_if_parent_extension_with_array_based_children_item_is_active() { + Facades\Permission::register('view seo reports'); + Facades\Permission::register('edit seo section defaults'); + Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') @@ -243,6 +246,9 @@ public function it_can_check_if_parent_extension_with_array_based_children_item_ #[Test] public function it_can_check_when_parent_and_array_based_child_extension_items_are_active() { + Facades\Permission::register('view seo reports'); + Facades\Permission::register('edit seo section defaults'); + Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') @@ -268,6 +274,9 @@ public function it_can_check_when_parent_and_array_based_child_extension_items_a #[Test] public function it_can_check_when_parent_and_array_based_descendant_of_child_extension_item_is_active() { + Facades\Permission::register('view seo reports'); + Facades\Permission::register('edit seo section defaults'); + Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') @@ -319,6 +328,9 @@ public function it_builds_extension_children_closure_when_not_active() #[Test] public function it_resolves_extension_children_closure_and_can_check_when_parent_item_is_active() { + Facades\Permission::register('view seo reports'); + Facades\Permission::register('edit seo section defaults'); + Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') @@ -346,6 +358,9 @@ public function it_resolves_extension_children_closure_and_can_check_when_parent #[Test] public function it_resolves_extension_children_closure_and_can_check_when_parent_and_child_item_are_active() { + Facades\Permission::register('view seo reports'); + Facades\Permission::register('edit seo section defaults'); + Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') @@ -373,6 +388,9 @@ public function it_resolves_extension_children_closure_and_can_check_when_parent #[Test] public function it_resolves_extension_children_closure_and_can_check_when_parent_and_descendant_of_child_item_is_active() { + Facades\Permission::register('view seo reports'); + Facades\Permission::register('edit seo section defaults'); + Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') From 1b918348d7626a043e4b1504e822c9e96457e7f5 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 28 Feb 2025 12:25:15 +0000 Subject: [PATCH 06/27] Move the middleware back to where they were originally. --- src/Providers/CpServiceProvider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Providers/CpServiceProvider.php b/src/Providers/CpServiceProvider.php index 2ef316489de..666ba482b33 100644 --- a/src/Providers/CpServiceProvider.php +++ b/src/Providers/CpServiceProvider.php @@ -94,10 +94,10 @@ protected function registerMiddlewareGroups() $router->middlewareGroup('statamic.cp.authenticated', [ \Statamic\Http\Middleware\CP\AuthenticateSession::class, - \Statamic\Http\Middleware\CP\BootPermissions::class, \Statamic\Http\Middleware\CP\Authorize::class, \Statamic\Http\Middleware\CP\Localize::class, \Statamic\Http\Middleware\CP\SelectedSite::class, + \Statamic\Http\Middleware\CP\BootPermissions::class, \Statamic\Http\Middleware\CP\BootPreferences::class, \Statamic\Http\Middleware\CP\BootUtilities::class, \Statamic\Http\Middleware\CP\CountUsers::class, From 77706753ea6e694c281d328fbf304003950f4f97 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 28 Feb 2025 12:29:18 +0000 Subject: [PATCH 07/27] Boot permissions in `Gate::before()` method... MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Until I figure out a better solution. 🤔 --- src/Providers/AuthServiceProvider.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Providers/AuthServiceProvider.php b/src/Providers/AuthServiceProvider.php index 0e3b7db1fee..ac60de55f11 100755 --- a/src/Providers/AuthServiceProvider.php +++ b/src/Providers/AuthServiceProvider.php @@ -85,6 +85,8 @@ public function boot() }); Gate::before(function ($user, $ability) { + Permission::boot(); + $isStatamicPermission = Permission::all()->first(fn ($permission) => $permission->value() === $ability); if ($isStatamicPermission) { From 19953741a8792317f64b64f6a6297caed3e2ad2f Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 28 Feb 2025 12:32:35 +0000 Subject: [PATCH 08/27] Update an existing test Since there's no `DroidsClass` policy, I've updated this test to authorize using a permission instead, which'll work. --- tests/CP/Navigation/NavTest.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/CP/Navigation/NavTest.php b/tests/CP/Navigation/NavTest.php index 91834f1d861..72b8ba48df7 100644 --- a/tests/CP/Navigation/NavTest.php +++ b/tests/CP/Navigation/NavTest.php @@ -73,12 +73,14 @@ public function it_can_create_a_nav_item_with_a_more_custom_config() { $this->actingAs(tap(User::make()->makeSuper())->save()); + Facades\Permission::register('view droids'); + Nav::droids('C-3PO') ->id('some::custom::id') ->active('threepio*') ->url('/human-cyborg-relations') ->view('cp.nav.importer') - ->can('index', 'DroidsClass') + ->can('view droids') ->attributes(['target' => '_blank', 'class' => 'red']); $item = $this->build()->get('Droids')->first(); @@ -89,8 +91,7 @@ public function it_can_create_a_nav_item_with_a_more_custom_config() $this->assertEquals('http://localhost/human-cyborg-relations', $item->url()); $this->assertEquals('cp.nav.importer', $item->view()); $this->assertEquals('threepio*', $item->active()); - $this->assertEquals('index', $item->authorization()->ability); - $this->assertEquals('DroidsClass', $item->authorization()->arguments); + $this->assertEquals('view droids', $item->authorization()->ability); $this->assertEquals(' target="_blank" class="red"', $item->attributes()); } From b3659092545b1131b9c493f8f1639aefb639f702 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 28 Feb 2025 12:45:20 +0000 Subject: [PATCH 09/27] Fix super authorization in AssetFolderPolicy --- src/Policies/AssetFolderPolicy.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Policies/AssetFolderPolicy.php b/src/Policies/AssetFolderPolicy.php index 5a7d304cfb7..bd8b9dcd5a3 100644 --- a/src/Policies/AssetFolderPolicy.php +++ b/src/Policies/AssetFolderPolicy.php @@ -8,13 +8,16 @@ class AssetFolderPolicy { - public function create($user, $assetContainer) + public function before($user) { - $user = User::fromUser($user); - - if ($user->isSuper()) { + if (User::fromUser($user)->isSuper()) { return true; } + } + + public function create($user, $assetContainer) + { + $user = User::fromUser($user); if (! $user->hasPermission("upload {$assetContainer->handle()} assets")) { return false; From 3ce770589ab0d6540368955299eacfb7f56ec723 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Thu, 6 Mar 2025 15:20:38 +0000 Subject: [PATCH 10/27] Return early when permissions have already been booted. --- src/Auth/Permissions.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Auth/Permissions.php b/src/Auth/Permissions.php index 7d20b524f22..c5920a17688 100644 --- a/src/Auth/Permissions.php +++ b/src/Auth/Permissions.php @@ -10,9 +10,14 @@ class Permissions protected $permissions = []; protected $groups = []; protected $pendingGroup = null; + protected $booted = false; public function boot() { + if ($this->booted) { + return; + } + $early = $this->permissions; $this->permissions = []; @@ -23,6 +28,7 @@ public function boot() } $this->permissions = array_merge($this->permissions, $early); + $this->booted = true; } public function extend($callback) From 88e702f69c20d1ac37d9da91a692f5705ebd9741 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 7 Mar 2025 12:29:31 +0000 Subject: [PATCH 11/27] Move super user check to `Gate::after()` --- src/Providers/AuthServiceProvider.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Providers/AuthServiceProvider.php b/src/Providers/AuthServiceProvider.php index ac60de55f11..a8561935246 100755 --- a/src/Providers/AuthServiceProvider.php +++ b/src/Providers/AuthServiceProvider.php @@ -84,7 +84,7 @@ public function boot() return new UserProvider; }); - Gate::before(function ($user, $ability) { + Gate::after(function ($user, $ability) { Permission::boot(); $isStatamicPermission = Permission::all()->first(fn ($permission) => $permission->value() === $ability); @@ -92,9 +92,7 @@ public function boot() if ($isStatamicPermission) { return optional(User::fromUser($user))->isSuper() ? true : null; } - }); - Gate::after(function ($user, $ability) { return optional(User::fromUser($user))->hasPermission($ability) === true ? true : null; }); From d9b956733463ad3aa34baa2aa6b74aedc62a5909 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 7 Mar 2025 14:47:03 +0000 Subject: [PATCH 12/27] Refactor super user / permission check --- src/Providers/AuthServiceProvider.php | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Providers/AuthServiceProvider.php b/src/Providers/AuthServiceProvider.php index a8561935246..6bfc64f90ff 100755 --- a/src/Providers/AuthServiceProvider.php +++ b/src/Providers/AuthServiceProvider.php @@ -87,13 +87,17 @@ public function boot() Gate::after(function ($user, $ability) { Permission::boot(); - $isStatamicPermission = Permission::all()->first(fn ($permission) => $permission->value() === $ability); + if (Permission::all()->map->value()->has($ability)) { + $user = User::fromUser($user); - if ($isStatamicPermission) { - return optional(User::fromUser($user))->isSuper() ? true : null; - } + if ($user->isSuper()) { + return true; + } - return optional(User::fromUser($user))->hasPermission($ability) === true ? true : null; + if ($user->hasPermission($ability)) { + return true; + } + } }); foreach ($this->policies as $key => $policy) { From 1327752c237bbbe5f32a49b1428d39335d7b2e13 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 7 Mar 2025 16:04:57 +0000 Subject: [PATCH 13/27] Re-work the logic a little to allow for wildcard permissions --- src/Providers/AuthServiceProvider.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Providers/AuthServiceProvider.php b/src/Providers/AuthServiceProvider.php index 6bfc64f90ff..16321b931e6 100755 --- a/src/Providers/AuthServiceProvider.php +++ b/src/Providers/AuthServiceProvider.php @@ -87,16 +87,14 @@ public function boot() Gate::after(function ($user, $ability) { Permission::boot(); - if (Permission::all()->map->value()->has($ability)) { - $user = User::fromUser($user); + $user = User::fromUser($user); - if ($user->isSuper()) { - return true; - } + if (Permission::all()->map->value()->has($ability) && $user->isSuper()) { + return true; + } - if ($user->hasPermission($ability)) { - return true; - } + if ($user->hasPermission($ability)) { + return true; } }); From ee1952329003c90a1ca5072db54d8fefffb88180 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 7 Mar 2025 16:05:29 +0000 Subject: [PATCH 14/27] Register permission in test --- tests/CP/Navigation/NavTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/CP/Navigation/NavTest.php b/tests/CP/Navigation/NavTest.php index 72b8ba48df7..749c34b4087 100644 --- a/tests/CP/Navigation/NavTest.php +++ b/tests/CP/Navigation/NavTest.php @@ -261,6 +261,8 @@ public function it_sets_parent_icon_on_children() #[Test] public function it_doesnt_build_children_that_the_user_is_not_authorized_to_see() { + Facades\Permission::register('view sith diaries'); + $this->setTestRoles(['sith' => ['view sith diaries']]); $this->actingAs(tap(User::make()->assignRole('sith'))->save()); From de15f9065d3388ef57e034271e24f35b90bcb8c2 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Tue, 11 Mar 2025 14:51:24 -0400 Subject: [PATCH 15/27] Test for booting once --- tests/Permissions/PermissionsTest.php | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/Permissions/PermissionsTest.php b/tests/Permissions/PermissionsTest.php index 914416508d3..bb9c85c0f28 100644 --- a/tests/Permissions/PermissionsTest.php +++ b/tests/Permissions/PermissionsTest.php @@ -2,6 +2,7 @@ namespace Tests\Permissions; +use Facades\Statamic\Auth\CorePermissions; use Illuminate\Support\Collection; use PHPUnit\Framework\Attributes\Test; use Statamic\Auth\Permissions; @@ -136,6 +137,25 @@ public function it_places_any_permissions_registered_early_without_extend_callba $this->assertEquals(['three', 'one', 'two'], $names); } + #[Test] + public function booting_is_only_done_once() + { + CorePermissions::shouldReceive('boot')->once(); + + $permissions = new Permissions; + + $callbackCount = 0; + $permissions->extend(function ($arg) use ($permissions, &$callbackCount) { + $this->assertEquals($permissions, $arg); + $callbackCount = true; + }); + + $permissions->boot(); + $permissions->boot(); + + $this->assertEquals(1, $callbackCount); + } + #[Test] public function it_makes_a_tree() { From f59a7e4cfde04ea211c93ecb0d81ae876b9e1b08 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Tue, 11 Mar 2025 15:19:24 -0400 Subject: [PATCH 16/27] nitpick --- src/Policies/AssetContainerPolicy.php | 5 +---- src/Policies/AssetPolicy.php | 5 +---- src/Policies/CollectionPolicy.php | 5 +---- src/Policies/EntryPolicy.php | 5 +---- src/Policies/FieldsetPolicy.php | 5 +---- src/Policies/FormPolicy.php | 5 +---- src/Policies/FormSubmissionPolicy.php | 5 +---- src/Policies/GlobalSetPolicy.php | 5 +---- src/Policies/NavPolicy.php | 5 +---- src/Policies/TaxonomyPolicy.php | 5 +---- src/Policies/TermPolicy.php | 5 +---- 11 files changed, 11 insertions(+), 44 deletions(-) diff --git a/src/Policies/AssetContainerPolicy.php b/src/Policies/AssetContainerPolicy.php index 06cca1bd0a1..2444b563daa 100644 --- a/src/Policies/AssetContainerPolicy.php +++ b/src/Policies/AssetContainerPolicy.php @@ -11,10 +11,7 @@ public function before($user, $ability) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure asset containers') - ) { + if ($user->isSuper() || $user->hasPermission('configure asset containers')) { return true; } } diff --git a/src/Policies/AssetPolicy.php b/src/Policies/AssetPolicy.php index 3e4de51d875..f15ca620b51 100644 --- a/src/Policies/AssetPolicy.php +++ b/src/Policies/AssetPolicy.php @@ -10,10 +10,7 @@ public function before($user) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure asset containers') - ) { + if ($user->isSuper() || $user->hasPermission('configure asset containers')) { return true; } } diff --git a/src/Policies/CollectionPolicy.php b/src/Policies/CollectionPolicy.php index dfd8aa1c2db..094096051e6 100644 --- a/src/Policies/CollectionPolicy.php +++ b/src/Policies/CollectionPolicy.php @@ -13,10 +13,7 @@ public function before($user) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure collections') - ) { + if ($user->isSuper() || $user->hasPermission('configure collections')) { return true; } } diff --git a/src/Policies/EntryPolicy.php b/src/Policies/EntryPolicy.php index 073b4afec19..c6cc1caaf71 100644 --- a/src/Policies/EntryPolicy.php +++ b/src/Policies/EntryPolicy.php @@ -12,10 +12,7 @@ public function before($user) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure collections') - ) { + if ($user->isSuper() || $user->hasPermission('configure collections')) { return true; } } diff --git a/src/Policies/FieldsetPolicy.php b/src/Policies/FieldsetPolicy.php index 4bf6ee807e8..ed9ecb1d7bb 100644 --- a/src/Policies/FieldsetPolicy.php +++ b/src/Policies/FieldsetPolicy.php @@ -10,10 +10,7 @@ public function before($user, $ability, $fieldset) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure fields') - ) { + if ($user->isSuper() || $user->hasPermission('configure fields')) { return true; } } diff --git a/src/Policies/FormPolicy.php b/src/Policies/FormPolicy.php index e75a6e6d86b..21348d97b41 100644 --- a/src/Policies/FormPolicy.php +++ b/src/Policies/FormPolicy.php @@ -11,10 +11,7 @@ public function before($user, $ability) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure forms') - ) { + if ($user->isSuper() || $user->hasPermission('configure forms')) { return true; } } diff --git a/src/Policies/FormSubmissionPolicy.php b/src/Policies/FormSubmissionPolicy.php index 2624b56a9de..123ce290c9c 100644 --- a/src/Policies/FormSubmissionPolicy.php +++ b/src/Policies/FormSubmissionPolicy.php @@ -10,10 +10,7 @@ public function before($user, $ability) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure forms') - ) { + if ($user->isSuper() || $user->hasPermission('configure forms')) { return true; } } diff --git a/src/Policies/GlobalSetPolicy.php b/src/Policies/GlobalSetPolicy.php index 2417dd553f4..d0710caf1a3 100644 --- a/src/Policies/GlobalSetPolicy.php +++ b/src/Policies/GlobalSetPolicy.php @@ -13,10 +13,7 @@ public function before($user) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure globals') - ) { + if ($user->isSuper() || $user->hasPermission('configure globals')) { return true; } } diff --git a/src/Policies/NavPolicy.php b/src/Policies/NavPolicy.php index 8f939ebadc4..02d55bdd82c 100644 --- a/src/Policies/NavPolicy.php +++ b/src/Policies/NavPolicy.php @@ -13,10 +13,7 @@ public function before($user) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure navs') - ) { + if ($user->isSuper() || $user->hasPermission('configure navs')) { return true; } } diff --git a/src/Policies/TaxonomyPolicy.php b/src/Policies/TaxonomyPolicy.php index db345ee857d..3161bfb7969 100644 --- a/src/Policies/TaxonomyPolicy.php +++ b/src/Policies/TaxonomyPolicy.php @@ -13,10 +13,7 @@ public function before($user) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure taxonomies') - ) { + if ($user->isSuper() || $user->hasPermission('configure taxonomies')) { return true; } } diff --git a/src/Policies/TermPolicy.php b/src/Policies/TermPolicy.php index f3fedceff9b..706a4f5e13d 100644 --- a/src/Policies/TermPolicy.php +++ b/src/Policies/TermPolicy.php @@ -12,10 +12,7 @@ public function before($user) { $user = User::fromUser($user); - if ( - $user->isSuper() || - $user->hasPermission('configure taxonomies') - ) { + if ($user->isSuper() || $user->hasPermission('configure taxonomies')) { return true; } } From 080311371978107ec4d98320933469e41b152175 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Wed, 12 Mar 2025 13:30:30 -0400 Subject: [PATCH 17/27] Add gate tests --- tests/Permissions/GateTest.php | 116 +++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 tests/Permissions/GateTest.php diff --git a/tests/Permissions/GateTest.php b/tests/Permissions/GateTest.php new file mode 100644 index 00000000000..e4635c728ff --- /dev/null +++ b/tests/Permissions/GateTest.php @@ -0,0 +1,116 @@ +delete(); + + User::all()->each->delete(); + + parent::tearDown(); + } + + #[Test] + #[DataProvider('gateProvider')] + public function gate_checks($userCallback, $permission, $expectsToBeAllowed) + { + // Add a Statamic permission. By adding a custom one it proves + // that it's not just "core" permissions that will work, but + // also any permission registered into Statamic. + Permission::extend(function () { + Permission::register('foo'); + }); + + Collection::make('blog')->save(); + + // Add a role that has the permission since permissions + // cannot be applied directly to users. + Role::make('test') + ->addPermission('foo') + ->addPermission('edit blog entries') + ->save(); + + // Add a gate, which is how someone would define + // something completely separate from Statamic. + Gate::define('bar', fn ($user) => $user->email === 'bar@domain.com'); + + $this->actingAs($userCallback()->save()); + + $this->assertEquals( + $expectsToBeAllowed, + Gate::allows($permission), + 'User should '.($expectsToBeAllowed ? '' : 'not ').'be allowed.' + ); + } + + public static function gateProvider() + { + return [ + 'statamic permission, super user' => [ + fn () => User::make()->makeSuper(), + 'foo', + true, + ], + 'statamic permission, user with permission' => [ + fn () => User::make()->assignRole('test'), + 'foo', + true, + ], + 'statamic permission, user without permission' => [ + fn () => User::make(), + 'foo', + false, + ], + 'statamic policy permission, super user' => [ + fn () => User::make()->makeSuper(), + 'edit blog entries', + true, + ], + 'statamic policy permission, user with permission' => [ + fn () => User::make()->assignRole('test'), + 'edit blog entries', + true, + ], + 'statamic policy permission, user without permission' => [ + fn () => User::make(), + 'edit blog entries', + false, + ], + 'non-statamic permission, super user' => [ + fn () => User::make()->makeSuper(), + 'bar', + false, + ], + 'non-statamic permission, user with permission' => [ + fn () => User::make()->email('bar@domain.com'), + 'bar', + true, + ], + 'non-statamic permission, user without permission' => [ + fn () => User::make()->email('baz@domain.com'), + 'bar', + false, + ], + ]; + } +} From 99d8acf7f1d3e7d80ae793a2706c1fbeb1482682 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Wed, 12 Mar 2025 16:34:58 -0400 Subject: [PATCH 18/27] Add a flattened method and use that to check if a given permission is a statamic one --- src/Auth/Permission.php | 44 +++++++++++++++++++++++++++ src/Auth/Permissions.php | 5 +++ src/Providers/AuthServiceProvider.php | 2 +- tests/Permissions/PermissionsTest.php | 28 +++++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/Auth/Permission.php b/src/Auth/Permission.php index 905ca852c4d..f2fe111019a 100644 --- a/src/Auth/Permission.php +++ b/src/Auth/Permission.php @@ -105,6 +105,50 @@ public function permissions() })->values(); } + public function flattened() + { + if (! $this->callback) { + return [ + $this, + ...$this->children()->map(function ($child) { + return (new self) + ->value($child->value()) + ->label($child->label()) + ->placeholder($this->placeholder) + ->placeholderLabel($this->placeholderLabel) + ->placeholderValue($this->placeholderValue) + ->children($child->children()->all()) + ->group($this->group()); + })->flatMap->flattened()->all(), + ]; + } + + $items = call_user_func($this->callback); + + return collect($items)->flatMap(function ($replacement) { + $replaced = (new self) + ->value($this->value) + ->label($this->label) + ->placeholder($this->placeholder) + ->placeholderLabel($replacement['label']) + ->placeholderValue($replacement['value']) + ->group($this->group()); + + $children = $this->children()->map(function ($child) use ($replacement) { + return (new self) + ->value($child->originalValue()) + ->label($child->originalLabel()) + ->placeholder($this->placeholder) + ->placeholderLabel($replacement['label']) + ->placeholderValue($replacement['value']) + ->children($child->children()->all()) + ->group($this->group()); + }); + + return [$replaced, ...$children->flatMap->flattened()->all()]; + })->values(); + } + public function children(?array $children = null) { return $this diff --git a/src/Auth/Permissions.php b/src/Auth/Permissions.php index c5920a17688..d677867ef4d 100644 --- a/src/Auth/Permissions.php +++ b/src/Auth/Permissions.php @@ -131,4 +131,9 @@ public function group($name, $label, $permissions = null) $this->pendingGroup = null; } + + public function flattened() + { + return collect($this->permissions)->flatMap->flattened(); + } } diff --git a/src/Providers/AuthServiceProvider.php b/src/Providers/AuthServiceProvider.php index 16321b931e6..0dcd3276c81 100755 --- a/src/Providers/AuthServiceProvider.php +++ b/src/Providers/AuthServiceProvider.php @@ -89,7 +89,7 @@ public function boot() $user = User::fromUser($user); - if (Permission::all()->map->value()->has($ability) && $user->isSuper()) { + if (Permission::flattened()->map->value()->contains($ability) && $user->isSuper()) { return true; } diff --git a/tests/Permissions/PermissionsTest.php b/tests/Permissions/PermissionsTest.php index bb9c85c0f28..847a98bcaa1 100644 --- a/tests/Permissions/PermissionsTest.php +++ b/tests/Permissions/PermissionsTest.php @@ -305,6 +305,34 @@ public function it_gets_all_permissions_in_a_flattened_structure() ])->sort()->values()->all(), $all->keys()->sort()->values()->all()); } + #[Test] + public function it_gets_all_permissions_with_placeholders_resolved_in_a_flat_array() + { + $this->setupComplicatedTest($permissions = new Permissions); + + $resolved = $permissions->flattened(); + + $this->assertEquals(collect([ + 'one', + 'child-one', + 'child-two', + + 'two', + 'child-three', + 'nested-child', + + 'three', + + 'four first', + 'replaced child first', + 'replaced nested child first', + + 'four second', + 'replaced child second', + 'replaced nested child second', + ])->all(), $resolved->map->value()->all()); + } + #[Test] public function existing_permissions_can_be_modified() { From ca39b674ca204f69c5557e550c86474f526ff62d Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Wed, 12 Mar 2025 16:57:59 -0400 Subject: [PATCH 19/27] fix test failures ... Since more stuff gets evaluated now (all the permissions are compiled) all the sites will be looped over. Many tests were setting up sites without explicit names. They don't really need them so here the site name will fall back to the handle. --- src/Sites/Site.php | 2 +- tests/Sites/SiteTest.php | 8 ++++++++ tests/Sites/SitesTest.php | 1 + 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Sites/Site.php b/src/Sites/Site.php index 9e81178e7b1..cc1f6f365aa 100644 --- a/src/Sites/Site.php +++ b/src/Sites/Site.php @@ -31,7 +31,7 @@ public function handle() public function name() { - return $this->config['name']; + return $this->config['name'] ?? $this->handle(); } public function locale() diff --git a/tests/Sites/SiteTest.php b/tests/Sites/SiteTest.php index a420833d49d..6244b306ca8 100644 --- a/tests/Sites/SiteTest.php +++ b/tests/Sites/SiteTest.php @@ -33,6 +33,14 @@ public function gets_name() $this->assertEquals('English', $site->name()); } + #[Test] + public function name_falls_back_to_handle() + { + $site = new Site('en', []); + + $this->assertEquals('en', $site->name()); + } + #[Test] public function gets_locale() { diff --git a/tests/Sites/SitesTest.php b/tests/Sites/SitesTest.php index 39897b12396..807f043885e 100644 --- a/tests/Sites/SitesTest.php +++ b/tests/Sites/SitesTest.php @@ -61,6 +61,7 @@ public function gets_authorized_sites() $this->actingAs(tap(User::make()->assignRole('test'))->save()); \Statamic\Facades\Site::shouldReceive('multiEnabled')->andReturnTrue(); + \Statamic\Facades\Site::shouldReceive('all')->andReturn(collect()); // CorePermissions calls this. It's irrelevant to this test. tap($this->sites->authorized(), function ($sites) { $this->assertInstanceOf(Collection::class, $sites); From 34d421cd8cadc33b36ea91d807d27d624bf21f33 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 13 Mar 2025 10:07:16 -0400 Subject: [PATCH 20/27] These tests aren't concerned with permissions so just remove the cans instead --- tests/CP/Navigation/ActiveNavItemTest.php | 48 +++++++---------------- 1 file changed, 15 insertions(+), 33 deletions(-) diff --git a/tests/CP/Navigation/ActiveNavItemTest.php b/tests/CP/Navigation/ActiveNavItemTest.php index 779da8e24bc..3da17b568a0 100644 --- a/tests/CP/Navigation/ActiveNavItemTest.php +++ b/tests/CP/Navigation/ActiveNavItemTest.php @@ -218,15 +218,12 @@ public function it_resolves_core_children_closure_and_can_check_when_parent_and_ #[Test] public function it_can_check_if_parent_extension_with_array_based_children_item_is_active() { - Facades\Permission::register('view seo reports'); - Facades\Permission::register('edit seo section defaults'); - Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') ->children([ - $nav->item('Reports')->url('/cp/seo-pro/reports')->can('view seo reports'), - $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults')->can('edit seo section defaults'), + $nav->item('Reports')->url('/cp/seo-pro/reports'), + $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults'), ]); }); @@ -246,15 +243,12 @@ public function it_can_check_if_parent_extension_with_array_based_children_item_ #[Test] public function it_can_check_when_parent_and_array_based_child_extension_items_are_active() { - Facades\Permission::register('view seo reports'); - Facades\Permission::register('edit seo section defaults'); - Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') ->children([ - $nav->item('Reports')->url('/cp/seo-pro/reports')->can('view seo reports'), - $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults')->can('edit seo section defaults'), + $nav->item('Reports')->url('/cp/seo-pro/reports'), + $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults'), ]); }); @@ -274,15 +268,12 @@ public function it_can_check_when_parent_and_array_based_child_extension_items_a #[Test] public function it_can_check_when_parent_and_array_based_descendant_of_child_extension_item_is_active() { - Facades\Permission::register('view seo reports'); - Facades\Permission::register('edit seo section defaults'); - Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') ->children([ - $nav->item('Reports')->url('/cp/seo-pro/reports')->can('view seo reports'), - $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults')->can('edit seo section defaults'), + $nav->item('Reports')->url('/cp/seo-pro/reports'), + $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults'), ]); }); @@ -307,9 +298,9 @@ public function it_builds_extension_children_closure_when_not_active() ->url('/cp/seo-pro') ->children(function () use ($nav) { return [ - $nav->item('Reports')->url('/cp/seo-pro/')->can('view seo reports'), - $nav->item('Site Defaults')->url('/cp/seo-pro/site-defaults')->can('edit seo site defaults'), - $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults')->can('edit seo section defaults'), + $nav->item('Reports')->url('/cp/seo-pro/'), + $nav->item('Site Defaults')->url('/cp/seo-pro/site-defaults'), + $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults'), ]; }); }); @@ -328,16 +319,13 @@ public function it_builds_extension_children_closure_when_not_active() #[Test] public function it_resolves_extension_children_closure_and_can_check_when_parent_item_is_active() { - Facades\Permission::register('view seo reports'); - Facades\Permission::register('edit seo section defaults'); - Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') ->children(function () use ($nav) { return [ - $nav->item('Reports')->url('/cp/seo-pro/reports')->can('view seo reports'), - $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults')->can('edit seo section defaults'), + $nav->item('Reports')->url('/cp/seo-pro/reports'), + $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults'), ]; }); }); @@ -358,16 +346,13 @@ public function it_resolves_extension_children_closure_and_can_check_when_parent #[Test] public function it_resolves_extension_children_closure_and_can_check_when_parent_and_child_item_are_active() { - Facades\Permission::register('view seo reports'); - Facades\Permission::register('edit seo section defaults'); - Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') ->children(function () use ($nav) { return [ - $nav->item('Reports')->url('/cp/seo-pro/reports')->can('view seo reports'), - $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults')->can('edit seo section defaults'), + $nav->item('Reports')->url('/cp/seo-pro/reports'), + $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults'), ]; }); }); @@ -388,16 +373,13 @@ public function it_resolves_extension_children_closure_and_can_check_when_parent #[Test] public function it_resolves_extension_children_closure_and_can_check_when_parent_and_descendant_of_child_item_is_active() { - Facades\Permission::register('view seo reports'); - Facades\Permission::register('edit seo section defaults'); - Facades\CP\Nav::extend(function ($nav) { $nav->tools('SEO Pro') ->url('/cp/seo-pro') ->children(function () use ($nav) { return [ - $nav->item('Reports')->url('/cp/seo-pro/reports')->can('view seo reports'), - $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults')->can('edit seo section defaults'), + $nav->item('Reports')->url('/cp/seo-pro/reports'), + $nav->item('Section Defaults')->url('/cp/seo-pro/section-defaults'), ]; }); }); From 705cc026a8d0da889b64b66f4bf64aa4ec05b8bf Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 13 Mar 2025 10:58:12 -0400 Subject: [PATCH 21/27] Test gate should not explicitly deny --- tests/Permissions/GateTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Permissions/GateTest.php b/tests/Permissions/GateTest.php index e4635c728ff..2e995cc11ba 100644 --- a/tests/Permissions/GateTest.php +++ b/tests/Permissions/GateTest.php @@ -52,7 +52,7 @@ public function gate_checks($userCallback, $permission, $expectsToBeAllowed) // Add a gate, which is how someone would define // something completely separate from Statamic. - Gate::define('bar', fn ($user) => $user->email === 'bar@domain.com'); + Gate::define('bar', fn ($user) => $user->email === 'bar@domain.com' ? true : null); $this->actingAs($userCallback()->save()); From fd7b7cb14433e866b11273c144fb6243aef1c276 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 13 Mar 2025 10:59:06 -0400 Subject: [PATCH 22/27] User clearer names --- tests/Permissions/GateTest.php | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/Permissions/GateTest.php b/tests/Permissions/GateTest.php index 2e995cc11ba..cfce86e0d95 100644 --- a/tests/Permissions/GateTest.php +++ b/tests/Permissions/GateTest.php @@ -38,7 +38,7 @@ public function gate_checks($userCallback, $permission, $expectsToBeAllowed) // that it's not just "core" permissions that will work, but // also any permission registered into Statamic. Permission::extend(function () { - Permission::register('foo'); + Permission::register('statamic'); }); Collection::make('blog')->save(); @@ -46,13 +46,13 @@ public function gate_checks($userCallback, $permission, $expectsToBeAllowed) // Add a role that has the permission since permissions // cannot be applied directly to users. Role::make('test') - ->addPermission('foo') + ->addPermission('statamic') ->addPermission('edit blog entries') ->save(); // Add a gate, which is how someone would define // something completely separate from Statamic. - Gate::define('bar', fn ($user) => $user->email === 'bar@domain.com' ? true : null); + Gate::define('gate', fn ($user) => $user->email === 'allowed@domain.com' ? true : null); $this->actingAs($userCallback()->save()); @@ -68,17 +68,17 @@ public static function gateProvider() return [ 'statamic permission, super user' => [ fn () => User::make()->makeSuper(), - 'foo', + 'statamic', true, ], 'statamic permission, user with permission' => [ fn () => User::make()->assignRole('test'), - 'foo', + 'statamic', true, ], 'statamic permission, user without permission' => [ fn () => User::make(), - 'foo', + 'statamic', false, ], 'statamic policy permission, super user' => [ @@ -98,17 +98,17 @@ public static function gateProvider() ], 'non-statamic permission, super user' => [ fn () => User::make()->makeSuper(), - 'bar', + 'gate', false, ], 'non-statamic permission, user with permission' => [ - fn () => User::make()->email('bar@domain.com'), - 'bar', + fn () => User::make()->email('allowed@domain.com'), + 'gate', true, ], 'non-statamic permission, user without permission' => [ - fn () => User::make()->email('baz@domain.com'), - 'bar', + fn () => User::make()->email('denied@domain.com'), + 'gate', false, ], ]; From 2de3e31be1a3a77bd77c3f39ed5642b4264c1f28 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 13 Mar 2025 11:03:10 -0400 Subject: [PATCH 23/27] Avoid even checking if user has permission if that ability is not a registered statamic permission --- src/Providers/AuthServiceProvider.php | 6 +++++- tests/Permissions/GateTest.php | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/Providers/AuthServiceProvider.php b/src/Providers/AuthServiceProvider.php index 0dcd3276c81..9ab8a2f8c66 100755 --- a/src/Providers/AuthServiceProvider.php +++ b/src/Providers/AuthServiceProvider.php @@ -87,9 +87,13 @@ public function boot() Gate::after(function ($user, $ability) { Permission::boot(); + if (! Permission::flattened()->map->value()->contains($ability)) { + return null; + } + $user = User::fromUser($user); - if (Permission::flattened()->map->value()->contains($ability) && $user->isSuper()) { + if ($user->isSuper()) { return true; } diff --git a/tests/Permissions/GateTest.php b/tests/Permissions/GateTest.php index cfce86e0d95..89a15263bc9 100644 --- a/tests/Permissions/GateTest.php +++ b/tests/Permissions/GateTest.php @@ -47,6 +47,7 @@ public function gate_checks($userCallback, $permission, $expectsToBeAllowed) // cannot be applied directly to users. Role::make('test') ->addPermission('statamic') + ->addPermission('gate') ->addPermission('edit blog entries') ->save(); @@ -111,6 +112,13 @@ public static function gateProvider() 'gate', false, ], + 'non-statamic permission, user has permission in role' => [ + fn () => User::make()->assignRole('test'), + 'gate', + // Even though the role has the permission, we should not be + // checking it if it's not registered as a Statamic permission. + false, + ], ]; } } From c04820e4af1eb56689713e9fde5191229235fb13 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 13 Mar 2025 11:04:17 -0400 Subject: [PATCH 24/27] Bring back existing can() usage, but with an actual gate policy, since we want code coverage for that. --- tests/CP/Navigation/NavTest.php | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/CP/Navigation/NavTest.php b/tests/CP/Navigation/NavTest.php index 749c34b4087..dc03409062a 100644 --- a/tests/CP/Navigation/NavTest.php +++ b/tests/CP/Navigation/NavTest.php @@ -2,6 +2,7 @@ namespace Tests\CP\Navigation; +use Illuminate\Support\Facades\Gate; use Illuminate\Support\Facades\Route; use PHPUnit\Framework\Attributes\Test; use Statamic\CP\Navigation\NavItem; @@ -71,16 +72,16 @@ public function it_can_more_explicitly_create_a_nav_item() #[Test] public function it_can_create_a_nav_item_with_a_more_custom_config() { - $this->actingAs(tap(User::make()->makeSuper())->save()); + Gate::policy(DroidsClass::class, DroidsPolicy::class); - Facades\Permission::register('view droids'); + $this->actingAs(tap(User::make()->makeSuper())->save()); Nav::droids('C-3PO') ->id('some::custom::id') ->active('threepio*') ->url('/human-cyborg-relations') ->view('cp.nav.importer') - ->can('view droids') + ->can('index', DroidsClass::class) ->attributes(['target' => '_blank', 'class' => 'red']); $item = $this->build()->get('Droids')->first(); @@ -91,7 +92,8 @@ public function it_can_create_a_nav_item_with_a_more_custom_config() $this->assertEquals('http://localhost/human-cyborg-relations', $item->url()); $this->assertEquals('cp.nav.importer', $item->view()); $this->assertEquals('threepio*', $item->active()); - $this->assertEquals('view droids', $item->authorization()->ability); + $this->assertEquals('index', $item->authorization()->ability); + $this->assertEquals(DroidsClass::class, $item->authorization()->arguments); $this->assertEquals(' target="_blank" class="red"', $item->attributes()); } @@ -723,3 +725,15 @@ protected function build() return Nav::build()->pluck('items', 'display'); } } + +class DroidsClass +{ +} + +class DroidsPolicy +{ + public function index() + { + return true; + } +} From 3e87ca535464be43c01eb662669398e0797a0e16 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 13 Mar 2025 11:05:11 -0400 Subject: [PATCH 25/27] Don't just register the successful one, and add a note. --- tests/CP/Navigation/NavTest.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/CP/Navigation/NavTest.php b/tests/CP/Navigation/NavTest.php index dc03409062a..4f5fd952300 100644 --- a/tests/CP/Navigation/NavTest.php +++ b/tests/CP/Navigation/NavTest.php @@ -263,7 +263,12 @@ public function it_sets_parent_icon_on_children() #[Test] public function it_doesnt_build_children_that_the_user_is_not_authorized_to_see() { + // Assume we're dealing with Statamic permissions. Technically nav items + // could use arbitrary ability strings that correspond to Gate::define(). + Facades\Permission::register('view jedi diaries'); + Facades\Permission::register('view jedi logs'); Facades\Permission::register('view sith diaries'); + Facades\Permission::register('view sith logs'); $this->setTestRoles(['sith' => ['view sith diaries']]); $this->actingAs(tap(User::make()->assignRole('sith'))->save()); From 691a0a8450106071af29b7f6ee88155dbd0ab247 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 13 Mar 2025 11:10:15 -0400 Subject: [PATCH 26/27] Make chainable --- src/Auth/Permissions.php | 4 +++- src/Providers/AuthServiceProvider.php | 4 +--- tests/Permissions/PermissionsTest.php | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Auth/Permissions.php b/src/Auth/Permissions.php index d677867ef4d..c3885d0e308 100644 --- a/src/Auth/Permissions.php +++ b/src/Auth/Permissions.php @@ -15,7 +15,7 @@ class Permissions public function boot() { if ($this->booted) { - return; + return $this; } $early = $this->permissions; @@ -29,6 +29,8 @@ public function boot() $this->permissions = array_merge($this->permissions, $early); $this->booted = true; + + return $this; } public function extend($callback) diff --git a/src/Providers/AuthServiceProvider.php b/src/Providers/AuthServiceProvider.php index 9ab8a2f8c66..787418147c8 100755 --- a/src/Providers/AuthServiceProvider.php +++ b/src/Providers/AuthServiceProvider.php @@ -85,9 +85,7 @@ public function boot() }); Gate::after(function ($user, $ability) { - Permission::boot(); - - if (! Permission::flattened()->map->value()->contains($ability)) { + if (! Permission::boot()->flattened()->map->value()->contains($ability)) { return null; } diff --git a/tests/Permissions/PermissionsTest.php b/tests/Permissions/PermissionsTest.php index 847a98bcaa1..e396e78b2f3 100644 --- a/tests/Permissions/PermissionsTest.php +++ b/tests/Permissions/PermissionsTest.php @@ -150,9 +150,9 @@ public function booting_is_only_done_once() $callbackCount = true; }); - $permissions->boot(); - $permissions->boot(); + $returned = $permissions->boot()->boot()->boot()->boot(); + $this->assertSame($permissions, $returned); $this->assertEquals(1, $callbackCount); } From 91162f8eb96867e911fdf52a4cc6e3d75b80539d Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Thu, 13 Mar 2025 11:12:46 -0400 Subject: [PATCH 27/27] Explain --- src/Providers/AuthServiceProvider.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Providers/AuthServiceProvider.php b/src/Providers/AuthServiceProvider.php index 787418147c8..dd7c5c9d50b 100755 --- a/src/Providers/AuthServiceProvider.php +++ b/src/Providers/AuthServiceProvider.php @@ -85,6 +85,7 @@ public function boot() }); Gate::after(function ($user, $ability) { + // If the ability isn't a Statamic permission, we don't want to get involved. 🙈 if (! Permission::boot()->flattened()->map->value()->contains($ability)) { return null; }