Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Ensure CanCan is called #135

Closed
ghost opened this Issue Aug 30, 2010 · 27 comments

Comments

Projects
None yet
5 participants
@ghost

ghost commented Aug 30, 2010

Ryan,

One of my coworkers was testing authorization in the controller. I said "You don't need to do that, the Ability file handles all of it", but then I got to thinking "Hmm, what if the CanCan methods are never called in the controller...?"

So, what do you think about white listing controllers as "safe"? Safe as in they do not require authorization.

My branch gives the details:
http://github.com/justinko/cancan/tree/cancan_safety

Let me know what you think.

Owner

ryanb commented Aug 30, 2010

That's an interesting idea, but I don't think it should be default because one may have a HomeController or InfoController which does not follow the REST pattern and I don't want to require every user add cancan_safe just to get CanCan working in their app. (edit: I see now you didn't intend for it to be default, you can ignore this paragraph.)

That said, I can see the benefit of this feature if one enables it manually to ensure every controller is authorized. Currently you can do this by checking the existence of the @current_ability instance variable.

# application_controller.rb
after_filter :ensure_cancan_used
def ensure_cancan_used
  raise "CanCan not used in this controller action." unless instance_variable_defined?(:@current_ability)
end

Will that work for your case? I think this would make a nice wiki tip.

@ghost

ghost commented Aug 31, 2010

My code needs some work, it has some issues. I was very tired when I wrote it.

hmmm..... I don't think unless instance_variable_defined?(:@current_ability) will work because some controllers, like "home" or "info" will not call authorize_resource, therefore @current_ability will never be defined.

And as you caught, ensure_cancan_safety! allows this feature to be optional.

Can you think of another way to do this without adding code? I cannot.

P.S. load_resource would not add @_cancan - don't know what I was thinking...

Woops, was logged into the wrong github account.

Owner

ryanb commented Aug 31, 2010

The authorize! method should bypass the safety message, this way manual authorization works as well.

I'll give this some thought and see if there are any alternatives ways to handle this. In the meantime I'd like to hear feedback from others as well.

Owner

ryanb commented Sep 3, 2010

adding check_authorization and skip_authorization controller class methods to ensure authorization is triggered (thanks justinko) - closed by 1af6c6f

voxik commented Sep 27, 2010

Unfortunately this doesn't work with engines ...

Owner

ryanb commented Sep 27, 2010

@voxik, do you mean that engines will inherit this behavior so they will always fail? If one is using engines I think it's worth creating a subclass of ApplicationController that all local controllers inherit from. Here you can place code that you want to effect your app without effecting engines.

voxik commented Sep 27, 2010

Yes, that is exactly what I meant. If I will follow your advice, then the advantage of check_authorization method is lost again and you should test it again if you did not forget. But something like

check_authorization :except => SomeEngineController

might solve the issue better from my point of view.

Owner

ryanb commented Sep 27, 2010

@voxik, would this work?

SomeEngineController.skip_authorization

You could place this anywhere after the engine is available, maybe in an initializer.

voxik commented Sep 27, 2010

Sounds good for me

Owner

ryanb commented Sep 27, 2010

It should work at the moment, try it out and let me know if not.

voxik commented Sep 28, 2010

It doesn't work for me. Actually the mentioned engine is Devise, so what I am tried to do is adding:

Devise::SessionsController.skip_authorization

at the end of devise.rb initializer but without success

Owner

ryanb commented Sep 29, 2010

Strange. Is it reporting an error or just not skipping the authorization?

voxik commented Sep 29, 2010

No error ... I just doesn't work.

Owner

ryanb commented Sep 29, 2010

I'll re-open this issue and look into it some more. Thanks for reporting.

Owner

ryanb commented Oct 4, 2010

@voxik, adding that line to the end of my devise.rb initializer file is working for me. Are you certain it is not working for you? Be sure to restart the server after adding it.

I'm seeing the same issue--- Devise::SessionsController.skip_authorization added to the end of devise.rb with no effect-- I still get an "uninitialized constant Session" exception. CanCan 1.4, Devise 1.1.3, and Rails 3.0

Owner

ryanb commented Oct 10, 2010

@msaffitz, did you try restarting the server after adding this?

yes. I tried several restarts of the server.

Owner

ryanb commented Oct 11, 2010

@msaffitz, I just realized you are getting an uninitialized constant Session error, it sounds like this is due to CanCan trying to load the Session model in that controller?

The skip_authorization call is only intended to skip the check_authorization, not the load_and_authorize_resource. I realize the confusion here so I'll probably change the name to skip_authorization_check or similar.

See issue #164 regarding skipping the actual authorization.

voxik commented Oct 18, 2010

@ryanb: actually you are right, it works, but just for the first time after the server restart :) So it is not very useful ...

Owner

ryanb commented Oct 19, 2010

@voxik, hmm, I wonder why it's resetting it after the first request. Maybe it clears out the before filters when it reload the class in Development? Hmm, I'll have to research it. If someone has ideas please let me know.

voxik commented Nov 7, 2010

Yes, the reloading is the issue. The Devise initializer is called just once, therefore the Devise::SessionsController is mokeypatched just for first run. Later on, the class is dropped and reloaded from the file again, without the monkeypatching.

Placing

module Devise
  class ConfirmationsController < ApplicationController; skip_authorization; end
  class PasswordsController < ApplicationController; skip_authorization; end
  class RegistrationsController < ApplicationController; skip_authorization; end
  class SessionsController < ApplicationController; skip_authorization; end
  class UnlocksController < ApplicationController; skip_authorization; end
end

at the end of application_controller.rb solved the issue. I'm not sure if there is cleaner solution but this seems to work for me.

koalo commented Dec 26, 2010

This is not working for me. There is no problem with the actions of the Devise controllers, but when I execute another action from another controller it says:

superclass mismatch for class ConfirmationsController

Any idea what is going wrong?

voxik commented Dec 26, 2010

Actually, you are right. I am using following method now:

before_filter {|controller| controller.instance_variable_set(:@_authorized, true) if controller.devise_controller? }

It is not sexy, but it works.

Owner

ryanb commented Jan 28, 2011

Closing this issue because what was addressed in the original post is solved, it's just the side effect of not having a good place to override behavior in engines which is keeping this open.

If someone is still having this issue, please post a separate issue about it so we can better address that specific topic.

koalo commented Feb 10, 2011

voxiks approach works

This issue was closed.

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