Refactoring: Migrate user permission handling to Pundit policies.

app/controllers/application_controller/handles_devices.rb

@@ -4,22 +4,16 @@ module ApplicationController::HandlesDevices
   extend ActiveSupport::Concern
 
   included do
-    before_action :user_device_check
+    before_action :user_device_log
   end
 
-  def user_device_check
-    return false if !user_device_log(current_user, 'session')
-
-    true
-  end
-
-  def user_device_log(user, type)
+  def user_device_log(user = current_user, type = 'session')
     switched_from_user_id = ENV['SWITCHED_FROM_USER_ID'] || session[:switched_from_user_id]
     return true if params[:controller] == 'init' # do no device logging on static initial page
     return true if switched_from_user_id
     return true if current_user_on_behalf # do no device logging for the user on behalf feature
     return true if !user
-    return true if !user.permissions?('user_preferences.device')
+    return true if !policy(UserDevice).log?
     return true if type == 'SSO'
 
     time_to_check = true

app/controllers/application_controller/handles_errors.rb

@@ -102,11 +102,7 @@ module ApplicationController::HandlesErrors
 
     if data[:error_human].present?
       data[:error] = data[:error_human]
-    elsif !current_user&.permissions?('admin')
-      # We want to avoid leaking of internal information but also want the user
-      # to give the administrator a reference to find the cause of the error.
-      # Therefore we generate a one time unique error ID that can be used to
-      # search the logs and find the actual error message.
+    elsif !policy(Exceptions).view_details?
       error_code_prefix = "Error ID #{SecureRandom.urlsafe_base64(6)}:"
       Rails.logger.error "#{error_code_prefix} #{data[:error]}"
       data[:error] = "#{error_code_prefix} Please contact your administrator."

app/controllers/application_controller/has_user.rb

@@ -10,10 +10,7 @@ module ApplicationController::HasUser
   private
 
   def current_user
-    user_on_behalf = current_user_on_behalf
-    return user_on_behalf if user_on_behalf
-
-    current_user_real
+    current_user_on_behalf || current_user_real
   end
 
   # Finds the User with the ID stored in the session with the key
@@ -21,10 +18,7 @@ module ApplicationController::HasUser
   # a Rails application; logging in sets the session value and
   # logging out removes it.
   def current_user_real
-    return @_current_user if @_current_user
-    return if !session[:user_id]
-
-    @_current_user = User.lookup(id: session[:user_id])
+    @_current_user ||= User.lookup(id: session[:user_id]) # rubocop:disable Naming/MemoizedInstanceVariableName
   end
 
   # Finds the user based on the id, login or email which is given
@@ -33,42 +27,24 @@ module ApplicationController::HasUser
   # to do changes with a user which is different from the admin user.
   # E.g. create a ticket as a customer user based on a user with admin rights.
   def current_user_on_behalf
-
-    # check header
-    return if request.headers['X-On-Behalf-Of'].blank?
-
-    # return user if set
-    return @_user_on_behalf if @_user_on_behalf
-
-    # get current user
-    user_real = current_user_real
-    return if !user_real
-
-    # check if the user has admin rights
-    raise Exceptions::Forbidden, "Current user has no permission to use 'X-On-Behalf-Of'!" if !user_real.permissions?('admin.user')
+    return if request.headers['X-On-Behalf-Of'].blank?  # require header
+    return @_user_on_behalf if @_user_on_behalf         # return memoized user
+    return if !current_user_real                        # require session user
+    if !SessionsPolicy.new(current_user_real, Sessions).impersonate?
+      raise Exceptions::Forbidden, "Current user has no permission to use 'X-On-Behalf-Of'!"
+    end
 
     # find user for execution based on the header
     %i[id login email].each do |field|
-      search_attributes = search_attributes(field)
-      @_user_on_behalf = User.find_by(search_attributes)
-      next if !@_user_on_behalf
+      @_user_on_behalf = User.find_by(field => request.headers['X-On-Behalf-Of'].to_s.downcase.strip)
 
-      return @_user_on_behalf
+      return @_user_on_behalf if @_user_on_behalf
     end
 
     # no behalf of user found
     raise Exceptions::Forbidden, "No such user '#{request.headers['X-On-Behalf-Of']}'"
   end
 
-  def search_attributes(field)
-    search_attributes = {}
-    search_attributes[field] = request.headers['X-On-Behalf-Of']
-    if %i[login email].include?(field)
-      search_attributes[field] = search_attributes[field].to_s.downcase.strip
-    end
-    search_attributes
-  end
-
   def current_user_set(user, auth_type = 'session')
     session[:user_id] = user.id
     @_auth_type = auth_type
@@ -79,11 +55,7 @@ module ApplicationController::HasUser
   # Sets the current user into a named Thread location so that it can be accessed
   # by models and observers
   def set_user
-    if !current_user
-      UserInfo.current_user_id = 1
-      return
-    end
-    UserInfo.current_user_id = current_user.id
+    UserInfo.current_user_id = current_user&.id || 1
   end
 
   # update session updated_at

app/controllers/attachments_controller.rb

@@ -1,9 +1,9 @@
 # Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
 
 class AttachmentsController < ApplicationController
+  prepend_before_action :authorize!, only: %i[show destroy]
   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
     content   = @file.content_preview if params[:preview] && @file.preferences[:content_preview]
