From b2acc416e7cdd502e2f394f1e6014210e6212b50 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 9 Dec 2022 10:34:25 +1000 Subject: [PATCH] FIX: Server-side hashtag lookups of secure categories for a user (#19377) * FIX: Use Category.secured(guardian) for hashtag datasource Follow up to comments in #19219, changing the category hashtag datasource to use Category.secured(guardian) instead of Site.new(guardian).categories here since the latter does more work for not much benefit, and the query time is the same. Also eliminates some Hash -> Model back and forth busywork. Add some more specs too. * FIX: Server-side hashtag lookup cooking user loading When we were using the PrettyText.options.currentUser and parsing back and forth with JSON for the hashtag lookups server-side, we had a bug where the user's secure categories were not loaded since we never actually loaded a User model from the database, only parsed it from JSON. This commit fixes the issue by instead using the PretyText.options.userId and looking up the user directly from the database when calling hashtag_lookup via the PrettyText::Helpers code when cooking server-side. Added the missing spec to check for this as well. --- .../hashtag-autocomplete.js | 6 +- app/models/category.rb | 4 ++ app/models/concerns/category_hashtag.rb | 22 +++---- app/services/category_hashtag_data_source.rb | 36 ++++------- lib/pretty_text.rb | 6 ++ lib/pretty_text/helpers.rb | 8 +-- lib/pretty_text/shims.js | 4 +- spec/lib/pretty_text/helpers_spec.rb | 16 ++--- spec/lib/pretty_text_spec.rb | 14 ++++- .../category_hashtag_data_source_spec.rb | 61 ++++++++++++++++++- .../hashtag_autocomplete_service_spec.rb | 5 +- 11 files changed, 118 insertions(+), 64 deletions(-) diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown/hashtag-autocomplete.js b/app/assets/javascripts/pretty-text/engines/discourse-markdown/hashtag-autocomplete.js index b920b72b1ab..65e2fca64bb 100644 --- a/app/assets/javascripts/pretty-text/engines/discourse-markdown/hashtag-autocomplete.js +++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown/hashtag-autocomplete.js @@ -14,11 +14,7 @@ function addHashtag(buffer, matches, state) { // slug lookup. const result = hashtagLookup && - hashtagLookup( - slug, - options.currentUser, - options.hashtagTypesInPriorityOrder - ); + hashtagLookup(slug, options.userId, options.hashtagTypesInPriorityOrder); // NOTE: When changing the HTML structure here, you must also change // it in the placeholder HTML code inside lib/hashtag-autocomplete, and vice-versa. diff --git a/app/models/category.rb b/app/models/category.rb index 7e7bd947b02..477744f9879 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -202,6 +202,10 @@ class Category < ActiveRecord::Base Category.clear_subcategory_ids end + def top_level? + self.parent_category_id.nil? + end + def self.scoped_to_permissions(guardian, permission_types) if guardian.try(:is_admin?) all diff --git a/app/models/concerns/category_hashtag.rb b/app/models/concerns/category_hashtag.rb index 959216baf30..db28efe789f 100644 --- a/app/models/concerns/category_hashtag.rb +++ b/app/models/concerns/category_hashtag.rb @@ -12,9 +12,7 @@ module CategoryHashtag slug_path = category_slug.split(SEPARATOR) return nil if slug_path.empty? || slug_path.size > 2 - if SiteSetting.slug_generation_method == "encoded" - slug_path.map! { |slug| CGI.escape(slug) } - end + slug_path.map! { |slug| CGI.escape(slug) } if SiteSetting.slug_generation_method == "encoded" parent_slug, child_slug = slug_path.last(2) categories = Category.where(slug: parent_slug) @@ -31,9 +29,8 @@ module CategoryHashtag # depth supported). # # @param {Array} category_slugs - Slug strings to look up, can also be in the parent:child format - # @param {Array} cached_categories - An array of Hashes representing categories, Site.categories - # should be used here since it is scoped to the Guardian. - def query_from_cached_categories(category_slugs, cached_categories) + # @param {Array} categories - An array of Category models scoped to the user's guardian permissions. + def query_loaded_from_slugs(category_slugs, categories) category_slugs .map(&:downcase) .map do |slug| @@ -51,18 +48,19 @@ module CategoryHashtag # by its slug then find the child by its slug and its parent's # ID to make sure they match. if child_slug.present? - parent_category = cached_categories.find { |cat| cat[:slug].downcase == parent_slug } + parent_category = categories.find { |cat| cat.slug.casecmp?(parent_slug) } if parent_category.present? - cached_categories.find do |cat| - cat[:slug].downcase == child_slug && cat[:parent_category_id] == parent_category[:id] + categories.find do |cat| + cat.slug.downcase == child_slug && cat.parent_category_id == parent_category.id end end else - cached_categories.find do |cat| - cat[:slug].downcase == parent_slug && cat[:parent_category_id].nil? + categories.find do |cat| + cat.slug.downcase == parent_slug && cat.top_level? end end - end.compact + end + .compact end end end diff --git a/app/services/category_hashtag_data_source.rb b/app/services/category_hashtag_data_source.rb index 3edab5c017c..ab912bf1c4f 100644 --- a/app/services/category_hashtag_data_source.rb +++ b/app/services/category_hashtag_data_source.rb @@ -8,9 +8,7 @@ class CategoryHashtagDataSource "folder" end - def self.category_to_hashtag_item(parent_category, category) - category = Category.new(category.slice(:id, :slug, :name, :parent_category_id, :description)) - + def self.category_to_hashtag_item(category) HashtagAutocompleteService::HashtagItem.new.tap do |item| item.text = category.name item.slug = category.slug @@ -22,7 +20,7 @@ class CategoryHashtagDataSource # categories here. item.ref = if category.parent_category_id - !parent_category ? category.slug : "#{parent_category[:slug]}:#{category.slug}" + "#{category.parent_category.slug}:#{category.slug}" else category.slug end @@ -30,31 +28,19 @@ class CategoryHashtagDataSource end def self.lookup(guardian, slugs) - # We use Site here because it caches all the categories the - # user has access to. - guardian_categories = Site.new(guardian).categories + user_categories = Category.secured(guardian).includes(:parent_category) Category - .query_from_cached_categories(slugs, guardian_categories) - .map do |category| - parent_category = - guardian_categories.find { |cat| cat[:id] == category[:parent_category_id] } - category_to_hashtag_item(parent_category, category) - end + .query_loaded_from_slugs(slugs, user_categories) + .map { |category| category_to_hashtag_item(category) } end def self.search(guardian, term, limit) - guardian_categories = Site.new(guardian).categories - - guardian_categories - .select do |category| - category[:name].downcase.include?(term) || category[:slug].downcase.include?(term) - end + Category + .secured(guardian) + .includes(:parent_category) + .where("LOWER(name) LIKE :term OR LOWER(slug) LIKE :term", term: "%#{term}%") .take(limit) - .map do |category| - parent_category = - guardian_categories.find { |cat| cat[:id] == category[:parent_category_id] } - category_to_hashtag_item(parent_category, category) - end + .map { |category| category_to_hashtag_item(category) } end def self.search_sort(search_results, term) @@ -79,6 +65,6 @@ class CategoryHashtagDataSource ) .order(topic_count: :desc) .take(limit) - .map { |category| category_to_hashtag_item(category.parent_category, category) } + .map { |category| category_to_hashtag_item(category) } end end diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index d8cb0cf94dd..632dcabd915 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -225,6 +225,12 @@ module PrettyText if opts[:user_id] buffer << "__optInput.userId = #{opts[:user_id].to_i};\n" + + # NOTE: If using this for server-side cooking you will end up + # with a Hash once it is passed to a PrettyText::Helper. If + # you use that hash to instanciate a User model, you will want to do + # user.reload before accessing data on this parsed User, otherwise + # AR relations will not be loaded. buffer << "__optInput.currentUser = #{User.find(opts[:user_id]).to_json}\n" end diff --git a/lib/pretty_text/helpers.rb b/lib/pretty_text/helpers.rb index 97d2a795585..cb6a5faa16c 100644 --- a/lib/pretty_text/helpers.rb +++ b/lib/pretty_text/helpers.rb @@ -110,15 +110,15 @@ module PrettyText end end - def hashtag_lookup(slug, cooking_user, types_in_priority_order) + def hashtag_lookup(slug, cooking_user_id, types_in_priority_order) # This is _somewhat_ expected since we need to be able to cook posts # etc. without a user sometimes, but it is still an edge case. - if cooking_user.blank? + if cooking_user_id.blank? cooking_user = Discourse.system_user + else + cooking_user = User.find(cooking_user_id) end - cooking_user = User.new(cooking_user) if cooking_user.is_a?(Hash) - result = HashtagAutocompleteService.new( Guardian.new(cooking_user) ).lookup([slug], types_in_priority_order) diff --git a/lib/pretty_text/shims.js b/lib/pretty_text/shims.js index 70c6ea56494..1c54183c922 100644 --- a/lib/pretty_text/shims.js +++ b/lib/pretty_text/shims.js @@ -113,8 +113,8 @@ function __categoryLookup(c) { return __helpers.category_tag_hashtag_lookup(c); } -function __hashtagLookup(slug, cookingUser, typesInPriorityOrder) { - return __helpers.hashtag_lookup(slug, cookingUser, typesInPriorityOrder); +function __hashtagLookup(slug, cookingUserId, typesInPriorityOrder) { + return __helpers.hashtag_lookup(slug, cookingUserId, typesInPriorityOrder); } function __lookupAvatar(p) { diff --git a/spec/lib/pretty_text/helpers_spec.rb b/spec/lib/pretty_text/helpers_spec.rb index 62ef9593327..4e95adc8449 100644 --- a/spec/lib/pretty_text/helpers_spec.rb +++ b/spec/lib/pretty_text/helpers_spec.rb @@ -55,7 +55,7 @@ RSpec.describe PrettyText::Helpers do fab!(:user) { Fabricate(:user) } it "handles tags and categories based on slug with type suffix" do - expect(PrettyText::Helpers.hashtag_lookup("somecooltag::tag", user, %w[category tag])).to eq( + expect(PrettyText::Helpers.hashtag_lookup("somecooltag::tag", user.id, %w[category tag])).to eq( { relative_url: tag.url, text: "somecooltag", @@ -66,7 +66,7 @@ RSpec.describe PrettyText::Helpers do type: "tag", }, ) - expect(PrettyText::Helpers.hashtag_lookup("someawesomecategory::category", user, %w[category tag])).to eq( + expect(PrettyText::Helpers.hashtag_lookup("someawesomecategory::category", user.id, %w[category tag])).to eq( { relative_url: category.url, text: "Some Awesome Category", @@ -81,7 +81,7 @@ RSpec.describe PrettyText::Helpers do it "handles categories based on slug" do expect( - PrettyText::Helpers.hashtag_lookup("someawesomecategory", user, %w[category tag]), + PrettyText::Helpers.hashtag_lookup("someawesomecategory", user.id, %w[category tag]), ).to eq( { relative_url: category.url, @@ -96,7 +96,7 @@ RSpec.describe PrettyText::Helpers do end it "handles tags and categories based on slug without type suffix" do - expect(PrettyText::Helpers.hashtag_lookup("somecooltag", user, %w[category tag])).to eq( + expect(PrettyText::Helpers.hashtag_lookup("somecooltag", user.id, %w[category tag])).to eq( { relative_url: tag.url, text: "somecooltag", @@ -107,7 +107,7 @@ RSpec.describe PrettyText::Helpers do type: "tag", }, ) - expect(PrettyText::Helpers.hashtag_lookup("someawesomecategory", user, %w[category tag])).to eq( + expect(PrettyText::Helpers.hashtag_lookup("someawesomecategory", user.id, %w[category tag])).to eq( { relative_url: category.url, text: "Some Awesome Category", @@ -123,10 +123,10 @@ RSpec.describe PrettyText::Helpers do it "does not include categories the cooking user does not have access to" do group = Fabricate(:group) private_category = Fabricate(:private_category, slug: "secretcategory", name: "Manager Hideout", group: group) - expect(PrettyText::Helpers.hashtag_lookup("secretcategory", user, %w[category tag])).to eq(nil) + expect(PrettyText::Helpers.hashtag_lookup("secretcategory", user.id, %w[category tag])).to eq(nil) GroupUser.create(group: group, user: user) - expect(PrettyText::Helpers.hashtag_lookup("secretcategory", user, %w[category tag])).to eq( + expect(PrettyText::Helpers.hashtag_lookup("secretcategory", user.id, %w[category tag])).to eq( { relative_url: private_category.url, text: "Manager Hideout", @@ -140,7 +140,7 @@ RSpec.describe PrettyText::Helpers do end it "returns nil when no tag or category that matches exists" do - expect(PrettyText::Helpers.hashtag_lookup("blah", user, %w[category tag])).to eq(nil) + expect(PrettyText::Helpers.hashtag_lookup("blah", user.id, %w[category tag])).to eq(nil) end it "uses the system user if the cooking_user is nil" do diff --git a/spec/lib/pretty_text_spec.rb b/spec/lib/pretty_text_spec.rb index 8168d4b8161..b953ce1353e 100644 --- a/spec/lib/pretty_text_spec.rb +++ b/spec/lib/pretty_text_spec.rb @@ -1456,16 +1456,24 @@ RSpec.describe PrettyText do SiteSetting.enable_experimental_hashtag_autocomplete = true user = Fabricate(:user) - category = Fabricate(:category, name: 'testing') - category2 = Fabricate(:category, name: 'known') + category = Fabricate(:category, name: 'testing', slug: 'testing') + category2 = Fabricate(:category, name: 'known', slug: 'known') + group = Fabricate(:group) + private_category = Fabricate(:private_category, name: 'secret', group: group, slug: 'secret') Fabricate(:topic, tags: [Fabricate(:tag, name: 'known')]) - cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing", user_id: user.id) + cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing #secret", user_id: user.id) expect(cooked).to include("#unknown::tag") expect(cooked).to include("known") expect(cooked).to include("known") expect(cooked).to include("testing") + expect(cooked).to include("#secret") + + # If the user hash access to the private category it should be cooked with the details + icon + group.add(user) + cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing #secret", user_id: user.id) + expect(cooked).to include("secret") cooked = PrettyText.cook("[`a` #known::tag here](http://example.com)", user_id: user.id) diff --git a/spec/services/category_hashtag_data_source_spec.rb b/spec/services/category_hashtag_data_source_spec.rb index a0a411628d6..7e0a88c9f7a 100644 --- a/spec/services/category_hashtag_data_source_spec.rb +++ b/spec/services/category_hashtag_data_source_spec.rb @@ -1,8 +1,11 @@ # frozen_string_literal: true RSpec.describe CategoryHashtagDataSource do - fab!(:category1) { Fabricate(:category, slug: "random", topic_count: 12) } - fab!(:category2) { Fabricate(:category, slug: "books", topic_count: 566) } + fab!(:parent_category) { Fabricate(:category, slug: "fun") } + fab!(:category1) do + Fabricate(:category, slug: "random", topic_count: 12, parent_category: parent_category) + end + fab!(:category2) { Fabricate(:category, name: "Book Section", slug: "books", topic_count: 566) } fab!(:category3) { Fabricate(:category, slug: "movies", topic_count: 245) } fab!(:group) { Fabricate(:group) } fab!(:category4) { Fabricate(:private_category, slug: "secret", group: group, topic_count: 40) } @@ -11,6 +14,60 @@ RSpec.describe CategoryHashtagDataSource do let(:guardian) { Guardian.new(user) } let(:uncategorized_category) { Category.find(SiteSetting.uncategorized_category_id) } + describe "#lookup" do + it "finds categories using their slug, downcasing for matches" do + result = described_class.lookup(guardian, ["movies"]).first + expect(result.ref).to eq("movies") + expect(result.slug).to eq("movies") + + result = described_class.lookup(guardian, ["BoOKs"]).first + expect(result.ref).to eq("books") + expect(result.slug).to eq("books") + end + + it "finds categories using the parent:child slug format" do + result = described_class.lookup(guardian, ["fun:random"]).first + expect(result.ref).to eq("fun:random") + expect(result.slug).to eq("random") + end + + it "does not find child categories by their standalone slug" do + expect(described_class.lookup(guardian, ["random"]).first).to eq(nil) + end + + it "does not find categories the user cannot access" do + expect(described_class.lookup(guardian, ["secret"]).first).to eq(nil) + group.add(user) + expect(described_class.lookup(Guardian.new(user), ["secret"]).first).not_to eq(nil) + end + end + + describe "#search" do + it "finds categories by partial name" do + result = described_class.search(guardian, "mov", 5).first + expect(result.ref).to eq("movies") + expect(result.slug).to eq("movies") + end + + it "finds categories by partial slug" do + result = described_class.search(guardian, "ook sec", 5).first + expect(result.ref).to eq("books") + expect(result.slug).to eq("books") + end + + it "does not find categories the user cannot access" do + expect(described_class.search(guardian, "secret", 5).first).to eq(nil) + group.add(user) + expect(described_class.search(Guardian.new(user), "secret", 5).first).not_to eq(nil) + end + + it "uses the correct ref format for a parent:child category that is found" do + result = described_class.search(guardian, "random", 5).first + expect(result.ref).to eq("fun:random") + expect(result.slug).to eq("random") + end + end + describe "#search_without_term" do it "returns distinct categories ordered by topic_count" do expect(described_class.search_without_term(guardian, 5).map(&:slug)).to eq( diff --git a/spec/services/hashtag_autocomplete_service_spec.rb b/spec/services/hashtag_autocomplete_service_spec.rb index 797ac062ba7..45a625e0c26 100644 --- a/spec/services/hashtag_autocomplete_service_spec.rb +++ b/spec/services/hashtag_autocomplete_service_spec.rb @@ -98,9 +98,8 @@ RSpec.describe HashtagAutocompleteService do it "does not search other data sources if the limit is reached by earlier type data sources" do # only expected once to try get the exact matches first - site_guardian_categories = Site.new(guardian).categories - Site.any_instance.expects(:categories).once.returns(site_guardian_categories) - subject.search("book", %w[tag category], limit: 1) + DiscourseTagging.expects(:filter_allowed_tags).never + subject.search("book", %w[category tag], limit: 1) end it "includes the tag count" do