From 3a2b6c7c9653ddbd32ea0c2434b0c9b638832b3e Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sat, 10 Dec 2016 12:02:37 +0000 Subject: [PATCH] Enforce issue assignee validation (#23921). git-svn-id: http://svn.redmine.org/redmine/trunk@16055 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/models/issue.rb | 24 ++-- app/models/project.rb | 2 +- test/functional/issues_controller_test.rb | 4 +- test/integration/api_test/issues_test.rb | 8 ++ test/unit/issue_test.rb | 128 ++++++++++------------ test/unit/mailer_test.rb | 3 +- test/unit/query_test.rb | 30 +++-- test/unit/user_test.rb | 3 + 8 files changed, 106 insertions(+), 96 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index f1bbe1c78..e49890e1a 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -377,6 +377,11 @@ class Issue < ActiveRecord::Base if category self.category = project.issue_categories.find_by_name(category.name) end + # Clear the assignee if not available in the new project for new issues (eg. copy) + # For existing issue, the previous assignee is still valid, so we keep it + if new_record? && assigned_to && !assignable_users.include?(assigned_to) + self.assigned_to_id = nil + end # Keep the fixed_version if it's still valid in the new_project if fixed_version && fixed_version.project != project && !project.shared_versions.include?(fixed_version) self.fixed_version = nil @@ -538,14 +543,7 @@ class Issue < ActiveRecord::Base self.status = statuses_allowed.first || default_status end if (u = attrs.delete('assigned_to_id')) && safe_attribute?('assigned_to_id') - if u.blank? - self.assigned_to_id = nil - else - u = u.to_i - if assignable_users.any?{|assignable_user| assignable_user.id == u} - self.assigned_to_id = u - end - end + self.assigned_to_id = u end @@ -708,6 +706,12 @@ class Issue < ActiveRecord::Base end end + if assigned_to_id_changed? && assigned_to_id.present? + unless assignable_users.include?(assigned_to) + errors.add :assigned_to_id, :invalid + end + end + # Checks parent issue assignment if @invalid_parent_issue_id.present? errors.add :parent_issue_id, :invalid @@ -868,7 +872,9 @@ class Issue < ActiveRecord::Base def assignable_users users = project.assignable_users(tracker).to_a users << author if author && author.active? - users << assigned_to if assigned_to + if assigned_to_id_was.present? && assignee = Principal.find_by_id(assigned_to_id_was) + users << assignee + end users.uniq.sort end diff --git a/app/models/project.rb b/app/models/project.rb index eecc3851e..2f258f3f9 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -780,7 +780,7 @@ class Project < ActiveRecord::Base def copy(project, options={}) project = project.is_a?(Project) ? project : Project.find(project) - to_be_copied = %w(wiki versions issue_categories issues members queries boards) + to_be_copied = %w(members wiki versions issue_categories issues queries boards) to_be_copied = to_be_copied & Array.wrap(options[:only]) unless options[:only].nil? Project.transaction do diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index 5426679c1..444e02171 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -4440,7 +4440,7 @@ class IssuesControllerTest < Redmine::ControllerTest :assigned_to_id => nil), Issue.create!(:project_id => 2, :tracker_id => 3, :status_id => 2, :priority_id => 1, :subject => 'issue 2', :author_id => 2, - :assigned_to_id => 3) + :assigned_to_id => 2) ] assert_difference 'Issue.count', issues.size do post :bulk_update, :ids => issues.map(&:id), :copy => '1', @@ -4496,7 +4496,7 @@ class IssuesControllerTest < Redmine::ControllerTest post :bulk_update, :ids => [1], :copy => '1', :notes => 'Copying one issue', :issue => { - :project_id => '', :tracker_id => '', :assigned_to_id => '4', + :project_id => '', :tracker_id => '', :status_id => '3', :start_date => '2009-12-01', :due_date => '2009-12-31' } end diff --git a/test/integration/api_test/issues_test.rb b/test/integration/api_test/issues_test.rb index 077ae9601..d4040201e 100644 --- a/test/integration/api_test/issues_test.rb +++ b/test/integration/api_test/issues_test.rb @@ -689,6 +689,14 @@ JSON assert_select 'errors error', :text => "Subject cannot be blank" end + test "PUT /issues/:id.xml with invalid assignee should return error" do + user = User.generate! + put '/issues/6.xml', {:issue => {:assigned_to_id => user.id}}, credentials('jsmith') + + assert_response :unprocessable_entity + assert_select 'errors error', :text => "Assignee is invalid" + end + test "PUT /issues/:id.json" do assert_difference('Journal.count') do put '/issues/6.json', diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb index fe056a70e..7adc53ec4 100644 --- a/test/unit/issue_test.rb +++ b/test/unit/issue_test.rb @@ -243,14 +243,14 @@ class IssueTest < ActiveSupport::TestCase def test_anonymous_should_not_see_private_issues_with_issues_visibility_set_to_default Role.anonymous.update!(:issues_visibility => 'default') - issue = Issue.generate!(:author => User.anonymous, :assigned_to => User.anonymous, :is_private => true) + issue = Issue.generate!(:author => User.anonymous, :is_private => true) assert_nil Issue.where(:id => issue.id).visible(User.anonymous).first assert !issue.visible?(User.anonymous) end def test_anonymous_should_not_see_private_issues_with_issues_visibility_set_to_own assert Role.anonymous.update!(:issues_visibility => 'own') - issue = Issue.generate!(:author => User.anonymous, :assigned_to => User.anonymous, :is_private => true) + issue = Issue.generate!(:author => User.anonymous, :is_private => true) assert_nil Issue.where(:id => issue.id).visible(User.anonymous).first assert !issue.visible?(User.anonymous) end @@ -344,24 +344,27 @@ class IssueTest < ActiveSupport::TestCase def test_visible_scope_for_member_with_groups_should_return_assigned_issues user = User.find(8) assert user.groups.any? - Member.create!(:principal => user.groups.first, :project_id => 1, :role_ids => [2]) + group = user.groups.first + Member.create!(:principal => group, :project_id => 1, :role_ids => [2]) Role.non_member.remove_permission!(:view_issues) - issue = Issue.create!(:project_id => 1, :tracker_id => 1, :author_id => 3, - :status_id => 1, :priority => IssuePriority.all.first, - :subject => 'Assignment test', - :assigned_to => user.groups.first, - :is_private => true) - - Role.find(2).update! :issues_visibility => 'default' - issues = Issue.visible(User.find(8)).to_a - assert issues.any? - assert issues.include?(issue) - - Role.find(2).update! :issues_visibility => 'own' - issues = Issue.visible(User.find(8)).to_a - assert issues.any? - assert_include issue, issues + with_settings :issue_group_assignment => '1' do + issue = Issue.create!(:project_id => 1, :tracker_id => 1, :author_id => 3, + :status_id => 1, :priority => IssuePriority.all.first, + :subject => 'Assignment test', + :assigned_to => group, + :is_private => true) + + Role.find(2).update! :issues_visibility => 'default' + issues = Issue.visible(User.find(8)).to_a + assert issues.any? + assert issues.include?(issue) + + Role.find(2).update! :issues_visibility => 'own' + issues = Issue.visible(User.find(8)).to_a + assert issues.any? + assert_include issue, issues + end end def test_visible_scope_for_member_with_limited_tracker_ids @@ -458,6 +461,7 @@ class IssueTest < ActiveSupport::TestCase def test_visible_and_nested_set_scopes user = User.generate! + Member.create!(:project_id => 1, :principal => user, :role_ids => [1]) parent = Issue.generate!(:assigned_to => user) assert parent.visible?(user) child1 = Issue.generate!(:parent_issue_id => parent.id, :assigned_to => user) @@ -761,13 +765,6 @@ class IssueTest < ActiveSupport::TestCase :project_id => 1, :author => user, :assigned_to => user) assert_equal [1, 2, 3, 4, 5], issue.new_statuses_allowed_to(user).map(&:id) - - group = Group.generate! - group.users << user - issue = Issue.generate!(:tracker => tracker, :status => status, - :project_id => 1, :author => user, - :assigned_to => group) - assert_equal [1, 2, 3, 4, 5], issue.new_statuses_allowed_to(user).map(&:id) end def test_new_statuses_allowed_to_should_consider_group_assignment @@ -775,12 +772,16 @@ class IssueTest < ActiveSupport::TestCase WorkflowTransition.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 4, :author => false, :assignee => true) - user = User.find(2) group = Group.generate! + Member.create!(:project_id => 1, :principal => group, :role_ids => [1]) + + user = User.find(2) group.users << user - issue = Issue.generate!(:author_id => 1, :assigned_to => group) - assert_include 4, issue.new_statuses_allowed_to(user).map(&:id) + with_settings :issue_group_assignment => '1' do + issue = Issue.generate!(:author_id => 1, :assigned_to => group) + assert_include 4, issue.new_statuses_allowed_to(user).map(&:id) + end end def test_new_statuses_allowed_to_should_return_all_transitions_for_admin @@ -895,40 +896,6 @@ class IssueTest < ActiveSupport::TestCase assert_nil issue.custom_field_value(cf2) end - def test_safe_attributes_should_ignore_unassignable_assignee - issue = Issue.new(:project_id => 1, :tracker_id => 1, :author_id => 3, - :status_id => 1, :priority => IssuePriority.all.first, - :subject => 'test_create') - assert issue.valid? - - # locked user, not allowed - issue.safe_attributes=({'assigned_to_id' => '5'}) - assert_nil issue.assigned_to_id - # no member - issue.safe_attributes=({'assigned_to_id' => '1'}) - assert_nil issue.assigned_to_id - # user 2 is ok - issue.safe_attributes=({'assigned_to_id' => '2'}) - assert_equal 2, issue.assigned_to_id - assert issue.save - - issue.reload - assert_equal 2, issue.assigned_to_id - issue.safe_attributes=({'assigned_to_id' => '5'}) - assert_equal 2, issue.assigned_to_id - issue.safe_attributes=({'assigned_to_id' => '1'}) - assert_equal 2, issue.assigned_to_id - # user 3 is also ok - issue.safe_attributes=({'assigned_to_id' => '3'}) - assert_equal 3, issue.assigned_to_id - assert issue.save - - # removal of assignee - issue.safe_attributes=({'assigned_to_id' => ''}) - assert_nil issue.assigned_to_id - assert issue.save - end - def test_editable_custom_field_values_should_return_non_readonly_custom_values cf1 = IssueCustomField.create!(:name => 'Writable field', :field_format => 'string', :is_for_all => true, :tracker_ids => [1, 2]) @@ -1250,6 +1217,15 @@ class IssueTest < ActiveSupport::TestCase assert_equal "125", issue.custom_value_for(2).value end + def test_copy_to_another_project_should_clear_assignee_if_not_valid + issue = Issue.generate!(:project_id => 1, :assigned_to_id => 2) + project = Project.generate! + + issue = Issue.new.copy_from(1) + issue.project = project + assert_nil issue.assigned_to + end + def test_copy_should_copy_status orig = Issue.find(8) assert orig.status != orig.default_status @@ -1759,14 +1735,16 @@ class IssueTest < ActiveSupport::TestCase end test "#copy should not create a journal" do - copy = Issue.find(1).copy({:project_id => 3, :tracker_id => 2, :assigned_to_id => 3}, :link => false) + copy = Issue.find(1).copy({:project_id => 3, :tracker_id => 2}, :link => false) copy.save! assert_equal 0, copy.reload.journals.size end test "#copy should allow assigned_to changes" do - copy = Issue.find(1).copy(:project_id => 3, :tracker_id => 2, :assigned_to_id => 3) - assert_equal 3, copy.assigned_to_id + user = User.generate! + Member.create!(:project_id => 3, :principal => user, :role_ids => [1]) + copy = Issue.find(1).copy(:project_id => 3, :tracker_id => 2, :assigned_to_id => user.id) + assert_equal user.id, copy.assigned_to_id end test "#copy should allow status changes" do @@ -2297,8 +2275,9 @@ class IssueTest < ActiveSupport::TestCase assert !issue.assignable_users.include?(user) end - test "#assignable_users should include the current assignee" do + def test_assignable_users_should_include_the_current_assignee user = User.generate! + Member.create!(:project_id => 1, :principal => user, :role_ids => [1]) issue = Issue.generate!(:assigned_to => user) user.lock! @@ -2674,7 +2653,9 @@ class IssueTest < ActiveSupport::TestCase end test "Issue#recipients should include the assigned to user if the assigned to user is active" do - issue = Issue.generate!(:assigned_to => User.generate!) + user = User.generate! + Member.create!(:project_id => 1, :principal => user, :role_ids => [1]) + issue = Issue.generate!(:assigned_to => user) assert issue.assigned_to, "No assigned_to set for Issue" assert issue.recipients.include?(issue.assigned_to.mail) end @@ -2692,7 +2673,9 @@ class IssueTest < ActiveSupport::TestCase end test "Issue#recipients should not include the assigned user if they are only notified of owned issues" do - issue = Issue.generate!(:assigned_to => User.generate!) + user = User.generate! + Member.create!(:project_id => 1, :principal => user, :role_ids => [1]) + issue = Issue.generate!(:assigned_to => user) issue.assigned_to.update!(:mail_notification => :only_owner) assert !issue.recipients.include?(issue.assigned_to.mail) end @@ -2980,10 +2963,13 @@ class IssueTest < ActiveSupport::TestCase def test_assigned_to_was_with_a_group group = Group.find(10) + Member.create!(:project_id => 1, :principal => group, :role_ids => [1]) - issue = Issue.generate!(:assigned_to => group) - issue.reload.assigned_to = nil - assert_equal group, issue.assigned_to_was + with_settings :issue_group_assignment => '1' do + issue = Issue.generate!(:assigned_to => group) + issue.reload.assigned_to = nil + assert_equal group, issue.assigned_to_was + end end def test_issue_overdue_should_respect_user_timezone diff --git a/test/unit/mailer_test.rb b/test/unit/mailer_test.rb index 8f9763288..b4983f925 100644 --- a/test/unit/mailer_test.rb +++ b/test/unit/mailer_test.rb @@ -627,8 +627,9 @@ class MailerTest < ActiveSupport::TestCase end def test_reminder_should_include_issues_assigned_to_groups - with_settings :default_language => 'en' do + with_settings :default_language => 'en', :issue_group_assignment => '1' do group = Group.generate! + Member.create!(:project_id => 1, :principal => group, :role_ids => [1]) group.users << User.find(2) group.users << User.find(3) diff --git a/test/unit/query_test.rb b/test/unit/query_test.rb index 5c34d58c3..6d27106cc 100644 --- a/test/unit/query_test.rb +++ b/test/unit/query_test.rb @@ -676,19 +676,25 @@ class QueryTest < ActiveSupport::TestCase def test_filter_assigned_to_me user = User.find(2) group = Group.find(10) - User.current = user - i1 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => user) - i2 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => group) - i3 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => Group.find(11)) group.users << user + other_group = Group.find(11) + Member.create!(:project_id => 1, :principal => group, :role_ids => [1]) + Member.create!(:project_id => 1, :principal => other_group, :role_ids => [1]) + User.current = user - query = IssueQuery.new(:name => '_', :filters => { 'assigned_to_id' => {:operator => '=', :values => ['me']}}) - result = query.issues - assert_equal Issue.visible.where(:assigned_to_id => ([2] + user.reload.group_ids)).sort_by(&:id), result.sort_by(&:id) - - assert result.include?(i1) - assert result.include?(i2) - assert !result.include?(i3) + with_settings :issue_group_assignment => '1' do + i1 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => user) + i2 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => group) + i3 = Issue.generate!(:project_id => 1, :tracker_id => 1, :assigned_to => other_group) + + query = IssueQuery.new(:name => '_', :filters => { 'assigned_to_id' => {:operator => '=', :values => ['me']}}) + result = query.issues + assert_equal Issue.visible.where(:assigned_to_id => ([2] + user.reload.group_ids)).sort_by(&:id), result.sort_by(&:id) + + assert result.include?(i1) + assert result.include?(i2) + assert !result.include?(i3) + end end def test_user_custom_field_filtered_on_me @@ -1689,7 +1695,7 @@ class QueryTest < ActiveSupport::TestCase @issue1 = Issue.generate!(:project => @project, :assigned_to_id => @manager.id) @issue2 = Issue.generate!(:project => @project, :assigned_to_id => @developer.id) @issue3 = Issue.generate!(:project => @project, :assigned_to_id => @boss.id) - @issue4 = Issue.generate!(:project => @project, :assigned_to_id => @guest.id) + @issue4 = Issue.generate!(:project => @project, :author_id => @guest.id, :assigned_to_id => @guest.id) @issue5 = Issue.generate!(:project => @project) @query = IssueQuery.new(:name => '_', :project => @project) diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index ff513a276..d39694e94 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -1140,6 +1140,7 @@ class UserTest < ActiveSupport::TestCase project = Project.find(1) author = User.generate! assignee = User.generate! + Member.create!(:user => assignee, :project => project, :role_ids => [1]) member = User.generate! Member.create!(:user => member, :project => project, :role_ids => [1]) issue = Issue.generate!(:project => project, :assigned_to => assignee, :author => author) @@ -1160,7 +1161,9 @@ class UserTest < ActiveSupport::TestCase def test_notify_about_issue_for_previous_assignee assignee = User.generate!(:mail_notification => 'only_assigned') + Member.create!(:user => assignee, :project_id => 1, :role_ids => [1]) new_assignee = User.generate!(:mail_notification => 'only_assigned') + Member.create!(:user => new_assignee, :project_id => 1, :role_ids => [1]) issue = Issue.generate!(:assigned_to => assignee) assert assignee.notify_about?(issue) -- 2.39.5