Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SECURITY: Default tags to show count of topics in unrestricted categories (#19916) 

Currently, `Tag#topic_count` is a count of all regular topics regardless of whether the topic is in a read restricted category or not. As a result, any users can technically poll a sensitive tag to determine if a new topic is created in a category which the user has not excess to. We classify this as a minor leak in sensitive information.

The following changes are introduced in this commit:

1. Introduce `Tag#public_topic_count` which only count topics which have been tagged with a given tag in public categories.
2. Rename `Tag#topic_count` to `Tag#staff_topic_count` which counts the same way as `Tag#topic_count`. In other words, it counts all topics tagged with a given tag regardless of the category the topic is in. The rename is also done so that we indicate that this column contains sensitive information. 
3. Change all previous spots which relied on `Topic#topic_count` to rely on `Tag.topic_column_count(guardian)` which will return the right "topic count" column to use based on the current scope. 
4. Introduce `SiteSetting.include_secure_categories_in_tag_counts` site setting to allow site administrators to always display the tag topics count using `Tag#staff_topic_count` instead.
This commit is contained in:
Alan Guo Xiang Tan 2023-01-20 09:50:24 +08:00 committed by GitHub
parent 4d2a95ffe6
commit f122f24b35
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
28 changed files with 602 additions and 127 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
app/controllers/tags_controller.rb
View File

@ -40,7 +40,7 @@ class TagsController < ::ApplicationController
if SiteSetting.tags_listed_by_group
ungrouped_tags = Tag.where("tags.id NOT IN (SELECT tag_id FROM tag_group_memberships)")
ungrouped_tags = ungrouped_tags.where("tags.topic_count > 0") unless show_all_tags
ungrouped_tags = ungrouped_tags.used_tags_in_regular_topics(guardian) unless show_all_tags
grouped_tag_counts =
TagGroup
@ -51,18 +51,14 @@ class TagsController < ::ApplicationController
{
id: tag_group.id,
name: tag_group.name,
tags:
self.class.tag_counts_json(
tag_group.none_synonym_tags,
show_pm_tags: guardian.can_tag_pms?,
),
tags: self.class.tag_counts_json(tag_group.none_synonym_tags, guardian),
}
end
@tags = self.class.tag_counts_json(ungrouped_tags, show_pm_tags: guardian.can_tag_pms?)
@tags = self.class.tag_counts_json(ungrouped_tags, guardian)
@extras = { tag_groups: grouped_tag_counts }
else
tags = show_all_tags ? Tag.all : Tag.where("tags.topic_count > 0")
tags = show_all_tags ? Tag.all : Tag.used_tags_in_regular_topics(guardian)
unrestricted_tags = DiscourseTagging.filter_visible(tags.where(target_tag_id: nil), guardian)
categories =
@ -77,13 +73,14 @@ class TagsController < ::ApplicationController
category_tags =
self.class.tag_counts_json(
DiscourseTagging.filter_visible(c.tags.where(target_tag_id: nil), guardian),
guardian,
)
next if category_tags.empty?
{ id: c.id, tags: category_tags }
end
.compact
@tags = self.class.tag_counts_json(unrestricted_tags, show_pm_tags: guardian.can_tag_pms?)
@tags = self.class.tag_counts_json(unrestricted_tags, guardian)
@extras = { categories: category_tag_counts }
end
@ -264,7 +261,7 @@ class TagsController < ::ApplicationController
tags_with_counts, filter_result_context =
DiscourseTagging.filter_allowed_tags(guardian, **filter_params, with_context: true)
tags = self.class.tag_counts_json(tags_with_counts, show_pm_tags: guardian.can_tag_pms?)
tags = self.class.tag_counts_json(tags_with_counts, guardian)
json_response = { results: tags }
@ -388,18 +385,22 @@ class TagsController < ::ApplicationController
end
end
def self.tag_counts_json(tags, show_pm_tags: true)
def self.tag_counts_json(tags, guardian)
show_pm_tags = guardian.can_tag_pms?
target_tags = Tag.where(id: tags.map(&:target_tag_id).compact.uniq).select(:id, :name)
tags
.map do |t|
next if t.topic_count == 0 && t.pm_topic_count > 0 && !show_pm_tags
topic_count = t.public_send(Tag.topic_count_column(guardian))
next if topic_count == 0 && t.pm_topic_count > 0 && !show_pm_tags
{
id: t.name,
text: t.name,
name: t.name,
description: t.description,
count: t.topic_count,
count: topic_count,
pm_count: show_pm_tags ? t.pm_topic_count : 0,
target_tag:
t.target_tag_id ? target_tags.find { |x| x.id == t.target_tag_id }&.name : nil,

69
app/models/tag.rb
View File

@ -4,6 +4,10 @@ class Tag < ActiveRecord::Base
include Searchable
include HasDestroyedWebHook
self.ignored_columns = [
"topic_count", # TODO(tgxworld): Remove on 1 July 2023
]
RESERVED_TAGS = [
"none",
"constructor", # prevents issues with javascript's constructor of objects
@ -25,11 +29,14 @@ class Tag < ActiveRecord::Base
# tags that have never been used and don't belong to a tag group
scope :unused,
-> {
where(topic_count: 0, pm_topic_count: 0).joins(
where(staff_topic_count: 0, pm_topic_count: 0).joins(
"LEFT JOIN tag_group_memberships tgm ON tags.id = tgm.tag_id",
).where("tgm.tag_id IS NULL")
}
scope :used_tags_in_regular_topics,
->(guardian) { where("tags.#{Tag.topic_count_column(guardian)} > 0") }
scope :base_tags, -> { where(target_tag_id: nil) }
has_many :tag_users, dependent: :destroy # notification settings
@ -62,7 +69,7 @@ class Tag < ActiveRecord::Base
def self.update_topic_counts
DB.exec <<~SQL
UPDATE tags t
SET topic_count = x.topic_count
SET staff_topic_count = x.topic_count
FROM (
SELECT COUNT(topics.id) AS topic_count, tags.id AS tag_id
FROM tags
@ -73,7 +80,31 @@ class Tag < ActiveRecord::Base
GROUP BY tags.id
) x
WHERE x.tag_id = t.id
AND x.topic_count <> t.topic_count
AND x.topic_count <> t.staff_topic_count
SQL
DB.exec <<~SQL
UPDATE tags t
SET public_topic_count = x.topic_count
FROM (
WITH tags_with_public_topics AS (
SELECT
COUNT(topics.id) AS topic_count,
tags.id AS tag_id
FROM tags
INNER JOIN topic_tags ON tags.id = topic_tags.tag_id
INNER JOIN topics ON topics.id = topic_tags.topic_id AND topics.deleted_at IS NULL AND topics.archetype != 'private_message'
INNER JOIN categories ON categories.id = topics.category_id AND NOT categories.read_restricted
GROUP BY tags.id
)
SELECT
COALESCE(tags_with_public_topics.topic_count, 0 ) AS topic_count,
tags.id AS tag_id
FROM tags
LEFT JOIN tags_with_public_topics ON tags_with_public_topics.tag_id = tags.id
) x
WHERE x.tag_id = t.id
AND x.topic_count <> t.public_topic_count;
SQL
DB.exec <<~SQL
@ -97,19 +128,18 @@ class Tag < ActiveRecord::Base
self.find_by("lower(name) = ?", name.downcase)
end
def self.top_tags(limit_arg: nil, category: nil, guardian: nil)
def self.top_tags(limit_arg: nil, category: nil, guardian: Guardian.new)
# we add 1 to max_tags_in_filter_list to efficiently know we have more tags
# than the limit. Frontend is responsible to enforce limit.
limit = limit_arg || (SiteSetting.max_tags_in_filter_list + 1)
scope_category_ids = (guardian || Guardian.new).allowed_category_ids
scope_category_ids = guardian.allowed_category_ids
scope_category_ids &= ([category.id] + category.subcategories.pluck(:id)) if category
return [] if scope_category_ids.empty?
filter_sql =
(
if guardian&.is_staff?
if guardian.is_staff?
""
else
" AND tags.id IN (#{DiscourseTagging.visible_tags(guardian).select(:id).to_sql})"
@ -130,6 +160,14 @@ class Tag < ActiveRecord::Base
tag_names_with_counts.map { |row| row.tag_name }
end
def self.topic_count_column(guardian)
if guardian&.is_staff? || SiteSetting.include_secure_categories_in_tag_counts
"staff_topic_count"
else
"public_topic_count"
end
end
def self.pm_tags(limit: 1000, guardian: nil, allowed_user: nil)
return [] if allowed_user.blank? || !(guardian || Guardian.new).can_tag_pms?
user_id = allowed_user.id
@ -214,14 +252,15 @@ end
#
# Table name: tags
#
# id :integer not null, primary key
# name :string not null
# topic_count :integer default(0), not null
# created_at :datetime not null
# updated_at :datetime not null
# pm_topic_count :integer default(0), not null
# target_tag_id :integer
# description :string
# id :integer not null, primary key
# name :string not null
# created_at :datetime not null
# updated_at :datetime not null
# pm_topic_count :integer default(0), not null
# target_tag_id :integer
# description :string
# public_topic_count :integer default(0), not null
# staff_topic_count :integer default(0), not null
#
# Indexes
#

13
app/models/topic.rb
View File

@ -958,6 +958,7 @@ class Topic < ActiveRecord::Base
def changed_to_category(new_category)
return true if new_category.blank? || Category.exists?(topic_id: id)
if new_category.id == SiteSetting.uncategorized_category_id &&
!SiteSetting.allow_uncategorized_topics
return false
@ -971,6 +972,15 @@ class Topic < ActiveRecord::Base
if old_category
Category.where(id: old_category.id).update_all("topic_count = topic_count - 1")
count =
if old_category.read_restricted && !new_category.read_restricted
1
elsif !old_category.read_restricted && new_category.read_restricted
-1
end
Tag.update_counters(self.tags, { public_topic_count: count }) if count
end
# when a topic changes category we may have to start watching it
@ -1781,12 +1791,15 @@ class Topic < ActiveRecord::Base
def convert_to_public_topic(user, category_id: nil)
public_topic = TopicConverter.new(self, user).convert_to_public_topic(category_id)
Tag.update_counters(public_topic.tags, { public_topic_count: 1 }) if !category.read_restricted
add_small_action(user, "public_topic") if public_topic
public_topic
end
def convert_to_private_message(user)
read_restricted = category.read_restricted
private_topic = TopicConverter.new(self, user).convert_to_private_message
Tag.update_counters(private_topic.tags, { public_topic_count: -1 }) if !read_restricted
add_small_action(user, "private_topic") if private_topic
private_topic
end

31
app/models/topic_tag.rb
View File

@ -9,14 +9,18 @@ class TopicTag < ActiveRecord::Base
if topic.archetype == Archetype.private_message
tag.increment!(:pm_topic_count)
else
tag.increment!(:topic_count)
counters_to_update = { staff_topic_count: 1 }
if topic.category_id
if stat = CategoryTagStat.find_by(tag_id: tag_id, category_id: topic.category_id)
stat.increment!(:topic_count)
else
CategoryTagStat.create(tag_id: tag_id, category_id: topic.category_id, topic_count: 1)
end
if Category.exists?(id: topic.category_id, read_restricted: false)
counters_to_update[:public_topic_count] = 1
end
Tag.update_counters(tag.id, counters_to_update)
if stat = CategoryTagStat.find_by(tag_id: tag_id, category_id: topic.category_id)
stat.increment!(:topic_count)
else
CategoryTagStat.create!(tag_id: tag_id, category_id: topic.category_id, topic_count: 1)
end
end
end
@ -27,12 +31,17 @@ class TopicTag < ActiveRecord::Base
if topic.archetype == Archetype.private_message
tag.decrement!(:pm_topic_count)
else
if topic.category_id &&
stat = CategoryTagStat.find_by(tag_id: tag_id, category: topic.category_id)
stat.topic_count == 1 ? stat.destroy : stat.decrement!(:topic_count)
if stat = CategoryTagStat.find_by(tag_id: tag_id, category: topic.category_id)
stat.topic_count == 1 ? stat.destroy! : stat.decrement!(:topic_count)
end
tag.decrement!(:topic_count)
counters_to_update = { staff_topic_count: -1 }
if Category.exists?(id: topic.category_id, read_restricted: false)
counters_to_update[:public_topic_count] = -1
end
Tag.update_counters(tag.id, counters_to_update)
end
end
end

3
app/serializers/concerns/topic_tags_mixin.rb
View File

@ -32,7 +32,8 @@ module TopicTagsMixin
if SiteSetting.tags_sort_alphabetically
topic.tags.sort_by(&:name)
else
topic.tags.sort_by(&:topic_count).reverse
topic_count_column = Tag.topic_count_column(scope)
topic.tags.sort_by { |tag| tag.public_send(topic_count_column) }.reverse
end
)
tags = tags.reject { |tag| scope.hidden_tag_names.include?(tag[:name]) } if !scope.is_staff?

4
app/serializers/concerns/user_sidebar_mixin.rb
View File

@ -2,9 +2,11 @@
module UserSidebarMixin
def sidebar_tags
topic_count_column = Tag.topic_count_column(scope)
object
.visible_sidebar_tags(scope)
.pluck(:name, :topic_count, :pm_topic_count)
.pluck(:name, topic_count_column, :pm_topic_count)
.reduce([]) do |tags, sidebar_tag|
tags.push(name: sidebar_tag[0], pm_only: sidebar_tag[1] == 0 && sidebar_tag[2] > 0)
end

2
app/serializers/detailed_tag_serializer.rb
View File

@ -6,7 +6,7 @@ class DetailedTagSerializer < TagSerializer
has_many :categories, serializer: BasicCategorySerializer
def synonyms
TagsController.tag_counts_json(object.synonyms)
TagsController.tag_counts_json(object.synonyms, scope)
end
def categories

4
app/serializers/tag_serializer.rb
View File

@ -3,6 +3,10 @@
class TagSerializer < ApplicationSerializer
attributes :id, :name, :topic_count, :staff, :description
def topic_count
object.public_send(Tag.topic_count_column(scope))
end
def staff
DiscourseTagging.staff_tag_names.include?(name)
end

24
app/services/tag_hashtag_data_source.rb
View File

@ -12,26 +12,30 @@ class TagHashtagDataSource
"tag"
end
def self.tag_to_hashtag_item(tag)
tag = Tag.new(tag.slice(:id, :name, :description).merge(topic_count: tag[:count])) if tag.is_a?(
Hash,
)
def self.tag_to_hashtag_item(tag, guardian)
topic_count_column = Tag.topic_count_column(guardian)
tag =
Tag.new(
tag.slice(:id, :name, :description).merge(topic_count_column => tag[:count]),
) if tag.is_a?(Hash)
HashtagAutocompleteService::HashtagItem.new.tap do |item|
item.text = tag.name
item.secondary_text = "x#{tag.topic_count}"
item.secondary_text = "x#{tag.public_send(topic_count_column)}"
item.description = tag.description
item.slug = tag.name
item.relative_url = tag.url
item.icon = icon
end
end
private_class_method :tag_to_hashtag_item
def self.lookup(guardian, slugs)
return [] if !SiteSetting.tagging_enabled
DiscourseTagging
.filter_visible(Tag.where_name(slugs), guardian)
.map { |tag| tag_to_hashtag_item(tag) }
.map { |tag| tag_to_hashtag_item(tag, guardian) }
end
def self.search(
@ -60,9 +64,9 @@ class TagHashtagDataSource
)
TagsController
.tag_counts_json(tags_with_counts)
.tag_counts_json(tags_with_counts, guardian)
.take(limit)
.map { |tag| tag_to_hashtag_item(tag) }
.map { |tag| tag_to_hashtag_item(tag, guardian) }
end
def self.search_sort(search_results, _)
@ -82,8 +86,8 @@ class TagHashtagDataSource
)
TagsController
.tag_counts_json(tags_with_counts)
.tag_counts_json(tags_with_counts, guardian)
.take(limit)
.map { |tag| tag_to_hashtag_item(tag) }
.map { |tag| tag_to_hashtag_item(tag, guardian) }
end
end

1
config/locales/server.en.yml
View File

@ -1651,6 +1651,7 @@ en:
content_security_policy_frame_ancestors: "Restrict who can embed this site in iframes via CSP. Control allowed hosts on <a href='%{base_path}/admin/customize/embedding'>Embedding</a>"
content_security_policy_script_src: "Additional allowlisted script sources. The current host and CDN are included by default. See <a href='https://meta.discourse.org/t/mitigate-xss-attacks-with-content-security-policy/104243' target='_blank'>Mitigate XSS Attacks with Content Security Policy.</a>"
invalidate_inactive_admin_email_after_days: "Admin accounts that have not visited the site in this number of days will need to re-validate their email address before logging in. Set to 0 to disable."
include_secure_categories_in_tag_counts: "When enabled, count of topics for a tag will include topics that are in read restricted categories for all users. When disabled, normal users are only shown a count of topics for a tag where all the topics are in public categories."
top_menu: "Determine which items appear in the homepage navigation, and in what order. Example latest|new|unread|categories|top|read|posted|bookmarks"
post_menu: "Determine which items appear on the post menu, and in what order. Example like|edit|flag|delete|share|bookmark|reply"
post_menu_hidden_items: "The menu items to hide by default in the post menu unless an expansion ellipsis is clicked on."

2
config/site_settings.yml
View File

@ -1798,6 +1798,8 @@ security:
suppress_secured_categories_from_admin:
default: false
hidden: true
include_secure_categories_in_tag_counts:
default: false
onebox:
post_onebox_maxlength:

28
db/migrate/20230118020114_add_public_topic_count_to_tags.rb Normal file
View File

@ -0,0 +1,28 @@
# frozen_string_literal: true
class AddPublicTopicCountToTags < ActiveRecord::Migration[7.0]
def up
add_column :tags, :public_topic_count, :integer, default: 0, null: false
execute <<~SQL
UPDATE tags t
SET public_topic_count = x.topic_count
FROM (
SELECT
COUNT(topics.id) AS topic_count,
tags.id AS tag_id
FROM tags
INNER JOIN topic_tags ON tags.id = topic_tags.tag_id
INNER JOIN topics ON topics.id = topic_tags.topic_id AND topics.deleted_at IS NULL AND topics.archetype != 'private_message'
INNER JOIN categories ON categories.id = topics.category_id AND NOT categories.read_restricted
GROUP BY tags.id
) x
WHERE x.tag_id = t.id
AND x.topic_count <> t.public_topic_count;
SQL
end
def down
raise ActiveRecord::IrreversibleMigration
end
end

27
db/migrate/20230119000943_add_staff_topic_count_to_tags.rb Normal file
View File

@ -0,0 +1,27 @@
# frozen_string_literal: true
class AddStaffTopicCountToTags < ActiveRecord::Migration[7.0]
def up
add_column :tags, :staff_topic_count, :integer, default: 0, null: false
execute <<~SQL
UPDATE tags t
SET staff_topic_count = x.topic_count
FROM (
SELECT COUNT(topics.id) AS topic_count, tags.id AS tag_id
FROM tags
LEFT JOIN topic_tags ON tags.id = topic_tags.tag_id
LEFT JOIN topics ON topics.id = topic_tags.topic_id
AND topics.deleted_at IS NULL
AND topics.archetype != 'private_message'
GROUP BY tags.id
) x
WHERE x.tag_id = t.id
AND x.topic_count <> t.staff_topic_count
SQL
end
def down
raise ActiveRecord::IrreversibleMigration
end
end

15
db/post_migrate/20230119024157_remove_topic_count_from_tags.rb Normal file
View File

@ -0,0 +1,15 @@
# frozen_string_literal: true
require "migration/column_dropper"
class RemoveTopicCountFromTags < ActiveRecord::Migration[7.0]
DROPPED_COLUMNS ||= { tags: %i[topic_count] }
def up
DROPPED_COLUMNS.each { |table, columns| Migration::ColumnDropper.execute_drop(table, columns) }
end
def down
raise ActiveRecord::IrreversibleMigration
end
end

12
lib/discourse_tagging.rb
View File

@ -323,17 +323,19 @@ module DiscourseTagging
outer_join = category.nil? || category.allow_global_tags || !category_has_restricted_tags
topic_count_column = Tag.topic_count_column(guardian)
distinct_clause =
if opts[:order_popularity]
"DISTINCT ON (topic_count, name)"
"DISTINCT ON (#{topic_count_column}, name)"
elsif opts[:order_search_results] && opts[:term].present?
"DISTINCT ON (lower(name) = lower(:cleaned_term), topic_count, name)"
"DISTINCT ON (lower(name) = lower(:cleaned_term), #{topic_count_column}, name)"
else
""
end
sql << <<~SQL
SELECT #{distinct_clause} t.id, t.name, t.topic_count, t.pm_topic_count, t.description,
SELECT #{distinct_clause} t.id, t.name, t.#{topic_count_column}, t.pm_topic_count, t.description,
tgr.tgm_id as tgm_id, tgr.tag_group_id as tag_group_id, tgr.parent_tag_id as parent_tag_id,
tgr.one_per_topic as one_per_topic, t.target_tag_id
FROM tags t
@ -479,9 +481,9 @@ module DiscourseTagging
end
if opts[:order_popularity]
builder.order_by("topic_count DESC, name")
builder.order_by("#{topic_count_column} DESC, name")
elsif opts[:order_search_results] && !term.blank?
builder.order_by("lower(name) = lower(:cleaned_term) DESC, topic_count DESC, name")
builder.order_by("lower(name) = lower(:cleaned_term) DESC, #{topic_count_column} DESC, name")
end
result = builder.query(builder_params).uniq { |t| t.id }

3
lib/topic_view.rb
View File

@ -245,7 +245,8 @@ class TopicView
@topic.category
title += " - #{@topic.category.name}"
elsif SiteSetting.tagging_enabled && @topic.tags.exists?
title += " - #{@topic.tags.order("tags.topic_count DESC").first.name}"
title +=
" - #{@topic.tags.order("tags.#{Tag.topic_count_column(@guardian)} DESC").first.name}"
end
end
title

104
spec/integration/tag_counts_spec.rb Normal file
View File

@ -0,0 +1,104 @@
# frozen_string_literal: true
RSpec.describe "Updating tag counts" do
fab!(:tag1) { Fabricate(:tag) }
fab!(:tag2) { Fabricate(:tag) }
fab!(:group) { Fabricate(:group) }
fab!(:public_category) { Fabricate(:category) }
fab!(:public_category2) { Fabricate(:category) }
fab!(:private_category) { Fabricate(:private_category, group: group) }
fab!(:private_category2) { Fabricate(:private_category, group: group) }
fab!(:topic_in_public_category) do
Fabricate(:topic, category: public_category, tags: [tag1, tag2]).tap do |topic|
Fabricate(:post, topic: topic)
end
end
fab!(:topic_in_private_category) do
Fabricate(:topic, category: private_category, tags: [tag1, tag2]).tap do |topic|
Fabricate(:post, topic: topic)
end
end
fab!(:private_message) do
topic = Fabricate(:private_message_post).topic
topic.update!(tags: [tag1, tag2])
topic
end
before do
expect(tag1.public_topic_count).to eq(1)
expect(tag1.staff_topic_count).to eq(2)
expect(tag1.pm_topic_count).to eq(1)
expect(tag2.reload.public_topic_count).to eq(1)
expect(tag2.staff_topic_count).to eq(2)
expect(tag2.pm_topic_count).to eq(1)
end
it "should decrease Tag#public_topic_count for all tags when topic's category is changed from a public category to a read restricted category" do
expect { topic_in_public_category.change_category_to_id(private_category.id) }.to change {
tag1.reload.public_topic_count
}.by(-1).and change { tag2.reload.public_topic_count }.by(-1)
end
it "should increase Tag#public_topic_count for all tags when topic's category is changed from a read restricted category to a public category" do
expect { topic_in_private_category.change_category_to_id(public_category.id) }.to change {
tag1.reload.public_topic_count
}.by(1).and change { tag2.reload.public_topic_count }.by(1)
end
it "should not change Tag#public_topic_count for all tags when topic's category is changed from a public category to another public category" do
expect do
topic_in_public_category.change_category_to_id(public_category2.id)
end.to not_change { tag1.reload.public_topic_count }.and not_change {
tag2.reload.public_topic_count
}
end
it "should not change Tag#public_topic_count for all tags when topic's category is changed from a read restricted category to another read restricted category" do
expect do
topic_in_private_category.change_category_to_id(private_category2.id)
end.to not_change { tag1.reload.public_topic_count }.and not_change {
tag2.reload.public_topic_count
}
end
it "increases Tag#public_topic_count for all tags when topic is converted from private message to a regular topic in a public category" do
expect do
private_message.convert_to_public_topic(
Discourse.system_user,
category_id: public_category.id,
)
end.to change { tag1.reload.public_topic_count }.by(1).and change {
tag2.reload.public_topic_count
}.by(1)
end
it "should not change Tag#public_topic_count for all tags when topic is converted from private message to a regular topic in a read restricted category" do
expect do
private_message.convert_to_public_topic(
Discourse.system_user,
category_id: private_category.id,
)
end.to not_change { tag1.reload.public_topic_count }.and not_change {
tag2.reload.public_topic_count
}
end
it "should decrease Tag#public_topic_count for all tags when regular topic in public category is converted to a private message" do
expect do
topic_in_public_category.convert_to_private_message(Discourse.system_user)
end.to change { tag1.reload.public_topic_count }.by(-1).and change {
tag2.reload.public_topic_count
}.by(-1)
end
it "should not change Tag#public_topic_count for all tags when regular topic in read restricted category is converted to a private message" do
expect do
topic_in_private_category.convert_to_private_message(Discourse.system_user)
end.to not_change { tag1.reload.public_topic_count }.and not_change {
tag2.reload.public_topic_count
}
end
end

14
spec/lib/discourse_tagging_spec.rb
View File

@ -641,9 +641,11 @@ RSpec.describe DiscourseTagging do
it "user does not get an error when editing their topic with a hidden tag" do
PostRevisor.new(post).revise!(admin, raw: post.raw, tags: [hidden_tag.name])
expect(
PostRevisor.new(post).revise!(topic.user, raw: post.raw + " edit", tags: []),
).to be_truthy
expect(topic.reload.tags).to eq([hidden_tag])
end
end
@ -967,8 +969,16 @@ RSpec.describe DiscourseTagging do
topic = Fabricate(:topic, tags: [tag2])
expect(DiscourseTagging.add_or_create_synonyms_by_name(tag1, [tag2.name])).to eq(true)
expect_same_tag_names(topic.reload.tags, [tag1])
expect(tag1.reload.topic_count).to eq(1)
expect(tag2.reload.topic_count).to eq(0)
tag1.reload
expect(tag1.public_topic_count).to eq(1)
expect(tag1.staff_topic_count).to eq(1)
tag2.reload
expect(tag2.public_topic_count).to eq(0)
expect(tag2.staff_topic_count).to eq(0)
end
end
end

4
spec/lib/topic_view_spec.rb
View File

@ -750,8 +750,8 @@ RSpec.describe TopicView do
end
describe "page_title" do
fab!(:tag1) { Fabricate(:tag) }
fab!(:tag2) { Fabricate(:tag, topic_count: 2) }
fab!(:tag1) { Fabricate(:tag, staff_topic_count: 0, public_topic_count: 0) }
fab!(:tag2) { Fabricate(:tag, staff_topic_count: 2, public_topic_count: 2) }
fab!(:op_post) { Fabricate(:post, topic: topic) }
fab!(:post1) { Fabricate(:post, topic: topic) }
fab!(:whisper) { Fabricate(:post, topic: topic, post_type: Post.types[:whisper]) }

106
spec/models/tag_spec.rb
View File

@ -202,29 +202,101 @@ RSpec.describe Tag do
end
end
describe "topic counts" do
describe ".ensure_consistency!" do
it "should exclude private message topics" do
topic
Fabricate(:private_message_topic, tags: [tag])
Tag.ensure_consistency!
tag.reload
expect(tag.topic_count).to eq(1)
expect(tag.staff_topic_count).to eq(1)
expect(tag.public_topic_count).to eq(1)
end
it "should update Tag#topic_count and Tag#public_topic_count correctly" do
tag = Fabricate(:tag, name: "tag1")
tag2 = Fabricate(:tag, name: "tag2")
tag3 = Fabricate(:tag, name: "tag3")
group = Fabricate(:group)
category = Fabricate(:category)
private_category = Fabricate(:private_category, group: group)
private_category2 = Fabricate(:private_category, group: group)
_topic_with_tag = Fabricate(:topic, category: category, tags: [tag])
_topic_with_tag_in_private_category =
Fabricate(:topic, category: private_category, tags: [tag])
_topic_with_tag2_in_private_category2 =
Fabricate(:topic, category: private_category2, tags: [tag2])
tag.update!(staff_topic_count: 123, public_topic_count: 456)
tag2.update!(staff_topic_count: 123, public_topic_count: 456)
tag3.update!(staff_topic_count: 123, public_topic_count: 456)
Tag.ensure_consistency!
tag.reload
tag2.reload
tag3.reload
expect(tag.staff_topic_count).to eq(2)
expect(tag.public_topic_count).to eq(1)
expect(tag2.staff_topic_count).to eq(1)
expect(tag2.public_topic_count).to eq(0)
expect(tag3.staff_topic_count).to eq(0)
expect(tag3.public_topic_count).to eq(0)
end
end
describe "unused tags scope" do
let!(:tags) do
[
Fabricate(:tag, name: "used_publically", topic_count: 2, pm_topic_count: 0),
Fabricate(:tag, name: "used_privately", topic_count: 0, pm_topic_count: 3),
Fabricate(:tag, name: "used_everywhere", topic_count: 0, pm_topic_count: 3),
Fabricate(:tag, name: "unused1", topic_count: 0, pm_topic_count: 0),
Fabricate(:tag, name: "unused2", topic_count: 0, pm_topic_count: 0),
Fabricate(
:tag,
name: "used_publically",
staff_topic_count: 2,
public_topic_count: 2,
pm_topic_count: 0,
),
Fabricate(
:tag,
name: "used_privately",
staff_topic_count: 0,
public_topic_count: 0,
pm_topic_count: 3,
),
Fabricate(
:tag,
name: "used_everywhere",
staff_topic_count: 0,
public_topic_count: 0,
pm_topic_count: 3,
),
Fabricate(
:tag,
name: "unused1",
staff_topic_count: 0,
public_topic_count: 0,
pm_topic_count: 0,
),
Fabricate(
:tag,
name: "unused2",
staff_topic_count: 0,
public_topic_count: 0,
pm_topic_count: 0,
),
]
end
let(:tag_in_group) do
Fabricate(:tag, name: "unused_in_group", topic_count: 0, pm_topic_count: 0)
Fabricate(
:tag,
name: "unused_in_group",
public_topic_count: 0,
staff_topic_count: 0,
pm_topic_count: 0,
)
end
let!(:tag_group) { Fabricate(:tag_group, tag_names: [tag_in_group.name]) }
@ -292,4 +364,22 @@ RSpec.describe Tag do
expect(category.reload.tags).to include(tag2)
end
end
describe ".topic_count_column" do
fab!(:admin) { Fabricate(:admin) }
it "returns 'staff_topic_count' when user is staff" do
expect(Tag.topic_count_column(Guardian.new(admin))).to eq("staff_topic_count")
end
it "returns 'public_topic_count' when user is not staff" do
expect(Tag.topic_count_column(Guardian.new(user))).to eq("public_topic_count")
end
it "returns 'staff_topic_count' when user is not staff but `include_secure_categories_in_tag_counts` site setting is enabled" do
SiteSetting.include_secure_categories_in_tag_counts = true
expect(Tag.topic_count_column(Guardian.new(user))).to eq("staff_topic_count")
end
end
end

39
spec/models/topic_tag_spec.rb
View File

@ -1,33 +1,56 @@
# frozen_string_literal: true
RSpec.describe TopicTag do
fab!(:group) { Fabricate(:group) }
fab!(:private_category) { Fabricate(:private_category, group: group) }
fab!(:topic) { Fabricate(:topic) }
fab!(:topic_in_private_category) { Fabricate(:topic, category: private_category) }
fab!(:tag) { Fabricate(:tag) }
let(:topic_tag) { Fabricate(:topic_tag, topic: topic, tag: tag) }
describe "#after_create" do
it "tag topic_count should be increased" do
expect { topic_tag }.to change(tag, :topic_count).by(1)
it "should increase Tag#staff_topic_count and Tag#public_topic_count for a regular topic in a public category" do
expect { topic_tag }.to change { tag.reload.staff_topic_count }.by(1).and change {
tag.reload.public_topic_count
}.by(1)
end
it "tag topic_count should not be increased" do
it "should only increase Tag#staff_topic_count for a regular topic in a read restricted category" do
expect { Fabricate(:topic_tag, topic: topic_in_private_category, tag: tag) }.to change {
tag.reload.staff_topic_count
}.by(1)
expect(tag.reload.public_topic_count).to eq(0)
end
it "should increase Tag#pm_topic_count for a private message topic" do
topic.archetype = Archetype.private_message
expect { topic_tag }.not_to change(tag, :topic_count)
expect { topic_tag }.to change { tag.reload.pm_topic_count }.by(1)
end
end
describe "#after_destroy" do
it "tag topic_count should be decreased" do
it "should decrease Tag#staff_topic_count and Tag#public_topic_count for a regular topic in a public category" do
topic_tag
expect { topic_tag.destroy }.to change(tag, :topic_count).by(-1)
expect { topic_tag.destroy! }.to change { tag.reload.staff_topic_count }.by(-1).and change {
tag.reload.public_topic_count
}.by(-1)
end
it "tag topic_count should not be decreased" do
it "should only decrease Topic#topic_count for a regular topic in a read restricted category" do
topic_tag = Fabricate(:topic_tag, topic: topic_in_private_category, tag: tag)
expect { topic_tag.destroy! }.to change { tag.reload.staff_topic_count }.by(-1)
expect(tag.reload.public_topic_count).to eq(0)
end
it "should decrease Tag#pm_topic_count for a private message topic" do
topic.archetype = Archetype.private_message
topic_tag
expect { topic_tag.destroy }.not_to change(tag, :topic_count)
expect { topic_tag.destroy! }.to change { tag.reload.pm_topic_count }.by(-1)
end
end
end

98
spec/requests/tags_controller_spec.rb
View File

@ -12,18 +12,43 @@ RSpec.describe TagsController do
describe "#index" do
fab!(:test_tag) { Fabricate(:tag, name: "test") }
fab!(:topic_tag) { Fabricate(:tag, name: "topic-test", topic_count: 1) }
fab!(:topic_tag) do
Fabricate(:tag, name: "topic-test", public_topic_count: 1, staff_topic_count: 1)
end
fab!(:synonym) { Fabricate(:tag, name: "synonym", target_tag: topic_tag) }
shared_examples "successfully retrieve tags with topic_count > 0" do
it "should return the right response" do
shared_examples "retrieves the right tags" do
it "retrieves all tags as a staff user" do
sign_in(admin)
get "/tags.json"
expect(response.status).to eq(200)
tags = response.parsed_body["tags"]
expect(tags[0]["name"]).to eq(test_tag.name)
expect(tags[0]["count"]).to eq(0)
expect(tags[0]["pm_count"]).to eq(0)
expect(tags[1]["name"]).to eq(topic_tag.name)
expect(tags[1]["count"]).to eq(1)
expect(tags[1]["pm_count"]).to eq(0)
end
it "only retrieve tags that have been used in public topics for non-staff user" do
sign_in(user)
get "/tags.json"
expect(response.status).to eq(200)
tags = response.parsed_body["tags"]
expect(tags.length).to eq(1)
expect(tags[0]["text"]).to eq("topic-test")
expect(tags[0]["name"]).to eq(topic_tag.name)
expect(tags[0]["count"]).to eq(1)
expect(tags[0]["pm_count"]).to eq(0)
end
end
@ -76,7 +101,7 @@ RSpec.describe TagsController do
context "with tags_listed_by_group enabled" do
before { SiteSetting.tags_listed_by_group = true }
include_examples "successfully retrieve tags with topic_count > 0"
include_examples "retrieves the right tags"
it "works for tags in groups" do
tag_group = Fabricate(:tag_group, tags: [test_tag, topic_tag, synonym])
@ -136,20 +161,7 @@ RSpec.describe TagsController do
context "with tags_listed_by_group disabled" do
before { SiteSetting.tags_listed_by_group = false }
include_examples "successfully retrieve tags with topic_count > 0"
end
context "when user can admin tags" do
it "successfully retrieve all tags" do
sign_in(admin)
get "/tags.json"
expect(response.status).to eq(200)
tags = response.parsed_body["tags"]
expect(tags.length).to eq(2)
end
include_examples "retrieves the right tags"
end
context "with hidden tags" do
@ -763,10 +775,10 @@ RSpec.describe TagsController do
expect(response.parsed_body["results"].map { |j| j["id"] }.sort).to eq(%w[stuff stumped])
end
it "returns tags ordered by topic_count, and prioritises exact matches" do
Fabricate(:tag, name: "tag1", topic_count: 10)
Fabricate(:tag, name: "tag2", topic_count: 100)
Fabricate(:tag, name: "tag", topic_count: 1)
it "returns tags ordered by public_topic_count, and prioritises exact matches" do
Fabricate(:tag, name: "tag1", public_topic_count: 10, staff_topic_count: 10)
Fabricate(:tag, name: "tag2", public_topic_count: 100, staff_topic_count: 100)
Fabricate(:tag, name: "tag", public_topic_count: 1, staff_topic_count: 1)
get "/tags/filter/search.json", params: { q: "tag", limit: 2 }
expect(response.status).to eq(200)
@ -976,11 +988,41 @@ RSpec.describe TagsController do
context "with some tags" do
let!(:tags) do
[
Fabricate(:tag, name: "used_publically", topic_count: 2, pm_topic_count: 0),
Fabricate(:tag, name: "used_privately", topic_count: 0, pm_topic_count: 3),
Fabricate(:tag, name: "used_everywhere", topic_count: 0, pm_topic_count: 3),
Fabricate(:tag, name: "unused1", topic_count: 0, pm_topic_count: 0),
Fabricate(:tag, name: "unused2", topic_count: 0, pm_topic_count: 0),
Fabricate(
:tag,
name: "used_publically",
public_topic_count: 2,
staff_topic_count: 2,
pm_topic_count: 0,
),
Fabricate(
:tag,
name: "used_privately",
public_topic_count: 0,
staff_topic_count: 0,
pm_topic_count: 3,
),
Fabricate(
:tag,
name: "used_everywhere",
public_topic_count: 0,
staff_topic_count: 0,
pm_topic_count: 3,
),
Fabricate(
:tag,
name: "unused1",
public_topic_count: 0,
staff_topic_count: 0,
pm_topic_count: 0,
),
Fabricate(
:tag,
name: "unused2",
public_topic_count: 0,
staff_topic_count: 0,
pm_topic_count: 0,
),
]
end

25
spec/serializers/tag_serializer_spec.rb Normal file
View File

@ -0,0 +1,25 @@
# frozen_string_literal: true
RSpec.describe TagSerializer do
fab!(:user) { Fabricate(:user) }
fab!(:admin) { Fabricate(:admin) }
fab!(:tag) { Fabricate(:tag) }
fab!(:group) { Fabricate(:group) }
fab!(:private_category) { Fabricate(:private_category, group: group) }
fab!(:topic_in_public_category) { Fabricate(:topic, tags: [tag]) }
fab!(:topic_in_private_category) { Fabricate(:topic, category: private_category, tags: [tag]) }
describe "#topic_count" do
it "should return the value of `Tag#public_topic_count` for a non-staff user" do
serialized = described_class.new(tag, scope: Guardian.new(user), root: false).as_json
expect(serialized[:topic_count]).to eq(1)
end
it "should return the vavlue of `Tag#topic_count` for a staff user" do
serialized = described_class.new(tag, scope: Guardian.new(admin), root: false).as_json
expect(serialized[:topic_count]).to eq(2)
end
end
end

30
spec/serializers/topic_view_serializer_spec.rb
View File

@ -279,9 +279,33 @@ RSpec.describe TopicViewSerializer do
end
describe "tags order" do
fab!(:tag1) { Fabricate(:tag, name: "ctag", description: "c description", topic_count: 5) }
fab!(:tag2) { Fabricate(:tag, name: "btag", description: "b description", topic_count: 9) }
fab!(:tag3) { Fabricate(:tag, name: "atag", description: "a description", topic_count: 3) }
fab!(:tag1) do
Fabricate(
:tag,
name: "ctag",
description: "c description",
staff_topic_count: 5,
public_topic_count: 5,
)
end
fab!(:tag2) do
Fabricate(
:tag,
name: "btag",
description: "b description",
staff_topic_count: 9,
public_topic_count: 9,
)
end
fab!(:tag3) do
Fabricate(
:tag,
name: "atag",
description: "a description",
staff_topic_count: 3,
public_topic_count: 3,
)
end
before do
topic.tags << tag1

20
spec/services/hashtag_autocomplete_service_spec.rb
View File

@ -3,7 +3,9 @@
RSpec.describe HashtagAutocompleteService do
fab!(:user) { Fabricate(:user) }
fab!(:category1) { Fabricate(:category, name: "The Book Club", slug: "the-book-club") }
fab!(:tag1) { Fabricate(:tag, name: "great-books", topic_count: 22) }
fab!(:tag1) do
Fabricate(:tag, name: "great-books", staff_topic_count: 22, public_topic_count: 22)
end
fab!(:topic1) { Fabricate(:topic) }
let(:guardian) { Guardian.new(user) }
@ -68,7 +70,7 @@ RSpec.describe HashtagAutocompleteService do
end
it "includes the tag count" do
tag1.update!(topic_count: 78)
tag1.update!(staff_topic_count: 78, public_topic_count: 78)
expect(subject.search("book", %w[tag category]).map(&:text)).to eq(
["great-books", "The Book Club"],
)
@ -149,8 +151,8 @@ RSpec.describe HashtagAutocompleteService do
category6 = Fabricate(:category, name: "Book Reviews", slug: "book-reviews")
Fabricate(:category, name: "Good Books", slug: "book", parent_category: category6)
Fabricate(:tag, name: "bookmania", topic_count: 15)
Fabricate(:tag, name: "awful-books", topic_count: 56)
Fabricate(:tag, name: "bookmania", staff_topic_count: 15, public_topic_count: 15)
Fabricate(:tag, name: "awful-books", staff_topic_count: 56, public_topic_count: 56)
expect(subject.search("book", %w[category tag]).map(&:ref)).to eq(
[
@ -220,9 +222,13 @@ RSpec.describe HashtagAutocompleteService do
end
fab!(:category4) { Fabricate(:category, name: "Bookworld", slug: "book", topic_count: 56) }
fab!(:category5) { Fabricate(:category, name: "Media", slug: "media", topic_count: 446) }
fab!(:tag2) { Fabricate(:tag, name: "mid-books", topic_count: 33) }
fab!(:tag3) { Fabricate(:tag, name: "terrible-books", topic_count: 2) }
fab!(:tag4) { Fabricate(:tag, name: "book", topic_count: 1) }
fab!(:tag2) do
Fabricate(:tag, name: "mid-books", staff_topic_count: 33, public_topic_count: 33)
end
fab!(:tag3) do
Fabricate(:tag, name: "terrible-books", staff_topic_count: 2, public_topic_count: 2)
end
fab!(:tag4) { Fabricate(:tag, name: "book", staff_topic_count: 1, public_topic_count: 1) }
it "returns the 'most polular' categories and tags (based on topic_count) that the user can access" do
category1.update!(read_restricted: true)

16
spec/services/tag_hashtag_data_source_spec.rb
View File

@ -1,16 +1,16 @@
# frozen_string_literal: true
RSpec.describe TagHashtagDataSource do
fab!(:tag1) { Fabricate(:tag, name: "fact", topic_count: 0) }
fab!(:tag2) { Fabricate(:tag, name: "factor", topic_count: 5) }
fab!(:tag3) { Fabricate(:tag, name: "factory", topic_count: 4) }
fab!(:tag4) { Fabricate(:tag, name: "factorio", topic_count: 3) }
fab!(:tag5) { Fabricate(:tag, name: "factz", topic_count: 1) }
fab!(:tag1) { Fabricate(:tag, name: "fact", public_topic_count: 0) }
fab!(:tag2) { Fabricate(:tag, name: "factor", public_topic_count: 5) }
fab!(:tag3) { Fabricate(:tag, name: "factory", public_topic_count: 4) }
fab!(:tag4) { Fabricate(:tag, name: "factorio", public_topic_count: 3) }
fab!(:tag5) { Fabricate(:tag, name: "factz", public_topic_count: 1) }
fab!(:user) { Fabricate(:user) }
let(:guardian) { Guardian.new(user) }
describe "#search" do
it "orders tag results by exact search match, then topic count, then name" do
it "orders tag results by exact search match, then public topic count, then name" do
expect(described_class.search(guardian, "fact", 5).map(&:slug)).to eq(
%w[fact factor factory factorio factz],
)
@ -31,7 +31,7 @@ RSpec.describe TagHashtagDataSource do
)
end
it "includes the topic count for the text of the tag in secondary text" do
it "includes the public topic count for the text of the tag in secondary text" do
expect(described_class.search(guardian, "fact", 5).map(&:secondary_text)).to eq(
%w[x0 x5 x4 x3 x1],
)
@ -52,7 +52,7 @@ RSpec.describe TagHashtagDataSource do
end
describe "#search_without_term" do
it "returns distinct tags sorted by topic_count" do
it "returns distinct tags sorted by public topic count" do
expect(described_class.search_without_term(guardian, 5).map(&:slug)).to eq(
%w[factor factory factorio factz fact],
)

4
spec/support/user_sidebar_serializer_attributes.rb
View File

@ -65,7 +65,9 @@ RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass|
describe "#sidebar_tags" do
fab!(:tag) { Fabricate(:tag, name: "foo") }
fab!(:pm_tag) { Fabricate(:tag, name: "bar", pm_topic_count: 5, topic_count: 0) }
fab!(:pm_tag) do
Fabricate(:tag, name: "bar", pm_topic_count: 5, staff_topic_count: 0, public_topic_count: 0)
end
fab!(:hidden_tag) { Fabricate(:tag, name: "secret") }
fab!(:staff_tag_group) do
Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["secret"])

4
spec/system/hashtag_autocomplete_spec.rb
View File

@ -10,8 +10,8 @@ describe "Using #hashtag autocompletion to search for and lookup categories and
fab!(:category2) do
Fabricate(:category, name: "Other Category", slug: "other-cat", topic_count: 23)
end
fab!(:tag) { Fabricate(:tag, name: "cooltag", topic_count: 324) }
fab!(:tag2) { Fabricate(:tag, name: "othertag", topic_count: 66) }
fab!(:tag) { Fabricate(:tag, name: "cooltag", staff_topic_count: 324, public_topic_count: 324) }
fab!(:tag2) { Fabricate(:tag, name: "othertag", staff_topic_count: 66, public_topic_count: 66) }
fab!(:topic) { Fabricate(:topic, category: category, tags: [tag]) }
fab!(:post) { Fabricate(:post, topic: topic) }
let(:uncategorized_category) { Category.find(SiteSetting.uncategorized_category_id) }