jro_apps/redmine Commit - r4279:0eb7d8f6149c

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

Jean-Philippe Lang -

r4279:0eb7d8f6149c

parent child

Context file:

r4279:0eb7d8f6149c

Collapse all files

app/controllers/issues_controller.rb +2 -13

                   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
                 @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
+                @issue.safe_attributes = params[:issue]
-                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
               end
               # TODO: Refactor, lots of extra code in here

app/models/issue.rb +21 -3

@@ -233,16 +233,34 class Issue < ActiveRecord::Base
233	lock_version	233	lock_version
234	) unless const_defined?(:SAFE_ATTRIBUTES)	234	) unless const_defined?(:SAFE_ATTRIBUTES)
235		235
		236	SAFE_ATTRIBUTES_ON_TRANSITION = %w(
		237	status_id
		238	assigned_to_id
		239	fixed_version_id
		240	done_ratio
		241	) unless const_defined?(:SAFE_ATTRIBUTES_ON_TRANSITION)
		242
236	# Safely sets attributes	243	# Safely sets attributes
237	# Should be called from controllers instead of #attributes=	244	# Should be called from controllers instead of #attributes=
238	# attr_accessible is too rough because we still want things like	245	# attr_accessible is too rough because we still want things like
239	# Issue.new(:project => foo) to work	246	# Issue.new(:project => foo) to work
240	# TODO: move workflow/permission checks from controllers to here	247	# TODO: move workflow/permission checks from controllers to here
241	def safe_attributes=(attrs, user=User.current)	248	def safe_attributes=(attrs, user=User.current)
242	return if attrs.~~nil?~~	249	return unless attrs.is_a?(Hash)
243	attrs = attrs.reject {\|k,v\| !SAFE_ATTRIBUTES.include?(k)}	250
		251	new_statuses_allowed = new_statuses_allowed_to(user)
		252
		253	# User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
		254	if new_record? \|\| user.allowed_to?(:edit_issues, project)
		255	attrs = attrs.reject {\|k,v\| !SAFE_ATTRIBUTES.include?(k)}
		256	elsif new_statuses_allowed.any?
		257	attrs = attrs.reject {\|k,v\| !SAFE_ATTRIBUTES_ON_TRANSITION.include?(k)}
		258	else
		259	return
		260	end
		261
244	if attrs['status_id']	262	if attrs['status_id']
245	unless new_statuses_allowed~~_to~~(~~user~~).collect(&:id).include?(attrs['status_id'].to_i)	263	unless new_statuses_allowed.collect(&:id).include?(attrs['status_id'].to_i)
246	attrs.delete('status_id')	264	attrs.delete('status_id')
247	end	265	end
248	end	266	end

test/functional/issues_controller_test.rb +100 -1

@@ -580,7 +580,7 class IssuesControllerTest < ActionController::TestCase
580	context "without workflow privilege" do	580	context "without workflow privilege" do
581	setup do	581	setup do
582	Workflow.delete_all(["role_id = ?", Role.anonymous.id])	582	Workflow.delete_all(["role_id = ?", Role.anonymous.id])
583	Role.anonymous.add_permission! :add_issues	583	Role.anonymous.add_permission! :add_issues, :add_issue_notes
584	end	584	end
585		585
586	context "#new" do	586	context "#new" do
@@ -605,6 +605,17 class IssuesControllerTest < ActionController::TestCase
605	assert_equal IssueStatus.default, issue.status	605	assert_equal IssueStatus.default, issue.status
606	end	606	end
607		607
		608	should "accept default status" do
		609	assert_difference 'Issue.count' do
		610	post :create, :project_id => 1,
		611	:issue => {:tracker_id => 1,
		612	:subject => 'This is an issue',
		613	:status_id => 1}
		614	end
		615	issue = Issue.last(:order => 'id')
		616	assert_equal IssueStatus.default, issue.status
		617	end
		618
