From e8e4929ecbb4fe8ae81feba45331aaab123e0237 Mon Sep 17 00:00:00 2001 From: Alexander Griffen Date: Fri, 27 Oct 2023 17:03:38 +0100 Subject: [PATCH] WIP: Attachments controller This does remove a few tests that were looking specifically at IDs (or, 'ugly' IDs) --- app/controllers/attachments_controller.rb | 2 +- config/routes.rb | 4 +- .../attachments_controller_spec.rb | 48 ++----------------- 3 files changed, 7 insertions(+), 47 deletions(-) diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index a414aae17d6..345b312289d 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -57,7 +57,7 @@ def find_info_request if public_token? InfoRequest.find_by!(public_token: public_token) else - InfoRequest.find(params[:id]) + InfoRequest.find_by(url_title: params[:url_title]) end end diff --git a/config/routes.rb b/config/routes.rb index 124cb73e883..0889f08be84 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -122,13 +122,13 @@ def matches?(request) :as => :similar_request, :via => :get - match '/request/:id/response/:incoming_message_id/attach/html' \ + match '/request/:url_title/response/:incoming_message_id/attach/html' \ '/(:part(/*file_name))' => 'attachments#show_as_html', :format => false, :as => :get_attachment_as_html, :via => :get, :constraints => { :part => /\d+/ } - match '/request/:id/response/:incoming_message_id/attach/:part(/*file_name)' => 'attachments#show', + match '/request/:url_title/response/:incoming_message_id/attach/:part(/*file_name)' => 'attachments#show', :format => false, :as => :get_attachment, :via => :get, diff --git a/spec/controllers/attachments_controller_spec.rb b/spec/controllers/attachments_controller_spec.rb index d64ef2c5cea..a79fcbf2ec9 100644 --- a/spec/controllers/attachments_controller_spec.rb +++ b/spec/controllers/attachments_controller_spec.rb @@ -43,39 +43,11 @@ def show(params = {}) part: attachment.url_part_number, file_name: attachment.display_filename } - default_params[:id] = info_request.id unless params[:public_token] + default_params[:url_title] = info_request.url_title unless params[:public_token] rebuild_raw_emails(info_request) get :show, params: default_params.merge(params) end - # This is a regression test for a bug where URLs of this form were causing - # 500 errors instead of 404s. - # - # (Note that in fact only the integer-prefix of the URL part is used, so - # there are *some* “ugly URLs containing a request id that isn\'t an - # integer” that actually return a 200 response. The point is that IDs of - # this sort were triggering an error in the error-handling path, causing - # the wrong sort of error response to be returned in the case where the - # integer prefix referred to the wrong request.) - # - # https://github.com/mysociety/alaveteli/issues/351 - it 'should return 404 for ugly URLs containing a request id that isn\'t an integer' do - ugly_id = '55195' - expect { show(id: ugly_id) } - .to raise_error(ActiveRecord::RecordNotFound) - end - - it 'should return 404 when incoming message and request ids don\'t match' do - expect { show(id: info_request.id + 1) } - .to raise_error(ActiveRecord::RecordNotFound) - end - - it 'should return 404 for ugly URLs contain a request id that isn\'t an integer, even if the integer prefix refers to an actual request' do - ugly_id = '#{FactoryBot.create(:info_request).id}95' - expect { show(id: ugly_id) } - .to raise_error(ActiveRecord::RecordNotFound) - end - it 'should redirect to the incoming message if there\'s a wrong part number and an ambiguous filename' do show( part: attachment.url_part_number + 1, @@ -144,7 +116,7 @@ def show(params = {}) allow(verifier).to receive(:generate).with( get_attachment_path( incoming_message_id: attachment.incoming_message_id, - id: info_request.id, + url_title: info_request.url_title, part: attachment.url_part_number, file_name: attachment.filename ) @@ -416,7 +388,7 @@ def show_as_html(params = {}) part: attachment.url_part_number, file_name: attachment.display_filename } - default_params[:id] = info_request.id unless params[:public_token] + default_params[:url_title] = info_request.url_title unless params[:public_token] get :show_as_html, params: default_params.merge(params) end @@ -438,18 +410,6 @@ def show_as_html(params = {}) expect(response.headers['X-Robots-Tag']).to eq 'noindex' end - it 'should return 404 for ugly URLs containing a request id that isn\'t an integer' do - ugly_id = '55195' - expect { show_as_html(id: ugly_id) } - .to raise_error(ActiveRecord::RecordNotFound) - end - - it 'should return 404 for ugly URLs contain a request id that isn\'t an integer, even if the integer prefix refers to an actual request' do - ugly_id = FactoryBot.create(:info_request).id.to_s + '95' - expect { show_as_html(id: ugly_id) } - .to raise_error(ActiveRecord::RecordNotFound) - end - context 'when attachment has not been masked' do let(:attachment) do FactoryBot.create( @@ -499,7 +459,7 @@ def show_as_html(params = {}) allow(verifier).to receive(:generate).with( get_attachment_as_html_path( incoming_message_id: attachment.incoming_message_id, - id: info_request.id, + url_title: info_request.url_title, part: attachment.url_part_number, file_name: attachment.filename )