From d24a7c558461d83dd1e030c829b398083d15acd6 Mon Sep 17 00:00:00 2001
From: Mantas Masalskis <mm@zammad.com>
Date: Tue, 12 Nov 2019 15:08:00 +0100
Subject: [PATCH] Fixes #2777: Knowledge Base - Attachments not working for
 internal readers.

---
 app/controllers/attachments_controller.rb     |   3 +-
 app/models/concerns/can_be_published.rb       |   4 +
 ...s_knowledge_base_attachment_permissions.rb |  15 +-
 .../answer/translation/content.rb             |   4 +
 .../knowledge_base/attachments_spec.rb        | 206 ++++++++++++++++++
 spec/support/knowledge_base_contexts.rb       |   4 +
 6 files changed, 232 insertions(+), 4 deletions(-)
 create mode 100644 spec/requests/knowledge_base/attachments_spec.rb

diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb
index 27569d789e7e..d5672c03c06d 100644
--- a/app/controllers/attachments_controller.rb
+++ b/app/controllers/attachments_controller.rb
@@ -1,5 +1,6 @@
 class AttachmentsController < ApplicationController
-  prepend_before_action :authentication_check, except: :show
+  prepend_before_action :authentication_check, except: %i[show destroy]
+  prepend_before_action :authentication_check_only, only: %i[show destroy]
   before_action :verify_object_permissions, only: %i[show destroy]
 
   def show
diff --git a/app/models/concerns/can_be_published.rb b/app/models/concerns/can_be_published.rb
index 18a11cc12c51..cf0bb2b5e720 100644
--- a/app/models/concerns/can_be_published.rb
+++ b/app/models/concerns/can_be_published.rb
@@ -10,6 +10,10 @@ def visible?
     can_be_published_aasm.published?
   end
 
+  def visible_internally?
+    can_be_published_aasm.internal? || visible?
+  end
+
   class_methods do
     def inverse_relation_name(scope_name)
       "can_be_published_#{scope_name}_#{model_name.plural}"
diff --git a/app/models/concerns/has_knowledge_base_attachment_permissions.rb b/app/models/concerns/has_knowledge_base_attachment_permissions.rb
index 31df0f02d732..10386a9b7221 100644
--- a/app/models/concerns/has_knowledge_base_attachment_permissions.rb
+++ b/app/models/concerns/has_knowledge_base_attachment_permissions.rb
@@ -6,12 +6,15 @@ module HasKnowledgeBaseAttachmentPermissions
     def can_show_attachment?(file, user)
       return true if user_kb_editor?(user)
 
-      find(file.o_id)&.visible?
+      object = find(file.o_id)
+
+      return object&.visible_internally? if user_kb_reader?(user)
+
+      object&.visible?
     end
 
-    def can_destroy_attachment?(file, user)
+    def can_destroy_attachment?(_file, user)
       return if !user_kb_editor?(user)
-      return if !HasRichText.attachment_inline?(file)
 
       true
     end
@@ -21,6 +24,12 @@ def user_kb_editor?(user)
 
       user.permissions? %w[knowledge_base.editor]
     end
+
+    def user_kb_reader?(user)
+      return false if user.nil?
+
+      user.permissions? %w[knowledge_base.reader]
+    end
   end
 
   included do
diff --git a/app/models/knowledge_base/answer/translation/content.rb b/app/models/knowledge_base/answer/translation/content.rb
index 9f8db8266e97..82d57f97fd36 100644
--- a/app/models/knowledge_base/answer/translation/content.rb
+++ b/app/models/knowledge_base/answer/translation/content.rb
@@ -15,6 +15,10 @@ def visible?
     translation.answer.visible?
   end
 
+  def visible_internally?
+    translation.answer.visible_internally?
+  end
+
   delegate :created_by_id, to: :translation
 
   def attributes_with_association_ids
diff --git a/spec/requests/knowledge_base/attachments_spec.rb b/spec/requests/knowledge_base/attachments_spec.rb
new file mode 100644
index 000000000000..dff5cc4020ca
--- /dev/null
+++ b/spec/requests/knowledge_base/attachments_spec.rb
@@ -0,0 +1,206 @@
+require 'rails_helper'
+
+RSpec.describe 'KnowledgeBase attachments', type: :request do
+  include_context 'basic Knowledge Base'
+
+  let(:attachment) do
+    attachment_file = File.open 'spec/fixtures/upload/hello_world.txt'
+
+    Store.add(
+      object:        object.class.to_s,
+      o_id:          object.id,
+      data:          attachment_file.read,
+      filename:      'hello_world.txt',
+      preferences:   {},
+      created_by_id: 1,
+    )
+  end
+
+  let(:endpoint) { "/api/v1/attachments/#{attachment.id}" }
+
+  describe 'visible when attached to' do
+    shared_examples 'a visible resource' do
+      it 'and returns correct status code' do
+        get endpoint
+        expect(response).to have_http_status(:ok)
+      end
+    end
+
+    shared_examples 'a non-existent resource' do
+      it 'and returns correct status code' do
+        get endpoint
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    describe 'draft answer' do
+      let(:object) { draft_answer }
+
+      describe 'as agent', authenticated_as: :agent_user do
+        it_behaves_like 'a non-existent resource'
+      end
+
+      context 'as admin', authenticated_as: :admin_user do
+        it_behaves_like 'a visible resource'
+      end
+
+      context 'as customer', authenticated_as: :customer_user do
+        it_behaves_like 'a non-existent resource'
+      end
+
+      context 'as guest' do
+        it_behaves_like 'a non-existent resource'
+      end
+    end
+
+    describe 'internal answer' do
+      let(:object) { internal_answer }
+
+      describe 'as agent', authenticated_as: :agent_user do
+        it_behaves_like 'a visible resource'
+      end
+
+      context 'as admin', authenticated_as: :admin_user do
+        it_behaves_like 'a visible resource'
+      end
+
+      context 'as customer', authenticated_as: :customer_user do
+        it_behaves_like 'a non-existent resource'
+      end
+
+      context 'as guest' do
+        it_behaves_like 'a non-existent resource'
+      end
+    end
+
+    describe 'published answer' do
+      let(:object) { published_answer }
+
+      describe 'as agent', authenticated_as: :agent_user do
+        it_behaves_like 'a visible resource'
+      end
+
+      context 'as admin', authenticated_as: :admin_user do
+        it_behaves_like 'a visible resource'
+      end
+
+      context 'as customer', authenticated_as: :customer_user do
+        it_behaves_like 'a visible resource'
+      end
+
+      context 'as guest' do
+        it_behaves_like 'a visible resource'
+      end
+    end
+
+    describe 'archived answer' do
+      let(:object) { archived_answer }
+
+      describe 'as agent', authenticated_as: :agent_user do
+        it_behaves_like 'a non-existent resource'
+      end
+
+      context 'as admin', authenticated_as: :admin_user do
+        it_behaves_like 'a visible resource'
+      end
+
+      context 'as customer', authenticated_as: :customer_user do
+        it_behaves_like 'a non-existent resource'
+      end
+
+      context 'as guest' do
+        it_behaves_like 'a non-existent resource'
+      end
+    end
+  end
+
+  describe 'deletable when attached to' do
+    shared_examples 'a deletable resource' do
+      it { expect { delete endpoint }.to change { Store.exists? attachment.id }.from(true).to(false) }
+    end
+
+    shared_examples 'a non-deletable resource' do
+      it { expect { delete endpoint }.not_to change { Store.exists? attachment.id }.from(true) }
+    end
+
+    describe 'draft answer' do
+      let(:object) { draft_answer }
+
+      describe 'as agent', authenticated_as: :agent_user do
+        it_behaves_like 'a non-deletable resource'
+      end
+
+      context 'as admin', authenticated_as: :admin_user do
+        it_behaves_like 'a deletable resource'
+      end
+
+      context 'as customer', authenticated_as: :customer_user do
+        it_behaves_like 'a non-deletable resource'
+      end
+
+      context 'as guest' do
+        it_behaves_like 'a non-deletable resource'
+      end
+    end
+
+    describe 'internal answer' do
+      let(:object) { internal_answer }
+
+      describe 'as agent', authenticated_as: :agent_user do
+        it_behaves_like 'a non-deletable resource'
+      end
+
+      context 'as admin', authenticated_as: :admin_user do
+        it_behaves_like 'a deletable resource'
+      end
+
+      context 'as customer', authenticated_as: :customer_user do
+        it_behaves_like 'a non-deletable resource'
+      end
+
+      context 'as guest' do
+        it_behaves_like 'a non-deletable resource'
+      end
+    end
+
+    describe 'published answer' do
+      let(:object) { published_answer }
+
+      describe 'as agent', authenticated_as: :agent_user do
+        it_behaves_like 'a non-deletable resource'
+      end
+
+      context 'as admin', authenticated_as: :admin_user do
+        it_behaves_like 'a deletable resource'
+      end
+
+      context 'as customer', authenticated_as: :customer_user do
+        it_behaves_like 'a non-deletable resource'
+      end
+
+      context 'as guest' do
+        it_behaves_like 'a non-deletable resource'
+      end
+    end
+
+    describe 'archived answer' do
+      let(:object) { archived_answer }
+
+      describe 'as agent', authenticated_as: :agent_user do
+        it_behaves_like 'a non-deletable resource'
+      end
+
+      context 'as admin', authenticated_as: :admin_user do
+        it_behaves_like 'a deletable resource'
+      end
+
+      context 'as customer', authenticated_as: :customer_user do
+        it_behaves_like 'a non-deletable resource'
+      end
+
+      context 'as guest' do
+        it_behaves_like 'a non-deletable resource'
+      end
+    end
+  end
+end
diff --git a/spec/support/knowledge_base_contexts.rb b/spec/support/knowledge_base_contexts.rb
index 81463903996c..8350b81f16c6 100644
--- a/spec/support/knowledge_base_contexts.rb
+++ b/spec/support/knowledge_base_contexts.rb
@@ -26,4 +26,8 @@
   let :internal_answer do
     create(:knowledge_base_answer, category: category, internal_at: 1.week.ago)
   end
+
+  let :archived_answer do
+    create(:knowledge_base_answer, category: category, archived_at: 1.week.ago)
+  end
 end