avoid STI conditions when updating/deleting with subqueries

it can lead to strange results where more rows were updated than were expected

also make sure to lock rows from the appropriate table when doing a subquery
update with a join in it

Change-Id: I16d0fee65a1d2b31ef72fd1933737b3715c441db
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/340620
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

config/brakeman.ignore

@@ -643,7 +643,7 @@
       "check_name": "SendFile",
       "message": "Model attribute used in file name",
       "file": "app/controllers/files_controller.rb",
-      "line": 1568,
+      "line": 1571,
       "link": "https://brakemanscanner.org/docs/warning_types/file_access/",
       "code": "send_file(Thumbnail.where(:id => params[:id], :uuid => params[:uuid]).first.full_filename, :content_type => Thumbnail.where(:id => params[:id], :uuid => params[:uuid]).first.content_type)",
       "render_path": null,
@@ -978,7 +978,7 @@
       "check_name": "UnsafeReflection",
       "message": "Unsafe reflection method `const_get` called with parameter value",
       "file": "app/controllers/application_controller.rb",
-      "line": 1461,
+      "line": 1468,
       "link": "https://brakemanscanner.org/docs/warning_types/remote_code_execution/",
       "code": "Object.const_get((params[:feed_code].split(\"_\", 2) or [\"group_membership\", params[:feed_code].split(\"_\", 3)[-1]])[0].classify, false)",
       "render_path": null,
@@ -1110,6 +1110,25 @@
       ],
       "note": ""
     },
+    {
+      "warning_type": "SQL Injection",
+      "warning_code": 0,
+      "fingerprint": "b6f831712d0a04b2fcc4efc4a94c6b022740cd92bef1cd6dbadf32e7f7ad8f77",
+      "check_name": "SQL",
+      "message": "Possible SQL injection",
+      "file": "config/initializers/active_record.rb",
+      "line": 1530,
+      "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
+      "code": "lock(\"#{lock_type_clause(lock_type)} OF #{connection.quote_local_table_name(klass.table_name)}\")",
+      "render_path": null,
+      "location": null,
+      "user_input": "lock_type_clause(lock_type)",
+      "confidence": "Medium",
+      "cwe_id": [
+        89
+      ],
+      "note": ""
+    },
     {
       "warning_type": "SQL Injection",
       "warning_code": 0,
@@ -1481,7 +1500,7 @@
       "check_name": "SQL",
       "message": "Possible SQL injection",
       "file": "config/initializers/active_record.rb",
-      "line": 1606,
+      "line": 1623,
       "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
       "code": "connection.select_value(\"SELECT COUNT(*) FROM pg_proc WHERE proname='#{procname}'\")",
       "render_path": null,

config/initializers/active_record.rb

@@ -1271,9 +1271,17 @@ end
 
 module LockForNoKeyUpdate
   def lock(lock_type = true)
-    lock_type = "FOR NO KEY UPDATE" if lock_type == :no_key_update
-    lock_type = "FOR NO KEY UPDATE SKIP LOCKED" if lock_type == :no_key_update_skip_locked
-    super(lock_type)
+    super(lock_type_clause(lock_type))
+  end
+
+  private
+
+  def lock_type_clause(lock_type)
+    return "FOR NO KEY UPDATE" if lock_type == :no_key_update
+    return "FOR NO KEY UPDATE SKIP LOCKED" if lock_type == :no_key_update_skip_locked
+    return "FOR UPDATE" if lock_type == true
+
+    lock_type
   end
 end
 ActiveRecord::Relation.prepend(LockForNoKeyUpdate)
@@ -1303,8 +1311,8 @@ ActiveRecord::Relation.class_eval do
   end
 
   def update_all_locked_in_order(lock_type: :no_key_update, **updates)
-    locked_scope = lock(lock_type).order(primary_key.to_sym)
-    unscoped.where(primary_key => locked_scope).update_all(updates)
+    locked_scope = lock_for_subquery_update(lock_type).order(primary_key.to_sym)
+    base_class.unscoped.where(primary_key => locked_scope).update_all(updates)
   end
 
   def touch_all(*names, time: nil)
@@ -1499,19 +1507,28 @@ Switchman::ActiveRecord::Relation.include(UpdateAndDeleteWithJoins)
 module UpdateAndDeleteAllWithLimit
   def delete_all(*args)
     if limit_value || offset_value
-      scope = except(:select).select(primary_key).lock
-      return unscoped.where(primary_key => scope).delete_all
+      scope = lock_for_subquery_update.except(:select).select(primary_key)
+      return base_class.unscoped.where(primary_key => scope).delete_all
     end
     super
   end
 
   def update_all(updates, *args)
     if limit_value || offset_value
-      scope = except(:select).select(primary_key).lock
-      return unscoped.where(primary_key => scope).update_all(updates)
+      scope = lock_for_subquery_update.except(:select).select(primary_key)
+      return base_class.unscoped.where(primary_key => scope).update_all(updates)
     end
     super
   end
+
+  private
+
+  def lock_for_subquery_update(lock_type = true)
+    return lock(lock_type) if !lock_type || joins_values.empty?
+
+    # make sure to lock the proper table
+    lock("#{lock_type_clause(lock_type)} OF #{connection.quote_local_table_name(klass.table_name)}")
+  end
 end
 Switchman::ActiveRecord::Relation.include(UpdateAndDeleteAllWithLimit)
 

spec/initializers/active_record_spec.rb

@@ -492,7 +492,7 @@ module ActiveRecord
       let(:scope) { User.active }
 
       it "uses FOR UPDATE on a normal exclusive lock" do
-        expect(scope.lock(true).lock_value).to be true
+        expect(scope.lock(true).lock_value).to eq "FOR UPDATE"
       end
 
       it "substitutes 'FOR NO KEY UPDATE' if specified" do