From 5c5c69a7858f63dea4e6a0dd089b8380bbb9d4f3 Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Mon, 13 Jul 2020 11:14:15 +0200 Subject: [PATCH] Fixes #3086 - Deletion of communication article works for admins --- .../ticket_zoom/article_action/delete.coffee | 3 +- app/policies/ticket/article_policy.rb | 37 ++++----- spec/requests/ticket/article_spec.rb | 77 ++++++++++++++----- spec/requests/ticket_spec.rb | 8 +- spec/system/ticket/zoom_spec.rb | 76 ++++++++++++------ 5 files changed, 135 insertions(+), 66 deletions(-) diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/article_action/delete.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/article_action/delete.coffee index bef0485fd..651a945f2 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/article_action/delete.coffee +++ b/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 diff --git a/app/policies/ticket/article_policy.rb b/app/policies/ticket/article_policy.rb index a5adc846e..69edf00c9 100644 --- a/app/policies/ticket/article_policy.rb +++ b/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?(__method__) - # don't let edge case exceptions raised in the TicketPolicy stop - # other possible positive authorization checks - rescue Pundit::NotAuthorizedError + return false if !access?('show?') + # agents can destroy articles of type 'note' - # which were created by themselves within the last 10 minutes - return missing_admin_permission if !user.permissions?('ticket.agent') - return missing_admin_permission if record.created_by_id != user.id - return missing_admin_permission if record.type.communication? && !record.internal? - return too_old_to_undo if deletable_timeframe? && record.created_at <= deletable_timeframe.ago + # which were created by themselves within the last x minutes + + if !user.permissions?('ticket.agent') + return not_authorized('agent permission required') if !user.permissions?('ticket.agent') + 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 diff --git a/spec/requests/ticket/article_spec.rb b/spec/requests/ticket/article_spec.rb index 65730f2ce..a24e7ae91 100644 --- a/spec/requests/ticket/article_spec.rb +++ b/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', - now: true, later: true, much_later: true + 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: true, later: true, much_later: true + now: false, later: false, much_later: false include_examples 'deleting', - item: 'article_note_communication', - now: true, later: true, much_later: true + 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 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', - now: true, later: true, much_later: false + 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 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 } diff --git a/spec/requests/ticket_spec.rb b/spec/requests/ticket_spec.rb index 18fc8931d..4f977c6d6 100644 --- a/spec/requests/ticket_spec.rb +++ b/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', diff --git a/spec/system/ticket/zoom_spec.rb b/spec/system/ticket/zoom_spec.rb index a190de21c..77d44a0aa 100644 --- a/spec/system/ticket/zoom_spec.rb +++ b/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(:agent) { create :agent, groups: [Group.first] } - let(:customer) { create :customer } - let(:ticket) { create :ticket, group: agent.groups.first, customer: customer } - let(:article) { send(item) } + let(:admin) { create :admin, groups: [Group.first] } + let(:agent) { create :agent, groups: [Group.first] } + let(:other_agent) { create :agent, groups: [Group.first] } + let(:customer) { create :customer } + 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 - create_ticket_article(sender_name: 'Agent', internal: true, type_name: 'note', updated_by: agent) + def article_note_self + 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', - now: true, later: true, much_later: true + 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: true, later: true, much_later: true + now: false, later: false, much_later: false include_examples 'deleting ticket article', - item: 'article_note_communication', - now: true, later: true, much_later: true + item: 'article_note_communication_self', + 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', - now: true, later: true, much_later: false + item: 'article_note_communication_self', + 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', expects_visible: false, 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 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 }