Skip to content

Add alternate_emails prop to OIDC userinfo#5442

Merged
jmhooper merged 16 commits intomainfrom
jmhooper-alternate-emails
Oct 20, 2021
Merged

Add alternate_emails prop to OIDC userinfo#5442
jmhooper merged 16 commits intomainfrom
jmhooper-alternate-emails

Conversation

@jmhooper
Copy link
Contributor

Why: So that if SPs have users with multiple email addresses, they can access those email addresses

This is a proposal for dealing with:

a.) A feature request from SPs to be able to see all of a user's email addresses
b.) A feature request from SPs to have a mechanism for selecting email addresses when a user signs in with a PIV (i.e. without an email)

Users with multiple email addresses will have the email addresses they did not use to sign in under an alternate_emails property. Users with only one email address will have an empty array there:

image

Additionally, I updated the consent screen to demonstrate that all of the user's email addresses are being shared. We should likely make this change anyway given that we may share a user's alternate email with an SP if they use it to sign in:

image

**Why**: So that if SPs have users with multiple email addresses, they can access those email addresses
@jmhooper jmhooper changed the title Add alternate_email prop to OIDC userinfo Add alternate_emails prop to OIDC userinfo Sep 24, 2021
@aduth
Copy link
Contributor

aduth commented Sep 27, 2021

For the consent screen, I might worry that it's easy to miss there being multiple emails shared, given the singular "Email address" label and single line of comma-separated emails. It might be helped with a pluralized "Email addresses" and/or one-email-per-line, though I think it'd be good to get UX weigh-in.

@jmhooper
Copy link
Contributor Author

I've added a new scope for the alternate email addresses. I'm thinking through how that should work in the UI for consent.

I'm thinking that we should show all email addresses regardless of which of the scopes is requested. My reasoning here is that any of the email addresses could be shared with the partner even if they aren't in that session. Does that make sense?

I think the other alternative is to split them out into 2 categories. Something like "Email address you sign in with" and "Other email addresses on your account". The "Other email addresses on your account" field would require an empty state.

@zachmargolis
Copy link
Contributor

I've added a new scope for the alternate email addresses. I'm thinking through how that should work in the UI for consent.

I also realized that having a separate scope means this is easier to apply to SAML, it's just another attribute

I'm thinking that we should show all email addresses regardless of which of the scopes is requested. My reasoning here is that any of the email addresses could be shared with the partner even if they aren't in that session. Does that make sense?

I think the other alternative is to split them out into 2 categories. Something like "Email address you sign in with" and "Other email addresses on your account". The "Other email addresses on your account" field would require an empty state.

I'm imagining this is like a new attribute, so when only the existing email is requested we show that default email (maybe label it as "current email"). Only when additional emails are requested do we show a second line for "additional email addresses" and show the full list there

@jmhooper
Copy link
Contributor Author

jmhooper commented Oct 1, 2021

Okay, I've gone ahead and thrown together a UI that we can start cleaning up. Here is what it like in a few cases. I still need to go through and add some tests for the behavior.

Here is signing in without the alternate_emails scope:
localhost_3000_sign_up_completed (3)

Here is signing in with the scope, but no other email addresses:
localhost_3000_sign_up_completed

Here is what it looks like with a single other email address:
localhost_3000_sign_up_completed (1)

Here is what it looks like with more than 2 email addresses:
localhost_3000_sign_up_completed (2)

@zachmargolis
Copy link
Contributor

Okay, I've gone ahead and thrown together a UI that we can start cleaning up

yes! love it

@jmhooper
Copy link
Contributor Author

jmhooper commented Oct 5, 2021

Met up with @anniehirshman-gsa and went over this. Here is the feedback I got:

  1. Let's keep all of the email addresses in the same row
  2. We should use the label to indicate what is being requested. Just email address if they are requesting the sign in email addresses. If they are requesting all email addresses, change the label to "All email addresses on your account"
  3. Each email should be on its own line

And reminder: We still need to consider what happens when information and how consent works there.

sorted_attributes = sorted_attribute_mapping.map do |raw_attribute, display_attribute|
display_attribute if (requested_attributes & raw_attribute).present?
end.compact
# If the SP requests all emails, there is no reason to show them the sign
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to add a test for this behavior

@jmhooper
Copy link
Contributor Author

jmhooper commented Oct 8, 2021

Okay, I've got some UI updates incorporating @anniehirshman-gsa's and @zachmargolis's feedback.

Here is what it looks like if the SP requests just the sign in email. No changes here:
image

Here is what it looks like if the SP requests all email addresses:
image

Here is what it looks like if the SP requests all email addresses and there is only 1 email address:
image

Finally, no need for an empty state since we changed the attribute to "all emails" instead of "all emails that aren't the sign in one".

@anniehirshman-gsa
Copy link
Contributor

Just one typo: label should be Email addresses on your account instead of Emails addresses. Otherwise looks great, thanks!


.success-bullets {
li {
.success-bullet {
Copy link
Contributor

@aduth aduth Oct 12, 2021

Choose a reason for hiding this comment

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

We're also using this success-bullets class here, which hasn't been updated yet in this branch to apply the new class to its li children:

Note to self: We should consider moving to design system Icon List component.

Edit: Ticket at LG-5226.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the reason for renaming the class was to prevent it from apply the success bullets list styles to this list item.

There may be a better way to accomplish that, but I don't know what it is.

)
end

context 'the all_emails scope is requested' do
Copy link
Contributor

Choose a reason for hiding this comment

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

should/can we add a negative spec (ex bar@example is not shown when only single email scope is requested?)

@jmhooper
Copy link
Contributor Author

@anniehirshman-gsa: Good catch on the typo. I fixed when I added translations

@zachmargolis and @aduth: I added tests, translations, and some other finishing touches. PTAL and lmk what you think!

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM! only gap is SAML but this seems good enough for now. Should we track with a JIRA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants