Conversation
|
@cbiesinger @domfarolino @wanderview @TallTed @yoshisatoyanagisawa @monica-ch |
There was a problem hiding this comment.
Pull request overview
Adds initial spec text for an IDP-declared “identity handler” Service Worker, including well-known declaration, UA-managed registration/unregistration, and storage-key isolation to keep FedCM registrations separate from first-party SW state.
Changes:
- Adds Service Workers and Storage spec link targets needed to reference registration concepts and “storage key”.
- Updates the config fetch flow to register or unregister the identity handler based on presence of
identity_handlerin the well-known file. - Introduces the
IdentityHandlerdictionary plus normative algorithms for UA-managed registration lifecycle and clearing behavior.
Comments suppressed due to low confidence (1)
spec/index.bs:1402
- This condition dereferences the registration’s [=active worker=] without checking for null, and compares script URLs using plain “is” rather than a URL equality concept. This can make the algorithm ill-defined when a registration exists but has no active worker yet, and it’s ambiguous how URL equality should be evaluated.
1. If |registration| is not null and its [=active worker=]'s script url is |scriptURL|:
1. [=Soft update=] |registration|.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| |configUrl| (see [=computing the manifest URL=]), so keying the handler this way guarantees | ||
| its registration scope can cover the endpoints it intercepts, regardless of where the | ||
| [=well-known file=] is hosted. | ||
| 1. Return |config|. |
There was a problem hiding this comment.
IdentityProviderWellKnown.provider_urls is a list and can hold URLs on distinct [=/origins=] under the same eTLD+1. Because registration is keyed to |configUrl|'s origin, each origin gets its own [=FedCM identity-handler storage key=] and its own independent registration — all sourced from the same well-known identity_handler.service_worker value.
Note: If a [=well-known file=] lists multiple [=/origins=] in {{IdentityProviderWellKnown/provider_urls}}, each [=/origin=] has its own [=FedCM identity-handler storage key=] and its own registration, even though they share a single {{IdentityProviderWellKnown/identity_handler}} declaration.
There was a problem hiding this comment.
Would fetching the well-known file end up in failure if provider_urls are > 1 currently due to :
- If one of the previous two steps threw an exception, or if the
[=list/size=] of |wellKnown|["{{IdentityProviderWellKnown/provider_urls}}"] is
greater than 1, set |wellKnown| to failure.
There was a problem hiding this comment.
You're right, I missed the size > 1 failure at L1249–1251. So this is moot on today's spec text.
npm1
left a comment
There was a problem hiding this comment.
Neat thanks for writing this up! I guess this is only part of the changes needed, the other part being actually using the registered service worker in the credentialed fetches?
| handler is declared. | ||
|
|
||
| <div algorithm> | ||
| To <dfn>register the identity handler</dfn> given a [=/URL=] |configUrl| and an |
There was a problem hiding this comment.
Can we avoid calling this configUrl? Perhaps serviceWorkerUrl or swUrl?
|
Thanks for drafting this. Since there are still several unresolved threads in the design doc, I just wanted to clarify if we are moving forward with parallel discussions here. Separately, speaking as a ServiceWorker editor, I'm concerned about the use of non-exported ServiceWorker algorithms in this PR. Those algorithms are not guaranteed to remain stable in the future, so we should avoid calling them directly. Instead, I'd recommend using the exported Fire Functional Event algorithm, which is intended for this kind of integration. |
|
I have definite desire and intent to review this, but I would prefer to do so after the existing comments are addressed, given their number. |
Thanks for taking a look. Yeah to clarify the design doc is more focused on a Chromium based impl of this and may have specifics related to Chromium and that implementation/architecture. In parallel we'd like to ensure we align on the spec'd behaviour/integrations. As for the exported methods and functional event. This PR focuses more on the JIT registration of a SW in FedCM flows and I'll post a follow up that focuses on the event firing (and does use Fire Function Event :) ). I wanted to try and split the spec change up a bit so it was a little more discrete size wise for reviewers to look at. For this though I'm not sure we currently have the necessary exported primitives to do JIT registration or unregistration. I think Payment Handler may also benefit from this. Would you be open to exporting soft-update and adding/exposing something in the SW spec like: |
npm1
left a comment
There was a problem hiding this comment.
Before we continue, we should probably align with service worker spec editors regarding whether we need to implement this using the fire functional event, or whether they are ok exporting some algorithms so that they can be used from FedCM
| {{IdentityProviderWellKnown/identity_handler}}. | ||
| 1. Otherwise, [=in parallel=], [=unregister the identity handler=] given |configUrl|'s | ||
| [=url/origin=]. | ||
| 1. Otherwise, [=enqueue steps=] to |idpOrigin|'s [=identity handler queue=] to |
There was a problem hiding this comment.
Is this a common pattern, eg having a queue per origin? Is it needed here, or is having a single queue for this type of task enough?
This change starts to update and split the proposed changes in #815. Specifically it focuses on the SW registration aspects and adds the
identity_handlerdeclaration to the well-known file and the registration lifecycle for it: register on config fetch, unregister when the declaration goes away, and clear alongside the rest of the IDP's storage. Registration is UA managed under a storage key isolated from the IDP's first-party one, which is where #833 looks to have landed on isolation.Dispatch, the event IDL, and the security and privacy considerations will follow in separate PRs.
For additional context the full proposal explainer may also help.
Preview | Diff