@@ -80,12 +80,12 @@ class AttachmentsController < ApplicationController
     raise Exceptions::Forbidden, "Invalid disposition #{disposition} requested. Only #{valid_disposition.join(', ')} are valid."
   end
 
-  def verify_object_permissions
+  def authorize!
     @file = Store.find(params[:id])
 
-    klass = @file&.store_object&.name&.safe_constantize
-    return if klass.send("can_#{params[:action]}_attachment?", @file, current_user)
-
+    record = @file&.store_object&.name&.safe_constantize&.find(@file.o_id)
+    authorize(record) if record
+  rescue Pundit::NotAuthorizedError
     raise ActiveRecord::RecordNotFound
   end
 end

app/controllers/form_controller.rb

@@ -6,7 +6,7 @@ class FormController < ApplicationController
   skip_before_action :verify_csrf_token
   before_action :cors_preflight_check
   after_action :set_access_control_headers_execute
-  skip_before_action :user_device_check
+  skip_before_action :user_device_log
 
   def configuration
     return if !fingerprint_exists?

app/controllers/knowledge_base/public/answers_controller.rb

@@ -13,10 +13,8 @@ class KnowledgeBase::Public::AnswersController < KnowledgeBase::Public::BaseCont
   private
 
   def render_alternative
-    @alternative = @knowledge_base
-                   .answers
+    @alternative = policy_scope(@knowledge_base.answers)
                    .eager_load(translations: :kb_locale)
-                   .check_published_unless_editor(current_user)
                    .find_by(id: params[:answer])
 
     raise ActiveRecord::RecordNotFound if !@alternative&.translations&.any?

app/controllers/knowledge_base/public/base_controller.rb

@@ -2,7 +2,7 @@
 
 class KnowledgeBase::Public::BaseController < ApplicationController
   before_action :load_kb
-  helper_method :system_locale_via_uri, :fallback_locale, :current_user, :editor?, :find_category, :filter_primary_kb_locale, :menu_items, :all_locales
+  helper_method :system_locale_via_uri, :fallback_locale, :current_user, :find_category, :filter_primary_kb_locale, :menu_items, :all_locales
 
   layout 'knowledge_base'
 
@@ -11,8 +11,7 @@ class KnowledgeBase::Public::BaseController < ApplicationController
   private
 
   def load_kb
-    @knowledge_base = KnowledgeBase
-                      .check_active_unless_editor(current_user)
+    @knowledge_base = policy_scope(KnowledgeBase)
                       .localed(guess_locale_via_uri)
                       .first
 
@@ -63,26 +62,20 @@ class KnowledgeBase::Public::BaseController < ApplicationController
     list
       .localed(system_locale_via_uri)
       .sorted
-      .to_a
-      .select { |elem| elem.visible_content_for?(current_user) }
+      .select { |category| policy(category).show? }
   end
 
   def find_answer(scope, id)
     return if scope.nil?
 
-    scope
+    policy_scope(scope)
       .localed(system_locale_via_uri)
       .include_contents
-      .check_published_unless_editor(current_user)
       .find_by(id: id)
   end
 
-  def editor?
-    current_user&.permissions? 'knowledge_base.editor'
-  end
-
   def not_found(e)
-    @knowledge_base = KnowledgeBase.check_active_unless_editor(current_user).first
+    @knowledge_base = policy_scope(KnowledgeBase).first
 
     if @knowledge_base.nil?
       super

app/controllers/knowledge_base/public/categories_controller.rb

@@ -7,30 +7,28 @@ class KnowledgeBase::Public::CategoriesController < KnowledgeBase::Public::BaseC
     @categories     = categories_filter(@knowledge_base.categories.root)
     @object_locales = find_locales(@knowledge_base)
 
-    raise ActiveRecord::RecordNotFound if !editor? && @categories.empty?
+    authorize(@categories, policy_class: KnowledgeBase::Public::CategoryPolicy)
+  rescue Pundit::NotAuthorizedError
+    raise ActiveRecord::RecordNotFound
   end
 
   def show
     @object = find_category(params[:category])
 
-    render_alternatives && return if !@object&.visible_content_for?(current_user)
+    render_alternatives && return if @object.nil? || !policy(@object).show?
 
     @categories     = categories_filter(@object.children)
     @object_locales = find_locales(@object)
 
-    @answers = @object
-               .answers
+    @answers = policy_scope(@object.answers)
                .localed(system_locale_via_uri)
-               .check_published_unless_editor(current_user)
                .sorted
 
     render :index
   end
 
   def forward_root
-    knowledge_base = KnowledgeBase
-                     .check_active_unless_editor(current_user)
-                     .first!
+    knowledge_base = policy_scope(KnowledgeBase).first!
 
     primary_locale = KnowledgeBase::Locale
                      .system_with_kb_locales(knowledge_base)
@@ -54,7 +52,7 @@ class KnowledgeBase::Public::CategoriesController < KnowledgeBase::Public::BaseC
                    .eager_load(translations: :kb_locale)
                    .find_by(id: params[:category])
 
-    if !@alternative&.translations&.any? || !@alternative&.visible_content_for?(current_user)
+    if @alternative.nil? || @alternative.translations.none? || !policy(@alternative).show?
       raise ActiveRecord::RecordNotFound
     end
 

