Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Added support for strong_parameters #763

Open
wants to merge 6 commits into from
@ollym

This also allows a user-defined pre-processing method for parameters if
set via the :params option.

ollym added some commits
@ollym ollym Added support for strong parameters
This also allows a user-defined pre-processing method for parameters if
set via the :params option.
e85d735
@ollym ollym Fixed Ruby 1.8.x build 7c31383
@ollym ollym Added docs for :params option c8e3f6d
@ollym ollym Only build parameters on update/create actions 1358b21
@ollym ollym Fixed faulty tests making build fail
Why should controller parameters be considered outside the
create/update actions of a resource controller?
3b859aa
@mikeycgto

Using your fork and branch now in an app. Love to see it get merged in. Being able to define where the resource parameters come from provides a good deal of flexibility and makes strong_parameter use very straightforward.

@ollym

@mikeycgto Yea, I'm using it in most of my projects. Only issue i've come across is that the parameter function has to be protected and not private. Otherwise it works well

@ndbroadbent

+1 for this, especially as Rails 4 is just around the corner.

@jonathanhefner

+1

This is great! I was just thinking of a fork like this last week!

@Chetane

+1

Had to use this approach for using Cancan with strong parameters (after watching both rails cast). It would be useful to integrate it in Cancan!

@ollym ollym referenced this pull request
Closed

2.0 #768

@zolzaya

+1

@ekampp

@ryanb is there a forecast for when you will get to take a look at this feature?

@ollym

@ekampp I think @ryanb is holding off to create a more elegant solution that works with strong parameters rather than around them. Something where you can define permitted attributes in one place for example:

def user_params
  if params[:action] == 'create'
    params.require(:user).permit :first_name, :last_name, :password
  elsif params[:action] == 'update'
    params.require(:user).permit :first_name, :last_name
  end
end

replaced to:

can [:create, :update], :users, [:first_name, :last_name]
can :create, :users, :password

I agree this is the right way forward however I think it should be bundled together in a CanCan v3 which includes changes to support the rest of Rails 4. Then CanCan v2 will stay to support Rails <= 3.x.x (with this patch to support the strong_parameters gem).

Whether Ryan will have time to do CanCan v3? I don't know.. but it's certainly something I'm getting ready to do as I want to start using Rails 4 in my projects and I heavily rely on CanCan. Maybe we can start a Wiki page with some suggestions for it?

@Fustrate

Well, Rails 4.0.0.beta1 is upon us. I was looking at my ability definitions, and having permitted parameters as a generic third parameter as @ollym suggested doesn't really excite me. Some cases might need to be a bit more dynamic than that, so what about having an additional way to do it:

can :update, :users do |user|
  # Use a new 'permit' method to permit certain attributes based on dynamic criteria.
  # This has to be done user-by-user because supervisors shouldn't be able to change their own schedule
  if current_user.supervises? user
    permit :schedule, :status
  else
    permit :first_name, :last_name, :email, :password
  end

  # Then return a boolean as usual
  current_user.supervises?(user) || current_user == user
end

It's late and I'm tired, but I think it might work. Basically just a tiny tiny one-method DSL for deeper parameter allowances based on model/user attributes. I have no idea what internal changes this might entail, though.

@ollym

@Fustrate love it. I've been thinking about this over the past few days as-well and believe there is a need to fork cancan, remove all legacy 1.8.x code, and refresh the internals to embrace Rails 4, including full support for arel.

In regard to your code, I don't like the boolean statement at the end of the block. I think it should be more like:

can :update, :users do |user|
  if current_user.supervises? user
    permit :schedule, :status
  elsif current_user == user
    permit :first_name, :last_name, :email, :password
  end
end

Then you can decide whether or not the user can be updated based on whether there are any permitted attributes or not.

The other thing I don't like is that you appear to want to keep everything in the single ability file. I think access control logic should be moved from a model to the controller so like... (similar to StrongParameters)

class UsersController < ApplicationController
  can :update do |user|
    if current_user.supervises? user
      permit :schedule, :status
    elsif current_user == user
      permit :first_name, :last_name, :email, :password
    end
  end

  def update
    # ...
  end

end

What do you think?? Some food for thought..

@Fustrate

Good points. Since the block would only do one thing again, what about just returning the parameters to permit instead? If the block returns nil, there's nothing to permit, so the user isn't allowed to do anything.

class UsersController < ApplicationController
  can :update do |user|
    if current_user.supervises? user
      [:schedule, :status]
    elsif current_user == user
      [:first_name, :last_name, :email, :password]
    end
  end

  def update
    # ...
  end

end

I think it's just as clear and wouldn't require as much of a code change. There should also be an option to pass the symbol name of a defined method, just to keep things clean:

class UsersController < ApplicationController
  can :update, :update_params
  can :create, :create_params
  # Or merge the two into one method call?
  # can update: :update_params, create: :create_params
  load_and_authorize_resource

  def update
    # ...
  end

  def create
    # ...
  end

  protected

  def update_params(user)
    if current_user.supervises? user
      [:schedule, :status]
    elsif current_user == user
      [:first_name, :last_name, :email, :password]
    end
  end

  def create_params
    # ...
  end

end
@ollym

Maybe... It's a little messy though and that works with write operations (create, update) but what about read (index, show). Some thoughts....

Compact version...

class UsersController < ApplicationController

  can :read, [:first_name, :last_name]
  can :read, user: :current_user

  can :update, [:first_name, :last_name, :email, :password], user: :current_user
  can [:read, :update], [:schedule, :status], -> (user) { current_user.supervises? user }

end

And using blocks...

class UsersController < ApplicationController

  can :update do
    if current_user.supervises? user
      [:schedule, :status]
    elsif current_user == user
      [:first_name, :last_name, :email, :password]
    end
  end

  can :read do |user|
    user == current_user || [:first_name, :last_name]
  end

end
@Fustrate

Controlling model attribute access for read actions moves past the scope of strong parameters and the type of authorization CanCan does, in my opinion. It would be a bit over-involved in the model if it were to start raising exceptions on things like @user.last_name.

I'd leave :read and :destroy to just look for booleans, at this point.

@ollym

@Fustrate i disagree! cancan should never enforces fine-grained control like that.. but does, and should continue to allow you to use it in your views. So like:

can :read, :users, :first_name
<table>
  <thead>
    <tr>
      <% if can? :read, :users, :first_name %><th>First Name</th><% end %>
      <th>Last Name</th>
    </tr>
  </thead>
  <tbody>
    <%= content_tag_for :tr, @users do |user| %>
      <td><%= can?(:read, user, :first_name) ? user.first_name : '-' %></td>
      <td><%= user.last_name %></td>
    <% end %>
  </tbody>
</table>
@Fustrate

Hmm. I'm not wild about it, and probably wouldn't have a need for it myself, but I can see where it would be useful to some. Just as long as it only keeps track of things from the outside, without injecting itself into attribute access to yell at me (since my code loves to yell at me).

@ryanb
Owner

Sorry to let this issue sit so long. With Rails 4 close to release it's becoming more urgent for me to get support for strong_parameters in. The plan is to get a work-around (similar to what's presented here in this PR, thanks @ollym!) for CanCan 1, and then research alternatives for CanCan 2.

CanCan 2 already has support for attribute-level authorization, but I may need to rethink some of it in light of strong_parameters. Either way CanCan 1 is more pressing at the moment so I'll work on this now. Thanks for the contributions everyone.

@ryanb
Owner

My current plan for CanCan 1 is to provide two options. One is permit which will allow you to declare which attributes to allow inline:

load_and_authorize_resource permit: [:name, :content]

And params which will work like it does in this PR.

load_and_authorize_resource params: :article_params

def article_params
  params.require(:article).permit(:name, :content)
end

I think the permit option will be sufficient for most simple cases, and the params option allows you to customize it depending upon the authorization needs. If you do this I recommend delegating the authorization logic into the Ability class. Like so:

load_and_authorize_resource params: :article_params

def article_params
  if can? :set, :article_name
    params.require(:article).permit(:name, :content)
  else
    params.require(:article).permit(:content)
  end
end

# in Ability
can :set, :article_name if user.admin?

Or perhaps this alternative.

load_and_authorize_resource params: :article_params

def article_params
  params.require(:article).permit(*current_ability.article_attributes)
end

# in Ability
def article_attributes
  if @user.admin?
    [:name, :content]
  else
    [:content]
  end
end

I think both of these workarounds will prove sufficient for now, and should be fairly easy to incorporate into CanCan. Thoughts? I'll let this sit for a couple days to get feedback and then work on the implementation.

@bryanrite

I really like the outlined permit and params way, it will solve my use-cases and is good solution for incorporating with strong_parameters in v1. :+1:

@ollym

Some further thoughts I had for 2.0... We can remove the MetaWhere dependency and replace it with Arel so for example:

users = Arel::Table.new(:users)
cannot :read, :users, users[:name].eq('bob').or(users[:age].lt(25))

Which can be used within the query like:

# User.accessible_by current_ability
# query = users[:name].eq('bob').or(users[:age].lt(25)

return User.where query.not # Query inverted

Seems like a clean alternative.

@stevenpetryk

@ollym You may want to revise this, since scoped is deprecated in Rails 4. Model.all has replaced it.

@kstevens715

@ryanb Your plans for CanCan 1 would work perfectly for me. I look forward to the update, thank you.

@Mik-die

You need to pass true as second parameter in respond_to?, because params_method can be private

@joshmcarthur

For anyone stuck on CanCan < 2.0 like me, I've forked and patched 1.6 to match @ollym's fix for 2.0 here: https://github.com/3months/cancan/tree/strong_parameters. I've also updated the tests to test the new functionality (tests passing).

If you wish to use this in your application, you can use my branch by adding to your Gemfile:

gem 'cancan', :git => 'https://github.com/3months/cancan', :branch => 'strong_parameters'

Please note that I'm not overly familiar with the source, in particular the differences between 1.6 and 2.0 beyond anything the README will tell you, so I may have missed an edge case or taken the wrong approach here - if so, please let me know so I can fix up my branch.

I'd also like to say that this is only intended as a temporary measure so that 1.6 can be used with strong_parameters until this change is properly backported by @ollym or @ryanb - so don't forget to add a TODO or FIXME comment to your Gemfile reminding yourself to revert back to a proper gem release!

Thanks heaps to @ollym for doing all the work!

@jfine

@ryanb Love CanCan but currently running into issues with Rails 4 and strong_params. Using this fix (#571 (comment)) but running into issues using respond_to/with on a destroy action. In other words... +1. :smile:

PS: My respond_to/with issue was caused by a wonky header. Totally unrelated.

@mattclar

@ollym Hi I was just trying to implement your branch of can can as above but Imust be missing something as im am still getting ForbiddenAttributesError from my controller. I used an example you gave above and your in-code doccumentation but im not really getting anywhere! Was just wondering if anyone here could give some more specific details must be something obvious which I am missing!!

EDIT:
OK think I worked it out, I was thinking that this modification made some sort of can can magic happen that would avoid the standard strong parameters implementation as detailed in the rails documentation. But Im guessing I am wrong and what it in-fact does is remove any barrier that cancan was putting in the way in regards to this default implementation. Glad to say I have got it working now!!

@s-edwards s-edwards referenced this pull request in web-cat/pythy
Closed

Self-enrollment assignment to protected attributes #57

@ollym

I opened this issue over a year ago and nothing's changed. I no longer use cancan but people may be still using my branch, I was planning to delete it but happy to leave it if people are still using it?

@kevinmccaughey

Can-can canned?

@zdavis

Kind of starting to look that way

@jfine

I surely hope not. I very much hope @ryanb doesn't go the way of _why. :cry:

@zbruhnke

@ollym what did you switch to instead of cancan? I'm looking at pundit now, unfortunately I adopted a project where someone decided to use cancan 1.6 with Rails 4 and needless to say its not playing nice

@ncri

We are still using CanCan 1.6 with Rails 4 too, works well enough for us, although its not ideal.

@lephyrius

@zbruhnke I also use CanCan 1.6 with Rails 4 you basically have to get used to the hacks which is never ideal but the best thing in this situation. :crying_cat_face:

@zbruhnke

@lephyrius are you just using load_and_authorize_resource like most everyone else? or anything else in particular you added to make it work better?

@Marslo

@openATM see it!!

@ncri

@lephyrius: We use a before_action for post and put requests:

def filter_resource_params
  return unless request.post? or request.put?
  resource = controller_path.singularize.gsub('/', '_').to_sym
  method = "#{resource}_params"
  params[resource] &&= send(method) if respond_to?(method, true)
end

It applies a param sanitization method "#{resource}_params" to the params. This method needs to be defined for each resource.

@ollym

@zbruhnke I'm using a hybrid of model scopes and strong parameters.

# app/controllers/users_controller.rb
def user_params
  if current_user.admin?
    params.require(:user).permit!
  else
    params.require(:user).permit(:first_name, :last_name, :email);
  end
end

And for scoping:

# app/model/project.rb
class Project < ActiveRecord::Base
  scope :accessible_by, -> (user) {
    includes(:user).where(users: { id: user.id });
  }
end

# app/controllers/projects_controller.rb
@projects = Project.accessible_by(current_user);

It allows me to keep parameter filtering and access control in the controller and scoping in the models which I think is correct.

@zbruhnke

@ollym got ya, I'm running across a differnet scenario where I need to create many types of users and allow users to belong to multiple types so I think this may just have to be a highly customized version if what's available I'm skeptical to use the gems available because of the number of gems that seem to end up unsupported

@ollym

@zbruhnke remember there's nothing CanCan can do that a combination of Arel in model scopes can't. I have a number of complex scopes that uses a lot of Arel predicates and it works really well.

@MattRogish

:+1: Would love CanCan to get updated for Rails4... 4.1 is right around the corner!

@americos

:+1: @ryanb We need you to lead us :)

