Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Use of after filters to ensure condition checking #334

Open
thatothermitch opened this Issue · 8 comments

6 participants

Mitch Williams Ryan Bates Miguel Gomez Ivgeni David Lee xhoy
Mitch Williams

Cancan 1.x and 2.x both seem rely on after filters to blanket-ensure authorization in check_authorization/enable_authorization respectively.

It seems to me that running this sort of check in an after isn't really safe. At most, you can prevent information form being returned in the HTTP response, but any side effects of the action itself (i.e. modifications to the database) have already taken place.

Ryan Bates
Owner

This check is intended to only be used in development and should never be reached in a production application. It is there primarily as a guide saying the authorization needs to be improved in that area.

You're right though, ideally this would be triggered before the request takes place, but I can't think of a way to do this without forcing all authorization to happen at a specific moment before the request, and I would rather not do that to keep the flexibility of using the authorize! method directly in the controller action.

I'll add a discussion tag to this to get feedback from others to see if there are any other concerns with the current solution or ideas on how to solve it.

Update: I don't think this is as big of a concern in CanCan 2 because there the basic controller authorization always takes place. The only things that are checked in the after_filter are cases where instance conditions/attributes aren't fully considered. That case isn't even caught with check_authorization in CanCan 1.

Mitch Williams

"I don't think this is as big of a concern in CanCan 2 because there the basic controller authorization always takes place. The only things that are checked in the after_filter are cases where instance conditions/attributes aren't fully considered. That case isn't even caught with check_authorization in CanCan 1."

Agreed, but in many cases I find that the object-instance level conditions are a fantastically useful way of enforcing permissions. It would be great to ensure that these checks aren't absentmindedly skipped.

To accomplish this I ended up creating authorize, a custom controller class-level method that works a little like load_and_authorize_resource. The authorize method, however, provides a more explicit way of specifying the procedure for loading resource instances, and forces users to explicitly whitelist actions for authorization, as this particular combination was useful in my project due to some deviations from restful patterns:

# evaluates the block, the authorizes and assigns result to @foo in a before_filter for actions 
# specified in the :actions key
authorize :instance => :foo, :actions => [:show, :update, :edit, :destroy] do
  Foo.find params[:id]
end

# Authorizes against explicit subject, no need for a block
authorize :subject => Foo, :actions => :index

# Bypasses authorization, allowing true public pages, or the freedom to authorize! elsewhere.
authorize :bypass => true, :actions => [:public, :custom]

I then implemented a test which loads the controllers, reflects on them to determine which methods they expose as actions (reflecting on the routes might be another option), and analyzes the set of authorize calls in a controller (and its ancestors) to determine whether or not all actions have been covered. If there's an authorization hole, you get a test failure. In practice, I found that this type of test compliments the unit-tests for one's Ability class quite nicely.

I know this may not be the most typical use case for CanCan, but I'd love to hear your thoughts on this approach, and if and/or how this type of test might figure into CanCan 2

Ryan Bates
Owner

@thatothermitch, thanks for the feedback. Have you tried out the implementation in the 2.0 branch? I've been using it in a project of mine and have found it works very well.

The way CanCan 2.0 works is authorization is triggered in every action. This means you don't have to worry about managing it in the controller level. If there is ever a time enough information isn't passed to do authorization then an exception will be raised.

This means authorization can be entirely covered by unit testing on the Ability. This frees up the functional and integration tests to not have to worry about every role/action branching possibility and it works very well. I encourage you to give it a try if you haven't already.

Ryan Bates
Owner

I've been thinking about this, and realize that while CanCan 2.0 does improve this greatly, I would like to get this problem fixed because having the authorization ensured in an after filter does leave the potential to accidentally grant access to something which the user shouldn't have. As you pointed out, even though it raises an exception later the damage is done. Thank you for bringing this to my attention.

I think with the way CanCan 2.0 works we won't need to use authorize! much at all, so custom authorization could be done in an authorize block like you showed. What I like about this is that it means we would no longer need to do the exception raise/rescue magic. If one is unauthorized it can render/redirect directly in that before filter. It always bugged me that we are raising an exception to handle a common situation.

I am marking this to be addressed in version 2.1, this way I can test the waters more with 2.0 and have a better idea on how to handle this.

Miguel Gomez

We are building on the 2.0 branch (now 1.6.3) and needn't use authorize! at all until now. I find this usage of authorize a great idea. Kind of giving more options to load_and_authorize_resource, so that authorization becomes absolutely encapsulated and easily testable, both for models and controllers.

Ivgeni

The method to authorize here could actually be part of a more generic library that deals with loading resources. Ultimately, the logic to load the resource does not HAVE to be part of the authorization process, however, such logic should enable authorization as a hook.

David Lee

@ryanb If check_authorization is "intended to only be used in development and should never be reached in a production application", then shouldn't that be clearly noted in the README? I was reading through the README and had a hard time trying to see the purpose of check_authorization.

xhoy

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.