laravel-permission
laravel-permission copied to clipboard
spatie
Support strings as numeric ids
testing https://github.com/spatie/laravel-permission/issues/2530#issuecomment-1779319899
Closes #2530 Closes #2543 Closes #2541
pro: this is consistent with laravel sync method, but
on this part it suggests that you can use int string as name (on wildcards), even so, wildcards seems to work, I don't know if it needs more tests
https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/tests/WildcardHasPermissionsTest.php#L244
On v5 was is_int, an is_string, so string as "42" always is taken as name
https://github.com/spatie/laravel-permission/blob/d829888b659cc7df9a7d10ad29f24cbfeb6c3297/src/Traits/HasPermissions.php#L138
https://github.com/spatie/laravel-permission/blob/d829888b659cc7df9a7d10ad29f24cbfeb6c3297/src/Traits/HasPermissions.php#L156-L168
I think we need to add some tests for this situation we're trying to fix.
A couple observations:
- The reported situations are where the data being passed is from form input: ids sent as strings via POST.
- The
getStoredPermissions()method doesn't check against IDs if the incoming param is an array; it only checks against names: https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/src/Traits/HasPermissions.php#L491-L503 It also checks against ALL guards ... hmmm....
the
getStoredPermissions()method doesn't check against IDs if the incoming param is an array
Ok, i see it now, but syncPermissions calls collectPermissions, then collectPermissions calls getStoredPermissions
collectPermissions uses flatten before getStoredPermission, In that case, would it be a string instead of an array?
https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/src/Traits/HasPermissions.php#L436-L439
https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/src/Traits/HasPermissions.php#L361-L368
On revokePermissionTo it will fail
https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/src/Traits/HasPermissions.php#L453-L455
I added some tests but I'm still not sure, I feel like I'm missing something 😕
Could this be a breaking change?
Possibly, if someone uses numbers as names in permissions/roles, they will no longer be able to access those models through the package.
https://github.com/spatie/laravel-permission/blob/62f22e192711fe56fb55b57b586001f8674b4396/tests/WildcardHasPermissionsTest.php#L244
Possibly [a breaking change], if someone uses numbers as names in permissions/roles, they will no longer be able to access those models through the package.
I wonder if that scenario could be simply handled with a configuration switch? IMO the number-as-a-name is a rare-use case, but number-as-model-id-integer is vast majority use-case.
IMO the number-as-a-name is a rare-use case, but number-as-model-id-integer is vast majority use-case.
Yes, most likely
After doing some digging it is not a breaking change(v5), it is a bug fix, #2404 break this(lack of tests) https://github.com/spatie/laravel-permission/blob/d829888b659cc7df9a7d10ad29f24cbfeb6c3297/src/Traits/HasPermissions.php#L444-L450 https://github.com/spatie/laravel-permission/blob/d829888b659cc7df9a7d10ad29f24cbfeb6c3297/src/Traits/HasRoles.php#L319-L325
Same issue here upgrading from v.5.5 to v.6.2.
Simple test in tinker:
<?php
$permissions = ["10", "20", "30"];
$user->syncPermissions($permissions);
// v.5.5 => no error
// v.6.2 => error: Spatie\Permission\Exceptions\PermissionDoesNotExist There is no [permission] named `10` for guard `web`.
Adopting the changes proposed in this PR v.6.2 works fine.
Same issue here upgrading from v.5.5 to v.6.2.
Simple test in tinker:
<?php $permissions = ["10", "20", "30"]; $user->syncPermissions($permissions); // v.5.5 => no error // v.6.2 => error: Spatie\Permission\Exceptions\PermissionDoesNotExist There is no [permission] named `10` for guard `web`.Adopting the changes proposed in this PR v.6.2 works fine.
Looks like this fixes a regression in v6, any plans for this to be merged soon? Is there anything missing in this PR?
Status: Trying to decide whether this is safe to merge into v6, or whether to tag it as v7 .... and then what the complaints will be about v7 :(
Status: Trying to decide whether this is safe to merge into v6, or whether to tag it as v7 .... and then what the complaints will be about v7 :(
Should probably be v7 to avoid issues with existing installs
Dear contributor,
because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.
