-
Notifications
You must be signed in to change notification settings - Fork 101
Update service-workers mode to all for credentialed requests #787
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
base: main
Are you sure you want to change the base?
Conversation
|
Could you send the whitespace removal as a separate PR so that it's easier to see what is being changed here? |
|
@cbiesinger — GitHub provides a handy "hide whitespace changes" option. It reveals that the text of three lines was changed. |
|
Yeah, I can clean it if needed. I just have my editor set to "trim trailing whitespace" |
|
Now whitespace change free |
|
Thanks Will! This PR needs more discussion before we merge it (from my side, I have started a thread with privacy) |
|
@npm1 sounds good, let me know how that goes |
|
@npm1 any updates on the privacy discussion? |
|
The reviewer asked some clarifications just this morning so I don't yet have an answer. |
|
With proposed changes, spec still requires additional refinements to fully support the service worker flow. Core Algorithm Changes
Connected Accounts Set
Validation & Restrictions
Network Requests
Security & Privacy
Documentation
Definitions & Concepts
|
|
@pottis none of those changes are proposed. Where did all that come from? I think you may have fundamentally misunderstood this PR. This PR allows the Identity Provider to set a service worker. This is the normal way fetch works - it is the origin of destination of the fetch request whose service worker is run as part of fetch. It is also not the intention that service workers require silent / no UI modes. Your comments seem to relate to how a Relying Party could use install a service worker to invoke and or customize FedCM. That's a wholly separate topic. |
|
I will add an explanatory note here, to make this more clear. |
|
Expanding security/privacy does seem applicable, but I agree that this doesnt introduce new UI or autosignin considerations |
|
This change is incorrect. It would let the RP's service worker intercept the requests, which is not what we want. |
I understand that this is your intention but that is not what the PR does, because that's not how service workers normally work. Because this is not how service workers normally work, this may require a monkey patch to the service worker spec, or perhaps describing more informally how this is supposed to work... Essentially, this is a form of https://developer.chrome.com/blog/foreign-fetch CC @domfarolino |
Thanks. You are right. @EmLauber fyi. I think I confused myself when reading about loginUrl. There, the FedCM spec emphasizes that the flow is a top-level traversable, i.e. it is not foreign fetch, but for the non-interactive requests, you are absolutely correct. Perhaps there's a better way to factor this than as a service worker (and thus no need to monkey patch foreign fetch into service worker spec). The FedCM config file could simply point to some sort of javascript file (a web worker?), and the procedures for the various algorithms could optionally call the web worker, if supplied in the config file. I will try to find some time to play around with that factoring. |
Resolves #80 . Service workers enable powerful customizations for the identity provider (IdP). They must be restricted to the credentialed endpoints to maintain FedCM's privacy posture.
Preview | Diff