search

fedidcg / FedCM

A privacy preserving identity exchange Web API
https://fedidcg.github.io/FedCM
Other
357 stars 66 forks source link

Add domainHint to the spec #512

Closed npm1 closed 6 months ago

npm1 commented 7 months ago

Fixes https://github.com/fedidcg/FedCM/issues/427. Adds a domainHint parameter that can be used in FedCM to filter accounts based on the corporate domains the belong to. This behaves similarly to login hint, except with the additional behavior that "*" matches any domain hint. In addition, the domainHint and loginHint are passed as query parameters in the IDP login URL.

Preview | Diff

samuelgoto commented 7 months ago

LGTM

@bvandersloot-mozilla WDYT?

bvandersloot-mozilla commented 7 months ago

It will be confusing for a newcomer to distinguish domain_hint and login_hint and which they want/need.

Thinking a bit more, couldn't we just make the Since they aren't used semantically as domain names, couldn't this be achieved by making IdentityProviderConfig.login_hints a sequence and failing when all members are not present in an account? The all case would require some finagling by the IDP, sure, but the reduced API complexity may well be worth it.

My concerns of adding more and more IDP-logic features into this API continue to rise, and this is an example of why. Nothing is wrong with this per-se but as it grows it is harder to get right, where we could defer to the experience of existing deployments of identity plaftorms.

npm1 commented 7 months ago

It will be confusing for a newcomer to distinguish domain_hint and login_hint and which they want/need.

It is possible for small IdPs yea, but I think this can be prevented through proper developer documentation.

Thinking a bit more, couldn't we just make the Since they aren't used semantically as domain names, couldn't this be achieved by making IdentityProviderConfig.login_hints a sequence and failing when all members are not present in an account? The all case would require some finagling by the IDP, sure, but the reduced API complexity may well be worth it.

Interesting idea! That could work, athough semantically it might be hard for IdPs. They'd need to add two different things to the same array and also figure out which corresponds to which when parsing the login_url which includes those fields. I actually think having the semantic separation makes it easier, not harder, for IdPs. This is why this field exists in Google Sign-In and also apparently in Microsoft[1]. Priority of constituents would imply that we should prioritize the IdP needs, WDYT?

[1] https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-oapx/86fb452d-e34a-494e-ac61-e526e263b6d8

My concerns of adding more and more IDP-logic features into this API continue to rise, and this is an example of why. Nothing is wrong with this per-se but as it grows it is harder to get right, where we could defer to the experience of existing deployments of identity plaftorms.

That is a valid concern regarding amount of features, although this one in particular is before user agrees to use federation, so in some sense is required to provide a good UX for cases where only certain accounts are valid.

bvandersloot-mozilla commented 6 months ago

Given that this exists in Microsoft identity too, I'm more apt to include it.

Is it possible that domainHint could be something more specific than a String? Or automatically populated? It feels weird to leave it so vague and effectively duplicative. I'm just not familiar with its typical use!

npm1 commented 6 months ago

Given that this exists in Microsoft identity too, I'm more apt to include it.

Great!

Is it possible that domainHint could be something more specific than a String? Or automatically populated? It feels weird to leave it so vague and effectively duplicative. I'm just not familiar with its typical use!

Hm well initially we were hoping to use something like trying to match the suffix of the email or something like that, which would make it automatic. However, we heard that this does not work in all cases: some domains go by multiple names so at least one would not really match anything about the user email. Given the requirement for this kind of flexibility, it's not possible to compute the domain hints automatically.

bvandersloot-mozilla commented 6 months ago

Looking more closely at Microsoft's use case, they don't have a second type of hint. So I would expect that "domain_hint" for Microsoft could just be a "login_hint". Which brings me back to my original thought: is there any semantic difference between a domain hint and login hint?

Priority of constituencies would guide us to building the easiest API to understand for all consumers, not just reproducing the conventions of one consumer into the API as an additional feature. It's my expectation that an identity dev would see an array of login_hints as an obvious place to stuff a domain_hint.

npm1 commented 6 months ago

Looking more closely at Microsoft's use case, they don't have a second type of hint. So I would expect that "domain_hint" for Microsoft could just be a "login_hint". Which brings me back to my original thought: is there any semantic difference between a domain hint and login hint?

There is a semantic difference for sure. login_hint answers the question "which is an identifier for the user I want"? whereas domain_hint answers the question "which corporation/server must this account belong to?". The former usually targets a single user whereas the latter targets a set of users. Because both are essentially account filters, they look similar in the spec, but the hopefully that clarifies the semantic difference? In fact, you can see in the Microsoft example that they also have login_hint so the semantic separation between these is there in that provider as well.

Priority of constituencies would guide us to building the easiest API to understand for all consumers, not just reproducing the conventions of one consumer into the API as an additional feature. It's my expectation that an identity dev would see an array of login_hints as an obvious place to stuff a domain_hint.

I disagree with this assessment. We have at least two examples of developers separating these, and due to the semantic difference explained above, I do believe an IdP implementing one or the other would not want to combine them.

bvandersloot-mozilla commented 6 months ago

In fact, you can see in the Microsoft example that they also have login_hint so the semantic separation between these is there in that provider as well.

Ah, I missed that!

I disagree with this assessment. We have at least two examples of developers separating these, and due to the semantic difference explained above, I do believe an IdP implementing one or the other would not want to combine them.

I'm not sure why they wouldn't, since they function identically. But you have done more outreach to IdPs than I have, so maybe there is insight there I don't get.

Given this, I suppose it makes sense, but isn't my preferred way of going about it. Are there more _hints lurking around the corner?

npm1 commented 6 months ago

I'm not sure why they wouldn't, since they function identically. But you have done more outreach to IdPs than I have, so maybe there is insight there I don't get.

What I mean is that IDPs that support both would probably not combine them because having them separate is easier to reason about. Plus if you combine them then you'd have to figure out how to untangle them from the query parameters in URLs. Whereas by not combining them you do not need to guess which is is login and which one is domain hint.

Given this, I suppose it makes sense, but isn't my preferred way of going about it. Are there more _hints lurking around the corner?

No more hints lurking to the best of my knowledge :)

Let me know if this PR makes sense to merge from your perspective.

bvandersloot-mozilla commented 6 months ago

This change in and of itself is reasonable, but I do still concerns over increased IDP logic in the API instead of rearchitecting to use an IDP navigatable as the way to handle several similar extensions.

samuelgoto commented 6 months ago

This change in and of itself is reasonable, but I do still concerns over increased IDP logic in the API instead of rearchitecting to use an IDP navigatable as the way to handle several similar extensions.

I tend to agree with both of you: I think that @npm1 is correct in thinking that these are semantically different enough that it is worth keeping them separate, but I also agree with you @bvandersloot-mozilla that we have to try to look at the API as holistically as possible (and yeah, I am not personally aware of any further _hints).

Some things are easier seen after the fact, as more IdPs use these extensions and tell us what works and what doesn't work. To the best of my knowledge, I think this is a reasonable approximation, but we are open to be told otherwise (and potentially incur in paying a price of backwards incompatibility).

This change in and of itself is reasonable

Either way, I'm planning to merge this PR since it seems like we have arrived at something that seems reasonable as a way to move forward, and let ourselves be open to revisiting this as we gather more deployment experience.

LMK if you feel strongly otherwise, but if not, I'll merge this momentarily to unblock forward progress.