Maintenance: Refactored attachment handling

(cherry picked from commit 26f7a45e9b6457985bed2157447977801923d29e)

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

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

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

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

app/policies/knowledge_base/answer_policy.rb

@@ -24,6 +24,10 @@ def destroy?
     access_editor?
   end
 
+  def user_required?
+    false
+  end
+
   private
 
   def access

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

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

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