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 sign-in loop when email is not set in Azure AD account #430

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

heguro
Copy link

@heguro heguro commented Sep 14, 2024

Fix #273, May related: #408, #418

The profile object returned when signing in from Azure AD may be missing the email property.
In this case, the following will occur.

  1. The following code will fail on profile.email.toLowerCase(), so AzureADProvider doesn't return profile and cause an sign-in loop.
  2. The email property is used in the userHashedId function to identify users in the database, but if the email property is undefined, the hashing fails.

In this PR, if the email property is missing, the preferred_username property, which contains the user principal name, is used as the email instead.

(Note: Personally, I believe using an immutable identifier like sub for userHashedId would be more robust than using email or preferred_username (which Microsoft's documentation states are mutable). However, implementing this idea is not included in this PR to maintain compatibility with previously stored data.)

@yasir-fayrooz
Copy link

+1 on this. Was about to make the same PR

@bwitzig-zen
Copy link

+1 for this request.

Some orgs don't have the email field setup in O365 and this will allow them to not require an additional step for onboarding the app

@oliverlabs
Copy link
Contributor

@thivy - this needs to be merged to main.

Copy link
Contributor

@oliverlabs oliverlabs left a comment

Choose a reason for hiding this comment

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

Approved.

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.

Application error: a server side exception has occurred
4 participants