From ec40242b5c93730da5d7f83b42fd51f284141ca3 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Wed, 12 Feb 2020 12:11:28 +0200 Subject: [PATCH] FIX: Make inline oneboxes work with secured topics in secured contexts (#8895) --- .../components/composer-editor.js.es6 | 5 +- .../pretty-text/inline-oneboxer.js.es6 | 10 +++- app/controllers/inline_onebox_controller.rb | 7 ++- lib/cooked_post_processor.rb | 4 +- lib/inline_oneboxer.rb | 10 ++-- lib/oneboxer.rb | 26 ++++++---- spec/components/inline_oneboxer_spec.rb | 48 +++++++++++++++++-- spec/fabricators/category_fabricator.rb | 17 ++----- 8 files changed, 89 insertions(+), 38 deletions(-) diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index 5dfa3b039a3..b600a1a43ff 100644 --- a/app/assets/javascripts/discourse/components/composer-editor.js.es6 +++ b/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) { diff --git a/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6 b/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6 index 6fe6cf32143..6645d1dac7e 100644 --- a/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6 +++ b/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) { diff --git a/app/controllers/inline_onebox_controller.rb b/app/controllers/inline_onebox_controller.rb index 16e4cf267b7..871be8c3080 100644 --- a/app/controllers/inline_onebox_controller.rb +++ b/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 diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 6936ac20887..b5f044e6658 100644 --- a/lib/cooked_post_processor.rb +++ b/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) diff --git a/lib/inline_oneboxer.rb b/lib/inline_oneboxer.rb index b76ebe848cf..edfd7465626 100644 --- a/lib/inline_oneboxer.rb +++ b/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 diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index 80b3085933b..3a18a9dda17 100644 --- a/lib/oneboxer.rb +++ b/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 diff --git a/spec/components/inline_oneboxer_spec.rb b/spec/components/inline_oneboxer_spec.rb index bdb08b2c917..d99bab6d425 100644 --- a/spec/components/inline_oneboxer_spec.rb +++ b/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: "a blog") - 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) diff --git a/spec/fabricators/category_fabricator.rb b/spec/fabricators/category_fabricator.rb index 0bf66a2cdf7..06563200b30 100644 --- a/spec/fabricators/category_fabricator.rb +++ b/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