@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
Commits on Oct 13, 2012
  1. @ollym

    Added support for strong parameters

    ollym authored
    This also allows a user-defined pre-processing method for parameters if
    set via the :params option.
  2. @ollym

    Fixed Ruby 1.8.x build

    ollym authored
  3. @ollym

    Added docs for :params option

    ollym authored
  4. @ollym
  5. @ollym

    Fixed faulty tests making build fail

    ollym authored
    Why should controller parameters be considered outside the
    create/update actions of a resource controller?
Commits on Dec 9, 2012
  1. @ollym

    Fixed instance_name override

    ollym authored
This page is out of date. Refresh to see the latest.
View
11 lib/cancan/controller_additions.rb
@@ -112,6 +112,17 @@ def load_and_authorize_resource(*args)
# [:+prepend+]
# Passing +true+ will use prepend_before_filter instead of a normal before_filter.
#
+ # [:+params+]
+ # Allows you to define a pre-processor for resource attributes. Set this to a symbol relating
+ # to a method name within the controller. In Rails 4 or when using the strong_parameters gem
+ # this defaults to +:<resource_name>_params+.
+ #
+ # load_resource :params => :resource_params
+ #
+ # def resource_params
+ # params[:resource].except :foo
+ # end
+ #
def load_resource(*args)
raise ImplementationRemoved, "The load_resource method has been removed, use load_and_authorize_resource instead."
cancan_resource_class.add_before_filter(self, {:load => true}, *args)
View
16 lib/cancan/controller_resource.rb
@@ -228,11 +228,19 @@ def name
@name || name_from_controller
end
+ def strong_parameters?
+ @params.class.name == 'ActionController::Parameters'
+ end
+
def resource_params
- if @options[:class]
- @params[@options[:class].to_s.underscore.gsub('/', '_')]
- else
- @params[namespaced_name.to_s.underscore.gsub("/", "_")]
+ if [:create, :update].member? @params[:action].to_sym
+ param_name = @options[:instance_name] || (@options[:class] || namespaced_name).to_s.underscore.gsub('/', '_')
+ if strong_parameters? || @options[:params]
+ params_method = (@options[:params] == true || ! @options[:params]) ?
+ "#{param_name}_params" : @options[:params]
+ return @controller.send params_method if @controller.send :respond_to?, params_method
+ end
+ @params[param_name]
end
end
View
15 spec/cancan/controller_resource_spec.rb
@@ -96,7 +96,7 @@ class Project < ::Project; end
end
it "overrides initial attributes with params" do
- @params.merge!(:action => "new", :project => {:name => "from params"})
+ @params.merge!(:action => "create", :project => {:name => "from params"})
@ability.can(:create, :projects, :name => "from conditions")
CanCan::ControllerResource.new(@controller, :load => true).process
@controller.instance_variable_get(:@project).name.should == "from params"
@@ -467,6 +467,19 @@ class Project < ::Project; end
lambda { resource.process }.should_not raise_error(CanCan::Unauthorized)
end
+ it "should allow controller methods for parameter pre-processing" do
+ @params.merge!(:action => "create")
+ @controller.class.send(:define_method, :project_parameters) do { :name => 'foobar'} end
+ CanCan::ControllerResource.new(@controller, :load => true, :params => :project_parameters).process
+ @controller.instance_variable_get(:@project).name.should == "foobar"
+ end
+
+ it "should revert back to parameters if the method does not exist within the controller" do
+ @params.merge!(:action => "create", :project => { :name => 'foobar' })
+ CanCan::ControllerResource.new(@controller, :load => true, :params => true).process
+ @controller.instance_variable_get(:@project).name.should == "foobar"
+ end
+
# it "raises ImplementationRemoved when adding :name option" do
# lambda {
# CanCan::ControllerResource.new(@controller, :name => :foo)
View
2  spec/cancan/inherited_resource_spec.rb
@@ -49,7 +49,7 @@
end
it "should override initial attributes with params" do
- @params.merge!(:action => "new", :project => {:name => "from params"})
+ @params.merge!(:action => "create", :project => {:name => "from params"})
@ability.can(:create, :projects, :name => "from conditions")
@controller.stub(:build_resource) { Struct.new(:name).new }
CanCan::ControllerResource.new(@controller, :load => true).process
Something went wrong with that request. Please try again.