FEATURE - Moderators can create and manage groups (#10432)

Enabling the moderators_manage_categories_and_groups site setting will allow moderator users to create/manage groups.

* show New Group form to moderators

* Allow moderators to update groups and read logs, where appropriate

* Rename site setting from create -> manage

* improved tests

* Migration should rename old log entries

* Log group changes, even if those changes mean you can no longer see the group

* Slight reshuffle

* RouteTo /g if they no longer have permissions to view group

app/assets/javascripts/discourse/app/components/group-manage-save-button.js

@@ -3,6 +3,7 @@ import discourseComputed from "discourse-common/utils/decorators";
 import Component from "@ember/component";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import { popupAutomaticMembershipAlert } from "discourse/controllers/groups-new";
+import DiscourseURL from "discourse/lib/url";
 
 export default Component.extend({
   saving: null,
@@ -24,7 +25,11 @@ export default Component.extend({
 
       return group
         .save()
-        .then(() => {
+        .then(data => {
+          if (data.route_to) {
+            DiscourseURL.routeTo(data.route_to);
+          }
+
           this.set("saved", true);
         })
         .catch(popupAjaxError)

app/assets/javascripts/discourse/app/components/groups-form-interaction-fields.js

@@ -70,5 +70,14 @@ export default Component.extend({
   )
   showEmailSettings(emailIn, automatic, isAdmin) {
     return emailIn && isAdmin && !automatic;
+  },
+
+  @discourseComputed(
+    "model.isCreated",
+    "model.can_admin_group",
+    "currentUser.can_create_group"
+  )
+  canAdminGroup(isCreated, canAdmin, canCreate) {
+    return (!isCreated && canCreate) || (isCreated && canAdmin);
   }
 });

app/assets/javascripts/discourse/app/controllers/group.js

@@ -128,7 +128,7 @@ export default Controller.extend({
     return (
       this.currentUser &&
       (this.currentUser.canManageGroup(model) ||
-        (this.currentUser.admin && automatic))
+        (model.can_admin_group && automatic))
     );
   },
 

app/assets/javascripts/discourse/app/models/user.js

@@ -833,7 +833,7 @@ const User = RestModel.extend({
   canManageGroup(group) {
     return group.get("automatic")
       ? false
-      : this.admin || group.get("is_group_owner");
+      : group.get("can_admin_group") || group.get("is_group_owner");
   },
 
   @discourseComputed("groups.@each.title", "badges.[]")

app/assets/javascripts/discourse/app/routes/group-manage.js

@@ -15,7 +15,7 @@ export default DiscourseRoute.extend({
   afterModel(group) {
     if (
       !this.currentUser ||
-      (!(this.currentUser.admin && group.get("automatic")) &&
+      (!(this.modelFor("group").can_admin_group && group.get("automatic")) &&
         !this.currentUser.canManageGroup(group))
     ) {
       this.transitionTo("group.members", group);

app/assets/javascripts/discourse/app/routes/groups-new.js

@@ -18,7 +18,7 @@ export default DiscourseRoute.extend({
   },
 
   afterModel() {
-    if (!(this.currentUser && this.currentUser.admin)) {
+    if (!this.get("currentUser.can_create_group")) {
       this.transitionTo("groups");
     }
   }

app/assets/javascripts/discourse/app/templates/components/groups-form-interaction-fields.hbs

@@ -1,4 +1,4 @@
-{{#if currentUser.admin}}
+{{#if canAdminGroup}}
   <div class="control-group">
     <label class="control-label">{{i18n "admin.groups.manage.interaction.visibility"}}</label>
     <label for="visiblity">{{i18n "admin.groups.manage.interaction.visibility_levels.title"}}</label>
@@ -62,7 +62,7 @@
   }}
 </div>
 
-{{#if currentUser.admin}}
+{{#if canAdminGroup}}
   <div class="control-group">
     <label>
       {{input type="checkbox"

app/assets/javascripts/discourse/app/templates/components/groups-form-membership-fields.hbs

@@ -1,4 +1,4 @@
-{{#if currentUser.admin}}
+{{#if model.can_admin_group}}
   <div class="control-group">
     <label class="control-label">{{i18n "admin.groups.manage.membership.automatic"}}</label>
 

app/assets/javascripts/discourse/app/templates/components/groups-form-profile-fields.hbs

@@ -1,5 +1,5 @@
 {{#if canEdit}}
-  {{#if this.currentUser.admin}}
+  {{#if this.currentUser.can_create_group}}
     <div class="control-group">
       <label class="control-label" for="name">{{i18n "groups.name"}}</label>
 
@@ -20,7 +20,7 @@
         value=model.full_name}}
   </div>
 
-  {{#if this.currentUser.admin}}
+  {{#if this.currentUser.can_create_group}}
     <div class="control-group">
       <label class="control-label" for="title">
         {{i18n "admin.groups.default_title"}}

app/assets/javascripts/discourse/app/templates/groups/index.hbs

@@ -1,6 +1,6 @@
 {{#d-section pageClass="groups"}}
   <div class="groups-header">
-    {{#if currentUser.admin}}
+    {{#if currentUser.can_create_group}}
       {{d-button
         action=(action "new")
         class="btn-default groups-header-new pull-right"

app/controllers/admin/groups_controller.rb

@@ -37,6 +37,8 @@ class Admin::GroupsController < Admin::AdminController
   end
 
   def create
+    guardian.ensure_can_create_group!
+
     attributes = group_params.to_h.except(:owner_usernames, :usernames)
     group = Group.new(attributes)
 

app/controllers/groups_controller.rb

@@ -140,11 +140,17 @@ class GroupsController < ApplicationController
 
   def update
     group = Group.find(params[:id])
-    guardian.ensure_can_edit!(group) unless current_user.admin
+    guardian.ensure_can_edit!(group) unless guardian.can_admin_group?(group)
 
     if group.update(group_params(automatic: group.automatic))
-      GroupActionLogger.new(current_user, group).log_change_group_settings
+      GroupActionLogger.new(current_user, group, skip_guardian: true).log_change_group_settings
+
+      if guardian.can_see?(group)
         render json: success_json
+      else
+        # They can no longer see the group after changing permissions
+        render json: { route_to: '/g' }
+      end
     else
       render_json_error(group)
     end
@@ -511,7 +517,7 @@ class GroupsController < ApplicationController
 
   def histories
     group = find_group(:group_id)
-    guardian.ensure_can_edit!(group) unless current_user.admin
+    guardian.ensure_can_edit!(group) unless guardian.can_admin_group?(group)
 
     page_size = 25
     offset = (params[:offset] && params[:offset].to_i) || 0
@@ -582,7 +588,7 @@ class GroupsController < ApplicationController
           membership_request_template
         }
 
-        if current_user.admin
+        if current_user.staff?
           default_params.push(*[
             :incoming_email,
             :smtp_server,

app/serializers/basic_group_serializer.rb

@@ -31,6 +31,7 @@ class BasicGroupSerializer < ApplicationSerializer
              :is_group_owner,
              :members_visibility_level,
              :can_see_members,
+             :can_admin_group,
              :publish_read_state
 
   def self.admin_attributes(*attrs)
@@ -115,6 +116,14 @@ class BasicGroupSerializer < ApplicationSerializer
     owner_group_ids.present?
   end
 
+  def can_admin_group
+    scope.can_admin_group?(object)
+  end
+
+  def include_can_admin_group?
+    scope.can_admin_group?(object)
+  end
+
   def is_group_owner
     owner_group_ids.include?(object.id)
   end

app/serializers/current_user_serializer.rb

@@ -38,6 +38,7 @@ class CurrentUserSerializer < BasicUserSerializer
              :seen_notification_id,
              :primary_group_id,
              :can_create_topic,
+             :can_create_group,
              :link_posting_access,
              :external_id,
              :top_category_ids,
@@ -62,6 +63,14 @@ class CurrentUserSerializer < BasicUserSerializer
     scope.can_create_topic?(nil)
   end
 
+  def can_create_group
+    scope.can_create_group?
+  end
+
+  def include_can_create_group?
+    scope.can_create_group?
+  end
+
   def read_faq
     object.user_stat.read_faq?
   end

app/services/group_action_logger.rb

@@ -2,9 +2,10 @@
 
 class GroupActionLogger
 
-  def initialize(acting_user, group)
+  def initialize(acting_user, group, opts = {})
     @acting_user = acting_user
     @group = group
+    @opts = opts
   end
 
   def log_make_user_group_owner(target_user)
@@ -44,7 +45,7 @@ class GroupActionLogger
   end
 
   def log_change_group_settings
-    can_edit?
+    @opts[:skip_guardian] || can_edit?
 
     @group.previous_changes.except(*excluded_attributes).each do |attribute_name, value|
       next if value[0].blank? && value[1].blank?

config/locales/server.en.yml

@@ -1547,7 +1547,7 @@ en:
     ga_universal_auto_link_domains: "Enable Google Universal Analytics (analytics.js) cross-domain tracking. Outgoing links to these domains will have the client id added to them. See <a href='https://support.google.com/analytics/answer/1034342?hl=en' target='_blank'>Google's Cross-Domain Tracking guide.</a>"
     gtm_container_id: "Google Tag Manager container id. eg: GTM-ABCDEF. <br/>Note: Third-party scripts loaded by GTM may need to be allowlisted in 'content security policy script src'."
     enable_escaped_fragments: "Fall back to Google's Ajax-Crawling API if no webcrawler is detected. See <a href='https://developers.google.com/webmasters/ajax-crawling/docs/learn-more' target='_blank'>https://developers.google.com/webmasters/ajax-crawling/docs/learn-more</a>"
-    moderators_create_categories: "Allow moderators to create new categories"
+    moderators_manage_categories_and_groups: "Allow moderators to manage categories and groups"
     cors_origins: "Allowed origins for cross-origin requests (CORS). Each origin must include http:// or https://. The DISCOURSE_ENABLE_CORS env variable must be set to true to enable CORS."
     use_admin_ip_allowlist: "Admins can only log in if they are at an IP address defined in the Screened IPs list (Admin > Logs > Screened Ips)."
     blocked_ip_blocks: "A list of private IP blocks that should never be crawled by Discourse"

config/routes.rb

@@ -92,7 +92,8 @@ Discourse::Application.routes.draw do
       get "reports/bulk" => "reports#bulk"
       get "reports/:type" => "reports#show"
 
-      resources :groups, constraints: AdminConstraint.new do
+      resources :groups, only: [:create]
+      resources :groups, except: [:create], constraints: AdminConstraint.new do
         collection do
           get 'bulk'
           get 'bulk-complete' => 'groups#bulk'
@@ -560,7 +561,7 @@ Discourse::Application.routes.draw do
 
         collection do
           get "check-name" => 'groups#check_name'
-          get 'custom/new' => 'groups#new', constraints: AdminConstraint.new
+          get 'custom/new' => 'groups#new', constraints: StaffConstraint.new
           get "search" => "groups#search"
         end
 

config/site_settings.yml

@@ -1452,7 +1452,7 @@ security:
     regex: "^(Lax|Strict|Disabled|None)$"
   enable_escaped_fragments: true
   allow_index_in_robots_txt: true
-  moderators_create_categories: false
+  moderators_manage_categories_and_groups: false
   moderators_view_emails:
     client: true
     default: false

db/migrate/20200810220841_rename_moderators_create_categories_setting.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class RenameModeratorsCreateCategoriesSetting < ActiveRecord::Migration[6.0]
+  def up
+    execute "UPDATE site_settings SET name = 'moderators_manage_categories_and_groups' WHERE name = 'moderators_create_categories'"
+    execute "UPDATE user_histories SET subject = 'moderators_manage_categories_and_groups' WHERE subject = 'moderators_create_categories'"
+  end
+
+  def down
+    execute "UPDATE site_settings SET name = 'moderators_create_categories' WHERE name = 'moderators_manage_categories_and_groups'"
+    execute "UPDATE user_histories SET subject = 'moderators_create_categories' WHERE subject = 'moderators_manage_categories_and_groups'"
+  end
+end

lib/guardian/category_guardian.rb

@@ -7,7 +7,7 @@ module CategoryGuardian
   def can_create_category?(parent = nil)
     is_admin? ||
     (
-      SiteSetting.moderators_create_categories &&
+      SiteSetting.moderators_manage_categories_and_groups &&
       is_moderator?
     )
   end
@@ -16,7 +16,7 @@ module CategoryGuardian
   def can_edit_category?(category)
     is_admin? ||
     (
-      SiteSetting.moderators_create_categories &&
+      SiteSetting.moderators_manage_categories_and_groups &&
       is_moderator? &&
       can_see_category?(category)
     )

lib/guardian/group_guardian.rb

@@ -3,6 +3,15 @@
 #mixin for all guardian methods dealing with group permissions
 module GroupGuardian
 
+  # Creating Method
+  def can_create_group?
+    is_admin? ||
+    (
+      SiteSetting.moderators_manage_categories_and_groups &&
+      is_moderator?
+    )
+  end
+
   # Edit authority for groups means membership changes only.
   # Automatic groups are not represented in the GROUP_USERS
   # table and thus do not allow membership changes.
@@ -11,7 +20,17 @@ module GroupGuardian
   end
 
   def can_log_group_changes?(group)
-    (is_admin? || group.users.where('group_users.owner').include?(user))
+    can_admin_group?(group) || group.users.where('group_users.owner').include?(user)
+  end
+
+  def can_admin_group?(group)
+    is_admin? ||
+    (
+      SiteSetting.moderators_manage_categories_and_groups &&
+      is_moderator? &&
+      can_see?(group) &&
+      group.id != Group::AUTO_GROUPS[:admins]
+    )
   end
 
   def can_see_group_messages?(group)

lib/site_settings/deprecated_settings.rb

@@ -9,7 +9,8 @@ module SiteSettings::DeprecatedSettings
     ['disable_edit_notifications', 'disable_system_edit_notifications', true, '2.4'],
     ['enable_category_group_review', 'enable_category_group_moderation', true, '2.7'],
     ['newuser_max_images', 'newuser_max_embedded_media', true, '2.7'],
-    ['min_trust_to_post_images', 'min_trust_to_post_embedded_media', true, '2.7']
+    ['min_trust_to_post_images', 'min_trust_to_post_embedded_media', true, '2.7'],
+    ['moderators_create_categories', 'moderators_manage_categories_and_groups', '2.7']
   ]
 
   def setup_deprecated_methods

spec/requests/groups_controller_spec.rb

@@ -829,6 +829,99 @@ describe GroupsController do
       end
     end
 
+    context "when user is a site moderator" do
+      before do
+        SiteSetting.moderators_manage_categories_and_groups = true
+        sign_in(moderator)
+      end
+
+      it 'should not be able to update the group if the SiteSetting is false' do
+        SiteSetting.moderators_manage_categories_and_groups = false
+
+        put "/groups/#{group.id}.json", params: { group: { name: 'testing' } }
+
+        expect(response.status).to eq(403)
+      end
+
+      it 'should not be able to update a group it cannot see' do
+        group.update!(visibility_level: 2)
+
+        put "/groups/#{group.id}.json", params: { group: { name: 'testing' } }
+
+        expect(response.status).to eq(403)
+      end
+
+      it 'should be able to update the group' do
+        put "/groups/#{group.id}.json", params: {
+          group: {
+            flair_color: 'BBB',
+            name: 'testing',
+            incoming_email: 'test@mail.org',
+            primary_group: true,
+            automatic_membership_email_domains: 'test.org',
+            grant_trust_level: 2,
+            visibility_level: 1,
+            members_visibility_level: 3,
+            tracking_category_ids: [category.id],
+            tracking_tags: [tag.name]
+          }
+        }
+
+        expect(response.status).to eq(200)
+
+        group.reload
+        expect(group.flair_color).to eq('BBB')
+        expect(group.name).to eq('testing')
+        expect(group.incoming_email).to eq("test@mail.org")
+        expect(group.primary_group).to eq(true)
+        expect(group.visibility_level).to eq(1)
+        expect(group.members_visibility_level).to eq(3)
+        expect(group.automatic_membership_email_domains).to eq('test.org')
+        expect(group.grant_trust_level).to eq(2)
+        expect(group.group_category_notification_defaults.first&.category).to eq(category)
+        expect(group.group_tag_notification_defaults.first&.tag).to eq(tag)
+
+        expect(Jobs::AutomaticGroupMembership.jobs.first["args"].first["group_id"])
+          .to eq(group.id)
+      end
+
+      it "should be able to update an automatic group" do
+        group = Group.find(Group::AUTO_GROUPS[:trust_level_4])
+
+        group.update!(
+          mentionable_level: 2,
+          messageable_level: 2,
+          default_notification_level: 2
+        )
+
+        put "/groups/#{group.id}.json", params: {
+          group: {
+            mentionable_level: 1,
+            messageable_level: 1,
+            default_notification_level: 1
+          }
+        }
+
+        expect(response.status).to eq(200)
+
+        group.reload
+        expect(group.flair_color).to eq(nil)
+        expect(group.name).to eq('trust_level_4')
+        expect(group.mentionable_level).to eq(1)
+        expect(group.messageable_level).to eq(1)
+        expect(group.default_notification_level).to eq(1)
+      end
+
+      it 'triggers a extensibility event' do
+        event = DiscourseEvent.track_events {
+          put "/groups/#{group.id}.json", params: { group: { flair_color: 'BBB' } }
+        }.last
+
+        expect(event[:event_name]).to eq(:group_updated)
+        expect(event[:params].first).to eq(group)
+      end
+    end
+
     context "when user is not a group owner or admin" do
       it 'should not be able to update the group' do
         sign_in(user)

test/javascripts/acceptance/group-manage-interaction-test.js

@@ -8,7 +8,13 @@ acceptance("Managing Group Interaction Settings", {
 });
 
 QUnit.test("As an admin", async assert => {
-  await visit("/g/discourse/manage/interaction");
+  updateCurrentUser({
+    moderator: false,
+    admin: true,
+    can_create_group: true
+  });
+
+  await visit("/g/alternative-group/manage/interaction");
 
   assert.equal(
     find(".groups-form-visibility-level").length,
@@ -42,13 +48,18 @@ QUnit.test("As an admin", async assert => {
 });
 
 QUnit.test("As a group owner", async assert => {
-  updateCurrentUser({ moderator: false, admin: false });
+  updateCurrentUser({
+    moderator: false,
+    admin: false,
+    can_create_group: false
+  });
+
   await visit("/g/discourse/manage/interaction");
 
   assert.equal(
     find(".groups-form-visibility-level").length,
     0,
-    "it should display visibility level selector"
+    "it should not display visibility level selector"
   );
 
   assert.equal(

test/javascripts/acceptance/group-manage-membership-test.js

@@ -6,7 +6,9 @@ acceptance("Managing Group Membership", {
 });
 
 QUnit.test("As an admin", async assert => {
-  await visit("/g/discourse/manage/membership");
+  updateCurrentUser({ can_create_group: true });
+
+  await visit("/g/alternative-group/manage/membership");
 
   assert.ok(
     find('label[for="automatic_membership"]').length === 1,

test/javascripts/acceptance/group-manage-profile-test.js

@@ -34,7 +34,11 @@ QUnit.test("As an admin", async assert => {
 });
 
 QUnit.test("As a group owner", async assert => {
-  updateCurrentUser({ moderator: false, admin: false });
+  updateCurrentUser({
+    moderator: false,
+    admin: false,
+    can_create_group: false
+  });
 
   await visit("/g/discourse/manage/profile");
 

test/javascripts/fixtures/group-fixtures.js

@@ -1273,5 +1273,27 @@ export default {
           "/user_avatar/meta.discourse.org/codinghorror/{size}/5297.png"
       }
     }
-  ]
+  ],
+  "/groups/alternative-group.json": {
+    group: {
+      id: 57,
+      automatic: false,
+      name: "alternative-group",
+      full_name: "Moderatable Table",
+      user_count: 8,
+      alias_level: 99,
+      visible: true,
+      public_admission: true,
+      public_exit: false,
+      flair_url: "fa-adjust",
+      is_group_owner: true,
+      mentionable: true,
+      messageable: true,
+      can_see_members: true,
+      can_admin_group: true,
+    },
+    extras: {
+      visible_group_names: ["alternative-group"]
+    }
+  }
 };

test/javascripts/fixtures/session-fixtures.js

@@ -14,6 +14,7 @@ export default {
       site_flagged_posts_count: 1,
       moderator: true,
       staff: true,
+      can_create_group: true,
       title: "co-founder",
       reply_count: 859,
       topic_count: 36,

test/javascripts/models/user-test.js

@@ -52,6 +52,7 @@ QUnit.test("canMangeGroup", assert => {
   );
 
   group.set("automatic", false);
+  group.setProperties({ can_admin_group: true });
 
   assert.equal(
     user.canManageGroup(group),