From 67ffa13f606570ff83cdb3abbbb4ba6e71609368 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Tue, 7 Jan 2025 09:40:07 -0500 Subject: [PATCH 01/30] changelog: Upcoming Features, Authentication, allow all_emails and email to return last sign in email --- .../connected_accounts/selected_email_controller.rb | 12 ++++++++++-- app/controllers/sign_up/select_email_controller.rb | 10 +++++++++- app/models/service_provider_identity.rb | 4 ++++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/app/controllers/accounts/connected_accounts/selected_email_controller.rb b/app/controllers/accounts/connected_accounts/selected_email_controller.rb index 1ae8b77a6fe..68118063f8d 100644 --- a/app/controllers/accounts/connected_accounts/selected_email_controller.rb +++ b/app/controllers/accounts/connected_accounts/selected_email_controller.rb @@ -20,7 +20,7 @@ def edit def update @select_email_form = build_select_email_form - result = @select_email_form.submit(form_params) + result = @select_email_form.submit(selected_email_id: selected_email_id) analytics.sp_select_email_submitted(**result) @@ -52,7 +52,15 @@ def identity @identity = current_user.identities.find_by(id: params[:identity_id]) end - def last_email + def selected_email_id + if current_sp.present? && current_sp.metadata[:all_emails] && current_sp.metadata[:emails] + last_email_id + else + form_params[:selected_email_id] + end + end + + def last_email_id current_user.last_sign_in_email_address.id end end diff --git a/app/controllers/sign_up/select_email_controller.rb b/app/controllers/sign_up/select_email_controller.rb index 2abfff30c60..0b23b19cbf9 100644 --- a/app/controllers/sign_up/select_email_controller.rb +++ b/app/controllers/sign_up/select_email_controller.rb @@ -25,7 +25,7 @@ def create analytics.sp_select_email_submitted(**result, needs_completion_screen_reason:) if result.success? - user_session[:selected_email_id_for_linked_identity] = form_params[:selected_email_id] + user_session[:selected_email_id_for_linked_identity] = selected_email_id redirect_to sign_up_completed_path else flash[:error] = result.first_error_message @@ -55,6 +55,14 @@ def last_email end end + def selected_email_id + if current_sp.present? && current_sp.metadata[:all_emails] && current_sp.metadata[:emails] + current_user.last_sign_in_email_address.email.id + else + form_params[:selected_email_id] + end + end + def verify_needs_completions_screen redirect_to account_url unless needs_completion_screen_reason end diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index 5ad3eaf6ede..b023fd286bc 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -57,6 +57,10 @@ def friendly_name sp_metadata[:friendly_name] end + def all_email_and_single_email_requested? + current_sp.metadata[:all_emails] && current_sp.metadata[:emails] + end + def service_provider_id service_provider_record&.id end From 1cf573d88af9a18301d6c9c29ef9dd84e11e8557 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Tue, 7 Jan 2025 21:23:04 -0500 Subject: [PATCH 02/30] add spec for identity check --- .../sign_up/select_email_controller.rb | 6 ++-- app/models/service_provider_identity.rb | 3 +- .../selected_email_controller_spec.rb | 34 ++++++++++++++++-- .../sign_up/select_email_controller_spec.rb | 35 +++++++++++++++---- 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/app/controllers/sign_up/select_email_controller.rb b/app/controllers/sign_up/select_email_controller.rb index 0b23b19cbf9..faa60281ca1 100644 --- a/app/controllers/sign_up/select_email_controller.rb +++ b/app/controllers/sign_up/select_email_controller.rb @@ -56,8 +56,10 @@ def last_email end def selected_email_id - if current_sp.present? && current_sp.metadata[:all_emails] && current_sp.metadata[:emails] - current_user.last_sign_in_email_address.email.id + if current_sp.present? && + current_sp.metadata[:attribute_bundle].include?('all_email') && + current_sp.metadata[:attribute_bundle].include?('email') + current_user.last_sign_in_email_address.id else form_params[:selected_email_id] end diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index b023fd286bc..f43a0cdead9 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -58,7 +58,8 @@ def friendly_name end def all_email_and_single_email_requested? - current_sp.metadata[:all_emails] && current_sp.metadata[:emails] + sp_metadata[:attribute_bundle].include?('all_email') && + sp_metadata[:attribute_bundle].include?('email') end def service_provider_id diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index 53826485151..d1f30d256ca 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -89,8 +89,8 @@ describe '#update' do let(:identity_id) { user.identities.take.id } - let(:selected_email) { user.confirmed_email_addresses.sample } - let(:params) { { identity_id:, select_email_form: { selected_email_id: selected_email.id } } } + let(:selected_email_id) { user.confirmed_email_addresses.sample } + let(:params) { { identity_id:, select_email_form: { selected_email_id: selected_email_id } } } subject(:response) { patch :update, params: } it 'redirects to connected accounts path with the appropriate flash message' do @@ -106,10 +106,38 @@ expect(@analytics).to have_logged_event( :sp_select_email_submitted, success: true, - selected_email_id: selected_email.id, + selected_email_id: selected_email_id, ) end + context ' with all_email and emails requested' do + let(:service_provider_attribute_bundle) { %w[email all_email] } + + let(:sp) do + create( + :service_provider, + attribute_bundle: service_provider_attribute_bundle, + ) + end + let(:identity) do + create(:service_provider_identity, :active, service_provider: sp.issuer, user: user) + end + + let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } + let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } + let(:selected_email_id) do + (available_email_ids - [last_sign_in_email_id]).sample + end + + it 'returns last sign in email' do + response + + expect( + controller.user_session[:selected_email_id_for_linked_identity], + ).to eq(last_sign_in_email_id) + end + end + context 'with invalid submission' do let(:params) { super().merge(select_email_form: { selected_email_id: '' }) } diff --git a/spec/controllers/sign_up/select_email_controller_spec.rb b/spec/controllers/sign_up/select_email_controller_spec.rb index 7eefddb0368..729d3b7747a 100644 --- a/spec/controllers/sign_up/select_email_controller_spec.rb +++ b/spec/controllers/sign_up/select_email_controller_spec.rb @@ -2,7 +2,13 @@ RSpec.describe SignUp::SelectEmailController do let(:user) { create(:user, :with_multiple_emails) } - let(:sp) { create(:service_provider) } + let(:service_provider_attribute_bundle) { %w[email] } + let(:sp) do + create( + :service_provider, + attribute_bundle: service_provider_attribute_bundle, + ) + end before do stub_sign_in(user) @@ -75,8 +81,8 @@ end describe '#create' do - let(:selected_email) { user.confirmed_email_addresses.sample } - let(:params) { { select_email_form: { selected_email_id: selected_email.id } } } + let(:selected_email_id) { user.confirmed_email_addresses.sample.id } + let(:params) { { select_email_form: { selected_email_id: selected_email_id } } } subject(:response) { post :create, params: params } @@ -85,7 +91,7 @@ expect( controller.user_session[:selected_email_id_for_linked_identity], - ).to eq(selected_email.id.to_s) + ).to eq(selected_email_id.to_s) end it 'logs analytics event' do @@ -97,10 +103,27 @@ :sp_select_email_submitted, success: true, needs_completion_screen_reason: :new_attributes, - selected_email_id: selected_email.id, + selected_email_id: selected_email_id, ) end + context ' with all_email and emails requested' do + let(:service_provider_attribute_bundle) { %w[email all_email] } + let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } + let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } + let(:selected_email_id) do + (available_email_ids - [last_sign_in_email_id]).sample + end + + it 'returns last sign in email' do + response + + expect( + controller.user_session[:selected_email_id_for_linked_identity], + ).to eq(last_sign_in_email_id) + end + end + context 'with a corrupted email selected_email_id form' do let(:other_user) { create(:user) } let(:selected_email) { other_user.confirmed_email_addresses.sample } @@ -122,7 +145,7 @@ success: false, error_details: { selected_email_id: { not_found: true } }, needs_completion_screen_reason: :new_attributes, - selected_email_id: selected_email.id, + selected_email_id: selected_email_id, ) end end From b5c52d21fafda9cae80fadc81bd8644643cd8cca Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Wed, 8 Jan 2025 09:34:53 -0500 Subject: [PATCH 03/30] update selected_email controller and spec --- .../accounts/connected_accounts/selected_email_controller.rb | 2 +- .../connected_accounts/selected_email_controller_spec.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/accounts/connected_accounts/selected_email_controller.rb b/app/controllers/accounts/connected_accounts/selected_email_controller.rb index 68118063f8d..0c9b61747c2 100644 --- a/app/controllers/accounts/connected_accounts/selected_email_controller.rb +++ b/app/controllers/accounts/connected_accounts/selected_email_controller.rb @@ -14,7 +14,7 @@ def edit @select_email_form = build_select_email_form @can_add_email = EmailPolicy.new(current_user).can_add_email? analytics.sp_select_email_visited - @email_id = @identity.email_address_id || last_email + @email_id = @identity.email_address_id || last_email_id end def update diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index d1f30d256ca..265c08efcf2 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -135,6 +135,8 @@ expect( controller.user_session[:selected_email_id_for_linked_identity], ).to eq(last_sign_in_email_id) + identity.reload + expect(identity.email_address_id).to eq(last_sign_in_email_id) end end From 3348e2a6df79a4c42a38c5b3c9a5338fa53adb46 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Wed, 8 Jan 2025 11:10:19 -0500 Subject: [PATCH 04/30] fix selected email working for all email and emails --- .../connected_accounts/selected_email_controller.rb | 2 +- app/controllers/sign_up/select_email_controller.rb | 4 ++-- app/models/service_provider_identity.rb | 4 ++-- .../selected_email_controller_spec.rb | 13 +++++++------ 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/app/controllers/accounts/connected_accounts/selected_email_controller.rb b/app/controllers/accounts/connected_accounts/selected_email_controller.rb index 0c9b61747c2..40d240e7299 100644 --- a/app/controllers/accounts/connected_accounts/selected_email_controller.rb +++ b/app/controllers/accounts/connected_accounts/selected_email_controller.rb @@ -53,7 +53,7 @@ def identity end def selected_email_id - if current_sp.present? && current_sp.metadata[:all_emails] && current_sp.metadata[:emails] + if identity.all_email_and_single_email_requested? last_email_id else form_params[:selected_email_id] diff --git a/app/controllers/sign_up/select_email_controller.rb b/app/controllers/sign_up/select_email_controller.rb index faa60281ca1..0cd97c71966 100644 --- a/app/controllers/sign_up/select_email_controller.rb +++ b/app/controllers/sign_up/select_email_controller.rb @@ -57,8 +57,8 @@ def last_email def selected_email_id if current_sp.present? && - current_sp.metadata[:attribute_bundle].include?('all_email') && - current_sp.metadata[:attribute_bundle].include?('email') + current_sp.attribute_bundle&.include?('all_email') && + current_sp.attribute_bundle.include?('email') current_user.last_sign_in_email_address.id else form_params[:selected_email_id] diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index f43a0cdead9..b7047ddb0f0 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -58,8 +58,8 @@ def friendly_name end def all_email_and_single_email_requested? - sp_metadata[:attribute_bundle].include?('all_email') && - sp_metadata[:attribute_bundle].include?('email') + service_provider_record&.attribute_bundle&.include?('all_email') && + service_provider_record&.attribute_bundle.include?('email') end def service_provider_id diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index 265c08efcf2..45afba3531f 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -89,7 +89,7 @@ describe '#update' do let(:identity_id) { user.identities.take.id } - let(:selected_email_id) { user.confirmed_email_addresses.sample } + let(:selected_email_id) { user.confirmed_email_addresses.sample.id } let(:params) { { identity_id:, select_email_form: { selected_email_id: selected_email_id } } } subject(:response) { patch :update, params: } @@ -120,7 +120,7 @@ ) end let(:identity) do - create(:service_provider_identity, :active, service_provider: sp.issuer, user: user) + create(:service_provider_identity, :active, service_provider: sp.issuer) end let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } @@ -129,12 +129,13 @@ (available_email_ids - [last_sign_in_email_id]).sample end + before do + identity.update!(user_id: user.id) + end + it 'returns last sign in email' do response - expect( - controller.user_session[:selected_email_id_for_linked_identity], - ).to eq(last_sign_in_email_id) identity.reload expect(identity.email_address_id).to eq(last_sign_in_email_id) end @@ -163,7 +164,7 @@ context 'signed out' do let(:other_user) { create(:user, identities: [create(:service_provider_identity, :active)]) } - let(:selected_email) { other_user.confirmed_email_addresses.sample } + let(:selected_email_id) { other_user.confirmed_email_addresses.sample.id } let(:identity_id) { other_user.identities.take.id } let(:user) { nil } From 321b9c334de1f3636dcd9939cea3a5d80b149172 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Wed, 8 Jan 2025 11:58:59 -0500 Subject: [PATCH 05/30] fix select email controller --- spec/controllers/sign_up/select_email_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/sign_up/select_email_controller_spec.rb b/spec/controllers/sign_up/select_email_controller_spec.rb index 729d3b7747a..7a8e3b37ea6 100644 --- a/spec/controllers/sign_up/select_email_controller_spec.rb +++ b/spec/controllers/sign_up/select_email_controller_spec.rb @@ -126,7 +126,7 @@ context 'with a corrupted email selected_email_id form' do let(:other_user) { create(:user) } - let(:selected_email) { other_user.confirmed_email_addresses.sample } + let(:selected_email_id) { other_user.confirmed_email_addresses.sample.id } it 'rejects email not belonging to the user' do expect(response).to redirect_to(sign_up_select_email_path) From 8c1833aeda35ada17096a579f299aa376de78a0a Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Mon, 13 Jan 2025 09:30:36 -0500 Subject: [PATCH 06/30] change ot proper attribute --- app/controllers/sign_up/select_email_controller.rb | 2 +- app/models/service_provider_identity.rb | 2 +- .../connected_accounts/selected_email_controller_spec.rb | 4 ++-- spec/controllers/sign_up/select_email_controller_spec.rb | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/sign_up/select_email_controller.rb b/app/controllers/sign_up/select_email_controller.rb index 0cd97c71966..39412d8e899 100644 --- a/app/controllers/sign_up/select_email_controller.rb +++ b/app/controllers/sign_up/select_email_controller.rb @@ -57,7 +57,7 @@ def last_email def selected_email_id if current_sp.present? && - current_sp.attribute_bundle&.include?('all_email') && + current_sp.attribute_bundle&.include?('all_emails') && current_sp.attribute_bundle.include?('email') current_user.last_sign_in_email_address.id else diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index b7047ddb0f0..2fd0d64018e 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -58,7 +58,7 @@ def friendly_name end def all_email_and_single_email_requested? - service_provider_record&.attribute_bundle&.include?('all_email') && + service_provider_record&.attribute_bundle&.include?('all_emails') && service_provider_record&.attribute_bundle.include?('email') end diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index 45afba3531f..b7c729e21ca 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -110,8 +110,8 @@ ) end - context ' with all_email and emails requested' do - let(:service_provider_attribute_bundle) { %w[email all_email] } + context ' with all_emails and emails requested' do + let(:service_provider_attribute_bundle) { %w[email all_emails] } let(:sp) do create( diff --git a/spec/controllers/sign_up/select_email_controller_spec.rb b/spec/controllers/sign_up/select_email_controller_spec.rb index 7a8e3b37ea6..07a41e0e5bc 100644 --- a/spec/controllers/sign_up/select_email_controller_spec.rb +++ b/spec/controllers/sign_up/select_email_controller_spec.rb @@ -108,7 +108,7 @@ end context ' with all_email and emails requested' do - let(:service_provider_attribute_bundle) { %w[email all_email] } + let(:service_provider_attribute_bundle) { %w[email all_emails] } let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } let(:selected_email_id) do From 21760807fc90448ef75248b98f535e5fb5d999a7 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Mon, 13 Jan 2025 11:58:56 -0500 Subject: [PATCH 07/30] update server identity and select email controller to only look for all_emails --- app/controllers/sign_up/select_email_controller.rb | 4 +--- app/models/service_provider_identity.rb | 3 +-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/app/controllers/sign_up/select_email_controller.rb b/app/controllers/sign_up/select_email_controller.rb index 39412d8e899..4fa0d5468ef 100644 --- a/app/controllers/sign_up/select_email_controller.rb +++ b/app/controllers/sign_up/select_email_controller.rb @@ -56,9 +56,7 @@ def last_email end def selected_email_id - if current_sp.present? && - current_sp.attribute_bundle&.include?('all_emails') && - current_sp.attribute_bundle.include?('email') + if current_sp&.attribute_bundle&.include?('all_emails') current_user.last_sign_in_email_address.id else form_params[:selected_email_id] diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index 2fd0d64018e..32ef48b9000 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -58,8 +58,7 @@ def friendly_name end def all_email_and_single_email_requested? - service_provider_record&.attribute_bundle&.include?('all_emails') && - service_provider_record&.attribute_bundle.include?('email') + service_provider_record&.attribute_bundle&.include?('all_emails') end def service_provider_id From 4574c6f57916a04996ab591c01732c7f205faf55 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Mon, 13 Jan 2025 13:42:30 -0500 Subject: [PATCH 08/30] move to check if all_emails and email given work --- .../selected_email_controller_spec.rb | 31 +++++++++++++++++++ .../sign_up/select_email_controller_spec.rb | 19 +++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index b7c729e21ca..9a4acc78c17 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -141,6 +141,37 @@ end end + context ' with all_emails requested' do + let(:service_provider_attribute_bundle) { %w[all_emails] } + + let(:sp) do + create( + :service_provider, + attribute_bundle: service_provider_attribute_bundle, + ) + end + let(:identity) do + create(:service_provider_identity, :active, service_provider: sp.issuer) + end + + let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } + let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } + let(:selected_email_id) do + (available_email_ids - [last_sign_in_email_id]).sample + end + + before do + identity.update!(user_id: user.id) + end + + it 'returns last sign in email' do + response + + identity.reload + expect(identity.email_address_id).to eq(last_sign_in_email_id) + end + end + context 'with invalid submission' do let(:params) { super().merge(select_email_form: { selected_email_id: '' }) } diff --git a/spec/controllers/sign_up/select_email_controller_spec.rb b/spec/controllers/sign_up/select_email_controller_spec.rb index 07a41e0e5bc..9ae9cf6635d 100644 --- a/spec/controllers/sign_up/select_email_controller_spec.rb +++ b/spec/controllers/sign_up/select_email_controller_spec.rb @@ -107,7 +107,7 @@ ) end - context ' with all_email and emails requested' do + context ' with all_email and emails requested for SP' do let(:service_provider_attribute_bundle) { %w[email all_emails] } let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } @@ -124,6 +124,23 @@ end end + context ' with all_emails requested for SP' do + let(:service_provider_attribute_bundle) { %w[all_emails] } + let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } + let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } + let(:selected_email_id) do + (available_email_ids - [last_sign_in_email_id]).sample + end + + it 'returns last sign in email' do + response + + expect( + controller.user_session[:selected_email_id_for_linked_identity], + ).to eq(last_sign_in_email_id) + end + end + context 'with a corrupted email selected_email_id form' do let(:other_user) { create(:user) } let(:selected_email_id) { other_user.confirmed_email_addresses.sample.id } From cd4a35d20e2b8166f19d237904dee7bfe505ba9e Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Tue, 14 Jan 2025 10:07:02 -0500 Subject: [PATCH 09/30] selected email returns proper attributes --- .../connected_accounts/selected_email_controller.rb | 4 +--- .../openid_connect/authorization_controller.rb | 10 +++++----- app/controllers/sign_up/select_email_controller.rb | 5 ++--- app/models/service_provider_identity.rb | 13 ++++++++++--- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/app/controllers/accounts/connected_accounts/selected_email_controller.rb b/app/controllers/accounts/connected_accounts/selected_email_controller.rb index 40d240e7299..9fc28a837f3 100644 --- a/app/controllers/accounts/connected_accounts/selected_email_controller.rb +++ b/app/controllers/accounts/connected_accounts/selected_email_controller.rb @@ -53,9 +53,7 @@ def identity end def selected_email_id - if identity.all_email_and_single_email_requested? - last_email_id - else + if identity.sp_only_single_email_requested? form_params[:selected_email_id] end end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 1480697d6b6..ccd9a26d775 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -89,12 +89,12 @@ def link_identity_to_service_provider end def email_address_id - return nil unless IdentityConfig.store.feature_select_email_to_share_enabled - if user_session[:selected_email_id_for_linked_identity].present? - return user_session[:selected_email_id_for_linked_identity] - end + return nil unless IdentityConfig.store.feature_select_email_to_share_enabled && + if user_session[:selected_email_id_for_linked_identity].present? + return user_session[:selected_email_id_for_linked_identity] + end identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) - identity&.email_address_id + identity&.email_address_for_sharing&.id end def ial_context diff --git a/app/controllers/sign_up/select_email_controller.rb b/app/controllers/sign_up/select_email_controller.rb index 4fa0d5468ef..61f91e6b61c 100644 --- a/app/controllers/sign_up/select_email_controller.rb +++ b/app/controllers/sign_up/select_email_controller.rb @@ -56,9 +56,8 @@ def last_email end def selected_email_id - if current_sp&.attribute_bundle&.include?('all_emails') - current_user.last_sign_in_email_address.id - else + if current_sp&.attribute_bundle&.include?('email') && + !current_sp&.attribute_bundle&.include?('all_emails') form_params[:selected_email_id] end end diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index 32ef48b9000..e6ecde503c0 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -57,8 +57,9 @@ def friendly_name sp_metadata[:friendly_name] end - def all_email_and_single_email_requested? - service_provider_record&.attribute_bundle&.include?('all_emails') + def sp_only_single_email_requested? + service_provider_record&.attribute_bundle&.include?('email') && + !service_provider_record&.attribute_bundle&.include?('all_emails') end def service_provider_id @@ -70,9 +71,15 @@ def happened_at end def email_address_for_sharing - if IdentityConfig.store.feature_select_email_to_share_enabled && email_address + if use_stored_email_address? return email_address end user.last_sign_in_email_address end + + def use_stored_email_address? + IdentityConfig.store.feature_select_email_to_share_enabled && + email_address && + sp_only_single_email_requested? + end end From 40ddc2116a31611adcbbfa5a392f687632a1bab5 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Tue, 14 Jan 2025 11:00:11 -0500 Subject: [PATCH 10/30] fix select email specs --- .../selected_email_controller_spec.rb | 40 +++++++++---------- .../sign_up/select_email_controller_spec.rb | 18 ++------- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index 9a4acc78c17..c2fb2c892c7 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -89,8 +89,22 @@ describe '#update' do let(:identity_id) { user.identities.take.id } + let(:service_provider_attribute_bundle) { %w[email] } let(:selected_email_id) { user.confirmed_email_addresses.sample.id } let(:params) { { identity_id:, select_email_form: { selected_email_id: selected_email_id } } } + let(:sp) do + create( + :service_provider, + attribute_bundle: service_provider_attribute_bundle, + ) + end + before do + identity = ServiceProviderIdentity.find(identity_id) + identity.user_id = user&.id + identity.service_provider = sp.issuer + identity.save! + end + subject(:response) { patch :update, params: } it 'redirects to connected accounts path with the appropriate flash message' do @@ -113,31 +127,19 @@ context ' with all_emails and emails requested' do let(:service_provider_attribute_bundle) { %w[email all_emails] } - let(:sp) do - create( - :service_provider, - attribute_bundle: service_provider_attribute_bundle, - ) - end let(:identity) do create(:service_provider_identity, :active, service_provider: sp.issuer) end - let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } - let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } - let(:selected_email_id) do - (available_email_ids - [last_sign_in_email_id]).sample - end - before do identity.update!(user_id: user.id) end - it 'returns last sign in email' do + it 'returns nil' do response identity.reload - expect(identity.email_address_id).to eq(last_sign_in_email_id) + expect(identity.email_address_id).to eq(nil) end end @@ -154,21 +156,15 @@ create(:service_provider_identity, :active, service_provider: sp.issuer) end - let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } - let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } - let(:selected_email_id) do - (available_email_ids - [last_sign_in_email_id]).sample - end - before do identity.update!(user_id: user.id) end - it 'returns last sign in email' do + it 'returns nil' do response identity.reload - expect(identity.email_address_id).to eq(last_sign_in_email_id) + expect(identity.email_address_id).to eq(nil) end end diff --git a/spec/controllers/sign_up/select_email_controller_spec.rb b/spec/controllers/sign_up/select_email_controller_spec.rb index 9ae9cf6635d..08784b4c029 100644 --- a/spec/controllers/sign_up/select_email_controller_spec.rb +++ b/spec/controllers/sign_up/select_email_controller_spec.rb @@ -109,35 +109,25 @@ context ' with all_email and emails requested for SP' do let(:service_provider_attribute_bundle) { %w[email all_emails] } - let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } - let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } - let(:selected_email_id) do - (available_email_ids - [last_sign_in_email_id]).sample - end - it 'returns last sign in email' do + it 'returns nil' do response expect( controller.user_session[:selected_email_id_for_linked_identity], - ).to eq(last_sign_in_email_id) + ).to eq(nil) end end context ' with all_emails requested for SP' do let(:service_provider_attribute_bundle) { %w[all_emails] } - let(:last_sign_in_email_id) { user.last_sign_in_email_address.id } - let(:available_email_ids) { user.confirmed_email_addresses.map(&:id) } - let(:selected_email_id) do - (available_email_ids - [last_sign_in_email_id]).sample - end - it 'returns last sign in email' do + it 'returns nil' do response expect( controller.user_session[:selected_email_id_for_linked_identity], - ).to eq(last_sign_in_email_id) + ).to eq(nil) end end From 7681ca4ac0793c37ee014986f812199e8297d3ea Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Tue, 14 Jan 2025 13:17:38 -0500 Subject: [PATCH 11/30] update service identity spec --- spec/models/service_provider_identity_spec.rb | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/spec/models/service_provider_identity_spec.rb b/spec/models/service_provider_identity_spec.rb index 54015505d9d..f1cf7bd0eea 100644 --- a/spec/models/service_provider_identity_spec.rb +++ b/spec/models/service_provider_identity_spec.rb @@ -201,8 +201,21 @@ ) end + let(:service_provider_attribute_bundle) { %w[email] } + let(:service_provider) do + create( + :service_provider, + attribute_bundle: service_provider_attribute_bundle, + ) + end + let(:identity) do - create(:service_provider_identity, user: user, session_uuid: SecureRandom.uuid) + create( + :service_provider_identity, + user: user, + session_uuid: SecureRandom.uuid, + service_provider: service_provider.issuer, + ) end context 'when email sharing feature is enabled' do @@ -211,7 +224,8 @@ .and_return(true) end - context 'when an email address for sharing has been set' do + context 'when an email address is set and service provider has email attribute' do + let(:service_provider_attribute_bundle) { %w[email] } before do identity.email_address = shared_email_address end @@ -221,6 +235,30 @@ end end + context 'when service provider has both email and all_emails attribute' do + let(:service_provider_attribute_bundle) { %w[email all_emails] } + + before do + identity.email_address = shared_email_address + end + + it 'returns the last sign in email' do + expect(identity.email_address_for_sharing).to eq(last_login_email_address) + end + end + + context 'when service provider has no email attributes' do + let(:service_provider_attribute_bundle) { %w[first_name last_name] } + + before do + identity.email_address = shared_email_address + end + + it 'returns the last sign in email' do + expect(identity.email_address_for_sharing).to eq(last_login_email_address) + end + end + context 'when an email address for sharing has not been set' do before do identity.email_address = nil From 5cc5b0317b4bafc63a0fd2acc0ff2878daab1052 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Wed, 15 Jan 2025 15:17:45 -0500 Subject: [PATCH 12/30] rework to use identity verified attributes and session attributes --- app/controllers/sign_up/select_email_controller.rb | 4 ++-- app/models/service_provider_identity.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/sign_up/select_email_controller.rb b/app/controllers/sign_up/select_email_controller.rb index 61f91e6b61c..4bf41a65af7 100644 --- a/app/controllers/sign_up/select_email_controller.rb +++ b/app/controllers/sign_up/select_email_controller.rb @@ -56,8 +56,8 @@ def last_email end def selected_email_id - if current_sp&.attribute_bundle&.include?('email') && - !current_sp&.attribute_bundle&.include?('all_emails') + if decorated_sp_session&.requested_attributes&.include?('email') && + !decorated_sp_session&.requested_attributes&.include?('all_emails') form_params[:selected_email_id] end end diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index e6ecde503c0..277d44bba29 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -58,8 +58,8 @@ def friendly_name end def sp_only_single_email_requested? - service_provider_record&.attribute_bundle&.include?('email') && - !service_provider_record&.attribute_bundle&.include?('all_emails') + verified_attributes&.include?('email') && + !verified_attributes.include?('all_emails') end def service_provider_id From ff3962116cf9c44c2ca755ac3f39eb51ac8154e7 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Thu, 16 Jan 2025 10:45:31 -0500 Subject: [PATCH 13/30] move back authorization controller --- .../authorization_controller.rb | 8 +++---- app/models/service_provider_identity.rb | 3 ++- db/schema.rb | 22 +------------------ 3 files changed, 7 insertions(+), 26 deletions(-) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index ccd9a26d775..f88161ad800 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -89,10 +89,10 @@ def link_identity_to_service_provider end def email_address_id - return nil unless IdentityConfig.store.feature_select_email_to_share_enabled && - if user_session[:selected_email_id_for_linked_identity].present? - return user_session[:selected_email_id_for_linked_identity] - end + return nil unless IdentityConfig.store.feature_select_email_to_share_enabled + if user_session[:selected_email_id_for_linked_identity].present? + return user_session[:selected_email_id_for_linked_identity] + end identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) identity&.email_address_for_sharing&.id end diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index 277d44bba29..b90667ffea8 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -71,7 +71,8 @@ def happened_at end def email_address_for_sharing - if use_stored_email_address? + if IdentityConfig.store.feature_select_email_to_share_enabled && + email_address return email_address end user.last_sign_in_email_address diff --git a/db/schema.rb b/db/schema.rb index 135f2b0231c..376cc842b52 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2024_12_03_163014) do +ActiveRecord::Schema[7.2].define(version: 2025_01_06_232958) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "pg_stat_statements" @@ -471,26 +471,6 @@ t.index ["user_id"], name: "index_profiles_on_user_id" end - create_table "proofing_components", force: :cascade do |t| - t.integer "user_id", null: false, comment: "sensitive=false" - t.string "document_check", comment: "sensitive=false" - t.string "document_type", comment: "sensitive=false" - t.string "source_check", comment: "sensitive=false" - t.string "resolution_check", comment: "sensitive=false" - t.string "address_check", comment: "sensitive=false" - t.datetime "verified_at", precision: nil, comment: "sensitive=false" - t.datetime "created_at", precision: nil, null: false, comment: "sensitive=false" - t.datetime "updated_at", precision: nil, null: false, comment: "sensitive=false" - t.string "liveness_check", comment: "sensitive=false" - t.string "device_fingerprinting_vendor", comment: "sensitive=false" - t.boolean "threatmetrix", comment: "sensitive=false" - t.string "threatmetrix_review_status", comment: "sensitive=false" - t.string "threatmetrix_risk_rating", comment: "sensitive=false" - t.string "threatmetrix_policy_score", comment: "sensitive=false" - t.index ["user_id"], name: "index_proofing_components_on_user_id", unique: true - t.index ["verified_at"], name: "index_proofing_components_on_verified_at" - end - create_table "registration_logs", force: :cascade do |t| t.integer "user_id", null: false, comment: "sensitive=false" t.datetime "registered_at", precision: nil, comment: "sensitive=false" From a537d917fc214a9dfd705760c6d18805e4935d84 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Thu, 16 Jan 2025 12:02:31 -0500 Subject: [PATCH 14/30] fix migration --- db/schema.rb | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 376cc842b52..135f2b0231c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_01_06_232958) do +ActiveRecord::Schema[7.2].define(version: 2024_12_03_163014) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "pg_stat_statements" @@ -471,6 +471,26 @@ t.index ["user_id"], name: "index_profiles_on_user_id" end + create_table "proofing_components", force: :cascade do |t| + t.integer "user_id", null: false, comment: "sensitive=false" + t.string "document_check", comment: "sensitive=false" + t.string "document_type", comment: "sensitive=false" + t.string "source_check", comment: "sensitive=false" + t.string "resolution_check", comment: "sensitive=false" + t.string "address_check", comment: "sensitive=false" + t.datetime "verified_at", precision: nil, comment: "sensitive=false" + t.datetime "created_at", precision: nil, null: false, comment: "sensitive=false" + t.datetime "updated_at", precision: nil, null: false, comment: "sensitive=false" + t.string "liveness_check", comment: "sensitive=false" + t.string "device_fingerprinting_vendor", comment: "sensitive=false" + t.boolean "threatmetrix", comment: "sensitive=false" + t.string "threatmetrix_review_status", comment: "sensitive=false" + t.string "threatmetrix_risk_rating", comment: "sensitive=false" + t.string "threatmetrix_policy_score", comment: "sensitive=false" + t.index ["user_id"], name: "index_proofing_components_on_user_id", unique: true + t.index ["verified_at"], name: "index_proofing_components_on_verified_at" + end + create_table "registration_logs", force: :cascade do |t| t.integer "user_id", null: false, comment: "sensitive=false" t.datetime "registered_at", precision: nil, comment: "sensitive=false" From ff56fa70cd07cbe7814842f17a04c11557f83fff Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Thu, 16 Jan 2025 12:04:58 -0500 Subject: [PATCH 15/30] fix schema --- db/schema.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 135f2b0231c..8ad9b56147d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2024_12_03_163014) do +ActiveRecord::Schema[7.2].define(version: 2025_01_06_232958) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "pg_stat_statements" @@ -490,7 +490,7 @@ t.index ["user_id"], name: "index_proofing_components_on_user_id", unique: true t.index ["verified_at"], name: "index_proofing_components_on_verified_at" end - + create_table "registration_logs", force: :cascade do |t| t.integer "user_id", null: false, comment: "sensitive=false" t.datetime "registered_at", precision: nil, comment: "sensitive=false" From 8c772ddfe6a6f94606c893bc9d5ec40e7b58def5 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Thu, 16 Jan 2025 12:05:30 -0500 Subject: [PATCH 16/30] remove proofing component schema --- db/schema.rb | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 8ad9b56147d..cc14f31e435 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -470,26 +470,6 @@ t.index ["user_id", "active"], name: "index_profiles_on_user_id_and_active", unique: true, where: "(active = true)" t.index ["user_id"], name: "index_profiles_on_user_id" end - - create_table "proofing_components", force: :cascade do |t| - t.integer "user_id", null: false, comment: "sensitive=false" - t.string "document_check", comment: "sensitive=false" - t.string "document_type", comment: "sensitive=false" - t.string "source_check", comment: "sensitive=false" - t.string "resolution_check", comment: "sensitive=false" - t.string "address_check", comment: "sensitive=false" - t.datetime "verified_at", precision: nil, comment: "sensitive=false" - t.datetime "created_at", precision: nil, null: false, comment: "sensitive=false" - t.datetime "updated_at", precision: nil, null: false, comment: "sensitive=false" - t.string "liveness_check", comment: "sensitive=false" - t.string "device_fingerprinting_vendor", comment: "sensitive=false" - t.boolean "threatmetrix", comment: "sensitive=false" - t.string "threatmetrix_review_status", comment: "sensitive=false" - t.string "threatmetrix_risk_rating", comment: "sensitive=false" - t.string "threatmetrix_policy_score", comment: "sensitive=false" - t.index ["user_id"], name: "index_proofing_components_on_user_id", unique: true - t.index ["verified_at"], name: "index_proofing_components_on_verified_at" - end create_table "registration_logs", force: :cascade do |t| t.integer "user_id", null: false, comment: "sensitive=false" From 9241d719ae092c66bf8d8b70f10be503b588381f Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Thu, 16 Jan 2025 12:05:59 -0500 Subject: [PATCH 17/30] remove spacing --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index cc14f31e435..376cc842b52 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -470,7 +470,7 @@ t.index ["user_id", "active"], name: "index_profiles_on_user_id_and_active", unique: true, where: "(active = true)" t.index ["user_id"], name: "index_profiles_on_user_id" end - + create_table "registration_logs", force: :cascade do |t| t.integer "user_id", null: false, comment: "sensitive=false" t.datetime "registered_at", precision: nil, comment: "sensitive=false" From dcb370050bbafa435e17c65b368d32cdd691bbac Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Thu, 16 Jan 2025 16:47:54 -0500 Subject: [PATCH 18/30] fix tests --- .../selected_email_controller_spec.rb | 36 +++---------------- .../sign_up/select_email_controller_spec.rb | 13 +++---- 2 files changed, 10 insertions(+), 39 deletions(-) diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index c2fb2c892c7..1f02ec61496 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -89,19 +89,15 @@ describe '#update' do let(:identity_id) { user.identities.take.id } - let(:service_provider_attribute_bundle) { %w[email] } + let(:verified_attributes) { %w[email] } let(:selected_email_id) { user.confirmed_email_addresses.sample.id } let(:params) { { identity_id:, select_email_form: { selected_email_id: selected_email_id } } } - let(:sp) do - create( - :service_provider, - attribute_bundle: service_provider_attribute_bundle, - ) - end + let(:sp) { create(:service_provider) } before do identity = ServiceProviderIdentity.find(identity_id) identity.user_id = user&.id identity.service_provider = sp.issuer + identity.verified_attributes = verified_attributes identity.save! end @@ -125,15 +121,7 @@ end context ' with all_emails and emails requested' do - let(:service_provider_attribute_bundle) { %w[email all_emails] } - - let(:identity) do - create(:service_provider_identity, :active, service_provider: sp.issuer) - end - - before do - identity.update!(user_id: user.id) - end + let(:verified_attributes) { %w[email all_emails] } it 'returns nil' do response @@ -144,21 +132,7 @@ end context ' with all_emails requested' do - let(:service_provider_attribute_bundle) { %w[all_emails] } - - let(:sp) do - create( - :service_provider, - attribute_bundle: service_provider_attribute_bundle, - ) - end - let(:identity) do - create(:service_provider_identity, :active, service_provider: sp.issuer) - end - - before do - identity.update!(user_id: user.id) - end + let(:verified_attributes) { %w[all_emails] } it 'returns nil' do response diff --git a/spec/controllers/sign_up/select_email_controller_spec.rb b/spec/controllers/sign_up/select_email_controller_spec.rb index 08784b4c029..b74ea5a368c 100644 --- a/spec/controllers/sign_up/select_email_controller_spec.rb +++ b/spec/controllers/sign_up/select_email_controller_spec.rb @@ -2,17 +2,15 @@ RSpec.describe SignUp::SelectEmailController do let(:user) { create(:user, :with_multiple_emails) } - let(:service_provider_attribute_bundle) { %w[email] } let(:sp) do - create( - :service_provider, - attribute_bundle: service_provider_attribute_bundle, - ) + create(:service_provider) end + let(:sp_session) { { requested_attributes: %w[email] } } before do stub_sign_in(user) allow(controller).to receive(:current_sp).and_return(sp) + allow(controller).to receive(:sp_session).and_return(sp_session) allow(controller).to receive(:needs_completion_screen_reason).and_return(:new_attributes) end @@ -108,7 +106,7 @@ end context ' with all_email and emails requested for SP' do - let(:service_provider_attribute_bundle) { %w[email all_emails] } + let(:sp_session) { { requested_attributes: %w[email all_emails] } } it 'returns nil' do response @@ -120,8 +118,7 @@ end context ' with all_emails requested for SP' do - let(:service_provider_attribute_bundle) { %w[all_emails] } - + let(:sp_session) { { requested_attributes: %w[all_emails] } } it 'returns nil' do response From d0875e8d17e3dcca9bad3aa92f950fc7bc4fc56f Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Fri, 17 Jan 2025 11:16:48 -0500 Subject: [PATCH 19/30] add verified attribute to identity --- spec/features/account_connected_apps_spec.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/features/account_connected_apps_spec.rb b/spec/features/account_connected_apps_spec.rb index 9ed731123a6..5113808cf40 100644 --- a/spec/features/account_connected_apps_spec.rb +++ b/spec/features/account_connected_apps_spec.rb @@ -3,6 +3,8 @@ RSpec.describe 'Account connected applications' do include NavigationHelper + let(:verified_attributes) { %w[email] } + let(:user) do create( :user, @@ -18,6 +20,7 @@ user: user, created_at: Time.zone.now - 80.days, service_provider: 'http://localhost:3000', + verified_attributes: verified_attributes, ) end let(:identity_without_link) do From a2e873fb388ea7d9a84089c38c2dc678a97774d6 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Fri, 17 Jan 2025 13:14:59 -0500 Subject: [PATCH 20/30] update identity spec --- app/models/service_provider_identity.rb | 3 +-- spec/models/service_provider_identity_spec.rb | 17 ++++++----------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index b90667ffea8..277d44bba29 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -71,8 +71,7 @@ def happened_at end def email_address_for_sharing - if IdentityConfig.store.feature_select_email_to_share_enabled && - email_address + if use_stored_email_address? return email_address end user.last_sign_in_email_address diff --git a/spec/models/service_provider_identity_spec.rb b/spec/models/service_provider_identity_spec.rb index f1cf7bd0eea..92ebcab2ebb 100644 --- a/spec/models/service_provider_identity_spec.rb +++ b/spec/models/service_provider_identity_spec.rb @@ -200,14 +200,9 @@ last_sign_in_at: 1.hour.ago, ) end + let(:verified_attributes) { %w[email] } - let(:service_provider_attribute_bundle) { %w[email] } - let(:service_provider) do - create( - :service_provider, - attribute_bundle: service_provider_attribute_bundle, - ) - end + let(:service_provider) { create(:service_provider) } let(:identity) do create( @@ -215,6 +210,7 @@ user: user, session_uuid: SecureRandom.uuid, service_provider: service_provider.issuer, + verified_attributes: verified_attributes, ) end @@ -225,7 +221,6 @@ end context 'when an email address is set and service provider has email attribute' do - let(:service_provider_attribute_bundle) { %w[email] } before do identity.email_address = shared_email_address end @@ -236,7 +231,7 @@ end context 'when service provider has both email and all_emails attribute' do - let(:service_provider_attribute_bundle) { %w[email all_emails] } + let(:verified_attributes) { %w[email all_emails] } before do identity.email_address = shared_email_address @@ -248,7 +243,7 @@ end context 'when service provider has no email attributes' do - let(:service_provider_attribute_bundle) { %w[first_name last_name] } + let(:verified_attributes) { %w[first_name last_name] } before do identity.email_address = shared_email_address @@ -259,7 +254,7 @@ end end - context 'when an email address for sharing has not been set' do + context 'when an email address for sharing has not been set and email is set' do before do identity.email_address = nil end From 74faffd9ca25157ffc1375e39312f1d0fe8ed257 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Tue, 21 Jan 2025 11:00:28 -0500 Subject: [PATCH 21/30] move validation for saml idp auth --- .../connected_accounts/selected_email_controller.rb | 8 +------- app/controllers/concerns/saml_idp_auth_concern.rb | 5 ++++- .../openid_connect/authorization_controller.rb | 4 +++- app/controllers/sign_up/select_email_controller.rb | 9 +-------- app/models/service_provider_identity.rb | 10 ++++++++-- 5 files changed, 17 insertions(+), 19 deletions(-) diff --git a/app/controllers/accounts/connected_accounts/selected_email_controller.rb b/app/controllers/accounts/connected_accounts/selected_email_controller.rb index 9fc28a837f3..2bf79e18cd1 100644 --- a/app/controllers/accounts/connected_accounts/selected_email_controller.rb +++ b/app/controllers/accounts/connected_accounts/selected_email_controller.rb @@ -20,7 +20,7 @@ def edit def update @select_email_form = build_select_email_form - result = @select_email_form.submit(selected_email_id: selected_email_id) + result = @select_email_form.submit(forms_params) analytics.sp_select_email_submitted(**result) @@ -52,12 +52,6 @@ def identity @identity = current_user.identities.find_by(id: params[:identity_id]) end - def selected_email_id - if identity.sp_only_single_email_requested? - form_params[:selected_email_id] - end - end - def last_email_id current_user.last_sign_in_email_address.id end diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index 7e19ba70df3..ed358b3f920 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -151,7 +151,10 @@ def email_address_id return user_session[:selected_email_id_for_linked_identity] end identity = current_user.identities.find_by(service_provider: sp_session['issuer']) - email_id = identity&.email_address_id + email_id = nil + if identity.sp_only_single_email_requested? + email_id = identity&.email_address_id + end return email_id if email_id.is_a? Integer end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index f88161ad800..64fee547ba9 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -94,7 +94,9 @@ def email_address_id return user_session[:selected_email_id_for_linked_identity] end identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) - identity&.email_address_for_sharing&.id + if identity&.sp_only_single_email_requested? + identity&.email_address_id + end end def ial_context diff --git a/app/controllers/sign_up/select_email_controller.rb b/app/controllers/sign_up/select_email_controller.rb index 4bf41a65af7..2abfff30c60 100644 --- a/app/controllers/sign_up/select_email_controller.rb +++ b/app/controllers/sign_up/select_email_controller.rb @@ -25,7 +25,7 @@ def create analytics.sp_select_email_submitted(**result, needs_completion_screen_reason:) if result.success? - user_session[:selected_email_id_for_linked_identity] = selected_email_id + user_session[:selected_email_id_for_linked_identity] = form_params[:selected_email_id] redirect_to sign_up_completed_path else flash[:error] = result.first_error_message @@ -55,13 +55,6 @@ def last_email end end - def selected_email_id - if decorated_sp_session&.requested_attributes&.include?('email') && - !decorated_sp_session&.requested_attributes&.include?('all_emails') - form_params[:selected_email_id] - end - end - def verify_needs_completions_screen redirect_to account_url unless needs_completion_screen_reason end diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index 277d44bba29..5dce04f35d0 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -8,6 +8,7 @@ class ServiceProviderIdentity < ApplicationRecord belongs_to :user validates :service_provider, presence: true + before_save :verify_email_address_id_needed # rubocop:disable Rails/InverseOf belongs_to :deleted_user, foreign_key: 'user_id', primary_key: 'user_id' @@ -70,6 +71,12 @@ def happened_at last_authenticated_at.in_time_zone('UTC') end + def verify_email_address_id_needed + if !sp_only_single_email_requested? + self.email_address_id = nil + end + end + def email_address_for_sharing if use_stored_email_address? return email_address @@ -79,7 +86,6 @@ def email_address_for_sharing def use_stored_email_address? IdentityConfig.store.feature_select_email_to_share_enabled && - email_address && - sp_only_single_email_requested? + email_address end end From 335490851298404df56c09779f76b5caf04a80d1 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Tue, 21 Jan 2025 11:17:11 -0500 Subject: [PATCH 22/30] remove unneeded method --- app/models/service_provider_identity.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index 5dce04f35d0..2d0e262e718 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -78,14 +78,9 @@ def verify_email_address_id_needed end def email_address_for_sharing - if use_stored_email_address? + if IdentityConfig.store.feature_select_email_to_share_enabled && email_address return email_address end user.last_sign_in_email_address end - - def use_stored_email_address? - IdentityConfig.store.feature_select_email_to_share_enabled && - email_address - end end From adbf97e67ad86701cd416fe8542bb3d2ca2eab4c Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Wed, 22 Jan 2025 13:59:00 -0500 Subject: [PATCH 23/30] add rspec for service provider identity --- .../selected_email_controller.rb | 2 +- app/models/service_provider_identity.rb | 7 +- .../selected_email_controller_spec.rb | 22 ----- .../authorization_controller_spec.rb | 90 ++++++++++++++++++ .../sign_up/select_email_controller_spec.rb | 25 ----- spec/models/service_provider_identity_spec.rb | 91 +++++++++++++------ 6 files changed, 159 insertions(+), 78 deletions(-) diff --git a/app/controllers/accounts/connected_accounts/selected_email_controller.rb b/app/controllers/accounts/connected_accounts/selected_email_controller.rb index 2bf79e18cd1..2d4b38bfc4f 100644 --- a/app/controllers/accounts/connected_accounts/selected_email_controller.rb +++ b/app/controllers/accounts/connected_accounts/selected_email_controller.rb @@ -20,7 +20,7 @@ def edit def update @select_email_form = build_select_email_form - result = @select_email_form.submit(forms_params) + result = @select_email_form.submit(form_params) analytics.sp_select_email_submitted(**result) diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index 2d0e262e718..f3f3a50f9a2 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -8,7 +8,7 @@ class ServiceProviderIdentity < ApplicationRecord belongs_to :user validates :service_provider, presence: true - before_save :verify_email_address_id_needed + before_save :clear_email_address_id_if_not_supported # rubocop:disable Rails/InverseOf belongs_to :deleted_user, foreign_key: 'user_id', primary_key: 'user_id' @@ -71,8 +71,9 @@ def happened_at last_authenticated_at.in_time_zone('UTC') end - def verify_email_address_id_needed - if !sp_only_single_email_requested? + def clear_email_address_id_if_not_supported + if !sp_only_single_email_requested? && + IdentityConfig.store.feature_select_email_to_share_enabled self.email_address_id = nil end end diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index 1f02ec61496..215ef08367c 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -120,28 +120,6 @@ ) end - context ' with all_emails and emails requested' do - let(:verified_attributes) { %w[email all_emails] } - - it 'returns nil' do - response - - identity.reload - expect(identity.email_address_id).to eq(nil) - end - end - - context ' with all_emails requested' do - let(:verified_attributes) { %w[all_emails] } - - it 'returns nil' do - response - - identity.reload - expect(identity.email_address_id).to eq(nil) - end - end - context 'with invalid submission' do let(:params) { super().merge(select_email_form: { selected_email_id: '' }) } diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 90637ce9bd4..76cd4a5beb2 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -2337,6 +2337,96 @@ expect(@analytics).to_not have_logged_event('SP redirect initiated') end end + + context 'with SP requesting a single email' do + let(:verified_attributes) { %w[email] } + let(:shared_email_address) do + create( + :email_address, + email: 'shared2@email.com', + user: user, + last_sign_in_at: 1.hour.ago, + ) + end + let(:identity) do + create( + :service_provider_identity, + user: user, + session_uuid: SecureRandom.uuid, + service_provider: service_provider.issuer, + verified_attributes: verified_attributes, + ) + end + before do + allow(IdentityConfig.store).to receive(:feature_select_email_to_share_enabled) + .and_return(true) + session[:selected_email_id_for_linked_identity] = shared_email_address + end + + it 'updates identity to be the value in session' do + identity = current_user.identities.find_by(service_provider: service_provider.issuer) + action + expect(identity.email_address_id).to eq(shared_email_address.id) + end + end + + context 'with SP requesting a single email and all emails' do + let(:verified_attributes) { %w[email all_emails] } + let(:shared_email_address) do + create( + :email_address, + email: 'shared2@email.com', + user: user, + last_sign_in_at: 1.hour.ago, + ) + end + let(:identity) do + create( + :service_provider_identity, + user: user, + session_uuid: SecureRandom.uuid, + service_provider: service_provider.issuer, + verified_attributes: verified_attributes, + email_address_id: shared_email_address.id, + ) + end + before do + allow(IdentityConfig.store).to receive(:feature_select_email_to_share_enabled) + .and_return(true) + end + + it 'updates identity email_address to be nil' do + end + end + + context 'with SP requesting no emails' do + let(:verified_attributes) { %w[first_name last_name] } + let(:shared_email_address) do + create( + :email_address, + email: 'shared2@email.com', + user: user, + last_sign_in_at: 1.hour.ago, + ) + end + let(:identity) do + create( + :service_provider_identity, + user: user, + session_uuid: SecureRandom.uuid, + service_provider: service_provider.issuer, + verified_attributes: verified_attributes, + email_address_id: shared_email_address.id, + ) + end + before do + allow(IdentityConfig.store).to receive(:feature_select_email_to_share_enabled) + .and_return(true) + end + + it 'updates identity email_address to be nil' do + end + end end context 'user is not signed in' do diff --git a/spec/controllers/sign_up/select_email_controller_spec.rb b/spec/controllers/sign_up/select_email_controller_spec.rb index b74ea5a368c..5a6b7826241 100644 --- a/spec/controllers/sign_up/select_email_controller_spec.rb +++ b/spec/controllers/sign_up/select_email_controller_spec.rb @@ -5,12 +5,10 @@ let(:sp) do create(:service_provider) end - let(:sp_session) { { requested_attributes: %w[email] } } before do stub_sign_in(user) allow(controller).to receive(:current_sp).and_return(sp) - allow(controller).to receive(:sp_session).and_return(sp_session) allow(controller).to receive(:needs_completion_screen_reason).and_return(:new_attributes) end @@ -105,29 +103,6 @@ ) end - context ' with all_email and emails requested for SP' do - let(:sp_session) { { requested_attributes: %w[email all_emails] } } - - it 'returns nil' do - response - - expect( - controller.user_session[:selected_email_id_for_linked_identity], - ).to eq(nil) - end - end - - context ' with all_emails requested for SP' do - let(:sp_session) { { requested_attributes: %w[all_emails] } } - it 'returns nil' do - response - - expect( - controller.user_session[:selected_email_id_for_linked_identity], - ).to eq(nil) - end - end - context 'with a corrupted email selected_email_id form' do let(:other_user) { create(:user) } let(:selected_email_id) { other_user.confirmed_email_addresses.sample.id } diff --git a/spec/models/service_provider_identity_spec.rb b/spec/models/service_provider_identity_spec.rb index 92ebcab2ebb..bffe3a17451 100644 --- a/spec/models/service_provider_identity_spec.rb +++ b/spec/models/service_provider_identity_spec.rb @@ -200,7 +200,6 @@ last_sign_in_at: 1.hour.ago, ) end - let(:verified_attributes) { %w[email] } let(:service_provider) { create(:service_provider) } @@ -210,7 +209,6 @@ user: user, session_uuid: SecureRandom.uuid, service_provider: service_provider.issuer, - verified_attributes: verified_attributes, ) end @@ -220,7 +218,7 @@ .and_return(true) end - context 'when an email address is set and service provider has email attribute' do + context 'when an email address is set' do before do identity.email_address = shared_email_address end @@ -230,48 +228,87 @@ end end - context 'when service provider has both email and all_emails attribute' do - let(:verified_attributes) { %w[email all_emails] } - + context 'when an email address for sharing has not been set' do before do - identity.email_address = shared_email_address + identity.email_address = nil end - - it 'returns the last sign in email' do + it 'returns the last login email' do expect(identity.email_address_for_sharing).to eq(last_login_email_address) end end + end - context 'when service provider has no email attributes' do - let(:verified_attributes) { %w[first_name last_name] } + context 'when email sharing feature is disabled' do + before do + allow(IdentityConfig.store).to receive(:feature_select_email_to_share_enabled) + .and_return(true) + end - before do - identity.email_address = shared_email_address - end + it 'returns the last login email' do + expect(identity.email_address_for_sharing).to eq(last_login_email_address) + end + end + end - it 'returns the last sign in email' do - expect(identity.email_address_for_sharing).to eq(last_login_email_address) - end + describe '#clear_email_address_id_if_not_supported' do + let(:verified_attributes) { %w[email] } + let!(:shared_email_address) do + create( + :email_address, + email: 'shared@email.com', + user: user, + last_sign_in_at: 1.hour.ago, + ) + end + let(:identity) do + create( + :service_provider_identity, + user: user, + session_uuid: SecureRandom.uuid, + service_provider: service_provider.issuer, + verified_attributes: verified_attributes, + email_address_id: shared_email_address.id, + ) + end + + context 'when user has only email as the verified attribute attribute' do + let(:new_shared_email_address) do + create( + :email_address, + email: 'shared2@email.com', + user: user, + last_sign_in_at: 1.hour.ago, + ) + end + before do + allow(IdentityConfig.store).to receive(:feature_select_email_to_share_enabled) + .and_return(true) end - context 'when an email address for sharing has not been set and email is set' do - before do - identity.email_address = nil - end - it 'returns the last login email' do - expect(identity.email_address_for_sharing).to eq(last_login_email_address) - end + it 'should save the new email properly on update' do + identity.update!(email_address_id: new_shared_email_address.id) + expect(identity.email_address).to eq(new_shared_email_address) end end - context 'when email sharing feature is disabled' do + context 'when user has all_emails as the verified attribute' do + let(:verified_attributes) { %w[all_emails] } + let(:new_shared_email_address) do + create( + :email_address, + email: 'shared2@email.com', + user: user, + last_sign_in_at: 1.hour.ago, + ) + end before do allow(IdentityConfig.store).to receive(:feature_select_email_to_share_enabled) .and_return(true) end - it 'returns the last login email' do - expect(identity.email_address_for_sharing).to eq(last_login_email_address) + it 'should make the email address to nil' do + identity.update!(email_address_id: new_shared_email_address.id) + expect(identity.email_address).to eq(nil) end end end From 65ee172ff764495b264a74ac5ae3ab904163eeea Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Thu, 23 Jan 2025 11:13:16 -0500 Subject: [PATCH 24/30] LG-15251: update spec to add specs for chnages to authorization and saml idp controllers --- .../concerns/saml_idp_auth_concern.rb | 2 +- .../authorization_controller_spec.rb | 21 ++++-- spec/controllers/saml_idp_controller_spec.rb | 72 +++++++++++++++++++ 3 files changed, 89 insertions(+), 6 deletions(-) diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index ed358b3f920..7722a1e536c 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -150,7 +150,7 @@ def email_address_id if user_session[:selected_email_id_for_linked_identity].present? return user_session[:selected_email_id_for_linked_identity] end - identity = current_user.identities.find_by(service_provider: sp_session['issuer']) + identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) email_id = nil if identity.sp_only_single_email_requested? email_id = identity&.email_address_id diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 76cd4a5beb2..8321f8e4a30 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -2339,6 +2339,8 @@ end context 'with SP requesting a single email' do + let(:acr_values) { Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF } + let(:vtr) { nil } let(:verified_attributes) { %w[email] } let(:shared_email_address) do create( @@ -2348,7 +2350,7 @@ last_sign_in_at: 1.hour.ago, ) end - let(:identity) do + let!(:identity) do create( :service_provider_identity, user: user, @@ -2360,12 +2362,13 @@ before do allow(IdentityConfig.store).to receive(:feature_select_email_to_share_enabled) .and_return(true) - session[:selected_email_id_for_linked_identity] = shared_email_address + controller.user_session[:selected_email_id_for_linked_identity] = shared_email_address.id end it 'updates identity to be the value in session' do - identity = current_user.identities.find_by(service_provider: service_provider.issuer) + identity = user.identities.find_by(service_provider: service_provider.issuer) action + identity.reload expect(identity.email_address_id).to eq(shared_email_address.id) end end @@ -2380,7 +2383,7 @@ last_sign_in_at: 1.hour.ago, ) end - let(:identity) do + let!(:identity) do create( :service_provider_identity, user: user, @@ -2396,6 +2399,10 @@ end it 'updates identity email_address to be nil' do + identity = user.identities.find_by(service_provider: service_provider.issuer) + action + identity.reload + expect(identity.email_address_id).to eq(nil) end end @@ -2409,7 +2416,7 @@ last_sign_in_at: 1.hour.ago, ) end - let(:identity) do + let!(:identity) do create( :service_provider_identity, user: user, @@ -2425,6 +2432,10 @@ end it 'updates identity email_address to be nil' do + identity = user.identities.find_by(service_provider: service_provider.issuer) + action + identity.reload + expect(identity.email_address_id).to eq(nil) end end end diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 7d5a2b5a49d..cc45f32860a 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1541,6 +1541,78 @@ def name_id_version(format_urn) end end + context 'with shared email feature turned on' do + let(:user) { create(:user, :fully_registered) } + let(:service_provider) { build(:service_provider, issuer: saml_settings.issuer) } + + before do + allow(IdentityConfig.store).to receive(:feature_select_email_to_share_enabled) + .and_return(true) + stub_sign_in(user) + session[:sign_in_flow] = :sign_in + end + + context 'with SP requesting a single email' do + let(:verified_attributes) { %w[email] } + let(:shared_email_address) do + create( + :email_address, + email: 'shared2@email.com', + user: user, + last_sign_in_at: 1.hour.ago, + ) + end + let!(:identity) do + create( + :service_provider_identity, + user: user, + session_uuid: SecureRandom.uuid, + service_provider: service_provider.issuer, + verified_attributes: verified_attributes, + ) + end + before do + controller.user_session[:selected_email_id_for_linked_identity] = shared_email_address.id + end + + it 'updates identity to be the value in session' do + identity = user.identities.find_by(service_provider: service_provider.issuer) + saml_get_auth(saml_settings) + identity.reload + expect(identity.email_address_id).to eq(shared_email_address.id) + end + end + + context 'with SP requesting a single email and all emails' do + let(:verified_attributes) { %w[email all_emails] } + let(:shared_email_address) do + create( + :email_address, + email: 'shared2@email.com', + user: user, + last_sign_in_at: 1.hour.ago, + ) + end + let!(:identity) do + create( + :service_provider_identity, + user: user, + session_uuid: SecureRandom.uuid, + service_provider: service_provider.issuer, + verified_attributes: verified_attributes, + email_address_id: shared_email_address.id, + ) + end + + it 'updates identity email_address to be nil' do + identity = user.identities.find_by(service_provider: service_provider.issuer) + saml_get_auth(saml_settings) + identity.reload + expect(identity.email_address_id).to eq(nil) + end + end + end + context 'POST to auth correctly stores SP in session' do let(:acr_values) do Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF + From 62686de834ddaa79d043b7df6648d34a738c423b Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Thu, 23 Jan 2025 11:44:31 -0500 Subject: [PATCH 25/30] update select email form --- .../selected_email_controller_spec.rb | 2 +- spec/forms/select_email_form_spec.rb | 4 +++- .../connected_accounts/show.html.erb_spec.rb | 17 +++++++++++++++-- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb index 215ef08367c..2d70ca3af20 100644 --- a/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb +++ b/spec/controllers/accounts/connected_accounts/selected_email_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Accounts::ConnectedAccounts::SelectedEmailController do - let(:identity) { create(:service_provider_identity, :active) } + let(:identity) { create(:service_provider_identity, :active, verified_attributes: ['email']) } let(:user) { create(:user, :with_multiple_emails, identities: [identity]) } before do diff --git a/spec/forms/select_email_form_spec.rb b/spec/forms/select_email_form_spec.rb index cc63aaabd3c..da74cc1ff3e 100644 --- a/spec/forms/select_email_form_spec.rb +++ b/spec/forms/select_email_form_spec.rb @@ -18,7 +18,9 @@ end context 'with associated identity' do - let(:identity) { create(:service_provider_identity, :consented, user:) } + let(:identity) do + create(:service_provider_identity, :consented, user:, verified_attributes: %w[email]) + end it 'updates linked email address' do expect { response }.to change { identity.reload.email_address_id } diff --git a/spec/views/accounts/connected_accounts/show.html.erb_spec.rb b/spec/views/accounts/connected_accounts/show.html.erb_spec.rb index 9a18c8cbb00..3f63f53cc0b 100644 --- a/spec/views/accounts/connected_accounts/show.html.erb_spec.rb +++ b/spec/views/accounts/connected_accounts/show.html.erb_spec.rb @@ -31,7 +31,14 @@ end context 'with a connected app' do - let!(:identity) { create(:service_provider_identity, user:) } + let(:verified_attributes) { %w[email] } + let!(:identity) do + create( + :service_provider_identity, + user:, + verified_attributes: verified_attributes, + ) + end it 'lists applications with link to revoke' do render @@ -70,8 +77,14 @@ context 'with connected app having linked email' do let(:email_address) { user.confirmed_email_addresses.take } + let(:verified_attributes) { %w[email] } let!(:identity) do - create(:service_provider_identity, user:, email_address_id: email_address.id) + create( + :service_provider_identity, + user:, + email_address_id: email_address.id, + verified_attributes: verified_attributes, + ) end it 'renders associated email with option to change' do From 50c9343bf6603228f4cf7d24aeb3944d146a5742 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Mon, 27 Jan 2025 11:19:49 -0500 Subject: [PATCH 26/30] dont save unless verified attributes --- app/controllers/concerns/saml_idp_auth_concern.rb | 6 ++---- app/controllers/openid_connect/authorization_controller.rb | 5 ++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index 11ea229b3a8..02f90b01bb6 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -154,14 +154,12 @@ def link_identity_from_session_data def email_address_id return nil unless IdentityConfig.store.feature_select_email_to_share_enabled + return nil if !identity&.sp_only_single_email_requested? if user_session[:selected_email_id_for_linked_identity].present? return user_session[:selected_email_id_for_linked_identity] end identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) - email_id = nil - if identity.sp_only_single_email_requested? - email_id = identity&.email_address_id - end + email_id = identity&.email_address_id return email_id if email_id.is_a? Integer end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 64fee547ba9..7c95497050b 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -90,13 +90,12 @@ def link_identity_to_service_provider def email_address_id return nil unless IdentityConfig.store.feature_select_email_to_share_enabled + return nil if !identity&.sp_only_single_email_requested? if user_session[:selected_email_id_for_linked_identity].present? return user_session[:selected_email_id_for_linked_identity] end identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) - if identity&.sp_only_single_email_requested? - identity&.email_address_id - end + identity&.email_address_id end def ial_context From 8ca969df46e44d37161951cd3d2942e05db964e2 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Mon, 27 Jan 2025 14:03:32 -0500 Subject: [PATCH 27/30] update authorization confirmation --- .../concerns/saml_idp_auth_concern.rb | 2 +- .../concerns/verify_sp_attributes_concern.rb | 12 +++++++ .../authorization_controller.rb | 3 +- .../authorization_confirmation_spec.rb | 34 +++++++++++++++++++ .../saml/authorization_confirmation_spec.rb | 33 ++++++++++++++++++ 5 files changed, 82 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index 02f90b01bb6..b57e0f3f4ed 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -154,11 +154,11 @@ def link_identity_from_session_data def email_address_id return nil unless IdentityConfig.store.feature_select_email_to_share_enabled + identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) return nil if !identity&.sp_only_single_email_requested? if user_session[:selected_email_id_for_linked_identity].present? return user_session[:selected_email_id_for_linked_identity] end - identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) email_id = identity&.email_address_id return email_id if email_id.is_a? Integer end diff --git a/app/controllers/concerns/verify_sp_attributes_concern.rb b/app/controllers/concerns/verify_sp_attributes_concern.rb index b761afe010a..b2d7a5de0c7 100644 --- a/app/controllers/concerns/verify_sp_attributes_concern.rb +++ b/app/controllers/concerns/verify_sp_attributes_concern.rb @@ -28,6 +28,7 @@ def update_verified_attributes verified_attributes: sp_session[:requested_attributes], last_consented_at: Time.zone.now, clear_deleted_at: true, + email_address_id:, ) end @@ -44,6 +45,17 @@ def consent_was_revoked?(sp_session_identity) sp_session_identity.deleted_at.present? end + def email_address_id + return nil unless IdentityConfig.store.feature_select_email_to_share_enabled + identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) + return nil if !identity&.sp_only_single_email_requested? + if user_session[:selected_email_id_for_linked_identity].present? + return user_session[:selected_email_id_for_linked_identity] + end + + identity&.email_address_id + end + def reverified_after_consent?(sp_session_identity) return false unless sp_session_identity return false if sp_session_identity.deleted_at.present? diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 7c95497050b..598b71153e4 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -90,11 +90,12 @@ def link_identity_to_service_provider def email_address_id return nil unless IdentityConfig.store.feature_select_email_to_share_enabled + identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) return nil if !identity&.sp_only_single_email_requested? if user_session[:selected_email_id_for_linked_identity].present? return user_session[:selected_email_id_for_linked_identity] end - identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) + identity&.email_address_id end diff --git a/spec/features/openid_connect/authorization_confirmation_spec.rb b/spec/features/openid_connect/authorization_confirmation_spec.rb index 8986fec0c5e..3212770fd1a 100644 --- a/spec/features/openid_connect/authorization_confirmation_spec.rb +++ b/spec/features/openid_connect/authorization_confirmation_spec.rb @@ -82,6 +82,40 @@ def create_user_and_remember_device it_behaves_like 'signing in with a different email prompts with the shared email' end + + context 'with requested attributes contains only email' do + it ' creates an identity with proper email_address_id' do + user = user_with_2fa + + sign_in_oidc_user(user) + check t('forms.messages.remember_device') + fill_in_code_with_last_phone_otp + click_submit_default + click_agree_and_continue + identity = user.identities.find_by(service_provider: OidcAuthHelper::OIDC_IAL1_ISSUER) + email_id = user.email_addresses.first.id + expect(identity.email_address_id).to eq(email_id) + end + end + + context 'with requested attributes contains is emails and all_emails' do + it 'creates an identity with no email_address_id saved' do + user = user_with_2fa + + params = ial1_params + params[:scope] = 'openid email all_emails' + oidc_path = openid_connect_authorize_path params + visit oidc_path + fill_in_credentials_and_submit(user.email, user.password) + click_submit_default + check t('forms.messages.remember_device') + fill_in_code_with_last_phone_otp + click_submit_default + click_agree_and_continue + identity = user.identities.find_by(service_provider: OidcAuthHelper::OIDC_IAL1_ISSUER) + expect(identity.email_address_id).to eq(nil) + end + end end context 'when email sharing feature is disabled' do diff --git a/spec/features/saml/authorization_confirmation_spec.rb b/spec/features/saml/authorization_confirmation_spec.rb index 34c59fb6515..68f03b54dbd 100644 --- a/spec/features/saml/authorization_confirmation_spec.rb +++ b/spec/features/saml/authorization_confirmation_spec.rb @@ -52,6 +52,39 @@ def create_user_and_remember_device continue_as(shared_email) expect(current_url).to eq(complete_saml_url) end + + context 'with requested attributes contains only email' do + it ' creates an identity with proper email_address_id' do + user = user_with_2fa + + sign_in_user(user) + check t('forms.messages.remember_device') + fill_in_code_with_last_phone_otp + click_submit_default + visit request_url + click_agree_and_continue + identity = user.identities.find_by(service_provider: SamlAuthHelper::SP_ISSUER) + email_id = user.email_addresses.first.id + expect(identity.email_address_id).to eq(email_id) + end + end + + context 'with requested attributes contains is emails and all_emails' do + it 'creates an identity with no email_address_id saved' do + user = user_with_2fa + + oidc_path = openid_connect_authorize_path params + visit oidc_path + fill_in_credentials_and_submit(user.email, user.password) + click_submit_default + check t('forms.messages.remember_device') + fill_in_code_with_last_phone_otp + click_submit_default + click_agree_and_continue + identity = user.identities.find_by(service_provider: SamlAuthHelper::SP_ISSUER) + expect(identity.email_address_id).to eq(nil) + end + end end context 'when email sharing feature is disabled' do From e98798a9188603ed1ee815498422f00d9788c8ea Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Mon, 27 Jan 2025 14:18:58 -0500 Subject: [PATCH 28/30] remove email address id check --- .../concerns/verify_sp_attributes_concern.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/app/controllers/concerns/verify_sp_attributes_concern.rb b/app/controllers/concerns/verify_sp_attributes_concern.rb index b2d7a5de0c7..b761afe010a 100644 --- a/app/controllers/concerns/verify_sp_attributes_concern.rb +++ b/app/controllers/concerns/verify_sp_attributes_concern.rb @@ -28,7 +28,6 @@ def update_verified_attributes verified_attributes: sp_session[:requested_attributes], last_consented_at: Time.zone.now, clear_deleted_at: true, - email_address_id:, ) end @@ -45,17 +44,6 @@ def consent_was_revoked?(sp_session_identity) sp_session_identity.deleted_at.present? end - def email_address_id - return nil unless IdentityConfig.store.feature_select_email_to_share_enabled - identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) - return nil if !identity&.sp_only_single_email_requested? - if user_session[:selected_email_id_for_linked_identity].present? - return user_session[:selected_email_id_for_linked_identity] - end - - identity&.email_address_id - end - def reverified_after_consent?(sp_session_identity) return false unless sp_session_identity return false if sp_session_identity.deleted_at.present? From 1e717ea65237ac47972a177aab48f98e9ad25586 Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Mon, 27 Jan 2025 14:36:18 -0500 Subject: [PATCH 29/30] test with verified attributes containing all_meails --- .../saml/authorization_confirmation_spec.rb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/spec/features/saml/authorization_confirmation_spec.rb b/spec/features/saml/authorization_confirmation_spec.rb index 68f03b54dbd..553820bd0ab 100644 --- a/spec/features/saml/authorization_confirmation_spec.rb +++ b/spec/features/saml/authorization_confirmation_spec.rb @@ -63,6 +63,8 @@ def create_user_and_remember_device click_submit_default visit request_url click_agree_and_continue + click_submit_default + visit sign_out_url identity = user.identities.find_by(service_provider: SamlAuthHelper::SP_ISSUER) email_id = user.email_addresses.first.id expect(identity.email_address_id).to eq(email_id) @@ -70,17 +72,22 @@ def create_user_and_remember_device end context 'with requested attributes contains is emails and all_emails' do + before do + allow_any_instance_of(ServiceProviderIdentity).to receive(:verified_attributes) + .and_return(%w[email all_emails]) + end it 'creates an identity with no email_address_id saved' do user = user_with_2fa - oidc_path = openid_connect_authorize_path params - visit oidc_path - fill_in_credentials_and_submit(user.email, user.password) - click_submit_default + sign_in_user(user) check t('forms.messages.remember_device') fill_in_code_with_last_phone_otp click_submit_default + visit request_url click_agree_and_continue + click_submit_default + visit sign_out_url + identity = user.identities.find_by(service_provider: SamlAuthHelper::SP_ISSUER) expect(identity.email_address_id).to eq(nil) end From e8cb1323ddfcf84329d55a832b523482fc4c6fba Mon Sep 17 00:00:00 2001 From: Malick DiarrA Date: Mon, 27 Jan 2025 16:04:49 -0500 Subject: [PATCH 30/30] update method to avoid merge conflict --- app/controllers/concerns/saml_idp_auth_concern.rb | 2 +- .../openid_connect/authorization_controller.rb | 2 +- app/models/service_provider_identity.rb | 13 +++++++------ 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index b57e0f3f4ed..c1ad5363a61 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -155,7 +155,7 @@ def link_identity_from_session_data def email_address_id return nil unless IdentityConfig.store.feature_select_email_to_share_enabled identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) - return nil if !identity&.sp_only_single_email_requested? + return nil if !identity&.verified_single_email_attribute? if user_session[:selected_email_id_for_linked_identity].present? return user_session[:selected_email_id_for_linked_identity] end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 598b71153e4..e92f4824c70 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -91,7 +91,7 @@ def link_identity_to_service_provider def email_address_id return nil unless IdentityConfig.store.feature_select_email_to_share_enabled identity = current_user.identities.find_by(service_provider: sp_session[:issuer]) - return nil if !identity&.sp_only_single_email_requested? + return nil if !identity&.verified_single_email_attribute? if user_session[:selected_email_id_for_linked_identity].present? return user_session[:selected_email_id_for_linked_identity] end diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index f3f3a50f9a2..b99a9a00b20 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -58,11 +58,6 @@ def friendly_name sp_metadata[:friendly_name] end - def sp_only_single_email_requested? - verified_attributes&.include?('email') && - !verified_attributes.include?('all_emails') - end - def service_provider_id service_provider_record&.id end @@ -71,8 +66,14 @@ def happened_at last_authenticated_at.in_time_zone('UTC') end + def verified_single_email_attribute? + verified_attributes.present? && + verified_attributes.include?('email') && + !verified_attributes.include?('all_emails') + end + def clear_email_address_id_if_not_supported - if !sp_only_single_email_requested? && + if !verified_single_email_attribute? && IdentityConfig.store.feature_select_email_to_share_enabled self.email_address_id = nil end