diff --git a/app/helpers/versions_helper.rb b/app/helpers/versions_helper.rb index d4c8fb4a368..0036641bdaa 100644 --- a/app/helpers/versions_helper.rb +++ b/app/helpers/versions_helper.rb @@ -41,6 +41,8 @@ module VersionsHelper def link_to_version(version, html_options = {}, options = {}) return '' unless version&.is_a?(Version) + html_options = html_options.merge(id: link_to_version_id(version)) + link_name = options[:before_text].to_s.html_safe + format_version_name(version, options[:project] || @project) link_to_if version.visible?, link_name, @@ -48,6 +50,10 @@ module VersionsHelper html_options end + def link_to_version_id(version) + ERB::Util.url_encode("version-#{version.name}") + end + def format_version_name(version, project = @project) h(version.to_s_for_project(project)) end diff --git a/app/uploaders/direct_fog_uploader.rb b/app/uploaders/direct_fog_uploader.rb index 037a3cea8b8..a40748d2799 100644 --- a/app/uploaders/direct_fog_uploader.rb +++ b/app/uploaders/direct_fog_uploader.rb @@ -3,50 +3,79 @@ require_relative 'fog_file_uploader' class DirectFogUploader < FogFileUploader include CarrierWaveDirect::Uploader - def self.for_attachment(attachment) - for_uploader attachment.file - end - - def self.for_uploader(fog_file_uploader) - raise ArgumentError, "FogFileUploader expected" unless fog_file_uploader.is_a? FogFileUploader - - uploader = self.new - - uploader.instance_variable_set "@file", fog_file_uploader.file - uploader.instance_variable_set "@key", fog_file_uploader.path - - uploader - end - ## - # Generates the direct upload form for the given attachment. - # - # @param attachment [Attachment] The attachment for which a file is to be uploaded. - # @param success_action_redirect [String] URL to redirect to if successful (none by default, using status). - # @param success_action_status [String] The HTTP status to return on success (201 by default). - # @param max_file_size [Integer] The maximum file size to be allowed in bytes. - def self.direct_fog_hash( - attachment:, - success_action_redirect: nil, - success_action_status: "201", - max_file_size: Setting.attachment_max_size * 1024 - ) - uploader = for_attachment attachment + # This needs to be true so that the necessary condition is included + # in S3 upload policy (only relevant for direct uploads). + def will_include_content_type + true + end - if success_action_redirect.present? - uploader.success_action_redirect = success_action_redirect - uploader.use_action_status = false - else - uploader.success_action_status = success_action_status - uploader.use_action_status = true + class << self + def for_attachment(attachment) + for_uploader attachment.file end - hash = uploader.direct_fog_hash(enforce_utf8: false, max_file_size: max_file_size) + def for_uploader(fog_file_uploader) + raise ArgumentError, "FogFileUploader expected" unless fog_file_uploader.is_a? FogFileUploader - if success_action_redirect.present? - hash.merge(success_action_redirect: success_action_redirect) - else - hash.merge(success_action_status: success_action_status) + uploader = self.new + + uploader.instance_variable_set "@file", fog_file_uploader.file + uploader.instance_variable_set "@key", fog_file_uploader.path + uploader.instance_variable_set "@model", fog_file_uploader.model + + uploader + end + + ## + # Generates the direct upload form for the given attachment. + # + # @param attachment [Attachment] The attachment for which a file is to be uploaded. + # @param success_action_redirect [String] URL to redirect to if successful (none by default, using status). + # @param success_action_status [String] The HTTP status to return on success (201 by default). + # @param max_file_size [Integer] The maximum file size to be allowed in bytes. + def direct_fog_hash( + attachment:, + success_action_redirect: nil, + success_action_status: "201", + max_file_size: Setting.attachment_max_size * 1024 + ) + uploader = direct_fog_hash_uploader attachment, success_action_redirect, success_action_status + hash = uploader + .direct_fog_hash(enforce_utf8: false, max_file_size: max_file_size) + .merge(extra_fog_hash_attributes(uploader: uploader)) + + if success_action_redirect.present? + hash.merge(success_action_redirect: success_action_redirect) + else + hash.merge(success_action_status: success_action_status) + end + end + + def extra_fog_hash_attributes(uploader:) + return {} unless include_content_type?(uploader) + + { + "Content-Type": uploader.fog_attributes[:"Content-Type"] + } + end + + private + + def include_content_type?(uploader) + uploader.will_include_content_type && uploader.fog_attributes.include?(:"Content-Type") + end + + def direct_fog_hash_uploader(attachment, success_action_redirect, success_action_status) + for_attachment(attachment).tap do |uploader| + if success_action_redirect.present? + uploader.success_action_redirect = success_action_redirect + uploader.use_action_status = false + else + uploader.success_action_status = success_action_status + uploader.use_action_status = true + end + end end end end diff --git a/app/uploaders/fog_file_uploader.rb b/app/uploaders/fog_file_uploader.rb index e1249c621ac..6593ade2da4 100644 --- a/app/uploaders/fog_file_uploader.rb +++ b/app/uploaders/fog_file_uploader.rb @@ -57,6 +57,16 @@ class FogFileUploader < CarrierWave::Uploader::Base super end + ## + # This is necessary for carrierwave to set the Content-Type in the S3 metadata for instance. + def fog_attributes + content_type = model.content_type + + return super if content_type.blank? + + super.merge "Content-Type": content_type + end + ## # Generates a download URL for this file. # diff --git a/app/views/versions/_roadmap_filter.html.erb b/app/views/versions/_roadmap_filter.html.erb index 981484787de..8420936c5b4 100644 --- a/app/views/versions/_roadmap_filter.html.erb +++ b/app/views/versions/_roadmap_filter.html.erb @@ -21,7 +21,6 @@ <% if @project.descendants.active.any? %>
- <%= hidden_field_tag 'with_subprojects', 0 %>
<%= styled_label_tag "with-subprojects", t(:label_subproject_plural) %> diff --git a/app/views/versions/_roadmap_version_links.html.erb b/app/views/versions/_roadmap_version_links.html.erb index cbb415e44d7..68c3b6814c4 100644 --- a/app/views/versions/_roadmap_version_links.html.erb +++ b/app/views/versions/_roadmap_version_links.html.erb @@ -1,5 +1,8 @@

