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

Allow multiple USPS confirmation codes #1661

Merged
merged 1 commit into from
Sep 8, 2017
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
12 changes: 2 additions & 10 deletions app/controllers/users/verify_account_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def index
@verify_account_form = VerifyAccountForm.new(user: current_user)

return unless FeatureManagement.reveal_usps_code?
@code = JSON.parse(user_session[:decrypted_pii])['otp']['raw']
@code = session[:last_usps_confirmation_code]
Copy link
Member Author

Choose a reason for hiding this comment

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

I kept OTP prefilling by sticking the OTP in the session. It's a little bit brittle, so I also considered displaying it in flash message on the come back later screen. No problem making it work that way if we think that's better.

end

def create
Expand All @@ -28,8 +28,7 @@ def create
def build_verify_account_form
VerifyAccountForm.new(
user: current_user,
otp: params_otp,
pii_attributes: decrypted_pii
otp: params_otp
)
end

Expand All @@ -41,12 +40,5 @@ def confirm_verification_needed
return if current_user.decorate.pending_profile_requires_verification?
redirect_to account_url
end

def decrypted_pii
@_decrypted_pii ||= begin
cacher = Pii::Cacher.new(current_user, user_session)
cacher.fetch
end
end
end
end
11 changes: 3 additions & 8 deletions app/controllers/verify/come_back_later_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,14 @@ module Verify
class ComeBackLaterController < ApplicationController
include IdvSession

before_action :confirm_idv_session_completed
before_action :confirm_usps_verification_method_chosen
before_action :confirm_user_needs_usps_confirmation

def show; end

private

def confirm_idv_session_completed
redirect_to account_path if idv_session.profile.blank?
end

def confirm_usps_verification_method_chosen
redirect_to account_path unless idv_session.address_verification_mechanism == 'usps'
def confirm_user_needs_usps_confirmation
redirect_to account_path unless current_user.decorate.needs_profile_usps_verification?
end
end
end
3 changes: 3 additions & 0 deletions app/controllers/verify/review_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ def create
init_profile
redirect_to verify_confirmations_path
analytics.track_event(Analytics::IDV_REVIEW_COMPLETE)

return unless FeatureManagement.reveal_usps_code?
session[:last_usps_confirmation_code] = idv_session.usps_otp
end

private
Expand Down
15 changes: 14 additions & 1 deletion app/controllers/verify/usps_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ def create
idv_session.address_verification_mechanism = :usps

if current_user.decorate.needs_profile_usps_verification?
redirect_to account_path
resend_letter
redirect_to verify_come_back_later_url
else
redirect_to verify_review_url
end
Expand All @@ -29,5 +30,17 @@ def confirm_mail_not_spammed
redirect_to verify_review_path if idv_session.address_mechanism_chosen? &&
usps_mail_service.mail_spammed?
end

def resend_letter
confirmation_maker = UspsConfirmationMaker.new(
pii: Pii::Cacher.new(current_user, user_session).fetch,
issuer: sp_session[:issuer],
profile: current_user.decorate.pending_profile
)
confirmation_maker.perform

return unless FeatureManagement.reveal_usps_code?
session[:last_usps_confirmation_code] = confirmation_maker.otp
Copy link
Contributor

Choose a reason for hiding this comment

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

CodeClimate is reporting this as not tested, do you think we should add test coverage? Since we expect to use this in our lower envs & development?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍. I'll write up a test

Copy link
Member Author

Choose a reason for hiding this comment

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

@zachmargolis: I actually found a bug writing the test (yay!) and I had to do some wacky things in the idv review controller to surface the OTP and put in the session. Mind taking another look to lmk what you think?

end
end
end
15 changes: 9 additions & 6 deletions app/forms/verify_account_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@ class VerifyAccountForm
attr_accessor :otp, :pii_attributes
attr_reader :user

def initialize(user:, otp: nil, pii_attributes: nil)
def initialize(user:, otp: nil)
@user = user
@otp = otp
@pii_attributes = pii_attributes
end

def submit
Expand All @@ -31,8 +30,14 @@ def pending_profile
@_pending_profile ||= user.decorate.pending_profile
end

def usps_confirmation_code
return if otp.blank? || pending_profile.blank?

pending_profile.usps_confirmation_codes.first_with_otp(otp)
end

def validate_otp_not_expired
return unless Idv::UspsMail.new(user).most_recent_otp_expired?
return unless usps_confirmation_code.present? && usps_confirmation_code.expired?

errors.add :otp, :usps_otp_expired
end
Expand All @@ -47,9 +52,7 @@ def validate_otp
end

def valid_otp?
otp.present? && ActiveSupport::SecurityUtils.secure_compare(
Base32::Crockford.normalize(otp), Base32::Crockford.normalize(pii_attributes.otp.to_s)
)
otp.present? && usps_confirmation_code.present?
end

def reset_sensitive_fields
Expand Down
1 change: 1 addition & 0 deletions app/models/profile.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class Profile < ApplicationRecord
belongs_to :user
has_many :usps_confirmation_codes

validates :active, uniqueness: { scope: :user_id, if: :active? }
validates :ssn_signature, uniqueness: { scope: :active, if: :active? }
Expand Down
16 changes: 16 additions & 0 deletions app/models/usps_confirmation_code.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class UspsConfirmationCode < ApplicationRecord
Copy link
Member Author

Choose a reason for hiding this comment

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

I am definitely not in love with this name. It is a bit confusing since we have a UspsConfirmation model. Happy to change it if someone has a better idea.

belongs_to :profile

