Disable phone OTP option in select US area codes by jmhooper · Pull Request #1559 · 18F/identity-idp

jmhooper · 2017-07-19T16:39:43Z

Why: Twilio does not allow us to call users in some US territories
which have a +1 internation code. This commit adds frontend code that
disables the phone option and messages the user that they will have to
receive an OTP via text.

jmhooper · 2017-07-19T16:40:37Z

Right now this only includes code for the frontend part of this. I still need to figure how this looks on the backend.

jmhooper · 2017-07-19T16:53:43Z

Initial screenshot:

zachmargolis

definitely worth re-doing

zachmargolis · 2017-07-19T16:59:42Z

app/assets/javascripts/app/phone-internationalization.js

Do we want to worry about translating these names?

Also I think we should move these values to something on the server, and expose them as data- attributes in the DOM or something (that way we can double check these values on the server which also works for clients without JS)

Yeah, still working on the direction for how this looks on the server and thinking in that direction.

zachmargolis · 2017-07-19T17:03:20Z

app/assets/javascripts/app/phone-internationalization.js

might be worth expanding our custom I18n.t in JS to do templating?

ex

I18n.t('devise.two_factor_authentication.otp_delivery_preference.phone_unsupported', { location: country })

zachmargolis · 2017-07-19T17:03:54Z

app/assets/javascripts/app/phone-internationalization.js

did you mean to query the entire document? or just a part of a particular form?

Ooops, meant to come back and clean this part up.

zachmargolis · 2017-07-19T20:06:39Z

spec/features/two_factor_authentication/sign_in_spec.rb

what if we called the variable guam_phone and removed the comment?

jmhooper · 2017-07-20T02:11:31Z

I've made some changes.

For forms with an OTP delivery method option, I added a validation to confirm that the OTP delivery method is supported for the given phone number. If it is not, it will add an error explaining the problem.

For forms without an OTP delivery method option (edit 2FA method form and idv phone confirmation) I added some code to make sure the OTP is sent via SMS and that the option to send a code via voice call is not presented to the user.

jmhooper · 2017-07-20T04:06:39Z

Changed it up a little bit more so that the unsupported area codes are fed to the form in a data attribute. This should be ready for review now.

monfresh · 2017-07-20T15:56:08Z

We should remove the "Mobile or landline okay" text, or make it dynamic such that it says "Mobile only" when voice is not supported.

monfresh · 2017-07-20T16:00:04Z

.rubocop.yml

Rubocop does not check Slim files. This can safely be removed.

We have rubocop enabled for slim-lint. It runs rubocop on the files after slim processes them. I added a multiline hash to one and since slim removes all indentation this cop got angry.

There is a ignored_cops options for slim-lint, but adding one cop to that appears to re-enable all of the cops that slim-lint ignores by default.

Huh. This must be new. I didn't know you could disable cops within .rubocop.yml for slim-lint to ignore.

jmhooper · 2017-07-20T16:08:06Z

app/views/users/two_factor_authentication_setup/index.html.slim

@monfresh: Here's the line that makes slim-lint upset. An alternative I'd prefer to disabling the IndentHash cop is figuring out how to make this a bit shorter.

Yes, I thought about that. You could set the hash in the controller and pass it to the view as a local variable.

zachmargolis · 2017-07-20T16:23:05Z

app/services/phone_number_capabilities.rb

Personally I think using Phonelib here is clearer, like we do here? https://github.com/18F/identity-idp/blob/master/app/forms/otp_delivery_selection_form.rb#L37-L48

Phonelib.parse(phone).area_code

Fancy 👔. So many phone gems.....

Alright, so this started breaking the spec for the IdV flow. The problem is that at this point the phone number has been formatted for the vendor and no longer includes the country code. It appears to still work for US numbers, but gets confused for numbers with a US country code.

Using Phony to normalize it to add the +1 works, so I may do that.

Related: We may want to make sure we only accept US phone numbers for IdV in #1544

monfresh · 2017-07-20T16:25:17Z

I'm seeing something weird in the UI when testing locally. See the Phone call button in the screenshot below:

zachmargolis · 2017-07-20T17:39:34Z

app/services/phone_number_capabilities.rb

so it looks like in all the uses of this, we do !capabilities.voice? -- maybe should we just invert it and switch to capabilities.sms_only?

monfresh · 2017-07-20T17:51:10Z

Here's another UI glitch on the OTP verification page:

monfresh · 2017-07-20T18:05:38Z

One thing this PR doesn't address is that in the screen where you enter your new phone number when you want to change it, it doesn't mention anything about what kind of phone you can enter. Perhaps we should replace that view with the same view as in the Phone Setup?

jmhooper · 2017-07-20T18:15:41Z

Re the UI issue on the OTP verification:

Looks like it also breaks if you have an authenticator app. It just says " or get a security code via authentication app."

