From 506a8575af7deb65bf33d34947083910cfc71514 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 2 Jan 2015 23:34:37 +0100 Subject: [PATCH] Improved replacement of notification tags (removed eval), added more tests. --- app/models/observer/ticket/notification.rb | 6 +- app/models/ticket.rb | 1 + app/models/ticket/article.rb | 1 + lib/notification_factory.rb | 95 ++++++++++++---------- test/unit/notification_factory_test.rb | 49 ++++++++--- 5 files changed, 93 insertions(+), 59 deletions(-) diff --git a/app/models/observer/ticket/notification.rb b/app/models/observer/ticket/notification.rb index 98026c81d..61a5bda7d 100644 --- a/app/models/observer/ticket/notification.rb +++ b/app/models/observer/ticket/notification.rb @@ -33,13 +33,12 @@ class Observer::Ticket::Notification < ActiveRecord::Observer :subject => 'New Ticket (#{ticket.title})', :body => 'Hi #{recipient.firstname}, - a new Ticket (#{ticket.title}) via i18n(#{article.type.name}). + a new Ticket (#{ticket.title}) via i18n(#{article.type.name}) by #{ticket.updated_by.fullname}. Group: #{ticket.group.name} Owner: #{ticket.owner.firstname} #{ticket.owner.lastname} State: i18n(#{ticket.state.name}) - From: #{article.from} #{article.body} @@ -68,12 +67,11 @@ class Observer::Ticket::Notification < ActiveRecord::Observer :subject => 'Updated (#{ticket.title})', :body => 'Hi #{recipient.firstname}, - updated (#{ticket.title}) via i18n(#{article.type.name}). + updated (#{ticket.title}) via i18n(#{article.type.name}) by #{ticket.updated_by.fullname}. Changes: ' + changes + ' - From: #{article.from} #{article.body} diff --git a/app/models/ticket.rb b/app/models/ticket.rb index b9d35ed37..7c7e809db 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -53,6 +53,7 @@ class Ticket < ApplicationModel belongs_to :owner, :class_name => 'User' belongs_to :customer, :class_name => 'User' belongs_to :created_by, :class_name => 'User' + belongs_to :updated_by, :class_name => 'User' belongs_to :create_article_type, :class_name => 'Ticket::Article::Type' belongs_to :create_article_sender, :class_name => 'Ticket::Article::Sender' diff --git a/app/models/ticket/article.rb b/app/models/ticket/article.rb index ee55a33d6..cf22c08e3 100644 --- a/app/models/ticket/article.rb +++ b/app/models/ticket/article.rb @@ -12,6 +12,7 @@ class Ticket::Article < ApplicationModel belongs_to :type, :class_name => 'Ticket::Article::Type' belongs_to :sender, :class_name => 'Ticket::Article::Sender' belongs_to :created_by, :class_name => 'User' + belongs_to :updated_by, :class_name => 'User' after_create :notify_clients_after_create after_update :notify_clients_after_update after_destroy :notify_clients_after_destroy diff --git a/lib/notification_factory.rb b/lib/notification_factory.rb index 38e15c8ea..9530fc08b 100644 --- a/lib/notification_factory.rb +++ b/lib/notification_factory.rb @@ -15,61 +15,70 @@ module NotificationFactory def self.build(data) - data[:string].gsub!( / \#\{ \s* ( .+? ) \s* \} /x ) { |placeholder| + data[:string].gsub!( / \#\{ \s* ( .+? ) \s* \} /xm ) { |placeholder| # store possible callback to work with # and check if it's valid for execution - original_string = $& - callback = $1 - callback_valid = nil + original_string = $& + callback = $1 - # use config params - callback.gsub!( /\A config\.( [\w\.]+ ) \z/x ) { |item| - callback_valid = true - name = $1 - item = "Setting.get('#{name}')" - } - - # use object params - callback.gsub!( /\A ( [\w]+ )( \.[\w\.]+ ) \z/x ) { |item| + object_name = nil + object_method = nil + if callback =~ /\A ( [\w]+ )\.( [\w\.]+ ) \z/x object_name = $1 object_method = $2 - - if data[:objects][object_name.to_sym] - callback_valid = true - item = "data[:objects]['#{object_name}'.to_sym]#{object_method}" - else - item = item - end - } - - # use quoted text - callback.gsub!( /\A ( data\[:objects\]\['article'\.to_sym\] ) \.body \z/x ) { |item| - callback_valid = true - if data[:objects][:article].content_type == 'text/html' - item = item + '.html2text.message_quote.chomp' - else - item = item + '.word_wrap( :line_width => 82 ).message_quote.chomp' - end - } + end # do validaton, ignore some methodes - callback.gsub!( /\.(save|destroy|delete|remove|drop|update|create\(|new|all|where|find)/ix ) { |item| - callback_valid = false - } + if callback =~ /(`|\.(|\s*)(save|destroy|delete|remove|drop|update\(|update_att|create\(|new|all|where|find))/i + placeholder = "#{original_string} (not allowed)" - if callback_valid - # replace value - begin - placeholder = eval callback - rescue Exception => e - Rails.logger.error "Evaluation error caused by callback '#{callback}'" - Rails.logger.error e.inspect + # get value based on object_name and object_method + elsif object_name && object_method + + # use config params + if object_name == 'config' + placeholder = Setting.get(object_method) + + # if object_name dosn't exist + elsif !data[:objects][object_name.to_sym] + placeholder = "\#{#{object_name} / no such object}" + else + value = nil + object_refs = data[:objects][object_name.to_sym] + object_methods = object_method.split('.') + object_methods_s = '' + object_methods.each {|method| + if object_methods_s != '' + object_methods_s += '.' + end + object_methods_s += method + + # if method exists + if !object_refs.respond_to?( method.to_sym ) + value = "\#{#{object_name}.#{object_methods_s} / no such method}" + break + end + object_refs = object_refs.send( method.to_sym ) + + # add body quote + if object_name == 'article' && method == 'body' + if data[:objects][:article].content_type == 'text/html' + object_refs = object_refs.html2text.message_quote.chomp + else + object_refs = object_refs.word_wrap( :line_width => 82 ).message_quote.chomp + end + end + } + if !value + placeholder = object_refs + else + placeholder = value + end end - else - placeholder = original_string end + placeholder } # translate diff --git a/test/unit/notification_factory_test.rb b/test/unit/notification_factory_test.rb index 0d86e789b..632e2ada3 100644 --- a/test/unit/notification_factory_test.rb +++ b/test/unit/notification_factory_test.rb @@ -9,8 +9,8 @@ class NotificationFactoryTest < ActiveSupport::TestCase :customer_id => 2, :state => Ticket::State.lookup( :name => 'new' ), :priority => Ticket::Priority.lookup( :name => '2 normal' ), - :updated_by_id => 1, - :created_by_id => 1, + :updated_by_id => 2, + :created_by_id => 2, ) article_plain = Ticket::Article.create( :ticket_id => ticket.id, @@ -53,6 +53,31 @@ class NotificationFactoryTest < ActiveSupport::TestCase :string => '\'i18n(#{ticket.state.name})\' ticket state', :result => '\'neu\' ticket state', }, + { + :locale => 'de', + :string => 'a #{not_existing_object.test}', + :result => 'a #{not_existing_object / no such object}', + }, + { + :locale => 'de', + :string => 'a #{ticket.level1}', + :result => 'a #{ticket.level1 / no such method}', + }, + { + :locale => 'de', + :string => 'a #{ticket.level1.level2}', + :result => 'a #{ticket.level1 / no such method}', + }, + { + :locale => 'de', + :string => 'a #{ticket.title.level2}', + :result => 'a #{ticket.title.level2 / no such method}', + }, + { + :locale => 'de', + :string => 'by #{ticket.updated_by.fullname}', + :result => 'by Nicole Braun', + }, { :locale => 'de', :string => 'Subject #{article.from}, Group: #{ticket.group.name}', @@ -66,7 +91,7 @@ class NotificationFactoryTest < ActiveSupport::TestCase { :locale => 'de', :string => '\#{puts `ls`}', - :result => '\#{puts `ls`}', + :result => '\#{puts `ls`} (not allowed)', }, ] tests.each { |test| @@ -165,44 +190,44 @@ class NotificationFactoryTest < ActiveSupport::TestCase { :locale => 'de', :string => '\#{puts `ls`}', - :result => '\#{puts `ls`}', + :result => '\#{puts `ls`} (not allowed)', }, { :locale => 'de', :string => 'attack#1 #{article.destroy}', - :result => 'attack#1 #{article.destroy}', + :result => 'attack#1 #{article.destroy} (not allowed)', }, { :locale => 'de', :string => 'attack#2 #{Article.where}', - :result => 'attack#2 #{Article.where}', + :result => 'attack#2 #{Article.where} (not allowed)', }, { :locale => 'de', :string => 'attack#1 #{article. destroy}', :result => 'attack#1 #{article. - destroy}', + destroy} (not allowed)', }, { :locale => 'de', :string => 'attack#1 #{article.find}', - :result => 'attack#1 #{article.find}', + :result => 'attack#1 #{article.find} (not allowed)', }, { :locale => 'de', - :string => 'attack#1 #{article.update}', - :result => 'attack#1 #{article.update}', + :string => 'attack#1 #{article.update(:name => "test")}', + :result => 'attack#1 #{article.update(:name => "test")} (not allowed)', }, { :locale => 'de', :string => 'attack#1 #{article.all}', - :result => 'attack#1 #{article.all}', + :result => 'attack#1 #{article.all} (not allowed)', }, { :locale => 'de', :string => 'attack#1 #{article.delete}', - :result => 'attack#1 #{article.delete}', + :result => 'attack#1 #{article.delete} (not allowed)', }, ] tests.each { |test|