Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 loginHint, RP context, and getUserInfo #470
Add loginHint, RP context, and getUserInfo #470
Changes from all commits
6210078
28f0682
c57d774
8372292
e0024bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 need to read into this more, but I have concerns. This seems to duplicate a lot of structure from the IdentityProviderAccount and the Credential discovery algorithms. I worry about desync- there is already a given_name givenName confusion happening.
I get that you probably don't want to just cache the user info to prevent stale information. But I don't think we need to necessarily hang fresh info on the Window object and duplicate structs.
An alternative is to put a similar function on the IdentityCredential interface that takes no arguments and returns an IdentityProviderAccount. This may require adding in the IDP config URI or even just the base domain to the IdentityCredential to facilitate (but I think that should be done anyway). This would make the algorithm a little bit easier because having an IdentityCredential means it was discovered correctly, so some of the sanity checks can be cleared.
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 also feels like it is solving a very specific convenience-centered problem while having a more general name and putting itself in a general context.
If the IDP's iframe wants access to the account information and already has gone through a FedCM flow, why wouldn't they just use their token to request it themselves?
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.
We use two dictionaries because one is describing JSON values while the other is describing the dictionary returned in JavaScript. They do need to be different, both due to semantics as well as difference in naming conventions.
I don't think it makes sense to add this to IdentityCredential, as this API does not return a credential, and it also does not require the JavaScript to have a credential around. I'm not sure what problem you are trying to solve with this suggestion.
Regarding why the IDP iframe can't just use the token: the FedCM call is done in the RP, not the IDP iframe. As such, the token should not allow querying updated account information on its own: this is power the IDP should not grant to the RP automatically. Using the token would also require the IDP to store this token somewhere in cookies so it can perform this query. So there really is no way to do this in a reasonable way, in the absence of third-party cookies.
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.
One benefit of WebIDL is that we can re-use a single definition for both.
I'm not sure which semantics you mean here. And is there external pressure on the spec to keep separate naming conventions for field names for single values? If I'm looking for a given name and I have to remember whether this is the case where I have to use obj['given_name'] or obj['givenName'], I would find that experience bad.
Polluting the window namespace.
Sure, but presumably the RP can pass the token to the IDP iframe?
I thought there was a responsibility by the IDP to make the most recent identifiers available. Restricting updates to chrome UI rather than including RPs seems like a tradeoff.
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 guess semantics are fairly similar so that would be fine. But the naming conventions are at https://www.w3.org/TR/design-principles/#casing-rules. I think they should be respected?
Your suggestion is to add the method to IdentityCredential instead of IdentityProvider. Either way the method is not polluting the namespace. What am I missing?
This assumes the IDP wants to have control over the storage space in the RP. It would need to do so to store some token that it can then pass to its iframe later on. This seems undesirable: the IDP should not need to touch RP storage!
Sure, the IDP needs to make the most recent information available, whenever that information needs to be used (FedCM or some other form of federated login or authentication). That does not in any way imply that the IDP should permit the RP unrestricted queries into the accounts that the user has because the user once used FedCM on that RP.
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 hadn't been thinking about this in the context of agl's proposal... However if that is adopted, there is a very big breaking change happening that allows other things to move around. We could have a
partial interface IdentityCredential { FedCMCredentialUtilityFunctions fedcm }
that holds this function.Going through the existing experimental uses in Chrome[1]:
navigator
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.
Yea not all are great examples. But one that I think is a good one is
IdentityProvider.close()
. We want the IDP to call this for the IDP SignIn Status API, to signal to the browser that the user successfully signed in to the IDP. I think it would not make sense to have it inIdentityCredential
?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.
Yeah, that doesn't quite make sense. That means that there isn't a great place for this to go. We could add an inline issue that we should evaluate where this goes long-term.
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.
With an inline issue, I'd call this LGTM.
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.
Added inline issue