def self.first_with_otp(otp)
find do |usps_confirmation_code|
Pii::Fingerprinter.verify(
Base32::Crockford.normalize(otp),
usps_confirmation_code.otp_fingerprint
)
end
end

def expired?
code_sent_at < Figaro.env.usps_confirmation_max_days.to_i.days.ago
end
end
8 changes: 1 addition & 7 deletions app/services/idv/profile_maker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,9 @@ def pii_from_applicant(appl, norm_appl)
zipcode: Pii::Attribute.new(raw: appl.zipcode, norm: norm_appl.zipcode),
dob: Pii::Attribute.new(raw: appl.dob, norm: norm_appl.dob),
ssn: Pii::Attribute.new(raw: appl.ssn, norm: norm_appl.ssn),
phone: Pii::Attribute.new(raw: appl.phone, norm: norm_appl.phone),
otp: generate_otp
phone: Pii::Attribute.new(raw: appl.phone, norm: norm_appl.phone)
)
end
# rubocop:enable MethodLength, AbcSize

def generate_otp(length: 10)
# Crockford encoding is 5 bits per character
Base32::Crockford.encode(SecureRandom.random_number(2**(5 * length)), length: length)
end
end
end
6 changes: 4 additions & 2 deletions app/services/idv/session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class Session
vendor_session_id
].freeze

attr_reader :current_user
attr_reader :current_user, :usps_otp

def initialize(user_session:, current_user:, issuer:)
@user_session = user_session
Expand Down Expand Up @@ -91,8 +91,10 @@ def create_usps_entry
if pii.is_a?(String)
self.pii = Pii::Attributes.new_from_json(user_session[:decrypted_pii])
end
confirmation_maker = UspsConfirmationMaker.new(pii: pii, issuer: issuer, profile: profile)
confirmation_maker.perform

UspsConfirmationMaker.new(pii: pii, issuer: issuer).perform
@usps_otp = confirmation_maker.otp
end

def alive?
Expand Down
7 changes: 0 additions & 7 deletions app/services/idv/usps_mail.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ module Idv
class UspsMail
MAX_MAIL_EVENTS = Figaro.env.max_mail_events.to_i
MAIL_EVENTS_WINDOW_DAYS = Figaro.env.max_mail_events_window_in_days.to_i
USPS_CONFIRMATION_WINDOW_DAYS = Figaro.env.usps_confirmation_max_days.to_i

def initialize(current_user)
@current_user = current_user
Expand All @@ -17,12 +16,6 @@ def any_mail_sent?
user_mail_events.any?
end

def most_recent_otp_expired?
return false unless any_mail_sent?

user_mail_events.first.updated_at < USPS_CONFIRMATION_WINDOW_DAYS.days.ago
end

private

attr_reader :current_user
Expand Down
2 changes: 1 addition & 1 deletion app/services/pii/attributes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ module Pii
Attributes = Struct.new(
:first_name, :middle_name, :last_name,
:address1, :address2, :city, :state, :zipcode,
:ssn, :dob, :phone, :otp,
:ssn, :dob, :phone,
:prev_address1, :prev_address2, :prev_city, :prev_state, :prev_zipcode
) do
def self.new_from_hash(hash)
Expand Down
20 changes: 17 additions & 3 deletions app/services/usps_confirmation_maker.rb
Original file line number Diff line number Diff line change
@@ -1,17 +1,26 @@
class UspsConfirmationMaker
def initialize(pii:, issuer:)
def initialize(pii:, issuer:, profile:)
@pii = pii
@issuer = issuer
@profile = profile
end

def otp
@otp ||= generate_otp
end

def perform
entry = UspsConfirmationEntry.new_from_hash(attributes)
UspsConfirmation.create!(entry: entry.encrypted)
UspsConfirmationCode.create!(
profile: profile,
otp_fingerprint: Pii::Fingerprinter.fingerprint(otp)
)
end

private

attr_reader :pii, :issuer
attr_reader :pii, :issuer, :profile

# rubocop:disable AbcSize, MethodLength
# This method is single statement spread across many lines for readability
Expand All @@ -20,7 +29,7 @@ def attributes
address1: pii[:address1].norm,
address2: pii[:address2].norm,
city: pii[:city].norm,
otp: pii[:otp].norm,
otp: otp,
first_name: pii[:first_name].norm,
last_name: pii[:last_name].norm,
state: pii[:state].norm,
Expand All @@ -29,4 +38,9 @@ def attributes
}
end
# rubocop:enable AbcSize, MethodLength

def generate_otp
# Crockford encoding is 5 bits per character
Base32::Crockford.encode(SecureRandom.random_number(2**(5 * 10)), length: 10)
end
end
12 changes: 12 additions & 0 deletions db/migrate/20170905144239_create_usps_confirmation_codes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class CreateUspsConfirmationCodes < ActiveRecord::Migration[5.1]
def change
create_table :usps_confirmation_codes do |t|
t.integer :profile_id, null: false
t.string :otp_fingerprint, null: false
t.datetime :code_sent_at, null: false, default: ->{ 'CURRENT_TIMESTAMP' }
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be a function? CURRENT_TIMESTAMP() ? Also does it need to be a proc or can it be a literal value?

Copy link
Member Author

Choose a reason for hiding this comment

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

CURRENT_TIMESTAMP is the Postgres syntax. It ends up becoming now() in the schema. We have to use the proc because otherwise Rails tries to treat it like a primitive and sets the default to 'CURRENT_TIMESTAMP' instead of CURRENT_TIMESTAMP.

ref: rails/rails#27077

t.index :profile_id, using: :btree

t.timestamps
end
end
end
Loading