From eceb9889b1fac44a403c95e8de58ceee0be22a93 Mon Sep 17 00:00:00 2001
From: Martin Edenhofer <me@edenhofer.de>
Date: Thu, 8 Sep 2016 21:18:26 +0200
Subject: [PATCH] Added database locking to prevent race conditions.

---
 app/controllers/application_controller.rb | 11 ++--
 app/controllers/tickets_controller.rb     | 44 ++++++-------
 app/controllers/users_controller.rb       | 52 ++++++++-------
 app/models/channel/email_parser.rb        | 77 ++++++++++++-----------
 lib/sessions.rb                           | 25 +++++---
 test/browser_test_helper.rb               | 60 ++++++++++++++----
 6 files changed, 160 insertions(+), 109 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index bea253172..af5720033 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -535,11 +535,14 @@ class ApplicationController < ActionController::Base
     clean_params = object.param_association_lookup(params)
     clean_params = object.param_cleanup(clean_params, true)
 
-    # save object
-    generic_object.update_attributes!(clean_params)
+    generic_object.with_lock do
 
-    # set relations
-    generic_object.param_set_associations(params)
+      # set attributes
+      generic_object.update_attributes!(clean_params)
+
+      # set relations
+      generic_object.param_set_associations(params)
+    end
 
     if params[:expand]
       render json: generic_object.attributes_with_relation_names, status: :ok
diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb
index 79ab1c0e1..56a7a0a50 100644
--- a/app/controllers/tickets_controller.rb
+++ b/app/controllers/tickets_controller.rb
@@ -114,25 +114,26 @@ class TicketsController < ApplicationController
 
     # create ticket
     ticket.save!
+    ticket.with_lock do
 
-    # create tags if given
-    if params[:tags] && !params[:tags].empty?
-      tags = params[:tags].split(/,/)
-      tags.each { |tag|
-        Tag.tag_add(
-          object: 'Ticket',
-          o_id: ticket.id,
-          item: tag,
-          created_by_id: current_user.id,
-        )
-      }
+      # create tags if given
+      if params[:tags] && !params[:tags].empty?
+        tags = params[:tags].split(/,/)
+        tags.each { |tag|
+          Tag.tag_add(
+            object: 'Ticket',
+            o_id: ticket.id,
+            item: tag,
+            created_by_id: current_user.id,
+          )
+        }
+      end
+
+      # create article if given
+      if params[:article]
+        article_create(ticket, params[:article])
+      end
     end
-
-    # create article if given
-    if params[:article]
-      article_create(ticket, params[:article])
-    end
-
     # create links (e. g. in case of ticket split)
     # links: {
     #   Ticket: {
@@ -191,10 +192,11 @@ class TicketsController < ApplicationController
       }
     end
 
-    ticket.update_attributes!(clean_params)
-
-    if params[:article]
-      article_create(ticket, params[:article])
+    ticket.with_lock do
+      ticket.update_attributes!(clean_params)
+      if params[:article]
+        article_create(ticket, params[:article])
+      end
     end
 
     if params[:expand]
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 724f7fdc7..5ad3e529e 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -254,29 +254,31 @@ class UsersController < ApplicationController
 
     # permission check
     permission_check_by_permission(params)
-    user.update_attributes(clean_params)
+    user.with_lock do
+      user.update_attributes(clean_params)
 
-    # only allow Admin's
-    if current_user.permissions?('admin.user') && (params[:role_ids] || params[:roles])
-      user.role_ids = params[:role_ids]
-      user.param_set_associations({ role_ids: params[:role_ids], roles: params[:roles] })
-    end
+      # only allow Admin's
+      if current_user.permissions?('admin.user') && (params[:role_ids] || params[:roles])
+        user.role_ids = params[:role_ids]
+        user.param_set_associations({ role_ids: params[:role_ids], roles: params[:roles] })
+      end
 
-    # only allow Admin's
-    if current_user.permissions?('admin.user') && (params[:group_ids] || params[:groups])
-      user.group_ids = params[:group_ids]
-      user.param_set_associations({ group_ids: params[:group_ids], groups: params[:groups] })
-    end
+      # only allow Admin's
+      if current_user.permissions?('admin.user') && (params[:group_ids] || params[:groups])
+        user.group_ids = params[:group_ids]
+        user.param_set_associations({ group_ids: params[:group_ids], groups: params[:groups] })
+      end
 
-    # only allow Admin's and Agent's
-    if current_user.permissions?(['admin.user', 'ticket.agent']) && (params[:organization_ids] || params[:organizations])
-      user.param_set_associations({ organization_ids: params[:organization_ids], organizations: params[:organizations] })
-    end
+      # only allow Admin's and Agent's
+      if current_user.permissions?(['admin.user', 'ticket.agent']) && (params[:organization_ids] || params[:organizations])
+        user.param_set_associations({ organization_ids: params[:organization_ids], organizations: params[:organizations] })
+      end
 
-    if params[:expand]
-      user = User.find(user.id).attributes_with_relation_names
-      render json: user, status: :ok
-      return
+      if params[:expand]
+        user = User.find(user.id).attributes_with_relation_names
+        render json: user, status: :ok
+        return
+      end
     end
 
     # get new data
@@ -704,7 +706,7 @@ curl http://localhost/api/v1/users/password_change.json -v -u #{login}:#{passwor
       render json: { message: 'failed', notice: ['Current password needed!'] }, status: :ok
       return
     end
-    user = User.authenticate( current_user.login, params[:password_old] )
+    user = User.authenticate(current_user.login, params[:password_old])
     if !user
       render json: { message: 'failed', notice: ['Current password is wrong!'] }, status: :ok
       return
@@ -763,10 +765,12 @@ curl http://localhost/api/v1/users/preferences.json -v -u #{login}:#{password} -
 
     if params[:user]
       user = User.find(current_user.id)
-      params[:user].each { |key, value|
-        user.preferences[key.to_sym] = value
-      }
-      user.save
+      user.with_lock do
+        params[:user].each { |key, value|
+          user.preferences[key.to_sym] = value
+        }
+        user.save
+      end
     end
     render json: { message: 'ok' }, status: :ok
   end
diff --git a/app/models/channel/email_parser.rb b/app/models/channel/email_parser.rb
index 2db520a5c..0613429c7 100644
--- a/app/models/channel/email_parser.rb
+++ b/app/models/channel/email_parser.rb
@@ -442,10 +442,10 @@ retrns
 
       # get ticket# based on email headers
       if mail[ 'x-zammad-ticket-id'.to_sym ]
-        ticket = Ticket.find_by( id: mail[ 'x-zammad-ticket-id'.to_sym ] )
+        ticket = Ticket.find_by(id: mail[ 'x-zammad-ticket-id'.to_sym ])
       end
       if mail[ 'x-zammad-ticket-number'.to_sym ]
-        ticket = Ticket.find_by( number: mail[ 'x-zammad-ticket-number'.to_sym ] )
+        ticket = Ticket.find_by(number: mail[ 'x-zammad-ticket-number'.to_sym ])
       end
 
       # set ticket state to open if not new
@@ -500,7 +500,6 @@ retrns
           priority_id: Ticket::Priority.find_by(name: '2 normal').id,
           preferences: preferences,
         )
-
         set_attributes_by_x_headers(ticket, 'ticket', mail)
 
         # create ticket
@@ -508,45 +507,47 @@ retrns
       end
 
       # set attributes
-      article = Ticket::Article.new(
-        ticket_id: ticket.id,
-        type_id: Ticket::Article::Type.find_by(name: 'email').id,
-        sender_id: Ticket::Article::Sender.find_by(name: 'Customer').id,
-        content_type: mail[:content_type],
-        body: mail[:body],
-        from: mail[:from],
-        to: mail[:to],
-        cc: mail[:cc],
-        subject: mail[:subject],
-        message_id: mail[:message_id],
-        internal: false,
-      )
+      ticket.with_lock do
+        article = Ticket::Article.new(
+          ticket_id: ticket.id,
+          type_id: Ticket::Article::Type.find_by(name: 'email').id,
+          sender_id: Ticket::Article::Sender.find_by(name: 'Customer').id,
+          content_type: mail[:content_type],
+          body: mail[:body],
+          from: mail[:from],
+          to: mail[:to],
+          cc: mail[:cc],
+          subject: mail[:subject],
+          message_id: mail[:message_id],
+          internal: false,
+        )
 
-      # x-headers lookup
-      set_attributes_by_x_headers(article, 'article', mail)
+        # x-headers lookup
+        set_attributes_by_x_headers(article, 'article', mail)
 
-      # create article
-      article.save!
+        # create article
+        article.save!
 
-      # store mail plain
-      Store.add(
-        object: 'Ticket::Article::Mail',
-        o_id: article.id,
-        data: msg,
-        filename: "ticket-#{ticket.number}-#{article.id}.eml",
-        preferences: {}
-      )
+        # store mail plain
+        Store.add(
+          object: 'Ticket::Article::Mail',
+          o_id: article.id,
+          data: msg,
+          filename: "ticket-#{ticket.number}-#{article.id}.eml",
+          preferences: {}
+        )
 
-      # store attachments
-      if mail[:attachments]
-        mail[:attachments].each do |attachment|
-          Store.add(
-            object: 'Ticket::Article',
-            o_id: article.id,
-            data: attachment[:data],
-            filename: attachment[:filename],
-            preferences: attachment[:preferences]
-          )
+        # store attachments
+        if mail[:attachments]
+          mail[:attachments].each do |attachment|
+            Store.add(
+              object: 'Ticket::Article',
+              o_id: article.id,
+              data: attachment[:data],
+              filename: attachment[:filename],
+              preferences: attachment[:preferences]
+            )
+          end
         end
       end
     end
diff --git a/lib/sessions.rb b/lib/sessions.rb
index 2542f64a0..21e55e3fe 100644
--- a/lib/sessions.rb
+++ b/lib/sessions.rb
@@ -294,25 +294,30 @@ returns
   def self.send(client_id, data)
     path     = "#{@path}/#{client_id}/"
     filename = "send-#{Time.now.utc.to_f}"
+    location = "#{path}#{filename}"
     check    = true
     count    = 0
     while check
-      if File.exist?(path + filename)
+      if File.exist?(location)
         count += 1
-        filename = "#{filename}-#{count}"
+        location = "#{path}#{filename}-#{count}"
       else
         check = false
       end
     end
     return false if !File.directory? path
-    File.open(path + 'a-' + filename, 'wb') { |file|
-      file.flock(File::LOCK_EX)
-      file.write data.to_json
-      file.flock(File::LOCK_UN)
-      file.close
-    }
-    return false if !File.exist?(path + 'a-' + filename)
-    FileUtils.mv(path + 'a-' + filename, path + filename)
+    begin
+      File.open(location, 'wb') { |file|
+        file.flock(File::LOCK_EX)
+        file.write data.to_json
+        file.flock(File::LOCK_UN)
+        file.close
+      }
+    rescue => e
+      log('error', e.inspect)
+      log('error', "error in writing message file '#{location}'")
+      return false
+    end
     true
   end
 
diff --git a/test/browser_test_helper.rb b/test/browser_test_helper.rb
index a851b71e6..2473fbe35 100644
--- a/test/browser_test_helper.rb
+++ b/test/browser_test_helper.rb
@@ -125,7 +125,7 @@ class TestCase < Test::Unit::TestCase
   def screenshot(params)
     instance = params[:browser] || @browser
     comment = params[:comment] || ''
-    filename = "tmp/#{Time.zone.now.strftime('screenshot_%Y_%m_%d__%H_%M_%S')}_#{comment}_#{instance.hash}.png"
+    filename = "tmp/#{Time.zone.now.strftime('screenshot_%Y_%m_%d__%H_%M_%S_%L')}_#{comment}#{instance.hash}.png"
     log('screenshot', { filename: filename })
     instance.save_screenshot(filename)
   end
@@ -415,6 +415,7 @@ class TestCase < Test::Unit::TestCase
     log('click', params)
 
     instance = params[:browser] || @browser
+    screenshot(browser: instance, comment: 'click_before')
     if params[:css]
 
       begin
@@ -501,6 +502,7 @@ class TestCase < Test::Unit::TestCase
 
   modal_disappear(
     browser: browser1,
+    timeout: 12, # default 6
   )
 
 =end
@@ -514,7 +516,7 @@ class TestCase < Test::Unit::TestCase
     watch_for_disappear(
       browser: instance,
       css:     '.modal',
-      timeout: 6,
+      timeout: params[:timeout] || 6,
     )
   end
 
@@ -599,6 +601,7 @@ class TestCase < Test::Unit::TestCase
     log('set', params)
 
     instance = params[:browser] || @browser
+    screenshot(browser: instance, comment: 'set_before')
 
     element = instance.find_elements(css: params[:css])[0]
     if !params[:no_click]
@@ -616,11 +619,24 @@ class TestCase < Test::Unit::TestCase
       }
     end
 
+    # it's not working stable with ff via selenium, use js
+    if browser =~ /firefox/i && params[:css] =~ /\[data-name=/
+      log('set_ff_check', params)
+      value = instance.find_elements(css: params[:css])[0].text
+      if value != params[:value]
+        log('set_ff_check_failed_use_js', params)
+        value_quoted = quote(params[:value])
+        puts "DEBUG $('#{params[:css]}').html('#{value_quoted}').trigger('focusout')"
+        instance.execute_script("$('#{params[:css]}').html('#{value_quoted}').trigger('focusout')")
+      end
+    end
+
     if params[:blur]
       instance.execute_script("$('#{params[:css]}').blur()")
     end
 
     sleep 0.2
+    screenshot(browser: instance, comment: 'set_after')
   end
 
 =begin
@@ -639,6 +655,7 @@ class TestCase < Test::Unit::TestCase
     log('select', params)
 
     instance = params[:browser] || @browser
+    screenshot(browser: instance, comment: 'select_before')
 
     # searchable select
     element = instance.find_elements(css: "#{params[:css]}.js-shadow")[0]
@@ -677,6 +694,7 @@ class TestCase < Test::Unit::TestCase
       #puts "select2 - #{params.inspect}"
     end
     sleep 0.4
+    screenshot(browser: instance, comment: 'select_after')
   end
 
 =begin
@@ -695,6 +713,7 @@ class TestCase < Test::Unit::TestCase
     log('switch', params)
 
     instance = params[:browser] || @browser
+    screenshot(browser: instance, comment: 'switch_before')
 
     element = instance.find_elements(css: "#{params[:css]} input[type=checkbox]")[0]
     checked = element.attribute('checked')
@@ -720,6 +739,7 @@ class TestCase < Test::Unit::TestCase
         raise 'Switch not off!' if checked
       end
     end
+    screenshot(browser: instance, comment: 'switch_after')
   end
 
 =begin
@@ -736,11 +756,13 @@ class TestCase < Test::Unit::TestCase
     log('check', params)
 
     instance = params[:browser] || @browser
+    screenshot(browser: instance, comment: 'check_before')
 
     instance.execute_script("if (!$('#{params[:css]}').prop('checked')) { $('#{params[:css]}').click() }")
     #element = instance.find_elements(css: params[:css])[0]
     #checked = element.attribute('checked')
     #element.click if !checked
+    screenshot(browser: instance, comment: 'check_after')
   end
 
 =begin
@@ -757,11 +779,13 @@ class TestCase < Test::Unit::TestCase
     log('uncheck', params)
 
     instance = params[:browser] || @browser
+    screenshot(browser: instance, comment: 'uncheck_before')
 
     instance.execute_script("if ($('#{params[:css]}').prop('checked')) { $('#{params[:css]}').click() }")
     #element = instance.find_elements(css: params[:css])[0]
     #checked = element.attribute('checked')
     #element.click if checked
+    screenshot(browser: instance, comment: 'uncheck_after')
   end
 
 =begin
@@ -779,10 +803,12 @@ class TestCase < Test::Unit::TestCase
     log('sendkey', params)
 
     instance = params[:browser] || @browser
+    screenshot(browser: instance, comment: 'sendkey_before')
     if params[:value].class == Array
       params[:value].each { |key|
         instance.action.send_keys(key).perform
       }
+      screenshot(browser: instance, comment: 'sendkey_after')
       return
     end
     instance.action.send_keys(params[:value]).perform
@@ -791,6 +817,7 @@ class TestCase < Test::Unit::TestCase
     else
       sleep 0.2
     end
+    screenshot(browser: instance, comment: 'sendkey_after')
   end
 
 =begin
@@ -825,9 +852,11 @@ class TestCase < Test::Unit::TestCase
       end
       if params[:should_not_match]
         if success
+          screenshot(browser: instance, comment: 'match_failed')
           raise "should not match '#{params[:value]}' in select list, but is matching"
         end
       elsif !success
+        screenshot(browser: instance, comment: 'match_failed')
         raise "not matching '#{params[:value]}' in select list"
       end
 
@@ -871,9 +900,11 @@ class TestCase < Test::Unit::TestCase
 
     if match
       if params[:should_not_match]
+        screenshot(browser: instance, comment: 'match_failed')
         raise "matching '#{params[:value]}' in content '#{text}' but should not!"
       end
     elsif !params[:should_not_match]
+      screenshot(browser: instance, comment: 'match_failed')
       raise "not matching '#{params[:value]}' in content '#{text}' but should!"
     end
     sleep 0.2
@@ -1032,10 +1063,11 @@ class TestCase < Test::Unit::TestCase
         if title =~ /#{data[:title]}/i
           assert(true, "matching '#{data[:title]}' in title '#{title}'")
         else
+          screenshot(browser: instance, comment: 'verify_task_failed')
           raise "not matching '#{data[:title]}' in title '#{title}'"
         end
       end
-      puts "tv #{params.inspect}"
+
       # verify modified
       if data.key?(:modified)
         exists      = instance.find_elements(css: '.tasks .is-active')[0]
@@ -1051,15 +1083,19 @@ class TestCase < Test::Unit::TestCase
           if is_modified
             assert(true, "task '#{data[:title]}' is modifed")
           elsif !exists
+            screenshot(browser: instance, comment: 'verify_task_failed')
             raise "task '#{data[:title]}' not exists, should not modified"
           else
+            screenshot(browser: instance, comment: 'verify_task_failed')
             raise "task '#{data[:title]}' is not modifed"
           end
         elsif !is_modified
           assert(true, "task '#{data[:title]}' is modifed")
         elsif !exists
+          screenshot(browser: instance, comment: 'verify_task_failed')
           raise "task '#{data[:title]}' not exists, should be not modified"
         else
+          screenshot(browser: instance, comment: 'verify_task_failed')
           raise "task '#{data[:title]}' is modifed, but should not"
         end
       end
@@ -1285,12 +1321,14 @@ wait untill text in selector disabppears
     switch_window_focus(params)
     log('shortcut', params)
     instance = params[:browser] || @browser
+    screenshot(browser: instance, comment: 'shortcut_before')
     instance.action.key_down(:control)
             .key_down(:alt)
             .send_keys(params[:key])
             .key_up(:alt)
             .key_up(:control)
             .perform
+    screenshot(browser: instance, comment: 'shortcut_after')
   end
 
 =begin
@@ -1829,14 +1867,6 @@ wait untill text in selector disabppears
         clear:    true,
         mute_log: true,
       )
