Skip to content
Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tgxworld committed Apr 8, 2022
1 parent 07d8189 commit 0f7b987
Show file tree
Hide file tree
Showing 9 changed files with 130 additions and 74 deletions.
Expand Up @@ -38,13 +38,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"
);
Expand Down
4 changes: 4 additions & 0 deletions app/assets/javascripts/discourse/app/models/category.js
Expand Up @@ -515,6 +515,10 @@ Category.reopenClass({
return category;
},

fetchVisibleGroups(id) {
return ajax(`/c/${id}/visible_groups.json`);
},

reloadById(id) {
return ajax(`/c/${id}/show.json`);
},
Expand Down
Expand Up @@ -14,8 +14,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: [],
})
);
});

Expand All @@ -30,6 +38,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"
Expand Down Expand Up @@ -73,8 +82,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"
);
Expand All @@ -85,12 +94,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");

Expand Down
Expand Up @@ -26,6 +26,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"]);
});
Expand Down
11 changes: 9 additions & 2 deletions app/controllers/categories_controller.rb
Expand Up @@ -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]

Expand Down Expand Up @@ -120,6 +120,7 @@ def show
if Category.topic_create_allowed(guardian).where(id: @category.id).exists?
@category.permission = CategoryGroup.permission_types[:full]
end

render_serialized(@category, CategorySerializer)
end

Expand Down Expand Up @@ -252,6 +253,11 @@ def find_by_slug
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
Expand Down Expand Up @@ -371,6 +377,7 @@ def category_params

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
Expand Down
4 changes: 4 additions & 0 deletions app/serializers/category_serializer.rb
Expand Up @@ -53,6 +53,10 @@ def group_permissions
end
end

def include_group_permissions?
scope&.can_edit?(object)
end

def include_available_groups?
scope && scope.can_edit?(object)
end
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Expand Up @@ -722,6 +722,7 @@ def patch(*) end # Disable PATCH requests
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' }
Expand Down
57 changes: 57 additions & 0 deletions spec/requests/categories_controller_spec.rb
Expand Up @@ -733,4 +733,61 @@
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
89 changes: 32 additions & 57 deletions spec/serializers/category_serializer_spec.rb
Expand Up @@ -43,76 +43,51 @@
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

before do
group.update!(name: 'aaa')

category.set_permissions(
:everyone => :readonly,
group.name => :readonly,
user_group.name => :full,
private_group.name => :full,
)

category.save!
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
category.set_permissions(
:everyone => :readonly,
group.name => :readonly,
user_group.name => :full,
private_group.name => :full,
)

expect(json[:group_permissions]).to eq([
{ permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name },
])
end
category.save!
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 "does not include the attribute for an anon user" do
json = described_class.new(category, scope: Guardian.new, 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: user_group.name },
])
end
expect(json[:group_permissions]).to eq(nil)
end

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
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 },
{ 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
expect(json[:group_permissions]).to eq(nil)
end

it "returns the group permissions for everyone group too" do
category.set_permissions(everyone: :readonly)
category.save!
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)

json = described_class.new(category, scope: Guardian.new(admin), root: false).as_json
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: '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

Expand Down

2 comments on commit 0f7b987

@discoursebot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://meta.discourse.org/t/what-does-only-visible-to-members-of-group-tell-me/224144/3

@discoursebot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://meta.discourse.org/t/what-does-only-visible-to-members-of-group-tell-me/224144/7

Please sign in to comment.