add download endpoint to attachment resource

lib/api/v3/attachments/attachment_representer.rb

@@ -86,8 +86,13 @@ module API
                             link: associated_container_link
 
         link :downloadLocation do
+          location = if represented.external_storage?
+                       represented.remote_url
+                     else
+                       api_v3_paths.attachment_content(represented.id)
+                     end
           {
-            href: api_v3_paths.attachment_download(represented.id, represented.filename)
+            href: location
           }
         end
 

lib/api/v3/attachments/attachments_api.rb

@@ -53,6 +53,19 @@ module API
               @attachment.container.attachments.delete(@attachment)
               status 204
             end
+
+            namespace :content do
+              get do
+                if @attachment.external_storage?
+                  redirect @attachment.external_url
+                else
+                  content_type @attachment.content_type
+                  header['Content-Disposition'] = "attachment; filename=#{@attachment.filename}"
+                  env['api.format'] = :binary
+                  @attachment.diskfile.read
+                end
+              end
+            end
           end
         end
       end

lib/api/v3/utilities/path_helper.rb

@@ -55,8 +55,8 @@ module API
             "#{root}/attachments/#{id}"
           end
 
-          def self.attachment_download(id, filename = nil)
-            download_attachment_path(id, filename)
+          def self.attachment_content(id)
+            "#{root}/attachments/#{id}/content"
           end
 
           def self.attachments_by_post(id)

spec/controllers/attachments_controller_spec.rb

@@ -31,26 +31,26 @@ require 'spec_helper'
 describe AttachmentsController, type: :controller do
   let(:user) { FactoryBot.create(:user) }
   let(:project) { FactoryBot.create(:project) }
-  let(:role) {
+  let(:role) do
     FactoryBot.create(:role,
-                       permissions: [:edit_work_packages,
-                                     :view_work_packages,
-                                     :delete_wiki_pages_attachments])
-  }
-  let!(:member) {
+                      permissions: [:edit_work_packages,
+                                    :view_work_packages,
+                                    :delete_wiki_pages_attachments])
+  end
+  let!(:member) do
     FactoryBot.create(:member,
-                       project: project,
-                       principal: user,
-                       roles: [role])
-  }
+                      project: project,
+                      principal: user,
+                      roles: [role])
+  end
 
-  before do allow(User).to receive(:current).and_return user end
+  before { allow(User).to receive(:current).and_return user }
 
   describe '#destroy' do
-    let(:attachment) {
+    let(:attachment) do
       FactoryBot.create(:attachment,
-                         container: container)
-    }
+                        container: container)
+    end
 
     shared_examples_for :deleted do
       subject { Attachment.find_by(id: attachment.id) }
@@ -67,11 +67,11 @@ describe AttachmentsController, type: :controller do
     end
 
     context 'work_package' do
-      let(:container) {
+      let(:container) do
         FactoryBot.create(:work_package,
-                           author: user,
-                           project: project)
-      }
+                          author: user,
+                          project: project)
+      end
       let(:redirect_path) { work_package_path(container) }
 
       before do
@@ -84,10 +84,10 @@ describe AttachmentsController, type: :controller do
     end
 
     context 'wiki' do
-      let(:container) {
+      let(:container) do
         FactoryBot.create(:wiki_page,
-                           wiki: project.wiki)
-      }
+                          wiki: project.wiki)
+      end
       let(:redirect_path) { project_wiki_path(project, project.wiki) }
 
       before do
@@ -133,9 +133,9 @@ describe AttachmentsController, type: :controller do
       allow(Attachment).to receive(:find).with(attachment.id.to_s).and_return(attachment)
     end
 
-    subject {
+    subject do
       get :download, params: { id: attachment.id }
-    }
+    end
 
     context 'with a local file' do
       let(:uploader) { LocalFileUploader }

spec/lib/api/v3/attachments/attachment_representer_spec.rb

@@ -108,6 +108,33 @@ describe ::API::V3::Attachments::AttachmentRepresenter do
       end
     end
 
+    describe 'downloadLocation link' do
+      context 'for a local attachment' do
+        it_behaves_like 'has an untitled link' do
+          let(:link) { 'downloadLocation' }
+          let(:href) { api_v3_paths.attachment_content(attachment.id) }
+        end
+      end
+
+      context 'for a remote attachment' do
+        let(:remote_url) { 'https://some.bogus/download/xyz' }
+
+        before do
+          allow(attachment)
+            .to receive(:external_storage?)
+            .and_return(true)
+          allow(attachment)
+            .to receive(:remote_url)
+            .and_return(remote_url)
+        end
+
+        it_behaves_like 'has an untitled link' do
+          let(:link) { 'downloadLocation' }
+          let(:href) { remote_url }
+        end
+      end
+    end
+
     it_behaves_like 'has a titled link' do
       let(:link) { 'author' }
       let(:href) { api_v3_paths.user(attachment.author.id) }

spec/lib/api/v3/utilities/path_helper_spec.rb

@@ -74,16 +74,10 @@ describe ::API::V3::Utilities::PathHelper do
     it_behaves_like 'api v3 path', '/attachments/1'
   end
 
-  describe '#attachment_download without file name' do
-    subject { helper.attachment_download 1 }
+  describe '#attachment_content' do
+    subject { helper.attachment_content 1 }
 
-    it_behaves_like 'path', '/attachments/1'
-  end
-
-  describe '#attachment_download with file name' do
-    subject { helper.attachment_download 1, 'file.png' }
-
-    it_behaves_like 'path', '/attachments/1/file.png'
+    it_behaves_like 'api v3 path', '/attachments/1/content'
   end
 
   describe '#attachments_by_post' do

spec/requests/api/v3/attachments/attachment_resource_spec.rb

@@ -146,4 +146,65 @@ describe 'API v3 Attachment resource', type: :request, content_type: :json do
       end
     end
   end
+
+  describe '#content' do
+    let(:path) { api_v3_paths.attachment_content attachment.id }
+
+    before do
+      get path
+    end
+
+    subject(:response) { last_response }
+
+    context 'with required permissions' do
+      context 'for a local file' do
+        let(:mock_file) { FileHelpers.mock_uploaded_file name: 'foobar.txt' }
+        let(:attachment) do
+          att = FactoryBot.create(:attachment, container: container, file: mock_file)
+
+          att.file.store!
+          att.send :write_attribute, :file, mock_file.original_filename
+          att.send :write_attribute, :content_type, mock_file.content_type
+          att.save!
+          att
+        end
+
+        it 'responds with 200 OK' do
+          expect(subject.status).to eq 200
+        end
+
+        it 'has the necessary headers' do
+          expect(subject.headers['Content-Disposition'])
+            .to eql "attachment; filename=#{mock_file.original_filename}"
+
+          expect(subject.headers['Content-Type'])
+            .to eql mock_file.content_type
+        end
+
+        it 'sends the file in binary' do
+          expect(subject.body)
+            .to match(mock_file.read)
+        end
+      end
+
+      context 'for a remote file' do
+        let(:remote_url) { 'http://some_service.org/blubs.gif' }
+        let(:mock_file) { FileHelpers.mock_uploaded_file name: 'foobar.txt' }
+        let(:attachment) do
+          FactoryBot.create(:attachment, container: container, file: mock_file) do |a|
+            # need to mock here to avoid dependency on external service
+            allow_any_instance_of(Attachment)
+              .to receive(:external_url)
+              .and_return(remote_url)
+          end
+        end
+
+        it 'responds with 302 Redirect' do
+          expect(subject.status).to eq 302
+          expect(subject.headers['Location'])
+            .to eql remote_url
+        end
+      end
+    end
+  end
 end