From d6f5baa87e22156e2e4670cad75d0c3159d3e633 Mon Sep 17 00:00:00 2001
From: Thorsten Eckel <te@zammad.com>
Date: Thu, 30 Jul 2020 15:58:49 +0200
Subject: [PATCH] Fixes #3109 - Race condition between #lookup and .update
 causes caching of obsolete data.

---
 app/models/application_model/can_lookup.rb | 41 +++++++++++++++++-----
 app/models/ticket.rb                       |  2 +-
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/app/models/application_model/can_lookup.rb b/app/models/application_model/can_lookup.rb
index c5e955928..b703dc87f 100644
--- a/app/models/application_model/can_lookup.rb
+++ b/app/models/application_model/can_lookup.rb
@@ -46,17 +46,42 @@ returns
 
     private
 
-    def find_and_save_to_cache_by(attr)
-      record = find_by(attr)
-      return nil if string_key?(attr.keys.first) && (record&.send(attr.keys.first) != attr.values.first.to_s) # enforce case-sensitivity on MySQL
-      return record if ActiveRecord::Base.connection.transaction_open? # rollbacks can invalidate cache entries
+    def find_and_save_to_cache_by(args)
+      attribute    = args.keys.first
+      lookup_value = args.values.first.to_s
 
-      cache_set(attr.values.first, record)
-      record
+      # rollbacks can invalidate cache entry
+      # therefore we don't write it
+      if ActiveRecord::Base.connection.transaction_open?
+        result = find_by(attribute => lookup_value)
+        # enforce case-sensitivity on MySQL
+        result = nil if !key_sensitive_match?(result, attribute, lookup_value)
+      else
+        # get the record via an `FOR UPDATE` DB lock inside of
+        # a transaction to ensure that we don't write obsolete
+        # data into the cache
+        transaction do
+          result = lock.find_by(attribute => lookup_value)
+          # enforce case-sensitivity on MySQL
+          if key_sensitive_match?(result, attribute, lookup_value)
+            # cache only if we got a key-sensitive match
+            cache_set(lookup_value, result)
+          else
+            # no key-sensitive match - no result
+            result = nil
+          end
+        end
+      end
+
+      result
     end
 
-    def string_key?(key)
-      type_for_attribute(key.to_s).type == :string
+    def key_sensitive_match?(record, attribute, lookup_value)
+      return false if record.blank?
+      return true if type_for_attribute(attribute.to_s).type != :string
+
+      record[attribute] == lookup_value
     end
+
   end
 end
diff --git a/app/models/ticket.rb b/app/models/ticket.rb
index 3fbbe4108..5f5ab0605 100644
--- a/app/models/ticket.rb
+++ b/app/models/ticket.rb
@@ -1680,7 +1680,7 @@ result
   def build_sms_recipients_list(value, article)
     Array(value['recipient'])
       .each_with_object([]) { |recipient_type, sum| sum.concat(Array(sms_recipients_by_type(recipient_type, article))) }
-      .map { |user_or_id| User.lookup(id: user_or_id) }
+      .map { |user_or_id| user_or_id.is_a?(User) ? user_or_id : User.lookup(id: user_or_id) }
       .uniq(&:id)
       .select { |user| user.mobile.present? }
   end