disable_risky_blocks() for disabling risky default block behaviour #622

Closed
wants to merge 2 commits into
from

4 participants

@emiltin

consider this simple ability definition:

class Ability < ActiveRecord::Base
  include CanCan::Ability
  def initialize(user)
    can :manage, Group do |group|
      user.admin?
    end
  end 
end

without this patch, you might get a nasty surprise when you find out that non-admins can create (as well as index) groups. this is because defining a block makes cancan always return true when authorizing the involved class (without an object). there seems to be no way to disallowing :new on Groups without effectively disabling the block logic.

with this patch, you can call disable_risky_blocks() to make blocks return false instead of true when authorizing a class:

class Ability < ActiveRecord::Base
  include CanCan::Ability
  def initialize(user)
    disable_risky_blocks
    can :manage, Group do |group|
      user.admin?
    end
  end 
end

now non-admins cannot create groups. if you want to allow it, just add:

  can :new, Group

seems to work for me.

@emiltin

anything particular you wanted to point to?

@andhapp
Collaborator

Oh sorry, if you read under "Only for Object Attributes".

@emiltin

thanks. i know that the block is not called when you pass a class. instead, true is automatically returned, which can cause unpleasant surprises. this pull request changes the default to false (whitelist approach instead of blacklist).

@emiltin

well, it doesn't directly change the default, it allows you to change it.

@rmcastil
Collaborator

@emiltin Can you include a spec for this use case in your pull request? Why not just change the behavior of the block so that it always returns false?

@emiltin

the problem is that the block is not called when the subject is a class. instead true is automatically returned.

@andhapp
Collaborator

@emiltin: Hey, can you please add some specs around this behaviour. It makes the pull request a good candidate for merging. Thanks.

@emiltin

ok i've added specs, and hope to push them later today.
btw, it seems a lot of specs fail under ruby 1.9?

@andhapp
Collaborator

@emiltin I confirmed with @ryanb and 1.6.7 is still developed on 1.8.7.

@emiltin

here's the spec. i rebased the branch on upstream/master

@emiltin

let me know if you would prefer a single commit

@andhapp
Collaborator

@emiltin: After looking at the implementation in CanCan 2.0, I think it'd be better for CanCan to just throw an error(Insufficient Information or something like that) and let the client handle it in a reasonable way. Similar, to how it does for un-authorised requests.

Sorry for requesting you to write specs. It just makes much more sense this (as mentioned above) way. What do you think? Please give your opinion. Thanks.

@emiltin

i don't see anything wrong in having sensible (safe) defaults.
i also don't see what info is missing.if you allow passing a class to can()

@emiltin

i don't see anything wrong in having sensible (safe) defaults.
i also don't see what info is missing. if you allow passing a class to can(), what else would you like the user to pass?

@andhapp
Collaborator

@emiltin: When I said error message, I didn't literally mean "Insufficient Information". But something that will tell the user that "Hey, I need the object instance to actually tell you if you can do anything with this object", something the wiki says already. To protect people from nasty surprises an explicit error can be thrown so there isn't any surprise.

What do you think?

@emiltin

but how can you ever check if a user is allowed to create or index a Something, if you're required to pass an instance? this doesn't make sense to me.

@andhapp
Collaborator
@emiltin

something like this:

def create
  authorize! :create, BlogEntry
  #create new blog entry
end

without the patch, authorize! always returns true.

@andhapp
Collaborator

Thanks. I'm guessing you can't do something like this:

can :create, Group if user.admin?

May be you have a complex condition to check against. Please don't take me wrong. I'm totally in favour of this feature that eliminates nasty surprises but just trying to think of a better implementation.

@emiltin

yes, different users have access to different groups, so i need the block. the example is just simplified a bit.

@ryanb
Owner

I realize this is a problem in CanCan 1.x but a proper fix isn't very simple which is the main motivation to change the way this works in CanCan 2. I don't plan to fix this in 1.x and instead want to focus on 2.0 release.

@ryanb ryanb closed this Jul 16, 2012
@emiltin

i myself found the patch to be simple enough and quite unobtrusive. but alright, i'll leave it here and let you focus on 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment