Allow user to verify phone OTP after signing out by pkarman · Pull Request #1369 · 18F/identity-idp

pkarman · 2017-04-13T22:26:20Z

Why: If a user completes the vendor verification process
but has not yet verified possession of a new phone number
via IdV, the user is prompted at next sign-in to verify their
phone. This sets up a parallel process to USPS verification,
where there is some indefinite amount of time where a profile
is in a pending state while the user verifies physical
access to their confirmed address (street or phone).

How: Adds a new boolean column to the Profile table called
phone_confirmed that indicates whether they have supplied
a vendor-confirmed phone number as part of their PII. There are
2 steps for all asserted PII: confirmation with vendor, and
verification via OTP. The 2nd step does not necessarily
have to happen in the same IdV session, as it cannot with
USPS delivery. Now phone supports the same scenario.

jessieay · 2017-04-14T20:06:08Z

app/controllers/concerns/verify_profile_concern.rb

public_send instead?

see

TO SEND OR TO PUBLIC SEND? THAT IS THE QUESTION.

via http://vaidehijoshi.github.io/blog/2015/05/05/metaprogramming-dynamic-methods-using-public-send/

jessieay · 2017-04-14T20:06:39Z

app/controllers/concerns/verify_profile_concern.rb

put this below the methods that use it?

jessieay · 2017-04-14T20:07:25Z

app/services/idv/session.rb

jessieay · 2017-04-14T20:11:20Z

app/controllers/concerns/verify_profile_concern.rb

wondering if there might be better names for this. When I first saw it in this PR, I was very curious about what finishing a profile means. Perhaps finish_verifying_profile_path instead?

I agree about the naming.

The original method was just profile_path and I needed something more nuanced. Maybe it's the profile_path. Maybe it's some derivation of profile_path depending on the status of your Profile record (pending vs active). If it's pending, then the route shunts you to finish verifying your profile, making it active.

So it isn't necessarily finish_verifying_profile_path. It's more like, find_the_next_path_either_to_profile_or_to_finish_verifying_profile

thanks for clarifying -- maybe something not as crazy verbose as your example but more verbose would be helpful: profile_or_verify_profile_path ?

jessieay · 2017-04-14T20:13:10Z

app/controllers/users/verify_profile_controller.rb

what's this change about?

just memoizing consistently with other controllers.

jessieay · 2017-04-14T20:14:54Z

spec/controllers/users/verify_profile_controller_spec.rb

I think @zachmargolis likes it when we keep this RSpec for good reasons that I myself cannot articulate on this fine Friday afternoon

jessieay · 2017-04-14T20:16:24Z

spec/controllers/users/verify_profile_controller_spec.rb

why is there so much more stubbing here than in spec/controllers/users/verify_profile_phone_controller_spec.rb ?

I was trying to avoid create(:user) and create(:profile) per @monfresh's quest to hit the db minimally. It means lots more stubbing required.

The other controller is not so honorable and just creates the db data.

cool - perhaps an opp to create a helper method with all that stubbing and re-use it for each file?

monfresh · 2017-04-19T15:47:22Z

app/controllers/saml_idp_controller.rb

If I'm reading the logic correctly, if needs_profile_finish is true, then profile_or_verify_profile_url will always be verify_profile_route, so might as well just go there directly without going through the logic, right?

it could be verify_profile_path or verify_profile_phone_path depending what is required to finish verifying the profile.

Yep, and that logic comes from verify_profile_route, right? I'm saying call the verify_profile_route method directly instead of going through the logic in profile_or_verify_profile_url, which will always return verify_profile_route in this scenario.

We also need this new logic in OpenidConnect::AuthorizationController. Since both SAML and OIDC need to be kept in sync, I wonder if it's time to extract the functionality so it can automatically be shared. Not in this PR, but soon.

monfresh · 2017-04-19T17:26:59Z

app/controllers/concerns/verify_profile_concern.rb

CC says this is not tested.

monfresh · 2017-04-19T17:27:12Z

app/controllers/concerns/verify_profile_concern.rb

CC says this is not tested.

I'm not sure there's a way, under the current business rules, to construct logic to ever reach that fallback profile return value. I put it there so that there is sane default should logical changes ever happen elsewhere. Alternately, we could throw an exception if we get to that point, since there's no logical reason to reach it.

I would vote for removing it entirely. If there's no logical reason to reach it, and there's no sane test we can write for it, why have it?

I would rather have an exception than drop it entirely. Otherwise, it's like a series of if/elsif with no final else -- which leaves open the possibility that someone changes the logic elsewhere and we end up breaking this routing logic.

I don't think an explicit exception is necessary. The final else will be nil if we remove this last line. If somehow this nil is reached, an error will be thrown automatically. As long as we have robust tests, it should protect us from someone breaking this logic.

monfresh · 2017-04-19T20:10:14Z

app/view_models/profile_index.rb

CC says this is not tested.

monfresh · 2017-04-19T20:11:46Z

db/migrate/20170413152832_add_profile_phone_confirmed.rb

Do we also need an index?

IME the index is only required if you intend to search or sort on a column. Otherwise it adds extraneous db overhead to keep the index updated. We only ever check the value of this flag after we've searched by user_id.

jessieay · 2017-04-19T20:29:20Z

app/decorators/user_decorator.rb

wow! the logic in here is sufficiently complicated that I wonder if we should consider breaking out a separate class. If not now, maybe l8r.

jessieay · 2017-04-19T20:30:19Z

app/decorators/user_decorator.rb

not seeing specs for either of these smaller methods

monfresh · 2017-04-19T21:15:35Z

spec/controllers/saml_idp_controller_spec.rb

What about the finish_profile: true scenario? Maybe also add a feature spec in spec/features/saml/loa3_sso_spec.rb to test this whole flow?

good call @monfresh -- added specs and found some bugs in the process.

zachmargolis · 2017-04-20T20:03:03Z

.reek

  max_statements: 6
  exclude:
    - Users::PhoneConfirmationController
+    - OpenidConnect::AuthorizationController


(ノಠ益ಠ)ノ彡 ǝʇɐɯıʃɔǝpoɔ
(ノಠ益ಠ)ノ彡 ʞǝǝɹ

If I had to choose between too many statements and too many methods/class too long, I would prefer smaller methods and then later take a look at the class and see if it really is doing too much.

ok I have added the class length + method count exclusions in lieu of the method length error.

pkarman · 2017-04-21T17:01:18Z

This needs an official GH review

monfresh · 2017-04-21T17:33:29Z

app/controllers/openid_connect/authorization_controller.rb

    def index
      return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated?
-      return redirect_to verify_url if identity_needs_verification?
+      return if idv_needed_or_in_process


I would expect this method to return true or false and for it to end in a question mark, but this method is redirecting, which is confusing. When I see return if [some condition is true], I expect the method to be exited without any side effects.

Maybe something like this?

return redirect_to_profile_or_verify_profile_url if profile_or_identity_needs_verification? def redirect_to_profile_or_verify_profile_url return redirect_to profile_or_verify_profile_url if profile_needs_verification? redirect_to verify_url if identity_needs_verification? end def profile_or_identity_needs_verification? profile_needs_verification? || identity_needs_verification? end

monfresh · 2017-04-21T20:03:31Z

spec/controllers/users/verify_profile_phone_controller_spec.rb

+        end
+      end
+
+      context 'phone is same as 2FA' do


I don't see any difference between this test and the one above. Did you mean to only set the profile_phone equal to the user phone in this test, and not in the test above as well? That way, we can test each scenario independently if it makes a difference.

monfresh · 2017-04-21T20:23:58Z

spec/decorators/user_decorator_spec.rb

+      expect(user_decorator.pending_profile_requires_verification?).to eq false
+    end
+
+    it 'returns true when no active profile exists' do


This spec title confused me at first. I thought the user did not have any profiles at all. Since there are 3 different reasons why a profile can be deactivated, how about a more specific title like "returns true when the user has a deactivated profile due to pending verification"?

And then please add 2 more tests to verify that if the deactivated_reason is password_reset or encryption_error, then this returns false.

I just realized there is a conditional in the decorator that checks if identity_not_verified?, which I think this test was meant for, but the test set up seems to be testing something else.

To test the identity_not_verified? conditional, I would create a profile with active: false, as opposed to one with a deactivation_reason.

To test the scenario where a user has a pending_profile, I would perhaps stub the decorator to receive pending_profile and return true, and then add more tests to the #pending_profile section to test the 2 other deactivation_reason scenarios.

Also, what do you think about using build_stubbed instead of create for the profiles to speed up the tests?

monfresh · 2017-04-21T20:55:52Z

spec/decorators/user_decorator_spec.rb

+      it 'returns true' do
+        expect(user_decorator.needs_profile_phone_verification?).to eq true
+      end
+    end


Since the pending_profile_requires_verification? method is unit tested above, and since the needs_profile_phone_verification? method depends on that method, what do you think about stubbing the decorator to receive pending_profile_requires_verification? with true and false and testing those scenarios, as opposed to creating a profile with a deactivated_reason. This would make this test less brittle.

monfresh · 2017-04-21T20:57:02Z

spec/decorators/user_decorator_spec.rb

+      it 'returns false' do
+        expect(user_decorator.needs_profile_usps_verification?).to eq false
+      end
+    end


Same comment here about stubbing pending_profile_requires_verification?.

zachmargolis · 2017-04-24T15:49:55Z

@monfresh since Peter has rotated off the team and gone on vacation, do you want to just update the branch with your latest feedback?

monfresh · 2017-04-24T19:39:35Z

Yes, will update.

zachmargolis · 2017-05-02T17:04:11Z

config/routes.rb

  post '/profile/reactivate' => 'users/reactivate_profile#create'
  get '/profile/verify' => 'users/verify_profile#index', as: :verify_profile
  post '/profile/verify' => 'users/verify_profile#create'
+  get '/profile/verify_phone' => 'users/verify_profile_phone#index', as: :verify_profile_phone


@monfresh for consistency, we've started renaming all the /profile URLs to /account and setting up redirects for old ones (see #1407). Since this is brand-new can we rename it to /account/verify_phone before it lands? (Also appears there are merge conflicts related to the rename....oops)

monfresh · 2017-05-10T19:08:25Z

I incorporated my feedback in this PR, squashed, and rebased with master. Now I need to address the failing specs.

monfresh · 2017-05-10T20:24:23Z

@zachmargolis This is good to go now. Please re-review. Thanks!

zachmargolis

Some small questions, but not blockers LGTM. Thanks for taking the time to clean all this up!

zachmargolis · 2017-05-11T13:02:35Z

app/controllers/concerns/verify_profile_concern.rb

now that we've made the name account for the default page, should we call it account_or_verify_profile_route?

Yeah, I thought about that. I can change it.

zachmargolis · 2017-05-11T13:04:54Z

app/services/idv/profile_maker.rb

What if we made this !!phone_confirmed ?

The reason is passing an explicit false here will result in true:

!false.nil? # => true !!false # => false

phone_confirmation in Idv::Session can either be nil or true. How about we force it to false if nil either when we call ProfileMaker in IdVSession, or change line 10 above to phone_confirmed || false?

I'm used to !!value as a convention, but if you feel strongly, value || false works too

I find that the double bang requires more mental effort. It's also discouraged by the Ruby Style Guide: https://github.com/bbatsov/ruby-style-guide#no-bang-bang

¯\_(ツ)_/¯ up to you. I like the Ruby Style Guide as a suggestion, but I definitely disagree with it on certain parts (ex about the readability of unless)

**Why**: If a user completes the vendor verification process but has not yet verified possession of a new phone number via IdV, the user is prompted at next sign-in to verify their phone. This sets up a parallel process to USPS verification, where there is some indefinite amount of time where a profile is in a pending state while the user verifies physical access to their confirmed address (street or phone). **How**: Adds a new boolean column to the `Profile` table called `phone_confirmed` that indicates whether they have supplied a vendor-confirmed phone number as part of their PII. There are 2 steps for all asserted PII: confirmation with vendor, and verification via OTP. The 2nd step does not necessarily have to happen in the same IdV session, as it cannot with USPS delivery. Now phone supports the same scenario.

pkarman added the status - work in progress label Apr 13, 2017

pkarman self-assigned this Apr 13, 2017

pkarman force-pushed the pek-unfinished-idv-phone-otp branch from 02f8b0b to 24a123f Compare April 14, 2017 14:29

pkarman added status - ready for review and removed status - work in progress labels Apr 14, 2017

pkarman requested review from jessieay, monfresh and zachmargolis April 14, 2017 14:29

pkarman changed the title ~~[WIP] Allow user to verify phone OTP after signing out~~ Allow user to verify phone OTP after signing out Apr 14, 2017

jessieay reviewed Apr 14, 2017

View reviewed changes

monfresh reviewed Apr 19, 2017

View reviewed changes

app/controllers/concerns/verify_profile_concern.rb Outdated

Copy link

Contributor

monfresh Apr 19, 2017

Choose a reason for hiding this comment

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

CC says this is not tested.

pkarman reacted with thumbs up emoji

monfresh reviewed Apr 19, 2017

View reviewed changes

app/view_models/profile_index.rb Outdated

Copy link

Contributor

monfresh Apr 19, 2017

Choose a reason for hiding this comment

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

CC says this is not tested.

monfresh reviewed Apr 19, 2017

View reviewed changes

jessieay reviewed Apr 19, 2017

View reviewed changes

monfresh reviewed Apr 19, 2017

View reviewed changes

pkarman force-pushed the pek-unfinished-idv-phone-otp branch from 803014c to cb45671 Compare April 20, 2017 19:03

zachmargolis reviewed Apr 20, 2017

View reviewed changes

monfresh reviewed Apr 21, 2017

View reviewed changes

zachmargolis assigned monfresh and unassigned pkarman May 2, 2017

zachmargolis suggested changes May 2, 2017

View reviewed changes

zachmargolis mentioned this pull request May 2, 2017

Rename more /profile URLs to /account #1414

Merged

monfresh force-pushed the pek-unfinished-idv-phone-otp branch 2 times, most recently from d8f9726 to ff26e9f Compare May 10, 2017 18:43

monfresh force-pushed the pek-unfinished-idv-phone-otp branch 2 times, most recently from e84d123 to 42e3a33 Compare May 10, 2017 20:23

This was referenced May 11, 2017

Redirect user with a pending profile to /account #1424

Merged

Adds USPS completion page and wires up USPS confirmation form #1421

Merged

zachmargolis approved these changes May 11, 2017

View reviewed changes

monfresh force-pushed the pek-unfinished-idv-phone-otp branch from 42e3a33 to 67fee0d Compare May 11, 2017 18:42

monfresh force-pushed the pek-unfinished-idv-phone-otp branch from 67fee0d to f8233b7 Compare May 11, 2017 20:53

monfresh merged commit c12fdeb into master May 11, 2017

monfresh deleted the pek-unfinished-idv-phone-otp branch May 11, 2017 21:17

monfresh mentioned this pull request Jan 7, 2018

Validate phone for Voice OTP delivery preference #1898

Merged

8 tasks

Conversation

pkarman commented Apr 13, 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!

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!

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!

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!

pkarman commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

monfresh Apr 21, 2017 •

edited

Loading