app/controllers/sessions_controller.rb

@@ -3,7 +3,7 @@
 class SessionsController < ApplicationController
   prepend_before_action -> { authentication_check && authorize! }, only: %i[switch_to_user list delete]
   skip_before_action :verify_csrf_token, only: %i[show destroy create_omniauth failure_omniauth]
-  skip_before_action :user_device_check, only: %i[create_sso]
+  skip_before_action :user_device_log, only: %i[create_sso]
 
   # "Create" a login, aka "log the user in"
   def create

app/helpers/knowledge_base_visibility_class_helper.rb

@@ -2,7 +2,7 @@
 
 module KnowledgeBaseVisibilityClassHelper
   def visibility_class_name(object)
-    return if !current_user&.permissions?('knowledge_base.editor')
+    return if !policy(:knowledge_base).edit?
 
     suffix = case object
              when CanBePublished

app/helpers/knowledge_base_visibility_note_helper.rb

@@ -2,7 +2,7 @@
 
 module KnowledgeBaseVisibilityNoteHelper
   def visibility_note(object)
-    return if !current_user&.permissions?('knowledge_base.editor')
+    return if !policy(:knowledge_base).edit?
 
     text = visibility_text(object)
 

app/models/activity_stream.rb

@@ -107,22 +107,9 @@ return all activity entries of an user
 =end
 
   def self.list(user, limit)
-    # do not return an activity stream for customers
-    return [] if !user.permissions?('ticket.agent') && !user.permissions?('admin')
-
-    permission_ids = user.permissions_with_child_ids
-    group_ids = user.group_ids_access('read')
-
-    if group_ids.blank?
-      ActivityStream.where('(permission_id IN (?) AND group_id IS NULL)', permission_ids)
+    ActivityStreamPolicy::Scope.new(user, self).resolve
                                .order(created_at: :desc)
                                .limit(limit)
-    else
-      ActivityStream.where('(permission_id IN (?) AND (group_id IS NULL OR group_id IN (?))) OR (permission_id IS NULL AND group_id IN (?))', permission_ids, group_ids, group_ids)
-                    .order(created_at: :desc)
-                    .limit(limit)
-    end
-
   end
 
 =begin

app/models/concerns/can_be_published.rb

@@ -79,20 +79,6 @@ module CanBePublished
     scope :date_later_or_nil, lambda { |field, timestamp|
       where arel_table[field].gt(timestamp).or(arel_table[field].eq(nil))
     }
-
-    scope :check_published_unless_editor, lambda { |user|
-      return if user&.permissions? 'knowledge_base.editor'
-
-      published
-    }
-
-    scope :check_internal_unless_editor, lambda { |user|
-      return if user&.permissions? 'knowledge_base.editor'
-
-      return internal if user&.permissions? 'knowledge_base.reader'
-
-      published
-    }
   end
 
   def update_user_references

app/models/concerns/has_knowledge_base_attachment_permissions.rb

@@ -1,39 +0,0 @@
-# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
-
-module HasKnowledgeBaseAttachmentPermissions
-  extend ActiveSupport::Concern
-
-  class_methods do
-    def can_show_attachment?(file, user)
-      return true if user_kb_editor?(user)
-
-      object = find(file.o_id)
-
-      return object&.visible_internally? if user_kb_reader?(user)
-
-      object&.visible?
-    end
-
-    def can_destroy_attachment?(_file, user)
-      return if !user_kb_editor?(user)
-
-      true
-    end
-
-    def user_kb_editor?(user)
-      return false if user.nil?
-
-      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
-    private_class_method :user_kb_editor?
-  end
-end

app/models/knowledge_base.rb

@@ -34,12 +34,6 @@ class KnowledgeBase < ApplicationModel
 
   scope :active, -> { where(active: true) }
 
-  scope :check_active_unless_editor, lambda { |user|
-    return if user&.permissions? 'knowledge_base.editor'
-
-    active
-  }
-
   alias assets_essential assets
 
   def assets(data)

app/models/knowledge_base/answer.rb

@@ -4,7 +4,6 @@ class KnowledgeBase::Answer < ApplicationModel
   include HasTranslations
   include HasAgentAllowedParams
   include CanBePublished
-  include HasKnowledgeBaseAttachmentPermissions
   include ChecksKbClientNotification
   include CanCloneAttachments
 

app/models/knowledge_base/answer/translation/content.rb

@@ -3,7 +3,6 @@
 class KnowledgeBase::Answer::Translation::Content < ApplicationModel
   include HasAgentAllowedParams
   include HasRichText
-  include HasKnowledgeBaseAttachmentPermissions
 
   AGENT_ALLOWED_ATTRIBUTES = %i[body].freeze
 

app/models/knowledge_base/category.rb

@@ -115,12 +115,6 @@ class KnowledgeBase::Category < ApplicationModel
     public_content?(kb_locale)
   end
 
-  def visible_content_for?(user)
-    return true if user&.permissions? 'knowledge_base.editor'
-
-    public_content?
-  end
-
   def api_url
     Rails.application.routes.url_helpers.knowledge_base_category_path(knowledge_base, self)
   end

app/models/ticket/overviews.rb

@@ -19,27 +19,10 @@ returns
 =end
 
   def self.all(data)
