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

class inheritance, and class as test subject #55

Open
jrochkind opened this issue Dec 7, 2022 · 4 comments · May be fixed by #56
Open

class inheritance, and class as test subject #55

jrochkind opened this issue Dec 7, 2022 · 4 comments · May be fixed by #56

Comments

@jrochkind
Copy link
Contributor

jrochkind commented Dec 7, 2022

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 a Bicycle < Vehicle. And we have a policy:

    class AccessPolicy
      include AccessGranted::Policy
      
      role :user do
        can :read, Vehicle
      end
    end

AccessPolicy.new(user).can? :read, Vehicle.new # true, of course
AccessPolicy.new(user).can? :read, Bicycle.new # still true, it works with the sub-class

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:

AccessPolicy.new(user).can? :read, Vehicle # this is true too, okay

That one does NOT work with inheritance:

AccessPolicy.new(user).can? :read, Bicycle # FALSE
# I believe it should be true

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?

@pokonski
Copy link
Contributor

pokonski commented Dec 7, 2022

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 😁

jrochkind added a commit to jrochkind/access-granted that referenced this issue Dec 7, 2022
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.
@jrochkind jrochkind linked a pull request Dec 7, 2022 that will close this issue
@jrochkind
Copy link
Contributor Author

jrochkind commented Dec 8, 2022

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 Car and Bicycle. And sometimes we want to check on the generic class of a sub-class: can? :create, Bicycle -- right now this will be false, but we wish it would be true. We can of course everywhere instead ask can? :create, Vehicle, which would be true, but we're actually in a certain case checking specifically for Bicycle, maybe in the future there will be a role that can only create Bicycles but not Cars.

Yep, this has come up in a real project. Right now, we just need to remember "out of band", "You have to ask can? :create, Vehicle, it won't work if you ask can? :create, Bicycle" -- and the semantics are for now sufficient for us (maybe not in the future), but I don't like that you always have to remember this and if you slip up and ask for Bicycle you'll get a wrong answer. It's a weird extra non-obvious convention to remember, for now -- and if in the future we change to have some people that can universally manage Bicycle but not Car, then we have to change convention again and change all the can? calls.

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 ==.... when I get into the details it's not quite as trivial as my first fantasy to keep everything working, but I guess it could be done.

@pokonski
Copy link
Contributor

pokonski commented Dec 8, 2022

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 :)

By the way, i really loooove access-granted. It is so simple and composeable and extendable while being so flexible and powerful.

I love to hear that 😁

@jrochkind
Copy link
Contributor Author

Any further thoughts, @pokonski?

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 a pull request may close this issue.

2 participants