diff --git a/app/controllers/case_workers/admin/management_information_controller.rb b/app/controllers/case_workers/admin/management_information_controller.rb index e6ab616347..2f3e588e94 100644 --- a/app/controllers/case_workers/admin/management_information_controller.rb +++ b/app/controllers/case_workers/admin/management_information_controller.rb @@ -5,9 +5,9 @@ module Admin class ManagementInformationController < CaseWorkers::Admin::ApplicationController include ActiveStorage::SetCurrent - skip_load_and_authorize_resource only: %i[index download generate create] - before_action -> { authorize! :view, :management_information }, only: %i[index download generate create] - before_action :validate_report_type, only: %i[download generate create] + skip_load_and_authorize_resource only: %i[index generate create] + before_action -> { authorize! :view, :management_information }, only: %i[index generate create] + before_action :validate_report_type, only: %i[generate create] before_action :validate_report_type_is_updatable, only: %i[generate create] before_action :validate_and_set_date, only: %i[create] @@ -17,17 +17,6 @@ def index end end - def download - log_download_start - record = Stats::StatsReport.most_recent_by_type(params[:report_type]) - - if record.document.attached? - redirect_to record.document.blob.url(disposition: 'attachment'), allow_other_host: true - else - redirect_to case_workers_admin_management_information_url, alert: t('.missing_report') - end - end - def generate StatsReportGenerationJob.perform_later(report_type: params[:report_type]) message = t('case_workers.admin.management_information.job_scheduled') @@ -69,13 +58,13 @@ def validate_and_set_date redirect_to case_workers_admin_management_information_url, alert: t('.invalid_report_date') end - def log_download_start - LogStuff.info(class: 'CaseWorkers::Admin::ManagementInformationController', - action: 'download', - downloading_user_id: @current_user&.id) do - 'MI Report download started' - end - end + # def log_download_start + # LogStuff.info(class: 'CaseWorkers::Admin::ManagementInformationController', + # action: 'download', + # downloading_user_id: @current_user&.id) do + # 'MI Report download started' + # end + # end end end end diff --git a/app/controllers/csp_reports_controller.rb b/app/controllers/csp_reports_controller.rb index f54ace393b..dfd5744192 100644 --- a/app/controllers/csp_reports_controller.rb +++ b/app/controllers/csp_reports_controller.rb @@ -3,13 +3,13 @@ class CspReportsController < ApplicationController skip_forgery_protection def create - slack_notifier.build_payload( - icon: ':security:', - title: 'Content Security Policy violation', - message: report.map { |key, value| "#{key}: #{value}" }.join("\n") + "\n\nUser agent: #{request.env['HTTP_USER_AGENT']}", - status: :fail - ) - slack_notifier.send_message + # slack_notifier.build_payload( + # icon: ':security:', + # title: 'Content Security Policy violation', + # message: report.map { |key, value| "#{key}: #{value}" }.join("\n") + "\n\nUser agent: #{request.env['HTTP_USER_AGENT']}", + # status: :fail + # ) + # slack_notifier.send_message head :ok end diff --git a/app/controllers/documents_controller.rb b/app/controllers/documents_controller.rb index 2e81dd2b86..a9560dcfb2 100644 --- a/app/controllers/documents_controller.rb +++ b/app/controllers/documents_controller.rb @@ -2,7 +2,7 @@ class DocumentsController < ApplicationController include ActiveStorage::SetCurrent respond_to :html - before_action :document, only: %i[show download destroy] + before_action :document, only: %i[show destroy] def index return render json: [] if params[:form_id].blank? @@ -12,11 +12,8 @@ def index def show raise ActiveRecord::RecordNotFound, 'Preview not found' unless document.converted_preview_document.attached? - redirect_to document.converted_preview_document.blob.url(disposition: :inline), allow_other_host: true - end - - def download - redirect_to document.document.blob.url(disposition: :attachment), allow_other_host: true + redirect_to document.converted_preview_document.rails_blob_path(message.attachment, disposition: 'attachment'), + allow_other_host: true end def create diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb index 210e17207d..b12c40ccc9 100644 --- a/app/controllers/messages_controller.rb +++ b/app/controllers/messages_controller.rb @@ -34,12 +34,6 @@ def create end end - def download_attachment - raise 'No attachment present on this message' unless message.attachment.attached? - - redirect_to message.attachment.blob.url(disposition: 'attachment'), allow_other_host: true - end - private def message diff --git a/app/models/ability.rb b/app/models/ability.rb index e6b1168089..f7e7c4e662 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -22,7 +22,7 @@ def initialize(user) end # applies to all external users and case workers - can %i[create download_attachment], Message + can %i[create], Message can %i[index update], UserMessageStatus can [:update_settings], User, id: user.id @@ -46,7 +46,7 @@ def external_user(persona) def case_worker_admin(persona) can %i[index show update archived], Claim::BaseClaim - can %i[show download], Document + can %i[show], Document can %i[index new create], CaseWorker can %i[show show_message_controls edit change_password update_password update destroy], CaseWorker can %i[new create], Allocation @@ -61,7 +61,7 @@ def case_worker(persona) claim.case_workers.include?(persona) end can_administer_any_provider if persona.roles.include?('provider_management') - can %i[show download], Document + can %i[show], Document can_manage_own_password(persona) can %i[dismiss], InjectionAttempt end @@ -87,7 +87,7 @@ def can_administer_documents_in_provider(persona) can %i[index create], Document # NOTE: for destroy action, at least, the document may not be persisted/saved - can %i[show download destroy], Document do |document| + can %i[show destroy], Document do |document| if document.external_user_id.nil? User.active.find(document.creator_id).persona.provider.id == persona.provider.id else @@ -127,7 +127,7 @@ def can_manage_own_claims_of_class(persona, claim_klass) def can_manage_own_documents(persona) can %i[index create], Document - can %i[show download destroy], Document do |document| + can %i[show destroy], Document do |document| if document.external_user_id.nil? document.creator_id == persona.user.id else diff --git a/app/presenters/message_presenter.rb b/app/presenters/message_presenter.rb index 4e44bc4260..ff63690cea 100644 --- a/app/presenters/message_presenter.rb +++ b/app/presenters/message_presenter.rb @@ -1,4 +1,5 @@ class MessagePresenter < BasePresenter + include Rails.application.routes.url_helpers presents :message def sender_is_a?(klass) @@ -32,7 +33,9 @@ def download_file_link h.concat( h.tag.a( "#{attachment_file_name} (#{attachment_file_size})", - href: "/messages/#{message.id}/download_attachment", + href: rails_blob_path(message.attachment, + disposition: 'attachment', + host: Rails.application.config.action_mailer.default_url_options[:host]), title: 'Download ' + attachment_file_name ) ) diff --git a/app/views/case_workers/admin/management_information/_stats_report.html.haml b/app/views/case_workers/admin/management_information/_stats_report.html.haml index 625e49913a..90ec7d1dc7 100644 --- a/app/views/case_workers/admin/management_information/_stats_report.html.haml +++ b/app/views/case_workers/admin/management_information/_stats_report.html.haml @@ -12,7 +12,7 @@ - if report_rec&.started_at.present? = govuk_link_button(t('download_html', scope: i18n_scope, report_name: report_name_html), - case_workers_admin_management_information_download_url(report_type: report_object.name, format: :csv)) + rails_blob_path(report_rec.document, disposition: "attachment")) - if report_object.updatable? = govuk_link_button_secondary(t('update_report_html', scope: i18n_scope, report_name: report_name_html), diff --git a/app/views/case_workers/admin/management_information/_stats_report_with_date_form.html.haml b/app/views/case_workers/admin/management_information/_stats_report_with_date_form.html.haml index 83ebbc2104..37667bd5bd 100644 --- a/app/views/case_workers/admin/management_information/_stats_report_with_date_form.html.haml +++ b/app/views/case_workers/admin/management_information/_stats_report_with_date_form.html.haml @@ -8,7 +8,7 @@ time: report_rec.started_at.strftime(Settings.report_date_format)) = govuk_link_button(t('download_html', scope: i18n_scope, report_name: report_name_html), - case_workers_admin_management_information_download_url(report_type: report_object.name, format: :csv)) + rails_blob_path(report_rec.document, disposition: "attachment")) - else %p = t('unavailable_report', scope: i18n_scope) diff --git a/app/views/external_users/claims/supporting_documents/_existing.html.haml b/app/views/external_users/claims/supporting_documents/_existing.html.haml index 13aed431f0..26f24e34fa 100644 --- a/app/views/external_users/claims/supporting_documents/_existing.html.haml +++ b/app/views/external_users/claims/supporting_documents/_existing.html.haml @@ -17,10 +17,10 @@ = govuk_table_td('data-label': t('.action')) do .app-link-group = govuk_link_to t('common.download_html', context: "#{document.document_file_name}"), - download_document_path(document), + rails_blob_url(document.document, disposition: 'inline'), class: 'download' = govuk_link_to t('common.remove_html', context: "#{document.document_file_name}"), - document_path(document), + rails_blob_url(document.document, disposition: 'inline'), method: :delete, remote: true, data: { confirm: 'Are you sure?' } diff --git a/app/views/external_users/claims/supporting_evidence/_summary.html.haml b/app/views/external_users/claims/supporting_evidence/_summary.html.haml index 4c3d45635b..2ae55fbdeb 100644 --- a/app/views/external_users/claims/supporting_evidence/_summary.html.haml +++ b/app/views/external_users/claims/supporting_evidence/_summary.html.haml @@ -15,7 +15,7 @@ - claim.documents.includes(:document_blob, :converted_preview_document_attachment).each do |document| %li = govuk_link_to document.document_file_name, - download_document_path(document), + rails_blob_url(document.document, disposition: 'inline'), class: 'download', 'aria-label': "Download document: #{document.document_file_name}" diff --git a/app/views/shared/_evidence_list.html.haml b/app/views/shared/_evidence_list.html.haml index 76ccca7466..95cde93a3f 100644 --- a/app/views/shared/_evidence_list.html.haml +++ b/app/views/shared/_evidence_list.html.haml @@ -46,5 +46,4 @@ .app-link-group - if document.converted_preview_document.present? = govuk_link_to t('common.view_html', context: "#{document.document_file_name}"), document_path(document) - - = govuk_link_to t('common.download_html', context: "#{document.document_file_name}"), download_document_path(document) + = govuk_link_to t('common.download_html', context: "#{document.document_file_name}"),rails_blob_path(document.attachment, disposition: "attachment") diff --git a/config/routes.rb b/config/routes.rb index 301285cede..3823f30396 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -58,13 +58,9 @@ resources :claim_intentions, only: [:create], format: :json - resources :documents do - get 'download', on: :member - end + resources :documents - resources :messages, only: [:create] do - get 'download_attachment', on: :member - end + resources :messages, only: [:create] resources :establishments, only: %i[index], format: :js resources :offences, only: [:index], format: :js @@ -189,7 +185,6 @@ end get 'management_information', to: 'management_information#index', as: :management_information - get 'management_information/download', to: 'management_information#download', as: :management_information_download get 'management_information/generate', to: 'management_information#generate', as: :management_information_generate post 'management_information/create', to: 'management_information#create', as: :management_information_create end @@ -213,7 +208,14 @@ # catch-all route # ------------------------------------------------- # WARNING: do not put routes below this point - unless Rails.env.development? - match '*path', to: 'errors#not_found', via: :all + # unless Rails.env.development? + # match '*path', to: 'errors#not_found', via: :all + # end + + if Rails.env.development? + scope format: true, constraints: { format: /jpg|png|gif|PNG/ } do + get '/*anything', to: proc { [404, {}, ['']] }, constraints: lambda { |request| !request.path_parameters[:anything].start_with?('rails/') } + end end + end diff --git a/spec/controllers/messages_controller_spec.rb b/spec/controllers/messages_controller_spec.rb index e5690de59a..1293600b9d 100644 --- a/spec/controllers/messages_controller_spec.rb +++ b/spec/controllers/messages_controller_spec.rb @@ -75,31 +75,6 @@ end end end - - describe 'GET #download_attachment' do - subject(:download_attachment) { get :download_attachment, params: { id: message.id } } - - context 'when message has attachment' do - let(:message) { create(:message) } - let(:test_url) { 'https://document.storage/attachment.doc#123abc' } - - before do - message.attachment.attach(io: StringIO.new, filename: 'attachment.doc') - allow(Message).to receive(:find).with(message[:id].to_s).and_return(message) - allow(message.attachment.blob).to receive(:url).and_return(test_url) - end - - it { is_expected.to redirect_to test_url } - end - - context 'when message does not have attachment' do - let(:message) { create(:message) } - - it 'redirects to 500 page' do - expect { download_attachment }.to raise_exception('No attachment present on this message') - end - end - end end context 'email notifications' do diff --git a/spec/requests/active_storage_spec.rb b/spec/requests/active_storage_spec.rb new file mode 100644 index 0000000000..67ae267d7c --- /dev/null +++ b/spec/requests/active_storage_spec.rb @@ -0,0 +1,37 @@ +# spec/requests/active_storage_spec.rb + +require 'rails_helper' + +RSpec.describe 'ActiveStorage', type: :request do + describe 'GET /rails/active_storage/blobs/:signed_id/:filename' do + let(:message) { create(:message) } + let(:attachment_content) { 'dummy content' } + let(:attachment_filename) { 'attachment.doc' } + let(:attachment_content_type) { 'application/msword' } + + before do + message.attachment.attach(io: StringIO.new(attachment_content), filename: attachment_filename, content_type: attachment_content_type) + end + + context 'when the message has an attachment' do + subject(:get_attachment) { get rails_blob_path(message.attachment, disposition: 'attachment') } + + it 'redirects to the ActiveStorage blob URL' do + get_attachment + expect(response).to have_http_status(:redirect) + expect(response.headers['Location']).to include('rails/active_storage/blobs/redirect/') + expect(response.headers['Location']).to include('?disposition=attachment') + end + end + + context 'when the message does not have an attachment' do + let(:message_without_attachment) { create(:message) } + + it 'raises an ActiveRecord::RecordNotFound error' do + expect { + get rails_blob_path(message_without_attachment.attachment, disposition: 'attachment', only_path: true) + }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end