<%= t(:label_version_plural) %>

<% @versions.each do |version| %> - <%= link_to format_version_name(version), "#{project_roadmap_url}##{version.name}" %>
+ <%= link_to format_version_name(version), + project_roadmap_path({ anchor: link_to_version_id(version) }.merge(params.permit(:completed, + :with_subprojects, + type_ids: []).to_h)) %>
<% end %> diff --git a/app/views/versions/index.html.erb b/app/views/versions/index.html.erb index ec5e3c756c1..cbb9b2fe4a1 100644 --- a/app/views/versions/index.html.erb +++ b/app/views/versions/index.html.erb @@ -45,7 +45,7 @@ See docs/COPYRIGHT.rdoc for more details.
<% @versions.each do |version| %>

- <%= link_to_version version, name: h(version.name) %> + <%= link_to_version(version, name: h(version.name), id: "version-#{version.name}") %>

<%= render partial: 'versions/overview', locals: {version: version} %> <%= render(partial: "wiki/content", locals: {content: version.wiki_page.content}) if version.wiki_page %> diff --git a/modules/costs/spec/requests/api/attachments/attachments_by_budget_resource_spec.rb b/modules/costs/spec/requests/api/attachments/attachments_by_budget_resource_spec.rb index 819a49e90d9..dd5f1e91eda 100644 --- a/modules/costs/spec/requests/api/attachments/attachments_by_budget_resource_spec.rb +++ b/modules/costs/spec/requests/api/attachments/attachments_by_budget_resource_spec.rb @@ -68,8 +68,8 @@ describe 'API v3 Attachments by budget resource', type: :request do let(:permissions) { %i[view_budgets edit_budgets] } let(:request_path) { api_v3_paths.attachments_by_budget budget.id } - let(:request_parts) { { metadata: metadata, file: file } } - let(:metadata) { { fileName: 'cat.png' }.to_json } + let(:request_parts) { { metadata: metadata.to_json, file: file } } + let(:metadata) { { fileName: 'cat.png' } } let(:file) { mock_uploaded_file(name: 'original-filename.txt') } let(:max_file_size) { 1 } # given in kiB @@ -99,19 +99,19 @@ describe 'API v3 Attachments by budget resource', type: :request do context 'file section is missing' do # rack-test won't send a multipart request without a file being present # however as long as we depend on correctly named sections this test should do just fine - let(:request_parts) { { metadata: metadata, wrongFileSection: file } } + let(:request_parts) { { metadata: metadata.to_json, wrongFileSection: file } } it_behaves_like 'invalid request body', I18n.t('api_v3.errors.multipart_body_error') end context 'metadata section is no valid JSON' do - let(:metadata) { '"fileName": "cat.png"' } + let(:request_parts) { { metadata: '"fileName": "cat.png"', file: file } } it_behaves_like 'parse error' end context 'metadata is missing the fileName' do - let(:metadata) { Hash.new.to_json } + let(:metadata) { Hash.new } it_behaves_like 'constraint violation' do let(:message) { "fileName #{I18n.t('activerecord.errors.messages.blank')}" } diff --git a/modules/documents/spec/requests/api/v3/attachments/attachments_by_documents_resource_spec.rb b/modules/documents/spec/requests/api/v3/attachments/attachments_by_documents_resource_spec.rb index e4f723c154a..6b1457fe3ff 100644 --- a/modules/documents/spec/requests/api/v3/attachments/attachments_by_documents_resource_spec.rb +++ b/modules/documents/spec/requests/api/v3/attachments/attachments_by_documents_resource_spec.rb @@ -69,8 +69,8 @@ describe 'API v3 Attachments by document resource', type: :request do let(:permissions) { %i[view_documents manage_documents] } let(:request_path) { api_v3_paths.attachments_by_document document.id } - let(:request_parts) { { metadata: metadata, file: file } } - let(:metadata) { { fileName: 'cat.png' }.to_json } + let(:request_parts) { { metadata: metadata.to_json, file: file } } + let(:metadata) { { fileName: 'cat.png' } } let(:file) { mock_uploaded_file(name: 'original-filename.txt') } let(:max_file_size) { 1 } # given in kiB @@ -100,19 +100,19 @@ describe 'API v3 Attachments by document resource', type: :request do context 'file section is missing' do # rack-test won't send a multipart request without a file being present # however as long as we depend on correctly named sections this test should do just fine - let(:request_parts) { { metadata: metadata, wrongFileSection: file } } + let(:request_parts) { { metadata: metadata.to_json, wrongFileSection: file } } it_behaves_like 'invalid request body', I18n.t('api_v3.errors.multipart_body_error') end context 'metadata section is no valid JSON' do - let(:metadata) { '"fileName": "cat.png"' } + let(:request_parts) { { metadata: '"fileName": "cat.png"', file: file } } it_behaves_like 'parse error' end context 'metadata is missing the fileName' do - let(:metadata) { Hash.new.to_json } + let(:metadata) { Hash.new } it_behaves_like 'constraint violation' do let(:message) { "fileName #{I18n.t('activerecord.errors.messages.blank')}" } diff --git a/spec/helpers/versions_helper_spec.rb b/spec/helpers/versions_helper_spec.rb index 2bc3423c7e1..4e14bf65a6a 100644 --- a/spec/helpers/versions_helper_spec.rb +++ b/spec/helpers/versions_helper_spec.rb @@ -59,7 +59,8 @@ describe VersionsHelper, type: :helper do context 'a version' do context 'with being allowed to see the version' do it 'does not create a link, without permission' do - expect(link_to_version(version)).to eq("#{test_project.name} - #{version.name}") + expect(link_to_version(version)) + .to eq("#{test_project.name} - #{version.name}") end end @@ -71,21 +72,22 @@ describe VersionsHelper, type: :helper do end it 'generates a link' do - expect(link_to_version(version)).to eq("#{test_project.name} - #{version.name}") + expect(link_to_version(version)) + .to be_html_eql("#{test_project.name} - #{version.name}") end it 'generates a link within a project' do @project = test_project - expect(link_to_version(version)).to eq("#{version.name}") + expect(link_to_version(version)) + .to be_html_eql("#{version.name}") end end end - describe 'an invalid version' do - let(:version) { Object } - - it 'does not generate a link' do - expect(link_to_version(Object)).to be_empty + describe '#link_to_version_id' do + it 'generates an escaped id' do + expect(link_to_version_id(version)) + .to eql("version-#{ERB::Util.url_encode(version.name)}") end end end diff --git a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb index e1aec441709..b44f070c0ef 100644 --- a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb +++ b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb @@ -42,8 +42,8 @@ shared_examples 'it supports direct uploads' do end describe 'POST /prepare', with_settings: { attachment_max_size: 512 } do - let(:request_parts) { { metadata: metadata, file: file } } - let(:metadata) { { fileName: 'cat.png', fileSize: file.size }.to_json } + let(:request_parts) { { metadata: metadata.to_json, file: file } } + let(:metadata) { { fileName: 'cat.png', fileSize: file.size, contentType: 'image/png' } } let(:file) { mock_uploaded_file(name: 'original-filename.txt') } def request! @@ -68,7 +68,7 @@ shared_examples 'it supports direct uploads' do end context 'with no filesize metadata' do - let(:metadata) { { fileName: 'cat.png' }.to_json } + let(:metadata) { { fileName: 'cat.png' } } it 'should respond with 422 due to missing file size metadata' do expect(subject.status).to eq(422) @@ -125,8 +125,21 @@ shared_examples 'it supports direct uploads' do "success_action_status" ) + expect(fields["Content-Type"]).to eq metadata[:contentType] + expect(fields["key"]).to end_with "cat.png" end + + it 'should also include the content type and the necessary policy in the form fields' do + fields = link["form_fields"] + + expect(fields).to include("policy", "Content-Type") + expect(fields["Content-Type"]).to eq metadata[:contentType] + + policy = Base64.decode64 fields["policy"] + + expect(policy).to include '["starts-with","$Content-Type",""]' + end end end end @@ -214,8 +227,8 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j let(:permissions) { Array(update_permission) } let(:request_path) { api_v3_paths.attachments } - let(:request_parts) { { metadata: metadata, file: file } } - let(:metadata) { { fileName: 'cat.png' }.to_json } + let(:request_parts) { { metadata: metadata.to_json, file: file } } + let(:metadata) { { fileName: 'cat.png' } } let(:file) { mock_uploaded_file(name: 'original-filename.txt') } let(:max_file_size) { 1 } # given in kiB @@ -248,19 +261,19 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j context 'file section is missing' do # rack-test won't send a multipart request without a file being present # however as long as we depend on correctly named sections this test should do just fine - let(:request_parts) { { metadata: metadata, wrongFileSection: file } } + let(:request_parts) { { metadata: metadata.to_json, wrongFileSection: file } } it_behaves_like 'invalid request body', I18n.t('api_v3.errors.multipart_body_error') end context 'metadata section is no valid JSON' do - let(:metadata) { '"fileName": "cat.png"' } + let(:request_parts) { { metadata: '"fileName": "cat.png"', file: file } } it_behaves_like 'parse error' end context 'metadata is missing the fileName' do - let(:metadata) { Hash.new.to_json } + let(:metadata) { Hash.new } it_behaves_like 'constraint violation' do let(:message) { "fileName #{I18n.t('activerecord.errors.messages.blank')}" } @@ -496,8 +509,8 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j describe '#post' do let(:request_path) { api_v3_paths.send "attachments_by_#{attachment_type}", container.id } - let(:request_parts) { { metadata: metadata, file: file } } - let(:metadata) { { fileName: 'cat.png' }.to_json } + let(:request_parts) { { metadata: metadata.to_json, file: file } } + let(:metadata) { { fileName: 'cat.png' } } let(:file) { mock_uploaded_file(name: 'original-filename.txt') } let(:max_file_size) { 1 } # given in kiB @@ -527,19 +540,19 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j context 'file section is missing' do # rack-test won't send a multipart request without a file being present # however as long as we depend on correctly named sections this test should do just fine - let(:request_parts) { { metadata: metadata, wrongFileSection: file } } + let(:request_parts) { { metadata: metadata.to_json, wrongFileSection: file } } it_behaves_like 'invalid request body', I18n.t('api_v3.errors.multipart_body_error') end context 'metadata section is no valid JSON' do - let(:metadata) { '"fileName": "cat.png"' } + let(:request_parts) { { metadata: '"fileName": "cat.png"', file: file } } it_behaves_like 'parse error' end context 'metadata is missing the fileName' do - let(:metadata) { Hash.new.to_json } + let(:metadata) { Hash.new } it_behaves_like 'constraint violation' do let(:message) { "fileName #{I18n.t('activerecord.errors.messages.blank')}" } diff --git a/spec/requests/api/v3/attachments_spec.rb b/spec/requests/api/v3/attachments_spec.rb index 99d9332bd34..a0956d3c488 100644 --- a/spec/requests/api/v3/attachments_spec.rb +++ b/spec/requests/api/v3/attachments_spec.rb @@ -50,8 +50,8 @@ describe API::V3::Attachments::AttachmentsAPI, type: :request do let(:permissions) { [] } let(:request_path) { api_v3_paths.prepare_new_attachment_upload } - let(:request_parts) { { metadata: metadata, file: file } } - let(:metadata) { { fileName: 'cat.png' }.to_json } + let(:request_parts) { { metadata: metadata.to_json, file: file } } + let(:metadata) { { fileName: 'cat.png' } } let(:file) { mock_uploaded_file(name: 'original-filename.txt') } before do