From 0eb7d8f6149c8b328cbc612035b8b11e4be4b382 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Thu, 11 Nov 2010 16:37:16 +0000 Subject: [PATCH] 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 --- app/controllers/issues_controller.rb | 15 +--- app/models/issue.rb | 24 ++++- test/functional/issues_controller_test.rb | 101 +++++++++++++++++++++- 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 -- 2.39.5