Making initial changes to add authorize after update

porting #668 to master branch

CHANGELOG.rdoc

@@ -1,3 +1,15 @@
+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

cancan.gemspec

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

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?
@@ -225,7 +235,7 @@ module CanCan
     end
 
     def namespace
-      @params[:controller].split("::")[0..-2]
+      @params[:controller].split(/::|\//)[0..-2]
     end
 
     def namespaced_name
@@ -250,6 +260,10 @@ module CanCan
       [:new, :create] + [@options[:new]].flatten
     end
 
+    def update_actions
+      [:update] #TODO: check what @options is for
+    end
+
     private
 
     def extract_key(value)

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

lib/cancan/rule.rb

@@ -116,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

spec/cancan/ability_spec.rb

@@ -269,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)
@@ -148,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)
@@ -156,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
@@ -171,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
@@ -187,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
@@ -238,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
@@ -298,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
@@ -367,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)
@@ -390,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

@@ -207,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]

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