From bfe1e2003f40f72beef54bfce43270f1e35b0446 Mon Sep 17 00:00:00 2001 From: Markus Kahl Date: Fri, 18 Oct 2019 11:57:03 +0100 Subject: [PATCH] make external avatar URLs valid for 24h --- app/models/attachment.rb | 4 +- app/uploaders/fog_file_uploader.rb | 45 +++++++++++++++---- lib/api/helpers/attachment_renderer.rb | 22 ++++++++- lib/open_project/configuration.rb | 2 + lib/open_project/patches/carrierwave.rb | 33 ++++++++++++++ .../lib/api/v3/users/user_avatar_api.rb | 2 +- .../spec/requests/user_avatar_api_spec.rb | 23 ++++++++++ spec/models/attachment_spec.rb | 23 ++++++++++ 8 files changed, 140 insertions(+), 14 deletions(-) create mode 100644 lib/open_project/patches/carrierwave.rb diff --git a/app/models/attachment.rb b/app/models/attachment.rb index e658b76c26..e714b58789 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -59,8 +59,8 @@ class Attachment < ActiveRecord::Base ## # Returns an URL if the attachment is stored in an external (fog) attachment storage # or nil otherwise. - def external_url - url = URI.parse file.download_url(content_disposition: content_disposition) # returns a path if local + def external_url(expires_in: nil) + url = URI.parse file.download_url(content_disposition: content_disposition, expires_in: expires_in) # returns a path if local url if url.host rescue URI::InvalidURIError diff --git a/app/uploaders/fog_file_uploader.rb b/app/uploaders/fog_file_uploader.rb index b472734189..9dc8113617 100644 --- a/app/uploaders/fog_file_uploader.rb +++ b/app/uploaders/fog_file_uploader.rb @@ -27,6 +27,7 @@ #++ require 'carrierwave/storage/fog' +require 'open_project/patches/carrierwave' class FogFileUploader < CarrierWave::Uploader::Base include FileUploader @@ -57,18 +58,20 @@ class FogFileUploader < CarrierWave::Uploader::Base super end + ## + # Generates a download URL for this file. + # + # @param options [Hash] Options hash. + # @option options [String] :content_disposition Pass this content disposition to S3 so that it serves the file with it. + # @option options [DateTime] :expires_at Date at which the link should expire (default: now + 5 minutes) + # @option options [ActiveSupport::Duration] :expires_in Duration in which the link should expire. + # + # @return [String] The URL to download the file from. def download_url(options = {}) url_options = {} - if options[:content_disposition].present? - url_options[:query] = { - # Passing this option to S3 will make it serve the file with the - # respective content disposition. Without it no content disposition - # header is sent. This only works for S3 but we don't support - # anything else anyway (see carrierwave.rb). - "response-content-disposition" => options[:content_disposition] - } - end + set_content_disposition! url_options, options: options + set_expires_at! url_options, options: options remote_file.url url_options end @@ -85,4 +88,28 @@ class FogFileUploader < CarrierWave::Uploader::Base rescue Excon::Errors::Forbidden false end + + private + + def set_content_disposition!(url_options, options:) + if options[:content_disposition].present? + url_options[:query] = { + # Passing this option to S3 will make it serve the file with the + # respective content disposition. Without it no content disposition + # header is sent. This only works for S3 but we don't support + # anything else anyway (see carrierwave.rb). + "response-content-disposition" => options[:content_disposition] + } + end + end + + def set_expires_at!(url_options, options:) + if options[:expires_in].present? + url_options[:expire_at] = ::Fog::Time.now + options[:expires_in] + end + + if options[:expires_at].present? + url_options[:expire_at] = ::Fog::Time.at options[:expires_at] - ::Fog::Time.offset + end + end end diff --git a/lib/api/helpers/attachment_renderer.rb b/lib/api/helpers/attachment_renderer.rb index 49fbe4d4ff..5cde04ca8c 100644 --- a/lib/api/helpers/attachment_renderer.rb +++ b/lib/api/helpers/attachment_renderer.rb @@ -38,9 +38,13 @@ module API # to the external storage, # # or by directly rendering the file - def respond_with_attachment(attachment) + # + # @param attachment [Attachment] Attachment to be responded with. + # @param external_link_expires_in [ActiveSupport::Duration] Time after which link expires. Default is 5 minutes. + # Only applicable in case of external storage. + def respond_with_attachment(attachment, external_link_expires_in: nil) if attachment.external_storage? - redirect attachment.external_url.to_s + redirect attachment.external_url(expires_in: external_link_expires_in).to_s else content_type attachment.content_type header['Content-Disposition'] = "#{attachment.content_disposition}; filename=#{attachment.filename}" @@ -48,6 +52,20 @@ module API attachment.diskfile.read end end + + def external_avatar_link_expires_in + seconds = external_avatar_link_expiry_seconds + + if seconds == 0 + nil + else + seconds.seconds + end + end + + def external_avatar_link_expiry_seconds + @external_avatar_link_expiry_seconds ||= OpenProject::Configuration.external_avatar_link_expiry_seconds.to_i + end end end end diff --git a/lib/open_project/configuration.rb b/lib/open_project/configuration.rb index 7148a841b1..36594d7ad5 100644 --- a/lib/open_project/configuration.rb +++ b/lib/open_project/configuration.rb @@ -133,6 +133,8 @@ module OpenProject # Allow in-context translations to be loaded with CSP 'crowdin_in_context_translations' => true, + 'external_avatar_link_expiry_seconds' => 24.hours.to_i, + # Default gravatar image, set to something other than 404 # to ensure a default is returned 'gravatar_fallback_image' => '404', diff --git a/lib/open_project/patches/carrierwave.rb b/lib/open_project/patches/carrierwave.rb new file mode 100644 index 0000000000..3b95ce54f0 --- /dev/null +++ b/lib/open_project/patches/carrierwave.rb @@ -0,0 +1,33 @@ +require 'carrierwave' + +## +# Code copied straight from the CarrierWave source. +# All we did is add `options[:expire_at]`. +# +# @todo Upgrade to CarrierWave 2.0.2 to make this patch obsolete. +class CarrierWave::Storage::Fog::File + def authenticated_url(options = {}) + if ['AWS', 'Google', 'Rackspace', 'OpenStack'].include?(@uploader.fog_credentials[:provider]) + # avoid a get by using local references + local_directory = connection.directories.new(:key => @uploader.fog_directory) + local_file = local_directory.files.new(:key => path) + expire_at = options[:expire_at] || ::Fog::Time.now + @uploader.fog_authenticated_url_expiration + case @uploader.fog_credentials[:provider] + when 'AWS', 'Google' + # Older versions of fog-google do not support options as a parameter + if url_options_supported?(local_file) + local_file.url(expire_at, options) + else + warn "Options hash not supported in #{local_file.class}. You may need to upgrade your Fog provider." + local_file.url(expire_at) + end + when 'Rackspace' + connection.get_object_https_url(@uploader.fog_directory, path, expire_at, options) + when 'OpenStack' + connection.get_object_https_url(@uploader.fog_directory, path, expire_at) + else + local_file.url(expire_at) + end + end + end +end diff --git a/modules/avatars/lib/api/v3/users/user_avatar_api.rb b/modules/avatars/lib/api/v3/users/user_avatar_api.rb index 3ac4e95734..3ead718a53 100644 --- a/modules/avatars/lib/api/v3/users/user_avatar_api.rb +++ b/modules/avatars/lib/api/v3/users/user_avatar_api.rb @@ -36,7 +36,7 @@ module API get '/avatar' do if local_avatar = local_avatar?(@user) - respond_with_attachment(local_avatar) + respond_with_attachment(local_avatar, external_link_expires_in: external_avatar_link_expires_in) elsif avatar_manager.gravatar_enabled? redirect build_gravatar_image_url(@user) else diff --git a/modules/avatars/spec/requests/user_avatar_api_spec.rb b/modules/avatars/spec/requests/user_avatar_api_spec.rb index 27c3079b40..d74dda1f34 100644 --- a/modules/avatars/spec/requests/user_avatar_api_spec.rb +++ b/modules/avatars/spec/requests/user_avatar_api_spec.rb @@ -37,6 +37,8 @@ describe 'API v3 User avatar resource', type: :request, content_type: :json do subject(:response) { last_response } + let(:setup) { nil } + before do login_as user end @@ -47,6 +49,8 @@ describe 'API v3 User avatar resource', type: :request, content_type: :json do .to receive(:plugin_openproject_avatars) .and_return(enable_gravatars: gravatars, enable_local_avatars: local_avatars) + setup + get api_v3_paths.user(user.id) + "/avatar" end @@ -82,6 +86,25 @@ describe 'API v3 User avatar resource', type: :request, content_type: :json do it 'serves the attachment file' do expect(response.status).to eq 200 end + + context 'with external file storage (S3)' do + let(:setup) do + allow_any_instance_of(Attachment).to receive(:external_storage?).and_return true + + expect_any_instance_of(Attachment) + .to receive(:external_url) + .with(expires_in: 86400) + .and_return("external URL") + end + + # we test that Attachment#external_url works in `attachments_spec.rb` + # so here we just make sue it's called accordingly when the external + # storage is configured + it 'redirects to temporary external URL' do + expect(response.status).to eq 302 + expect(response.location).to eq "external URL" + end + end end end end diff --git a/spec/models/attachment_spec.rb b/spec/models/attachment_spec.rb index a7e85be1ae..7bdf27a102 100644 --- a/spec/models/attachment_spec.rb +++ b/spec/models/attachment_spec.rb @@ -268,6 +268,24 @@ describe Attachment, type: :model do CarrierWave::Configuration.configure_fog! credentials: {}, directory: "my-bucket", public: false end + shared_examples "it has a temporary download link" do + let(:url_options) { {} } + let(:query) { attachment.external_url(url_options).to_s.split("?").last } + + it "should have a default expiry time" do + expect(query).to include "X-Amz-Expires=" + expect(query).not_to include "X-Amz-Expires=3600" + end + + context "with a custom expiry time" do + let(:url_options) { { expires_in: 1.hour } } + + it "should use that time" do + expect(query).to include "X-Amz-Expires=3600" + end + end + end + describe "for an image file" do before { image_attachment.save! } @@ -275,6 +293,11 @@ describe Attachment, type: :model do expect(image_attachment.content_disposition).to eq "inline" expect(image_attachment.external_url.to_s).to include "response-content-disposition=inline" end + + # this is independent from the type of file uploaded so we just test this for the first one + it_behaves_like "it has a temporary download link" do + let(:attachment) { image_attachment } + end end describe "for a text file" do