SECURITY: Category group permissions leaked to normal users.

After this commit, category group permissions can only be seen by users
that are allowed to manage a category. In the past, we inadvertently
included a category's group permissions settings in `CategoriesController#show`
and `CategoriesController#find_by_slug` endpoints for normal users when
those settings are only a concern to users that can manage a category.

app/assets/javascripts/discourse/app/controllers/share-topic.js

@@ -31,13 +31,12 @@ export default Controller.extend(
         return;
       }
 
-      Category.reloadBySlugPath(this.model.slug).then((result) => {
-        const groups = result.category.group_permissions.mapBy("group_name");
-        if (groups && !groups.any((group) => group === "everyone")) {
+      Category.fetchVisibleGroups(this.model.id).then((result) => {
+        if (result.groups.length > 0) {
           this.flash(
             I18n.t("topic.share.restricted_groups", {
-              count: groups.length,
-              groupNames: groups.join(", "),
+              count: result.groups.length,
+              groupNames: result.groups.join(", "),
             }),
             "warning"
           );

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

@@ -519,6 +519,10 @@ Category.reopenClass({
     return category;
   },
 
+  fetchVisibleGroups(id) {
+    return ajax(`/c/${id}/visible_groups.json`);
+  },
+
   reloadById(id) {
     return ajax(`/c/${id}/show.json`);
   },

app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js

@@ -15,8 +15,16 @@ acceptance("Share and Invite modal", function (needs) {
   needs.user();
 
   needs.pretender((server, helper) => {
-    server.get("/c/feature/find_by_slug.json", () =>
-      helper.response(200, CategoryFixtures["/c/1/show.json"])
+    server.get(`/c/2481/visible_groups.json`, () =>
+      helper.response(200, {
+        groups: ["group_name_1", "group_name_2"],
+      })
+    );
+
+    server.get(`/c/2/visible_groups.json`, () =>
+      helper.response(200, {
+        groups: [],
+      })
     );
   });
 
@@ -31,6 +39,7 @@ acceptance("Share and Invite modal", function (needs) {
     await click("#topic-footer-button-share-and-invite");
 
     assert.ok(exists(".share-topic-modal"), "it shows the modal");
+
     assert.notOk(
       exists("#modal-alert.alert-warning"),
       "it does not show the alert with restricted groups"
@@ -80,8 +89,8 @@ acceptance("Share and Invite modal", function (needs) {
     assert.strictEqual(
       query("#modal-alert.alert-warning").innerText,
       I18n.t("topic.share.restricted_groups", {
-        count: 1,
-        groupNames: "moderators",
+        count: 2,
+        groupNames: "group_name_1, group_name_2",
       }),
       "it shows correct restricted group name"
     );
@@ -92,12 +101,6 @@ acceptance("Share and Invite modal - mobile", function (needs) {
   needs.user();
   needs.mobileView();
 
-  needs.pretender((server, helper) => {
-    server.get("/c/feature/find_by_slug.json", () =>
-      helper.response(200, CategoryFixtures["/c/1/show.json"])
-    );
-  });
-
   test("Topic footer mobile button", async function (assert) {
     await visit("/t/internationalization-localization/280");
 

app/assets/javascripts/discourse/tests/acceptance/topic-test.js

@@ -25,6 +25,12 @@ import CategoryFixtures from "discourse/tests/fixtures/category-fixtures";
 acceptance("Topic", function (needs) {
   needs.user();
   needs.pretender((server, helper) => {
+    server.get("/c/2/visible_groups.json", () =>
+      helper.response(200, {
+        groups: [],
+      })
+    );
+
     server.get("/c/feature/find_by_slug.json", () => {
       return helper.response(200, CategoryFixtures["/c/1/show.json"]);
     });

app/controllers/categories_controller.rb

@@ -2,9 +2,9 @@
 
 class CategoriesController < ApplicationController
 
-  requires_login except: [:index, :categories_and_latest, :categories_and_top, :show, :redirect, :find_by_slug]
+  requires_login except: [:index, :categories_and_latest, :categories_and_top, :show, :redirect, :find_by_slug, :visible_groups]
 
-  before_action :fetch_category, only: [:show, :update, :destroy]
+  before_action :fetch_category, only: [:show, :update, :destroy, :visible_groups]
   before_action :initialize_staff_action_logger, only: [:create, :update, :destroy]
   skip_before_action :check_xhr, only: [:index, :categories_and_latest, :categories_and_top, :redirect]
 
@@ -117,6 +117,7 @@ class CategoriesController < ApplicationController
     if Category.topic_create_allowed(guardian).where(id: @category.id).exists?
       @category.permission = CategoryGroup.permission_types[:full]
     end
+
     render_serialized(@category, CategorySerializer)
   end
 
@@ -249,6 +250,11 @@ class CategoriesController < ApplicationController
     render_serialized(@category, CategorySerializer)
   end
 
+  def visible_groups
+    @guardian.ensure_can_see!(@category)
+    render json: success_json.merge(groups: @category.groups.merge(Group.visible_groups(current_user)).pluck("name"))
+  end
+
   private
 
   def self.topics_per_page
@@ -365,6 +371,7 @@ class CategoriesController < ApplicationController
 
   def fetch_category
     @category = Category.find_by_slug(params[:id]) || Category.find_by(id: params[:id].to_i)
+    raise Discourse::NotFound if @category.blank?
   end
 
   def initialize_staff_action_logger

app/serializers/category_serializer.rb

@@ -53,6 +53,10 @@ class CategorySerializer < SiteCategorySerializer
     end
   end
 
+  def include_group_permissions?
+    scope&.can_edit?(object)
+  end
+
   def include_available_groups?
     scope && scope.can_edit?(object)
   end

config/routes.rb

@@ -717,6 +717,7 @@ Discourse::Application.routes.draw do
     get "categories_and_top" => "categories#categories_and_top"
 
     get "c/:id/show" => "categories#show"
+    get "c/:id/visible_groups" => "categories#visible_groups"
 
     get "c/*category_slug/find_by_slug" => "categories#find_by_slug"
     get "c/*category_slug/edit(/:tab)" => "categories#find_by_slug", constraints: { format: 'html' }

spec/requests/categories_controller_spec.rb

@@ -652,4 +652,61 @@ describe CategoriesController do
       end
     end
   end
+
+  describe '#visible_groups' do
+    fab!(:public_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:public], name: 'aaa') }
+    fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') }
+    fab!(:user_only_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc') }
+
+    it 'responds with 404 when id param is invalid' do
+      get "/c/-9999/visible_groups.json"
+
+      expect(response.status).to eq(404)
+    end
+
+    it "responds with 403 when category is restricted to the current user" do
+      category.set_permissions(private_group.name => :full)
+      category.save!
+
+      get "/c/#{category.id}/visible_groups.json"
+
+      expect(response.status).to eq(403)
+    end
+
+    it "returns the names of the groups that are visible to an admin" do
+      sign_in(admin)
+
+      category.set_permissions(
+        "everyone" => :readonly,
+        private_group.name => :full,
+        public_group.name => :full,
+        user_only_group.name => :full,
+      )
+
+      category.save!
+
+      get "/c/#{category.id}/visible_groups.json"
+
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["groups"]).to eq([public_group.name, private_group.name, user_only_group.name])
+    end
+
+    it "returns the names of the groups that are visible to a user and excludes the everyone group" do
+      sign_in(user)
+
+      category.set_permissions(
+        "everyone" => :readonly,
+        private_group.name => :full,
+        public_group.name => :full,
+        user_only_group.name => :full,
+      )
+
+      category.save!
+
+      get "/c/#{category.id}/visible_groups.json"
+
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["groups"]).to eq([public_group.name])
+    end
+  end
 end

spec/serializers/category_serializer_spec.rb

@@ -45,76 +45,51 @@ describe CategorySerializer do
   end
 
   describe '#group_permissions' do
-    context "category without group permissions configured" do
-      it "returns the right category group permissions for an anon user" do
-        json = described_class.new(category, scope: Guardian.new, root: false).as_json
+    fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') }
 
-        expect(json[:group_permissions]).to eq([
-          { permission_type: CategoryGroup.permission_types[:full], group_name: Group[:everyone]&.name }
-        ])
+    fab!(:user_group) do
+      Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc').tap do |g|
+        g.add(user)
       end
     end
 
-    context "category with group permissions configured" do
-      fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') }
+    before do
+      group.update!(name: 'aaa')
 
-      fab!(:user_group) do
-        Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc').tap do |g|
-          g.add(user)
-        end
-      end
+      category.set_permissions(
+        :everyone => :readonly,
+        group.name => :readonly,
+        user_group.name => :full,
+        private_group.name => :full,
+      )
 
-      before do
-        group.update!(name: 'aaa')
+      category.save!
+    end
 
-        category.set_permissions(
-          :everyone => :readonly,
-          group.name => :readonly,
-          user_group.name => :full,
-          private_group.name => :full,
-        )
+    it "does not include the attribute for an anon user" do
+      json = described_class.new(category, scope: Guardian.new, root: false).as_json
 
-        category.save!
-      end
+      expect(json[:group_permissions]).to eq(nil)
+    end
 
-      it "returns the right category group permissions for an anon user" do
-        json = described_class.new(category, scope: Guardian.new, root: false).as_json
+    it "does not include the attribute for a regular user" do
+      json = described_class.new(category, scope: Guardian.new(user), root: false).as_json
 
-        expect(json[:group_permissions]).to eq([
-          { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name },
-        ])
-      end
+      expect(json[:group_permissions]).to eq(nil)
+    end
 
-      it "returns the right category group permissions for a regular user ordered by ascending group name" do
-        json = described_class.new(category, scope: Guardian.new(user), root: false).as_json
+    it "returns the right category group permissions for a user that can edit the category" do
+      SiteSetting.moderators_manage_categories_and_groups = true
+      user.update!(moderator: true)
 
-        expect(json[:group_permissions]).to eq([
-          { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name },
-          { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name },
-        ])
-      end
+      json = described_class.new(category, scope: Guardian.new(user), root: false).as_json
 
-      it "returns the right category group permission for a staff user ordered by ascending group name" do
-        json = described_class.new(category, scope: Guardian.new(admin), root: false).as_json
-
-        expect(json[:group_permissions]).to eq([
-          { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name },
-          { permission_type: CategoryGroup.permission_types[:full], group_name: private_group.name },
-          { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name },
-          { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' },
-        ])
-      end
-
-      it "returns the group permissions for everyone group too" do
-        category.set_permissions(everyone: :readonly)
-        category.save!
-
-        json = described_class.new(category, scope: Guardian.new(admin), root: false).as_json
-
-        expect(json[:group_permissions]).to eq([
-          { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' },
-        ])
-      end
+      expect(json[:group_permissions]).to eq([
+        { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name },
+        { permission_type: CategoryGroup.permission_types[:full], group_name: private_group.name },
+        { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name },
+        { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' },
+      ])
     end
   end