ryanb / cancan

Authorization Gem for Ruby on Rails.
MIT License
6.26k stars 782 forks source link

How does CanCan handle mass-assignment? #571

Open ianmurrays opened 12 years ago

ianmurrays commented 12 years ago

Given all the fuss about mass-assignment in rails, I've been wondering how CanCan handles this. What do we need to be concerned about?

Didn't think of any place better to ask this. Thanks!

skarpesh commented 12 years ago

I think nohow.

ianmurrays commented 12 years ago

The thing is, of CanCan does something like this for create actions for example:

@resource = Resource.new params[:resource]

even if we overwrite params afterwards (and after deleting all dangerous params), those would still have been already set by CanCan. Am I wrong about this?

Thanks!

travisp commented 12 years ago

You are not wrong ianmurrays, cancan will assign them and does not provide a way to filter parameters in the controller (a big flaw, IMHO). You can help protect by protecting your model against mass assignment (attr_accessible and attr_protected), but it would be good to be able to do this in the controller, or have cancan use santitizer_for_mass_assignment

mrkcor commented 12 years ago

You can add a before_filter in your controller before the load_and_authorize_resource call to slice out unwanted params beforehand

mcbsys commented 12 years ago

With Rails 3.2.3, if you set config.active_record.mass_assignment_sanitizer = :strict and try to mass-assign an attribute that is not listed in attr_accessible, CanCan's default #create will fail with a MassAssignment error because you're passing the inaccessible parameter to Resource.new params[:resource].

I discovered this because I have an owner_id that is not in attr_accessible but that certain users can change, i.e. it must appear in the view and be custom-assigned by the controller based on certain conditions. To get that to work, I had to suppress CanCan's load_and_authorize_resource for the create action:

load_and_authorize_resource :except => :create

In the create action, I had to store the parameter, delete it from params, then manually load and authorize the resource:

owner_id_param = params[:contact][:owner_id]
params[:contact].delete(:owner_id)       # remove param to avoid mass assignment error
@contact = Contact.new(params[:contact]) # manual load now that owner_id removed from params
authorize! :create, @contact             # manual CanCan authorization

After that I check conditions and optionally change owner_id.

skarpesh commented 12 years ago

https://github.com/roundlake/heimdallr https://github.com/roundlake/heimdallr-resource It's very new solution. I didn't use it. But developers say Heimdallr secures your model level and Heimdallr-Resource secures your controller level and works very much like CanCan (with load_and_authorize_resource, ect).

derekprior commented 12 years ago

@mcbsys have you considered instead adding owner_id to attr_accessible and then doing as dhh suggests? Your post_params method would remove owner_id except when your conditions are met.

I'm tagging this issue as a documentation issue. Given the somewhat recent dustup around mass assignment, I think it would be good to have a doc outline what role CanCan plays and how to work around some of the issues discussed here.

Any more opinions?

mcbsys commented 12 years ago

@derekprior, thanks for the suggestion. Yes, if I put owner_id in attr_accessible, I wouldn't need the manual CanCan load and authorize. I could still use the params[:contact].delete(:owner_id) to get rid of the parameter I don't want updated, or I could follow dhh's params[:contact].slice(<param list>) approach. The latter is more consistent with whitelisting allowed params, which seems to be preferred by security folks. But it seems error-prone and not very DRY to have to maintain slightly different lists of updatable params in both the controller (slice) and the model (attr_accessible).

It would be nice if CanCan's load_and_authorize_resource, for the create action, would parse out attribute(s) that are not attr_accessible before calling .new. CanCan should store the deleted attributes in local variables named after the attributes, or perhaps in an inaccessible_params hash, so the programmer could use them in manual updates if desired.

mcbsys commented 12 years ago

Just saw that the "slice pattern" referenced in @derekprior's post is already a done deal for Rails 4.0: Strong parameters: Dealing with mass assignment in the controller instead of the model. So with 4.0, attr_accessible will no longer be in the model, access will controlled only in the controller, and my DRY concern will go away.

mcbsys commented 12 years ago

Okay I've now implemented the strong_parameters gem to move authorization from the model to the controller. It takes as much or more work to get around CanCan (though I didn't try mckeed's before filters). Plus I miss the errors raised by mass assignment when it's not allowed--I wound up raising my own as a CanCan error.

I still have to suppress CanCan's load_and_authorize_resource for the create action:

load_and_authorize_resource :except => :create

In the create action, I load the special "sliced" contact_params, then manually authorize the resource:

