diff --git a/app/assets/javascripts/discourse/views/composer/composer_view.js b/app/assets/javascripts/discourse/views/composer/composer_view.js index 26b223946..8621f22e2 100644 --- a/app/assets/javascripts/discourse/views/composer/composer_view.js +++ b/app/assets/javascripts/discourse/views/composer/composer_view.js @@ -282,10 +282,15 @@ Discourse.ComposerView = Discourse.View.extend(Ember.Evented, { // done $uploadTarget.on('fileuploaddone', function (e, data) { - var markdown = Discourse.Utilities.getUploadMarkdown(data.result); - // appends a space at the end of the inserted markdown - composerView.addMarkdown(markdown + " "); - composerView.set('isUploading', false); + // make sure we have a url + if (data.result.url) { + var markdown = Discourse.Utilities.getUploadMarkdown(data.result); + // appends a space at the end of the inserted markdown + composerView.addMarkdown(markdown + " "); + composerView.set('isUploading', false); + } else { + bootbox.alert(I18n.t('post.errors.upload')); + } }); // fail diff --git a/app/assets/javascripts/discourse/views/modal/avatar_selector_view.js b/app/assets/javascripts/discourse/views/modal/avatar_selector_view.js index e15761e72..9cf871ce1 100644 --- a/app/assets/javascripts/discourse/views/modal/avatar_selector_view.js +++ b/app/assets/javascripts/discourse/views/modal/avatar_selector_view.js @@ -54,20 +54,25 @@ Discourse.AvatarSelectorView = Discourse.ModalBodyView.extend({ // when the upload is successful $upload.on("fileuploaddone", function (e, data) { - // indicates the users is using an uploaded avatar - self.get("controller").setProperties({ - has_uploaded_avatar: true, - use_uploaded_avatar: true - }); - // display a warning whenever the image is not a square - self.set("imageIsNotASquare", data.result.width !== data.result.height); - // in order to be as much responsive as possible, we're cheating a bit here - // indeed, the server gives us back the url to the file we've just uploaded - // often, this file is not a square, so we need to crop it properly - // this will also capture the first frame of animated avatars when they're not allowed - Discourse.Utilities.cropAvatar(data.result.url, data.files[0].type).then(function(avatarTemplate) { - self.get("controller").set("uploaded_avatar_template", avatarTemplate); - }); + // make sure we have a url + if (data.result.url) { + // indicates the users is using an uploaded avatar + self.get("controller").setProperties({ + has_uploaded_avatar: true, + use_uploaded_avatar: true + }); + // display a warning whenever the image is not a square + self.set("imageIsNotASquare", data.result.width !== data.result.height); + // in order to be as much responsive as possible, we're cheating a bit here + // indeed, the server gives us back the url to the file we've just uploaded + // often, this file is not a square, so we need to crop it properly + // this will also capture the first frame of animated avatars when they're not allowed + Discourse.Utilities.cropAvatar(data.result.url, data.files[0].type).then(function(avatarTemplate) { + self.get("controller").set("uploaded_avatar_template", avatarTemplate); + }); + } else { + bootbox.alert(I18n.t('post.errors.upload')); + } }); // when there has been an error with the upload diff --git a/app/jobs/regular/generate_avatars.rb b/app/jobs/regular/generate_avatars.rb index a2a23d3b6..9614e24a0 100644 --- a/app/jobs/regular/generate_avatars.rb +++ b/app/jobs/regular/generate_avatars.rb @@ -5,15 +5,15 @@ module Jobs class GenerateAvatars < Jobs::Base def execute(args) - upload_id = args[:upload_id] - raise Discourse::InvalidParameters.new(:upload_id) unless upload_id.present? + raise Discourse::ImageMagickMissing.new unless system("command -v convert >/dev/null;") - user_id = args[:user_id] - raise Discourse::InvalidParameters.new(:user_id) unless user_id.present? + upload_id, user_id = args[:upload_id], args[:user_id] + raise Discourse::InvalidParameters.new(:upload_id) if upload_id.blank? + raise Discourse::InvalidParameters.new(:user_id) if user_id.blank? upload = Upload.where(id: upload_id).first user = User.where(id: user_id).first - return unless upload.present? || user.present? + return if upload.nil? || user.nil? external_copy = Discourse.store.download(upload) if Discourse.store.external? original_path = if Discourse.store.external? @@ -22,22 +22,34 @@ module Jobs Discourse.store.path_for(upload) end - # we'll extract the first frame when it's a gif source = original_path + # extract the first frame when it's a gif source << "[0]" unless SiteSetting.allow_animated_avatars + image = ImageSorcery.new(source) + extension = File.extname(original_path) [120, 45, 32, 25, 20].each do |s| # handle retina too [s, s * 2].each do |size| - # create a temp file with the same extension as the original - temp_file = Tempfile.new(["discourse-avatar", File.extname(original_path)]) - temp_path = temp_file.path - # create a centered square thumbnail - if ImageSorcery.new(source).convert(temp_path, gravity: "center", thumbnail: "#{size}x#{size}^", extent: "#{size}x#{size}", background: "transparent") - Discourse.store.store_avatar(temp_file, upload, size) + begin + # create a temp file with the same extension as the original + temp_file = Tempfile.new(["discourse-avatar", extension]) + # create a transparent centered square thumbnail + if image.convert(temp_file.path, + gravity: "center", + thumbnail: "#{size}x#{size}^", + extent: "#{size}x#{size}", + background: "transparent") + if Discourse.store.store_avatar(temp_file, upload, size).blank? + Rails.logger.error("Failed to store avatar #{size} for #{upload.url} from #{source}") + end + else + Rails.logger.error("Failed to create avatar #{size} for #{upload.url} from #{source}") + end + ensure + # close && remove temp file + temp_file && temp_file.close! end - # close && remove temp file - temp_file.close! end end diff --git a/app/jobs/regular/pull_hotlinked_images.rb b/app/jobs/regular/pull_hotlinked_images.rb new file mode 100644 index 000000000..7d29923ee --- /dev/null +++ b/app/jobs/regular/pull_hotlinked_images.rb @@ -0,0 +1,100 @@ +module Jobs + + class PullHotlinkedImages < Jobs::Base + + def initialize + # maximum size of the file in bytes + @max_size = SiteSetting.max_image_size_kb * 1024 + end + + def execute(args) + # we don't want to run the job if we're not allowed to crawl images + return unless SiteSetting.crawl_images? + + post_id = args[:post_id] + raise Discourse::InvalidParameters.new(:post_id) unless post_id.present? + + post = Post.where(id: post_id).first + return unless post.present? + + raw = post.raw.dup + downloaded_urls = {} + + extract_images_from(post.cooked).each do |image| + src = image['src'] + + if is_valid_image_url(src) + begin + # have we already downloaded that file? + if !downloaded_urls.include?(src) + hotlinked = download(src) + if hotlinked.size <= @max_size + filename = File.basename(URI.parse(src).path) + file = ActionDispatch::Http::UploadedFile.new(tempfile: hotlinked, filename: filename) + upload = Upload.create_for(post.user_id, file, hotlinked.size, src) + downloaded_urls[src] = upload.url + else + Rails.logger.warn("Failed to pull hotlinked image: #{src} - Image is bigger than #{@max_size}") + end + end + # have we successfuly downloaded that file? + if downloaded_urls[src].present? + url = downloaded_urls[src] + escaped_src = src.gsub("?", "\\?").gsub(".", "\\.").gsub("+", "\\+") + # there are 5 ways to insert an image in a post + # HTML tag - + raw.gsub!(/src=["']#{escaped_src}["']/i, "src='#{url}'") + # BBCode tag - [img]http://...[/img] + raw.gsub!(/\[img\]#{escaped_src}\[\/img\]/i, "[img]#{url}[/img]") + # Markdown inline - ![alt](http://...) + raw.gsub!(/!\[([^\]]*)\]\(#{escaped_src}\)/) { "![#{$1}](#{url})" } + # Markdown reference - [x]: http:// + raw.gsub!(/\[(\d+)\]: #{escaped_src}/) { "[#{$1}]: #{url}" } + # Direct link + raw.gsub!(src, "") + end + rescue => e + Rails.logger.error("Failed to pull hotlinked image: #{src}\n" + e.message + "\n" + e.backtrace.join("\n")) + ensure + # close & delete the temp file + hotlinked && hotlinked.close! + end + end + + end + + # TODO: make sure the post hasnĀ“t changed while we were downloading remote images + if raw != post.raw + options = { force_new_version: true } + post.revise(Discourse.system_user, raw, options) + end + + end + + def extract_images_from(html) + doc = Nokogiri::HTML::fragment(html) + doc.css("img") - doc.css(".onebox-result img") - doc.css("img.avatar") + end + + def is_valid_image_url(src) + src.present? && !Discourse.store.has_been_uploaded?(src) + end + + def download(url) + extension = File.extname(URI.parse(url).path) + tmp = Tempfile.new(["discourse-hotlinked", extension]) + + File.open(tmp.path, "wb") do |f| + hotlinked = open(url, "rb", read_timeout: 5) + while f.size <= @max_size && data = hotlinked.read(@max_size) + f.write(data) + end + hotlinked.close! + end + + tmp + end + + end + +end diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 6fc88dd82..e25e4b460 100644 --- a/app/models/optimized_image.rb +++ b/app/models/optimized_image.rb @@ -4,40 +4,60 @@ class OptimizedImage < ActiveRecord::Base belongs_to :upload def self.create_for(upload, width, height) - return unless width && height + return unless width > 0 && height > 0 - @image_sorcery_loaded ||= require "image_sorcery" + # do we already have that thumbnail? + thumbnail = where(upload_id: upload.id, width: width, height: height).first - external_copy = Discourse.store.download(upload) if Discourse.store.external? - original_path = if Discourse.store.external? - external_copy.path - else - Discourse.store.path_for(upload) + # make sure the previous thumbnail has not failed + if thumbnail && thumbnail.url.blank? + thumbnail.destroy + thumbnail = nil end - # create a temp file with the same extension as the original - temp_file = Tempfile.new(["discourse-thumbnail", File.extname(original_path)]) - temp_path = temp_file.path + # create the thumbnail otherwise + unless thumbnail + @image_sorcery_loaded ||= require "image_sorcery" - if ImageSorcery.new("#{original_path}[0]").convert(temp_path, resize: "#{width}x#{height}!") - thumbnail = OptimizedImage.create!( - upload_id: upload.id, - sha1: Digest::SHA1.file(temp_path).hexdigest, - extension: File.extname(temp_path), - width: width, - height: height, - url: "", - ) - # store the optimized image and update its url - thumbnail.url = Discourse.store.store_optimized_image(temp_file, thumbnail) - thumbnail.save + external_copy = Discourse.store.download(upload) if Discourse.store.external? + original_path = if Discourse.store.external? + external_copy.path + else + Discourse.store.path_for(upload) + end + + # create a temp file with the same extension as the original + extension = File.extname(original_path) + temp_file = Tempfile.new(["discourse-thumbnail", extension]) + temp_path = temp_file.path + + if ImageSorcery.new("#{original_path}[0]").convert(temp_path, resize: "#{width}x#{height}!") + thumbnail = OptimizedImage.create!( + upload_id: upload.id, + sha1: Digest::SHA1.file(temp_path).hexdigest, + extension: File.extname(temp_path), + width: width, + height: height, + url: "", + ) + # store the optimized image and update its url + url = Discourse.store.store_optimized_image(temp_file, thumbnail) + if url.present? + thumbnail.url = url + thumbnail.save + else + Rails.logger.error("Failed to store avatar #{size} for #{upload.url} from #{source}") + end + else + Rails.logger.error("Failed to create optimized image #{width}x#{height} for #{upload.url}") + end + + # close && remove temp file + temp_file.close! + # make sure we remove the cached copy from external stores + external_copy.close! if Discourse.store.external? end - # close && remove temp file - temp_file.close! - # make sure we remove the cached copy from external stores - external_copy.close! if Discourse.store.external? - thumbnail end diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 9eb2f451b..009b1321c 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -138,8 +138,7 @@ class PostAction < ActiveRecord::Base def self.remove_act(user, post, post_action_type_id) if action = where(post_id: post.id, user_id: user.id, - post_action_type_id: - post_action_type_id).first + post_action_type_id: post_action_type_id).first action.remove_act!(user) end end diff --git a/app/models/upload.rb b/app/models/upload.rb index 68b291be9..051dceb5a 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -1,12 +1,10 @@ -require 'digest/sha1' -require 'image_sizer' -require 'tempfile' -require 'pathname' +require "digest/sha1" +require "image_sizer" class Upload < ActiveRecord::Base belongs_to :user - has_many :post_uploads + has_many :post_uploads, dependent: :destroy has_many :posts, through: :post_uploads has_many :optimized_images, dependent: :destroy @@ -14,19 +12,16 @@ class Upload < ActiveRecord::Base validates_presence_of :filesize validates_presence_of :original_filename - def thumbnail(width = nil, height = nil) - width ||= self.width - height ||= self.height + def thumbnail(width = self.width, height = self.height) optimized_images.where(width: width, height: height).first end - def has_thumbnail?(width = nil, height = nil) + def has_thumbnail?(width, height) thumbnail(width, height).present? end def create_thumbnail!(width, height) return unless SiteSetting.create_thumbnails? - return if has_thumbnail?(width, height) thumbnail = OptimizedImage.create_for(self, width, height) if thumbnail optimized_images << thumbnail @@ -47,12 +42,19 @@ class Upload < ActiveRecord::Base File.extname(original_filename) end - def self.create_for(user_id, file, filesize) + def self.create_for(user_id, file, filesize, origin = nil) # compute the sha sha1 = Digest::SHA1.file(file.tempfile).hexdigest # check if the file has already been uploaded - unless upload = Upload.where(sha1: sha1).first - # deal with width & heights for images + upload = Upload.where(sha1: sha1).first + # delete the previously uploaded file if there's been an error + if upload && upload.url.blank? + upload.destroy + upload = nil + end + # create the upload + unless upload + # deal with width & height for images if SiteSetting.authorized_image?(file) # retrieve image info image_info = FastImage.new(file.tempfile, raise_on_failure: true) @@ -61,6 +63,8 @@ class Upload < ActiveRecord::Base # make sure we're at the beginning of the file (FastImage is moving the pointer) file.rewind end + # trim the origin if any + origin = origin[0...1000] if origin # create a db record (so we can use the id) upload = Upload.create!( user_id: user_id, @@ -70,11 +74,16 @@ class Upload < ActiveRecord::Base url: "", width: width, height: height, + origin: origin, ) # store the file and update its url - upload.url = Discourse.store.store_upload(file, upload) - # save the url - upload.save + url = Discourse.store.store_upload(file, upload) + if url.present? + upload.url = url + upload.save + else + Rails.logger.error("Failed to store upload ##{upload.id} for user ##{user_id}") + end end # return the uploaded file upload @@ -82,8 +91,7 @@ class Upload < ActiveRecord::Base def self.get_from_url(url) # we store relative urls, so we need to remove any host/cdn - asset_host = Rails.configuration.action_controller.asset_host - url = url.gsub(/^#{asset_host}/i, "") if asset_host.present? + url = url.gsub(/^#{Discourse.asset_host}/i, "") if Discourse.asset_host.present? Upload.where(url: url).first if Discourse.store.has_been_uploaded?(url) end diff --git a/db/migrate/20131105101051_add_origin_to_uploads.rb b/db/migrate/20131105101051_add_origin_to_uploads.rb new file mode 100644 index 000000000..dcbe31b12 --- /dev/null +++ b/db/migrate/20131105101051_add_origin_to_uploads.rb @@ -0,0 +1,5 @@ +class AddOriginToUploads < ActiveRecord::Migration + def change + add_column :uploads, :origin, :string, limit: 1000 + end +end diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 4eb0cd6dc..9c02d6810 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -1,7 +1,7 @@ # Post processing that we can do after a post has already been cooked. # For example, inserting the onebox content, or image sizes/thumbnails. -require_dependency 'oneboxer' +require_dependency "oneboxer" class CookedPostProcessor include ActionView::Helpers::NumberHelper @@ -12,27 +12,38 @@ class CookedPostProcessor @post = post @doc = Nokogiri::HTML::fragment(post.cooked) @size_cache = {} - @has_been_uploaded_cache = {} end def post_process - clean_up_reverse_index - post_process_attachments + keep_reverse_index_up_to_date post_process_images post_process_oneboxes + optimize_urls + pull_hotlinked_images end - def clean_up_reverse_index - PostUpload.delete_all(post_id: @post.id) - end + def keep_reverse_index_up_to_date + upload_ids = Set.new - def post_process_attachments - attachments.each do |attachment| - href = attachment['href'] - attachment['href'] = relative_to_absolute(href) - # update reverse index + @doc.search("a").each do |a| + href = a["href"].to_s if upload = Upload.get_from_url(href) - associate_to_post(upload) + upload_ids << upload.id + end + end + + @doc.search("img").each do |img| + src = img["src"].to_s + if upload = Upload.get_from_url(src) + upload_ids << upload.id + end + end + + values = upload_ids.map{ |u| "(#{@post.id},#{u})" }.join(",") + PostUpload.transaction do + PostUpload.delete_all(post_id: @post.id) + if upload_ids.length > 0 + PostUpload.exec_sql("INSERT INTO post_uploads (post_id, upload_id) VALUES #{values}") end end end @@ -42,78 +53,53 @@ class CookedPostProcessor return if images.blank? images.each do |img| - if img['src'].present? - # keep track of the original src - src = img['src'] - # make sure the src is absolute (when working with locally uploaded files) - img['src'] = relative_to_absolute(src) - # make sure the img has proper width and height attributes - update_dimensions!(img) - # retrieve the associated upload, if any - if upload = Upload.get_from_url(src) - # update reverse index - associate_to_post(upload) - end - # lightbox treatment - convert_to_link!(img, upload) - # mark the post as dirty whenever the src has changed - @dirty |= src != img['src'] - end + src, width, height = img["src"], img["width"], img["height"] + limit_size!(img) + convert_to_link!(img) + @dirty |= (src != img["src"]) || (width.to_i != img["width"].to_i) || (height.to_i != img["height"].to_i) end - # Extract the first image from the first post and use it as the 'topic image' - extract_topic_image(images) - end - - def post_process_oneboxes - args = { post_id: @post.id } - args[:invalidate_oneboxes] = true if @opts[:invalidate_oneboxes] - # bake onebox content into the post - result = Oneboxer.apply(@doc) do |url, element| - Oneboxer.onebox(url, args) - end - # mark the post as dirty whenever a onebox as been baked - @dirty |= result.changed? + update_topic_image(images) end def extract_images - # do not extract images inside a onebox or a quote + # do not extract images inside oneboxes or quotes @doc.css("img") - @doc.css(".onebox-result img") - @doc.css(".quote img") end - def relative_to_absolute(src) - if src =~ /^\/[^\/]/ - Discourse.base_url_no_prefix + src - else - src + def limit_size!(img) + w, h = get_size_from_image_sizes(img["src"], @opts[:image_sizes]) || get_size(img["src"]) + # limit the size of the thumbnail + img["width"], img["height"] = ImageSizer.resize(w, h) + end + + def get_size_from_image_sizes(src, image_sizes) + return unless image_sizes.present? + image_sizes.each do |image_size| + url, size = image_size[0], image_size[1] + return [size["width"], size["height"]] if url.include?(src) end end - def update_dimensions!(img) - w, h = get_size_from_image_sizes(img['src'], @opts[:image_sizes]) || get_size(img['src']) - # make sure we limit the size of the thumbnail - w, h = ImageSizer.resize(w, h) - # check whether the dimensions have changed - @dirty = (img['width'].to_i != w) || (img['height'].to_i != h) - # update the dimensions - img['width'] = w - img['height'] = h + def get_size(url) + absolute_url = url + absolute_url = Discourse.base_url_no_prefix + absolute_url if absolute_url =~ /^\/[^\/]/ + # FastImage fails when there's no scheme + absolute_url = (SiteSetting.use_ssl? ? "https:" : "http:") + absolute_url if absolute_url.start_with?("//") + return unless is_valid_image_url?(absolute_url) + # we can *always* crawl our own images + return unless SiteSetting.crawl_images? || Discourse.store.has_been_uploaded?(url) + @size_cache[url] ||= FastImage.size(absolute_url) + rescue Zlib::BufError # FastImage.size raises BufError for some gifs end - def associate_to_post(upload) - return if PostUpload.where(post_id: @post.id, upload_id: upload.id).count > 0 - PostUpload.create(post_id: @post.id, upload_id: upload.id) - rescue ActiveRecord::RecordNotUnique - # do not care if it's already associated + def is_valid_image_url?(url) + uri = URI.parse(url) + %w(http https).include? uri.scheme + rescue URI::InvalidURIError end - def optimize_image!(img) - # TODO - # 1) optimize using image_optim - # 2) .png vs. .jpg (> 1.5x) - end - - def convert_to_link!(img, upload=nil) + def convert_to_link!(img) src = img["src"] return unless src.present? @@ -123,12 +109,10 @@ class CookedPostProcessor return if original_width.to_i <= width && original_height.to_i <= height return if original_width.to_i <= SiteSetting.max_image_width && original_height.to_i <= SiteSetting.max_image_height - return if is_a_hyperlink(img) + return if is_a_hyperlink?(img) - if upload - # create a thumbnail + if upload = Upload.get_from_url(src) upload.create_thumbnail!(width, height) - # optimize image # TODO: optimize_image!(img) end @@ -137,7 +121,7 @@ class CookedPostProcessor @dirty = true end - def is_a_hyperlink(img) + def is_a_hyperlink?(img) parent = img.parent while parent return if parent.name == "a" @@ -155,21 +139,20 @@ class CookedPostProcessor # then, the link to our larger image a = Nokogiri::XML::Node.new("a", @doc) img.add_next_sibling(a) - a["href"] = img['src'] + a["href"] = img["src"] a["class"] = "lightbox" a.add_child(img) # replace the image by its thumbnail - w = img["width"] - h = img["height"] - img['src'] = relative_to_absolute(upload.thumbnail(w, h).url) if upload && upload.has_thumbnail?(w, h) + w, h = img["width"].to_i, img["height"].to_i + img["src"] = upload.thumbnail(w, h).url if upload && upload.has_thumbnail?(w, h) # then, some overlay informations meta = Nokogiri::XML::Node.new("div", @doc) meta["class"] = "meta" img.add_next_sibling(meta) - filename = get_filename(upload, img['src']) + filename = get_filename(upload, img["src"]) informations = "#{original_width}x#{original_height}" informations << " #{number_to_human_size(upload.filesize)}" if upload @@ -181,48 +164,70 @@ class CookedPostProcessor def get_filename(upload, src) return File.basename(src) unless upload return upload.original_filename unless upload.original_filename =~ /^blob(\.png)?$/i - return I18n.t('upload.pasted_image_filename') + return I18n.t("upload.pasted_image_filename") end def create_span_node(klass, content=nil) span = Nokogiri::XML::Node.new("span", @doc) span.content = content if content - span['class'] = klass + span["class"] = klass span end - def extract_topic_image(images) + def update_topic_image(images) if @post.post_number == 1 img = images.first - @post.topic.update_column :image_url, img['src'] if img['src'].present? + @post.topic.update_column(:image_url, img["src"]) if img["src"].present? end end - def get_size_from_image_sizes(src, image_sizes) - [image_sizes[src]["width"], image_sizes[src]["height"]] if image_sizes.present? && image_sizes[src].present? + def post_process_oneboxes + args = { + post_id: @post.id, + invalidate_oneboxes: !!@opts[:invalidate_oneboxes], + } + + result = Oneboxer.apply(@doc) do |url, element| + Oneboxer.onebox(url, args) + end + + @dirty |= result.changed? end - def get_size(url) - uri = url - # make sure urls have a scheme (otherwise, FastImage will fail) - uri = (SiteSetting.use_ssl? ? "https:" : "http:") + url if url && url.start_with?("//") - return unless is_valid_image_uri?(uri) - # we can *always* crawl our own images - return unless SiteSetting.crawl_images? || Discourse.store.has_been_uploaded?(url) - @size_cache[url] ||= FastImage.size(uri) - rescue Zlib::BufError # FastImage.size raises BufError for some gifs + def optimize_urls + @doc.search("a").each do |a| + href = a["href"].to_s + if Discourse.store.has_been_uploaded?(href) + a["href"] = schemaless relative_to_absolute(href) + end + end + + @doc.search("img").each do |img| + src = img["src"].to_s + if Discourse.store.has_been_uploaded?(src) + img["src"] = schemaless relative_to_absolute(src) + end + end end - def is_valid_image_uri?(url) - uri = URI.parse(url) - %w(http https).include? uri.scheme - rescue URI::InvalidURIError + def relative_to_absolute(url) + url =~ /^\/[^\/]/ ? (Discourse.asset_host || Discourse.base_url_no_prefix) + url : url end - def attachments - attachments = @doc.css("a.attachment[href^=\"#{Discourse.store.absolute_base_url}\"]") - attachments += @doc.css("a.attachment[href^=\"#{Discourse.store.relative_base_url}\"]") if Discourse.store.internal? - attachments + def schemaless(url) + url.gsub(/^https?:/, "") + end + + def pull_hotlinked_images + # we don't want to run the job if we're not allowed to crawl images + return unless SiteSetting.crawl_images? + # we only want to run the job whenever it's changed by a user + return if @post.updated_by == Discourse.system_user + # make sure no other job is scheduled + Jobs.cancel_scheduled_job(:pull_hotlinked_images, post_id: @post.id) + # schedule the job + delay = SiteSetting.ninja_edit_window + 1 + Jobs.enqueue_in(delay.minutes.to_i, :pull_hotlinked_images, post_id: @post.id) end def dirty? diff --git a/lib/discourse.rb b/lib/discourse.rb index 0f71095d6..2a8e9df98 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -22,6 +22,9 @@ module Discourse # When a setting is missing class SiteSettingMissing < Exception; end + # When ImageMagick is missing + class ImageMagickMissing < Exception; end + # Cross site request forgery class CSRF < Exception; end @@ -72,11 +75,11 @@ module Discourse end end - def self.base_uri default_value="" + def self.base_uri(default_value = "") if !ActionController::Base.config.relative_url_root.blank? - return ActionController::Base.config.relative_url_root + ActionController::Base.config.relative_url_root else - return default_value + default_value end end @@ -142,10 +145,10 @@ module Discourse def self.store if SiteSetting.enable_s3_uploads? @s3_store_loaded ||= require 'file_store/s3_store' - S3Store.new + FileStore::S3Store.new else @local_store_loaded ||= require 'file_store/local_store' - LocalStore.new + FileStore::LocalStore.new end end @@ -157,6 +160,10 @@ module Discourse @current_user_provider = val end + def self.asset_host + Rails.configuration.action_controller.asset_host + end + private def self.maintenance_mode_key diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb new file mode 100644 index 000000000..b139f62bd --- /dev/null +++ b/lib/file_store/base_store.rb @@ -0,0 +1,46 @@ +module FileStore + + class BaseStore + + def store_upload(file, upload) + end + + def store_optimized_image(file, optimized_image) + end + + def store_avatar(file, avatar, size) + end + + def remove_upload(upload) + end + + def remove_optimized_image(optimized_image) + end + + def has_been_uploaded?(url) + end + + def absolute_base_url + end + + def relative_base_url + end + + def external? + end + + def internal? + end + + def path_for(upload) + end + + def download(upload) + end + + def absolute_avatar_template(avatar) + end + + end + +end diff --git a/lib/file_store/local_store.rb b/lib/file_store/local_store.rb index a3bcd2682..22b6e2bef 100644 --- a/lib/file_store/local_store.rb +++ b/lib/file_store/local_store.rb @@ -1,149 +1,139 @@ -class LocalStore +require 'file_store/base_store' - def store_upload(file, upload) - path = get_path_for_upload(file, upload) - store_file(file, path) - end +module FileStore - def store_optimized_image(file, optimized_image) - path = get_path_for_optimized_image(file, optimized_image) - store_file(file, path) - end + class LocalStore < BaseStore - def store_avatar(file, upload, size) - path = get_path_for_avatar(file, upload, size) - store_file(file, path) - end - - def remove_upload(upload) - remove_file(upload.url) - end - - def remove_optimized_image(optimized_image) - remove_file(optimized_image.url) - end - - def remove_avatars(upload) - return unless upload.url =~ /avatars/ - remove_directory(File.dirname(upload.url)) - end - - def has_been_uploaded?(url) - is_relative?(url) || is_local?(url) - end - - def absolute_base_url - url = asset_host.present? ? asset_host : Discourse.base_url_no_prefix - "#{url}#{relative_base_url}" - end - - def relative_base_url - "/uploads/#{RailsMultisite::ConnectionManagement.current_db}" - end - - def external? - !internal? - end - - def internal? - true - end - - def path_for(upload) - "#{public_dir}#{upload.url}" - end - - def absolute_avatar_template(upload) - avatar_template(upload, absolute_base_url) - end - - private - - def get_path_for_upload(file, upload) - unique_sha1 = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0..15] - extension = File.extname(file.original_filename) - clean_name = "#{unique_sha1}#{extension}" - # path - "#{relative_base_url}/#{upload.id}/#{clean_name}" - end - - def get_path_for_optimized_image(file, optimized_image) - # 1234567890ABCDEF_100x200.jpg - filename = [ - optimized_image.sha1[6..15], - "_#{optimized_image.width}x#{optimized_image.height}", - optimized_image.extension, - ].join - # /uploads//_optimized/<1A3>// - File.join( - relative_base_url, - "_optimized", - optimized_image.sha1[0..2], - optimized_image.sha1[3..5], - filename - ) - end - - def get_path_for_avatar(file, upload, size) - relative_avatar_template(upload).gsub("{size}", size.to_s) - end - - def relative_avatar_template(upload) - avatar_template(upload, relative_base_url) - end - - def avatar_template(upload, base_url) - File.join( - base_url, - "avatars", - upload.sha1[0..2], - upload.sha1[3..5], - upload.sha1[6..15], - "{size}#{upload.extension}" - ) - end - - def store_file(file, path) - # copy the file to the right location - copy_file(file, "#{public_dir}#{path}") - # url - Discourse.base_uri + path - end - - def copy_file(file, path) - FileUtils.mkdir_p Pathname.new(path).dirname - # move the file to the right location - # not using cause mv, cause permissions are no good on move - File.open(path, "wb") do |f| - f.write(file.read) + def store_upload(file, upload) + path = get_path_for_upload(file, upload) + store_file(file, path) end - end - def remove_file(url) - File.delete("#{public_dir}#{url}") if has_been_uploaded?(url) - rescue Errno::ENOENT - # don't care if the file isn't there - end + def store_optimized_image(file, optimized_image) + path = get_path_for_optimized_image(file, optimized_image) + store_file(file, path) + end - def remove_directory(path) - directory = "#{public_dir}/#{path}" - FileUtils.rm_rf(directory) - end + def store_avatar(file, avatar, size) + path = get_path_for_avatar(file, avatar, size) + store_file(file, path) + end - def is_relative?(url) - url.start_with?(relative_base_url) - end + def remove_upload(upload) + remove_file(upload.url) + end - def is_local?(url) - url.start_with?(absolute_base_url) - end + def remove_optimized_image(optimized_image) + remove_file(optimized_image.url) + end - def public_dir - "#{Rails.root}/public" - end + def has_been_uploaded?(url) + is_relative?(url) || is_local?(url) + end + + def absolute_base_url + "#{Discourse.base_url_no_prefix}#{relative_base_url}" + end + + def relative_base_url + "/uploads/#{RailsMultisite::ConnectionManagement.current_db}" + end + + def external? + !internal? + end + + def internal? + true + end + + def path_for(upload) + "#{public_dir}#{upload.url}" + end + + def absolute_avatar_template(avatar) + avatar_template(avatar, absolute_base_url) + end + + private + + def get_path_for_upload(file, upload) + unique_sha1 = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0..15] + extension = File.extname(file.original_filename) + clean_name = "#{unique_sha1}#{extension}" + # path + "#{relative_base_url}/#{upload.id}/#{clean_name}" + end + + def get_path_for_optimized_image(file, optimized_image) + # 1234567890ABCDEF_100x200.jpg + filename = [ + optimized_image.sha1[6..15], + "_#{optimized_image.width}x#{optimized_image.height}", + optimized_image.extension, + ].join + # path + "#{relative_base_url}/_optimized/#{optimized_image.sha1[0..2]}/#{optimized_image.sha1[3..5]}/#{filename}" + end + + def get_path_for_avatar(file, avatar, size) + relative_avatar_template(avatar).gsub("{size}", size.to_s) + end + + def relative_avatar_template(avatar) + avatar_template(avatar, relative_base_url) + end + + def avatar_template(avatar, base_url) + File.join( + base_url, + "avatars", + avatar.sha1[0..2], + avatar.sha1[3..5], + avatar.sha1[6..15], + "{size}#{avatar.extension}" + ) + end + + def store_file(file, path) + # copy the file to the right location + copy_file(file, "#{public_dir}#{path}") + # url + "#{Discourse.base_uri}#{path}" + end + + def copy_file(file, path) + FileUtils.mkdir_p(Pathname.new(path).dirname) + # move the file to the right location + # not using cause mv, cause permissions are no good on move + File.open(path, "wb") do |f| + f.write(file.read) + end + end + + def remove_file(url) + File.delete("#{public_dir}#{url}") if is_relative?(url) + rescue Errno::ENOENT + # don't care if the file isn't there + end + + def is_relative?(url) + url.start_with?(relative_base_url) + end + + def is_local?(url) + absolute_url = url.start_with?("//") ? (SiteSetting.use_ssl? ? "https:" : "http:") + url : url + absolute_url.start_with?(absolute_base_url) || absolute_url.start_with?(absolute_base_cdn_url) + end + + def absolute_base_cdn_url + "#{Discourse.asset_host}#{relative_base_url}" + end + + def public_dir + "#{Rails.root}/public" + end - def asset_host - Rails.configuration.action_controller.asset_host end end diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index 4644625ce..54ec22344 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -1,141 +1,144 @@ -require 'digest/sha1' -require 'open-uri' +require 'file_store/base_store' -class S3Store - @fog_loaded ||= require 'fog' +module FileStore - def store_upload(file, upload) - # - path = "#{upload.id}#{upload.sha1}#{upload.extension}" + class S3Store < BaseStore + @fog_loaded ||= require 'fog' - # if this fails, it will throw an exception - upload(file.tempfile, path, upload.original_filename, file.content_type) - - # returns the url of the uploaded file - "#{absolute_base_url}/#{path}" - end - - def store_optimized_image(file, optimized_image) - # _x - path = [ - optimized_image.id, - optimized_image.sha1, - "_#{optimized_image.width}x#{optimized_image.height}", - optimized_image.extension - ].join - - # if this fails, it will throw an exception - upload(file, path) - - # returns the url of the uploaded file - "#{absolute_base_url}/#{path}" - end - - def store_avatar(file, upload, size) - # /avatars//200.jpg - path = File.join( - "avatars", - upload.sha1, - "#{size}#{upload.extension}" - ) - - # if this fails, it will throw an exception - upload(file, path) - - # returns the url of the avatar - "#{absolute_base_url}/#{path}" - end - - def remove_upload(upload) - remove_file(upload.url) - end - - def remove_optimized_image(optimized_image) - remove_file(optimized_image.url) - end - - def remove_avatars(upload) - - end - - def remove_file(url) - remove File.basename(url) if has_been_uploaded?(url) - end - - def has_been_uploaded?(url) - url.start_with?(absolute_base_url) - end - - def absolute_base_url - "//#{s3_bucket}.s3.amazonaws.com" - end - - def external? - true - end - - def internal? - !external? - end - - def download(upload) - temp_file = Tempfile.new(["discourse-s3", File.extname(upload.original_filename)]) - url = (SiteSetting.use_ssl? ? "https:" : "http:") + upload.url - - File.open(temp_file.path, "wb") do |f| - f.write open(url, "rb", read_timeout: 20).read + def store_upload(file, upload) + path = get_path_for_upload(file, upload) + store_file(file, path, upload.original_filename, file.content_type) end - temp_file - end + def store_optimized_image(file, optimized_image) + path = get_path_for_optimized_image(file, optimized_image) + store_file(file, path) + end - private + def store_avatar(file, avatar, size) + path = get_path_for_avatar(file, avatar, size) + store_file(file, path) + end - def s3_bucket - SiteSetting.s3_upload_bucket.downcase - end + def remove_upload(upload) + remove_file(upload.url) + end - def check_missing_site_settings - raise Discourse::SiteSettingMissing.new("s3_upload_bucket") if SiteSetting.s3_upload_bucket.blank? - raise Discourse::SiteSettingMissing.new("s3_access_key_id") if SiteSetting.s3_access_key_id.blank? - raise Discourse::SiteSettingMissing.new("s3_secret_access_key") if SiteSetting.s3_secret_access_key.blank? - end + def remove_optimized_image(optimized_image) + remove_file(optimized_image.url) + end - def get_or_create_directory(bucket) - check_missing_site_settings + def has_been_uploaded?(url) + url.start_with?(absolute_base_url) + end - fog = Fog::Storage.new s3_options + def absolute_base_url + "//#{s3_bucket}.s3.amazonaws.com" + end - directory = fog.directories.get(bucket) - directory = fog.directories.create(key: bucket) unless directory - directory - end + def external? + true + end - def s3_options - options = { - provider: 'AWS', - aws_access_key_id: SiteSetting.s3_access_key_id, - aws_secret_access_key: SiteSetting.s3_secret_access_key, - } - options[:region] = SiteSetting.s3_region unless SiteSetting.s3_region.empty? - options - end + def internal? + !external? + end - def upload(file, unique_filename, filename=nil, content_type=nil) - args = { - key: unique_filename, - public: true, - body: file - } - args[:content_disposition] = "attachment; filename=\"#{filename}\"" if filename - args[:content_type] = content_type if content_type + def download(upload) + @open_uri_loaded ||= require 'open-uri' - get_or_create_directory(s3_bucket).files.create(args) - end + extension = File.extname(upload.original_filename) + temp_file = Tempfile.new(["discourse-s3", extension]) + url = (SiteSetting.use_ssl? ? "https:" : "http:") + upload.url + + File.open(temp_file.path, "wb") do |f| + f.write(open(url, "rb", read_timeout: 5).read) + end + + temp_file + end + + def absolute_avatar_template(avatar) + "#{absolute_base_url}/avatars/#{avatar.sha1}/{size}#{avatar.extension}" + end + + private + + def get_path_for_upload(file, upload) + "#{upload.id}#{upload.sha1}#{upload.extension}" + end + + def get_path_for_optimized_image(file, optimized_image) + "#{optimized_image.id}#{optimized_image.sha1}_#{optimized_image.width}x#{optimized_image.height}#{optimized_image.extension}" + end + + def get_path_for_avatar(file, avatar, size) + "avatars/#{avatar.sha1}/#{size}#{avatar.extension}" + end + + def store_file(file, path, filename = nil, content_type = nil) + # if this fails, it will throw an exception + upload(file, path, filename, content_type) + # url + "#{absolute_base_url}/#{path}" + end + + def remove_file(url) + return unless has_been_uploaded?(url) + filename = File.basename(url) + remove(filename) + end + + def s3_bucket + SiteSetting.s3_upload_bucket.downcase + end + + def check_missing_site_settings + raise Discourse::SiteSettingMissing.new("s3_upload_bucket") if SiteSetting.s3_upload_bucket.blank? + raise Discourse::SiteSettingMissing.new("s3_access_key_id") if SiteSetting.s3_access_key_id.blank? + raise Discourse::SiteSettingMissing.new("s3_secret_access_key") if SiteSetting.s3_secret_access_key.blank? + end + + def get_or_create_directory(bucket) + check_missing_site_settings + + fog = Fog::Storage.new(s3_options) + + directory = fog.directories.get(bucket) + directory = fog.directories.create(key: bucket) unless directory + directory + end + + def s3_options + options = { + provider: 'AWS', + aws_access_key_id: SiteSetting.s3_access_key_id, + aws_secret_access_key: SiteSetting.s3_secret_access_key, + } + options[:region] = SiteSetting.s3_region unless SiteSetting.s3_region.empty? + options + end + + def upload(file, unique_filename, filename=nil, content_type=nil) + args = { + key: unique_filename, + public: true, + body: file + } + args[:content_disposition] = "attachment; filename=\"#{filename}\"" if filename + args[:content_type] = content_type if content_type + + get_or_create_directory(s3_bucket).files.create(args) + end + + def remove(unique_filename) + check_missing_site_settings + + fog = Fog::Storage.new(s3_options) + + fog.delete_object(s3_bucket, unique_filename) + end - def remove(unique_filename) - fog = Fog::Storage.new s3_options - fog.delete_object(s3_bucket, unique_filename) end end diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index 7ccdeae13..1af5e8dae 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -24,9 +24,7 @@ module PrettyText return "" unless username user = User.where(username_lower: username.downcase).first - if user.present? - user.avatar_template - end + user.avatar_template if user.present? end def is_username_valid(username) @@ -97,7 +95,6 @@ module PrettyText end def self.v8 - return @ctx if @ctx # ensure we only init one of these @@ -143,8 +140,6 @@ module PrettyText baked = context.eval('Discourse.Markdown.markdownConverter(opts).makeHtml(raw)') end - # we need some minimal server side stuff, apply CDN and TODO filter disallowed markup - baked = apply_cdn(baked, Rails.configuration.action_controller.asset_host) baked end @@ -160,35 +155,12 @@ module PrettyText r end - def self.apply_cdn(html, url) - return html unless url - - image = /\.(png|jpg|jpeg|gif|bmp|tif|tiff)$/i - relative = /^\/[^\/]/ - - doc = Nokogiri::HTML.fragment(html) - - doc.css("a").each do |l| - href = l["href"].to_s - l["href"] = url + href if href =~ relative && href =~ image - end - - doc.css("img").each do |l| - src = l["src"].to_s - l["src"] = url + src if src =~ relative - end - - doc.to_s - end - def self.cook(text, opts={}) cloned = opts.dup # we have a minor inconsistency cloned[:topicId] = opts[:topic_id] sanitized = markdown(text.dup, cloned) - if SiteSetting.add_rel_nofollow_to_user_content - sanitized = add_rel_nofollow_to_user_content(sanitized) - end + sanitized = add_rel_nofollow_to_user_content(sanitized) if SiteSetting.add_rel_nofollow_to_user_content sanitized end @@ -196,9 +168,7 @@ module PrettyText whitelist = [] l = SiteSetting.exclude_rel_nofollow_domains - if l.present? - whitelist = l.split(",") - end + whitelist = l.split(",") if l.present? site_uri = nil doc = Nokogiri::HTML.fragment(html) @@ -208,9 +178,9 @@ module PrettyText uri = URI(href) site_uri ||= URI(Discourse.base_url) - if !uri.host.present? || - uri.host.ends_with?(site_uri.host) || - whitelist.any?{|u| uri.host.ends_with?(u)} + if !uri.host.present? || + uri.host.ends_with?(site_uri.host) || + whitelist.any?{|u| uri.host.ends_with?(u)} # we are good no need for nofollow else l["rel"] = "nofollow" @@ -245,7 +215,6 @@ module PrettyText links end - def self.excerpt(html, max_length, options={}) ExcerptParser.get_excerpt(html, max_length, options) end diff --git a/lib/tasks/images.rake b/lib/tasks/images.rake index 3d9a61985..79e19bbe1 100644 --- a/lib/tasks/images.rake +++ b/lib/tasks/images.rake @@ -9,103 +9,3 @@ task "images:compress" => :environment do end end end - -desc "download all hotlinked images" -task "images:pull_hotlinked" => :environment do - RailsMultisite::ConnectionManagement.each_connection do |db| - # currently only works when using the local storage - next if Discourse.store.external? - - puts "Pulling hotlinked images for: #{db}" - - # shorthand to the asset host - asset_host = Rails.configuration.action_controller.asset_host - # maximum size of the file in bytes - max_size = SiteSetting.max_image_size_kb * 1024 - # will hold the urls of the already downloaded images - upload_urls = {} - - Post.find_each do |post| - has_changed = false - - extract_images_from(post.cooked).each do |image| - src = image['src'] - if src.present? && - src !~ /^\/[^\/]/ && - !src.starts_with?(Discourse.base_url_no_prefix) && - !(asset_host.present? && src.starts_with?(asset_host)) - begin - # have we already downloaded that file? - if !upload_urls.include?(src) - # initialize - upload_urls[src] = nil - # download the file - hotlinked = download(src, max_size) - # if the hotlinked image is OK - if hotlinked.size <= max_size - file = ActionDispatch::Http::UploadedFile.new(tempfile: hotlinked, filename: File.basename(URI.parse(src).path)) - upload_urls[src] = Upload.create_for(post.user_id, file, hotlinked.size).url - else - puts "\nFailed to pull: #{src} for post ##{post.id} - too large\n" - end - end - # if we have downloaded a file - if upload_urls[src].present? - src_for_regexp = src.gsub("?", "\\?").gsub(".", "\\.").gsub("+", "\\+") - # there are 5 ways to insert an image in a post - # HTML tag - - post.raw.gsub!(/src=["']#{src_for_regexp}["']/i, "src='#{upload_urls[src]}'") - # BBCode tag - [img]http://...[/img] - post.raw.gsub!(/\[img\]#{src_for_regexp}\[\/img\]/i, "[img]#{upload_urls[src]}[/img]") - # Markdown inline - ![alt](http://...) - post.raw.gsub!(/!\[([^\]]*)\]\(#{src_for_regexp}\)/) { "![#{$1}](#{upload_urls[src]})" } - # Markdown reference - [x]: http:// - post.raw.gsub!(/\[(\d+)\]: #{src_for_regexp}/) { "[#{$1}]: #{upload_urls[src]}" } - # Direct link - post.raw.gsub!(src, "") - # mark the post as changed - has_changed = true - end - rescue => e - puts "\nFailed to pull: #{src} for post ##{post.id} - #{e}\n" - ensure - # close & delete the temporary file - hotlinked && hotlinked.close! - end - end - end - - if has_changed - # since the raw has changed, we cook the post once again - post.cooked = post.cook(post.raw, topic_id: post.topic_id, invalidate_oneboxes: true) - # update both raw & cooked version of the post - Post.exec_sql('update posts set cooked = ?, raw = ? where id = ?', post.cooked, post.raw, post.id) - # trigger the post processing - post.trigger_post_process - putc "#" - else - putc "." - end - end - end - puts "\ndone." -end - -def extract_images_from(html) - doc = Nokogiri::HTML::fragment(html) - doc.css("img") - doc.css(".onebox-result img") - doc.css("img.avatar") -end - -def download(url, max_size) - # create a temporary file - temp_file = Tempfile.new(["discourse-hotlinked", File.extname(URI.parse(url).path)]) - # download the hotlinked image - File.open(temp_file.path, "wb") do |f| - hotlinked = open(url, "rb", read_timeout: 5) - while f.size <= max_size && data = hotlinked.read(max_size) - f.write(data) - end - hotlinked.close - end - temp_file -end diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb index 480bb01c7..af039220d 100644 --- a/spec/components/cooked_post_processor_spec.rb +++ b/spec/components/cooked_post_processor_spec.rb @@ -1,106 +1,49 @@ -require 'spec_helper' -require 'cooked_post_processor' +require "spec_helper" +require "cooked_post_processor" describe CookedPostProcessor do - context "post_process" do + context ".post_process" do let(:post) { build(:post) } let(:cpp) { CookedPostProcessor.new(post) } let(:post_process) { sequence("post_process") } it "post process in sequence" do - cpp.expects(:clean_up_reverse_index).in_sequence(post_process) - cpp.expects(:post_process_attachments).in_sequence(post_process) + cpp.expects(:keep_reverse_index_up_to_date).in_sequence(post_process) cpp.expects(:post_process_images).in_sequence(post_process) cpp.expects(:post_process_oneboxes).in_sequence(post_process) + cpp.expects(:optimize_urls).in_sequence(post_process) + cpp.expects(:pull_hotlinked_images).in_sequence(post_process) cpp.post_process end end - context "clean_up_reverse_index" do + context ".keep_reverse_index_up_to_date" do - let(:post) { build(:post) } + let(:post) { build(:post_with_uploads, id: 123) } let(:cpp) { CookedPostProcessor.new(post) } + it "finds all the uploads in the post" do + Upload.expects(:get_from_url).with("/uploads/default/2/2345678901234567.jpg") + Upload.expects(:get_from_url).with("/uploads/default/1/1234567890123456.jpg") + cpp.keep_reverse_index_up_to_date + end + it "cleans the reverse index up for the current post" do PostUpload.expects(:delete_all).with(post_id: post.id) - cpp.clean_up_reverse_index + cpp.keep_reverse_index_up_to_date end end - context "post_process_attachments" do + context ".post_process_images" do - context "with attachment" do - - let(:upload) { Fabricate(:upload) } - let(:post) { Fabricate(:post_with_an_attachment) } - let(:cpp) { CookedPostProcessor.new(post) } - - # all in one test to speed things up - it "works" do - Upload.expects(:get_from_url).returns(upload) - cpp.post_process_attachments - # ensures absolute urls on attachment - cpp.html.should =~ /#{Discourse.store.absolute_base_url}/ - # keeps the reverse index up to date - post.uploads.reload - post.uploads.count.should == 1 - end - - end - - end - - context "post_process_images" do - - context "with images in quotes and oneboxes" do - - let(:post) { build(:post_with_images_in_quote_and_onebox) } - let(:cpp) { CookedPostProcessor.new(post) } - before { cpp.post_process_images } - - it "does not process them" do - cpp.html.should match_html post.cooked - cpp.should_not be_dirty - end - - it "has no topic image if there isn't one in the post" do - post.topic.image_url.should be_blank - end - - end - - context "with locally uploaded images" do - - let(:upload) { Fabricate(:upload) } - let(:post) { Fabricate(:post_with_uploaded_image) } - let(:cpp) { CookedPostProcessor.new(post) } - before { FastImage.stubs(:size).returns([200, 400]) } - - # all in one test to speed things up - it "works" do - Upload.expects(:get_from_url).returns(upload) - cpp.post_process_images - # ensures absolute urls on uploaded images - cpp.html.should =~ /#{LocalStore.new.absolute_base_url}/ - # dirty - cpp.should be_dirty - # keeps the reverse index up to date - post.uploads.reload - post.uploads.count.should == 1 - end - - end - - context "with sized images" do + context "with image_sizes" do let(:post) { build(:post_with_image_url) } - let(:cpp) { CookedPostProcessor.new(post, image_sizes: {'http://foo.bar/image.png' => {'width' => 111, 'height' => 222}}) } - - before { FastImage.stubs(:size).returns([150, 250]) } + let(:cpp) { CookedPostProcessor.new(post, image_sizes: {"http://foo.bar/image.png" => {"width" => 111, "height" => 222}}) } it "adds the width from the image sizes provided" do cpp.post_process_images @@ -134,7 +77,6 @@ describe CookedPostProcessor do SiteSetting.stubs(:max_image_height).returns(2000) SiteSetting.stubs(:create_thumbnails?).returns(true) Upload.expects(:get_from_url).returns(upload) - cpp.stubs(:associate_to_post) FastImage.stubs(:size).returns([1000, 2000]) # optimized_image FileUtils.stubs(:mkdir_p) @@ -144,7 +86,7 @@ describe CookedPostProcessor do it "generates overlay information" do cpp.post_process_images - cpp.html.should match_html '
+ cpp.html.should match_html '' cpp.should be_dirty @@ -159,37 +101,107 @@ describe CookedPostProcessor do let(:cpp) { CookedPostProcessor.new(post) } it "adds a topic image if there's one in the post" do - FastImage.stubs(:size).returns([100, 100]) + FastImage.stubs(:size) + post.topic.image_url.should be_nil cpp.post_process_images post.topic.reload - post.topic.image_url.should == "http://test.localhost/uploads/default/2/3456789012345678.png" + post.topic.image_url.should be_present end end end - context "post_process_oneboxes" do + context ".extract_images" do - let(:post) { build(:post_with_youtube, id: 123) } - let(:cpp) { CookedPostProcessor.new(post, invalidate_oneboxes: true) } + let(:post) { build(:post_with_images_in_quote_and_onebox) } + let(:cpp) { CookedPostProcessor.new(post) } - before do - Oneboxer.expects(:onebox).with("http://www.youtube.com/watch?v=9bZkp7q19f0", post_id: 123, invalidate_oneboxes: true).returns('
GANGNAM STYLE
') - cpp.post_process_oneboxes - end - - it "should be dirty" do - cpp.should be_dirty - end - - it "inserts the onebox without wrapping p" do - cpp.html.should match_html "
GANGNAM STYLE
" + it "does not extract images inside oneboxes or quotes" do + cpp.extract_images.length.should == 0 end end - context "get_filename" do + context ".get_size_from_image_sizes" do + + let(:post) { build(:post) } + let(:cpp) { CookedPostProcessor.new(post) } + + it "returns the size" do + image_sizes = { "http://my.discourse.org/image.png" => { "width" => 111, "height" => 222 } } + cpp.get_size_from_image_sizes("/image.png", image_sizes).should == [111, 222] + end + + end + + context ".get_size" do + + let(:post) { build(:post) } + let(:cpp) { CookedPostProcessor.new(post) } + + it "ensures urls are absolute" do + cpp.expects(:is_valid_image_url?).with("http://test.localhost/relative/url/image.png") + cpp.get_size("/relative/url/image.png") + end + + it "ensures urls have a default scheme" do + cpp.expects(:is_valid_image_url?).with("http://schemaless.url/image.jpg") + cpp.get_size("//schemaless.url/image.jpg") + end + + it "caches the results" do + SiteSetting.stubs(:crawl_images?).returns(true) + FastImage.expects(:size).returns([200, 400]) + cpp.get_size("http://foo.bar/image3.png") + cpp.get_size("http://foo.bar/image3.png").should == [200, 400] + end + + context "when crawl_images is disabled" do + + before { SiteSetting.stubs(:crawl_images?).returns(false) } + + it "doesn't call FastImage" do + FastImage.expects(:size).never + cpp.get_size("http://foo.bar/image1.png").should == nil + end + + it "is always allowed to crawl our own images" do + store = stub + store.expects(:has_been_uploaded?).returns(true) + Discourse.expects(:store).returns(store) + FastImage.expects(:size).returns([100, 200]) + cpp.get_size("http://foo.bar/image2.png").should == [100, 200] + end + + end + + end + + context ".is_valid_image_url?" do + + let(:post) { build(:post) } + let(:cpp) { CookedPostProcessor.new(post) } + + it "validates HTTP(s) urls" do + cpp.is_valid_image_url?("http://domain.com").should == true + cpp.is_valid_image_url?("https://domain.com").should == true + end + + it "doesn't validate other urls" do + cpp.is_valid_image_url?("ftp://domain.com").should == false + cpp.is_valid_image_url?("ftps://domain.com").should == false + cpp.is_valid_image_url?("/tmp/image.png").should == false + cpp.is_valid_image_url?("//domain.com").should == false + end + + it "doesn't throw an exception with a bad URI" do + cpp.is_valid_image_url?("http://doGANGNAM STYLE
") + cpp.post_process_oneboxes end - context "crawl_images is disabled" do - - before { SiteSetting.stubs(:crawl_images?).returns(false) } - - it "doesn't call FastImage" do - FastImage.expects(:size).never - cpp.get_size("http://foo.bar/image1.png").should == nil - end - - it "is always allowed to crawl our own images" do - store = {} - Discourse.expects(:store).returns(store) - store.expects(:has_been_uploaded?).returns(true) - FastImage.expects(:size).returns([100, 200]) - cpp.get_size("http://foo.bar/image2.png").should == [100, 200] - end - + it "is dirty" do + cpp.should be_dirty end - it "caches the results" do - SiteSetting.stubs(:crawl_images?).returns(true) - FastImage.expects(:size).returns([200, 400]) - cpp.get_size("http://foo.bar/image3.png") - cpp.get_size("http://foo.bar/image3.png").should == [200, 400] + it "inserts the onebox without wrapping p" do + cpp.html.should match_html "
GANGNAM STYLE
" end end - context "is_valid_image_uri?" do + context ".optimize_urls" do + + let(:post) { build(:post_with_uploads_and_links) } + let(:cpp) { CookedPostProcessor.new(post) } + + it "uses schemaless url for uploads" do + cpp.optimize_urls + cpp.html.should match_html 'Link + Google + ' + end + + context "when CDN is enabled" do + + it "uses schemaless CDN url for uploads" do + Rails.configuration.action_controller.stubs(:asset_host).returns("http://my.cdn.com") + cpp.optimize_urls + cpp.html.should match_html 'Link + Google + ' + end + + end + + end + + context ".pull_hotlinked_images" do let(:post) { build(:post) } let(:cpp) { CookedPostProcessor.new(post) } - it "needs the scheme to be either http or https" do - cpp.is_valid_image_uri?("http://domain.com").should == true - cpp.is_valid_image_uri?("https://domain.com").should == true - cpp.is_valid_image_uri?("ftp://domain.com").should == false - cpp.is_valid_image_uri?("ftps://domain.com").should == false - cpp.is_valid_image_uri?("//domain.com").should == false - cpp.is_valid_image_uri?("/tmp/image.png").should == false + it "does not run when crawl images is disabled" do + SiteSetting.stubs(:crawl_images).returns(false) + Jobs.expects(:cancel_scheduled_job).never + cpp.pull_hotlinked_images end - it "doesn't throw an exception with a bad URI" do - cpp.is_valid_image_uri?("http://dohello","http://a.com").should == - "hello" - end - - it "should not touch non images" do - PrettyText.apply_cdn("hello","http://a.com").should == - "hello" - end - - it "should not touch schemaless links" do - PrettyText.apply_cdn("hello","http://a.com").should == - "hello" - end - end end diff --git a/spec/controllers/admin/versions_controller_spec.rb b/spec/controllers/admin/versions_controller_spec.rb index 364c5503a..3825ceed9 100644 --- a/spec/controllers/admin/versions_controller_spec.rb +++ b/spec/controllers/admin/versions_controller_spec.rb @@ -34,4 +34,4 @@ describe Admin::VersionsController do end end end -end \ No newline at end of file +end diff --git a/spec/fabricators/post_fabricator.rb b/spec/fabricators/post_fabricator.rb index e3ac09475..c34e1c5f5 100644 --- a/spec/fabricators/post_fabricator.rb +++ b/spec/fabricators/post_fabricator.rb @@ -59,13 +59,29 @@ Fabricator(:post_with_unsized_images, from: :post) do end Fabricator(:post_with_image_url, from: :post) do - cooked '' + cooked '' end Fabricator(:post_with_large_image, from: :post) do cooked '' end +Fabricator(:post_with_uploads, from: :post) do + cooked ' +Link + +' +end + +Fabricator(:post_with_uploads_and_links, from: :post) do + cooked ' +Link + +Google + +' +end + Fabricator(:post_with_external_links, from: :post) do user topic diff --git a/spec/models/optimized_image_spec.rb b/spec/models/optimized_image_spec.rb index 743c3bb0f..0e663ea5f 100644 --- a/spec/models/optimized_image_spec.rb +++ b/spec/models/optimized_image_spec.rb @@ -4,53 +4,88 @@ describe OptimizedImage do it { should belong_to :upload } - let(:upload) { Fabricate(:upload) } - let(:oi) { OptimizedImage.create_for(upload, 100, 200) } + let(:upload) { build(:upload) } + + before { upload.id = 42 } describe ".create_for" do - before { ImageSorcery.any_instance.expects(:convert).returns(true) } + context "when using an internal store" do - describe "internal store" do + let(:store) { FakeInternalStore.new } + before { Discourse.stubs(:store).returns(store) } + + context "when an error happened while generatign the thumbnail" do + + before { ImageSorcery.any_instance.stubs(:convert).returns(false) } + + it "returns nil" do + OptimizedImage.create_for(upload, 100, 200).should be_nil + end + + end + + context "when the thumbnail is properly generated" do + + before { ImageSorcery.any_instance.stubs(:convert).returns(true) } + + it "does not download a copy of the original image" do + store.expects(:download).never + OptimizedImage.create_for(upload, 100, 200) + end + + it "closes and removes the tempfile" do + Tempfile.any_instance.expects(:close!) + OptimizedImage.create_for(upload, 100, 200) + end + + it "works" do + oi = OptimizedImage.create_for(upload, 100, 200) + oi.sha1.should == "da39a3ee5e6b4b0d3255bfef95601890afd80709" + oi.extension.should == ".jpg" + oi.width.should == 100 + oi.height.should == 200 + oi.url.should == "/internally/stored/optimized/image.jpg" + end - it "works" do - Tempfile.any_instance.expects(:close!) - oi.sha1.should == "da39a3ee5e6b4b0d3255bfef95601890afd80709" - oi.extension.should == ".jpg" - oi.width.should == 100 - oi.height.should == 200 - oi.url.should == "/uploads/default/_optimized/da3/9a3/ee5e6b4b0d_100x200.jpg" end end describe "external store" do - require 'file_store/s3_store' - require 'fog' + let(:store) { FakeExternalStore.new } + before { Discourse.stubs(:store).returns(store) } - let(:store) { S3Store.new } + context "when an error happened while generatign the thumbnail" do + + before { ImageSorcery.any_instance.stubs(:convert).returns(false) } + + it "returns nil" do + OptimizedImage.create_for(upload, 100, 200).should be_nil + end - before do - Discourse.stubs(:store).returns(store) - SiteSetting.stubs(:s3_upload_bucket).returns("S3_Upload_Bucket") - SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id") - SiteSetting.stubs(:s3_secret_access_key).returns("s3_secret_access_key") - Fog.mock! end - it "works" do - # fake downloaded file - downloaded_file = {} - downloaded_file.expects(:path).returns("/path/to/fake.png") - downloaded_file.expects(:close!) - store.expects(:download).returns(downloaded_file) - # assertions - oi.sha1.should == "da39a3ee5e6b4b0d3255bfef95601890afd80709" - oi.extension.should == ".png" - oi.width.should == 100 - oi.height.should == 200 - oi.url.should =~ /^\/\/s3_upload_bucket.s3.amazonaws.com\/[0-9a-f]+_100x200.png/ + context "when the thumbnail is properly generated" do + + before { ImageSorcery.any_instance.stubs(:convert).returns(true) } + + it "downloads a copy of the original image" do + Tempfile.any_instance.expects(:close!).twice + store.expects(:download).with(upload).returns(Tempfile.new(["discourse-external", ".jpg"])) + OptimizedImage.create_for(upload, 100, 200) + end + + it "works" do + oi = OptimizedImage.create_for(upload, 100, 200) + oi.sha1.should == "da39a3ee5e6b4b0d3255bfef95601890afd80709" + oi.extension.should == ".jpg" + oi.width.should == 100 + oi.height.should == 200 + oi.url.should == "/externally/stored/optimized/image.jpg" + end + end end @@ -58,3 +93,44 @@ describe OptimizedImage do end end + +class FakeInternalStore + + def internal? + true + end + + def external? + !internal? + end + + def path_for(upload) + upload.url + end + + def store_optimized_image(file, optimized_image) + "/internally/stored/optimized/image#{optimized_image.extension}" + end + +end + +class FakeExternalStore + + def external? + true + end + + def internal? + !external? + end + + def store_optimized_image(file, optimized_image) + "/externally/stored/optimized/image#{optimized_image.extension}" + end + + def download(upload) + extension = File.extname(upload.original_filename) + Tempfile.new(["discourse-s3", extension]) + end + +end diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 7c812ffe3..4006bc796 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -46,18 +46,10 @@ describe Upload do upload.create_thumbnail!(100, 100) end - it "does not create another thumbnail" do - SiteSetting.expects(:create_thumbnails?).returns(true) - upload.expects(:has_thumbnail?).returns(true) - OptimizedImage.expects(:create_for).never - upload.create_thumbnail!(100, 100) - end - it "creates a thumbnail" do upload = Fabricate(:upload) thumbnail = Fabricate(:optimized_image, upload: upload) SiteSetting.expects(:create_thumbnails?).returns(true) - upload.expects(:has_thumbnail?).returns(false) OptimizedImage.expects(:create_for).returns(thumbnail) upload.create_thumbnail!(100, 100) upload.reload @@ -66,7 +58,7 @@ describe Upload do end - context ".create_for" do + context "#create_for" do it "does not create another upload if it already exists" do Upload.expects(:where).with(sha1: image_sha1).returns([upload]) diff --git a/spec/models/user_action_spec.rb b/spec/models/user_action_spec.rb index 98f553e55..d0b34a274 100644 --- a/spec/models/user_action_spec.rb +++ b/spec/models/user_action_spec.rb @@ -210,7 +210,6 @@ describe UserAction do end end - describe 'private messages' do let(:user) do