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

Do not run block if given a class instead of an instance #16

Closed erlingur closed 8 years ago

erlingur commented 8 years ago

I ran into an issue where I defined a permission like so:

can :manage, Course do |course, user|
  user.school_ids.include? course.school_id
end

I wanted to be able to check if the user can manage any course in general like so:

can? :manage, Course

And then when the user would drill down further I could determine if he could manage the specific courses like so:

can? :manage, course

However, when I did this I ran into an error since it will try to run the block I gave in the permission definition, even when I give it a class, not an instance of that class. That sparks an undefined method 'school_id' error.

This PR only runs the block if you have given it something that is an instance (i.e. does not respond to #new).

pokonski commented 8 years ago

Thanks! Good catch, this is definitely a bug!

One thing before I merge this: can you change respond_to? :new to is_a? Class? This is a safer way to check if something is a class, because we might have objects responding to new and it will break in such case.

erlingur commented 8 years ago

Awesome! I was struggling to find a good way to determine between an instance and a Class. Your way is much better.

I've updated the PR.

On a sidenote, I really like this gem! Thanks!

pokonski commented 8 years ago

Sweet, let's get this merged and released :+1:

On a sidenote, I really like this gem! Thanks!

Thanks, I'm glad you find it useful :)

erlingur commented 8 years ago

:+1: :)

pokonski commented 8 years ago

And released as 1.0.3 to Rubygems :dash:

erlingur commented 8 years ago

Awesome, thanks! I can get back to work then :)

erlingur commented 8 years ago

Actually, thinking about it... should it even run the matches_hash_conditions? when given a class? Can you think of a case where you would need to check some attribute of the class itself?

erlingur commented 8 years ago

Should it maybe be like this? https://github.com/erlingur/access-granted/commit/b3eecf5514735c99e12d98ddd54a8ee4692c6143

pokonski commented 8 years ago

I think it can be done a level higher, I'll see :)

On 26 November 2015 at 22:24, Erlingur Þorsteinsson < notifications@github.com> wrote:

Should it maybe be like this? erlingur@b3eecf5 https://github.com/erlingur/access-granted/commit/b3eecf5514735c99e12d98ddd54a8ee4692c6143

— Reply to this email directly or view it on GitHub https://github.com/chaps-io/access-granted/pull/16#issuecomment-159995589 .

Piotr Okoński piotrek@okonski.org | +48 792 097 151

erlingur commented 8 years ago

:+1: