Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust response code for SMS reply #2325

Merged
merged 2 commits into from
Jul 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions app/controllers/sms_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -44,7 +36,11 @@ def process_failure(result)
result.to_h
)

head :forbidden
if !@message.signature_valid?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of signature_valid?, we could do something like this:

if @message.unsupported_message_type?
  head :ok
else
  head :forbidden
end

...since what we really want is render a 200 when the message type is unsupported, and error in all other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the general approach, but that method feels a little contrived to serve this particular case. I'm trying to think of a good way to address the concern but with a better method. (I think the concept of 'valid' seems clearer and more standardized.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the important things is that we not swallow an error here. Currently, this would swallow an error for missing fields in the request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was misreading how this works in the message class. This looks like it works.

head :forbidden
else
head :ok
end
end

# `http_basic_authenticate_with name` had issues related to testing, so using
Expand All @@ -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
8 changes: 4 additions & 4 deletions app/services/twilio_service/sms/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand All @@ -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
26 changes: 20 additions & 6 deletions spec/requests/sms_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -30,7 +32,7 @@
receive(:validate).and_return(true)
)

post_message
post_message(help_message)

expect(response).to have_http_status(:accepted)
end
Expand All @@ -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
Expand All @@ -69,7 +83,7 @@

expect(SmsReplySenderJob).to receive(:perform_later)

post_message
post_message(help_message)

expect(response).to have_http_status(:accepted)
end
Expand All @@ -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
Expand Down