Repository: spatie/laravel-permission (php)

Issue:
Syncing an incorrect permission results in loss of all permissions
**Before creating a new bug report**
Please check if there isn't a similar issue on [the issue tracker](https://github.com/spatie/laravel-permission/issues) or in [the discussions](https://github.com/spatie/laravel-permission/discussions).

**Describe the bug**
When using the `syncPermissions` function from the `HasPermissions` trait, providing an accidental wrongly named permission will result in loss of all permissions. This is caused by the fact that `syncPermissions` eagerly removes all existing permissions.

**Versions**
You can use `composer show` to get the version numbers of:
- spatie/laravel-permission package version: `main` at commit 490fa00b0c8286659a534d822f7e74c684c86d27
- illuminate/framework package: N/A

PHP version: any

Database version: N/A


**To Reproduce**
Steps to reproduce the behavior:
1. Create a model that uses the `HasPermissions` trait
2. Assign some valid permissions to the model
3. Use the `syncPermissions` helper, with a combination of any number of valid permissions and at least one invalid permission
Result: all permissions will be lost on the user

**Example Test**
Here is a link to my Github repo containing a minimal Laravel application which shows my problem:
https://github.com/spatie/laravel-permission/compare/main...jnoordsij:spatie-laravel-permission:permission-sync-fail

**Expected behavior**
Result: the operation fails and the permissions remain the same as before 
OR operation does partial work, removing only those permissions which are not synced.
 

**Proposed (possible) solutions**
1. Use the native Laravel `sync` function with detaching; this does not do a full rollback, but it does ensure only pivots that are no longer present are removed, instead of eagerly removing all of them
2. Copy the behavior of the Laravel `sync` function to achieve something similar here
3. Do a "manual backup" of pre-existing permissions and force restore on error
4. Skip over any non-existing permissions and throw Exception afterwards


Pull request description:
Avoid loss of all permissions/roles pivots on sync error
Closes #2339

>**Describe the bug**
When using the syncPermissions function from the HasPermissions trait, providing an accidental wrongly named permission will result in loss of all permissions. This is caused by the fact that syncPermissions eagerly removes all existing permissions.
>
>**Steps to reproduce the behavior:**
>Create a model that uses the HasPermissions trait
Assign some valid permissions to the model
Use the syncPermissions helper, with a combination of any number of valid permissions and at least one invalid permission
Result: all permissions will be lost on the user
>
>**Example Test**
Here is a link to my Github repo containing a minimal Laravel application which shows my problem:
https://github.com/spatie/laravel-permission/compare/main...jnoordsij:spatie-laravel-permission:permission-sync-fail

Patch:
diff --git a/src/Traits/HasPermissions.php b/src/Traits/HasPermissions.php
index 7e2b91f..ee6d07d 100644
--- a/src/Traits/HasPermissions.php
+++ b/src/Traits/HasPermissions.php
@@ -343,9 +343,8 @@ trait HasPermissions
      * 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 @@ trait HasPermissions
      */
     public function givePermissionTo(...$permissions)
     {
-        $permissions = $this->collectPermissions(...$permissions);
+        $permissions = $this->collectPermissions($permissions);
 
         $model = $this->getModel();
 
@@ -412,6 +411,8 @@ trait HasPermissions
      */
     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
index 78787df..267528c 100644
--- a/src/Traits/HasRoles.php
+++ b/src/Traits/HasRoles.php
@@ -96,14 +96,13 @@ trait HasRoles
     }
 
     /**
-     * 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 @@ trait HasRoles
 
                 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 @@ trait HasRoles
      */
     public function syncRoles(...$roles)
     {
+        $this->collectRoles($roles);
+
         $this->roles()->detach();
 
         return $this->assignRole($roles);


Test patch:
diff --git a/tests/HasPermissionsTest.php b/tests/HasPermissionsTest.php
index 61bba12..a718675 100644
--- a/tests/HasPermissionsTest.php
+++ b/tests/HasPermissionsTest.php
@@ -476,6 +476,18 @@ class HasPermissionsTest extends TestCase
         $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
index 0031b58..1e7dcc8 100644
--- a/tests/HasRolesTest.php
+++ b/tests/HasRolesTest.php
@@ -227,6 +227,18 @@ class HasRolesTest extends TestCase
         $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
index 35e287d..e76582f 100644
--- a/tests/RoleTest.php
+++ b/tests/RoleTest.php
@@ -155,6 +155,18 @@ class RoleTest extends TestCase
         $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()
     {