mysociety / alaveteli

Provide a Freedom of Information request system for your jurisdiction
https://alaveteli.org
Other
387 stars 195 forks source link

Check capitalisation doesn't impact which domains are permitted to respond to authority_only threads #7354

Closed RichardTaylor closed 1 year ago

RichardTaylor commented 1 year ago

We have a case with a thread set to authority_only where the request email was:

@dwp.gov.uk

and a response which it has been claimed was sent, but didn't get through, came from

@DWP.GOV.UK

The comparison check here should be case insensitive. I presume it is but I'm raising this ticket in-case it isn't.

Relates to WDTK request 805093

garethrees commented 1 year ago

I checked this and we downcase before checking.

I added this spec…

diff --git a/spec/models/info_request/response_gatekeeper/authority_only_spec.rb b/spec/models/info_request/response_gatekeeper/authority_only_spec.rb
index c2d0f068f..af7a3a625 100644
--- a/spec/models/info_request/response_gatekeeper/authority_only_spec.rb
+++ b/spec/models/info_request/response_gatekeeper/authority_only_spec.rb
@@ -108,6 +108,21 @@

     end

+    context 'if the email is from the authority in a different case' do
+      it 'allows the email' do
+        raw = <<-EOF.strip_heredoc
+        From: authority@EXAMPLE.COM
+        To: request-123@example.com
+        Subject: Basic Email
+        Hello, World
+        EOF
+        email = MailHandler.mail_from_raw_email(raw)
+        gatekeeper = described_class.new(FactoryBot.build(:info_request))
+
+        expect(gatekeeper.allow?(email)).to eq(true)
+      end
+    end
+
     context 'if the request already has a reply from a given domain' do

       it 'allows the email' do

…and added a pry binding…

diff --git a/app/models/info_request/response_gatekeeper/authority_only.rb b/app/models/info_request/response_gatekeeper/authority_only.rb
index 27d3b8e78..374250e29 100644
--- a/app/models/info_request/response_gatekeeper/authority_only.rb
+++ b/app/models/info_request/response_gatekeeper/authority_only.rb
@@ -9,6 +9,8 @@ def allow?(email)
       private

       def calculate_allow_reason(info_request, email)
+        binding.pry
+
         sender_email = MailHandler.get_from_address(email)
         if sender_email.nil?
           allow = false

…to step through manually:

From: /alaveteli/app/models/info_request/response_gatekeeper/authority_only.rb:12 InfoRequest::ResponseGatekeeper::AuthorityOnly#calculate_allow_reason:

    11: def calculate_allow_reason(info_request, email)
 => 12:   binding.pry
    13:
    14:   sender_email = MailHandler.get_from_address(email)
    15:   if sender_email.nil?
    16:     allow = false
    17:     reason = _('Only the authority can reply to this request, but there is no "From" address to check against')
    18:   else
    19:     sender_domain = PublicBody.extract_domain_from_email(sender_email)
    20:     reason = _("Only the authority can reply to this request, and I don't recognise the address this reply was sent from")
    21:     allow = false
    22:     # Allow any domain that has already sent reply
    23:     info_request.who_can_followup_to.each do |_, email_address, _|
    24:       request_domain = PublicBody.extract_domain_from_email(email_address)
    25:       if request_domain == sender_domain
    26:         allow = true
    27:         reason = nil
    28:       end
    29:     end
    30:   end
    31:
    32:   [allow, reason]
    33: end

[1] pry(#<InfoRequest::ResponseGatekeeper::AuthorityOnly>)> sender_email = MailHandler.get_from_address(email)
=> "authority@EXAMPLE.COM"
[2] pry(#<InfoRequest::ResponseGatekeeper::AuthorityOnly>)> sender_domain = PublicBody.extract_domain_from_email(sender_email)
=> "example.com"
[3] pry(#<InfoRequest::ResponseGatekeeper::AuthorityOnly>)> info_request.who_can_followup_to.size
=> 1

# …snipped some typos…

[6] pry(#<InfoRequest::ResponseGatekeeper::AuthorityOnly>)> follow_up_to = info_request.who_can_followup_to.first
=> ["Example Public Body 1", "request@example.com", nil]
[7] pry(#<InfoRequest::ResponseGatekeeper::AuthorityOnly>)> email_address = follow_up_to[1]
=> "request@example.com"
[8] pry(#<InfoRequest::ResponseGatekeeper::AuthorityOnly>)> request_domain = PublicBody.extract_domain_from_email(email_address)
=> "example.com"
[9] pry(#<InfoRequest::ResponseGatekeeper::AuthorityOnly>)> request_domain == sender_domain
=> true

As can be seen, the PublicBody.extract_domain_from_email(sender_email) returns a downcased domain, so we're comparing like for like.