Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -52,7 +52,7 @@ def identity
@identity = current_user.identities.find_by(id: params[:identity_id])
end

def last_email
def last_email_id
Copy link
Copy Markdown
Contributor

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
3 changes: 2 additions & 1 deletion app/controllers/concerns/saml_idp_auth_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

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
end

Expand Down
8 changes: 8 additions & 0 deletions app/models/service_provider_identity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down 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,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

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

Expand Down
101 changes: 101 additions & 0 deletions spec/controllers/openid_connect/authorization_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
72 changes: 72 additions & 0 deletions spec/controllers/saml_idp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 +
Expand Down
16 changes: 9 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,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)
Expand Down Expand Up @@ -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 }

Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion spec/features/account_connected_apps_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading