FIX: user_id handling on remove user from group

FIX: user_id handling on remove user from group

Under some conditions it was possible to pass in a user_id as an integer, but we would try and parse it as a comma delimited string resulting in an error. This has been fixed so that we are no longer mapping the user_id param to user_ids.

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 19ae0b4..ee322a7 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -327,7 +327,6 @@ class GroupsController < ApplicationController
 
     # Maintain backwards compatibility
     params[:usernames] = params[:username] if params[:username].present?
-    params[:user_ids] = params[:user_id] if params[:user_id].present?
     params[:user_emails] = params[:user_email] if params[:user_email].present?
 
     users = users_from_params
@@ -492,8 +491,11 @@ class GroupsController < ApplicationController
     if params[:usernames].present?
       users = User.where(username_lower: params[:usernames].split(",").map(&:downcase))
       raise Discourse::InvalidParameters.new(:usernames) if users.blank?
+    elsif params[:user_id].present?
+      users = User.where(id: params[:user_id].to_i)
+      raise Discourse::InvalidParameters.new(:user_id) if users.blank?
     elsif params[:user_ids].present?
-      users = User.where(id: params[:user_ids].split(","))
+      users = User.where(id: params[:user_ids].to_s.split(","))
       raise Discourse::InvalidParameters.new(:user_ids) if users.blank?
     elsif params[:user_emails].present?
       users = User.with_email(params[:user_emails].split(","))
diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb
index 9a21289..1e42565 100644
--- a/spec/requests/groups_controller_spec.rb
+++ b/spec/requests/groups_controller_spec.rb
@@ -1017,6 +1017,15 @@ describe GroupsController do
           expect(response.status).to eq(200)
         end
 
+        it "removes by id with integer in json" do
+          expect do
+            headers = { "CONTENT_TYPE": "application/json" }
+            delete "/groups/#{group.id}/members.json", params: "{\"user_id\":#{user.id}}", headers: headers
+          end.to change { group.users.count }.by(-1)
+
+          expect(response.status).to eq(200)
+        end
+
         it "removes by username" do
           expect do
             delete "/groups/#{group.id}/members.json", params: { username: user.username }
@@ -1102,6 +1111,15 @@ describe GroupsController do
             expect(response.status).to eq(200)
           end
 
+          it "removes by id with integer in json" do
+            expect do
+              headers = { "CONTENT_TYPE": "application/json" }
+              delete "/groups/#{group1.id}/members.json", params: "{\"user_ids\":#{user1.id}}", headers: headers
+            end.to change { group1.users.count }.by(-1)
+
+            expect(response.status).to eq(200)
+          end
+
           it "removes by email" do
             expect do
               delete "/groups/#{group1.id}/members.json",

GitHub sha: de47b35b

2 Likes

This commit has been mentioned on Discourse Meta. There might be relevant details there: