FEATURE: add support for group members visibility level (#8004)

There are 5 visibility levels (similar to group visibility)

public (default)
logged-in users
members only
staff
owners

Admins & group owners always have visibility to group members.

app/assets/javascripts/discourse/controllers/group-index.js.es6

@@ -29,7 +29,7 @@ export default Ember.Controller.extend({
     this.set("loading", true);
     const model = this.model;
 
-    if (model) {
+    if (model && model.can_see_members) {
       model.findMembers(this.memberParams).finally(() => {
         this.set(
           "application.showFooter",

app/assets/javascripts/discourse/models/group.js.es6

@@ -162,6 +162,7 @@ const Group = RestModel.extend({
       mentionable_level: this.mentionable_level,
       messageable_level: this.messageable_level,
       visibility_level: this.visibility_level,
+      members_visibility_level: this.members_visibility_level,
       automatic_membership_email_domains: this.emailDomains,
       automatic_membership_retroactive: !!this.automatic_membership_retroactive,
       title: this.title,

app/assets/javascripts/discourse/routes/group-activity-index.js.es6

@@ -1,5 +1,10 @@
 export default Ember.Route.extend({
-  beforeModel: function() {
-    this.transitionTo("group.activity.posts");
+  beforeModel() {
+    const group = this.modelFor("group");
+    if (group.can_see_members) {
+      this.transitionTo("group.activity.posts");
+    } else {
+      this.transitionTo("group.activity.mentions");
+    }
   }
 });

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

@@ -14,6 +14,21 @@
       {{i18n 'admin.groups.manage.interaction.visibility_levels.description'}}
     </div>
   </div>
+
+  <div class="control-group">
+    <label for="visiblity">{{i18n 'admin.groups.manage.interaction.members_visibility_levels.title'}}</label>
+
+    {{combo-box name="alias"
+        valueAttribute="value"
+        value=model.members_visibility_level
+        content=visibilityLevelOptions
+        castInteger=true
+        class="groups-form-members-visibility-level"}}
+
+    <div class="control-instructions">
+      {{i18n 'admin.groups.manage.interaction.members_visibility_levels.description'}}
+    </div>
+  </div>
 {{/if}}
 
 <div class="control-group">

app/assets/javascripts/discourse/templates/group-index.hbs

@@ -1,10 +1,12 @@
 <section class="user-content">
 
 <div class="group-members-actions">
-    {{text-field value=filterInput
-        placeholderKey=filterPlaceholder
-        autocomplete="discourse"
-        class="group-username-filter no-blur"}}
+    {{#if model.can_see_members}}
+      {{text-field value=filterInput
+          placeholderKey=filterPlaceholder
+          autocomplete="discourse"
+          class="group-username-filter no-blur"}}
+    {{/if}}
 
   <div class="group-members-manage">
     {{#if canManageGroup}}
@@ -76,9 +78,13 @@
   {{/load-more}}
 
   {{conditional-loading-spinner condition=loading}}
-{{else}}
+{{else if model.can_see_members}}
   <br>
 
   <div>{{i18n "groups.empty.members"}}</div>
+{{else}}
+  <br>
+
+  <div>{{i18n "groups.members.forbidden"}}</div>
 {{/if}}
-</section>
+</section>

app/assets/javascripts/discourse/templates/group/activity.hbs

@@ -1,7 +1,9 @@
   <section class="user-secondary-navigation">
   {{#mobile-nav class='activity-nav' desktopClass='action-list activity-list nav-stacked' currentPath=router._router.currentPath}}
-    {{group-activity-filter filter="posts" categoryId=category_id}}
-    {{group-activity-filter filter="topics" categoryId=category_id}}
+    {{#if model.can_see_members}}
+      {{group-activity-filter filter="posts" categoryId=category_id}}
+      {{group-activity-filter filter="topics" categoryId=category_id}}
+    {{/if}}
     {{#if siteSettings.enable_mentions}}
       {{group-activity-filter filter="mentions" categoryId=category_id}}
     {{/if}}

app/controllers/admin/groups_controller.rb

@@ -135,6 +135,7 @@ class Admin::GroupsController < Admin::AdminController
       :mentionable_level,
       :messageable_level,
       :visibility_level,
+      :members_visibility_level,
       :automatic_membership_email_domains,
       :automatic_membership_retroactive,
       :title,

app/controllers/groups_controller.rb

@@ -159,6 +159,8 @@ class GroupsController < ApplicationController
 
   def posts
     group = find_group(:group_id)
+    guardian.ensure_can_see_group_members!(group)
+
     posts = group.posts_for(
       guardian,
       params.permit(:before_post_id, :category_id)
@@ -168,6 +170,8 @@ class GroupsController < ApplicationController
 
   def posts_feed
     group = find_group(:group_id)
+    guardian.ensure_can_see_group_members!(group)
+
     @posts = group.posts_for(
       guardian,
       params.permit(:before_post_id, :category_id)
@@ -204,6 +208,8 @@ class GroupsController < ApplicationController
   def members
     group = find_group(:group_id)
 
+    guardian.ensure_can_see_group_members!(group)
+
     limit = (params[:limit] || 20).to_i
     offset = params[:offset].to_i
 
@@ -542,6 +548,7 @@ class GroupsController < ApplicationController
             :incoming_email,
             :primary_group,
             :visibility_level,
+            :members_visibility_level,
             :name,
             :grant_trust_level,
             :automatic_membership_email_domains,

app/controllers/list_controller.rb

@@ -161,6 +161,7 @@ class ListController < ApplicationController
     group = Group.find_by(name: params[:group_name])
     raise Discourse::NotFound unless group
     guardian.ensure_can_see_group!(group)
+    guardian.ensure_can_see_group_members!(group)
 
     list_opts = build_topic_list_options
     list = generate_list_for("group_topics", group, list_opts)

app/models/group.rb

@@ -153,6 +153,59 @@ class Group < ActiveRecord::Base
     groups
   }
 
+  scope :members_visible_groups, Proc.new { |user, order, opts|
+    groups = self.order(order || "name ASC")
+
+    if !opts || !opts[:include_everyone]
+      groups = groups.where("groups.id > 0")
+    end
+
+    unless user&.admin
+      sql = <<~SQL
+        groups.id IN (
+          SELECT g.id FROM groups g WHERE g.members_visibility_level = :public
+
+          UNION ALL
+
+          SELECT g.id FROM groups g
+          WHERE g.members_visibility_level = :logged_on_users AND :user_id IS NOT NULL
+
+          UNION ALL
+
+          SELECT g.id FROM groups g
+          JOIN group_users gu ON gu.group_id = g.id AND
+                                 gu.user_id = :user_id
+          WHERE g.members_visibility_level = :members
+
+          UNION ALL
+
+          SELECT g.id FROM groups g
+          LEFT JOIN group_users gu ON gu.group_id = g.id AND
+                                 gu.user_id = :user_id AND
+                                 gu.owner
+          WHERE g.members_visibility_level = :staff AND (gu.id IS NOT NULL OR :is_staff)
+
+          UNION ALL
+
+          SELECT g.id FROM groups g
+          JOIN group_users gu ON gu.group_id = g.id AND
+                                 gu.user_id = :user_id AND
+                                 gu.owner
+          WHERE g.members_visibility_level = :owners
+
+        )
+      SQL
+
+      groups = groups.where(
+        sql,
+        Group.visibility_levels.to_h.merge(user_id: user&.id, is_staff: !!user&.staff?)
+      )
+
+    end
+
+    groups
+  }
+
   scope :mentionable, lambda { |user|
     where(self.mentionable_sql_clause,
       levels: alias_levels(user),
@@ -828,6 +881,7 @@ end
 #  membership_request_template        :text
 #  messageable_level                  :integer          default(0)
 #  mentionable_level                  :integer          default(0)
+#  members_visibility_level           :integer          default(0), not null
 #
 # Indexes
 #

app/serializers/basic_group_serializer.rb

@@ -29,7 +29,9 @@ class BasicGroupSerializer < ApplicationSerializer
              :default_notification_level,
              :membership_request_template,
              :is_group_user,
-             :is_group_owner
+             :is_group_owner,
+             :members_visibility_level,
+             :can_see_members
 
   def include_display_name?
     object.automatic
@@ -81,6 +83,10 @@ class BasicGroupSerializer < ApplicationSerializer
     owner_group_ids.include?(object.id)
   end
 
+  def can_see_members
+    scope.can_see_group_members?(object)
+  end
+
   private
 
   def staff?

app/serializers/user_serializer.rb

@@ -143,7 +143,7 @@ class UserSerializer < BasicUserSerializer
 
   def groups
     object.groups.order(:id)
-      .visible_groups(scope.user)
+      .visible_groups(scope.user).members_visible_groups(scope.user)
   end
 
   def group_users

config/locales/client.en.yml

@@ -651,6 +651,7 @@ en:
         remove_owner: "Remove as Owner"
         remove_owner_description: "Remove <b>%{username}</b> as an owner of this group"
         owner: "Owner"
+        forbidden: "You're not allowed to view the members."
       topics: "Topics"
       posts: "Posts"
       mentions: "Mentions"
@@ -3211,6 +3212,9 @@ en:
               staff: "Group owners and staff"
               owners: "Group owners"
               description: "Admins can see all groups."
+            members_visibility_levels:
+              title: "Who can see this group members?"
+              description: "Admins can see members of all groups."
 
           membership:
             automatic: Automatic

db/migrate/20190812141433_add_members_visibility_level_to_groups.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddMembersVisibilityLevelToGroups < ActiveRecord::Migration[5.2]
+  def change
+    add_column :groups, :members_visibility_level, :integer, default: 0, null: false
+  end
+end

lib/guardian.rb

@@ -209,6 +209,26 @@ class Guardian
     true
   end
 
+  def can_see_group_members?(group)
+    return false if group.blank?
+    return true if group.members_visibility_level == Group.visibility_levels[:public]
+    return true if is_admin?
+    return true if is_staff? && group.members_visibility_level == Group.visibility_levels[:staff]
+    return true if authenticated? && group.members_visibility_level == Group.visibility_levels[:logged_on_users]
+    return false if user.blank?
+
+    membership = GroupUser.find_by(group_id: group.id, user_id: user.id)
+
+    return false unless membership
+
+    if !membership.owner
+      return false if group.members_visibility_level == Group.visibility_levels[:owners]
+      return false if group.members_visibility_level == Group.visibility_levels[:staff]
+    end
+
+    true
+  end
+
   def can_see_groups?(groups)
     return false if groups.blank?
     return true if groups.all? { |g| g.visibility_level == Group.visibility_levels[:public] }

spec/components/guardian_spec.rb

@@ -3048,6 +3048,90 @@ describe Guardian do
 
   end
 
+  describe(:can_see_group_members) do
+    it 'Correctly handles group members visibility for owner' do
+      group = Group.new(name: 'group', members_visibility_level: Group.visibility_levels[:owners])
+
+      member = Fabricate(:user)
+      group.add(member)
+      group.save!
+
+      owner = Fabricate(:user)
+      group.add_owner(owner)
+      group.reload
+
+      expect(Guardian.new(admin).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new(another_user).can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new(moderator).can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new(member).can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new.can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new(owner).can_see_group_members?(group)).to eq(true)
+    end
+
+    it 'Correctly handles group members visibility for staff' do
+      group = Group.new(name: 'group', members_visibility_level: Group.visibility_levels[:staff])
+
+      member = Fabricate(:user)
+      group.add(member)
+      group.save!
+
+      owner = Fabricate(:user)
+      group.add_owner(owner)
+      group.reload
+
+      expect(Guardian.new(another_user).can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new(member).can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new(admin).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new(moderator).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new(owner).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new.can_see_group_members?(group)).to eq(false)
+    end
+
+    it 'Correctly handles group members visibility for member' do
+      group = Group.new(name: 'group', members_visibility_level: Group.visibility_levels[:members])
+
+      member = Fabricate(:user)
+      group.add(member)
+      group.save!
+
+      owner = Fabricate(:user)
+      group.add_owner(owner)
+      group.reload
+
+      expect(Guardian.new(moderator).can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new.can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new(another_user).can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new(admin).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new(member).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new(owner).can_see_group_members?(group)).to eq(true)
+    end
+
+    it 'Correctly handles group members visibility for logged-on-user' do
+      group = Group.new(name: 'group', members_visibility_level: Group.visibility_levels[:logged_on_users])
+      member = Fabricate(:user)
+      group.add(member)
+      group.save!
+
+      owner = Fabricate(:user)
+      group.add_owner(owner)
+      group.reload
+
+      expect(Guardian.new.can_see_group_members?(group)).to eq(false)
+      expect(Guardian.new(moderator).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new(admin).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new(member).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new(owner).can_see_group_members?(group)).to eq(true)
+      expect(Guardian.new(another_user).can_see_group_members?(group)).to eq(true)
+    end
+
+    it 'Correctly handles group members visibility for public' do
+      group = Group.new(name: 'group', members_visibility_level: Group.visibility_levels[:public])
+
+      expect(Guardian.new.can_see_group_members?(group)).to eq(true)
+    end
+
+  end
+
   describe '#can_see_groups?' do
     it 'correctly handles owner visible groups' do
       group = Group.new(name: 'group', visibility_level: Group.visibility_levels[:owners])

spec/models/group_spec.rb

@@ -716,6 +716,71 @@ describe Group do
 
   end
 
+  describe ".members_visible_groups" do
+
+    def can_view?(user, group)
+      Group.members_visible_groups(user).exists?(id: group.id)
+    end
+
+    it 'correctly restricts group members visibility' do
+      group = Fabricate.build(:group, members_visibility_level: Group.visibility_levels[:owners])
+      logged_on_user = Fabricate(:user)
+      member = Fabricate(:user)
+      group.add(member)
+      group.save!
+
+      owner = Fabricate(:user)
+      group.add_owner(owner)
+
+      moderator = Fabricate(:user, moderator: true)
+      admin = Fabricate(:user, admin: true)
+
+      expect(can_view?(admin, group)).to eq(true)
+      expect(can_view?(owner, group)).to eq(true)
+      expect(can_view?(moderator, group)).to eq(false)
+      expect(can_view?(member, group)).to eq(false)
+      expect(can_view?(logged_on_user, group)).to eq(false)
+      expect(can_view?(nil, group)).to eq(false)
+
+      group.update_columns(members_visibility_level: Group.visibility_levels[:staff])
+
+      expect(can_view?(admin, group)).to eq(true)
+      expect(can_view?(owner, group)).to eq(true)
+      expect(can_view?(moderator, group)).to eq(true)
+      expect(can_view?(member, group)).to eq(false)
+      expect(can_view?(logged_on_user, group)).to eq(false)
+      expect(can_view?(nil, group)).to eq(false)
+
+      group.update_columns(members_visibility_level: Group.visibility_levels[:members])
+
+      expect(can_view?(admin, group)).to eq(true)
+      expect(can_view?(owner, group)).to eq(true)
+      expect(can_view?(moderator, group)).to eq(false)
+      expect(can_view?(member, group)).to eq(true)
+      expect(can_view?(logged_on_user, group)).to eq(false)
+      expect(can_view?(nil, group)).to eq(false)
+
+      group.update_columns(members_visibility_level: Group.visibility_levels[:public])
+
+      expect(can_view?(admin, group)).to eq(true)
+      expect(can_view?(owner, group)).to eq(true)
+      expect(can_view?(moderator, group)).to eq(true)
+      expect(can_view?(member, group)).to eq(true)
+      expect(can_view?(logged_on_user, group)).to eq(true)
+      expect(can_view?(nil, group)).to eq(true)
+
+      group.update_columns(members_visibility_level: Group.visibility_levels[:logged_on_users])
+
+      expect(can_view?(admin, group)).to eq(true)
+      expect(can_view?(owner, group)).to eq(true)
+      expect(can_view?(moderator, group)).to eq(true)
+      expect(can_view?(member, group)).to eq(true)
+      expect(can_view?(logged_on_user, group)).to eq(true)
+      expect(can_view?(nil, group)).to eq(false)
+    end
+
+  end
+
   describe '#remove' do
     before { group.add(user) }
 

spec/requests/admin/groups_controller_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe Admin::GroupsController do
           owner_usernames: [user.username].join(","),
           allow_membership_requests: true,
           membership_request_template: 'Testing',
+          members_visibility_level: Group.visibility_levels[:staff]
         }
       }
     end
@@ -35,6 +36,7 @@ RSpec.describe Admin::GroupsController do
       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
 
     context "custom_fields" do

spec/requests/groups_controller_spec.rb

@@ -335,6 +335,15 @@ describe GroupsController do
       expect(response.status).to eq(403)
     end
 
+    it "ensures the group members can be seen" do
+      sign_in(Fabricate(:user))
+      group.update!(members_visibility_level: Group.visibility_levels[:owners])
+
+      get "/groups/#{group.name}/posts.json"
+
+      expect(response.status).to eq(403)
+    end
+
     it "calls `posts_for` and responds with JSON" do
       sign_in(user)
       post = Fabricate(:post, user: user)
@@ -369,6 +378,14 @@ describe GroupsController do
       expect(response.status).to eq(403)
     end
 
+    it "ensures the group members can be seen" do
+      group.update!(members_visibility_level: Group.visibility_levels[:logged_on_users])
+
+      get "/groups/#{group.name}/members.json", params: { limit: 1 }
+
+      expect(response.status).to eq(403)
+    end
+
     it "ensures that membership can be paginated" do
 
       freeze_time
@@ -613,6 +630,7 @@ describe GroupsController do
       it 'should be able to update the group' do
         group.update!(
           visibility_level: 2,
+          members_visibility_level: 2,
           automatic_membership_retroactive: false,
           grant_trust_level: 0
         )
@@ -626,7 +644,8 @@ describe GroupsController do
             automatic_membership_email_domains: 'test.org',
             automatic_membership_retroactive: true,
             grant_trust_level: 2,
-            visibility_level: 1
+            visibility_level: 1,
+            members_visibility_level: 3
           }
         }
 
@@ -638,6 +657,7 @@ describe GroupsController do
         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.automatic_membership_retroactive).to eq(true)
         expect(group.grant_trust_level).to eq(2)

spec/requests/list_controller_spec.rb

@@ -261,6 +261,16 @@ RSpec.describe ListController do
           expect(response.status).to eq(403)
         end
       end
+
+      describe 'group members visibility restricted to logged-on-users' do
+        before { group.update!(members_visibility_level: Group.visibility_levels[:logged_on_users]) }
+
+        it 'should return the right response' do
+          get "/topics/groups/#{group.name}.json"
+
+          expect(response.status).to eq(403)
+        end
+      end
     end
 
     describe 'for a normal user' do

spec/serializers/basic_group_serializer_spec.rb

@@ -89,4 +89,29 @@ describe BasicGroupSerializer do
       end
     end
   end
+
+  describe '#can_see_members' do
+    fab!(:group) { Fabricate(:group, members_visibility_level: Group.visibility_levels[:members]) }
+
+    describe 'for a group user' do
+      fab!(:user) { Fabricate(:user) }
+      let(:guardian) { Guardian.new(user) }
+
+      before do
+        group.add(user)
+      end
+
+      it 'should be true' do
+        expect(subject.as_json[:can_see_members]).to eq(true)
+      end
+    end
+
+    describe 'for a normal user' do
+      let(:guardian) { Guardian.new(Fabricate(:user)) }
+
+      it 'should be false' do
+        expect(subject.as_json[:can_see_members]).to eq(false)
+      end
+    end
+  end
 end

test/javascripts/fixtures/group-fixtures.js.es6

@@ -46,7 +46,8 @@ export default {
       flair_url: "fa-adjust",
       is_group_owner: true,
       mentionable: true,
-      messageable: true
+      messageable: true,
+      can_see_members: true
     },
     extras: {
       visible_group_names: ["discourse"]