Fixes #3086 - Deletion of communication article works for admins

app/assets/javascripts/app/controllers/ticket_zoom/article_action/delete.coffee

@@ -18,7 +18,6 @@ class Delete
     actions
 
   @isDeletable: (actions, ticket, article, ui) ->
-    return { isDeletable: true }  if ui.permissionCheck('admin')
     return { isDeletable: false } if !@deletableForAgent(actions, ticket, article, ui)
     return { isDeletable: true }  if !@hasDeletableTimeframe()
 
@@ -44,7 +43,7 @@ class Delete
   @deletableForAgent: (actions, ticket, article, ui) ->
     return false if !ui.permissionCheck('ticket.agent')
     return false if article.created_by_id != App.User.current()?.id
-    return false if article.type.communication and !article.internal
+    return false if article.type.communication
 
     true
 

app/policies/ticket/article_policy.rb

@@ -16,17 +16,26 @@ class Ticket::ArticlePolicy < ApplicationPolicy
   end
 
   def destroy?
-    return true if user.permissions?('admin')
+    return false if !access?('show?')
-    return false if !access?(__method__)
+
-  # don't let edge case exceptions raised in the TicketPolicy stop
-  # other possible positive authorization checks
-  rescue Pundit::NotAuthorizedError
     # agents can destroy articles of type 'note'
-    # which were created by themselves within the last 10 minutes
+    # which were created by themselves within the last x minutes
-    return missing_admin_permission if !user.permissions?('ticket.agent')
+
-    return missing_admin_permission if record.created_by_id != user.id
+    if !user.permissions?('ticket.agent')
-    return missing_admin_permission if record.type.communication? && !record.internal?
+      return not_authorized('agent permission required') if !user.permissions?('ticket.agent')
-    return too_old_to_undo          if deletable_timeframe? && record.created_at <= deletable_timeframe.ago
+    end
+
+    if record.created_by_id != user.id
+      return not_authorized('you can only delete your own notes')
+    end
+
+    if record.type.communication?
+      return not_authorized('communication articles cannot be deleted')
+    end
+
+    if deletable_timeframe? && record.created_at <= deletable_timeframe.ago
+      return not_authorized('note is too old to be deleted')
+    end
 
     true
   end
@@ -53,12 +62,4 @@ class Ticket::ArticlePolicy < ApplicationPolicy
     ticket = Ticket.lookup(id: record.ticket_id)
     Pundit.authorize(user, ticket, query)
   end
-
-  def missing_admin_permission
-    not_authorized('admin permission required')
-  end
-
-  def too_old_to_undo
-    not_authorized('articles more than 10 minutes old may not be deleted')
-  end
 end

spec/requests/ticket/article_spec.rb

@@ -481,6 +481,8 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
   end
 
   describe 'DELETE /api/v1/ticket_articles/:id', authenticated_as: -> { user } do
+    let(:other_agent) { create(:agent, groups: [Group.first]) }
+
     let(:ticket) do
       create(:ticket, group: Group.first)
     end
@@ -491,10 +493,16 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
              updated_by_id: agent.id, created_by_id: agent.id )
     end
 
-    let(:article_note) do
+    let(:article_note_self) do
       create(:ticket_article,
              sender_name: 'Agent', internal: true, type_name: 'note', ticket: ticket,
-             updated_by_id: agent.id, created_by_id: agent.id )
+             updated_by_id: user.id, created_by_id: user.id )
+    end
+
+    let(:article_note_other) do
+      create(:ticket_article,
+             sender_name: 'Agent', internal: true, type_name: 'note', ticket: ticket,
+             updated_by_id: other_agent.id, created_by_id: other_agent.id )
     end
 
     let(:article_note_customer) do
@@ -503,12 +511,20 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
              updated_by_id: customer.id, created_by_id: customer.id )
     end
 
-    let(:article_note_communication) do
+    let(:article_note_communication_self) do
       create(:ticket_article_type, name: 'note_communication', communication: true)
 
       create(:ticket_article,
              sender_name: 'Agent', internal: true, type_name: 'note_communication', ticket: ticket,
-             updated_by_id: agent.id, created_by_id: agent.id )
+             updated_by_id: user.id, created_by_id: user.id )
+    end
+
+    let(:article_note_communication_other) do
+      create(:ticket_article_type, name: 'note_communication', communication: true)
+
+      create(:ticket_article,
+             sender_name: 'Agent', internal: true, type_name: 'note_communication', ticket: ticket,
+             updated_by_id: other_agent.id, created_by_id: other_agent.id )
     end
 
     def delete_article_via_rest(article)
@@ -552,19 +568,27 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
 
       include_examples 'deleting',
                        item: 'article_communication',
-                       now: true, later: true, much_later: true
+                       now: false, later: false, much_later: false
 
       include_examples 'deleting',
-                       item: 'article_note',
+                       item: 'article_note_self',
-                       now: true, later: true, much_later: true
+                       now: true, later: true, much_later: false
+
+      include_examples 'deleting',
+                       item: 'article_note_other',
+                       now: false, later: false, much_later: false
 
       include_examples 'deleting',
                        item: 'article_note_customer',
-                       now: true, later: true, much_later: true
+                       now: false, later: false, much_later: false
 
       include_examples 'deleting',
-                       item: 'article_note_communication',
+                       item: 'article_note_communication_self',
-                       now: true, later: true, much_later: true
+                       now: false, later: false, much_later: false
+
+      include_examples 'deleting',
+                       item: 'article_note_communication_other',
+                       now: false, later: false, much_later: false
     end
 
     context 'as agent' do
@@ -575,17 +599,24 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
                        now: false, later: false, much_later: false
 
       include_examples 'deleting',
-                       item: 'article_note',
+                       item: 'article_note_self',
                        now: true, later: true, much_later: false
 
+      include_examples 'deleting',
+                       item: 'article_note_other',
+                       now: false, later: false, much_later: false
+
       include_examples 'deleting',
                        item: 'article_note_customer',
                        now: false, later: false, much_later: false
 
       include_examples 'deleting',
-                       item: 'article_note_communication',
+                       item: 'article_note_communication_self',
-                       now: true, later: true, much_later: false
+                       now: false, later: false, much_later: false
 
+      include_examples 'deleting',
+                       item: 'article_note_communication_other',
+                       now: false, later: false, much_later: false
     end
 
     context 'as customer' do
@@ -596,7 +627,7 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
                        now: false, later: false, much_later: false
 
       include_examples 'deleting',
-                       item: 'article_note',
+                       item: 'article_note_other',
                        now: false, later: false, much_later: false
 
       include_examples 'deleting',
@@ -604,7 +635,11 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
                        now: false, later: false, much_later: false
 
       include_examples 'deleting',
-                       item: 'article_note_communication',
+                       item: 'article_note_communication_self',
+                       now: false, later: false, much_later: false
+
+      include_examples 'deleting',
+                       item: 'article_note_communication_other',
                        now: false, later: false, much_later: false
 
     end
@@ -612,15 +647,21 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
     context 'with custom timeframe' do
       before { Setting.set 'ui_ticket_zoom_article_delete_timeframe', 6000 }
 
-      let(:article) { article_note }
+      let(:article) { article_note_self }
 
       context 'as admin' do
         let(:user) { admin }
 
+        context 'deleting before timeframe' do
+          before { article && travel(5000.seconds) }
+
+          include_examples 'succeeds'
+        end
+
         context 'deleting after timeframe' do
           before { article && travel(8000.seconds) }
 
-          include_examples 'succeeds'
+          include_examples 'fails'
         end
       end
 
@@ -644,7 +685,7 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
     context 'with timeframe as 0' do
       before { Setting.set 'ui_ticket_zoom_article_delete_timeframe', 0 }
 
-      let(:article) { article_note }
+      let(:article) { article_note_self }
 
       context 'as agent' do
         let(:user) { agent }

spec/requests/ticket_spec.rb

@@ -894,7 +894,7 @@ RSpec.describe 'Ticket', type: :request do
       delete "/api/v1/ticket_articles/#{json_response['id']}", params: {}, as: :json
       expect(response).to have_http_status(:unauthorized)
       expect(json_response).to be_a_kind_of(Hash)
-      expect(json_response['error']).to eq('Not authorized (admin permission required)!')
+      expect(json_response['error']).to eq('Not authorized (communication articles cannot be deleted)!')
 
       delete "/api/v1/tickets/#{ticket.id}", params: {}, as: :json
       expect(response).to have_http_status(:unauthorized)
