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

Merged
merged 33 commits into from
Jan 30, 2025
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
67ffa13
changelog: Upcoming Features, Authentication, allow all_emails and em…
mdiarra3 Jan 7, 2025
1cf573d
add spec for identity check
mdiarra3 Jan 8, 2025
b5c52d2
update selected_email controller and spec
mdiarra3 Jan 8, 2025
3348e2a
fix selected email working for all email and emails
mdiarra3 Jan 8, 2025
321b9c3
fix select email controller
mdiarra3 Jan 8, 2025
8c1833a
change ot proper attribute
mdiarra3 Jan 13, 2025
2176080
update server identity and select email controller to only look for a…
mdiarra3 Jan 13, 2025
4574c6f
move to check if all_emails and email given work
mdiarra3 Jan 13, 2025
cd4a35d
selected email returns proper attributes
mdiarra3 Jan 14, 2025
40ddc21
fix select email specs
mdiarra3 Jan 14, 2025
7681ca4
update service identity spec
mdiarra3 Jan 14, 2025
5cc5b03
rework to use identity verified attributes and session attributes
mdiarra3 Jan 15, 2025
ff39621
move back authorization controller
mdiarra3 Jan 16, 2025
a19f8a0
Merge remote-tracking branch 'origin/main' into LG-15251-avoid-linkin…
mdiarra3 Jan 16, 2025
a537d91
fix migration
mdiarra3 Jan 16, 2025
ff56fa7
fix schema
mdiarra3 Jan 16, 2025
8c772dd
remove proofing component schema
mdiarra3 Jan 16, 2025
9241d71
remove spacing
mdiarra3 Jan 16, 2025
dcb3700
fix tests
mdiarra3 Jan 16, 2025
d0875e8
add verified attribute to identity
mdiarra3 Jan 17, 2025
a2e873f
update identity spec
mdiarra3 Jan 17, 2025
74faffd
move validation for saml idp auth
mdiarra3 Jan 21, 2025
3354908
remove unneeded method
mdiarra3 Jan 21, 2025
adbf97e
add rspec for service provider identity
mdiarra3 Jan 22, 2025
65ee172
LG-15251: update spec to add specs for chnages to authorization and s…
mdiarra3 Jan 23, 2025
d41b386
Merge remote-tracking branch 'origin/main' into LG-15251-avoid-linkin…
mdiarra3 Jan 23, 2025
62686de
update select email form
mdiarra3 Jan 23, 2025
50c9343
dont save unless verified attributes
mdiarra3 Jan 27, 2025
8ca969d
update authorization confirmation
mdiarra3 Jan 27, 2025
e98798a
remove email address id check
mdiarra3 Jan 27, 2025
1e717ea
test with verified attributes containing all_meails
mdiarra3 Jan 27, 2025
e8cb132
update method to avoid merge conflict
mdiarra3 Jan 27, 2025
bed0636
Merge remote-tracking branch 'origin/main' into LG-15251-avoid-linkin…
mdiarra3 Jan 29, 2025
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
mdiarra3 marked this conversation as resolved.
Show resolved Hide resolved
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,13 @@ def identity
@identity = current_user.identities.find_by(id: params[:identity_id])
end

def last_email
def selected_email_id
if identity.sp_only_single_email_requested?
form_params[:selected_email_id]
end
end

def last_email_id
Copy link
Member

Choose a reason for hiding this comment

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

Nice clarifying rename 👍

current_user.last_sign_in_email_address.id
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def email_address_id
return user_session[:selected_email_id_for_linked_identity]
Copy link
Member

Choose a reason for hiding this comment

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

I'm still seeing ways that email_address_id is going to be assigned regardless of what attributes are requested by the service provider. This session value is assigned when the user grants consent, and will be returned here before we get a chance to evaluate sp_only_single_email_requested?.

if user_session[:selected_email_id_for_linked_identity].nil?
user_session[:selected_email_id_for_linked_identity] = current_user
.last_sign_in_email_address.id
end

I think we should do an audit of User#last_sign_in_email_address and Identity#email_address_for_sharing to make sure that they won't be used to assign email_address_id of an Identity unless valid for the requested / verified attributes.

It could also be a good idea to have an integration test that has the user walk through a consent flow for different requested attributes and check the resulting behavior / email_address_id value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the test. above. But looking at the calls it looks like this location is the only place that the identity linker is being updated with email address id. the authorization controller and saml_auth_concern. @aduth

Copy link
Member

Choose a reason for hiding this comment

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

Ok in that case I think it makes sense the change to change order to ensure we abort as early as possible in this method if the identity doesn't have the correct requested_attributes. 👍

end
identity = current_user.identities.find_by(service_provider: sp_session[:issuer])
identity&.email_address_id
identity&.email_address_for_sharing&.id
mdiarra3 marked this conversation as resolved.
Show resolved Hide resolved
end

def ial_context
Expand Down
9 changes: 8 additions & 1 deletion app/controllers/sign_up/select_email_controller.rb
mdiarra3 marked this conversation as resolved.
Show resolved Hide resolved
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,13 @@ 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
Expand Down
13 changes: 12 additions & 1 deletion 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 sp_only_single_email_requested?
verified_attributes&.include?('email') &&
!verified_attributes.include?('all_emails')
end

def service_provider_id
service_provider_record&.id
end
Expand All @@ -66,9 +71,15 @@ def happened_at
end

def email_address_for_sharing
mdiarra3 marked this conversation as resolved.
Show resolved Hide resolved
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
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -106,10 +116,32 @@
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_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: '' }) }

Expand All @@ -133,7 +165,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
41 changes: 34 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,11 +2,15 @@

RSpec.describe SignUp::SelectEmailController do
let(:user) { create(:user, :with_multiple_emails) }
let(:sp) { create(:service_provider) }
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

Expand Down Expand Up @@ -75,8 +79,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 +89,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 +101,36 @@
: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 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) { 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 +149,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
3 changes: 3 additions & 0 deletions spec/features/account_connected_apps_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
RSpec.describe 'Account connected applications' do
include NavigationHelper

let(:verified_attributes) { %w[email] }

let(:user) do
create(
:user,
Expand All @@ -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
Expand Down
39 changes: 36 additions & 3 deletions spec/models/service_provider_identity_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,18 @@
last_sign_in_at: 1.hour.ago,
)
end
let(:verified_attributes) { %w[email] }

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,
verified_attributes: verified_attributes,
)
end

context 'when email sharing feature is enabled' do
Expand All @@ -211,7 +220,7 @@
.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
before do
identity.email_address = shared_email_address
end
Expand All @@ -221,7 +230,31 @@
end
end

context 'when an email address for sharing has not been set' do
context 'when service provider has both email and all_emails attribute' do
let(:verified_attributes) { %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(:verified_attributes) { %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 and email is set' do
before do
identity.email_address = nil
end
Expand Down