Skip to content

Revert changes to find_with_email #2497

Merged
monfresh merged 1 commit intomasterfrom
mb-fix-find-with-email
Sep 8, 2018
Merged

Revert changes to find_with_email #2497
monfresh merged 1 commit intomasterfrom
mb-fix-find-with-email

Conversation

@monfresh
Copy link
Copy Markdown
Contributor

@monfresh monfresh commented Sep 7, 2018

Why: These changes were made as part of #2478 in order to make the
test in spec/features/users/sign_in_spec.rb:244 pass, but they
masked a bug in Users::SessionsController. Moreover, they made the
heavily used find_with_email method almost 4 times slower. The goal of
this particular test is to make sure that the app fails loudly if we
mess up the list of keys during key rotation. Preferably, we want to
fail as early as possible.

Here is the flow that explains why this test was passing before the
changes in #2478:

  1. User signs in with email and password
  2. Users::SessionsController#create is called
  3. track_authentication_attempt(auth_params[:email]) is called
  4. user_signed_in_and_not_locked_out?(user) is called
  5. return false unless current_user is called
  6. Devise calls current_user, then Warden calls UpdateUser in
    config/initializers/session_limitable.rb:9, which raises
    Encryption::EncryptionError because it is trying to decrypt the
    phone encryptable attribute.

With the changes in #2478, UpdateUser no longer tries to decrypt the
phone, which is a good thing. The flow then continues as follows:

  1. User#need_two_factor_authentication? is called
  2. two_factor_enabled? is called and returns true right after it sees
    that phone_configuration&.mfa_enabled? is true.

Note that so far, no encrypted attributes have been accessed.

  1. After a few more calls, cache_active_profile is reached and
    cacher.save(auth_params[:password], profile) is called
  2. Within Pii::Cacher#save, stale_attributes? is called.
  3. user.phone_configuration&.stale_encrypted_phone? is called, which
    which raises Encryption::EncryptionError.

However, Users::SessionsController#cache_active_profile rescues
this error, and then profile.deactivate(:encryption_error) is called,
which raises an error because profile is nil.

The reason why we never saw this bug in SessionsController is because
we haven't had problems rotating keys, and because so far, up until the
changes in #2478, Encryption::EncryptionError was raised before
cache_active_profile was reached. For example, before we introduced
the PhoneConfiguration table, the error was raised early via
two_factor_enabled?, which accessed the phone encryptable attribute.

Similarly, if you change the spec to use a user
:with_authentication_app instead of :signed_up, the test will pass
because two_factor_enabled? will call totp_enabled?, which will
try to decrypt the otp_secret_key.

To make the test pass with an MFA-enabled user, we can wrap the 2 lines
(in cache_active_profile) after the rescue in an if profile block,
which we should do anyways. This will then allow the user to sign in,
and will then raise the Encryption::EncryptionError on the 2FA page
when it tries to decrypt the phone number.

Ideally, we want to fail as early as possible, but with the current
design of cache_active_profile, that's not possible because it is
coupling actions that only apply to verified users with actions
that apply to all users when keys are rotated.

In a follow-up PR, I will attempt to extract the key rotation so that
decryption attempts fail early and are not rescued.

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

  • 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.

  • 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.

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

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

  • 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.

  • 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.

**Why**: These changes were made as part of #2478 in order to make the
test in `spec/features/users/sign_in_spec.rb:244` pass, but they
masked a bug in `Users::SessionsController`. Moreover, they made the
heavily used `find_with_email` method almost 4 times slower. The goal of
this particular test is to make sure that the app fails loudly if we
mess up the list of keys during key rotation. Preferably, we want to
fail as early as possible.

Here is the flow that explains why this test was passing before the
changes in #2478:

1. User signs in with email and password
2. `Users::SessionsController#create` is called
3. `track_authentication_attempt(auth_params[:email])` is called
4. `user_signed_in_and_not_locked_out?(user)` is called
5. `return false unless current_user` is called
6. Devise calls `current_user`, then Warden calls `UpdateUser` in
`config/initializers/session_limitable.rb:9`, which raises
`Encryption::EncryptionError` because it is trying to decrypt the
`phone` encryptable attribute.

With the changes in #2478, `UpdateUser` no longer tries to decrypt the
phone, which is a good thing. The flow then continues as follows:

7. `User#need_two_factor_authentication?` is called
8. `two_factor_enabled?` is called and returns true right after it sees
that `phone_configuration&.mfa_enabled?` is true.

Note that so far, no encrypted attributes have been accessed.

9. After a few more calls, `cache_active_profile` is reached and
`cacher.save(auth_params[:password], profile)` is called
10. Within `Pii::Cacher#save`, `stale_attributes?` is called.
11. `user.phone_configuration&.stale_encrypted_phone?` is called, which
which raises `Encryption::EncryptionError`.

However, `Users::SessionsController#cache_active_profile` rescues
this error, and then `profile.deactivate(:encryption_error)` is called,
which raises an error because `profile` is `nil`.

The reason why we never saw this bug in `SessionsController` is because
we haven't had problems rotating keys, and because so far, up until the
changes in #2478, `Encryption::EncryptionError` was raised before
`cache_active_profile` was reached. For example, before we introduced
the PhoneConfiguration table, the error was raised early via
`two_factor_enabled?`, which accessed the `phone` encryptable attribute.

Similarly, if you change the spec to use a user
`:with_authentication_app` instead of `:signed_up`, the test will pass
because `two_factor_enabled?` will call `totp_enabled?`, which will
try to decrypt the `otp_secret_key`.

To make the test pass with an MFA-enabled user, we can wrap the 2 lines
(in `cache_active_profile`) after the rescue in an `if profile` block,
which we should do anyways. This will then allow the user to sign in,
and will then raise the `Encryption::EncryptionError` on the 2FA page
when it tries to decrypt the phone number.

Ideally, we want to fail as early as possible, but with the current
design of `cache_active_profile`, that's not possible because it is
coupling actions that only apply to verified users with actions
that apply to all users when keys are rotated.

In a follow-up PR, I will attempt to extract the key rotation so that
decryption attempts fail early and are not rescued.
profile.deactivate(:encryption_error)
analytics.track_event(Analytics::PROFILE_ENCRYPTION_INVALID, error: err.message)
if profile
profile.deactivate(:encryption_error)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting that with this change, but no change to the specs, everything passes. I thought about going down this route originally, but the test was looking for a particular decryption error to get thrown, which couldn't come from this code since it gets rescued.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The error comes later. See my commit message.

@monfresh monfresh merged commit 36c10fa into master Sep 8, 2018
@monfresh monfresh deleted the mb-fix-find-with-email branch September 8, 2018 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants