Parent resource may be overwritten by initial_attributes #947

HermanHiddema opened this Issue Sep 30, 2013 · 0 comments

1 participant


I ran into a rather obscure issue. The behaviour I encountered may have been by design, but it seems counter-intuitive.

I have a company model:

class Company < ActiveRecord::Base
  has_many :users, :dependent => :destroy

and a user model:

class User < ActiveRecord::Base
  belongs_to :company

Some users can have role owner (of a company), and in ability.rb, I have:

if user.has_role? :owner
    # company owner can manage his own company and users
    can :manage,      User,     :company_id => user.company_id
    can :manage,      Company,  :id => user.company_id

Additionally, some users are admin:

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

So an admin should be able to create users for any company.

In my controller, I use load_and_authorize_resource, it looks like this:

class UsersController < ApplicationController
  load_and_authorize_resource :company
  load_and_authorize_resource :user, through: :company, shallow: true

  def show

  def index

  def new

  def create
      redirect_to company_url(, anchor: 'users')
      render :action => 'new'

When, as an admin, I visit the path /companies/123/users/new, where I render a form with simple_form_for [@company, @user] I get the correct form, which has action /companies/123/users/ and method post. When submitting the form, the user is created with the content of the form, but the user belongs to company 1, the company of the admin user, not to company 123.

After digging into the CanCan code, it seems that this is the result of the assign_attributes method in /lib/cancan/controller_resource.rb. First, the parent resource is set, then the initial_attributes are set.

Since the initial_attributes are based on ability.rb, and since my admin user also has the owner role, @user.company_id is overwritten with current_user.company_id, which caused my issue.

I was able to fix this by changing my ability.rb to redefine the owner's ability to manage users like this:

can :manage, User, :company => { :id => user.company_id }

Now, company_id is no longer overwritten.

But it still seems counter-intuitive that a form post to /companies/123/users/ could result in a new user on company 1.

I think (but have not checked) that this would also mean that theoretically the owner of company 234 could post to /companies/123/users/ to create a new user on his own (234) company, since the authorize! call will succeed if the property is overwritten. If someone runs into a similar issue and fixes it by adding code like @user.company_id = params[:company_id] as the first line of their create action, this would actually create a security hole, as that happens after the authorize call. In that case, people could create child resources on the wrong parent.

The easy fix, it seems, would be to simply change the order of these operations in assign_attributes. First assign initial attributes, then the parent. If the user is then built with the correct parent resource, the authorize call would also work correctly. My insight into the code is not good enough, however, to see whether this would have unintended other side-effects.

Anyway, anyone who runs into a similar issue can use the fix I used above, and perhaps someone with better insight into CanCan's code can determine whether this needs a code fix or not.

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