diff --git a/app/controllers/accounts/connected_accounts/selected_email_controller.rb b/app/controllers/accounts/connected_accounts/selected_email_controller.rb index 1ae8b77a6fe..2d4b38bfc4f 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 @@ -52,7 +52,7 @@ def identity @identity = current_user.identities.find_by(id: params[:identity_id]) end - def last_email + def last_email_id current_user.last_sign_in_email_address.id end end diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index 80fd3eb54ee..d98ee3e8f1c 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -154,10 +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&.verified_single_email_attribute? 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/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 1480697d6b6..e92f4824c70 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -90,10 +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&.verified_single_email_attribute? 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/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index 3da98d4eb9f..b99a9a00b20 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 :clear_email_address_id_if_not_supported # rubocop:disable Rails/InverseOf belongs_to :deleted_user, foreign_key: 'user_id', primary_key: 'user_id' @@ -71,6 +72,13 @@ def verified_single_email_attribute? !verified_attributes.include?('all_emails') end + def clear_email_address_id_if_not_supported + if !verified_single_email_attribute? && + IdentityConfig.store.feature_select_email_to_share_enabled + self.email_address_id = nil + end + end + def email_address_for_sharing if IdentityConfig.store.feature_select_email_to_share_enabled && email_address return email_address 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..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 @@ -89,8 +89,18 @@ 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(: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) { 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 + subject(:response) { patch :update, params: } it 'redirects to connected accounts path with the appropriate flash message' do @@ -106,7 +116,7 @@ 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 @@ -133,7 +143,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 } diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 68287933ad6..f1e5572af78 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -2324,6 +2324,107 @@ expect(@analytics).to_not have_logged_event('SP redirect initiated') end 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( + :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) + 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) + action + 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 + 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 + identity = user.identities.find_by(service_provider: service_provider.issuer) + action + identity.reload + expect(identity.email_address_id).to eq(nil) + 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 + identity = user.identities.find_by(service_provider: service_provider.issuer) + action + identity.reload + expect(identity.email_address_id).to eq(nil) + end + end end context 'user is not signed in' do diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 27875bd504b..e85933a64f1 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1567,6 +1567,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 + diff --git a/spec/controllers/sign_up/select_email_controller_spec.rb b/spec/controllers/sign_up/select_email_controller_spec.rb index 7eefddb0368..5a6b7826241 100644 --- a/spec/controllers/sign_up/select_email_controller_spec.rb +++ b/spec/controllers/sign_up/select_email_controller_spec.rb @@ -2,7 +2,9 @@ RSpec.describe SignUp::SelectEmailController do let(:user) { create(:user, :with_multiple_emails) } - let(:sp) { create(:service_provider) } + let(:sp) do + create(:service_provider) + end before do stub_sign_in(user) @@ -75,8 +77,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 +87,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,13 +99,13 @@ :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 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) @@ -122,7 +124,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 diff --git a/spec/features/account_connected_apps_spec.rb b/spec/features/account_connected_apps_spec.rb index c37ad9cd25f..5538f2d20e8 100644 --- a/spec/features/account_connected_apps_spec.rb +++ b/spec/features/account_connected_apps_spec.rb @@ -18,7 +18,7 @@ user: user, created_at: Time.zone.now - 80.days, service_provider: 'http://localhost:3000', - verified_attributes: ['email'], + verified_attributes: %w[email], ) end let(:identity_without_link) do 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..553820bd0ab 100644 --- a/spec/features/saml/authorization_confirmation_spec.rb +++ b/spec/features/saml/authorization_confirmation_spec.rb @@ -52,6 +52,46 @@ 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 + 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) + end + 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 + + 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 + end end context 'when email sharing feature is disabled' 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/models/service_provider_identity_spec.rb b/spec/models/service_provider_identity_spec.rb index f188c84368f..e2c81d1ad9c 100644 --- a/spec/models/service_provider_identity_spec.rb +++ b/spec/models/service_provider_identity_spec.rb @@ -243,8 +243,15 @@ ) end + let(:service_provider) { create(:service_provider) } + 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 @@ -253,7 +260,7 @@ .and_return(true) end - context 'when an email address for sharing has been set' do + context 'when an email address is set' do before do identity.email_address = shared_email_address end @@ -284,4 +291,67 @@ end end 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 + + 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 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 '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 end