Making initial changes to add authorize after update

porting #668 to master branch

CHANGELOG.rdoc

@@ -1,3 +1,26 @@
+1.6.10 (May 7, 2013)
+
+* fix matches_conditons_hash for string values on 1.8 (thanks rrosen)
+
+* work around SQL injection vulnerability in older Rails versions (thanks steerio) - issue #800
+
+* add support for nested join conditions (thanks yuszuv) - issue #806
+
+* fix load_resource "find_by" in mongoid resources (thanks albertobajo) - issue #705
+
+* fix namespace split behavior (thanks xinuc) - issue #668
+
+1.6.9 (February 4, 2013)
+
+* fix inserting AND (NULL) to end of SQL queries (thanks jonsgreen) - issue #687
+
+* fix merge_joins for nested association hashes (thanks DavidMikeSimon) - issues #655, #560
+
+* raise error on recursive alias_action (thanks fl00r) - issue #660
+
+* fix namespace controllers not loading params (thanks andhapp) - issues #670, #664
+
+
 1.6.8 (June 25, 2012)
 
 * improved support for namespaced controllers and models

CONTRIBUTING.md

@@ -0,0 +1,11 @@
+### Please read before contributing
+
+1) If you have any questions about CanCan, search the [Wiki](https://github.com/ryanb/cancan/wiki) or use [Stack Overflow](http://stackoverflow.com/questions/tagged/cancan). Do not post questions here.
+
+2) If you find a security bug, **DO NOT** submit an issue here. Please send an e-mail to [ryan@railscasts.com](mailto:ryan@railscasts.com) instead.
+
+3) Do a small search on the issues tracker before submitting your issue to see if it was already reported / fixed. In case it was not, create your report including Rails and CanCan versions. If you are getting exceptions, please include the full backtrace.
+
+That's it! The more information you give, the more easy it becomes for us to track it down and fix it. Ideal scenario would be adding the issue to CanCan test suite or to a sample application.
+
+Thanks!

README.rdoc

