Microsoft Entra ID authentication with multi-tenancy

[gvkries]

Fixes Microsoft Entra ID authentication for multi-tenant app registrations by adding missing token validation.

Fixes #14802

[sebastienros]

@MichaelPetrinolis what do you think?

[MichaelPetrinolis]

@sebastienros when someone registers an AAD application for accounts in any organizational directory, he allows anyone with an AAD account to authenticate through this app.

@gvkries I think that we should also define the audiences (tenants) we wish to give access to the OC site and validate for those audiences, otherwise anyone with an enterprise Microsoft account could authenticate and then the script could potentially assign app roles to him.

If needed, an option to allow any tenant should be explicitly selected by the admin when configuring Microsoft Entra ID, but we should clearly state the consequences and provide an example with a secure script to assign roles (check the issuer of the claims etc.)

//cc: @kevinchalet what are your thoughts on this?

[gvkries]

This PR just fixes the missing validation when multiple Azure AD tenants should be allowed. It must be explicitly configured in the Orchard configuration by setting the tenant ID to "common", so it has no security implications by default.

Configuring the allowed tenants would be a nice addition, but in many cases this is not the intention. We want to allow any user to register with their organization account the same way as with personal Microsoft (or other) accounts. It is also already possible to use the registration script to configure allowed tenants, by inspecting the tenant ID claim.

[MichaelPetrinolis]

@gvkries it is nice that to enable it apart from configuring the App Registration in Entra ID you must also explicitly configure OC (we should update the docs about setting the value 'common' to TenantId). My concern is that we should also make it clear to anyone who enables the feature to correctly configure any role assignments through script or code with additional checks to avoid any security side effects. Unfortunately, it is not possible for the moment to deny login with the IExternalLoginEventHandler interface.
Indeed, configuring a valid audience (list of allowed tenants) could be another PR unless Entra ID already supports it.

[gvkries]

@MichaelPetrinolis that is true, one cannot deny login with the script. I'm not that concerned about the login script, because it also must be explicitly written by an admin. There are no defaults that assign any roles at all, even the app registration must be configured manually before. Anyone configuring the authentication/registration settings are already exposed to a lot of possibly dangerous settings and hopefully know what they are doing. But documentation is always a good thing 😊

[Piedone]

A couple of questions, but otherwise I like this. When addressing review feedback, please adhere to the following:

• Apply code suggestions directly so the reviewer doesn't have to eyeball the changes. These resolve themselves after applying them, that's fine.
• Don't resolve other conversations so it's easier to track for the reviewer. Then, the reviewer will resolve them.
• Feel free to mark conversations that you addressed to keep track of them with an emoji or otherwise, just don't resolve them.
• Please keep conversations happening in line comment in those convos, otherwise communication will be a mess. If you have trouble finding them, see this video.
• Please click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.

[Piedone]

Please don't forget to re-request review from the top-right corner, otherwise reviewers won't know that you're done.

[Piedone]

Great, thank you!