Fix Microsoft OAuth callback for Graph users whose mail field is null

[jbaxendale-ut]

The strategy was mapping OmniAuth’s normalized info.email directly from Microsoft Graph’s mail field, then using that email during domain verification.

Microsoft Graph can return mail: null for valid users, while still returning userPrincipalName. In that case, domain verification tried to compare a nil email domain and the OAuth callback failed before the app could store the token.

Now falls back to userPrincipalName when mail is missing, and the domain verifier is nil-safe. Domain verification still runs; this only changes the identity value used when Graph does not provide mail.

---

^{Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.}

[Copilot]

Pull request overview

This PR addresses OAuth callback failures for Microsoft Graph users whose mail attribute is null by falling back to userPrincipalName for OmniAuth’s normalized info.email, and by making domain verification logic robust to missing email values.

Changes:

• Fallback info.email to raw_info["userPrincipalName"] when raw_info["mail"] is nil.
• Update DomainVerifier to compute an “effective email” (email or UPN) and avoid nil-unsafe domain comparisons.
• Add specs covering the fallback behavior and domain verification outcomes when mail/email is missing.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
spec/omniauth/strategies/microsoft_graph_oauth2_spec.rb	Adds coverage ensuring info.email falls back to userPrincipalName when mail is missing.
spec/omniauth/microsoft_graph/domain_verifier_spec.rb	Adds scenarios for missing email/UPN and for disabled verification with missing identity fields.
lib/omniauth/strategies/microsoft_graph.rb	Implements the mail → userPrincipalName fallback for info.email.
lib/omniauth/microsoft_graph/domain_verifier.rb	Uses an effective email fallback and makes the domain checks nil-safe.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jbaxendale-ut]

Some ongoing concerns that I'm not going to rush to change this yet:

› are we ever doing a new mismatch between those two fields, or were we doing one before?

• No new allowed mismatch is being introduced.

Before this change, when both fields existed:

• info.email came from Graph mail
• domain verification compared mail’s domain to userPrincipalName’s domain
• if domains differed, verification failed unless xms_edov or skip_domain_verification allowed it

After this change, that behavior is still the same when mail is present.

The only changed case is when mail is absent:

• before: info.email = nil, then verification could fail because there was no email domain to compare
• now: info.email = userPrincipalName, and verification compares the UPN-derived domain against the UPN domain, so it passes as the verified Microsoft identity domain

One nuance: the verifier has always compared domains, not the full address. So mail = jane@example.com and userPrincipalName = j.smith@example.com is accepted because the domains match.
But mail = jane@other.com and userPrincipalName = j.smith@example.com is still rejected unless one of the explicit bypass/verification mechanisms applies.

› If we are falling back to userPrincipalName is there any concern about a true == true situation, in that data that should be invalid or incompatible is now passing?

• Yes, there is a real “self-comparison” shape in the fallback case, but it is not creating a new mail/UPN mismatch bypass.

When mail is present, we still compare:

mail domain == userPrincipalName domain

So incompatible data like:

mail: jane@unverified.example
userPrincipalName: jane@company.com

still fails unless xms_edov or an explicit skip option allows it.

When mail is missing, there is no second email claim to validate against. The fallback changes the comparison to effectively:

userPrincipalName domain == userPrincipalName domain

That passes by construction. The security argument is that userPrincipalName is coming from Microsoft Graph and its domain is supposed to be one of the tenant’s verified domains, so
this is not accepting an arbitrary user-controlled mail value. It is accepting the Microsoft principal address as the identity email when Graph provides no mailbox email.

The main concern is semantic, not the original domain-mismatch vulnerability: downstream code may treat info.email as a real deliverable mailbox address. userPrincipalName is usually
email-shaped, but it is primarily a login principal, not guaranteed to be the user’s mailbox. If this gem’s contract is “give me a verified Microsoft identity address,” the fallback is
reasonable. If the contract is “give me the user’s mailbox email,” then falling back to UPN can be surprising.

A possible hardening improvement would be to require the fallback UPN to be email-shaped and have a non-empty domain before accepting it, instead of relying on split('@').last alone.

› it sounds like we could be getting an email mismatch now then, in regards to setting the omni email path to something that is potentially invalid?

• Yes. That is the tradeoff.

The change does not allow a present mail value to mismatch userPrincipalName. But it does mean OmniAuth info.email can now be populated with userPrincipalName when Microsoft Graph gives
us no mail.

So the new possible behavior is:

raw_info["mail"] # nil
raw_info["userPrincipalName"] # "user@tenant.onmicrosoft.com"

