-
Notifications
You must be signed in to change notification settings - Fork 121
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Tried it out locally and behaved as described in AC
app/controllers/accounts/connected_accounts/selected_email_controller.rb
Show resolved
Hide resolved
@@ -94,7 +94,9 @@ def email_address_id | |||
return user_session[:selected_email_id_for_linked_identity] |
There was a problem hiding this comment.
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?
.
identity-idp/app/controllers/sign_up/completions_controller.rb
Lines 24 to 27 in d32e350
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
@@ -52,7 +52,7 @@ def identity | |||
@identity = current_user.identities.find_by(id: params[:identity_id]) | |||
end | |||
|
|||
def last_email | |||
def last_email_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice clarifying rename 👍
🎫 Ticket
Link to the relevant ticket:
LG-15251
🛠 Summary of changes
This allows Service providers with both 'all_emails' and 'email' attribute bundle to receive the last email sued for sign in instead of the email selected by the user.