33 improve id token extraction and validation

[tom-brouwer-bex]

Possible solution to solve #33

It:

1. Uses 'tid' + 'oid' for the UUID, since the 'oid' claim used before was not guaranteed to be unique accross tenants
2. Only uses the 'email' claim to derive the email of the user, since, with the correct Entra ID settings, this should contain only verified email addresses
3. Adds claim validation
4. Adds a part to the docs about configuring the entra-id app to only give out verified email addresses

Notes/Questions:

1. Currently the 'access_token' contents are merged with the id_token contents. Do we know if this was exclusively used to derive the 'upn' in the past? In that case, this part should be removed.
2. It seems like there's a seperate token URL for 'custom_policies'. I'm assuming (I can't find anything conflicting this in the docs), that the 'url' in the 'iss' claim never includes the custom policy part.
3. The 'iss' is not validated if a multi-tenant app is used, since the 'iss' will be different depending on the organization the user logs in to.

Note that this should still be operationally tested with both single-tenant and multi-tenant applications

[pond]

I'll have to get to this properly next week but didn't want to leave you in suspense until then. I'll get the questions answered then too. At a glance, the bones of the PR look great and I'm especially happy to see the URLs for relevant documentation in comments. I love that.

Thanks for the work here. I'm pretty sure we will be able to get this through next week; fingers crossed!

[pond]

NB, on question (1): I do note the commentary for raw_info which explains a little, and if I look at the MS docs for ID token vs access token, then decode their V1 and V2 examples of each, I can see that the examples show things like name making more sense in the auth token so overwriting the ID token entry might make sense. Sadly upn is absent from any of those examples so we can't make any deductions about that. Since what is there seems to work, and doesn't seem to be harmful, I'm reluctant to remove that merge.

And having only just said I'd look more into this next week, I can also answer (2) - #19.

I don't think (3) is a question so much as an explanation, and there are already comments in your new implementation to explain that; it seems fine.

[tom-brouwer-bex]

@pond Thanks for the initial feedback.

Regarding (1) in my case at least the 'upn' came from the auth_token when I tested and was not in the ID token, but the commit message that added the merge indeed seems to indicate there's more relevant information to be found in the auth_token. However, I don't know if (1) the same information cannot be found in the id_token, if the proper claims are specified in the 'app registration' configuration in the azure portal and (2) if the 'auth_token' may also contain an email claim, and if so, whether this claim is also scrubbed from the auth token if the email is not validated. If this is not the case, it's a potential risk. Since from my experience Microsoft docs generally cannot be trusted to be complete or correct, I wonder if we can find this out somehow...

Looking forward to your feedback later.

[pond]

OK, going with a V3 option with renamed gem.

On upn, I lost the will to live with MS documentation very quickly and figure I'll just try this out in V3 and see how it performs.

Upgrade guide is going to say this about UIDs:

• If you can determine the tenant IDs for all users in your database, you can just migrate the UIDs. The new UID is just a simple concatenation of tenant ID and object ID, so treating the UID as a string, add the tenant ID as a prefix without any other changes in your migration and things should work fine thereafter.
• Otherwise, you should lazy-migrate:
  • As usual, in your OAuth callback handler, request.env['omniauth.auth'].uid gives the UID - but now that's the "new" Entra gem's value which includes tenant ID.
  • If you can find a user with that ID, then all good - they've been migrated already or got connected to Entra after you started using the updated gem
  • Otherwise, check request.env['omniauth.auth'].extra.oid - this gives the value that the old Azure ActiveDirectory V2 gem used as UID
  • Look up the user with this ID. If you find them, great; remember to migrate their record by updating their stored auth ID to the new request.env['omniauth.auth'].uid value.
  • If the user can't be found by either means, then they've not been connected to your system yet. Your existing handling path for such a condition applies.

[pond]

EDIT: Text below kept for context, but this just turns out to be because the issuer validation code was checking options.tenant_id for nil, but it'll actually be the string "common" if not provided due to multi-tenant use. I've made the relevant amendments in test code and will update PR #37 in due course. Don't worry about the below 😅

EDITED EDIT: Fixed in 16c55b0.

---

@tom-brouwer-bex The PR for v3 at #37 is sufficiently advanced that I decided to test it for real with our own AD integration, so branched our large main app, updated things per the upgrading guide and tried to log in. This fails, because of the new JWT validation:

JWT::InvalidIssuerError, Invalid issuer. Expected ["https://login.microsoftonline.com/common/v2.0"],
received https://login.microsoftonline.com/<ENTRA-TENANT-ID>v2.0

I've tested this, and the tenant ID being used there is that of the MS Entra tenanted user who is logging in. That means in order to validate the JWT, the tenant ID needs to be known in advance. Our application is multi-(our)-tenant, which means in turn that one of our tenants, choosing to sign in using Entra, will have an issuer value that we simply cannot know and cannot possibly specify using the tenant_id option in the OmniAuth gem configuration.

For this reason, either that part of validation needs to be removed, or become optional (which could be complex for people).

Do you have an opinions or suggestions here?

Thanks.

[tom-brouwer-bex]

@pond Your change looks good.

Indeed the code was meant to only validate the issuer for single-tenant apps, because only then you know what tenant-id to expect. I overlooked the part where options.tenant_id was set to 'common' if it was not provided.