Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spike permission check framework #10164

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tristan-orourke
Copy link
Member

πŸ€– Related to #10144.

πŸ‘‹ Introduction

Testing a framework which generalizes the multiple ways a user might have access to a resource.

πŸ•΅οΈ Details

When users have a permission related to a resource, it may apply to all instances of the resource, or specific resources owned by the user, or resources associated with a team. This framework is meant to avoid devs having to check each instance appropriately. This may especially helpful as we the notion of 'team' becomes more abstract.

This is very early, just looking for feedback for now.

πŸ§ͺ Testing

Assist reviewers with steps they can take to test that the PR does what it says it does.

  1. ...
  2. ...

πŸ“Έ Screenshot

Add a screenshot (if possible).

🚚 Deployment

Add any additional details that are required for deploying the application.

Examples of when this is required include:

  • re-running database seeders
  • environment variable changes

Notes

  • Remove deployment section if no steps are needed
  • Add deployment label to the linked issue if deployment steps are needed

Copy link
Contributor

@petertgiles petertgiles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! A lot to like here - I think you're on the right track. Good stuff. 😎

use App\Models\User;
use Illuminate\Support\Facades\Auth;

class PermissionCheckService
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this class looks a lot like UserChecker.php used by Laratrust. userCan instead of currentUserHasPermission, for example. I wonder if there's anyway to integrate this more tightly into Laratrust?

Compare to ProtectedRequestUserChecker and UserDefaultChecker.

}
public function resourceName(): string
{
return 'pool';
Copy link
Contributor

@petertgiles petertgiles Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there's any way to link this in a more expressive/explicit way? Maybe config('rolepermission.resources.pool')?

protected $permissionCheckService;

// Put PermissionCheckService into a variable in the constructor
public function __construct(PermissionCheckService $permissionCheckService)
Copy link
Contributor

@petertgiles petertgiles Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You use the DI container to inject a PermissionCheckService into the constructor here but then you new up a new instance in every method below anyway?

The more idiomatic way for Laravel (I think?) would be have this service as a singleton that you get from the container like this and then reuse it over and over. You'd probably have to pass in $user as a parameter in the function calls but this would make the service very testable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants