LG-610 Don't show recovery code before IdV flow

[monfresh]

Why: During LOA1 account creation, the user is given their personal
key after setting up 2FA. However, once an LOA3 user passes the
verification steps, and after they are prompted for their password in
order to encrypt their data, they must receive a new personal key.
In order to provide a better account creation experience for LOA3 users,
we only show them the personal key once, instead of once after 2FA
setup, and then again after verification.

This was working as expected for users who used a phone for 2FA, but the
logic for authentication app users was wrong.

Hi! Before submitting your PR for review, and/or before merging it, please
go through the checklists below. These represent the more critical elements
of our code quality guidelines. The rest of the list can be found in
CONTRIBUTING.md

Controllers

• When adding a new controller that requires the user to be fully
  authenticated, make sure to add before_action :confirm_two_factor_authenticated
  as the first callback.

Database

• Unsafe migrations are implemented over several PRs and over several
  deploys to avoid production errors. The strong_migrations gem
  will warn you about unsafe migrations and has great step-by-step instructions
  for various scenarios.

• Indexes were added if necessary. This article provides a good overview
  of indexes in Rails.

• Verified that the changes don't affect other apps (such as the dashboard)

• When relevant, a rake task is created to populate the necessary DB columns
  in the various environments right before deploying, taking into account the users
  who might not have interacted with this column yet (such as users who have not
  set a password yet)

• Migrations against existing tables have been tested against a copy of the
  production database. See LG-228 Make migrations safer and more resilient #2127 for an example when a migration caused deployment
  issues. In that case, all the migration did was add a new column and an index to
  the Users table, which might seem innocuous.

Encryption

• The changes are compatible with data that was encrypted with the old code.

Routes

• GET requests are not vulnerable to CSRF attacks (i.e. they don't change
  state or result in destructive behavior).

Session

• When adding user data to the session, use the user_session helper
  instead of the session helper so the data does not persist beyond the user's
  session.

Testing

• Tests added for this feature/bug
• Prefer feature/integration specs over controller specs
• When adding code that reads data, write tests for nil values, empty strings,
  and invalid inputs.

[jmhooper]

I'm not sure I understand the naming and use of these, especially since we are initializing 2 different policy objects for the same resource in the controller.

Why does there need to be 2 policy objects. It seems like we could have one PersonalKeyConfirmationPolicy object that handles this for both cases.

[monfresh]

I'm open to other names. I was trying to clarify the difference between LOA3 and other users, and that this logic specifically applies during the account creation process only (as opposed to confirming a new personal key at other points in time). One of the reasons for 2 separate policy objects is that they are not always both needed at the same time. In TwoFactorAuthenticatable, only the non-LOA3 policy is needed, whereas in the TOTP Verification Controller, both are needed to determine where to redirect.

One way to have only one class could be to name it NewUserPolicy and have methods like user_signing_up_directly_or_via_loa1? and user_signing_up_via_loa3? methods, but that seems less readable than the 2 separate classes.

What I'm trying to communicate are the 2 scenarios:

• signing up via LOA3 request
• signing up via non-LOA3 request, which includes visiting the site directly or via an LOA1 request

How would you name this class and methods?

[monfresh]

Or maybe UserSigningUpPolicy with methods via_loa3? and not_via_loa3??

[monfresh]

@jmhooper and I discussed this offline, and we agreed that the naming of the policy classes, and the fact that there were two of them, was not ideal. We disagreed on calling this new class a Policy Object versus a Decorator Object, based on Jonathan's primary use and experience with Policy Objects as being similar to Pundit, where they determine whether or not a user is authorized to act on a particular resource.

After doing some more research, I believe Policy Objects are the right choice. The main definition of a Policy Object that I've seen expressed in a variety of articles online is that they encapsulate a business rule and perform read operations in the form of a question. They could be used to ask simple questions, such as whether or not a user is 2FA enabled, or for more complex authorization rules, which is how Pundit uses them.

Decorators, on the other hand, are defined as extending an object, and I've seen them used in different ways, such as acting similar to Service Objects where they might trigger a write operation, or being similar to presenters where they only perform read operations, but don't ask any questions.

Here are some articles for reference:
https://codeclimate.com/blog/7-ways-to-decompose-fat-activerecord-models/ (a classic)
https://medium.com/@steverob/policy-object-d158495265fa
https://pdabrowski.com/blog/ruby-on-rails/refactoring/policy-object-pattern
https://brandonhilkert.com/blog/adding-functionality-to-ruby-classes-with-decorators/

Our current UserDecorator ended up becoming a dumping ground for any methods that we would have placed in the User model. That's not how decorators are intended to be used, and we'll need to extract out some of those methods into separate decorators, policies, and presenters.

What I came up with for this PR is a single policy class called PersonalKeyForNewUserPolicy with a single method called show_personal_key_after_initial_2fa_setup?. Whaddayathink?

[monfresh]

@jmhooper If you have time to give this another look, it would be great to have it included in this release so that LOA3 users have a better experience.

[jmhooper]

💯