info["email"] # "user@tenant.onmicrosoft.com"

That value is verified as a Microsoft principal domain, but it may not be the user’s real mailbox address or a deliverable email. Before this change, info.email would have stayed nil.

So I’d describe the concern this way:

• Security mismatch: probably not new, because there is no conflicting mail value being accepted.
• Data semantics mismatch: yes, info.email may now contain a login principal rather than a mailbox email.
• Validity concern: yes, if downstream code assumes info.email is deliverable email, this fallback may be too broad.

A cleaner version might keep domain verification fallback logic, but avoid assigning UPN into info.email, or only do so behind an option like fallback_email_to_user_principal_name:
true. Another middle ground is to only fallback when UPN is clearly email-shaped, but that still does not prove it is a mailbox.

[jbaxendale-ut]

› why would we get a null 'mail' but not 'principal'

• Because mail and userPrincipalName are not the same kind of field.

userPrincipalName is the Microsoft Entra sign-in principal. Microsoft requires it when a user is created, and its domain must be in the tenant’s verified domains. So a real Entra user
normally has one.

mail is the user’s SMTP email/mailbox address. It can be absent even when the account is valid, for example if:

• the user has no Exchange mailbox/license
• the mailbox has not been provisioned yet
• the tenant is hybrid/on-prem synced and the source mail attribute is empty
• it is a guest, B2C, service-style, or otherwise nonstandard user account
• the user signs in with a UPN that is not their mailbox address

So this shape is plausible:

{
"mail": null,
"userPrincipalName": "user@company.com"
}

That means “Microsoft knows who this user is and what verified tenant principal they sign in with,” but Graph is not giving us a mailbox SMTP address.

Microsoft’s Graph docs describe mail as the user’s SMTP address, while userPrincipalName is the required sign-in name whose domain must be in the tenant’s verified domains:
https://learn.microsoft.com/en-us/graph/api/resources/user?view=graph-rest-1.0#properties

› so when would it be valid to fallback to principal? is that safe for a situation like oauth flows?

• It is valid to fall back to userPrincipalName only if the consuming app treats that value as a Microsoft account principal, not as a guaranteed mailbox email.

For OAuth flows, the safe identity key should be something immutable from Microsoft, usually Graph id / token oid, often paired with tid for tenant context. Microsoft explicitly warns
not to use email, preferred_username, unique_name, or upn for authorization or durable user storage because they are mutable. See Microsoft’s claims guidance:
https://learn.microsoft.com/en-us/entra/identity-platform/claims-validation and optional claims reference:
https://learn.microsoft.com/en-us/entra/identity-platform/optional-claims-reference.

So:

• Safe-ish: using UPN as a display/login hint, or as a fallback “user handle” when mail is absent.
• Safe-ish: using UPN domain as evidence of tenant verified-domain membership, because Graph says UPN’s domain must be in the tenant’s verified domain collection.
• Not safe: using fallback UPN as a deliverable email address.
• Not safe: using fallback UPN to link an OAuth login to an existing local user by email.
• Not safe: using fallback UPN/email domain alone for authorization.

For this gem specifically, the current fix is defensible for “don’t fail a valid Microsoft OAuth callback when mail is null.” But it does blur the meaning of info.email. A stricter
design would keep:

info["email"] = raw_info["mail"]

and put UPN somewhere else, while letting domain verification use UPN when mail is absent. That avoids teaching downstream apps that UPN is definitely the user’s email address.

[jbaxendale-ut]

› So this branch will now tell clients that the user's email is the principal, whereas maybe it should be doign so

• Yes. This branch currently tells clients:

info["email"] = raw_info["mail"] || raw_info["userPrincipalName"]

So when mail is null, clients see the principal as the user’s OmniAuth email. That may be the wrong contract.

A cleaner split would be:

info.email = Graph mail only
extra.raw_info = full Graph response, including userPrincipalName
domain verifier = may use userPrincipalName when mail is absent

That way the callback can still succeed for users with mail: nil, but the gem does not misrepresent UPN as a mailbox email. Consumers that want to use UPN can read it explicitly from
extra.raw_info["userPrincipalName"].

I’d frame the safer fix as: “make domain verification tolerate missing mail when UPN is present, but do not populate info.email from UPN by default.” This avoids breaking the original
nOAuth guard while preserving the meaning of info.email.

[jbaxendale-ut]

Holding off on this for now, as I'm not totally convinced these changes are "correct" and might ask with the host gem that understands more about the logic