Non verified emails can be used for authentication and uid may not be unique

[tom-brouwer-bex]

Describe the bug

1. The strategy defaults to 'UPN' if no email is provided. However, if the 'email' claim is not provided in an ID tokens, this means the email address is not verified (At least: For recent app registrations with default settings).
2. The strategy uses 'oid' as the 'uid'. However, this identifier is only unique within a single tenant, meaning there's a chance of colisions for multi-tenant app registrations. Microsoft advises to use the combination of tid + oid as a unique identifier.

To Reproduce In an Azure tenant that does not use office 365 (does not assign mailboxes):

1. Add a random verified domain (e.g. a domain for which actually Google Workspace email is used)
2. Add a new user on that domain to Entra ID
3. (Note that ownership of the email-inbox is never verified)
4. You can now login with this account (There's no 'email' claim in the token, but there's a 'upn' extracted from the access token)

Note that Microsoft also supports on-premise AD to Azure sync, so it may be possible to exploit this even without having the domain itself verified, I'm not in a position to check this

Expected behavior

1. Only people that have verified email address for their Microsoft Account are succesfully authenticated by the strategy
2. 'uid' is still unique if the strategy is used with a multi-tenant app registration

Note that both these changes are not backwards compatible!

[pond]

@tom-brouwer-bex

Thanks for the report and the details, including links.

Sorry for the slow response here. Certainly can't see a way to fix this in a backwards-compatible way, so I suppose something like a deprecation or other warning on the current gem to try and alert existing users to a need to upgrade, followed by a major version bump, would be the way forward.

Leave it to Microsoft to have unverified users able to sign in via token auth, then making it everyone else's problem.

If I'm reading things correctly:

• The fallback to upn should be dropped and an appropriate "cannot auth" error returned in its place
• The uid call inside the strategy needs to concatenate TID and OID as you suggest

...except that (this part of the documentation from Microsoft)[https://learn.microsoft.com/en-us/entra/identity-platform/migrate-off-email-claim-authorization#using-the-xms_edov-optional-claim-to-determine-email-verification-status-and-migrate-users] seems to suggest that the first bullet point above isn't correct, and perhaps the gem needs to be checking xms_edov? Further, there appears to be a (giant list of additional things)[https://learn.microsoft.com/en-us/entra/identity-platform/claims-validation] that Microsoft seem to imply an OAuth client has to do - even though (via OAuth) a user has successfully logged in to a valid Microsoft account via e-mail address, password and OTP.

I'm probably missing some of the point here; I always find Microsoft documentation extremely verbose yet simultaneously obtuse and confusing. It's a strange combination of explaining things in great detail while also assuming the reader has a truly vast amount of Microsoft-specific knowledge and understanding of precise per-document domain background context.

[tom-brouwer-bex]

@pond

Thanks for your response. I completely agree with your comments about Microsoft docs, and the way they handled email claims in the past. It all looks quite inconsistent on their side, and also their docs sometimes seem to conflict one-another. I mean, the entire goal of acting as an 'identity provider', is that you provide 'verified' email addresses right? Otherwise it's an identity provider that can't be trusted, making it a useless one...

Anyway, regarding your comment on checking xms_edov:

1. The article says: To secure applications from mistakes with unverified email addresses, all new multi-tenant applications are automatically opted-in to a new default behavior that removes email addresses with unverified domain owners from tokens as of June 2023, and there's a method to enable this for existing applications. So for new Azure app registrations this property is not needed as far as I can see, and for older ones, they can also be configured so that it's not needed.
2. If we would check the xms_edov value, all users of this package would have to enable it. It seems like this cannot easily be done from the azure portal, so I would not recommend making this attribute a requirement to be able to use this gem.

Hence I think the best way to go I think is to (1) include the first point in the readme and possibly link to it in the deprecation warning, and (2) use only the 'email' claim without the UPN as fallback.

Regarding your comment about claims validation: This is something for example 'omniauth-google-oauth2' already does, and is relatively straightforward to implement. Although we get the tokens in the back-end via a HTTPS connection, doing the additional claims validation can't hurt I think, and aligns better with the official OpenID Connect specification.

I can create a PR to implement the 3 changes (Remove UPN fallback, use tid + oid as UID, add the claims validation), but how do we want to handle the deprecation warning?

Another option, instead of creating a new major version of this package, which people may mindlessly update to (especially if they didn't implement strict version requirements in their gemfile), may I suggest actually renaming the package? Since the service is now called 'Microsoft Entra ID', perhaps a rename to 'omniauth-microsoft-entra-id' is in place?

[pond]

@tom-brouwer-bex Well...

• On the one hand, renaming can create big problems for migration - e.g. in our own database we store the name of the auth provider with which a given user in a given tenancy has signed in (our local concept, I don't mean Azure's idea of tenancy). That'd need migration and it'd be a thorny thing to do without downtime.

• On the other hand, I'm sure this gem faces a growing discovery issue as fewer and fewer people are going to know of it as Azure AD rather than Entra in the future.

So I think your idea is fundamentally correct. This isn't something I've done before, but the answer is probably as simple as renaming this repo with a branch for a "new version" of the gem that includes its new name, since GitHub apparently just redirects everything for you:

• https://docs.github.com/en/repositories/creating-and-managing-repositories/renaming-a-repository

I'm certainly not going to say "no" to a PR that comes in here! I'm very short on time right now; we're rather under-resourced (to put it mildly) at present. I've created a target branch for a V3 gem which will include renaming to Entra - see https://github.com/RIPAGlobal/omniauth-azure-activedirectory-v2/tree/v3_with_entra_rename. Please target a PR there - no need to do the rename bits; just your preferred approach to resolve the issues.

If you find you might not have time or encounter difficulties please "@"-me in a comment on this thread & we can work together on it.

[tom-brouwer-bex]

@pond

I've added a draft PR here. I've already added some tests for the code updates. Once we have agreed on the code itself, I can also do some manual tests with single-tenant and multi-tenant entra ID apps.

Regarding the name change: You could rename the Repo, but I think the more standard thing may be to actually (1) fork it to a repo with the new name (2) keep this repo for reference, add a deprecation notice with a link to the new repo in the readme, (3) archive this repo. This way people are still able to find the gem & repo by the old name.

Perhaps we should also consider adding a 'migration guide' then (mainly to indicate that the uids have changed, and that the old 'uid', is basically the suffix of the new one).