Allow multiple USPS confirmation codes by jmhooper · Pull Request #1661 · 18F/identity-idp

jmhooper · 2017-09-07T20:56:09Z

Why: So that when a user resends a letter, it does not invalidate
the USPS confirmation code in preceding letters. This means that if a
user resends a letter before receiving their first letter, the code in
the first letter can still be used to verify their account.

How: This commit adds a new model named UspsConfirmationCode. This
model is associated with a profile and has an asymmetrically encrypted
OTP. When the user enters an OTP, the record with the matching OTP
fingerprint is found. This is used to determine whether the entered OTP
is valid for verifying their account.

jmhooper · 2017-09-07T20:57:31Z

app/controllers/users/verify_account_controller.rb

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.

jmhooper · 2017-09-07T20:59:05Z

app/models/usps_confirmation_code.rb

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.

zachmargolis · 2017-09-07T21:04:14Z

app/models/usps_confirmation_code.rb

I'm super unfamiliar with find with a block --- is this a table scan?

Oh I see this is used on a relation -- makes wayyyy more sense, but still used the wrong way could be a table scan.

Do you think it would be clearer if we made this a scope? Same result but hopefully it would make it more obvious to users that it's suppsed to be combined with other scopes and not just be a finder mehtod like our others (that are used for point queries)

scope :first_with_otp, ->(otp) { to_a.find { |usps_confirmation_code| ... } }

Good idea 👍

Hmm, this is turning out to be problematic since scopes have to return a relation and find returns a record 🤔

ah dangit, my bad. maybe just a rename to first_with_otp will make it clearer it's not a point query

zachmargolis

Some small questions, but no blockers, LGTM!

This does not include the logic about "only allow 3 in flight at a time" or anything, and we can add that later, right?

zachmargolis · 2017-09-07T21:48:21Z

app/controllers/verify/usps_controller.rb

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?

👍. I'll write up a test

@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?

zachmargolis · 2017-09-07T21:49:59Z

db/migrate/20170905144239_create_usps_confirmation_codes.rb

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

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

zachmargolis · 2017-09-07T21:54:53Z

spec/factories/usps_confirmation_codes.rb

there's no unique index ... would it be easier it we just fingerprinted a fixed value like '000000' or something? And we can override in tests where we need a specific value?

Good catch. Originally I had a uniqueness constraint on this column before I realized that was a bad idea and this is leftover from that.

zachmargolis

LGTM!

**Why**: So that when a user resends a letter, it does not invalidate the USPS confirmation code in preceding letters. This means that if a user resends a letter before receiving their first letter, the code in the first letter can still be used to verify their account. **How**: This commit adds a new model named `UspsConfirmationCode`. This model is associated with a profile and has an asymmetrically encrypted OTP. When the user enters an OTP, the record with the matching OTP fingerprint is found. This is used to determine whether the entered OTP is valid for verifying their account.

jmhooper added the status - ready for review label Sep 7, 2017

jmhooper requested review from monfresh and zachmargolis September 7, 2017 20:56

jmhooper commented Sep 7, 2017

View reviewed changes

zachmargolis reviewed Sep 7, 2017

View reviewed changes

zachmargolis approved these changes Sep 8, 2017

View reviewed changes

jmhooper force-pushed the jmhooper-remove-otp-from-encrypted-attributes branch from 5cdf174 to 5c3ea90 Compare September 8, 2017 20:50

jmhooper merged commit b1bcd64 into master Sep 8, 2017

jmhooper deleted the jmhooper-remove-otp-from-encrypted-attributes branch December 12, 2017 20:15

Conversation

jmhooper commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants