Skip to content

Commit

Permalink
Encrypt voice OTP code
Browse files Browse the repository at this point in the history
**Why**: URL params and GET params are in our logs,
so this keeps plaintext codes out of our logs
  • Loading branch information
zachmargolis committed Oct 17, 2017
1 parent 4583bd1 commit 395f69f
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 19 deletions.
14 changes: 12 additions & 2 deletions app/controllers/voice/otp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,14 @@ def show

protected

def encrypted_code
params[:encrypted_code].to_s
end

def code
params[:code].to_s
return unless encrypted_code.present?

cipher.decrypt(encrypted_code)
end

def message
Expand All @@ -37,10 +43,14 @@ def action_url

BasicAuthUrl.build(
voice_otp_url(
code: code,
encrypted_code: encrypted_code,
repeat_count: repeat_count - 1
)
)
end

def cipher
Gibberish::AES.new(Figaro.env.attribute_encryption_key)
end
end
end
11 changes: 10 additions & 1 deletion app/jobs/voice_otp_sender_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,17 @@ def otp_valid?(otp_created_at)
def send_otp(twilio_service, code, phone)
twilio_service.place_call(
to: phone,
url: BasicAuthUrl.build(voice_otp_url(code: code, locale: locale_url_param)),
url: BasicAuthUrl.build(
voice_otp_url(
encrypted_code: cipher.encrypt(code),
locale: locale_url_param
)
),
record: Figaro.env.twilio_record_voice == 'true'
)
end

def cipher
Gibberish::AES.new(Figaro.env.attribute_encryption_key)
end
end
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
match '/api/saml/auth' => 'saml_idp#auth', via: %i[get post]

post '/api/service_provider' => 'service_provider#update'
match '/api/voice/otp/:code' => 'voice/otp#show',
match '/api/voice/otp' => 'voice/otp#show',
via: [:get, :post],
as: :voice_otp,
defaults: { format: :xml }
Expand Down
31 changes: 17 additions & 14 deletions spec/controllers/voice/otp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,15 @@
describe '#show' do
subject(:action) do
get :show,
params: { code: code, repeat_count: repeat_count, locale: locale },
params: { encrypted_code: encrypted_code, repeat_count: repeat_count, locale: locale },
format: :xml
end
let(:code) { nil }
let(:locale) { nil }
let(:repeat_count) { nil }
let(:cipher) { Gibberish::AES.new(Figaro.env.attribute_encryption_key) }

context 'without a code in the URL' do
let(:code) { nil }

it 'cannot route to the controller' do
expect { action }.to raise_error(ActionController::UrlGenerationError)
end
end

context 'with a blank code in the URL' do
let(:code) { '' }
context 'with a blank encrypted_code in the URL' do
let(:encrypted_code) { '' }

it 'renders a blank 400' do
action
Expand All @@ -30,10 +22,11 @@
end
end

context 'with a code in the URL' do
context 'with an encrypted_code in the URL' do
render_views

let(:code) { 1234 }
let(:code) { '1234' }
let(:encrypted_code) { cipher.encrypt(code) }

it 'tells Twilio to <Say> the code with pauses in between' do
action
Expand Down Expand Up @@ -107,6 +100,16 @@
expect(gather[:action]).to include('repeat_count=4')
end

it 'puts the encrypted code in the <Gather> action' do
action

doc = Nokogiri::XML(response.body)
gather = doc.css('Gather').first
params = URIService.params(gather[:action])

expect(cipher.decrypt(params[:encrypted_code])).to eq(code)
end

context 'when repeat_count counts down to 1' do
let(:repeat_count) { 1 }

Expand Down
4 changes: 3 additions & 1 deletion spec/jobs/voice_otp_sender_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
FakeVoiceCall.calls = []
end

let(:cipher) { Gibberish::AES.new(Figaro.env.attribute_encryption_key) }

it 'initiates the phone call to deliver the OTP', twilio: true do
I18n.with_locale(:fr) do
VoiceOtpSenderJob.perform_now(
Expand All @@ -24,8 +26,8 @@
expect(call.to).to eq('555-5555')
expect(call.from).to match(/(\+19999999999|\+12222222222)/)

expect(call.url).to include('1234')
params = URIService.params(call.url)
expect(cipher.decrypt(params[:encrypted_code])).to eq('1234')
expect(params[:locale]).to eq('fr')
end

Expand Down

0 comments on commit 395f69f

Please sign in to comment.