usage of singleton does not authorize correctly #304

Closed
jensb1 opened this Issue Mar 14, 2011 · 10 comments

Projects

None yet

2 participants

@jensb1

Sorry if this is a bit messy, but don't have so much time here. Hope it makes sense.

When the object was created in the restful controller (a post from new to create) everything was fine. BUT after this, and I clicked on a link (or entered in the URL) the /new path again, the ability didn't fire (even though I specied all abilites explicitly and the customer.activation was reset to nil (and saved!)!

what i had was something like:

ability:

can :show, Activation do
    // only possible if user has activation
end
can :new, Activation do
    // false if user has activation
end

controller:

  load_and_authorize_resource :customer
  load_and_authorize_resource :activation, :through => :customer, :singleton => true

  def new
  end
  def show
  end
  def create
    # create the activation object (to requested)
  end
@ryanb
Owner

Hmm, I think it's setting the @activation instance variable to nil which means it's going to be checking only on the Activation class which is why it skips the block. But why is it setting the @activation instance to nil? I'm not sure exactly. Sounds like a bug but I'll need to do some more research.

@jensb1

Ok, I didn't know it only checked the class if the instance variable was set to nil. The reason it may happen is because I am referencing an activation object that doesn't exist.

But that may also explain my problem. Because when I access an activation reference (id) that doesn't exist, I would expect the outcome would be that I am not authorized. Correct?

Instead it is possible to bypass the check completely.

@ryanb
Owner

But that may also explain my problem. Because when I access an activation reference (id) that doesn't exist, I would expect the outcome would be that I am not authorized. Correct?

I'm a little confused, are you going to the showaction to access an activation which doesn't exist?

Internally CanCan does @customer.activation to fetch the singular activation which returns nil and explains your problem. Probably the best thing to do would be to return a 404 response.

def show
  raise ActiveRecord::RecordNotFound if @activation.nil?
end

Does that have the behavior you want?

@jensb1

Yes, I assumed so, but you may expect it to use the Activation#find so you get the same behaviour as when singleton is false.

But anyway, another problem I described is when you go to the #new action, why does that reset the @customer.activation to nil? That seem stranger to me.

Does CanCan internally reset the object somehow?

@ryanb
Owner

CanCan internally calls @customer.build_activation on the new action which I didn't think changed the database. Is it saving the nil state to the database? If so that is very strange behavior.

I don't work with has_one associations much. I guess Active Record doesn't like it when calling build_activation when there's an existing one already. Can you do some experimenting in the console to see if that assumption is correct? Also try Activation.new(:customer => @customer). If that doesn't mess with the database I'll change it to use that instead of build_activation.

@jensb1

Customer.find(2).prescription_activation
#PrescriptionActivation:0x10473ccd8 {
:id => xxx
}
Customer.find(2).build_prescription_activation
#PrescriptionActivation:0x10468f4e8 {
id => nil
}

c = Customer.find(2)
c.reload
c.prescription_activation
nil

so, yes it seems that build is not a very good idea...

@jensb1

Stupid system for the layout, but I hope you get the idea,

xxx.build_activation

removes the activation and saves to database.

@jensb1

I don't know why this happened, because when I tried to replicate it again build_ didn't remove the activation. So I am out of suggestions... :/

@ryanb
Owner

Thanks for reporting this and researching. I'll try to duplicate it in a test and add a fix soon. In the meantime you'll need to build the record manually.

load_and_authorize_resource :customer
before_filter :build_activation, :only => [:new, :create]
load_and_authorize_resource :activation, :through => :customer, :singleton => true

private

def build_activation
  @activation = Activation.new(:customer => @customer)
end
@ryanb
Owner

use Item.new instead of build_item for singleton resource so it doesn't mess up database - closed by 3f6cecb

@mphalliday mphalliday pushed a commit that referenced this issue Oct 11, 2011
@ryanb use Item.new instead of build_item for singleton resource so it doesn…
…'t mess up database - closes #304
3f6cecb
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment