From 9050b1bf5aa32e4e085281563b966936b26cd6eb Mon Sep 17 00:00:00 2001
From: Bianca Nenciu <nbianca@users.noreply.github.com>
Date: Tue, 23 Apr 2019 05:51:30 +0300
Subject: [PATCH] FIX: Add unique index on group_requests(group_id, user_id).
 (#7399)

---
 app/controllers/groups_controller.rb              | 15 +++++++--------
 config/locales/server.en.yml                      |  1 +
 ...18113814_add_unique_index_to_group_requests.rb |  6 ++++++
 spec/requests/groups_controller_spec.rb           | 14 ++++++++++++++
 4 files changed, 28 insertions(+), 8 deletions(-)
 create mode 100644 db/migrate/20190418113814_add_unique_index_to_group_requests.rb

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index df5e1e03867..9fc82d1e4e8 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -408,12 +408,13 @@ class GroupsController < ApplicationController
   def request_membership
     params.require(:reason)
 
-    unless current_user.staff?
-      RateLimiter.new(current_user, "request_group_membership", 1, 1.day).performed!
-    end
-
     group = find_group(:id)
-    group_name = group.name
+
+    begin
+      GroupRequest.create!(group: group, user: current_user, reason: params[:reason])
+    rescue ActiveRecord::RecordNotUnique => e
+      return render json: failed_json.merge(error: I18n.t("groups.errors.already_requested_membership")), status: 409
+    end
 
     usernames = [current_user.username].concat(
       group.users.where('group_users.owner')
@@ -432,15 +433,13 @@ class GroupsController < ApplicationController
     EOF
 
     post = PostCreator.new(current_user,
-      title: I18n.t('groups.request_membership_pm.title', group_name: group_name),
+      title: I18n.t('groups.request_membership_pm.title', group_name: group.name),
       raw: raw,
       archetype: Archetype.private_message,
       target_usernames: usernames.join(','),
       skip_validations: true
     ).create!
 
-    GroupRequest.create!(group: group, user: current_user, reason: params[:reason])
-
     render json: success_json.merge(relative_url: post.topic.relative_url)
   end
 
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 8373056e036..ab44a7efb75 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -364,6 +364,7 @@ en:
       email_already_used_in_group: "'%{email}' is already used by the group '%{group_name}'."
       email_already_used_in_category: "'%{email}' is already used by the category '%{category_name}'."
       cant_allow_membership_requests: "You cannot allow membership requests for a group without any owners."
+      already_requested_membership: "You have already requested membership for this group."
     default_names:
       everyone: "everyone"
       admins: "admins"
diff --git a/db/migrate/20190418113814_add_unique_index_to_group_requests.rb b/db/migrate/20190418113814_add_unique_index_to_group_requests.rb
new file mode 100644
index 00000000000..194777d6af1
--- /dev/null
+++ b/db/migrate/20190418113814_add_unique_index_to_group_requests.rb
@@ -0,0 +1,6 @@
+class AddUniqueIndexToGroupRequests < ActiveRecord::Migration[5.2]
+  def change
+    execute "DELETE FROM group_requests WHERE id NOT IN (SELECT MIN(id) FROM group_requests GROUP BY group_id, user_id)"
+    add_index :group_requests, [:group_id, :user_id], unique: true
+  end
+end
diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb
index 51cf5865b49..f4cac122fe1 100644
--- a/spec/requests/groups_controller_spec.rb
+++ b/spec/requests/groups_controller_spec.rb
@@ -1275,6 +1275,20 @@ describe GroupsController do
       expect(response.status).to eq(400)
     end
 
+    it 'checks for duplicates' do
+      sign_in(user)
+
+      post "/groups/#{group.name}/request_membership.json",
+        params: { reason: 'Please add me in' }
+
+      expect(response.status).to eq(200)
+
+      post "/groups/#{group.name}/request_membership.json",
+        params: { reason: 'Please add me in' }
+
+      expect(response.status).to eq(409)
+    end
+
     it 'should create the right PM' do
       owner1 = Fabricate(:user, last_seen_at: Time.zone.now)
       owner2 = Fabricate(:user, last_seen_at: Time.zone.now - 1 .day)