Add loginHint, RP context, and getUserInfo #470
Conversation
|
PTAL |
| @@ -1082,6 +1106,115 @@ and a |responseBody|, run the following steps. This returns an [=ordered map=]. | |||
| 1. Return |json|. | |||
| </div> | |||
|
|
|||
| <!-- ============================================================ --> | |||
There was a problem hiding this comment.
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.
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.
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.
We use two dictionaries because one is describing JSON values while the other is describing the dictionary returned in JavaScript.
One benefit of WebIDL is that we can re-use a single definition for both.
They do need to be different, both due to semantics as well as difference in naming conventions.
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.
I'm not sure what problem you are trying to solve with this suggestion.
Polluting the window namespace.
Regarding why the IDP iframe can't just use the token: the FedCM call is done in the RP, not the IDP iframe.
Sure, but presumably the RP can pass the token to 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
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.
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.
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?
Polluting the window namespace.
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?
Sure, but presumably the RP can pass the token to the IDP iframe?
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!
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.
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.
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]:
- this proposal
- sign-in status API that Sam proposed moving to
navigator - IDP registration (abandoned?)
- A new one to me! but that looks like it would be just as at home on IdentityCredential
There was a problem hiding this comment.
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 in IdentityCredential?
There was a problem hiding this comment.
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.
With an inline issue, I'd call this LGTM.
|
Let me know if you have any other comments, thanks! |
SHA: 47a2bca Reason: push, by npm1 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #426, #456, and #382
Preview | Diff