@@ -991,7 +991,7 @@ RSpec.describe 'Ticket', type: :request do
       expect(json_response['type_id']).to eq(Ticket::Article::Type.lookup(name: 'email').id)
 
       delete "/api/v1/ticket_articles/#{json_response['id']}", params: {}, as: :json
-      expect(response).to have_http_status(:ok)
+      expect(response).to have_http_status(:unauthorized)
 
       delete "/api/v1/tickets/#{ticket.id}", params: {}, as: :json
       expect(response).to have_http_status(:ok)
@@ -1237,7 +1237,7 @@ RSpec.describe 'Ticket', type: :request do
       delete "/api/v1/ticket_articles/#{article_json_response['id']}", params: {}, as: :json
       expect(response).to have_http_status(:unauthorized)
       expect(json_response).to be_a_kind_of(Hash)
-      expect(json_response['error']).to eq('Not authorized (admin permission required)!')
+      expect(json_response['error']).to eq('Not authorized (agent permission required)!')
 
       params = {
         ticket_id: ticket.id,
@@ -1261,7 +1261,7 @@ RSpec.describe 'Ticket', type: :request do
       delete "/api/v1/ticket_articles/#{json_response['id']}", params: {}, as: :json
       expect(response).to have_http_status(:unauthorized)
       expect(json_response).to be_a_kind_of(Hash)
-      expect(json_response['error']).to eq('Not authorized (admin permission required)!')
+      expect(json_response['error']).to eq('Not authorized (agent permission required)!')
 
       params = {
         from:      'something which should not be changed on server side',

spec/system/ticket/zoom_spec.rb

@@ -208,28 +208,39 @@ RSpec.describe 'Ticket zoom', type: :system do
   end
 
   describe 'delete article', authenticated_as: :user do
-    let(:admin)    { create :admin, groups: [Group.first] }
+    let(:admin)       { create :admin, groups: [Group.first] }
-    let(:agent)    { create :agent, groups: [Group.first] }
+    let(:agent)       { create :agent, groups: [Group.first] }
-    let(:customer) { create :customer }
+    let(:other_agent) { create :agent, groups: [Group.first] }
-    let(:ticket)        { create :ticket, group: agent.groups.first, customer: customer }
+    let(:customer)    { create :customer }
-    let(:article)       { send(item) }
+    let(:ticket)      { create :ticket, group: agent.groups.first, customer: customer }
+    let(:article)     { send(item) }
 
     def article_communication
       create_ticket_article(sender_name: 'Agent', internal: false, type_name: 'email', updated_by: customer)
     end
 
-    def article_note
+    def article_note_self
-      create_ticket_article(sender_name: 'Agent', internal: true, type_name: 'note', updated_by: agent)
+      create_ticket_article(sender_name: 'Agent', internal: true, type_name: 'note', updated_by: user)
+    end
+
+    def article_note_other
+      create_ticket_article(sender_name: 'Agent', internal: true, type_name: 'note', updated_by: other_agent)
     end
 
     def article_note_customer
       create_ticket_article(sender_name: 'Customer', internal: false, type_name: 'note', updated_by: customer)
     end
 
-    def article_note_communication
+    def article_note_communication_self
       create(:ticket_article_type, name: 'note_communication', communication: true)
 
-      create_ticket_article(sender_name: 'Agent', internal: true, type_name: 'note_communication', updated_by: agent)
+      create_ticket_article(sender_name: 'Agent', internal: true, type_name: 'note_communication', updated_by: user)
+    end
+
+    def article_note_communication_other
+      create(:ticket_article_type, name: 'note_communication', communication: true)
+
+      create_ticket_article(sender_name: 'Agent', internal: true, type_name: 'note_communication', updated_by: other_agent)
     end
 
     def create_ticket_article(sender_name:, internal:, type_name:, updated_by:)
@@ -243,7 +254,7 @@ RSpec.describe 'Ticket zoom', type: :system do
     context 'going through full stack' do
       context 'as admin' do
         let(:user)   { admin }
-        let(:item)   { 'article_communication' }
+        let(:item)   { 'article_note_self' }
         let(:offset) { 0.minutes }
 
         it 'succeeds' do
@@ -298,19 +309,27 @@ RSpec.describe 'Ticket zoom', type: :system do
 
         include_examples 'deleting ticket article',
                          item: 'article_communication',
-                         now: true, later: true, much_later: true
+                         now: false, later: false, much_later: false
 
         include_examples 'deleting ticket article',
-                         item: 'article_note',
+                         item: 'article_note_self',
-                         now: true, later: true, much_later: true
+                         now: true, later: true, much_later: false
+
+        include_examples 'deleting ticket article',
+                         item: 'article_note_other',
+                         now: false, later: false, much_later: false
 
         include_examples 'deleting ticket article',
                          item: 'article_note_customer',
-                         now: true, later: true, much_later: true
+                         now: false, later: false, much_later: false
 
         include_examples 'deleting ticket article',
-                         item: 'article_note_communication',
+                         item: 'article_note_communication_self',
-                         now: true, later: true, much_later: true
+                         now: false, later: false, much_later: false
+
+        include_examples 'deleting ticket article',
+                         item: 'article_note_communication_other',
+                         now: false, later: false, much_later: false
       end
 
       context 'as agent' do
@@ -321,16 +340,24 @@ RSpec.describe 'Ticket zoom', type: :system do
                          now: false, later: false, much_later: false
 
         include_examples 'deleting ticket article',
-                         item: 'article_note',
+                         item: 'article_note_self',
                          now: true, later: true, much_later: false
 
+        include_examples 'deleting ticket article',
+                         item: 'article_note_other',
+                         now: false, later: false, much_later: false
+
         include_examples 'deleting ticket article',
                          item: 'article_note_customer',
                          now: false, later: false, much_later: false
 
         include_examples 'deleting ticket article',
-                         item: 'article_note_communication',
+                         item: 'article_note_communication_self',
-                         now: true, later: true, much_later: false
+                         now: false, later: false, much_later: false
+
+        include_examples 'deleting ticket article',
+                         item: 'article_note_communication_other',
+                         now: false, later: false, much_later: false
       end
 
       context 'as customer' do
@@ -352,14 +379,15 @@ RSpec.describe 'Ticket zoom', type: :system do
         context 'as admin' do
           let(:user) { admin }
 
-          include_examples 'according to permission matrix', item: 'article_note', expects_visible: true, offset: 8000.seconds, description: 'outside of delete timeframe'
+          include_examples 'according to permission matrix', item: 'article_note_self', expects_visible: true,  offset: 5000.seconds, description: 'outside of delete timeframe'
+          include_examples 'according to permission matrix', item: 'article_note_self', expects_visible: false, offset: 8000.seconds, description: 'outside of delete timeframe'
         end
 
         context 'as agent' do
           let(:user) { agent }
 
-          include_examples 'according to permission matrix', item: 'article_note', expects_visible: true,  offset: 5000.seconds, description: 'outside of delete timeframe'
+          include_examples 'according to permission matrix', item: 'article_note_self', expects_visible: true,  offset: 5000.seconds, description: 'outside of delete timeframe'
-          include_examples 'according to permission matrix', item: 'article_note', expects_visible: false, offset: 8000.seconds, description: 'outside of delete timeframe'
+          include_examples 'according to permission matrix', item: 'article_note_self', expects_visible: false, offset: 8000.seconds, description: 'outside of delete timeframe'
         end
       end
 
@@ -369,7 +397,7 @@ RSpec.describe 'Ticket zoom', type: :system do
         context 'as agent' do
           let(:user) { agent }
 
-          include_examples 'according to permission matrix', item: 'article_note', expects_visible: true, offset: 99.days, description: 'long after'
+          include_examples 'according to permission matrix', item: 'article_note_self', expects_visible: true, offset: 99.days, description: 'long after'
         end
       end
     end
@@ -378,7 +406,7 @@ RSpec.describe 'Ticket zoom', type: :system do
       before         { Setting.set 'ui_ticket_zoom_article_delete_timeframe', 5 }
 
       let(:user)     { agent }
-      let(:item)     { 'article_note' }
+      let(:item)     { 'article_note_self' }
       let!(:article) { send(item) }
       let(:offset)   { 0.seconds }