Merge pull request #2325 from 18F/mryenq-adjust-invalid-sms-response

Adjust response code for SMS reply

app/controllers/sms_controller.rb

@@ -5,29 +5,21 @@ class SmsController < ApplicationController
   # Twilio supports HTTP Basic Auth for request URL
   # https://www.twilio.com/docs/usage/security
   before_action :authenticate
+  before_action :set_message, only: [:receive]
 
   # Disable CSRF check
   skip_before_action :verify_authenticity_token, only: [:receive]
 
   def receive
-    signature = request.headers[TwilioService::Sms::Request::SIGNATURE_HEADER]
-    message = TwilioService::Sms::Request.new(request.url, params, signature)
+    result = SmsForm.new(@message).submit
 
-    handle_result(message, SmsForm.new(message).submit)
+    result.success? ? process_success(result) : process_failure(result)
   end
 
   private
 
-  def handle_result(message, result)
-    if result.success?
-      process_success(message, result)
-    else
-      process_failure(result)
-    end
-  end
-
-  def process_success(message, result)
-    response = TwilioService::Sms::Response.new(message)
+  def process_success(result)
+    response = TwilioService::Sms::Response.new(@message)
     SmsReplySenderJob.perform_later(response.reply)
 
     analytics.track_event(
@@ -44,7 +36,11 @@ def process_failure(result)
       result.to_h
     )
 
-    head :forbidden
+    if !@message.signature_valid?
+      head :forbidden
+    else
+      head :ok
+    end
   end
 
   # `http_basic_authenticate_with name` had issues related to testing, so using
@@ -71,4 +67,10 @@ def auth_configured?(env)
     env.twilio_http_basic_auth_username.present? &&
       env.twilio_http_basic_auth_password.present?
   end
+
+  def set_message
+    signature = request.headers[TwilioService::Sms::Request::SIGNATURE_HEADER]
+
+    @message = TwilioService::Sms::Request.new(request.url, params, signature)
+  end
 end

app/services/twilio_service/sms/request.rb

@@ -32,10 +32,6 @@ def extra_analytics_attributes
         }
       end
 
-      private
-
-      attr_reader :url, :params, :signature
-
       # First, validate the message signature using Twilio's library:
       # https://github.com/twilio/twilio-ruby/wiki/Request-Validator
       def signature_valid?
@@ -51,6 +47,10 @@ def message_valid?
         # before processing the submission; however this is on-hold during the
         # initial phase.
       end
+
+      private
+
+      attr_reader :url, :params, :signature
     end
   end
 end

spec/requests/sms_spec.rb

@@ -7,6 +7,8 @@
   let(:password) { 'auth_password' }
   let(:access_denied) { 'HTTP Basic: Access denied' }
   let(:credentials) { "Basic #{Base64.encode64("#{username}:#{password}")}" }
+  let(:help_message) { 'help' }
+  let(:invalid_message) { 'blargh' }
 
   describe 'HTTP Basic Authentication' do
     context 'without required credentials' do
@@ -30,7 +32,7 @@
           receive(:validate).and_return(true)
         )
 
-        post_message
+        post_message(help_message)
 
         expect(response).to have_http_status(:accepted)
       end
@@ -48,17 +50,29 @@
     end
 
     context 'when failing' do
-      it 'does not send a reply' do
+      it 'does not send a reply and 403s when signature is invalid' do
         allow_any_instance_of(Twilio::Security::RequestValidator).to(
           receive(:validate).and_return(false)
         )
 
         expect(SmsReplySenderJob).to_not receive(:perform_later)
 
-        post_message
+        post_message(help_message)
 
         expect(response).to have_http_status(:forbidden)
       end
+
+      it 'responds with a 200 status when signature is valid' do
+        allow_any_instance_of(Twilio::Security::RequestValidator).to(
+          receive(:validate).and_return(true)
+        )
+
+        expect(SmsReplySenderJob).to_not receive(:perform_later)
+
+        post_message(invalid_message)
+
+        expect(response).to have_http_status(:ok)
+      end
     end
 
     context 'when successful' do
@@ -69,7 +83,7 @@
 
         expect(SmsReplySenderJob).to receive(:perform_later)
 
-        post_message
+        post_message(help_message)
 
         expect(response).to have_http_status(:accepted)
       end
@@ -78,10 +92,10 @@
 
   private
 
-  def post_message
+  def post_message(body)
     post(
       api_sms_receive_path,
-      params: { Body: 'help' },
+      params: { Body: body },
       headers: { 'HTTP_AUTHORIZATION': credentials }
     )
   end