search

w3c-fedid / FedCM

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

Allow auto-reauthentication in FedCM #458

Closed npm1 closed 1 year ago

npm1 commented 1 year ago

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

Preview | Diff

yi-gu commented 1 year ago

Do we need to update the algorithm to take mediation requirements into account? Or can it be implied?

cbiesinger commented 1 year ago

Do we need to update the algorithm to take mediation requirements into account? Or can it be implied?

Pretty sure we do, the algorithm is currently normatively requiring showing a dialog e.g. in step 7 of https://fedidcg.github.io/FedCM/#create-an-identitycredential

npm1 commented 1 year ago

Hmm I thought we might get the logic for free from credential management but this appears to not be the case. We currently have all our stuff in collecting Credentials from the credential store since it requires the user to tell the user agent what IDP and account they would like to use. Credential management would only work for free if we had some of the logic in asking the user to choose a Credential. But I don't think we can move this because otherwise having only 1 possible credential with mediation equal to "optional" or "silent" would mean that we sign in the user without them having provided any consent before. So we probably need to keep doing all our stuff in collecting Credentials from the credential store and our asking the user to choose a Credential is a no-op, but that does mean we need to specify the logic in the spec. I can take a stab at it in this PR.

npm1 commented 1 year ago

Ok, I took a stab at it, please take a look? I can loop in Mozilla folks once it looks good to you.

npm1 commented 1 year ago

@bvandersloot-mozilla @cboozar @martinthomson - requesting Mozilla review! As y'all had requested, we're not filing a standards position issue. So consider this it :) Please take a look.

cboozar commented 1 year ago

I think these changes make sense, not sure why some of the variables needed shortening but readable either way

npm1 commented 1 year ago

Thanks for the review, Cameron! Merging then. Re variable shortening, perhaps you mean acc, the reason is that I wanted it to be different from account and I'm bad at coming up with names. Happy to get suggestions on the naming.