From bd0f10a50b13fc029f44180cbeb42b2b8ed003cb Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 24 Mar 2022 15:56:50 +1000 Subject: [PATCH] SECURITY: Hide private categories in user activity export (#16276) In some of the user's own activity export data, we sometimes showed a secure category's name or exposed the existence of a secure category. --- app/jobs/regular/export_user_archive.rb | 28 +++++++-------- spec/jobs/export_user_archive_spec.rb | 45 ++++++++++++++++++++----- 2 files changed, 51 insertions(+), 22 deletions(-) diff --git a/app/jobs/regular/export_user_archive.rb b/app/jobs/regular/export_user_archive.rb index ba2442df774..489dac28b23 100644 --- a/app/jobs/regular/export_user_archive.rb +++ b/app/jobs/regular/export_user_archive.rb @@ -261,15 +261,16 @@ module Jobs CategoryUser .where(user_id: @current_user.id) - .select(:category_id, :notification_level, :last_seen_at) + .includes(:category) + .merge(Category.secured(guardian)) .each do |cu| - yield [ - cu.category_id, - piped_category_name(cu.category_id), - NotificationLevels.all[cu.notification_level], - cu.last_seen_at - ] - end + yield [ + cu.category_id, + piped_category_name(cu.category_id, cu.category), + NotificationLevels.all[cu.notification_level], + cu.last_seen_at + ] + end end def flags_export @@ -408,10 +409,9 @@ module Jobs @guardian ||= Guardian.new(@current_user) end - def piped_category_name(category_id) - return "-" unless category_id - category = Category.find_by(id: category_id) - return "#{category_id}" unless category + def piped_category_name(category_id, category) + return "#{category_id}" if category_id && !category + return "-" if !guardian.can_see_category?(category) categories = [category.name] while category.parent_category_id && category = category.parent_category categories << category.name @@ -433,10 +433,10 @@ module Jobs user_archive_array = [] topic_data = user_archive.topic user_archive = user_archive.as_json - topic_data = Topic.with_deleted.find_by(id: user_archive['topic_id']) if topic_data.nil? + topic_data = Topic.with_deleted.includes(:category).find_by(id: user_archive['topic_id']) if topic_data.nil? return user_archive_array if topic_data.nil? - categories = piped_category_name(topic_data.category_id) + categories = piped_category_name(topic_data.category_id, topic_data.category) is_pm = topic_data.archetype == "private_message" ? I18n.t("csv_export.boolean_yes") : I18n.t("csv_export.boolean_no") url = "#{Discourse.base_url}/t/#{topic_data.slug}/#{topic_data.id}/#{user_archive['post_number']}" diff --git a/spec/jobs/export_user_archive_spec.rb b/spec/jobs/export_user_archive_spec.rb index b1cd6ebc8c2..0c44943d2e8 100644 --- a/spec/jobs/export_user_archive_spec.rb +++ b/spec/jobs/export_user_archive_spec.rb @@ -16,7 +16,7 @@ describe Jobs::ExportUserArchive do let(:component) { raise 'component not set' } fab!(:admin) { Fabricate(:admin) } - fab!(:category) { Fabricate(:category_with_definition) } + fab!(:category) { Fabricate(:category_with_definition, name: "User Archive Category") } fab!(:subcategory) { Fabricate(:category_with_definition, parent_category_id: category.id) } fab!(:topic) { Fabricate(:topic, category: category) } let(:post) { Fabricate(:post, user: user, topic: topic) } @@ -169,6 +169,17 @@ describe Jobs::ExportUserArchive do _, csv_out = make_component_csv expect(csv_out).to match cat2_id.to_s end + + it "can export a post from a secure category, obscuring the category name" do + cat2 = Fabricate(:private_category, group: Fabricate(:group), name: "Secret Cat") + topic2 = Fabricate(:topic, category: cat2, user: user, title: "This is a test secure topic") + _post2 = Fabricate(:post, topic: topic2, user: user) + data, csv_out = make_component_csv + expect(csv_out).not_to match "Secret Cat" + expect(data.length).to eq(1) + expect(data[0][:topic_title]).to eq("This is a test secure topic") + expect(data[0][:categories]).to eq("-") + end end context 'preferences' do @@ -315,9 +326,11 @@ describe Jobs::ExportUserArchive do context 'category_preferences' do let(:component) { 'category_preferences' } - let(:subsubcategory) { Fabricate(:category_with_definition, parent_category_id: subcategory.id) } - let(:announcements) { Fabricate(:category_with_definition) } - let(:deleted_category) { Fabricate(:category) } + let(:subsubcategory) { Fabricate(:category_with_definition, parent_category_id: subcategory.id, name: "User Archive Subcategory") } + let(:announcements) { Fabricate(:category_with_definition, name: "Announcements") } + let(:deleted_category) { Fabricate(:category, name: "Deleted Category") } + let(:secure_category_group) { Fabricate(:group) } + let(:secure_category) { Fabricate(:private_category, group: secure_category_group, name: "Super Secret Category") } let(:reset_at) { DateTime.parse('2017-03-01 12:00') } @@ -342,11 +355,12 @@ describe Jobs::ExportUserArchive do deleted_category.destroy! end - it 'correctly exports the CategoryUser table' do + it 'correctly exports the CategoryUser table, excluding deleted categories' do data, _csv_out = make_component_csv - expect(data.find { |r| r['category_id'] == category.id }).to be_nil - expect(data.length).to eq(4) + expect(data.find { |r| r['category_id'] == category.id.to_s }).to be_nil + expect(data.find { |r| r['category_id'] == deleted_category.id.to_s }).to be_nil + expect(data.length).to eq(3) data.sort! { |a, b| a['category_id'].to_i <=> b['category_id'].to_i } expect(data[0][:category_id]).to eq(subcategory.id.to_s) @@ -362,8 +376,23 @@ describe Jobs::ExportUserArchive do expect(data[2][:category_names]).to eq(announcements.name) expect(data[2][:notification_level]).to eq('watching_first_post') expect(data[2][:dismiss_new_timestamp]).to eq('') + end - expect(data[3][:category_names]).to eq(data[3][:category_id]) + it "does not include any secure categories the user does not have access to, even if the user has a CategoryUser record" do + CategoryUser.set_notification_level_for_category(user, NotificationLevels.all[:muted], secure_category.id) + data, _csv_out = make_component_csv + + expect(data.any? { |r| r['category_id'] == secure_category.id.to_s }).to eq(false) + expect(data.length).to eq(3) + end + + it "does include secure categories that the user has access to" do + CategoryUser.set_notification_level_for_category(user, NotificationLevels.all[:muted], secure_category.id) + GroupUser.create!(user: user, group: secure_category_group) + data, _csv_out = make_component_csv + + expect(data.any? { |r| r['category_id'] == secure_category.id.to_s }).to eq(true) + expect(data.length).to eq(4) end end