From 880b2fd131203ac84af70dac65c15e64f1d97ce5 Mon Sep 17 00:00:00 2001 From: Martin Gruner Date: Thu, 7 Oct 2021 16:37:57 +0200 Subject: [PATCH] Follow-up: 276c45b2e9 - Improved ticket policy scope handling. --- app/policies/ticket_policy/base_scope.rb | 21 ++-- .../policies/ticket_policy/shared_examples.rb | 97 ++++++++++++++++++- 2 files changed, 107 insertions(+), 11 deletions(-) diff --git a/app/policies/ticket_policy/base_scope.rb b/app/policies/ticket_policy/base_scope.rb index 9f4e9eaa3..4d43608f1 100644 --- a/app/policies/ticket_policy/base_scope.rb +++ b/app/policies/ticket_policy/base_scope.rb @@ -13,7 +13,7 @@ class TicketPolicy < ApplicationPolicy super end - def resolve # rubocop:disable Metrics/AbcSize + def resolve # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity raise NoMethodError, <<~ERR.chomp if instance_of?(TicketPolicy::BaseScope) specify an access type using a subclass of TicketPolicy::BaseScope ERR @@ -26,12 +26,19 @@ class TicketPolicy < ApplicationPolicy bind.push(user.group_ids_access(self.class::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) + if user.permissions?('ticket.customer') + 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 + end + + # The report permission can access all tickets. + if sql.empty? && !user.permissions?('report') + sql.push '0 = 1' # Forbid unlimited access for all other permissions. end scope.where sql.join(' OR '), *bind diff --git a/spec/policies/ticket_policy/shared_examples.rb b/spec/policies/ticket_policy/shared_examples.rb index 03c06d94a..27b878ab6 100644 --- a/spec/policies/ticket_policy/shared_examples.rb +++ b/spec/policies/ticket_policy/shared_examples.rb @@ -12,15 +12,13 @@ RSpec.shared_examples 'for agent user' do |access_type| shared_examples 'shown' do it 'returns its groups’ tickets' do - expect(scope.resolve) - .to match_array(Ticket.where(group: member_groups)) + 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 + expect(scope.resolve).to be_empty end end @@ -47,6 +45,62 @@ RSpec.shared_examples 'for agent user' do |access_type| end end + context 'without any role permission' do + + let(:role_without_rights) { create(:role) } + let(:user) { create(:agent, groups: member_groups, role_ids: [ role_without_rights.id ]) } + + context 'when checking 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 'hidden' + end + + context 'when limited to "read" access' do + before { user.user_groups.each { |ug| ug.update_columns(access: 'read') } } + + include_examples 'hidden' + end + + context 'when limited to "overview" access' do + before { user.user_groups.each { |ug| ug.update_columns(access: 'overview') } } + + include_examples 'hidden' + end + end + + context 'with report permission' do + + let(:report_role) { create(:role).tap { |role| role.permission_grant('report') } } + let(:user) { create(:agent, groups: member_groups, role_ids: [ report_role.id ]) } + + context 'when checking 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') } } + + it 'grants access to all tickets' do + expect(scope.resolve).to match_array(Ticket.all) + end + end + + context 'when limited to "read" access' do + before { user.user_groups.each { |ug| ug.update_columns(access: 'read') } } + + it 'grants access to all tickets' do + expect(scope.resolve).to match_array(Ticket.all) + end + end + + context 'when limited to "overview" access' do + before { user.user_groups.each { |ug| ug.update_columns(access: 'overview') } } + + it 'grants access to all tickets' do + expect(scope.resolve).to match_array(Ticket.all) + end + 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) } @@ -70,6 +124,41 @@ RSpec.shared_examples 'for agent user' do |access_type| include_examples access_type == 'overview' ? 'shown' : 'hidden' end end + + context 'when checking access via customer organization but no customer permissions' do + let(:user) { create(:agent, organization: organization) } + + let(:teammate) { create(:agent, organization: organization) } + + before do + create_list(:ticket, 2, customer: user) + create_list(:ticket, 2, customer: teammate) + end + + context 'with no #organization' do + let(:organization) { nil } + + it 'returns no tickets' do + expect(scope.resolve).to be_empty + end + end + + context 'with a non-shared #organization' do + let(:organization) { create(:organization, shared: false) } + + it 'returns no tickets' do + expect(scope.resolve).to be_empty + end + end + + context 'with a shared #organization (default)' do + let(:organization) { create(:organization, shared: true) } + + it 'returns no tickets' do + expect(scope.resolve).to be_empty + end + end + end end RSpec.shared_examples 'for agent user with predefined but impossible context' do