@@ -1,4 +1,4 @@
-= CanCan {<img src="https://secure.travis-ci.org/ryanb/cancan.png" />}[http://travis-ci.org/ryanb/cancan]
+= CanCan {<img src="https://fury-badge.herokuapp.com/rb/cancan.png" alt="Gem Version" />}[http://badge.fury.io/rb/cancan] {<img src="https://secure.travis-ci.org/ryanb/cancan.png?branch=master" />}[http://travis-ci.org/ryanb/cancan] {<img src="https://codeclimate.com/badge.png" />}[https://codeclimate.com/github/ryanb/cancan]
 
 Wiki[https://github.com/ryanb/cancan/wiki] | RDocs[http://rdoc.info/projects/ryanb/cancan] | Screencast[http://railscasts.com/episodes/192-authorization-with-cancan]
 

cancan.gemspec

@@ -1,6 +1,6 @@
 Gem::Specification.new do |s|
   s.name        = "cancan"
-  s.version     = "1.6.8"
+  s.version     = "1.6.10"
   s.author      = "Ryan Bates"
   s.email       = "ryan@railscasts.com"
   s.homepage    = "http://github.com/ryanb/cancan"

lib/cancan/ability.rb

@@ -172,10 +172,16 @@ module CanCan
     # This way one can use params[:action] in the controller to determine the permission.
     def alias_action(*args)
       target = args.pop[:to]
+      validate_target(target)
       aliased_actions[target] ||= []
       aliased_actions[target] += args
     end
 
+    # User shouldn't specify targets with names of real actions or it will cause Seg fault
+    def validate_target(target)
+      raise Error, "You can't specify target (#{target}) as alias because it is real action name" if aliased_actions.values.flatten.include? target
+    end
+
     # Returns a hash of aliased actions. The key is the target and the value is an array of actions aliasing the key.
     def aliased_actions
       @aliased_actions ||= default_alias_actions

lib/cancan/controller_additions.rb

@@ -96,6 +96,11 @@ module CanCan
       #
       #     load_resource :find_by => :permalink # will use find_by_permalink!(params[:id])
       #
+      # [:+id_param+]
+      #   Find using a param key other than :id. For example:
+      #
+      #     load_resource :id_key => :url # will use find(params[:url])
+      #
       # [:+collection+]
       #   Specify which actions are resource collection actions in addition to :+index+. This
       #   is usually not necessary because it will try to guess depending on if the id param is present.

lib/cancan/controller_resource.rb

@@ -24,6 +24,10 @@ module CanCan
     def load_and_authorize_resource
       load_resource
       authorize_resource
+      if update_actions.include?(@params[:action].to_sym)
+        update_resource
+        authorize_resource # Reauthorize the now-updated resource
+      end
     end
 
     def load_resource
@@ -86,6 +90,10 @@ module CanCan
       assign_attributes(resource)
     end
 
+    def update_resource
+      resource.assign_attributes(resource_params || {})
+    end
+
     def assign_attributes(resource)
       resource.send("#{parent_name}=", parent_resource) if @options[:singleton] && parent_resource
       initial_attributes.each do |attr_name, value|
@@ -107,6 +115,8 @@ module CanCan
         if @options[:find_by]
           if resource_base.respond_to? "find_by_#{@options[:find_by]}!"
             resource_base.send("find_by_#{@options[:find_by]}!", id_param)
+          elsif resource_base.respond_to? "find_by"
+            resource_base.send("find_by", { @options[:find_by].to_sym => id_param })
           else
             resource_base.send(@options[:find_by], id_param)
           end
@@ -129,7 +139,7 @@ module CanCan
         @params[@options[:id_param]]
       else
         @params[parent? ? :"#{name}_id" : :id]
-      end
+      end.to_s
     end
 
     def member_action?
@@ -213,14 +223,19 @@ module CanCan
 
     def resource_params
       if @options[:class]
-        @params[@options[:class].to_s.underscore.gsub('/', '_')]
-      else
-        @params[namespaced_name.to_s.underscore.gsub("/", "_")]
+        params_key = extract_key(@options[:class])
+        return @params[params_key] if @params[params_key]
       end
+
+      resource_params_by_namespaced_name
+    end
+
+    def resource_params_by_namespaced_name
+      @params[extract_key(namespaced_name)]
     end
 
     def namespace
-      @params[:controller].split("::")[0..-2]
+      @params[:controller].split(/::|\//)[0..-2]
     end
 
     def namespaced_name
@@ -244,5 +259,15 @@ module CanCan
     def new_actions
       [:new, :create] + [@options[:new]].flatten
     end
+
+    def update_actions
+      [:update] #TODO: check what @options is for
+    end
+
+    private
+
+    def extract_key(value)
+       value.to_s.underscore.gsub('/', '_')
+    end
   end
 end

lib/cancan/model_adapters/active_record_adapter.rb

@@ -66,11 +66,22 @@ module CanCan
         return conditions unless conditions.kind_of? Hash
         conditions.inject({}) do |result_hash, (name, value)|
           if value.kind_of? Hash
+            value = value.dup
             association_class = model_class.reflect_on_association(name).class_name.constantize
-            name = model_class.reflect_on_association(name).table_name.to_sym
-            value = tableized_conditions(value, association_class)
+            nested = value.inject({}) do |nested,(k,v)|
+              if v.kind_of? Hash
+                value.delete(k)
+                nested[k] = v
+              else
+                name = model_class.reflect_on_association(name).table_name.to_sym
+                result_hash[name] = value
+              end
+              nested
+            end
+            result_hash.merge!(tableized_conditions(nested,association_class))
+          else
+            result_hash[name] = value
           end
-          result_hash[name] = value
           result_hash
         end
       end
@@ -145,8 +156,8 @@ module CanCan
       # Takes two hashes and does a deep merge.
       def merge_joins(base, add)
         add.each do |name, nested|
-          if base[name].is_a?(Hash) && !nested.empty?
-            merge_joins(base[name], nested)
+          if base[name].is_a?(Hash)
+            merge_joins(base[name], nested) unless nested.empty?
           else
             base[name] = nested
           end

lib/cancan/rule.rb

@@ -55,7 +55,8 @@ module CanCan
     end
 
     def unmergeable?
-      @conditions.respond_to?(:keys) && (! @conditions.keys.first.kind_of? Symbol)
+      @conditions.respond_to?(:keys) && @conditions.present? &&
+        (!@conditions.keys.first.kind_of? Symbol)
     end
 
     def associations_hash(conditions = @conditions)
@@ -115,7 +116,7 @@ module CanCan
                 else
                   !attribute.nil? && matches_conditions_hash?(attribute, value)
                 end
-              elsif value.kind_of?(Enumerable)
+              elsif !value.is_a?(String) && value.kind_of?(Enumerable)
                 value.include? attribute
               else
                 attribute == value

lib/generators/cancan/ability/templates/ability.rb

@@ -11,18 +11,22 @@ class Ability
     #     can :read, :all
     #   end
     #
-    # The first argument to `can` is the action you are giving the user permission to do.
-    # If you pass :manage it will apply to every action. Other common actions here are
-    # :read, :create, :update and :destroy.
+    # The first argument to `can` is the action you are giving the user 
+    # permission to do.
+    # If you pass :manage it will apply to every action. Other common actions
+    # here are :read, :create, :update and :destroy.
     #
-    # The second argument is the resource the user can perform the action on. If you pass
-    # :all it will apply to every resource. Otherwise pass a Ruby class of the resource.
+    # The second argument is the resource the user can perform the action on. 
+    # If you pass :all it will apply to every resource. Otherwise pass a Ruby
+    # class of the resource.
     #
-    # The third argument is an optional hash of conditions to further filter the objects.
+    # The third argument is an optional hash of conditions to further filter the
+    # objects.
     # For example, here the user can only update published articles.
     #
     #   can :update, Article, :published => true
     #
-    # See the wiki for details: https://github.com/ryanb/cancan/wiki/Defining-Abilities
+    # See the wiki for details:
+    # https://github.com/ryanb/cancan/wiki/Defining-Abilities
   end
 end

spec/cancan/ability_spec.rb

@@ -87,6 +87,10 @@ describe CanCan::Ability do
     @ability.can?(:increment, 123).should be_true
   end
 
+  it "should raise an Error if alias target is an exist action" do
+    lambda{ @ability.alias_action :show, :to => :show }.should raise_error(CanCan::Error, "You can't specify target (show) as alias because it is real action name")
+  end
+
   it "should always call block with arguments when passing no arguments to can" do
     @ability.can do |action, object_class, object|
       action.should == :foo
@@ -265,6 +269,12 @@ describe CanCan::Ability do
     @ability.can?(:read, []).should be_false
   end
 
+  it "should match strings but not substrings specified in a conditions hash" do
+    @ability.can :read, String, :presence => "declassified"
+    @ability.can?(:read, "declassified").should be_true
+    @ability.can?(:read, "classified").should be_false
+  end
+
   it "should not stop at cannot definition when comparing class" do
     @ability.can :read, Range
     @ability.cannot :read, Range, :begin => 1

spec/cancan/controller_resource_spec.rb

@@ -20,7 +20,7 @@ describe CanCan::ControllerResource do
   end
 
   it "should not load resource into an instance variable if already set" do
-    @params.merge!(:action => "show", :id => 123)
+    @params.merge!(:action => "show", :id => "123")
     @controller.instance_variable_set(:@project, :some_project)
     resource = CanCan::ControllerResource.new(@controller)
     resource.load_resource
@@ -67,6 +67,16 @@ describe CanCan::ControllerResource do
     @controller.instance_variable_get(:@project).should == project
   end
 
+  it "has the specified nested resource_class when using / for namespace" do
+    module Admin
+      class Dashboard; end
+    end
+    @ability.can(:index, "admin/dashboard")
+    @params.merge!(:controller => "admin/dashboard", :action => "index")
+    resource = CanCan::ControllerResource.new(@controller, :authorize => true)
+    resource.send(:resource_class).should == Admin::Dashboard
+  end
+
   it "should build a new resource with hash if params[:id] is not specified" do
     @params.merge!(:action => "create", :project => {:name => "foobar"})
     resource = CanCan::ControllerResource.new(@controller)
@@ -75,13 +85,19 @@ describe CanCan::ControllerResource do
   end
 
   it "should build a new resource for namespaced model with hash if params[:id] is not specified" do
-    project = Sub::Project.create!
     @params.merge!(:action => "create", 'sub_project' => {:name => "foobar"})
     resource = CanCan::ControllerResource.new(@controller, :class => ::Sub::Project)
     resource.load_resource
     @controller.instance_variable_get(:@project).name.should == "foobar"
   end
 
+  it "should build a new resource for namespaced controller and namespaced model with hash if params[:id] is not specified" do
+    @params.merge!(:controller => "Admin::SubProjectsController", :action => "create", 'sub_project' => {:name => "foobar"})
+    resource = CanCan::ControllerResource.new(@controller, :class => Project)
+    resource.load_resource
+    @controller.instance_variable_get(:@sub_project).name.should == "foobar"
+  end
+
   it "should build a new resource with attributes from current ability" do
     @params.merge!(:action => "new")
     @ability.can(:create, Project, :name => "from conditions")
@@ -142,7 +158,7 @@ describe CanCan::ControllerResource do
   end
 
   it "should perform authorization using controller action and loaded model" do
-    @params.merge!(:action => "show", :id => 123)
+    @params.merge!(:action => "show", :id => "123")
     @controller.instance_variable_set(:@project, :some_project)
     stub(@controller).authorize!(:show, :some_project) { raise CanCan::AccessDenied }
     resource = CanCan::ControllerResource.new(@controller)
@@ -150,14 +166,14 @@ describe CanCan::ControllerResource do
   end
 
   it "should perform authorization using controller action and non loaded model" do
-    @params.merge!(:action => "show", :id => 123)
+    @params.merge!(:action => "show", :id => "123")
     stub(@controller).authorize!(:show, Project) { raise CanCan::AccessDenied }
     resource = CanCan::ControllerResource.new(@controller)
     lambda { resource.authorize_resource }.should raise_error(CanCan::AccessDenied)
   end
 
   it "should call load_resource and authorize_resource for load_and_authorize_resource" do
-    @params.merge!(:action => "show", :id => 123)
+    @params.merge!(:action => "show", :id => "123")
     resource = CanCan::ControllerResource.new(@controller)
     mock(resource).load_resource
     mock(resource).authorize_resource
@@ -165,7 +181,7 @@ describe CanCan::ControllerResource do
   end
 
   it "should not build a single resource when on custom collection action even with id" do
-    @params.merge!(:action => "sort", :id => 123)
+    @params.merge!(:action => "sort", :id => "123")
     resource = CanCan::ControllerResource.new(@controller, :collection => [:sort, :list])
     resource.load_resource
     @controller.instance_variable_get(:@project).should be_nil
@@ -181,7 +197,7 @@ describe CanCan::ControllerResource do
   end
 
   it "should build a resource when on custom new action even when params[:id] exists" do
-    @params.merge!(:action => "build", :id => 123)
+    @params.merge!(:action => "build", :id => "123")
     stub(Project).new { :some_project }
     resource = CanCan::ControllerResource.new(@controller, :new => :build)
     resource.load_resource
@@ -232,30 +248,30 @@ describe CanCan::ControllerResource do
   end
 
   it "should load resource through the association of another parent resource using instance variable" do
-    @params.merge!(:action => "show", :id => 123)
+    @params.merge!(:action => "show", :id => "123")
     category = Object.new
     @controller.instance_variable_set(:@category, category)
-    stub(category).projects.stub!.find(123) { :some_project }
+    stub(category).projects.stub!.find("123") { :some_project }
     resource = CanCan::ControllerResource.new(@controller, :through => :category)
     resource.load_resource
     @controller.instance_variable_get(:@project).should == :some_project
   end
 
   it "should load resource through the custom association name" do
-    @params.merge!(:action => "show", :id => 123)
+    @params.merge!(:action => "show", :id => "123")
     category = Object.new
     @controller.instance_variable_set(:@category, category)
-    stub(category).custom_projects.stub!.find(123) { :some_project }
+    stub(category).custom_projects.stub!.find("123") { :some_project }
     resource = CanCan::ControllerResource.new(@controller, :through => :category, :through_association => :custom_projects)
     resource.load_resource
     @controller.instance_variable_get(:@project).should == :some_project
   end
 
   it "should load resource through the association of another parent resource using method" do
-    @params.merge!(:action => "show", :id => 123)
+    @params.merge!(:action => "show", :id => "123")
     category = Object.new
     stub(@controller).category { category }
-    stub(category).projects.stub!.find(123) { :some_project }
+    stub(category).projects.stub!.find("123") { :some_project }
     resource = CanCan::ControllerResource.new(@controller, :through => :category)
     resource.load_resource
     @controller.instance_variable_get(:@project).should == :some_project
@@ -292,10 +308,10 @@ describe CanCan::ControllerResource do
   end
 
   it "should load through first matching if multiple are given" do
-    @params.merge!(:action => "show", :id => 123)
+    @params.merge!(:action => "show", :id => "123")
     category = Object.new
     @controller.instance_variable_set(:@category, category)
-    stub(category).projects.stub!.find(123) { :some_project }
+    stub(category).projects.stub!.find("123") { :some_project }
     resource = CanCan::ControllerResource.new(@controller, :through => [:category, :user])
     resource.load_resource
     @controller.instance_variable_get(:@project).should == :some_project
@@ -361,7 +377,7 @@ describe CanCan::ControllerResource do
   end
 
   it "should authorize based on resource name if class is false" do
-    @params.merge!(:action => "show", :id => 123)
+    @params.merge!(:action => "show", :id => "123")
     stub(@controller).authorize!(:show, :project) { raise CanCan::AccessDenied }
     resource = CanCan::ControllerResource.new(@controller, :class => false)
     lambda { resource.authorize_resource }.should raise_error(CanCan::AccessDenied)
@@ -384,6 +400,13 @@ describe CanCan::ControllerResource do
     @controller.instance_variable_get(:@project).should == project
   end
 
+  # CVE-2012-5664
+  it "should always convert id param to string" do
+    @params.merge!(:action => "show", :the_project => { :malicious => "I am" })
+    resource = CanCan::ControllerResource.new(@controller, :id_param => :the_project)
+    resource.send(:id_param).class.should == String
+  end
+
   it "should load resource using custom find_by attribute" do
     project = Project.create!(:name => "foo")
     @params.merge!(:action => "show", :id => "foo")

spec/cancan/model_adapters/active_record_adapter_spec.rb

@@ -20,10 +20,12 @@ if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
         t.boolean "secret"
         t.integer "priority"
         t.integer "category_id"
+        t.integer "user_id"
       end
       model do
         belongs_to :category
         has_many :comments
+        belongs_to :user
       end
     end
 
@@ -37,6 +39,15 @@ if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
       end
     end
 
+    with_model :user do
+      table do |t|
+
+      end
+      model do
+        has_many :articles
+      end
+    end
+
     before(:each) do
       Article.delete_all
       Comment.delete_all
@@ -196,6 +207,16 @@ if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
       @ability.model_adapter(Article, :read).conditions.should == "'t'='t'"
     end
 
+    it "should return appropriate sql conditions in complex case with nested joins" do
+      @ability.can :read, Comment, :article => { :category => { :visible => true } }
+      @ability.model_adapter(Comment, :read).conditions.should == { Category.table_name.to_sym => { :visible => true } }
+    end
+
+    it "should return appropriate sql conditions in complex case with nested joins of different depth" do
+      @ability.can :read, Comment, :article => { :published => true, :category => { :visible => true } }
+      @ability.model_adapter(Comment, :read).conditions.should == { Article.table_name.to_sym => { :published => true }, Category.table_name.to_sym => { :visible => true } }
+    end
+
     it "should not forget conditions when calling with SQL string" do
       @ability.can :read, Article, :published => true
       @ability.can :read, Article, ['secret=?', false]
@@ -227,6 +248,21 @@ if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
       @ability.model_adapter(Article, :read).joins.should == [:project]
     end
 
+    it "should merge nested and non-nested joins" do
+      @ability.can :read, Article, :project => { :blocked => false }
+      @ability.can :read, Article, :project => { :comments => { :spam => true } }
+      @ability.model_adapter(Article, :read).joins.should == [{:project=>[:comments]}]
+    end
+
+    it "should merge :all conditions with other conditions" do
+      user = User.create!
+      article = Article.create!(:user => user)
+      ability = Ability.new(user)
+      ability.can :manage, :all
+      ability.can :manage, Article, :user_id => user.id
+      Article.accessible_by(ability).should == [article]
+    end
+
     it "should restrict articles given a MetaWhere condition" do
       @ability.can :read, Article, :priority.lt => 2
       article1 = Article.create!(:priority => 1)

spec/cancan/rule_spec.rb

@@ -44,4 +44,9 @@ describe CanCan::Rule do
 
     @rule.should be_unmergeable
   end
+
+  it "should be mergeable if conditions is an empty hash" do
+    @conditions = {}
+    @rule.should_not be_unmergeable
+  end
 end

spec/spec_helper.rb

@@ -20,6 +20,18 @@ RSpec.configure do |config|
   config.extend WithModel if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
 end
 
+# Working around CVE-2012-5664 requires us to convert all ID params
+# to strings. Let's switch to using string IDs in tests, otherwise
+# SuperModel and/or RR will fail (as strings are not fixnums).
+
+module SuperModel
+  class Base
+    def generate_id
+      object_id.to_s
+    end
+  end
+end
+
 class Ability
   include CanCan::Ability