diff --git a/app/models/issue.rb b/app/models/issue.rb index a1066c4c2..2f36f6779 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -194,7 +194,11 @@ class Issue < ActiveRecord::Base # Returns true if user or current user is allowed to add notes to the issue def notes_addable?(user=User.current) - user_tracker_permission?(user, :add_issue_notes) + is_addable = user_tracker_permission?(user, :add_issue_notes) + if self.closed? && !self.closing? + is_addable &&= user.allowed_to?(:add_note_to_closed_issue, self.project) + end + is_addable end # Returns true if user or current user is allowed to delete the issue diff --git a/app/models/mail_handler.rb b/app/models/mail_handler.rb index 4259eacb2..4c1a827f8 100644 --- a/app/models/mail_handler.rb +++ b/app/models/mail_handler.rb @@ -225,9 +225,9 @@ class MailHandler < ActionMailer::Base # check permission unless handler_options[:no_permission_check] - unless user.allowed_to?(:add_issue_notes, issue.project) || - user.allowed_to?(:edit_issues, issue.project) - raise UnauthorizedAction, "not allowed to add notes on issues to project [#{project.name}]" + if !user.allowed_to?(:edit_issues, issue.project) && + (!user.allowed_to?(:add_issue_notes, issue.project) || (issue.closed? && !user.allowed_to?(:add_note_to_closed_issue, issue.project))) + raise UnauthorizedAction, "not allowed to add notes on issues to project [#{issue.project.name}]" end end diff --git a/lib/redmine.rb b/lib/redmine.rb index 668a65da2..6ad51696a 100644 --- a/lib/redmine.rb +++ b/lib/redmine.rb @@ -111,6 +111,7 @@ Redmine::AccessControl.map do |map| map.permission :set_issues_private, {} map.permission :set_own_issues_private, {}, :require => :loggedin map.permission :add_issue_notes, {:issues => [:edit, :update], :journals => [:new], :attachments => :upload} + map.permission :add_note_to_closed_issue, {:issues => [:edit, :update], :journals => [:new], :attachments => :upload} map.permission :edit_issue_notes, {:journals => [:edit, :update]}, :require => :loggedin map.permission :edit_own_issue_notes, {:journals => [:edit, :update]}, :require => :loggedin map.permission :view_private_notes, {}, :read => true, :require => :member diff --git a/lib/redmine/default_data/loader.rb b/lib/redmine/default_data/loader.rb index 272351fc6..a8c9a816a 100644 --- a/lib/redmine/default_data/loader.rb +++ b/lib/redmine/default_data/loader.rb @@ -64,6 +64,7 @@ module Redmine :manage_issue_relations, :manage_subtasks, :add_issue_notes, + :add_note_to_closed_issue, :save_queries, :view_gantt, :view_calendar, @@ -91,6 +92,7 @@ module Redmine :permissions => [:view_issues, :add_issues, :add_issue_notes, + :add_note_to_closed_issue, :save_queries, :view_gantt, :view_calendar, @@ -111,6 +113,7 @@ module Redmine Role.non_member.update_attribute :permissions, [:view_issues, :add_issues, :add_issue_notes, + :add_note_to_closed_issue, :save_queries, :view_gantt, :view_calendar, diff --git a/test/fixtures/roles.yml b/test/fixtures/roles.yml index 13433874e..a1b1010f1 100644 --- a/test/fixtures/roles.yml +++ b/test/fixtures/roles.yml @@ -21,6 +21,7 @@ roles_001: - :manage_issue_relations - :manage_subtasks - :add_issue_notes + - :add_note_to_closed_issue - :delete_issues - :view_issue_watchers - :add_issue_watchers @@ -87,6 +88,7 @@ roles_002: - :manage_issue_relations - :manage_subtasks - :add_issue_notes + - :add_note_to_closed_issue - :delete_issues - :view_issue_watchers - :save_queries @@ -135,6 +137,7 @@ roles_003: - :edit_issues - :manage_issue_relations - :add_issue_notes + - :add_note_to_closed_issue - :view_issue_watchers - :save_queries - :view_gantt @@ -174,6 +177,7 @@ roles_004: - :edit_issues - :manage_issue_relations - :add_issue_notes + - :add_note_to_closed_issue - :save_queries - :view_gantt - :view_calendar @@ -203,6 +207,7 @@ roles_005: --- - :view_issues - :add_issue_notes + - :add_note_to_closed_issue - :view_gantt - :view_calendar - :view_time_entries diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index 62d4894c3..f1a7e61c6 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -6388,6 +6388,44 @@ class IssuesControllerTest < Redmine::ControllerTest assert_equal 2, issue.reload.assigned_to_id end + def test_update_without_add_note_to_closed_issue_permission_when_closed + @request.session[:user_id] = 2 + Role.find(2).remove_permission! :add_note_to_closed_issue + issue = Issue.find(4) + issue.close! + + assert_no_difference 'Journal.count' do + put( + :update, + :params => { + :id => issue.id, + :issue => { + :notes => 'notes' + } + } + ) + end + end + + def test_update_without_add_note_to_closed_issue_permission_when_closing + @request.session[:user_id] = 2 + Role.find(2).remove_permission! :add_note_to_closed_issue + issue = Issue.find(4) + + assert_difference 'Journal.count' do + put( + :update, + :params => { + :id => issue.id, + :issue => { + :status_id => 5, + :notes => 'notes' + } + } + ) + end + end + def test_get_bulk_edit @request.session[:user_id] = 2 get(:bulk_edit, :params => {:ids => [1, 3]}) diff --git a/test/unit/mail_handler_test.rb b/test/unit/mail_handler_test.rb index 1672edccb..cea57b134 100644 --- a/test/unit/mail_handler_test.rb +++ b/test/unit/mail_handler_test.rb @@ -903,6 +903,28 @@ class MailHandlerTest < ActiveSupport::TestCase assert !journal.notes.match(/^Start Date:/i) end + def test_update_issue_without_permission_should_raise_an_error + Role.find(1).remove_permission! :add_note_to_closed_issue + Role.find(1).remove_permission! :edit_issues + Issue.find(2).close! + + Rails.logger.expects(:error).with('MailHandler: unauthorized attempt from John Smith: not allowed to add notes on issues to project [eCookbook]') + assert_no_difference 'Journal.count' do + submit_email('ticket_reply_with_status.eml') + end + end + + def test_update_issue_without_permission_should_add_journal_when_cloging + Role.find(1).remove_permission! :add_note_to_closed_issue + Role.find(1).remove_permission! :edit_issues + + assert_difference 'Journal.count' do + submit_email('ticket_reply_with_status.eml') do |raw| + raw.gsub! /^Status: .*$/, 'Status: Closed' + end + end + end + def test_update_issue_with_attachment assert_difference 'Journal.count' do assert_difference 'JournalDetail.count' do