Merge pull request #2341 from erikn69/patch-2

Avoid loss of all permissions/roles pivots on sync error
spatie · Mar 16, 2023 · 70a2fdb · 70a2fdb
2 parents 490fa00 + 248e93f
commit 70a2fdb
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 8 deletions.
diff --git a/src/Traits/HasPermissions.php b/src/Traits/HasPermissions.php
@@ -343,9 +343,8 @@ public function getAllPermissions(): Collection
      * Returns permissions ids as array keys
      *
      * @param  string|int|array|\Spatie\Permission\Contracts\Permission|\Illuminate\Support\Collection  $permissions
-     * @return array
      */
-    public function collectPermissions(...$permissions)
+    private function collectPermissions(...$permissions): array
     {
         return collect($permissions)
             ->flatten()
@@ -376,7 +375,7 @@ public function collectPermissions(...$permissions)
      */
     public function givePermissionTo(...$permissions)
     {
-        $permissions = $this->collectPermissions(...$permissions);
+        $permissions = $this->collectPermissions($permissions);
 
         $model = $this->getModel();
 
@@ -412,6 +411,8 @@ function ($object) use ($permissions, $model) {
      */
     public function syncPermissions(...$permissions)
     {
+        $this->collectPermissions($permissions);
+
         $this->permissions()->detach();
 
         return $this->givePermissionTo($permissions);

diff --git a/src/Traits/HasRoles.php b/src/Traits/HasRoles.php
@@ -96,14 +96,13 @@ public function scopeRole(Builder $query, $roles, $guard = null): Builder
     }
 
     /**
-     * Assign the given role to the model.
+     * Returns roles ids as array keys
      *
-     * @param  array|string|int|\Spatie\Permission\Contracts\Role|\Illuminate\Support\Collection  ...$roles
-     * @return $this
+     * @param  array|string|int|\Spatie\Permission\Contracts\Role|\Illuminate\Support\Collection  $roles
      */
-    public function assignRole(...$roles)
+    private function collectRoles(...$roles): array
     {
-        $roles = collect($roles)
+        return collect($roles)
             ->flatten()
             ->reduce(function ($array, $role) {
                 if (empty($role)) {
@@ -122,6 +121,17 @@ public function assignRole(...$roles)
 
                 return $array;
             }, []);
+    }
+
+    /**
+     * Assign the given role to the model.
+     *
+     * @param  array|string|int|\Spatie\Permission\Contracts\Role|\Illuminate\Support\Collection  ...$roles
+     * @return $this
+     */
+    public function assignRole(...$roles)
+    {
+        $roles = $this->collectRoles($roles);
 
         $model = $this->getModel();
 
@@ -175,6 +185,8 @@ public function removeRole($role)
      */
     public function syncRoles(...$roles)
     {
+        $this->collectRoles($roles);
+
         $this->roles()->detach();
 
         return $this->assignRole($roles);

diff --git a/tests/HasPermissionsTest.php b/tests/HasPermissionsTest.php
@@ -476,6 +476,18 @@ public function sync_permission_ignores_null_inputs()
         $this->assertFalse($this->testUser->hasDirectPermission('edit-news'));
     }
 
+    /** @test */
+    public function sync_permission_error_does_not_detach_permissions()
+    {
+        $this->testUser->givePermissionTo('edit-news');
+
+        $this->expectException(PermissionDoesNotExist::class);
+
+        $this->testUser->syncPermissions('edit-articles', 'permission-that-does-not-exist');
+
+        $this->assertTrue($this->testUser->fresh()->hasDirectPermission('edit-news'));
+    }
+
     /** @test */
     public function it_does_not_remove_already_associated_permissions_when_assigning_new_permissions()
     {

diff --git a/tests/HasRolesTest.php b/tests/HasRolesTest.php
@@ -227,6 +227,18 @@ public function it_will_remove_all_roles_when_an_empty_array_is_passed_to_sync_r
         $this->assertFalse($this->testUser->hasRole('testRole2'));
     }
 
+    /** @test */
+    public function sync_roles_error_does_not_detach_roles()
+    {
+        $this->testUser->assignRole('testRole');
+
+        $this->expectException(RoleDoesNotExist::class);
+
+        $this->testUser->syncRoles('testRole2', 'role-that-does-not-exist');
+
+        $this->assertTrue($this->testUser->fresh()->hasRole('testRole'));
+    }
+
     /** @test */
     public function it_will_sync_roles_to_a_model_that_is_not_persisted()
     {

diff --git a/tests/RoleTest.php b/tests/RoleTest.php
@@ -155,6 +155,18 @@ public function it_will_remove_all_permissions_when_passing_an_empty_array_to_sy
         $this->assertFalse($this->testUserRole->hasPermissionTo('edit-news'));
     }
 
+    /** @test */
+    public function sync_permission_error_does_not_detach_permissions()
+    {
+        $this->testUserRole->givePermissionTo('edit-news');
+
+        $this->expectException(PermissionDoesNotExist::class);
+
+        $this->testUserRole->syncPermissions('edit-articles', 'permission-that-does-not-exist');
+
+        $this->assertTrue($this->testUserRole->fresh()->hasDirectPermission('edit-news'));
+    }
+
     /** @test */
     public function it_can_revoke_a_permission()
     {