From 237faf2edc8c0c6d9b0a5923a11caf61cd4c49d6 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 16 Nov 2023 12:36:55 -0500 Subject: [PATCH 1/2] LG-11535 Encrypt the pending and active profile when a user updates their password In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. As a result of this change we can encrypt both the pending and active profile with user's password. This means both profiles can be decrypted on sign-in. There are consequences for recovery with personal key. When the user changes their password and their data is encrypted their recovery PII is also encrypted as a consequence. The pending and active profile are both encrypted with a different personal key. In this commit I elected to display the pending profile personal key to the user so that profile is recoverable when it becomes active. [skip changelog] --- app/forms/update_user_password_form.rb | 9 +- app/services/active_profile_encryptor.rb | 22 ---- app/services/user_profiles_encryptor.rb | 28 +++++ .../users/passwords_controller_spec.rb | 1 + spec/forms/update_user_password_form_spec.rb | 30 +++-- .../services/active_profile_encryptor_spec.rb | 25 ----- spec/services/user_profiles_encryptor_spec.rb | 105 ++++++++++++++++++ 7 files changed, 160 insertions(+), 60 deletions(-) delete mode 100644 app/services/active_profile_encryptor.rb create mode 100644 app/services/user_profiles_encryptor.rb delete mode 100644 spec/services/active_profile_encryptor_spec.rb create mode 100644 spec/services/user_profiles_encryptor_spec.rb diff --git a/app/forms/update_user_password_form.rb b/app/forms/update_user_password_form.rb index 70a8e652aaa..ef9a7a3188c 100644 --- a/app/forms/update_user_password_form.rb +++ b/app/forms/update_user_password_form.rb @@ -24,7 +24,7 @@ def submit(params) def process_valid_submission update_user_password - encrypt_user_profile_if_active + encrypt_user_profiles end def update_user_password @@ -32,15 +32,14 @@ def update_user_password UpdateUser.new(user: user, attributes: attributes).call end - def encrypt_user_profile_if_active - active_profile = user.active_profile - return if active_profile.blank? + def encrypt_user_profiles + return if user.active_or_pending_profile.blank? encryptor.call end def encryptor - @encryptor ||= ActiveProfileEncryptor.new(user, user_session, password) + @encryptor ||= UserProfilesEncryptor.new(user, user_session, password) end def extra_analytics_attributes diff --git a/app/services/active_profile_encryptor.rb b/app/services/active_profile_encryptor.rb deleted file mode 100644 index 20b6b87ffa9..00000000000 --- a/app/services/active_profile_encryptor.rb +++ /dev/null @@ -1,22 +0,0 @@ -class ActiveProfileEncryptor - attr_reader :personal_key - - def initialize(user, user_session, password) - @user = user - @user_session = user_session - @password = password - end - - def call - active_profile = @user.active_profile - @personal_key = active_profile.encrypt_pii(current_pii, @password) - active_profile.save! - end - - private - - def current_pii - cacher = Pii::Cacher.new(@user, @user_session) - cacher.fetch - end -end diff --git a/app/services/user_profiles_encryptor.rb b/app/services/user_profiles_encryptor.rb new file mode 100644 index 00000000000..9c6e8e4bfff --- /dev/null +++ b/app/services/user_profiles_encryptor.rb @@ -0,0 +1,28 @@ +class UserProfilesEncryptor + attr_reader :personal_key + + def initialize(user, user_session, password) + @user = user + @user_session = user_session + @password = password + end + + def call + if user.active_profile.present? + encrypt_pii_for_profile(user.active_profile) + end + if user.pending_profile.present? + encrypt_pii_for_profile(user.pending_profile) + end + end + + private + + attr_reader :user, :password, :user_session + + def encrypt_pii_for_profile(profile) + pii = Pii::Cacher.new(user, user_session).fetch(profile.id) + @personal_key = profile.encrypt_pii(pii, password) + profile.save! + end +end diff --git a/spec/controllers/users/passwords_controller_spec.rb b/spec/controllers/users/passwords_controller_spec.rb index 40eb505c572..2dd34444438 100644 --- a/spec/controllers/users/passwords_controller_spec.rb +++ b/spec/controllers/users/passwords_controller_spec.rb @@ -48,6 +48,7 @@ stub_sign_in(user) Pii::Cacher.new(user, controller.user_session).save_decrypted_pii( Pii::Attributes.new(ssn: '111-222-3333'), + user.active_profile.id, ) params = { diff --git a/spec/forms/update_user_password_form_spec.rb b/spec/forms/update_user_password_form_spec.rb index f3222eb6738..92453925365 100644 --- a/spec/forms/update_user_password_form_spec.rb +++ b/spec/forms/update_user_password_form_spec.rb @@ -34,7 +34,7 @@ } expect(UpdateUser).not_to receive(:new) - expect(ActiveProfileEncryptor).not_to receive(:new) + expect(UserProfilesEncryptor).not_to receive(:new) expect(subject.submit(params).to_h).to include( success: false, errors: errors, @@ -70,11 +70,15 @@ context 'when the user has an active profile' do let(:profile) { create(:profile, :active, :verified, pii: { ssn: '1234' }) } let(:user) { profile.user } - let(:user_session) { { decrypted_pii: { ssn: '1234' }.to_json } } + let(:user_session) { {} } + + before do + Pii::Cacher.new(user, user_session).save_decrypted_pii({ ssn: '1234' }, profile.id) + end it 'encrypts the active profile' do - encryptor = instance_double(ActiveProfileEncryptor) - allow(ActiveProfileEncryptor).to receive(:new). + encryptor = instance_double(UserProfilesEncryptor) + allow(UserProfilesEncryptor).to receive(:new). with(user, user_session, password).and_return(encryptor) allow(encryptor).to receive(:call) @@ -96,11 +100,21 @@ context 'the user has a pending profile' do let(:profile) { create(:profile, :verify_by_mail_pending, :verified, pii: { ssn: '1234' }) } let(:user) { profile.user } + let(:user_session) { {} } + + before do + Pii::Cacher.new(user, user_session).save_decrypted_pii({ ssn: '1234' }, profile.id) + end - it 'does not call ActiveProfileEncryptor' do - expect(ActiveProfileEncryptor).to_not receive(:new) + it 'encrypts the pending profile' do + encryptor = instance_double(UserProfilesEncryptor) + allow(UserProfilesEncryptor).to receive(:new). + with(user, user_session, password).and_return(encryptor) + allow(encryptor).to receive(:call) subject.submit(params) + + expect(encryptor).to have_received(:call) end it 'logs that the user has a pending profile' do @@ -114,8 +128,8 @@ end context 'when the user does not have a profile' do - it 'does not call ActiveProfileEncryptor' do - expect(ActiveProfileEncryptor).to_not receive(:new) + it 'does not call UserProfilesEncryptor' do + expect(UserProfilesEncryptor).to_not receive(:new) subject.submit(params) end diff --git a/spec/services/active_profile_encryptor_spec.rb b/spec/services/active_profile_encryptor_spec.rb deleted file mode 100644 index 568ff20e0de..00000000000 --- a/spec/services/active_profile_encryptor_spec.rb +++ /dev/null @@ -1,25 +0,0 @@ -require 'rails_helper' - -RSpec.describe ActiveProfileEncryptor do - describe '#call' do - it 'encrypts the profile' do - decrypted_pii = { ssn: '1234' }.to_json - user_session = { decrypted_pii: decrypted_pii } - profile = create(:profile, :active, :verified, pii: { ssn: '1234' }) - user = profile.user - password = user.password - current_pii = Pii::Attributes.new_from_json(decrypted_pii) - - allow(user).to receive(:active_profile).and_return(profile) - allow(profile).to receive(:encrypt_pii) - allow(profile).to receive(:save!) - allow(Pii::Attributes).to receive(:new_from_json).with(decrypted_pii). - and_return(current_pii) - - ActiveProfileEncryptor.new(user, user_session, password).call - - expect(profile).to have_received(:encrypt_pii).with(current_pii, password) - expect(profile).to have_received(:save!) - end - end -end diff --git a/spec/services/user_profiles_encryptor_spec.rb b/spec/services/user_profiles_encryptor_spec.rb new file mode 100644 index 00000000000..70e0b15a3bd --- /dev/null +++ b/spec/services/user_profiles_encryptor_spec.rb @@ -0,0 +1,105 @@ +require 'rails_helper' + +RSpec.describe UserProfilesEncryptor do + describe '#call' do + let(:user_session) { {}.with_indifferent_access } + let(:pii) { Pii::Attributes.new(ssn: '1234') } + let(:profile) { create(:profile, :active, :verified, pii: pii.to_h) } + let(:user) { profile.user } + let(:password) { 'a new and incredibly exciting password' } + + before do + Pii::Cacher.new(user, user_session).save_decrypted_pii(pii, profile.id) + end + + context 'when the user has an active profile' do + it 'encrypts the PII for the active profile with the password' do + encryptor = UserProfilesEncryptor.new(user, user_session, password) + encryptor.call + + profile.reload + + personal_key = PersonalKeyGenerator.new(user).normalize(encryptor.personal_key) + + decrypted_profile_pii = profile.decrypt_pii(password) + decrypted_profile_recovery_pii = profile.recover_pii(personal_key) + + expect(pii).to eq(decrypted_profile_pii) + expect(pii).to eq(decrypted_profile_recovery_pii) + expect(user.valid_personal_key?(personal_key)).to eq(true) + end + end + + context 'when the user has a pending profile' do + let(:profile) { create(:profile, :verify_by_mail_pending, :verified, pii: pii.to_h) } + + it 'encrypts the PII for the pending profile with the password' do + encryptor = UserProfilesEncryptor.new(user, user_session, password) + encryptor.call + + profile.reload + + personal_key = PersonalKeyGenerator.new(user).normalize(encryptor.personal_key) + + decrypted_profile_pii = profile.decrypt_pii(password) + decrypted_profile_recovery_pii = profile.recover_pii(personal_key) + + expect(pii).to eq(decrypted_profile_pii) + expect(pii).to eq(decrypted_profile_recovery_pii) + expect(user.valid_personal_key?(personal_key)).to eq(true) + end + end + + context 'when the user has an active and a pending profile' do + let(:active_pii) { pii } + let(:active_profile) { profile } + let(:pending_pii) { Pii::Attributes.new(ssn: '5555') } + let(:pending_profile) do + create( + :profile, + :verify_by_mail_pending, + :verified, + pii: pending_pii.to_h, + user: user, + ) + end + + before do + Pii::Cacher.new(user, user_session).save_decrypted_pii(pending_pii, pending_profile.id) + end + + it 'encrypts the PII for both profiles with the password' do + encryptor = UserProfilesEncryptor.new(user, user_session, password) + encryptor.call + + active_profile.reload + pending_profile.reload + + decrypted_active_profile_pii = active_profile.decrypt_pii(password) + decrypted_pending_profile_pii = pending_profile.decrypt_pii(password) + + expect(decrypted_active_profile_pii).to eq(active_pii) + expect(decrypted_pending_profile_pii).to eq(pending_pii) + end + + it 'sets the pending profile personal key as the personal key' do + encryptor = UserProfilesEncryptor.new(user, user_session, password) + encryptor.call + + active_profile.reload + pending_profile.reload + + personal_key = PersonalKeyGenerator.new(user).normalize(encryptor.personal_key) + + expect do + active_profile.recover_pii(personal_key) + end.to raise_error(Encryption::EncryptionError) + + decrypted_pending_profile_recovery_pii = pending_profile.recover_pii(personal_key) + expect(decrypted_pending_profile_recovery_pii).to eq(pending_pii) + + expect(user.valid_personal_key?(personal_key)).to eq(true) + end + end + end +end From 07154bf77e7d996585336b6f9c59a8c7e0cfdd49 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Fri, 17 Nov 2023 10:29:13 -0500 Subject: [PATCH 2/2] quick cleanup --- app/forms/update_user_password_form.rb | 8 +++-- app/services/user_profiles_encryptor.rb | 4 +-- spec/forms/update_user_password_form_spec.rb | 12 +++---- spec/services/user_profiles_encryptor_spec.rb | 32 ++++++++++++++----- 4 files changed, 38 insertions(+), 18 deletions(-) diff --git a/app/forms/update_user_password_form.rb b/app/forms/update_user_password_form.rb index ef9a7a3188c..9bda95c022e 100644 --- a/app/forms/update_user_password_form.rb +++ b/app/forms/update_user_password_form.rb @@ -35,11 +35,15 @@ def update_user_password def encrypt_user_profiles return if user.active_or_pending_profile.blank? - encryptor.call + encryptor.encrypt end def encryptor - @encryptor ||= UserProfilesEncryptor.new(user, user_session, password) + @encryptor ||= UserProfilesEncryptor.new( + user: user, + user_session: user_session, + password: password, + ) end def extra_analytics_attributes diff --git a/app/services/user_profiles_encryptor.rb b/app/services/user_profiles_encryptor.rb index 9c6e8e4bfff..5f86d3bf58a 100644 --- a/app/services/user_profiles_encryptor.rb +++ b/app/services/user_profiles_encryptor.rb @@ -1,13 +1,13 @@ class UserProfilesEncryptor attr_reader :personal_key - def initialize(user, user_session, password) + def initialize(user:, user_session:, password:) @user = user @user_session = user_session @password = password end - def call + def encrypt if user.active_profile.present? encrypt_pii_for_profile(user.active_profile) end diff --git a/spec/forms/update_user_password_form_spec.rb b/spec/forms/update_user_password_form_spec.rb index 92453925365..6778e84fd35 100644 --- a/spec/forms/update_user_password_form_spec.rb +++ b/spec/forms/update_user_password_form_spec.rb @@ -79,12 +79,12 @@ it 'encrypts the active profile' do encryptor = instance_double(UserProfilesEncryptor) allow(UserProfilesEncryptor).to receive(:new). - with(user, user_session, password).and_return(encryptor) - allow(encryptor).to receive(:call) + with(user: user, user_session: user_session, password: password).and_return(encryptor) + allow(encryptor).to receive(:encrypt) subject.submit(params) - expect(encryptor).to have_received(:call) + expect(encryptor).to have_received(:encrypt) end it 'logs that the user has an active profile' do @@ -109,12 +109,12 @@ it 'encrypts the pending profile' do encryptor = instance_double(UserProfilesEncryptor) allow(UserProfilesEncryptor).to receive(:new). - with(user, user_session, password).and_return(encryptor) - allow(encryptor).to receive(:call) + with(user: user, user_session: user_session, password: password).and_return(encryptor) + allow(encryptor).to receive(:encrypt) subject.submit(params) - expect(encryptor).to have_received(:call) + expect(encryptor).to have_received(:encrypt) end it 'logs that the user has a pending profile' do diff --git a/spec/services/user_profiles_encryptor_spec.rb b/spec/services/user_profiles_encryptor_spec.rb index 70e0b15a3bd..4602d8fb5d6 100644 --- a/spec/services/user_profiles_encryptor_spec.rb +++ b/spec/services/user_profiles_encryptor_spec.rb @@ -14,8 +14,12 @@ context 'when the user has an active profile' do it 'encrypts the PII for the active profile with the password' do - encryptor = UserProfilesEncryptor.new(user, user_session, password) - encryptor.call + encryptor = UserProfilesEncryptor.new( + user: user, + user_session: user_session, + password: password, + ) + encryptor.encrypt profile.reload @@ -34,8 +38,12 @@ let(:profile) { create(:profile, :verify_by_mail_pending, :verified, pii: pii.to_h) } it 'encrypts the PII for the pending profile with the password' do - encryptor = UserProfilesEncryptor.new(user, user_session, password) - encryptor.call + encryptor = UserProfilesEncryptor.new( + user: user, + user_session: user_session, + password: password, + ) + encryptor.encrypt profile.reload @@ -69,8 +77,12 @@ end it 'encrypts the PII for both profiles with the password' do - encryptor = UserProfilesEncryptor.new(user, user_session, password) - encryptor.call + encryptor = UserProfilesEncryptor.new( + user: user, + user_session: user_session, + password: password, + ) + encryptor.encrypt active_profile.reload pending_profile.reload @@ -83,8 +95,12 @@ end it 'sets the pending profile personal key as the personal key' do - encryptor = UserProfilesEncryptor.new(user, user_session, password) - encryptor.call + encryptor = UserProfilesEncryptor.new( + user: user, + user_session: user_session, + password: password, + ) + encryptor.encrypt active_profile.reload pending_profile.reload