kolan
/
Redmine
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

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
This commit is contained in:
Jean-Philippe Lang 2010-11-11 16:37:16 +00:00
parent aa84e6c179
commit 0eb7d8f614
3 changed files with 123 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
app/controllers/issues_controller.rb
View File

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

24
app/models/issue.rb
View File

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

101
test/functional/issues_controller_test.rb
View File

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