Handle race condition when creating an MR approval

Add two migrations for deleting duplicates and adding a
unique index for user_id and merge_request_id on approvals
table.

Since this migration will run on MySQL too, a separate
query needs to be implemented for it as the one for PostgreSQL
doesn't work on MySQL.

This will prevent creation of duplicate records in case
concurrent requests try to do the same thing.

db/schema.rb

@@ -292,6 +292,7 @@ ActiveRecord::Schema.define(version: 20190404231137) do
     t.datetime "created_at"
     t.datetime "updated_at"
     t.index ["merge_request_id"], name: "index_approvals_on_merge_request_id", using: :btree
+    t.index ["user_id", "merge_request_id"], name: "index_approvals_on_user_id_and_merge_request_id", unique: true, using: :btree
   end
 
   create_table "approver_groups", force: :cascade do |t|

ee/app/services/merge_requests/approval_service.rb

@@ -5,7 +5,7 @@ module MergeRequests
     def execute(merge_request)
       approval = merge_request.approvals.new(user: current_user)
 
-      if approval.save
+      if save_approval(approval)
         merge_request.reset_approval_cache!
 
         create_approval_note(merge_request)
@@ -22,6 +22,12 @@ module MergeRequests
 
     private
 
+    def save_approval(approval)
+      approval.save
+    rescue ActiveRecord::RecordNotUnique
+      false
+    end
+
     def create_approval_note(merge_request)
       SystemNoteService.approve_mr(merge_request, current_user)
     end

ee/changelogs/unreleased/security-approval-race-condition.yml

+---
+title: Handle race condition when creating an MR approval
+merge_request:
+author:
+type: security

ee/db/migrate/20190322025939_remove_duplicates_from_approvals.rb

+# frozen_string_literal: true
+
+class RemoveDuplicatesFromApprovals < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    if Gitlab::Database.mysql?
+      execute <<-SQL
+        DELETE FROM a
+        USING approvals AS a
+        INNER JOIN (
+          SELECT user_id, merge_request_id, MIN(id) as min_id
+          FROM approvals
+          GROUP BY user_id, merge_request_id
+          HAVING COUNT(id) > 1
+        ) as approvals_with_duplicates
+        ON approvals_with_duplicates.user_id = a.user_id
+        AND approvals_with_duplicates.merge_request_id = a.merge_request_id
+        WHERE approvals_with_duplicates.min_id <> a.id;
+      SQL
+    else
+      execute <<-SQL
+        DELETE FROM approvals
+        USING (
+          SELECT user_id, merge_request_id, MIN(id) as min_id
+          FROM approvals
+          GROUP BY user_id, merge_request_id
+          HAVING COUNT(id) > 1
+        ) as approvals_with_duplicates
+        WHERE approvals_with_duplicates.user_id = approvals.user_id
+        AND approvals_with_duplicates.merge_request_id = approvals.merge_request_id
+        AND approvals_with_duplicates.min_id <> approvals.id;
+      SQL
+    end
+  end
+
+  def down
+  end
+end

ee/db/migrate/20190322030018_add_unique_constraint_to_approvals_user_id_and_merge_request_id.rb

+# frozen_string_literal: true
+
+class AddUniqueConstraintToApprovalsUserIdAndMergeRequestId < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :approvals, [:user_id, :merge_request_id], unique: true
+  end
+
+  def down
+    remove_concurrent_index :approvals, [:user_id, :merge_request_id]
+  end
+end

ee/spec/migrations/remove_duplicates_from_approvals_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'migrate', '20190322025939_remove_duplicates_from_approvals.rb')
+
+describe RemoveDuplicatesFromApprovals, :migration do
+  let(:migration) { described_class.new }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:approvals) { table(:approvals) }
+
+  describe '#up' do
+    before do
+      namespaces.create(id: 1, name: 'ns', path: 'ns')
+      projects.create(id: 1, namespace_id: 1)
+      merge_requests.create(id: 1, target_branch: 'master', source_branch: 'feature-1', target_project_id: 1)
+      merge_requests.create(id: 2, target_branch: 'master', source_branch: 'feature-2', target_project_id: 1)
+    end
+
+    it 'deletes duplicate records and keeps the first one' do
+      first_approval = approvals.create(id: 1, merge_request_id: 1, user_id: 1)
+      approvals.create(id: 2, merge_request_id: 1, user_id: 1)
+
+      migration.up
+
+      expect(approvals.all.to_a).to contain_exactly(first_approval)
+    end
+
+    it 'does not delete unique records' do
+      unique_approvals = [
+        approvals.create(id: 1, merge_request_id: 1, user_id: 1),
+        approvals.create(id: 2, merge_request_id: 1, user_id: 2),
+        approvals.create(id: 3, merge_request_id: 2, user_id: 1)
+      ]
+
+      migration.up
+
+      expect(approvals.all.to_a).to contain_exactly(*unique_approvals)
+    end
+  end
+end