@contact = Contact.new(contact_params)   # manual load
authorize! :create, @contact             # manual CanCan authorization
# If owner_id param was authorized, use it; otherwise set to current_user.id
@contact.owner_id ||= current_user.id    # assign if @contact.owner_id==nil

Update is handled without override because it doesn't use contact_params until after CanCan does its load_and_authorize:

def update
  # CanCan:  @contact = Contact.find(params[:id])
  if @contact.update_attributes(contact_params)
  ...

The conditions for using owner_id move into the contact_params method. Seems awfully complex but not sure how to clean it up:

private
def contact_params
  # Check if owner_id change requested and allowed
  owner_id_param = params[:contact][:owner_id]
  # when creating record, @contact doesn't exist yet; assume current_user is owner
  current_owner = @contact || current_user 
  if ( (!owner_id_param.blank?) && (owner_id_param != current_owner.id) )
    # owner_id change requested
    if ( (current_user.role_atleast? :account_admin) &&
         current_user.account.users.find(owner_id_param) )
      # The owner_id provided exists as a user in the current user's account
      params.require(:contact).permit(:first_name, :last_name, :email, :owner_id)
    else
      raise CanCan::AccessDenied.new("Not authorized!", :update, Contact)          
    end
  else
    # no owner_id change requested
    params.require(:contact).permit(:first_name, :last_name, :email)
  end
end
tetherit commented 12 years ago

@mcbsys thank you for that post, but isn't it easier to just remove cancan altogether then? - Looking at Ryan Bates' 371 Strong Parameters screencast it seems implementing the cancan logic from scratch with strong_parameters might be easier than trying to make strong_parameters work with cancan? - or am I missing something?

mcbsys commented 12 years ago

