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

app/controllers/concerns/ticket_stats.rb

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

app/controllers/tickets_controller.rb

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

app/models/ticket.rb

@@ -93,43 +93,6 @@ class Ticket < ApplicationModel
 
 =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
 
   processed_tickets = Ticket.process_pending
@@ -500,9 +463,18 @@ get count of tickets and tickets which match on selector
         return [ticket_count, tickets]
       end
 
-      access_condition = Ticket.access_condition(current_user, access)
-      ticket_count = Ticket.distinct.where(access_condition).where(query, *bind_params).joins(tables).count
-      tickets = Ticket.distinct.where(access_condition).where(query, *bind_params).joins(tables).limit(limit)
+      ticket_count = "TicketPolicy::#{access.camelize}Scope".constantize
+                                                            .new(current_user).resolve
+                                                            .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]
     rescue ActiveRecord::StatementInvalid => e

app/models/ticket/overviews.rb

@@ -92,10 +92,6 @@ returns
     )
     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
     list = []
     overviews.each do |overview|
@@ -129,13 +125,12 @@ returns
         end
       end
 
-      overview_access_condition = access_condition
+      scope = TicketPolicy::OverviewScope
       if overview.condition['ticket.mention_user_ids'].present?
-        overview_access_condition = access_condition_read
+        scope = TicketPolicy::ReadScope
       end
-
-      ticket_result = Ticket.distinct
-                            .where(overview_access_condition)
+      ticket_result = scope.new(user).resolve
+                                                .distinct
                                                 .where(query_condition, *bind_condition)
                                                 .joins(tables)
                                                 .order(Arel.sql("#{order_by} #{direction}"))
@@ -149,7 +144,12 @@ returns
         }
       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 = {
         overview: {
           name:       overview.name,

app/models/ticket/screen_options.rb

@@ -177,16 +177,14 @@ returns
     state_id_list_open   = Ticket::State.by_category(:open).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
-    tickets_open = Ticket.where(
+    tickets_open = TicketPolicy::ReadScope.new(data[:current_user]).resolve
+                                          .where(
                                             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 = {}
     ticket_ids_open = []
     tickets_open.each do |ticket|
@@ -194,12 +192,13 @@ returns
       assets = ticket.assets(assets)
     end
 
-    tickets_closed = Ticket.where(
+    tickets_closed = TicketPolicy::ReadScope.new(data[:current_user]).resolve
+                                            .where(
                                               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 = []
     tickets_closed.each do |ticket|
       ticket_ids_closed.push ticket.id

app/models/ticket/search.rb

@@ -190,9 +190,6 @@ returns
         return tickets
       end
 
-      # fallback do sql query
-      access_condition = Ticket.access_condition(current_user, 'read')
-
       # do query
       # - stip out * we already search for *query* -
 
@@ -200,8 +197,8 @@ returns
       order_sql        = sql_helper.get_order(sort_by, order_by, 'tickets.updated_at DESC')
       if query
         query.delete! '*'
-        tickets_all = Ticket.select("DISTINCT(tickets.id), #{order_select_sql}")
-                            .where(access_condition)
+        tickets_all = TicketPolicy::ReadScope.new(current_user).resolve
+                                             .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}%")
                                              .joins(:articles)
                                              .order(Arel.sql(order_sql))
@@ -209,9 +206,9 @@ returns
                                              .limit(limit)
       else
         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
+                                             .select("DISTINCT(tickets.id), #{order_select_sql}")
                                              .joins(tables)
-                            .where(access_condition)
                                              .where(query_condition, *bind_condition)
                                              .order(Arel.sql(order_sql))
                                              .offset(offset)

app/policies/application_policy.rb

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

app/policies/application_policy/scope.rb

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

app/policies/organization_policy.rb

@@ -14,17 +14,4 @@ class OrganizationPolicy < ApplicationPolicy
 
     false
   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

app/policies/organization_policy/scope.rb

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

app/policies/ticket_policy/base_scope.rb

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

app/policies/ticket_policy/full_scope.rb

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

app/policies/ticket_policy/overview_scope.rb

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

app/policies/ticket_policy/read_scope.rb

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

app/policies/user_policy.rb

@@ -37,15 +37,4 @@ class UserPolicy < ApplicationPolicy
 
     record.organization_id == user.organization_id
   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

app/policies/user_policy/scope.rb

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

spec/policies/ticket_policy/base_scope_spec.rb

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

spec/policies/ticket_policy/full_scope_spec.rb

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

spec/policies/ticket_policy/overview_scope_spec.rb

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

spec/policies/ticket_policy/read_scope_spec.rb

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

spec/policies/ticket_policy/shared_examples.rb

@@ -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 customer’s 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 customer’s 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 customer’s or organization’s tickets' do
+      expect(scope.resolve).to match_array(user_tickets | teammate_tickets)
+    end
+  end
+end