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 vs Instance subjects #57

Open
jrochkind opened this issue Dec 8, 2022 · 7 comments
Open

Class vs Instance subjects #57

jrochkind opened this issue Dec 8, 2022 · 7 comments

Comments

@jrochkind
Copy link
Contributor

jrochkind commented Dec 8, 2022

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 method published' 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
Copy link
Contributor Author

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
Copy link
Contributor

pokonski commented Dec 8, 2022

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
Copy link
Contributor Author

jrochkind commented Dec 8, 2022

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:

  • can? :read, an_individual_post
  • can? :read, Post # meaning can they read all posts, generally?

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
Copy link
Contributor

pokonski commented Dec 9, 2022

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

@jrochkind
Copy link
Contributor Author

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
Copy link
Contributor

pokonski commented Dec 10, 2022

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
Copy link
Contributor Author

jrochkind commented Dec 10, 2022

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.

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

2 participants