nhost / hasura-auth

Authentication for Hasura.
https://nhost.io
MIT License
376 stars 111 forks source link

fix #404 Azure not storing users email #405

Closed hk-meko closed 1 year ago

hk-meko commented 1 year ago

Fixes #404

changeset-bot[bot] commented 1 year ago

⚠️ No Changeset found

Latest commit: 1b574657643c739358d1d3356fe9fc8be3bb6a0b

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 commented 1 year ago

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 commented 1 year ago

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

dbarrosop commented 1 year ago

good catch!

dbarrosop commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

hk-meko commented 1 year ago

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

xmlking commented 6 months ago

@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 commented 6 months ago

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

image image