From b2edb1a98dd97d99a83f8c07bb231f6db1281f72 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 7 Sep 2018 13:26:07 -0400 Subject: [PATCH] Revert find_with_email changes **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. --- app/controllers/users/sessions_controller.rb | 6 ++++-- app/models/concerns/user_encrypted_attribute_overrides.rb | 3 +-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index b48093f87c0..54cfaddb588 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -130,8 +130,10 @@ def cache_active_profile begin cacher.save(auth_params[:password], profile) rescue Encryption::EncryptionError => err - profile.deactivate(:encryption_error) - analytics.track_event(Analytics::PROFILE_ENCRYPTION_INVALID, error: err.message) + if profile + profile.deactivate(:encryption_error) + analytics.track_event(Analytics::PROFILE_ENCRYPTION_INVALID, error: err.message) + end end end diff --git a/app/models/concerns/user_encrypted_attribute_overrides.rb b/app/models/concerns/user_encrypted_attribute_overrides.rb index 4abaa71b73c..ae5670e1115 100644 --- a/app/models/concerns/user_encrypted_attribute_overrides.rb +++ b/app/models/concerns/user_encrypted_attribute_overrides.rb @@ -15,8 +15,7 @@ def find_with_email(email) email = email.downcase.strip email_fingerprint = create_fingerprint(email) - resource = find_by(email_fingerprint: email_fingerprint) - resource if resource&.email == email + find_by(email_fingerprint: email_fingerprint) end def create_fingerprint(email)