search

cartalyst / sentinel

A framework agnostic authentication & authorization system.
BSD 3-Clause "New" or "Revised" License
1.51k stars 238 forks source link

Allow EloquentUser to ignore empty secondary permissons. #501

Closed rydurham closed 5 years ago

rydurham commented 5 years ago

Hello,

Thank you for this excellent package - I have been using it for years and I am very grateful for the work you have done.

This PR addresses a potential bug I have found, though it is possible that I am operating with incomplete knowledge.

It is my understanding that the migration provided for Laravel applications allows the permissions field on users and roles to be nullable. The preparePermissions method in the PermissionsTrait class requires that the permissions be provided as an array. When constructing user permissions for a user that belongs to a role with no permissions, the createPermissions method on the EloquentUser class will pass on a null value to the preparePermissions method, throwing an error.

This PR updates that createPermissions method to ignore roles with empty permissions.

Thanks again!

suwardany commented 5 years ago

Hello @rydurham thanks for the PR! I had actually missed using the new getter method which takes care of that to ensure an array is always returned on these two occurrences.

I just pushed a fix to the 3.0 branch, can you give it a try and let me know if it works as expected!

suwardany commented 5 years ago

@rydurham if you can submit that test alone on a new PR, that would still be helpful to include! Will need minor but covers a case that was missing.

rydurham commented 5 years ago

Hello,

Thanks for taking a look at this. I have confirmed that the changes you made in b85c26dd9ad925fd23f1a7dc8dae3e6c5daf6d76 will fix this problem. Thank you!

I would be happy to submit a new PR with a test, though I am not sure an updated version of the test in this PR would be very helpful. Most likely we would need to mock the getPermissions method on the $mockRole object, and the mock would need to return an empty array, right? Is that still useful?

I looked at adding a test to PermissionsTraitTest.php that would confirm that null permissions are always converted to an empty array, but it doesn't look like there is an easy way create a PermissionsStub that has null permissions.

suwardany commented 5 years ago

Great to hear it fixed your issue!

Just duplicate this test here https://github.com/cartalyst/sentinel/blob/b85c26dd9ad925fd23f1a7dc8dae3e6c5daf6d76/tests/Users/EloquentUserTest.php#L400-L417 into a new method and set permissions to null there instead of an array.

That's all we need here :)

rydurham commented 5 years ago

Sounds good - will do. Thanks!