From af9907bb503a2f99c87b4f572310888e6ebf9b28 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 8 Dec 2022 10:03:31 +1000 Subject: [PATCH] FIX: Duplicate hashtag lookup results based on permissions (#19337) When looking up hashtags which were conflicting (e.g. management::tag and management) where the user did not have permission for one of them, we ended up returning the one they did have permission to (e.g. the tag) twice because of the way the lookup fallback code worked. This fixes the issue, and another related one where the ::type was not added to the found item's .ref, and so the hashtag replacement on the client was not working correctly. --- .../discourse/app/lib/hashtag-autocomplete.js | 10 ++-------- app/services/hashtag_autocomplete_service.rb | 18 +++++++++++++++++- spec/lib/pretty_text/helpers_spec.rb | 4 ++-- spec/lib/pretty_text_spec.rb | 14 +++++--------- .../hashtag_autocomplete_service_spec.rb | 8 ++++++++ 5 files changed, 34 insertions(+), 20 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/hashtag-autocomplete.js b/app/assets/javascripts/discourse/app/lib/hashtag-autocomplete.js index 5d11a50ccf4..da3e020d7e2 100644 --- a/app/assets/javascripts/discourse/app/lib/hashtag-autocomplete.js +++ b/app/assets/javascripts/discourse/app/lib/hashtag-autocomplete.js @@ -221,19 +221,13 @@ function _searchRequest(term, contextualHashtagConfiguration, resultFunc) { } function _findAndReplaceSeenHashtagPlaceholder( - slug, + slugRef, contextualHashtagConfiguration, hashtagSpan ) { contextualHashtagConfiguration.forEach((type) => { - // remove type suffixes - const typePostfix = `::${type}`; - if (slug.endsWith(typePostfix)) { - slug = slug.slice(0, slug.length - typePostfix.length); - } - // replace raw span for the hashtag with a cooked one - const matchingSeenHashtag = seenHashtags[type]?.[slug]; + const matchingSeenHashtag = seenHashtags[type]?.[slugRef]; if (matchingSeenHashtag) { // NOTE: When changing the HTML structure here, you must also change // it in the hashtag-autocomplete markdown rule, and vice-versa. diff --git a/app/services/hashtag_autocomplete_service.rb b/app/services/hashtag_autocomplete_service.rb index 77fd2c62be9..66ed4ee72f4 100644 --- a/app/services/hashtag_autocomplete_service.rb +++ b/app/services/hashtag_autocomplete_service.rb @@ -122,7 +122,18 @@ class HashtagAutocompleteService # composer we want a slug without a suffix to be a category first, tag second. if slugs_without_suffixes.any? types_in_priority_order.each do |type| - found_from_slugs = execute_lookup!(lookup_results, type, guardian, slugs_without_suffixes) + # We do not want to continue fallback if there are conflicting slugs where + # one has a type and one does not, this may result in duplication. An + # example: + # + # A category with slug `management` is not found because of permissions + # and we also have a slug with suffix in the form of `management::tag`. + # There is a tag that exists with the `management` slug. The tag should + # not be found here but rather in the next lookup since it's got a more + # specific lookup with the type. + slugs_to_lookup = + slugs_without_suffixes.reject { |slug| slugs_with_suffixes.include?("#{slug}::#{type}") } + found_from_slugs = execute_lookup!(lookup_results, type, guardian, slugs_to_lookup) slugs_without_suffixes = slugs_without_suffixes - found_from_slugs.map(&:ref) break if slugs_without_suffixes.empty? @@ -139,6 +150,11 @@ class HashtagAutocompleteService .map { |slug| slug.gsub("::#{type}", "") } next if slugs_for_type.empty? execute_lookup!(lookup_results, type, guardian, slugs_for_type) + + # Make sure the refs are the same going out as they were going in. + lookup_results[type.to_sym].each do |item| + item.ref = "#{item.ref}::#{type}" if slugs_with_suffixes.include?("#{item.ref}::#{type}") + end end end diff --git a/spec/lib/pretty_text/helpers_spec.rb b/spec/lib/pretty_text/helpers_spec.rb index 34d478789f1..62ef9593327 100644 --- a/spec/lib/pretty_text/helpers_spec.rb +++ b/spec/lib/pretty_text/helpers_spec.rb @@ -62,7 +62,7 @@ RSpec.describe PrettyText::Helpers do description: "Coolest things ever", icon: "tag", slug: "somecooltag", - ref: "somecooltag", + ref: "somecooltag::tag", type: "tag", }, ) @@ -73,7 +73,7 @@ RSpec.describe PrettyText::Helpers do description: "Really great stuff here", icon: "folder", slug: "someawesomecategory", - ref: "someawesomecategory", + ref: "someawesomecategory::category", type: "category", }, ) diff --git a/spec/lib/pretty_text_spec.rb b/spec/lib/pretty_text_spec.rb index ea80323d036..8168d4b8161 100644 --- a/spec/lib/pretty_text_spec.rb +++ b/spec/lib/pretty_text_spec.rb @@ -1462,14 +1462,10 @@ RSpec.describe PrettyText do cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing", user_id: user.id) - [ - "#unknown::tag", - "known", - "known", - "testing" - ].each do |element| - expect(cooked).to include(element) - end + expect(cooked).to include("#unknown::tag") + expect(cooked).to include("known") + expect(cooked).to include("known") + expect(cooked).to include("testing") cooked = PrettyText.cook("[`a` #known::tag here](http://example.com)", user_id: user.id) @@ -1485,7 +1481,7 @@ RSpec.describe PrettyText do cooked = PrettyText.cook("test #known::tag", user_id: user.id) html = <<~HTML -

test known

+

test known

HTML expect(cooked).to eq(html.strip) diff --git a/spec/services/hashtag_autocomplete_service_spec.rb b/spec/services/hashtag_autocomplete_service_spec.rb index ef9b9d58692..b6393fb91a0 100644 --- a/spec/services/hashtag_autocomplete_service_spec.rb +++ b/spec/services/hashtag_autocomplete_service_spec.rb @@ -377,6 +377,14 @@ RSpec.describe HashtagAutocompleteService do expect(result[:bookmark].map(&:slug)).to eq(["coolrock"]) end + it "handles type suffix lookups where there is another type with a conflicting slug that the user cannot access" do + category1.update!(read_restricted: true) + Fabricate(:tag, name: "book-club") + result = subject.lookup(%w[book-club::tag book-club], %w[category tag]) + expect(result[:category].map(&:ref)).to eq([]) + expect(result[:tag].map(&:ref)).to eq(["book-club::tag"]) + end + context "when not tagging_enabled" do before { SiteSetting.tagging_enabled = false }