From 555acea7804e6a93b133fe7398f490cb083c05b2 Mon Sep 17 00:00:00 2001
From: Go MAEDA <maeda@farend.jp>
Date: Thu, 2 Mar 2023 04:32:34 +0000
Subject: [PATCH] Check if the user has the permission to add notes or edit an
 issue when adding an issue attachments (#38297).

Patch by Holger Just.


git-svn-id: https://svn.redmine.org/redmine/trunk@22122 e93f8b46-1217-0410-a6f0-8f06a7374b81
---
 app/controllers/issues_controller.rb | 12 ++++-
 app/models/issue.rb                  |  4 ++
 app/views/issues/_edit.html.erb      |  3 +-
 test/integration/issues_test.rb      | 74 ++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index c3f8ec8d5..07de47c0d 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -192,8 +192,16 @@ class IssuesController < ApplicationController
   def update
     return unless update_issue_from_params
 
-    @issue.save_attachments(params[:attachments] ||
-                             (params[:issue] && params[:issue][:uploads]))
+    attachments = params[:attachments] || params.dig(:issue, :uploads)
+    if @issue.attachments_addable?
+      @issue.save_attachments(attachments)
+    else
+      attachments = attachments.to_unsafe_hash if attachments.respond_to?(:to_unsafe_hash)
+      if [Hash, Array].any? { |klass| attachments.is_a?(klass) } && attachments.any?
+        flash[:warning] = l(:warning_attachments_not_saved, attachments.size)
+      end
+    end
+
     saved = false
     begin
       saved = save_issue_with_child_records
diff --git a/app/models/issue.rb b/app/models/issue.rb
index a0c2006ad..f267f3f48 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -199,6 +199,10 @@ class Issue < ActiveRecord::Base
     )
   end
 
+  def attachments_addable?(user=User.current)
+    attributes_editable?(user) || notes_addable?(user)
+  end
+
   # Overrides Redmine::Acts::Attachable::InstanceMethods#attachments_editable?
   def attachments_editable?(user=User.current)
     attributes_editable?(user)
diff --git a/app/views/issues/_edit.html.erb b/app/views/issues/_edit.html.erb
index 226b6f988..d816ff6fc 100644
--- a/app/views/issues/_edit.html.erb
+++ b/app/views/issues/_edit.html.erb
@@ -42,7 +42,8 @@
 
       <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %>
       </fieldset>
-
+    <% end %>
+    <% if @issue.attachments_addable? %>
       <fieldset id="add_attachments"><legend><%= l(:label_attachment_plural) %></legend>
         <% if @issue.attachments.any? && @issue.safe_attribute?('deleted_attachment_ids') %>
         <div class="contextual"><%= link_to l(:label_edit_attachments), '#', :onclick => "$('#existing-attachments').toggle(); return false;" %></div>
diff --git a/test/integration/issues_test.rb b/test/integration/issues_test.rb
index c9f5c3537..fe9cb19d0 100644
--- a/test/integration/issues_test.rb
+++ b/test/integration/issues_test.rb
@@ -140,6 +140,80 @@ class IssuesTest < Redmine::IntegrationTest
     assert_equal 0, Issue.find(1).attachments.length
   end
 
+  def test_edit_add_attachment_form
+    log_user('jsmith', 'jsmith')
+    role = Role.find(1)
+
+    role.add_permission! :edit_issues
+    role.remove_permission! :edit_own_issues
+    role.remove_permission! :add_issue_notes
+
+    get '/issues/1'
+    assert_response :success
+    assert_select 'div#new-attachments', 1
+
+    get '/issues/1/edit'
+    assert_response :success
+    assert_select 'div#new-attachments', 1
+
+    role.remove_permission! :edit_issues
+    role.add_permission! :edit_own_issues
+    role.remove_permission! :add_issue_notes
+
+    get '/issues/1'
+    assert_response :success
+    assert_select 'div#new-attachments', 1
+
+    get '/issues/1/edit'
+    assert_response :success
+    assert_select 'div#new-attachments', 1
+
+    role.remove_permission! :edit_issues
+    role.remove_permission! :edit_own_issues
+    role.add_permission! :add_issue_notes
+
+    get '/issues/1'
+    assert_response :success
+    assert_select 'div#new-attachments', 1
+
+    get '/issues/1/edit'
+    assert_response :success
+    assert_select 'div#new-attachments', 1
+  end
+
+  def test_edit_check_permission_for_add_attachment
+    log_user('jsmith', 'jsmith')
+    role = Role.find(1)
+
+    role.remove_permission! :edit_issues
+    role.remove_permission! :edit_own_issues
+    role.add_permission! :add_issue_notes
+
+    role.permissions_all_trackers = {'view_issues' => '0', 'add_issue_notes' => '0' }
+    role.permissions_tracker_ids = {'view_issues' => ['1'], 'add_issue_notes' => ['2'] }
+    role.save!
+
+    assert_no_difference 'Attachment.count' do
+      put(
+        '/issues/1',
+        :params => {
+          :issue => {:notes => 'Some notes'},
+          :attachments => {
+            '1' => {
+              'file' => uploaded_test_file('testfile.txt', 'text/plain'),
+              'description' => 'This is an attachment'
+            }
+          }
+        }
+      )
+    end
+    assert_redirected_to '/issues/1'
+
+    follow_redirect!
+    assert_response :success
+    assert_select '.flash', '1 file(s) could not be saved.'
+  end
+
   def test_next_and_previous_links_should_be_displayed_after_query_grouped_and_sorted_by_version
     with_settings :default_language => 'en' do
       get '/projects/ecookbook/issues?set_filter=1&group_by=fixed_version&sort=priority:desc,fixed_version,id'
-- 
2.39.5