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

Add domainHint to the spec #512

Merged
merged 4 commits into from
Jan 2, 2024
Merged

Add domainHint to the spec #512

merged 4 commits into from
Jan 2, 2024

Conversation

npm1
Copy link
Collaborator

@npm1 npm1 commented Nov 9, 2023

Fixes #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

@npm1 npm1 requested a review from cbiesinger November 9, 2023 20:49
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
Copy link
Collaborator

@cbiesinger cbiesinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated
Comment on lines 774 to 782
1. If {{IdentityProviderConfig/domainHint}} is "any", remove |account| from
|accountList| if |account|'s {{IdentityProviderAccount/domain_hints}} is not empty.
1. Otherwise, remove |account| from |accountList| if |account|'s
{{IdentityProviderAccount/domain_hints}} does not [=list/contain=] |provider|'s
{{IdentityProviderConfig/domainHint}}.
Copy link
Contributor

@TallTed TallTed Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This includes the fix suggested by @samuelgoto.

It is not clear to me what "otherwise" refers to, so it is not clear how to rephrase it explicitly (which would be better). Here's my best guess:

Suggested change
1. If {{IdentityProviderConfig/domainHint}} is "any", remove |account| from
|accountList| if |account|'s {{IdentityProviderAccount/domain_hints}} is not empty.
1. Otherwise, remove |account| from |accountList| if |account|'s
{{IdentityProviderAccount/domain_hints}} does not [=list/contain=] |provider|'s
{{IdentityProviderConfig/domainHint}}.
1. If {{IdentityProviderConfig/domainHint}} is "any" and |account|'s
{{IdentityProviderAccount/domain_hints}} is empty, remove
|account| from |accountList|.
1. If {{IdentityProviderConfig/domainHint}} is not "any" and
|account|'s {{IdentityProviderAccount/domain_hints}} does not
[=list/contain=] |provider|'s {{IdentityProviderConfig/domainHint}},
remove |account| from |accountList|.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the previous step into two so hopefully the "otherwise" is clear now?

spec/index.bs Outdated Show resolved Hide resolved
@samuelgoto
Copy link
Collaborator

@bvandersloot-mozilla
Copy link
Collaborator

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
Copy link
Collaborator Author

npm1 commented Dec 1, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator Author

npm1 commented Dec 7, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator Author

npm1 commented Dec 21, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator Author

npm1 commented Dec 21, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@samuelgoto samuelgoto merged commit 828e96b into main Jan 2, 2024
2 checks passed
@samuelgoto samuelgoto deleted the domainHint branch January 2, 2024 16:32
github-actions bot added a commit that referenced this pull request Jan 2, 2024
SHA: 828e96b
Reason: push, by samuelgoto

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to mattdanielbrown/WebID that referenced this pull request Jan 4, 2024
SHA: 828e96b
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants