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

fix #404 Azure not storing users email #405

Closed
wants to merge 1 commit into from

Conversation

hk-meko
Copy link

@hk-meko hk-meko commented Jul 11, 2023

Fixes #404

@changeset-bot
Copy link

changeset-bot bot commented Jul 11, 2023

⚠️ No Changeset found

Latest commit: 1b57465

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dbarrosop
Copy link
Member

Thanks for the PR, my only concern is that the docs say this:

The username of the user. May be a phone number, email address, or unformatted string. Only use for display purposes and providing username hints in reauthentication scenarios.

Not sure if we should validate the value and return an error if it's not an email.

@hk-meko
Copy link
Author

hk-meko commented Jul 11, 2023

You are right, but if I see it correctly there is already a validation set up.

@dbarrosop
Copy link
Member

good catch!

@dbarrosop
Copy link
Member

Reading more about this, I am not an Azure AD expert but I am thinking this may not be correct. The UPN doesn't seem to be the email and googling around a bit you should be able to ask for the email claim:

https://stackoverflow.com/questions/65128383/cant-get-email-claim-in-access-token-in-azure-ad

@onehassan
Copy link
Member

@hk-meko Are we sure that the upn is the user's email address? As I read here a user's UPN might or might not be the same as the email address of the user

@hk-meko
Copy link
Author

hk-meko commented Jul 11, 2023

You guys are right, working with the AD admins, we found you need to add the 'Microsoft Graph email' API permission, as well as including it as an optional claim in the id token as stated here and described here.
I'm gonna check that again tomorrow, I think this PR can then be closed.
Thank you for challenging this!

@dbarrosop
Copy link
Member

Thanks! closing PR and issue, feel free to re-open if needed.

@hk-meko
Copy link
Author

hk-meko commented Jul 12, 2023

Could be considered, adding the info to the docu in the future.

@xmlking
Copy link
Contributor

xmlking commented Feb 1, 2024

@dbarrosop

stackoverflow https://stackoverflow.com/a/49679604 suggest, email can be mapped to different fields based on what azure subscription company has:

For personal accounts, the email address is returned in an email field like one would expect.
For company accounts, the email address is returned in a preferred_username field.

For my case Azure AD is sending email at two places: 1. preferred_username 2. upn

image

can you provide options for developers to map email address via configuration ? or may be fallback with:

return {
  id: payload.oid,
  displayName: payload.name,
  email: payload.email ?? payload.preferred_username ?? payload.upn
};

@xmlking
Copy link
Contributor

xmlking commented Feb 1, 2024

b.t.w I also tried adding Permissions suggested above, but email claim is not comming.

image image

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.

Signing up with Azure AD does not set the users email
4 participants