608	should "ignore unauthorized status" do	619	should "ignore unauthorized status" do
609	assert_difference 'Issue.count' do	620	assert_difference 'Issue.count' do
610	post :create, :project_id => 1,	621	post :create, :project_id => 1,
@@ -616,6 +627,94 class IssuesControllerTest < ActionController::TestCase
616	assert_equal IssueStatus.default, issue.status	627	assert_equal IssueStatus.default, issue.status
617	end	628	end
618	end	629	end
		630
		631	context "#update" do
		632	should "ignore status change" do
		633	assert_difference 'Journal.count' do
		634	put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 3}
		635	end
		636	assert_equal 1, Issue.find(1).status_id
		637	end
		638
		639	should "ignore attributes changes" do
		640	assert_difference 'Journal.count' do
		641	put :update, :id => 1, :notes => 'just trying', :issue => {:subject => 'changed', :assigned_to_id => 2}
		642	end
		643	issue = Issue.find(1)
		644	assert_equal "Can't print recipes", issue.subject
		645	assert_nil issue.assigned_to
		646	end
		647	end
		648	end
		649
		650	context "with workflow privilege" do
		651	setup do
		652	Workflow.delete_all(["role_id = ?", Role.anonymous.id])
		653	Workflow.create!(:role => Role.anonymous, :tracker_id => 1, :old_status_id => 1, :new_status_id => 3)
		654	Workflow.create!(:role => Role.anonymous, :tracker_id => 1, :old_status_id => 1, :new_status_id => 4)
		655	Role.anonymous.add_permission! :add_issues, :add_issue_notes
		656	end
		657
		658	context "#update" do
		659	should "accept authorized status" do
		660	assert_difference 'Journal.count' do
		661	put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 3}
		662	end
		663	assert_equal 3, Issue.find(1).status_id
		664	end
		665
		666	should "ignore unauthorized status" do
		667	assert_difference 'Journal.count' do
		668	put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 2}
		669	end
		670	assert_equal 1, Issue.find(1).status_id
		671	end
		672
		673	should "accept authorized attributes changes" do
		674	assert_difference 'Journal.count' do
		675	put :update, :id => 1, :notes => 'just trying', :issue => {:assigned_to_id => 2}
		676	end
		677	issue = Issue.find(1)
		678	assert_equal 2, issue.assigned_to_id
		679	end
		680
		681	should "ignore unauthorized attributes changes" do
		682	assert_difference 'Journal.count' do
		683	put :update, :id => 1, :notes => 'just trying', :issue => {:subject => 'changed'}
		684	end
		685	issue = Issue.find(1)
		686	assert_equal "Can't print recipes", issue.subject
		687	end
		688	end
		689
		690	context "and :edit_issues permission" do
		691	setup do
		692	Role.anonymous.add_permission! :add_issues, :edit_issues
		693	end
		694
		695	should "accept authorized status" do
		696	assert_difference 'Journal.count' do
		697	put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 3}
		698	end
		699	assert_equal 3, Issue.find(1).status_id
		700	end
		701
		702	should "ignore unauthorized status" do
		703	assert_difference 'Journal.count' do
		704	put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 2}
		705	end
		706	assert_equal 1, Issue.find(1).status_id
		707	end
		708
		709	should "accept authorized attributes changes" do
		710	assert_difference 'Journal.count' do
		711	put :update, :id => 1, :notes => 'just trying', :issue => {:subject => 'changed', :assigned_to_id => 2}
		712	end
		713	issue = Issue.find(1)
		714	assert_equal "changed", issue.subject
		715	assert_equal 2, issue.assigned_to_id
		716	end
		717	end
619	end	718	end
620		719
621	def test_copy_issue	720	def test_copy_issue

General Comments 0

Write
Preview

You need to be logged in to leave comments. Login now

No TODOs yet

	Site-wide shortcuts
/	Use quick search box
g h	Goto home page
g g	Goto my private gists page
g G	Goto my public gists page
g 0-9	Goto bookmarked items from 0-9
n r	New repository page
n g	New gist page

	Repositories
g s	Goto summary page
g c	Goto changelog page
g f	Goto files page
g F	Goto files page with file search activated
g p	Goto pull requests page
g o	Goto repository settings
g O	Goto repository access permissions settings
t s	Toggle sidebar on some pages