Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

nested resources won't authorize index action #301

Closed
sethvargo opened this Issue · 37 comments

8 participants

@sethvargo

I have the following setup:

# routes.rb
resources :organization
  resources :memberships
end

I'd like the following behavior: A user can only view his or her membership, and a user can only access the :index and :show actions of an organization.

I've defined by ability.rb as follows:

# ability.rb
...
can :read, Organization
can :show, Membership do |m|
  m.user_id = user.id
end

This works fine, as expected. An exception is raised when I try to visit the /organizations/:organization_id/memberships Now, however, I want to allow a user to display the :index action for the memberships if the user is an officer. I added the following to my ability.rb:

# ability.rb
...
can :index, Membership do |m|
  user.is_officer?(m.organization)
end

However, now everything breaks... Any user (regardless of whether they are an officer or not) can access the index action. I've tested my is_officer? method, and it works. Furthermore, it seems like nothing in the block actually gets evaluated. I tried all of the following variations, and, as long as the can :index... is defined, whatever is in the block is completely ignored:

can :index, Membership do |m|
  false
end

can :index, Membership do |m|
  m == nil && m != nil # always false
end

can :index, Membership do |m|
  !true
end

In all the above, the user can still access the index action, despite false clearly being the result of the block...

Additionally, I have the following declaration at the top of my memberships_controller.rb:

load_and_authorize_resource :organization
load_and_authorize_resource :membership, :through => :organization

How can I make sure that only officers can view the index page?

@ryanb
Owner

The block only gets evaluated when there is an instance of the object to test permission on. In the index action there is no membership object so it does not call the block. You should place conditions outside of the block which are not dependent on a membership instance.

can :index, Membership if user.is_officer?(m.organization)

See the checking with class section in the Checking Abilities wiki page for why this is.

I will mark this ticket for improved documentation so there's less confusion on the role of the block.

@sethvargo

How does cancan know what m is in m.organization above? I'm still really confused with this.

The problem is that an instance of a membership is necessary to check the condition, because its the membership that actually determines whether the user is an officer of not...

@ryanb
Owner

I've updated the documentation in the Defining Abilities with Blocks wiki page to be more clear that blocks are only to be used when an object is present (and will not be triggered in the index action).

But I see now that doesn't completely clear up your issue here. I missed that m was being accessed above. You have a tricky case where you need to authorize the actions based on the parent's (organization's) attributes. So the permissions are for the organization, not the membership. You could do this in Ability.

can :read, Organization do |organization|
  user.is_officer?(organization)
end

However this assumes you want to define that permission for organizations no matter what context they are in. If you want to restrict permissions so they only apply to the context of the MembershipsController you can create a custom authorization behavior through a before_filter.

# in controller
load_resource :organization # note authorization isn't on this line
before_filter :authorize_organization
load_and_authorize_resource :membership, :through => :organization

private

def authorize_organization
  authorize! :access_memberships, @organization
end

# in Ability
can :read, Organization # anyone can read organizations
can :access_memberships, Organization do |organization|
  user.is_officer?(organization) # only officers can access membership of an organization
end
can :read, Membership # beyond that, everyone can read memberships

Does that make sense?

@sethvargo

:(. I was hoping I wouldn't have to resort to writing a custom method. Yea, that makes sense, thanks!

@ryanb
Owner

Yeah, the Ability class knows very little about the controller and the request. It only knows the information the controller passes it, which is one resource at a time. This means parent-child permission checks like this are not its strong point.

I am thinking of changing this up in CanCan 2.0 and making it a little more integrated with controller actions, but I haven't thought of a good way which still keeps the convenience and modularity. Let me know if you have any ideas.

@sethvargo

hmmm.... I'm really tired and have been coding all day (it's 3am EST haha). Do you mind if I fork tomorrow? I'll see if I can come up with anything...

@ryanb
Owner

Sure, feel free to fork the project and experiment. I'll mark this issue for discussion in the meantime.

@ryanb
Owner

I'm closing this because in CanCan 2.0 it will be possible to add authorizations which are specific to certain model attributes. Nested resources will be treated as attributes so it will be easy to add permissions to a parent resource which only apply when accessing child resources.

# in Ability
can :access, :organizations, :memberships do |organization|
  user.is_officer?(organization)
end

Here memberships is considered an attribute on an organization, so the authorization only gets triggered when fetching memberships on an organization. That code may not be the final interface, I'll need to play around with it a little as 2.0 gets further into development.

See issues #191 and #154 for details.

@ryanb
Owner

After thinking about this a bit more that interface may not work well for nested resources. I'm going to re-open this ticket to ensure this isn't lost when I work on 2.0.

@ryanb
Owner

I have an interesting idea for this that I think will work in the current release. This is untested, but you should be able to define permissions inside the block of another permission. For example.

# in controller
load_and_authorize_resource :organization # no customization needed here
load_and_authorize_resource :membership, :through => :organization

# in Ability
can :read, Organization do |organization|
  if user.is_officer?(organization)
    can :read, Membership, :organization_id => organization.id
  end
  true # return true to grant read permission on organizations
end

See if that does the job for you. If so I'll add some documentation for this.

@sethvargo

sorry, I've been really busy and just got your email. I'm currently in class, but I'll try whenever I'm done and let you know!

@ncri

Interesting, just stumbled upon the same problem. Has this been tested in the meantime? I think nesting abilities is a good way to go. Not sure though if its good if one always has to define an outer ability at the same time. Will the interface for CanCan 2.0 be any different? Has it been finalized?

@ncri

Well, if it works, it seems actually a nice way to deal with nesting.
Wonder though if I can nest deeper than one level.

I just coded this

  can :read, Project do |project|
    can :read, TodoList do |todo_list|
      can :read, Todos if user.can?(:view_todos, todo_list)
      can :create, Todos if user.can?(:create_todos, todo_list)
      can :update, Todos if user.can?(:edit_todos, todo_list)
      can :destroy, Todos if user.can?(:delete_todos, todo_list)
      user.project.todo_lists.include? todo_list
    end
    can :create, TodoList if user.can?(:create_todo_list, project)
    can :update, TodoList if user.can?(:edit_todo_list, project)
    can :destroy, TodoList if user.can?(:delete_todo_list, project)
    can :manage, ProjectAssignment if user.can?(:manage_users, project)
    user.projects.include? project
  end

(The can function on the user is my own).

Will test later if it works... ;-)

@ryanb
Owner

@ncri, I haven't had a chance to test this yet. Let me know if it ends up working or not. So far I haven't addressed nested resources any differently in CanCan 2.0 but I would like to find a solid solution to this problem.

@ncri
 # in controller
 load_and_authorize_resource :organization # no customization needed here
 load_and_authorize_resource :membership, :through => :organization

Actually I don't think your example will work, ryan, because the first line tries to load the organization using the :id param, and the second line tries to load the membership with the same id param. Or is there a way to specify what parameter to use to fetch the records? I couldn't find anything in the rdocs.

Nico

@ncri

I think for now I will roll my own authorization for nested ressources. I'd love to use CanCan though. Well, maybe the 2.0 release will have a good way to deal with nested resources. ;-) Possibly I create a fork of the current release and modify it to my needs. I will report if I come up with a good solution...

@ncri

Okay, finally got around to test this: Nesting the abilities works fine!

And also

    load_and_authorize_resource :organization # no customization needed here
    load_and_authorize_resource :membership, :through => :organization

Does work, I got that wrong. Even nesting deeper then one level works fine.

@ryanb
Owner

@ncri, I'm glad you got it working. Keep an eye on CanCan 2.0 because I hope to improve how nesting works there. If you have any suggestions on how I can make this more intuitive let me know.

@ryanb
Owner

I don't think I'll get around to addressing nesting in CanCan 2.0, but I plan to focus on it in 2.1. The 2.0 release will be big enough and I'd rather not delay it.

@ncri

Yep, sure, I understand. Actually it works really well already, so I wouldn't worry too much. Yes, a somewhat cleaner syntax would be useful. Also nice would be a way to change the name of the association a record is fetched through. For instance if I have a PostComment model I might rather call the association on Post only comments. Haven't found a way though to make this work with cancan and nesting. It expects a blog_comments association. Have I overlooked a switch? It would be alos nice to be able to supply the current user as a parent "model" as I find it a common pattern to fetch records through the user.

Anyway, thanks for the great work! ;-)

@ryanb
Owner

@ncri, both of those features should work. You can pass :through => :current_user and it will look for a current_user method and use that as the parent. You can pass :through => :post, :through_association => :comments to change the association.

http://rdoc.info/github/ryanb/cancan/master/CanCan/ControllerAdditions/ClassMethods:load_resource

@ncri

Thanks for the info, great! I seem to constantly overlook those options... ;-)

@ncri

Okay, I tried that out now: :through => :post, :through_association => :comments works fine. However, when I try to load something through the current user I get CanCan::AccessDenied, although the resource I load through the user should be accessible, according to the abilities. Loading through the user is not too important though, as the resource is authorized against the current_user anyway. Just curious what goes wrong.

Nico

@ryanb
Owner

@ncri that's strange. Have you tried playing duplicating the problem in the console as shown on the Debugging Abilities page?

@ryanb ryanb closed this
@ryanb ryanb reopened this
@ncri

I tried this and as expected the user has access to the resource (a project). But Despite that I get the CanCan::AccessDenied as soon as I do :through => :current_user. I might debug a bit more at some point. I guess I need to step through the cancan code to find out what's going on.

@ncri

Okay, did debug already. ;-) The error is raised in controller_resource.rb resource_base. The problem is in fetch_parent @controller.respond_to? 'current_user' returns false.

@ncri

Okay, its because my current_user method is private in the application controller, but defined as a helper_method. If I make it public, it works. Maybe CanCan should support private helper methods for through associations as well? They are rather common.

@ryanb
Owner

@ncri good find. Can you open a separate issue on making :through work with private methods? I should be able to get a fix for that in 1.6 and I would like to keep this issue open for other nesting ideas in 2.1.

@fabianoalmeida

Hi @ryanb. I'm using the version 1.6.5 and I didn't get that this example (above) works fine like you and @ncri commented. In my project I'm using Devise and I have an object called Client where this object has the User object as "father" (inherit).

And, for my reality, there is a model called Contract where should only showed through the Client model. For example:

# Client model
class Client => User # the inheritor symbol wasn't working 
end
# Contract model
class Contract => ActiveRecord::Base  # the inheritor symbol wasn't working 
  belongs_to :user
  ...
end
# Contract Controller
class ContractsController => AuthorizedController # the inheritor symbol wasn't working 
  load_and_authorize_resource :client
  load_and_authorize_resource :contract, :through => :client
  ...
end
# Ability
  def initialize(user)
    user ||= User.new # guest user (not logged in)
    
    if user.role? :admin
      can :manage, :all
    else
      can :read, Client do |client|
        can :index, Contract, :user_id => user.id
        client.id.eql? user.id
      end
    end
  end

But, like I showed above, this isn't working. Always I receive "Access denied" message. This example just working in CanCan 2.0?

Thank you so much and congratulations. This project is really awesome.

@ncri

Have you tried load_and_authorize_resource :contract, :through => :client, :through_association => :user ?

@fabianoalmeida

@ncri I just tried your suggestion, but didn't work. Do you have this example working in some project?

@fabianoalmeida

@ncri, I tried to use your tip and this:

can [:read, :update], Client do |client|
  can :read, Contract do |contract|
    contract.user.id.eql? client.id
  end
  client.id.eql? user.id
end

But didn't work too. =~

Do you have other tip? Thanks!

@fabianoalmeida