I did also consider pulling the same UI we use for 2FA setup into the edit 2FA view, but wanted to pause on that and prioritize international calling and then come back to it if we have time at the end of this sprint or the beginning of the next one.

jmhooper · 2017-07-20T19:02:02Z

Alright, I think I've addressed everything above^

zachmargolis

LGTM

monfresh · 2017-07-20T19:31:13Z

app/presenters/two_factor_auth_code/phone_delivery_presenter.rb

Code Climate says this line is not tested.

Ah yeah, this presenter has basically no tests. I can stick some in there.

@monfresh: Added tests for the parts I changed.

monfresh · 2017-07-20T19:59:13Z

spec/support/shared_examples_for_otp_delivery_preference_validation.rb

We discourage the use of allow_any_instance_of, as do the RSpec docs: https://relishapp.com/rspec/rspec-mocks/docs/working-with-legacy-code/any-instance

The problem with this before statement is that it makes it look like UpdateUser should be called even if the validation failed, which is not true.

Instead, we do something like:

update_user = instance_double(UpdateUser) attributes = { otp_delivery_preference: 'voice' } allow(UpdateUser).to receive(:new).with(user: user, attributes: attributes).and_return(update_user) expect(update_user).to receive(:call) # when validation passes capabilities = instance_double(PhoneNumberCapabilities) allow(PhoneNumberCapabilities).to receive(:new).with(phone).and_return(capabilities) allow(capabilities).to receive(:sms_only).and_return(false) result = subject.submit(phone: phone, otp_delivery_preference: 'voice')

and

expect(update_user).to_not receive(:call) # when validation fails

@monfresh: Feels weird to stub new 🤔, but I went ahead and changed the test. I used a spy for the phone number capabilities because it handles some other calls to build the error message.

monfresh · 2017-07-20T20:18:33Z

spec/support/shared_examples_for_otp_delivery_preference_validation.rb

Remove the comment?

monfresh · 2017-07-20T20:18:40Z

spec/support/shared_examples_for_otp_delivery_preference_validation.rb

Remove the comment?

That's what I get for copy/pasting 😳

monfresh

LGTM

**Why**: Twilio does not allow us to call users in some US territories which have a +1 internation code. This commit adds frontend code that disables the phone option and messages the user that they will have to receive an OTP via text. Validate that phone and otp delivery method match **Why**: If we can't support voice for a user's phone number, and they manage to submit the form (e.g. they have JS disabled) we want to display an error message explaining the problem Disable unsupported voice cofirm in phone forms **Why**: If a user provides a phone number that is unsupported for voice in the form to edit their 2FA phone number or the idv phone form, we want to send them an SMS and not send them give them the option to select phone. Load unsupported phone area codes in data attr **Why**: We don't want to maintain a list of unsupported area codes in both the JS and in the ruby code.

jmhooper added the status - work in progress label Jul 19, 2017

zachmargolis reviewed Jul 19, 2017

View reviewed changes

jmhooper changed the title ~~[WIP] Disable phone OTP option in select US area codes~~ Disable phone OTP option in select US area codes Jul 20, 2017

jmhooper added status - ready for review and removed status - work in progress labels Jul 20, 2017

jmhooper requested a review from monfresh July 20, 2017 04:06

monfresh reviewed Jul 20, 2017

View reviewed changes

jmhooper commented Jul 20, 2017

View reviewed changes

zachmargolis reviewed Jul 20, 2017

View reviewed changes

zachmargolis approved these changes Jul 20, 2017

View reviewed changes

jmhooper force-pushed the jmhooper-unsupported-phone-area-code branch from 4faec7e to 662d2cf Compare July 20, 2017 19:12

monfresh reviewed Jul 20, 2017

View reviewed changes

spec/support/shared_examples_for_otp_delivery_preference_validation.rb Outdated

Copy link

Contributor

monfresh Jul 20, 2017

Choose a reason for hiding this comment

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

Remove the comment?

monfresh reviewed Jul 20, 2017

View reviewed changes

monfresh approved these changes Jul 20, 2017

View reviewed changes

jmhooper force-pushed the jmhooper-unsupported-phone-area-code branch from 9c03886 to 2f053bc Compare July 20, 2017 20:32

jmhooper merged commit e709b85 into master Jul 20, 2017

jmhooper deleted the jmhooper-unsupported-phone-area-code branch July 20, 2017 20:44

Conversation

jmhooper commented Jul 19, 2017

Uh oh!

jmhooper commented Jul 19, 2017

Uh oh!

jmhooper commented Jul 19, 2017

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!

jmhooper commented Jul 20, 2017

Uh oh!

jmhooper commented Jul 20, 2017

Uh oh!

monfresh commented Jul 20, 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!

monfresh commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Jul 20, 2017

Uh oh!

monfresh commented Jul 20, 2017

Uh oh!

jmhooper commented Jul 20, 2017

Uh oh!

jmhooper commented Jul 20, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh left a comment