diff --git a/app/assets/javascripts/discourse/templates/group-index.hbs b/app/assets/javascripts/discourse/templates/group-index.hbs
index 397cf7808e7..a29fc281b6f 100644
--- a/app/assets/javascripts/discourse/templates/group-index.hbs
+++ b/app/assets/javascripts/discourse/templates/group-index.hbs
@@ -1,10 +1,12 @@
- {{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}}
{{#if canManageGroup}}
@@ -76,9 +78,13 @@
{{/load-more}}
{{conditional-loading-spinner condition=loading}}
-{{else}}
+{{else if model.can_see_members}}
{{i18n "groups.empty.members"}}
+{{else}}
+
+
+
{{i18n "groups.members.forbidden"}}
{{/if}}
-
\ No newline at end of file
+
diff --git a/app/assets/javascripts/discourse/templates/group/activity.hbs b/app/assets/javascripts/discourse/templates/group/activity.hbs
index 24c06e20c90..896228f9594 100644
--- a/app/assets/javascripts/discourse/templates/group/activity.hbs
+++ b/app/assets/javascripts/discourse/templates/group/activity.hbs
@@ -1,7 +1,9 @@
{{#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}}
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 371f7a2bc0f..48ea9822b35 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/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,
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 4746262541f..724d9818af5 100644
--- a/app/controllers/groups_controller.rb
+++ b/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,
diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb
index 55c922794ce..69a6c96d6a5 100644
--- a/app/controllers/list_controller.rb
+++ b/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)
diff --git a/app/models/group.rb b/app/models/group.rb
index f4bf49a75c2..7e560dcf2a7 100644
--- a/app/models/group.rb
+++ b/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
#
diff --git a/app/serializers/basic_group_serializer.rb b/app/serializers/basic_group_serializer.rb
index e2dcce3604c..3f1cb66dab8 100644
--- a/app/serializers/basic_group_serializer.rb
+++ b/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?
diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb
index a093c836e9a..0379feede45 100644
--- a/app/serializers/user_serializer.rb
+++ b/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
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 37e196144e7..583e88637c9 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -651,6 +651,7 @@ en:
remove_owner: "Remove as Owner"
remove_owner_description: "Remove %{username} 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
diff --git a/db/migrate/20190812141433_add_members_visibility_level_to_groups.rb b/db/migrate/20190812141433_add_members_visibility_level_to_groups.rb
new file mode 100644
index 00000000000..70fbb7a8082
--- /dev/null
+++ b/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
diff --git a/lib/guardian.rb b/lib/guardian.rb
index ece28e891ed..28e7d369364 100644
--- a/lib/guardian.rb
+++ b/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] }
diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb
index 69bdc1e12ba..926bf310e83 100644
--- a/spec/components/guardian_spec.rb
+++ b/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])
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 68656b14fbd..4739357d192 100644
--- a/spec/models/group_spec.rb
+++ b/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) }
diff --git a/spec/requests/admin/groups_controller_spec.rb b/spec/requests/admin/groups_controller_spec.rb
index c9e94223151..ea7e77bbbda 100644
--- a/spec/requests/admin/groups_controller_spec.rb
+++ b/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
diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb
index 002cd646efd..124eaf5c7d7 100644
--- a/spec/requests/groups_controller_spec.rb
+++ b/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)
diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb
index 0b3c93e73a5..dd943550452 100644
--- a/spec/requests/list_controller_spec.rb
+++ b/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
diff --git a/spec/serializers/basic_group_serializer_spec.rb b/spec/serializers/basic_group_serializer_spec.rb
index eafb7e68027..3dace7fcf44 100644
--- a/spec/serializers/basic_group_serializer_spec.rb
+++ b/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
diff --git a/test/javascripts/fixtures/group-fixtures.js.es6 b/test/javascripts/fixtures/group-fixtures.js.es6
index a3f48f3451f..89842f3245a 100644
--- a/test/javascripts/fixtures/group-fixtures.js.es6
+++ b/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"]