Fixes #3997 - KB: granular permissions misconfiguration is allowed by UI

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'

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()

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'

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

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)

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)
     )?

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 role.accessLevelIsDisabled[key]: %>
-              <% if key == 'editor' && role.limit != 'editor': %>disabled<% end %>
+              disabled
-              <% if key == 'reader' && role.limit == 'none': %>disabled<% end %>
+              data-permanently-disabled
             <% end %>
             />
           <%- @Icon('radio', 'icon-unchecked') %>

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)
+    if (base_error = e.try(:record)&.errors&.messages&.find { |key, _| key.match? %r{[\w+.]?base} }&.last&.last)
-      data[:error_human] = message
+      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

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

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

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

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

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

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_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

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.not_to allow_access_value(nil) }
-    it { is_expected.to allow_value('editor').for(:access) }
+    it { is_expected.not_to allow_access_value('foobar') }
-    it { is_expected.to allow_value('reader').for(:access) }
+
-    it { is_expected.to allow_value('none').for(:access) }
+    context 'when role is editor' do
-    it { is_expected.not_to allow_value('foobar').for(:access) }
+      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

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

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)
+          .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']", 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)
+          .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']", 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