-
-      # it's not working stable via selenium, use js
-      value = instance.find_elements(css: '.content .newTicket div[data-name=body]')[0].text
-      #puts "V #{value.inspect}"
-      if value != data[:body]
-        body_quoted = quote(data[:body])
-        instance.execute_script("$('.content.active div[data-name=body]').html('#{body_quoted}').trigger('focusout')")
-      end
     end
     if data[:customer]
       element = instance.find_elements(css: '.active .newTicket input[name="customer_id_completion"]')[0]
@@ -2260,12 +2290,14 @@ wait untill text in selector disabppears
       browser: instance,
       js: '$(".content.active .sidebar").css("display", "block")',
     )
+    screenshot(browser: instance, comment: 'ticket_open_by_overview')
     instance.find_elements(css: ".content.active .sidebar a[href=\"#{params[:link]}\"]")[0].click
     sleep 1
     execute(
       browser: instance,
       js: '$(".content.active .sidebar").css("display", "none")',
     )
+    screenshot(browser: instance, comment: 'ticket_open_by_overview_search')
     instance.find_elements(partial_link_text: params[:number])[0].click
     sleep 1
     number = instance.find_elements(css: '.active .ticketZoom-header .ticket-number')[0].text
@@ -2310,8 +2342,9 @@ wait untill text in selector disabppears
     sleep 1
 
     # open ticket
+    screenshot(browser: instance, comment: 'ticket_open_by_search')
     #instance.find_element(partial_link_text: params[:number] } ).click
-    instance.execute_script("$(\".js-global-search-result a:contains('#{params[:value]}') .nav-tab-icon\").click()")
+    instance.execute_script("$(\".js-global-search-result a:contains('#{params[:number]}') .nav-tab-icon\").first().click()")
     sleep 1
     number = instance.find_elements(css: '.active .ticketZoom-header .ticket-number')[0].text
     if number !~ /#{params[:number]}/
@@ -2345,6 +2378,7 @@ wait untill text in selector disabppears
     sleep 3
 
     # open ticket
+    screenshot(browser: instance, comment: 'ticket_open_by_title_search')
     #instance.find_element(partial_link_text: params[:title] } ).click
     instance.execute_script("$(\".js-global-search-result a:contains('#{params[:title]}') .nav-tab-icon\").click()")
     sleep 1
@@ -2466,6 +2500,8 @@ wait untill text in selector disabppears
     element.clear
     element.send_keys(params[:value])
     sleep 3
+
+    screenshot(browser: instance, comment: 'user_open_by_search')
     #instance.find_element(partial_link_text: params[:value]).click
     instance.execute_script("$(\".js-global-search-result a:contains('#{params[:value]}') .nav-tab-icon\").click()")
     sleep 1