From 2686ee5ab29b33edc1fcb32811421c703bd60ff6 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Tue, 13 Dec 2016 16:39:44 +0800
Subject: [PATCH] FIX: Admin can't add/remove public group users.

---
 app/controllers/groups_controller.rb |  8 +++--
 spec/integration/groups_spec.rb      | 47 ++++++++++++++++++----------
 2 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 058aa8851bc..e4b5fc14331 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -132,7 +132,9 @@ class GroupsController < ApplicationController
     raise Discourse::NotFound if users.blank?
 
     if group.public
-      raise Discourse::InvalidAccess unless current_user == users.first
+      if !guardian.can_log_group_changes?(group) && current_user != users.first
+        raise Discourse::InvalidAccess
+      end
 
       unless current_user.staff?
         RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed!
@@ -183,7 +185,9 @@ class GroupsController < ApplicationController
     raise Discourse::NotFound unless user
 
     if group.public
-      raise Discourse::InvalidAccess unless current_user == user
+      if !guardian.can_log_group_changes?(group) && current_user != user
+        raise Discourse::InvalidAccess
+      end
 
       unless current_user.staff?
         RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed!
diff --git a/spec/integration/groups_spec.rb b/spec/integration/groups_spec.rb
index 7d4b2a0644c..fbf620c4a38 100644
--- a/spec/integration/groups_spec.rb
+++ b/spec/integration/groups_spec.rb
@@ -237,20 +237,6 @@ describe "Groups" do
         expect(group_history.target_user).to eq(user2)
       end
 
-      it "can make incremental deletes" do
-        expect do
-          xhr :delete, "/groups/#{group.id}/members", username: user.username
-        end.to change { group.users.count }.by(-1)
-
-        expect(response).to be_success
-
-        group_history = GroupHistory.last
-
-        expect(group_history.action).to eq(GroupHistory.actions[:remove_user_from_group])
-        expect(group_history.acting_user).to eq(admin)
-        expect(group_history.target_user).to eq(user)
-      end
-
       it "cannot add members to automatic groups" do
         group.update!(automatic: true)
 
@@ -296,6 +282,22 @@ describe "Groups" do
           group.update!(public: true)
         end
 
+        context 'admin' do
+          it "can make incremental adds" do
+            expect do
+              xhr :put, "/groups/#{group.id}/members", usernames: other_user.username
+            end.to change { group.users.count }.by(1)
+
+            expect(response).to be_success
+
+            group_history = GroupHistory.last
+
+            expect(group_history.action).to eq(GroupHistory.actions[:add_user_to_group])
+            expect(group_history.acting_user).to eq(admin)
+            expect(group_history.target_user).to eq(other_user)
+          end
+        end
+
         it 'should allow a user to join the group' do
           sign_in(other_user)
 
@@ -305,7 +307,9 @@ describe "Groups" do
           expect(response).to be_success
         end
 
-        it 'should not allow a user to add another user to a group' do
+        it 'should not allow an underprivilege user to add another user to a group' do
+          sign_in(user)
+
           xhr :put, "/groups/#{group.id}/members", usernames: other_user.username
 
           expect(response).to be_forbidden
@@ -364,6 +368,15 @@ describe "Groups" do
             group.update!(public: true)
           end
 
+          context "admin" do
+            it "removes by username" do
+              expect { xhr :delete, "/groups/#{group.id}/members", username: other_user.username }
+                .to change { group.users.count }.by(-1)
+
+              expect(response).to be_success
+            end
+          end
+
           it 'should allow a user to leave a group' do
             sign_in(other_user)
 
@@ -373,7 +386,9 @@ describe "Groups" do
             expect(response).to be_success
           end
 
-          it 'should not allow a user to leave a group for another user' do
+          it 'should not allow a underprivilege user to leave a group for another user' do
+            sign_in(user)
+
             xhr :delete, "/groups/#{group.id}/members", username: other_user.username
 
             expect(response).to be_forbidden