https://www.redmine.org/https://www.redmine.org/favicon.ico?16793021292021-04-19T22:27:17ZRedmineRedmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1021092021-04-19T22:27:17ZMarius BĂLTEANU
<ul></ul><p>From <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: Allow users to edit issues without adding notes. (New)" href="https://www.redmine.org/issues/17599#note-2">#17599#note-2</a>:</p>
<p>Jean-Philippe Lang wrote:</p>
<blockquote>
<p>The "Add notes" permission lets users add notes without editing the issue. But whenever a user is allowed to edit an issue, he is allowed to add notes by design.</p>
</blockquote>
<p>I think we should stick to that decision and if we really think that it's better to change the design, then it should be addressed in a new issue.</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1021322021-04-20T09:16:18ZHolger Just
<ul><li><strong>Related to</strong> <i><a class="issue tracker-2 status-1 priority-4 priority-default" href="/issues/17599">Feature #17599</a>: Allow users to edit issues without adding notes.</i> added</li></ul> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1021362021-04-20T09:20:36ZHolger Just
<ul></ul><p>Thanks for confirming!</p>
<p>In that case, the patch from #33689 should be reverted. The permissions check for add_issue_notes then needs to be updated so that the notes form field is also shown if the user ONLY has the <code>edit_issues</code> permission. #33689 and <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: Allow users to edit issues without adding notes. (New)" href="https://www.redmine.org/issues/17599">#17599</a> should then be changed to closed/wont fix.</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1021842021-04-22T22:25:02ZMarius BĂLTEANU
<ul><li><strong>Assignee</strong> set to <i>Go MAEDA</i></li></ul><p>Go Maeda, #33689 was handled by you, what do you think about this?</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022002021-04-23T08:47:20ZGo MAEDA
<ul></ul><p>As Holger wrote, it is a problem that users cannot add notes even though you have edit_issue permission.</p>
<p>I think that users who do not have either edit_issue or add_notes permissions should not be able to add notes via API, but I am not against reverting #33689 for now.</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022032021-04-23T10:20:29ZHolger Just
<ul></ul><p>If we revert the behavior change of #33689, we should at the same time adapt <code>Issue#notes_addable?</code> to bring the UI behavior in line with that of the MailHandler and the API, e.g.</p>
<pre>
def notes_addable(user=User.current)
user_tracker_permission?(user, :add_issue_notes) || user_tracker_permission?(user, :edit_issues)
end
</pre>
<p>(Note that I have not conclusively tested the above code for now.)</p>
<p>Apart from the changing tests, this should however allow to retain the model change in #33689.</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022042021-04-23T10:23:23ZMarius BĂLTEANU
<ul></ul><p>I agree with you, Holger. Unfortunately, until Saturday night, I don't have time to work on this.</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022132021-04-25T08:23:19ZMarius BĂLTEANU
<ul></ul><p>Looking deeper in the code, I've observed that <a class="changeset" title="Adds permission to edit and delete issues by role/tracker (#285)." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/15466">r15466</a> for <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Tracker role-based permissioning (Closed)" href="https://www.redmine.org/issues/285">#285</a> changed the existing behaviour and explicit required the <code>add_notes</code> permission to allow users to add notes to an issue. Since then, the <code>add_notes</code> permission is configurable per each tracker (together with <code>view_issues</code>, <code>add_issues</code>, <code>edit_issues</code> and <code>delete_issues</code>) which makes harder to revert to the old behaviour. Also, considering those granular permissions, it is less confusing if we keep them separated without any inheritance.</p>
<p>Considering these new findings, I'm in favour of properly check the permission when the notes are added from email and add <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: Allow users to edit issues without adding notes. (New)" href="https://www.redmine.org/issues/17599">#17599</a> to Changelog.</p>
<p>What do you think?</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022142021-04-25T08:43:51ZGo MAEDA
<ul></ul><p>Marius BALTEANU wrote:</p>
<blockquote>
<p>Considering these new findings, I'm in favour of properly check the permission when the notes are added from email and add <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: Allow users to edit issues without adding notes. (New)" href="https://www.redmine.org/issues/17599">#17599</a> to Changelog.</p>
</blockquote>
<p>It is rather a big change, so it's probably better to apply the change in Redmine 5.0 instead of a minor version. The change may cause some existing instances to suddenly stop updating issues via email.</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022152021-04-25T08:55:36ZMarius BĂLTEANU
<ul></ul><p>How this change is different with the one from #33689 which was delivered in a minor release? From my point of view, it's the same thing.</p>
<p>I agree with you that this change was big, but considering that it's in the UI since Redmine 3.3.0 and in the API since Redmine 4.0.8, I don't see a real reason to wait for Redmine 5.0.0 to have these permissions consistent.</p>
<p>The patch is the following:<br /><pre><code class="patch syntaxhl"><span class="gd">--- a/app/models/mail_handler.rb
</span><span class="gi">+++ b/app/models/mail_handler.rb
</span><span class="p">@@ -225,8 +225,7 @@</span> class MailHandler < ActionMailer::Base
# check permission
unless handler_options[:no_permission_check]
<span class="gd">- unless user.allowed_to?(:add_issue_notes, issue.project) ||
- user.allowed_to?(:edit_issues, issue.project)
</span><span class="gi">+ unless issue.notes_addable?
</span> raise UnauthorizedAction, "not allowed to add notes on issues to project [#{issue.project.name}]"
end
end
<span class="gh">diff --git a/test/unit/mail_handler_test.rb b/test/unit/mail_handler_test.rb
index 836df11d6..3fd3ce072 100644
</span><span class="gd">--- a/test/unit/mail_handler_test.rb
</span><span class="gi">+++ b/test/unit/mail_handler_test.rb
</span><span class="p">@@ -1051,9 +1051,11 @@</span> class MailHandlerTest < ActiveSupport::TestCase
end
end
- def test_reply_to_a_issue_without_permission
<span class="gi">+ def test_reply_to_an_issue_without_permission
</span> set_tmp_attachments_directory
<span class="gd">- Role.all.each {|r| r.remove_permission! :add_issue_notes, :edit_issues}
</span><span class="gi">+ # "add_issue_notes" permission is explicit required to allow users to add notes
+ # "edit_issue" permission no longer includes the "add_issue_notes" permission
+ Role.all.each {|r| r.remove_permission! :add_issue_notes}
</span> assert_no_difference 'Issue.count' do
assert_no_difference 'Journal.count' do
assert_not submit_email('ticket_reply_with_status.eml')
<span class="err">(END)</span>
</code></pre></p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022162021-04-25T08:58:04ZJean-Philippe Langjp_lang@yahoo.fr
<ul></ul><p>Marius BALTEANU wrote:</p>
<blockquote>
<p>Also, considering those granular permissions, it is less confusing if we keep them separated without any inheritance.</p>
</blockquote>
<p>I agree with this. Even if editing an issue with having the ability to add note may not be very common, it makes it more clear to separate permissions.</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022172021-04-25T10:45:46ZMarius BĂLTEANU
<ul><li><strong>Target version</strong> set to <i>4.0.9</i></li></ul> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022182021-04-25T12:47:18ZGo MAEDA
<ul><li><strong>Subject</strong> changed from <i>Inconsistent permissions for issue_edit and add_issue_notes</i> to <i>Mail handler bypasses add_issue_notes permission</i></li></ul> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022192021-04-25T13:33:00ZGo MAEDA
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Resolved</i></li></ul><p>Committed the change <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Mail handler bypasses add_issue_notes permission (Closed)" href="https://www.redmine.org/issues/35045#note-11">#35045#note-11</a>.</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022342021-04-26T16:19:30ZMarius BĂLTEANU
<ul><li><strong>Status</strong> changed from <i>Resolved</i> to <i>Closed</i></li></ul> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1022762021-04-28T08:24:19ZHolger Just
<ul></ul><p>CVE-2021-31864 has been assigned for this.</p> Redmine - Defect #35045: Mail handler bypasses add_issue_notes permissionhttps://www.redmine.org/issues/35045?journal_id=1070202022-06-21T06:11:32ZMarius BĂLTEANU
<ul><li><strong>Project</strong> changed from <i>2</i> to <i>Redmine</i></li><li><strong>Category</strong> set to <i>Email receiving</i></li><li><strong>Resolution</strong> set to <i>Fixed</i></li></ul>