URL Perms: Attachments controller

Replace request IDs in URLs with titles This does remove a few tests that were looking specifically at IDs, or 'ugly' IDs)
mysociety · Dec 7, 2023 · 659adc5 · 659adc5
1 parent f1ef467
commit 659adc5
Show file tree

Hide file tree

Showing 11 changed files with 40 additions and 64 deletions.
diff --git 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[:request_url_title])
       end
   end
 

diff --git a/app/helpers/info_request_helper.rb b/app/helpers/info_request_helper.rb
@@ -315,7 +315,8 @@ def attachment_params(attachment, options = {})
     if public_token?
       attach_params[:public_token] = public_token
     else
-      attach_params[:id] = attachment.incoming_message.info_request_id
+      attach_params[:request_url_title] = attachment.incoming_message.
+        info_request.url_title
     end
     if options[:html]
       attach_params[:file_name] = "#{attachment.display_filename}.html"

diff --git a/config/routes.rb b/config/routes.rb
@@ -127,13 +127,14 @@ def matches?(request)
         :as => :similar_request,
         :via => :get
 
-  match '/request/:id/response/:incoming_message_id/attach/html' \
-        '/(:part(/*file_name))' => 'attachments#show_as_html',
+  match '/request/: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/:request_url_title/response/:incoming_message_id/attach' \
+        '/:part(/*file_name)' => 'attachments#show',
         :format => false,
         :as => :get_attachment,
         :via => :get,

diff --git a/config/routes/redirects.rb b/config/routes/redirects.rb
@@ -33,3 +33,12 @@ def redirect_to_info_request_url_title_path(request:, locale:, id:, suffix: nil)
       redirect_to_info_request_url_title_path(request: request, **params)
     end
   )
+
+get '/request/:id(/*suffix)',
+  format: false,
+  constraints: { id: /\d+/, suffix: %r(response/\d+/attach(/html)?/\d+/.*) },
+  to: redirect(
+    -> (params, request) do
+      redirect_to_info_request_url_title_path(request: request, **params)
+    end
+  )
diff --git a/spec/controllers/attachments_controller_spec.rb b/spec/controllers/attachments_controller_spec.rb
@@ -43,39 +43,13 @@ def show(params = {})
         part: attachment.url_part_number,
         file_name: attachment.display_filename
       }
-      default_params[:id] = info_request.id unless params[:public_token]
+      unless params[:public_token]
+        default_params[:request_url_title] = info_request.url_title
+      end
       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,