-    current_user        = data[:current_user]
-    overview_filter     = {}
-    overview_filter_not = { out_of_office: true }
-
-    return [] if !current_user.permissions?('ticket.customer') && !current_user.permissions?('ticket.agent')
-
-    role_ids = User.joins(:roles).where(users: { id: current_user.id, active: true }, roles: { active: true }).pluck('roles.id')
-
-    if data[:links].present?
-      overview_filter[:link] = data[:links]
-    end
-
-    overview_filter[:active] = true
-    if User.where('out_of_office = ? AND out_of_office_start_at <= ? AND out_of_office_end_at >= ? AND out_of_office_replacement_id = ? AND active = ?', true, Time.zone.today, Time.zone.today, current_user.id, true).count.positive?
-      overview_filter_not = {}
-    end
-    if !current_user.organization_id || !current_user.organization.shared
-      overview_filter[:organization_shared] = false
-    end
-
-    Overview.joins(:roles).left_joins(:users).where(overviews_roles: { role_id: role_ids }, overviews_users: { user_id: nil }, overviews: overview_filter).or(Overview.joins(:roles).left_joins(:users).where(overviews_roles: { role_id: role_ids }, overviews_users: { user_id: current_user.id }, overviews: overview_filter)).where.not(overview_filter_not).distinct('overview.id').order(:prio, :name)
+    Ticket::OverviewsPolicy::Scope.new(data[:current_user], Overview).resolve
+                                  .where({ link: data[:links] }.compact)
+                                  .distinct
+                                  .order(:prio, :name)
   end
 
 =begin

app/policies/activity_stream_policy.rb

@@ -0,0 +1,3 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class ActivityStreamPolicy < ApplicationPolicy; end

app/policies/activity_stream_policy/scope.rb

@@ -0,0 +1,30 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class ActivityStreamPolicy < ApplicationPolicy
+  class Scope < ApplicationPolicy::Scope
+    def resolve
+      if customer?
+        scope.where(id: nil)
+      elsif group_ids.blank?
+        scope.where(permission_id: permission_ids, group_id: nil)
+      else
+        scope.where(permission_id: [*permission_ids, nil], group_id: [*group_ids, nil])
+          .where.not('permission_id IS NULL AND group_id IS NULL')
+      end
+    end
+
+    private
+
+    def customer?
+      !user.permissions?(%w[admin ticket.agent])
+    end
+
+    def permission_ids
+      @permission_ids ||= user.permissions_with_child_ids
+    end
+
+    def group_ids
+      @group_ids ||= user.group_ids_access('read')
+    end
+  end
+end

app/policies/exceptions_policy.rb

@@ -0,0 +1,11 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class ExceptionsPolicy < ApplicationPolicy
+  # We want to avoid leaking of internal information but also want the user
+  # to give the administrator a reference to find the cause of the error.
+  # Therefore we generate a one time unique error ID that can be used to
+  # search the logs and find the actual error message.
+  def view_details?
+    user&.permissions?('admin')
+  end
+end

app/policies/knowledge_base/answer/translation/content_policy.rb

@@ -0,0 +1,14 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class KnowledgeBase::Answer::Translation::ContentPolicy < ApplicationPolicy
+  def show?
+    return true if user&.permissions?(%w[knowledge_base.editor])
+
+    record.translation.answer.visible? ||
+      (user&.permissions?(%w[knowledge_base.reader]) && record.translation.answer.visible_internally?)
+  end
+
+  def destroy?
+    user&.permissions?(%w[knowledge_base.editor])
+  end
+end

app/policies/knowledge_base/answer_policy.rb

@@ -0,0 +1,14 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class KnowledgeBase::AnswerPolicy < ApplicationPolicy
+  def show?
+    return true if user&.permissions?(%w[knowledge_base.editor])
+
+    record.visible? ||
+      (user&.permissions?(%w[knowledge_base.reader]) && record.visible_internally?)
+  end
+
+  def destroy?
+    user&.permissions?(%w[knowledge_base.editor])
+  end
+end

app/policies/knowledge_base/answer_policy/scope.rb

@@ -0,0 +1,19 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class KnowledgeBase::AnswerPolicy < ApplicationPolicy
+  class Scope < ApplicationPolicy::Scope
+    def resolve
+      if user&.permissions?('knowledge_base.editor')
+        scope
+      else
+        scope.published
+      end
+    end
+
+    private
+
+    def user_required?
+      false
+    end
+  end
+end

app/policies/knowledge_base/category_policy.rb

@@ -0,0 +1,15 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class KnowledgeBase::CategoryPolicy < ApplicationPolicy
+  def show?
+    return true if user&.permissions?('knowledge_base.editor')
+
+    record.public_content?
+  end
+
+  private
+
+  def user_required?
+    false
+  end
+end

app/policies/knowledge_base/public/category_policy.rb

@@ -0,0 +1,10 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class KnowledgeBase::Public::CategoryPolicy < ApplicationPolicy
+  def index?
+    return true if user&.permissions?('knowledge_base.editor')
+    return true if record.any?
+
+    false
+  end
+end

app/policies/knowledge_base_policy.rb

@@ -0,0 +1,7 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class KnowledgeBasePolicy < ApplicationPolicy
+  def edit?
+    user&.permissions?('knowledge_base.editor')
+  end
+end

app/policies/knowledge_base_policy/scope.rb

@@ -0,0 +1,19 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class KnowledgeBasePolicy < ApplicationPolicy
+  class Scope < ApplicationPolicy::Scope
+    def resolve
+      if user&.permissions?('knowledge_base.editor')
+        scope
+      else
+        scope.active
+      end
+    end
+
+    private
+
+    def user_required?
+      false
+    end
+  end
+end

app/policies/sessions_policy.rb

@@ -0,0 +1,7 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class SessionsPolicy < ApplicationPolicy
+  def impersonate?
+    user.permissions?('admin.user')
+  end
+end

app/policies/ticket/overviews_policy.rb

@@ -0,0 +1,3 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class Ticket::OverviewsPolicy < ApplicationPolicy; end

app/policies/ticket/overviews_policy/scope.rb

@@ -0,0 +1,54 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class Ticket::OverviewsPolicy < ApplicationPolicy
+  class Scope < ApplicationPolicy::Scope
+    def resolve
+      if user.permissions?('ticket.customer')
+        customer_scope
+      elsif user.permissions?('ticket.agent')
+        agent_scope
+      else
+        empty_scope
+      end
+    end
+
+    private
+
+    def customer_scope
+      if user.organization&.shared
+        base_query
+      else
+        base_query.where(organization_shared: false)
+      end
+    end
+
+    def agent_scope
+      if user_is_someones_out_of_office_replacement?
+        base_query
+      else
+        base_query.where.not(out_of_office: true)
+      end
+    end
+
+    def empty_scope
+      scope.where(id: nil)
+    end
+
+    def base_query
+      scope.joins(roles: :users)
+              .where(active: true)
+              .where(roles: { active: true })
+              .where(users: { id: user.id, active: true })
+              .left_joins(:users)
+              .where(overviews_users: { user_id: [nil, user.id] })
+    end
+
+    def user_is_someones_out_of_office_replacement?
+      User.where(out_of_office: true)
+          .where('out_of_office_start_at <= ?', Time.zone.today)
+          .where('out_of_office_end_at >= ?', Time.zone.today)
+          .where(out_of_office_replacement_id: user.id)
+          .exists?(active: true)
+    end
+  end
+end

app/policies/user_device_policy.rb

@@ -0,0 +1,7 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class UserDevicePolicy < ApplicationPolicy
+  def log?
+    user&.permissions?('user_preferences.device')
+  end
+end

app/views/layouts/knowledge_base.html.erb

@@ -16,7 +16,7 @@
 <%= canonical_link_tag @knowledge_base, @category, @object %>
 
 <div class="wrapper js-wrapper">
-  <%= render 'knowledge_base/public/top_banner', object: @object || @knowledge_base if editor? %>
+  <%= render 'knowledge_base/public/top_banner', object: @object || @knowledge_base if policy(:knowledge_base).edit? %>
 
   <header class="header js-header">
     <div class="container">

config/brakeman.ignore

@@ -460,26 +460,6 @@
       "confidence": "Medium",
       "note": "Admin configured RegExp"
     },
-    {
-      "warning_type": "Dangerous Send",
-      "warning_code": 23,
-      "fingerprint": "b24120f184a097dbf931fd0c251e26b431fc4a0ac679f02ed2c85a9ebb338cc7",
-      "check_name": "Send",
-      "message": "User controlled method execution",
-      "file": "app/controllers/attachments_controller.rb",
-      "line": 87,
-      "link": "https://brakemanscanner.org/docs/warning_types/dangerous_send/",
-      "code": "Store.find(params[:id]).store_object.name.safe_constantize.send(\"can_#{params[:action]}_attachment?\", Store.find(params[:id]), current_user)",
-      "render_path": null,
-      "location": {
-        "type": "method",
-        "class": "AttachmentsController",
-        "method": "verify_object_permissions"
-      },
-      "user_input": "params[:action]",
-      "confidence": "High",
-      "note": "Save because 'before_action' is limited to defined actions"
-    },
     {
       "warning_type": "Remote Code Execution",
       "warning_code": 24,
@@ -507,7 +487,7 @@
       "check_name": "Execute",
       "message": "Possible command injection",
       "file": "lib/mysql_strategy.rb",
-      "line": 64,
+      "line": 62,
       "link": "https://brakemanscanner.org/docs/warning_types/command_injection/",
       "code": "system(\"mysqldump #{mysql_arguments} > #{backup_file}\", :exception => true)",
       "render_path": null,
@@ -603,7 +583,7 @@
     {
       "warning_type": "Remote Code Execution",
       "warning_code": 24,
-      "fingerprint": "e8789f715661836374232aa2ba4aa9eacbba8ea4961f5b26468a0d96c4c91e33",
+      "fingerprint": "dfe8a5a18f3d403c3cb32a50bf9b10da7254fa6b958c45fa5d6b8d97ae017961",
       "check_name": "UnsafeReflection",
       "message": "Unsafe reflection method `safe_constantize` called with model attribute",
       "file": "app/controllers/attachments_controller.rb",
@@ -614,11 +594,11 @@
       "location": {
         "type": "method",
         "class": "AttachmentsController",
-        "method": "verify_object_permissions"
+        "method": "authorize!"
       },
       "user_input": "Store.find(params[:id]).store_object",
       "confidence": "Medium",
-      "note": "Store#store_object returns defined Model objects"
+      "note": "Works as designed."
     },
     {
       "warning_type": "Denial of Service",
@@ -703,7 +683,7 @@
       "check_name": "Execute",
       "message": "Possible command injection",
       "file": "lib/mysql_strategy.rb",
-      "line": 56,
+      "line": 54,
       "link": "https://brakemanscanner.org/docs/warning_types/command_injection/",
       "code": "system(\"mysql #{mysql_arguments} < #{backup_file}\", :exception => true)",
       "render_path": null,
@@ -737,6 +717,6 @@
       "note": "Admin configured RegExp"
     }
   ],
-  "updated": "2021-07-22 13:52:48 +0200",
+  "updated": "2021-07-23 08:25:01 +0200",
   "brakeman_version": "5.1.1"
 }

lib/exceptions.rb

@@ -8,4 +8,8 @@ module Exceptions
 
   class UnprocessableEntity < StandardError; end
 
+  def self.policy_class
+    ExceptionsPolicy
+  end
+
 end

spec/models/knowledge_base_spec.rb

@@ -48,20 +48,6 @@ RSpec.describe KnowledgeBase, type: :model do
         it 'filter by activity' do
           expect(described_class.active).to contain_exactly(knowledge_base)
         end
-
-        it 'skip activity check for editors when filtering by activity' do
-          user = create(:admin)
-          expect(described_class.check_active_unless_editor(user).count).to eq(2)
-        end
-
-        it 'check activity if user is not editor when filtering by activity' do
-          user = create(:agent)
-          expect(described_class.check_active_unless_editor(user)).to contain_exactly(knowledge_base)
-        end
-
-        it 'skip activity check for guests when filtering by activity' do
-          expect(described_class.check_active_unless_editor(nil)).to contain_exactly(knowledge_base)
-        end
       end
     end
   end

spec/policies/activity_stream_policy/scope_spec.rb

@@ -0,0 +1,101 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe ActivityStreamPolicy::Scope do
+  subject(:scope) { described_class.new(user, ActivityStream) }
+
+  describe '#resolve' do
+    let!(:activity_streams) do
+      {
+        permissionless: {
+          grouped:   create(:activity_stream, permission_id: nil, group_id: Group.first.id),
+          groupless: create(:activity_stream, permission_id: nil, group_id: nil),
+        },
+        admin:          {
+          grouped:   create(:activity_stream, permission_id: admin_permission.id, group_id: Group.first.id),
+          groupless: create(:activity_stream, permission_id: admin_permission.id, group_id: nil),
+        },
+        agent:          {
+          grouped:   create(:activity_stream, permission_id: agent_permission.id, group_id: Group.first.id),
+          groupless: create(:activity_stream, permission_id: agent_permission.id, group_id: nil),
+        }
+      }
+    end
+
+    let(:admin_permission) { Permission.find_by(name: 'admin') }
+    let(:agent_permission) { Permission.find_by(name: 'ticket.agent') }
+
+    context 'with customer' do
+      let(:user) { create(:customer) }
+
+      it 'returns an empty ActiveRecord::Relation (no arrays--must be chainable!)' do
+        expect(scope.resolve)
+          .to be_empty
+          .and be_an(ActiveRecord::Relation)
+      end
+    end
+
+    context 'with groupless agent' do
+      let(:user) { create(:agent, groups: []) }
+
+      it 'returns agent ActivityStreams (w/o permission: nil)' do
+        expect(scope.resolve)
+          .to match_array([activity_streams[:agent][:groupless]])
+      end
+
+      it 'does not include groups’ agent ActivityStreams' do
+        expect(scope.resolve)
+          .not_to include([activity_streams[:agent][:grouped]])
+      end
+    end
+
+    context 'with grouped agent' do
+      let(:user) { create(:agent, groups: [Group.first]) }
+
+      it 'returns same ActivityStreams as groupless agent, plus groups’ (WITH permission: nil)' do
+        expect(scope.resolve)
+          .to match_array([activity_streams[:permissionless][:grouped],
+                           *activity_streams[:agent].values])
+      end
+    end
+
+    context 'with groupless admin' do
+      # Why do we need Import Mode?
+      # Without it, create(:admin) generates yet another ActivityStream
+      let(:user) do
+        Setting.set('import_mode', true)
+          .then { create(:admin, groups: []) }
+          .tap { Setting.set('import_mode', false) }
+      end
+
+      it 'returns agent/admin ActivityStreams (w/o permission: nil)' do
+        expect(scope.resolve)
+          .to match_array([activity_streams[:admin][:groupless],
+                           activity_streams[:agent][:groupless]])
+      end
+
+      it 'does not include groups’ agent ActivityStreams' do
+        expect(scope.resolve)
+          .not_to include([activity_streams[:admin][:grouped]])
+      end
+    end
+
+    context 'with grouped admin' do
+      # Why do we need Import Mode?
+      # Without it, create(:admin) generates yet another ActivityStream
+      let(:user) do
+        Setting.set('import_mode', true)
+          .then { create(:admin, groups: [Group.first]) }
+          .tap { Setting.set('import_mode', false) }
+      end
+
+      it 'returns same ActivityStreams as groupless admin, plus groups’ (WITH permission: nil)' do
+        expect(scope.resolve)
+          .to match_array([activity_streams[:permissionless][:grouped],
+                           *activity_streams[:admin].values,
+                           *activity_streams[:agent].values])
+      end
+    end
+  end
+end

spec/policies/knowledge_base/answer_policy/scope_spec.rb

@@ -0,0 +1,53 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe KnowledgeBase::AnswerPolicy::Scope, current_user_id: 1 do
+  subject(:scope) { described_class.new(user, original_collection) }
+
+  let(:original_collection) { KnowledgeBase::Answer }
+
+  before do # populate DB
+    create_list(:'knowledge_base/answer', 2, published_at: Time.zone.yesterday)
+    create_list(:'knowledge_base/answer', 2, published_at: Time.zone.yesterday, archived_at: Time.zone.now)
+  end
+
+  describe '#resolve' do
+    let(:roles) { user.roles }
+    let(:permission) { Permission.find_by(name: 'knowledge_base.editor') }
+
+    context 'without user' do
+      let(:user) { nil }
+
+      it 'removes unpublished and archived answers' do
+        expect(scope.resolve)
+          .to match_array(original_collection.where(<<~QUERY, now: Time.zone.now))
+            published_at < :now AND (archived_at IS NULL OR archived_at > :now)
+          QUERY
+      end
+    end
+
+    context 'without "knowledge_base.editor" permissions' do
+      let(:user) { create(:admin) }
+
+      before { roles.each { |r| r.permissions.delete(permission) } }
+
+      it 'removes unpublished and archived answers' do
+        expect(scope.resolve)
+          .to match_array(original_collection.where(<<~QUERY, now: Time.zone.now))
+            published_at < :now AND (archived_at IS NULL OR archived_at > :now)
+          QUERY
+      end
+    end
+
+    context 'with "knowledge_base.editor" permissions' do
+      let(:user) { create(:user) }
+
+      before { roles.first.permissions << permission }
+
+      it 'returns the given collection (unfiltered)' do
+        expect(scope.resolve).to eq(original_collection)
+      end
+    end
+  end
+end

spec/policies/knowledge_base_policy/scope_spec.rb

@@ -0,0 +1,47 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe KnowledgeBasePolicy::Scope do
+  subject(:scope) { described_class.new(user, original_collection) }
+
+  let(:original_collection) { KnowledgeBase }
+
+  before do # populate DB
+    create_list(:knowledge_base, 2, active: true)
+    create_list(:knowledge_base, 2, active: false)
+  end
+
+  describe '#resolve' do
+    let(:roles) { user.roles }
+    let(:permission) { Permission.find_by(name: 'knowledge_base.editor') }
+
+    context 'without user' do
+      let(:user) { nil }
+
+      it 'removes only inactive knowledge bases' do
+        expect(scope.resolve).to eq(original_collection.where(active: true))
+      end
+    end
+
+    context 'without "knowledge_base.editor" permissions' do
+      let(:user) { create(:admin) }
+
+      before { roles.each { |r| r.permissions.delete(permission) } }
+
+      it 'removes only inactive knowledge bases' do
+        expect(scope.resolve).to eq(original_collection.where(active: true))
+      end
+    end
+
+    context 'with "knowledge_base.editor" permissions' do
+      let(:user) { create(:user) }
+
+      before { roles.first.permissions << permission }
+
+      it 'returns the given collection (unfiltered)' do
+        expect(scope.resolve).to eq(original_collection)
+      end
+    end
+  end
+end

spec/policies/organization_policy_spec.rb

@@ -11,13 +11,13 @@ describe OrganizationPolicy do
     let(:user) { create(:customer, organization: record) }
 
     it { is_expected.to permit_actions(%i[show]) }
-    it { is_expected.not_to permit_actions(%i[update]) }
+    it { is_expected.to forbid_actions(%i[update]) }
   end
 
   context 'when customer without organization' do
     let(:user) { create(:customer) }
 
-    it { is_expected.not_to permit_actions(%i[show update]) }
+    it { is_expected.to forbid_actions(%i[show update]) }
   end
 
   context 'when agent and customer' do

spec/policies/ticket/article_policy_spec.rb

@@ -37,13 +37,13 @@ describe Ticket::ArticlePolicy do
         create(:agent_and_customer, roles: [customer_role])
       end
 
-      it { is_expected.not_to permit_actions(%i[show]) }
+      it { is_expected.to forbid_actions(%i[show]) }
     end
 
     context 'when customer' do
       let(:user) { ticket_customer }
 
-      it { is_expected.not_to permit_actions(%i[show]) }
+      it { is_expected.to forbid_actions(%i[show]) }
     end
   end
 

spec/policies/ticket_policy_spec.rb

@@ -10,7 +10,7 @@ describe TicketPolicy do
   context 'when given ticket’s owner' do
     let(:user) { record.owner }
 
-    it { is_expected.not_to permit_actions(%i[show full]) }
+    it { is_expected.to forbid_actions(%i[show full]) }
 
     context 'when owner has ticket.agent permission' do
 
@@ -33,7 +33,7 @@ describe TicketPolicy do
   context 'when given a user that is neither owner nor customer' do
     let(:user) { create(:agent) }
 
-    it { is_expected.not_to permit_actions(%i[show full]) }
+    it { is_expected.to forbid_actions(%i[show full]) }
 
     context 'but the user is an agent with full access to ticket’s group' do
       before { user.group_names_access_map = { record.group.name => 'full' } }
