Syncing an incorrect permission results in loss of all permissions

[jnoordsij]

Before creating a new bug report
Please check if there isn't a similar issue on the issue tracker or in the 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 490fa00
• 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:
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

[erikn69]

if you lost all permissions it's a big problem, but it did not happen to me because I use transactions, small example:

DB::beginTransaction();
try{
    // here all your sql transactions
    DB::commit();
}catch(Exception $e){
    DB::rollback();
}

With that, when exception raised rollback is made and I do not lose or change anything

[jnoordsij]

Hey, thanks for mentioning this! I do think that's a great workaround indeed for those encoutering a similar problem and looking for a way to fix this with their own codebase.

In my case I only ran into this problem on my local environment, as my production environment code does not have non-existing permission issues, so no loss of real data for me luckily.

This issue is thus mainly an effort to raise awareness of this possibly unexpected behavior, get some input on the best approach to solve this more subtly within this package, and therefore hopefully saving some people some pain in the future!

[erikn69]

hopefully saving some people some pain in the future!

@jnoordsij a fix for your tests #2341 bbaacf2

[jnoordsij]

That looks like a great starting point! It's quite specifically tied to this case though, so maybe one of the approaches I suggested may be more preferable, as they are less prone to other exceptions happening while trying to sync.

However they might be more technically complex, so maybe it's a great start to go with your fix!