index action not blocking? #275

Closed
sekrett opened this Issue Feb 11, 2011 · 15 comments

Comments

Projects
None yet
2 participants

sekrett commented Feb 11, 2011

Hello!

Here is my ability class:

class Ability
  include CanCan::Ability

  def initialize(user)
    can(:manage, :all) { false }
  end
end

And I'm still able to go to /pages, /categories and so on. I expect it should prohibit everything.

sekrett commented Feb 11, 2011

class CategoriesController < ApplicationController
  load_and_authorize_resource
  ...
end

class PagesController < ApplicationController
  load_and_authorize_resource
  ...
end

Designshop::Application.routes.draw do
  resources :pages

  scope :except => [:new, :destroy] do
    resources :categories do
      resources :products
    end
  end

  devise_for :users

  root :to => "welcome#index"
end
Owner

ryanb commented Feb 11, 2011

The block is only executed when an actual object is passed into the can? call because it's used to further define permissions based on the object passed. Since the index action does not pass an object (only the class) the block is never called.

See the "Checking with Class" section of the Checking Abilities wiki page for details.

I'm guessing you have some conditions in the block there? In that case place the conditions outside of the block.

if user.admin?
  can :manage, :all
end

Only use a block when you need to change behavior based on the attributes of an object.

sekrett commented Feb 11, 2011

Ok, I understand now, thank you. But it was not clear from Checking Abilities page.

Why don't you implement different default behavior? If I leave Ability class absolutely empty, I would not be able to open any (home page is an exception) url and it is correct. If it is not clear what to do (object not loaded, impossible to check, etc), it would be secure to deny, so a man will notice that and add some rules to fix.

sekrett commented Feb 11, 2011

Or even better not to deny, but ignore that 'can' definition. If there was something declared before, it will not be overridden, and if nothing is declared after, it will be denied. So I think 'ignore' is best behavior.

Owner

ryanb commented Feb 11, 2011

But it was not clear from Checking Abilities page.

Hmm, I'll try to improve the documentation on this. Any suggestions on making it clearer?

If it is not clear what to do (object not loaded, impossible to check, etc), it would be secure to deny, so a man will notice that and add some rules to fix.

I'm not sure what you mean by this. The default behavior is to deny everything unless you give permission. When you say can :manage, :all you are providing permission for that user to manage everything.

Blocks are only intended to be used for defining abilities based on an object's attributes. For example, let's say you had a visible attribute on every model and you want to change permissions based on that attribute.

can :manage, :all do |object|
  object.visible?
end

That is the only case when a block should be used because the block is only executed when an object is available. All other conditions should be defined outside the block.

Note, in this case a hash of conditions would be better so it can be used when fetching records.

can :manage, :all, :visible => true

Does that make sense?

sekrett commented Feb 11, 2011

Hmm, I'll try to improve the documentation on this. Any suggestions on making it clearer?

Of course. I have some ideas, but will try later to write some text about it.

The default behavior is to deny everything unless you give permission.

Absolutely right! But let's see your documentation:

Think of it as asking "can the current user read a project?". The user can read a project, so this returns true.

It's a difficult case, you don't have an object, you really don't know, if to give permission or not. And you decide to give that permission. The question is: why? My purpose is to ignore such rules. You don't know, so do nothing. And that action will be denied by default at the end of ability definitions.

Sorry my bad english.

Owner

ryanb commented Feb 11, 2011

Let's say you have this ability.

can :manage, Category, :visible => true

This is what would happen in the index action with load_and_authorize_resource.

authorize! :index, Category
@categories = Category.accessible_by(current_ability)

That will end up only fetching the visible categories, which is the correct behavior. If it defaulted to denying access the first authorize! call would always make this fail so we would not be able to see the visible categories.

Also, what if we have a New Category link and only want to show it if they can create categories:

if can? :create, Category

Currently this returns true because here they do have permission to create a category, but only if it matches certain attributes. We don't know those attributes until they try creating it. If this returned false then there would be no way we can perform a generic check like this.

Whenever a check on a class is performed, it should always be followed up with a check on the instance when that is available. That is either through accessible_by to only fetch records which match the conditions, or inside the create action when attributes are present.

Whenever you have a can call in the Ability model you are granting permission. The block or hash conditions only narrow down what objects that permission applies to. If the check on the class always failed then that would make it pretty much useless because you would always need an instance to check against, but sometimes you don't have an instance.

Does that make sense?

sekrett commented Feb 12, 2011

First of all subject of this issue has changed, if it is not clear. It's true, that I can't use block { user.is_admin? }, because block should contain a database object, as it is mentioned in wiki pages.

I understood that returning false is a bad idea. Later I said that CanCan should ignore (or skip) that rule only in that particular cases, when it is impossible to execute block or evaluate a hash.

Returning true may be convenient, as you say, but it is a security hole, maybe even a little hole. I don't know your goals, some people like strict rules, others prefer comfortable environment.

If you implement this change, people will have to add one more rule to be absolutely clear, that index action is really permitted. But it will be more correct and secure. People should learn to write correct rules until they get the desired effect, CanCan should not guess "well, he wants to see some records, probably I should permit to see all".

Are you afraid it is hard for people to add more rules to be more specific?

Owner

ryanb commented Feb 14, 2011

Can you provide a concrete example on how adding more specific rules would resolve this problem? I'm open to changing how this works if there is a solution which makes sense to the majority and improves security.

I don't see the current implementation as a security hole, but more of a misunderstanding in how Ability blocks work. If the ability block is only used for defining permissions with the object attributes it shouldn't open any security issues.

At the very least this should have better documentation. Thanks for bringing this to my attention.

sekrett commented Feb 14, 2011

An example is easy:
can :manage, Project, :public => true
can :index, Project

You're right, if one understands and strictly follows the documentation, it is wrong to say that there is a security hole. You have warned in documentation but it's a very very uncommon feature.

Ok, let's see if anybody else want this change.

Owner

ryanb commented Feb 15, 2011

Ok, I'll add the discussion tag to this and hopefully more people will chime in.

Owner

ryanb commented Feb 15, 2011

The problem with the above two rules is that it's not clear what permissions you are setting on the :index action. The above rules make it look like one can access private projects in the index action. So what should the accessible_by call return?

authorize! :index, Project
@projects = Project.accessible_by(current_ability)

In this case it would return private projects as well which I don't think is the behavior you want.

sekrett commented Feb 15, 2011

I meant that index is allowed for all projects: you can list, but can show only the public ones. Yes, it may be not what I really want, BUT I would get EXACTLY what I specified in Ability class. If I need to limit index action, I will add a default_scope in my model, and I would have a clear understanding of what I'm doing. It's more code but no guessing "ok, let's permit everything".

Owner

ryanb commented Feb 17, 2011

If I need to limit index action, I will add a default_scope in my model, and I would have a clear understanding of what I'm doing.

I don't see how a default_scope would work here. You need the behavior to be dynamic based on the current user permissions which default_scope cannot do. This also means permissions are spread across models and not all in the Ability class.

Owner

ryanb commented Mar 28, 2011

I am closing this because it will not be as big of an issue in CanCan 2.0. That version will raise an InsufficientAuthorizationCheck exception if a block is given and the action doesn't check on an instance. This way access cannot easily be granted where it isn't intended.

ryanb closed this Mar 28, 2011

@yan13to yan13to pushed a commit to paupauorg/cancan that referenced this issue May 30, 2017

@Senjai Senjai Merge pull request #275 from bambycha/bug/pass-name-as-symbol
Fix attempt to #constantize a symbolic name
fbff9a1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment