From e7b06654fd2b560616c732de61eedf07dc1081ec Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Wed, 29 Jun 2022 08:13:09 +0200 Subject: [PATCH] Maintenance: Refactored attachment handling (cherry picked from commit 26f7a45e9b6457985bed2157447977801923d29e) --- .../application_controller/handles_errors.rb | 10 ++- app/controllers/attachments_controller.rb | 7 +- .../attachments_controller_policy.rb | 41 +++++++++++ .../answer/translation/content_policy.rb | 18 +++-- app/policies/knowledge_base/answer_policy.rb | 4 ++ .../attachments_controller_policy_spec.rb | 70 +++++++++++++++++++ .../answer/translation/content_policy_spec.rb | 61 ++++++++++++++++ spec/requests/attachments_controller_spec.rb | 50 +++++++++++++ 8 files changed, 248 insertions(+), 13 deletions(-) create mode 100644 app/policies/controllers/attachments_controller_policy.rb create mode 100644 spec/policies/controllers/attachments_controller_policy_spec.rb create mode 100644 spec/policies/knowledge_base/answer/translation/content_policy_spec.rb create mode 100644 spec/requests/attachments_controller_spec.rb diff --git a/app/controllers/application_controller/handles_errors.rb b/app/controllers/application_controller/handles_errors.rb index 18ca242465c0..5362b7219fad 100644 --- a/app/controllers/application_controller/handles_errors.rb +++ b/app/controllers/application_controller/handles_errors.rb @@ -56,8 +56,14 @@ def pundit_not_authorized_error(e) # check if a special authorization_error should be shown in the result payload # which was raised in one of the policies. Fall back to a simple "Not authorized" # error to hide actual cause for security reasons. - exeption = e.policy&.custom_exception || Exceptions::Forbidden.new(__('Not authorized')) - forbidden(exeption) + exception = e.policy&.custom_exception || Exceptions::Forbidden.new(__('Not authorized')) + + case exception + when ActiveRecord::RecordNotFound + not_found(exception) + else + forbidden(exception) + end end private diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 75867574f656..074e8b6dc740 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -82,10 +82,7 @@ def render_calendar_preview render json: { error: e.message }, status: :unprocessable_entity end - def authorize! - record = download_file&.store_object&.name&.safe_constantize&.find(download_file.o_id) - authorize(record) if record - rescue Pundit::NotAuthorizedError - raise ActiveRecord::RecordNotFound + def user_not_authorized(e) + not_found(e) end end diff --git a/app/policies/controllers/attachments_controller_policy.rb b/app/policies/controllers/attachments_controller_policy.rb new file mode 100644 index 000000000000..313933a4b1d5 --- /dev/null +++ b/app/policies/controllers/attachments_controller_policy.rb @@ -0,0 +1,41 @@ +# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/ + +class Controllers::AttachmentsControllerPolicy < Controllers::ApplicationControllerPolicy + def show? + store_object_policy(store_object_owner)&.show? + end + + def destroy? + store_object_policy(store_object_owner)&.destroy? + end + + def user_required? + false + end + + def custom_exception + ActiveRecord::RecordNotFound.new + end + + private + + def download_file + record.send(:download_file) + end + + def store_object_class + download_file + &.store_object + &.name + &.safe_constantize + end + + def store_object_policy(target) + Pundit.policy user, target + end + + def store_object_owner + store_object_class + &.find download_file.o_id + end +end diff --git a/app/policies/knowledge_base/answer/translation/content_policy.rb b/app/policies/knowledge_base/answer/translation/content_policy.rb index 1e53a231fd39..3b95a3d69f83 100644 --- a/app/policies/knowledge_base/answer/translation/content_policy.rb +++ b/app/policies/knowledge_base/answer/translation/content_policy.rb @@ -1,14 +1,20 @@ # Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/ class KnowledgeBase::Answer::Translation::ContentPolicy < ApplicationPolicy - def show? - return true if user&.permissions?(%w[knowledge_base.editor]) + delegate :show?, to: :parent_answer_policy + delegate :destroy?, to: :parent_answer_policy - record.translation.answer.visible? || - (user&.permissions?(%w[knowledge_base.reader]) && record.translation.answer.visible_internally?) + def user_required? + false end - def destroy? - user&.permissions?(%w[knowledge_base.editor]) + private + + def parent_answer_policy + Pundit.policy user, parent_answer + end + + def parent_answer + record.translation.answer end end diff --git a/app/policies/knowledge_base/answer_policy.rb b/app/policies/knowledge_base/answer_policy.rb index bd030b05a587..5a3b7ed30938 100644 --- a/app/policies/knowledge_base/answer_policy.rb +++ b/app/policies/knowledge_base/answer_policy.rb @@ -24,6 +24,10 @@ def destroy? access_editor? end + def user_required? + false + end + private def access diff --git a/spec/policies/controllers/attachments_controller_policy_spec.rb b/spec/policies/controllers/attachments_controller_policy_spec.rb new file mode 100644 index 000000000000..3024a8c378d7 --- /dev/null +++ b/spec/policies/controllers/attachments_controller_policy_spec.rb @@ -0,0 +1,70 @@ +# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/ + +require 'rails_helper' + +describe Controllers::AttachmentsControllerPolicy do + subject { described_class.new(user, record) } + + include_context 'basic Knowledge Base' + + let(:record_class) { AttachmentsController } + let(:object) { create(:knowledge_base_answer, visibility, :with_attachment, category: category) } + let(:params) { { id: object.attachments.first.id } } + + let(:record) do + rec = record_class.new + # rec.action_name = action_name + rec.params = params + + rec + end + + context 'with no user' do + let(:user) { nil } + + context 'with published object' do + let(:visibility) { :published } + + it { is_expected.to permit_actions :show } + it { is_expected.to forbid_actions :destroy } + end + + context 'with private object' do + let(:visibility) { :internal } + + it { is_expected.to forbid_actions :show, :destroy } + end + end + + context 'with a user' do + context 'with full access' do + let(:user) { create :admin } + let(:visibility) { :published } + + it { is_expected.to permit_actions :show, :destroy } + end + + context 'with limited access' do + let(:user) { create :agent } + let(:visibility) { :internal } + + it { is_expected.to permit_actions :show } + it { is_expected.to forbid_actions :destroy } + end + + context 'with no access' do + let(:user) { create :agent } + let(:visibility) { :draft } + + it { is_expected.to forbid_actions :show, :destroy } + end + + context 'with object that does not have a policy' do + let(:file) { create :store_image, object: 'NonExistingObject' } + let(:params) { { id: file.id } } + let(:user) { create :admin } + + it { is_expected.to forbid_actions :show, :destroy } + end + end +end diff --git a/spec/policies/knowledge_base/answer/translation/content_policy_spec.rb b/spec/policies/knowledge_base/answer/translation/content_policy_spec.rb new file mode 100644 index 000000000000..fbc2f0515b84 --- /dev/null +++ b/spec/policies/knowledge_base/answer/translation/content_policy_spec.rb @@ -0,0 +1,61 @@ +# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/ + +require 'rails_helper' + +describe KnowledgeBase::Answer::Translation::ContentPolicy do + subject(:policy) { described_class.new(user, record) } + + include_context 'basic Knowledge Base' + + let(:record) { answer.translation.content } + + context 'without user' do + let(:user) { nil } + + context 'with a public answer' do + let(:answer) { published_answer } + + it { is_expected.to permit_actions :show } + it { is_expected.to forbid_actions :destroy } + end + + context 'with a non public answer' do + let(:answer) { internal_answer } + + it { is_expected.to forbid_actions :show, :destroy } + end + end + + context 'with kb editor' do + let(:user) { create(:admin) } + + context 'with an internal answer' do + let(:answer) { internal_answer } + + it { is_expected.to permit_actions :show, :destroy } + end + + context 'with a draft answer' do + let(:answer) { draft_answer } + + it { is_expected.to permit_actions :show, :destroy } + end + end + + context 'with kb reader' do + let(:user) { create(:agent) } + + context 'with an internal answer' do + let(:answer) { internal_answer } + + it { is_expected.to permit_action :show } + it { is_expected.to forbid_action :destroy } + end + + context 'with a draft answer' do + let(:answer) { draft_answer } + + it { is_expected.to forbid_actions :show, :destroy } + end + end +end diff --git a/spec/requests/attachments_controller_spec.rb b/spec/requests/attachments_controller_spec.rb new file mode 100644 index 000000000000..3e0af0b9245e --- /dev/null +++ b/spec/requests/attachments_controller_spec.rb @@ -0,0 +1,50 @@ +# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/ + +require 'rails_helper' + +RSpec.describe AttachmentsController, type: :request do + include_context 'basic Knowledge Base' + + let(:object) { create(:knowledge_base_answer, :draft, :with_attachment, category: category) } + let(:attachment_id) { object.attachments.first.id } + + describe '#show' do + it 'returns 404 when does not exist' do + get '/api/v1/attachments/123' + + expect(response).to have_http_status(:not_found) + end + + it 'returns 404 when no access', authenticated_as: -> { create(:agent) } do + get "/api/v1/attachments/#{attachment_id}" + + expect(response).to have_http_status(:not_found) + end + + it 'returns ok on success', authenticated_as: -> { create(:admin) } do + get "/api/v1/attachments/#{attachment_id}" + + expect(response).to have_http_status(:ok) + end + end + + describe '#destroy' do + it 'returns 404 when does not exist' do + delete '/api/v1/attachments/123' + + expect(response).to have_http_status(:not_found) + end + + it 'returns 404 when no access', authenticated_as: -> { create(:agent) } do + delete "/api/v1/attachments/#{attachment_id}" + + expect(response).to have_http_status(:not_found) + end + + it 'returns ok on success', authenticated_as: -> { create(:admin) } do + delete "/api/v1/attachments/#{attachment_id}" + + expect(response).to have_http_status(:ok) + end + end +end