From 50821924a361f2ba65ee8d53983b8177c6cab162 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Tue, 17 Dec 2024 12:38:42 +0000 Subject: [PATCH 1/5] Prevent duplicate roles & groups --- src/Auth/File/User.php | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/Auth/File/User.php b/src/Auth/File/User.php index 8935121f33..56a0887cee 100644 --- a/src/Auth/File/User.php +++ b/src/Auth/File/User.php @@ -171,11 +171,13 @@ public function explicitRoles($roles = null) public function assignRole($role) { - $roles = collect(Arr::wrap($role))->map(function ($role) { - return is_string($role) ? $role : $role->handle(); - })->all(); + $roles = collect(Arr::wrap($role)) + ->map(fn ($role) => is_string($role) ? $role : $role->handle()) + ->merge($this->get('roles', [])) + ->unique() + ->all(); - $this->set('roles', array_merge($this->get('roles', []), $roles)); + $this->set('roles', $roles); return $this; } @@ -198,11 +200,13 @@ public function removeRole($role) public function addToGroup($group) { - $groups = collect(Arr::wrap($group))->map(function ($group) { - return is_string($group) ? $group : $group->handle(); - })->all(); + $groups = collect(Arr::wrap($group)) + ->map(fn ($group) => is_string($group) ? $group : $group->handle()) + ->merge($this->get('groups', [])) + ->unique() + ->all(); - $this->set('groups', array_merge($this->get('groups', []), $groups)); + $this->set('groups', $groups); return $this; } From 794d7c077f4054eaa82b4bb562a395beb664c941 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Tue, 17 Dec 2024 18:06:09 +0000 Subject: [PATCH 2/5] Collect existing roles/groups before merging the new ones. --- src/Auth/File/User.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Auth/File/User.php b/src/Auth/File/User.php index 56a0887cee..2335ea696e 100644 --- a/src/Auth/File/User.php +++ b/src/Auth/File/User.php @@ -171,9 +171,9 @@ public function explicitRoles($roles = null) public function assignRole($role) { - $roles = collect(Arr::wrap($role)) + $roles = collect($this->get('roles', [])) + ->merge(Arr::wrap($role)) ->map(fn ($role) => is_string($role) ? $role : $role->handle()) - ->merge($this->get('roles', [])) ->unique() ->all(); @@ -200,9 +200,9 @@ public function removeRole($role) public function addToGroup($group) { - $groups = collect(Arr::wrap($group)) + $groups = collect($this->get('groups', [])) + ->merge(Arr::wrap($group)) ->map(fn ($group) => is_string($group) ? $group : $group->handle()) - ->merge($this->get('groups', [])) ->unique() ->all(); From 62549f96e5722b6fac3c2064fd2b778ecafcacda Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 20 Dec 2024 10:52:44 +0000 Subject: [PATCH 3/5] Call `->values()`. We don't want keys. --- src/Auth/File/User.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Auth/File/User.php b/src/Auth/File/User.php index 2335ea696e..4da8ee696e 100644 --- a/src/Auth/File/User.php +++ b/src/Auth/File/User.php @@ -175,6 +175,7 @@ public function assignRole($role) ->merge(Arr::wrap($role)) ->map(fn ($role) => is_string($role) ? $role : $role->handle()) ->unique() + ->values() ->all(); $this->set('roles', $roles); @@ -204,6 +205,7 @@ public function addToGroup($group) ->merge(Arr::wrap($group)) ->map(fn ($group) => is_string($group) ? $group : $group->handle()) ->unique() + ->values() ->all(); $this->set('groups', $groups); From 4e1704320b712ee622d9b82a53a5ed484bf648bf Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 20 Dec 2024 11:04:24 +0000 Subject: [PATCH 4/5] Add tests --- tests/Auth/PermissibleContractTests.php | 70 +++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/Auth/PermissibleContractTests.php b/tests/Auth/PermissibleContractTests.php index 816036a03f..12b97a46bd 100644 --- a/tests/Auth/PermissibleContractTests.php +++ b/tests/Auth/PermissibleContractTests.php @@ -81,6 +81,51 @@ public function handle(?string $handle = null) $this->assertEquals($user, $return); } + #[Test] + public function it_prevents_duplicate_roles_being_assigned() + { + // Prevent the anonymous role classes throwing errors when getting serialized + // during event handling unrelated to this test. + Event::fake(); + + $roleA = new class extends Role + { + public function handle(?string $handle = null) + { + return 'a'; + } + }; + $roleB = new class extends Role + { + public function handle(?string $handle = null) + { + return 'b'; + } + }; + $roleC = new class extends Role + { + public function handle(?string $handle = null) + { + return 'c'; + } + }; + + RoleAPI::shouldReceive('find')->with('a')->andReturn($roleA); + RoleAPI::shouldReceive('find')->with('b')->andReturn($roleB); + RoleAPI::shouldReceive('find')->with('c')->andReturn($roleC); + RoleAPI::shouldReceive('all')->andReturn(collect([$roleA, $roleB])); // the stache calls this when getting a user. unrelated to test. + + $user = $this->createPermissible(); + $user->assignRole('a'); + $user->save(); + + $this->assertEquals(['a'], $user->get('roles')); + + $user->assignRole(['a', 'b', 'c']); + + $this->assertEquals(['a', 'b', 'c'], $user->get('roles')); + } + #[Test] public function it_removes_a_role_assignment() { @@ -344,6 +389,31 @@ public function it_adds_to_groups() $this->assertTrue($user->isInGroup($groupC)); } + #[Test] + public function it_prevents_adding_duplicate_groups_to_a_user() + { + $groupA = (new UserGroup)->handle('a'); + $groupB = (new UserGroup)->handle('b'); + $groupC = (new UserGroup)->handle('c'); + $user = $this->createPermissible(); + + $this->assertFalse($user->isInGroup($groupA)); + $this->assertFalse($user->isInGroup($groupB)); + $this->assertFalse($user->isInGroup($groupC)); + + UserGroupAPI::shouldReceive('find')->with('a')->andReturn($groupA); + UserGroupAPI::shouldReceive('find')->with('b')->andReturn($groupB); + UserGroupAPI::shouldReceive('find')->with('c')->andReturn($groupC); + + $user->addToGroup('a'); + + $this->assertEquals(['a'], $user->get('groups')); + + $user->addToGroup(['a', 'b', 'c']); + + $this->assertEquals(['a', 'b', 'c'], $user->get('groups')); + } + #[Test] public function it_sets_all_the_groups() { From 9a9dc948f5bf32813fad753491f9fcac5ea1ef80 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Fri, 20 Dec 2024 11:07:25 +0000 Subject: [PATCH 5/5] Move tests to `FileUserTest` as they're specific to the file implementation --- tests/Auth/FileUserTest.php | 45 ++++++++++++++++ tests/Auth/PermissibleContractTests.php | 70 ------------------------- 2 files changed, 45 insertions(+), 70 deletions(-) diff --git a/tests/Auth/FileUserTest.php b/tests/Auth/FileUserTest.php index 6332c16446..6ef2f75244 100644 --- a/tests/Auth/FileUserTest.php +++ b/tests/Auth/FileUserTest.php @@ -9,7 +9,9 @@ use Statamic\Contracts\Auth\Role as RoleContract; use Statamic\Contracts\Auth\UserGroup as UserGroupContract; use Statamic\Facades\Role; +use Statamic\Facades\Role as RoleAPI; use Statamic\Facades\UserGroup; +use Statamic\Facades\UserGroup as UserGroupAPI; use Statamic\Support\Arr; use Tests\PreventSavingStacheItemsToDisk; use Tests\TestCase; @@ -123,4 +125,47 @@ public function it_gets_permissions_from_a_cache() // Doing it a second time should give the same result but without multiple calls. $this->assertEquals($expectedPermissions, $user->permissions()->all()); } + + #[Test] + public function it_prevents_saving_duplicate_roles() + { + $roleA = (new \Statamic\Auth\File\Role)->handle('a'); + $roleB = (new \Statamic\Auth\File\Role)->handle('b'); + $roleC = (new \Statamic\Auth\File\Role)->handle('c'); + + RoleAPI::shouldReceive('find')->with('a')->andReturn($roleA); + RoleAPI::shouldReceive('find')->with('b')->andReturn($roleB); + RoleAPI::shouldReceive('find')->with('c')->andReturn($roleC); + RoleAPI::shouldReceive('all')->andReturn(collect([$roleA, $roleB])); // the stache calls this when getting a user. unrelated to test. + + $user = $this->createPermissible(); + $user->assignRole('a'); + + $this->assertEquals(['a'], $user->get('roles')); + + $user->assignRole(['a', 'b', 'c']); + + $this->assertEquals(['a', 'b', 'c'], $user->get('roles')); + } + + #[Test] + public function it_prevents_saving_duplicate_groups() + { + $groupA = (new \Statamic\Auth\File\UserGroup)->handle('a'); + $groupB = (new \Statamic\Auth\File\UserGroup)->handle('b'); + $groupC = (new \Statamic\Auth\File\UserGroup)->handle('c'); + + UserGroupAPI::shouldReceive('find')->with('a')->andReturn($groupA); + UserGroupAPI::shouldReceive('find')->with('b')->andReturn($groupB); + UserGroupAPI::shouldReceive('find')->with('c')->andReturn($groupC); + + $user = $this->createPermissible(); + $user->addToGroup('a'); + + $this->assertEquals(['a'], $user->get('groups')); + + $user->addToGroup(['a', 'b', 'c']); + + $this->assertEquals(['a', 'b', 'c'], $user->get('groups')); + } } diff --git a/tests/Auth/PermissibleContractTests.php b/tests/Auth/PermissibleContractTests.php index 12b97a46bd..816036a03f 100644 --- a/tests/Auth/PermissibleContractTests.php +++ b/tests/Auth/PermissibleContractTests.php @@ -81,51 +81,6 @@ public function handle(?string $handle = null) $this->assertEquals($user, $return); } - #[Test] - public function it_prevents_duplicate_roles_being_assigned() - { - // Prevent the anonymous role classes throwing errors when getting serialized - // during event handling unrelated to this test. - Event::fake(); - - $roleA = new class extends Role - { - public function handle(?string $handle = null) - { - return 'a'; - } - }; - $roleB = new class extends Role - { - public function handle(?string $handle = null) - { - return 'b'; - } - }; - $roleC = new class extends Role - { - public function handle(?string $handle = null) - { - return 'c'; - } - }; - - RoleAPI::shouldReceive('find')->with('a')->andReturn($roleA); - RoleAPI::shouldReceive('find')->with('b')->andReturn($roleB); - RoleAPI::shouldReceive('find')->with('c')->andReturn($roleC); - RoleAPI::shouldReceive('all')->andReturn(collect([$roleA, $roleB])); // the stache calls this when getting a user. unrelated to test. - - $user = $this->createPermissible(); - $user->assignRole('a'); - $user->save(); - - $this->assertEquals(['a'], $user->get('roles')); - - $user->assignRole(['a', 'b', 'c']); - - $this->assertEquals(['a', 'b', 'c'], $user->get('roles')); - } - #[Test] public function it_removes_a_role_assignment() { @@ -389,31 +344,6 @@ public function it_adds_to_groups() $this->assertTrue($user->isInGroup($groupC)); } - #[Test] - public function it_prevents_adding_duplicate_groups_to_a_user() - { - $groupA = (new UserGroup)->handle('a'); - $groupB = (new UserGroup)->handle('b'); - $groupC = (new UserGroup)->handle('c'); - $user = $this->createPermissible(); - - $this->assertFalse($user->isInGroup($groupA)); - $this->assertFalse($user->isInGroup($groupB)); - $this->assertFalse($user->isInGroup($groupC)); - - UserGroupAPI::shouldReceive('find')->with('a')->andReturn($groupA); - UserGroupAPI::shouldReceive('find')->with('b')->andReturn($groupB); - UserGroupAPI::shouldReceive('find')->with('c')->andReturn($groupC); - - $user->addToGroup('a'); - - $this->assertEquals(['a'], $user->get('groups')); - - $user->addToGroup(['a', 'b', 'c']); - - $this->assertEquals(['a', 'b', 'c'], $user->get('groups')); - } - #[Test] public function it_sets_all_the_groups() {