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

Authorizable method_exists vs is_callable #1147

Closed
jehoshua02 opened this issue Dec 18, 2018 · 3 comments
Closed

Authorizable method_exists vs is_callable #1147

jehoshua02 opened this issue Dec 18, 2018 · 3 comments

Comments

@jehoshua02
Copy link

My team has all our policies extending an abstract policy that does some generic permission checking.

Then we ran into issues trying to lock down relationship modifications in a similarly generic fashion. To lock down a single relationship, we have to:

  1. Define attach method in policy that simply proxies through to our abstract.
  2. Define a detach method in policy that also proxies through to abstract.
  3. Define inverse methods on policy for related model.
  4. Triple check for missing methods, copy/paste errors, etc.

This seems very tedious when we want to lock down relationships by default and open them up one at a time using our permission system.

If we could magically implement attach / detach methods in the abstract class, all our problems would be solved.

Digging into source code I notice that our issue is caused by the Authorizable trait using method_exists which makes it impossible to magically program attach and detach methods.

Could this be easily updated to use is_callable instead? Or is there a reason for using method_exists?

Many thanks! Great framework!

@davidhemphill
Copy link
Contributor

I'm not sure what you're asking us to change inside of Nova. Please post a new issue if you can reproduce an issue inside of it. Thanks!

@jehoshua02
Copy link
Author

jehoshua02 commented Jul 13, 2019

@davidhemphill before I dive into investing time in preparing a pull request, I wanted to have some discussion about the issue.

Nova's Authorizable trait uses method_exists to check policy on relationships (addMyModel, attachMyModel, attachAnyMyModel, detachMyModel, etc).

My team has implemented most our policy logic in a custom AbstractPolicy which all our Policies extend.

But when it came to relationships (addMyModel, attachMyModel, attachAnyMyModel, detachMyModel, etc) we could not implement those methods purely in the AbstractPolicy because Nova's Authorizable trait utilizes method_exists to see if the policy actually has the method.

We want to trick Nova Authorizable trait into thinking the policy does have the method by using is_callable instead of method_exists, then use magic __call in our custom AbstractPolicy to dynamically implement our relationship methods.

But I just did some research just now, and there's a problem with that: is_callable will return true for any possible method if the __call is defined on the class.

So a more sophisticated / different solution is required, if it's worth the effort.

Bottom line

  1. We want all relationships locked down by default
  2. Our policy logic for all relationships is identical

So we want to implement this in a dynamic way within our AbstractPolicy.

What does the Nova team think? Is the problem worth solving? Can we have a discussion about how to solve it?

After some discussion and a clear idea of how it should work I'd be happy to submit a pull request.

@didix16
Copy link

didix16 commented Feb 7, 2020

Hello guys. I'm in the same situation that @jehoshua02 say.
Is there any chance we can call attach/dettach methods using __call ?

Thanks in advance

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

No branches or pull requests

3 participants