Restore the download for hidden requests

This was disabled for hidden requests as the download was by redirect,
allowing people who have not been authenticated to conceivably access
the download. We'll be moving to send_file instead, so can restore it.

5 files changed, 62 insertions, 30 deletions

diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index 0180ad840..8b978cc01 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -868,10 +868,6 @@ class RequestController < ApplicationController
         @locale = self.locale_from_params()
         I18n.with_locale(@locale) do
             @info_request = InfoRequest.find_by_url_title!(params[:url_title])
-            # Test for whole request being hidden or requester-only
-            if !@info_request.all_can_view?
-                return render_hidden
-            end
             if authenticated?(
                               :web => _("To download the zip file"),
                               :email => _("Then you can download a zip file of {{info_request_title}}.",
@@ -879,6 +875,10 @@ class RequestController < ApplicationController
                               :email_subject => _("Log in to download a zip file of {{info_request_title}}",
                                            :info_request_title=>@info_request.title)
                               )
+                # Test for whole request being hidden or requester-only
+                if !@info_request.user_can_view?(@user)
+                    return render_hidden
+                end
                 @url_path = File.join("/download",
                                        request_dirs(@info_request),
                                        @info_request.last_update_hash,
diff --git a/app/views/request/_after_actions.html.erb b/app/views/request/_after_actions.html.erb
index b54a8f5fb..f780e3a37 100644
--- a/app/views/request/_after_actions.html.erb
+++ b/app/views/request/_after_actions.html.erb
@@ -15,11 +15,9 @@
                 <%= link_to _('Update the status of this request'), '#describe_state_form_1' %>
             </li>
         <% end %>
-      <% if @info_request.all_can_view? %>
       <li>
           <%= link_to _("Download a zip file of all correspondence"), download_entire_request_path(:url_title => @info_request.url_title) %>
       </li>
-      <% end %>
     </ul>
     </div>
     <% if ! @info_request.is_external? %>
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 8f8e3afa0..c5ee8cbf7 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -858,12 +858,6 @@ describe RequestController, "when handling prominence" do
              response.should render_template('show')
          end
 
-         it 'should not allow download of the entire request by admin user (or anyone)' do
-             session[:user_id] = FactoryGirl.create(:admin_user).id
-             get :download_entire_request, :url_title => @info_request.url_title
-             expect_hidden('hidden')
-         end
-
          it 'should not cache an attachment when showing an attachment to the requester or admin' do
              session[:user_id] = @info_request.user.id
              incoming_message = @info_request.incoming_messages.first
diff --git a/spec/integration/download_request_spec.rb b/spec/integration/download_request_spec.rb
index 8af1c9ff1..dd492e42a 100644
--- a/spec/integration/download_request_spec.rb
+++ b/spec/integration/download_request_spec.rb
@@ -56,7 +56,60 @@ describe 'when making a zipfile available' do
             end
         end
 
+        context 'when a request is "requester_only"' do
+
+            before do
+                @non_owner = login(FactoryGirl.create(:user))
+                @info_request = FactoryGirl.create(:info_request_with_incoming,
+                                                   :prominence => 'requester_only')
+                @request_owner = login(@info_request.user)
+                @admin = login(FactoryGirl.create(:admin_user))
+            end
+
+
+            it 'should allow a download of the request by the request owner and admin only' do
+                # Requester can access the zip
+                inspect_zip_download(@request_owner, @info_request) do |zip|
+                    zip.count.should == 1
+                    zip.read('correspondence.txt').should match('hereisthetext')
+                end
+                # Non-owner can't
+                @non_owner.get_via_redirect "request/#{@info_request.url_title}/download"
+                @non_owner.response.code.should == '410'
+                # Admin can
+                inspect_zip_download(@admin, @info_request) do |zip|
+                    zip.count.should == 1
+                    zip.read('correspondence.txt').should match('hereisthetext')
+                end
+            end
+        end
+
+        context 'when a request is made "hidden"' do
+
+            it 'should not allow a download of the request by an admin only' do
+                @non_owner = login(FactoryGirl.create(:user))
+                @info_request = FactoryGirl.create(:info_request_with_incoming,
+                                                   :prominence => 'hidden')
+                @request_owner = login(@info_request.user)
+                @admin = login(FactoryGirl.create(:admin_user))
+
+                # Requester can't access the zip
+                @request_owner.get_via_redirect "request/#{@info_request.url_title}/download"
+                @request_owner.response.code.should == '410'
+                # Non-owner can't
+                @non_owner.get_via_redirect "request/#{@info_request.url_title}/download"
+                @non_owner.response.code.should == '410'
+                # Admin can
+                inspect_zip_download(@admin, @info_request) do |zip|
+                    zip.count.should == 1
+                    zip.read('correspondence.txt').should match('hereisthetext')
+                end
+            end
+
+        end
+
         context 'when an incoming message is made "requester_only"' do
+
             it 'should not include the incoming message or attachments in a download of the entire request
                 by a non-request owner' do
 
@@ -90,7 +143,7 @@ describe 'when making a zipfile available' do
 
         end
 
-        it 'should successfully make a zipfile for an external request', :focus => true do
+        it 'should successfully make a zipfile for an external request' do
             external_request = FactoryGirl.create(:external_request)
             user = login(FactoryGirl.create(:user))
             inspect_zip_download(user, external_request){ |zip|  zip.count.should == 1 }
diff --git a/spec/views/request/_after_actions.html.erb_spec.rb b/spec/views/request/_after_actions.html.erb_spec.rb
index ae398f4ce..833323d68 100644
--- a/spec/views/request/_after_actions.html.erb_spec.rb
+++ b/spec/views/request/_after_actions.html.erb_spec.rb
@@ -69,24 +69,11 @@ describe 'when displaying actions that can be taken with regard to a request' do
         end
     end
 
-    describe 'if the request is viewable by all' do
 
-        it 'should display the link to download the entire request' do
-            render :partial => 'request/after_actions'
-            response.should have_selector('div#anyone_actions') do |div|
-                div.should have_selector('a', :content => 'Download a zip file of all correspondence')
-            end
-        end
-    end
-
-    describe 'if the request is not viewable by all' do
-
-        it 'should not  display the link to download the entire request' do
-            @mock_request.stub!(:all_can_view?).and_return(false)
-            render :partial => 'request/after_actions'
-            response.should have_selector('div#anyone_actions') do |div|
-                div.should_not have_selector('a', :content => 'Download a zip file of all correspondence')
-            end
+    it 'should display the link to download the entire request' do
+        render :partial => 'request/after_actions'
+        response.should have_selector('div#anyone_actions') do |div|
+            div.should have_selector('a', :content => 'Download a zip file of all correspondence')
         end
     end