Skip to content

Commit

Permalink
Fixes #3086 - Deletion of communication article works for admins
Browse files Browse the repository at this point in the history
  • Loading branch information
@mantas @thorsteneckel
mantas authored and thorsteneckel committed Jul 13, 2020
1 parent 7f14332 commit 5c5c69a
Show file tree
Hide file tree
Showing 5 changed files with 135 additions and 66 deletions.
3 changes: 1 addition & 2 deletions app/assets/javascripts/app/controllers/ticket_zoom/article_action/delete.coffee
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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

Expand Down
37 changes: 19 additions & 18 deletions app/policies/ticket/article_policy.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,26 @@ def update?
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
Expand All @@ -53,12 +62,4 @@ def access?(query)
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
77 changes: 59 additions & 18 deletions spec/requests/ticket/article_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,8 @@
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
Expand All @@ -491,10 +493,16 @@
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
Expand All @@ -503,12 +511,20 @@
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)
Expand Down Expand Up @@ -552,19 +568,27 @@ def delete_article_via_rest(article)

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_self',
now: true, later: true, much_later: false

include_examples 'deleting',
item: 'article_note',
now: true, later: true, much_later: true
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
Expand All @@ -575,17 +599,24 @@ def delete_article_via_rest(article)
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
Expand All @@ -596,31 +627,41 @@ def delete_article_via_rest(article)
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',
item: 'article_note_customer',
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

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

Expand All @@ -644,7 +685,7 @@ def delete_article_via_rest(article)
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 }
Expand Down
8 changes: 4 additions & 4 deletions spec/requests/ticket_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -894,7 +894,7 @@
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)
Expand Down Expand Up @@ -991,7 +991,7 @@
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)
Expand Down Expand Up @@ -1237,7 +1237,7 @@
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,
Expand All @@ -1261,7 +1261,7 @@
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',
Expand Down

0 comments on commit 5c5c69a

Please sign in to comment.