Ok @ncri, I solved my problem. I saw this Devise's page and my application was calling the method load_and_authorize_resource from AuthorizedController and didn't from ContractController. So, I got it the situation and solved my problem. Thanks so much.

@Antoine-Falquerho

have some problem with cancan and a nested routes.

I have this routes :

resources :companies do
   resources :projects
end

I have no problem with the abilities for Company model but for the Project model I want to deny the access to Project#index if they are not admin of the company.

The next code works :

can :show, Company do |company|
     if user.admins.include?(company) #check if the user is admin of the company
      can :index, Schedule, :company_id => company.id
   end
end

But how I can't do :

can? :index, Project

I tried by renamed the method like that :

can :index_projects, Company do |company|
   if user.admins.include?(company) #check if the user is admin of the company
      can :index, Schedule, :company_id => company.id
   end
end

and use :

can? :index_projects, @company

But it doesn't work. Do you know how to do it?

Thanks.

@emilford

Hi Ryan,

First off, thanks for the awesome gem. It's been a pleasure to use on several projects now.

I have spent the last couple of hours working out several issues with loading and authorizing nested resources. My research lead me to this thread and I implemented a solution based on your examples that appears to be working. But, I'm admittedly uncertain as to how.

I spent the last hour digging around trying to understand why one of my CanCan specs was failing and it lead me to more uncover more things and have more questions. I noticed that when I dump the CanCan rules for current_user via StandardError, I see mention of all of the defined abilities (subjects) with the exception of the one pertaining to Affiliation (see below). Is this expected behavior with nested abilities?

Also, it concerns me that the Affiliation spec is failing despite the loading/authorizing seemingly working in the browser. It's been a long day, so this piece could very well just be my goof, but as far as I can see the spec should pass. Is there something tricky going on with the nested abilities?

Thanks again.

load_and_authorize_resource :network
load_and_authorize_resource :affiliation, through: :network
can :read, Network do |network|

  can :manage, Affiliation do |affiliation|
    NetworkManagementPolicy.new(network).can_modify?(current_user)
  end

  NetworkManagementPolicy.new(network).can_read?(current_user)
end
class NetworkManagementPolicy
  attr_reader :network

  def initialize(network)
    @network = network
  end

  def can_modify?(user)
    membership = membership_for(user)
    membership && membership.admin?
  end

  def can_invite?(user)
    membership = membership_for(user)
    membership && membership.can_send_invitations?
  end

  def can_read?(user)
    !! membership_for(user)
  end

  private

  def membership_for(user)
    network.memberships.where(user_id: user).first
  end
end
describe "Affiliation" do
  let(:affiliation) { Affiliation.new }

  describe "#manage" do
   #
   # This spec fails
   #
    it "allows a user that meets the can_modify? requirements" do
      NetworkManagementPolicy.stub_chain(:new, :can_modify?).and_return(true)
      ability_for(user).should be_able_to(:manage, affiliation)
    end

    it "denies a user that does not meet the can_modify? requirements" do
      NetworkManagementPolicy.stub_chain(:new, :can_modify?).and_return(false)
      ability_for(user).should_not be_able_to(:manage, affiliation)
    end
  end
end
@graywh

Not sure if this warrants a new issue or not, but I'm using an attribute to grant read permissions on some records that can optionally be indexed through a parent. My issue is that access is denied when the user has no read access on any of the possible parents, even when accessing the controller in a non-nested fashion.

routes

map.resources :parents do |parent|
  parent.resources :records, :only => [:index, :new, :create]
end
map.resources :records

controller

RecordsController
  load_and_authorize_resource :parent
  load_and_authorize_resource :record, :through => :parent, :shallow => true

and abilities

can :read, Record, :id => 1
cannot :read, Parent

With permission to read some records, an exception is thrown when a user tries to access /records or /records/1 because he has no read access on any parents.

@natebird

It might be time to close this issue and move any remaining questions to new issues. This one seems to be spawning multiple questions over the 2 years it's been open. :-)

@sethvargo sethvargo closed this
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.