FIX: Make inline oneboxes work with secured topics in secured contexts (#8895)

app/assets/javascripts/discourse/components/composer-editor.js.es6

@@ -542,7 +542,10 @@ export default Component.extend({
   },
 
   _loadInlineOneboxes(inline) {
-    applyInlineOneboxes(inline, ajax);
+    applyInlineOneboxes(inline, ajax, {
+      categoryId: this.get("composer.category.id"),
+      topicId: this.get("composer.topic.id")
+    });
   },
 
   _loadOneboxes(oneboxes) {

app/assets/javascripts/pretty-text/inline-oneboxer.js.es6

@@ -5,14 +5,20 @@ import {
 
 const _cache = {};
 
-export function applyInlineOneboxes(inline, ajax) {
+export function applyInlineOneboxes(inline, ajax, opts) {
+  opts = opts || {};
+
   Object.keys(inline).forEach(url => {
     // cache a blank locally, so we never trigger a lookup
     _cache[url] = {};
   });
 
   return ajax("/inline-onebox", {
-    data: { urls: Object.keys(inline) }
+    data: {
+      urls: Object.keys(inline),
+      category_id: opts.categoryId,
+      topic_id: opts.topicId
+    }
   }).then(result => {
     result["inline-oneboxes"].forEach(onebox => {
       if (onebox.title) {

app/controllers/inline_onebox_controller.rb

@@ -5,7 +5,12 @@ class InlineOneboxController < ApplicationController
 
   def show
     hijack do
-      oneboxes = InlineOneboxer.new(params[:urls] || []).process
+      oneboxes = InlineOneboxer.new(
+        params[:urls] || [],
+        user_id: current_user.id,
+        category_id: params[:category_id].to_i,
+        topic_id: params[:topic_id].to_i
+      ).process
       render json: { "inline-oneboxes" => oneboxes }
     end
   end

lib/cooked_post_processor.rb

@@ -710,7 +710,9 @@ class CookedPostProcessor
   def process_inline_onebox(element)
     inline_onebox = InlineOneboxer.lookup(
       element.attributes["href"].value,
-      invalidate: !!@opts[:invalidate_oneboxes]
+      invalidate: !!@opts[:invalidate_oneboxes],
+      user_id: @post&.user_id,
+      category_id: @post&.topic&.category_id
     )
 
     if title = inline_onebox&.dig(:title)

lib/inline_oneboxer.rb

@@ -33,11 +33,11 @@ class InlineOneboxer
     return unless url
 
     if route = Discourse.route_for(url)
-      if route[:controller] == "topics" &&
-        route[:action] == "show" &&
-        topic = Topic.where(id: route[:topic_id].to_i).first
-
-        return onebox_for(url, topic.title, opts) if Guardian.new.can_see?(topic)
+      if route[:controller] == "topics"
+        if topic = Oneboxer.local_topic(url, route, opts)
+          opts[:skip_cache] = true
+          return onebox_for(url, topic.title, opts)
+        end
       end
     end
 

lib/oneboxer.rb

@@ -208,15 +208,15 @@ module Oneboxer
     end
   end
 
-  def self.local_topic_html(url, route, opts)
-    return unless current_user = User.find_by(id: opts[:user_id])
+  def self.local_topic(url, route, opts)
+    if current_user = User.find_by(id: opts[:user_id])
+      if current_category = Category.find_by(id: opts[:category_id])
+        return unless Guardian.new(current_user).can_see_category?(current_category)
+      end
 
-    if current_category = Category.find_by(id: opts[:category_id])
-      return unless Guardian.new(current_user).can_see_category?(current_category)
-    end
-
-    if current_topic = Topic.find_by(id: opts[:topic_id])
-      return unless Guardian.new(current_user).can_see_topic?(current_topic)
+      if current_topic = Topic.find_by(id: opts[:topic_id])
+        return unless Guardian.new(current_user).can_see_topic?(current_topic)
+      end
     end
 
     topic = Topic.find_by(id: route[:topic_id])
@@ -224,10 +224,16 @@ module Oneboxer
     return unless topic
     return if topic.private_message?
 
-    if current_category&.id != topic.category_id
+    if current_category.blank? || current_category.id != topic.category_id
       return unless Guardian.new.can_see_topic?(topic)
     end
 
+    topic
+  end
+
+  def self.local_topic_html(url, route, opts)
+    return unless topic = local_topic(url, route, opts)
+
     post_number = route[:post_number].to_i
 
     post = post_number > 1 ?
@@ -236,7 +242,7 @@ module Oneboxer
 
     return if !post || post.hidden || !allowed_post_types.include?(post.post_type)
 
-    if post_number > 1 && current_topic&.id == topic.id
+    if post_number > 1 && opts[:topic_id] == topic.id
       excerpt = post.excerpt(SiteSetting.post_onebox_maxlength)
       excerpt.gsub!(/[\r\n]+/, " ")
       excerpt.gsub!("[/quote]", "[quote]") # don't break my quote

spec/components/inline_oneboxer_spec.rb

@@ -30,14 +30,19 @@ describe InlineOneboxer do
     end
 
     it "puts an entry in the cache" do
-      expect(InlineOneboxer.cache_lookup(topic.url)).to be_blank
+      SiteSetting.enable_inline_onebox_on_all_domains = true
+      url = "https://example.com/random-url"
+      stub_request(:get, url).to_return(status: 200, body: "<html><head><title>a blog</title></head></html>")
 
-      result = InlineOneboxer.lookup(topic.url)
+      InlineOneboxer.purge(url)
+      expect(InlineOneboxer.cache_lookup(url)).to be_blank
+
+      result = InlineOneboxer.lookup(url)
       expect(result).to be_present
 
-      cached = InlineOneboxer.cache_lookup(topic.url)
-      expect(cached[:url]).to eq(topic.url)
-      expect(cached[:title]).to eq(topic.title)
+      cached = InlineOneboxer.cache_lookup(url)
+      expect(cached[:url]).to eq(url)
+      expect(cached[:title]).to eq('a blog')
     end
 
     it "puts an entry in the cache for failed onebox" do
@@ -57,6 +62,39 @@ describe InlineOneboxer do
   end
 
   context ".lookup" do
+    let(:category) { Fabricate(:private_category, group: Group[:staff]) }
+    let(:category2) { Fabricate(:private_category, group: Group[:staff]) }
+
+    let(:admin) { Fabricate(:admin) }
+
+    it "can lookup private topics if in same category" do
+      topic = Fabricate(:topic, category: category)
+      topic1 = Fabricate(:topic, category: category)
+      topic2 = Fabricate(:topic, category: category2)
+
+      # Link to `topic` from new topic (same category)
+      onebox = InlineOneboxer.lookup(topic.url, user_id: admin.id, category_id: category.id, skip_cache: true)
+      expect(onebox).to be_present
+      expect(onebox[:url]).to eq(topic.url)
+      expect(onebox[:title]).to eq(topic.title)
+
+      # Link to `topic` from `topic`
+      onebox = InlineOneboxer.lookup(topic.url, user_id: admin.id, category_id: topic.category_id, topic_id: topic.id, skip_cache: true)
+      expect(onebox).to be_present
+      expect(onebox[:url]).to eq(topic.url)
+      expect(onebox[:title]).to eq(topic.title)
+
+      # Link to `topic` from `topic1` (same category)
+      onebox = InlineOneboxer.lookup(topic.url, user_id: admin.id, category_id: topic1.category_id, topic_id: topic1.id, skip_cache: true)
+      expect(onebox).to be_present
+      expect(onebox[:url]).to eq(topic.url)
+      expect(onebox[:title]).to eq(topic.title)
+
+      # Link to `topic` from `topic2` (different category)
+      onebox = InlineOneboxer.lookup(topic.url, user_id: admin.id, category_id: topic2.category_id, topic_id: topic2.id, skip_cache: true)
+      expect(onebox).to be_blank
+    end
+
     it "can lookup one link at a time" do
       topic = Fabricate(:topic)
       onebox = InlineOneboxer.lookup(topic.url, skip_cache: true)

spec/fabricators/category_fabricator.rb

@@ -10,23 +10,13 @@ Fabricator(:category_with_definition, from: :category) do
   skip_category_definition false
 end
 
-Fabricator(:diff_category, from: :category) do
-  name "Different Category"
-  user
-end
-
-Fabricator(:happy_category, from: :category) do
-  name 'Happy Category'
-  slug 'happy'
-  user
-end
-
 Fabricator(:private_category, from: :category) do
   transient :group
 
-  name 'Private Category'
-  slug 'private'
+  name { sequence(:name) { |n| "Private Category #{n}" } }
+  slug { sequence(:slug) { |n| "private#{n}" } }
   user
+
   after_build do |cat, transients|
     cat.update!(read_restricted: true)
     cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full])
@@ -36,6 +26,7 @@ end
 Fabricator(:private_category_with_definition, from: :private_category) do
   skip_category_definition false
 end
+
 Fabricator(:link_category, from: :category) do
   before_validation { |category, transients| category.topic_featured_link_allowed = true }
 end