chaps-io / access-granted

Multi-role and whitelist based authorization gem for Rails (and not only Rails!)
MIT License
774 stars 41 forks source link

Class vs Instance subjects #57

Open jrochkind opened 1 year ago

jrochkind commented 1 year ago

As I'm trying to do something a bit more convoluted in my app, I'm realizing access-granted doesn't work quite as I thought for being able to kind of interchangeably use a Class (with the semantics "all of them", as in the can? :create, Post example) and an instance of a class interchangeably.

I was imagining on the same permission you could kind of use both interchangeably. But if we try say:

    role :admin, { role: "admin" } do
      can :manage, Post
    end

    role :user do
      can :manage, Post, { published: true }
    end

and then we try can? :create, Post, then of course you get undefined methodpublished' for Post:Class`.

Because it's trying to call published on the specific Post object you passed in, of course it is, you did specify that condition naturally. And Post.published is not a thing.

So I guess any given permission, you need to decide you are going to only use with a class object (like in README example for :create), or only use it with an instance (like in README examples with :update or :make_manager).

In my example above, I shouldn't say can :manage, Post, but instead maybe:

can [:read, :update, :destroy], Post, { published: false }

and then maybe only can :create, Post only on admin role.

My actual use case, I was hoping to mix and match. I had a read role defined like above, where admin's can read everything, everyone else can only read published things.

So when I have a specific item, of course I can just ask can? :read, specific_post,

But I want to know if I should show a UI widget, say, "include unpublished posts", and I should only show that widget to those who can see even unpublished posts, and I was hoping to be able to do can? :read, Post and have it be only true for Post.

But that won't work.

Curious if you have any advice. Should I just define a new permission :can_see_unpublished_posts which I give only to admin, so I can check that to decide whether to show the "include unpublished posts" button? It seems duplicative, but...

I suppose we could do another PR where access_granted, for hash conditions, first checks to make sure the method exists (with respond_to?), and if it doesn't, that's just false?

Or I guess I could write the condition long-hand:

    can :read, Post do |post, user|
       post.respond_to?(:published) && post.published
    end

And now I guess it'll work if I ask can? :read, Post (answer is no if they can only read published posts, because Post.respond_to?(:published) is false.... but still return properly for Post instances (based on published).

I'm not super happy with any of these solutions, I am curious your feedback!

jrochkind commented 1 year ago

Oops, I just realized it could get even worse though.

I realized it could get even worse though….

I get an exception now because Post.published is not a method… but what if it was a method, a scope for fetching published posts! That actually is a frequent convention… Post.published.where(title: foo).

Then the access_granted would decide that ordinary users could :read, Post because Post.published would return something other than false or nil. (It would return an ActiveRecord relation).

Maybe I'm just trying to be too clever -- any given permission should always only be used with a class object (as in :create examples) or only with instance objects (as in your :update examples).

It makes the :manage shortcut a bit tricker to use if you are ever adding conditions, since it includes :create conventionally used with Class as well as the others with instances. Oh well, it's just a shortcut.

and then whatever, I just need to create a new permission :can_read_all_posts or something that only applies to admin.

I'm not sure?

pokonski commented 1 year ago

I would not worry about the :manage shortcut, it's basically just something I reused from Cancan for easier management - but in retrospect I am not too fond about it. I'd be okay with getting rid of it

jrochkind commented 1 year ago

Yeah, the manage shortcut just made things even more obvious, but even without it, I was hoping I could do:

    role :admin, { role: "admin" } do
      can :read, Post
    end

    role :user do
      can :read, Post, { published: true }
    end

And then ask both:

but of course I can't.

Do you have a pattern to recommend when I have a policy like above, but sometimes need to answer "Should I show the button for show unpublished posts, are they allowed to see them generally?"

pokonski commented 1 year ago

Personally I go with a separate permission to mean reading generic lists, like :list or :index

jrochkind commented 1 year ago

Thanks that's helpful. In this case it's not a permission for "reading lists", but specifically for reading lists including unpublished records. We can easily filter the records already, just:

filtered = records.collect { |r| can? :read, r }

So there are people looking at the list view that can see unpublished records, and others that can't see unpublisehd records. Same list view. We filter the records.

But then we have some UI elements on the list view that only apply to people who can see unpublished records...

but I guess a separate permission is the way to go, maybe

can :list_unpublished, Posts

It's annoying to me, because it is really duplicate information, it really duplicates:

can :read, Posts # with no conditions, unlike other roles that have conditions, published-only

But I guess that's just the way it is, no big deal.

pokonski commented 1 year ago

Thanks for going into detail with the example, really helpful!

Yeah if you have some UI elements that don't relate to specific object of a model then sadly, you most likely need a separate permission to separate them.

I didn't really want to include the AR integration and the ability to filter records because I found those to be messy at the time, but maybe something more generic could work (your example with the published class method).

In regards to being too clever another possibility would be to have can be used like this:

# policy
can :read, Post do |record, user|
  if record.is_a?(Class)
    user.admin?
  else
    record.published? || user.admin?
  end
end

Then in your UI you could do

if can? :read, Post

around the button to show all posts.

It looks explicit but should handle both cases. Though I am writing that from the top of my head so I am not entirely sure we allow passing blocks like that for non-instance can method calls. I'll give it a try :)

jrochkind commented 1 year ago

I agree no specific AR-integration needs to be provided by access-granted, the API already present is fine and lets us easily filter whatever we want.

Still mulling over these suggestions, thanks! if can? :read, Post is what I tried first, on the analogy of can? :create, Post in the docs -- but the issue is that doesn't work when this is my policy:

    role :admin, { role: "admin" } do
      can :read, Post
    end

    role :user do
      can :read, Post, { published: true }
    end

In that case, asking can? :read, Post results in undefined method 'published' for Post:Class. Since the {published: true} check is there for asking can? :read, specific_post_obj.