@@ -54,14 +54,14 @@ describe TicketPolicy do
       context 'but organization.shared is false' do
         before { customer.organization.update(shared: false) }
 
-        it { is_expected.not_to permit_actions(%i[show full]) }
+        it { is_expected.to forbid_actions(%i[show full]) }
       end
     end
 
     context 'when user is admin with group access' do
       let(:user) { create(:user, roles: Role.where(name: %w[Admin])) }
 
-      it { is_expected.not_to permit_actions(%i[show full]) }
+      it { is_expected.to forbid_actions(%i[show full]) }
     end
   end
 

spec/policies/user_policy_spec.rb

@@ -56,35 +56,35 @@ describe UserPolicy do
         let(:record) { create(:admin) }
 
         it { is_expected.to permit_action(:show) }
-        it { is_expected.not_to permit_actions(%i[update destroy]) }
+        it { is_expected.to forbid_actions(%i[update destroy]) }
       end
 
       context 'when record is an agent user' do
         let(:record) { create(:agent) }
 
         it { is_expected.to permit_action(:show) }
-        it { is_expected.not_to permit_actions(%i[update destroy]) }
+        it { is_expected.to forbid_actions(%i[update destroy]) }
       end
 
       context 'when record is a customer user' do
         let(:record) { create(:customer) }
 
         it { is_expected.to permit_action(:show) }
-        it { is_expected.not_to permit_actions(%i[update destroy]) }
+        it { is_expected.to forbid_actions(%i[update destroy]) }
       end
 
       context 'when record is any user' do
         let(:record) { create(:user) }
 
         it { is_expected.to permit_action(:show) }
-        it { is_expected.not_to permit_actions(%i[update destroy]) }
+        it { is_expected.to forbid_actions(%i[update destroy]) }
       end
 
       context 'when record is the same user' do
         let(:record) { user }
 
         it { is_expected.to permit_action(:show) }
-        it { is_expected.not_to permit_actions(%i[update destroy]) }
+        it { is_expected.to forbid_actions(%i[update destroy]) }
       end
     end
   end
@@ -96,35 +96,35 @@ describe UserPolicy do
       let(:record) { create(:admin) }
 
       it { is_expected.to permit_action(:show) }
-      it { is_expected.not_to permit_actions(%i[update destroy]) }
+      it { is_expected.to forbid_actions(%i[update destroy]) }
     end
 
     context 'when record is an agent user' do
       let(:record) { create(:agent) }
 
       it { is_expected.to permit_action(:show) }
-      it { is_expected.not_to permit_actions(%i[update destroy]) }
+      it { is_expected.to forbid_actions(%i[update destroy]) }
     end
 
     context 'when record is a customer user' do
       let(:record) { create(:customer) }
 
       it { is_expected.to permit_actions(%i[show update]) }
-      it { is_expected.not_to permit_action(:destroy) }
+      it { is_expected.to forbid_action(:destroy) }
     end
 
     context 'when record is any user' do
       let(:record) { create(:user) }
 
-      it { is_expected.to permit_action(:show) }
-      it { is_expected.not_to permit_actions(%i[update destroy]) }
+      it { is_expected.to permit_actions(%i[show update]) }
+      it { is_expected.to forbid_action(:destroy) }
     end
 
     context 'when record is the same user' do
       let(:record) { user }
 
       it { is_expected.to permit_action(:show) }
-      it { is_expected.not_to permit_actions(%i[update destroy]) }
+      it { is_expected.to forbid_actions(%i[update destroy]) }
     end
   end
 
@@ -134,25 +134,25 @@ describe UserPolicy do
     context 'when record is an admin user' do
       let(:record) { create(:admin) }
 
-      it { is_expected.not_to permit_actions(%i[show update destroy]) }
+      it { is_expected.to forbid_actions(%i[show update destroy]) }
     end
 
     context 'when record is an agent user' do
       let(:record) { create(:agent) }
 
-      it { is_expected.not_to permit_actions(%i[show update destroy]) }
+      it { is_expected.to forbid_actions(%i[show update destroy]) }
     end
 
     context 'when record is a customer user' do
       let(:record) { create(:customer) }
 
-      it { is_expected.not_to permit_actions(%i[show update destroy]) }
+      it { is_expected.to forbid_actions(%i[show update destroy]) }
     end
 
     context 'when record is any user' do
       let(:record) { create(:user) }
 
-      it { is_expected.not_to permit_actions(%i[show update destroy]) }
+      it { is_expected.to forbid_actions(%i[show update destroy]) }
     end
 
     context 'when record is a colleague' do
@@ -160,14 +160,14 @@ describe UserPolicy do
       let(:record) { create(:customer, organization: user.organization) }
 
       it { is_expected.to permit_action(:show) }
-      it { is_expected.not_to permit_actions(%i[update destroy]) }
+      it { is_expected.to forbid_actions(%i[update destroy]) }
     end
 
     context 'when record is the same user' do
       let(:record) { user }
 
       it { is_expected.to permit_action(:show) }
-      it { is_expected.not_to permit_actions(%i[update destroy]) }
+      it { is_expected.to forbid_actions(%i[update destroy]) }
     end
   end
 end