From a74805d3f8d8cf16641e404d2a46e898ec0fe133 Mon Sep 17 00:00:00 2001
From: jbrw <jamie@goatforce5.org>
Date: Mon, 19 Oct 2020 16:30:21 -0400
Subject: [PATCH] FIX: moderators can add/remove group owners (#10960)

If `SiteSetting.moderators_manage_categories_and_groups` is enabled, a moderator shoud be able to add/remove group owners.
---
 .../app/components/group-member-dropdown.js   |   2 +-
 .../discourse/app/templates/group-index.hbs   |   1 +
 .../app/templates/mobile/group-index.hbs      |   1 +
 .../app/templates/modal/group-add-members.hbs |   2 +-
 .../discourse/tests/acceptance/group-test.js  |  30 ++++++
 .../tests/fixtures/group-fixtures.js          |  28 +++++
 app/controllers/admin/groups_controller.rb    |   3 +
 config/routes.rb                              |  11 +-
 spec/requests/admin/groups_controller_spec.rb | 101 ++++++++++++++++++
 9 files changed, 172 insertions(+), 7 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/group-member-dropdown.js b/app/assets/javascripts/discourse/app/components/group-member-dropdown.js
index 86620d8698d..f4cb9ad287f 100644
--- a/app/assets/javascripts/discourse/app/components/group-member-dropdown.js
+++ b/app/assets/javascripts/discourse/app/components/group-member-dropdown.js
@@ -23,7 +23,7 @@ export default DropdownSelectBoxComponent.extend({
       },
     ];
 
-    if (this.get("currentUser.admin")) {
+    if (this.canAdminGroup) {
       if (this.member.owner) {
         items.push({
           id: "removeOwner",
diff --git a/app/assets/javascripts/discourse/app/templates/group-index.hbs b/app/assets/javascripts/discourse/app/templates/group-index.hbs
index c21bc1b5abe..973f0aa4ce0 100644
--- a/app/assets/javascripts/discourse/app/templates/group-index.hbs
+++ b/app/assets/javascripts/discourse/app/templates/group-index.hbs
@@ -59,6 +59,7 @@
                 {{#if canManageGroup}}
                   {{group-member-dropdown
                     member=m
+                    canAdminGroup=model.can_admin_group
                     onChange=(action "actOnGroup" m)
                   }}
                 {{/if}}
diff --git a/app/assets/javascripts/discourse/app/templates/mobile/group-index.hbs b/app/assets/javascripts/discourse/app/templates/mobile/group-index.hbs
index 36ec17ebdfa..5ef942e30cf 100644
--- a/app/assets/javascripts/discourse/app/templates/mobile/group-index.hbs
+++ b/app/assets/javascripts/discourse/app/templates/mobile/group-index.hbs
@@ -45,6 +45,7 @@
               {{#if canManageGroup}}
                 {{group-member-dropdown
                   member=user
+                  canAdminGroup=model.can_admin_group
                   onChange=(action "actOnGroup" user)
                 }}
               {{/if}}
diff --git a/app/assets/javascripts/discourse/app/templates/modal/group-add-members.hbs b/app/assets/javascripts/discourse/app/templates/modal/group-add-members.hbs
index 4d07bdbe368..1222da13bc8 100644
--- a/app/assets/javascripts/discourse/app/templates/modal/group-add-members.hbs
+++ b/app/assets/javascripts/discourse/app/templates/modal/group-add-members.hbs
@@ -16,7 +16,7 @@
           id="group-add-members-user-selector"}}
     </div>
 
-    {{#if currentUser.admin}}
+    {{#if model.can_admin_group}}
       <div class="control-group group-add-members-make-owner">
         <label>
           {{input type="checkbox"
diff --git a/app/assets/javascripts/discourse/tests/acceptance/group-test.js b/app/assets/javascripts/discourse/tests/acceptance/group-test.js
index 3f7565e34ab..d96b5e9c783 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/group-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/group-test.js
@@ -256,3 +256,33 @@ test("Admin Viewing Group", async (assert) => {
     "it should display the group name"
   );
 });
+
+test("Moderator Viewing Group", async (assert) => {
+  await visit("/g/alternative-group");
+
+  assert.ok(
+    find(".nav-pills li a[title='Manage']").length === 1,
+    "it should show manage group tab if user can_admin_group"
+  );
+
+  await click(".group-members-add.btn");
+
+  assert.ok(
+    find(".group-add-members-modal .group-add-members-make-owner"),
+    "it allows moderators to set group owners"
+  );
+
+  await click(".group-add-members-modal .modal-close");
+
+  const memberDropdown = selectKit(".group-member-dropdown:first");
+  await memberDropdown.expand();
+
+  assert.equal(
+    memberDropdown.rowByIndex(0).name(),
+    I18n.t("groups.members.remove_member")
+  );
+  assert.equal(
+    memberDropdown.rowByIndex(1).name(),
+    I18n.t("groups.members.make_owner")
+  );
+});
diff --git a/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js
index 2b1b070dc9d..f1a7867be6f 100644
--- a/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js
+++ b/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js
@@ -1295,5 +1295,33 @@ export default {
     extras: {
       visible_group_names: ["alternative-group"]
     }
+  },
+  "/groups/alternative-group/members.json": {
+    owners: [],
+    members: [
+      {
+        id: 2770,
+        username: "awesomerobot",
+        uploaded_avatar_id: 33872,
+        avatar_template:
+          "/user_avatar/meta.discourse.org/awesomerobot/{size}/33872.png",
+        name: "",
+        last_seen_at: "2015-01-23T15:53:17.844Z"
+      },
+      {
+        id: 32,
+        username: "codinghorror",
+        uploaded_avatar_id: 5297,
+        avatar_template:
+          "/user_avatar/meta.discourse.org/codinghorror/{size}/5297.png",
+        name: "Jeff Atwood",
+        last_seen_at: "2015-01-23T06:05:25.457Z"
+      },
+    ],
+    meta: {
+      total: 2,
+      limit: 50,
+      offset: 0
+    }
   }
 };
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index fa44fc75b9d..8fdf6c40caa 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -90,6 +90,8 @@ class Admin::GroupsController < Admin::AdminController
     raise Discourse::NotFound unless group
 
     return can_not_modify_automatic if group.automatic
+    guardian.ensure_can_edit_group!(group)
+
     users = User.where(username: group_params[:usernames].split(","))
 
     users.each do |user|
@@ -117,6 +119,7 @@ class Admin::GroupsController < Admin::AdminController
     raise Discourse::NotFound unless group
 
     return can_not_modify_automatic if group.automatic
+    guardian.ensure_can_edit_group!(group)
 
     user = User.find(params[:user_id].to_i)
     group.group_users.where(user_id: user.id).update_all(owner: false)
diff --git a/config/routes.rb b/config/routes.rb
index 8da5504df0e..021a5b71d71 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -91,7 +91,12 @@ Discourse::Application.routes.draw do
       get "reports/bulk" => "reports#bulk"
       get "reports/:type" => "reports#show"
 
-      resources :groups, only: [:create]
+      resources :groups, only: [:create] do
+        member do
+          put "owners" => "groups#add_owners"
+          delete "owners" => "groups#remove_owner"
+        end
+      end
       resources :groups, except: [:create], constraints: AdminConstraint.new do
         collection do
           get 'bulk'
@@ -99,10 +104,6 @@ Discourse::Application.routes.draw do
           put 'bulk' => 'groups#bulk_perform'
           put "automatic_membership_count" => "groups#automatic_membership_count"
         end
-        member do
-          put "owners" => "groups#add_owners"
-          delete "owners" => "groups#remove_owner"
-        end
       end
 
       get "groups/:type" => "groups#show", constraints: AdminConstraint.new
diff --git a/spec/requests/admin/groups_controller_spec.rb b/spec/requests/admin/groups_controller_spec.rb
index f0d9320b1bd..721a449d763 100644
--- a/spec/requests/admin/groups_controller_spec.rb
+++ b/spec/requests/admin/groups_controller_spec.rb
@@ -282,4 +282,105 @@ RSpec.describe Admin::GroupsController do
       expect(response.parsed_body["user_count"]).to eq(0)
     end
   end
+
+  context "when moderators_manage_categories_and_groups is enabled" do
+    let(:group_params) do
+      {
+        group: {
+          name: 'testing-as-moderator',
+          usernames: [admin.username, user.username].join(","),
+          owner_usernames: [user.username].join(","),
+          allow_membership_requests: true,
+          membership_request_template: 'Testing',
+          members_visibility_level: Group.visibility_levels[:staff]
+        }
+      }
+    end
+
+    before do
+      SiteSetting.moderators_manage_categories_and_groups = true
+    end
+
+    context "the user is a moderator" do
+      before do
+        user.update!(moderator: true)
+        sign_in(user)
+      end
+
+      it 'should allow groups to be created' do
+        post "/admin/groups.json", params: group_params
+
+        expect(response.status).to eq(200)
+
+        group = Group.last
+
+        expect(group.name).to eq('testing-as-moderator')
+        expect(group.users).to contain_exactly(admin, user)
+        expect(group.allow_membership_requests).to eq(true)
+        expect(group.membership_request_template).to eq('Testing')
+        expect(group.members_visibility_level).to eq(Group.visibility_levels[:staff])
+      end
+
+      it 'should allow group owners to be added' do
+        put "/admin/groups/#{group.id}/owners.json", params: {
+          group: {
+            usernames: [user.username, admin.username].join(",")
+          }
+        }
+
+        expect(response.status).to eq(200)
+
+        response_body = response.parsed_body
+
+        expect(response_body["usernames"]).to contain_exactly(user.username, admin.username)
+
+        expect(group.group_users.where(owner: true).map(&:user))
+          .to contain_exactly(user, admin)
+      end
+
+      it 'should allow groups owners to be removed' do
+        group.add_owner(user)
+
+        delete "/admin/groups/#{group.id}/owners.json", params: {
+          user_id: user.id
+        }
+
+        expect(response.status).to eq(200)
+        expect(group.group_users.where(owner: true)).to eq([])
+      end
+    end
+
+    context "the user is not a moderator or admin" do
+      before do
+        user.update!(moderator: false, admin: false)
+        sign_in(user)
+      end
+
+      it 'should not allow groups to be created' do
+        post "/admin/groups.json", params: group_params
+
+        expect(response.status).to eq(404)
+      end
+
+      it 'should not allow group owners to be added' do
+        put "/admin/groups/#{group.id}/owners.json", params: {
+          group: {
+            usernames: [user.username, admin.username].join(",")
+          }
+        }
+
+        expect(response.status).to eq(404)
+      end
+
+      it 'should not allow groups owners to be removed' do
+        group.add_owner(user)
+
+        delete "/admin/groups/#{group.id}/owners.json", params: {
+          user_id: user.id
+        }
+
+        expect(response.status).to eq(404)
+      end
+    end
+  end
 end