search

w3c-fedid / FedCM

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

Fix assertion in DiscoverFromExternalSource algorithm #460

Closed RByers closed 2 months ago

RByers commented 1 year ago

Make providers required in the identity options dictionary so that the assert is true. Replace the providers size assert with an explicit check and throw, matching chromium code.

Preview | Diff

RByers commented 1 year ago

@cbiesinger PTAL. If there's agreement that this is correct, then I'll also update chromium to make providers required.

cbiesinger commented 1 year ago

Looks good except that I am not sure if we need to create the exception in a task

npm1 commented 1 year ago

I think this change makes sense but it actually does not match Chromium implementation (required is missing from the IDL file). I guess the only difference is that the implementation currently will just silently return a trivial promise instead of throwing an error.

npm1 commented 1 year ago

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=1440522 by the way.

npm1 commented 9 months ago

This is still relevant. Are you planning on addressing the comments or should I start a new PR for this?

npm1 commented 2 months ago

I see spec now has 'required' which we added elsewhere already. This also asserts the size, which we would get rid of when we add multi IDP support. So closing this