@hackeron, I've actually decided not to use strong parameters for now. It seems too undeveloped, especially the support for nested parameters, so I'm back to my approach posted 4/30/2012 above (still need to explore @mkremer's idea of using before filter instead).

Re. removing cancan, that might not be a big deal if you only have one authorization level. I have three levels of users, and being able to manage which user levels have which kind of access access to which models, all in a central place, is something I wouldn't want to do without.

tetherit commented 12 years ago

@mcbsys seems strong parameters support nesting now have a look at Ryan B's latest screencast. Going to try to implement my permission logic as he shows there, seems very clean and complete :)

the8472 commented 12 years ago

My personal approach to this is to use cells and let the cells do the mass assignment where each cell acts as a separate role for mass assignment security. Then I just grant access to cell classes with cancan.

I.e. I'm working on a component-base instead of individual fields.

rewritten commented 12 years ago

This is enough to overcome the limitation (put it before the load_and_authorize_resource):

  before_filter do
    params[:user] &&= user_params
  end

so when CanCan retrieves the params, they already pass your field-based filtering.

wojtha commented 12 years ago

I've resolved that like this:

class SitesController < ApplicationController
  before_filter :insecure_params_filter
  load_and_authorize_resource

  # ....

  def create
    assign_protected_group(insecure_params)
    @site.user = current_user
    respond_to do |format|
    # ....
  end

  def update
    assign_protected_group(insecure_params)
    @site.user ||= current_user
    respond_to do |format|
    # ....
  end

  private

  def assign_protected_group(params)
    if params.has_key?(:group_id)
      group = Group.find(params[:group_id])
      authorize! :read, group
      @site.group = group
    end
  end

  def insecure_params
    @_insecure_params
  end

  def insecure_params_filter
    model = 'site'
    attrs = ['group_id', 'user_id']
    @_insecure_params ||= {}
    if params && params[model].kind_of?(Hash)
      @_insecure_params = params[model].select{ |k| attrs.include?(k) }
      @_insecure_params.each{ |k,v| params[model].delete(k) }
    end
  end
end

You can try to parametrize the filter using proc or block as well.

cdale77 commented 11 years ago

I was having the worst time getting scoped mass assignment to work with my create action. Thank you mcbsys!

I was able to solve the problem in a simpler way, however. I'm a noob, though, so I might be making a mistake.

CanCan limits my User create action to only admin. So anybody making a user should be able to assign protected attributes.

Per mcbsys, I did this:

load_and_authorize_resource :except => :create

Then, just:

@user = User.new(params[:user], :as => :admin)
authorize! :create, @user
if @user.save
  flash[:success] = "Successfully created new user."
  redirect_to @user
else
  render 'new'
end

I didn't stash the protected params separately.

yonkeltron commented 11 years ago

I can confirm this issue comes up for me for update as well using Rails 3.2.11 with CanCan 1.6.9 and strong_parameters 0.1.6. Can I provide more information to help in the diagnosis of this issue and/or get a status update?

Crystark commented 11 years ago

Using rails4 and CanCan 1.6.9, @rewritten's workaround worked for me.

  prepend_before_filter do
    params[:user] &&= user_params
  end

I had to use prepend_before_filter as i'm calling load_and_authorize_resource in my ApplicationController

ansonhoyt commented 11 years ago

Since most of my controllers use load_and_authorize_resource, I placed @rewritten's workaround into ApplicationController to DRY things up, like thus:

class ApplicationController < ActionController::Base

  # Apply strong_parameters filtering before CanCan authorization
  # See https://github.com/ryanb/cancan/issues/571#issuecomment-10753675
  before_filter do
    resource = controller_name.singularize.to_sym
    method = "#{resource}_params"
    params[resource] &&= send(method) if respond_to?(method, true)
  end

end

I have skip_authorization_check on the few controllers that don't have a resource_params method.

Edit: As @Crystark mentions, updating to work for private #{resource}_params. I'd made this change locally so thanks for raising that.

Crystark commented 11 years ago

I modified @ansonhoyt's code for this to work with private #{resource}_params methods. See respond_to? params.

  before_filter do
    resource = controller_name.singularize.to_sym
    method = "#{resource}_params"
    params[resource] &&= send(method) if respond_to?(method, true)
  end
sstarr commented 11 years ago

@rewritten, @ansonhoyt & @Crystark - Thanks a lot for this; I've just started using strong_parameters & CanCan together for the first time and getting them to play nicely has been driving me crazy. This workaround is the neatest solution I've found :relieved:

shoutsid commented 11 years ago

+1 to merge, thanks for the work around guys.

scaryguy commented 11 years ago

Workaround above works BUT only when controller and model exist and both are properly named. This breaks things when you want to use a controller without a model.

Say I have a controller named menu_controller.rb . If I don't have a model named Menu, above codes breaks application with this error:

NameError in Admin::MenuController#index
uninitialized constant Menu
zamith commented 11 years ago

What about not doing load_and_authorize_resource but only authorize_resource? You can load stuff when you need to and call the authorize! method.

armahillo commented 11 years ago

I've been encountering this issue on my app. I've found a basic solution for now, but it should be generalized to make it a little more elegant.

I have a "manage" namespace that is protected by CanCan, and within that namespace I have a "ManageController"

class Manage::ManageController < ApplicationController
  load_and_authorize_resource :except => [:create, :update]
  before_filter :strong_params, :only => [:create, :update]

protected
  def strong_params(form_key = nil, *permitted_keys)
    return if form_key.nil?
    @strong_params = params.require(form_key.to_sym).permit(*permitted_keys)
  end
end

All of my manage namespace controllers inherit from this one. The "strong_params method is intended to be overridden on the individual controllers, like this, in the users_controller:

class Manage::SlidesController < Manage::ManageController
  # ... stuff ... 
 def create
   @user = User.new(@strong_params)
   respond_with do |wants|
      if @user.save
        flash[:notice] = "You may pass"
        wants.html { redirect_to(manage_users_path) }
      else
        flash[:error] = "Agggghhhhhh!"
        wants.html { render :action => "new" }
      end
    end
 end

  def update
    @slide.update_attributes(@strong_params)
    respond_with do |wants|
      wants.html { redirect_to edit_manage_slide_path(@slide) }
    end
  end

protected
  def strong_params
    super(:user, :name, :quest, :favorite_color)
  end
end

Right now, the "strong_params" method needs to be added manually into each controller. IIRC, @ryanb was working on some way of working in the permit hash into the load_and_authorize_resource line? There's probably a better way to do this, I only just resolved the mysterious error yesterday, so it still needs tweaking. :)

Disha-Shah commented 10 years ago

@rewritten Thank you so much! Your solution worked like a miracle! :)

r4m commented 10 years ago

If could be useful, I've adopted another solution that I briefly describe here.

xhoy commented 10 years ago

Thanks for your submission! The ryanb/cancan repository has been inactive since Sep 06, 2013. Since only Ryan himself has commit permissions, the CanCan project is on a standstill.

CanCan has many open issues, including missing support for Rails 4. To keep CanCan alive, an active fork exists at cancancommunity/cancancan. The new gem is cancancan. More info is available at #994.

If your pull request or issue is still applicable, it would be really appreciated if you resubmit it to CanCanCan.

We hope to see you on the other side!