make external avatar URLs valid for 24h

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

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

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

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',

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

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

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

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