From 70b427ed386cc2ef475753f9177304ce4c8f8a61 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Wed, 17 Apr 2024 18:26:12 +0100 Subject: [PATCH 01/11] Return a user's roles via groups from `User@roles`. Let's see what breaks. --- src/Auth/File/User.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Auth/File/User.php b/src/Auth/File/User.php index bd84104d49..9ab0b440f2 100644 --- a/src/Auth/File/User.php +++ b/src/Auth/File/User.php @@ -163,7 +163,10 @@ protected function getRoles() return collect($this->get('roles', [])) ->map(function ($role) { return Facades\Role::find($role); - })->filter()->keyBy->handle(); + }) + ->filter() + ->merge($this->groups()->map->roles()->flatten()) + ->keyBy->handle(); } public function assignRole($role) From 562c226ca08e40776837ba096a888a36fc324b27 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Wed, 17 Apr 2024 18:33:04 +0100 Subject: [PATCH 02/11] filter out items after merging --- src/Auth/File/User.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Auth/File/User.php b/src/Auth/File/User.php index 9ab0b440f2..df064db396 100644 --- a/src/Auth/File/User.php +++ b/src/Auth/File/User.php @@ -164,8 +164,8 @@ protected function getRoles() ->map(function ($role) { return Facades\Role::find($role); }) - ->filter() ->merge($this->groups()->map->roles()->flatten()) + ->filter() ->keyBy->handle(); } From 090e7e8846bf9541fc1acc98058de417431b6a25 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Wed, 17 Apr 2024 18:37:44 +0100 Subject: [PATCH 03/11] actually, just merge stuff in the `hasRole` method, keep things as they are in the "main" `getRoles` method --- src/Auth/File/User.php | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Auth/File/User.php b/src/Auth/File/User.php index df064db396..612154d6f7 100644 --- a/src/Auth/File/User.php +++ b/src/Auth/File/User.php @@ -163,10 +163,7 @@ protected function getRoles() return collect($this->get('roles', [])) ->map(function ($role) { return Facades\Role::find($role); - }) - ->merge($this->groups()->map->roles()->flatten()) - ->filter() - ->keyBy->handle(); + })->filter()->keyBy->handle(); } public function assignRole($role) @@ -200,7 +197,9 @@ public function hasRole($role) { $role = $role instanceof RoleContract ? $role->handle() : $role; - return $this->roles()->has($role); + return $this->roles() + ->merge($this->groups()->map->roles()->flatten()->filter()->keyBy->handle()) + ->has($role); } public function addToGroup($group) From 018d29e8f1050afe5523d063c5f32ec64d5c76c9 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Wed, 17 Apr 2024 18:39:25 +0100 Subject: [PATCH 04/11] fix test --- tests/Auth/FileUserTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Auth/FileUserTest.php b/tests/Auth/FileUserTest.php index d26ab67ee1..ba6769c590 100644 --- a/tests/Auth/FileUserTest.php +++ b/tests/Auth/FileUserTest.php @@ -97,7 +97,7 @@ public function it_gets_permissions_from_a_cache() $userGroup->shouldReceive('id')->andReturn('usergroup'); $userGroup->shouldReceive('handle')->andReturn('usergroup'); - $userGroup->shouldReceive('roles')->once()->andReturn(collect([$userGroupRole])); + $userGroup->shouldReceive('roles')->once()->andReturn(collect([$userGroupRole]))->times(3); Role::shouldReceive('find')->with('direct')->andReturn($directRole); Role::shouldReceive('all')->andReturn(collect([$directRole])); // the stache calls this when getting a user. unrelated to test. From 94f27bef74a23cde1a5aa3a4fc3c82201804c8dc Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Wed, 17 Apr 2024 18:40:09 +0100 Subject: [PATCH 05/11] copy change to eloquent user --- src/Auth/Eloquent/User.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/Auth/Eloquent/User.php b/src/Auth/Eloquent/User.php index 830ba65d39..cd3e08dcb6 100644 --- a/src/Auth/Eloquent/User.php +++ b/src/Auth/Eloquent/User.php @@ -2,6 +2,7 @@ namespace Statamic\Auth\Eloquent; +use Statamic\Contracts\Auth\Role as RoleContract; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Hash; @@ -145,9 +146,11 @@ public function removeRole($role) public function hasRole($role) { - return $this->roles()->has( - is_string($role) ? $role : $role->handle() - ); + $role = $role instanceof RoleContract ? $role->handle() : $role; + + return $this->roles() + ->merge($this->groups()->map->roles()->flatten()->filter()->keyBy->handle()) + ->has($role); } public function groups($groups = null) From b7d117aeb62a2c706f46348156a71ff0010ac65d Mon Sep 17 00:00:00 2001 From: duncanmcclean Date: Wed, 17 Apr 2024 17:41:50 +0000 Subject: [PATCH 06/11] Fix styling --- src/Auth/Eloquent/User.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Auth/Eloquent/User.php b/src/Auth/Eloquent/User.php index cd3e08dcb6..3b71db36d7 100644 --- a/src/Auth/Eloquent/User.php +++ b/src/Auth/Eloquent/User.php @@ -2,11 +2,11 @@ namespace Statamic\Auth\Eloquent; -use Statamic\Contracts\Auth\Role as RoleContract; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Hash; use Statamic\Auth\User as BaseUser; +use Statamic\Contracts\Auth\Role as RoleContract; use Statamic\Data\ContainsSupplementalData; use Statamic\Facades\Role; use Statamic\Facades\UserGroup; From 00b840fd75e604b62cad3164e11b265dc6caaaa9 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Wed, 17 Apr 2024 18:46:09 +0100 Subject: [PATCH 07/11] get rid of it on the eloquent user too --- src/Auth/Eloquent/User.php | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/Auth/Eloquent/User.php b/src/Auth/Eloquent/User.php index cd3e08dcb6..358aeb711d 100644 --- a/src/Auth/Eloquent/User.php +++ b/src/Auth/Eloquent/User.php @@ -144,15 +144,6 @@ public function removeRole($role) return $this; } - public function hasRole($role) - { - $role = $role instanceof RoleContract ? $role->handle() : $role; - - return $this->roles() - ->merge($this->groups()->map->roles()->flatten()->filter()->keyBy->handle()) - ->has($role); - } - public function groups($groups = null) { return is_null($groups) From b65882ca5b49ed8eeb1b83b24f8fa731ae6cfbfa Mon Sep 17 00:00:00 2001 From: duncanmcclean Date: Wed, 17 Apr 2024 17:47:58 +0000 Subject: [PATCH 08/11] Fix styling --- src/Auth/Eloquent/User.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Auth/Eloquent/User.php b/src/Auth/Eloquent/User.php index 6c70387b86..d5a94c624d 100644 --- a/src/Auth/Eloquent/User.php +++ b/src/Auth/Eloquent/User.php @@ -6,7 +6,6 @@ use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Hash; use Statamic\Auth\User as BaseUser; -use Statamic\Contracts\Auth\Role as RoleContract; use Statamic\Data\ContainsSupplementalData; use Statamic\Facades\Role; use Statamic\Facades\UserGroup; From fda2da3d1a052229dc71f2f8ac4006ebeb812dc3 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Thu, 18 Apr 2024 11:21:43 +0100 Subject: [PATCH 09/11] flip flop back to `hasRole` not doing any merging --- src/Auth/Eloquent/User.php | 8 ++++++++ src/Auth/File/User.php | 4 +--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Auth/Eloquent/User.php b/src/Auth/Eloquent/User.php index d5a94c624d..d4fdf7e0d4 100644 --- a/src/Auth/Eloquent/User.php +++ b/src/Auth/Eloquent/User.php @@ -6,6 +6,7 @@ use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Hash; use Statamic\Auth\User as BaseUser; +use Statamic\Contracts\Auth\Role as RoleContract; use Statamic\Data\ContainsSupplementalData; use Statamic\Facades\Role; use Statamic\Facades\UserGroup; @@ -150,6 +151,13 @@ public function groups($groups = null) : $this->setGroups($groups); } + public function hasRole($role) + { + $role = $role instanceof RoleContract ? $role->handle() : $role; + + return $this->roles()->has($role); + } + protected function getGroups() { return $this->groups = $this->groups diff --git a/src/Auth/File/User.php b/src/Auth/File/User.php index 612154d6f7..bd84104d49 100644 --- a/src/Auth/File/User.php +++ b/src/Auth/File/User.php @@ -197,9 +197,7 @@ public function hasRole($role) { $role = $role instanceof RoleContract ? $role->handle() : $role; - return $this->roles() - ->merge($this->groups()->map->roles()->flatten()->filter()->keyBy->handle()) - ->has($role); + return $this->roles()->has($role); } public function addToGroup($group) From 18f5ea97d9ed745f2ec33626d4f372af38b0d55d Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Thu, 18 Apr 2024 12:03:38 +0100 Subject: [PATCH 10/11] Split up merged roles and explicit roles into different methods --- src/Auth/Eloquent/User.php | 27 +++++++++---------- src/Auth/File/User.php | 24 +++++++---------- src/Auth/User.php | 8 ++++++ src/Console/Commands/ImportUsers.php | 2 +- src/Contracts/Auth/User.php | 6 ++++- .../Controllers/CP/Users/UsersController.php | 4 +-- src/Http/Controllers/UserController.php | 2 +- tests/Preferences/PrecedenceTest.php | 2 +- 8 files changed, 41 insertions(+), 34 deletions(-) diff --git a/src/Auth/Eloquent/User.php b/src/Auth/Eloquent/User.php index d4fdf7e0d4..7f5a704447 100644 --- a/src/Auth/Eloquent/User.php +++ b/src/Auth/Eloquent/User.php @@ -4,6 +4,7 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Carbon; +use Illuminate\Support\Collection; use Illuminate\Support\Facades\Hash; use Statamic\Auth\User as BaseUser; use Statamic\Contracts\Auth\Role as RoleContract; @@ -85,30 +86,28 @@ public function status() // TODO } - public function roles($roles = null) + public function roles(): Collection { - return is_null($roles) - ? $this->getRoles() - : $this->setRoles($roles); + return $this->explicitRoles() + ->merge($this->groups()->flatMap->roles()->keyBy->handle()); } - protected function getRoles() + public function explicitRoles($roles = null) { + if (func_num_args() === 1) { + $this->roles = collect(); + + $this->assignRole($roles); + + return $this; + } + return $this->roles = $this->roles ?? (new Roles($this))->all()->map(function ($row) { return Role::find($row->role_id); })->keyBy->handle(); } - protected function setRoles($roles) - { - $this->roles = collect(); - - $this->assignRole($roles); - - return $this; - } - protected function saveRoles() { $roles = $this->roles()->map->id(); diff --git a/src/Auth/File/User.php b/src/Auth/File/User.php index bd84104d49..8935121f33 100644 --- a/src/Auth/File/User.php +++ b/src/Auth/File/User.php @@ -3,10 +3,10 @@ namespace Statamic\Auth\File; use Carbon\Carbon; +use Illuminate\Support\Collection; use Illuminate\Support\Facades\Hash; use Statamic\Auth\PermissionCache; use Statamic\Auth\User as BaseUser; -use Statamic\Contracts\Auth\Role as RoleContract; use Statamic\Contracts\Auth\UserGroup as UserGroupContract; use Statamic\Data\ContainsData; use Statamic\Data\Data; @@ -151,16 +151,19 @@ public function getRememberTokenName() return 'remember_token'; } - public function roles($roles = null) + public function roles(): Collection { - return is_null($roles) - ? $this->getRoles() - : $this->set('roles', $roles); + return $this->explicitRoles() + ->merge($this->groups()->flatMap->roles()->keyBy->handle()); } - protected function getRoles() + public function explicitRoles($roles = null) { - return collect($this->get('roles', [])) + if (func_num_args() === 1) { + return $this->set('roles', $roles); + } + + return collect($this->get('roles')) ->map(function ($role) { return Facades\Role::find($role); })->filter()->keyBy->handle(); @@ -193,13 +196,6 @@ public function removeRole($role) return $this; } - public function hasRole($role) - { - $role = $role instanceof RoleContract ? $role->handle() : $role; - - return $this->roles()->has($role); - } - public function addToGroup($group) { $groups = collect(Arr::wrap($group))->map(function ($group) { diff --git a/src/Auth/User.php b/src/Auth/User.php index 9d5c4c29ff..c7feecc49e 100644 --- a/src/Auth/User.php +++ b/src/Auth/User.php @@ -13,6 +13,7 @@ use Illuminate\Notifications\Notifiable; use Illuminate\Support\Facades\Password; use Statamic\Auth\Passwords\PasswordReset; +use Statamic\Contracts\Auth\Role as RoleContract; use Statamic\Contracts\Auth\User as UserContract; use Statamic\Contracts\Data\Augmentable; use Statamic\Contracts\Data\Augmented; @@ -147,6 +148,13 @@ public function getAuthPasswordName() return 'password'; } + public function hasRole($role) + { + $role = $role instanceof RoleContract ? $role->handle() : $role; + + return $this->roles()->has($role); + } + /** * Get or set the blueprint. * diff --git a/src/Console/Commands/ImportUsers.php b/src/Console/Commands/ImportUsers.php index 854ab786f4..bd3b33b8d5 100644 --- a/src/Console/Commands/ImportUsers.php +++ b/src/Console/Commands/ImportUsers.php @@ -100,7 +100,7 @@ private function importUsers() } if (count($data->get('roles', [])) > 0) { - $eloquentUser->roles($data->get('roles')); + $eloquentUser->explicitRoles($data->get('roles')); } $eloquentUser->saveToDatabase(); diff --git a/src/Contracts/Auth/User.php b/src/Contracts/Auth/User.php index d5a644a99e..9bce1848f3 100644 --- a/src/Contracts/Auth/User.php +++ b/src/Contracts/Auth/User.php @@ -2,6 +2,8 @@ namespace Statamic\Contracts\Auth; +use Illuminate\Support\Collection; + interface User { /** @@ -20,7 +22,9 @@ public function email($email = null); */ public function password($password = null); - public function roles(); + public function roles(): Collection; + + public function explicitRoles($roles = null); public function assignRole($role); diff --git a/src/Http/Controllers/CP/Users/UsersController.php b/src/Http/Controllers/CP/Users/UsersController.php index 35a195e793..792fd05f68 100644 --- a/src/Http/Controllers/CP/Users/UsersController.php +++ b/src/Http/Controllers/CP/Users/UsersController.php @@ -179,7 +179,7 @@ public function store(Request $request) ->data($values); if ($request->roles && User::current()->can('assign roles')) { - $user->roles($request->roles); + $user->explicitRoles($request->roles); } if ($request->groups && User::current()->can('assign user groups')) { @@ -295,7 +295,7 @@ public function update(Request $request, $user) $user->email($request->email); if (User::current()->can('assign roles')) { - $user->roles($request->roles); + $user->explicitRoles($request->roles); } if (User::current()->can('assign user groups')) { diff --git a/src/Http/Controllers/UserController.php b/src/Http/Controllers/UserController.php index 92c6a5d421..db58c9f308 100644 --- a/src/Http/Controllers/UserController.php +++ b/src/Http/Controllers/UserController.php @@ -84,7 +84,7 @@ public function register(Request $request) ->data($values); if ($roles = config('statamic.users.new_user_roles')) { - $user->roles($roles); + $user->explicitRoles($roles); } if ($groups = config('statamic.users.new_user_groups')) { diff --git a/tests/Preferences/PrecedenceTest.php b/tests/Preferences/PrecedenceTest.php index 1526fdc220..ccab589973 100644 --- a/tests/Preferences/PrecedenceTest.php +++ b/tests/Preferences/PrecedenceTest.php @@ -92,7 +92,7 @@ public function it_gives_precedence_to_role_order_assigned_on_user() 'pleb' => Role::make()->permissions('super')->preferences(['alpha' => 'bar', 'charlie' => 'charlie']), ]); - $this->actingAs(User::make()->roles(['author', 'pleb'])); + $this->actingAs(User::make()->explicitRoles(['author', 'pleb'])); $expected = [ 'alpha' => 'foo', // This should be `foo`, because the `author` role is set first From 574fdfde1bae4ad50a0aca9a6fab277006a43102 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Thu, 18 Apr 2024 12:12:19 +0100 Subject: [PATCH 11/11] tweak assertions --- tests/Auth/FileUserTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Auth/FileUserTest.php b/tests/Auth/FileUserTest.php index ba6769c590..15c8dc1b3c 100644 --- a/tests/Auth/FileUserTest.php +++ b/tests/Auth/FileUserTest.php @@ -90,14 +90,14 @@ public function it_gets_permissions_from_a_cache() $userGroupRole->shouldReceive('id')->andReturn('usergrouprole'); $userGroupRole->shouldReceive('handle')->andReturn('usergrouprole'); - $userGroupRole->shouldReceive('permissions')->once()->andReturn(collect([ + $userGroupRole->shouldReceive('permissions')->twice()->andReturn(collect([ 'permission one through user group', 'permission two through user group', ])); $userGroup->shouldReceive('id')->andReturn('usergroup'); $userGroup->shouldReceive('handle')->andReturn('usergroup'); - $userGroup->shouldReceive('roles')->once()->andReturn(collect([$userGroupRole]))->times(3); + $userGroup->shouldReceive('roles')->once()->andReturn(collect([$userGroupRole]))->times(4); Role::shouldReceive('find')->with('direct')->andReturn($directRole); Role::shouldReceive('all')->andReturn(collect([$directRole])); // the stache calls this when getting a user. unrelated to test.