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.

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(
+    hashtagLookup(slug, options.userId, options.hashtagTypesInPriorityOrder);
-      slug,
-      options.currentUser,
-      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.

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

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) } if SiteSetting.slug_generation_method == "encoded"
-        slug_path.map! { |slug| CGI.escape(slug) }
-      end
 
       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
+    # @param {Array} categories - An array of Category models scoped to the user's guardian permissions.
-    #                                    should be used here since it is scoped to the Guardian.
+    def query_loaded_from_slugs(category_slugs, categories)
-    def query_from_cached_categories(category_slugs, cached_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|
+              categories.find do |cat|
-                cat[:slug].downcase == child_slug && cat[:parent_category_id] == parent_category[:id]
+                cat.slug.downcase == child_slug && cat.parent_category_id == parent_category.id
               end
             end
           else
-            cached_categories.find do |cat|
+            categories.find do |cat|
-              cat[:slug].downcase == parent_slug && cat[:parent_category_id].nil?
+              cat.slug.downcase == parent_slug && cat.top_level?
             end
           end
-        end.compact
+        end
+        .compact
     end
   end
 end

app/services/category_hashtag_data_source.rb

@@ -8,9 +8,7 @@ class CategoryHashtagDataSource
     "folder"
   end
 
-  def self.category_to_hashtag_item(parent_category, category)
+  def self.category_to_hashtag_item(category)
-    category = Category.new(category.slice(:id, :slug, :name, :parent_category_id, :description))
-
     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_categories = Category.secured(guardian).includes(:parent_category)
-    # user has access to.
-    guardian_categories = Site.new(guardian).categories
     Category
-      .query_from_cached_categories(slugs, guardian_categories)
+      .query_loaded_from_slugs(slugs, user_categories)
-      .map do |category|
+      .map { |category| category_to_hashtag_item(category) }
-        parent_category =
-          guardian_categories.find { |cat| cat[:id] == category[:parent_category_id] }
-        category_to_hashtag_item(parent_category, category)
-      end
   end
 
   def self.search(guardian, term, limit)
-    guardian_categories = Site.new(guardian).categories
+    Category
-
+      .secured(guardian)
-    guardian_categories
+      .includes(:parent_category)
-      .select do |category|
+      .where("LOWER(name) LIKE :term OR LOWER(slug) LIKE :term", term: "%#{term}%")
-        category[:name].downcase.include?(term) || category[:slug].downcase.include?(term)
-      end
       .take(limit)
-      .map do |category|
+      .map { |category| category_to_hashtag_item(category) }
-        parent_category =
-          guardian_categories.find { |cat| cat[:id] == category[:parent_category_id] }
-        category_to_hashtag_item(parent_category, category)
-      end
   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

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
 

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)

lib/pretty_text/shims.js

@@ -113,8 +113,8 @@ function __categoryLookup(c) {
   return __helpers.category_tag_hashtag_lookup(c);
 }
 
-function __hashtagLookup(slug, cookingUser, typesInPriorityOrder) {
+function __hashtagLookup(slug, cookingUserId, typesInPriorityOrder) {
-  return __helpers.hashtag_lookup(slug, cookingUser, typesInPriorityOrder);
+  return __helpers.hashtag_lookup(slug, cookingUserId, typesInPriorityOrder);
 }
 
 function __lookupAvatar(p) {

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

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')
+    category = Fabricate(:category, name: 'testing', slug: 'testing')
-    category2 = Fabricate(:category, name: 'known')
+    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("<span class=\"hashtag-raw\">#unknown::tag</span>")
     expect(cooked).to include("<a class=\"hashtag-cooked\" href=\"#{category2.url}\" data-type=\"category\" data-slug=\"known\"><svg class=\"fa d-icon d-icon-folder svg-icon svg-node\"><use href=\"#folder\"></use></svg><span>known</span></a>")
     expect(cooked).to include("<a class=\"hashtag-cooked\" href=\"/tag/known\" data-type=\"tag\" data-slug=\"known\" data-ref=\"known::tag\"><svg class=\"fa d-icon d-icon-tag svg-icon svg-node\"><use href=\"#tag\"></use></svg><span>known</span></a>")
     expect(cooked).to include("<a class=\"hashtag-cooked\" href=\"#{category.url}\" data-type=\"category\" data-slug=\"testing\"><svg class=\"fa d-icon d-icon-folder svg-icon svg-node\"><use href=\"#folder\"></use></svg><span>testing</span></a>")
+    expect(cooked).to include("<span class=\"hashtag-raw\">#secret</span>")
+
+    # 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("<a class=\"hashtag-cooked\" href=\"#{private_category.url}\" data-type=\"category\" data-slug=\"secret\"><svg class=\"fa d-icon d-icon-folder svg-icon svg-node\"><use href=\"#folder\"></use></svg><span>secret</span></a>")
 
     cooked = PrettyText.cook("[`a` #known::tag here](http://example.com)", user_id: user.id)
 

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!(:parent_category) { Fabricate(:category, slug: "fun") }
-  fab!(:category2) { Fabricate(:category, slug: "books", topic_count: 566) }
+  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(

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
+      DiscourseTagging.expects(:filter_allowed_tags).never
-      Site.any_instance.expects(:categories).once.returns(site_guardian_categories)
+      subject.search("book", %w[category tag], limit: 1)
-      subject.search("book", %w[tag category], limit: 1)
     end
 
     it "includes the tag count" do