Use caching that allows pre-authentication.

Add test code for hidden requests.
Make super users able to view hidden requests.

9 files changed, 72 insertions, 51 deletions

diff --git a/app/controllers/admin_censor_rule_controller.rb b/app/controllers/admin_censor_rule_controller.rb
index b51c1e01e..28130b6ee 100644
--- a/app/controllers/admin_censor_rule_controller.rb
+++ b/app/controllers/admin_censor_rule_controller.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2008 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: admin_censor_rule_controller.rb,v 1.6 2009-06-23 13:52:25 francis Exp $
+# $Id: admin_censor_rule_controller.rb,v 1.7 2009-06-30 14:28:25 francis Exp $
 
 class AdminCensorRuleController < AdminController
     def new
@@ -14,7 +14,6 @@ class AdminCensorRuleController < AdminController
     def create
         params[:censor_rule][:last_edit_editor] = admin_http_auth_user()
         @censor_rule = CensorRule.new(params[:censor_rule])
-        expire_for_request(@censor_rule.info_request)
         if @censor_rule.save
             expire_for_request(@censor_rule.info_request)
             flash[:notice] = 'CensorRule was successfully created.'
@@ -31,8 +30,6 @@ class AdminCensorRuleController < AdminController
     def update
         params[:censor_rule][:last_edit_editor] = admin_http_auth_user()
         @censor_rule = CensorRule.find(params[:id])
-        # expire before and after change, in case filename changes
-        expire_for_request(@censor_rule.info_request)
         if @censor_rule.update_attributes(params[:censor_rule])
             expire_for_request(@censor_rule.info_request)
             flash[:notice] = 'CensorRule was successfully updated.'
@@ -46,13 +43,10 @@ class AdminCensorRuleController < AdminController
         censor_rule = CensorRule.find(params[:censor_rule_id])
         info_request = censor_rule.info_request
 
-        # expire before and after change, in case filename changes
-        expire_for_request(info_request)
         censor_rule.destroy
         expire_for_request(info_request)
 
         flash[:notice] = "CensorRule was successfully destroyed."
-
         redirect_to admin_url('request/show/' + info_request.id.to_s)
     end
 
diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb
index 5810b4b04..9ca891bf2 100644
--- a/app/controllers/admin_controller.rb
+++ b/app/controllers/admin_controller.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2009 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: admin_controller.rb,v 1.25 2009-06-23 13:52:25 francis Exp $
+# $Id: admin_controller.rb,v 1.26 2009-06-30 14:28:25 francis Exp $
 
 
 class AdminController < ApplicationController
@@ -18,22 +18,20 @@ class AdminController < ApplicationController
 
     # Expire cached attachment files for a request
     def expire_for_request(info_request)
-        # So is using latest censor rules
-        info_request.reload
-
-        # clear out cached entries
-        for incoming_message in info_request.incoming_messages
-            for attachment in incoming_message.get_attachments_for_display
-                expire_page :controller => 'request', :action => "get_attachment", :id => info_request.id,
-                    :incoming_message_id => incoming_message.id, 
-                    :part => attachment.url_part_number, :file_name => attachment.display_filename
-                expire_page :controller => 'request', :action => "get_attachment_as_html", :id => info_request.id,
-                    :incoming_message_id => incoming_message.id, 
-                    :part => attachment.url_part_number, :file_name => attachment.display_filename
-            end
-        end
+        # Clear out cached entries - use low level expire_fragment, even though
+        # we are clearing results from caches_action, for several reasons:
+        # * We can't use expire_action here, as doesn't  seem to be
+        # compatible with the :only_path we used in the caches_action
+        # call. 
+        # * expire_fragment lets us use a regular expression which is
+        # simpler than having to get all the parameters right for the
+        # path, and calling for HTML version vs. raw attachment version.
+        # * Regular expression means we cope properly with filenames
+        # changed by censor rules, which change the URL.
+        # * It's also possible to load a file with any name by changing
+        # the URL, the regular expression makes sure the cache is
+        # cleared even if someone did that.
+        expire_fragment /views\/request\/#{info_request.id}.*/
     end
-
-
 end
 
diff --git a/app/controllers/admin_request_controller.rb b/app/controllers/admin_request_controller.rb
index fe7b6e8a6..df22f1341 100644
--- a/app/controllers/admin_request_controller.rb
+++ b/app/controllers/admin_request_controller.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: admin_request_controller.rb,v 1.36 2009-06-23 13:52:25 francis Exp $
+# $Id: admin_request_controller.rb,v 1.37 2009-06-30 14:28:25 francis Exp $
 
 class AdminRequestController < AdminController
     def index
@@ -48,12 +48,6 @@ class AdminRequestController < AdminController
         old_allow_new_responses_from = @info_request.allow_new_responses_from
         old_handle_rejected_responses = @info_request.handle_rejected_responses
 
-        expire = false
-        if @info_request.prominence != params[:info_request][:prominence]
-            # in case it has become hidden, clear cache after saving
-            expire = true
-        end
-
         @info_request.title = params[:info_request][:title]
         @info_request.prominence = params[:info_request][:prominence]
         if @info_request.described_state != params[:info_request][:described_state]
@@ -65,9 +59,6 @@ class AdminRequestController < AdminController
 
         if @info_request.valid?
             @info_request.save!
-            if expire
-                expire_for_request(@info_request)
-            end
             @info_request.log_event("edit", 
                 { :editor => admin_http_auth_user(), 
                     :old_title => old_title, :title => @info_request.title, 
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index 7579ee63e..435c73724 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: request_controller.rb,v 1.163 2009-06-23 13:52:25 francis Exp $
+# $Id: request_controller.rb,v 1.164 2009-06-30 14:28:25 francis Exp $
 
 class RequestController < ApplicationController
     
@@ -497,7 +497,20 @@ class RequestController < ApplicationController
     end
 
     # Download an attachment
-    caches_page :get_attachment
+
+    before_filter :authenticate_attachment, :only => [ :get_attachment, :get_attachment_as_html ]
+    def authenticate_attachment
+        # Test for hidden
+        incoming_message = IncomingMessage.find(params[:incoming_message_id])
+        if !incoming_message.info_request.user_can_view?(authenticated_user)
+            render :template => 'request/hidden'
+        end
+    end
+
+    # use :cache_path here so domain doesn't appear in filename, as /admin
+    # interface runs on separate domain (so can use HTTPS without its own
+    # certificate) in mySociety server configuration.
+    caches_action :get_attachment, :cache_path => { :only_path => true }
     def get_attachment
         if !get_attachment_internal
             return
@@ -513,7 +526,7 @@ class RequestController < ApplicationController
         render :text => @attachment.body
     end
 
-    caches_page :get_attachment_as_html
+    caches_action :get_attachment_as_html, :cache_path => { :only_path => true }
     def get_attachment_as_html
         if !get_attachment_internal
             return
@@ -545,11 +558,8 @@ class RequestController < ApplicationController
         @part_number = params[:part].to_i
         @filename = params[:file_name]
        
-        # Test for hidden
-        if !@info_request.user_can_view?(authenticated_user)
-            render :template => 'request/hidden'
-            return false
-        end
+        # check permissions
+        raise "internal error, pre-auth filter should have caught this" if !@info_request.user_can_view?(authenticated_user)
   
         @attachment = IncomingMessage.get_attachment_by_url_part_number(@incoming_message.get_attachments_for_display, @part_number)
 
diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index 9d14e70d2..aa9b49c83 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -24,7 +24,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: info_request.rb,v 1.196 2009-06-27 02:25:31 francis Exp $
+# $Id: info_request.rb,v 1.197 2009-06-30 14:28:26 francis Exp $
 
 require 'digest/sha1'
 require File.join(File.dirname(__FILE__),'../../vendor/plugins/acts_as_xapian/lib/acts_as_xapian')
@@ -819,10 +819,10 @@ public
     end
 
     def user_can_view?(user)
-        return self.prominence != 'hidden'
-        # || self.is_owning_user?(user) # XXX this doesn't work, as have to
-        # mess with caching of HTML versions - need to change from using
-        # caches_pages in the request controller first.
+        if self.prominence == 'hidden' 
+            return User.view_hidden_requests?(user)
+        end
+        return true
     end
 
     # This is called from cron regularly.
diff --git a/app/models/user.rb b/app/models/user.rb
index 13b2c58d0..03a076c24 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -23,7 +23,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: user.rb,v 1.95 2009-06-26 14:28:38 francis Exp $
+# $Id: user.rb,v 1.96 2009-06-30 14:28:26 francis Exp $
 
 require 'digest/sha1'
 
@@ -219,7 +219,11 @@ class User < ActiveRecord::Base
     def self.owns_every_request?(user)
       !user.nil? && user.owns_every_request?  
     end
-    
+
+    def self.view_hidden_requests?(user)
+      !user.nil? && user.admin_level == 'super'
+    end
+     
     # Does the user get "(admin)" links on each page on the main site?
     def admin_page_links?
         self.admin_level == 'super'
diff --git a/app/views/request/hidden.rhtml b/app/views/request/hidden.rhtml
index e32e7fa12..781bc9184 100644
--- a/app/views/request/hidden.rhtml
+++ b/app/views/request/hidden.rhtml
@@ -7,7 +7,7 @@
 </p>
 
 <p>The request you have tried to view has been removed. There are
-lots of reasons why we might have done this, sorry we can't
+various reasons why we might have done this, sorry we can't
 be more specific here. Please <a
     href="/help/contact">contact us</a> if you have any questions.
 </p>
diff --git a/public/.cvsignore b/public/.cvsignore
index f1e08352c..2a51cd65f 100644
--- a/public/.cvsignore
+++ b/public/.cvsignore
@@ -1,4 +1,3 @@
 down.html
 foi-live-creation.png
 foi-user-use.png
-request
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 57d092afb..13117506c 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -49,6 +49,15 @@ describe RequestController, "when showing one request" do
         get :show, :url_title => info_requests(:naughty_chicken_request).id
         response.should redirect_to(:action => 'show', :url_title => info_requests(:naughty_chicken_request).url_title)
     end
+
+    it "should not show hidden requests" do
+        ir = info_requests(:fancy_dog_request)
+        ir.prominence = 'hidden'
+        ir.save!
+
+        get :show, :url_title => 'why_do_you_have_such_a_fancy_dog'
+        response.should render_template('hidden')
+    end
     
     describe 'when handling an update_status parameter' do
         
@@ -108,6 +117,22 @@ describe RequestController, "when showing one request" do
             response.content_type.should == "text/plain"
             response.should have_text(/First hello/)        
         end
+
+        it "should not download attachments if hidden" do
+            ir = info_requests(:fancy_dog_request) 
+            ir.prominence = 'hidden'
+            ir.save!
+            receive_incoming_mail('incoming-request-two-same-name.email', ir.incoming_email)
+
+            get :get_attachment, :incoming_message_id => ir.incoming_messages[1].id, :id => ir.id, :part => 2
+            response.content_type.should == "text/html"
+            response.should_not have_text(/Second hello/)        
+            response.should render_template('request/hidden')
+            get :get_attachment, :incoming_message_id => ir.incoming_messages[1].id, :id => ir.id, :part => 3
+            response.content_type.should == "text/html"
+            response.should_not have_text(/First hello/)        
+            response.should render_template('request/hidden')
+        end
     end
 end