Refactoring: Drop Ticket.access_condition in favor of using Pundit scopes.

This commit is contained in:
Ryan Lue 2021-07-20 17:35:02 +00:00 committed by Martin Gruner
parent 9f7bec8942
commit 276c45b2e9
21 changed files with 428 additions and 160 deletions

View file

@ -16,7 +16,7 @@ module TicketStats
assets_of_tickets(tickets, assets) assets_of_tickets(tickets, assets)
end end
def ticket_stats_last_year(condition, access_condition) def ticket_stats_last_year(condition)
volume_by_year = [] volume_by_year = []
now = Time.zone.now now = Time.zone.now
@ -26,16 +26,16 @@ module TicketStats
date_end = "#{date_to_check.year}-#{date_to_check.month}-#{date_to_check.end_of_month.day} 00:00:00" date_end = "#{date_to_check.year}-#{date_to_check.month}-#{date_to_check.end_of_month.day} 00:00:00"
# created # created
created = Ticket.where('created_at > ? AND created_at < ?', date_start, date_end) created = TicketPolicy::ReadScope.new(current_user).resolve
.where(access_condition) .where('created_at > ? AND created_at < ?', date_start, date_end)
.where(condition) .where(condition)
.count .count
# closed # closed
closed = Ticket.where('close_at > ? AND close_at < ?', date_start, date_end) closed = TicketPolicy::ReadScope.new(current_user).resolve
.where(access_condition) .where('close_at > ? AND close_at < ?', date_start, date_end)
.where(condition) .where(condition)
.count .count
data = { data = {
month: date_to_check.month, month: date_to_check.month,

View file

@ -23,8 +23,10 @@ class TicketsController < ApplicationController
per_page = 100 per_page = 100
end end
access_condition = Ticket.access_condition(current_user, 'read') tickets = TicketPolicy::ReadScope.new(current_user).resolve
tickets = Ticket.where(access_condition).order(id: :asc).offset(offset).limit(per_page) .order(id: :asc)
.offset(offset)
.limit(per_page)
if response_expand? if response_expand?
list = [] list = []
@ -317,36 +319,29 @@ class TicketsController < ApplicationController
ticket = Ticket.find(params[:ticket_id]) ticket = Ticket.find(params[:ticket_id])
assets = ticket.assets({}) assets = ticket.assets({})
# open tickets by customer tickets = TicketPolicy::ReadScope.new(current_user).resolve
access_condition = Ticket.access_condition(current_user, 'read') .where(
customer_id: ticket.customer_id,
ticket_lists = Ticket state_id: Ticket::State.by_category(:open).select(:id),
.where( )
customer_id: ticket.customer_id, .where.not(id: [ ticket.id ])
state_id: Ticket::State.by_category(:open).pluck(:id), # rubocop:disable Rails/PluckInWhere .order(created_at: :desc)
) .limit(6)
.where(access_condition)
.where.not(id: [ ticket.id ])
.order(created_at: :desc)
.limit(6)
# if we do not have open related tickets, search for any tickets # if we do not have open related tickets, search for any tickets
if ticket_lists.blank? tickets ||= TicketPolicy::ReadScope.new(current_user).resolve
ticket_lists = Ticket .where(
.where( customer_id: ticket.customer_id,
customer_id: ticket.customer_id, ).where.not(
).where.not( state_id: Ticket::State.by_category(:merged).pluck(:id),
state_id: Ticket::State.by_category(:merged).pluck(:id), )
) .where.not(id: [ ticket.id ])
.where(access_condition) .order(created_at: :desc)
.where.not(id: [ ticket.id ]) .limit(6)
.order(created_at: :desc)
.limit(6)
end
# get related assets # get related assets
ticket_ids_by_customer = [] ticket_ids_by_customer = []
ticket_lists.each do |ticket_list| tickets.each do |ticket_list|
ticket_ids_by_customer.push ticket_list.id ticket_ids_by_customer.push ticket_list.id
assets = ticket_list.assets(assets) assets = ticket_list.assets(assets)
end end
@ -534,7 +529,6 @@ class TicketsController < ApplicationController
# lookup open user tickets # lookup open user tickets
limit = 100 limit = 100
assets = {} assets = {}
access_condition = Ticket.access_condition(current_user, 'read')
user_tickets = {} user_tickets = {}
if params[:user_id] if params[:user_id]
@ -573,7 +567,7 @@ class TicketsController < ApplicationController
condition = { condition = {
'tickets.customer_id' => user.id, 'tickets.customer_id' => user.id,
} }
user_tickets[:volume_by_year] = ticket_stats_last_year(condition, access_condition) user_tickets[:volume_by_year] = ticket_stats_last_year(condition)
end end
@ -615,7 +609,7 @@ class TicketsController < ApplicationController
condition = { condition = {
'tickets.organization_id' => organization.id, 'tickets.organization_id' => organization.id,
} }
org_tickets[:volume_by_year] = ticket_stats_last_year(condition, access_condition) org_tickets[:volume_by_year] = ticket_stats_last_year(condition)
end end
# return result # return result

View file

@ -93,43 +93,6 @@ class Ticket < ApplicationModel
=begin =begin
get user access conditions
conditions = Ticket.access_condition( User.find(1) , 'full')
returns
result = [user1, user2, ...]
=end
def self.access_condition(user, access)
sql = []
bind = []
if user.permissions?('ticket.agent')
sql.push('group_id IN (?)')
bind.push(user.group_ids_access(access))
end
if user.permissions?('ticket.customer')
if !user.organization || (!user.organization.shared || user.organization.shared == false)
sql.push('tickets.customer_id = ?')
bind.push(user.id)
else
sql.push('(tickets.customer_id = ? OR tickets.organization_id = ?)')
bind.push(user.id)
bind.push(user.organization.id)
end
end
return if sql.blank?
[ sql.join(' OR ') ].concat(bind)
end
=begin
processes tickets which have reached their pending time and sets next state_id processes tickets which have reached their pending time and sets next state_id
processed_tickets = Ticket.process_pending processed_tickets = Ticket.process_pending
@ -500,9 +463,18 @@ get count of tickets and tickets which match on selector
return [ticket_count, tickets] return [ticket_count, tickets]
end end
access_condition = Ticket.access_condition(current_user, access) ticket_count = "TicketPolicy::#{access.camelize}Scope".constantize
ticket_count = Ticket.distinct.where(access_condition).where(query, *bind_params).joins(tables).count .new(current_user).resolve
tickets = Ticket.distinct.where(access_condition).where(query, *bind_params).joins(tables).limit(limit) .distinct
.where(query, *bind_params)
.joins(tables)
.count
tickets = "TicketPolicy::#{access.camelize}Scope".constantize
.new(current_user).resolve
.distinct
.where(query, *bind_params)
.joins(tables)
.limit(limit)
return [ticket_count, tickets] return [ticket_count, tickets]
rescue ActiveRecord::StatementInvalid => e rescue ActiveRecord::StatementInvalid => e

View file

@ -92,10 +92,6 @@ returns
) )
return [] if overviews.blank? return [] if overviews.blank?
# get only tickets with permissions
access_condition = Ticket.access_condition(user, 'overview')
access_condition_read = Ticket.access_condition(user, 'read')
ticket_attributes = Ticket.new.attributes ticket_attributes = Ticket.new.attributes
list = [] list = []
overviews.each do |overview| overviews.each do |overview|
@ -129,18 +125,17 @@ returns
end end
end end
overview_access_condition = access_condition scope = TicketPolicy::OverviewScope
if overview.condition['ticket.mention_user_ids'].present? if overview.condition['ticket.mention_user_ids'].present?
overview_access_condition = access_condition_read scope = TicketPolicy::ReadScope
end end
ticket_result = scope.new(user).resolve
ticket_result = Ticket.distinct .distinct
.where(overview_access_condition) .where(query_condition, *bind_condition)
.where(query_condition, *bind_condition) .joins(tables)
.joins(tables) .order(Arel.sql("#{order_by} #{direction}"))
.order(Arel.sql("#{order_by} #{direction}")) .limit(2000)
.limit(2000) .pluck(:id, :updated_at, Arel.sql(order_by))
.pluck(:id, :updated_at, Arel.sql(order_by))
tickets = ticket_result.map do |ticket| tickets = ticket_result.map do |ticket|
{ {
@ -149,7 +144,12 @@ returns
} }
end end
count = Ticket.distinct.where(overview_access_condition).where(query_condition, *bind_condition).joins(tables).count() count = TicketPolicy::OverviewScope.new(user).resolve
.distinct
.where(query_condition, *bind_condition)
.joins(tables)
.count()
item = { item = {
overview: { overview: {
name: overview.name, name: overview.name,

View file

@ -177,16 +177,14 @@ returns
state_id_list_open = Ticket::State.by_category(:open).pluck(:id) state_id_list_open = Ticket::State.by_category(:open).pluck(:id)
state_id_list_closed = Ticket::State.by_category(:closed).pluck(:id) state_id_list_closed = Ticket::State.by_category(:closed).pluck(:id)
# open tickets by customer
access_condition = Ticket.access_condition(data[:current_user], 'read')
# get tickets # get tickets
tickets_open = Ticket.where( tickets_open = TicketPolicy::ReadScope.new(data[:current_user]).resolve
customer_id: data[:customer_id], .where(
state_id: state_id_list_open customer_id: data[:customer_id],
) state_id: state_id_list_open
.where(access_condition) )
.limit(data[:limit] || 15).order(created_at: :desc) .limit(data[:limit] || 15)
.order(created_at: :desc)
assets = {} assets = {}
ticket_ids_open = [] ticket_ids_open = []
tickets_open.each do |ticket| tickets_open.each do |ticket|
@ -194,12 +192,13 @@ returns
assets = ticket.assets(assets) assets = ticket.assets(assets)
end end
tickets_closed = Ticket.where( tickets_closed = TicketPolicy::ReadScope.new(data[:current_user]).resolve
customer_id: data[:customer_id], .where(
state_id: state_id_list_closed customer_id: data[:customer_id],
) state_id: state_id_list_closed
.where(access_condition) )
.limit(data[:limit] || 15).order(created_at: :desc) .limit(data[:limit] || 15)
.order(created_at: :desc)
ticket_ids_closed = [] ticket_ids_closed = []
tickets_closed.each do |ticket| tickets_closed.each do |ticket|
ticket_ids_closed.push ticket.id ticket_ids_closed.push ticket.id

View file

@ -190,9 +190,6 @@ returns
return tickets return tickets
end end
# fallback do sql query
access_condition = Ticket.access_condition(current_user, 'read')
# do query # do query
# - stip out * we already search for *query* - # - stip out * we already search for *query* -
@ -200,22 +197,22 @@ returns
order_sql = sql_helper.get_order(sort_by, order_by, 'tickets.updated_at DESC') order_sql = sql_helper.get_order(sort_by, order_by, 'tickets.updated_at DESC')
if query if query
query.delete! '*' query.delete! '*'
tickets_all = Ticket.select("DISTINCT(tickets.id), #{order_select_sql}") tickets_all = TicketPolicy::ReadScope.new(current_user).resolve
.where(access_condition) .select("DISTINCT(tickets.id), #{order_select_sql}")
.where('(tickets.title LIKE ? OR tickets.number LIKE ? OR ticket_articles.body LIKE ? OR ticket_articles.from LIKE ? OR ticket_articles.to LIKE ? OR ticket_articles.subject LIKE ?)', "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%") .where('(tickets.title LIKE ? OR tickets.number LIKE ? OR ticket_articles.body LIKE ? OR ticket_articles.from LIKE ? OR ticket_articles.to LIKE ? OR ticket_articles.subject LIKE ?)', "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%")
.joins(:articles) .joins(:articles)
.order(Arel.sql(order_sql)) .order(Arel.sql(order_sql))
.offset(offset) .offset(offset)
.limit(limit) .limit(limit)
else else
query_condition, bind_condition, tables = selector2sql(condition) query_condition, bind_condition, tables = selector2sql(condition)
tickets_all = Ticket.select("DISTINCT(tickets.id), #{order_select_sql}") tickets_all = TicketPolicy::ReadScope.new(current_user).resolve
.joins(tables) .select("DISTINCT(tickets.id), #{order_select_sql}")
.where(access_condition) .joins(tables)
.where(query_condition, *bind_condition) .where(query_condition, *bind_condition)
.order(Arel.sql(order_sql)) .order(Arel.sql(order_sql))
.offset(offset) .offset(offset)
.limit(limit) .limit(limit)
end end
# build result list # build result list

View file

@ -8,14 +8,4 @@ class ApplicationPolicy
def initialize_context(record) def initialize_context(record)
@record = record @record = record
end end
class Scope
include PunditPolicy
attr_reader :scope
def initialize_context(scope)
@scope = scope
end
end
end end

View file

@ -0,0 +1,13 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class ApplicationPolicy
class Scope
include PunditPolicy
attr_reader :scope
def initialize_context(scope)
@scope = scope
end
end
end

View file

@ -14,17 +14,4 @@ class OrganizationPolicy < ApplicationPolicy
false false
end end
class Scope < ApplicationPolicy::Scope
def resolve
if user.permissions?(['ticket.agent', 'admin.organization'])
scope.all
elsif user.organization_id
scope.where(id: user.organization_id)
else
scope.none
end
end
end
end end

View file

@ -0,0 +1,16 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class OrganizationPolicy < ApplicationPolicy
class Scope < ApplicationPolicy::Scope
def resolve
if user.permissions?(['ticket.agent', 'admin.organization'])
scope.all
elsif user.organization_id
scope.where(id: user.organization_id)
else
scope.none
end
end
end
end

View file

@ -0,0 +1,48 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
# Abstract base class for various "types" of ticket access.
#
# Do NOT instantiate directly; instead,
# choose the appropriate subclass from below
# (see commit message for details).
class TicketPolicy < ApplicationPolicy
class BaseScope < ApplicationPolicy::Scope
# overwrite PunditPolicy#initialize to make `context` optional and use Ticket as default
def initialize(user, context = Ticket)
super
end
def resolve # rubocop:disable Metrics/AbcSize
raise NoMethodError, <<~ERR.chomp if instance_of?(TicketPolicy::BaseScope)
specify an access type using a subclass of TicketPolicy::BaseScope
ERR
sql = []
bind = []
if user.permissions?('ticket.agent')
access_type = self.class.name.demodulize.slice(%r{.*(?=Scope)}).underscore
sql.push('group_id IN (?)')
bind.push(user.group_ids_access(access_type))
end
if user.organization&.shared
sql.push('(tickets.customer_id = ? OR tickets.organization_id = ?)')
bind.push(user.id, user.organization.id)
else
sql.push('tickets.customer_id = ?')
bind.push(user.id)
end
scope.where sql.join(' OR '), *bind
end
# #resolve is UNDEFINED BEHAVIOR for the abstract base class (but not its subclasses)
def respond_to?(*args)
return false if args.first.to_s == 'resolve' && instance_of?(TicketPolicy::BaseScope)
super(*args)
end
end
end

View file

@ -0,0 +1,6 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class TicketPolicy < ApplicationPolicy
class FullScope < BaseScope
end
end

View file

@ -0,0 +1,6 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class TicketPolicy < ApplicationPolicy
class OverviewScope < BaseScope
end
end

View file

@ -0,0 +1,6 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class TicketPolicy < ApplicationPolicy
class ReadScope < BaseScope
end
end

View file

@ -37,15 +37,4 @@ class UserPolicy < ApplicationPolicy
record.organization_id == user.organization_id record.organization_id == user.organization_id
end end
class Scope < ApplicationPolicy::Scope
def resolve
if user.permissions?(['ticket.agent', 'admin.user'])
scope.all
else
scope.where(id: user.id)
end
end
end
end end

View file

@ -0,0 +1,14 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class UserPolicy < ApplicationPolicy
class Scope < ApplicationPolicy::Scope
def resolve
if user.permissions?(['ticket.agent', 'admin.user'])
scope.all
else
scope.where(id: user.id)
end
end
end
end

View file

@ -0,0 +1,25 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
require 'rails_helper'
RSpec.describe TicketPolicy::BaseScope do
subject(:scope) { described_class.new(user) }
describe '#resolve' do
context 'when querying for agent user' do
let(:user) { create(:agent) }
it 'raises NoMethodError (undefined on abstract base class)' do
expect { scope.resolve }.to raise_error(NoMethodError)
end
end
context 'when querying for customer user' do
let(:user) { create(:customer) }
it 'raises NoMethodError (undefined on abstract base class)' do
expect { scope.resolve }.to raise_error(NoMethodError)
end
end
end
end

View file

@ -0,0 +1,28 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
require 'rails_helper'
require 'policies/ticket_policy/shared_examples'
RSpec.describe TicketPolicy::FullScope do
context 'with default scope' do
subject(:scope) { described_class.new(user) }
describe '#resolve' do
context 'when querying for agent user' do
include_examples 'for agent user', 'full'
end
context 'when querying for customer user' do
include_examples 'for customer user'
end
end
end
context 'with predefined, impossible scope' do
subject(:scope) { described_class.new(user, Ticket.where(id: -1)) }
describe '#resolve' do
include_examples 'for agent user with predefined but impossible context'
end
end
end

View file

@ -0,0 +1,28 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
require 'rails_helper'
require 'policies/ticket_policy/shared_examples'
RSpec.describe TicketPolicy::OverviewScope do
context 'with default scope' do
subject(:scope) { described_class.new(user) }
describe '#resolve' do
context 'when querying for agent user' do
include_examples 'for agent user', 'overview'
end
context 'when querying for customer user' do
include_examples 'for customer user'
end
end
end
context 'with predefined, impossible scope' do
subject(:scope) { described_class.new(user, Ticket.where(id: -1)) }
describe '#resolve' do
include_examples 'for agent user with predefined but impossible context'
end
end
end

View file

@ -0,0 +1,29 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
require 'rails_helper'
require 'policies/ticket_policy/shared_examples'
RSpec.describe TicketPolicy::ReadScope do
context 'with default scope' do
subject(:scope) { described_class.new(user) }
describe '#resolve' do
context 'when querying for agent user' do
include_examples 'for agent user', 'read'
end
context 'when querying for customer user' do
include_examples 'for customer user'
end
end
end
context 'with predefined, impossible scope' do
subject(:scope) { described_class.new(user, Ticket.where(id: -1)) }
describe '#resolve' do
include_examples 'for agent user with predefined but impossible context'
end
end
end

View file

@ -0,0 +1,121 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
RSpec.shared_examples 'for agent user' do |access_type|
let(:member_groups) { create_list(:group, 2) }
let(:nonmember_group) { create(:group) }
before do
create(:ticket, group: member_groups.first)
create(:ticket, group: member_groups.second)
create(:ticket, group: nonmember_group)
end
shared_examples 'shown' do
it 'returns its groups tickets' do
expect(scope.resolve)
.to match_array(Ticket.where(group: member_groups))
end
end
shared_examples 'hidden' do
it 'does not return its groups tickets' do
expect(scope.resolve)
.to be_empty
end
end
context 'with direct access via User#groups' do
let(:user) { create(:agent, groups: member_groups) }
context 'when checkin for "full" access' do
# this is already true by default, but it doesn't hurt to be explicit
before { user.user_groups.each { |ug| ug.update_columns(access: 'full') } }
include_examples 'shown'
end
context 'when limited to "read" access' do
before { user.user_groups.each { |ug| ug.update_columns(access: 'read') } }
include_examples access_type == 'read' ? 'shown' : 'hidden'
end
context 'when limited to "overview" access' do
before { user.user_groups.each { |ug| ug.update_columns(access: 'overview') } }
include_examples access_type == 'overview' ? 'shown' : 'hidden'
end
end
context 'with indirect access via Role#groups' do
let(:user) { create(:agent).tap { |u| u.roles << role } }
let(:role) { create(:role, groups: member_groups) }
context 'when checkin for "full" access' do
# this is already true by default, but it doesn't hurt to be explicit
before { role.role_groups.each { |rg| rg.update_columns(access: 'full') } }
include_examples 'shown'
end
context 'when limited to "read" access' do
before { role.role_groups.each { |rg| rg.update_columns(access: 'read') } }
include_examples access_type == 'read' ? 'shown' : 'hidden'
end
context 'when limited to "overview" access' do
before { role.role_groups.each { |rg| rg.update_columns(access: 'overview') } }
include_examples access_type == 'overview' ? 'shown' : 'hidden'
end
end
end
RSpec.shared_examples 'for agent user with predefined but impossible context' do
let(:member_groups) { create_list(:group, 2) }
let(:nonmember_group) { create(:group) }
let(:user) { create(:agent, groups: member_groups) }
before do
create(:ticket, group: member_groups.first)
create(:ticket, group: member_groups.second)
create(:ticket, group: nonmember_group)
end
it 'does not find tickets because of restrictive predefined scope' do
expect(scope.resolve).to match_array(Ticket.where(id: -1))
end
end
RSpec.shared_examples 'for customer user' do
let(:user) { create(:customer, organization: organization) }
let!(:user_tickets) { create_list(:ticket, 2, customer: user) }
let(:teammate) { create(:customer, organization: organization) }
let!(:teammate_tickets) { create_list(:ticket, 2, customer: teammate) }
context 'with no #organization' do
let(:organization) { nil }
it 'returns only the customers tickets' do
expect(scope.resolve).to match_array(user_tickets)
end
end
context 'with a non-shared #organization' do
let(:organization) { create(:organization, shared: false) }
it 'returns only the customers tickets' do
expect(scope.resolve).to match_array(user_tickets)
end
end
context 'with a shared #organization (default)' do
let(:organization) { create(:organization, shared: true) }
it 'returns only the customers or organizations tickets' do
expect(scope.resolve).to match_array(user_tickets | teammate_tickets)
end
end
end