-
Notifications
You must be signed in to change notification settings - Fork 66
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
Conversation
There was a problem hiding this 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
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}}. |
There was a problem hiding this comment.
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:
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|. |
There was a problem hiding this comment.
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?
LGTM @bvandersloot-mozilla WDYT? |
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 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. |
It is possible for small IdPs yea, but I think this can be prevented through proper developer documentation.
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
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. |
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! |
Great!
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. |
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. |
There is a semantic difference for sure.
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. |
Ah, I missed that!
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 |
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.
No more hints lurking to the best of my knowledge :) Let me know if this PR makes sense to merge from your perspective. |
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 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).
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. |
SHA: 828e96b Reason: push, by samuelgoto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 828e96b Reason: push, by pull[bot] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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, thedomainHint
andloginHint
are passed as query parameters in the IDP login URL.Preview | Diff