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

BREAKING: Treating the User Principal Name as an email address is now opt-in #37

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

seanfisher
Copy link
Owner

Previously, if the Graph API did not return a value in the mail field, the User Principal Name (UPN) would be substituted automatically. This was beneficial for those who follow the recommended convention of having the UPN be the same as the email address.

However, the UPN does not always reflect the user's email address and can be changed by an administrator.

To avoid consuming applications blindly relying on an assumption that UPN and email is always the same, this behavior is now opt-in. If a valid email is found in the mail field, it will be added to the emails array in the Passport.js profile. If the new option, addUPNAsEmail, is set to true in the configuration, the UPN will be added into the emails array as well. This means there could be a scenario where zero, one, or two email address are found in the profile's emails field.

For those who wish to maintain the semantic separation of email and UPN, the UPN is also available now as a separate property on the profile, userPrincipalName.

On a related note, it is not recommended for applications to use one of the emails or the UPN as a user identifier, as these values can be changed and are not guaranteed to represent a stable identity. Instead, use the id field.

@seanfisher seanfisher merged commit 60169f4 into master Mar 21, 2024
@seanfisher seanfisher deleted the upn-as-email-option branch March 21, 2024 19:30
@robert-bo-davis
Copy link

On a related note, it is not recommended for applications to use one of the emails or the UPN as a user identifier, as these values can be changed and are not guaranteed to represent a stable identity. Instead, use the id field.

This should probably be highlighted in big bold red letters in the docs because trusting the email returned by the common endpoint is even more insidious than it is described here.

There is no ownership validation for the email field set in a user profile in a Entra ID user. So an application that uses the common endpoint for global ms auth and also uses the email field as an access identifier is open to an attack where an attacker could create an Entra ID user in their own account with an arbitrary email address, auth with those credentials, and gain access to an account linked to that email address.

Depending on email-ish UPNs as an identifier is a little more safe using the common endpoint because there is domain ownership validation, but unless you are specifying a tenant id and establishing trust that way, the email returned by the graph profile should never be trusted as an access identifier.

Thanks for the great work on this library! I'm happy to submit a docs PR, but wanted to discuss first.

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.

None yet

2 participants