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

tid + oid a mistake? #42

Open
s-andringa opened this issue Dec 2, 2024 · 1 comment
Open

tid + oid a mistake? #42

s-andringa opened this issue Dec 2, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@s-andringa
Copy link

s-andringa commented Dec 2, 2024

In #33 was concluded that Microsoft advises to use tid+oid as the uid, and now, starting from version 3 this gem uses that to identify users. From what I can tell, this decision was based on a particular section from this document, stating:

Multi-tenant applications should index on a mapping of two uniquely identifying claims, tid + oid. This will segment tenants by the tid, and segment users by their oid.

However, to me that does not unequivocally imply that oid alone may not be unique across tenants and that the two claims should be concatenated for reliable identification. Many other sources from Microsoft contradict this.

For instance, in the same document is stated that identification should be performed based on a globally unique identifier (GUID), while this page tells us that oid alone is a GUID:

This ID uniquely identifies the user across applications [...]. The oid claim is a GUID and can't be reused.

Here someone from Microsoft confirms that oid is unique:

I assume you are referring to the object ID/oid attribute. If this is the case, the oid claim or ObjectId property is immutable and unique, so it will uniquely identify the relevant directory object. When a single user resides in multiple Entra ID tenants, the user will contain a different object ID/oid in each tenant.

And, finally the most unmistakable statement from this page:

Use claims to reliably identify a user

When identifying a user, it's critical to use information that remains constant and unique across time. [...] Instead, use the claims provided by the OIDC standard, or the extension claims provided by Microsoft - the sub and oid claims.

To correctly store information per-user, use sub or oid alone (which as GUIDs are unique), with tid used for routing or sharding if needed. [...]

At this point, I am not fully convinced that this change and the migration that it necessitates are actually necessary. I’d genuinely appreciate it if you could share any additional sources or reasoning to support this decision.

For now, we are opting out of tid+oid. For anyone who wishes to do the same: it can be done by subclassing the entra_id strategy and defining your own uid builder using the uid macro:

module OmniAuth
  module Strategies
    class EntraIdWithOid < OmniAuth::Strategies::EntraId
      uid { raw_info["oid"] }
    end
  end
end 
@s-andringa s-andringa added the bug Something isn't working label Dec 2, 2024
@pond
Copy link
Member

pond commented Dec 23, 2024

Sorry for delay. RIPA couldn't survive the dire economic conditions caused by our current administration's ill-advised austerity-based approach to financial "management" and closed down a few weeks ago. I don't know if I'll be able to keep maintaining this software in my free time; I'd certainly have to fork it to my own repo and publish from there.

Anyway, I wasn't an expert on the TID + OID stuff and relied upon the submitter of the prior PR for this. It was painful, too, given the need to create a new major version and migrate our own code base. Microsoft's documentation on anything to do with Entra or AAD - and some of it is still called Active Directory, sigh - can be, if I'm being both very charitable and polite, described as a total clusterfuck. If I were being uncharitable and impolite, I'd use stronger language 😂

If it is indeed possible to not have the TID+OID for people confident in that... Well, I'm open to suggestions on how to sort that out in the gem. We don't want yet another new major version / backwards compatibility breakage. Perhaps it's just a question of updating the documentation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants