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

Authorizing access to specific fields on a model #154

Closed
macek opened this Issue Sep 22, 2010 · 24 comments

Comments

Projects
None yet
7 participants

macek commented Sep 22, 2010

Cancan is my favorite authorization plugin for Rails but I seem to find difficulty when getting down to the nitty gritty. How would we go about role-based permissions for specific fields on any given model?

For example, say sales and customer service staff can edit the address fields on a user, but only admin or sales can update a user's customer number. Loading the user resource with cancan is no problem, but the field-based permission logic in the views get very ugly very quickly.

# e.g., app/views/users/_form.html.erb

<% if current_user.role?(:admin, :sales) %>
  <%= f.text_field :customer_number %>
<% else %>
  <%= @user.customer_number %>
<% end %>

<% if current_user.role?(:sales, :csr) %>
  <%# edit address fields %>
<% else %>
  <%# display address %>
<% end %>

This seems to be a very common issue I see people running into (myself included), but I'm never quite sure how to address it in the best way. If possible, I'd like to get a discussion going on this topic.

Note: If it's not in cancan's scope, I completely understand closing this issue.

dapi commented Sep 22, 2010

I'm very interested in this feature too.

cwade commented Sep 22, 2010

Me too.

Owner

ryanb commented Sep 23, 2010

One solution is to use a custom action, see this wiki page for details.

Another solution is to use the trusted-params plugin, but that is not yet Rails 3 compatible. It also does some ugly overriding of internal Rails methods and all hashes, which is not something I want built into CanCan.

I agree neither of these are ideal and don't flow with CanCan's handling of permissions. Does anyone have some other ideas on better ways to do this through CanCan?

dapi commented Sep 24, 2010

Yes, overriding ActiveRecord methods is not the best way.

Now I use solution like this:

Controller:

def update
  @user = User.find( params[:id] )

  attrs = params[:user]
  authorize? :update, @user, attrs

  @user.update_attributes(attrs)

 ...

Ability:

can :update, User do | subject, params |
  ! user.toucheble_parameters(subject).each { |p|
    break if params.exists? p
  }.nil?
end

Where User#toucheble_parameters is a list of the subjects parameters that can be touched by the user.

May be better to have just methods like can/can? but without actions?

Example:

can_touch_attributes User, [:name, :email], :user_id=>user.id  # Anybody can touch his name and email

can_touch_attributes User, [:is_admin], :role => 'admin'  # Only admin can touch is_admin

...

can_touch_attributes? @user, attrs

I think we don't need actions for checking attributes. But if somebody needs it, he can use the first solution.

macek commented Sep 24, 2010

dapi,

Excellent comments here. I like the technique you're using in Ability.rb.

I'm still thinking about situations like this:

<%= @product.price if current_user.role?(:admin, :sales :distributor) %>
<%= @product.cost if current_user.role?(:admin, :sales) %>

In these examples, I need to control who can :read certain attributes. This worsens; look at an edit product form.

<% form_for @product do |f| %>
  ...
  <% if current_user.role?(:admin) %>
    <%= f.text_field :price %>
    <%= f.text_field :cost %>
  <% elsif current_user.role?(:sales) %>
    <%= @product.price %>
    <%= @product.cost %>
  <%= elsif current_user.role?(:customer_service) %>
    <%= @product.price %>
  <% end %>
  ...
<% end %>

I think you're on to something with your can_touch_attributes macro, but I can think of several examples where I'd want permission based on the action/attribute combination; not just the attribute alone.

macek commented Sep 24, 2010

dapi,

I'm trying to come up with a good combination of cancan Custom Actions and the techniques you described above...

Keeping everything defined in Ability.rb is very attractive to me. Let's keep poking at this and see if can come up with something :)

Owner

ryanb commented Sep 24, 2010

What if can supported an additional parameter that is a symbol representing the attribute. This could be an array of symbols too.

can :manage, Article, :content, :user_id => user.id

And similar for can?.

can? :update, @article, :content

In both cases the last symbol is optional, and if not present it will represent all attributes which is the current behavior. The check for each attribute would happen automatically in authorize_resource in the controller.

This conflicts a little with the existing API but let's not worry about that when brainstorming.

dapi commented Sep 25, 2010

Yes. Something like this.

Views

To check parameters that are not permitted to update. Don't show it in the form.

<% if can? :update, @user, [:is_admin, :roles] %>
  <%= form.input :is_admin %>
  <%= form.input :roles %>

or

<% if can? :update, @article, :banned  %>
  <%= form.input :banned %>

Abilities

Admin can update any attributes:

can :update, Article, user.is_admin => true

# is similar to

can :update, Article, :attributes => :all, user.is_admin=>true

Owner can update any attributes except :banned

can :update, Article, :exclude => [ :banned ], :user_id=>user.id 

or

can :update, Article, :attributes => [ :subject, :content, :published ], :user_id=>user.id

or just

can :update, Article, [ :subject, :content, :published ], :user_id=>user.id

Controller

Raise an exception if the user has no the ability or if he try to update banned attributes.

authorize! :update, @article, params[:user]

# or

authorize! :update, @article, params[:user].keys

Raise an exception if the user has no any declared abilities to update the Article.

authorize! :update, Article

And the hit of brainstorming: If he has some abilities then remove banned attributes silently.

#  params[:user] = { :content=>'test', :banned=>false }

sanitize :update, @article, params[:user]

# now params[:user] = { :content=>'test' }
Owner

ryanb commented Sep 27, 2010

Perhaps the :exclude option can be handled with cannot.

can :update, Article
cannot :update, Article, :banned

Also silently removing attributes with sanitize might cause some confusion. I find that to be the case with the current behavior of attr_accessible. Better to raise an authorization error and let the developer rescue from that early if he wants.

Everything else looks good though. I'll definitely consider this in a major update to CanCan.

macek commented Sep 27, 2010

dapi, this looks great. A couple comments.

Abilities

You use the hash key :exclude. The Rails convention here seems to be the key :except. I think we should stick with that.

If we use with :attributes => :all or :attributes => [...], We might not need an API change. We could simply treat :attributes and :except as reserved keys. This might not be entirely favorable, though, as someone might have an attributes field on one of their models.

# user can only see blue products
can :read, Product, :attributes=>"blue"

Seems unlikely, but it shouldn't be overlooked entirely.

Controller

First, a little error in your code. You're authorizing @article but using params[:user].

I think we'd want to use authorize! :update, @article, params[:article]. I think it's assumed that we'd want to use the keys of the params hash, so calling params[:user].keys every time would be kind of silly. Plus, I think it would be confusing for people that are new to the plugin. It'd be easy to do something like,

def check_attributes(attributes)
  if attributes.is_a? Hash
    attributes = attributes.keys
  end
  ...
end

Moving on to your sanitize method. I love this. I've recently been tinkering with Searchlogic and sanitizing form params can be tedious. I like the idea of not shoving sanitization aside or implementing it as an afterthought.

macek commented Sep 27, 2010

Ryan,

Ah, I was likely writing my previous comment the same time as you. I didn't see your comment until after I posted.

Handling :except with cannot seems to do the trick. This actually seems very elegant. If the attribute list is always an array, you can skip the need for :attributes => :all too.

dapi, to be more explicit, I think this is what Ryan is going for.

can :update, Article                        # all attributes
cannot :update, Article, [:banned]          # except for :banned

can :update, Project, [:priority, :status]  # only :priority and :status

This brings up another idea. What if the attributes array contains associations or nested attributes? Would something like this work?

# associations
can update, Project, [:categories, :members]

# nested attributes
can :update, Project, [:notes_attributes]

I also agree that raising an exception is good for the default behavior. If desired, perhaps a flag could be passed to sanitize to "force" cleanse the params?

Owner

ryanb commented Sep 27, 2010

@macek, nested attributes partially work since they are considered an attribute on the model. However, if you want to control access to specific, deeply nested attributes then that will be a problem. This happens outside of the controller, so there's no way to know that notes_attributes should be checked on the Note model.

Actually nested attributes are a problem with CanCan's current behavior as well, but I haven't heard any complaints. I'm guessing they just work around it with before filters.

Regarding sanitize, I have a hard time seeing the use case, but that doesn't mean there isn't one. I'm up to discussing it further, but maybe under a separate issue after we see this in action more.

Owner

ryanb commented Sep 27, 2010

Just some more brainstorming ideas. It will be tedious to perform the can? check on each individual field in the view. Maybe there can be a separate form builder gem which accepts the current_ability and automatically hides/shows each field depending on the user's permission. This could integrate with Formtastic as well for further convenience.

dapi commented Sep 28, 2010

Ryan,

I agree with you, it's better to raise an authorization error when user tries to update secret fields.

About Formtastic. It's very good idea.
Can we use sanitize to automaticly remove fields and have the clear form? There is no needs to any Formtastic's changes.

def new
  authorize! :create, User
  @user = User.new
  sanitize :create, @user # Removes secret fields
end

...

= semantic_form_for @user do |form|
  = form.inputs      # Shows existent fields only

or

= semantic_form_for sanitize( @user ) do |form|
  = form.inputs

In this case sanitize automaticly detects an action.

macek commented Sep 28, 2010

dapi,

I'm pretty sure most people won't be using the <%= f.inputs %> macro to dump all the fields at once. The ability to build the form manually would be essential.

rbritom commented Oct 5, 2010

@ryanb I guess you already know but rails 3(activerecord) has a mass assignment security module,

http://github.com/rails/rails/blob/master/activemodel/lib/active_model/mass_assignment_security.rb

cancan should take advantage of this and i think trusted params could be reworked to take advantage of this.

Owner

ryanb commented Oct 5, 2010

@rbritom, wow that's awesome, I did not know that. Time to investigate and maybe I'll do a Railscasts episode on it.

rbritom commented Oct 5, 2010

@ryanb =D

I said is part of activerecord but is really part of active model so it can be applied to classes that dont have a DB backing them up.

dapi commented Oct 7, 2010

Will be nice to have something like sanitize_for_mass_assignment from CanCan :)

Owner

ryanb commented Mar 16, 2011

I'll be moving forward with this soon in a new 2.0 branch. Please see issue #191 for a good overview of the different changes planned for 2.0 and provide your feedback there.

macek commented Mar 25, 2011

Awesome. I'm super excited for this :)

Owner

ryanb commented Mar 25, 2011

Closing because this is now in the 2.0 branch. It may not be completely polished and fully working yet, but the functionality is there.

@ryanb ryanb closed this Mar 25, 2011

Hi, what happened with this? it seems to be abandoned, any new?

Fedcomp commented Aug 28, 2015

@ssoulless use CanCanCan

@adeelejaz adeelejaz referenced this issue in CanCanCommunity/cancancan Jun 7, 2017

Open

Define Permissions on Specific Resource Attributes #420

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