diff --git a/app/assets/javascripts/app/controllers/knowledge_base/agent_controller.coffee b/app/assets/javascripts/app/controllers/knowledge_base/agent_controller.coffee index ef73b0d3a..75ee788df 100644 --- a/app/assets/javascripts/app/controllers/knowledge_base/agent_controller.coffee +++ b/app/assets/javascripts/app/controllers/knowledge_base/agent_controller.coffee @@ -358,10 +358,10 @@ class App.KnowledgeBaseAgentController extends App.Controller parentController: @ ) - access: (params) -> + access: (params) => @constructor .pickObjectUsing(params, @) - ?.access() + ?.access(@kb_locale()) isEditor: -> @access(@lastParams) == 'editor' diff --git a/app/assets/javascripts/app/controllers/knowledge_base/permissions_dialog.coffee b/app/assets/javascripts/app/controllers/knowledge_base/permissions_dialog.coffee index 33fb447c6..895cc1843 100644 --- a/app/assets/javascripts/app/controllers/knowledge_base/permissions_dialog.coffee +++ b/app/assets/javascripts/app/controllers/knowledge_base/permissions_dialog.coffee @@ -54,6 +54,19 @@ class App.KnowledgeBasePermissionsDialog extends App.ControllerModal elem.accessLevel = role_name elem.limit = _.findWhere(data.inherited, { role_id: elem.id })?.access + if elem.limit? + elem.accessLevelIsDisabled = { + editor: elem.limit != 'editor' + reader: elem.limit == 'none' + none: false + } + else + elem.accessLevelIsDisabled = { + editor: elem.accessLevel != 'editor' + reader: false + none: false + } + load: => @ajax( id: 'knowledge_base_permissions_get' @@ -68,7 +81,7 @@ class App.KnowledgeBasePermissionsDialog extends App.ControllerModal ) toggleDisabled: (state) => - @el.find('input, button').attr('disabled', state) + @el.find('input:not([data-permanently-disabled]), button').attr('disabled', state) onSubmit: (e) => @clearAlerts() diff --git a/app/assets/javascripts/app/lib/mixins/knowledge_base_access.coffee b/app/assets/javascripts/app/lib/mixins/knowledge_base_access.coffee index 8e5df11cb..d9b5bbd86 100644 --- a/app/assets/javascripts/app/lib/mixins/knowledge_base_access.coffee +++ b/app/assets/javascripts/app/lib/mixins/knowledge_base_access.coffee @@ -1,5 +1,5 @@ InstanceMethods = - access: -> + access: (kb_locale) -> permission_reader = App.Permission.findByAttribute('name', 'knowledge_base.reader') permission_editor = App.Permission.findByAttribute('name', 'knowledge_base.editor') @@ -20,6 +20,8 @@ InstanceMethods = return 'editor' when 'reader' access = 'reader' + when 'none' + access = 'reader' if kb_locale && @visiblePublicly(kb_locale) else if role = App.Role.find(role_id) if role.permission_ids.indexOf(permission_editor.id) > -1 return 'editor' diff --git a/app/assets/javascripts/app/models/knowledge_base.coffee b/app/assets/javascripts/app/models/knowledge_base.coffee index cc4b12a0f..3a085d6f2 100644 --- a/app/assets/javascripts/app/models/knowledge_base.coffee +++ b/app/assets/javascripts/app/models/knowledge_base.coffee @@ -77,7 +77,7 @@ class App.KnowledgeBase extends App.Model , initial visibleInternally: (kb_locale) -> - @active && @access() != 'none' + @active && @access(kb_locale) != 'none' visiblePublicly: (kb_locale) -> @active diff --git a/app/assets/javascripts/app/models/knowledge_base_answer.coffee b/app/assets/javascripts/app/models/knowledge_base_answer.coffee index 3c7c12729..f418e2e98 100644 --- a/app/assets/javascripts/app/models/knowledge_base_answer.coffee +++ b/app/assets/javascripts/app/models/knowledge_base_answer.coffee @@ -96,4 +96,7 @@ class App.KnowledgeBaseAnswer extends App.Model 'Answer' visibleInternally: (kb_locale) => - (@is_internally_published(kb_locale) && @access() != 'none') || @is_published(kb_locale) + (@is_internally_published(kb_locale) && @access(kb_locale) != 'none') || @is_published(kb_locale) + + visiblePublicly: (kb_locale) => + @is_published(kb_locale) diff --git a/app/assets/javascripts/app/models/knowledge_base_category.coffee b/app/assets/javascripts/app/models/knowledge_base_category.coffee index bcdda5256..f0a02c36c 100644 --- a/app/assets/javascripts/app/models/knowledge_base_category.coffee +++ b/app/assets/javascripts/app/models/knowledge_base_category.coffee @@ -167,8 +167,6 @@ class App.KnowledgeBaseCategory extends App.Model 'draft' visibleInternally: (kb_locale) => - #return false if @access() == 'none' - @findDeepAnswer( (record) -> record.is_internally_published(kb_locale) )? diff --git a/app/assets/javascripts/app/views/knowledge_base/permissions_dialog.jst.eco b/app/assets/javascripts/app/views/knowledge_base/permissions_dialog.jst.eco index 9ade2d4d0..014deb9cd 100644 --- a/app/assets/javascripts/app/views/knowledge_base/permissions_dialog.jst.eco +++ b/app/assets/javascripts/app/views/knowledge_base/permissions_dialog.jst.eco @@ -18,9 +18,9 @@ value="<%= key %>" name="<%= role.id %>" <% if @params[role.id] == key: %>checked<% end %> - <% if role.limit?: %> - <% if key == 'editor' && role.limit != 'editor': %>disabled<% end %> - <% if key == 'reader' && role.limit == 'none': %>disabled<% end %> + <% if role.accessLevelIsDisabled[key]: %> + disabled + data-permanently-disabled <% end %> /> <%- @Icon('radio', 'icon-unchecked') %> diff --git a/app/controllers/application_controller/handles_errors.rb b/app/controllers/application_controller/handles_errors.rb index 4439ccb6f..5f71d5ee8 100644 --- a/app/controllers/application_controller/handles_errors.rb +++ b/app/controllers/application_controller/handles_errors.rb @@ -84,8 +84,10 @@ module ApplicationController::HandlesErrors error: e.message } - if (message = e.try(:record)&.errors&.full_messages&.first) - data[:error_human] = message + if (base_error = e.try(:record)&.errors&.messages&.find { |key, _| key.match? %r{[\w+.]?base} }&.last&.last) + data[:error_human] = base_error + elsif (first_error = e.try(:record)&.errors&.full_messages&.first) + data[:error_human] = first_error elsif e.message.match?(%r{(already exists|duplicate key|duplicate entry)}i) data[:error_human] = __('Object already exists!') elsif e.message =~ %r{null value in column "(.+?)" violates not-null constraint}i || e.message =~ %r{Field '(.+?)' doesn't have a default value}i diff --git a/app/models/knowledge_base.rb b/app/models/knowledge_base.rb index b28815b4b..cb9e94edb 100644 --- a/app/models/knowledge_base.rb +++ b/app/models/knowledge_base.rb @@ -179,6 +179,14 @@ class KnowledgeBase < ApplicationModel KnowledgeBase::Permission.any? end + def public_content?(kb_locale = nil) + scope = answers.published + + scope = scope.localed(kb_locale.system_locale) if kb_locale + + scope.any? + end + private def set_defaults diff --git a/app/models/knowledge_base/permission.rb b/app/models/knowledge_base/permission.rb index 4f5b4dd33..e5d630969 100644 --- a/app/models/knowledge_base/permission.rb +++ b/app/models/knowledge_base/permission.rb @@ -4,13 +4,33 @@ class KnowledgeBase::Permission < ApplicationModel belongs_to :permissionable, polymorphic: true, touch: true belongs_to :role - validates :access, inclusion: { in: %w[editor reader none] } validates :role, uniqueness: { scope: %i[permissionable_id permissionable_type] } + validate :ensure_access_matches_role + # cache key for calculated permissions # @param permissionable [KnowledgeBase::Category, KnowledgeBase] # @return [String] def self.cache_key(permissionable) "#{permissionable.class}::aws::#{permissionable.id}::permission::#{permissionable.updated_at}" end + + private + + def ensure_access_matches_role + return if role.blank? + return if allowed_access.include? access + + errors.add :base, __('This permission level is not available based on the current roles permissions.') + end + + def allowed_access + if role.with_permission? 'knowledge_base.editor' + %w[editor reader none] + elsif role.with_permission? 'knowledge_base.reader' + %w[reader none] + else + [] + end + end end diff --git a/i18n/zammad.pot b/i18n/zammad.pot index 1b138a99b..0c6af53fb 100644 --- a/i18n/zammad.pot +++ b/i18n/zammad.pot @@ -9132,6 +9132,10 @@ msgstr "" msgid "This page is invisible to the public." msgstr "" +#: app/models/knowledge_base/permission.rb +msgid "This permission level is not available based on the current roles permissions." +msgstr "" + #: app/assets/javascripts/app/controllers/_integration/github.coffee #: app/assets/javascripts/app/controllers/_integration/gitlab.coffee #: app/assets/javascripts/app/controllers/_integration/idoit.coffee diff --git a/lib/knowledge_base/effective_permission.rb b/lib/knowledge_base/effective_permission.rb index 901e3e3d0..2d9d10f2b 100644 --- a/lib/knowledge_base/effective_permission.rb +++ b/lib/knowledge_base/effective_permission.rb @@ -13,14 +13,25 @@ class KnowledgeBase @user.roles.reduce('none') do |memo, role| access = access_role_effective(role) - return access if access == 'editor' + return 'editor' if access == 'editor' - memo == 'reader' ? memo : access + access_role_reducer(memo, access) end end private + def access_role_reducer(memo, access) + case access + when 'reader' + 'reader' + when 'public_reader' + memo == 'reader' ? memo : access + when 'none' + memo + end + end + def permissions @permissions ||= @object.permissions_effective end diff --git a/spec/factories/knowledge_base_permissions.rb b/spec/factories/knowledge_base_permissions.rb index 620118071..c0e71b842 100644 --- a/spec/factories/knowledge_base_permissions.rb +++ b/spec/factories/knowledge_base_permissions.rb @@ -3,7 +3,7 @@ FactoryBot.define do factory 'knowledge_base/permission', aliases: %i[knowledge_base_permission] do permissionable { create(:knowledge_base_category) } - role { create(:role) } + role { create(:role, permission_names: 'knowledge_base.editor') } access { 'editor' } end end diff --git a/spec/lib/knowledge_base/permissions_update_spec.rb b/spec/lib/knowledge_base/permissions_update_spec.rb index 7af4ae3a0..4790cb749 100644 --- a/spec/lib/knowledge_base/permissions_update_spec.rb +++ b/spec/lib/knowledge_base/permissions_update_spec.rb @@ -5,8 +5,10 @@ require 'rails_helper' RSpec.describe KnowledgeBase::PermissionsUpdate do describe '#update!' do include_context 'basic Knowledge Base' - let(:role_editor) { create(:role, permission_names: %w[knowledge_base.editor]) } - let(:role_another) { create(:role, permission_names: %w[knowledge_base.editor]) } + + let(:role_editor) { create(:role, permission_names: %w[knowledge_base.editor]) } + let(:role_another) { create(:role, permission_names: %w[knowledge_base.editor]) } + let(:role_reader) { create(:role, permission_names: %w[knowledge_base.reader]) } let(:child_category) { create(:knowledge_base_category, parent: category) } describe 'updating itself' do @@ -35,6 +37,11 @@ RSpec.describe KnowledgeBase::PermissionsUpdate do expect { described_class.new(object).update! role_editor => 'reader' } .not_to change(object, :updated_at) end + + it 'throws error when role does not allow given access' do + expect { described_class.new(object).update! role_reader => 'editor' } + .to raise_error(%r{Validation failed}) + end end context 'when saving role on KB itself' do diff --git a/spec/models/knowledge_base/permission_spec.rb b/spec/models/knowledge_base/permission_spec.rb index 099fca702..553f92207 100644 --- a/spec/models/knowledge_base/permission_spec.rb +++ b/spec/models/knowledge_base/permission_spec.rb @@ -41,10 +41,44 @@ RSpec.describe KnowledgeBase::Permission, type: :model do end describe '#access' do - it { is_expected.to validate_presence_of(:access).with_message(%r{}) } - it { is_expected.to allow_value('editor').for(:access) } - it { is_expected.to allow_value('reader').for(:access) } - it { is_expected.to allow_value('none').for(:access) } - it { is_expected.not_to allow_value('foobar').for(:access) } + it { is_expected.not_to allow_access_value(nil) } + it { is_expected.not_to allow_access_value('foobar') } + + context 'when role is editor' do + it { is_expected.to allow_access_value('editor') } + it { is_expected.to allow_access_value('reader') } + it { is_expected.to allow_access_value('none') } + end + + context 'when role is reader' do + subject(:kb_category_permission) { build(:knowledge_base_permission, role: create(:role, permission_names: 'knowledge_base.reader')) } + + it { is_expected.not_to allow_access_value('editor') } + it { is_expected.to allow_access_value('reader') } + it { is_expected.to allow_access_value('none') } + end + + context 'when role has no KB access' do + subject(:kb_category_permission) { build(:knowledge_base_permission, role: create(:role)) } + + it { is_expected.not_to allow_access_value('editor') } + it { is_expected.not_to allow_access_value('reader') } + it { is_expected.not_to allow_access_value('none') } + end + end + + matcher :allow_access_value do + match do + actual.access = expected + actual.valid? + end + + failure_message do + "Expected to allow #{expected} as access, but was not allowed" + end + + failure_message_when_negated do + "Expected to not allow #{expected} as access, but was allowed" + end end end diff --git a/spec/system/knowledge_base/locale/category/permissions_spec.rb b/spec/system/knowledge_base/locale/category/permissions_spec.rb index d241cc2d0..61f23d4ca 100644 --- a/spec/system/knowledge_base/locale/category/permissions_spec.rb +++ b/spec/system/knowledge_base/locale/category/permissions_spec.rb @@ -43,6 +43,7 @@ RSpec.describe 'Knowledge Base Locale Category Permissions', type: :system do expect(page) .to have_css("input[name='#{role_reader.id}'][value='reader']:not([disabled])", visible: :all) .and(have_css("input[name='#{role_reader.id}'][value='none'][checked]:not([disabled])", visible: :all)) + .and(have_css("input[name='#{role_reader.id}'][value='editor'][disabled]", visible: :all)) end end @@ -55,6 +56,7 @@ RSpec.describe 'Knowledge Base Locale Category Permissions', type: :system do expect(page) .to have_css("input[name='#{role_reader.id}'][value='reader']:not([disabled])", visible: :all) .and(have_css("input[name='#{role_reader.id}'][value='none'][checked]:not([disabled])", visible: :all)) + .and(have_css("input[name='#{role_reader.id}'][value='editor'][disabled]", visible: :all)) end end @@ -93,6 +95,18 @@ RSpec.describe 'Knowledge Base Locale Category Permissions', type: :system do expect(page) .to have_css("input[name='#{role_reader.id}'][value='none'][checked]:not([disabled])", visible: :all) .and(have_css("input[name='#{role_reader.id}'][value='reader'][disabled]", visible: :all)) + .and(have_css("input[name='#{role_reader.id}'][value='editor'][disabled]", visible: :all)) + end + end + + it 'shows reader permissions limited by role itself' do + open_page child_category + + in_modal disappears: false do + expect(page) + .to have_css("input[name='#{role_reader.id}'][value='none']:not([disabled])", visible: :all) + .and(have_css("input[name='#{role_reader.id}'][value='reader'][checked]:not([disabled])", visible: :all)) + .and(have_css("input[name='#{role_reader.id}'][value='editor'][disabled]", visible: :all)) end end end diff --git a/spec/system/knowledge_base/locale/knowledge_base/permissions_spec.rb b/spec/system/knowledge_base/locale/knowledge_base/permissions_spec.rb index aa25fcea8..098f99e9d 100644 --- a/spec/system/knowledge_base/locale/knowledge_base/permissions_spec.rb +++ b/spec/system/knowledge_base/locale/knowledge_base/permissions_spec.rb @@ -26,8 +26,9 @@ RSpec.describe 'Knowledge Base Locale Knowledge Base Permissions', type: :system in_modal disappears: false do expect(page) - .to have_css("input[name='#{role_editor.id}'][value='editor'][checked]", visible: :all) - .and(have_css("input[name='#{role_editor.id}'][value='reader']", visible: :all)) + .to have_css("input[name='#{role_editor.id}'][value='editor'][checked]:not([disabled])", visible: :all) + .and(have_css("input[name='#{role_editor.id}'][value='reader']:not([disabled])", visible: :all)) + .and(have_css("input[name='#{role_editor.id}'][value='none']:not([disabled])", visible: :all)) end end @@ -38,8 +39,20 @@ RSpec.describe 'Knowledge Base Locale Knowledge Base Permissions', type: :system in_modal disappears: false do expect(page) - .to have_css("input[name='#{role_another_editor.id}'][value='reader'][checked]", visible: :all) - .and(have_css("input[name='#{role_another_editor.id}'][value='editor']", visible: :all)) + .to have_css("input[name='#{role_another_editor.id}'][value='reader'][checked]:not([disabled])", visible: :all) + .and(have_css("input[name='#{role_another_editor.id}'][value='editor']:not([disabled])", visible: :all)) + .and(have_css("input[name='#{role_another_editor.id}'][value='none']:not([disabled])", visible: :all)) + end + end + + it 'shows reader permissions limited by role itself' do + open_page + + in_modal disappears: false do + expect(page) + .to have_css("input[name='#{role_reader.id}'][value='none']:not([disabled])", visible: :all) + .and(have_css("input[name='#{role_reader.id}'][value='reader'][checked]:not([disabled])", visible: :all)) + .and(have_css("input[name='#{role_reader.id}'][value='editor'][disabled]", visible: :all)) end end end