-
Notifications
You must be signed in to change notification settings - Fork 41
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
class inheritance, and class as test subject #55
Comments
Hello, Jonathan! Thanks for the suggestion, from the top of my head I cannot think of any downsides to this BUT I'd treat it as a breaking change warranting a major version bump to to the gem. Did you encounter this in a real life project? I'd love to hear about that use case. In any case please do send a PR 😁 |
if Bicycle < Vehicle, and you have a policy `can :read, Vehicle`, then already `can?(:read, Vehicle.new)` and `can?(:read, Bicycle.new)` are both true. `can?(:read, Vehicle)` is also true. I believe `can?(:read, Bicycle)` should also be true, it should respect the subclass. Bicycle is a kind of Vehicle, so if they have been granted permission to read all Vehicles, that applies to all Bicycles too. Closes chaps-io#55, see more there.
Thanks for the response! Yes, I encountered this in a real project. We use model inheritance (Rails single-table inheritance, but it doesn't really matter). So we have a class hieararchy. We want to express policies on the superclass if it's the same for all of them, say (and this is still a simplified extraction of course): role :admin, proc { |user| user.admin? } do
can :manage, Vehicle # all of them
end
role :contributor do
can :manage, Vehicle do |model, user|
model.owner == user
end
end But Vehicle is a superclass, maybe there's also Yep, this has come up in a real project. Right now, we just need to remember "out of band", "You have to ask By the way, i really loooove access-granted. It is so simple and composeable and extendable while being so flexible and powerful. Anyway, as you see, PR filed. I have no opinion on backwards compat/major version... while I can see the argument it is technically a backwards incompat, I have trouble imagining a real scenario that it would break... but I guess I reluctantly admit that is not a great argument for not considering it a backwards incompat. Hmm, if I really needed to work around this in present access-granted, I could define my own wrapper class that redefines |
Thanks for the detailed expalnation, maybe making a major bump for this is indeed an overkill 😬 I'll release a new version most likely next week, I noticed some possible performance improvements around the code you modified so I'll tweak stuff a bit after merging this :)
I love to hear that 😁 |
Any further thoughts, @pokonski? |
I found what I think is an inconvenient inconsistency around inheritance; I also have a simple PR to fix it.
access_granted normally works great with inheritance. Imagine we have a
Vehicle
and aBicycle < Vehicle
. And we have a policy:But access_granted also has a convenient feature where you can check on the class instead of an instance, intended to be used with the meaning of sort of generic/any/all objects, like often for
create
, but you can use it for anything:That one does NOT work with inheritance:
I think this is a bug, or would be an improvement if above were true also. I think I have a simple PR to make it so.
Thoughts?
The text was updated successfully, but these errors were encountered: