w3c-fedid / FedCM

A privacy preserving identity exchange Web API
https://w3c-fedid.github.io/FedCM/
Other
375 stars 72 forks source link

Add API to show error messages from failed token fetches #498

Open npm1 opened 1 year ago

npm1 commented 1 year ago

Adds the capability for the FedCM API to show error dialogs in certain scenarios after the user has chosen to perform federated login with an account. For this purpose:

Fixes https://github.com/fedidcg/FedCM/issues/488


Preview | Diff

npm1 commented 1 year ago

Ready for another look @yi-gu

npm1 commented 1 year ago

Hey @bvandersloot-mozilla @martinthomson @cboozar requesting Mozilla to take a look.

samuelgoto commented 8 months ago

@npm1 just checking: is there anything else we need to act on before we merge this?

npm1 commented 8 months ago

@npm1 just checking: is there anything else we need to act on before we merge this?

Yes, I need to rebase and change the code to whatever we agree on (awaiting @bvandersloot-mozilla's confirmation about whether error is acceptable)

npm1 commented 2 months ago

I don't think we need to make this web-observable and doing so risks leaking information to the RP about shared browser state.

What is 'this'? And can you elaborate on the leak you are thinking about?

bvandersloot-mozilla commented 2 months ago

"this" is the IdentityCredentialError itself, particularly the code variable which contains an unspecified string (presumably one of "invalid_request", "unauthorized_client", "access_denied", "server_error", and "temporarily_unavailable"). If I understand this patch correctly, that ends up returned from navigator.credentials.get and reveals internal information about the credential discovery to the RP.

npm1 commented 2 months ago

"this" is the IdentityCredentialError itself, particularly the code variable which contains an unspecified string (presumably one of "invalid_request", "unauthorized_client", "access_denied", "server_error", and "temporarily_unavailable"). If I understand this patch correctly, that ends up returned from navigator.credentials.get and reveals internal information about the credential discovery to the RP.

That is correct, but the IdP needs to set this value itself in the response in order for it to be passed to the RP (it is not 'leaked'). Also perhaps worth noting that the error is passed from the ID assertion fetch, so the user must have tried to use FedCM in the RP.

bvandersloot-mozilla commented 2 months ago

Ah, I didn't realize this is happening on the id assertion endpoint only. In that case, this is reasonable, but we should probably enumerate the possible values for code and specify when they are returned.

npm1 commented 2 months ago

Ah, I didn't realize this is happening on the id assertion endpoint only. In that case, this is reasonable, but we should probably enumerate the possible values for code and specify when they are returned.

I was hoping to leave the values for error (previously code) unspecified similar to how we do not specify the format/return of token. That way the IdP can send the information needed for the RP to process the error, and the user agent may choose to show generic UI or show more specific UI if it chooses to do so based on the error string. What do you think? I did mention the strings Chrome uses to show more specific UI in case it is of interest for an implementor to match.

npm1 commented 2 months ago

Putting this on hold to get some discussion with other WGs on error handling, hopefully to occur during TPAC.

npm1 commented 1 month ago

Rebased this PR in case people want to take another look ahead of TPAC discussion