Moved some permission checks for issue update from controller to model.

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@4393 e93f8b46-1217-0410-a6f0-8f06a7374b81

3 files changed, 123 insertions, 17 deletions

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index 641847729..befc88f78 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -150,11 +150,7 @@ class IssuesController < ApplicationController
       end
     end
   end
-  
-  # Attributes that can be updated on workflow transition (without :edit permission)
-  # TODO: make it configurable (at least per role)
-  UPDATABLE_ATTRS_ON_TRANSITION = %w(status_id assigned_to_id fixed_version_id done_ratio) unless const_defined?(:UPDATABLE_ATTRS_ON_TRANSITION)
-  
+    
   def edit
     update_issue_from_params
 
@@ -276,14 +272,7 @@ private
     
     @notes = params[:notes] || (params[:issue].present? ? params[:issue][:notes] : nil)
     @issue.init_journal(User.current, @notes)
-    # User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
-    if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
-      attrs = params[:issue].dup
-      attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
-      attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
-      @issue.safe_attributes = attrs
-    end
-
+    @issue.safe_attributes = params[:issue]
   end
 
   # TODO: Refactor, lots of extra code in here
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 68d6a2bd5..d47ab74ad 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -233,16 +233,34 @@ class Issue < ActiveRecord::Base
     lock_version
   ) unless const_defined?(:SAFE_ATTRIBUTES)
   
+  SAFE_ATTRIBUTES_ON_TRANSITION = %w(
+    status_id
+    assigned_to_id
+    fixed_version_id
+    done_ratio
+  ) unless const_defined?(:SAFE_ATTRIBUTES_ON_TRANSITION)
+
   # Safely sets attributes
   # Should be called from controllers instead of #attributes=
   # attr_accessible is too rough because we still want things like
   # Issue.new(:project => foo) to work
   # TODO: move workflow/permission checks from controllers to here
   def safe_attributes=(attrs, user=User.current)
-    return if attrs.nil?
-    attrs = attrs.reject {|k,v| !SAFE_ATTRIBUTES.include?(k)}
+    return unless attrs.is_a?(Hash)
+    
+    new_statuses_allowed = new_statuses_allowed_to(user)
+    
+    # User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
+    if new_record? || user.allowed_to?(:edit_issues, project)
+      attrs = attrs.reject {|k,v| !SAFE_ATTRIBUTES.include?(k)}
+    elsif new_statuses_allowed.any?
+      attrs = attrs.reject {|k,v| !SAFE_ATTRIBUTES_ON_TRANSITION.include?(k)}
+    else
+      return
+    end
+    
     if attrs['status_id']
-      unless new_statuses_allowed_to(user).collect(&:id).include?(attrs['status_id'].to_i)
+      unless new_statuses_allowed.collect(&:id).include?(attrs['status_id'].to_i)
         attrs.delete('status_id')
       end
     end
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index e81556d5f..ab02acf00 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -580,7 +580,7 @@ class IssuesControllerTest < ActionController::TestCase
   context "without workflow privilege" do
     setup do
       Workflow.delete_all(["role_id = ?", Role.anonymous.id])
-      Role.anonymous.add_permission! :add_issues
+      Role.anonymous.add_permission! :add_issues, :add_issue_notes
     end
     
     context "#new" do
@@ -605,6 +605,17 @@ class IssuesControllerTest < ActionController::TestCase
         assert_equal IssueStatus.default, issue.status
       end
       
+      should "accept default status" do
+        assert_difference 'Issue.count' do
+          post :create, :project_id => 1, 
+                     :issue => {:tracker_id => 1,
+                                :subject => 'This is an issue',
+                                :status_id => 1}
+        end
+        issue = Issue.last(:order => 'id')
+        assert_equal IssueStatus.default, issue.status
+      end
+      
       should "ignore unauthorized status" do
         assert_difference 'Issue.count' do
           post :create, :project_id => 1, 
@@ -616,6 +627,94 @@ class IssuesControllerTest < ActionController::TestCase
         assert_equal IssueStatus.default, issue.status
       end
     end
+    
+    context "#update" do
+      should "ignore status change" do
+        assert_difference 'Journal.count' do
+          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 3}
+        end
+        assert_equal 1, Issue.find(1).status_id
+      end
+      
+      should "ignore attributes changes" do
+        assert_difference 'Journal.count' do
+          put :update, :id => 1, :notes => 'just trying', :issue => {:subject => 'changed', :assigned_to_id => 2}
+        end
+        issue = Issue.find(1)
+        assert_equal "Can't print recipes", issue.subject
+        assert_nil issue.assigned_to
+      end
+    end
+  end
+  
+  context "with workflow privilege" do
+    setup do
+      Workflow.delete_all(["role_id = ?", Role.anonymous.id])
+      Workflow.create!(:role => Role.anonymous, :tracker_id => 1, :old_status_id => 1, :new_status_id => 3)
+      Workflow.create!(:role => Role.anonymous, :tracker_id => 1, :old_status_id => 1, :new_status_id => 4)
+      Role.anonymous.add_permission! :add_issues, :add_issue_notes
+    end
+    
+    context "#update" do
+      should "accept authorized status" do
+        assert_difference 'Journal.count' do
+          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 3}
+        end
+        assert_equal 3, Issue.find(1).status_id
+      end
+      
+      should "ignore unauthorized status" do
+        assert_difference 'Journal.count' do
+          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 2}
+        end
+        assert_equal 1, Issue.find(1).status_id
+      end
+      
+      should "accept authorized attributes changes" do
+        assert_difference 'Journal.count' do
+          put :update, :id => 1, :notes => 'just trying', :issue => {:assigned_to_id => 2}
+        end
+        issue = Issue.find(1)
+        assert_equal 2, issue.assigned_to_id
+      end
+      
+      should "ignore unauthorized attributes changes" do
+        assert_difference 'Journal.count' do
+          put :update, :id => 1, :notes => 'just trying', :issue => {:subject => 'changed'}
+        end
+        issue = Issue.find(1)
+        assert_equal "Can't print recipes", issue.subject
+      end
+    end
+    
+    context "and :edit_issues permission" do
+      setup do
+        Role.anonymous.add_permission! :add_issues, :edit_issues
+      end
+
+      should "accept authorized status" do
+        assert_difference 'Journal.count' do
+          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 3}
+        end
+        assert_equal 3, Issue.find(1).status_id
+      end
+      
+      should "ignore unauthorized status" do
+        assert_difference 'Journal.count' do
+          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 2}
+        end
+        assert_equal 1, Issue.find(1).status_id
+      end
+      
+      should "accept authorized attributes changes" do
+        assert_difference 'Journal.count' do
+          put :update, :id => 1, :notes => 'just trying', :issue => {:subject => 'changed', :assigned_to_id => 2}
+        end
+        issue = Issue.find(1)
+        assert_equal "changed", issue.subject
+        assert_equal 2, issue.assigned_to_id
+      end
+    end
   end
   
   def test_copy_issue