##// END OF EJS Templates
jro_apps » redmine
RSS
Moved some permission checks for issue update from controller to model....
Jean-Philippe Lang -
r4279:0eb7d8f6149c
parent child
Show More
Side by Side Unified
Context file:
r4279:0eb7d8f6149c Loading diff...:
Show file before | Show file after | Hide comments
@@ -150,11 +150,7 class IssuesController < ApplicationController
150 150 end
151 151 end
152 152 end
153
154 # Attributes that can be updated on workflow transition (without :edit permission)
155 # TODO: make it configurable (at least per role)
156 UPDATABLE_ATTRS_ON_TRANSITION = %w(status_id assigned_to_id fixed_version_id done_ratio) unless const_defined?(:UPDATABLE_ATTRS_ON_TRANSITION)
157
153
158 154 def edit
159 155 update_issue_from_params
160 156
@@ -276,14 +272,7 private
276 272
277 273 @notes = params[:notes] || (params[:issue].present? ? params[:issue][:notes] : nil)
278 274 @issue.init_journal(User.current, @notes)
279 # User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
280 if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
281 attrs = params[:issue].dup
282 attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
283 attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
284 @issue.safe_attributes = attrs
285 end
286
275 @issue.safe_attributes = params[:issue]
287 276 end
288 277
289 278 # TODO: Refactor, lots of extra code in here
Show file before | Show file after | Hide comments
@@ -233,16 +233,34 class Issue < ActiveRecord::Base
233 233 lock_version
234 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 243 # Safely sets attributes
237 244 # Should be called from controllers instead of #attributes=
238 245 # attr_accessible is too rough because we still want things like
239 246 # Issue.new(:project => foo) to work
240 247 # TODO: move workflow/permission checks from controllers to here
241 248 def safe_attributes=(attrs, user=User.current)
242 return if attrs.nil?
243 attrs = attrs.reject {|k,v| !SAFE_ATTRIBUTES.include?(k)}
249 return unless attrs.is_a?(Hash)
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 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 264 attrs.delete('status_id')
247 265 end
248 266 end
Show file before | Show file after | Hide comments
@@ -580,7 +580,7 class IssuesControllerTest < ActionController::TestCase
580 580 context "without workflow privilege" do
581 581 setup do
582 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 584 end
585 585
586 586 context "#new" do
@@ -605,6 +605,17 class IssuesControllerTest < ActionController::TestCase
605 605 assert_equal IssueStatus.default, issue.status
606 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 619 should "ignore unauthorized status" do
609 620 assert_difference 'Issue.count' do
610 621 post :create, :project_id => 1,
@@ -616,6 +627,94 class IssuesControllerTest < ActionController::TestCase
616 627 assert_equal IssueStatus.default, issue.status
617 628 end
618 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 718 end
620 719
621 720 def test_copy_issue
General Comments 0
You need to be logged in to leave comments. Login now