From 4f6aed18569e834d2f36772192da1b092b1792b6 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Tue, 27 Apr 2021 10:17:15 +0200 Subject: [PATCH] Maintenance: Improved test coverage of link requests. --- .rubocop/todo.rspec.yml | 4 +++ app/controllers/links_controller.rb | 1 + app/controllers/tickets_controller.rb | 1 + app/models/link.rb | 20 ++++++++++- spec/factories/link.rb | 4 +-- spec/requests/links_spec.rb | 49 +++++++++++++++++++++++++++ spec/requests/ticket_spec.rb | 37 ++++++++++++++++++++ 7 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 spec/requests/links_spec.rb diff --git a/.rubocop/todo.rspec.yml b/.rubocop/todo.rspec.yml index 0510128b9..30b8f3599 100644 --- a/.rubocop/todo.rspec.yml +++ b/.rubocop/todo.rspec.yml @@ -285,6 +285,7 @@ RSpec/ExampleLength: - 'spec/requests/integration/twilio_sms_spec.rb' - 'spec/requests/integration/user_device_spec.rb' - 'spec/requests/knowledge_base/answer_attachments_cloning_spec.rb' + - 'spec/requests/links_spec.rb' - 'spec/requests/long_polling_spec.rb' - 'spec/requests/o_auth_spec.rb' - 'spec/requests/organization_spec.rb' @@ -567,6 +568,7 @@ RSpec/MultipleExpectations: - 'spec/requests/integration/telegram_spec.rb' - 'spec/requests/integration/twilio_sms_spec.rb' - 'spec/requests/integration/user_device_spec.rb' + - 'spec/requests/links_spec.rb' - 'spec/requests/long_polling_spec.rb' - 'spec/requests/o_auth_spec.rb' - 'spec/requests/organization_spec.rb' @@ -610,6 +612,7 @@ RSpec/NamedSubject: RSpec/NestedGroups: Exclude: + - 'app/models/user/avatar.rb' - 'spec/lib/secure_mailing/smime_spec.rb' - 'spec/models/channel/driver/twitter_spec.rb' - 'spec/models/channel/email_parser_spec.rb' @@ -618,6 +621,7 @@ RSpec/NestedGroups: - 'spec/models/trigger_spec.rb' - 'spec/models/user/has_ticket_create_screen_impact_examples.rb' - 'spec/models/user_spec.rb' + - 'spec/requests/links_spec.rb' - 'spec/system/ticket/create_spec.rb' RSpec/RepeatedDescription: diff --git a/app/controllers/links_controller.rb b/app/controllers/links_controller.rb index f533634b5..1bc6e9851 100644 --- a/app/controllers/links_controller.rb +++ b/app/controllers/links_controller.rb @@ -8,6 +8,7 @@ class LinksController < ApplicationController links = Link.list( link_object: params[:link_object], link_object_value: params[:link_object_value], + user: current_user, ) linked_objects = links diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index be36ffae9..30a1ca2f2 100644 --- a/app/controllers/tickets_controller.rb +++ b/app/controllers/tickets_controller.rb @@ -703,6 +703,7 @@ class TicketsController < ApplicationController links = Link.list( link_object: 'Ticket', link_object_value: ticket.id, + user: current_user, ) assets = Link.reduce_assets(assets, links) diff --git a/app/models/link.rb b/app/models/link.rb index ba87f2a88..c52362215 100644 --- a/app/models/link.rb +++ b/app/models/link.rb @@ -53,7 +53,11 @@ class Link < ApplicationModel items.push link end - items + return items if data[:user].blank? + + items.select do |item| + authorized_item?(data[:user], item) + end end =begin @@ -272,4 +276,18 @@ class Link < ApplicationModel ', object1_id, object2_id, object1_value, object2_value, object1_id, object2_id, object1_value, object2_value) end + def self.authorized_item?(user, item) + record = item['link_object'].constantize.lookup(id: item['link_object_value']) + + # non-ID records are not checked for authorization + return true if record.blank? + + Pundit.authorize(user, record, :show?).present? + rescue Pundit::NotAuthorizedError + false + rescue NameError, Pundit::NotDefinedError + # NameError: no Model means no authorization check possible + # Pundit::NotDefinedError: no Policy means no authorization check necessary + true + end end diff --git a/spec/factories/link.rb b/spec/factories/link.rb index 42ffc4858..dc3d43cf9 100644 --- a/spec/factories/link.rb +++ b/spec/factories/link.rb @@ -4,8 +4,8 @@ FactoryBot.define do factory :link do transient do link_type { 'normal' } - link_object_source { 'Ticket' } - link_object_target { 'Ticket' } + link_object_source { from.class.name } + link_object_target { to.class.name } from { Ticket.first } to { Ticket.last } end diff --git a/spec/requests/links_spec.rb b/spec/requests/links_spec.rb new file mode 100644 index 000000000..0572df2c0 --- /dev/null +++ b/spec/requests/links_spec.rb @@ -0,0 +1,49 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'rails_helper' + +RSpec.describe 'Link', type: :request do + + describe 'GET /api/v1/links' do + + context 'when requesting links of Ticket', authenticated_as: -> { agent } do + + subject!(:ticket) { create(:ticket) } + + let(:agent) { create(:agent, groups: [ticket.group]) } + + let(:params) do + { + link_object: ticket.class.name, + link_object_value: ticket.id, + } + end + let(:linked) { create(:ticket, group: ticket.group) } + + before do + create(:link, from: ticket, to: linked) + get '/api/v1/links', params: params, as: :json + end + + it 'is present in response' do + expect(response).to have_http_status(:ok) + expect(json_response['links']).to eq([ + { + 'link_type' => 'normal', + 'link_object' => 'Ticket', + 'link_object_value' => linked.id + } + ]) + end + + context 'without permission to linked Ticket Group' do + let(:linked) { create(:ticket) } + + it 'is not present in response' do + expect(response).to have_http_status(:ok) + expect(json_response['links']).to be_blank + end + end + end + end +end diff --git a/spec/requests/ticket_spec.rb b/spec/requests/ticket_spec.rb index 2c8a4ab92..d4aeedced 100644 --- a/spec/requests/ticket_spec.rb +++ b/spec/requests/ticket_spec.rb @@ -2339,6 +2339,43 @@ RSpec.describe 'Ticket', type: :request do end end + describe 'GET /api/v1/tickets/:id' do + + subject!(:ticket) { create(:ticket) } + + let(:agent) { create(:agent, groups: [ticket.group]) } + + context 'links present', authenticated_as: -> { agent } do + + before do + create(:link, from: ticket, to: linked) + get "/api/v1/tickets/#{ticket.id}", params: { all: 'true' }, as: :json + end + + let(:linked) { create(:ticket, group: ticket.group) } + + it 'is present in response' do + expect(response).to have_http_status(:ok) + expect(json_response['links']).to eq([ + { + 'link_type' => 'normal', + 'link_object' => 'Ticket', + 'link_object_value' => linked.id + } + ]) + end + + context 'no permission to linked Ticket Group' do + let(:linked) { create(:ticket) } + + it 'is not present in response' do + expect(response).to have_http_status(:ok) + expect(json_response['links']).to be_blank + end + end + end + end + describe 'GET /api/v1/ticket_customer' do subject(:ticket) { create(:ticket, customer: customer_authorized) }