From 3d581ce159b42577cc2520a34979b013ab1923d2 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 8 Apr 2022 11:14:06 +0800 Subject: [PATCH] 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. --- .../discourse/app/controllers/share-topic.js | 9 +- .../discourse/app/models/category.js | 4 + .../tests/acceptance/share-topic-test.js | 23 ++--- .../discourse/tests/acceptance/topic-test.js | 6 ++ app/controllers/categories_controller.rb | 11 ++- app/serializers/category_serializer.rb | 4 + config/routes.rb | 1 + spec/requests/categories_controller_spec.rb | 57 ++++++++++++ spec/serializers/category_serializer_spec.rb | 89 +++++++------------ 9 files changed, 130 insertions(+), 74 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/share-topic.js b/app/assets/javascripts/discourse/app/controllers/share-topic.js index 3e620fc9ef9..8085fa56480 100644 --- a/app/assets/javascripts/discourse/app/controllers/share-topic.js +++ b/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" ); diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index 1636d8ddb50..8f4ede04c7f 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/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`); }, diff --git a/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js index 16617a45960..ece245b6e46 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js +++ b/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"); diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js index 131b437a2af..a089910ebf8 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js +++ b/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"]); }); diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index cddfb3931d6..36c4ea35de5 100644 --- a/app/controllers/categories_controller.rb +++ b/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 diff --git a/app/serializers/category_serializer.rb b/app/serializers/category_serializer.rb index 7ff76772d4c..b7ab43b5806 100644 --- a/app/serializers/category_serializer.rb +++ b/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 diff --git a/config/routes.rb b/config/routes.rb index 32e5a05c1db..6ee3ba5b4cd 100644 --- a/config/routes.rb +++ b/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' } diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index b9e09cf3bf3..235e1a6030a 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/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 diff --git a/spec/serializers/category_serializer_spec.rb b/spec/serializers/category_serializer_spec.rb index b2cca6993a1..ecd41c7025f 100644 --- a/spec/serializers/category_serializer_spec.rb +++ b/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