From 92f7d9307c71145a4e7bcd8556d3ea13a6582e3b Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Tue, 24 Mar 2020 16:40:53 +0100 Subject: [PATCH] Fixes #2982 - Trigger with current_user.id as precondition is executed correctly --- app/models/ticket.rb | 34 +++++++++++++---- spec/models/trigger_spec.rb | 36 ++++++++++++++++++ test/unit/ticket_selector_test.rb | 63 +++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 8 deletions(-) diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 1dd3ba68a..ea9f48881 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -423,8 +423,22 @@ returns get count of tickets and tickets which match on selector +@param [Hash] selectors hash with conditions +@oparam [Hash] options + +@option options [String] :access can be 'full', 'read', 'create' or 'ignore' (ignore means a selector over all tickets), defaults to 'full' +@option options [Integer] :limit of tickets to return +@option options [User] :user is a current user +@option options [Integer] :execution_time is a current user + +@return [Integer, []] + +@example ticket_count, tickets = Ticket.selectors(params[:condition], limit: limit, current_user: current_user, access: 'full') + ticket_count # count of found tickets + tickets # tickets + =end def self.selectors(selectors, options) @@ -438,7 +452,7 @@ get count of tickets and tickets which match on selector ActiveRecord::Base.transaction(requires_new: true) do - if !current_user + if !current_user || access == 'ignore' ticket_count = Ticket.distinct.where(query, *bind_params).joins(tables).count tickets = Ticket.distinct.where(query, *bind_params).joins(tables).limit(limit) return [ticket_count, tickets] @@ -1118,18 +1132,22 @@ perform active triggers on ticket } end - # verify is condition is matching - ticket_count, tickets = Ticket.selectors(condition, limit: 1, execution_time: true) - - next if ticket_count.blank? - next if ticket_count.zero? - next if tickets.first.id != ticket.id - user_id = ticket.updated_by_id if article user_id = article.updated_by_id end + user = if user_id != 1 + User.lookup(id: user_id) + end + + # verify is condition is matching + ticket_count, tickets = Ticket.selectors(condition, limit: 1, execution_time: true, current_user: user, access: 'ignore') + + next if ticket_count.blank? + next if ticket_count.zero? + next if tickets.first.id != ticket.id + if recursive == false && local_options[:loop_count] > 1 message = "Do not execute recursive triggers per default until Zammad 3.0. With Zammad 3.0 and higher the following trigger is executed '#{trigger.name}' on Ticket:#{ticket.id}. Please review your current triggers and change them if needed." logger.info { message } diff --git a/spec/models/trigger_spec.rb b/spec/models/trigger_spec.rb index aa0f482b4..54516a81c 100644 --- a/spec/models/trigger_spec.rb +++ b/spec/models/trigger_spec.rb @@ -294,4 +294,40 @@ RSpec.describe Trigger, type: :model do end end end + + context 'with pre condition current_user.id' do + let(:perform) do + { 'ticket.title'=>{ 'value'=>'triggered' } } + end + + let(:user) do + user = create(:agent_user) + user.roles.first.groups << group + user + end + + let(:group) { Group.first } + + let(:ticket) do + create(:ticket, + title: 'Test Ticket', group: group, + owner_id: user.id, created_by_id: user.id, updated_by_id: user.id) + end + + shared_examples 'successful trigger' do |attribute:| + let(:attribute) { attribute } + + let(:condition) do + { attribute => { operator: 'is', pre_condition: 'current_user.id', value: '', value_completion: '' } } + end + + it "for #{attribute}" do + ticket && trigger + expect { Observer::Transaction.commit }.to change { ticket.reload.title }.to('triggered') + end + end + + it_behaves_like 'successful trigger', attribute: 'ticket.updated_by_id' + it_behaves_like 'successful trigger', attribute: 'ticket.owner_id' + end end diff --git a/test/unit/ticket_selector_test.rb b/test/unit/ticket_selector_test.rb index 6baec5784..f79577a10 100644 --- a/test/unit/ticket_selector_test.rb +++ b/test/unit/ticket_selector_test.rb @@ -1199,4 +1199,67 @@ class TicketSelectorTest < ActiveSupport::TestCase end + test 'access: "ignore"' do + Ticket.destroy_all + + Ticket.create!( + title: 'some title1', + group: @group, + customer_id: @customer1.id, + owner_id: @agent1.id, + state: Ticket::State.lookup(name: 'new'), + priority: Ticket::Priority.lookup(name: '2 normal'), + created_at: '2015-02-05 16:37:00', + updated_by_id: 1, + created_by_id: 1, + ) + + Ticket.create!( + title: 'some title2', + group: @group, + customer_id: @customer1.id, + owner_id: @agent1.id, + state: Ticket::State.lookup(name: 'new'), + priority: Ticket::Priority.lookup(name: '2 normal'), + created_at: '2015-02-05 16:37:00', + updated_by_id: @agent2.id, + created_by_id: 1, + ) + + condition = { + 'ticket.title' => { + operator: 'contains', + value: 'some', + }, + } + + # visible by owner + ticket_count, _tickets = Ticket.selectors(condition, limit: 10, current_user: @agent1) + assert_equal(2, ticket_count) + + # not visible by another agent + ticket_count, _tickets = Ticket.selectors(condition, limit: 10, current_user: @agent2) + assert_equal(0, ticket_count) + + # visible by another user when access: "ignore". For example, when tickets are performed after action of another user + ticket_count, _tickets = Ticket.selectors(condition, limit: 10, current_user: @agent2, access: 'ignore') + assert_equal(2, ticket_count) + + condition2 = { + 'ticket.updated_by_id' => { + operator: 'is', + pre_condition: 'current_user.id', + value: '', + value_completion: '' + } + } + + # not visible by another agent even if matches current user precondition + ticket_count, _tickets = Ticket.selectors(condition2, limit: 10, current_user: @agent2) + assert_equal(0, ticket_count) + + # visible by another user when access: "ignore" if matches current user precondition + ticket_count, _tickets = Ticket.selectors(condition2, limit: 10, current_user: @agent2, access: 'ignore') + assert_equal(1, ticket_count) + end end