laravel-permission
laravel-permission copied to clipboard
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?
Could this be a breaking change?
As in need to bump to 7.0
immediately?
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?
@erikn69 @drbyte any updates on if/when this will be merged?
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