From 9b579de9e234e378fb5081d79bea02d175495db7 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Tue, 22 Jul 2008 17:55:19 +0000 Subject: [PATCH] Appends the filename to the attachment url so that clients that ignore content-disposition http header get the real filename (#1649). git-svn-id: http://redmine.rubyforge.org/svn/trunk@1686 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/attachments_controller.rb | 3 +++ app/helpers/application_helper.rb | 11 +++++++++++ app/helpers/issues_helper.rb | 4 ++-- app/models/attachment.rb | 2 +- app/views/attachments/_links.rhtml | 2 +- app/views/attachments/diff.rhtml | 2 +- app/views/attachments/file.rhtml | 2 +- app/views/projects/list_files.rhtml | 3 +-- config/routes.rb | 6 ++++-- test/functional/attachments_controller_test.rb | 15 +++++++++++++++ 10 files changed, 40 insertions(+), 10 deletions(-) diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 07fee1269..1e8f566e6 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -43,6 +43,9 @@ class AttachmentsController < ApplicationController private def find_project @attachment = Attachment.find(params[:id]) + # Show 404 if the filename in the url is wrong + raise ActiveRecord::RecordNotFound if params[:filename] && params[:filename] != @attachment.filename + @project = @attachment.project permission = @attachment.container.is_a?(Version) ? :view_files : "view_#{@attachment.container.class.name.underscore.pluralize}".to_sym allowed = User.current.allowed_to?(permission, @project) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 6c69d8e6f..6e39d093f 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -47,6 +47,17 @@ module ApplicationHelper link_to "#{issue.tracker.name} ##{issue.id}", {:controller => "issues", :action => "show", :id => issue}, options end + # Generates a link to an attachment. + # Options: + # * :text - Link text (default to attachment filename) + # * :download - Force download (default: false) + def link_to_attachment(attachment, options={}) + text = options.delete(:text) || attachment.filename + action = options.delete(:download) ? 'download' : 'show' + + link_to(h(text), {:controller => 'attachments', :action => action, :id => attachment, :filename => attachment.filename }, options) + end + def toggle_link(name, id, options={}) onclick = "Element.toggle('#{id}'); " onclick << (options[:focus] ? "Form.Element.focus('#{options[:focus]}'); " : "this.blur(); ") diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 403697178..9e419ab43 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -95,9 +95,9 @@ module IssuesHelper label = content_tag('strong', label) old_value = content_tag("i", h(old_value)) if detail.old_value old_value = content_tag("strike", old_value) if detail.old_value and (!detail.value or detail.value.empty?) - if detail.property == 'attachment' && !value.blank? && Attachment.find_by_id(detail.prop_key) + if detail.property == 'attachment' && !value.blank? && a = Attachment.find_by_id(detail.prop_key) # Link to the attachment if it has not been removed - value = link_to(value, :controller => 'attachments', :action => 'show', :id => detail.prop_key) + value = link_to_attachment(a) else value = content_tag("i", h(value)) if value end diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 7d6a74ebb..8f3f530c7 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -26,7 +26,7 @@ class Attachment < ActiveRecord::Base validates_length_of :disk_filename, :maximum => 255 acts_as_event :title => :filename, - :url => Proc.new {|o| {:controller => 'attachments', :action => 'download', :id => o.id}} + :url => Proc.new {|o| {:controller => 'attachments', :action => 'download', :id => o.id, :filename => o.filename}} cattr_accessor :storage_path @@storage_path = "#{RAILS_ROOT}/files" diff --git a/app/views/attachments/_links.rhtml b/app/views/attachments/_links.rhtml index 9e3ac747c..9aae909fe 100644 --- a/app/views/attachments/_links.rhtml +++ b/app/views/attachments/_links.rhtml @@ -1,6 +1,6 @@
<% for attachment in attachments %> -

<%= link_to attachment.filename, {:controller => 'attachments', :action => 'show', :id => attachment }, :class => 'icon icon-attachment' -%> +

<%= link_to_attachment attachment, :class => 'icon icon-attachment' -%> <%= h(" - #{attachment.description}") unless attachment.description.blank? %> (<%= number_to_human_size attachment.filesize %>) <% if options[:delete_url] %> diff --git a/app/views/attachments/diff.rhtml b/app/views/attachments/diff.rhtml index 4a9f5c9bd..7b64dca17 100644 --- a/app/views/attachments/diff.rhtml +++ b/app/views/attachments/diff.rhtml @@ -3,7 +3,7 @@

<%= h("#{@attachment.description} - ") unless @attachment.description.blank? %> <%= @attachment.author %>, <%= format_time(@attachment.created_on) %>

-

<%= link_to l(:button_download), {:controller => 'attachments', :action => 'download', :id => @attachment } -%> +

<%= link_to_attachment @attachment, :text => l(:button_download), :download => true -%> (<%= number_to_human_size @attachment.filesize %>)

diff --git a/app/views/attachments/file.rhtml b/app/views/attachments/file.rhtml index 7988d0aed..468c6b666 100644 --- a/app/views/attachments/file.rhtml +++ b/app/views/attachments/file.rhtml @@ -3,7 +3,7 @@

<%= h("#{@attachment.description} - ") unless @attachment.description.blank? %> <%= @attachment.author %>, <%= format_time(@attachment.created_on) %>

-

<%= link_to l(:button_download), {:controller => 'attachments', :action => 'download', :id => @attachment } -%> +

<%= link_to_attachment @attachment, :text => l(:button_download), :download => true -%> (<%= number_to_human_size @attachment.filesize %>)

diff --git a/app/views/projects/list_files.rhtml b/app/views/projects/list_files.rhtml index 43687c50a..79e41f16d 100644 --- a/app/views/projects/list_files.rhtml +++ b/app/views/projects/list_files.rhtml @@ -23,8 +23,7 @@ <% for file in version.attachments %> "> - <%= link_to(h(file.filename), {:controller => 'attachments', :action => 'download', :id => file}, - :title => file.description) %> + <%= link_to_attachment file, :download => true, :title => file.description %> <%= format_time(file.created_on) %> <%= number_to_human_size(file.filesize) %> <%= file.downloads %> diff --git a/config/routes.rb b/config/routes.rb index a77a8833b..c34052f86 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -32,8 +32,10 @@ ActionController::Routing::Routes.draw do |map| omap.repositories_revision 'repositories/revision/:id/:rev', :action => 'revision' end - map.connect 'attachments/:id', :controller => 'attachments', :action => 'show' - + map.connect 'attachments/:id', :controller => 'attachments', :action => 'show', :id => /\d+/ + map.connect 'attachments/:id/:filename', :controller => 'attachments', :action => 'show', :id => /\d+/, :filename => /.*/ + map.connect 'attachments/download/:id/:filename', :controller => 'attachments', :action => 'download', :id => /\d+/, :filename => /.*/ + # Allow downloading Web Service WSDL as a file with an extension # instead of a file named 'wsdl' map.connect ':controller/service.wsdl', :action => 'wsdl' diff --git a/test/functional/attachments_controller_test.rb b/test/functional/attachments_controller_test.rb index af73eb77e..06a6343ba 100644 --- a/test/functional/attachments_controller_test.rb +++ b/test/functional/attachments_controller_test.rb @@ -33,6 +33,21 @@ class AttachmentsControllerTest < Test::Unit::TestCase User.current = nil end + def test_routing + assert_routing('/attachments/1', :controller => 'attachments', :action => 'show', :id => '1') + assert_routing('/attachments/1/filename.ext', :controller => 'attachments', :action => 'show', :id => '1', :filename => 'filename.ext') + assert_routing('/attachments/download/1', :controller => 'attachments', :action => 'download', :id => '1') + assert_routing('/attachments/download/1/filename.ext', :controller => 'attachments', :action => 'download', :id => '1', :filename => 'filename.ext') + end + + def test_recognizes + assert_recognizes({:controller => 'attachments', :action => 'show', :id => '1'}, '/attachments/1') + assert_recognizes({:controller => 'attachments', :action => 'show', :id => '1'}, '/attachments/show/1') + assert_recognizes({:controller => 'attachments', :action => 'show', :id => '1', :filename => 'filename.ext'}, '/attachments/1/filename.ext') + assert_recognizes({:controller => 'attachments', :action => 'download', :id => '1'}, '/attachments/download/1') + assert_recognizes({:controller => 'attachments', :action => 'download', :id => '1', :filename => 'filename.ext'},'/attachments/download/1/filename.ext') + end + def test_show_diff get :show, :id => 5 assert_response :success