From b924dbfa801d907749748c8079de329e9f228545 Mon Sep 17 00:00:00 2001 From: Mantas Date: Sat, 26 Feb 2022 00:46:10 +0200 Subject: [PATCH] Fixes #3978 - KB: Unpermitted categories are displayed after browsing --- app/models/knowledge_base/answer.rb | 34 ++++--- app/models/knowledge_base/category.rb | 33 +++++-- spec/models/knowledge_base/answer_spec.rb | 102 ++++++++++++++++++++ spec/models/knowledge_base/category_spec.rb | 46 +++++++++ 4 files changed, 196 insertions(+), 19 deletions(-) diff --git a/app/models/knowledge_base/answer.rb b/app/models/knowledge_base/answer.rb index 4f97139ca..abcba7247 100644 --- a/app/models/knowledge_base/answer.rb +++ b/app/models/knowledge_base/answer.rb @@ -47,19 +47,8 @@ class KnowledgeBase::Answer < ApplicationModel data = category.assets(data) # include all siblings to make sure ordering is always up to date. Reader gets only accessible siblings. - siblings = category.answers + data = ApplicationModel::CanAssets.reduce(assets_siblings, data) - if !User.lookup(id: UserInfo.current_user_id)&.permissions?('knowledge_base.editor') - ep = KnowledgeBase::EffectivePermission.new User.find(UserInfo.current_user_id), category - - siblings = if ep.access_effective == 'public_reader' - siblings.published - else - siblings.internal - end - end - - data = ApplicationModel::CanAssets.reduce(siblings, data) ApplicationModel::CanAssets.reduce(translations, data) end @@ -111,6 +100,27 @@ class KnowledgeBase::Answer < ApplicationModel private + def assets_siblings(siblings: category.answers, current_user: User.lookup(id: UserInfo.current_user_id)) + if KnowledgeBase.granular_permissions? + ep = KnowledgeBase::EffectivePermission.new current_user, category + + case ep.access_effective + when 'public_reader' + siblings.published + when 'none' + siblings.none + when 'reader' + siblings.internal + else + siblings + end + elsif !current_user&.permissions?('knowledge_base.editor') + siblings.internal + else + siblings + end + end + def reordering_callback return if !category_id_changed? && !position_changed? diff --git a/app/models/knowledge_base/category.rb b/app/models/knowledge_base/category.rb index 6f351277b..b9ce6d444 100644 --- a/app/models/knowledge_base/category.rb +++ b/app/models/knowledge_base/category.rb @@ -46,13 +46,7 @@ class KnowledgeBase::Category < ApplicationModel data = knowledge_base.assets(data) # include all siblings to make sure ordering is always up to date - siblings = sibling_categories - - if !User.lookup(id: UserInfo.current_user_id)&.permissions?('knowledge_base.editor') - siblings = siblings.select(&:internal_content?) - end - - data = ApplicationModel::CanAssets.reduce(siblings, data) + data = ApplicationModel::CanAssets.reduce(assets_siblings, data) data = ApplicationModel::CanAssets.reduce(translations, data) # include parent category or KB for root to have full path @@ -141,6 +135,31 @@ class KnowledgeBase::Category < ApplicationModel private + def assets_siblings(siblings: sibling_categories, current_user: User.lookup(id: UserInfo.current_user_id)) + granular = KnowledgeBase.granular_permissions? + + if !granular && !current_user&.permissions?('knowledge_base.editor') + siblings.select(&:internal_content?) + elsif granular + siblings.select { |elem| assets_siblings_applicable?(elem, current_user) } + else + siblings + end + end + + def assets_siblings_applicable?(elem, current_user) + ep = KnowledgeBase::EffectivePermission.new(current_user, elem) + + case ep.access_effective + when 'none' + false + when 'reader' + elem.internal_content? + else + true + end + end + def cannot_be_child_of_parent errors.add(:parent_id, 'cannot be a child of the parent') if self_parent?(self) end diff --git a/spec/models/knowledge_base/answer_spec.rb b/spec/models/knowledge_base/answer_spec.rb index 09be1c971..6859dfeb0 100644 --- a/spec/models/knowledge_base/answer_spec.rb +++ b/spec/models/knowledge_base/answer_spec.rb @@ -23,4 +23,106 @@ RSpec.describe KnowledgeBase::Answer, type: :model, current_user_id: 1 do it { expect(kb_answer.attachments).to be_present } end + + describe '#assets', current_user_id: -> { user.id } do + let(:assets) { another_category_answer && internal_answer.assets } + let(:user) { create(:agent) } + let(:another_category) { create(:knowledge_base_category, knowledge_base: knowledge_base) } + let(:another_category_answer) { create(:knowledge_base_answer, :internal, category: another_category) } + + include_context 'basic Knowledge Base' + + context 'without permissions' do + it { expect(assets).to include_assets_of internal_answer } + it { expect(assets).to include_assets_of category } + it { expect(assets).to include_assets_of another_category } + + context 'with internal and published articles in category' do + before do + internal_answer + published_answer + end + + it 'internal sibling returned' do + expect(published_answer.assets).to include_assets_of(internal_answer, category) + end + + it 'published sibling returned' do + expect(internal_answer.assets).to include_assets_of(published_answer, category) + end + end + end + + context 'with readable another category' do + before do + KnowledgeBase::PermissionsUpdate + .new(another_category) + .update! user.roles.first => 'reader' + end + + it { expect(assets).to include_assets_of internal_answer } + it { expect(assets).to include_assets_of category } + it { expect(assets).to include_assets_of another_category } + + context 'with internal and published articles in category' do + before do + KnowledgeBase::PermissionsUpdate + .new(category) + .update! user.roles.first => 'reader' + + internal_answer + published_answer + end + + it 'internal sibling returned' do + expect(published_answer.assets).to include_assets_of(internal_answer, category) + end + + it 'published sibling returned' do + expect(internal_answer.assets).to include_assets_of(published_answer, category) + end + end + end + + context 'with hidden another category' do + before do + KnowledgeBase::PermissionsUpdate + .new(another_category) + .update! user.roles.first => 'none' + end + + it { expect(assets).to include_assets_of internal_answer } + it { expect(assets).to include_assets_of category } + it { expect(assets).not_to include_assets_of another_category } + + context 'with internal and published articles in category' do + before do + KnowledgeBase::PermissionsUpdate + .new(category) + .update! user.roles.first => 'none' + + internal_answer + published_answer + end + + it 'internal sibling not returned' do + expect(published_answer.assets).to not_include_assets_of(internal_answer).and(include_assets_of(category)) + end + + it 'published sibling returned' do + expect(internal_answer.assets).to include_assets_of(published_answer, category) + end + end + + context 'with published answer' do + let(:another_category_published_answer) { create(:knowledge_base_answer, :published, category: another_category) } + + before { another_category_published_answer } + + it { expect(assets).to include_assets_of internal_answer } + it { expect(assets).to include_assets_of category } + it { expect(assets).to include_assets_of another_category } + end + end + end end diff --git a/spec/models/knowledge_base/category_spec.rb b/spec/models/knowledge_base/category_spec.rb index 22aebaa0d..01a99a510 100644 --- a/spec/models/knowledge_base/category_spec.rb +++ b/spec/models/knowledge_base/category_spec.rb @@ -131,4 +131,50 @@ RSpec.describe KnowledgeBase::Category, type: :model, current_user_id: 1 do include_examples 'verify visibility in given state', state: :draft, is_visible: false include_examples 'verify visibility in given state', state: :archived, is_visible: false end + + describe '#assets', current_user_id: -> { user.id } do + subject(:assets) { another_category_answer && internal_answer && category.assets } + + include_context 'basic Knowledge Base' + + let(:user) { create(:agent) } + let(:another_category) { create(:knowledge_base_category, knowledge_base: knowledge_base) } + let(:another_category_answer) { create(:knowledge_base_answer, :internal, category: another_category) } + + context 'without permissions' do + it { expect(assets).to include_assets_of category } + it { expect(assets).to include_assets_of another_category } + end + + context 'with readable another category' do + before do + KnowledgeBase::PermissionsUpdate + .new(another_category) + .update! user.roles.first => 'reader' + end + + it { expect(assets).to include_assets_of category } + it { expect(assets).to include_assets_of another_category } + end + + context 'with hidden another category' do + before do + KnowledgeBase::PermissionsUpdate + .new(another_category) + .update! user.roles.first => 'none' + end + + it { expect(assets).to include_assets_of category } + it { expect(assets).not_to include_assets_of another_category } + + context 'with published answer' do + let(:another_category_published_answer) { create(:knowledge_base_answer, :published, category: another_category) } + + before { another_category_published_answer } + + it { expect(assets).to include_assets_of category } + it { expect(assets).to include_assets_of another_category } + end + end + end end