From c23e174ee04737e43b9f436aaaba496390d3a04f Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Fri, 23 Jul 2021 13:07:16 +0000 Subject: [PATCH] Refactoring: Migrate user permission handling to Pundit policies. --- .../application_controller/handles_devices.rb | 12 +-- .../application_controller/handles_errors.rb | 6 +- .../application_controller/has_user.rb | 50 ++------- app/controllers/attachments_controller.rb | 10 +- app/controllers/form_controller.rb | 2 +- .../public/answers_controller.rb | 4 +- .../knowledge_base/public/base_controller.rb | 17 +-- .../public/categories_controller.rb | 16 ++- app/controllers/sessions_controller.rb | 2 +- .../knowledge_base_visibility_class_helper.rb | 2 +- .../knowledge_base_visibility_note_helper.rb | 2 +- app/models/activity_stream.rb | 19 +--- app/models/concerns/can_be_published.rb | 14 --- ...s_knowledge_base_attachment_permissions.rb | 39 ------- app/models/knowledge_base.rb | 6 -- app/models/knowledge_base/answer.rb | 1 - .../answer/translation/content.rb | 1 - app/models/knowledge_base/category.rb | 6 -- app/models/ticket/overviews.rb | 25 +---- app/policies/activity_stream_policy.rb | 3 + app/policies/activity_stream_policy/scope.rb | 30 ++++++ app/policies/exceptions_policy.rb | 11 ++ .../answer/translation/content_policy.rb | 14 +++ app/policies/knowledge_base/answer_policy.rb | 14 +++ .../knowledge_base/answer_policy/scope.rb | 19 ++++ .../knowledge_base/category_policy.rb | 15 +++ .../knowledge_base/public/category_policy.rb | 10 ++ app/policies/knowledge_base_policy.rb | 7 ++ app/policies/knowledge_base_policy/scope.rb | 19 ++++ app/policies/sessions_policy.rb | 7 ++ app/policies/ticket/overviews_policy.rb | 3 + app/policies/ticket/overviews_policy/scope.rb | 54 ++++++++++ app/policies/user_device_policy.rb | 7 ++ app/views/layouts/knowledge_base.html.erb | 2 +- config/brakeman.ignore | 32 ++---- lib/exceptions.rb | 4 + spec/models/knowledge_base_spec.rb | 14 --- .../activity_stream_policy/scope_spec.rb | 101 ++++++++++++++++++ .../answer_policy/scope_spec.rb | 53 +++++++++ .../knowledge_base_policy/scope_spec.rb | 47 ++++++++ spec/policies/organization_policy_spec.rb | 4 +- spec/policies/ticket/article_policy_spec.rb | 4 +- spec/policies/ticket_policy_spec.rb | 8 +- spec/policies/user_policy_spec.rb | 34 +++--- 44 files changed, 494 insertions(+), 256 deletions(-) delete mode 100644 app/models/concerns/has_knowledge_base_attachment_permissions.rb create mode 100644 app/policies/activity_stream_policy.rb create mode 100644 app/policies/activity_stream_policy/scope.rb create mode 100644 app/policies/exceptions_policy.rb create mode 100644 app/policies/knowledge_base/answer/translation/content_policy.rb create mode 100644 app/policies/knowledge_base/answer_policy.rb create mode 100644 app/policies/knowledge_base/answer_policy/scope.rb create mode 100644 app/policies/knowledge_base/category_policy.rb create mode 100644 app/policies/knowledge_base/public/category_policy.rb create mode 100644 app/policies/knowledge_base_policy.rb create mode 100644 app/policies/knowledge_base_policy/scope.rb create mode 100644 app/policies/sessions_policy.rb create mode 100644 app/policies/ticket/overviews_policy.rb create mode 100644 app/policies/ticket/overviews_policy/scope.rb create mode 100644 app/policies/user_device_policy.rb create mode 100644 spec/policies/activity_stream_policy/scope_spec.rb create mode 100644 spec/policies/knowledge_base/answer_policy/scope_spec.rb create mode 100644 spec/policies/knowledge_base_policy/scope_spec.rb diff --git a/app/controllers/application_controller/handles_devices.rb b/app/controllers/application_controller/handles_devices.rb index a78164ddb..c05543841 100644 --- a/app/controllers/application_controller/handles_devices.rb +++ b/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 diff --git a/app/controllers/application_controller/handles_errors.rb b/app/controllers/application_controller/handles_errors.rb index 4ec11225a..562f5b752 100644 --- a/app/controllers/application_controller/handles_errors.rb +++ b/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." diff --git a/app/controllers/application_controller/has_user.rb b/app/controllers/application_controller/has_user.rb index 2a880dc8f..beb53457a 100644 --- a/app/controllers/application_controller/has_user.rb +++ b/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 diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index a49f9407e..8c2a7cd72 100644 --- a/app/controllers/attachments_controller.rb +++ b/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 diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index 6c12229a4..35945247a 100644 --- a/app/controllers/form_controller.rb +++ b/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? diff --git a/app/controllers/knowledge_base/public/answers_controller.rb b/app/controllers/knowledge_base/public/answers_controller.rb index e06eb334f..dc0b448d2 100644 --- a/app/controllers/knowledge_base/public/answers_controller.rb +++ b/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? diff --git a/app/controllers/knowledge_base/public/base_controller.rb b/app/controllers/knowledge_base/public/base_controller.rb index f0cabc91a..19e4d87d7 100644 --- a/app/controllers/knowledge_base/public/base_controller.rb +++ b/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 diff --git a/app/controllers/knowledge_base/public/categories_controller.rb b/app/controllers/knowledge_base/public/categories_controller.rb index 317ef6933..ab4a0ddef 100644 --- a/app/controllers/knowledge_base/public/categories_controller.rb +++ b/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 diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 9e321e990..f429940ab 100644 --- a/app/controllers/sessions_controller.rb +++ b/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 diff --git a/app/helpers/knowledge_base_visibility_class_helper.rb b/app/helpers/knowledge_base_visibility_class_helper.rb index 8092aa637..a3ac707fb 100644 --- a/app/helpers/knowledge_base_visibility_class_helper.rb +++ b/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 diff --git a/app/helpers/knowledge_base_visibility_note_helper.rb b/app/helpers/knowledge_base_visibility_note_helper.rb index ff3d30b39..389e359ab 100644 --- a/app/helpers/knowledge_base_visibility_note_helper.rb +++ b/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) diff --git a/app/models/activity_stream.rb b/app/models/activity_stream.rb index 18d82eaaf..f35d513a5 100644 --- a/app/models/activity_stream.rb +++ b/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) - .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 - + ActivityStreamPolicy::Scope.new(user, self).resolve + .order(created_at: :desc) + .limit(limit) end =begin diff --git a/app/models/concerns/can_be_published.rb b/app/models/concerns/can_be_published.rb index 7f7bf6e6f..8a898cd31 100644 --- a/app/models/concerns/can_be_published.rb +++ b/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 diff --git a/app/models/concerns/has_knowledge_base_attachment_permissions.rb b/app/models/concerns/has_knowledge_base_attachment_permissions.rb deleted file mode 100644 index 43507cbfc..000000000 --- a/app/models/concerns/has_knowledge_base_attachment_permissions.rb +++ /dev/null @@ -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 diff --git a/app/models/knowledge_base.rb b/app/models/knowledge_base.rb index 73d6c07f2..7c1389a80 100644 --- a/app/models/knowledge_base.rb +++ b/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) diff --git a/app/models/knowledge_base/answer.rb b/app/models/knowledge_base/answer.rb index 2f3083184..17b939216 100644 --- a/app/models/knowledge_base/answer.rb +++ b/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 diff --git a/app/models/knowledge_base/answer/translation/content.rb b/app/models/knowledge_base/answer/translation/content.rb index 3f36cce83..e6aa85418 100644 --- a/app/models/knowledge_base/answer/translation/content.rb +++ b/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 diff --git a/app/models/knowledge_base/category.rb b/app/models/knowledge_base/category.rb index a208509ae..365af40e5 100644 --- a/app/models/knowledge_base/category.rb +++ b/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 diff --git a/app/models/ticket/overviews.rb b/app/models/ticket/overviews.rb index 204228d75..83bca2fda 100644 --- a/app/models/ticket/overviews.rb +++ b/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 diff --git a/app/policies/activity_stream_policy.rb b/app/policies/activity_stream_policy.rb new file mode 100644 index 000000000..6ce7fa5ff --- /dev/null +++ b/app/policies/activity_stream_policy.rb @@ -0,0 +1,3 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +class ActivityStreamPolicy < ApplicationPolicy; end diff --git a/app/policies/activity_stream_policy/scope.rb b/app/policies/activity_stream_policy/scope.rb new file mode 100644 index 000000000..e664328a4 --- /dev/null +++ b/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 diff --git a/app/policies/exceptions_policy.rb b/app/policies/exceptions_policy.rb new file mode 100644 index 000000000..f3c953837 --- /dev/null +++ b/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 diff --git a/app/policies/knowledge_base/answer/translation/content_policy.rb b/app/policies/knowledge_base/answer/translation/content_policy.rb new file mode 100644 index 000000000..257f7a499 --- /dev/null +++ b/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 diff --git a/app/policies/knowledge_base/answer_policy.rb b/app/policies/knowledge_base/answer_policy.rb new file mode 100644 index 000000000..f127c3697 --- /dev/null +++ b/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 diff --git a/app/policies/knowledge_base/answer_policy/scope.rb b/app/policies/knowledge_base/answer_policy/scope.rb new file mode 100644 index 000000000..07d4d335a --- /dev/null +++ b/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 diff --git a/app/policies/knowledge_base/category_policy.rb b/app/policies/knowledge_base/category_policy.rb new file mode 100644 index 000000000..c98cf7b84 --- /dev/null +++ b/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 diff --git a/app/policies/knowledge_base/public/category_policy.rb b/app/policies/knowledge_base/public/category_policy.rb new file mode 100644 index 000000000..6994cd5a0 --- /dev/null +++ b/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 diff --git a/app/policies/knowledge_base_policy.rb b/app/policies/knowledge_base_policy.rb new file mode 100644 index 000000000..ce78385e7 --- /dev/null +++ b/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 diff --git a/app/policies/knowledge_base_policy/scope.rb b/app/policies/knowledge_base_policy/scope.rb new file mode 100644 index 000000000..de3cd0d9d --- /dev/null +++ b/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 diff --git a/app/policies/sessions_policy.rb b/app/policies/sessions_policy.rb new file mode 100644 index 000000000..27eef0cab --- /dev/null +++ b/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 diff --git a/app/policies/ticket/overviews_policy.rb b/app/policies/ticket/overviews_policy.rb new file mode 100644 index 000000000..1f0e47285 --- /dev/null +++ b/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 diff --git a/app/policies/ticket/overviews_policy/scope.rb b/app/policies/ticket/overviews_policy/scope.rb new file mode 100644 index 000000000..4049af964 --- /dev/null +++ b/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 diff --git a/app/policies/user_device_policy.rb b/app/policies/user_device_policy.rb new file mode 100644 index 000000000..7ba69d6bd --- /dev/null +++ b/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 diff --git a/app/views/layouts/knowledge_base.html.erb b/app/views/layouts/knowledge_base.html.erb index 067709cd6..642aca299 100644 --- a/app/views/layouts/knowledge_base.html.erb +++ b/app/views/layouts/knowledge_base.html.erb @@ -16,7 +16,7 @@ <%= canonical_link_tag @knowledge_base, @category, @object %>
- <%= 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? %>
diff --git a/config/brakeman.ignore b/config/brakeman.ignore index dda7bfbd7..5dac02a60 100644 --- a/config/brakeman.ignore +++ b/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" } diff --git a/lib/exceptions.rb b/lib/exceptions.rb index 9c9b8dac2..40068f820 100644 --- a/lib/exceptions.rb +++ b/lib/exceptions.rb @@ -8,4 +8,8 @@ module Exceptions class UnprocessableEntity < StandardError; end + def self.policy_class + ExceptionsPolicy + end + end diff --git a/spec/models/knowledge_base_spec.rb b/spec/models/knowledge_base_spec.rb index c183ea418..067d57ad1 100644 --- a/spec/models/knowledge_base_spec.rb +++ b/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 diff --git a/spec/policies/activity_stream_policy/scope_spec.rb b/spec/policies/activity_stream_policy/scope_spec.rb new file mode 100644 index 000000000..1d05ce26c --- /dev/null +++ b/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 diff --git a/spec/policies/knowledge_base/answer_policy/scope_spec.rb b/spec/policies/knowledge_base/answer_policy/scope_spec.rb new file mode 100644 index 000000000..f7c2a7c47 --- /dev/null +++ b/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 diff --git a/spec/policies/knowledge_base_policy/scope_spec.rb b/spec/policies/knowledge_base_policy/scope_spec.rb new file mode 100644 index 000000000..1fa0f5421 --- /dev/null +++ b/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 diff --git a/spec/policies/organization_policy_spec.rb b/spec/policies/organization_policy_spec.rb index ee4f395e7..393fb3887 100644 --- a/spec/policies/organization_policy_spec.rb +++ b/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 diff --git a/spec/policies/ticket/article_policy_spec.rb b/spec/policies/ticket/article_policy_spec.rb index 6f88bf5cb..b3d1577a9 100644 --- a/spec/policies/ticket/article_policy_spec.rb +++ b/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 diff --git a/spec/policies/ticket_policy_spec.rb b/spec/policies/ticket_policy_spec.rb index dac9e6ead..6cf6d0e57 100644 --- a/spec/policies/ticket_policy_spec.rb +++ b/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 diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb index b12152814..1212bfbcb 100644 --- a/spec/policies/user_policy_spec.rb +++ b/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