Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lg 15251 avoid linking email address #11717

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ 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
@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)

Expand Down Expand Up @@ -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 identity.all_email_and_single_email_requested?
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
Expand Down
12 changes: 11 additions & 1 deletion app/controllers/sign_up/select_email_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -55,6 +55,16 @@ def last_email
end
end

def selected_email_id
if current_sp.present? &&
current_sp.attribute_bundle&.include?('all_email') &&
Copy link
Member

Choose a reason for hiding this comment

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

There is no attribute all_email. This should be all_emails.

https://developers.login.gov/attributes/

current_sp.attribute_bundle.include?('email')
current_user.last_sign_in_email_address.id
else
form_params[:selected_email_id]
end
end

def verify_needs_completions_screen
redirect_to account_url unless needs_completion_screen_reason
end
Expand Down
5 changes: 5 additions & 0 deletions app/models/service_provider_identity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ def friendly_name
sp_metadata[:friendly_name]
end

def all_email_and_single_email_requested?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the behavior we're expecting. A partner would only request either email or all_emails in most cases. We only want to link the email if they requested a singular email, since this is when we show the option for the user to choose which email should be shared.

service_provider_record&.attribute_bundle&.include?('all_email') &&
service_provider_record&.attribute_bundle.include?('email')
end

def service_provider_id
service_provider_record&.id
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.id }
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
Expand All @@ -106,10 +106,41 @@
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)
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: '' }) }

Expand All @@ -133,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 }

Expand Down
37 changes: 30 additions & 7 deletions spec/controllers/sign_up/select_email_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 }

Expand All @@ -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
Expand All @@ -97,13 +103,30 @@
: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 }
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)
Expand All @@ -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
Expand Down