@@ -143,7 +117,7 @@ def show(params = {})
           allow(controller).to receive(:verifier).and_return(verifier)
           allow(verifier).to receive(:generate).with(
             get_attachment_path(
-              info_request.id,
+              info_request.url_title,
               incoming_message_id: attachment.incoming_message_id,
               part: attachment.url_part_number,
               file_name: attachment.filename
@@ -416,7 +390,9 @@ 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]
+      unless params[:public_token]
+        default_params[:request_url_title] = info_request.url_title
+      end
       get :show_as_html, params: default_params.merge(params)
     end
 
@@ -438,18 +414,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(
@@ -498,7 +462,7 @@ def show_as_html(params = {})
           allow(controller).to receive(:verifier).and_return(verifier)
           allow(verifier).to receive(:generate).with(
             get_attachment_as_html_path(
-              info_request.id,
+              info_request.url_title,
               incoming_message_id: attachment.incoming_message_id,
               part: attachment.url_part_number,
               file_name: attachment.filename

diff --git a/spec/helpers/info_request_helper_spec.rb b/spec/helpers/info_request_helper_spec.rb
@@ -595,7 +595,7 @@
     context 'when given no format options' do
       it 'returns the path to the attachment with a cookie cookie_passthrough param' do
         expect(attachment_path(jpeg_attachment)).to eq(
-          "/request/#{incoming_message.info_request_id}" \
+          "/request/#{incoming_message.info_request.url_title}" \
           "/response/#{incoming_message.id}/" \
           "attach/#{jpeg_attachment.url_part_number}" \
           "/interesting.jpg?cookie_passthrough=1"
@@ -606,7 +606,7 @@
     context 'when given an html format option' do
       it 'returns the path to the HTML version of the attachment' do
         expect(attachment_path(jpeg_attachment, html: true)).to eq(
-          "/request/#{incoming_message.info_request_id}" \
+          "/request/#{incoming_message.info_request.url_title}" \
           "/response/#{incoming_message.id}" \
           "/attach/html/#{jpeg_attachment.url_part_number}" \
           "/interesting.jpg.html"
@@ -637,7 +637,7 @@
       it 'returns the URL to the attachment with a cookie cookie_passthrough param' do
         expect(attachment_url(jpeg_attachment)).to eq(
           "http://test.host" \
-          "/request/#{incoming_message.info_request_id}" \
+          "/request/#{incoming_message.info_request.url_title}" \
           "/response/#{incoming_message.id}" \
           "/attach/#{jpeg_attachment.url_part_number}" \
           "/interesting.jpg?cookie_passthrough=1"
@@ -649,7 +649,7 @@
       it 'returns the URL to the HTML version of the attachment' do
         expect(attachment_url(jpeg_attachment, html: true)).to eq(
           "http://test.host" \
-          "/request/#{incoming_message.info_request_id}" \
+          "/request/#{incoming_message.info_request.url_title}" \
           "/response/#{incoming_message.id}" \
           "/attach/html/#{jpeg_attachment.url_part_number}" \
           "/interesting.jpg.html"

diff --git a/spec/integration/errors_spec.rb b/spec/integration/errors_spec.rb
@@ -90,6 +90,7 @@
     it 'should render a 403 with text body for attempts at directory listing for attachments' do
       info_request = FactoryBot.create(:info_request_with_pdf_attachment)
       id = info_request.id
+      url_title = info_request.url_title
       prefix = id.to_s[0..2]
       msg_id = info_request.incoming_messages.first.id
 
@@ -99,11 +100,11 @@
       )
       FileUtils.mkdir_p(cache_key_path)
 
-      get "/request/#{id}/response/#{msg_id}/attach/html/1/"
+      get "/request/#{url_title}/response/#{msg_id}/attach/html/1/"
       expect(response.body).to include('Directory listing not allowed')
       expect(response.code).to eq('403')
 
-      get "/request/#{id}/response/#{msg_id}/attach/html"
+      get "/request/#{url_title}/response/#{msg_id}/attach/html"
       expect(response.body).to include('Directory listing not allowed')
       expect(response.code).to eq('403')
     end

diff --git a/spec/integration/incoming_mail_spec.rb b/spec/integration/incoming_mail_spec.rb
@@ -23,14 +23,14 @@
                           email_to: info_request.incoming_email)
 
     attachment_1_path = get_attachment_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'hello world.txt',
       skip_cache: 1
     )
     attachment_2_path = get_attachment_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 3,
       file_name: 'hello world.txt',
@@ -65,7 +65,7 @@
     receive_incoming_mail('incoming-request-two-same-name.email',
                           email_to: info_request.incoming_email)
     attachment_path = get_attachment_as_html_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'hello world.txt.html',
@@ -83,7 +83,7 @@
     receive_incoming_mail('incoming-request-pdf-attachment.email',
                           email_to: info_request.incoming_email)
     attachment_path = get_attachment_as_html_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'fs 50379341.pdf.html',
@@ -103,7 +103,7 @@
     # asking for an attachment by the wrong filename should result in
     # redirecting back to the incoming message
     visit get_attachment_as_html_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'hello world.txt.baz.html',
@@ -118,7 +118,7 @@
                           email_to: info_request.incoming_email)
 
     attachment_path = get_attachment_path(
-      info_request.id,
+      info_request.url_title,
       incoming_message_id: info_request.incoming_messages.first.id,
       part: 2,
       file_name: 'hello.qwglhm',

diff --git a/spec/integration/prominence/viewing_attachment_as_html_spec.rb b/spec/integration/prominence/viewing_attachment_as_html_spec.rb
@@ -8,7 +8,7 @@
   let(:within_session) do
     -> {
       visit get_attachment_as_html_url(
-        info_request.id,
+        info_request.url_title,
         incoming_message_id: attachment.incoming_message_id,
         part: attachment.url_part_number,
         file_name: "#{attachment.display_filename}.html"

diff --git a/spec/integration/prominence/viewing_raw_attachment_spec.rb b/spec/integration/prominence/viewing_raw_attachment_spec.rb
@@ -10,7 +10,7 @@
       rebuild_raw_emails(info_request)
 
       visit get_attachment_url(
-        info_request.id,
+        info_request.url_title,
         incoming_message_id: attachment.incoming_message_id,
         part: attachment.url_part_number,
         file_name: attachment.display_filename

diff --git a/spec/integration/view_request_spec.rb b/spec/integration/view_request_spec.rb
@@ -51,7 +51,7 @@
       )
       rebuild_raw_emails(info_request)
 
-      attachment_url = "/es/request/#{info_request.id}/response/" \
+      attachment_url = "/es/request/#{info_request.url_title}/response/" \
         "#{incoming_message.id}/attach/#{attachment.url_part_number}/" \
         "#{attachment.filename}"
       using_session(non_owner) { visit(attachment_url) }