diff options
| -rw-r--r-- | app/controllers/admin_request_controller.rb | 10 | ||||
| -rw-r--r-- | app/models/info_request.rb | 37 | ||||
| -rw-r--r-- | db/migrate/101_add_hash_to_info_request.rb | 22 | ||||
| -rw-r--r-- | spec/controllers/admin_request_controller_spec.rb | 11 | ||||
| -rw-r--r-- | spec/fixtures/info_requests.yml | 6 | ||||
| -rw-r--r-- | spec/models/info_request_spec.rb | 34 |
6 files changed, 98 insertions, 22 deletions
diff --git a/app/controllers/admin_request_controller.rb b/app/controllers/admin_request_controller.rb index a617b06df..2b471400b 100644 --- a/app/controllers/admin_request_controller.rb +++ b/app/controllers/admin_request_controller.rb @@ -293,15 +293,7 @@ class AdminRequestController < AdminController end # 2. Match the email address in the message without matching the hash - @info_requests = [] - addresses = - (@raw_email.incoming_message.mail.to || []) + - (@raw_email.incoming_message.mail.cc || []) + - (@raw_email.incoming_message.mail.envelope_to || []) - addresses.uniq! - for address in addresses - @info_requests += InfoRequest.guess_by_incoming_email(address) - end + @info_requests = InfoRequest.guess_by_incoming_email(@raw_email.incoming_message) # 3. Give a reason why it's in the holding pen last_event = @raw_email.incoming_message.info_request.get_last_event diff --git a/app/models/info_request.rb b/app/models/info_request.rb index 158e6319e..419546c99 100644 --- a/app/models/info_request.rb +++ b/app/models/info_request.rb @@ -17,7 +17,6 @@ # allow_new_responses_from :string(255) default("anybody"), not null # handle_rejected_responses :string(255) default("bounce"), not null # - # models/info_request.rb: # A Freedom of Information request. # @@ -309,13 +308,20 @@ public # Return list of info requests which *might* be right given email address # e.g. For the id-hash email addresses, don't match the hash. - def InfoRequest.guess_by_incoming_email(incoming_email) - id, hash = InfoRequest._extract_id_hash_from_email(incoming_email) - begin - return [InfoRequest.find(id)] - rescue ActiveRecord::RecordNotFound - return [] + def InfoRequest.guess_by_incoming_email(incoming_message) + guesses = [] + # 1. Try to guess based on the email address(es) + addresses = + (incoming_message.mail.to || []) + + (incoming_message.mail.cc || []) + + (incoming_message.mail.envelope_to || []) + addresses.uniq! + for address in addresses + id, hash = InfoRequest._extract_id_hash_from_email(address) + guesses.push(InfoRequest.find_by_id(id)) + guesses.push(InfoRequest.find_by_idhash(hash)) end + return guesses.select{|x| !x.nil?}.uniq end # Internal function used by find_by_magic_email and guess_by_incoming_email @@ -326,7 +332,7 @@ public # The optional bounce- dates from when we used to have separate emails for the envelope from. # (that was abandoned because councils would send hand written responses to them, not just # bounce messages) - incoming_email =~ /request-(?:bounce-)?(\d+)-([a-z0-9]+)/ + incoming_email =~ /request-(?:bounce-)?([a-z0-9]+)-([a-z0-9]+)/ id = $1.to_i hash = $2 @@ -678,6 +684,7 @@ public end end return nil + end def get_last_response_event for e in self.info_request_events.reverse @@ -838,15 +845,25 @@ public def InfoRequest.magic_email_for_id(prefix_part, id) magic_email = MySociety::Config.get("INCOMING_EMAIL_PREFIX", "") magic_email += prefix_part + id.to_s - magic_email += "-" + Digest::SHA1.hexdigest(id.to_s + MySociety::Config.get("INCOMING_EMAIL_SECRET", 'dummysecret'))[0,8] + magic_email += "-" + InfoRequest.hash_from_id(id) magic_email += "@" + MySociety::Config.get("INCOMING_EMAIL_DOMAIN", "localhost") return magic_email end + before_validation :compute_idhash + + def compute_idhash + self.idhash = InfoRequest.hash_from_id(self.id) + end + + def InfoRequest.hash_from_id(id) + return Digest::SHA1.hexdigest(id.to_s + MySociety::Config.get("INCOMING_EMAIL_SECRET", 'dummysecret'))[0,8] + end + # Called by find_by_incoming_email - and used to be called by separate # function for envelope from address, until we abandoned it. def InfoRequest.find_by_magic_email(id, hash) - expected_hash = Digest::SHA1.hexdigest(id.to_s + MySociety::Config.get("INCOMING_EMAIL_SECRET", 'dummysecret'))[0,8] + expected_hash = InfoRequest.hash_from_id(id) #print "expected: " + expected_hash + "\nhash: " + hash + "\n" if hash != expected_hash return nil diff --git a/db/migrate/101_add_hash_to_info_request.rb b/db/migrate/101_add_hash_to_info_request.rb new file mode 100644 index 000000000..20608aac1 --- /dev/null +++ b/db/migrate/101_add_hash_to_info_request.rb @@ -0,0 +1,22 @@ +require 'digest/sha1' + +class AddHashToInfoRequest < ActiveRecord::Migration + def self.up + add_column :info_requests, :idhash, :string + + # Create the missing events for requests already sent + InfoRequest.find(:all).each do |info_request| + info_request.idhash = Digest::SHA1.hexdigest(info_request.id.to_s + MySociety::Config.get("INCOMING_EMAIL_SECRET", 'dummysecret'))[0,8] + info_request.save! + puts info_request.idhash + end + change_column :info_requests, :idhash, :string, :null => false + puts InfoRequest.find_by_idhash + end + def self.down + remove_column :info_requests, :idhash + end +end + + + diff --git a/spec/controllers/admin_request_controller_spec.rb b/spec/controllers/admin_request_controller_spec.rb index 0b12fa97a..04e412e76 100644 --- a/spec/controllers/admin_request_controller_spec.rb +++ b/spec/controllers/admin_request_controller_spec.rb @@ -57,4 +57,15 @@ describe AdminRequestController, "when administering the holding pen" do response.should have_text(/Only the authority can reply to this request/) end + it "guesses a misdirected request" do + ir = info_requests(:fancy_dog_request) + ir.handle_rejected_responses = 'holding_pen' + ir.save! + mail_to = "request-#{ir.id}-asdfg@example.com" + receive_incoming_mail('incoming-request-plain.email', mail_to) + get :show_raw_email, :id => InfoRequest.holding_pen_request.get_last_response.raw_email.id + response.should have_text(/Could not identify the request/) + assigns[:info_requests][0].should == ir + end + end diff --git a/spec/fixtures/info_requests.yml b/spec/fixtures/info_requests.yml index bdeef4800..c1e3c1910 100644 --- a/spec/fixtures/info_requests.yml +++ b/spec/fixtures/info_requests.yml @@ -8,6 +8,7 @@ fancy_dog_request: user_id: 1 described_state: waiting_response awaiting_description: true + idhash: 50929748 naughty_chicken_request: id: 103 title: How much public money is wasted on breeding naughty chickens? @@ -17,6 +18,5 @@ naughty_chicken_request: public_body_id: 2 user_id: 1 described_state: waiting_response - awaiting_description: false - - + awaiting_description: false + idhash: e8d18c84 diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb index d0b0e0e32..ba80256ab 100644 --- a/spec/models/info_request_spec.rb +++ b/spec/models/info_request_spec.rb @@ -2,6 +2,40 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') describe InfoRequest do + describe "guessing a request from an email" do + fixtures :info_requests, :public_bodies, :incoming_messages, :raw_emails + + before do + @im = incoming_messages(:useless_incoming_message) + load_raw_emails_data(raw_emails) + end + + it 'should compute a hash' do + @info_request = InfoRequest.new(:title => "testing", + :public_body => public_bodies(:geraldine_public_body), + :user_id => 1) + @info_request.save! + @info_request.idhash.should_not == nil + end + + it 'should find a request based on an email with an intact id and a broken hash' do + ir = info_requests(:fancy_dog_request) + id = ir.id + @im.mail.to = "request-#{id}-asdfg@example.com" + guessed = InfoRequest.guess_by_incoming_email(@im) + guessed[0].idhash.should == ir.idhash + end + + it 'should find a request based on an email with a broken id and an intact hash' do + ir = info_requests(:fancy_dog_request) + idhash = ir.idhash + @im.mail.to = "request-123ab-#{idhash}@example.com" + guessed = InfoRequest.guess_by_incoming_email(@im) + guessed[0].id.should == ir.id + end + + end + describe "making up the URL title" do before do @info_request = InfoRequest.new |