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

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.

diff --git a/app/assets/javascripts/discourse/controllers/group-index.js.es6 b/app/assets/javascripts/discourse/controllers/group-index.js.es6
index 0018563..690997a 100644
--- a/app/assets/javascripts/discourse/controllers/group-index.js.es6
+++ b/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",
diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6
index c25f0ce..5ea8f11 100644
--- a/app/assets/javascripts/discourse/models/group.js.es6
+++ b/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,
diff --git a/app/assets/javascripts/discourse/routes/group-activity-index.js.es6 b/app/assets/javascripts/discourse/routes/group-activity-index.js.es6
index 84011db..4ad23d5 100644
--- a/app/assets/javascripts/discourse/routes/group-activity-index.js.es6
+++ b/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");
+    }
   }
 });
diff --git a/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs b/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs
index b2dc61b..b2f254c 100644
--- a/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs
+++ b/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">
diff --git a/app/assets/javascripts/discourse/templates/group-index.hbs b/app/assets/javascripts/discourse/templates/group-index.hbs
index 397cf78..a29fc28 100644
--- a/app/assets/javascripts/discourse/templates/group-index.hbs
+++ b/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>
\ No newline at end of file
+</section>
diff --git a/app/assets/javascripts/discourse/templates/group/activity.hbs b/app/assets/javascripts/discourse/templates/group/activity.hbs
index 24c06e2..896228f 100644
--- a/app/assets/javascripts/discourse/templates/group/activity.hbs
+++ b/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}}
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 371f7a2..48ea982 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 4746262..724d981 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 55c9227..69a6c96 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 f4bf49a..7e560dc 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

[... diff too long, it was truncated ...]

GitHub sha: 88359b0f

2 Likes

This commit has been mentioned on Discourse Meta. There might be relevant details there:

This is quite the SQL statement to add to the UserSerializer - can you confirm that this does not have any performance issues? I suspect things could be much slower with these 5+ queries each time a user is serialized.

2 Likes

This adds this to current user serialization

Which mean we run this thing on first load.

It certainly is not free and every paper cut counts. It does not fire on subsequent topic visits or topic list visits. It is not a major perf regression.

I am a bit uneasy in the change of semantics here. A group may be visible to current user, only members are not visible.

I would like this particular change reverted and a different approach taken.

  def groups
    object.groups.order(:id)
+     .visible_groups(scope.user)
-     .visible_groups(scope.user).members_visible_groups(scope.user)
  end
1 Like

In every user profile pages it is loading the list of groups where that user have membership. I’m hiding it to not reveal the membership.

Anyway if profile user is equal to scope.user then we should kip it :+1:

No, current user serialization have different visibility_level scope only (which is similar to this one).

2 Likes

Oh I see, site serializer is issuing this:

I am still confused, why does current user serializer need the same list again?

Seems to me like we are doubling up on info here on first load?

As per the above commit groups in current_user_serializer is list of visible groups where the user have membership. But the groups in site_serializer is list of all visible groups in the site.

Ahhh I see why we have the 2 queries.

I don’t think we need to do anymore here.

3 Likes