ryanb / cancan

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

Destroy action forces logout and cannot destroy resource. #382

Closed ekampp closed 13 years ago

ekampp commented 13 years ago

Hi there.

I have set up devise and CanCan. And the index, show, new, create, edit and update methods are working fine, but for some reason the destroy method is breaking the authorization chain. For some reason CanCan is denying the admin (with :magage :all abilities) to destroy a record.

I have tried to simply display the can? :destroy, resource to se if i (logged in as the admin) would have access to destroying the resource (I trust that the view can? and the can? that load_and_authorize_resourceeventually calls are the same?!) and it seems that I should be able to destroy the record.

Is there any simple explenation for this, or is there something broken? I haven't been able to find any indications of similar problems online. So bare with me if the issue has a simple solution that I haven't found.

Best regards Emil

ekampp commented 13 years ago

I have tried to change the permission of any user to can :manage, :all and now the resource is deleted, but the user is still logged out afterwards.

ready4god2513 commented 13 years ago

I noticed that you have :magage. That might be affecting it.

ekampp commented 13 years ago

I just rechecked. I don't have that in my Ability class. So that was a typo when writing up the issue.

ryanb commented 13 years ago

CanCan isn't doing anything special when destroying records. I don't see any reason why it would be logging the admin out unless that is part of the behavior when failing authorization.

Try removing the load_and_authorize_resource from the controller temporarily and see if it still logs the admin out. If not try adding this to an action.

raise CanCan::AccessDenied

and see if that has the same logout behavior.

ekampp commented 13 years ago

If I remove the load_and_authorize_resource from the controller, then it raises the AuthorizationNotPerformed error. So I assume that I need to also write in the skip_authorization_check in the same controller?

If the above is the case, then it still logs me out after destroying an object. Good catch. I guess that means that it's something with devise?

ekampp commented 13 years ago

Further information: I have tried to put the load_and_authorize_resource back into the controller, and the logout problem persists (as expected) but also I'm still not allowed to destroy records.

def initialize(user) user ||= User.new

if user.account_admin? can :manage, :account end if user.admin? can :manage, :all else can :read, :all end

ekampp commented 13 years ago

I found something more. It turns out that I did have the AccessDenied in my ApplicationController. And that it did redirect to my login path each time it would deny someone access. And in turn devise logs out any user accessing the login-form. So it turns out that CanCan is denying the admin (with the :manage, :all-ability) permission to destroy records.

ryanb commented 13 years ago

Try Debugging Abilities in the console and see if you can duplicate this behavior. If it works there then perhaps the current_user isn't being passed through properly on the controller side. Hmm.

ekampp commented 13 years ago

Ok. I tried that, and the ability.can?(:destroy, project) returned true, so it seems that this isn't the problem. But how would I check if the current_user is parsed probably? Where in the CanCan code is the Ability.new(current_user) called? So I can work from there?

ekampp commented 13 years ago

After some trawling I found that you'r right. The user session has stopped existing at the time the can? method was called. I fixed this by adding the csrf-tag to the html view. This was apparently a thing about clearance.

ryanb commented 13 years ago

Glad you got it working. In case anyone else comes across a similar issue, the Ability.new(current_user) code is inside the ControllerAdditions#current_ability module in CanCan. You can override current_ability in any controller to change the behavior and test it there.

dankozlowski commented 13 years ago

Awesome. I have been struggling with this issue for several hours; the title should be appended with: "and cannot destroy resource".

ryanb commented 13 years ago

@dankozlowski changed the title, thanks for the suggestion.